Adopt Skylos for dead-code detection - #224
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis change adds a strict Skylos dead-code scan to ChangesDead-code benchmark
Skylos lint integration
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Makefile
participant Skylos
participant pyproject.toml
Developer->>Makefile: Run make lint
Makefile->>Skylos: Run strict dead_code scan
Skylos->>pyproject.toml: Load documented exceptions
Skylos-->>Makefile: Return gate status
Makefile-->>Developer: Report lint result
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 6 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Bumpy Road Aheadbenchmarks/dead_code/score.py: score_findings What lead to degradation?score_findings has 3 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function Why does this problem occur?A Bumpy Road is a function that contains multiple chunks of nested conditional logic inside the same function. The deeper the nesting and the more bumps, the lower the code health. How to fix it?Bumpy Road implementations indicate a lack of encapsulation. Check out the detailed description of the Bumpy Road code health issue. |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Overall Code Complexitybenchmarks/dead_code/score.py: What lead to degradation?This module has a mean cyclomatic complexity of 5.13 across 8 functions. The mean complexity threshold is 4 Why does this problem occur?Overall Code Complexity is measured by the mean cyclomatic complexity across all functions in the file. The lower the number, the better. How to fix it?You address the overall cyclomatic complexity by a) modularizing the code, and b) abstract away the complexity. Let's look at some examples: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfeaf81c5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| PYLINT_PYPY_SHIM = git+https://github.com/leynos/pylint-pypy-shim.git@$(PYLINT_PYPY_SHIM_REF) | ||
| PYLINT = $(UV_ENV) $(UV) tool run --python $(PYLINT_PYTHON) --from '$(PYLINT_PYPY_SHIM)' pylint-pypy | ||
| SKYLOS = $(UV_ENV) $(UV) run skylos | ||
| SKYLOS_TARGETS ?= $(PYLINT_TARGETS) |
There was a problem hiding this comment.
When production code is referenced only by tests, reusing PYLINT_TARGETS places episodic and tests in the same Skylos graph, so those test references prevent the gate from reporting dead production code. This already occurs for _build_payload_dataclass: benchmarks/dead_code/results/production-adjudication.json:32-35 classifies it as actionable because it is referenced only by direct unit tests, and the only repository references outside its definition are in tests/test_api_helpers.py, yet the configured lint gate passes. Split the production and test scans, or exclude tests from the production scan, so the blocking gate can enforce the documented remove-genuine-dead-code policy.
AGENTS.md reference: AGENTS.md:L73-L78
Useful? React with 👍 / 👎.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Define a reproducible, two-lane comparison of pyscn and Skylos that separates unused-symbol detection from control-flow unreachability. Record the fairness constraints, evidence requirements, stop conditions, and validation workflow before building the corpus. Index the plan and refresh the generated spelling policy for the new documentation vocabulary.
Pin the released tool versions and successful smoke commands. Preserve the observed difference between control-flow and unused-symbol findings, and document the project-root marker needed to bound Skylos discovery.
Normalize pyscn control-flow findings and Skylos unused-symbol findings to tool-neutral source locations. Score labelled expectations by analysis lane while retaining misses and unmatched reports as visible errors. Cover both released JSON schemas and the lane-level confusion matrix with focused unit tests.
Freeze 19 tool-neutral liveness expectations before detector execution. Cover ordinary unused symbols, direct and exported uses, dynamic and decorator-driven calls, and five kinds of control-flow unreachability. Bound project-root discovery with an isolated corpus project and document the oracle's reuse and change policy.
Retain the raw pyscn 1.28.0 and Skylos 4.30.0 JSON reports and timing metadata for the frozen corpus. Resolve pyscn's relative report paths from the declared corpus root. Keep findings outside the preregistered oracle visible as unmatched rather than misclassifying them as labelled false positives.
Retain the labelled corpus scores and repository-scale scan evidence for pyscn 1.28.0 and Skylos 4.30.0.\n\nExplain the tools' distinct analysis lanes, practical precision, operational caveats, and recommended uses.
Run a local, blocking Skylos scan from `make lint` and document every intentional dynamic surface in version-controlled configuration. Provide a guarded workflow for adding reasoned named exceptions. Remove the dead reference-document loader found during adoption, record the tooling decision, and cover the lint contract with focused tests.
Tell agents that `make lint` includes blocking dead-code detection. Require investigation before suppression and point verified false positives to the reasoned entry-point or named-exception workflows.
Regenerate `uv.lock` from the mainline baseline and the rebased manifest. Resolve the compatible Skylos 4.31.0 release validated by the full gate stack.
Split scoring into policy-focused helpers and preserve location-based deduplication, matching, and unmatched accounting semantics. Decompose pyscn report traversal while retaining validation context and finding order, and cover the public scoring behaviour directly.
Name the expectation, finding, and unmatched-expectation scoring steps directly so score_findings exposes the benchmark policy. Extend the cross-lane characterization to prove reports are counted against the expectation lane.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/dead_code/corpus/dynamic.py`:
- Line 1: Replace the file-wide Ruff suppression in
benchmarks/dead_code/corpus/dynamic.py at lines 1-1 with rule-specific inline
ignores only on intentional dynamic-resolution constructs; likewise remove the
blanket suppression in benchmarks/dead_code/corpus/flow.py at lines 1-1 and
annotate only intentional unreachable assignments, and in
benchmarks/dead_code/corpus/symbols.py at lines 1-1 annotate only intentional
unused symbols. Do not use blanket noqa, file-level skips, or category-wide
suppressions.
In `@benchmarks/dead_code/corpus/flow.py`:
- Around line 10-12: Update after_raise to raise a specific built-in or
corpus-specific exception instead of RuntimeError, and update the corresponding
handler to catch that same exception. Refresh the line-anchored expectation and
retained report to reflect the fixture change.
In `@benchmarks/dead_code/results/production-adjudication.json`:
- Around line 8-9: Update the production-adjudication record’s raw_report and
sha256 fields to reference retained report artefacts that exist, or add the
named .json.gz reports to the repository; ensure the SHA-256 matches the
referenced file exactly so the evidence can be verified and reproduced.
In `@benchmarks/dead_code/score.py`:
- Around line 19-48: Expand the docstrings for the public dataclasses
Expectation, Finding, and LaneScore, plus parse_pyscn_findings,
parse_skylos_findings, and score_findings, into complete NumPy-style structured
documentation. Add the appropriate Attributes sections for dataclasses and
Parameters/Returns sections for functions, accurately documenting their fields,
arguments, and return values while preserving the existing summaries and
behavior.
- Around line 1-9: Remove the `from __future__ import annotations` import from
the new module, leaving the remaining imports and implementation unchanged.
- Around line 51-95: Add concise single-line NumPy-style docstrings to the
private validators _mapping, _sequence, _string, _positive_line, and
_relative_source_path, describing each function’s validation or
path-normalization behavior. Do not alter their existing logic or signatures.
In `@docs/pyscn-skylos-dead-code-head-to-head.md`:
- Around line 144-146: Update the recommendation in the pyscn/Skylos comparison
documentation to scope the advisory-only statement specifically to the
zero-threshold Skylos 4.30.0 benchmark. Separately document the tuned Skylos
gate invoked by the Makefile as a blocking contributor workflow, consistent with
ADR-016, while preserving the existing guidance about project-specific
suppression and framework modelling for the untuned comparison.
- Around line 100-103: Update docs/pyscn-skylos-dead-code-head-to-head.md lines
100-103 to record the confirmed removal of _load_reference_documents_for_target
instead of claiming production code was not deleted. Update
docs/execplans/benchmark-pyscn-skylos-dead-code.md lines 202-205 to acknowledge
the same production-helper removal in the validation outcome, keeping both
maintainer-facing reports consistent with the changed behavior.
In `@Makefile`:
- Around line 94-103: Update the skylos-allow target to stop interpolating NAME
and REASON into shell source: export them via target-scoped variables using
their raw values, then reference the exported variables as quoted shell
expansions in both validation checks and the whitelist invocation. Add a
regression test proving quotes and shell metacharacters remain arguments and
cannot execute commands.
In `@tests/test_dead_code_benchmark.py`:
- Around line 44-51: The assertions in tests/test_dead_code_benchmark.py lack
required failure messages. Update every bare assert in the file, including the
assertions around Finding comparison and the referenced ranges, to provide a
concise message using the existing assertion style while preserving each
assertion’s condition and expected behavior.
- Around line 131-193: Extract the identical expectations tuple from
test_score_findings_classifies_dead_and_live_matched_expectations and
test_score_findings_classifies_dead_and_live_unmatched_expectations into a
shared pytest fixture, placing it in the repository’s established conftest.py or
fixtures location. Update both tests to receive and reuse that fixture while
preserving their existing assertions.
In `@tests/test_skylos_lint_contract.py`:
- Line 23: Update every assertion in tests/test_skylos_lint_contract.py,
including the assertions near the dependency check and the referenced ranges, to
include a concise failure message after the condition. Make each message
identify the missing or invalid Skylos configuration or contract requirement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aa005eae-d897-4a46-ab07-d91e71ec9ef5
⛔ Files ignored due to path filters (3)
benchmarks/dead_code/results/pyscn-1.28.0-episodic.json.gzis excluded by!**/*.gzbenchmarks/dead_code/results/skylos-4.30.0-episodic.json.gzis excluded by!**/*.gzuv.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.gitignoreAGENTS.mdMakefilebenchmarks/__init__.pybenchmarks/dead_code/README.mdbenchmarks/dead_code/__init__.pybenchmarks/dead_code/corpus/__init__.pybenchmarks/dead_code/corpus/dynamic.pybenchmarks/dead_code/corpus/flow.pybenchmarks/dead_code/corpus/pyproject.tomlbenchmarks/dead_code/corpus/symbols.pybenchmarks/dead_code/expectations.jsonbenchmarks/dead_code/results/production-adjudication.jsonbenchmarks/dead_code/results/pyscn-1.28.0-episodic.metricsbenchmarks/dead_code/results/pyscn-1.28.0.jsonbenchmarks/dead_code/results/pyscn-1.28.0.metricsbenchmarks/dead_code/results/scores.jsonbenchmarks/dead_code/results/skylos-4.30.0-episodic.metricsbenchmarks/dead_code/results/skylos-4.30.0.jsonbenchmarks/dead_code/results/skylos-4.30.0.metricsbenchmarks/dead_code/score.pydocs/adr/adr-016-adopt-skylos-dead-code-detection.mddocs/contents.mddocs/developers-guide.mddocs/execplans/benchmark-pyscn-skylos-dead-code.mddocs/pyscn-skylos-dead-code-head-to-head.mddocs/repository-layout.mdepisodic/canonical/profile_templates/_brief_loaders.pypyproject.tomltests/test_dead_code_benchmark.pytests/test_skylos_lint_contract.pytypos.toml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/hecate(auto-detected)leynos/femtologging(auto-detected)leynos/tei-rapporteur(auto-detected)leynos/falcon-correlate(auto-detected)leynos/shared-actions(auto-detected)
💤 Files with no reviewable changes (1)
- episodic/canonical/profile_templates/_brief_loaders.py
| @@ -0,0 +1,38 @@ | |||
| # ruff: noqa | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace the blanket Ruff suppressions with narrow exceptions.
# ruff: noqa disables every Ruff rule in each fixture and masks accidental
violations outside the intentional dead-code examples. Use justified, rule-specific
inline suppressions only where the benchmark requires them.
benchmarks/dead_code/corpus/dynamic.py#L1-L1: remove the file-wide suppression and
add narrow ignores only for intentional dynamic-resolution constructs.benchmarks/dead_code/corpus/flow.py#L1-L1: remove the file-wide suppression and add
narrow ignores only for intentional unreachable assignments.benchmarks/dead_code/corpus/symbols.py#L1-L1: remove the file-wide suppression and
add narrow ignores only for intentional unused symbols.
As per path instructions, “Blanket # noqa, file-level skips, and categories are
forbidden.”
📍 Affects 3 files
benchmarks/dead_code/corpus/dynamic.py#L1-L1(this comment)benchmarks/dead_code/corpus/flow.py#L1-L1benchmarks/dead_code/corpus/symbols.py#L1-L1
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/dead_code/corpus/dynamic.py` at line 1, Replace the file-wide Ruff
suppression in benchmarks/dead_code/corpus/dynamic.py at lines 1-1 with
rule-specific inline ignores only on intentional dynamic-resolution constructs;
likewise remove the blanket suppression in benchmarks/dead_code/corpus/flow.py
at lines 1-1 and annotate only intentional unreachable assignments, and in
benchmarks/dead_code/corpus/symbols.py at lines 1-1 annotate only intentional
unused symbols. Do not use blanket noqa, file-level skips, or category-wide
suppressions.
Source: Path instructions
| def after_raise() -> None: | ||
| raise RuntimeError("expected benchmark exception") | ||
| unreachable_after_raise = 47 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Raise and catch a specific exception.
Replace RuntimeError with a specific built-in exception such as ValueError, or a
corpus-specific Error, then update the matching handler. Refresh the line-anchored
expectation and retained report after changing the fixture.
As per coding guidelines, “Raise specific built-in exceptions or domain-specific
exceptions instead of generic Exception or catch-all RuntimeError.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/dead_code/corpus/flow.py` around lines 10 - 12, Update after_raise
to raise a specific built-in or corpus-specific exception instead of
RuntimeError, and update the corresponding handler to catch that same exception.
Refresh the line-anchored expectation and retained report to reflect the fixture
change.
Sources: Coding guidelines, Path instructions
| "raw_report": "pyscn-1.28.0-episodic.json.gz", | ||
| "sha256": "bf4b969ed08691b1e9878fb0def660b9619e29e1e32461a98c750a34ce1d227f" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reference retained reports that actually exist.
Commit the named .json.gz reports, or update raw_report and its SHA-256 to the
retained .json artefacts. The current record points to absent evidence, so its
integrity hashes cannot be verified or reproduced.
Also applies to: 16-17
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/dead_code/results/production-adjudication.json` around lines 8 -
9, Update the production-adjudication record’s raw_report and sha256 fields to
reference retained report artefacts that exist, or add the named .json.gz
reports to the repository; ensure the SHA-256 matches the referenced file
exactly so the evidence can be verified and reproduced.
| """Normalize and score dead-code detector findings against labelled source.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import dataclasses as dc | ||
| import enum | ||
| import typing as typ | ||
| from collections import abc as cabc | ||
| from pathlib import Path |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Drop from __future__ import annotations from this new module.
This is a brand-new file, and the repo's own rule states the import must not be added in new or modified files because the project baseline is Python 3.14.
🐍 Proposed fix
"""Normalize and score dead-code detector findings against labelled source."""
-from __future__ import annotations
-
import dataclasses as dc
import enum
import typing as typ
from collections import abc as cabc
from pathlib import PathAs per coding guidelines, "Do not add from __future__ import annotations in new or modified project files because the project baseline is Python 3.14."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """Normalize and score dead-code detector findings against labelled source.""" | |
| from __future__ import annotations | |
| import dataclasses as dc | |
| import enum | |
| import typing as typ | |
| from collections import abc as cabc | |
| from pathlib import Path | |
| """Normalize and score dead-code detector findings against labelled source.""" | |
| import dataclasses as dc | |
| import enum | |
| import typing as typ | |
| from collections import abc as cabc | |
| from pathlib import Path |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/dead_code/score.py` around lines 1 - 9, Remove the `from
__future__ import annotations` import from the new module, leaving the remaining
imports and implementation unchanged.
Source: Coding guidelines
| @dc.dataclass(frozen=True, slots=True, kw_only=True) | ||
| class Expectation: | ||
| """A tool-neutral liveness label at one source location.""" | ||
|
|
||
| identifier: str | ||
| path: str | ||
| line: int | ||
| lane: Lane | ||
| is_dead: bool | ||
|
|
||
|
|
||
| @dc.dataclass(frozen=True, slots=True, kw_only=True) | ||
| class Finding: | ||
| """A normalized detector finding at one source location.""" | ||
|
|
||
| path: str | ||
| line: int | ||
| lane: Lane | ||
| category: str | ||
|
|
||
|
|
||
| @dc.dataclass(frozen=True, slots=True, kw_only=True) | ||
| class LaneScore: | ||
| """A confusion matrix for one dead-code analysis lane.""" | ||
|
|
||
| true_positives: int | ||
| false_positives: int | ||
| false_negatives: int | ||
| true_negatives: int | ||
| unmatched_findings: int |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Expand public-interface docstrings to full numpy-style structured docs.
Expectation, Finding, LaneScore, parse_pyscn_findings, parse_skylos_findings, and score_findings are all public interfaces but carry only a one-line summary each, with no Parameters/Returns/Attributes sections.
As per path instructions, "Docstrings must follow the numpy style guide. ... full structured docs for all public interfaces."
Also applies to: 98-104, 186-191, 217-221
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/dead_code/score.py` around lines 19 - 48, Expand the docstrings
for the public dataclasses Expectation, Finding, and LaneScore, plus
parse_pyscn_findings, parse_skylos_findings, and score_findings, into complete
NumPy-style structured documentation. Add the appropriate Attributes sections
for dataclasses and Parameters/Returns sections for functions, accurately
documenting their fields, arguments, and return values while preserving the
existing summaries and behavior.
Source: Path instructions
| - Use both in a cleanup programme: pyscn as a focused control-flow gate and | ||
| Skylos as an advisory inventory. Do not make zero-threshold Skylos findings a | ||
| blocking gate without project-specific suppression and framework modelling. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Separate the untuned benchmark from the adopted blocking gate.
Scope this recommendation to the zero-threshold Skylos 4.30.0 comparison. The
repository now runs a tuned Skylos dead-code gate from Makefile, and ADR-016
records that gate as blocking; leaving Skylos described only as an advisory
inventory contradicts the contributor workflow.
As per coding guidelines, keep maintainer-facing documentation synchronized with
the current operating contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/pyscn-skylos-dead-code-head-to-head.md` around lines 144 - 146, Update
the recommendation in the pyscn/Skylos comparison documentation to scope the
advisory-only statement specifically to the zero-threshold Skylos 4.30.0
benchmark. Separately document the tuned Skylos gate invoked by the Makefile as
a blocking contributor workflow, consistent with ADR-016, while preserving the
existing guidance about project-specific suppression and framework modelling for
the untuned comparison.
Source: Coding guidelines
| skylos-allow: ## Document one intentional Skylos finding | ||
| @test -n "$(strip $(NAME))" || { \ | ||
| printf "Error: NAME is required\n" >&2; \ | ||
| exit 2; \ | ||
| } | ||
| @test -n "$(strip $(REASON))" || { \ | ||
| printf "Error: REASON is required\n" >&2; \ | ||
| exit 2; \ | ||
| } | ||
| $(SKYLOS) whitelist "$(NAME)" --reason "$(REASON)" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Injection (CWE-78): Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')
Stop interpolating allowlist arguments into shell source.
NAME and REASON are inserted directly into recipe text. A value containing
" or ; can close the quoted argument and execute another command in both the
validation and whitelist steps. Pass the values through exported environment
variables and reference them as "$${VAR}", or apply a proven shell-escaping
routine; add a regression test covering quotes and shell metacharacters.
🔒 Proposed shell-safe direction
-skylos-allow: ## Document one intentional Skylos finding
- `@test` -n "$(strip $(NAME))" || { \
+skylos-allow: export SKYLOS_NAME = $(value NAME)
+skylos-allow: export SKYLOS_REASON = $(value REASON)
+skylos-allow: ## Document one intentional Skylos finding
+ `@test` -n "$${SKYLOS_NAME}" || { \
printf "Error: NAME is required\n" >&2; \
exit 2; \
}
- `@test` -n "$(strip $(REASON))" || { \
+ `@test` -n "$${SKYLOS_REASON}" || { \
printf "Error: REASON is required\n" >&2; \
exit 2; \
}
- $(SKYLOS) whitelist "$(NAME)" --reason "$(REASON)"
+ $(SKYLOS) whitelist "$${SKYLOS_NAME}" --reason "$${SKYLOS_REASON}"🧰 Tools
🪛 checkmake (0.3.2)
[warning] 94-94: Target body for "skylos-allow" exceeds allowed length of 5 lines (9).
(maxbodylength)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 94 - 103, Update the skylos-allow target to stop
interpolating NAME and REASON into shell source: export them via target-scoped
variables using their raw values, then reference the exported variables as
quoted shell expansions in both validation checks and the whitelist invocation.
Add a regression test proving quotes and shell metacharacters remain arguments
and cannot execute commands.
| assert findings == ( | ||
| Finding( | ||
| path="flow.py", | ||
| line=7, | ||
| lane=Lane.UNREACHABLE_STATEMENT, | ||
| category="unreachable_after_return", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add failure messages to every bare assert.
None of the assertions in this file carry a message, contrary to the repo's explicit test-style rule.
✅ Proposed fix (representative example)
- assert scores[Lane.UNUSED_SYMBOL].true_positives == 1
- assert scores[Lane.UNREACHABLE_STATEMENT].unmatched_findings == 0
+ assert scores[Lane.UNUSED_SYMBOL].true_positives == 1, "expected one true positive in the unused-symbol lane"
+ assert scores[Lane.UNREACHABLE_STATEMENT].unmatched_findings == 0, "cross-lane duplicate must not count as unmatched"Apply the same treatment to every other bare assertion in this file.
As per path instructions, "Use assert …, "message" over bare asserts."
Also applies to: 68-81, 112-113, 127-128, 166-167, 191-192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_dead_code_benchmark.py` around lines 44 - 51, The assertions in
tests/test_dead_code_benchmark.py lack required failure messages. Update every
bare assert in the file, including the assertions around Finding comparison and
the referenced ranges, to provide a concise message using the existing assertion
style while preserving each assertion’s condition and expected behavior.
Source: Path instructions
| def test_score_findings_classifies_dead_and_live_matched_expectations() -> None: | ||
| """Use expectation labels and lanes for matched reports.""" | ||
| expectations = ( | ||
| Expectation( | ||
| identifier="dead-flow", | ||
| path="flow.py", | ||
| line=7, | ||
| lane=Lane.UNREACHABLE_STATEMENT, | ||
| is_dead=True, | ||
| ), | ||
| Expectation( | ||
| identifier="live-symbol", | ||
| path="symbols.py", | ||
| line=2, | ||
| lane=Lane.UNUSED_SYMBOL, | ||
| is_dead=False, | ||
| ), | ||
| ) | ||
| findings = ( | ||
| Finding( | ||
| path="flow.py", | ||
| line=7, | ||
| lane=Lane.UNUSED_SYMBOL, | ||
| category="unused_variables", | ||
| ), | ||
| Finding( | ||
| path="symbols.py", | ||
| line=2, | ||
| lane=Lane.UNREACHABLE_STATEMENT, | ||
| category="unreachable_after_return", | ||
| ), | ||
| ) | ||
|
|
||
| scores = score_findings(expectations, findings) | ||
|
|
||
| assert scores[Lane.UNREACHABLE_STATEMENT].true_positives == 1 | ||
| assert scores[Lane.UNUSED_SYMBOL].false_positives == 1 | ||
|
|
||
|
|
||
| def test_score_findings_classifies_dead_and_live_unmatched_expectations() -> None: | ||
| """Use expectation labels and lanes when no report is present.""" | ||
| expectations = ( | ||
| Expectation( | ||
| identifier="dead-flow", | ||
| path="flow.py", | ||
| line=7, | ||
| lane=Lane.UNREACHABLE_STATEMENT, | ||
| is_dead=True, | ||
| ), | ||
| Expectation( | ||
| identifier="live-symbol", | ||
| path="symbols.py", | ||
| line=2, | ||
| lane=Lane.UNUSED_SYMBOL, | ||
| is_dead=False, | ||
| ), | ||
| ) | ||
|
|
||
| scores = score_findings(expectations, ()) | ||
|
|
||
| assert scores[Lane.UNREACHABLE_STATEMENT].false_negatives == 1 | ||
| assert scores[Lane.UNUSED_SYMBOL].true_negatives == 1 | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract the duplicated expectations tuple into a shared fixture.
test_score_findings_classifies_dead_and_live_matched_expectations and test_score_findings_classifies_dead_and_live_unmatched_expectations build an identical expectations tuple verbatim.
♻️ Proposed fix
+@pytest.fixture
+def mixed_lane_expectations() -> tuple[Expectation, ...]:
+ """Provide one dead unreachable-statement label and one live unused-symbol label."""
+ return (
+ Expectation(
+ identifier="dead-flow",
+ path="flow.py",
+ line=7,
+ lane=Lane.UNREACHABLE_STATEMENT,
+ is_dead=True,
+ ),
+ Expectation(
+ identifier="live-symbol",
+ path="symbols.py",
+ line=2,
+ lane=Lane.UNUSED_SYMBOL,
+ is_dead=False,
+ ),
+ )
+
+
-def test_score_findings_classifies_dead_and_live_matched_expectations() -> None:
+def test_score_findings_classifies_dead_and_live_matched_expectations(
+ mixed_lane_expectations: tuple[Expectation, ...],
+) -> None:
"""Use expectation labels and lanes for matched reports."""
- expectations = (
- Expectation(...),
- Expectation(...),
- )
findings = (...)
- scores = score_findings(expectations, findings)
+ scores = score_findings(mixed_lane_expectations, findings)As per path instructions, "Use pytest fixtures for shared setup (conftest.py or fixtures/)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_dead_code_benchmark.py` around lines 131 - 193, Extract the
identical expectations tuple from
test_score_findings_classifies_dead_and_live_matched_expectations and
test_score_findings_classifies_dead_and_live_unmatched_expectations into a
shared pytest fixture, placing it in the repository’s established conftest.py or
fixtures location. Update both tests to receive and reuse that fixture while
preserving their existing assertions.
Source: Path instructions
| dependency_groups = typ.cast("dict[str, list[str]]", config["dependency-groups"]) | ||
|
|
||
| dependencies = dependency_groups["dev"] | ||
| assert any(dependency.startswith("skylos") for dependency in dependencies) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add assertion messages.
Attach concise failure messages to every assertion so a broken Skylos contract identifies
the missing configuration immediately. As per path instructions, “Use assert …, "message" over bare asserts”.
Also applies to: 34-46, 57-59, 66-70, 77-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_skylos_lint_contract.py` at line 23, Update every assertion in
tests/test_skylos_lint_contract.py, including the assertions near the dependency
check and the referenced ranges, to include a concise failure message after the
condition. Make each message identify the missing or invalid Skylos
configuration or contract requirement.
Source: Path instructions
Pass the repository pyproject file to the dead-code gate so CI uses the reviewed entry-point and whitelist policy consistently. Keep the lint contract aligned with the explicit configuration path.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Mark verified Alembic, framework, protocol, and dataclass validation callbacks as precise dead-code entry points. Keep Skylos strict for all other unused symbols while preventing CI from misclassifying implicit runtime callers as dead code.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Mark framework callbacks, dataclass validation hooks, and protocol implementations as methods so their exact entry-point rules match Skylos symbol classification. Add a contract test that prevents future method rules from being declared as module functions.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Document precise typed entry-point rules for implicit runtime callers, including method classification, full symbol identity, and caller reasons. Reserve `skylos-allow` for named whitelist exceptions and make that scope explicit in its help, errors, and contract test.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
1 participant
Summary
This branch establishes an evidence-led basis for choosing a Python dead-code detector and adopts Skylos as a local, blocking lint check. It adds a reproducible pyscn-versus-Skylos benchmark with retained raw evidence and production adjudication, then configures Skylos with narrow, reasoned exceptions so useful detection is enforced without cloud services or automatic source mutation.
Completed ExecPlan: benchmark pyscn and Skylos dead-code detection.
Review walkthrough
Validation
make --no-print-directory check-fmt: passed; 447 files already formatted.make --no-print-directory test: passed; 1,041 tests passed, one skipped and 23 snapshots passed.make --no-print-directory typecheck: passed.make --no-print-directory lint: passed; Hecate, Ruff, Pylint and the Skylos dead-code gate succeeded.Notes
origin/mainat72177bff5181b939eff13d158073eedc9103da99, retaining main’s Cyclopts, Hypothesis andtyupgrades.References