* docs(test-standards): enforce no-source-grep rule with CI linter + update CONTRIBUTING.md
Adds scripts/lint-no-source-grep.cjs — a static linter that detects readFileSync
on .cjs source files in tests without an allow-test-rule annotation. Wires it
into CI as a new lint-tests job in test.yml and as npm run lint:tests.
Resolves all 9 existing violations across the test suite:
- Rewrites workspace routing tests (3) as behavioral runGsdTools calls that
verify each command is router-recognized (exit != "Unknown init workflow")
- Adds allow-test-rule annotations with explanatory comments to 7 legitimate
structural tests: architectural invariants (locking, orphan-worktree),
structural regression guards (milestone-regex-global), docs-parity
(config-field-docs), integration-test-input (copilot-install), and
structural-implementation-guards (bug-1891, discuss-mode)
Updates CONTRIBUTING.md Testing Standards section with:
- "Prohibited: Source-Grep Tests" section with the before/after pattern,
root cause analysis of why it breaks (commit 990c3e64), and CI reference
- allow-test-rule exemption table (6 recognized categories with when-to-use)
- "CI Test Quality Checks" table showing lint-tests job and local run command
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix: resolve CodeRabbit findings on PR #2700
- CONTRIBUTING.md: "four recognized categories" → "six" (table has 6 rows)
- workspace.test.cjs: use positional args in routing tests (no --name flag)
- lint-no-source-grep.cjs: add source-dir guard to READ_WITH_INLINE_CJS_RE
(mirrors CJS_PATH_CONST_RE's protection against false positives on temp files)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(lint): tighten allow-test-rule and add recursive test discovery
- ALLOW_ANNOTATION now requires at least one non-whitespace char after the
colon so bare '// allow-test-rule:' cannot bypass the lint gate
- findTestFiles() recurses into subdirectories so nested *.test.cjs files
are covered if the tests/ tree ever grows subdirs
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
116 lines
4.5 KiB
JavaScript
116 lines
4.5 KiB
JavaScript
// allow-test-rule: structural-regression-guard
|
|
// milestone.cjs must use replace()+compare, not test()+replace(), to avoid regex
|
|
// lastIndex corruption with global flags. A behavioral test cannot distinguish which
|
|
// pattern was used — it can only observe wrong output after multiple calls, which is
|
|
// fragile. Structural inspection locks the correct fix in place.
|
|
|
|
/**
|
|
* Regression tests for regex global state bug in milestone.cjs
|
|
*
|
|
* The original code used test() + replace() with global-flag regexes.
|
|
* test() advances lastIndex, so a subsequent replace() on the same
|
|
* regex object starts from the wrong position and can miss the match.
|
|
*
|
|
* The fix uses replace() directly and compares before/after to detect
|
|
* whether a substitution occurred, avoiding the lastIndex pitfall.
|
|
*/
|
|
|
|
'use strict';
|
|
|
|
const { describe, test, before } = require('node:test');
|
|
const assert = require('node:assert/strict');
|
|
const fs = require('fs');
|
|
const path = require('path');
|
|
|
|
const MILESTONE_SRC = path.join(__dirname, '..', 'get-shit-done', 'bin', 'lib', 'milestone.cjs');
|
|
|
|
describe('milestone.cjs regex global state fix', () => {
|
|
let src;
|
|
|
|
before(() => {
|
|
src = fs.readFileSync(MILESTONE_SRC, 'utf-8');
|
|
});
|
|
|
|
test('checkbox update uses replace() + compare, not test() + replace()', () => {
|
|
// The old pattern: if (pattern.test(content)) { content = content.replace(pattern, ...); }
|
|
// The new pattern: const after = content.replace(pattern, ...); if (after !== content) { ... }
|
|
const funcBody = src.slice(
|
|
src.indexOf('function cmdRequirementsMarkComplete'),
|
|
src.indexOf('function cmdMilestoneComplete')
|
|
);
|
|
|
|
// Should NOT have test() followed by replace() on the same pattern for checkboxes
|
|
assert.ok(
|
|
!funcBody.includes('checkboxPattern.test(reqContent)'),
|
|
'Should not call test() on checkboxPattern — use replace() + compare instead'
|
|
);
|
|
|
|
// Should have the replace-then-compare pattern
|
|
assert.ok(
|
|
funcBody.includes('afterCheckbox !== reqContent') ||
|
|
funcBody.includes('afterCheckbox!==reqContent'),
|
|
'Should compare before/after replace to detect checkbox changes'
|
|
);
|
|
});
|
|
|
|
test('table update uses replace() + compare, not test() + replace()', () => {
|
|
const funcBody = src.slice(
|
|
src.indexOf('function cmdRequirementsMarkComplete'),
|
|
src.indexOf('function cmdMilestoneComplete')
|
|
);
|
|
|
|
// Should NOT have test() followed by replace() on the same pattern for tables
|
|
assert.ok(
|
|
!funcBody.includes('tablePattern.test(reqContent)'),
|
|
'Should not call test() on tablePattern — use replace() + compare instead'
|
|
);
|
|
|
|
// Should have the replace-then-compare pattern
|
|
assert.ok(
|
|
funcBody.includes('afterTable !== reqContent') ||
|
|
funcBody.includes('afterTable!==reqContent'),
|
|
'Should compare before/after replace to detect table changes'
|
|
);
|
|
});
|
|
|
|
test('done-check regexes use non-global flag (only need existence check)', () => {
|
|
const funcBody = src.slice(
|
|
src.indexOf('function cmdRequirementsMarkComplete'),
|
|
src.indexOf('function cmdMilestoneComplete')
|
|
);
|
|
|
|
// The doneCheckbox and doneTable patterns should use 'i' not 'gi'
|
|
// since test() with 'g' flag has stateful lastIndex
|
|
const doneCheckboxMatch = funcBody.match(/doneCheckbox\s*=\s*new RegExp\([^)]+,\s*'([^']+)'\)/);
|
|
const doneTableMatch = funcBody.match(/doneTable\s*=\s*new RegExp\([^)]+,\s*'([^']+)'\)/);
|
|
|
|
assert.ok(doneCheckboxMatch, 'doneCheckbox regex should exist');
|
|
assert.ok(doneTableMatch, 'doneTable regex should exist');
|
|
assert.ok(
|
|
!doneCheckboxMatch[1].includes('g'),
|
|
'doneCheckbox should not use global flag (only needs existence check via test())'
|
|
);
|
|
assert.ok(
|
|
!doneTableMatch[1].includes('g'),
|
|
'doneTable should not use global flag (only needs existence check via test())'
|
|
);
|
|
});
|
|
|
|
test('no duplicate regex construction for the same pattern', () => {
|
|
const funcBody = src.slice(
|
|
src.indexOf('function cmdRequirementsMarkComplete'),
|
|
src.indexOf('function cmdMilestoneComplete')
|
|
);
|
|
|
|
// The old code created the table pattern twice — once for test(), once for replace().
|
|
// Count lines that construct a regex with 'tablePattern' or the Pending table pattern.
|
|
const tableConstructions = funcBody.split('\n').filter(
|
|
line => line.includes('tablePattern') && line.includes('new RegExp')
|
|
);
|
|
assert.ok(
|
|
tableConstructions.length <= 1,
|
|
`Table pattern regex should be constructed at most once, found ${tableConstructions.length}`
|
|
);
|
|
});
|
|
});
|