diff --git a/.changeset/quick-panthers-cross.md b/.changeset/quick-panthers-cross.md new file mode 100644 index 000000000..a7f9d7dfa --- /dev/null +++ b/.changeset/quick-panthers-cross.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4575 +--- +**The worktree-path guard no longer fails open under CI/process load** — it combined three sequential `git` subprocess spawns into one, cutting the worktree-escape check's worst-case latency so a busy runner can no longer push the guard past its own timeout into a silent allow. (#4515) diff --git a/eslint-rules/no-adhoc-timeout-literal.allowlist.json b/eslint-rules/no-adhoc-timeout-literal.allowlist.json index 58fd17217..00acade09 100644 --- a/eslint-rules/no-adhoc-timeout-literal.allowlist.json +++ b/eslint-rules/no-adhoc-timeout-literal.allowlist.json @@ -34,10 +34,8 @@ "tests/emitted-ack-trailer.test.cjs", "tests/emitted-attribution.test.cjs", "tests/execute-wave-post-gate-pipeline-e2e.test.cjs", - "tests/faulty-deps.test.cjs", "tests/feat-2483-review-claude-mds-guard.test.cjs", "tests/federated-config.test.cjs", - "tests/fragment-single-edit-propagation.install.test.cjs", "tests/gate-predicate-evaluator.test.cjs", "tests/gemini-runtime-removed.test.cjs", "tests/gen-context-index.test.cjs", @@ -54,11 +52,6 @@ "tests/hooks-commonjs-marker.test.cjs", "tests/hooks-crash-policy.test.cjs", "tests/init.test.cjs", - "tests/install-minimal-hooks.test.cjs", - "tests/install-regressions.test.cjs", - "tests/install-runtime-artifacts.test.cjs", - "tests/install-write-confinement.test.cjs", - "tests/install.test.cjs", "tests/kilo-upgrades.test.cjs", "tests/kimi-upgrades.test.cjs", "tests/kimi-variant-disambiguation.test.cjs", @@ -71,9 +64,7 @@ "tests/loop-walk.qa.test.cjs", "tests/milestone-lock.test.cjs", "tests/no-pending-3212-markers.test.cjs", - "tests/npm-integrity-gate.test.cjs", "tests/opencode-plugin-adapter.test.cjs", - "tests/packaging-shipped-scripts-require-only-shipped.test.cjs", "tests/pattern.test.cjs", "tests/perf-316-state-lock-buffer-alloc.test.cjs", "tests/perf-317-context-monitor-fs.test.cjs", @@ -83,7 +74,6 @@ "tests/plan-phase-stall-detection.test.cjs", "tests/plan-pre-hook-e2e.test.cjs", "tests/plan-review-convergence.test.cjs", - "tests/plugin-manifest.test.cjs", "tests/policy-160-route0-resume.test.cjs", "tests/prohibition-enforcement.test.cjs", "tests/prompt-injection-scan.security.test.cjs", @@ -92,7 +82,6 @@ "tests/read-guard.test.cjs", "tests/read-injection-scanner.property.test.cjs", "tests/read-injection-scanner.security.test.cjs", - "tests/release-tarball-smoke.install.test.cjs", "tests/representative-corpus.test.cjs", "tests/review-lane-invocation.test.cjs", "tests/reviewer-manifest-body.test.cjs", diff --git a/hooks/gsd-worktree-path-guard.js b/hooks/gsd-worktree-path-guard.js index 691577900..186c02f5e 100644 --- a/hooks/gsd-worktree-path-guard.js +++ b/hooks/gsd-worktree-path-guard.js @@ -163,17 +163,37 @@ process.stdin.on('end', () => { // returns a path containing .git/worktrees/ as a component. // In the main repo or a submodule it returns .git (or a path without /worktrees/). // This approach works even when cwd is a subdirectory of the worktree. - const gitDirResult = git(['rev-parse', '--git-dir'], cwd); + // Combined into one spawn — git rev-parse accepts multiple query flags in + // one invocation and prints one line of output per flag, in the exact + // order given, reducing this guard's worst-case subprocess count under + // CI/load contention (three spawns collapse into one). `--abbrev-ref HEAD` + // is used instead of `symbolic-ref --short HEAD` because it is combinable + // (a single `rev-parse` call) and behaviorally equivalent for this guard's + // branch-acceptance check, including on detached HEAD: `--abbrev-ref` + // returns the literal string `HEAD` there (exit 0), which the acceptance + // regex below also rejects — the same guard outcome as symbolic-ref's + // exit-128/empty-stdout failure. Do not change any timeout value as part + // of this change, only the spawn count. + const combinedResult = git(['rev-parse', '--git-dir', '--abbrev-ref', 'HEAD', '--show-toplevel'], cwd); // #3911: a timeout/spawn-failure result is indistinguishable from a clean // "not a git repo" answer by status/stdout alone — reportIfUndetermined // is a no-op on a genuine negative and only fires the diagnostic when the // probe itself could not run. The allow() below is UNCHANGED either way. - reportIfUndetermined('gsd-worktree-path-guard', 'git rev-parse --git-dir', gitDirResult); - if (gitDirResult.status !== 0 || !gitDirResult.stdout) { + reportIfUndetermined( + 'gsd-worktree-path-guard', + 'git rev-parse --git-dir --abbrev-ref HEAD --show-toplevel', + combinedResult + ); + if (combinedResult.status !== 0 || !combinedResult.stdout) { allow(undefined); // not a git repo — pass through } - const gitDir = gitDirResult.stdout.trim(); + const combinedLines = combinedResult.stdout.split('\n').map((l) => l.trim()).filter((l) => l.length > 0); + if (combinedLines.length < 3) { + allow(undefined); // malformed/short output — can't determine root, fail open + } + const [gitDir, branch, wtTopRaw] = combinedLines; + // A linked worktree's --git-dir contains .git/worktrees/ as a path component const isLinkedWorktree = /[/\\]\.git[/\\]worktrees[/\\]/.test(gitDir); if (!isLinkedWorktree) { @@ -186,23 +206,14 @@ process.stdin.on('end', () => { // created linked worktree (plain non-GSD work, e.g. Claude Code plan-mode) is // on the user's own branch, so the guard must be a no-op there. Detached HEAD // / error → not GSD-managed → no-op. - const branchResult = git(['symbolic-ref', '--short', 'HEAD'], cwd); - reportIfUndetermined('gsd-worktree-path-guard', 'git symbolic-ref --short HEAD', branchResult); - const branch = branchResult.status === 0 && branchResult.stdout ? branchResult.stdout.trim() : ''; // #3021: accept worktree-wf_- branches (Workflow backend's naming). if (!/^((worktree-)?agent-|worktree-wf_)[A-Za-z0-9._/-]+$/.test(branch)) { allow(undefined); // not a GSD-managed executor worktree — no-op } - // Get the raw --show-toplevel output for the worktree (cwd). + // wtTopRaw: the raw --show-toplevel output for the worktree (cwd). // We keep it raw (not path.resolve'd) to compare directly with the // file's toplevel — same git binary, same format, no normalization needed. - const wtTopResult = git(['rev-parse', '--show-toplevel'], cwd); - reportIfUndetermined('gsd-worktree-path-guard', 'git rev-parse --show-toplevel (worktree cwd)', wtTopResult); - if (wtTopResult.status !== 0 || !wtTopResult.stdout) { - allow(undefined); // can't determine root — fail open - } - const wtTopRaw = wtTopResult.stdout.trim(); // #2595 (review Major 3): read the field TYPED. `?.file_path || ''` let a // non-string through — `[]` and `{}` are truthy, so they survived the diff --git a/tests/faulty-deps.test.cjs b/tests/faulty-deps.test.cjs index f6728404f..04b5ec75c 100644 --- a/tests/faulty-deps.test.cjs +++ b/tests/faulty-deps.test.cjs @@ -33,6 +33,17 @@ const { defaultPhaseCleanCommitTimesMs } = require(path.join(__dirname, '..', 'g const { createTempGitProject, createTempDir, cleanup } = require('./helpers.cjs'); const { makeFaultyGit, withFaultyFs } = require('./helpers/faulty-deps.cjs'); +// A bound so small process creation cannot complete inside it, making the +// ETIMEDOUT kill path deterministic on a real (unmocked) git call; used at +// both sentinel sites in this file. +const DETERMINISTIC_TIMEOUT_SENTINEL_MS = 1; + +// Bounds a real (unmocked) `git var GIT_EDITOR` call proving env passthrough; +// deliberately not GIT_TIMEOUT_MS (15000ms) since this site's pre-existing +// value differs and this migration never raises a bound without a fresh +// bench citation. +const GIT_VAR_PROBE_TIMEOUT_MS = 5000; + // ─── A. execGit normalization (#3071) ────────────────────────────────────── describe('A. execGit normalization (#3071)', () => { @@ -71,7 +82,7 @@ describe('A. execGit normalization (#3071)', () => { // complete inside 1ms, so the kill path is deterministic, matching the // existing "wall-clock timeout" pattern used for dispatchGsdCommand in // shell-command-projection-dispatch.test.cjs. - const result = execGit(['status', '--porcelain'], { cwd: tmpDir, timeout: 1 }); + const result = execGit(['status', '--porcelain'], { cwd: tmpDir, timeout: DETERMINISTIC_TIMEOUT_SENTINEL_MS }); assert.strictEqual(result.timedOut, true); assert.strictEqual(result.error && result.error.code, 'ETIMEDOUT'); }); @@ -117,7 +128,7 @@ describe('A. execGit normalization (#3071)', () => { const result = execGit(['var', 'GIT_EDITOR'], { cwd: tmpDir, env: { GIT_EDITOR: 'fault-3056-sentinel-editor' }, - timeout: 5000, + timeout: GIT_VAR_PROBE_TIMEOUT_MS, }); assert.strictEqual(result.exitCode, 0); assert.strictEqual(result.stdout, 'fault-3056-sentinel-editor'); @@ -128,7 +139,7 @@ describe('A. execGit normalization (#3071)', () => { t.after(() => cleanup(tmpDir)); const success = execGit(['status', '--porcelain'], { cwd: tmpDir }); const enoent = execTool('definitely-not-a-real-program-fault-3056', []); - const timeout = execGit(['status', '--porcelain'], { cwd: tmpDir, timeout: 1 }); + const timeout = execGit(['status', '--porcelain'], { cwd: tmpDir, timeout: DETERMINISTIC_TIMEOUT_SENTINEL_MS }); assert.strictEqual(typeof success.exitCode, 'number'); assert.strictEqual(typeof enoent.exitCode, 'number'); assert.strictEqual(typeof timeout.exitCode, 'number'); diff --git a/tests/fragment-single-edit-propagation.install.test.cjs b/tests/fragment-single-edit-propagation.install.test.cjs index 44e73274f..5442e0903 100644 --- a/tests/fragment-single-edit-propagation.install.test.cjs +++ b/tests/fragment-single-edit-propagation.install.test.cjs @@ -60,6 +60,7 @@ const { gitOrThrow } = require('./helpers/git-fixture.cjs'); const { cleanup, createTempDir, readFileNormalized } = require('./helpers.cjs'); const { RUNTIME_META, installerEnv } = require('./helpers/install-shared.cjs'); +const { INSTALL_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); const { buildOverlayRepo, REPO_ROOT, @@ -199,6 +200,37 @@ const ORIGINAL_WORKFLOW_CONTENT = readFileNormalized(PILOT_WORKFLOW_PATH); const FRAGMENT_SENTINEL = 'GSD-2933-FRAGMENT-EDIT-SENTINEL-4c1a9f'; +/** + * Bounds `runOverlayCheck`/`runOverlayGenerate` — a single overlay + * `scripts/gen-*.cjs` generator invocation (never the installer proper). + * The digits coincide with `INSTALL_TIMEOUT_MS` (also 120000ms), but this + * is a different operation class kept as its own local constant — + * coincidence, not collision. + */ +const GENERATOR_SCRIPT_TIMEOUT_MS = 120000; + +/** + * Bounds `trackedFileSet`'s `git ls-files` over the FULL overlay tree. + * Deliberately NOT the shared `GIT_TIMEOUT_MS` (15000ms, `helpers/timeouts.cjs`), + * which describes small-fixture-repo git plumbing — a much lighter shape + * than scanning this file's overlay tree. + */ +const TRACKED_FILE_SET_TIMEOUT_MS = 120000; + +/** + * Bounds the real `npm run regen:derived` spawn (row 21) — a full build + * plus eight generators, the single heaviest subprocess in this suite. + * 300000 (5min) was observed to be killed (status: null) near the very end + * of a genuinely completed run on a loaded bench (linux-node22), not from a + * real hang. 900000 (15min) is deliberately generous so this can never + * again flake on load while still catching a true hang. + * allow-spawn-timeout-ceiling: regen:derived is a full build plus eight + * generators; 300000 was observed killing a genuinely-completed run near + * the end on a loaded bench, not a real hang, so 900000 is deliberately + * above the 600000 ceiling to never repeat that flake. + */ +const REGEN_DERIVED_TIMEOUT_MS = 900000; + // ─── Helpers ──────────────────────────────────────────────────────────────── /** @@ -226,7 +258,7 @@ function installOverlay(overlayRoot, runtime, extraArgs = []) { const result = runNode(args, { cwd: root, env: installerEnv({ HOME: root, USERPROFILE: root }), - timeoutMs: 120000, + timeoutMs: INSTALL_TIMEOUT_MS, }); result.status = result.exitCode; assert.equal( @@ -262,7 +294,7 @@ function installOverlayExpectingFailure(overlayRoot, runtime, extraArgs = []) { const result = runNode(args, { cwd: root, env: installerEnv({ HOME: root, USERPROFILE: root }), - timeoutMs: 120000, + timeoutMs: INSTALL_TIMEOUT_MS, }); result.status = result.exitCode; return { configDir: root, root, result }; @@ -281,7 +313,7 @@ function runOverlayCheck(overlayRoot, scriptRelPath, extraArgs = []) { const result = runNode([scriptPath, '--check', ...extraArgs], { cwd: overlayRoot, env: installerEnv(), - timeoutMs: 120000, + timeoutMs: GENERATOR_SCRIPT_TIMEOUT_MS, }); result.status = result.exitCode; return result; @@ -299,7 +331,7 @@ function runOverlayGenerate(overlayRoot, scriptRelPath) { const result = runNode([scriptPath], { cwd: overlayRoot, env: installerEnv(), - timeoutMs: 120000, + timeoutMs: GENERATOR_SCRIPT_TIMEOUT_MS, }); result.status = result.exitCode; return result; @@ -327,7 +359,7 @@ function runOverlayGenerate(overlayRoot, scriptRelPath) { function trackedFileSet(repoRoot) { const stdout = gitOrThrow(['-c', 'safe.directory=*', 'ls-files'], { cwd: repoRoot, - timeoutMs: 120000, + timeoutMs: TRACKED_FILE_SET_TIMEOUT_MS, }); return stdout.split('\n').map((line) => line.trim()).filter(Boolean); } @@ -1198,17 +1230,9 @@ test('regenDerivedPropagatesSingleFragmentEditWithNoSecondSourceSurface', { cwd: overlay, encoding: 'utf8', env: installerEnv(), - // `regen:derived` chains a full `npm run build` plus eight generators — - // the single heaviest subprocess in this suite. 300_000 (5min) was - // observed to be killed (status: null) near the very end of a genuinely - // completed run on a loaded bench (linux-node22), not from a real hang. - // 900_000 (15min) is deliberately generous so this can never again flake - // on load while still catching a true hang. - // allow-spawn-timeout-ceiling: regen:derived is a full build plus eight - // generators; 300_000 was observed killing a genuinely-completed run - // near the end on a loaded bench, not a real hang, so 900_000 is - // deliberately above the 600000 ceiling to never repeat that flake. - timeout: 900000, + // See REGEN_DERIVED_TIMEOUT_MS's own doc comment (top of file) for the + // full rationale, including the allow-spawn-timeout-ceiling annotation. + timeout: REGEN_DERIVED_TIMEOUT_MS, maxBuffer: 64 * 1024 * 1024, }); assert.equal( diff --git a/tests/helpers/timeouts.cjs b/tests/helpers/timeouts.cjs index fb39959b7..f5b1b40fa 100644 --- a/tests/helpers/timeouts.cjs +++ b/tests/helpers/timeouts.cjs @@ -127,6 +127,20 @@ const SEAM_DEFAULT_TIMEOUT_MS = 60000; */ const QUICK_SPAWN_TIMEOUT_MS = 10000; +/** + * NOT a subprocess spawn timeout. This is fixture DATA -- the numeric value + * placed inside a fixture JSON object that mimics a Claude Code + * `settings.json` hook-entry's own `timeout` field, whose schema expresses + * that field in SECONDS. Do not pass this into a `spawnSync`/`execFileSync` + * options object -- every other constant in this file is milliseconds, this + * one is not. + * + * Shared across >=2 files in batch #4515 of the ad hoc timeout literal + * migration, epic #4445 -- that is why it lives here rather than as a + * file-local constant. + */ +const FIXTURE_HOOK_TIMEOUT_SECONDS = 5; + module.exports = { PROBE_TIMEOUT_MS, HOOK_FANOUT_TIMEOUT_MS, @@ -136,4 +150,5 @@ module.exports = { INSTALL_TIMEOUT_MS, SEAM_DEFAULT_TIMEOUT_MS, QUICK_SPAWN_TIMEOUT_MS, + FIXTURE_HOOK_TIMEOUT_SECONDS, }; diff --git a/tests/install-minimal-hooks.test.cjs b/tests/install-minimal-hooks.test.cjs index 080c6a323..1fd8cbf83 100644 --- a/tests/install-minimal-hooks.test.cjs +++ b/tests/install-minimal-hooks.test.cjs @@ -29,6 +29,21 @@ const os = require('node:os'); const { runNode } = require('./helpers/process-seam.cjs'); const { throwIfFailed } = require('./helpers/git-fixture.cjs'); +const { + PROBE_TIMEOUT_MS, + INSTALL_TIMEOUT_MS, + FIXTURE_HOOK_TIMEOUT_SECONDS, +} = require('./helpers/timeouts.cjs'); + +/** + * Bounds executing a single already-staged hook script directly (not the + * installer itself, just the emitted script under a real node process). + * 30000ms digits coincide with the shared BUILD_TIMEOUT_MS, but this is a + * different operation class (running staged output vs. bundling it), so it + * is kept as its own local constant rather than aliased onto that norm. + */ +const INSTALLED_HOOK_EXEC_TIMEOUT_MS = 30000; + const { createTempDir, cleanup } = require('./helpers.cjs'); const { @@ -115,7 +130,7 @@ describe('install-profiles: MINIMAL_SKILL_ALLOWLIST', () => { describe('install: --help profile counts match PROFILES (#834)', () => { function helpText() { - const r = runNode([INSTALL_SCRIPT, '--help'], { env: installerEnv(), timeoutMs: 15000 }); + const r = runNode([INSTALL_SCRIPT, '--help'], { env: installerEnv(), timeoutMs: PROBE_TIMEOUT_MS }); throwIfFailed(r, `node ${INSTALL_SCRIPT} --help`); return r.stdout; } @@ -410,7 +425,7 @@ function sharedMinimalManifestInstall() { const targetDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-minimal-shared-')); runNode( [INSTALL_SCRIPT, '--claude', '--global', '--config-dir', targetDir, '--minimal'], - { env: installerEnv(), timeoutMs: 120000 }, + { env: installerEnv(), timeoutMs: INSTALL_TIMEOUT_MS }, ); const manifestPath = path.join(targetDir, MANIFEST_NAME); const m = fs.existsSync(manifestPath) ? JSON.parse(fs.readFileSync(manifestPath, 'utf8')) : {}; @@ -433,7 +448,7 @@ describe('install: manifest records mode for both profiles', () => { try { runNode( [INSTALL_SCRIPT, '--claude', '--global', '--config-dir', targetDir, ...extraArgs], - { env: installerEnv(), timeoutMs: 120000 }, + { env: installerEnv(), timeoutMs: INSTALL_TIMEOUT_MS }, ); const manifestPath = path.join(targetDir, MANIFEST_NAME); if (!fs.existsSync(manifestPath)) return { mode: '', skillCount: 0, agentCount: 0 }; @@ -486,7 +501,7 @@ describe('install-minimal-backcompat: --minimal and --profile=core produce same try { runNode( [INSTALL_SCRIPT, '--claude', '--global', '--config-dir', targetDir, ...extraArgs], - { env: installerEnv(), timeoutMs: 120000 }, + { env: installerEnv(), timeoutMs: INSTALL_TIMEOUT_MS }, ); const manifestPath = path.join(targetDir, MANIFEST_NAME); if (!fs.existsSync(manifestPath)) return { mode: null, skillCount: 0, profileMarker: null }; @@ -562,7 +577,7 @@ describe('install: Codex full → minimal downgrade cleans stale agent state', ( // config.toml (both under targetDir), so the sandbox has no effect on intent. const result = runNode( [INSTALL_SCRIPT, '--codex', '--global', '--config-dir', targetDir, '--minimal'], - { env: installerEnv({ HOME: targetDir, USERPROFILE: targetDir }), timeoutMs: 120000 }, + { env: installerEnv({ HOME: targetDir, USERPROFILE: targetDir }), timeoutMs: INSTALL_TIMEOUT_MS }, ); assert.ok(result.stdout || result.stderr); @@ -599,7 +614,7 @@ describe('install: Claude full → minimal downgrade removes stale agents', () = runNode( [INSTALL_SCRIPT, '--claude', '--global', '--config-dir', targetDir, '--minimal'], - { env: installerEnv(), timeoutMs: 120000 }, + { env: installerEnv(), timeoutMs: INSTALL_TIMEOUT_MS }, ); const remaining = fs.existsSync(agentsDir) ? fs.readdirSync(agentsDir) : []; @@ -691,7 +706,7 @@ describe('#1821/#2305: ZCode receives no dead hook files; Kilo/OpenCode/Claude k try { const result = runNode( [INSTALL_SCRIPT, `--${runtime}`, '--global', '--config-dir', targetDir], - { env: installerEnv(), timeoutMs: 120000 }, + { env: installerEnv(), timeoutMs: INSTALL_TIMEOUT_MS }, ); assert.strictEqual(result.exitCode, 0, `installer exited with status ${result.exitCode} for --${runtime} --global\nstdout: ${result.stdout}\nstderr: ${result.stderr}`); @@ -2851,7 +2866,7 @@ function runInstaller(configDir) { // in install-smoke.yml). throwIfFailed( runNode([INSTALL_SCRIPT, '--claude', '--global', '--yes', '--no-sdk'], { - timeoutMs: 120000, + timeoutMs: INSTALL_TIMEOUT_MS, env: { ...process.env, CLAUDE_CONFIG_DIR: configDir, @@ -2970,7 +2985,7 @@ describe('#4087 regression: Codex install stages the hook helpers its hooks requ // child's env. throwIfFailed( runNode([INSTALL_SCRIPT, '--codex', '--global', '--yes', '--no-sdk', '--config-dir', configDir], { - timeoutMs: 120000, + timeoutMs: INSTALL_TIMEOUT_MS, env: { ...process.env, HOME: configDir, USERPROFILE: configDir }, }), `node ${INSTALL_SCRIPT} --codex --global --config-dir ${configDir}`, @@ -3224,7 +3239,7 @@ describe('#4087 review: Windsurf install stages the hook helpers its hooks requi // HOME/USERPROFILE sandboxed for the CHILD, for the same reason as installCodex. throwIfFailed( runNode([INSTALL_SCRIPT, '--windsurf', '--global', '--yes', '--config-dir', configDir], { - timeoutMs: 120000, + timeoutMs: INSTALL_TIMEOUT_MS, env: { ...process.env, HOME: configDir, USERPROFILE: configDir }, }), `node ${INSTALL_SCRIPT} --windsurf --global --config-dir ${configDir}`, @@ -3238,7 +3253,7 @@ describe('#4087 review: Windsurf install stages the hook helpers its hooks requi for (const script of ['gsd-windsurf-pre-write.js', 'gsd-windsurf-pre-command.js']) { const hook = path.join(hooksDir, script); assert.ok(fs.existsSync(hook), `precondition: ${script} must be staged`); - const result = runNode([hook], { timeoutMs: 30000, input: '{}', env: { ...process.env } }); + const result = runNode([hook], { timeoutMs: INSTALLED_HOOK_EXEC_TIMEOUT_MS, input: '{}', env: { ...process.env } }); assert.strictEqual(result.outcome, 'exited', `${script} must run to completion, not time out or be killed. outcome=${result.outcome}`); assert.strictEqual(result.exitCode, 0, @@ -3391,7 +3406,7 @@ describe('bug #3981: blocking-guard timeout budget + migration', () => { hooks: { PreToolUse: [{ matcher: 'Write|Edit', - hooks: [{ type: 'command', command: `node ${path.join(targetDir, 'hooks', guard)}`, timeout: 5 }], + hooks: [{ type: 'command', command: `node ${path.join(targetDir, 'hooks', guard)}`, timeout: FIXTURE_HOOK_TIMEOUT_SECONDS }], }], }, }; @@ -3423,7 +3438,7 @@ describe('bug #3981: blocking-guard timeout budget + migration', () => { }); test('non-managed timeout:5 entries are left alone (#3981)', () => { - const mine = { type: 'command', command: 'node /usr/local/bin/my-own-hook.js', timeout: 5 }; + const mine = { type: 'command', command: 'node /usr/local/bin/my-own-hook.js', timeout: FIXTURE_HOOK_TIMEOUT_SECONDS }; const settings = { hooks: { PreToolUse: [{ matcher: 'Write', hooks: [mine] }] } }; const localCmd = (hookFile) => `node ${path.join(targetDir, 'hooks', hookFile)}`; captureConsole(() => { diff --git a/tests/install-regressions.test.cjs b/tests/install-regressions.test.cjs index 54db5a12a..6fb7d39dc 100644 --- a/tests/install-regressions.test.cjs +++ b/tests/install-regressions.test.cjs @@ -21,7 +21,12 @@ const fs = require('node:fs'); const path = require('node:path'); const { runNode } = require('./helpers/process-seam.cjs'); const { throwIfFailed } = require('./helpers/git-fixture.cjs'); -const { INSTALL_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const { INSTALL_TIMEOUT_MS, FIXTURE_HOOK_TIMEOUT_SECONDS } = require('./helpers/timeouts.cjs'); + +// Bounds a scoped/targeted runNode([INSTALL_SCRIPT, ...]) invocation — a +// lighter shape than the shared full-install INSTALL_TIMEOUT_MS class +// (120000ms), so kept as its own constant rather than raised to that bound. +const SCOPED_INSTALL_TIMEOUT_MS = 60_000; const { createTempDir, cleanup, mockPartialWriteThenThrow } = require('./helpers.cjs'); const { @@ -95,7 +100,7 @@ describe('#2429 regression: Codex local skills stay project-scoped', () => { const result = runNode( [INSTALL_SCRIPT, '--codex', '--local', '--profile=core'], - { cwd: projectDir, env, timeoutMs: 60_000 }, + { cwd: projectDir, env, timeoutMs: SCOPED_INSTALL_TIMEOUT_MS }, ); assert.strictEqual( @@ -1118,7 +1123,7 @@ describe('#1004 regression: installer does not duplicate managed hooks when regi { type: 'http', url: hookUrl, - timeout: 5, + timeout: FIXTURE_HOOK_TIMEOUT_SECONDS, }, ], }, @@ -1970,7 +1975,7 @@ describe('#1874 F6 (#338 migration): a malformed settings.local.json is preserve delete env.GSD_TEST_MODE; const result = runNode( [INSTALL_SCRIPT, '--claude', '--local'], - { cwd: root, env, timeoutMs: 60_000 }, + { cwd: root, env, timeoutMs: SCOPED_INSTALL_TIMEOUT_MS }, ); assert.strictEqual(result.exitCode, 0, `installer exited ${result.exitCode}\n${result.stdout}\n${result.stderr}`); @@ -2018,7 +2023,7 @@ describe('#1874 F6 adjacent: malformed settings.local.json does not crash the in delete env.GSD_TEST_MODE; const result = runNode( [INSTALL_SCRIPT, '--claude', '--local'], - { cwd: root, env, timeoutMs: 60_000 }, + { cwd: root, env, timeoutMs: SCOPED_INSTALL_TIMEOUT_MS }, ); assert.strictEqual(result.exitCode, 0, diff --git a/tests/install-runtime-artifacts.test.cjs b/tests/install-runtime-artifacts.test.cjs index 4d6a1d4b5..5d189134b 100644 --- a/tests/install-runtime-artifacts.test.cjs +++ b/tests/install-runtime-artifacts.test.cjs @@ -32,6 +32,20 @@ const { splitLines, joinLines } = require('../gsd-core/bin/lib/text-lines.cjs'); const { createTempDir, cleanup, writePackageSourceMarkerFixture } = require('./helpers.cjs'); const { runNode } = require('./helpers/process-seam.cjs'); const { INSTALL_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); + +// Bound the writer subprocess so a regression that hangs the writer +// (or the dispatcher) cannot deadlock CI (PR #3003 CR feedback). +// 30s is generous for what should complete in <1s; if it trips, +// surface that as a clear test failure rather than CI hanging. +const WRITER_SUBPROCESS_TIMEOUT_MS = 30_000; + +// Bounds a real `gsd-tools.cjs capability install` subprocess run against a +// fully-installed tree; digits coincide with the shared +// HOOK_FANOUT_TIMEOUT_MS/SEAM_DEFAULT_TIMEOUT_MS constants but this is +// neither a hook fan-out nor an omitted-default seam call, so kept as its +// own constant. +const CAPABILITY_INSTALL_TIMEOUT_MS = 60000; + const { INSTALL_SCRIPT, MANIFEST_NAME, @@ -5564,11 +5578,9 @@ describe('Bug #2973: dev-preferences default writer path is skills/gsd-dev-prefe // landed in the developer's live config dir instead of tmpHome. env: Object.assign({}, process.env, TEST_ENV_BASE, { HOME: tmpHome, USERPROFILE: tmpHome }), encoding: 'utf-8', - // Bound the subprocess so a regression that hangs the writer - // (or the dispatcher) cannot deadlock CI (PR #3003 CR feedback). - // 30s is generous for what should complete in <1s; if it trips, - // surface that as a clear test failure rather than CI hanging. - timeout: 30_000, + // See WRITER_SUBPROCESS_TIMEOUT_MS's own doc comment (top of file) + // for the full rationale. + timeout: WRITER_SUBPROCESS_TIMEOUT_MS, }); assert.equal(result.signal, null, `writer subprocess was killed by signal ${result.signal} (likely timeout): ${result.stderr}`); @@ -6948,7 +6960,7 @@ function realInstall() { const res = spawnSync( process.execPath, [INSTALL, '--claude', '--global', '--config-dir', dir], - { encoding: 'utf8', timeout: 120000, env: childEnv }, + { encoding: 'utf8', timeout: INSTALL_TIMEOUT_MS, env: childEnv }, ); assert.strictEqual(res.status, 0, `install --claude failed: ${res.stderr || res.stdout}`); return dir; @@ -6992,7 +7004,7 @@ describe('Gap 1 (end-to-end CLI): installed capability install uses the real hos const res = spawnSync( process.execPath, [installedTools, 'capability', 'install', src, '--scope', 'global', '--yes', '--json'], - { cwd, env, encoding: 'utf8', timeout: 60000 }, + { cwd, env, encoding: 'utf8', timeout: CAPABILITY_INSTALL_TIMEOUT_MS }, ); const combined = `${res.stdout || ''}\n${res.stderr || ''}`; assert.doesNotMatch( diff --git a/tests/install-write-confinement.test.cjs b/tests/install-write-confinement.test.cjs index 138f7d97f..663c7236c 100644 --- a/tests/install-write-confinement.test.cjs +++ b/tests/install-write-confinement.test.cjs @@ -2048,6 +2048,7 @@ const os = require('node:os'); const path = require('node:path'); const { runNode } = require('./helpers/process-seam.cjs'); const { cleanup } = require('./helpers.cjs'); +const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); const ROOT = path.resolve(__dirname, '..'); const DRIFT_LINT = path.join(ROOT, 'scripts', 'lint-shell-command-projection-drift.cjs'); @@ -2055,7 +2056,7 @@ const DRIFT_LINT = path.join(ROOT, 'scripts', 'lint-shell-command-projection-dri function runLint(targetFile) { const result = runNode([DRIFT_LINT, targetFile], { cwd: ROOT, - timeoutMs: 15000, + timeoutMs: PROBE_TIMEOUT_MS, }); result.status = result.exitCode; return result; @@ -3836,6 +3837,7 @@ const crypto = require('node:crypto'); const ROOT = path.join(__dirname, '..'); const INSTALL = require(path.join(ROOT, 'bin', 'install.js')); const { cleanup, sandboxHome, scrubConfigLocationEnv } = require('./helpers.cjs'); +const { INSTALL_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); const MANIFEST_NAME = 'gsd-file-manifest.json'; const PATCHES_DIR_NAME = 'gsd-local-patches'; @@ -3977,7 +3979,7 @@ describe('Bug #4086: saveLocalPatches resolves skills/ manifest keys at the runt assert.deepEqual(modifiedList, [], 'without a runtime the old skip behavior applies'); }); - test('end-to-end codex reinstall backs up the modified skill (#4086)', { timeout: 120_000 }, () => { + test('end-to-end codex reinstall backs up the modified skill (#4086)', { timeout: INSTALL_TIMEOUT_MS }, () => { const origLog = console.log; const origWarn = console.warn; console.log = () => {}; diff --git a/tests/install.test.cjs b/tests/install.test.cjs index 4dfa07d04..17dd24085 100644 --- a/tests/install.test.cjs +++ b/tests/install.test.cjs @@ -34,7 +34,20 @@ const { runNode } = require('./helpers/process-seam.cjs'); const pkg = require('../package.json'); // #3145: class-norm timeout, not a per-suite value — see helpers/timeouts.cjs. -const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const { PROBE_TIMEOUT_MS, QUICK_SPAWN_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); + +// Fixture-JSON value (seconds, not ms) simulating the PRE-migration legacy +// settings.json hook-entry timeout under test (#3981-style migration test). +// Deliberately a different value than the shared FIXTURE_HOOK_TIMEOUT_SECONDS +// constant (5) since this site tests the old value specifically (batch #4515, +// epic #4445). +const FIXTURE_LEGACY_HOOK_TIMEOUT_SECONDS = 10; + +// Bounds an `install.js --dry-run` run (no file writes) — a lighter shape than +// the shared full-write INSTALL_TIMEOUT_MS class (120000ms), kept separate +// rather than raised. Shared by both dry-run sites in this file (batch #4515, +// epic #4445). +const DRY_RUN_INSTALL_TIMEOUT_MS = 30_000; const { getConfigDirFromHome, @@ -1127,7 +1140,7 @@ describe('install — fix-slash-commands.cjs lands at scripts/fix-slash-commands const result = spawnSync( process.execPath, [gsdToolsPath, 'query', 'init.new-project'], - { encoding: 'utf8', timeout: 15000 }, + { encoding: 'utf8', timeout: PROBE_TIMEOUT_MS }, ); assert.ok( !result.stderr.includes('MODULE_NOT_FOUND'), @@ -1215,7 +1228,7 @@ describe('readCmdNames() — tolerates missing commands/gsd directory (#1223)', const spawnResult = spawnSync(process.execPath, ['-e', script], { encoding: 'utf8', - timeout: 10000, + timeout: QUICK_SPAWN_TIMEOUT_MS, env: { ...process.env, GSD_TEST_MODE: '1' }, }); assert.ok( @@ -5568,7 +5581,7 @@ describe('#338 case 3: migration of prior local install GSD entries from setting { type: 'command', command: `${process.execPath} ${path.join(claudeDir, 'hooks', 'gsd-context-monitor.js')}`, - timeout: 10, + timeout: FIXTURE_LEGACY_HOOK_TIMEOUT_SECONDS, } ] } @@ -7368,7 +7381,7 @@ function installAndRead(runtime) { const res = spawnSync( process.execPath, [INSTALL, `--${runtime}`, '--global', '--config-dir', dir], - { encoding: 'utf8', timeout: 120000, env: { ...process.env, HOME: dir, USERPROFILE: dir } }, + { encoding: 'utf8', timeout: INSTALL_TIMEOUT_MS, env: { ...process.env, HOME: dir, USERPROFILE: dir } }, ); assert.strictEqual(res.status, 0, `install --${runtime} failed: ${res.stderr || res.stdout}`); const wf = path.join(dir, 'gsd-core', 'workflows', 'execute-phase.md'); @@ -7809,7 +7822,7 @@ describe('#3026: installer --help documents every accepted runtime flag', () => test('every accepted runtime flag appears in --help output', () => { // Derive accepted flags behaviorally from the installer's argument parser. - const r = spawnSync(process.execPath, [INSTALL_PATH, '--help'], { encoding: 'utf-8', timeout: 10000 }); + const r = spawnSync(process.execPath, [INSTALL_PATH, '--help'], { encoding: 'utf-8', timeout: QUICK_SPAWN_TIMEOUT_MS }); const helpText = r.stdout; // The installer's getRuntimeArgs defines which -- flags it accepts. @@ -7976,7 +7989,7 @@ describe('#607 --dry-run flag: spawned installer exits 0 and mutates nothing', ( }, cwd: REPO_ROOT, encoding: 'utf8', - timeout: 30_000, + timeout: DRY_RUN_INSTALL_TIMEOUT_MS, } ); @@ -8051,7 +8064,7 @@ describe('#607 --dry-run flag: spawned installer exits 0 and mutates nothing', ( }, cwd: REPO_ROOT, encoding: 'utf8', - timeout: 30_000, + timeout: DRY_RUN_INSTALL_TIMEOUT_MS, } ); diff --git a/tests/npm-integrity-gate.test.cjs b/tests/npm-integrity-gate.test.cjs index 7cde6ff77..28b480a90 100644 --- a/tests/npm-integrity-gate.test.cjs +++ b/tests/npm-integrity-gate.test.cjs @@ -23,11 +23,18 @@ const assert = require('node:assert/strict'); const { spawnSync } = require('node:child_process'); const path = require('node:path'); const { runNode } = require('./helpers/process-seam.cjs'); +const { QUICK_SPAWN_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); const ROOT = path.resolve(__dirname, '..'); const SCRIPT = path.join(ROOT, 'scripts', 'check-npm-integrity.cjs'); const FIXTURES = path.join(__dirname, 'fixtures', 'npm-integrity'); +// Bounds a real check-npm-integrity.cjs run via the process seam; digits +// coincide with the shared BUILD_TIMEOUT_MS but this is a different +// operation class (an integrity check, not a hooks build), so kept as its +// own constant rather than aliased. +const NPM_INTEGRITY_CHECK_TIMEOUT_MS = 30_000; + /** * Run the integrity gate script against a fixture directory. * @@ -37,7 +44,7 @@ const FIXTURES = path.join(__dirname, 'fixtures', 'npm-integrity'); */ function runGate(fixtureName, extraArgs = []) { const fixtureDir = path.join(FIXTURES, fixtureName); - const r = runNode([SCRIPT, ...extraArgs], { cwd: fixtureDir, timeoutMs: 30_000 }); + const r = runNode([SCRIPT, ...extraArgs], { cwd: fixtureDir, timeoutMs: NPM_INTEGRITY_CHECK_TIMEOUT_MS }); return { status: r.exitCode ?? 1, stdout: r.stdout, @@ -145,7 +152,7 @@ describe('#114: npm integrity gate — --help output', () => { const result = spawnSync(process.execPath, [SCRIPT, '--help'], { cwd: ROOT, encoding: 'utf-8', - timeout: 10_000, + timeout: QUICK_SPAWN_TIMEOUT_MS, }); assert.strictEqual(result.status, 0, '--help should exit 0'); }); @@ -154,7 +161,7 @@ describe('#114: npm integrity gate — --help output', () => { const result = spawnSync(process.execPath, [SCRIPT, '--help'], { cwd: ROOT, encoding: 'utf-8', - timeout: 10_000, + timeout: QUICK_SPAWN_TIMEOUT_MS, }); // The .cjs script writes --help to stdout. const helpText = (result.stdout ?? '') + (result.stderr ?? ''); diff --git a/tests/packaging-shipped-scripts-require-only-shipped.test.cjs b/tests/packaging-shipped-scripts-require-only-shipped.test.cjs index b140ab8ec..7684fc952 100644 --- a/tests/packaging-shipped-scripts-require-only-shipped.test.cjs +++ b/tests/packaging-shipped-scripts-require-only-shipped.test.cjs @@ -23,6 +23,19 @@ const REPO_ROOT = path.join(__dirname, '..'); const { extractRequires } = require('./helpers/copy-script-fixture.cjs'); +/** + * package.json's `prepack`/`prepare` runs a full `npm run build:lib` (tsc) + * before `npm pack` computes the file list — a bounded but non-trivial + * subprocess (~2s in isolation). Under the full parallel 28,000+-test CI + * matrix this has been observed to take 60.6s and trip a 60_000ms bound + * (#2931 gsd-test run d52d2ee4, linux-node24, duration_ms 60637.56), + * aborting this file's `before()` hook and cascading every sibling test to + * "cancelled". 120s keeps the bound finite (never unbounded, per the + * subprocess-timeout convention) while giving real headroom for a + * contended CI box. + */ +const NPM_PACK_TIMEOUT_MS = 120_000; + /** * Resolve the tarball file list via `npm pack --dry-run --json`. * This is the ACTUAL set of files that ship — not a hardcoded list — so adding @@ -34,16 +47,7 @@ function resolveTarballFiles() { encoding: 'utf-8', shell: true, // Windows: npm is npm.cmd and needs a shell stdio: ['pipe', 'pipe', 'pipe'], - // package.json's `prepack`/`prepare` runs a full `npm run build:lib` - // (tsc) before `npm pack` computes the file list — a bounded but - // non-trivial subprocess (~2s in isolation). Under the full parallel - // 28,000+-test CI matrix this has been observed to take 60.6s and trip - // a 60_000ms bound (#2931 gsd-test run d52d2ee4, linux-node24, - // duration_ms 60637.56), aborting this file's `before()` hook and - // cascading every sibling test to "cancelled". 120s keeps the bound - // finite (never unbounded, per the subprocess-timeout convention) while - // giving real headroom for a contended CI box. - timeout: 120_000, + timeout: NPM_PACK_TIMEOUT_MS, }); const parsed = JSON.parse(raw); return packListToPathSet(parsed); diff --git a/tests/plugin-manifest.test.cjs b/tests/plugin-manifest.test.cjs index d06c9c6a1..99c156c50 100644 --- a/tests/plugin-manifest.test.cjs +++ b/tests/plugin-manifest.test.cjs @@ -25,10 +25,16 @@ const identity = require(path.join(ROOT, 'gsd-core', 'bin', 'lib', 'package-iden const pkg = require(path.join(ROOT, 'package.json')); const { MANAGED_HOOKS } = require(path.join(ROOT, 'hooks', 'managed-hooks-registry.cjs')); const { cleanup, TEST_ENV_BASE } = require('./helpers.cjs'); +const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); const PLUGIN_JSON_PATH = path.join(ROOT, '.claude-plugin', 'plugin.json'); const HOOKS_JSON_PATH = path.join(ROOT, 'hooks', 'hooks.json'); +// Bounds a `claude --version` PATH probe; kept separate from the shared +// PROBE_TIMEOUT_MS (15000ms) since this site's pre-existing value differs +// and this migration never raises a bound without a fresh bench citation. +const CLAUDE_VERSION_PROBE_TIMEOUT_MS = 5000; + // ─── Section A: plugin.json ─────────────────────────────────────────────────── describe('A: .claude-plugin/plugin.json', () => { @@ -401,7 +407,7 @@ describe('C: plugin.json schema validation', () => { try { const result = spawnSync('claude', ['--version'], { encoding: 'utf-8', - timeout: 5000, + timeout: CLAUDE_VERSION_PROBE_TIMEOUT_MS, env: claudeCliEnv(), }); return result.status === 0; @@ -524,7 +530,7 @@ describe('C: plugin.json schema validation', () => { const result = spawnSync('claude', ['plugin', 'validate', pluginRoot, '--strict'], { cwd: ROOT, encoding: 'utf-8', - timeout: 15000, + timeout: PROBE_TIMEOUT_MS, env: claudeCliEnv(), }); assert.equal( diff --git a/tests/release-tarball-smoke.install.test.cjs b/tests/release-tarball-smoke.install.test.cjs index f54db4e2c..f33ead312 100644 --- a/tests/release-tarball-smoke.install.test.cjs +++ b/tests/release-tarball-smoke.install.test.cjs @@ -474,6 +474,11 @@ const { execFileSync } = require('node:child_process'); // The helpers under test. const { isolatedNpmEnv, cleanup } = require('./helpers.cjs'); +// Bounds the `node -e` npm-harness spawn pattern shared by the three +// execFileSync(process.execPath, ['-e', script], ...) sites in this file, +// each of which requires helpers.cjs and calls runNpm(...). +const NPM_HARNESS_SPAWN_TIMEOUT_MS = 30_000; + // Resolve a filesystem path to its canonical (symlink-free) form even if the // leaf does not exist yet (e.g. ~/.npm before npm has written its cache). // Walks up to the nearest existing ancestor, resolves that, then re-appends @@ -540,7 +545,7 @@ describe('bug-131: runNpm isolates HOME from the caller environment', () => { try { stdout = execFileSync(process.execPath, ['-e', script], { encoding: 'utf-8', - timeout: 30_000, + timeout: NPM_HARNESS_SPAWN_TIMEOUT_MS, }); } catch (err) { stdout = err.stdout || ''; @@ -594,7 +599,7 @@ describe('bug-131: runNpm isolates HOME from the caller environment', () => { try { stdout = execFileSync(process.execPath, ['-e', script], { encoding: 'utf-8', - timeout: 30_000, + timeout: NPM_HARNESS_SPAWN_TIMEOUT_MS, }); } catch (err) { stdout = err.stdout || ''; @@ -728,7 +733,7 @@ describe('bug-131: runNpm isolates HOME from the caller environment', () => { try { stdout = execFileSync(process.execPath, ['-e', script], { encoding: 'utf-8', - timeout: 30_000, + timeout: NPM_HARNESS_SPAWN_TIMEOUT_MS, }); } catch (err) { stdout = err.stdout || '';