test(#4527): migrate planning/review-lane batch to named timeout constants (#4680)

Batch 16 of 17 in the ad hoc timeout literal migration (epic #4445).
Replaces every bare numeric timeout/timeoutMs object-literal property in
9 test files with a named constant, per
eslint-rules/no-adhoc-timeout-literal.cjs. No src/bin file touched, no
numeric value changed anywhere.

Ground truth via eslint found 48 sites, not the issue's stated 38 —
tests/code-review.test.cjs alone has 11, not 1 (a 10-site undercount,
the largest single-file drift in this epic). All 11 are migrated.

Reuses PROBE_TIMEOUT_MS (17 sites across assumption-delta.test.cjs and
code-review.test.cjs), LOOP_HOOK_POINT_CLI_TIMEOUT_MS (1 site),
QUICK_SPAWN_TIMEOUT_MS (1 site). Adds a new shared constant,
HTTP_REACHABLE_PROBE_TIMEOUT_FIXTURE_MS, promoted because two files
(reviewer-manifest-body.test.cjs, reviewer-trust-disclosure.test.cjs)
independently arrived at the same probe-fixture value across 3 sites.

File-local constants elsewhere for values not shared across files:
FALLOW_AUDIT_TIMEOUT_MS (code-review-pipeline-regression.test.cjs);
a 5-constant set covering runBashScript's own override/forced-timeout/
boundary-triple tests (plan-phase-stall-detection.test.cjs); 9
lane-specific NATIVE_TIMEOUT_MS constants, one per shipped reviewer CLI
tool, even where 4 lanes coincidentally share a value
(review-lane-invocation.test.cjs); and a 6-constant set covering
reviewer-manifest-body.test.cjs's own probe-kind fixtures and its
separate, unrelated boundary/invalid set that coincidentally overlaps
in shape (not value) with plan-phase-stall-detection.test.cjs's triple.

Fixes applied inline from review: GENERATOR_SCRIPT_TIMEOUT_MS was
misapplied to 3 sites in plan-review-convergence.test.cjs whose actual
shape (nested bash -> node -> gsd-tools.cjs chain) doesn't match that
constant's documented direct-spawn class, confirmed against its own
cited precedent files — replaced with a new file-local constant
naming the correct class, same value. Two stray un-renamed literals in
review-lane-invocation.test.cjs (missed by eslint's object-literal-only
detection) were investigated and left as disclosed literals with an
explanatory comment rather than force-fit onto an unrelated lane's
constant, since they belong to a distinct config-override code path.

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 02:35:23 -04:00
committed by GitHub
parent 2a5d919a03
commit a841575037
11 changed files with 164 additions and 71 deletions

View File

@@ -1,15 +1,6 @@
[
"tests/assumption-delta-checkpoint-e2e.test.cjs",
"tests/assumption-delta.test.cjs",
"tests/cjs-command-router-adapter.test.cjs",
"tests/code-review-pipeline-regression.test.cjs",
"tests/code-review.test.cjs",
"tests/dispatcher.test.cjs",
"tests/plan-phase-stall-detection.test.cjs",
"tests/plan-review-convergence.test.cjs",
"tests/review-lane-invocation.test.cjs",
"tests/reviewer-manifest-body.test.cjs",
"tests/reviewer-trust-disclosure.test.cjs",
"tests/run-tests-temp-root.test.cjs",
"tests/shell-command-projection-dispatch.test.cjs"
]

View File

@@ -22,6 +22,7 @@ const path = require('node:path');
const { execFileSync } = require('node:child_process');
const { cleanup, TEST_ENV_BASE } = require('./helpers.cjs');
const { LOOP_HOOK_POINT_CLI_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
const TOOLS_PATH = path.join(__dirname, '..', 'gsd-core', 'bin', 'gsd-tools.cjs');
@@ -36,7 +37,7 @@ function runTools(args, cwd) {
cwd,
encoding: 'utf-8',
env: { ...process.env, ...TEST_ENV_BASE },
timeout: 60000,
timeout: LOOP_HOOK_POINT_CLI_TIMEOUT_MS,
});
return { success: true, output: stdout.trim(), exitCode: 0, error: '' };
} catch (err) {

View File

@@ -15,6 +15,7 @@ const assert = require('node:assert/strict');
const { spawnSync } = require('node:child_process');
const path = require('node:path');
const { exitCodeFor } = require('../gsd-core/bin/lib/exit-code-registry.cjs');
const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
const MODULE_PATH = path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'assumption-delta.cjs');
@@ -184,7 +185,7 @@ describe('assumption-delta CLI — STDIN exit codes (mirrors ui-safety-gate)', (
const res = spawnSync(process.execPath, [MODULE_PATH], {
input: stdin,
encoding: 'utf-8',
timeout: 15000,
timeout: PROBE_TIMEOUT_MS,
});
return { status: res.status, stdout: res.stdout ?? '', stderr: res.stderr ?? '' };
}
@@ -212,7 +213,7 @@ describe('assumption-delta CLI — STDIN exit codes (mirrors ui-safety-gate)', (
const res = spawnSync(process.execPath, [MODULE_PATH, '--json'], {
input: 'introduces a configurable retry policy',
encoding: 'utf-8',
timeout: 15000,
timeout: PROBE_TIMEOUT_MS,
});
assert.strictEqual(res.status, 0);
const parsed = JSON.parse(res.stdout);
@@ -225,7 +226,7 @@ describe('assumption-delta CLI — STDIN exit codes (mirrors ui-safety-gate)', (
const res = spawnSync(process.execPath, [MODULE_PATH, '--json'], {
input: 'just a routine refactor',
encoding: 'utf-8',
timeout: 15000,
timeout: PROBE_TIMEOUT_MS,
});
assert.strictEqual(res.status, 1);
const parsed = JSON.parse(res.stdout);
@@ -239,7 +240,7 @@ describe('assumption-delta CLI — STDIN exit codes (mirrors ui-safety-gate)', (
const noFire = spawnSync(process.execPath, [MODULE_PATH, '--terms', 'xyzzy', '--json'], {
input: 'adds a second platform',
encoding: 'utf-8',
timeout: 15000,
timeout: PROBE_TIMEOUT_MS,
});
assert.strictEqual(noFire.status, 1);
assert.strictEqual(JSON.parse(noFire.stdout).detected, false);
@@ -247,7 +248,7 @@ describe('assumption-delta CLI — STDIN exit codes (mirrors ui-safety-gate)', (
const fire = spawnSync(process.execPath, [MODULE_PATH, '--terms', 'xyzzy', '--json'], {
input: 'introduces an xyzzy adapter',
encoding: 'utf-8',
timeout: 15000,
timeout: PROBE_TIMEOUT_MS,
});
assert.strictEqual(fire.status, 0);
const parsed = JSON.parse(fire.stdout);
@@ -405,7 +406,7 @@ describe('assumption-delta hardening (Codex review)', () => {
const res = spawnSync(process.execPath, [MODULE_PATH, '--terms', '', '--json'], {
input: 'adds a second platform',
encoding: 'utf-8',
timeout: 15000,
timeout: PROBE_TIMEOUT_MS,
});
assert.strictEqual(res.status, 0, 'empty --terms must fall back to defaults → detected');
const parsed = JSON.parse(res.stdout);

View File

@@ -36,6 +36,13 @@ const {
findShellFencedMatches,
} = require('./helpers/shell-doc-scan.cjs');
/**
* A single invocation of the external `fallow` binary's `audit`
* subcommand against the REAL current repo tree (CI-only) -- a heavier
* third-party tool operation distinct from any existing shared constant.
*/
const FALLOW_AUDIT_TIMEOUT_MS = 120000;
const ROOT = path.resolve(__dirname, '..');
const WORKFLOW_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'code-review.md');
const PRE_PASS_STEP_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'code-review', 'steps', 'structural-pre-pass.md');
@@ -1066,7 +1073,7 @@ describe('Bug 5 (#3191) — same anchored, portable phase-scope grep at all thre
const audit = execTool(
requireFallowBinary({ cwd: ROOT, envPath: '' }),
['audit', '--changed-since', fallowBase[0], '--format', 'json'],
{ cwd: repo, timeout: 120000 },
{ cwd: repo, timeout: FALLOW_AUDIT_TIMEOUT_MS },
);
assert.ok([0, 1].includes(audit.exitCode), `fallow root audit exit=${audit.exitCode}; stderr=${audit.stderr}`);
console.log(`fallow-root-audit normal-exit=${audit.exitCode}`);

View File

@@ -42,6 +42,7 @@ const os = require('os');
const { runGsdTools, createTempProject, createTempGitProject, cleanup } = require('./helpers.cjs');
const { runNode } = require('./helpers/process-seam.cjs');
const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs');
const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
const REPO_ROOT = path.join(__dirname, '..');
const GSD_TOOLS_BIN = path.join(REPO_ROOT, 'gsd-core', 'bin', 'gsd-tools.cjs');
@@ -610,7 +611,7 @@ describe('CR-CONFIG: config key registration', () => {
function renderHooksEnvelope(tmpDir, point) {
const result = runNode(
[GSD_TOOLS_BIN, 'loop', 'render-hooks', point, '--cwd', tmpDir],
{ cwd: REPO_ROOT, timeoutMs: 15000 },
{ cwd: REPO_ROOT, timeoutMs: PROBE_TIMEOUT_MS },
);
assert.strictEqual(result.exitCode, 0, `Expected exit 0 for render-hooks ${point}. stderr: ` + (result.stderr || ''));
return JSON.parse(result.stdout.trim());
@@ -737,7 +738,7 @@ describe('CR-REVIEWER-LANES: optional external source-reviewer dispatch (#4209)'
'echo "CODE_REVIEW_POINT=[$CODE_REVIEW_POINT]"',
'echo "EXPLICIT_JOINED=[$EXPLICIT_JOINED]"',
].join('\n');
const result = require('node:child_process').spawnSync('bash', ['-c', driver], { cwd: tmpDir, encoding: 'utf8', timeout: 15000 });
const result = require('node:child_process').spawnSync('bash', ['-c', driver], { cwd: tmpDir, encoding: 'utf8', timeout: PROBE_TIMEOUT_MS });
assert.equal(result.status, 0, `driver failed: stdout=${result.stdout} stderr=${result.stderr}`);
assert.match(result.stdout, /CODE_REVIEW_POINT=\[execute:post\]/,
`CODE_REVIEW_POINT must be computed and survive within the SAME fence that later uses it for --point, got: ${result.stdout}`);
@@ -788,7 +789,7 @@ describe('CR-REVIEWER-LANES: optional external source-reviewer dispatch (#4209)'
'--repo-root', tmpDir, '--depth', 'standard', '--base-sha', 'deadbeef',
'--run-dir', tmpDir, '--cwd', tmpDir, '--explicit', 'not-a-real-reviewer-xyz',
'--cap-id', 'code-review', '--point', 'execute:post', '--raw'],
{ cwd: REPO_ROOT, timeoutMs: 15000, input: 'src/foo.ts\n' },
{ cwd: REPO_ROOT, timeoutMs: PROBE_TIMEOUT_MS, input: 'src/foo.ts\n' },
);
assert.strictEqual(result.exitCode, 0, `expected exit 0, stderr: ${result.stderr || ''}`);
const parsed = JSON.parse(result.stdout.trim());
@@ -811,7 +812,7 @@ describe('CR-REVIEWER-LANES: optional external source-reviewer dispatch (#4209)'
'--repo-root', tmpDir, '--depth', 'standard', '--base-sha', 'deadbeef',
'--run-dir', tmpDir, '--cwd', tmpDir, '--explicit', 'codex',
'--cap-id', 'no-such-capability-xyz', '--point', 'execute:post', '--raw'],
{ cwd: REPO_ROOT, timeoutMs: 15000, input: 'src/foo.ts\n' },
{ cwd: REPO_ROOT, timeoutMs: PROBE_TIMEOUT_MS, input: 'src/foo.ts\n' },
);
assert.strictEqual(result.exitCode, 0, `expected exit 0, stderr: ${result.stderr || ''}`);
const parsed = JSON.parse(result.stdout.trim());
@@ -830,7 +831,7 @@ describe('CR-REVIEWER-LANES: optional external source-reviewer dispatch (#4209)'
[GSD_TOOLS_BIN, 'review-lane', 'dispatch-step',
'--repo-root', tmpDir, '--depth', 'standard', '--base-sha', 'deadbeef',
'--run-dir', tmpDir, '--cwd', tmpDir, '--explicit', 'codex', '--raw'],
{ cwd: REPO_ROOT, timeoutMs: 15000, input: 'src/foo.ts\n' },
{ cwd: REPO_ROOT, timeoutMs: PROBE_TIMEOUT_MS, input: 'src/foo.ts\n' },
);
assert.strictEqual(result.exitCode, 0, `expected exit 0, stderr: ${result.stderr || ''}`);
const parsed = JSON.parse(result.stdout.trim());
@@ -849,7 +850,7 @@ describe('CR-REVIEWER-LANES: optional external source-reviewer dispatch (#4209)'
'--repo-root', tmpDir, '--depth', 'standard', '--base-sha', 'deadbeef',
'--run-dir', tmpDir, '--cwd', tmpDir, '--explicit', 'codex',
'--cap-id', 'code-review', '--raw'],
{ cwd: REPO_ROOT, timeoutMs: 15000, input: 'src/foo.ts\n' },
{ cwd: REPO_ROOT, timeoutMs: PROBE_TIMEOUT_MS, input: 'src/foo.ts\n' },
);
assert.strictEqual(result.exitCode, 0, `expected exit 0, stderr: ${result.stderr || ''}`);
assert.match(result.stderr, /--cap-id and --point must both be given/,
@@ -864,7 +865,7 @@ describe('CR-REVIEWER-LANES: optional external source-reviewer dispatch (#4209)'
test('review-lane explicit-from-argv matches CLI flags against the merged roster (#4209 RQ-02)', () => {
const result = runNode(
[GSD_TOOLS_BIN, 'review-lane', 'explicit-from-argv', '--', '--codex', '--agy'],
{ cwd: REPO_ROOT, timeoutMs: 15000 },
{ cwd: REPO_ROOT, timeoutMs: PROBE_TIMEOUT_MS },
);
assert.strictEqual(result.exitCode, 0, `expected exit 0, stderr: ${result.stderr || ''}`);
assert.strictEqual(result.stdout.trim(), 'antigravity,codex');
@@ -873,7 +874,7 @@ describe('CR-REVIEWER-LANES: optional external source-reviewer dispatch (#4209)'
test('review-lane explicit-from-argv resolves to empty when no known flag is present', () => {
const result = runNode(
[GSD_TOOLS_BIN, 'review-lane', 'explicit-from-argv', '--'],
{ cwd: REPO_ROOT, timeoutMs: 15000 },
{ cwd: REPO_ROOT, timeoutMs: PROBE_TIMEOUT_MS },
);
assert.strictEqual(result.exitCode, 0, `expected exit 0, stderr: ${result.stderr || ''}`);
assert.strictEqual(result.stdout.trim(), '');
@@ -957,7 +958,7 @@ describe('CR-REVIEWER-LANES: optional external source-reviewer dispatch (#4209)'
'--repo-root', tmpDir, '--depth', 'standard', '--base-sha', 'deadbeef',
'--run-dir', tmpDir, '--cwd', tmpDir,
'--cap-id', 'code-review', '--point', 'execute:post', '--raw'],
{ cwd: REPO_ROOT, timeoutMs: 15000, input: 'src/foo.ts\n' },
{ cwd: REPO_ROOT, timeoutMs: PROBE_TIMEOUT_MS, input: 'src/foo.ts\n' },
);
assert.strictEqual(result.exitCode, 0, `expected exit 0, stderr: ${result.stderr || ''}`);
const parsed = JSON.parse(result.stdout.trim());
@@ -976,7 +977,7 @@ describe('CR-REVIEWER-LANES: optional external source-reviewer dispatch (#4209)'
'--repo-root', tmpDir, '--depth', 'standard', '--base-sha', 'deadbeef',
'--run-dir', tmpDir, '--cwd', tmpDir, '--explicit', 'not-a-real-reviewer-xyz',
'--cap-id', 'code-review', '--point', 'execute:post', '--raw'],
{ cwd: REPO_ROOT, timeoutMs: 15000, input: 'src/foo.ts\n' },
{ cwd: REPO_ROOT, timeoutMs: PROBE_TIMEOUT_MS, input: 'src/foo.ts\n' },
);
assert.strictEqual(result.exitCode, 0, `expected exit 0, stderr: ${result.stderr || ''}`);
const parsed = JSON.parse(result.stdout.trim());
@@ -1010,7 +1011,7 @@ describe('CR-REVIEWER-LANES: optional external source-reviewer dispatch (#4209)'
function runReducer(dispatchJson) {
const script = `DISPATCH_JSON=${JSON.stringify(dispatchJson)}\n${extractEvidenceReducer()}\necho "$EVIDENCE_LIST"`;
const result = require('node:child_process').spawnSync('bash', ['-c', script], { encoding: 'utf8', timeout: 15000 });
const result = require('node:child_process').spawnSync('bash', ['-c', script], { encoding: 'utf8', timeout: PROBE_TIMEOUT_MS });
return { stdout: result.stdout, stderr: result.stderr, status: result.status };
}

View File

@@ -319,6 +319,18 @@ const GSD_TOOLS_CLI_MODERATE_TIMEOUT_MS = 30000;
*/
const TASK_RESOLVER_INVOKE_TIMEOUT_MS = 10000;
/**
* NOT a subprocess spawn timeout. Fixture DATA -- the default `timeoutMs`
* value for an `http-reachable`-kind reviewer-lane probe declaration,
* validated by `validateReviewerBody()` (pure schema validation, never a
* real HTTP call or spawn in these tests).
*
* Shared across 2 files in batch #4527 of the ad hoc timeout literal
* migration, epic #4445 -- every site independently arrived at this exact
* value -- that is why it lives here rather than as a file-local constant.
*/
const HTTP_REACHABLE_PROBE_TIMEOUT_FIXTURE_MS = 2000;
module.exports = {
PROBE_TIMEOUT_MS,
HOOK_FANOUT_TIMEOUT_MS,
@@ -338,4 +350,5 @@ module.exports = {
PATHOLOGICAL_INPUT_TEST_TIMEOUT_MS,
GSD_TOOLS_CLI_MODERATE_TIMEOUT_MS,
TASK_RESOLVER_INVOKE_TIMEOUT_MS,
HTTP_REACHABLE_PROBE_TIMEOUT_FIXTURE_MS,
};

View File

@@ -657,6 +657,24 @@ describe('bug #2650 plan-phase — all five planner/plan-checker spawns dispatch
// ─────────────────────────────────────────────────────────────────────────────
describe('#2650 follow-up: runBashScript bounds and reports a bash fan-out correctly', () => {
/**
* An arbitrary value proving `runBashScript`'s explicit timeoutMs
* argument overrides its own class-norm default (HOOK_FANOUT_TIMEOUT_MS).
*/
const RUN_BASH_SCRIPT_OVERRIDE_TIMEOUT_MS = 1234;
/**
* Deliberately tiny (not generous headroom) to force a real `sleep 5`
* command past the bound within this test's own lifetime, proving an
* exceeded bound reports TIMED_OUT, not a bare null status.
*/
const RUN_BASH_SCRIPT_FORCED_TIMEOUT_MS = 250;
/** CLAUDE.md boundary-coverage triple (limit-1/limit/limit+1) on runBashScript's own timeoutMs value-domain validation: a negative bound must be rejected. */
const RUN_BASH_SCRIPT_TIMEOUT_BOUNDARY_NEGATIVE_MS = -1;
/** CLAUDE.md boundary-coverage triple (limit-1/limit/limit+1) on runBashScript's own timeoutMs value-domain validation: zero must be rejected, never read as unbounded. Used at both occurrences in this test (the throw assertion and the later leak-check re-throw). */
const RUN_BASH_SCRIPT_TIMEOUT_BOUNDARY_ZERO_MS = 0;
/** CLAUDE.md boundary-coverage triple (limit-1/limit/limit+1) on runBashScript's own timeoutMs value-domain validation: the smallest positive bound is valid and must be accepted. */
const RUN_BASH_SCRIPT_TIMEOUT_BOUNDARY_ONE_MS = 1;
test('bounds the fan-out with the class norm, and an explicit override still wins', (t) => {
t.after(() => mock.restoreAll());
const seen = [];
@@ -672,14 +690,14 @@ describe('#2650 follow-up: runBashScript bounds and reports a bash fan-out corre
`default bound must be the bash-fan-out class norm (${HOOK_FANOUT_TIMEOUT_MS}ms), not a probe-sized literal; got ${seen[0].timeoutMs}`);
assert.equal(seen[0].interpreter, 'bash', 'the seam must be told to run the script under bash');
runBashScript('echo hi\n', [], { timeoutMs: 1234 });
assert.equal(seen[1].timeoutMs, 1234, 'an explicit timeoutMs must override the class norm');
runBashScript('echo hi\n', [], { timeoutMs: RUN_BASH_SCRIPT_OVERRIDE_TIMEOUT_MS });
assert.equal(seen[1].timeoutMs, RUN_BASH_SCRIPT_OVERRIDE_TIMEOUT_MS, 'an explicit timeoutMs must override the class norm');
});
test('an exceeded bound reports TIMED_OUT, not a bare null status', () => {
// A real sleep against a deliberately tiny bound. The assertion is on the
// CLASSIFICATION, never on elapsed time.
const result = runBashScript('sleep 5\n', [], { timeoutMs: 250 });
const result = runBashScript('sleep 5\n', [], { timeoutMs: RUN_BASH_SCRIPT_FORCED_TIMEOUT_MS });
assert.equal(result.outcome, OUTCOME.TIMED_OUT,
`an exceeded bound must name itself; got outcome=${result.outcome} status=${result.status}`);
assert.equal(result.timedOut, true, 'timedOut must be true when the bound is exceeded');
@@ -702,18 +720,18 @@ describe('#2650 follow-up: runBashScript bounds and reports a bash fan-out corre
// all", which is precisely the unbounded-spawn hazard local/no-unbounded-spawn
// exists to prevent (CONTRIBUTING.md: "`timeout: 0` — Node reads zero as *no
// timeout*"). The seam rejects it instead of honouring it.
assert.throws(() => runBashScript('exit 0\n', [], { timeoutMs: 0 }), TypeError,
assert.throws(() => runBashScript('exit 0\n', [], { timeoutMs: RUN_BASH_SCRIPT_TIMEOUT_BOUNDARY_ZERO_MS }), TypeError,
'limit: zero must be rejected, never read as unbounded');
assert.throws(() => runBashScript('exit 0\n', [], { timeoutMs: -1 }), TypeError,
assert.throws(() => runBashScript('exit 0\n', [], { timeoutMs: RUN_BASH_SCRIPT_TIMEOUT_BOUNDARY_NEGATIVE_MS }), TypeError,
'limit-1: a negative bound must be rejected');
assert.doesNotThrow(() => runBashScript('exit 0\n', [], { timeoutMs: 1 }),
assert.doesNotThrow(() => runBashScript('exit 0\n', [], { timeoutMs: RUN_BASH_SCRIPT_TIMEOUT_BOUNDARY_ONE_MS }),
'limit+1: the smallest positive bound is valid and must be accepted');
// The helper must still clean up its temp dir when the seam throws — the throw
// escapes through runBashScript's `finally`, which is what makes the rejection safe
// to rely on rather than a resource leak.
const before = fs.readdirSync(os.tmpdir()).filter((n) => n.startsWith('gsd-2650-sh-')).length;
assert.throws(() => runBashScript('exit 0\n', [], { timeoutMs: 0 }), TypeError);
assert.throws(() => runBashScript('exit 0\n', [], { timeoutMs: RUN_BASH_SCRIPT_TIMEOUT_BOUNDARY_ZERO_MS }), TypeError);
const after = fs.readdirSync(os.tmpdir()).filter((n) => n.startsWith('gsd-2650-sh-')).length;
assert.equal(after, before, 'a rejected bound must not leak the script temp dir');
});

View File

@@ -34,9 +34,23 @@ const path = require('path');
const { execFileSync } = require('node:child_process');
const { readFileNormalized, readWorkflowCombined, createTempDir, cleanup } = require('./helpers.cjs');
const { runHook, OUTCOME } = require('./helpers/process-seam.cjs');
const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
const { PROBE_TIMEOUT_MS, QUICK_SPAWN_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
const fc = require('fast-check');
// A nested bash → node → gsd-tools.cjs cold-start chain: `execFileSync('bash',
// ['-c', script], ...)` where `script` itself invokes `node "$GSD_TOOLS_PATH"
// ...`. Deliberately NOT `GENERATOR_SCRIPT_TIMEOUT_MS` despite the
// coincidentally-matching value: that constant's own doc comment (see
// tests/helpers/timeouts.cjs) scopes it to "a single scripts/*.cjs generator
// or lint script, spawned directly ... no fan-out" — and its cited precedent
// sites (tests/adr-index-gate.test.cjs, tests/check-env.test.cjs) confirm
// that shape in practice: both spawn `process.execPath` against a single
// script file directly via spawnSync, with no intermediate shell. This
// file's sites are a heavier, different shape (bash wrapping a further node
// spawn), so they get their own file-local constant instead of borrowing one
// whose doc comment describes something else.
const NESTED_BASH_GSD_TOOLS_TIMEOUT_MS = 30000;
const COMMAND_PATH = path.join(__dirname, '..', 'commands', 'gsd', 'plan-review-convergence.md');
const WORKFLOW_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'plan-review-convergence.md');
const CONFIG_DOC_PATH = path.join(__dirname, '..', 'docs', 'CONFIGURATION.md');
@@ -52,7 +66,7 @@ const WORKFLOW_REVIEW_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows',
// for the same skip pattern). Behavioral tests that exercise the deployed jq
// pipeline skip when jq is absent; structural tests still run.
let jqAvailable = false;
try { execFileSync('jq', ['--version'], { stdio: 'ignore', timeout: 10000, killSignal: 'SIGKILL' }); jqAvailable = true; } catch { /* no jq on PATH */ }
try { execFileSync('jq', ['--version'], { stdio: 'ignore', timeout: QUICK_SPAWN_TIMEOUT_MS, killSignal: 'SIGKILL' }); jqAvailable = true; } catch { /* no jq on PATH */ }
// #2800: the hand-written per-flag `grep -q '\-\-<flag>'` whitelist lines were
// replaced by a loop deriving REVIEWER_FLAGS from `gsd_run review-lane flags`
@@ -77,13 +91,14 @@ function runReviewerFlagsParseBlock(block, args) {
return execFileSync('bash', ['-c', script], {
env: { ...process.env, ARGUMENTS: args, GSD_TOOLS_PATH },
encoding: 'utf8',
// 30s covers the nested bash → node → gsd-tools.cjs cold-start chain this
// helper spawns. 5s was too tight: on a loaded bench (30k tests running
// in parallel) that budget was consumed by process-spawn scheduling
// latency alone, producing a spurious ETIMEDOUT with no genuine hang.
// 30_000 matches the convention other script-invocation tests in this
// repo already use (see e.g. adr-index-gate.test.cjs, check-env.test.cjs).
timeout: 30_000,
// NESTED_BASH_GSD_TOOLS_TIMEOUT_MS (30s) covers the nested bash → node →
// gsd-tools.cjs cold-start chain this helper spawns. 5s was too tight: on
// a loaded bench (30k tests running in parallel) that budget was consumed
// by process-spawn scheduling latency alone, producing a spurious
// ETIMEDOUT with no genuine hang. See that constant's doc comment for why
// this is a distinct class from GENERATOR_SCRIPT_TIMEOUT_MS despite the
// matching value.
timeout: NESTED_BASH_GSD_TOOLS_TIMEOUT_MS,
}).trim();
}
@@ -403,10 +418,11 @@ describe('plan-review-convergence: #2315 respects review.default_reviewers (no-f
return execFileSync('bash', ['-c', script], {
env: { ...process.env, ARGUMENTS: args, GSD_TEST_DEFAULT_REVIEWERS: defaultReviewers ?? '', GSD_TOOLS_PATH },
encoding: 'utf8',
// 30s covers the same nested bash → node → gsd-tools.cjs cold-start
// chain as runReviewerFlagsParseBlock above; see that helper's
// comment for why 5s flaked under bench load.
timeout: 30_000,
// NESTED_BASH_GSD_TOOLS_TIMEOUT_MS covers the same nested bash → node
// → gsd-tools.cjs cold-start chain as runReviewerFlagsParseBlock
// above; see that helper's comment for why 5s flaked under bench
// load.
timeout: NESTED_BASH_GSD_TOOLS_TIMEOUT_MS,
});
};
@@ -460,10 +476,11 @@ describe('plan-review-convergence: #2315 respects review.default_reviewers (no-f
return execFileSync('bash', ['-c', script], {
env: { ...process.env, ARGUMENTS: args, GSD_TEST_DEFAULT_REVIEWERS: defaultReviewers ?? '', GSD_TOOLS_PATH },
encoding: 'utf8',
// 30s covers the same nested bash → node → gsd-tools.cjs cold-start
// chain as runReviewerFlagsParseBlock above; see that helper's
// comment for why 5s flaked under bench load.
timeout: 30_000,
// NESTED_BASH_GSD_TOOLS_TIMEOUT_MS covers the same nested bash → node
// → gsd-tools.cjs cold-start chain as runReviewerFlagsParseBlock
// above; see that helper's comment for why 5s flaked under bench
// load.
timeout: NESTED_BASH_GSD_TOOLS_TIMEOUT_MS,
});
};

View File

@@ -69,29 +69,53 @@ function resolve(slug, { config = FULL_CONFIG, effortArgs = ['--effort', 'high']
const FILE_REF = fileRefPrompt(`${RUN}/gsd-review-prompt.md`, ROOT);
/**
* Fixture/golden-contract DATA describing each shipped reviewer CLI tool's
* own real-world native timeout convention. NOT a real subprocess spawn
* timeout in this file's own execution -- each row is exercised only
* through the pure resolveLanePlan() function, never a real spawn here.
* Several lanes coincidentally share a value with another lane below;
* each still gets its own named constant since each describes an
* independently-configured external tool, not a shared internal class norm.
*/
const GEMINI_NATIVE_TIMEOUT_MS = 900000;
const CLAUDE_NATIVE_TIMEOUT_MS = 1200000;
const CODEX_NATIVE_TIMEOUT_MS = 1200000;
const CODERABBIT_NATIVE_TIMEOUT_MS = 360000;
const OPENCODE_NATIVE_TIMEOUT_MS = 660000;
const QWEN_NATIVE_TIMEOUT_MS = 900000;
const CURSOR_NATIVE_TIMEOUT_MS = 900000;
/**
* Mirrors a PRODUCTION `{{nativeTimeout}}` placeholder default resolved
* elsewhere in resolveLanePlan (#3274) -- this row's own comment states
* it proves the unconfigured default reproduces this exact literal.
*/
const ANTIGRAVITY_NATIVE_TIMEOUT_MS = 600000;
const KIMI_CODE_NATIVE_TIMEOUT_MS = 900000;
/**
* One row per shipped lane: the exact argv its bash leg produced, with a model configured and
* effort available. `stdin` is the prompt path for a stdin lane, `null` otherwise.
*/
const GOLDEN = [
{ slug: 'gemini', binary: 'gemini', argv: ['-m', 'G', '-p', '-'], stdin: true, out: 'stdout', timeout: 900000 },
{ slug: 'claude', binary: 'claude', argv: ['--model', 'C', '--effort', 'high', '-p', '-'], stdin: true, out: 'stdout', timeout: 1200000 },
{ slug: 'gemini', binary: 'gemini', argv: ['-m', 'G', '-p', '-'], stdin: true, out: 'stdout', timeout: GEMINI_NATIVE_TIMEOUT_MS },
{ slug: 'claude', binary: 'claude', argv: ['--model', 'C', '--effort', 'high', '-p', '-'], stdin: true, out: 'stdout', timeout: CLAUDE_NATIVE_TIMEOUT_MS },
{
slug: 'codex',
binary: 'codex',
// `exec` is a SUBCOMMAND and must stay first; the output file lands mid-argv and the bare `-`
// stays last. Splicing injected flags positionally produced an invalid invocation.
argv: ['exec', '--ephemeral', '--model', 'X', '--effort', 'high', '--skip-git-repo-check', '-o', `${RUN}/gsd-review-codex.md`, '-'],
stdin: true, out: 'file', timeout: 1200000,
stdin: true, out: 'file', timeout: CODEX_NATIVE_TIMEOUT_MS,
},
{ slug: 'coderabbit', binary: 'coderabbit', argv: ['review', '--prompt-only'], stdin: false, out: 'stdout', timeout: 360000 },
{ slug: 'opencode', binary: 'opencode', argv: ['run', '--model', 'O', '--effort', 'high', '--format', 'json', '-'], stdin: true, out: 'stdout', timeout: 660000 },
{ slug: 'qwen', binary: 'qwen', argv: ['-'], stdin: true, out: 'stdout', timeout: 900000 },
{ slug: 'cursor', binary: 'cursor-agent', argv: ['-p', '--model', 'U', '--mode', 'ask', '--trust', '--output-format', 'text', FILE_REF], stdin: false, out: 'stdout', timeout: 900000 },
{ slug: 'coderabbit', binary: 'coderabbit', argv: ['review', '--prompt-only'], stdin: false, out: 'stdout', timeout: CODERABBIT_NATIVE_TIMEOUT_MS },
{ slug: 'opencode', binary: 'opencode', argv: ['run', '--model', 'O', '--effort', 'high', '--format', 'json', '-'], stdin: true, out: 'stdout', timeout: OPENCODE_NATIVE_TIMEOUT_MS },
{ slug: 'qwen', binary: 'qwen', argv: ['-'], stdin: true, out: 'stdout', timeout: QWEN_NATIVE_TIMEOUT_MS },
{ slug: 'cursor', binary: 'cursor-agent', argv: ['-p', '--model', 'U', '--mode', 'ask', '--trust', '--output-format', 'text', FILE_REF], stdin: false, out: 'stdout', timeout: CURSOR_NATIVE_TIMEOUT_MS },
// resolveLanePlan fully resolves {{nativeTimeout}} itself (#3274) — this row proves the
// unconfigured default reproduces the original literal exactly.
{ slug: 'antigravity', binary: 'agy', argv: ['--print-timeout', '540s', '--model', 'A', '-p', FILE_REF], stdin: false, out: 'stdout', timeout: 600000 },
{ slug: 'kimi-code', binary: 'kimi', argv: ['-m', 'K', '-p', FILE_REF], stdin: false, out: 'stdout', timeout: 900000 },
{ slug: 'antigravity', binary: 'agy', argv: ['--print-timeout', '540s', '--model', 'A', '-p', FILE_REF], stdin: false, out: 'stdout', timeout: ANTIGRAVITY_NATIVE_TIMEOUT_MS },
{ slug: 'kimi-code', binary: 'kimi', argv: ['-m', 'K', '-p', FILE_REF], stdin: false, out: 'stdout', timeout: KIMI_CODE_NATIVE_TIMEOUT_MS },
];
describe('reviewer lane invocation — golden plans (the strangler-fig contract)', () => {
@@ -153,6 +177,11 @@ describe('#3274 — timeoutConfigKey resolves the outer wall-clock cap', () => {
test('a configured positive number overrides timeoutFloorMs, seconds -> ms (row 2)', () => {
const r = resolve('antigravity', { config: { [AGY_KEY]: 900 } });
// 900_000 here is the arithmetic result of this test's own input (900
// configured seconds * 1000), not a reuse of any lane's *_NATIVE_TIMEOUT_MS
// golden default -- it only coincidentally matches GEMINI/QWEN/CURSOR/
// KIMI_CODE_NATIVE_TIMEOUT_MS (all 900000). antigravity's own native
// default is ANTIGRAVITY_NATIVE_TIMEOUT_MS (600000), unrelated here.
assert.equal(r.plan.timeoutMs, 900_000);
});
@@ -243,6 +272,9 @@ describe('#3274 — timeoutConfigKey resolves the outer wall-clock cap', () => {
test('a configured antigravity timeout derives both the outer cap and the native flag (row 10)', () => {
const r = resolve('antigravity', { config: { [AGY_KEY]: 900 } });
// 900_000 is this test's own 900-configured-seconds * 1000, not a reuse
// of a *_NATIVE_TIMEOUT_MS golden constant -- see the row-2 test above
// for the same coincidental-match note.
assert.equal(r.plan.timeoutMs, 900_000);
const i = r.plan.argv.indexOf('--print-timeout');
assert.equal(r.plan.argv[i + 1], '840s');

View File

@@ -33,6 +33,17 @@ const path = require('node:path');
const fc = require('fast-check');
const { cleanup } = require('./helpers.cjs');
const { HTTP_REACHABLE_PROBE_TIMEOUT_FIXTURE_MS } = require('./helpers/timeouts.cjs');
/** NOT a subprocess spawn timeout. Fixture DATA: the default valid `timeoutMs` for a command-capability-kind probe. */
const COMMAND_CAPABILITY_PROBE_TIMEOUT_FIXTURE_MS = 3000;
/** NOT a subprocess spawn timeout. Fixture DATA: manifest probe.timeoutMs boundary set (this validator's own -1/0/1 triple is unrelated to, and coincidentally overlaps with, tests/plan-phase-stall-detection.test.cjs's runBashScript seam-level boundary triple -- disclosed, not merged; different validator, different file). */
const MANIFEST_PROBE_TIMEOUT_BOUNDARY_ZERO_MS = 0;
const MANIFEST_PROBE_TIMEOUT_BOUNDARY_ONE_MS = 1;
const MANIFEST_PROBE_TIMEOUT_BOUNDARY_NEGATIVE_MS = -1;
const MANIFEST_PROBE_TIMEOUT_BOUNDARY_NON_INTEGER_MS = 1.5;
/** NOT a subprocess spawn timeout. Fixture DATA: an arbitrary value proving a command-exists-kind probe rejects ANY timeoutMs field -- the specific number is not itself under test. */
const COMMAND_EXISTS_FORBIDDEN_TIMEOUT_MS = 500;
const {
LANE_SLUG_RE,
@@ -96,7 +107,7 @@ function validHttpLane() {
kind: 'http-reachable',
hostConfigKey: 'lmStudio.baseUrl',
path: '/v1/models',
timeoutMs: 2000,
timeoutMs: HTTP_REACHABLE_PROBE_TIMEOUT_FIXTURE_MS,
},
invoke: {
hostConfigKey: 'lmStudio.baseUrl',
@@ -778,7 +789,7 @@ describe('E. probe (D7) — bounded-probe control', () => {
// only coverage until Phase 5b ships one.
test('commandCapabilityProbeIsValidDespiteNoShippedLane', () => {
const lane = laneOverride((l) => {
l.probe = { kind: 'command-capability', binary: 'my-lane', needle: 'v1.2', timeoutMs: 3000 };
l.probe = { kind: 'command-capability', binary: 'my-lane', needle: 'v1.2', timeoutMs: COMMAND_CAPABILITY_PROBE_TIMEOUT_FIXTURE_MS };
});
const errs = validateReviewerBody({ id: 'x', reviewer: lane });
assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`);
@@ -786,7 +797,7 @@ describe('E. probe (D7) — bounded-probe control', () => {
test('httpReachableProbeIsValid', () => {
const lane = laneOverride((l) => {
l.probe = { kind: 'http-reachable', hostConfigKey: 'lmStudio.baseUrl', path: '/v1/models', timeoutMs: 2000 };
l.probe = { kind: 'http-reachable', hostConfigKey: 'lmStudio.baseUrl', path: '/v1/models', timeoutMs: HTTP_REACHABLE_PROBE_TIMEOUT_FIXTURE_MS };
});
const errs = validateReviewerBody({ id: 'x', reviewer: lane });
assert.deepEqual(errs, [], `expected no errors, got: ${JSON.stringify(errs)}`);
@@ -816,7 +827,7 @@ describe('E. probe (D7) — bounded-probe control', () => {
test('probeTimeoutZeroIsRejected', () => {
const lane = laneOverride((l) => {
l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: 0 };
l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: MANIFEST_PROBE_TIMEOUT_BOUNDARY_ZERO_MS };
});
const errs = validateReviewerBody({ id: 'x', reviewer: lane });
assert.ok(
@@ -827,7 +838,7 @@ describe('E. probe (D7) — bounded-probe control', () => {
test('probeTimeoutOneIsAccepted', () => {
const lane = laneOverride((l) => {
l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: 1 };
l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: MANIFEST_PROBE_TIMEOUT_BOUNDARY_ONE_MS };
});
const errs = validateReviewerBody({ id: 'x', reviewer: lane });
assert.deepEqual(errs, [], `expected no errors (boundary: limit), got: ${JSON.stringify(errs)}`);
@@ -835,7 +846,7 @@ describe('E. probe (D7) — bounded-probe control', () => {
test('probeNegativeTimeoutIsRejected', () => {
const lane = laneOverride((l) => {
l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: -1 };
l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: MANIFEST_PROBE_TIMEOUT_BOUNDARY_NEGATIVE_MS };
});
const errs = validateReviewerBody({ id: 'x', reviewer: lane });
assert.ok(
@@ -846,7 +857,7 @@ describe('E. probe (D7) — bounded-probe control', () => {
test('probeFractionalTimeoutIsRejected', () => {
const lane = laneOverride((l) => {
l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: 1.5 };
l.probe = { kind: 'http-reachable', hostConfigKey: 'x.y', path: '/z', timeoutMs: MANIFEST_PROBE_TIMEOUT_BOUNDARY_NON_INTEGER_MS };
});
const errs = validateReviewerBody({ id: 'x', reviewer: lane });
assert.ok(
@@ -895,7 +906,7 @@ describe('E. probe (D7) — bounded-probe control', () => {
test('commandExistsProbeRejectsTimeout', () => {
const lane = laneOverride((l) => {
l.probe = { kind: 'command-exists', binary: 'my-lane', timeoutMs: 500 };
l.probe = { kind: 'command-exists', binary: 'my-lane', timeoutMs: COMMAND_EXISTS_FORBIDDEN_TIMEOUT_MS };
});
const errs = validateReviewerBody({ id: 'x', reviewer: lane });
assert.ok(
@@ -915,7 +926,7 @@ describe('E. probe (D7) — bounded-probe control', () => {
test('commandCapabilityProbeRequiresNeedle', () => {
const lane = laneOverride((l) => {
l.probe = { kind: 'command-capability', binary: 'my-lane', timeoutMs: 3000 };
l.probe = { kind: 'command-capability', binary: 'my-lane', timeoutMs: COMMAND_CAPABILITY_PROBE_TIMEOUT_FIXTURE_MS };
});
const errs = validateReviewerBody({ id: 'x', reviewer: lane });
assert.ok(

View File

@@ -32,6 +32,7 @@ const path = require('node:path');
const fc = require('fast-check');
const { cleanup } = require('./helpers.cjs');
const { HTTP_REACHABLE_PROBE_TIMEOUT_FIXTURE_MS } = require('./helpers/timeouts.cjs');
const trust = require('../gsd-core/bin/lib/capability-trust.cjs');
@@ -94,7 +95,7 @@ function httpLaneManifest(mutator) {
kind: 'http-reachable',
hostConfigKey: 'lmStudio.baseUrl',
path: '/v1/models',
timeoutMs: 2000,
timeoutMs: HTTP_REACHABLE_PROBE_TIMEOUT_FIXTURE_MS,
},
invoke: {
hostConfigKey: 'lmStudio.baseUrl',