test(#4528): migrate final seam-dispatch batch and retire the timeout-literal allowlist (#4684)

Batch 17 of 17 — the terminal batch — in the ad hoc timeout literal
migration (epic #4445). Replaces every bare numeric timeout/timeoutMs
object-literal property in tests/cjs-command-router-adapter.test.cjs,
tests/dispatcher.test.cjs, tests/run-tests-temp-root.test.cjs, and
tests/shell-command-projection-dispatch.test.cjs with a named constant,
per eslint-rules/no-adhoc-timeout-literal.cjs. No src/bin file touched,
no numeric value changed anywhere.

Eslint ground truth (9 sites) matches the issue's own stated count
exactly for the first time in this epic — no drift to disclose.

Reuses PROBE_TIMEOUT_MS (1 site) and QUICK_SPAWN_TIMEOUT_MS (1 site).
Adds four file-local constants for shapes with no existing match:
RUN_TESTS_ISOLATED_PROBE_TIMEOUT_MS and RUN_TESTS_HARNESS_SPAWN_TIMEOUT_MS
(run-tests-temp-root.test.cjs, distinguishing a `node -e` isolated
function call from a real end-to-end spawn of the test runner itself,
despite each coinciding numerically with an unrelated existing
constant), and EXEC_TOOL_OPTION_PASSTHROUGH_TIMEOUT_MS and
DISPATCH_FORCED_TIMEOUT_MS (shell-command-projection-dispatch.test.cjs
— a mocked-spawnSync pass-through fixture and a deliberately-forced
real timeout, neither a real subprocess bound in the usual sense).

Terminal-batch cleanup: deletes
eslint-rules/no-adhoc-timeout-literal.allowlist.json entirely, drops
its require and the allowlist option from eslint.config.mjs's
local/no-adhoc-timeout-literal registration (now a bare 'error',
mirroring local/no-unbounded-spawn's own already-terminal
configuration in the same file), and updates TESTING-STANDARDS.md's
enforcement note to match — a stale pointer to the deleted file caught
by review, fixed inline. A full-repo eslint run with no cache confirms
zero violations anywhere in the tree under the now allowlist-free rule.

Co-authored-by: sim <sim@local>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-09-13 03:44:51 -04:00
committed by GitHub
parent a841575037
commit ccb39aec15
7 changed files with 57 additions and 21 deletions

View File

@@ -204,7 +204,7 @@ const r = runHookSeam(WORKER_PATH, [], { timeoutMs: NPM_VIEW_TIMEOUT_MS + WORKER
Import an existing class-norm constant from `tests/helpers/timeouts.cjs` (`PROBE_TIMEOUT_MS`, `GIT_TIMEOUT_MS`, `BUILD_TIMEOUT_MS`, `INSTALL_TIMEOUT_MS`) when the call is the same class of subprocess, or declare a local one with a comment justifying why it is a distinct class — see CONTRIBUTING.md's "Use Centralized Test Helpers" section. Import an existing class-norm constant from `tests/helpers/timeouts.cjs` (`PROBE_TIMEOUT_MS`, `GIT_TIMEOUT_MS`, `BUILD_TIMEOUT_MS`, `INSTALL_TIMEOUT_MS`) when the call is the same class of subprocess, or declare a local one with a comment justifying why it is a distinct class — see CONTRIBUTING.md's "Use Centralized Test Helpers" section.
**Enforcement:** `local/no-adhoc-timeout-literal` (ESLint, `error`). A non-literal value (an `Identifier`, `MemberExpression`, or `CallExpression`) is trusted; only a resolvable numeric literal is flagged. There is no marker-comment escape — the fix is always to extract a named constant. `allowlist` (`eslint-rules/no-adhoc-timeout-literal.allowlist.json`) exempts pre-existing legacy violations and only ever ratchets down. **Enforcement:** `local/no-adhoc-timeout-literal` (ESLint, `error`). A non-literal value (an `Identifier`, `MemberExpression`, or `CallExpression`) is trusted; only a resolvable numeric literal is flagged. There is no marker-comment escape — the fix is always to extract a named constant. There is no allowlist. `eslint-rules/no-adhoc-timeout-literal.allowlist.json` grandfathered pre-existing legacy violations; the epic that introduced it (#4445) migrated every site across seventeen batches and deleted the file in its terminal batch, so `local/no-adhoc-timeout-literal` now runs with **no exemption surface**.
### Mutation testing — 80 % threshold ### Mutation testing — 80 % threshold

View File

@@ -1,6 +0,0 @@
[
"tests/cjs-command-router-adapter.test.cjs",
"tests/dispatcher.test.cjs",
"tests/run-tests-temp-root.test.cjs",
"tests/shell-command-projection-dispatch.test.cjs"
]

View File

@@ -42,7 +42,6 @@ import noAdhocTimeoutLiteral from './eslint-rules/no-adhoc-timeout-literal.cjs';
import noRenderedTextLengthAssert from './eslint-rules/no-rendered-text-length-assert.cjs'; import noRenderedTextLengthAssert from './eslint-rules/no-rendered-text-length-assert.cjs';
import noUnconfinedPathJoin from './eslint-rules/no-unconfined-path-join.cjs'; import noUnconfinedPathJoin from './eslint-rules/no-unconfined-path-join.cjs';
const adhocTimeoutLiteralAllowlist = require('./eslint-rules/no-adhoc-timeout-literal.allowlist.json');
const unconfinedPathJoinAllowlist = require('./eslint-rules/no-unconfined-path-join.allowlist.json'); const unconfinedPathJoinAllowlist = require('./eslint-rules/no-unconfined-path-join.allowlist.json');
const localPlugin = { const localPlugin = {
@@ -781,9 +780,9 @@ export default tseslint.config(
'local/no-unbounded-spawn': 'error', 'local/no-unbounded-spawn': 'error',
// Ban a bare numeric `timeout`/`timeoutMs` literal in tests (DEFECT.AD-HOC-TIMEOUT-LITERAL, // Ban a bare numeric `timeout`/`timeoutMs` literal in tests (DEFECT.AD-HOC-TIMEOUT-LITERAL,
// #4428): two independently-guessed copies of the same magic number can drift apart, or // #4428): two independently-guessed copies of the same magic number can drift apart, or
// collide exactly into a zero-margin race. Allowlist starts empty; a pre-existing violation // collide exactly into a zero-margin race. No allowlist: the epic (#4445) migrated every
// gets grandfathered in here as it's found, per eslint-rules/no-adhoc-timeout-literal.allowlist.json. // site; the rule runs with no exemption surface.
'local/no-adhoc-timeout-literal': ['error', { allowlist: adhocTimeoutLiteralAllowlist }], 'local/no-adhoc-timeout-literal': 'error',
// Ban a consolidation-epic folded suite appearing twice in one host file (#3271). // Ban a consolidation-epic folded suite appearing twice in one host file (#3271).
// A second copy runs the same tests twice on every lane and drifts silently. // A second copy runs the same tests twice on every lane and drifts silently.
'local/no-duplicate-fold-marker': 'error', 'local/no-duplicate-fold-marker': 'error',

View File

@@ -522,6 +522,7 @@ const path = require('node:path');
const os = require('node:os'); const os = require('node:os');
const { execFileSync } = require('node:child_process'); const { execFileSync } = require('node:child_process');
const { cleanup } = require('./helpers.cjs'); const { cleanup } = require('./helpers.cjs');
const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
const GSD_TOOLS = path.resolve(__dirname, '..', 'gsd-core', 'bin', 'gsd-tools.cjs'); const GSD_TOOLS = path.resolve(__dirname, '..', 'gsd-core', 'bin', 'gsd-tools.cjs');
@@ -532,7 +533,7 @@ function run(args, cwd) {
stdout: execFileSync(process.execPath, [GSD_TOOLS, ...args], { stdout: execFileSync(process.execPath, [GSD_TOOLS, ...args], {
cwd, cwd,
encoding: 'utf-8', encoding: 'utf-8',
timeout: 15000, timeout: PROBE_TIMEOUT_MS,
}), }),
}; };
} catch (e) { } catch (e) {

View File

@@ -452,6 +452,7 @@ const path = require('node:path');
const { spawnSync } = require('node:child_process'); const { spawnSync } = require('node:child_process');
const SDK_CLI = path.join(__dirname, '..', 'sdk', 'dist', 'cli.js'); const SDK_CLI = path.join(__dirname, '..', 'sdk', 'dist', 'cli.js');
const fs = require('node:fs'); const fs = require('node:fs');
const { QUICK_SPAWN_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
describe('bug #3026 (CR Major outside-diff): SDK forwards plain-text help from gsd-tools fallback', () => { describe('bug #3026 (CR Major outside-diff): SDK forwards plain-text help from gsd-tools fallback', () => {
test('gsd-sdk query phase --help (fallback path) returns usage, not a JSON parse error', (t) => { test('gsd-sdk query phase --help (fallback path) returns usage, not a JSON parse error', (t) => {
@@ -472,7 +473,7 @@ describe('bug #3026 (CR Major outside-diff): SDK forwards plain-text help from g
const result = spawnSync(process.execPath, [SDK_CLI, 'query', 'phase', '--help'], { const result = spawnSync(process.execPath, [SDK_CLI, 'query', 'phase', '--help'], {
encoding: 'utf8', encoding: 'utf8',
stdio: ['ignore', 'pipe', 'pipe'], stdio: ['ignore', 'pipe', 'pipe'],
timeout: 10000, timeout: QUICK_SPAWN_TIMEOUT_MS,
}); });
// The fallback gsd-tools.cjs emits exit 0 with usage on stdout. // The fallback gsd-tools.cjs emits exit 0 with usage on stdout.
assert.strictEqual(result.status, 0, assert.strictEqual(result.status, 0,

View File

@@ -23,6 +23,30 @@ const { createTempDir, cleanup } = require('./helpers.cjs');
const RUNNER = path.join(__dirname, '..', 'scripts', 'run-tests.cjs'); const RUNNER = path.join(__dirname, '..', 'scripts', 'run-tests.cjs');
/**
* A `node -e` snippet that requires `scripts/run-tests.cjs` and calls one of
* its exported functions in an isolated child process, purely so the call
* doesn't poison the PARENT process's own os.tmpdir() resolution for every
* other subtest. No subprocess fan-out beneath the single node child.
* Coincides numerically with GENERATOR_SCRIPT_TIMEOUT_MS, BUILD_TIMEOUT_MS,
* and others in tests/helpers/timeouts.cjs, none of which describe this
* shape (scripts/run-tests.cjs is the test runner itself, not a generator/
* lint script, a hooks bundle, or a git plumbing call) -- kept file-local
* and separately named rather than forced onto an unrelated shared norm.
*/
const RUN_TESTS_ISOLATED_PROBE_TIMEOUT_MS = 30000;
/**
* A REAL end-to-end spawn of scripts/run-tests.cjs itself, running one
* trivial test file through its full lifecycle (temp-root setup, discovery,
* execution, sweep, teardown) -- heavier than
* RUN_TESTS_ISOLATED_PROBE_TIMEOUT_MS above, which only calls one exported
* function in isolation. Coincides numerically with INSTALL_TIMEOUT_MS (a
* full bin/install.js run) but describes a completely unrelated operation
* -- disclosed, not merged.
*/
const RUN_TESTS_HARNESS_SPAWN_TIMEOUT_MS = 120000;
describe('#4020 — run-tests temp root', () => { describe('#4020 — run-tests temp root', () => {
// setupRunTempRoot mutates the PROCESS env (TMPDIR/TEMP/TMP), so these rows // setupRunTempRoot mutates the PROCESS env (TMPDIR/TEMP/TMP), so these rows
// drive it in an isolated child — an in-process call would poison every other // drive it in an isolated child — an in-process call would poison every other
@@ -43,7 +67,7 @@ describe('#4020 — run-tests temp root', () => {
t.after(() => cleanup(outer)); t.after(() => cleanup(outer));
const r = runNode(['-e', setupProbe], { const r = runNode(['-e', setupProbe], {
timeoutMs: 30_000, timeoutMs: RUN_TESTS_ISOLATED_PROBE_TIMEOUT_MS,
env: { ...process.env, TMPDIR: outer, TEMP: outer, TMP: outer }, env: { ...process.env, TMPDIR: outer, TEMP: outer, TMP: outer },
}); });
assert.equal(r.exitCode, 0, `probe failed: ${r.stderr.slice(-300)}`); assert.equal(r.exitCode, 0, `probe failed: ${r.stderr.slice(-300)}`);
@@ -68,7 +92,7 @@ describe('#4020 — run-tests temp root', () => {
t.after(() => cleanup(outer)); t.after(() => cleanup(outer));
const r = runNode(['-e', probe], { const r = runNode(['-e', probe], {
timeoutMs: 30_000, timeoutMs: RUN_TESTS_ISOLATED_PROBE_TIMEOUT_MS,
env: { ...process.env, TMPDIR: outer, TEMP: outer, TMP: outer }, env: { ...process.env, TMPDIR: outer, TEMP: outer, TMP: outer },
}); });
assert.equal(r.exitCode, 0, `probe failed: ${r.stderr.slice(-300)}`); assert.equal(r.exitCode, 0, `probe failed: ${r.stderr.slice(-300)}`);
@@ -136,7 +160,7 @@ describe('#4020 — run-tests temp root', () => {
const r = runNode( const r = runNode(
[RUNNER, '--files', path.basename(target)], [RUNNER, '--files', path.basename(target)],
{ timeoutMs: 120_000, env: { ...process.env, TMPDIR: sandbox, TEMP: sandbox, TMP: sandbox } }, { timeoutMs: RUN_TESTS_HARNESS_SPAWN_TIMEOUT_MS, env: { ...process.env, TMPDIR: sandbox, TEMP: sandbox, TMP: sandbox } },
); );
assert.equal(r.exitCode, 0, `runner should pass: ${r.stderr.slice(-400)}`); assert.equal(r.exitCode, 0, `runner should pass: ${r.stderr.slice(-400)}`);
const m = /tmp-root=(\S+)/.exec(r.stderr); const m = /tmp-root=(\S+)/.exec(r.stderr);
@@ -159,7 +183,7 @@ describe('#4020 — run-tests temp root', () => {
const r = runNode( const r = runNode(
[RUNNER, '--files', path.basename(target)], [RUNNER, '--files', path.basename(target)],
{ timeoutMs: 120_000, env: { ...process.env, TMPDIR: inherited, TEMP: inherited, TMP: inherited } }, { timeoutMs: RUN_TESTS_HARNESS_SPAWN_TIMEOUT_MS, env: { ...process.env, TMPDIR: inherited, TEMP: inherited, TMP: inherited } },
); );
assert.equal(r.exitCode, 0, `nested runner should pass: ${r.stderr.slice(-400)}`); assert.equal(r.exitCode, 0, `nested runner should pass: ${r.stderr.slice(-400)}`);
const m = /tmp-root=(\S+)/.exec(r.stderr); const m = /tmp-root=(\S+)/.exec(r.stderr);
@@ -175,7 +199,7 @@ describe('#4020 — run-tests temp root', () => {
fs.mkdirSync(sibling); fs.mkdirSync(sibling);
const r2 = runNode( const r2 = runNode(
[RUNNER, '--files', path.basename(target)], [RUNNER, '--files', path.basename(target)],
{ timeoutMs: 120_000, env: { ...process.env, TMPDIR: inherited, TEMP: inherited, TMP: inherited } }, { timeoutMs: RUN_TESTS_HARNESS_SPAWN_TIMEOUT_MS, env: { ...process.env, TMPDIR: inherited, TEMP: inherited, TMP: inherited } },
); );
assert.equal(r2.exitCode, 0, `second nested runner should pass: ${r2.stderr.slice(-300)}`); assert.equal(r2.exitCode, 0, `second nested runner should pass: ${r2.stderr.slice(-300)}`);
assert.ok(fs.existsSync(sibling), assert.ok(fs.existsSync(sibling),

View File

@@ -35,6 +35,23 @@ const {
const { createTempGitProject, createTempDir, cleanup } = require('./helpers.cjs'); const { createTempGitProject, createTempDir, cleanup } = require('./helpers.cjs');
/**
* NOT a subprocess timeout -- childProcess.spawnSync is fully mocked in
* this test, so no real spawn ever runs. Arbitrary fixture value proving
* execTool forwards its caller-supplied `timeout` option to spawnSync
* verbatim (a pass-through assertion, not a timing assertion).
*/
const EXEC_TOOL_OPTION_PASSTHROUGH_TIMEOUT_MS = 1234;
/**
* Deliberately far smaller than any real dispatchGsdCommand invocation
* could complete, forcing a genuine wall-clock timeout so this test can
* assert timedOut:true is reported correctly (mirrors the
* RUN_BASH_SCRIPT_FORCED_TIMEOUT_MS / BOUNDED_SHELL_FORCED_TIMEOUT_MS
* pattern used elsewhere in this migration).
*/
const DISPATCH_FORCED_TIMEOUT_MS = 1;
// ─── execGit ───────────────────────────────────────────────────────────────── // ─── execGit ─────────────────────────────────────────────────────────────────
describe('execGit', () => { describe('execGit', () => {
@@ -1114,10 +1131,10 @@ describe('execTool (#3411 windows resolution)', () => {
}); });
t.after(() => mock.restoreAll()); t.after(() => mock.restoreAll());
execTool('some-tool', [], { cwd: '/tmp/x', env: { FOO: 'bar' }, timeout: 1234 }); execTool('some-tool', [], { cwd: '/tmp/x', env: { FOO: 'bar' }, timeout: EXEC_TOOL_OPTION_PASSTHROUGH_TIMEOUT_MS });
assert.equal(receivedOptions.cwd, '/tmp/x'); assert.equal(receivedOptions.cwd, '/tmp/x');
assert.equal(receivedOptions.timeout, 1234); assert.equal(receivedOptions.timeout, EXEC_TOOL_OPTION_PASSTHROUGH_TIMEOUT_MS);
assert.equal(receivedOptions.env.FOO, 'bar'); assert.equal(receivedOptions.env.FOO, 'bar');
// Case-insensitive: process.env's actual key casing is OS-dependent (Windows // Case-insensitive: process.env's actual key casing is OS-dependent (Windows
// conventionally sets `Path`, not `PATH`), and the merged object here is a // conventionally sets `Path`, not `PATH`), and the merged object here is a
@@ -1198,7 +1215,7 @@ describe('dispatchGsdCommand', () => {
test('a wall-clock timeout is reported via timedOut:true, ok:false — never throws', () => { test('a wall-clock timeout is reported via timedOut:true, ok:false — never throws', () => {
assert.doesNotThrow(() => { assert.doesNotThrow(() => {
const result = dispatchGsdCommand({ family: 'progress', subcommand: 'json', cwd: tmpDir, timeout: 1 }); const result = dispatchGsdCommand({ family: 'progress', subcommand: 'json', cwd: tmpDir, timeout: DISPATCH_FORCED_TIMEOUT_MS });
assert.equal(result.ok, false); assert.equal(result.ok, false);
assert.equal(result.timedOut, true); assert.equal(result.timedOut, true);
}); });