The #452 ESLint migration replaced the homegrown source-grep regex linters with the local/no-source-grep AST rule (eslint-rules/no-source-grep.cjs, wired in eslint.config.mjs) but left the old scripts on disk, wired to nothing. Removed: - scripts/lint-no-source-grep.cjs — CLI linter; no module.exports, never required, not referenced by package.json / CI / eslint. Superseded by the rule. - scripts/lint-no-source-grep-extras.cjs — var-binding + wrapped-assert-ok regex detectors; the ESLint rule covers both forms via AST (VariableDeclarator/AssignmentExpression tracking + member-call checks). - tests/bug-2982-lint-var-binding.test.cjs — exercised only the deleted extras module; the #2982 var-binding scenario is already covered against the live rule by tests/eslint-rules.test.cjs. Also dropped the now-dangling 'scripts/lint-no-source-grep.cjs' entry from DEFAULT_RELATIVE_FILES in scripts/lint-pr-check-project-dir.cjs (the list is existsSync-filtered, so this is tidy-up, not a behavior change). No functional change: source-grep enforcement remains intact via the ESLint rule, and the full unit suite stays green. Three surviving comment-only mentions of "lint-no-source-grep" refer to the rule concept (which lives on in ESLint), not the deleted files. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Joe
@@ -1,81 +0,0 @@
|
||||
'use strict';
|
||||
|
||||
/**
|
||||
* Extended detector for the no-source-grep rule (#2982).
|
||||
*
|
||||
* The base lint (scripts/lint-no-source-grep.cjs) only catches the
|
||||
* direct-chain form: readFileSync(...).includes(...). The much more common
|
||||
* var-binding form escapes it:
|
||||
*
|
||||
* const src = fs.readFileSync(p, 'utf8');
|
||||
* // ... 50 lines later ...
|
||||
* assert.ok(src.includes('foo')); // ← still source-grep, lint missed it
|
||||
*
|
||||
* This module exposes pure detectors that scan source text and return
|
||||
* structured violation records. The CLI wrapper (in the base lint) calls
|
||||
* these for each test file.
|
||||
*
|
||||
* Tests assert on the typed VIOLATION enum codes, not on prose messages.
|
||||
*/
|
||||
|
||||
const VIOLATION = Object.freeze({
|
||||
VAR_FROM_READFILE_USED_IN_TEXT_MATCH: 'var_from_readfile_used_in_text_match',
|
||||
WRAPPED_ASSERT_OK_MATCH: 'wrapped_assert_ok_match',
|
||||
});
|
||||
|
||||
const TEXT_MATCH_METHODS = ['includes', 'startsWith', 'endsWith', 'match', 'search'];
|
||||
|
||||
/**
|
||||
* Single-pass scanner. Tracks variables bound from a readFileSync call,
|
||||
* then flags any subsequent <var>.<method>( use where method is one of
|
||||
* TEXT_MATCH_METHODS.
|
||||
*/
|
||||
function detectVarBindingViolations(src) {
|
||||
// Pass 1: collect variables bound from readFileSync.
|
||||
// Matches: const|let|var <name> = [fs.]readFileSync(
|
||||
const bindRe = /(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*(?:[A-Za-z_$][\w$.]*\.)?readFileSync\s*\(/g;
|
||||
const boundVars = new Set();
|
||||
let m;
|
||||
while ((m = bindRe.exec(src)) !== null) {
|
||||
boundVars.add(m[1]);
|
||||
}
|
||||
if (boundVars.size === 0) return [];
|
||||
|
||||
// Pass 2: find <var>.<method>( on any bound var.
|
||||
const findings = [];
|
||||
// Build a regex alternation from the bound var names.
|
||||
const alt = [...boundVars].map((v) => v.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')).join('|');
|
||||
const useRe = new RegExp(
|
||||
`\\b(${alt})\\s*\\.\\s*(${TEXT_MATCH_METHODS.join('|')})\\s*\\(`,
|
||||
'g',
|
||||
);
|
||||
while ((m = useRe.exec(src)) !== null) {
|
||||
findings.push({
|
||||
kind: VIOLATION.VAR_FROM_READFILE_USED_IN_TEXT_MATCH,
|
||||
variable: m[1],
|
||||
method: m[2],
|
||||
});
|
||||
}
|
||||
return findings;
|
||||
}
|
||||
|
||||
/**
|
||||
* Detects assert.ok(<expr>.match(/.../)) and assert.ok(<expr>.match(<expr>))
|
||||
* which is the same anti-pattern as assert.match but escapes the simpler
|
||||
* regex used by the base lint.
|
||||
*/
|
||||
function detectWrappedAssertOkMatch(src) {
|
||||
const re = /assert\.ok\s*\(\s*[A-Za-z_$][\w$.]*\.match\s*\(/g;
|
||||
const findings = [];
|
||||
let m;
|
||||
while ((m = re.exec(src)) !== null) {
|
||||
findings.push({ kind: VIOLATION.WRAPPED_ASSERT_OK_MATCH });
|
||||
}
|
||||
return findings;
|
||||
}
|
||||
|
||||
function detectAll(src) {
|
||||
return [...detectVarBindingViolations(src), ...detectWrappedAssertOkMatch(src)];
|
||||
}
|
||||
|
||||
module.exports = { detectVarBindingViolations, detectWrappedAssertOkMatch, detectAll, VIOLATION };
|
||||
@@ -1,174 +0,0 @@
|
||||
#!/usr/bin/env node
|
||||
/**
|
||||
* lint-no-source-grep.cjs
|
||||
*
|
||||
* Enforces the "no source-grep tests" rule:
|
||||
* Tests must NOT read source-code .cjs files with readFileSync to assert string
|
||||
* presence. That pattern (source-grep theater) proves a literal exists in source,
|
||||
* not that the runtime behavior is correct.
|
||||
*
|
||||
* ALLOWED:
|
||||
* - require('../gsd-core/bin/lib/foo.cjs') -- runs the module, not text inspection
|
||||
* - readFileSync on .md / .json / .txt files -- product-content or config output
|
||||
* - Files annotated: // allow-test-rule: <reason>
|
||||
*
|
||||
* DISALLOWED (without allow-test-rule):
|
||||
* - readFileSync where the path argument ends in a .cjs filename literal
|
||||
* - A path constant (e.g. CONFIG_PATH) assigned to a .cjs lib file, used in readFileSync
|
||||
*
|
||||
* Exit 0 = clean. Exit 1 = violations found (with diagnostics).
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
|
||||
const TESTS_DIR = path.join(__dirname, '..', 'tests');
|
||||
const ALLOW_ANNOTATION = /\/\/\s*allow-test-rule:\s*\S/;
|
||||
|
||||
// Matches constant definitions that hold a .cjs path in a SOURCE directory.
|
||||
// Requires a source-dir indicator ('bin', 'lib', 'gsd-core') to avoid
|
||||
// flagging temp files like path.join(tmpDir, 'example.cjs').
|
||||
// const CONFIG_PATH = path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'config-schema.cjs');
|
||||
const CJS_PATH_CONST_RE = /(?:const|let|var)\s+(\w+)\s*=\s*path\.join\s*\([^)]*(?:'bin'|"bin"|'lib'|"lib"|'gsd-core'|"gsd-core")[^)]*['"][^'"]*\.cjs['"]/gm;
|
||||
|
||||
// Matches readFileSync with a named variable as first arg
|
||||
const READ_WITH_CONST_RE = /readFileSync\s*\(\s*([A-Za-z_][A-Za-z0-9_]*)\s*,/gm;
|
||||
|
||||
// Matches readFileSync with an inline path.join(.cjs) as first arg
|
||||
const READ_WITH_INLINE_CJS_RE = /readFileSync\s*\([^,)]*path\.join\s*\([^)]*(?:'bin'|"bin"|'lib'|"lib"|'gsd-core'|"gsd-core")[^)]*['"][^'"]*\.cjs['"]/;
|
||||
|
||||
/**
|
||||
* #2962-class violations: raw text matching against process output or file
|
||||
* content. The rule from CONTRIBUTING.md "Prohibited: Raw Text Matching on
|
||||
* Test Outputs": tests assert on typed structured fields, never on rendered
|
||||
* text. Patterns below are the obvious anti-patterns; subtler hidden forms
|
||||
* (e.g. wrapping the same logic in a parser function) are still forbidden
|
||||
* by the prose rule but cannot be detected lexically without an AST.
|
||||
*/
|
||||
const RAW_MATCH_PATTERNS = [
|
||||
{
|
||||
re: /assert\.(?:match|doesNotMatch)\s*\(\s*[A-Za-z_$][A-Za-z0-9_$]*\.(?:stdout|stderr)\b/,
|
||||
label: 'assert.match/doesNotMatch on .stdout/.stderr (emit --json from the SUT and assert on typed fields)',
|
||||
},
|
||||
{
|
||||
re: /\.(?:stdout|stderr)\.(?:includes|startsWith|endsWith)\s*\(/,
|
||||
label: '.stdout/.stderr substring match (emit --json and assert on typed fields)',
|
||||
},
|
||||
{
|
||||
re: /readFileSync\s*\([^)]*\)\s*\.(?:includes|startsWith|endsWith)\s*\(/,
|
||||
label: 'readFileSync(...).<includes|startsWith|endsWith> (expose an IR from production code; assert on its fields)',
|
||||
},
|
||||
];
|
||||
|
||||
function setFromMatches(content, re) {
|
||||
const found = new Set();
|
||||
let m;
|
||||
const cloned = new RegExp(re.source, re.flags);
|
||||
while ((m = cloned.exec(content)) !== null) found.add(m[1]);
|
||||
return found;
|
||||
}
|
||||
|
||||
function check(filepath) {
|
||||
const content = fs.readFileSync(filepath, 'utf-8');
|
||||
const rel = path.relative(path.join(__dirname, '..'), filepath);
|
||||
|
||||
if (ALLOW_ANNOTATION.test(content)) return null;
|
||||
|
||||
const violations = [];
|
||||
|
||||
// Pattern A: readFileSync(path.join(..., 'foo.cjs'), ...)
|
||||
if (READ_WITH_INLINE_CJS_RE.test(content)) {
|
||||
violations.push({
|
||||
reason: 'readFileSync with inline .cjs path literal',
|
||||
fix: 'Replace with runGsdTools() behavioral test, or add // allow-test-rule: <reason>',
|
||||
});
|
||||
}
|
||||
|
||||
// Pattern B: const FOO_PATH = path.join(..., 'foo.cjs') + readFileSync(FOO_PATH, ...)
|
||||
const cjsConsts = setFromMatches(content, CJS_PATH_CONST_RE);
|
||||
if (cjsConsts.size > 0) {
|
||||
const readConsts = setFromMatches(content, READ_WITH_CONST_RE);
|
||||
const overlap = [...cjsConsts].filter(c => readConsts.has(c));
|
||||
if (overlap.length > 0) {
|
||||
violations.push({
|
||||
reason: `source .cjs path constant(s) used in readFileSync: ${overlap.join(', ')}`,
|
||||
fix: 'Replace with runGsdTools() behavioral test, or add // allow-test-rule: <reason>',
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// Patterns C..E: raw text matching against process output or file content.
|
||||
// See CONTRIBUTING.md "Prohibited: Raw Text Matching on Test Outputs".
|
||||
for (const { re, label } of RAW_MATCH_PATTERNS) {
|
||||
if (re.test(content)) {
|
||||
violations.push({
|
||||
reason: label,
|
||||
fix: 'Expose typed IR from production code; assert on structured fields. Or add // allow-test-rule: <reason>',
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// Patterns F..G (#2982): var-binding readFileSync().<text-method>() and
|
||||
// assert.ok(<expr>.match(...)). These escape the simpler patterns above
|
||||
// because the bind and the use are on different lines or wrapped.
|
||||
const extras = require('./lint-no-source-grep-extras.cjs');
|
||||
const varBindFindings = extras.detectVarBindingViolations(content);
|
||||
if (varBindFindings.length > 0) {
|
||||
const samples = varBindFindings.slice(0, 3)
|
||||
.map((f) => `${f.variable}.${f.method}()`)
|
||||
.join(', ');
|
||||
violations.push({
|
||||
reason: `readFileSync-bound variable used in text-match method: ${samples}${varBindFindings.length > 3 ? `, …+${varBindFindings.length - 3} more` : ''}`,
|
||||
fix: 'Expose typed IR; assert on structured fields. Or // allow-test-rule: <reason>',
|
||||
});
|
||||
}
|
||||
const wrappedFindings = extras.detectWrappedAssertOkMatch(content);
|
||||
if (wrappedFindings.length > 0) {
|
||||
violations.push({
|
||||
reason: `assert.ok(<expr>.match(...)) — escapes assert.match rule (${wrappedFindings.length} occurrence${wrappedFindings.length > 1 ? 's' : ''})`,
|
||||
fix: 'Use assert.equal on a typed field, not regex match on text. Or // allow-test-rule: <reason>',
|
||||
});
|
||||
}
|
||||
|
||||
if (violations.length === 0) return null;
|
||||
return { file: rel, violations };
|
||||
}
|
||||
|
||||
function findTestFiles(dir) {
|
||||
const results = [];
|
||||
for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
|
||||
const full = path.join(dir, entry.name);
|
||||
if (entry.isDirectory()) {
|
||||
results.push(...findTestFiles(full));
|
||||
} else if (entry.name.endsWith('.test.cjs')) {
|
||||
results.push(full);
|
||||
}
|
||||
}
|
||||
return results;
|
||||
}
|
||||
|
||||
const testFiles = findTestFiles(TESTS_DIR);
|
||||
|
||||
const violations = testFiles.map(check).filter(Boolean);
|
||||
|
||||
if (violations.length === 0) {
|
||||
console.log(`ok lint-no-source-grep: ${testFiles.length} test files checked, 0 violations`);
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
const totalIssues = violations.reduce((n, v) => n + v.violations.length, 0);
|
||||
process.stderr.write(`\nERROR lint-no-source-grep: ${totalIssues} violation(s) across ${violations.length} file(s)\n\n`);
|
||||
for (const f of violations) {
|
||||
process.stderr.write(` ${f.file}\n`);
|
||||
for (const v of f.violations) {
|
||||
process.stderr.write(` Problem : ${v.reason}\n`);
|
||||
process.stderr.write(` Fix : ${v.fix}\n`);
|
||||
}
|
||||
process.stderr.write('\n');
|
||||
}
|
||||
process.stderr.write('See CONTRIBUTING.md "Prohibited: Source-Grep Tests" and\n');
|
||||
process.stderr.write('"Prohibited: Raw Text Matching on Test Outputs" for guidance.\n');
|
||||
process.stderr.write('Structural tests that legitimately read source files: add // allow-test-rule: <reason>\n\n');
|
||||
process.exit(1);
|
||||
@@ -10,7 +10,6 @@ const DEFAULT_RELATIVE_FILES = [
|
||||
'.github/workflows/test.yml',
|
||||
'.github/workflows/pr-template-format.yml',
|
||||
'.github/workflows/changeset-required.yml',
|
||||
'scripts/lint-no-source-grep.cjs',
|
||||
'scripts/lint-command-contract.cjs',
|
||||
'scripts/lint-skill-deps.cjs',
|
||||
'scripts/lint-descriptions.cjs',
|
||||
|
||||
@@ -1,103 +0,0 @@
|
||||
// allow-test-rule: structural-regression-guard
|
||||
// Reads hook .js or bin/install.js source to assert structural invariants
|
||||
// (search array order, function wiring, path constants) that cannot be
|
||||
// verified by observing runtime outputs alone. Per CONTRIBUTING.md exception matrix.
|
||||
'use strict';
|
||||
// Migrated (#455): detectVarBindingViolations/detectWrappedAssertOkMatch are pure
|
||||
// functions returning typed violation records; all assertions use typed fields.
|
||||
|
||||
process.env.GSD_TEST_MODE = '1';
|
||||
|
||||
const { test, describe } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const path = require('node:path');
|
||||
|
||||
const { detectVarBindingViolations, VIOLATION } = require(path.join(__dirname, '..', 'scripts', 'lint-no-source-grep-extras.cjs'));
|
||||
|
||||
// detectVarBindingViolations is pure: takes source text, returns a list of
|
||||
// violation records. Tests assert on the structured records, not on the
|
||||
// detector's prose (per "Prohibited: Raw Text Matching on Test Outputs").
|
||||
|
||||
describe('Bug #2982: var-binding readFileSync.includes() detector', () => {
|
||||
test('VIOLATION enum exposes the documented codes', () => {
|
||||
assert.deepEqual(
|
||||
Object.keys(VIOLATION).sort(),
|
||||
['VAR_FROM_READFILE_USED_IN_TEXT_MATCH', 'WRAPPED_ASSERT_OK_MATCH'].sort(),
|
||||
);
|
||||
});
|
||||
|
||||
test('flags a single var bound from readFileSync then used with .includes() later', () => {
|
||||
const src = [
|
||||
"const x = fs.readFileSync('foo.cjs', 'utf8');",
|
||||
'// some lines later',
|
||||
"if (x.includes('foo')) { /* ... */ }",
|
||||
].join('\n');
|
||||
const findings = detectVarBindingViolations(src);
|
||||
assert.equal(findings.length, 1);
|
||||
assert.equal(findings[0].kind, VIOLATION.VAR_FROM_READFILE_USED_IN_TEXT_MATCH);
|
||||
assert.equal(findings[0].variable, 'x');
|
||||
assert.equal(findings[0].method, 'includes');
|
||||
});
|
||||
});
|
||||
|
||||
describe('Bug #2982: var-binding detector — coverage of methods (#2982)', () => {
|
||||
const { detectVarBindingViolations, VIOLATION } = require(require('node:path').join(__dirname, '..', 'scripts', 'lint-no-source-grep-extras.cjs'));
|
||||
|
||||
for (const method of ['includes', 'startsWith', 'endsWith', 'match', 'search']) {
|
||||
test(`flags <var>.${method}( on a readFileSync-bound variable`, () => {
|
||||
const src = `const c = fs.readFileSync('x.cjs','utf8');\nc.${method}('foo');\n`;
|
||||
const findings = detectVarBindingViolations(src);
|
||||
assert.equal(findings.length, 1);
|
||||
assert.equal(findings[0].method, method);
|
||||
});
|
||||
}
|
||||
|
||||
test('flags multiple violations across multiple variables', () => {
|
||||
const src = [
|
||||
"const a = readFileSync('a.cjs', 'utf8');",
|
||||
"const b = fs.readFileSync('b.cjs');",
|
||||
"if (a.includes('x')) {}",
|
||||
"if (b.startsWith('y')) {}",
|
||||
"a.endsWith('z');",
|
||||
].join('\n');
|
||||
const findings = detectVarBindingViolations(src);
|
||||
assert.equal(findings.length, 3);
|
||||
const byVar = findings.reduce((acc, f) => {
|
||||
acc[f.variable] = (acc[f.variable] || []).concat(f.method);
|
||||
return acc;
|
||||
}, {});
|
||||
assert.deepEqual(byVar.a.sort(), ['endsWith', 'includes']);
|
||||
assert.deepEqual(byVar.b, ['startsWith']);
|
||||
});
|
||||
|
||||
test('does NOT flag .includes() on a variable that was not bound from readFileSync', () => {
|
||||
const src = "const arr = [1, 2, 3];\nif (arr.includes(2)) {}";
|
||||
assert.deepEqual(detectVarBindingViolations(src), []);
|
||||
});
|
||||
|
||||
test('does NOT flag a fresh string literal substring check', () => {
|
||||
const src = "if ('hello world'.includes('world')) {}";
|
||||
assert.deepEqual(detectVarBindingViolations(src), []);
|
||||
});
|
||||
});
|
||||
|
||||
describe('Bug #2982: assert.ok(...match(...)) detector', () => {
|
||||
const { detectWrappedAssertOkMatch, VIOLATION } = require(require('node:path').join(__dirname, '..', 'scripts', 'lint-no-source-grep-extras.cjs'));
|
||||
|
||||
test('flags assert.ok(text.match(/.../)) which escapes assert.match', () => {
|
||||
const src = "assert.ok(text.match(/Failures: \\d+/));";
|
||||
const findings = detectWrappedAssertOkMatch(src);
|
||||
assert.equal(findings.length, 1);
|
||||
assert.equal(findings[0].kind, VIOLATION.WRAPPED_ASSERT_OK_MATCH);
|
||||
});
|
||||
|
||||
test('does NOT flag assert.match itself (covered by base lint)', () => {
|
||||
const src = "assert.match(text, /foo/);";
|
||||
assert.deepEqual(detectWrappedAssertOkMatch(src), []);
|
||||
});
|
||||
|
||||
test('does NOT flag .matchAll(...) — matchAll is not match, so assert.ok(.matchAll(...)) is not flagged', () => {
|
||||
const src = "assert.ok([...text.matchAll(/foo/g)].length > 0);";
|
||||
assert.deepEqual(detectWrappedAssertOkMatch(src), []);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user