Skip to content

fix(detect): gate coverage/ pruning on real report markers (#2339) - #2353

Closed
Rishet11 wants to merge 1 commit into
Graphify-Labs:v8from
Rishet11:fix/2339-coverage-skipdir
Closed

fix(detect): gate coverage/ pruning on real report markers (#2339)#2353
Rishet11 wants to merge 1 commit into
Graphify-Labs:v8from
Rishet11:fix/2339-coverage-skipdir

Conversation

@Rishet11

Copy link
Copy Markdown
Contributor

Closes #2339

The bug

"coverage" and "lcov-report" were in the unconditional _SKIP_DIRS set (graphify/detect.py:798), and _is_noise_dir(part, parent) matches a bare directory name at any depth with no content check. A real source package named coverage/ was therefore pruned before anything read it, with no warning.

/tmp/cov-repro/main.py            (import coverage)
/tmp/cov-repro/coverage/__init__.py
/tmp/cov-repro/coverage/impact.py

before: detect(...)["files"]["code"] == ["/private/tmp/cov-repro/main.py"]
after:  ["/private/tmp/cov-repro/coverage/__init__.py",
         "/private/tmp/cov-repro/coverage/impact.py",
         "/private/tmp/cov-repro/main.py"]

The fix

No new mechanism — this follows the precedent already in the same function. env/.env/*_env and snapshots were previously moved out of the unconditional set and are pruned only on real evidence (_has_venv_markers, and the *.snap / _JS_SNAPSHOT_TEST_ROOTS gate) for exactly this bug class (#1666 / #2058).

  • coverage/lcov-report move out of _SKIP_DIRS.
  • New _has_coverage_markers(d), styled after _has_venv_markers: prunes on lcov.info, coverage-final.json, clover.xml, coverage.xml, cobertura.xml, an index.html alongside Istanbul/nyc static assets (base.css/prettify.js/block-navigation.js/sorter.js), any *.gcov, or a nested lcov-report/.
  • As with the existing gates, a None parent cannot be verified, so the directory is kept.

__snapshots__ and storybook-static stay unconditional — those names are unambiguous.

Scope decision, stated rather than hidden

visual-tests/visual-test (detect.py:799) and dist-protected (:802) are pruned the same unconditional way and are plausible real directory names, so they are the same bug class. I looked for evidence as crisp as the coverage markers and did not find any — Playwright/visual-regression output has no equivalent of lcov.info or index.html+base.css. Rather than guess at a marker set, I left both alone. Happy to follow up if you know the right signal.

Verification

  • 6 new tests in tests/test_detect.py: real coverage/ package kept, genuine lcov/Istanbul report still pruned, both again for a coverage/ nested deeper in the tree, and _is_noise_dir("coverage", None) is False.
  • Red before the fix: stashed only detect.py, 3 of the 6 fail (the 3 "still pruned" ones pass either way by design).
  • tests/test_detect.py: 232 passed. test_detect_skips_coverage_dir and test_detect_skips_visual_tests_dir stay green for the right reason — they plant report-like files, so the evidence gate still prunes them.
  • Full suite: 3829 passed / 36 skipped / 8 failed, and the same 8 fail on a clean v8 tree in this environment (1 in test_llm_backends.py from a missing openai module, 7 env-dependent ollama backend-detection tests). Failure set unchanged — I verified that by stashing and re-running, not by assuming.
  • ruff clean. python -m tools.skillgen --checkcheck OK: 134 artifact(s) match committed output and expected/.

Reviewed by a fresh-eyes pass that traced _is_noise_dir end to end, confirmed the new gate is actually reached (no earlier _SKIP_DIRS short-circuit), confirmed the only _is_noise_dir caller (detect.py:1334, the os.walk prune loop) always passes a real parent, and confirmed _has_coverage_markers runs once per directory rather than once per file.

…Labs#2339)

"coverage" and "lcov-report" were in the unconditional _SKIP_DIRS set, and
_is_noise_dir matches a bare directory name at any depth with no content
check, so a real source package named coverage/ was pruned before anything
read it.

Move both names out of the unconditional set and prune them only on actual
report evidence via a new _has_coverage_markers helper, mirroring the
existing gating for env/.env/*_env (_has_venv_markers) and snapshots. As
with those, a None parent cannot be verified and keeps the directory.
Copilot AI review requested due to automatic review settings July 31, 2026 20:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).

Graphify reviewed this change.

Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).


Graphify review — findings

This PR changes how the directory-pruning logic in graphify/detect.py handles directories named coverage and lcov-report. Instead of always skipping them by name, it removes them from the unconditional _SKIP_DIRS set and adds a new _has_coverage_markers helper plus a branch in _is_noise_dir so these dirs are only pruned when actual coverage/report evidence is present on disk (e.g. lcov.info, coverage-final.json, an index.html with Istanbul assets, *.gcov, or a nested lcov-report/). The test suite adds several new cases in tests/test_detect.py covering both the "real source package named coverage/ is kept" and "genuine report dir is still pruned" scenarios, including nested paths and the no-parent case, and imports _is_noise_dir to test it directly. The surface area is limited to the noise-directory detection path and its associated tests.

No blocking issues surfaced. 2 lower-confidence candidates did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 1180 functions depend on the 433 functions this change touches.

Health — this change adds coupling hotspots:

  • worse: detect() — 85 callers, 11 callees
  • worse: _is_noise_dir() — 8 callers, 2 callees

Verification — 1180 functions in the blast radius were not formally verified this run (proofs are advisory here).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

  • verification_scope: 626 function(s) in the blast radius were not formally verified this run

· 1 grounded finding(s) anchored inline below; 1 more finding(s) on lines outside this diff (see the check run).

Comment thread graphify/detect.py
return False


def _is_noise_dir(part: str, parent: "Path | None" = None) -> bool:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Health regression_is_noise_dir()

8 callers depend on it (afferent coupling).

Grounded coupling-delta finding (deterministic), not an LLM guess.

@Rishet11

Rishet11 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Correction to my own PR body, before you hit it while reviewing.

I wrote that the only _is_noise_dir caller is detect.py:1334 and that it always passes a real parent. That is wrong. On v8 there are four more in graphify/extract.py:

site parent passed? effect of this PR
extract.py:3669 no walks into a genuine report dir
extract.py:5664 no a target path with a coverage component is no longer refused outright
extract.py:5675 yes (dp) evidence-gated, genuine reports still pruned
extract.py:5696 yes (dp) evidence-gated, genuine reports still pruned

That claim came from a review pass I ran, and I repeated it without grepping it myself. My mistake.

What it means in practice, having looked at each:

  • extract.py:3669 is the C# helper walk, and it only collects *.cs. With no parent the gate cannot verify, so it now descends into a real Istanbul/lcov report instead of skipping it by name. Nothing gets indexed from there — HTML reports contain no .cs — so this is wasted directory walking on a large report tree, not over-inclusion.
  • extract.py:5664 checks the components of the target path itself. Before this PR, graphify ./coverage returned [] and refused to scan at all. Now it proceeds, which is the actual point of coverage/ in _SKIP_DIRS silently drops real Python packages named 'coverage' (same class as #1666 snapshots / #2058 env) #2339 for someone whose package is named coverage/, and if a user explicitly points at a report directory, scanning it is defensible.
  • 5675 and 5696 both pass the parent, so the two real scan walks — plus detect.py:1334 — still prune genuine reports on evidence. coverage/*.html should be filtered by directory, not extension #870 is honored where it matters.

Worth noting this is not new behavior I am introducing so much as the existing shape of the gate: env/.env/*_env (#2058) and snapshots (#1666) already return False at those same parentless sites, deliberately, with the comment "cannot verify; keep a possibly-real code dir". coverage now behaves the same way. If you would rather the parentless sites stayed strict for coverage specifically, the cleanest fix is to thread the parent into 3669/5664, and I am happy to do that — but that touches call sites beyond #2339, so I did not assume it.

The detect.py:799 / :802 citations in the body are correct as written; they refer to v8, not to the post-fix file.

@safishamsi

Copy link
Copy Markdown
Collaborator

Closing as superseded. The coverage/ prune fix for #2339 shipped in v0.9.32 via #2351. Thanks, @Rishet11.

@safishamsi safishamsi closed this Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

coverage/ in _SKIP_DIRS silently drops real Python packages named 'coverage' (same class as #1666 snapshots / #2058 env)

3 participants