* test(#3148): bound the long tail and delete the allowlist Migrates the final 170 unbounded sync spawn sites across 49 files, then removes the allowlist entirely. local/no-unbounded-spawn now runs with no exemption surface across tests/**: there is no file to add a name to. drift-detection's throw-native git() helper routes to gitOrThrow -- bare runGit would have taken 16 call sites quiet on failure. commands.test.cjs has two independently-scoped runGsdTools/runCli helpers, one already bounded and one not; they are kept distinct rather than unified, the same trap as the two same-named git() helpers in Wave 1. runNpm's bound was erasable. Its options spread callerOptions after the defaults, so an explicit timeout:undefined silently dropped the 180000ms bound -- the rule flagged it and was right; it was not a false positive. Fixed by destructuring with a default, with a test that fails when the default is removed. Two sites stay on a raw spawn with an explicit timeout because the seam cannot express them: one needs shell:true for npm.cmd on Windows, one redirects stdout to a real fd. Both are the rule's own documented second option, not an escape from it. Closure verified rather than asserted: the derivation scan reports 0 unbounded spawn helpers and 0 unbounded direct git call sites, and a temporary file carrying an unbounded spawn still errors with the allowlist gone. Closes #3064. * test(#3148): close a hole in the guard's own eslint-disable ban The ban listed only the top level of tests/, so it was blind to 37 .cjs files under tests/helpers, qa, observability, fixtures and dispatch. With the allowlist deleted this test is the sole remaining way to detect someone silencing the rule inline, so the gap was load-bearing: a nested file could carry an unbounded spawn plus an eslint-disable and pass everything. Proven before and after. A probe planted under tests/helpers with both was invisible to the guard and clean under eslint; after making the listing recursive the guard fails on it. The scanned set goes from 771 files to 808. Pre-existing since the guard shipped, but this wave is what promoted it to sole defense, so it is fixed here rather than filed. Also converts the last hand-rolled throw check to throwIfFailed and the last re-derived legacy shape to compose toLegacyResult, which makes the epic's none-remain claim true rather than nearly true. toLegacyResult itself is not widened -- eight callers depend on its shape and one consumer does not justify changing a shared contract. * fix(#3148): correct seam incoherence at the bound and a slow review-lane error path Two real failures from the remote runner, both fixed at the cause. The seam could return outcome TIMED_OUT together with exitCode 0. At the exact bound spawnSync reports ETIMEDOUT while the child has already exited with a real status, and toSeamResult classified on the error code while passing status straight through -- an incoherent pair its own boundary test was written to catch, and did. A status that is not null is direct evidence the child exited on its own, so it now decides the outcome before the error-code branches run. process-seam.cjs was deliberately untouched by every earlier wave; this is a defect in the module itself, kept surgical, with a unit test that fails against the old logic. review-lane with an unknown subcommand fell through to its usage error only after loading the capability registry and building a per-lane plan, which spawns one child process per lane -- up to twelve. The error path took ~1288ms instead of ~119ms, and under bench load it outran a caller's spawn timeout and was killed before writing anything, which is the empty stdout and stderr CI saw. It now fails fast before any of that work begins. This is the epic's first production change. It is user-facing, so it carries a changeset rather than a no-changelog label. * test(#3148): replace a real-race timeout test with a deterministic one E9 raced git rev-parse against a 1ms bound and assumed git always lost. On a warm container git finishes first, spawnSync returns status 0 with no error at all, the seam correctly classifies EXITED, and gitOrThrow correctly does not throw -- so the test failed on both lanes. A probe confirms a genuine timeout always carries status null, so this was never the seam misbehaving. Raising the bound would only lengthen the odds, which is the same defect with better luck. The test now drives gitOrThrow against a stubbed runGit that returns a synthetic TIMED_OUT result, so it asserts exactly what it always meant to -- that a timeout propagates as a throw -- with no timing dependence. Five consecutive runs are identical where the old one varied. I wrote this test in Wave 0; it is a real-race test by construction and CLAUDE.md says to replace those rather than re-run them. * chore(#3148): backfill changeset PR number 3192 --------- Co-authored-by: sim <sim@local>
177 lines
7.9 KiB
JavaScript
177 lines
7.9 KiB
JavaScript
// allow-test-rule: structural-regression-guard [#3143]
|
|
/**
|
|
* no-unbounded-spawn-allowlist.test.cjs
|
|
*
|
|
* `eslint-rules/no-unbounded-spawn.allowlist.json` was deleted by #3148 (the
|
|
* terminal wave of epic #3064): the migration reached zero remaining
|
|
* violations, so the allowlist option was dropped from the rule's wiring in
|
|
* `eslint.config.mjs` and `local/no-unbounded-spawn` now runs with no
|
|
* exemption surface at all under `tests/**`. The former D4/D5/D6/D8 guards
|
|
* here (dead entries, baseline ratchet, separator normalization, canonical
|
|
* sort/dedupe) all referenced that now-deleted file and are gone with it.
|
|
*
|
|
* What remains — D7, the inline-disable ban — matters MORE now, not less:
|
|
* with no allowlist to grandfather a file, an inline `eslint-disable`
|
|
* naming this rule is the ONLY remaining way to silence it. This guard is
|
|
* the sole remaining defense against that, so it stays.
|
|
*
|
|
* D7 needs to inspect test-file *contents* for an inline directive that
|
|
* disables this rule by name — the absence of that pattern is the contract
|
|
* this guard protects (a contributor cannot silence the check by disabling
|
|
* it inline instead of fixing the timeout). That is a `readFileSync` +
|
|
* text-search on `.cjs` files, which is exactly what `local/no-source-grep`
|
|
* exists to catch — hence the
|
|
* `// allow-test-rule: structural-regression-guard [#3143]` annotation on
|
|
* its own line above, per CONTRIBUTING.md's documented exemption.
|
|
*
|
|
* The scan covers ALL of `tests/`, RECURSIVELY — not just the top-level
|
|
* directory. `listTestFiles()` walks every subdirectory (`tests/helpers/`,
|
|
* `tests/qa/`, `tests/observability/`, `tests/fixtures/`, `tests/dispatch/`,
|
|
* etc.) via `fs.readdirSync(..., { recursive: true })`. A non-recursive scan
|
|
* left ~37 nested `.cjs` files completely unchecked: a file in a subdirectory
|
|
* could carry both an unbounded spawn AND an inline eslint-disable directive
|
|
* naming this rule (see `GUARDED_RULE` below), and this guard would never see
|
|
* it. With the allowlist gone, this is the SOLE remaining defense against
|
|
* silencing the rule — recursion is not optional.
|
|
*/
|
|
|
|
'use strict';
|
|
|
|
const { describe, test } = require('node:test');
|
|
const assert = require('node:assert/strict');
|
|
const fs = require('fs');
|
|
const path = require('path');
|
|
|
|
const TESTS_DIR = path.join(__dirname);
|
|
const REPO_ROOT = path.join(__dirname, '..');
|
|
|
|
/**
|
|
* Recursively list every `.cjs` file under `tests/`, including subdirectories
|
|
* (`tests/helpers/`, `tests/qa/`, `tests/observability/`, `tests/fixtures/`,
|
|
* `tests/dispatch/`, etc.). `fs.readdirSync(dir, { recursive: true,
|
|
* withFileTypes: true })` is available on the repo's Node floor (>=22.0.0 per
|
|
* package.json `engines`; the option landed in Node 20.1). Each returned
|
|
* `Dirent` carries `parentPath` — its containing directory, which for a
|
|
* nested entry is the subdirectory, not `TESTS_DIR` — so the joined path is
|
|
* correct at any depth. `node_modules` is skipped defensively in case one is
|
|
* ever vendored under `tests/`.
|
|
*/
|
|
function listTestFiles() {
|
|
const out = [];
|
|
const entries = fs.readdirSync(TESTS_DIR, { withFileTypes: true, recursive: true });
|
|
for (const entry of entries) {
|
|
if (!entry.isFile() || !entry.name.endsWith('.cjs')) continue;
|
|
const dir = entry.parentPath || entry.path || TESTS_DIR;
|
|
if (dir.split(path.sep).includes('node_modules')) continue;
|
|
out.push(path.join(dir, entry.name));
|
|
}
|
|
return out;
|
|
}
|
|
|
|
// Built via concatenation, not a string literal, so this file does not
|
|
// itself contain the literal directive text (`local/no-unbounded-spawn`)
|
|
// that D7 below scans every test file for — a literal here would make this
|
|
// guard flag itself.
|
|
const GUARDED_RULE = 'local' + '/' + 'no-unbounded-spawn';
|
|
|
|
function containsDisableDirective(contents, ruleName) {
|
|
return new RegExp(`eslint-disable[^\\n]*${ruleName}`).test(contents);
|
|
}
|
|
|
|
describe('no-unbounded-spawn allowlist: D7 — no inline disable of this rule', () => {
|
|
test('no test file inline-disables the unbounded-spawn guard', () => {
|
|
const offenders = [];
|
|
for (const filePath of listTestFiles()) {
|
|
const contents = fs.readFileSync(filePath, 'utf8');
|
|
if (containsDisableDirective(contents, GUARDED_RULE)) {
|
|
offenders.push(path.relative(REPO_ROOT, filePath));
|
|
}
|
|
}
|
|
assert.deepEqual(
|
|
offenders,
|
|
[],
|
|
`test files inline-disabling the unbounded-spawn guard (forbidden — fix the timeout instead): ${JSON.stringify(offenders)}`
|
|
);
|
|
});
|
|
|
|
test('detection logic actually flags a synthetic inline-disable directive', () => {
|
|
const syntheticContents = [
|
|
"'use strict';",
|
|
'// eslint-disable-next-line ' + GUARDED_RULE,
|
|
"spawnSync('git', ['status'], {});",
|
|
].join('\n');
|
|
assert.equal(containsDisableDirective(syntheticContents, GUARDED_RULE), true);
|
|
assert.equal(containsDisableDirective("'use strict';\nspawnSync('git', ['status'], {});", GUARDED_RULE), false);
|
|
});
|
|
|
|
test('listTestFiles() recurses into subdirectories, not just the top level', (t) => {
|
|
// Regression for the reviewer-proven hole: a non-recursive scan sees
|
|
// only TESTS_DIR itself and is blind to tests/helpers/, tests/qa/,
|
|
// tests/observability/, tests/fixtures/, tests/dispatch/, etc.
|
|
const probeDir = path.join(TESTS_DIR, 'helpers');
|
|
const probePath = path.join(probeDir, '__probe_recursion_3148.cjs');
|
|
fs.writeFileSync(
|
|
probePath,
|
|
[
|
|
"'use strict';",
|
|
'// eslint-disable-next-line ' + GUARDED_RULE,
|
|
"spawnSync('git', ['status'], {});",
|
|
'',
|
|
].join('\n'),
|
|
);
|
|
t.after(() => {
|
|
// helpers.cleanup() refuses any path outside a recognized temp root;
|
|
// this probe deliberately lives under tests/helpers/ (the thing under
|
|
// test is recursion into a real subdirectory of tests/, not a temp
|
|
// dir), so a raw, force-flagged, single-file rmSync of a path this
|
|
// same test just created is the correct tool here.
|
|
// eslint-disable-next-line local/no-raw-rmsync-in-tests -- probe file lives under tests/helpers/, not a temp root; helpers.cleanup() would refuse it
|
|
fs.rmSync(probePath, { force: true });
|
|
});
|
|
|
|
const found = listTestFiles();
|
|
assert.ok(
|
|
found.includes(probePath),
|
|
'listTestFiles() must include .cjs files nested in subdirectories of tests/',
|
|
);
|
|
});
|
|
|
|
test('a subdirectory file inline-disabling the rule is caught end-to-end', (t) => {
|
|
// Same probe, but exercised through the actual D7 detection path (the
|
|
// same offenders-collection loop the first test in this describe runs),
|
|
// proving the fix closes the hole rather than just listTestFiles().
|
|
const probeDir = path.join(TESTS_DIR, 'helpers');
|
|
const probePath = path.join(probeDir, '__probe_recursion_detect_3148.cjs');
|
|
fs.writeFileSync(
|
|
probePath,
|
|
[
|
|
"'use strict';",
|
|
'// eslint-disable-next-line ' + GUARDED_RULE,
|
|
"spawnSync('git', ['status'], {});",
|
|
'',
|
|
].join('\n'),
|
|
);
|
|
t.after(() => {
|
|
// helpers.cleanup() refuses any path outside a recognized temp root;
|
|
// this probe deliberately lives under tests/helpers/ (the thing under
|
|
// test is recursion into a real subdirectory of tests/, not a temp
|
|
// dir), so a raw, force-flagged, single-file rmSync of a path this
|
|
// same test just created is the correct tool here.
|
|
// eslint-disable-next-line local/no-raw-rmsync-in-tests -- probe file lives under tests/helpers/, not a temp root; helpers.cleanup() would refuse it
|
|
fs.rmSync(probePath, { force: true });
|
|
});
|
|
|
|
const offenders = [];
|
|
for (const filePath of listTestFiles()) {
|
|
const contents = fs.readFileSync(filePath, 'utf8');
|
|
if (containsDisableDirective(contents, GUARDED_RULE)) {
|
|
offenders.push(path.relative(REPO_ROOT, filePath));
|
|
}
|
|
}
|
|
assert.ok(
|
|
offenders.includes(path.relative(REPO_ROOT, probePath)),
|
|
`expected the nested inline-disable probe to be caught; offenders: ${JSON.stringify(offenders)}`,
|
|
);
|
|
});
|
|
});
|