diff --git a/eslint-rules/no-adhoc-timeout-literal.allowlist.json b/eslint-rules/no-adhoc-timeout-literal.allowlist.json index 485e13b5a..3e0f5cb9a 100644 --- a/eslint-rules/no-adhoc-timeout-literal.allowlist.json +++ b/eslint-rules/no-adhoc-timeout-literal.allowlist.json @@ -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" ] diff --git a/tests/assumption-delta-checkpoint-e2e.test.cjs b/tests/assumption-delta-checkpoint-e2e.test.cjs index 18b55c0c7..abc3c3f05 100644 --- a/tests/assumption-delta-checkpoint-e2e.test.cjs +++ b/tests/assumption-delta-checkpoint-e2e.test.cjs @@ -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) { diff --git a/tests/assumption-delta.test.cjs b/tests/assumption-delta.test.cjs index 1a5e420a5..1f78d9b72 100644 --- a/tests/assumption-delta.test.cjs +++ b/tests/assumption-delta.test.cjs @@ -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); diff --git a/tests/code-review-pipeline-regression.test.cjs b/tests/code-review-pipeline-regression.test.cjs index 3f3ae474d..1781c3a3e 100644 --- a/tests/code-review-pipeline-regression.test.cjs +++ b/tests/code-review-pipeline-regression.test.cjs @@ -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}`); diff --git a/tests/code-review.test.cjs b/tests/code-review.test.cjs index 03b7cd118..e1ce88b05 100644 --- a/tests/code-review.test.cjs +++ b/tests/code-review.test.cjs @@ -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 }; } diff --git a/tests/helpers/timeouts.cjs b/tests/helpers/timeouts.cjs index 5b35c27dc..9da725e35 100644 --- a/tests/helpers/timeouts.cjs +++ b/tests/helpers/timeouts.cjs @@ -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, }; diff --git a/tests/plan-phase-stall-detection.test.cjs b/tests/plan-phase-stall-detection.test.cjs index 4e5c05e26..b5e34958f 100644 --- a/tests/plan-phase-stall-detection.test.cjs +++ b/tests/plan-phase-stall-detection.test.cjs @@ -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'); }); diff --git a/tests/plan-review-convergence.test.cjs b/tests/plan-review-convergence.test.cjs index 888b5554e..cb6aeebec 100644 --- a/tests/plan-review-convergence.test.cjs +++ b/tests/plan-review-convergence.test.cjs @@ -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 '\-\-'` 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, }); }; diff --git a/tests/review-lane-invocation.test.cjs b/tests/review-lane-invocation.test.cjs index d8bc9be93..06258d545 100644 --- a/tests/review-lane-invocation.test.cjs +++ b/tests/review-lane-invocation.test.cjs @@ -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'); diff --git a/tests/reviewer-manifest-body.test.cjs b/tests/reviewer-manifest-body.test.cjs index d1dab0182..69dfec1f8 100644 --- a/tests/reviewer-manifest-body.test.cjs +++ b/tests/reviewer-manifest-body.test.cjs @@ -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( diff --git a/tests/reviewer-trust-disclosure.test.cjs b/tests/reviewer-trust-disclosure.test.cjs index 7b3407fff..285b46021 100644 --- a/tests/reviewer-trust-disclosure.test.cjs +++ b/tests/reviewer-trust-disclosure.test.cjs @@ -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',