Skip to content

Skip extracted test modules in the hazard scanner by how they load, not by name - #443

Closed
lamemustafa wants to merge 2 commits into
masterfrom
fix/hazard-scanner-extracted-test-modules
Closed

Skip extracted test modules in the hazard scanner by how they load, not by name#443
lamemustafa wants to merge 2 commits into
masterfrom
fix/hazard-scanner-extracted-test-modules

Conversation

@lamemustafa

@lamemustafa lamemustafa commented Sep 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

#440, #441 and #442 moved inline #[cfg(test)] mod tests { … } blocks into sibling files loaded by #[cfg(test)] #[path = "…"] mod tests;. check-tally-request-builder-hazards.mjs skipped only the inline form, so it began reading those files as production code. That could raise false alarms. The fix below skips extracted test files without ever exempting a production file, because that would silently hide a real hazard.

How it decides, and why a lexer mistake can't hide a hazard

1. The quarantine (testOnlyModuleFiles) marks a file test-only when a trusted edge reaches it and no veto names it.

  • Edges: a brace-depth-0 out-of-line mod with a cfg that implies test, either cfg(test) or test inside cfg(all(…)). The mod must carry #[path], or be bare in a crate root or mod.rs. Any declaration made by an already test-only file also counts, iterated to a fixed point.
  • Vetoes: gathered from every .rs file in the repo and matched by basename, case-insensitively.
    • any other mod name; at any depth
    • any path = "…" or include*! string that isn't part of an edge
    • every Cargo root, including inline tables, single quotes and build =
  • Refusal: if any file contains a loader the lexer doesn't model, the quarantine throws and names the file and line. That covers mod $metavar, mod r#name, a comment inside a declaration or path attribute, a backslash path, include! without a literal argument, and include! of a non-.rs file. None of these occur in the repo today.

2. The pinned set (EXPECTED_TEST_MODULE_FILES) lists the 74 skipped paths exactly, the same way the violations are pinned. This is the safety net. Lexing Rust can't see every way a file might be loaded, so no mistake may silently add a skip. Any change fails the gate, names the file, and needs a person to confirm the file is loaded only under #[cfg(test)] before adding it.

How it got here

  • Design critique, before any code. An independent critique found four ways the first draft could skip a production file, and showed it would still have scanned 24 of the 74 extracted files. The result was the veto set, the fixed point, and the cfg(all(test, …)) and bare-mod.rs shapes.
  • Adversarial review of the first implementation. It built valid-Rust inputs that got a hazard through: macro metavariables, several include! forms, comments inside declarations, c-strings and emoji char literals mis-lexing a file, comments inside cfg, and Cargo root forms. The first implementation skipped the hazard file in 21 of its 23 cases. Its recommendation to pin the set rather than a count also closes a swap: one wrong skip replacing one removed module would leave a count unchanged.

Measured

  • Attack harness: every one of the 23 inputs is now scanned or refused loudly. None skips the file.
  • Real repo: the pinned set is exactly the 74 files that Report pins a branch drops and modules it leaves unsealed, without refusing either #436's rustc-validated module graph marks test-only, compared path by path. The 10 violations are unchanged.
  • Tests: 22. 30 mutations each fail a named test: the original 15, the reviewer's survivors, and one per fix. Two survived at first because their fixtures didn't reach the gap: an escape instead of the literal emoji, and a path string in an attribute instead of a loose include_str!. The fixtures were corrected.
  • Entry point: import.meta.main falls back to comparing real paths. The job that runs this gate (tally-portable) doesn't pin Node, and an older Node would otherwise turn the gate into a silent no-op. A test runs the real script as a child process.
  • Other checks: the frontend suite passes (261 node, 113 vitest, 10 Playwright), and check-ci-workflow-consistency.mjs passes. The scanner is itself pinned: resealing changed exactly its hash, and --verify exits 0.

Tradeoffs to know

  • New extraction: a future PR that extracts another test module must add its path to EXPECTED_TEST_MODULE_FILES. The gate says so, by name.
  • Refusal false alarms: the refusal patterns match raw source, so the same text in a comment or string can trip them. That fails loudly rather than skipping wrongly.
  • Scope: check-unbounded-reads.mjs still treats *_tests?.rs as test code by name and is untouched. Say if you want it moved onto this rule.

🤖 Generated with Claude Code

@lamemustafa @claude
Skip extracted test modules in the hazard scanner by how they load, n…
cc1510b 
…ot by name

#440, #441 and #442 moved inline `#[cfg(test)] mod tests { ... }` blocks
into sibling files loaded by `#[cfg(test)] #[path = "..."] mod tests;`.
check-tally-request-builder-hazards.mjs skipped only the inline form, so it
began reading those files as production code. Results did not change, but
a test that builds a hazard-shaped string as an expectation would have
raised a false alarm.

Skipping any file named *_tests.rs was rejected: a safety scanner must not
exempt production code for its file name. Instead a file is skipped only
when it is actually loaded as a test module:

- Edges: a brace-depth-0 out-of-line `mod name;` whose attribute group has
  a cfg implying test (cfg(test), or test as an argument of cfg(all(...))),
  with #[path] or bare in a crate root or mod.rs; plus any such declaration
  made by a file already test-only, iterated to a fixed point.
- Vetoes, from every .rs file in the repository and matched by basename
  case-insensitively: any other `mod name;` at any depth (including macro
  bodies and inline modules), any `path = "..."` or include!-family string
  not part of an edge (covering cfg_attr), and every Cargo crate root.
- Files whose braces underflow or do not end balanced contribute no edges.
- Everything unhandled -- cfg(any(...)), #[path] inside an inline module,
  r#name -- leaves the file scanned, so an error is a false alarm, never a
  missed hazard.

The design was critiqued before implementation. The critique found four
ways the first draft could skip a production file -- a non-test
declaration it did not recognise, a Cargo target, a declaration outside the
scan roots, case variants -- and that it would still have scanned 24 of the
74 extracted files: 22 declared by other test files, one under
cfg(all(test, ...)), one bare in a mod.rs. The veto set, the fixed point and
those two cfg/bare shapes are the response.

Checked, not asserted: on this repository the skipped set is exactly the
74 files the rustc-validated module graph from #436 marks test-only, path
for path; the 10 pinned violations are unchanged. The skipped count is now
pinned too, so a lexer regression that quarantines too much fails the gate.

Main now runs under `import.meta.main` so the functions can be imported;
a test runs the real script as a child process so a main that silently
does nothing cannot pass. 15 fixture tests; 15 mutations of the scanner
(trust file names, accept any cfg mentioning test, drop each veto, trust
unbalanced files, accept nested declarations, drop propagation, drop the
crate-root veto, case-sensitive veto, #[path] under the stem, main never
running, no bare-mod detection, strings or comments not skipped, only the
first attribute, no raw-string path) each fail a named test. One of those
first survived because a fixture's production declaration masked the
lexer; the fixture was fixed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 16, 2026

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@lamemustafa @claude
Pin the skipped set and refuse loaders the lexer cannot model
2521479 
Adversarial review of the previous commit found it could skip a
production file. The current tree is unaffected, but 21 of 23 attack
inputs -- all valid Rust -- made the scanner skip a file holding a
hazard: modules declared through macro metavariables, include! written
any way but the one form the lexer read, comments inside declarations and
path attributes, a c-string or a non-BMP char literal mis-lexing the rest
of a file, a comment or raw string inside cfg(all(...)), and Cargo roots in
inline tables, single quotes or `build =`.

The lesson is that proving "only test code loads this file" by lexing Rust
cannot be made complete, and every gap is a missed hazard. So:

- Pin the exact skipped set, not its count. A lexer mistake can no longer
  add a skip silently: the gate fails naming the file, and a person adds
  it only after confirming it is loaded solely under #[cfg(test)]. Pinning
  a count would also have let one wrong skip swap for one removed module.
- Refuse, naming the file and line, when any .rs file contains a loader
  the lexer does not model: `mod $metavar`, `mod r#name`, a comment inside
  a module declaration or path attribute, a backslash path, include!
  without a literal argument, or include! of a non-.rs file. Matched on raw
  source so it errs loud. None occurs in the repository.
- Fix the demonstrated lexing gaps: c"/cr#" strings, non-BMP char
  literals, arbitrary whitespace before a path/include string, and pruning
  only a crate's own target/ rather than any directory with that name.
- A comment or raw string inside a cfg is not parsed and does not imply
  test. Cargo roots are read from inline tables, single quotes and `build =`.
- `import.meta.main` falls back to a real-path comparison: the job that
  runs this gate does not pin Node, and an older Node would otherwise make
  the gate a silent no-op.

Measured, not asserted: the reviewer's attack harness now reports every
input as scanned or refused -- none skipped -- where the previous commit
skipped 21. On the repository the pinned set is unchanged at the 74 files
the rustc-validated module graph marks test-only; the 10 violations are
unchanged. 22 tests. 30 mutations -- the original 15, the reviewer's
surviving ones, and one per fix above -- each fail a named test; two first
survived because their fixtures did not reach the gap (an escape instead
of the literal character, and a path string in an attribute instead of a
loose include_str!), and the fixtures were corrected.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lamemustafa

lamemustafa commented Sep 16, 2026

Copy link
Copy Markdown
Owner Author

Closing without merging. We chose the status quo.

What this tried to fix. After #440, #441 and #442 extracted test modules into their own files, the request-builder hazard scanner reads those files as production code. That is a latent false-alarm risk only. Results are unchanged today: the same 10 pinned violations and no false alarm.

Why not merge. Every precise fix trades that latent, loud false alarm for a possible silent missed hazard in a safety gate, and master has no missed-hazard risk.

  • First review: 21 of 23 valid-Rust inputs made the first implementation skip a file that contained a hazard.
  • Hardening: pinning the exact skip set, refusing loader forms the lexer couldn't read, and fixing the lexer closed all 23.
  • Second review: eight more forms that still get through, each confirmed with rustc. They include an odd quote inside a comment in an attribute, escaped characters in an include! path, macros assembling mod $k $n, #[path] built from macro literals, Unicode whitespace Rust accepts but JavaScript's \s doesn't, and symlinks. In any of these forms, a pinned test file later loaded by production code would hide a hazard while the gate still passes.
  • Other problems: the second review also found that the pin comparison itself was untested, that the refusal list rejected ordinary rustfmt'd include!( "x.rs" ), and that local results could differ from CI because the scanner walks .claude/worktrees.

The lesson: proving "only test code loads this file" by lexing Rust can't be made complete, and in a safety scanner every gap in that proof is a silent miss. A future false alarm from an extracted test file will be loud, and we can handle it when it appears.

Kept for reference: the design critique, both reviews' findings, and the attack harnesses (23 and 35 cases, with rustc confirmation) stay available if this is revisited. The branch is retained.

check-unbounded-reads.mjs still treats *_tests?.rs as test code by name. It is unchanged here.

lamemustafa added a commit that referenced this pull request Sep 16, 2026
@lamemustafa @claude
Move the compatibility tool's inline tests out of its pinned lib.rs
4d84519 
docs/proposed-rust-module-conventions.md item 2, continuing #440-#442, for
the last large inline test block among pinned files: the `tests` module of
tools/bridge-tally-compatibility/src/lib.rs -- the tool that validates the
compatibility surface -- moves to lib_tests.rs beside it via #[path].
lib.rs goes from 2,853 to 1,686 lines. No production code followed the
block.

Checked, not asserted:
- lib.rs rebuilds byte-identical to HEAD from the new parent plus the
  re-indented child.
- rustfmt's only non-whitespace change is one trailing comma; all 169
  string and char literals are identical.
- Test lists are identical before and after: 24 lib tests by default, 30
  with bills-native-outstandings-probe-receipt. The full crate passes under
  both, with no warnings.
- Before resealing, real_tree_has_complete_migration_and_report_surface_coverage
  failed with surface_file_changed -- the expected result of changing a
  pinned file -- and passes after; reseal --verify exits 0 and exactly one
  pinned hash changed.
- clippy -D warnings is clean for CI's tools-workspace step and its
  probe-receipt feature step.
- Text readers: check-tally-live-read-boundary.mjs allowlists this lib.rs
  for probe identifiers; the moved tests contain none, and it passes. The
  request-builder hazard scanner now reads lib_tests.rs as production (see
  #443) and still reports exactly its 10 pinned violations. The other
  scanning scripts pass, and the frontend suite passes (239 node, 113
  vitest, 10 Playwright) on a clean tree -- the merge-driver test refuses
  to run with uncommitted surface changes.

Coverage, stated rather than assumed. lib_tests.rs is unpinned, following
#416's rule and #440-#442: tests decide nothing about what Bridge posts
or emits. For this file the loss is sharper than before, because
real_tree_has_complete_migration_and_report_surface_coverage holds the only
executable check of the reserve bound, `MAX_SURFACE_FILES -
files.len() <= RESERVED_SURFACE_FILES`. The cap itself stays enforced in
the pinned lib.rs. Editing or deleting that assertion no longer changes a
pinned hash.

The one #[cfg(windows)] test, normalise_surface_path_uses_forward_slashes,
could not run on the macOS host that did this move; CI's windows-latest
native job runs the tools workspace tests and covers it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 16, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@lamemustafa