chore(#3502): widen no-source-grep, close four measured blind spots (#3505)

Phase 3 of #3464, following #3465 and #3466. Those phases cut the exemption
ceiling 305 -> 278 by removing vestigial markers and rewriting real
source-greps behaviorally. This one addresses why the ratchet was weak in the
first place: it counts markers, and markers correlated only loosely with
violations, because the rule's implementation was far narrower than its intent.

Four gaps, each measured against 833 files under tests/ before any code was
written:

  A  TEXT_METHODS omitted matchAll, split and replace, and never handled the
     regex-side form re.test(tracked) / /lit/.test(tracked) where the tracked
     value is the ARGUMENT rather than the callee object.
  B  The extension test was /\.(?:cjs|js|ts)/, which does not match .cts,
     .mts or .mjs. Under ADR-457 this repo's production modules live in
     src/**/*.cts, so the rule has been structurally blind to the entire
     TypeScript source surface since that migration. Highest-value fix here.
  C  Tracking stopped at one hop, so an intermediate transform
     (const b = strip(a); b.match(...)) escaped.
  D  Variables were tracked by NAME in a flat Set<string>, with no scope
     resolution, so a name reused across describe/test blocks was conflated.

D removes one verified false positive where an outer `const src =
readFileSync(...)` was cross-attributed to a shadowed arrow-function parameter
of the same name.

Deliberately NOT implemented: flagging reads whose path cannot be statically
resolved. Measured at 4255 sites across 255 files, 66 of them newly red, with a
6-of-6 false-positive rate in spot-checking -- every sampled site read a
markdown workflow or fixture doc through a path variable, not JS source. The
heuristic "no JS extension literal present" inverts to "not a source file" in
this codebase. Shipping it would have manufactured exactly the marker-spam
dynamic this epic exists to stop. The principled version needs real static
resolution (constant-folding path.join and template literals) and is left to a
future phase.

The widening surfaced two genuinely-invisible violations, handled on their
merits rather than uniformly:

  tests/verifier-behavior-unverified.test.cjs read src/verification.cts and
  regexed it for the VERIFIER_STATUSES array. Fixed BEHAVIORALLY with no
  marker: that constant is already exported, so the test now asserts the real
  runtime value -- strictly stronger, and immune to source formatting.
  Mutation-checked: injecting present_behavior_unverified into the exported
  array turns it red, restoring turns it green.

  tests/adr-index-gate.test.cjs scans src/plan-drift-guard.cts for
  docs/adr/<name>.md citations and asserts each cited ADR exists. Those
  citations live in COMMENTS, erased at compile time: no exported value, no
  runtime observable, and making it behavioral would mean contorting production
  code into exporting its own documentation citations. Irreducible, so it takes
  one marker, cited to #3502, stating exactly why. Documenting a real exemption
  beats leaving the violation invisible, which was the status quo.

Two defects in this branch's own work, both found by review and fixed here
rather than shipped:

  FALSE POSITIVE (adversarial review). Hop propagation walked every Identifier
  in a declarator init and treated any reference to a tracked variable as
  derivation, regardless of whether the derived VALUE still carried source
  text. So `const len = raw.length; /^\d+$/.test(len)` was reported as a
  source-grep. Propagation is now value-shape aware: it follows identity,
  string-returning string methods, split/join, template embedding, string
  concatenation, conditional branches and call arguments; it stops at .length,
  numeric methods (indexOf/search/charCodeAt), boolean methods
  (includes/startsWith/test), comparisons, negation, typeof, and
  Number/parseInt/Boolean coercions. Unrecognized shapes still propagate --
  the conservative default for a linter is a rarer false positive over a silent
  false negative, and that choice is documented inline. Seven RuleTester rows
  now cover this axis, which was previously untested.

  SUPER-QUADRATIC SCAN (security review). resolveVariable() resolved each
  identifier with two linear Array.find passes over scope.references and
  scope.variables, once per identifier walked -- O(vars-in-scope) per lookup.
  On a synthetic single-scope file of N consts each referencing ~20 priors:
  4.76s at N=3000 and 24.32s at N=6000 (~5.1x for 2x N). Replaced with a
  Map<IdentifierNode, Variable> built once per file, lazily, from the scope
  manager: 0.19s and 0.37s for the same inputs (~1.95x for 2x N, linear).
  Semantics unchanged. No measurable effect on the real repo either way, but
  this is exactly the bug class ADR-3212 / local/no-unbounded-quantifier
  exists to catch, and this repo has a prior incident where a rule written to
  catch complexity bugs shipped with one of its own.

The per-widening measurement had a flaw worth recording: each widening was
measured in isolation, so a site needing TWO at once appeared in neither
column, and the UNION column was dominated by the rejected dynamic-path noise
and never inspected for interactions. adr-index-gate needs both B (.cts) and A
(.matchAll) and was missed for exactly that reason. Real newly-red count was 2,
not the 1 predicted. Corrected on #3502 rather than quietly amended.

Marker-bearing files 277 -> 278 against an unchanged ceiling of 278. The
ceiling is NOT raised: the sole addition is the cited irreducible exemption.

27 RuleTester rows cover the change. The valid rows carry as much weight as the
invalid ones -- shadowed same-name bindings, sibling block scopes, .md and
.json literal reads, dynamic path variables, non-textual derivations, reads
never text-searched, and require() of a .cjs must all stay valid. A widening
that flagged those would be worse than the status quo, because it would push
contributors toward adding markers to silence noise.

Known limit, documented rather than papered over: a tracked value round-tripped
through an array or object literal and read back via destructuring is still not
tracked. Pre-existing, not introduced here, and deliberately not widened for --
closing it means tracking member identity, with its own false-positive surface.

Closes #3502

Co-authored-by: sim <sim@local>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-08-14 17:19:24 -04:00
committed by GitHub
parent e2f4c16d9e
commit e57e3b5d6e
4 changed files with 961 additions and 40 deletions

View File

@@ -683,6 +683,11 @@ const ADR_DIR = path.join(REPO_ROOT, 'docs', 'adr');
// the real gate treats fenced (and inline) code as code — markdown does not
// render a link there, so masking it out is correct, not a regression.
// allow-test-rule: source-text-is-the-product — the ADR citation is a comment in src/plan-drift-guard.cts, erased at compile time, so no runtime observation can reach it (#3502)
// This site surfaced only after no-source-grep was widened to recognize .cts
// reads and .matchAll() (#3502); it is irreducible, not unconverted — there is
// no exported value to require() in its place, since a comment leaves no
// runtime trace to assert against.
test('the ADR path cited by src/plan-drift-guard.cts exists', () => {
// This module is compiled into the published payload, so a wrong citation
// here ships to users.

View File

@@ -138,6 +138,557 @@ describe('no-source-grep rule', () => {
});
});
// ─── no-source-grep widening (#3502 / Phase 3 of #3464) ─────────────────────
//
// One RuleTester case per row of .gsd/phase/chore-3464-widen-source-grep/
// 50-test-matrix.md. Row numbers in test names refer to that matrix.
describe('no-source-grep rule — widening (#3502)', () => {
test('row 1: baseline literal .cjs read + .includes() (happy regression)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
src.includes('x');
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
test('row 2: .cts source read + .match() (gap B)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
const ROOT = '/repo';
const src = fs.readFileSync(path.join(ROOT, 'src', 'verification.cts'), 'utf-8');
src.match(/x/);
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
test('row 3: .mts source read + .match() (gap B)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
const ROOT = '/repo';
const src = fs.readFileSync(path.join(ROOT, 'src', 'x.mts'), 'utf-8');
src.match(/x/);
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
test('row 4: .mjs source read + .match() (gap B)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
const ROOT = '/repo';
const src = fs.readFileSync(path.join(ROOT, 'src', 'x.mjs'), 'utf-8');
src.match(/x/);
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
test('row 5: .matchAll() on a tracked read (gap A)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
src.matchAll(/x/g);
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
test('row 6: regex.test(tracked) (gap A, argument-side detection)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
const re = /x/;
re.test(src);
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
test('row 7: /lit/.test(tracked) (gap A, argument-side detection)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
/x/.test(src);
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
test('row 8: .split() / .replace() probes (gap A)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
src.split('\\n');
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
{
code: `
const fs = require('fs');
const path = require('path');
const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
src.replace(/x/, '');
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
test('row 9: two-hop derived variable (gap C)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
function strip(x) { return x; }
const a = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
const b = strip(a);
b.match(/x/);
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
test('row 10: three-hop derived variable — at the depth bound (gap C, boundary)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
const a = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
const b = a;
const c = b;
c.includes('x');
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
test('row 11: hop chain beyond the configured depth is a documented limit (gap C, boundary)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [
{
code: `
const fs = require('fs');
const path = require('path');
const a = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
const b = a;
const c = b;
const d = c;
d.includes('x');
`,
filename: 'tests/foo.test.cjs',
},
],
invalid: [],
});
});
test('row 12: shadowed same-name param — false-positive guard (gap D)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [
{
code: `
const fs = require('fs');
const path = require('path');
const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
const fn = (src) => src.replace(/x/, 'y');
fn('unrelated');
`,
filename: 'tests/foo.test.cjs',
},
],
invalid: [],
});
});
test('row 13: same name, sibling block scopes — false-positive guard (gap D)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [
{
code: `
const fs = require('fs');
const path = require('path');
{
const c = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
}
{
const c = 'x';
c.includes('y');
}
`,
filename: 'tests/foo.test.cjs',
},
],
invalid: [],
});
});
test('row 14: .md literal read + .includes() (negative space, unchanged)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [
{
code: `
const fs = require('fs');
const path = require('path');
const content = fs.readFileSync(path.join(__dirname, '..', 'workflows', 'a.md'), 'utf-8');
content.includes('x');
`,
filename: 'tests/foo.test.cjs',
},
],
invalid: [],
});
});
test('row 15: .json literal read + .match() (negative space, unchanged)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [
{
code: `
const fs = require('fs');
const path = require('path');
const content = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.json'), 'utf-8');
content.match(/x/);
`,
filename: 'tests/foo.test.cjs',
},
],
invalid: [],
});
});
test('row 16: dynamic path variable → .includes() — deliberately not flagged (rejected widening)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [
{
code: `
const fs = require('fs');
function readIt(p) {
const content = fs.readFileSync(p, 'utf-8');
content.includes('x');
}
`,
filename: 'tests/foo.test.cjs',
},
],
invalid: [],
});
});
test('row 17: tracked read, no text search (negative space, unchanged)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [
{
code: `
const fs = require('fs');
const path = require('path');
const data = JSON.parse(fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8'));
`,
filename: 'tests/foo.test.cjs',
},
],
invalid: [],
});
});
test('row 18: require() of a .cjs (negative space, unchanged)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [
{
code: `
const mod = require('../lib/a.cjs');
mod.someMethod();
`,
filename: 'tests/foo.test.cjs',
},
],
invalid: [],
});
});
test('row 19: file carrying the file-level suppression annotation suppresses an otherwise-invalid fixture', () => {
// The raw marker text is assembled via string concatenation so this
// FILE's own bytes never contain a contiguous "allow" + "-test-rule:"
// token (scripts/lint-allow-test-rule-refs.cjs does a raw whole-file
// substring scan). At RuleTester-run time the concatenation resolves to
// a real single-line comment, which the rule under test honors normally.
const marker = '// ' + 'allow' + '-test-rule: split marker for row 19, see #3502';
const code = [
"const fs = require('fs');",
"const path = require('path');",
marker,
"const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');",
"src.includes('x');",
].join('\n');
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [
{
code,
filename: 'tests/foo.test.cjs',
},
],
invalid: [],
});
});
test('row 20: marker text inside a string literal (not a comment) does not suppress', () => {
// Same split-marker technique as row 19, applied to a STRING literal
// (not a comment) — this row exists to prove the rule's suppression
// check only honors an actual comment, per the #3465 discriminator.
const stringMarkerLine = "const note = '" + 'allow' + "-test-rule: this is just data, not a directive';";
const code = [
"const fs = require('fs');",
"const path = require('path');",
stringMarkerLine,
"const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');",
'src.includes(note);',
].join('\n');
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
});
// ─── no-source-grep hop-propagation value-shape (adversarial-review fix) ────
//
// minTrackedHop() used to walk EVERY Identifier under a derivation's RHS
// and treat any bare reference to a tracked variable as propagating,
// regardless of whether the derived VALUE could still carry text (e.g.
// `.length`). These rows cover the value-shape gate that replaced that
// blind walk: propagate only through derivations that plausibly still
// carry the source file's text; do not propagate through scalar-producing
// shapes (member access, numeric/boolean methods, comparisons, Number()
// et al).
describe('no-source-grep rule — hop-propagation value-shape (adversarial-review fix)', () => {
test('valid: .length derivation does not propagate (reported false-positive repro)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [
{
code: `
const fs = require('fs');
const path = require('path');
const raw = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
const len = raw.length;
if (/^\\d+$/.test(len)) {}
`,
filename: 'tests/foo.test.cjs',
},
],
invalid: [],
});
});
test('invalid: numeric-returning method derivation does not cascade to a second error', () => {
// raw.indexOf('x') is itself already flagged directly (indexOf is one
// of the TEXT_METHODS this rule flags on a tracked receiver, unrelated
// to hop propagation). The important assertion here is that there is
// exactly ONE error, not two: the numeric result of .indexOf() must
// NOT stay tracked, so String(n).includes('1') is not a second finding.
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
const raw = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
const n = raw.indexOf('x');
String(n).includes('1');
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
test('invalid: boolean-returning method derivation does not cascade to a second error', () => {
// Same shape as above with a boolean-returning method: raw.includes('x')
// is itself already flagged directly. The boolean result must NOT stay
// tracked, so String(ok).includes('true') is not a second finding.
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
const raw = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
const ok = raw.includes('x');
String(ok).includes('true');
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
test('valid: comparison of a tracked derivation does not propagate', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [
{
code: `
const fs = require('fs');
const path = require('path');
const raw = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
const same = raw.length === 0;
`,
filename: 'tests/foo.test.cjs',
},
],
invalid: [],
});
});
test('invalid: string-returning method derivation still propagates and is caught', () => {
// raw.replace(...) is flagged directly (replace is a TEXT_METHOD, same
// as the indexOf/includes rows above) AND the string-returning result
// (b) correctly stays tracked, so b.includes('y') is a second, distinct
// finding. Two errors total, both real.
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
const raw = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
const b = raw.replace(/x/, '');
b.includes('y');
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }, { messageId: 'noSourceGrep' }],
},
],
});
});
test('invalid: template-literal derivation still propagates and is caught', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
const raw = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
const b = \`\${raw}\`;
b.match(/y/);
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
test('invalid: direct .includes() on the tracked source read is unchanged (no regression)', () => {
ruleTester.run('no-source-grep', noSourceGrep, {
valid: [],
invalid: [
{
code: `
const fs = require('fs');
const path = require('path');
const raw = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8');
raw.includes('x');
`,
filename: 'tests/foo.test.cjs',
errors: [{ messageId: 'noSourceGrep' }],
},
],
});
});
});
// ─── no-magic-sleep-in-tests ─────────────────────────────────────────────────
describe('no-magic-sleep-in-tests rule', () => {

View File

@@ -81,13 +81,18 @@ test('PARITY: per-truth state never leaks into the overall-status vocabulary', (
});
test('overall-status enum in verification.cts is unchanged (no per-truth leak)', () => {
const cts = fs.readFileSync(path.join(ROOT, 'src', 'verification.cts'), 'utf-8');
// eslint-disable-next-line local/no-unbounded-quantifier -- parses this repo's own bounded src/verification.cts source, not adversarial input
const m = cts.match(/VERIFIER_STATUSES[^=]*=\s*\[([^\]]*)\]/);
assert.ok(m, 'VERIFIER_STATUSES array must be present');
assert.doesNotMatch(m[1], /present_behavior_unverified/i);
// Assert on the real exported runtime value rather than regexing the
// source text — strictly stronger (exercises the built module) and
// immune to source formatting changes.
const verificationLib = require(path.join(ROOT, 'gsd-core', 'bin', 'lib', 'verification.cjs'));
const { VERIFIER_STATUSES } = verificationLib;
assert.ok(Array.isArray(VERIFIER_STATUSES), 'VERIFIER_STATUSES array must be present');
assert.ok(
!VERIFIER_STATUSES.includes('present_behavior_unverified'),
'VERIFIER_STATUSES must not leak the per-truth present_behavior_unverified state',
);
for (const s of ['passed', 'gaps_found', 'human_needed']) {
assert.match(m[1], new RegExp(`'${s}'`));
assert.ok(VERIFIER_STATUSES.includes(s), `VERIFIER_STATUSES must contain ${s}`);
}
});