From 59dbc480ebd67e50cf035cda8b2394e49fedbc3c Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 28 Jul 2026 20:29:00 -0500 Subject: [PATCH] test(csv-gate): drive the real skip filter from the guard, not a copy of its rule #33 fixed `_is_skipped` to judge `_SKIP_DIRS` on repo-RELATIVE parts, which is correct. Its regression test, though, defines a local `skipped_relative_to` and asserts against that -- it never calls `_is_skipped`. A copy of the rule inside the test passes however the real function behaves. Nothing else covered the gap. AST-walking every test in the file, only `test_repo_root_scans_exclude_nested_worktrees` reaches `_is_skipped`, and it cannot tell the two forms apart from the main checkout -- measured, both keep the same 960 of 11422 files. So a silent revert to `set(path.parts)` was invisible: CI green, local green, guard asleep. Demonstrated rather than argued. Injecting the SAME regression into both versions: #33's guard + `set(path.parts)` -> 18 passed (blind) this guard + `set(path.parts)` -> 1 failed (killed) `_is_skipped` now takes `root: Path = _REPO` so the guard can drive the shipped function from both vantage points with synthetic paths -- filesystem-free, and therefore effective in CI, which never runs from a worktree. Hardcoding `_REPO` is precisely what forced the test to re-implement the rule. The defect class is the one the fix was about, one level up: a check that holds because of how it is written rather than because of what the code does. Verified: ruff format + ruff check clean; 18 passed on the file, 94 passed across it plus the three modules that reference it. mypy reports 5 errors in this file both before and after -- pre-existing, and `tests/` is not in CI's mypy scope (`mypy messagefoundry messagefoundry_webconsole`). --- tests/test_csv_formula_consistency.py | 30 +++++++++++++++++++-------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/tests/test_csv_formula_consistency.py b/tests/test_csv_formula_consistency.py index c66bdfda..e70225b3 100644 --- a/tests/test_csv_formula_consistency.py +++ b/tests/test_csv_formula_consistency.py @@ -253,8 +253,8 @@ 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. +def _is_skipped(path: Path, root: Path = _REPO) -> bool: + """True if `path` lies under a skipped directory, judged RELATIVE TO `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 @@ -265,8 +265,16 @@ def _is_skipped(path: Path) -> bool: 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. + + `root` is a PARAMETER rather than a hardcoded `_REPO` so the guard below can drive this exact + function from both vantage points using synthetic paths. That matters more than it looks: with + `_REPO` baked in, a test can only re-implement the rule and assert against its own copy — which + passes no matter what this function does. The two forms are also indistinguishable from the main + checkout (measured: both keep the same 960 files), so CI cannot tell them apart either, and a + silent revert to `path.parts` would go unnoticed everywhere. `scripts/security/scan_forbidden.py` + settled on the same repo-relative rule for the same reason. """ - return bool(_SKIP_DIRS & set(path.relative_to(_REPO).parts)) + return bool(_SKIP_DIRS & set(path.relative_to(root).parts)) def _spreadsheet_writer_sites() -> set[str]: @@ -534,21 +542,25 @@ def test_the_skip_filter_is_judged_relative_to_the_repo_root() -> None: 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. + + Every assertion drives the SHIPPED `_is_skipped` via its `root` parameter, never a local + re-implementation of the rule. A copy of the rule inside the test would pass however the real + function behaves, which is the same defect one level up: a check that holds because of how it is + written rather than because of what the code does. Synthetic paths keep it filesystem-free, so + unlike `test_repo_root_scans_exclude_nested_worktrees` — which can only catch this when the suite + happens to run FROM a worktree — this one has teeth in CI. """ 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" + assert _is_skipped(sibling, main), "a sibling worktree must not be scanned" + assert not _is_skipped(own, main), "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), ( + assert not _is_skipped(sibling, worktree), ( "running from a worktree must not exclude that worktree's own files" )