From 8e5b15ed5d00f64e36e09e6e0ff03cec9036aca9 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 15 Aug 2026 14:00:33 -0400 Subject: [PATCH] docs(#3523): correct allow-test-rule placement guidance for site-scoping (#3549) Phase 4 of #3464 (#3508) changed allow-test-rule suppression from file-wide to site-scoped, bounded by an 8-line comment-pure lookahead. No phase of that epic updated the contributor docs, so CONTRIBUTING.md still told contributors to annotate 'before the file's opening block comment' -- a placement that, post-#3508, suppresses nothing below the require block. The documented remedy did not work. Found by running /adr-phase-coverage over the epic: the checker reported this as the epic's one orphan-decision, owned by no phase. Corrects the placement rule and its example, states that a marker binds to either half of the read+search pair, warns that copying the old file-header placement yields an inert marker, and documents what the gate's two numbers mean -- in particular that an 'unverified' marker is not evidence the marker is vestigial. Refs #3523 Co-authored-by: sim --- CONTRIBUTING.md | 48 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 9 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1e62d1287..93408a848 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -795,19 +795,49 @@ Some tests legitimately read source files. There are six recognized categories: | `structural-implementation-guard` | A feature's interception or wiring point is not reachable end-to-end via `runGsdTools`. Used temporarily until a behavioral path exists. | | `pending-migration-to-typed-ir` | **Tracked for correction, not exempted.** Test was identified by the lint as carrying a raw-text-matching pattern that contradicts the rule above. Each annotated file MUST cite the open migration issue (e.g. `// allow-test-rule: pending-migration-to-typed-ir [#NNNN]`) so the tracking is auditable. New tests cannot use this category — they must refactor production to expose typed IR. The annotation is removed when the test is corrected. | -Annotate with a standalone `//` comment before the file's opening block comment: +**Suppression is site-scoped, not file-wide.** A marker suppresses only the violation it sits next +to. Put it immediately above the flagged line — or trailing on that line — with nothing but blank +lines and other comment lines in between, and no more than **8 lines** above it +(`MAX_MARKER_LOOKAHEAD_LINES` in `eslint-rules/no-source-grep.cjs`). A single line of real code +between the marker and the call ends the window, even when the two are physically close. ```javascript -// allow-test-rule: architectural-invariant -// state.cjs locking must use Atomics.wait(), not a spin-loop. Behavioral tests -// cannot observe which sleep primitive was chosen — only source inspection can. - -/** - * Regression tests for locking bugs #1909... - */ +test('locking uses Atomics.wait, not a spin-loop', () => { + // allow-test-rule: architectural-invariant (#1909) + // state.cjs locking must use Atomics.wait(). Behavioral tests cannot observe + // which sleep primitive was chosen — only source inspection can. + const src = fs.readFileSync(STATE_PATH, 'utf8'); + assert.ok(src.includes('Atomics.wait')); +}); ``` -The annotation **must** be a standalone `// allow-test-rule:` line, not inside a `/** */` block comment — the CI linter scans for the pattern `// allow-test-rule:`. +A violation is a **read + search pair**, and a marker adjacent to *either* half suppresses it — so +annotating the `readFileSync` directly (the intuitive placement) works just as well as annotating +the `.includes()`. + +> **A marker parked at the top of the file no longer suppresses anything below it.** Before #3508 +> suppression was file-wide, so one justified exemption silently absolved every other source-grep +> in that file — including ones added later by someone else. If you are copying the old +> file-header placement from an existing test, it is almost certainly inert: the file's `require` +> block sits between it and the code, and real code closes the window. + +The annotation **must** be a standalone `// allow-test-rule:` line — the marker text must be the +first thing on its comment line (a leading JSDoc `*` is fine), not buried mid-sentence in prose. +The reason **must** cite a tracking issue (`#NNN`) or an `https://` URL, per +[ADR-456](docs/adr/456-test-rigor-architecture.md); `scripts/lint-allow-test-rule-refs.cjs` fails +the build on an uncited one. + +That gate reports two separate numbers, and they mean different things: + +| Number | Meaning | Gated? | +|---|---|---| +| **Effective exemptions** | markers that actually suppress a violation the rule detects | tightly ratcheted — it may only go down | +| **Unverified markers** | marker-bearing files where the rule detects nothing to suppress | tracked with a loose ceiling; growth fails, shrinkage never does | + +A marker landing in the *unverified* bucket does **not** mean it is vestigial and safe to delete — +it usually means the rule cannot yet see the read (identifier indirection, a dynamic path, a `.sh` +file). Shrinking that pool is a rule-coverage job backed by measurement, not a delete-the-markers +job. ### Prohibited: Raw Text Matching on Test Outputs (file content, stdout, stderr)