fix(tests): judge the .claude/ skip filter relative to the repo root, not the absolute path by wshallwshall · Pull Request #33 · MEFORORG/MessageFoundry

wshallwshall · 2026-07-29T01:01:44Z

Follow-up to #29, whose .claude/ exclusion
inverts when the suite runs from a worktree.

The filter tested _SKIP_DIRS & set(path.parts) on the absolute path. docs/WORKTREES.md puts
sibling worktrees at .claude/worktrees/<name>/, so running from one, the checkout itself sits
under .claude/ — every absolute path in the repo contains it, the filter matches everything, and the
walk collapses.

Measured in a worktree: 5712 .py files found, 0 kept. Both tests red, and every recorded
spreadsheet writer read as "no longer exists" — because nothing was scanned to find them.

Why this was easy to miss

#29's docstring reasons from 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
holds: 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, which is precisely the
local-only redness that same docstring warns turns a gate into one people learn to ignore.

The fix

Judge the exclusion relative to the repo root. Both directions verified:

Running from	Sibling worktree file	Own files
main checkout	excluded ✅ (#29's purpose, preserved)	scanned ✅
a worktree	n/a	scanned ✅ (was 0)

Three call sites shared the defect and are now one _is_skipped helper — including the leaked
assertion, which also matched absolute parts. Fixing only the scan would have inverted that one
instead, flagging every correctly-kept file as leaked; a partial fix just swaps one red for another.

Credit where it's 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 instead of leaving a baffling
"file no longer exists". The gate caught its own blindness.

One caveat on what green means here

This fix is invisible in CI — CI has no nested worktrees, so both tests passed there before and
after. The required checks going green does not confirm it. The evidence is the local full suite
(9159 passed, 797 skipped, from 2 failed) plus a regression test asserting both directions,
since neither is observable from the other.

… 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/<name>/`, 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/<x>/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.

wshallwshall · 2026-07-29T01:07:59Z

I shipped #29 — this is my bug, and the diagnosis is exact. Reproduced independently before reviewing: running the shipped filter with _REPO pointed at a nested worktree gives 5712 .py found → 0 kept, matching your number to the file.

The leaked catch is the sharp part of this PR. That assertion also matched absolute parts, so repairing only the scan would have flipped it red instead — swapping one failure for another and looking, from the outside, like the fix didn't work.

One thing to fix before this merges: the new test doesn't reach the code it guards

test_the_skip_filter_is_judged_relative_to_the_repo_root defines a local skipped_relative_to and asserts against that. It never calls _is_skipped. I AST-walked every test in the file for calls to the shipped helpers:

test_no_unrecorded_spreadsheet_writer_exists              _spreadsheet_writer_sites
test_repo_root_scans_exclude_nested_worktrees             _is_skipped
test_the_skip_filter_is_judged_relative_to_the_repo_root  -- reaches NEITHER --
(16 others)                                               -- reaches NEITHER --

And the one test that does call _is_skipped can't distinguish the two filters either. From the main checkout — which is all CI ever runs:

scanned 11422 .py files
  absolute-parts filter (the BUG) kept 960
  relative-parts filter (the FIX) kept 960
  differ by 0 files

So the caveat is broader than "the fix is invisible in CI." Revert _is_skipped to set(path.parts) tomorrow and this entire file stays green in CI. Nothing observes the change. The new test asserts that a relative filter behaves relatively — true by construction, and it stays true regardless of what ships.

That's the identity-confirmation shape: it passes because of how it is written, not because of what the code does. Same class as the bug it is fixing, one level up.

Remedy

_is_skipped hardcodes _REPO, which is why the test had to reimplement it. Parameterize the root and the test can drive the real function with synthetic paths — no filesystem dependency, CI-observable, and killable by mutation:

def _is_skipped(path: Path, root: Path = _REPO) -> bool:
    return bool(_SKIP_DIRS & set(path.relative_to(root).parts))

assert _is_skipped(sibling, main)            # from the main checkout: sibling worktree excluded
assert not _is_skipped(own, main)            # the repo's own files scanned
assert not _is_skipped(sibling, worktree)    # from inside it: that worktree's own files scanned

Worth confirming it fails as expected with the absolute form restored, so the guard is known-live rather than assumed-live.

Not blocking

The production fix is correct and I'd merge it as-is rather than let the inversion sit on main — the guard gap is a follow-up, not a reason to hold this. Flagging it because a green CI on this PR is currently indistinguishable from a green CI on the unfixed code, which is the exact thing the assertion was added to prevent.

Credit where it's due on the other side: you're right that len(scanned) > 500 is what named the failure. That assertion exists because a silent zero-file walk had already burned us once, and it did its job in someone else's hands.

… of its rule (#34) #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`).

…DR 0034 triage register (#37) * test(cert-cli): assert the exact DN instead of a hostname substring CodeQL py/incomplete-url-substring-sanitization (alerts 119/120/121) flagged three `"<host>" in <str>` assertions in the cert-inventory tests. There is no URL and no sanitization here — `cert inventory` is a read-only report and nothing in the engine makes a trust decision from the rendered subject/issuer — so it is not the vulnerability the query models. The assertions were genuinely weak, though: * `"good.example.org" in g["subject"]` also passes for a lookalike CN, and the test's own SAN list contains `www.good.example.org`; `read_cert_facts` returns `cert.subject.rfc4514_string()`, so the exact expected value is `CN=good.example.org`. * `"human.example.org" in printed` is satisfied by the SAN line alone, so it would keep passing if the human renderer stopped emitting the subject line at all. Tightened to an exact DN comparison and to a subject-line-specific check. No coverage is removed; both tests now fail on a defect they previously accepted. * fix(api): kill the quadratic Content-Disposition scan in the multipart parser CodeQL alert 125 (py/polynomial-redos, high). `(\w+)="([^"]*)"` restarts at every offset inside a word run and walks the rest of that run before failing, so a Content-Disposition line of n word characters costs O(n^2). The header block is attacker-supplied, bounded only by [store].max_upload_bytes (25 MiB default), and parse_single_file_upload runs synchronously on the asyncio event loop -- so one POST /uploads could wedge the entire engine. Measured before: 2k->10ms, 4k->39ms, 8k->156ms, 16k->613ms, 32k->2895ms (clean quadratic); extrapolated to the 25 MiB cap, ~22 days of blocked event loop. After: the same 25 MiB hostile body parses in 353 ms. The fix is a leading (?<!\w) lookbehind, which is O(1) and rejects every offset inside a run immediately, leaving one \w+ walk per run. It removes no match -- `=` is not a word character, so \w+ starting inside a run can only succeed at that run's end, meaning an interior offset matches iff the run's first offset does, and the leftmost scan always reaches the first offset earlier. Pinned by a differential test against the pre-guard pattern plus a growth-ratio test. Refs ADR 0034. * fix(ide): resolve each scanned module once, by descriptor, not twice by path CodeQL js/file-system-race (alert 111). buildSymbolIndex size-checked with statSync(path) and then read with readFileSync(path) — two independent path resolutions with a window between them, so the file that was READ need not be the file that was CHECKED. Exploitability is low (same-privilege, same extension host, over a config dir the extension itself enumerated, and the checked property is a resource guard rather than an authorization decision — an attacker who can swap the file can just write a large .py directly). The reason to fix is non-adversarial correctness: in a live workspace a save, a formatter or a codegen step rewrites a module between the two calls routinely, so the maxBytes guard was unsound as written. The read now goes through readCapped(), which opens once and does fstatSync + readFileSync on that descriptor; a finally closes it on every exit, including the oversize skip (readFileSync does not close a descriptor it is handed). Behaviour is otherwise unchanged: still never throws, still skips an unreadable or oversized file. Pinned by two tests: the size cap still holds, and fs spies assert readFileSync is only ever handed a NUMBER (never a path), that no path-based statSync runs, and that every opened descriptor is closed. Refs ADR 0034. * fix(ci): drop the unpinned pip bootstrap from the SBOM scratch venv Scorecard PinnedDependenciesID (alerts 115 and 118): release.yml:177 and security.yml:140 each ran `/tmp/sbomenv/bin/pip install --upgrade pip` — an unpinned, unverified PyPI install — immediately before the only thing that venv ever installs, a fully ==-pinned, hash-verified lock. --require-hashes performs no dependency resolution at all, so the pip ensurepip provisions is sufficient and the upgrade bought nothing. In release.yml it ran inside the job holding contents/id-token/attestations: write. Deleting the command both removes an unpinned install from the release path and closes the alert, so these two are the only PinnedDependencies findings in the group that resolve without a CI-tool lock (the rest stay open, blocked on DEP-1). Deliberately NOT replaced with `python -m venv --upgrade-deps`, which performs the same unpinned fetch while hiding it from the scanner — ADR 0034 option 3, rejected in favour of a visible dismissal over an invisible filter. The new test fails on either regression, and both halves were mutation-checked by reintroducing the deleted line and the --upgrade-deps variant. Refs ADR 0034. * test(api): make the ReDoS growth-ratio check noise-proof The linear-time guard on the Content-Disposition regex (alert 125) compares a 20k-char scan against an 80k one and fails above 8x. Both samples were single shots of ~0.2ms/0.9ms, so one scheduling slice on a loaded CI runner could inflate the large sample past the threshold and red the build for a timing hiccup rather than a real regression. Take the best of three per size instead: a hiccup can only inflate a sample, never deflate one, so the minimum is the noise-free estimate. Measured here at 219us / 875us (ratio 4.0) with the guard, and 1.0s / 15.7s (ratio 15.8 — the gate fires) against the pre-guard pattern, so the check still detects the very regression it exists for. * fix(ci): the DEP-1 lock-check venv had the same unpinned pip bootstrap The previous pass deleted `<venv>/bin/pip install --upgrade pip` from the two SBOM scratch venvs but missed the third instance of the identical construct, 63 lines above one of them: security.yml's `/tmp/lockcheck`, whose only install is `--require-hashes -r requirements.lock`. Every word of that pass's rationale applies verbatim — --require-hashes rejects any un-hashed requirement and so performs no resolution at all, making the pip ensurepip provisions sufficient — so the bootstrap bought nothing while adding an unpinned, unverified PyPI fetch to the DEP-1 gate itself. It is code-scanning alert #71, currently dismissed "won't fix" with the reason "CI uses editable installs (pip install -e .[extras]) for testing, which cannot use --require-hashes." That is factually wrong for this line: nothing here is an editable install, and the very next line IS a --require-hashes install. ADR 0034 requires a recorded reason; a wrong one is worse than an open finding, so #71 must be closed as fixed rather than renewed. Both earlier edits are also made LINE-NEUTRAL. This scanner re-raises the same expression at a new line as a NEW alert number — dismissed #18 (__main__.py:976) re-fired as open 122 (:1535), dismissed #39 (dependabot-auto-merge.yml:28) as open 87 (:44), both pure line drift. The 11 added comment lines would have shifted dismissed #35/#69 in release.yml and #74/#75/#76 in security.yml onto new lines, re-opening ~5 findings to close 2. Each rationale block is now one line; the argument lives in the test module's docstring, where it cannot move an anchor. The guard is generalized accordingly and renamed: it covers all three lock-only scratch venvs, and matches `<venv>/bin/python -m pip install` as well as `<venv>/bin/pip install` — the former is the spelling used elsewhere in these same workflows and the old guard was blind to it. Verified by mutation: reinstating the bootstrap in either spelling, and hiding it behind `venv --upgrade-deps`, each turn the guard red. * fix(api): bound a multipart part's header block before parsing it The `(?<!\w)` lookbehind made the Content-Disposition scan linear, but left its INPUT sized by the attacker. `max_file_bytes` caps a part's content, and only after its header has already been parsed, so the header block's real bound was the request body cap — `[store].max_upload_bytes`, 25 MiB by default and 512 MiB at the ceiling, raised for the two upload paths by the body middleware. Linear is not free at that size: `parse_single_file_upload` runs synchronously on the asyncio event loop that also drives every listener, router worker, transform worker and delivery worker, so one request blocks the whole engine for ~0.35 s at the default and ~7 s at the ceiling. A stalled loop stops ACKing MLLP senders and stops draining the staged queue. `_MAX_PART_HEADER_BYTES` (16 KiB) is orders of magnitude above anything a real client sends — a Content-Disposition plus a Content-Type is a couple hundred bytes, pinned by a non-vacuity test asserting a realistic header is 50x under the limit. Refuse rather than skip: skipping would surface as the confusing "no file part" error instead of naming the actual problem. It maps to the existing 400. The two controls stay independent on purpose. The cap is a size policy someone could reasonably raise; the linear-time assertion holds at any size. Incidentally the bound also gives the ReDoS analysis a length-bounded source rather than an attacker-sized one, which matters because the query models a lookaround as a zero-width assertion and may not credit the lookbehind. Verified by mutation: deleting the guard block reds test_oversized_part_header_is_refused_not_parsed. * docs(ide): claim only the identity guarantee the fd rewrite actually buys readCapped's docstring implied the fd rewrite restored the maxBytes cap. It does not. `fs.readFileSync(fd)` re-stats the descriptor itself and reads whatever length it then finds, so a file appended to in place between the `fstatSync` and the read is still read in full — the size check is an early-out, not a bound. What the rewrite does buy is real and worth stating exactly: one path resolution instead of two, so `fstatSync(fd)` and `readFileSync(fd)` cannot disagree about WHICH file they touched. That is the identity guarantee, and it is what CodeQL js/file-system-race flagged. The residual is recorded rather than papered over: maxBytes has always been a best-effort resource guard ("a generated blob isn't a feed"), never an authorization decision; the cost of losing it is memory for one oversized regex pass; and scanModuleSymbols returns only {name, kind, file, line}, never file content, so nothing leaks through it. Making the cap sound would take a bounded readSync into a pre-sized buffer — deliberately not done here, since it would also rewrite the fd-spy test and this worktree cannot run the ide mocha suite. Comment-only: the 127 non-comment lines are byte-identical before and after. * docs(adr): record the second static-analysis triage round in ADR 0034 ADR 0034 AC-3 requires the class-level rationale and the accepted-risk register to live in-repo so re-scans converge instead of re-litigating. 32 open findings were triaged and the register had not been touched, so the whole round existed only in per-alert comments. Appended as a dated amendment rather than an edit: the repo's own slug-rot guard treats docs/adr/ as historical by construction — an ADR should describe the topology of ITS day — so the 2026-06-26 text stays intact and this section governs where they disagree. Four things the amendment records that a re-scan would otherwise lose: Topology correction. The original Context and the whole Scorecard register rest on MEFORORG being a read-only mirror fed by force-pushed snapshots. Since the cutover it is the primary development repo; publish.ps1 and the release-sync check are gone. Three dismissals reasoned ENTIRELY on that premise — BranchProtectionID (#33), CodeReviewID (#77), MaintainedID (#78) — now carry a justification that is no longer true and must be re-triaged, not renewed. log-injection, narrowed. The register claims control characters "in every emitted record" are neutralized by ControlCharScrubFilter. Precisely: that filter scrubs the RENDERED MESSAGE (record.getMessage() -> record.msg, so the %-args are covered) and does NOT touch record.exc_text or record.stack_info. RedactionFilter renders and PHI-redacts those but does not escape control characters, and the text formatter then appends exc_text verbatim. Verified by execution: a ValueError carrying a newline, logged with exc_info=True, lands on its own physical line. All four log-injection alerts this round are lazy %-arg sinks with no exc_info so the dismissals stand on their own traces — but the class rationale must not be inherited by a future finding on a log.exception site, and the engine has many. PinnedDependenciesID, corrected. "CI installs editably, which cannot use --require-hashes" is structurally true of the editable installs and was applied too widely: three scratch venvs whose only install is a hash-verified lock carried an unpinned pip bootstrap that bought nothing. Also records the proof, from this repo's own alert data, that an == pin does NOT satisfy the check (bandit==1.9.4 is still flagged) — only --require-hashes does, which couples to DEP-1. Convergence and residuals. The line-drift rule (same expression, new line, new alert number, with both confirmed instances), and a table of five hardening items found while justifying won't-fix dismissals — an unpinned sigstore inside the signing job foremost — which a dismissal would otherwise make invisible. AC-5/6/7 added for the guards this round introduced.

wshallwshall enabled auto-merge (squash) July 29, 2026 01:02

wshallwshall mentioned this pull request Jul 29, 2026

fix(tests): the spreadsheet-writer gate scanned a sibling worktree's entire checkout #29

Merged

wshallwshall merged commit 9921fe9 into main Jul 29, 2026
32 checks passed

wshallwshall deleted the claude/nested-worktree-filter-fix branch July 29, 2026 01:23

wshallwshall mentioned this pull request Jul 29, 2026

test(csv-gate): drive the real skip filter from the guard, not a copy of its rule #34

Merged

Navigation Menu

Uh oh!

fix(tests): judge the .claude/ skip filter relative to the repo root, not the absolute path - #33

fix(tests): judge the .claude/ skip filter relative to the repo root, not the absolute path#33
wshallwshall merged 1 commit into
mainfrom
claude/nested-worktree-filter-fix

wshallwshall commented Jul 29, 2026

Uh oh!

wshallwshall commented Jul 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

wshallwshall commented Jul 29, 2026

Why this was easy to miss

The fix

Credit where it's due

One caveat on what green means here

Uh oh!

wshallwshall commented Jul 29, 2026

One thing to fix before this merges: the new test doesn't reach the code it guards

Remedy

Not blocking

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant