Clean Code exemplar cleanup — 9-commit refactor pass#2
Merged
Conversation
network_scanner: bracketed IPv6 peers ([2001:db8::1]:443) were silently
miscategorised because rpartition(":") left the closing bracket in
peer_ip, so IPv6 C2 addresses never matched bare-IP entries in
c2.ips. Replace the rpartition with _split_host_port, which handles
plain IPv4 and bracketed IPv6 uniformly.
threat_profile: TOML typos like [threat].ecosytem were silently
dropped to defaults. Add per-section _KEYS schemas and a _check_keys
helper that raises UnknownProfileKeyError on any unknown key.
Wired into _load_from_dir's catch tuple, so a bad profile fails loud
with the file path and offending key.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
scan_iocs took (results, threat, ecosystem, policy, roots, resolve_c2) and scan_source_and_configs took (results, threat, ecosystem, roots). Every _scan_* helper in ioc_scanner.py re-passed subsets of the same five values. The audit flagged this as a Data Clump and recommended a frozen ScanContext dataclass. ScanContext now carries (threat, ecosystem, policy, roots, resolve_c2). Phase signatures become (results, ctx); scanner.main constructs one ctx per threat. Leaf scanners (scan_caches, scan_history, scan_persistence) keep their primitive signatures — they take 2-3 trivial args and have direct unit tests that benefit from the simpler shape. _scan_file_lines still carries its own pattern clump; that one is decomposed in Pass 5 alongside source_scanner restructure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…system_base skip_report previously held a module-level singleton SkipReport mutated by every walker / reader, with a getter, a resetter, and free helpers (note_permission_error, note_read_error). Tests needed an autouse fixture to reset it between runs — the docstring acknowledged the hidden coupling. Plumb SkipReport explicitly. The orchestrator constructs one per scan, attaches it to every per-threat ScanContext, and passes it to the anti-worm pre-pass. pruned_walk, read_if_contains, build_repo_index, find_phantom_deps, find_package_metadata, scan_persistence, scan_history, _check_known_paths and their internal helpers now take a SkipReport argument. Tests instantiate one per test; the autouse reset fixture is gone. ecosystem_base._ecosystem_cache (module-level dict) becomes @functools.lru_cache on get_ecosystem — same memoisation, no hand-rolled global. Three tests in test_skip_report.py that exercised the removed singleton API are deleted; the dataclass and instrumentation tests remain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
scan_iocs orchestrated eight sub-scanners (walk_files, known_paths, c2_connections, malicious_pods, phantom_deps, windows_extras, persistence, caches, history) and lazy-imported three of them inside the function body, hiding them from the static dependency graph. Move the orchestration to scanner._run_phase3_iocs. The full set of phase-3 scanners is now visible at module top, and cache_scanner / history_scanner / persistence_scanner are imported at the top of scanner.py rather than inside scan_iocs. ioc_scanner exposes its individual scanners as public names; the scan_iocs wrapper is gone. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…anner scanner.main was 67 lines covering logging setup, banner, threat-list display, roots caching, anti-worm dispatch, per-threat dispatch, and exit code computation. Split into _configure_logging, _print_run_banner, _cache_search_roots, _dispatch_threats, _compute_exit_code. main() now returns an int instead of calling sys.exit at the bottom — the console-script entry point handles the int correctly. ioc_scanner._scan_for_c2_connections was 70 lines, cyclomatic 11. Extract _run_network_probe (subprocess + parse) and _emit_c2_finding (per-match print + emit). The matching loop now reads as data flow, not control flow. ioc_scanner._scan_walk_files reached nesting depth 7. Extract _hash_matches (returns bool, records skip on error) so the walker's inner block is flat. source_scanner.scan_source_and_configs was 67 lines and _scan_file_lines took 7 parameters — a Data Clump audit finding. Introduce _SourcePatterns (package + 3 compiled regexes) and _FileSelector (filename/extension classifiers); _scan_file_lines now takes the patterns dataclass. scan_source_and_configs becomes a 4-line orchestrator over _print_scan_header and _walk_and_scan_files. The _is_config_file free function is removed in favour of _FileSelector.classify; one test file is updated accordingly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cache_scanner._scan_cache_dir, ecosystem_pypi.extract_version, and ecosystem_npm.extract_version previously swallowed PermissionError / OSError into logger.debug only — an operator running the scanner against an inaccessible cache or install directory got a clean verdict and no clue why nothing was inspected. Plumb SkipReport through scan_caches, scan_environments, and the EcosystemPlugin extract_version protocol so these failures land in the post-scan summary alongside every other walker / reader skip. persistence_scanner._check_config_dir attributed any per-file read error to the parent directory because the try/except wrapped the whole glob loop. Split into per-file scope so the recorded path matches the offending file; the glob enumeration itself is wrapped separately for the rare case where the directory listing fails. _resolve_c2_ips silenced socket.gaierror to logger.debug when the operator explicitly opted into --resolve-c2. Tally resolved / failed counts and print them so a takedown that NXDOMAINs every C2 domain produces an explicit 'coverage incomplete' line instead of a silent partial scan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t type threat_profile previously resolved the per-user threats directory at import time from LOCALAPPDATA / XDG_CONFIG_HOME / HOME, so tests that monkeypatched those env vars after import saw no effect. Move the resolution into _user_threat_dir(), called lazily from load_all_threats — matching the function-scoped pattern that cache_scanner already uses for platform-dependent paths. report.py was 369 lines across six section dividers covering four distinct outputs (per-threat report, anti-worm pre-pass, post-scan skip summary, multi-threat summary). Split into a report/ package with one module per output type: _references, _threat, _anti_worm, _skip, _summary. report/__init__.py re-exports the public surface so every existing `from .report import X` keeps working unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rt params scanner.main, _print_run_banner, _cache_search_roots, _dispatch_threats, and _indicator_count had implicit ``policy``/``indicators`` arguments; annotate them as PlatformPolicy and WormIndicators respectively. Every skip_report parameter that survived from Pass 3's plumbing was left bare. Annotate them as SkipReport (via TYPE_CHECKING-only imports where the alternative would create an import cycle) across ecosystem_base, ecosystem_pypi, ecosystem_npm, ioc_scanner, and version_checker. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* pyproject.toml: add `dev` optional-dependencies group with ruff and mypy.
uv.lock is committed alongside per the project's uv discipline.
* README.md: project structure refreshed to reflect the Pass 1–8 cleanup
— `report.py` is now the `report/` package; `scan_context.py`,
`skip_report.py`, `anti_worm_scanner.py`, `git_repo_index.py` are
listed; `ioc_scanner.py` is described as individual scanners (not the
orchestrator); test count 355 → 413. Overview corrected to "five
built-in profiles". A short note about the strict-schema TOML
validation is added to the "Writing a threat profile" section.
* Apply `ruff format` across the package — 23 files reformatted, pure
whitespace and wrapping, no behavior change.
* Fix `mypy --strict` errors (66 → 0). Mechanical annotation work:
- `re.Pattern` → `re.Pattern[str]` in every plugin and consumer
- `dict` / `list` → `dict[str, Any]` / `list[T]` in TOML parsers
- Untyped helpers in `formatting.py`, `ioc_windows.py`, and the
`report/` package gained explicit parameter / return types
- `_for_current_platform` made generic via TypeVar so its callers
no longer cascade `no-any-return`
- `ecosystem_npm.extract_version` validates the JSON shape before
returning so the result is narrowed from `Any` to `str | None`
- New TYPE_CHECKING imports for `EcosystemPlugin`, `SkipReport`,
`ConnectionRecord`, `Finding`, `Callable`, `Any` where needed
Verification: `uv run pytest` → 413 passed; `uv run ruff check
scan_supply_chain/ tests/` → clean; `uv run mypy --strict
scan_supply_chain/` → no issues in 37 source files.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The two POSIX-permission-bit tests in test_skip_report.py used @pytest.mark.skipif(os.geteuid() == 0, ...). On Windows that lookup crashes at collection time (no os.geteuid), failing all three Windows matrix jobs in PR #2 CI. NTFS ACLs ignore chmod(0o000), so the chmod-based assertions are vacuous on Windows for the same reason they are vacuous under root. Fold both cases into a single _POSIX_PERMS_INEFFECTIVE guard evaluated via hasattr, so the decorator no longer crashes during import on non-POSIX runners. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to e394904 — that commit added the guard constant but left the two @pytest.mark.skipif decorators still calling os.geteuid() directly, so Windows collection still crashed with AttributeError. Replace both decorators to reference the guard constant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three pre-existing failures surfaced once Windows collection started working (commits e394904, 9f948a0). These are real cross-platform issues in the tests, not in the scanner logic, and they were latent on master because Windows CI was crashing earlier in the pipeline. 1. test_anti_worm_scanner.py::test_corroboration_is_per_repo_not_global The scanner emits str(Path(repo_root)) as evidence. On Windows, str(Path("/repo/A")) is "\\repo\\A", not "/repo/A". Build the expected dict the same way the scanner builds the evidence string so the comparison holds on every platform. 2. test_persistence_scanner.py::TestCheckTmpScripts (5 tests) _check_tmp_scripts itself short-circuits on win32 (it guards Path("/tmp") behind sys.platform != "win32" at line 201). Mark the whole class skipif(win32) — testing a documented POSIX-only code path on Windows is testing nothing. The class's "ignores" tests were passing on Windows by accident (empty findings vs an empty-list assertion); the explicit skip removes that false-pass. Local: full pytest 413 passed; ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Brings the 26 test files and conftest.py up to the same strict-mypy standard the production package already meets. 402 errors → 0 errors; 413 tests still pass; ruff still clean; production package unchanged. Mostly mechanical: ` -> None` returns on test methods, parameter types on fixtures (`tmp_path: Path`, `monkeypatch: pytest.MonkeyPatch`, fixture-consumed types pulled from conftest's declared return types), and missing return types on @pytest.fixture functions. Three substantive fixes: - conftest.py threat-profile builders (make_litellm_threat, make_axios_threat) widened the `defaults` dict to dict[str, Any] and annotated **overrides: Any so the splat into ThreatProfile(...) type-checks. Override semantics preserved. - conftest.py tmp_as_tmp fixture rewritten to use monkeypatch.setattr instead of `ps.Path = lambda ...`. Removes three strict errors (lambda assignment to type[Path], untyped lambda, attr-defined on Path) and gets teardown for free. - conftest.py StubPolicy / StubEcosystem method bodies left intact, signatures annotated to match their respective Protocols (PlatformPolicy, EcosystemPlugin). No `# type: ignore` added in this commit. One pre-existing ignore on the DNS-call-recording lambda in test_ioc_artifact_detection.py was left as-is. Local: pytest 413 passed, mypy --strict tests/ clean, mypy --strict scan_supply_chain/ still clean, ruff check tests/ clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI lint runs ruff format --check . over the whole repo. Local verification only ran ruff check tests/, missing the formatter gate. The post-edit ruff format hook caught most files but five slipped through during the strict-mypy sweep (66e7a67). No semantic change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CognitiveSand
pushed a commit
that referenced
this pull request
May 15, 2026
Required by release.py, which refuses to bump the version unless CHANGELOG.md already contains an entry for the new version. Folds the prior Unreleased feature work (anti-worm pre-pass, three new threat profiles, branch_name_regexes, persistence keywords, git-artifact scoring, fail-loud profile loading, regex-at-load-time) together with PR #2's clean-code cleanup (ScanContext, module-state removal, phase-3 hoist, function decomposition, report split, complete type annotations, mypy --strict tests/, Windows test portability). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CognitiveSand
pushed a commit
that referenced
this pull request
May 15, 2026
Required by release.py, which refuses to bump the version unless CHANGELOG.md already contains an entry for the new version. Folds the prior Unreleased feature work (anti-worm pre-pass, three new threat profiles, branch_name_regexes, persistence keywords, git-artifact scoring, fail-loud profile loading, regex-at-load-time) together with PR #2's clean-code cleanup (ScanContext, module-state removal, phase-3 hoist, function decomposition, report split, complete type annotations, mypy --strict tests/, Windows test portability). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Nine-commit cleanup pass against the code-quality audit findings. Every commit leaves tests green; the branch ends at 413 passing tests with
ruff check,mypy --strict, andpytestall clean.Commits
246acd9ff5f230ScanContextdataclass — collapses the 6-param data clump across phase signaturesd14ececSkipReportexplicitly, replace_ecosystem_cachedict withfunctools.lru_cache, remove autouse reset fixture0fb17a7ioc_scannerintoscanner._run_phase3_iocs; drop deferred imports00d04e7scanner.main,_scan_for_c2_connections,_scan_walk_files,scan_source_and_configs; introduce_SourcePatterns/_FileSelector;main()returns int19eab37SkipReportconsistently; surface NXDOMAIN coverage on--resolve-c2; narrow_check_config_dirper-file scope4041a9c_user_threat_dir(); split 369-linereport.pyinto areport/package with five sub-modules (_references,_threat,_anti_worm,_skip,_summary)a79dba6skip_reportparameter85e5594ruff format; passmypy --strict(66 → 0 errors)Audit findings addressed
All MAJOR findings (CQ-01 through CQ-09) and all WARNING findings (CQ-11 through CQ-16, CQ-18). The audit's weakest axis was structure at 0.5 — that's the substance of passes 2–5.
Test plan
uv run pytest— 413 passed (was 408 baseline; +5 new regression tests for IPv6 parsing and TOML schema typo detection)uv run ruff check scan_supply_chain/ tests/— cleanuv run mypy --strict scan_supply_chain/— no issues in 37 source filespython3 run_scan.py --list-threatson a real machine — see belowpython3 run_scan.pyfull scan — see belowManual smoke tests
Run on Linux (kernel 7.0.0-15-generic), Python 3, from
~/git/scan-supply-chainas a non-root user.python3 run_scan.py --list-threatsAll 5 threat profiles render with header, package, compromised versions, and date.
python3 run_scan.py(full scan)Full scan output (click to expand)
Smoke-test verdict: every refactor pass holds end-to-end. Banner, phase-0 anti-worm pre-pass (59 repos × 43 indicators), per-threat phase 1–4 output, multi-threat summary, axios config-reference NOTE, and the consolidated skip summary (65 permission-denied paths) all render correctly. Scan completes clean — no compromise detected.
Note: two absolute paths in the axios phase-4 output (under
~/.vscode/extensions/mermaidchart.vscode-mermaid-chart-2.6.6/) were rewritten to tilde form in this PR description so the body would pass the project'sblock-abs-pathsBash hook; the scanner itself prints the full absolute paths.🤖 Generated with Claude Code