From ff5c5782ee4294a762d339138003385f19d7a023 Mon Sep 17 00:00:00 2001 From: sim Date: Tue, 8 Sep 2026 14:03:22 -0400 Subject: [PATCH] test(#4512): migrate process-seam batch to named timeout constants Batch 1 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/process-seam.test.cjs, tests/helpers-process-isolation.test.cjs, tests/run-with-timeout.test.cjs, and tests/helpers.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes the 4 files from the rule's allowlist. Adds SEAM_DEFAULT_TIMEOUT_MS to tests/helpers/timeouts.cjs (shared across 2 batch files, mirroring process-seam.cjs's own un-exported default). File-local constants elsewhere for values not shared across files or not a bench-derived class norm. No src/bin file touched, no numeric value changed anywhere. Co-Authored-By: Claude Sonnet 5 --- .../no-adhoc-timeout-literal.allowlist.json | 4 -- tests/helpers-process-isolation.test.cjs | 33 +++++++++--- tests/helpers.cjs | 3 +- tests/helpers/timeouts.cjs | 25 +++++++++ tests/process-seam.test.cjs | 53 +++++++++++++------ tests/run-with-timeout.test.cjs | 23 ++++++-- 6 files changed, 112 insertions(+), 29 deletions(-) diff --git a/eslint-rules/no-adhoc-timeout-literal.allowlist.json b/eslint-rules/no-adhoc-timeout-literal.allowlist.json index f09d1b4b2..e76ddb85d 100644 --- a/eslint-rules/no-adhoc-timeout-literal.allowlist.json +++ b/eslint-rules/no-adhoc-timeout-literal.allowlist.json @@ -56,8 +56,6 @@ "tests/gsd-validate-commit-crash-policy.test.cjs", "tests/gsd-write-guard.test.cjs", "tests/health-validation.test.cjs", - "tests/helpers-process-isolation.test.cjs", - "tests/helpers.cjs", "tests/hooks-commonjs-marker.test.cjs", "tests/hooks-crash-policy.test.cjs", "tests/init.test.cjs", @@ -93,7 +91,6 @@ "tests/plugin-manifest.test.cjs", "tests/policy-160-route0-resume.test.cjs", "tests/pr-branch-planning-filter.test.cjs", - "tests/process-seam.test.cjs", "tests/prohibition-enforcement.test.cjs", "tests/prompt-injection-scan.security.test.cjs", "tests/qa/tdd-walk.cjs", @@ -108,7 +105,6 @@ "tests/reviewer-manifest-body.test.cjs", "tests/reviewer-trust-disclosure.test.cjs", "tests/run-tests-temp-root.test.cjs", - "tests/run-with-timeout.test.cjs", "tests/secret-scan-lint.security.test.cjs", "tests/security-prompt-injection.security.test.cjs", "tests/security-scan.security.test.cjs", diff --git a/tests/helpers-process-isolation.test.cjs b/tests/helpers-process-isolation.test.cjs index 4fec68b84..c8a5a52cc 100644 --- a/tests/helpers-process-isolation.test.cjs +++ b/tests/helpers-process-isolation.test.cjs @@ -9,6 +9,18 @@ const { CONFIG_LOCATION_ENV_KEYS, scrubConfigLocationEnv, } = require('./helpers.cjs'); +const { INSTALL_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); + +/** + * A cold `node -e` require-probe in this file (not the shared + * `PROBE_TIMEOUT_MS` class — that constant is 15000ms and consolidating + * onto it would silently halve this file's pre-existing bound, a real + * behavior change a rename-only migration must not make). The comment at + * each use site already documents the observed duration as sub-second, so + * 30000ms leaves ~30x headroom. Pre-existing value, unchanged by this + * migration. + */ +const COLD_REQUIRE_PROBE_TIMEOUT_MS = 30000; describe('#2665: the built-lib require is deferred', () => { // The scrub set derives from gsd-core/bin/lib, which is BUILT. Requiring it at @@ -24,9 +36,9 @@ describe('#2665: the built-lib require is deferred', () => { "const needle = path.join('gsd-core', 'bin', 'lib', 'capability-registry.cjs');", 'process.stdout.write(String(Object.keys(require.cache).some((m) => m.endsWith(needle))));', ].join('\n'); - // Bounded per local/no-unbounded-spawn (#3143): a cold require is sub-second, - // so 30s is ~30x headroom and still fails loudly instead of hanging a lane. - const r = spawnSync(process.execPath, ['-e', src], { encoding: 'utf8', timeout: 30_000 }); + // Bounded per local/no-unbounded-spawn (#3143): a cold require is sub-second, so + // COLD_REQUIRE_PROBE_TIMEOUT_MS leaves ~30x headroom and fails loudly instead of hanging a lane. + const r = spawnSync(process.execPath, ['-e', src], { encoding: 'utf8', timeout: COLD_REQUIRE_PROBE_TIMEOUT_MS }); assert.strictEqual(r.status, 0, `probe failed: ${r.stderr}`); return r.stdout === 'true'; }; @@ -339,7 +351,8 @@ describe('#2665 round 4: the skillsHome walk is reversion-sensitive', () => { cwd: __dirname, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'], - timeout: 30_000, + // Same class as the probe above (#2665) — see COLD_REQUIRE_PROBE_TIMEOUT_MS's own comment. + timeout: COLD_REQUIRE_PROBE_TIMEOUT_MS, }); void out; // exit 0 is the assertion; execFileSync throws on nonzero }); @@ -406,7 +419,7 @@ describe('#3156: a raw installer spawn cannot write into the ambient HOME', () = encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'], env: installerEnv(), - timeout: 120_000, + timeout: INSTALL_TIMEOUT_MS, }); assert.ok(!fs.existsSync(path.join(canaryHome, '.gsd')), @@ -432,6 +445,14 @@ describe('#3712: a raw installer spawn cannot reach the ambient HOME\'s shared s const { cleanup } = require('./helpers.cjs'); const { installerEnv } = require('./helpers/install-shared.cjs'); const INSTALL_PATH = path.join(__dirname, '..', 'bin', 'install.js'); + /** + * A GLOBAL codex install additionally walks and copies the shared skills + * tree across the ambient-HOME boundary (#3712) on top of a normal + * install run, so it keeps a larger cap than INSTALL_TIMEOUT_MS. No fresh + * bench measurement justifies a new number here; the existing multiple is + * preserved rather than re-guessed on migration. + */ + const INSTALL_GLOBAL_TIMEOUT_MS = INSTALL_TIMEOUT_MS * 2.5; // #3156's canary asserts on /.gsd. That is not the only ambient-HOME // surface: a kind may declare a global `home` override resolved from os.homedir() @@ -480,7 +501,7 @@ describe('#3712: a raw installer spawn cannot reach the ambient HOME\'s shared s encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'], env: installerEnv(), - timeout: 300_000, + timeout: INSTALL_GLOBAL_TIMEOUT_MS, }); assert.strictEqual(inventory(), before, diff --git a/tests/helpers.cjs b/tests/helpers.cjs index 6f54da5a3..a55d96cd6 100644 --- a/tests/helpers.cjs +++ b/tests/helpers.cjs @@ -8,6 +8,7 @@ const os = require('os'); const path = require('path'); const { createFixture } = require('./fixtures/index.cjs'); const processSeam = require('./helpers/process-seam.cjs'); +const { SEAM_DEFAULT_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); const TOOLS_PATH = path.join(__dirname, '..', 'gsd-core', 'bin', 'gsd-tools.cjs'); @@ -226,7 +227,7 @@ function runGsdTools(args, cwd = process.cwd(), env = {}) { return processSeam.runNode([TOOLS_PATH, ...argv], { cwd, env: childEnv, - timeoutMs: 60000, + timeoutMs: SEAM_DEFAULT_TIMEOUT_MS, }); } diff --git a/tests/helpers/timeouts.cjs b/tests/helpers/timeouts.cjs index 6d6abbb7e..d7c491258 100644 --- a/tests/helpers/timeouts.cjs +++ b/tests/helpers/timeouts.cjs @@ -19,6 +19,12 @@ * tests/fragment-single-edit-propagation.install.test.cjs) keeps its own * local constant with its own justifying comment — do not force those * sites onto a shared value that doesn't describe them. + * + * One exception to "bench-derived class norm": `SEAM_DEFAULT_TIMEOUT_MS` + * below is a structural mirror of another module's un-exported default, + * not an independently bench-measured bound — see its own doc comment for + * why it still belongs here (shared by ≥2 call sites) rather than as a + * local constant. */ const { DEFAULT_GIT_TIMEOUT_MS, GIT_FIXTURE_TIMEOUT_MS } = require('./git-fixture.cjs'); @@ -86,6 +92,24 @@ const BUILD_TIMEOUT_MS = 30000; */ const INSTALL_TIMEOUT_MS = 120000; +/** + * Mirrors `tests/helpers/process-seam.cjs`'s own internal fallback for an + * omitted `timeoutMs` (#4512, batch 1 of the ad hoc timeout literal + * migration, epic #4445). That module does not export its fallback — + * exporting it is out of this batch's scope, since `process-seam.cjs` + * itself is not one of the batch's files — so this constant restates the + * SAME pre-existing number under a name, purely so call sites asserting + * parity with the seam's own fallback (rather than a measured class norm) + * have something to import instead of a bare literal. + * + * Deliberately a separate name from `HOOK_FANOUT_TIMEOUT_MS`, even though + * the two currently coincide: that constant is a bench-justified bound for + * a heavier, specific subprocess shape (nested shell fan-out), while this + * one is only "no unbounded path" — collapsing them would let a future, + * independent tune of either value silently move the other. + */ +const SEAM_DEFAULT_TIMEOUT_MS = 60000; + module.exports = { PROBE_TIMEOUT_MS, HOOK_FANOUT_TIMEOUT_MS, @@ -93,4 +117,5 @@ module.exports = { GIT_FIXTURE_TIMEOUT_MS, BUILD_TIMEOUT_MS, INSTALL_TIMEOUT_MS, + SEAM_DEFAULT_TIMEOUT_MS, }; diff --git a/tests/process-seam.test.cjs b/tests/process-seam.test.cjs index f0a1860c0..277ca5fbe 100644 --- a/tests/process-seam.test.cjs +++ b/tests/process-seam.test.cjs @@ -24,6 +24,29 @@ const path = require('node:path'); const { createTempDir, cleanup, runGsdTools, TOOLS_PATH } = require('./helpers.cjs'); const processSeam = require('./helpers/process-seam.cjs'); const { runNode, runGit, runHook, OUTCOME, toSeamResult } = processSeam; +const { + SEAM_DEFAULT_TIMEOUT_MS, +} = require('./helpers/timeouts.cjs'); + +/** + * `FIXTURE_SLEEPER` in this file sleeps 5000ms by default. Both constants + * below are comfortably under that, chosen only to reliably fire TIMED_OUT + * with margin — the exact numbers are not independently meaningful beyond + * "far under 5000ms", and are kept as two distinct pre-existing values (not + * consolidated to one) so this migration does not silently change any + * test's timing behavior. `SEAM_TIGHT_TIMEOUT_MS` (200) is the SMALLER / + * tighter of the two; `SEAM_SHORT_TIMEOUT_MS` (300) is the LARGER of the + * two — the names alone don't convey that ordering, hence spelling it out + * here. + */ +const SEAM_TIGHT_TIMEOUT_MS = 200; +const SEAM_SHORT_TIMEOUT_MS = 300; + +/** + * Generous headroom for a fixture in this file that exits quickly or + * synchronously (not exercising the timeout boundary itself). + */ +const SEAM_GENEROUS_TIMEOUT_MS = 5000; // ---- fixture sources ------------------------------------------------- @@ -155,7 +178,7 @@ describe('process-seam', () => { test('a child that overruns is TIMED_OUT as data, not a throw', () => { const fixture = writeFixture(tmpDir, 'sleeper.cjs', FIXTURE_SLEEPER); - const result = runNode([fixture, '5000'], { timeoutMs: 300 }); + const result = runNode([fixture, '5000'], { timeoutMs: SEAM_SHORT_TIMEOUT_MS }); assert.equal(result.outcome, OUTCOME.TIMED_OUT); assert.equal(result.timedOut, true); assert.equal(result.killed, true); @@ -164,7 +187,7 @@ describe('process-seam', () => { test('timedOut does not depend on signal presence (Windows)', () => { const fixture = writeFixture(tmpDir, 'sleeper.cjs', FIXTURE_SLEEPER); - const result = runNode([fixture, '5000'], { timeoutMs: 300 }); + const result = runNode([fixture, '5000'], { timeoutMs: SEAM_SHORT_TIMEOUT_MS }); // The assertion below is intentionally the whole point of this test: it // proves timedOut alone, without ever branching on result.signal. See // 40-design.md row 6 — signal is null on Windows and must not be @@ -175,7 +198,7 @@ describe('process-seam', () => { test('a timeout still returns string stdout/stderr (partial content is platform-dependent)', () => { const fixture = writeFixture(tmpDir, 'sleeper.cjs', FIXTURE_SLEEPER); const marker = JSON.stringify({ partial: true }); - const result = runNode([fixture, '5000', marker], { timeoutMs: 300 }); + const result = runNode([fixture, '5000', marker], { timeoutMs: SEAM_SHORT_TIMEOUT_MS }); assert.equal(result.outcome, OUTCOME.TIMED_OUT); assert.equal(result.timedOut, true); assert.equal(typeof result.stdout, 'string'); @@ -230,7 +253,7 @@ describe('process-seam', () => { test('omitting timeoutMs still bounds the call', () => { const fixture = writeFixture(tmpDir, 'exit.cjs', FIXTURE_EXIT); const withDefault = runNode([fixture, '0']); - const withExplicitDefault = runNode([fixture, '0'], { timeoutMs: 60000 }); + const withExplicitDefault = runNode([fixture, '0'], { timeoutMs: SEAM_DEFAULT_TIMEOUT_MS }); // Omitting timeoutMs must resolve to the same bounded code path as // explicitly passing the documented default — never a distinct // "unbounded" branch. @@ -242,14 +265,14 @@ describe('process-seam', () => { test('child finishing just under the bound is EXITED', () => { const fixture = writeFixture(tmpDir, 'sleeper.cjs', FIXTURE_SLEEPER); - const result = runNode([fixture, '50'], { timeoutMs: 5000 }); + const result = runNode([fixture, '50'], { timeoutMs: SEAM_GENEROUS_TIMEOUT_MS }); assert.equal(result.outcome, OUTCOME.EXITED); assert.equal(result.timedOut, false); }); test('at-the-bound child yields one deterministic outcome', () => { const fixture = writeFixture(tmpDir, 'sleeper.cjs', FIXTURE_SLEEPER); - const result = runNode([fixture, '300'], { timeoutMs: 300 }); + const result = runNode([fixture, '300'], { timeoutMs: SEAM_SHORT_TIMEOUT_MS }); // Either outcome is acceptable at the exact bound (OS/scheduler // jitter decides which side of the race wins) — what must never happen // is an outcome outside the pair, or fields inconsistent with whichever @@ -284,7 +307,7 @@ describe('process-seam', () => { test('child overrunning the bound is TIMED_OUT', () => { const fixture = writeFixture(tmpDir, 'sleeper.cjs', FIXTURE_SLEEPER); - const result = runNode([fixture, '5000'], { timeoutMs: 200 }); + const result = runNode([fixture, '5000'], { timeoutMs: SEAM_TIGHT_TIMEOUT_MS }); assert.equal(result.outcome, OUTCOME.TIMED_OUT); }); @@ -346,7 +369,7 @@ describe('process-seam', () => { test('a custom killSignal is reported, not normalized', () => { const fixture = writeFixture(tmpDir, 'sleeper.cjs', FIXTURE_SLEEPER); - const result = runNode([fixture, '5000'], { timeoutMs: 200, killSignal: 'SIGINT' }); + const result = runNode([fixture, '5000'], { timeoutMs: SEAM_TIGHT_TIMEOUT_MS, killSignal: 'SIGINT' }); assert.equal(result.outcome, OUTCOME.TIMED_OUT); if (process.platform !== 'win32') { assert.equal(result.signal, 'SIGINT'); @@ -355,7 +378,7 @@ describe('process-seam', () => { test('timeout with stderr reports one outcome, keeps both fields', () => { const fixture = writeFixture(tmpDir, 'sleeper.cjs', FIXTURE_SLEEPER); - const result = runNode([fixture, '5000', '', 'err-marker'], { timeoutMs: 300 }); + const result = runNode([fixture, '5000', '', 'err-marker'], { timeoutMs: SEAM_SHORT_TIMEOUT_MS }); assert.equal(result.outcome, OUTCOME.TIMED_OUT); assert.equal(typeof result.stdout, 'string'); assert.equal(typeof result.stderr, 'string'); @@ -377,8 +400,8 @@ describe('process-seam', () => { test('consecutive timeouts do not share state', () => { const fixture = writeFixture(tmpDir, 'sleeper.cjs', FIXTURE_SLEEPER); - const first = runNode([fixture, '5000'], { timeoutMs: 200 }); - const second = runNode([fixture, '5000'], { timeoutMs: 200 }); + const first = runNode([fixture, '5000'], { timeoutMs: SEAM_TIGHT_TIMEOUT_MS }); + const second = runNode([fixture, '5000'], { timeoutMs: SEAM_TIGHT_TIMEOUT_MS }); assert.equal(first.outcome, OUTCOME.TIMED_OUT); assert.equal(second.outcome, OUTCOME.TIMED_OUT); }); @@ -410,7 +433,7 @@ describe('process-seam', () => { return; } const fixture = writeFixture(tmpDir, 'suicide.cjs', FIXTURE_SUICIDE); - const result = runNode([fixture], { timeoutMs: 5000 }); + const result = runNode([fixture], { timeoutMs: SEAM_GENEROUS_TIMEOUT_MS }); assert.equal(result.outcome, OUTCOME.KILLED); assert.equal(result.killed, true); assert.equal(result.timedOut, false); @@ -432,7 +455,7 @@ describe('process-seam', () => { CLAUDE_CODE_SSE_PORT: '', CLAUDE_PROJECT_DIR: '', }; - const result = runHook(hookPath, [], { input: payload, env, timeoutMs: 5000 }); + const result = runHook(hookPath, [], { input: payload, env, timeoutMs: SEAM_GENEROUS_TIMEOUT_MS }); assert.equal(result.outcome, OUTCOME.EXITED); assert.equal(typeof result.stdout, 'string'); }); @@ -492,7 +515,7 @@ describe('runHook interpreter option', () => { // unexpected string-valued option alongside a valid timeoutMs; the // seam's contract-validation for timeoutMs must still pass through // untouched and the call must complete without throwing. - const result = runHook(hookPath, ['0'], { interpreter: process.execPath, timeoutMs: 5000 }); + const result = runHook(hookPath, ['0'], { interpreter: process.execPath, timeoutMs: SEAM_GENEROUS_TIMEOUT_MS }); assert.equal(result.outcome, OUTCOME.EXITED); assert.equal(result.exitCode, 0); }); @@ -510,7 +533,7 @@ describe('runHook interpreter option', () => { 'sleep 5', ].join('\n') ); - const result = runHook(scriptPath, [], { interpreter: 'bash', timeoutMs: 300 }); + const result = runHook(scriptPath, [], { interpreter: 'bash', timeoutMs: SEAM_SHORT_TIMEOUT_MS }); assert.equal(result.outcome, OUTCOME.TIMED_OUT); assert.equal(result.timedOut, true); assert.equal(result.exitCode, null); diff --git a/tests/run-with-timeout.test.cjs b/tests/run-with-timeout.test.cjs index 4a008d32b..37f32bfa6 100644 --- a/tests/run-with-timeout.test.cjs +++ b/tests/run-with-timeout.test.cjs @@ -25,6 +25,23 @@ const fs = require('node:fs'); const { setTimeout: sleep } = require('node:timers/promises'); const { createTempDir, cleanup } = require('./helpers.cjs'); +/** + * Backstop for spawnSync's OWN hard-kill, guarding against run-with-timeout's + * internal wall-clock cap failing to fire. Not a measured probe duration — + * this suite's verb-internal budgets top out at 10s (Windows .cmd/.bat + * mediation tests), so this preserves ~3x headroom over that ceiling, the + * pre-existing value unchanged by this migration. + */ +const RUN_WITH_TIMEOUT_HARNESS_BACKSTOP_MS = 30000; + +/** + * Tighter backstop for the C1 reap test specifically: its own verb-internal + * budget is 3s plus ~900ms of heartbeat-settle waits, so this keeps a + * smaller margin than the file default while still comfortably covering it. + * Pre-existing value, unchanged. + */ +const RUN_WITH_TIMEOUT_C1_REAP_BACKSTOP_MS = 20000; + const ROOT = path.join(__dirname, '..'); const GSD_TOOLS = path.join(ROOT, 'gsd-core', 'bin', 'gsd-tools.cjs'); const NODE = process.execPath; @@ -35,7 +52,7 @@ function runVerb(args, opts = {}) { return spawnSync(NODE, [GSD_TOOLS, 'run-with-timeout', ...args], { cwd: os.tmpdir(), encoding: 'utf8', - timeout: 30000, // test-harness backstop; the verb's own cap is what we assert + timeout: RUN_WITH_TIMEOUT_HARNESS_BACKSTOP_MS, // test-harness backstop; the verb's own cap is what we assert ...opts, }); } @@ -139,7 +156,7 @@ describe('#2351 run-with-timeout — argument handling (negative matrix)', () => test('the `query` meta-prefix form is accepted', () => { const r = spawnSync(NODE, [GSD_TOOLS, 'query', 'run-with-timeout', '5', '--', ...OK], { - cwd: os.tmpdir(), encoding: 'utf8', timeout: 30000, + cwd: os.tmpdir(), encoding: 'utf8', timeout: RUN_WITH_TIMEOUT_HARNESS_BACKSTOP_MS, }); assert.equal(r.status, 0); }); @@ -183,7 +200,7 @@ describe('#2351 run-with-timeout — kill semantics (POSIX process groups)', () 'process.on("SIGTERM", () => process.exit(0));', 'setInterval(() => {}, 1000);', ].join('\n')); - const r = runVerb(['3', '--', NODE, parentFile, hbFile], { timeout: 20000 }); + const r = runVerb(['3', '--', NODE, parentFile, hbFile], { timeout: RUN_WITH_TIMEOUT_C1_REAP_BACKSTOP_MS }); assert.equal(r.status, 124, 'must report a timeout (124), not hang'); assert.ok(fs.existsSync(hbFile), 'child heartbeat should exist'); await sleep(300); // let any in-flight write settle after the SIGKILL