diff --git a/eslint-rules/no-source-grep.cjs b/eslint-rules/no-source-grep.cjs index 6f383f204..825425d83 100644 --- a/eslint-rules/no-source-grep.cjs +++ b/eslint-rules/no-source-grep.cjs @@ -45,6 +45,95 @@ const MAX_TRANSITIVE_HOPS = 3; // site 40+ lines later (test-matrix.md row 4, the defect this closes). const MAX_MARKER_LOOKAHEAD_LINES = 8; +// Matches a `allow-test-rule: ` directive inside a comment's VALUE +// (the AST comment node's text with delimiters stripped -- i.e. this is +// tested against `c.value`, never against raw source text). Exported so +// external consumers (scripts/lint-allow-test-rule-refs.cjs) that need to +// find marker comments via the SAME AST-comment definition the rule itself +// honors can import this instead of hand-rolling an equivalent pattern that +// could silently drift from what the rule actually recognizes. +const MARKER_COMMENT_RE = /allow-test-rule:\s*\S/; + +/** + * Given every comment in a file (`sourceCode.getAllComments()`), compute the + * two line-level facts `isSuppressedAt` needs: which lines carry a + * `allow-test-rule:` marker, and which lines are wholly comment (used for the + * "nothing but blank/comment lines between the marker and the violation" + * purity check). Extracted verbatim from the per-file computation `create()` + * used to do inline, so a second consumer (the effective-exemption counter) + * can derive the identical inputs without re-deriving the marker-detection + * logic itself. + * + * @param {{value: string, loc: {start: {line:number}, end: {line:number}}}[]} allComments + * @returns {{markerLines: number[], commentLineSet: Set}} + */ +function collectMarkerAndCommentLines(allComments) { + const markerLines = []; + const commentLineSet = new Set(); + for (const c of allComments) { + for (let l = c.loc.start.line; l <= c.loc.end.line; l++) { + commentLineSet.add(l); + } + if (MARKER_COMMENT_RE.test(c.value)) { + for (let l = c.loc.start.line; l <= c.loc.end.line; l++) { + markerLines.push(l); + } + } + } + return { markerLines, commentLineSet }; +} + +/** + * Site-scoped suppression predicate: is `violationLine` suppressed by any of + * `markerLines`? Extracted verbatim from the logic `create()` used to close + * over directly (previously named `isSuppressed`), generalized to take its + * per-file inputs as parameters instead of reading them off closure state, so + * a second consumer (scripts/lint-allow-test-rule-refs.cjs, the + * effective-exemption counter) can call the EXACT SAME adjacency arithmetic + * the rule uses at report time -- rather than reimplementing it, which is the + * generative-fix-divergence defect class this repo has shipped before. + * + * A violation at `violationLine` is suppressed if some marker sits on that + * exact line (trailing form) or on an earlier line within `maxLookahead`, + * with every line strictly between the marker and the violation being blank + * and/or itself a comment line -- i.e. no live code sits between the marker + * and the call it suppresses. This is what makes suppression SITE-scoped + * rather than file-wide. + * + * @param {object} opts + * @param {number[]} opts.markerLines - line numbers carrying a marker. + * @param {number} opts.violationLine - the candidate violation's line. + * @param {Set} opts.commentLineSet - lines wholly occupied by a comment. + * @param {string[]} opts.lines - the file's source lines (sourceCode.lines). + * @param {number} [opts.maxLookahead] - defaults to MAX_MARKER_LOOKAHEAD_LINES. + * @returns {boolean} + */ +function isSuppressedAt({ + markerLines, + violationLine, + commentLineSet, + lines, + maxLookahead = MAX_MARKER_LOOKAHEAD_LINES, +}) { + function isBlankLine(line) { + const text = lines[line - 1]; + return text !== undefined && text.trim() === ''; + } + for (const markerLine of markerLines) { + if (markerLine > violationLine) continue; + if (violationLine - markerLine > maxLookahead) continue; + let pure = true; + for (let l = markerLine + 1; l < violationLine; l++) { + if (!isBlankLine(l) && !commentLineSet.has(l)) { + pure = false; + break; + } + } + if (pure) return true; + } + return false; +} + const TEXT_METHODS = new Set([ 'includes', 'match', @@ -120,10 +209,32 @@ const rule = { 'Disallow reading source .cjs/.cts/.js/.mjs/.mts/.ts files with readFileSync and then doing text search on the result', category: 'Best Practices', }, - schema: [], + // `neutralizeSuppression` is a diagnostic-only knob for + // scripts/lint-allow-test-rule-refs.cjs (the effective-exemption + // counter): when true, every candidate violation is reported regardless + // of a marker, so the script can enumerate the FULL site inventory via + // one real ESLint pass, then classify each site with isSuppressedAt + // (exported below) against the real markers. No config in this repo + // passes this option, so default (real) linting is unaffected -- this is + // an extract-and-export refactor of existing logic, not a behavior + // change to `no-source-grep` itself. + schema: [ + { + type: 'object', + properties: { + neutralizeSuppression: { type: 'boolean' }, + }, + additionalProperties: false, + }, + ], messages: { noSourceGrep: 'Source-grep test: do not read source .cjs/.cts/.js/.mjs/.mts/.ts files with readFileSync and call .includes/.match/.matchAll/.startsWith/.indexOf/.split/.replace/.search (or regex.test()) on the result. Use require() to run the module instead. Add // allow-test-rule: (#NNN) directly above (or trailing) the flagged line to suppress just that site.', + // Diagnostic-only companion to `noSourceGrep`, emitted ONLY when the + // `neutralizeSuppression` schema option is set (see its doc comment + // and `reportUnlessSuppressed` above) -- never fires with the real + // (shipped) config, so this never appears in real lint output. + noSourceGrepDiagnosticReadLine: '{{readLine}}', }, }, create(context) { @@ -136,34 +247,18 @@ const rule = { const allComments = sourceCode.getAllComments(); // Line numbers of every `// allow-test-rule: ` marker comment in - // the file. A marker may span one line (the normal `//` form) or several - // (a block comment) -- record every line it occupies so a violation on - // any of those lines counts as "same line" (trailing-marker form, row 2 - // of the test matrix). - const markerLines = []; - for (const c of allComments) { - if (/allow-test-rule:\s*\S/.test(c.value)) { - for (let l = c.loc.start.line; l <= c.loc.end.line; l++) { - markerLines.push(l); - } - } - } + // the file (a marker may span one line or several -- see + // collectMarkerAndCommentLines' doc comment above), and the set of lines + // fully occupied by ANY comment (used by isSuppressedAt's purity check). + const { markerLines, commentLineSet } = collectMarkerAndCommentLines(allComments); - // Line numbers fully occupied by ANY comment (marker or not) -- a marker - // followed by ordinary prose lines before the flagged call is the repo's - // real style (test-matrix.md row 3), so those in-between lines must not - // disqualify the marker. - const commentLineSet = new Set(); - for (const c of allComments) { - for (let l = c.loc.start.line; l <= c.loc.end.line; l++) { - commentLineSet.add(l); - } - } - - function isBlankLine(line) { - const text = sourceCode.lines[line - 1]; - return text !== undefined && text.trim() === ''; - } + // Diagnostic-only: see the `neutralizeSuppression` schema option doc + // comment above. Never true for any config in this repo. + const neutralizeSuppression = !!( + context.options && + context.options[0] && + context.options[0].neutralizeSuppression + ); // A violation at `violationLine` is suppressed if some marker sits on // that exact line (trailing form) or on an earlier line within @@ -173,21 +268,17 @@ const rule = { // ostensibly about) sits between the marker and the call it suppresses. // This is what makes suppression SITE-scoped rather than file-wide: a // marker parked far above an unrelated later violation (test-matrix.md - // row 4) no longer reaches it. + // row 4) no longer reaches it. Delegates to the exported isSuppressedAt + // predicate (see its doc comment) rather than duplicating the adjacency + // arithmetic here. function isSuppressed(violationLine) { - for (const markerLine of markerLines) { - if (markerLine > violationLine) continue; - if (violationLine - markerLine > MAX_MARKER_LOOKAHEAD_LINES) continue; - let pure = true; - for (let l = markerLine + 1; l < violationLine; l++) { - if (!isBlankLine(l) && !commentLineSet.has(l)) { - pure = false; - break; - } - } - if (pure) return true; - } - return false; + if (neutralizeSuppression) return false; + return isSuppressedAt({ + markerLines, + violationLine, + commentLineSet, + lines: sourceCode.lines, + }); } // Map from Identifier AST node -> resolved ESLint `Variable`, built once @@ -548,6 +639,25 @@ const rule = { // site-scoped, way to annotate it). `readLine` is optional -- pass // it whenever the call site can determine one. function reportUnlessSuppressed(node, readLine) { + if (neutralizeSuppression) { + // Diagnostic-only mode (see the `neutralizeSuppression` schema + // option doc comment): report every candidate site regardless of + // suppression, PLUS a paired companion message at the exact same + // node carrying `readLine` -- the other half of the read+search + // pair that the real (non-neutralized) suppression check above + // also consults. This lets a consumer (the effective-exemption + // counter) replicate this rule's own OR-of-two-lines suppression + // decision from the OUTSIDE via `isSuppressedAt` without this + // rule re-exposing its internal hop/scope-resolution machinery. + // Never fires with the real (shipped) config. + context.report({ node, messageId: 'noSourceGrep' }); + context.report({ + node, + messageId: 'noSourceGrepDiagnosticReadLine', + data: { readLine: readLine === undefined || readLine === null ? '' : String(readLine) }, + }); + return; + } if (isSuppressed(node.loc.start.line)) return; if (readLine !== undefined && readLine !== null && isSuppressed(readLine)) { return; @@ -596,3 +706,14 @@ const rule = { }; module.exports = rule; +// Named exports consumed by scripts/lint-allow-test-rule-refs.cjs (the +// effective-exemption counter) and its tests -- ESLint itself only reads +// `.create`/`.meta` off this module, so these extra properties are inert to +// ESLint and exist purely as the single source of truth for anything that +// needs to reason about marker suppression outside the rule's own +// Program:exit walk. See each function's doc comment above for why this +// extraction exists (generative-fix-divergence prevention). +module.exports.MAX_MARKER_LOOKAHEAD_LINES = MAX_MARKER_LOOKAHEAD_LINES; +module.exports.MARKER_COMMENT_RE = MARKER_COMMENT_RE; +module.exports.collectMarkerAndCommentLines = collectMarkerAndCommentLines; +module.exports.isSuppressedAt = isSuppressedAt; diff --git a/package.json b/package.json index 2cb3b2b0c..cb568dfac 100644 --- a/package.json +++ b/package.json @@ -28,6 +28,7 @@ "!scripts/affected-tests-lib.cjs", "!scripts/run-affected-tests.cjs", "!scripts/lint-no-adhoc-regex-escape.cjs", + "!scripts/lint-allow-test-rule-refs.cjs", "pi", "vscode" ], diff --git a/scripts/lint-allow-test-rule-refs.allowlist.json b/scripts/lint-allow-test-rule-refs.allowlist.json index 9f0bbd3ec..10163f743 100644 --- a/scripts/lint-allow-test-rule-refs.allowlist.json +++ b/scripts/lint-allow-test-rule-refs.allowlist.json @@ -43,9 +43,6 @@ "tests/edge-probe-planner-contract.test.cjs :: source-text-is-the-product", "tests/edge-probe-spec-phase-contract.test.cjs :: source-text-is-the-product", "tests/edit-phase.test.cjs :: source-text-is-the-product", - "tests/eslint-rules.test.cjs :: must still error", - "tests/eslint-rules.test.cjs :: pending migration", - "tests/eslint-rules.test.cjs :: source-text-is-the-product", "tests/execute-phase-active-flags.test.cjs :: source-text-is-the-product", "tests/execute-phase-step-5-5-deviation-doc.test.cjs :: source-text-is-the-product", "tests/execute-phase-wave.test.cjs :: source-text-is-the-product", @@ -60,6 +57,7 @@ "tests/gsd-researcher-app-aware.test.cjs :: source-text-is-the-product", "tests/gsd-researcher-flow-diagram.test.cjs :: source-text-is-the-product", "tests/gsd-settings-advanced.test.cjs :: source-text-is-the-product", + "tests/helpers/live-command-registry.cjs :: source-text-is-the-product", "tests/hermes-skills-migration.test.cjs :: source-text-is-the-product", "tests/import-command.test.cjs :: source-text-is-the-product", "tests/ingest-docs.test.cjs :: source-text-is-the-product", diff --git a/scripts/lint-allow-test-rule-refs.ceiling.json b/scripts/lint-allow-test-rule-refs.ceiling.json deleted file mode 100644 index 8b97e48ff..000000000 --- a/scripts/lint-allow-test-rule-refs.ceiling.json +++ /dev/null @@ -1,4 +0,0 @@ -{ - "maxFiles": 278, - "grace": 3 -} diff --git a/scripts/lint-allow-test-rule-refs.cjs b/scripts/lint-allow-test-rule-refs.cjs index b1a1fb0f7..818fd88c7 100644 --- a/scripts/lint-allow-test-rule-refs.cjs +++ b/scripts/lint-allow-test-rule-refs.cjs @@ -3,23 +3,16 @@ /** * lint-allow-test-rule-refs.cjs — enforce that NEW `allow-test-rule:` exemption - * comments carry a tracking-issue reference, AND that the total exemption - * count only ever ratchets down. + * comments carry a tracking-issue reference, AND that the number of exemptions + * that ACTUALLY suppress a detected violation only ever ratchets down. * * ## Why * * `allow-test-rule:` is an inline comment that disables the `no-source-grep` - * ESLint rule for a whole test file. Today many such comments exist with no - * issue reference, making it impossible to audit or revisit them. Per ADR-456 + * ESLint rule at a specific site. Per ADR-456 * (docs/adr/456-test-rigor-architecture.md) every NEW exemption must carry a * `#NNN` issue reference or an https:// URL so the decision is traceable. * - * A citation alone doesn't stop the raw count from growing forever — a cited - * exemption is legitimate under ADR-456 §(d), but nothing previously stopped - * the total (cited + uncited) from creeping up PR by PR. This is the same - * masking gap the citation ratchet fixes for identity, applied to the count: - * a cited exemption looks compliant while the aggregate debt keeps growing. - * * ## What "compliant" means (citation check) * * A compliant `allow-test-rule:` comment is one whose reason text (everything @@ -48,75 +41,365 @@ * If a file has multiple non-compliant comments with the SAME reason text, only * one identifier is recorded (deduped via Set). * - * ## Total-count ceiling + * ## Effective exemptions vs. unverified markers (#3520 / epic #3464 phase 5) * - * Independently of citation status, the number of DISTINCT files carrying at - * least one `allow-test-rule:` marker is checked against a tight ceiling in - * scripts/lint-allow-test-rule-refs.ceiling.json via `assertTightCeiling` - * (scripts/lib/allowlist-ratchet.cjs). The ceiling may only decrease — a - * shrinking count that leaves too much slack above the ceiling fails the gate - * just as much as growth past it, forcing the ceiling to track the real - * high-water mark rather than sitting stale and loose. + * A marker-bearing FILE is not the same thing as a marker that suppresses + * anything. Historically this gate counted "distinct files containing marker + * text" (294 at one baseline), but only a handful of those markers actually + * sit next to a violation the `no-source-grep` rule can detect — the rest are + * "counted" only because they contain the literal substring, which made the + * old ceiling ~98% noise and free to bump. + * + * This gate now reports and ratchets TWO separate numbers instead: + * + * 1. **Effective exemption SITES** — a site (one flagged read+search pair) + * where a marker is adjacent enough (per the rule's own + * `isSuppressedAt` site-scoped adjacency predicate) to suppress a + * violation the rule actually detects there. Tightly ratcheted via + * `assertTightCeiling` (both directions — growth past ceiling fails, + * and slack beyond `grace` fails, forcing the ceiling to track the + * real high-water mark). + * + * 2. **Unverified marker-bearing FILES** — files that carry an + * `allow-test-rule:` marker but for which no detectable violation was + * found adjacent to any marker in the file (rule-coverage gap, not + * necessarily a vestigial marker — see "Known limits" below). Reported + * with its own ceiling so it cannot silently balloon, but NOT tightly + * ratcheted: dropping below the ceiling never fails the gate, because + * shrinking this pool means widening rule coverage file-by-file with + * evidence, not deleting markers on the strength of this metric alone. + * + * A file with >=1 effective site counts ONLY as effective, never also as + * unverified (test-matrix.md #3520 row 5) — no double counting. + * + * ## Scan scope: every glob that registers the rule, not just `tests/*.test.cjs` + * + * `local/no-source-grep` is registered by `eslint.config.mjs` on several + * glob blocks, not only `tests/** /*.cjs`: also `scripts/** /*.cjs`, + * `eslint-rules/** /*.cjs`, `bin/lib/** /*.cjs`, `pi/** /*.cjs`, + * `examples/** /*.cjs`, `gsd-core/bin/** /*.cjs`, `vscode/*.js`, + * `.kilo/plugins/*.js`, and `.opencode/plugins/*.js`. A prior version of + * this script only walked `tests/** /*.test.cjs`, which silently dropped + * every non-`.test.cjs` file under `tests/` AND every file under every one + * of those other blocks from BOTH reported numbers — a marker there would + * vanish rather than trip a ceiling. `deriveNoSourceGrepGlobs` (below) reads + * the authoritative glob list directly out of `eslint.config.mjs` at run + * time (dynamic `import()`, since this is a `.cjs` script and the config is + * ESM) so the scan set cannot drift from what ESLint itself actually lints. + * + * ## Single source of truth for detection + * + * This script does NOT reimplement the `no-source-grep` rule's AST walk or + * its site-scoped adjacency predicate — doing so would be the + * generative-fix-divergence defect class this repo has shipped before. + * Instead it drives the REAL rule through ESLint's `Linter` API with its + * `neutralizeSuppression` diagnostic option (see the rule's `meta.schema` + * doc comment) to enumerate every candidate violation SITE in a file + * regardless of markers, then classifies each site by calling the rule's own + * exported `isSuppressedAt` predicate against that file's real markers (also + * obtained via the rule's exported `collectMarkerAndCommentLines`) — the + * exact function the rule itself calls at report time. See + * `eslint-rules/no-source-grep.cjs`. + * + * ## Known limits (do not read "effective" as "all exemptions") + * + * The effective-sites count is only as good as `no-source-grep`'s own + * detection. Real source-text-search violations behind identifier + * indirection (a path bound to a separate `const` the rule never resolves + * back to its literal), dynamic paths, non-`.js`-family extensions (`.sh`), + * and array/object round-trips are documented, accepted blind spots of the + * rule (see its own "Known limits" in 40-design.md) and remain invisible to + * BOTH numbers here. Two such sites are known and tracked separately: + * tests/security-prompt-injection.security.test.cjs and + * tests/check-update-config-dir.test.cjs. This is stated explicitly in this + * script's own `ok` output so the effective count can never be read as "all + * exemptions are accounted for." + * + * ## Linter scope: marker-bearing files only (#3464 perf follow-up) + * + * The byte-based `walkGlobs` walk above still reads EVERY file in every + * `local/no-source-grep`-registered glob (~1200 files) — that walk is cheap + * (raw `fs.readFileSync`, no parsing) and stays repo-wide so no marker can + * hide from the inventory. But driving the real ESLint `Linter` over all + * ~1200 files to classify effective/live sites was the actual cost (~12s+ + * under CI load, enough to blow the test harness's spawn timeout). Only + * files containing the literal `allow-test-rule:` substring can possibly + * produce a genuine directive (`extractGenuineMarkerReasons` requires that + * substring to be present) or an "effective"/"unverified" classification — + * so `main` filters to marker-bearing files (a plain `String#includes` scan + * over content already in memory from the walk, deliberately NOT a shell + * `grep`: `tests/security-prompt-injection.security.test.cjs` contains a + * literal NUL byte that a shell grep pipeline handles differently than + * Node's own string/buffer handling does) BEFORE calling `classifySites`, + * narrowing the Linter pass from ~1200 files to the ~294 that actually carry + * a marker. + * + * This means the "live violations" figure this script reports now only + * covers marker-bearing files. A live (unsuppressed) violation in a file + * with NO marker at all is still caught — just not by this script: it is + * exactly what `local/no-source-grep` itself flags when `npm run lint` / + * `npm run lint:ci` runs across the same globs, and that run fails the build + * independently. This script re-checking unmarked files for live violations + * would be pure duplicate work with no distinct consequence (the marker + * inventory and the citation/ratchet checks — the reason this script exists + * — only ever concern marker-bearing files in the first place). If a live + * violation IS found in a marker-bearing file, it still fails this gate + * exactly as before this change. * * See docs/adr/456-test-rigor-architecture.md for the full policy. */ const fs = require('fs'); const path = require('path'); +const { pathToFileURL } = require('url'); +const { Linter } = require('eslint'); const { assertWithinAllowlist, assertTightCeiling } = require('./lib/allowlist-ratchet.cjs'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); +const noSourceGrepRule = require('../eslint-rules/no-source-grep.cjs'); const ROOT = path.join(__dirname, '..'); +const ESLINT_CONFIG_PATH = path.join(ROOT, 'eslint.config.mjs'); const TESTS_DIR = process.env.GSD_LINT_ALLOW_TEST_RULE_TESTS_DIR || path.join(ROOT, 'tests'); +// Overrides the ROOT that non-`tests/` glob base directories (scripts/, +// eslint-rules/, bin/lib/, pi/, examples/, gsd-core/bin/, vscode/, +// .kilo/plugins/, .opencode/plugins/ — see deriveNoSourceGrepGlobs) are +// resolved under. TESTS_DIR already has its own override (above) for the +// `tests/**/*.cjs` block; this is the equivalent knob for every OTHER block +// that registers `local/no-source-grep`, so sandbox fixtures can exercise +// the widened scan scope (#3464 phase 5 BLOCKER fix) without the test suite +// re-scanning this repo's real scripts/eslint-rules/etc. trees on every run. +const EXTRA_ROOT = process.env.GSD_LINT_ALLOW_TEST_RULE_EXTRA_ROOT || ROOT; const ALLOWLIST_PATH = process.env.GSD_LINT_ALLOW_TEST_RULE_ALLOWLIST || path.join(__dirname, 'lint-allow-test-rule-refs.allowlist.json'); -const CEILING_PATH = - process.env.GSD_LINT_ALLOW_TEST_RULE_CEILING || - path.join(__dirname, 'lint-allow-test-rule-refs.ceiling.json'); +const EFFECTIVE_CEILING_PATH = + process.env.GSD_LINT_ALLOW_TEST_RULE_EFFECTIVE_CEILING || + path.join(__dirname, 'lint-allow-test-rule-refs.effective-ceiling.json'); +const UNVERIFIED_CEILING_PATH = + process.env.GSD_LINT_ALLOW_TEST_RULE_UNVERIFIED_CEILING || + path.join(__dirname, 'lint-allow-test-rule-refs.unverified-ceiling.json'); -/** - * Extracts the reason text after `allow-test-rule:` from a single line of source - * text in any comment form that the no-source-grep ESLint rule honours. - * - * The ESLint rule tests `c.value` (AST comment node value, delimiters stripped) - * with /allow-test-rule:\s*\S/, which fires on BOTH: - * // allow-test-rule: (line comment) - * /* allow-test-rule: * / (block comment, single-line) - * - * By scanning line-by-line and extracting everything after `allow-test-rule:` on - * each line, we cover both forms without a cross-line regex (which was previously - * matching arbitrary `/* ... * /` pairs spanning hundreds of lines, causing false - * positives). - * - * The trailing `*\/` and whitespace are stripped so block-comment closers don't - * bleed into the extracted reason. - */ -const ALLOW_TEST_RULE_LINE_RE = /allow-test-rule:\s*(.+)/; /** Matches a compliant issue reference or URL */ const ISSUE_REF_RE = /#\d+|https?:\/\//; /** - * Recursively read every *.test.cjs file under dir, once. + * Extract the "genuine directive" reason text(s) from a file's real AST + * comments (never string/template literals, never source code — only actual + * `//` or `/* * /` comment tokens, obtained the same way the rule itself does + * via `sourceCode.getAllComments()`). * - * Both the citation check and the total-count ceiling need the same file set - * and content — walking twice would be the generative-fix-divergence class of - * bug (two scans that can silently drift apart), so both classifiers below - * consume this single walk's output. + * A prior version of this scanned raw file TEXT line-by-line for the + * substring `allow-test-rule:` anywhere on a line. That was safe while the + * scan was confined to `tests/*.test.cjs` (real test files essentially never + * discuss the marker syntax in prose), but widening the scan to + * `scripts/** /*.cjs` and `eslint-rules/** /*.cjs` (#3464 phase 5 BLOCKER fix) + * means it now also walks the RULE's own implementation and THIS script — + * files that must document `// allow-test-rule: (#NNN)` as example + * syntax in prose, JSDoc, and the rule's own message string. A raw + * substring scan cannot tell "a real directive" apart from "a sentence that + * mentions the directive" or "a string literal that quotes it" — so a + * directly-widened raw scan produced dozens of false "uncited exemption" + * failures on this repo's own documentation. * - * @param {string} dir absolute path to scan - * @returns {{relpath: string, content: string}[]} + * Fixed here by requiring a match to be BOTH (a) inside a real comment token + * (excludes string/template literals and code entirely — mirrors row 11's + * "marker text inside a string literal is not a directive" contract) AND + * (b) the FIRST non-whitespace content of that comment's line, once an + * optional JSDoc continuation-line `*` prefix is stripped (excludes prose + * that merely MENTIONS `allow-test-rule:` mid-sentence, e.g. "Matches a + * `allow-test-rule: ` directive..."). This is deliberately STRICTER + * than the rule's own `MARKER_COMMENT_RE` (which matches the substring + * anywhere in a comment's value, by design, so a marker's suppression reach + * cannot be defeated by trailing prose on the same comment) — citation + * tracking asks a narrower question ("is this meant as a real directive + * that needs a tracking ref") than suppression does, so the two are allowed + * to diverge. + * + * @param {{value: string, loc: {start: {line: number}}}[]} allComments + * @returns {string[]} raw reason text for every genuine directive line found */ -function walkTestFiles(dir) { - const files = []; +function extractGenuineMarkerReasons(allComments) { + const reasons = []; + for (const comment of allComments) { + for (const rawLine of comment.value.split('\n')) { + const trimmed = rawLine.replace(/^\s*\*\s?/, '').trim(); + if (!/^allow-test-rule:/.test(trimmed)) continue; + const reason = trimmed.slice('allow-test-rule:'.length).trim(); + if (reason) reasons.push(reason); + } + } + return reasons; +} - function scan(current) { - for (const entry of fs.readdirSync(current, { withFileTypes: true })) { - const full = path.join(current, entry.name); - if (entry.isDirectory()) { - scan(full); - } else if (entry.isFile() && entry.name.endsWith('.test.cjs')) { - const relpath = path.relative(ROOT, full).split(path.sep).join('/'); +/** + * Derive the authoritative set of glob patterns that `eslint.config.mjs` + * actually registers `local/no-source-grep` on (#3464 phase 5 BLOCKER fix). + * + * Historically this script hardcoded `tests/** /*.test.cjs` as "the" scan + * set, which silently missed every non-`.test.cjs` file under `tests/` + * (`tests/helpers/**`, `tests/qa/**`, `tests/fixtures/**`, ...) AND every one + * of the other config blocks the rule is registered on (`scripts/** /*.cjs`, + * `eslint-rules/** /*.cjs`, `bin/lib/** /*.cjs`, `pi/** /*.cjs`, + * `examples/** /*.cjs`, `gsd-core/bin/** /*.cjs`, `vscode/*.js`, + * `.kilo/plugins/*.js`, `.opencode/plugins/*.js`). A marker in any of those + * would vanish from BOTH reported numbers instead of tripping anything. + * + * Rather than hand-maintain a second copy of that glob list (the exact + * generative-fix-divergence class this script's own doc comment above + * already warns about for the rule's AST walk), this dynamically imports + * the REAL flat config and reads which config objects set + * `rules['local/no-source-grep']` to something other than `'off'`/`0`, + * collecting their `files` globs. If `eslint.config.mjs` ever adds, removes, + * or narrows a block that registers this rule, this list moves with it + * automatically — there is no second list to fall out of sync. + * + * @returns {Promise} sorted, deduped glob patterns + */ +async function deriveNoSourceGrepGlobs() { + const mod = await import(pathToFileURL(ESLINT_CONFIG_PATH).href); + const config = mod.default; + if (!Array.isArray(config)) { + throw new Error( + `lint-allow-test-rule-refs: expected eslint.config.mjs's default export to be an array, got ${typeof config}` + ); + } + + const globs = new Set(); + for (const entry of config) { + if (!entry || typeof entry !== 'object' || !entry.rules || !Array.isArray(entry.files)) continue; + if (!Object.prototype.hasOwnProperty.call(entry.rules, 'local/no-source-grep')) continue; + const ruleValue = entry.rules['local/no-source-grep']; + const level = Array.isArray(ruleValue) ? ruleValue[0] : ruleValue; + if (level === 'off' || level === 0) continue; + for (const glob of entry.files) globs.add(glob); + } + + if (globs.size === 0) { + throw new Error( + 'lint-allow-test-rule-refs: derived zero glob patterns for local/no-source-grep from eslint.config.mjs ' + + '— this would silently scan nothing; the config shape likely changed and this script needs updating.' + ); + } + + return [...globs].sort(); +} + +/** + * The literal (non-wildcard) leading path segments of a glob — the + * directory that must actually be walked on disk to find candidate files. + * e.g. `tests/** /*.cjs` -> `tests`, `.kilo/plugins/*.js` -> `.kilo/plugins`. + * + * The LAST segment is always excluded from consideration even when it has + * no wildcard of its own (e.g. the single-file fixture globs + * `tests/_ff_lint_violation.cjs` / `tests/_ff_lint_clean.cjs`, #1279/#2126): + * it is the filename pattern to MATCH against directory entries, never a + * directory to descend INTO. Treating a fully-literal glob's whole path as + * a "directory" would try to `readdirSync` a plain file and silently find + * nothing (caught by `walkGlobs`'s existing not-a-directory guard) — quietly + * dropping that glob from the scan entirely, the same silent-omission defect + * class this fix exists to eliminate. + * + * @param {string} glob + * @returns {string} + */ +function globBaseDir(glob) { + const segments = glob.split('/'); + const literalSegments = []; + for (let i = 0; i < segments.length - 1; i++) { + if (/[*?[\]{}]/.test(segments[i])) break; + literalSegments.push(segments[i]); + } + return literalSegments.join('/'); +} + +/** + * Compile a glob pattern (only `*` and `**` wildcards appear in this repo's + * config — no `?`, `{}`, or `[]` are used on any `local/no-source-grep` + * block) into an anchored RegExp matched against a POSIX-slash repo-relative + * path, mirroring minimatch/eslint's own glob semantics for those two + * operators: `** /` matches zero or more path segments, a bare `*` matches + * within a single segment only. + * + * @param {string} glob + * @returns {RegExp} + */ +function globToRegExp(glob) { + let re = ''; + for (let i = 0; i < glob.length; i++) { + const c = glob[i]; + if (c === '*' && glob[i + 1] === '*') { + i++; // consume second '*' + if (glob[i + 1] === '/') { + re += '(?:.*/)?'; + i++; // consume the following '/' + } else { + re += '.*'; + } + } else if (c === '*') { + re += '[^/]*'; + } else if ('.+^${}()|[]\\'.includes(c)) { + re += `\\${c}`; + } else { + re += c; + } + } + return new RegExp(`^${re}$`); +} + +/** + * Recursively read every file matching ANY of `globs`, once, deduped by + * absolute path. + * + * The citation check, the total marker-file inventory, and the + * effective-site classification all need the same file set and content — + * walking twice would be the generative-fix-divergence class of bug (two + * scans that can silently drift apart), so every classifier below consumes + * this single walk's output. + * + * Each glob's literal base directory (`globBaseDir`) is resolved to an + * actual directory on disk: the `tests` base uses `testsDir` (the existing + * `GSD_LINT_ALLOW_TEST_RULE_TESTS_DIR` override contract, unchanged), every + * other base is resolved under `extraRoot` (defaults to `ROOT`, overridable + * via `GSD_LINT_ALLOW_TEST_RULE_EXTRA_ROOT` so sandbox fixtures can exercise + * the widened scan scope in isolation). Matching itself is always done + * against the glob's own repo-relative path string (e.g. `tests/foo.cjs`, + * `scripts/bar.cjs`), never against the physical scan location, so a + * sandboxed base directory is matched exactly as if it WERE that + * repo-relative path. + * + * @param {string[]} globs + * @param {{testsDir: string, extraRoot: string, root: string}} opts + * @returns {{relpath: string, full: string, content: string}[]} + */ +function walkGlobs(globs, { testsDir, extraRoot, root }) { + const compiled = globs.map((glob) => ({ + baseDir: globBaseDir(glob), + regex: globToRegExp(glob), + })); + const seen = new Map(); // full path -> {relpath, full, content} + + for (const { baseDir, regex } of compiled) { + const scanRoot = baseDir === 'tests' ? testsDir : path.join(extraRoot, baseDir); + + const scan = (current, relParts) => { + let entries; + try { + entries = fs.readdirSync(current, { withFileTypes: true }); + } catch { + // Base directory does not exist (e.g. an isolated sandbox EXTRA_ROOT + // that only sets up the fixture-relevant subtree) — nothing to scan. + return; + } + for (const entry of entries) { + const full = path.join(current, entry.name); + if (entry.isDirectory()) { + scan(full, [...relParts, entry.name]); + continue; + } + if (!entry.isFile()) continue; + const matchPath = [baseDir, ...relParts, entry.name].join('/'); + if (!regex.test(matchPath)) continue; + if (seen.has(full)) continue; let content; try { content = fs.readFileSync(full, 'utf8'); @@ -124,40 +407,30 @@ function walkTestFiles(dir) { // skip unreadable files (e.g. binary) continue; } - files.push({ relpath, content }); + const relpath = path.relative(root, full).split(path.sep).join('/'); + seen.set(full, { relpath, full, content }); } - } + }; + + scan(scanRoot, []); } - scan(dir); - return files; + return [...seen.values()]; } /** * Collect offender identifiers (files with an UNCITED allow-test-rule marker) - * from an already-walked file set. + * from a per-file map of genuine directive reasons (see + * `extractGenuineMarkerReasons`). * - * @param {{relpath: string, content: string}[]} files + * @param {Map} markerReasonsByFile relpath -> reasons * @returns {string[]} sorted, deduped list of ` :: ` strings */ -function collectUncitedOffenders(files) { +function collectUncitedOffenders(markerReasonsByFile) { const offenders = new Set(); - for (const { relpath, content } of files) { - // Scan line-by-line. By testing each line for `allow-test-rule:` we - // cover BOTH comment forms without a cross-line regex: - // // allow-test-rule: ← line comment - // /* allow-test-rule: */ ← single-line block comment - // - // For each matching line we extract the reason (everything after the - // colon), then strip any trailing block-comment closer `*/` and - // whitespace so the identifier stays clean. - for (const line of content.split('\n')) { - const m = ALLOW_TEST_RULE_LINE_RE.exec(line); - if (!m) continue; - // Strip trailing block-comment closer and whitespace if present - const reason = m[1].replace(/\s*\*\/\s*$/, '').trim(); - if (!reason) continue; + for (const [relpath, reasons] of markerReasonsByFile) { + for (const reason of reasons) { if (ISSUE_REF_RE.test(reason)) continue; // compliant — skip offenders.add(`${relpath} :: ${reason}`); } @@ -167,37 +440,317 @@ function collectUncitedOffenders(files) { } /** - * Count distinct files carrying at least one allow-test-rule marker, cited or - * not — the raw total the ceiling ratchets down, independent of citation - * status. + * Files carrying at least one genuine allow-test-rule directive, cited or + * not — the universe that effective/unverified partitions. * - * @param {{relpath: string, content: string}[]} files + * @param {Map} markerReasonsByFile relpath -> reasons * @returns {string[]} sorted, deduped list of relpaths */ -function collectExemptionFiles(files) { - const marked = new Set(); +function collectExemptionFiles(markerReasonsByFile) { + const marked = []; + for (const [relpath, reasons] of markerReasonsByFile) { + if (reasons.length > 0) marked.push(relpath); + } + return marked.sort(); +} - for (const { relpath, content } of files) { - if (ALLOW_TEST_RULE_LINE_RE.test(content)) { - marked.add(relpath); +/** + * ESLint eslintrc-format config fragment that runs ONLY `no-source-grep`, + * with suppression neutralized so every candidate site is reported + * regardless of a marker being present. Shared by every file classified + * below. + * + * Deliberately eslintrc-shaped (`parserOptions`/`env`), not flat-config + * (`languageOptions`/`plugins`+`files`) (#3464 phase 5 follow-up). Flat + * config's `Linter#verify` still validates that the filename resolves to an + * ancestor of the Linter's `cwd` even for a config with no `files` glob + * (i.e. one meant to apply universally) — any filename that only resolves + * via a leading `..` (exactly what `walkGlobs`' `path.relative(ROOT, full)` + * produces for a sandbox fixture outside ROOT, e.g. in CI's `/tmp`) trips + * flat config's "No matching configuration found" error, indistinguishable + * from this script's own defensive throw for a real misconfiguration + * (`classifySiteLines`, `configProblem` below). eslintrc-format + * `Linter#verify` performs no such cwd/ancestor check at all — the config + * passed here fully determines what runs, `filename` is used only for + * per-file diagnostics/overrides, never resolved against `cwd` — so + * classification behaves identically for repo files and out-of-tree sandbox + * fixtures. `sourceType`/`ecmaVersion`/node globals mirror the + * `languageOptions` on every `eslint.config.mjs` block that registers + * `local/no-source-grep` (see module doc comment above); verified to + * produce byte-identical `Linter#verify` messages against flat config for + * real repo files before this change shipped. + */ +const NEUTRALIZED_CONFIG = { + parserOptions: { ecmaVersion: 2022, sourceType: 'commonjs' }, + env: { node: true }, + rules: { 'local/no-source-grep': ['error', { neutralizeSuppression: true }] }, +}; + +/** + * Build an eslintrc-format `Linter` with `local/no-source-grep` registered + * directly via `defineRule` (no plugin-name resolution, no flat-config + * `files` matching) — see `NEUTRALIZED_CONFIG` doc comment for why this + * sidesteps the cwd/ancestor constraint entirely. + * + * @returns {import('eslint').Linter} + */ +function makeLinter() { + const linter = new Linter({ configType: 'eslintrc' }); + linter.defineRule('local/no-source-grep', noSourceGrepRule); + return linter; +} + +/** + * For one file, drive the REAL `no-source-grep` rule via ESLint's `Linter` + * API (suppression neutralized) to enumerate every candidate violation site, + * then classify each site with the rule's own exported `isSuppressedAt` + * predicate against that file's real markers (obtained via the rule's own + * exported `collectMarkerAndCommentLines`, fed by the SAME parse the + * enumeration pass just produced via `linter.getSourceCode()`). + * + * This does not hand-roll a second AST walk or a second adjacency + * arithmetic: both the enumeration (the rule's Program:exit walk) and the + * classification (`isSuppressedAt`) are the rule's own code, imported and + * invoked directly. Returns per-site LINE NUMBERS (not just counts) so a + * consumer (notably the parity test in + * tests/lint-allow-test-rule-refs.test.cjs) can compare this classification + * site-by-site against the real rule's independent report/no-report outcome. + * + * Also returns `markerReasons` (see `extractGenuineMarkerReasons`) computed + * from the SAME parse, so the citation check and exemption-file inventory + * (`main`, via `classifySites`) never re-parse a file a second time just to + * find its markers. + * + * @param {import('eslint').Linter} linter + * @param {{relpath: string, content: string}} file + * @returns {{effectiveLines: number[], liveLines: number[], markerReasons: string[]}} + */ +function classifySiteLines(linter, file) { + let messages; + try { + messages = linter.verify(file.content, NEUTRALIZED_CONFIG, file.relpath); + } catch (err) { + // An unparseable file must NOT be silently classified as "zero sites" + // (indistinguishable from a genuinely clean file) — that is exactly the + // false-clean failure mode this counter exists to eliminate, and it + // would be inconsistent with the sibling "No matching configuration + // found" case just below, which correctly throws. Fail loudly with the + // file path and the underlying error instead. If a specific file + // genuinely cannot be parsed for a known, accepted reason, exempt it + // explicitly by name with a comment here — never blanket-swallow. + throw new Error( + `lint-allow-test-rule-refs: failed to parse/verify ${file.relpath}: ` + + `${err && err.message ? err.message : err}` + ); + } + + // A parse error does NOT throw a JS exception from linter.verify() -- ESLint + // catches it internally and reports it as a `fatal: true` message instead, + // with `sourceCode` left unset (getSourceCode() below would return null). + // This is the same silent-clean failure mode the catch block above guards + // against, just reached via a different ESLint API shape: without this + // check a genuinely unparseable file would either crash on the next line + // (`sourceCode.getAllComments()` on null) or, before that line existed, + // silently fall through the `sites.length === 0` early return below and be + // reported as zero violations -- indistinguishable from a clean file. Fail + // loudly with the file path and message instead, matching the sibling + // "No matching configuration found" case just below. + const fatalProblem = messages.find((m) => m.fatal); + if (fatalProblem) { + throw new Error( + `lint-allow-test-rule-refs: failed to parse/verify ${file.relpath}: ` + + `${fatalProblem.message} (line ${fatalProblem.line}, column ${fatalProblem.column})` + ); + } + + // Defensive: "No matching configuration found for ..." was ESLint's + // flat-config-specific signal that it could not match ANY configuration to + // a filename (notably, when the Linter's cwd was not an ancestor of the + // file -- exactly what an out-of-tree sandbox fixture path hit, #3464 + // phase 5 follow-up). `makeLinter`/`NEUTRALIZED_CONFIG` now use + // eslintrc-format `Linter#verify`, which performs no cwd/ancestor + // resolution at all, so this specific message should never fire in + // practice. Kept as a defensive throw (rather than deleted) in case a + // future ESLint version emits an equivalent `ruleId === null` signal for + // eslintrc-format verification too -- silently reporting zero rule + // messages would be indistinguishable from a genuinely clean file, exactly + // the kind of silently-wrong number this counter exists to avoid. + const configProblem = messages.find( + (m) => m.ruleId === null && /No matching configuration found/.test(m.message) + ); + if (configProblem) { + throw new Error( + `lint-allow-test-rule-refs: ESLint could not lint ${file.relpath} (${configProblem.message}).` + ); + } + + const sourceCode = linter.getSourceCode(); + const allComments = sourceCode.getAllComments(); + const markerReasons = extractGenuineMarkerReasons(allComments); + + const sites = messages.filter((m) => m.messageId === 'noSourceGrep'); + if (sites.length === 0) return { effectiveLines: [], liveLines: [], markerReasons }; + + // The rule's own suppression check (isSuppressed, called from + // reportUnlessSuppressed) is an OR over TWO lines: the search call's own + // line, and the line of the readFileSync() call that originated the + // tracked value (a marker adjacent to EITHER half suppresses). In + // neutralizeSuppression mode the rule emits a paired + // `noSourceGrepDiagnosticReadLine` message at the SAME node carrying that + // read line, so this classification can replicate the identical two-line + // OR check the rule performs internally -- via the rule's OWN + // `isSuppressedAt` predicate, called exactly as `reportUnlessSuppressed` + // does, rather than only checking the search line (which would + // misclassify the documented "marker adjacent to the read, not the + // search" placement style as unsuppressed). + const readLineByPos = new Map(); + for (const m of messages) { + if (m.messageId !== 'noSourceGrepDiagnosticReadLine') continue; + readLineByPos.set(`${m.line}:${m.column}`, m.message === '' ? null : Number(m.message)); + } + + const { markerLines, commentLineSet } = noSourceGrepRule.collectMarkerAndCommentLines(allComments); + + const effectiveLines = []; + const liveLines = []; + for (const site of sites) { + const readLine = readLineByPos.get(`${site.line}:${site.column}`) ?? null; + const suppressedBySearchLine = noSourceGrepRule.isSuppressedAt({ + markerLines, + violationLine: site.line, + commentLineSet, + lines: sourceCode.lines, + }); + const suppressedByReadLine = + readLine !== null && + noSourceGrepRule.isSuppressedAt({ + markerLines, + violationLine: readLine, + commentLineSet, + lines: sourceCode.lines, + }); + if (suppressedBySearchLine || suppressedByReadLine) effectiveLines.push(site.line); + else liveLines.push(site.line); + } + + return { effectiveLines, liveLines, markerReasons }; +} + +/** + * Count-only view of classifySiteLines, used by the aggregate walk below. + * + * @param {import('eslint').Linter} linter + * @param {{relpath: string, content: string}} file + * @returns {{effectiveSites: number, liveSites: number, markerReasons: string[]}} + */ +function classifyFile(linter, file) { + const { effectiveLines, liveLines, markerReasons } = classifySiteLines(linter, file); + return { effectiveSites: effectiveLines.length, liveSites: liveLines.length, markerReasons }; +} + +/** + * Convenience wrapper for tests: classify a single in-memory code string + * without a caller having to construct a Linter instance or a file object. + * + * @param {string} code + * @param {string} [relpath] - filename passed to ESLint (affects nothing but + * diagnostics; the eslintrc-format `Linter` built by `makeLinter` never + * resolves it against a base path, so this may be any string, in-tree or + * not). Defaults to a generic test filename. + * @returns {{effectiveLines: number[], liveLines: number[]}} + */ +function classifyCode(code, relpath = 'tests/fixture.test.cjs') { + return classifySiteLines(makeLinter(), { relpath, content: code }); +} + +/** + * Classify every file's violation sites into effective (suppressed by a real + * marker) vs. live (unsuppressed — should be zero in a clean tree), and + * derive which files count as "effective" (>=1 effective site). + * + * @param {{relpath: string, content: string}[]} files + * Also returns `markerReasonsByFile` (relpath -> genuine directive reasons, + * see `extractGenuineMarkerReasons`), collected from the SAME per-file parse + * this function already performs, so `main` can derive the citation check + * and exemption-file inventory without a second file-content scan. + * + * @returns {{ + * effectiveSiteCount: number, + * effectiveFiles: string[], + * liveSiteCount: number, + * liveFiles: string[], + * markerReasonsByFile: Map, + * }} + */ +function classifySites(files) { + const linter = makeLinter(); + let effectiveSiteCount = 0; + let liveSiteCount = 0; + const effectiveFiles = new Set(); + const liveFiles = new Set(); + const markerReasonsByFile = new Map(); + + for (const file of files) { + const { effectiveSites, liveSites, markerReasons } = classifyFile(linter, file); + if (effectiveSites > 0) { + effectiveSiteCount += effectiveSites; + effectiveFiles.add(file.relpath); + } + if (liveSites > 0) { + liveSiteCount += liveSites; + liveFiles.add(file.relpath); + } + if (markerReasons.length > 0) { + markerReasonsByFile.set(file.relpath, markerReasons); } } - return [...marked].sort(); + return { + effectiveSiteCount, + effectiveFiles: [...effectiveFiles].sort(), + liveSiteCount, + liveFiles: [...liveFiles].sort(), + markerReasonsByFile, + }; } -function main() { +async function main() { const args = process.argv.slice(2); const unknown = args.filter((a) => a !== '--help'); if (unknown.length > 0) { throw new ExitError(2, `lint-allow-test-rule-refs: unknown argument(s): ${unknown.join(', ')}`); } - const files = walkTestFiles(TESTS_DIR); - const current = collectUncitedOffenders(files); + const globs = await deriveNoSourceGrepGlobs(); + const files = walkGlobs(globs, { testsDir: TESTS_DIR, extraRoot: EXTRA_ROOT, root: ROOT }); const known = JSON.parse(fs.readFileSync(ALLOWLIST_PATH, 'utf8')); - const exemptionFiles = collectExemptionFiles(files); - const ceiling = JSON.parse(fs.readFileSync(CEILING_PATH, 'utf8')); + const effectiveCeiling = JSON.parse(fs.readFileSync(EFFECTIVE_CEILING_PATH, 'utf8')); + const unverifiedCeiling = JSON.parse(fs.readFileSync(UNVERIFIED_CEILING_PATH, 'utf8')); + + // Narrow the expensive Linter pass to marker-bearing files only (see + // "Linter scope" doc comment above) — a plain in-memory substring scan + // over content the walk already read, never a shell grep (NUL-byte + // fixture, see comment). This is a strict superset filter: a file with no + // `allow-test-rule:` substring at all cannot produce any genuine directive + // (extractGenuineMarkerReasons requires the substring) or count toward + // effective/unverified, so nothing is lost by skipping it here. + const markerBearingFiles = files.filter((f) => f.content.includes('allow-test-rule:')); + + // classifySites does one Linter parse per marker-bearing file and returns + // both the effective/live site classification AND (as a byproduct of that + // SAME parse) every file's genuine marker reasons — the citation check and + // exemption-file inventory below consume that instead of re-scanning file + // content a second time. + const { effectiveSiteCount, effectiveFiles, liveSiteCount, liveFiles, markerReasonsByFile } = + classifySites(markerBearingFiles); + + const current = collectUncitedOffenders(markerReasonsByFile); + const exemptionFiles = collectExemptionFiles(markerReasonsByFile); + + // Row 5 (#3520 test-matrix): a file with >=1 effective site counts ONLY as + // effective, never also as unverified — no double counting. + const effectiveFileSet = new Set(effectiveFiles); + const unverifiedFiles = exemptionFiles.filter((f) => !effectiveFileSet.has(f)); const failures = []; const { novel } = assertWithinAllowlist({ @@ -208,14 +761,48 @@ function main() { pruneHint: 'edit scripts/lint-allow-test-rule-refs.allowlist.json', }); + // Effective exemption SITES: tightly ratcheted, both directions — the + // metric this gate actually governs. assertTightCeiling({ - label: 'allow-test-rule-total-files', - actualMax: exemptionFiles.length, - ceiling: ceiling.maxFiles, - grace: ceiling.grace, - fail: (msg) => failures.push(`${msg}\n(edit scripts/lint-allow-test-rule-refs.ceiling.json)`), + label: 'allow-test-rule-effective-sites', + actualMax: effectiveSiteCount, + ceiling: effectiveCeiling.maxSites, + grace: effectiveCeiling.grace, + fail: (msg) => failures.push(`${msg}\n(edit scripts/lint-allow-test-rule-refs.effective-ceiling.json)`), }); + // Unverified marker-bearing FILES: a loose upper bound only — growth past + // the ceiling fails, but a drop below it never does (test-matrix.md row 9; + // shrinking this pool is a rule-coverage improvement, not something to + // ratchet on deletion). + if (unverifiedFiles.length > unverifiedCeiling.maxFiles) { + failures.push( + `[allow-test-rule-unverified-markers] ${unverifiedFiles.length} unverified marker-bearing ` + + `file(s) exceeds ceiling ${unverifiedCeiling.maxFiles} (marker-bearing files with no ` + + `detectable violation grew — audit the new markers before raising the ceiling; this is ` + + `growth of the UNVERIFIED pool, distinct from the effective-sites ratchet above).\n` + + `(edit scripts/lint-allow-test-rule-refs.unverified-ceiling.json)` + ); + } + + // Live (unsuppressed) violations: this only covers MARKER-BEARING files + // (see "Linter scope" doc comment above) — a live violation in a file with + // NO marker at all is not re-checked here because `npx eslint .` / + // `npm run lint` / `npm run lint:ci` already fail the build on those via + // `local/no-source-grep` directly. Surfacing the marker-bearing subset + // here makes it visible in this gate's own output rather than only in a + // separate lint pass, without duplicating the repo-wide check. + if (liveSiteCount > 0) { + failures.push( + `[allow-test-rule-live-violations] ${liveSiteCount} unsuppressed no-source-grep ` + + `violation(s) found in ${liveFiles.length} marker-bearing file(s) (unmarked files are ` + + `already enforced separately by npm run lint / npm run lint:ci; this gate only re-checks ` + + `files that carry an allow-test-rule: marker) ` + + `— fix at the source or add a site-scoped // allow-test-rule: (#NNN):\n` + + liveFiles.map((f) => ` - ${f}`).join('\n') + ); + } + if (failures.length > 0) { for (const msg of failures) process.stderr.write(`${msg}\n`); if (novel.length > 0) { @@ -229,8 +816,38 @@ function main() { console.log( `ok lint-allow-test-rule-refs: ${current.length} grandfathered exemption(s) tracked, ` + - `${exemptionFiles.length}/${ceiling.maxFiles} exemption file(s) (ceiling), no novel untracked offenders` + `no novel untracked offenders; ` + + `effective exemptions: ${effectiveSiteCount}/${effectiveCeiling.maxSites} site(s) ` + + `across ${effectiveFiles.length} file(s) (ratcheted); ` + + `unverified markers: ${unverifiedFiles.length}/${unverifiedCeiling.maxFiles} file(s) ` + + `(tracked, not ratcheted); ` + + `live violations in marker-bearing files: ${liveSiteCount} (unmarked files are enforced ` + + `separately by npm run lint / npm run lint:ci, not re-checked here). ` + + `Known limit: "effective" is bounded by no-source-grep's own detection — ` + + `identifier-indirection, dynamic paths, .sh files, and array/object round-trips are ` + + `real blind spots this count cannot see, so it is never "all exemptions."` ); } -runMain(main); +// Exported for tests/lint-allow-test-rule-refs.test.cjs (unit-level +// classification checks and the parity/teeth structural guard) — never +// re-implemented in the test file, imported directly like the CLI itself +// does. `runMain(main)` only fires when this file is executed as the CLI +// entrypoint (`node scripts/lint-allow-test-rule-refs.cjs` / +// `runNode([SCRIPT])`), not when it is `require()`d for its exports. +module.exports = { + deriveNoSourceGrepGlobs, + globBaseDir, + globToRegExp, + walkGlobs, + collectUncitedOffenders, + collectExemptionFiles, + classifySiteLines, + classifyFile, + classifySites, + classifyCode, +}; + +if (require.main === module) { + runMain(main); +} diff --git a/scripts/lint-allow-test-rule-refs.effective-ceiling.json b/scripts/lint-allow-test-rule-refs.effective-ceiling.json new file mode 100644 index 000000000..3546fc07b --- /dev/null +++ b/scripts/lint-allow-test-rule-refs.effective-ceiling.json @@ -0,0 +1,4 @@ +{ + "maxSites": 10, + "grace": 2 +} diff --git a/scripts/lint-allow-test-rule-refs.unverified-ceiling.json b/scripts/lint-allow-test-rule-refs.unverified-ceiling.json new file mode 100644 index 000000000..7518b69f2 --- /dev/null +++ b/scripts/lint-allow-test-rule-refs.unverified-ceiling.json @@ -0,0 +1,3 @@ +{ + "maxFiles": 280 +} diff --git a/tests/lint-allow-test-rule-refs.test.cjs b/tests/lint-allow-test-rule-refs.test.cjs index 4b57617e9..b7e60138b 100644 --- a/tests/lint-allow-test-rule-refs.test.cjs +++ b/tests/lint-allow-test-rule-refs.test.cjs @@ -1,12 +1,15 @@ 'use strict'; -// Tests for scripts/lint-allow-test-rule-refs.cjs — the dual guard that (a) +// Tests for scripts/lint-allow-test-rule-refs.cjs — the guard that (a) // ratchets exemption-marker comments on IDENTITY (uncited comments must -// carry a tracking-issue ref or be grandfathered) and (b) ratchets the total -// distinct exemption-file count against a tight ceiling via assertTightCeiling -// (scripts/lib/allowlist-ratchet.cjs). Uses the script's env overrides to -// point at sandbox fixture dirs/files; never touches the real tests/ dir or -// the real allowlist/ceiling JSON. +// carry a tracking-issue ref or be grandfathered), (b) ratchets EFFECTIVE +// exemption SITES (a marker actually suppressing a violation the +// `no-source-grep` rule detects there) against a tight ceiling, and (c) +// tracks UNVERIFIED marker-bearing files (a marker with no detectable +// violation nearby) against a loose ceiling that never fails on a drop +// (#3520 / epic #3464 phase 5). Uses the script's env overrides to point at +// sandbox fixture dirs/files; never touches the real tests/ dir or the real +// allowlist/ceiling JSON. // // Identifier note: the script computes `relpath = path.relative(ROOT, full)` // where ROOT is the repo root (path.join(__dirname, '..') inside the script), @@ -21,6 +24,7 @@ const { describe, test, before, after } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); +const { Linter } = require('eslint'); const { createTempDir, cleanup } = require('./helpers.cjs'); const { runNode } = require('./helpers/process-seam.cjs'); const { toLegacyResult } = require('./helpers/git-fixture.cjs'); @@ -29,13 +33,33 @@ const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); const ROOT = path.join(__dirname, '..'); const SCRIPT = path.join(ROOT, 'scripts', 'lint-allow-test-rule-refs.cjs'); +// The "repo baseline" row below (unlike every other row in this file) points +// the script at the REAL repo tests/scripts/eslint-rules/etc trees with no +// sandbox override, so it drives ESLint's `Linter` over every real +// marker-bearing file the script finds (~294 files after the #3464 perf +// follow-up narrowed the Linter pass off the full ~1200-file glob walk). +// That is a heavier class than `PROBE_TIMEOUT_MS` describes ("a single short +// CLI query or `node -e` probe against a temp fixture") — measured ~2.3-3.3s +// locally post-narrowing, down from ~7-12s pre-narrowing, which is what +// previously died at exactly the `PROBE_TIMEOUT_MS=15000` bound under CI +// load (empty stdout/stderr, exitCode null — SIGKILLed by the harness +// timeout, not a real assertion failure). 30000ms is ~10x the measured local +// runtime, leaving real headroom for slower CI runners without hand-waving +// the bound back up to "whatever makes it pass this once." +const REPO_BASELINE_LINT_TIMEOUT_MS = 30000; + // Deliberately split so this file's OWN source never contains the contiguous // exemption-marker substring the script under test scans for — the script -// does a raw-text scan (not AST/comment parsing), so an unsplit literal here -// would make THIS test file register as its own offender when the real -// lint:ci run scans tests/. +// does a raw-text scan (not AST/comment parsing) for the marker-inventory +// check, so an unsplit literal here would make THIS test file register as +// its own offender when the real lint:ci run scans tests/. const MARKER = 'allow' + '-test-rule:'; +// Row 13/14/15 (structural guard) import the rule and the script under test +// directly by reference, never re-declaring their logic. +const noSourceGrepRule = require('../eslint-rules/no-source-grep.cjs'); +const scriptUnderTest = require('../scripts/lint-allow-test-rule-refs.cjs'); + let sandbox; let fixtureCount = 0; @@ -48,30 +72,63 @@ function relToRoot(fullPath) { /** * Writes `files` (name -> content) into a fresh sandbox tests dir, plus an - * allowlist JSON and a ceiling JSON, then invokes the script via its three + * allowlist JSON and the two ceiling JSONs, then invokes the script via its * env-var overrides. * * @param {object} opts - * @param {Object} opts.files - filename -> file content. + * @param {Object} opts.files - filename (may include `/` for + * a nested subdirectory, e.g. `helpers/foo.cjs`) -> file content. * @param {string[]} [opts.allowlist] - allowlist array (default []). - * @param {{maxFiles:number,grace:number}} [opts.ceiling] - default is - * deliberately generous so a case testing ONLY the citation check does not - * accidentally also trip the ceiling check. + * @param {{maxSites:number,grace:number}} [opts.effectiveCeiling] - default + * is deliberately generous so a case not targeting the effective-sites + * ratchet does not accidentally also trip it. + * @param {{maxFiles:number}} [opts.unverifiedCeiling] - default is + * deliberately generous, same reasoning. * @param {string[]} [opts.args] - extra CLI argv. + * @param {Object} [opts.extraFiles] - filename (relative to a + * FRESH per-call `extraRoot`, e.g. `scripts/fixture.cjs`) -> file content, + * for exercising the non-`tests/` glob blocks (scripts/** , eslint-rules/** , + * etc. — #3464 phase 5 BLOCKER fix widened scan scope) in isolation. Left + * empty by default, which keeps every existing row's runtime unaffected: a + * per-call `extraRoot` is always created (never shared/reused across + * calls, so nothing leaks between rows) and set as + * `GSD_LINT_ALLOW_TEST_RULE_EXTRA_ROOT`; with no `extraFiles` it stays an + * empty directory, so every non-`tests/` glob costs one cheap empty + * `readdirSync`, never a real-repo scan. */ -function runLint({ files, allowlist = [], ceiling = { maxFiles: 1000, grace: 1000 }, args = [] }) { +function runLint({ + files, + allowlist = [], + effectiveCeiling = { maxSites: 1000, grace: 1000 }, + unverifiedCeiling = { maxFiles: 1000 }, + args = [], + extraFiles = {}, +}) { const testsDir = path.join(sandbox, `tests-${fixtureCount}`); fs.mkdirSync(testsDir, { recursive: true }); const relpaths = {}; for (const [name, content] of Object.entries(files)) { const full = path.join(testsDir, name); + fs.mkdirSync(path.dirname(full), { recursive: true }); fs.writeFileSync(full, content); relpaths[name] = relToRoot(full); } + + const extraRoot = path.join(sandbox, `extra-${fixtureCount}`); + fs.mkdirSync(extraRoot, { recursive: true }); + for (const [name, content] of Object.entries(extraFiles)) { + const full = path.join(extraRoot, name); + fs.mkdirSync(path.dirname(full), { recursive: true }); + fs.writeFileSync(full, content); + relpaths[`extra:${name}`] = relToRoot(full); + } + const allowlistPath = path.join(sandbox, `allowlist-${fixtureCount}.json`); fs.writeFileSync(allowlistPath, JSON.stringify(allowlist)); - const ceilingPath = path.join(sandbox, `ceiling-${fixtureCount}.json`); - fs.writeFileSync(ceilingPath, JSON.stringify(ceiling)); + const effectiveCeilingPath = path.join(sandbox, `effective-ceiling-${fixtureCount}.json`); + fs.writeFileSync(effectiveCeilingPath, JSON.stringify(effectiveCeiling)); + const unverifiedCeilingPath = path.join(sandbox, `unverified-ceiling-${fixtureCount}.json`); + fs.writeFileSync(unverifiedCeilingPath, JSON.stringify(unverifiedCeiling)); fixtureCount += 1; const result = runNode([SCRIPT, ...args], { @@ -80,11 +137,13 @@ function runLint({ files, allowlist = [], ceiling = { maxFiles: 1000, grace: 100 env: { ...process.env, GSD_LINT_ALLOW_TEST_RULE_TESTS_DIR: testsDir, + GSD_LINT_ALLOW_TEST_RULE_EXTRA_ROOT: extraRoot, GSD_LINT_ALLOW_TEST_RULE_ALLOWLIST: allowlistPath, - GSD_LINT_ALLOW_TEST_RULE_CEILING: ceilingPath, + GSD_LINT_ALLOW_TEST_RULE_EFFECTIVE_CEILING: effectiveCeilingPath, + GSD_LINT_ALLOW_TEST_RULE_UNVERIFIED_CEILING: unverifiedCeilingPath, }, }); - return { ...toLegacyResult(result), relpaths, testsDir }; + return { ...toLegacyResult(result), relpaths, testsDir, extraRoot }; } describe('lint-allow-test-rule-refs', () => { @@ -96,35 +155,22 @@ describe('lint-allow-test-rule-refs', () => { cleanup(sandbox); }); - test('passes when a known uncited exemption is grandfathered', () => { - const files = { 'foo.test.cjs': `// ${MARKER} some-reason\n` }; - const testsDir = path.join(sandbox, `tests-${fixtureCount}`); - fs.mkdirSync(testsDir, { recursive: true }); - const full = path.join(testsDir, 'foo.test.cjs'); - fs.writeFileSync(full, files['foo.test.cjs']); - const relpath = relToRoot(full); - const allowlistPath = path.join(sandbox, `allowlist-${fixtureCount}.json`); - fs.writeFileSync(allowlistPath, JSON.stringify([`${relpath} :: some-reason`])); - const ceilingPath = path.join(sandbox, `ceiling-${fixtureCount}.json`); - fs.writeFileSync(ceilingPath, JSON.stringify({ maxFiles: 100, grace: 100 })); - fixtureCount += 1; + // ─── citation check (unchanged contract) ─────────────────────────────── - const r = toLegacyResult( - runNode([SCRIPT], { - cwd: ROOT, - timeoutMs: PROBE_TIMEOUT_MS, - env: { - ...process.env, - GSD_LINT_ALLOW_TEST_RULE_TESTS_DIR: testsDir, - GSD_LINT_ALLOW_TEST_RULE_ALLOWLIST: allowlistPath, - GSD_LINT_ALLOW_TEST_RULE_CEILING: ceilingPath, - }, - }) - ); + test('passes when a known uncited exemption is grandfathered', () => { + // runLint's testsDir naming (`tests-${fixtureCount}`) is deterministic + // and only increments once files are written, so the relpath the script + // will compute can be predicted before the run. + const testsDir = path.join(sandbox, `tests-${fixtureCount}`); + const relpath = relToRoot(path.join(testsDir, 'foo.test.cjs')); + const r = runLint({ + files: { 'foo.test.cjs': `// ${MARKER} some-reason\n` }, + allowlist: [`${relpath} :: some-reason`], + }); assert.strictEqual(r.status, 0, `stderr: ${r.stderr}`); }); - test('fails on a novel uncited exemption not in the allowlist', () => { + test('fails on a novel uncited exemption not in the allowlist (test-matrix row 10)', () => { const r = runLint({ files: { 'foo.test.cjs': `// ${MARKER} mystery-reason\n` }, allowlist: [], @@ -135,7 +181,11 @@ describe('lint-allow-test-rule-refs', () => { test('fails on a stale allowlist entry (ratchet-down enforcement)', () => { const r = runLint({ - files: { 'foo.test.cjs': 'no marker here\n' }, + // Must be VALID JS (parsed by the real Linter, not just grepped) — a + // bare "no marker here" is not valid syntax and would now correctly + // throw a loud parse failure (the SECOND FIX below) rather than + // silently exercising the ratchet-down path this test targets. + files: { 'foo.test.cjs': '// no marker here\n' }, allowlist: ['tests/definitely-stale-file.test.cjs :: stale-reason'], }); assert.notStrictEqual(r.status, 0); @@ -143,152 +193,560 @@ describe('lint-allow-test-rule-refs', () => { assert.match(r.stderr, /definitely-stale-file\.test\.cjs/); }); - test('passes when exemption-file count == ceiling exactly (boundary)', () => { - const r = runLint({ - files: { - 'a.test.cjs': `// ${MARKER} see #123\n`, - 'b.test.cjs': `// ${MARKER} see #123\n`, - 'c.test.cjs': `// ${MARKER} see #123\n`, - }, - allowlist: [], - ceiling: { maxFiles: 3, grace: 0 }, - }); - assert.strictEqual(r.status, 0, `stderr: ${r.stderr}`); - }); - - test('passes when exemption-file count == ceiling - 1 (limit-1 boundary)', () => { - // 2 files, ceiling {maxFiles:3, grace:1} -> slack = 3-2 = 1, not > grace. - // NOTE: grace:0 here (matching the sibling boundary tests) would fail on - // the SEPARATE slack-ratchet check (slack 1 > grace 0), not the count - // check this test targets, so grace is widened to 1 to isolate the axis - // under test. - const r = runLint({ - files: { - 'a.test.cjs': `// ${MARKER} see #123\n`, - 'b.test.cjs': `// ${MARKER} see #123\n`, - }, - allowlist: [], - ceiling: { maxFiles: 3, grace: 1 }, - }); - assert.strictEqual(r.status, 0, `stderr: ${r.stderr}`); - }); - - test('fails when exemption-file count == ceiling + 1 (limit+1 boundary)', () => { - const r = runLint({ - files: { - 'a.test.cjs': `// ${MARKER} see #123\n`, - 'b.test.cjs': `// ${MARKER} see #123\n`, - 'c.test.cjs': `// ${MARKER} see #123\n`, - 'd.test.cjs': `// ${MARKER} see #123\n`, - }, - allowlist: [], - ceiling: { maxFiles: 3, grace: 0 }, - }); - assert.notStrictEqual(r.status, 0); - assert.match(r.stderr, /exceeds budget ceiling/); - }); - - test('passes when slack == grace exactly (boundary)', () => { - // 2 files, ceiling {maxFiles:5, grace:3} -> slack = 5-2 = 3, not > grace. - const r = runLint({ - files: { - 'a.test.cjs': `// ${MARKER} see #123\n`, - 'b.test.cjs': `// ${MARKER} see #123\n`, - }, - allowlist: [], - ceiling: { maxFiles: 5, grace: 3 }, - }); - assert.strictEqual(r.status, 0, `stderr: ${r.stderr}`); - }); - - test('passes when slack == grace - 1 (limit-1 boundary)', () => { - // 3 files, ceiling {maxFiles:5, grace:3} -> slack = 5-3 = 2, less than grace (2 < 3, not > grace). - const r = runLint({ - files: { - 'a.test.cjs': `// ${MARKER} see #123\n`, - 'b.test.cjs': `// ${MARKER} see #123\n`, - 'c.test.cjs': `// ${MARKER} see #123\n`, - }, - allowlist: [], - ceiling: { maxFiles: 5, grace: 3 }, - }); - assert.strictEqual(r.status, 0, `stderr: ${r.stderr}`); - }); - - test('fails when slack == grace + 1 (one past boundary)', () => { - // 1 file, ceiling {maxFiles:5, grace:3} -> slack = 5-1 = 4 > grace. - const r = runLint({ - files: { - 'a.test.cjs': `// ${MARKER} see #123\n`, - }, - allowlist: [], - ceiling: { maxFiles: 5, grace: 3 }, - }); - assert.notStrictEqual(r.status, 0); - assert.match(r.stderr, /Tighten the ceiling/); - }); - test('a cited exemption passes the citation check with an empty allowlist', () => { const r = runLint({ files: { 'cited.test.cjs': `// ${MARKER} see #456\n` }, allowlist: [], - ceiling: { maxFiles: 100, grace: 100 }, }); assert.strictEqual(r.status, 0, `stderr: ${r.stderr}`); }); - test('the SAME cited exemption still counts toward the ceiling total', () => { + test('the SAME cited exemption (no adjacent violation) still counts toward the unverified-files ceiling', () => { + // This marker has NOTHING to suppress (no fs.readFileSync at all in the + // file), so under the effective/unverified split it lands in the + // UNVERIFIED pool, not the effective one — distinct from the old + // single-ceiling behavior this replaces. const r = runLint({ files: { 'cited.test.cjs': `// ${MARKER} see #456\n` }, allowlist: [], - ceiling: { maxFiles: 0, grace: 0 }, + unverifiedCeiling: { maxFiles: 0 }, }); assert.notStrictEqual(r.status, 0); - assert.match(r.stderr, /allow-test-rule-total-files/); + assert.match(r.stderr, /allow-test-rule-unverified-markers/); }); test('a duplicate uncited reason in one file dedupes to one identifier', () => { - const files = { - 'dup.test.cjs': `// ${MARKER} dup-reason\n// ${MARKER} dup-reason\n`, - }; const testsDir = path.join(sandbox, `tests-${fixtureCount}`); - fs.mkdirSync(testsDir, { recursive: true }); - const full = path.join(testsDir, 'dup.test.cjs'); - fs.writeFileSync(full, files['dup.test.cjs']); - const relpath = relToRoot(full); - const allowlistPath = path.join(sandbox, `allowlist-${fixtureCount}.json`); + const relpath = relToRoot(path.join(testsDir, 'dup.test.cjs')); // A SINGLE allowlist entry suffices — if the dedupe regressed (two // identifiers produced), this single-entry allowlist would leave one // novel offender and fail. - fs.writeFileSync(allowlistPath, JSON.stringify([`${relpath} :: dup-reason`])); - const ceilingPath = path.join(sandbox, `ceiling-${fixtureCount}.json`); - fs.writeFileSync(ceilingPath, JSON.stringify({ maxFiles: 100, grace: 100 })); - fixtureCount += 1; - - const r = toLegacyResult( - runNode([SCRIPT], { - cwd: ROOT, - timeoutMs: PROBE_TIMEOUT_MS, - env: { - ...process.env, - GSD_LINT_ALLOW_TEST_RULE_TESTS_DIR: testsDir, - GSD_LINT_ALLOW_TEST_RULE_ALLOWLIST: allowlistPath, - GSD_LINT_ALLOW_TEST_RULE_CEILING: ceilingPath, - }, - }) - ); + const r = runLint({ + files: { 'dup.test.cjs': `// ${MARKER} dup-reason\n// ${MARKER} dup-reason\n` }, + allowlist: [`${relpath} :: dup-reason`], + }); assert.strictEqual(r.status, 0, `stderr: ${r.stderr}`); }); - test('repo baseline passes (real tests/ dir against real allowlist + ceiling)', () => { - const r = runNode([SCRIPT], { cwd: ROOT, timeoutMs: PROBE_TIMEOUT_MS }); - assert.strictEqual(r.exitCode, 0, `stderr: ${r.stderr}\nstdout: ${r.stdout}`); - }); - test('unknown CLI arguments are rejected with exit code 2', () => { const r = runLint({ files: {}, allowlist: [], args: ['--bogus'] }); assert.strictEqual(r.status, 2); assert.match(r.stderr, /unknown argument/); assert.match(r.stderr, /--bogus/); }); + + test('repo baseline passes (real tests/ dir against real allowlist + ceilings)', () => { + const r = runNode([SCRIPT], { cwd: ROOT, timeoutMs: REPO_BASELINE_LINT_TIMEOUT_MS }); + assert.strictEqual(r.exitCode, 0, `stderr: ${r.stderr}\nstdout: ${r.stdout}`); + assert.match(r.stdout, /effective exemptions:/); + assert.match(r.stdout, /unverified markers:/); + assert.match(r.stdout, /Known limit:/); + }); + + // ─── #3520 test-matrix rows 1-12 ──────────────────────────────────────── + + test('row 1: marker adjacent to a detected violation counts as effective, not unverified', () => { + const code = [ + "const fs = require('fs');", + "const path = require('path');", + `// ${MARKER} reason (#1)`, + "const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8'); src.includes('x');", + ].join('\n'); + // Pin the exact effective-site count via a boundary ceiling: passes at + // exactly 1, fails at 0 — that is how this file proves "counted as + // effective" without needing to parse the ok-message text. + const pass = runLint({ + files: { 'row1.test.cjs': code }, + effectiveCeiling: { maxSites: 1, grace: 0 }, + unverifiedCeiling: { maxFiles: 0 }, // must NOT also count as unverified + }); + assert.strictEqual(pass.status, 0, `stderr: ${pass.stderr}`); + + const failsAtZero = runLint({ + files: { 'row1.test.cjs': code }, + effectiveCeiling: { maxSites: 0, grace: 0 }, + }); + assert.notStrictEqual(failsAtZero.status, 0); + assert.match(failsAtZero.stderr, /allow-test-rule-effective-sites/); + }); + + test('row 2: marker with no detectable violation counts as unverified, not effective', () => { + const code = [ + `// ${MARKER} reason (#2)`, + "const fs = require('fs');", + "const path = require('path');", + "const content = fs.readFileSync(path.join(__dirname, '..', 'docs', 'readme.md'), 'utf-8');", + "content.includes('hello');", + ].join('\n'); + const pass = runLint({ + files: { 'row2.test.cjs': code }, + effectiveCeiling: { maxSites: 0, grace: 0 }, // must NOT count as effective + unverifiedCeiling: { maxFiles: 1 }, + }); + assert.strictEqual(pass.status, 0, `stderr: ${pass.stderr}`); + + const failsAtZeroUnverified = runLint({ + files: { 'row2.test.cjs': code }, + unverifiedCeiling: { maxFiles: 0 }, + }); + assert.notStrictEqual(failsAtZeroUnverified.status, 0); + assert.match(failsAtZeroUnverified.stderr, /allow-test-rule-unverified-markers/); + }); + + test('row 3: no marker, no violation — counted in neither pool', () => { + const code = [ + "const assert = require('node:assert/strict');", + "assert.strictEqual(1 + 1, 2);", + ].join('\n'); + const r = runLint({ + files: { 'row3.test.cjs': code }, + effectiveCeiling: { maxSites: 0, grace: 0 }, + unverifiedCeiling: { maxFiles: 0 }, + }); + assert.strictEqual(r.status, 0, `stderr: ${r.stderr}`); + }); + + test('row 4: a violation with NO marker anywhere in the file is not flagged by this gate (#3464 perf follow-up narrowed live-violation detection to marker-bearing files; npm run lint / npm run lint:ci enforce unmarked files separately)', () => { + const code = [ + "const fs = require('fs');", + "const path = require('path');", + "const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8'); src.includes('x');", + ].join('\n'); + const r = runLint({ files: { 'row4.test.cjs': code } }); + assert.strictEqual(r.status, 0, `stderr: ${r.stderr}`); + }); + + test('row 4b: a live violation in a file that DOES carry a marker (elsewhere, unrelated) still fails the gate', () => { + // Same shape as the parity corpus's "marker far above an unrelated later + // violation" fixture: M1 suppresses V1, but V2 is far enough away (and + // has no marker of its own) to stay live. Proves the marker-bearing + // narrowing (row 4 above) does not also let a genuine live violation + // slip through in a file this gate DOES still lint. + const code = [ + "const fs = require('fs');", + "const path = require('path');", + `// ${MARKER} reason for V1 (#1)`, + "const s1 = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8'); s1.includes('x');", + ...Array.from({ length: 20 }, (_, i) => `// unrelated filler line ${i + 1}`), + "const s2 = fs.readFileSync(path.join(__dirname, '..', 'lib', 'b.cjs'), 'utf-8'); s2.includes('y');", + ].join('\n'); + const r = runLint({ files: { 'row4b.test.cjs': code } }); + assert.notStrictEqual(r.status, 0); + assert.match(r.stderr, /allow-test-rule-live-violations/); + assert.match(r.stderr, /row4b\.test\.cjs/); + }); + + test('row 5: a file with one effective site and one inert marker (nothing nearby to suppress) counts as effective only', () => { + // "One suppressing, one not" per test-matrix row 5, realized without an + // actual unsuppressed violation (which would make row 5 indistinguishable + // from row 4's assertion): M1 suppresses a real V1; M2 sits above a .md + // read the rule never flags, so it suppresses nothing because there is + // nothing there to suppress. The file must still classify as effective + // (>=1 effective site), never also unverified — no double counting. + const code = [ + "const fs = require('fs');", + "const path = require('path');", + `// ${MARKER} effective site (#1)`, + "const s1 = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8'); s1.includes('x');", + '', + `// ${MARKER} inert marker, nothing nearby (#2)`, + "const content = fs.readFileSync(path.join(__dirname, '..', 'docs', 'readme.md'), 'utf-8');", + "content.includes('hello');", + ].join('\n'); + const r = runLint({ + files: { 'row5.test.cjs': code }, + effectiveCeiling: { maxSites: 1, grace: 0 }, + unverifiedCeiling: { maxFiles: 0 }, // the file must NOT also appear here + }); + assert.strictEqual(r.status, 0, `stderr: ${r.stderr}`); + }); + + test('row 6: effective count exceeds its ceiling fails, message names effective sites', () => { + const code = [ + "const fs = require('fs');", + "const path = require('path');", + `// ${MARKER} reason (#1)`, + "const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8'); src.includes('x');", + ].join('\n'); + const r = runLint({ + files: { 'row6.test.cjs': code }, + effectiveCeiling: { maxSites: 0, grace: 0 }, + }); + assert.notStrictEqual(r.status, 0); + assert.match(r.stderr, /allow-test-rule-effective-sites/); + assert.match(r.stderr, /exceeds budget ceiling/); + }); + + test('row 7: effective count far below ceiling (slack beyond grace) fails — ratchet-down enforced', () => { + const code = [ + "const fs = require('fs');", + "const path = require('path');", + `// ${MARKER} reason (#1)`, + "const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8'); src.includes('x');", + ].join('\n'); + // actualMax=1, ceiling=10, grace=2 -> slack=9 > grace -> fails. + const r = runLint({ + files: { 'row7.test.cjs': code }, + effectiveCeiling: { maxSites: 10, grace: 2 }, + }); + assert.notStrictEqual(r.status, 0); + assert.match(r.stderr, /allow-test-rule-effective-sites/); + assert.match(r.stderr, /Tighten the ceiling/); + }); + + test('row 8: unverified count exceeds its ceiling fails with a message distinct from row 6', () => { + const code = [ + `// ${MARKER} reason (#2)`, + "const fs = require('fs');", + "const path = require('path');", + "const content = fs.readFileSync(path.join(__dirname, '..', 'docs', 'readme.md'), 'utf-8');", + "content.includes('hello');", + ].join('\n'); + const r = runLint({ + files: { 'row8.test.cjs': code }, + unverifiedCeiling: { maxFiles: 0 }, + }); + assert.notStrictEqual(r.status, 0); + assert.match(r.stderr, /allow-test-rule-unverified-markers/); + assert.doesNotMatch(r.stderr, /allow-test-rule-effective-sites/); + }); + + test('row 9: unverified count dropping below its ceiling still passes (not tightly ratcheted)', () => { + const code = [ + "const assert = require('node:assert/strict');", + "assert.ok(true);", + ].join('\n'); + // Zero unverified files, ceiling deliberately loose (100) -- a real + // "tight" ratchet (like effective-sites) would force this down; the + // unverified ceiling must not. + const r = runLint({ + files: { 'row9.test.cjs': code }, + unverifiedCeiling: { maxFiles: 100 }, + }); + assert.strictEqual(r.status, 0, `stderr: ${r.stderr}`); + }); + + test('row 10: uncited marker is still an allowlist offender (citation contract unchanged)', () => { + const r = runLint({ + files: { 'row10.test.cjs': `// ${MARKER} no citation here\n` }, + allowlist: [], + }); + assert.notStrictEqual(r.status, 0); + assert.match(r.stderr, /no citation here/); + }); + + test('row 11: marker text inside a string literal is not a directive — the violation it sits above stays live', () => { + const code = [ + "const fs = require('fs');", + "const path = require('path');", + `const note = 'not a directive: ${MARKER} fake reason';`, + "const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8'); src.includes(note);", + ].join('\n'); + const r = runLint({ files: { 'row11.test.cjs': code } }); + assert.notStrictEqual(r.status, 0); + assert.match(r.stderr, /allow-test-rule-live-violations/); + assert.match(r.stderr, /row11\.test\.cjs/); + }); + + test('row 12: a NUL-byte file is discovered and classified without a shell grep or a crash', () => { + const testsDir = path.join(sandbox, `tests-${fixtureCount}`); + fs.mkdirSync(testsDir, { recursive: true }); + const full = path.join(testsDir, 'row12.test.cjs'); + // A literal NUL byte embedded in otherwise-valid-looking source, plus a + // marker comment, written via a real byte buffer (never a shell + // redirect/grep, which would choke on the NUL differently than Node's + // own fs + parser do). + const buf = Buffer.concat([ + Buffer.from(`// ${MARKER} reason (#1)\nconst x = '`), + Buffer.from([0]), + Buffer.from("';\n"), + ]); + fs.writeFileSync(full, buf); + const extraRoot = path.join(sandbox, `extra-${fixtureCount}`); + fs.mkdirSync(extraRoot, { recursive: true }); + const effectiveCeilingPath = path.join(sandbox, `effective-ceiling-${fixtureCount}.json`); + fs.writeFileSync(effectiveCeilingPath, JSON.stringify({ maxSites: 1000, grace: 1000 })); + const unverifiedCeilingPath = path.join(sandbox, `unverified-ceiling-${fixtureCount}.json`); + fs.writeFileSync(unverifiedCeilingPath, JSON.stringify({ maxFiles: 1000 })); + const allowlistPath = path.join(sandbox, `allowlist-${fixtureCount}.json`); + fs.writeFileSync(allowlistPath, JSON.stringify([])); + fixtureCount += 1; + + const result = runNode([SCRIPT], { + cwd: ROOT, + timeoutMs: PROBE_TIMEOUT_MS, + env: { + ...process.env, + GSD_LINT_ALLOW_TEST_RULE_TESTS_DIR: testsDir, + GSD_LINT_ALLOW_TEST_RULE_EXTRA_ROOT: extraRoot, + GSD_LINT_ALLOW_TEST_RULE_ALLOWLIST: allowlistPath, + GSD_LINT_ALLOW_TEST_RULE_EFFECTIVE_CEILING: effectiveCeilingPath, + GSD_LINT_ALLOW_TEST_RULE_UNVERIFIED_CEILING: unverifiedCeilingPath, + }, + }); + const r = toLegacyResult(result); + // Never crashes with an uncaught exception (no raw Node stack trace) — + // it either passes or fails cleanly through the script's own messaging. + assert.ok( + r.status === 0 || r.status === 1, + `unexpected exit status ${r.status}; stderr: ${r.stderr}` + ); + assert.doesNotMatch(r.stderr, /at Object\./); + assert.doesNotMatch(r.stderr, /SyntaxError/); + }); + + // ─── #3464 phase 5 BLOCKER fix rows: widened scan scope + loud parse failure ─── + + test('row 16: a non-.test.cjs file nested under tests/ carrying a marker is counted (unverified) — the live-command-registry.cjs case', () => { + // A prior version of the scan only walked `tests/**/*.test.cjs`, so a + // marker in a `tests/helpers/*.cjs`-shaped file (this repo's real + // tests/helpers/live-command-registry.cjs instance) was invisible to + // BOTH reported numbers. This file has no adjacent readFileSync+search + // call, so it lands in the unverified pool, not effective. + const code = [ + `// ${MARKER} reason (#2)`, + "const fs = require('fs');", + "const path = require('path');", + "const content = fs.readFileSync(path.join(__dirname, '..', 'docs', 'readme.md'), 'utf-8');", + "content.includes('hello');", + ].join('\n'); + const pass = runLint({ + files: { 'helpers/row16.cjs': code }, + effectiveCeiling: { maxSites: 0, grace: 0 }, // must NOT count as effective + unverifiedCeiling: { maxFiles: 1 }, + }); + assert.strictEqual(pass.status, 0, `stderr: ${pass.stderr}`); + + const failsAtZero = runLint({ + files: { 'helpers/row16.cjs': code }, + unverifiedCeiling: { maxFiles: 0 }, + }); + assert.notStrictEqual(failsAtZero.status, 0); + assert.match(failsAtZero.stderr, /allow-test-rule-unverified-markers/); + }); + + test('row 17: a marker in a NON-tests registered glob (scripts/**/*.cjs-shaped sandbox path) is counted as effective', () => { + // Exercises the widened scan scope itself (not just the tests/ side): + // `eslint.config.mjs` also registers local/no-source-grep on + // scripts/**/*.cjs (among others), and this must now actually be + // scanned, not merely assumed reachable in theory. + const code = [ + "const fs = require('fs');", + "const path = require('path');", + `// ${MARKER} reason (#1)`, + "const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8'); src.includes('x');", + ].join('\n'); + const pass = runLint({ + files: {}, + extraFiles: { 'scripts/row17-fixture.cjs': code }, + effectiveCeiling: { maxSites: 1, grace: 0 }, + unverifiedCeiling: { maxFiles: 0 }, + }); + assert.strictEqual(pass.status, 0, `stderr: ${pass.stderr}`); + + const failsAtZero = runLint({ + files: {}, + extraFiles: { 'scripts/row17-fixture.cjs': code }, + effectiveCeiling: { maxSites: 0, grace: 0 }, + }); + assert.notStrictEqual(failsAtZero.status, 0); + assert.match(failsAtZero.stderr, /allow-test-rule-effective-sites/); + }); + + test('row 18: an unparseable file makes the gate throw loudly instead of counting it as clean', () => { + // Sibling to the "No matching configuration found" case, which already + // throws — before this fix, a parse/verify failure inside + // classifySiteLines was silently swallowed into `{effectiveLines: [], + // liveLines: []}`, indistinguishable from a genuinely clean file. Genuine + // invalid JS syntax (unbalanced braces / stray token), not just an + // embedded NUL byte (row 12 proves NUL bytes alone parse fine). + // + // Must carry a marker (#3464 perf follow-up): the script now only drives + // the Linter over marker-bearing files (a cheap substring pre-filter + // over raw content — see the script's "Linter scope" doc comment), so an + // unparseable file with NO marker at all is out of scope for THIS gate + // entirely (an unparseable file fails `npm run lint` on its own, via + // ESLint's own parse error, independent of this script). The marker + // comment itself is valid syntax; only what follows it is broken. + const code = `// ${MARKER} reason (#1)\nconst x = {{{ this is not valid javascript syntax ]`; + const r = runLint({ files: { 'row18.test.cjs': code } }); + assert.notStrictEqual(r.status, 0); + assert.match(r.stderr, /failed to parse\/verify/); + assert.match(r.stderr, /row18\.test\.cjs/); + // Never the "ok" success message — a parse failure must never present as + // a clean run. + assert.doesNotMatch(r.stdout, /^ok lint-allow-test-rule-refs/); + }); + + // ─── #3520 test-matrix rows 13-15: structural guard (parity + teeth) ─── + + test('row 13: the script imports the predicate/detection from the rule module — no second copy of the adjacency arithmetic', () => { + const scriptSource = fs.readFileSync(SCRIPT, 'utf8'); + // Imports the rule module directly. + assert.match(scriptSource, /require\(['"]\.\.\/eslint-rules\/no-source-grep\.cjs['"]\)/); + // Calls the rule's own exported predicate/collector, not a re-derivation. + assert.match(scriptSource, /\.isSuppressedAt\(/); + assert.match(scriptSource, /\.collectMarkerAndCommentLines\(/); + // No duplicated lookahead constant or adjacency loop in the script's own + // source — the tell for a hand-rolled second copy of isSuppressedAt's + // arithmetic would be a re-declared "MAX_MARKER_LOOKAHEAD_LINES =" + // assignment or a hand-written `markerLine + 1; l < violationLine` + // purity loop. + assert.doesNotMatch(scriptSource, /MAX_MARKER_LOOKAHEAD_LINES\s*=\s*\d/); + assert.doesNotMatch(scriptSource, /markerLine \+ 1;\s*l\s*<\s*violationLine/); + // Reference identity: what the script requires IS the same object the + // rule module exports (not a structurally-similar copy). + assert.strictEqual( + require('../eslint-rules/no-source-grep.cjs').isSuppressedAt, + noSourceGrepRule.isSuppressedAt + ); + }); + + test('row 14: parity — the script\'s suppressed verdict equals the real rule\'s independent report/no-report outcome, for every site in a fixture corpus', () => { + // Ground truth, computed WITHOUT using isSuppressedAt or + // collectMarkerAndCommentLines at all: run the REAL (non-neutralized) + // `no-source-grep` rule through ESLint's Linter and record which lines + // it reports. This is an independent code path from + // scriptUnderTest.classifySiteLines (which calls the rule's exported + // predicate directly) — the two must still agree everywhere, because + // both are ultimately driven by the same rule. + function realRuleReportedLines(code, filename) { + const linter = new Linter({ configType: 'flat' }); + const config = { + languageOptions: { ecmaVersion: 2022, sourceType: 'commonjs' }, + plugins: { local: { rules: { 'no-source-grep': noSourceGrepRule } } }, + rules: { 'local/no-source-grep': 'error' }, + }; + return linter + .verify(code, config, filename) + .filter((m) => m.messageId === 'noSourceGrep') + .map((m) => m.line) + .sort((a, b) => a - b); + } + + const AT = MARKER; // already split above; reuse for fixture text + + const corpus = [ + // adjacent marker suppresses its violation + [ + "const fs = require('fs');", + "const path = require('path');", + `// ${AT} reason (#1)`, + "const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8'); src.includes('x');", + ].join('\n'), + // no marker: violation stays live + [ + "const fs = require('fs');", + "const path = require('path');", + "const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'b.cjs'), 'utf-8'); src.includes('x');", + ].join('\n'), + // marker far above an unrelated later violation: does not reach it + [ + "const fs = require('fs');", + "const path = require('path');", + `// ${AT} reason for V1 (#1)`, + "const s1 = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8'); s1.includes('x');", + ...Array.from({ length: 20 }, (_, i) => `// unrelated filler line ${i + 1}`), + "const s2 = fs.readFileSync(path.join(__dirname, '..', 'lib', 'b.cjs'), 'utf-8'); s2.includes('y');", + ].join('\n'), + // marker in a string literal does not suppress + [ + "const fs = require('fs');", + "const path = require('path');", + `const note = 'not a directive: ${AT} fake reason';`, + "const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf-8'); src.includes(note);", + ].join('\n'), + // marker directly above the read, search several comment-pure lines later + [ + "const fs = require('fs');", + "const path = require('path');", + `// ${AT} reason (#1)`, + "const src = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf8');", + '// comment-pure line one', + '// comment-pure line two', + "src.includes('x');", + ].join('\n'), + // boundary: marker exactly MAX_MARKER_LOOKAHEAD_LINES (8) above is suppressed + [ + "const fs = require('fs');", + "const path = require('path');", + `// ${AT} reason (#1)`, + ...Array.from({ length: 7 }, (_, i) => `// filler comment line ${i + 1}`), + "const s = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf8'); s.includes('x');", + ].join('\n'), + // boundary: marker one line beyond the lookahead is not suppressed + [ + "const fs = require('fs');", + "const path = require('path');", + `// ${AT} reason (#1)`, + ...Array.from({ length: 8 }, (_, i) => `// filler comment line ${i + 1}`), + "const s = fs.readFileSync(path.join(__dirname, '..', 'lib', 'a.cjs'), 'utf8'); s.includes('x');", + ].join('\n'), + // no violation at all + [ + "const assert = require('node:assert/strict');", + "assert.ok(true);", + ].join('\n'), + ]; + + for (const [i, code] of corpus.entries()) { + const filename = `tests/parity-fixture-${i}.test.cjs`; + const { liveLines } = scriptUnderTest.classifyCode(code, filename); + const realLines = realRuleReportedLines(code, filename); + assert.deepStrictEqual( + [...liveLines].sort((a, b) => a - b), + realLines, + `fixture ${i}: script's live-line verdict disagreed with the real rule's report/no-report outcome` + ); + } + }); + + test('row 15 (teeth for row 14, by construction): the exported isSuppressedAt is the SAME function the rule calls internally, so its default lookahead cannot diverge from the rule\'s', () => { + // Runtime mutation of MAX_MARKER_LOOKAHEAD_LINES is not possible: it is + // declared `const` at module scope inside eslint-rules/no-source-grep.cjs + // and isSuppressedAt's default parameter closes over that lexical + // binding directly, not over a mutable exported property — reassigning + // `noSourceGrepRule.MAX_MARKER_LOOKAHEAD_LINES` would silently do + // nothing to the real default. That immutability is deliberate (the + // constant is not meant to be a runtime knob), so per the brief this + // teeth check is asserted BY CONSTRUCTION rather than by faking a + // mutation: + // + // 1. isSuppressedAt is a single exported function (verified by + // reference identity in row 13) — the script never has its own copy. + // 2. Its `maxLookahead` parameter is genuinely load-bearing (not a + // decorative unused default): explicitly overriding it changes the + // verdict at the exact boundary where the DEFAULT (backed by + // MAX_MARKER_LOOKAHEAD_LINES) says "suppressed". + // 3. Because the rule's own internal isSuppressed() call site and the + // script's classification call site both invoke this identical + // function with NO override (so both fall back to the same + // MAX_MARKER_LOOKAHEAD_LINES default), any future edit to that + // constant necessarily moves both call sites together — there is no + // second constant anywhere to drift out of step with it. + // Default (no override) -> uses MAX_MARKER_LOOKAHEAD_LINES: land exactly + // at the real boundary using the exported constant itself, so this test + // tracks the constant's actual value rather than hardcoding "8". + const gapAtBoundary = noSourceGrepRule.MAX_MARKER_LOOKAHEAD_LINES; + const atBoundary = noSourceGrepRule.isSuppressedAt({ + markerLines: [1], + violationLine: 1 + gapAtBoundary, + commentLineSet: new Set(Array.from({ length: gapAtBoundary }, (_, i) => i + 2)), + lines: Array.from({ length: gapAtBoundary + 1 }, () => '// filler'), + }); + assert.strictEqual(atBoundary, true, 'default lookahead should suppress exactly at the boundary'); + + // Same fixture, EXPLICIT override tighter than the real default: proves + // the parameter actually drives the verdict (not ignored), which is the + // load-bearing fact that makes "both call sites use the same default" + // meaningful rather than vacuous. + const overridden = noSourceGrepRule.isSuppressedAt({ + markerLines: [1], + violationLine: 1 + gapAtBoundary, + commentLineSet: new Set(Array.from({ length: gapAtBoundary }, (_, i) => i + 2)), + lines: Array.from({ length: gapAtBoundary + 1 }, () => '// filler'), + maxLookahead: gapAtBoundary - 1, + }); + assert.strictEqual(overridden, false, 'an explicit tighter lookahead must change the verdict'); + }); });