From f6f2ff1a9c5af202536ca5fe88f8ddbcea747040 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 28 Jul 2026 19:10:38 -0500 Subject: [PATCH] fix(tests): judge the .claude/ skip filter relative to the repo root, not the absolute path The spreadsheet-writer gate excludes `.claude/` so a repo-root walk cannot wander into a sibling worktree's checkout (#29). It tested `_SKIP_DIRS & set(path.parts)` on the ABSOLUTE path -- and docs/WORKTREES.md puts sibling worktrees at `.claude/worktrees//`, so when the suite runs FROM one of them the checkout ITSELF sits under `.claude/`. Every absolute path in the repo then contains `.claude`, the filter matches everything, and the walk collapses. Measured in a worktree: 5712 .py files found, 0 kept. Both tests red -- every recorded spreadsheet writer read as "no longer exists", because nothing was scanned to find them. The inversion is the subtle part. #29's docstring reasons about the main checkout and concludes that "in a sibling worktree (or in CI) there is nothing under `.claude/` to find, so it passes trivially". From a worktree the opposite is true: the filter matches EVERYTHING. A guard written to stop a scan leaking INTO `.claude/` blinded itself completely when run FROM there, and CI never sees it because CI has no nested worktrees -- which is exactly the local-only redness that docstring warns turns a gate into something people learn to ignore. Judged relative to the repo root, both directions hold: from the main checkout a sibling worktree's file is `.claude/worktrees//foo.py` and is still excluded (#29's actual purpose, preserved); from inside a worktree the same file is `harness/foo.py` and is kept. Verified both. Three call sites shared the defect and are now one helper (`_is_skipped`), including the `leaked` assertion -- fixing only the scan would have inverted that one instead, since it also matched on absolute parts and would have flagged every correctly-kept file as leaked. Credit where it is due: #29 shipped a non-vacuity assertion on its own gate (`len(scanned) > 500`, "the walk collapsed, so a pass proves nothing") and that is what named the failure precisely. The gate caught its own blindness. Added a regression test asserting BOTH directions, because neither is observable from the other and CI only ever exercises one of them. --- tests/test_csv_formula_consistency.py | 60 +++++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 3 deletions(-) diff --git a/tests/test_csv_formula_consistency.py b/tests/test_csv_formula_consistency.py index 62522757..c66bdfda 100644 --- a/tests/test_csv_formula_consistency.py +++ b/tests/test_csv_formula_consistency.py @@ -253,11 +253,27 @@ def _spreadsheet_writer_sites_in(source: str) -> bool: return False +def _is_skipped(path: Path) -> bool: + """True if `path` lies under a skipped directory, judged RELATIVE TO THE REPO ROOT. + + Relative is load-bearing, and the absolute form is actively wrong. docs/WORKTREES.md puts sibling + worktrees at `.claude/worktrees//`, so when the suite runs FROM one of them the checkout + ITSELF sits under `.claude/` — every absolute path in the repo then contains `.claude`, the filter + matches everything, and the walk collapses to zero files. Measured here: 5712 .py files found, + 0 kept. + + Both directions still work relatively: from the main checkout a sibling worktree's file is + `.claude/worktrees//foo.py` and is excluded; from inside a worktree the same file is + `harness/foo.py` and is kept. + """ + return bool(_SKIP_DIRS & set(path.relative_to(_REPO).parts)) + + def _spreadsheet_writer_sites() -> set[str]: """Repo-relative paths of every module that writes a spreadsheet format.""" found: set[str] = set() for path in _REPO.rglob("*.py"): - if _SKIP_DIRS & set(path.parts): + if _is_skipped(path): continue try: source = path.read_text(encoding="utf-8") @@ -495,10 +511,48 @@ def test_repo_root_scans_exclude_nested_worktrees() -> None: "a repo-root scan that does not skip .claude/ will walk a sibling worktree's full checkout" ) # And prove the exclusion actually bites: no scanned path may sit under .claude/. - scanned = [p for p in _REPO.rglob("*.py") if not _SKIP_DIRS & set(p.parts)] - leaked = [str(p.relative_to(_REPO)) for p in scanned if ".claude" in p.parts] + scanned = [p for p in _REPO.rglob("*.py") if not _is_skipped(p)] + # REPO-RELATIVE parts here too. Against absolute parts this check inverts when the suite runs from + # a worktree — the checkout lives under `.claude/`, so every kept file would read as "leaked" and + # the assertion would fail on a correct scan. + leaked = [str(p.relative_to(_REPO)) for p in scanned if ".claude" in p.relative_to(_REPO).parts] assert not leaked, f"nested-worktree files reached the scan: {leaked[:5]}" # Non-vacuity: the scan must actually be finding this repo's own files. assert len(scanned) > 500, ( f"only {len(scanned)} files scanned — the walk collapsed, so a pass proves nothing" ) + + +def test_the_skip_filter_is_judged_relative_to_the_repo_root() -> None: + """The exclusion must be repo-RELATIVE, or it inverts when the suite runs from a worktree. + + docs/WORKTREES.md puts sibling worktrees at `.claude/worktrees//`. Running from one, the + checkout itself sits under `.claude/`, so an ABSOLUTE-parts filter matches every path in the repo + and the walk collapses to zero — which is exactly what happened: 5712 .py files found, 0 kept, and + every recorded spreadsheet writer read as "no longer exists". + + Both directions are asserted here because fixing only one is what makes this subtle: the guard + must still exclude a sibling worktree when run from the MAIN checkout, which is its whole purpose. + Neither case is observable from the other, and CI only ever sees the main-checkout one. + """ + main = Path("/repo") + sibling = main / ".claude" / "worktrees" / "other" / "harness" / "report.py" + own = main / "harness" / "report.py" + + def skipped_relative_to(root: Path, path: Path) -> bool: + return bool(_SKIP_DIRS & set(path.relative_to(root).parts)) + + # From the main checkout: a sibling worktree's file is excluded, our own is not. + assert skipped_relative_to(main, sibling), "a sibling worktree must not be scanned" + assert not skipped_relative_to(main, own), "the repo's own files must be scanned" + + # From INSIDE that worktree the same file is `harness/report.py` — it must be scanned, not skipped. + worktree = main / ".claude" / "worktrees" / "other" + assert not skipped_relative_to(worktree, sibling), ( + "running from a worktree must not exclude that worktree's own files" + ) + + # And the absolute form this replaced fails that last case — pinned so it cannot come back. + assert _SKIP_DIRS & set(sibling.parts), ( + "sanity: the absolute path does contain a skipped part, which is why matching on it is wrong" + )