diff --git a/CONTEXT.md b/CONTEXT.md index a444d407b..81629d0cb 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -420,6 +420,9 @@ Five-axis story decomposition discipline (**S**pike, **P**aths, **I**nterfaces, ### Clock seam An injectable time abstraction accepted as an optional parameter by production code (`{ clock = Date } = {}`). Test code substitutes `node:test` `mock.timers` to control time deterministically without waiting for real OS scheduler events. Canonical pattern established by ADR 456 (`docs/adr/456-test-rigor-architecture.md`). +### Process seam +The single subprocess-spawning primitive test code uses (`tests/helpers/process-seam.cjs`, #3055): `runNode` / `runGit` / `runHook`, each returning one discriminated union `{ outcome, exitCode, stdout, stderr, timedOut, signal, killed, code }` where `outcome` is the frozen `OUTCOME` enum (`EXITED` / `KILLED` / `TIMED_OUT` / `BUFFER_OVERFLOW` / `SPAWN_FAILED`). Every call is timeout-bounded — there is no unbounded code path — and nothing throws for a child's exit code, kill, timeout, buffer overflow, or spawn failure; all five are data. KILLED is a child terminated by a signal the seam did not send (a genuine OOM kill): `spawnSync` reports no `error` for that case, so it must be distinguished from EXITED, and the `runGsdTools` adapter retries it exactly as the pre-seam `isKilled()` did. This is what makes `timedOut` and `signal` assertable, so a fail-open guard's degraded verdict can be tested instead of merely observing that the call did not throw. Discrimination order is forced by runtime behavior: a timeout and a maxBuffer overflow are identical on both `status` (`null`) and `signal` (`SIGTERM`) and differ only by `code` (`ETIMEDOUT` vs `ENOBUFS`), so overflow is classified first. Per-suite wrappers remain and bind fixtures (cwd, env, payload); only the spawn body delegates here. Deliberately **not** a fault-injection surface — it cannot distinguish an injected timeout from a genuine bench OOM and would retry it; injection is in-process via `deps` (#3056). `runGsdTools` is an adapter over it that preserves its own legacy `{ success, output, error, exitCode }` shape and retry-once-on-kill behavior. + ### Deterministic scheduler Test-execution model in which all timing and concurrency outcomes are fully controlled by the test (via clock seam, explicit `await` ordering, or synchronous stepping) rather than by the OS thread scheduler. Opposed to real-race tests, which are non-deterministic on loaded CI runners. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 517c1e49e..b6c38f9dd 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -373,6 +373,50 @@ const { createTempProject, createTempGitProject, createTempDir, cleanup, runGsdT | `cleanup(tmpDir)` | Removes directory recursively | Always use in `afterEach` | | `runGsdTools(args, cwd, env?)` | Executes gsd-tools.cjs | Testing CLI commands | +### Spawning a subprocess: use the process seam + +Anything that shells out goes through `tests/helpers/process-seam.cjs` — never a hand-rolled +`spawnSync`/`execFileSync` in your suite. + +```javascript +const { runNode, runGit, runHook, OUTCOME } = require('./helpers/process-seam.cjs'); + +const r = runHook(HOOK_PATH, [], { input: JSON.stringify(payload), timeoutMs: 5000 }); +assert.equal(r.outcome, OUTCOME.EXITED); +assert.equal(r.exitCode, 0); +``` + +| Primitive | Spawns | +|---|---| +| `runNode(argv, opts)` | `process.execPath` | +| `runGit(argv, opts)` | `git` | +| `runHook(scriptPath, argv, opts)` | `opts.interpreter` (default `process.execPath`; pass `'bash'` for a shell script) | + +`opts`: `{ cwd, env, input, timeoutMs, killSignal, interpreter }`. + +Every call returns the same discriminated union — `{ outcome, exitCode, stdout, stderr, timedOut, +signal, killed, code }` — and **never throws** for a child's exit code, a timeout, a buffer +overflow, or a spawn failure. All four are data, so you assert on them: + +```javascript +assert.equal(r.outcome, OUTCOME.TIMED_OUT); +assert.equal(r.timedOut, true); +``` + +Two rules the seam enforces for you: + +- **Every call is timeout-bounded.** `timeoutMs` defaults to 60s; there is no unbounded path. An + unbounded subprocess is an indefinite hang, and it is how macOS CI silently stops reporting. +- **`outcome` distinguishes cases that look identical.** A timeout and a `maxBuffer` overflow both + report `exitCode: null` and `signal: 'SIGTERM'`, differing only in `code` (`ETIMEDOUT` vs + `ENOBUFS`). Branch on `outcome`, never on `signal`. + +The seam is **not** a fault-injection surface — it cannot tell an injected timeout from a genuine +bench OOM. Inject faults in-process through a module's `deps` parameter instead. + +Per-suite wrappers are still expected and encouraged: bind your fixture (cwd, env, payload) in a +local helper and delegate the spawn to the seam. + ### Test Structure ```javascript diff --git a/eslint.config.mjs b/eslint.config.mjs index 719797c76..d6ffc66a4 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -351,7 +351,7 @@ export default tseslint.config( // ── tests/**/*.test.cjs ───────────────────────────────────────────────────── { - files: ['tests/**/*.test.cjs'], + files: ['tests/**/*.cjs'], plugins: { 'no-only-tests': noOnlyTests, local: localPlugin, diff --git a/tests/api-coverage-gate-e2e.test.cjs b/tests/api-coverage-gate-e2e.test.cjs index d5e94caee..c9f49f70f 100644 --- a/tests/api-coverage-gate-e2e.test.cjs +++ b/tests/api-coverage-gate-e2e.test.cjs @@ -19,9 +19,9 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); -const { execFileSync } = require('node:child_process'); const { cleanup } = require('./helpers.cjs'); +const { runNode, OUTCOME } = require('./helpers/process-seam.cjs'); // In-process seam for the fail-closed read-injection tests at the bottom of this // file (#2365 review): readPhaseScope is the pure phase-scope reader behind the // gate. Those tests monkeypatch fs rather than drive a subprocess. @@ -51,22 +51,23 @@ function runTools(args, cwd) { ? args : (args.match(/(?:[^\s"']+|"[^"]*"|'[^']*')+/g) || []) .map((t) => t.replace(/"([^"]*)"/g, '$1').replace(/'([^']*)'/g, '$1')); - try { - const stdout = execFileSync(process.execPath, [TOOLS_PATH, ...argv], { - cwd, - encoding: 'utf-8', - env: { ...process.env, ...TEST_ENV_BASE }, - timeout: 60000, - }); - return { success: true, output: stdout.trim(), exitCode: 0, error: '' }; - } catch (err) { - return { - success: false, - output: err.stdout?.toString().trim() || '', - error: err.stderr?.toString().trim() || err.message, - exitCode: err.status ?? 1, - }; + const r = runNode([TOOLS_PATH, ...argv], { + cwd, + env: { ...process.env, ...TEST_ENV_BASE }, + timeoutMs: 60000, + }); + if (r.outcome === OUTCOME.EXITED && r.exitCode === 0) { + return { success: true, output: r.stdout.trim(), exitCode: 0, error: '' }; } + return { + success: false, + output: r.stdout.trim(), + // Non-EXITED outcomes (timeout, spawn failure, buffer overflow) never + // populate stderr, so fall back to the seam's outcome label — mirroring + // execFileSync's err.message fallback when err.stderr was empty. + error: r.stderr.trim() || `process-seam: ${r.outcome}`, + exitCode: r.exitCode ?? 1, + }; } function makeProject(workflow) { diff --git a/tests/execute-phase-worktree-guard.test.cjs b/tests/execute-phase-worktree-guard.test.cjs index f0eab98b4..de6ddeb2c 100644 --- a/tests/execute-phase-worktree-guard.test.cjs +++ b/tests/execute-phase-worktree-guard.test.cjs @@ -10,8 +10,9 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); -const { execFileSync, spawnSync } = require('node:child_process'); +const { execFileSync } = require('node:child_process'); const { cleanup } = require('./helpers.cjs'); +const { runHook } = require('./helpers/process-seam.cjs'); const ROOT = path.resolve(__dirname, '..'); const WORKFLOW = path.join(ROOT, 'gsd-core', 'workflows', 'execute-phase.md'); @@ -57,13 +58,15 @@ function commitFile(dir, name, msg) { /** Run the extracted guard in `dir`. Never throws — returns the observed result. */ function runGuard(dir) { - const res = spawnSync('bash', ['-c', guardScript()], { + // 30s: already bounded pre-migration (unchanged) — the guard runs a handful + // of git plumbing calls (rev-parse, log, status) against a small fixture repo. + const res = runHook('-c', [guardScript()], { + interpreter: 'bash', cwd: dir, - encoding: 'utf8', - timeout: 30_000, + timeoutMs: 30_000, env: { ...process.env, GIT_TERMINAL_PROMPT: '0' }, }); - return { status: res.status, stdout: res.stdout || '', stderr: res.stderr || '' }; + return { status: res.exitCode, stdout: res.stdout || '', stderr: res.stderr || '' }; } test('#1856: refusal names the stranded commits and the dirty tree', () => { diff --git a/tests/fix-2587-cursor-hook-workspace-roots.test.cjs b/tests/fix-2587-cursor-hook-workspace-roots.test.cjs index 695f89318..089421772 100644 --- a/tests/fix-2587-cursor-hook-workspace-roots.test.cjs +++ b/tests/fix-2587-cursor-hook-workspace-roots.test.cjs @@ -31,6 +31,7 @@ const fs = require('node:fs'); const path = require('node:path'); const { execFileSync } = require('node:child_process'); const { createTempDir, cleanup } = require('./helpers.cjs'); +const { runHook: runHookSeam } = require('./helpers/process-seam.cjs'); const HOOKS = path.join(__dirname, '..', 'hooks'); const SESSION_START = path.join(HOOKS, 'gsd-cursor-session-start.js'); @@ -49,13 +50,12 @@ const STOP_REMINDER_FRAGMENT = 'Agent stopping'; /** Run a hook script with an explicit cwd and stdin payload; return parsed stdout JSON. */ function runHook(script, { cwd, payload }) { - const stdout = execFileSync(process.execPath, [script], { + const r = runHookSeam(script, [], { cwd, input: typeof payload === 'string' ? payload : JSON.stringify(payload), - encoding: 'utf8', - timeout: 20000, + timeoutMs: 20000, }); - return JSON.parse(stdout || '{}'); + return JSON.parse(r.stdout || '{}'); } /** A directory containing .planning/STATE.md. */ diff --git a/tests/graphify-auto-update.slow.test.cjs b/tests/graphify-auto-update.slow.test.cjs index 8f59afa4b..5866e5438 100644 --- a/tests/graphify-auto-update.slow.test.cjs +++ b/tests/graphify-auto-update.slow.test.cjs @@ -14,6 +14,7 @@ const path = require('path'); const os = require('node:os'); const { execFileSync, spawnSync } = require('child_process'); const { createTempProject, cleanup, runGsdTools, delay } = require('./helpers.cjs'); +const { runHook: seamRunHook } = require('./helpers/process-seam.cjs'); const { graphifyStatus, @@ -249,7 +250,12 @@ describe('auto-update', () => { const PATH = pathPrepend ? `${pathPrepend}${path.delimiter}${process.env.PATH || ''}` : process.env.PATH || ''; - return spawnSync('bash', [HOOK], { + // 30000ms: already bounded pre-migration (unchanged) — this is the `slow` + // suite and the hook itself dispatches a detached graphify rebuild that + // some tests wait on separately; the hook's own synchronous return (gate + // checks + status-file write) is fast, so 30s stays generous headroom. + const r = seamRunHook(HOOK, [], { + interpreter: 'bash', cwd: tmpDir, input: JSON.stringify(toolPayload), env: { @@ -258,9 +264,9 @@ describe('auto-update', () => { CI: '', ...env, }, - encoding: 'utf8', - timeout: 30000, + timeoutMs: 30000, }); + return { status: r.exitCode, stdout: r.stdout, stderr: r.stderr }; } // Wait until the detached rebuild writes a terminal status, with a generous diff --git a/tests/gsd-check-update-worker-platform-gate.test.cjs b/tests/gsd-check-update-worker-platform-gate.test.cjs index 1e8f90e4a..f3938b066 100644 --- a/tests/gsd-check-update-worker-platform-gate.test.cjs +++ b/tests/gsd-check-update-worker-platform-gate.test.cjs @@ -495,7 +495,7 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); -const { spawnSync } = require('node:child_process'); +const { runHook: seamRunHook } = require('./helpers/process-seam.cjs'); const { cleanup } = require('./helpers.cjs'); const HOOK_PATH = path.join(__dirname, '..', 'hooks', 'gsd-update-banner.js'); @@ -653,10 +653,16 @@ describe('gsd-update-banner.js end-to-end', () => { } function runHook(home) { - return spawnSync(process.execPath, [HOOK_PATH], { + // 10000ms: previously UNBOUNDED (no `timeout` option passed to + // spawnSync). gsd-update-banner.js only reads a small cache file from + // disk and prints JSON — no subprocess/network work — so 10s is + // generous headroom over its sub-second worst case even on a + // contended CI runner. + const r = seamRunHook(HOOK_PATH, [], { env: { ...process.env, HOME: home, USERPROFILE: home }, - encoding: 'utf8', + timeoutMs: 10_000, }); + return { status: r.exitCode, stdout: r.stdout, stderr: r.stderr }; } function writeCache(home, contents) { diff --git a/tests/gsd-statusline.test.cjs b/tests/gsd-statusline.test.cjs index 00331b38d..6944b482b 100644 --- a/tests/gsd-statusline.test.cjs +++ b/tests/gsd-statusline.test.cjs @@ -609,7 +609,7 @@ describe('readGsdState', () => { // ─── CLAUDE_CODE_AUTO_COMPACT_WINDOW context meter (#2219) ────────────────── describe('context meter respects CLAUDE_CODE_AUTO_COMPACT_WINDOW (#2219)', () => { - const { execFileSync } = require('node:child_process'); + const { runHook: runHookSeam } = require('./helpers/process-seam.cjs'); const hookPath = path.join(__dirname, '..', 'hooks', 'gsd-statusline.js'); /** @@ -639,17 +639,8 @@ describe('context meter respects CLAUDE_CODE_AUTO_COMPACT_WINDOW (#2219)', () => delete env.CLAUDE_CODE_AUTO_COMPACT_WINDOW; } - let stdout = ''; - try { - stdout = execFileSync(process.execPath, [hookPath], { - input: payload, - env, - encoding: 'utf8', - timeout: 4000, - }); - } catch (e) { - stdout = e.stdout || ''; - } + const r = runHookSeam(hookPath, [], { input: payload, env, timeoutMs: 4000 }); + const stdout = r.stdout; // Parse normalized used% from the statusline bar output (e.g. "60%") // Strip ANSI escape codes then extract the percentage digit(s) before "%" @@ -1549,7 +1540,7 @@ test('config-set statusline.show_context_tokens yes → rejected', () => { const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); - const { execFileSync } = require('node:child_process'); + const { runHook: runHookSeam } = require('./helpers/process-seam.cjs'); const { cleanup } = require('./helpers.cjs'); const { formatTokens, contextTokenSuffix } = require('../hooks/gsd-statusline.js'); const { VALID_CONFIG_KEYS } = require('../gsd-core/bin/lib/config-schema.cjs'); @@ -1639,16 +1630,9 @@ test('config-set statusline.show_context_tokens yes → rejected', () => { }, }, }); - let stdout = ''; - try { - stdout = execFileSync(process.execPath, [hookPath], { - input: payload, encoding: 'utf8', timeout: 4000, - }); - } catch (e) { - stdout = e.stdout || ''; - } + const r = runHookSeam(hookPath, [], { input: payload, timeoutMs: 4000 }); // eslint-disable-next-line no-control-regex -- stripping ANSI SGR sequences from captured CLI output - return stdout.replace(/\x1b\[[0-9;]*m/g, ''); + return r.stdout.replace(/\x1b\[[0-9;]*m/g, ''); } test('flag=true appends the token count after the percentage', () => { @@ -1986,6 +1970,7 @@ test('config-set statusline.show_context_tokens yes → rejected', () => { const os = require('node:os'); const path = require('node:path'); const { execFileSync } = require('node:child_process'); + const { runHook: runHookSeam } = require('./helpers/process-seam.cjs'); const { cleanup } = require('./helpers.cjs'); const statusline = require('../hooks/gsd-statusline.js'); const { parseGitStatus, buildGitSegment, readGitStatus, composeStatusline } = statusline; @@ -2210,16 +2195,9 @@ test('config-set statusline.show_context_tokens yes → rejected', () => { workspace: { current_dir: dir }, session_id: `test-git-${Date.now()}-${Math.random().toString(36).slice(2)}`, }); - let stdout = ''; - try { - stdout = execFileSync(process.execPath, [hookPath], { - input: payload, encoding: 'utf8', timeout: 4000, - }); - } catch (e) { - stdout = e.stdout || ''; - } + const r = runHookSeam(hookPath, [], { input: payload, timeoutMs: 4000 }); // eslint-disable-next-line no-control-regex -- stripping ANSI SGR sequences from captured CLI output - return stdout.replace(/\x1b\[[0-9;]*m/g, ''); + return r.stdout.replace(/\x1b\[[0-9;]*m/g, ''); } test('flag=true renders the branch segment', () => { diff --git a/tests/gsd-write-guard.test.cjs b/tests/gsd-write-guard.test.cjs index 61ac1b412..afcc5e77d 100644 --- a/tests/gsd-write-guard.test.cjs +++ b/tests/gsd-write-guard.test.cjs @@ -26,8 +26,8 @@ const { describe, test, before, after } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); -const { spawnSync } = require('node:child_process'); const { createTempDir, cleanup } = require('./helpers.cjs'); +const { runHook: runHookSeam } = require('./helpers/process-seam.cjs'); const HOOK_PATH = path.join(__dirname, '..', 'hooks', 'gsd-write-guard.js'); @@ -35,16 +35,25 @@ const HOOK_PATH = path.join(__dirname, '..', 'hooks', 'gsd-write-guard.js'); * Run the hook with a given payload. The override env var is stripped by * default so an outer environment can never leak a bypass into the tests; * pass extraEnv to set it explicitly. + * + * Returns an object shaped like the raw spawnSync() result (status/stdout/ + * stderr) because every call site in this file was written against that + * shape; the seam itself returns exitCode, not status, so it is mapped here. + * 10_000ms: gsd-write-guard.js does no subprocess work of its own (pure + * fs reads + JSON, no execFileSync/spawnSync inside the hook) — generous + * headroom over the fs-bound workload without matching the 30_000ms figure + * sibling suites use for guards that shell out to git. */ function runHook(payload, extraEnv = {}) { const env = { ...process.env }; delete env.GSD_ALLOW_PLANNING_SHRINK; Object.assign(env, extraEnv); - return spawnSync(process.execPath, [HOOK_PATH], { + const r = runHookSeam(HOOK_PATH, [], { input: typeof payload === 'string' ? payload : JSON.stringify(payload), - encoding: 'utf8', env, + timeoutMs: 10_000, }); + return { status: r.exitCode, stdout: r.stdout, stderr: r.stderr }; } function lines(n, tag = 'line') { diff --git a/tests/helpers.cjs b/tests/helpers.cjs index 43f25477a..19e5983e2 100644 --- a/tests/helpers.cjs +++ b/tests/helpers.cjs @@ -7,6 +7,7 @@ const fs = require('fs'); const os = require('os'); const path = require('path'); const { createFixture } = require('./fixtures/index.cjs'); +const processSeam = require('./helpers/process-seam.cjs'); const TOOLS_PATH = path.join(__dirname, '..', 'gsd-core', 'bin', 'gsd-tools.cjs'); const TEST_ENV_BASE = { @@ -44,72 +45,150 @@ function runGsdTools(args, cwd = process.cwd(), env = {}) { : (args.match(/(?:[^\s"']+|"[^"]*"|'[^']*')+/g) || []) .map(t => t.replace(/"([^"]*)"/g, '$1').replace(/'([^']*)'/g, '$1')); + // Adapter over tests/helpers/process-seam.cjs (#3055). The seam returns a + // typed { outcome, exitCode, stdout, stderr, timedOut, signal, killed, code } + // result — never throws for a kill/timeout/buffer-overflow/spawn-failure. + // This adapter is the ONLY place that retries and the ONLY place that + // reconstructs runGsdTools's legacy { success, output, error, exitCode } + // shape, so all 136 callers keep their existing contract byte-identically. + // + // `processSeam.runNode` is looked up on the module object (not destructured + // at require time) so tests can `mock.method(processSeam, 'runNode', fn)` + // to inject TIMED_OUT / BUFFER_OVERFLOW / SPAWN_FAILED without waiting on + // real subprocess timers. function attempt() { - // Split shell-style string into argv, stripping surrounding quotes, so we - // can invoke execFileSync with process.execPath instead of relying on - // `node` being on PATH (it isn't in Claude Code shell sessions). - // Apply shell-style quote removal: strip surrounding quotes from quoted - // sequences anywhere in a token (handles both "foo bar" and --"foo bar"). - return execFileSync(process.execPath, [TOOLS_PATH, ...argv], { + return processSeam.runNode([TOOLS_PATH, ...argv], { cwd, - encoding: 'utf-8', - stdio: ['pipe', 'pipe', 'pipe'], env: childEnv, - timeout: 60000, + timeoutMs: 60000, }); } - // isKilled: true when the subprocess was terminated by a signal or timed out. - // This indicates host resource starvation (OOM, scheduler contention), NOT a - // product assertion failure. - function isKilled(err) { - return err.killed || err.signal != null || err.code === 'ETIMEDOUT'; - } - - function throwResourceStarvation(err) { + function throwResourceStarvation(result) { throw new Error( `[runGsdTools: resource-starvation / subprocess-kill after retry] ` + `gsd-tools was killed before completion ` + - `(signal=${err.signal}, code=${err.code}, killed=${err.killed}). ` + + `(signal=${result.signal}, code=${result.code}, killed=${result.killed}). ` + `This indicates host OOM or scheduler contention, not a product bug. ` + - `stdout=${err.stdout?.toString().trim() || ''} ` + - `stderr=${err.stderr?.toString().trim() || ''}` + `stdout=${(result.stdout || '').trim()} ` + + `stderr=${(result.stderr || '').trim()}` ); } - try { - const result = attempt(); - return { success: true, output: result.trim(), exitCode: 0 }; - } catch (firstErr) { - // Kill-signal discrimination (#969): transient OOM/contention usually - // succeeds on retry; retry ONCE before surfacing the labeled error. - if (isKilled(firstErr)) { - try { - const result = attempt(); - return { success: true, output: result.trim(), exitCode: 0 }; - } catch (retryErr) { - // Still killed after retry — persistent resource starvation, throw. - throwResourceStarvation(retryErr); + function toLegacyShape(result) { + if (result.outcome === processSeam.OUTCOME.EXITED) { + if (result.exitCode === 0) { + return { success: true, output: (result.stdout || '').trim(), exitCode: 0 }; } + // Clean non-zero exit (real command error, no kill signal, no spawn + // failure): return normally. No retry, no throw — preserves existing + // test behavior that asserts on error shape. + const stderrRaw = (result.stderr || '').trim(); + // Prefer actual stderr content; fall back to the same "Command failed: + // " message Node's execFileSync used to synthesize + // for a clean non-zero exit with no stderr (verified against this + // runtime's child_process internals: checkExecSyncError() only builds + // that message when `ret.error` is absent and `ret.status !== 0`, and + // never appends stderr when it is empty). If stderr is empty, append a + // note so CI logs show "stderr: (empty)" rather than silently losing + // the fact that the child process produced no error output — empty + // stderr with a non-zero exit code is a signal of OS-level crash (OOM + // kill, worker thread fatal error) rather than a gsd-tools application + // error. + const commandLine = [process.execPath, TOOLS_PATH, ...argv].join(' '); + const error = stderrRaw + || `Command failed: ${commandLine} [stderr: (empty) exit:${result.exitCode ?? 1}]`; + return { + success: false, + output: (result.stdout || '').trim(), + error, + exitCode: result.exitCode ?? 1, + }; } - // Clean non-zero exit (real command error, no kill signal): return normally. - // No retry, no throw — preserves existing test behavior that asserts on - // error shape. - const stderrRaw = firstErr.stderr?.toString().trim() || ''; - // Prefer actual stderr content; fall back to err.message (which contains - // the command invocation). If stderr is empty, append a note so CI logs - // show "stderr: (empty)" rather than silently losing the fact that the - // child process produced no error output — empty stderr with a non-zero - // exit code is a signal of OS-level crash (OOM kill, worker thread fatal - // error) rather than a gsd-tools application error. - const error = stderrRaw || `${firstErr.message} [stderr: (empty) exit:${firstErr.status ?? 1}]`; + if (result.outcome === processSeam.OUTCOME.BUFFER_OVERFLOW) { + // Never retried. This is a DELIBERATE divergence from the old + // execFileSync-based helper, not an oversight: the old code saw a + // maxBuffer overflow as `err.signal === 'SIGTERM'`, which made the old + // `isKilled(err)` true and triggered a retry. The new seam classifies + // overflow as its own BUFFER_OVERFLOW outcome specifically so it stops + // being conflated with a kill — the child ran fine and produced too + // much output, so retrying wastes 60s and fails identically every + // time. + // + // exitCode is coerced to 1 here — RETRACTED claim from an earlier + // revision of this comment that it was "never coerced to exitCode:1, + // unlike the pre-seam helper": that was wrong. A real caller + // (tests/context-predicates-query.test.cjs) asserts + // `typeof r.exitCode === 'number'`, matching the old code's + // `err.status ?? 1` on every non-retried failure path. The SEAM layer + // still reports `exitCode: null` (see toSeamResult) — that typed + // result is where the "no numeric exit code exists" information + // lives, discriminated via `outcome`. This LEGACY adapter's job is to + // preserve the old numeric contract for existing callers, so it + // coerces null to 1 here rather than propagating the seam's null. + return { + success: false, + output: (result.stdout || '').trim(), + error: `gsd-tools output exceeded the subprocess buffer limit (code=${result.code})`, + exitCode: 1, + }; + } + if (result.outcome === processSeam.OUTCOME.KILLED) { + // Defensive only: the retry loop below always retries KILLED once and + // throws throwResourceStarvation() if it is still KILLED afterward, so + // this function is never actually invoked with a KILLED result that + // has not already survived a retry. It is handled explicitly (instead + // of falling into the SPAWN_FAILED catch-all below, whose message + // would be misleading) so a KILLED result can never silently render as + // a generic {success:false, exitCode:1}-shaped spawn failure. + return { + success: false, + output: (result.stdout || '').trim(), + error: `gsd-tools was killed by signal (signal=${result.signal}, code=${result.code})`, + exitCode: null, + }; + } + // SPAWN_FAILED: the process never started (matches old behavior — ENOENT + // and friends carry no signal, so the old `isKilled(err)` was false). + // Never retried — retrying is pointless. + // + // exitCode is coerced to 1 here — same retraction as the BUFFER_OVERFLOW + // branch above: this was previously described as "never coerced to + // exitCode:1," which was wrong for the ADAPTER path. The old + // execFileSync-based helper returned `err.status ?? 1` on every + // non-retried failure, i.e. always `1` for a spawn failure, and a real + // caller depends on `typeof exitCode === 'number'`. The SEAM's own + // `toSeamResult` still reports `exitCode: null` for SPAWN_FAILED — that + // typed layer is where "no numeric exit code exists" is expressed via + // `outcome`; this legacy adapter re-applies the old numeric contract on + // top of it. return { success: false, - output: firstErr.stdout?.toString().trim() || '', - error, - exitCode: firstErr.status ?? 1, + output: (result.stdout || '').trim(), + error: `gsd-tools failed to spawn (code=${result.code})`, + exitCode: 1, }; } + + // Kill-signal discrimination (#969): transient OOM/contention usually + // succeeds on retry; retry ONCE before surfacing the labeled error. + // TIMED_OUT and KILLED are retried — together they reproduce the OLD + // execFileSync-based `isKilled(err)` semantics exactly: + // old = err.killed || err.signal != null || err.code === 'ETIMEDOUT' + // TIMED_OUT covers the timeout case; KILLED covers a child terminated by a + // signal nobody in the seam sent (e.g. an external OOM kill) — the exact + // #969 case this retry exists for. BUFFER_OVERFLOW and SPAWN_FAILED are + // not kills and are never retried (see their branches in toLegacyShape). + const first = attempt(); + if (first.outcome === processSeam.OUTCOME.TIMED_OUT || first.outcome === processSeam.OUTCOME.KILLED) { + const retry = attempt(); + if (retry.outcome === processSeam.OUTCOME.TIMED_OUT || retry.outcome === processSeam.OUTCOME.KILLED) { + // Still killed after retry — persistent resource starvation, throw. + throwResourceStarvation(retry); + } + return toLegacyShape(retry); + } + return toLegacyShape(first); } // Create a bare temp directory (no .planning/ structure) @@ -333,7 +412,11 @@ function captureConsole(fn) { console.error = origError; } if (threw) throw threw; - const strip = (s) => s.replace(/\x1b\[[0-9;]*m/g, ''); + // Built via String.fromCharCode (not a literal control character in a + // regex, which `no-control-regex` rejects) so the ESC byte itself is + // matched at runtime — this strips real ANSI color codes, not a decoy. + const ansiPattern = new RegExp(`${String.fromCharCode(0x1b)}\\[[0-9;]*m`, 'g'); + const strip = (s) => s.replace(ansiPattern, ''); return { stdout: stdout.map(strip).join('\n'), stderr: stderr.map(strip).join('\n'), diff --git a/tests/helpers/emitted-provenance.cjs b/tests/helpers/emitted-provenance.cjs index 5127cd26a..b47d11573 100644 --- a/tests/helpers/emitted-provenance.cjs +++ b/tests/helpers/emitted-provenance.cjs @@ -40,8 +40,6 @@ const path = require('node:path'); -const REPO_ROOT = path.join(__dirname, '..', '..'); - const { cleanup } = require('../helpers.cjs'); const { MANIFEST_FAMILIES, runMinimalInstall, buildParityManifest } = require('./install-shared.cjs'); diff --git a/tests/helpers/emitted-runtime.cjs b/tests/helpers/emitted-runtime.cjs index d7db4b83f..2cd65ee3a 100644 --- a/tests/helpers/emitted-runtime.cjs +++ b/tests/helpers/emitted-runtime.cjs @@ -36,7 +36,7 @@ const path = require('node:path'); const crypto = require('node:crypto'); const { execFileSync } = require('node:child_process'); -const { cleanup } = require('../helpers.cjs'); +const { cleanup, runNpm } = require('../helpers.cjs'); const { MANIFEST_FAMILIES, MINIMUM_MANIFEST_FAMILIES, @@ -689,8 +689,8 @@ function buildBaselineAtRef(ref, { cwd = REPO_ROOT } = {}) { fs.symlinkSync(sharedNodeModules, path.join(worktreeDir, 'node_modules'), 'dir'); } - execFileSync('npm', ['run', 'build:lib'], { - cwd: worktreeDir, encoding: 'utf8', timeout: BUILD_LIB_TIMEOUT_MS, stdio: ['ignore', 'pipe', 'pipe'], + runNpm(['run', 'build:lib'], { + cwd: worktreeDir, timeout: BUILD_LIB_TIMEOUT_MS, stdio: ['ignore', 'pipe', 'pipe'], }); // Run `cwd`'s OWN generator (not the worktree's — see the function doc for why), diff --git a/tests/helpers/install-shared.cjs b/tests/helpers/install-shared.cjs index ece0be060..7b39d124d 100644 --- a/tests/helpers/install-shared.cjs +++ b/tests/helpers/install-shared.cjs @@ -205,9 +205,11 @@ const EXCLUDED_PREFIXES = ['gsd-core/bin/lib/']; // ─── Helper functions ───────────────────────────────────────────────────────── -function stripAnsi(str) { +const ANSI_ESCAPE = String.fromCharCode(27); +const ANSI_SGR_RE = new RegExp(`${ANSI_ESCAPE}\\[[0-9;]*m`, 'g'); - return str.replace(/\x1b\[[0-9;]*m/g, ''); +function stripAnsi(str) { + return str.replace(ANSI_SGR_RE, ''); } // A version string can itself contain regex metacharacters (`.`, and — via diff --git a/tests/helpers/process-seam.cjs b/tests/helpers/process-seam.cjs new file mode 100644 index 000000000..cde3c68e7 --- /dev/null +++ b/tests/helpers/process-seam.cjs @@ -0,0 +1,226 @@ +'use strict'; + +/** + * Process seam — the single spawnSync-based primitive test helpers use to + * run a subprocess and get back a typed, discriminated-union result. + * + * Design contract: .gsd/phase/test-3055-process-seam-module/40-design.md + * Test matrix: .gsd/phase/test-3055-process-seam-module/50-test-matrix.md + * + * Scope (Phase 1, #3055): this module and the `runGsdTools` adapter in + * tests/helpers.cjs. The 23 local wrapper helpers are migrated in a later + * wave — they are not touched here. + * + * Why spawnSync (not execFileSync): execFileSync throws on any non-zero + * exit or spawn error, forcing every caller through try/catch to recover + * `stdout`/`stderr`/`status`. spawnSync returns all of that as data, which + * is what lets this module express a single discriminated-union return + * shape instead of a throw-shaped side channel. + * + * OUTCOME discrimination (verified empirically against this Node runtime's + * spawnSync — see PR discussion for the probe transcripts, since the + * design doc's stated error-code assumptions do not match observed + * behavior on this platform/Node version): + * + * - No `result.error` and `signal === null` -> EXITED (a clean or + * non-zero exit with no signal involved). + * - No `result.error` but `signal !== null` -> KILLED. A child terminated + * by a signal nobody in this seam sent (e.g. an external OOM killer, or + * `process.kill(pid, 'SIGKILL')` from outside) does NOT populate + * `result.error` on this runtime — verified empirically: `spawnSync` + * returns `{status: null, signal: 'SIGKILL', error: undefined}` for an + * externally-killed child. Treating that as EXITED would report + * `exitCode: null` under an EXITED outcome, an incoherent shape, and + * would silently drop the #969 kill-discrimination retry for the exact + * case it exists to catch. KILLED is reported as its own outcome so the + * `runGsdTools` adapter can retry it exactly like TIMED_OUT. + * - `result.error.code` is a buffer-overflow code (`ENOBUFS` on this + * runtime, or the `ERR_CHILD_PROCESS_STDIO_MAXBUFFER` code documented + * for the async exec()/execFile() family, accepted defensively in case + * a different Node version/platform surfaces it here) -> BUFFER_OVERFLOW. + * - `result.error.code === 'ETIMEDOUT'` OR `signal !== null` -> TIMED_OUT. + * A timeout is identified POSITIVELY now, not by elimination: on this + * runtime spawnSync's own timeout kill reports `ETIMEDOUT`, and on a + * platform whose timeout errno differs, the child is still killed by a + * signal on the way out, so `signal !== null` still catches it. This + * replaces an earlier `status === null` catch-all that was too greedy — + * it also matched a spawn that never started at all (e.g. Windows + * `ENAMETOOLONG` from an oversized argv: `status: null, signal: null`), + * misclassifying a non-retryable spawn failure as a retryable timeout + * and driving the adapter into a retry loop that could never succeed. + * - Any other populated `result.error` -> SPAWN_FAILED. This subsumes the + * `ENOENT` case (binary not found) along with every other spawn-time + * errno (`ENAMETOOLONG`, `E2BIG`, `EACCES`, `EPERM`, …) — none of them + * carry a timeout errno or a signal, so none of them satisfy the + * TIMED_OUT branch above. A dedicated `ENOENT`-only branch was dropped + * since it produced the exact same outcome as this fallback; keeping it + * would have implied ENOENT gets special handling it does not need. + */ + +const { spawnSync } = require('child_process'); + +const DEFAULT_TIMEOUT_MS = 60000; + +const OUTCOME = Object.freeze({ + EXITED: 'exited', + KILLED: 'killed', + TIMED_OUT: 'timed_out', + BUFFER_OVERFLOW: 'buffer_overflow', + SPAWN_FAILED: 'spawn_failed', +}); + +const BUFFER_OVERFLOW_CODES = new Set(['ENOBUFS', 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER']); + +/** + * Resolve and validate `timeoutMs`. `undefined` means "use the bounded + * default" and is the only valid way to get an unspecified timeout — there + * is no code path in this module that spawns without one. + */ +function resolveTimeoutMs(timeoutMs) { + if (timeoutMs === undefined) return DEFAULT_TIMEOUT_MS; + if (typeof timeoutMs !== 'number' || !Number.isFinite(timeoutMs) || timeoutMs <= 0) { + throw new TypeError( + `process-seam: timeoutMs must be a finite positive number, or omitted for the ` + + `${DEFAULT_TIMEOUT_MS}ms default; received ${String(timeoutMs)}` + ); + } + return timeoutMs; +} + +/** + * Classify a raw spawnSync() result into the seam's discriminated union. + */ +function toSeamResult(result) { + const { error, status, signal } = result; + const stdout = typeof result.stdout === 'string' ? result.stdout : ''; + const stderr = typeof result.stderr === 'string' ? result.stderr : ''; + const errorCode = error ? (error.code ?? null) : null; + + let outcome; + if (!error) { + outcome = signal === null ? OUTCOME.EXITED : OUTCOME.KILLED; + } else if (BUFFER_OVERFLOW_CODES.has(errorCode)) { + outcome = OUTCOME.BUFFER_OVERFLOW; + } else if (errorCode === 'ETIMEDOUT' || signal !== null) { + outcome = OUTCOME.TIMED_OUT; + } else { + // Any other populated `result.error` — ENOENT, ENAMETOOLONG, E2BIG, + // EACCES, EPERM, etc. — is a spawn failure: the process never started + // (or started and errored in a way that carries neither a timeout + // errno nor a signal), so retrying can never succeed. + outcome = OUTCOME.SPAWN_FAILED; + } + + const timedOut = outcome === OUTCOME.TIMED_OUT; + // KILLED always has signal !== null by construction (see the branch + // above), so this single check also satisfies "killed is true for both + // KILLED and TIMED_OUT" without narrowing existing behavior for the other + // outcomes (e.g. a signaled BUFFER_OVERFLOW kill). + const killed = timedOut || signal !== null; + + return { + outcome, + exitCode: status, + stdout, + stderr, + timedOut, + signal: signal ?? null, + killed, + code: errorCode, + }; +} + +/** + * Core seam primitive: spawn `command` with argv `args` and return the + * typed OUTCOME-discriminated result. Never throws on a child's exit code, + * a timeout, a buffer overflow, or a spawn failure — those are all data. + * Throws (TypeError) only for a seam-contract violation: a non-array + * `args`, or an invalid `timeoutMs`. + * + * @param {string} command - binary to spawn (never shell-interpreted). + * @param {string[]} args - argv array. No shell-string parsing. + * @param {object} [options] + * @param {string} [options.cwd] + * @param {object} [options.env] + * @param {string} [options.input] - stdin payload. Omit entirely to leave + * stdin unwritten-then-closed; passing `''` is a different, deliberate + * choice callers may still make, but it is never implied by omission. + * @param {number} [options.timeoutMs] - bounded; see resolveTimeoutMs. + * @param {string} [options.killSignal] - forwarded to spawnSync verbatim; + * the seam asserts nothing about which signal is used. + * @returns {{outcome:string, exitCode:number|null, stdout:string, + * stderr:string, timedOut:boolean, signal:string|null, killed:boolean, + * code:string|null}} + */ +function spawnSeam(command, args, options = {}) { + if (!Array.isArray(args)) { + throw new TypeError('process-seam: args must be an argv array, not a shell string'); + } + const timeoutMs = resolveTimeoutMs(options.timeoutMs); + + const spawnOptions = { + encoding: 'utf-8', // Never caller-controlled — the seam always forces string output. + timeout: timeoutMs, + }; + if (options.cwd !== undefined) spawnOptions.cwd = options.cwd; + if (options.env !== undefined) spawnOptions.env = options.env; + if (options.input !== undefined) spawnOptions.input = options.input; + if (options.killSignal !== undefined) spawnOptions.killSignal = options.killSignal; + + const result = spawnSync(command, args, spawnOptions); + return toSeamResult(result); +} + +/** + * Run a Node script/module via the current interpreter (`process.execPath`). + * @param {string[]} args - argv passed to the spawned Node process. + * @param {object} [options] - see spawnSeam. + */ +function runNode(args, options = {}) { + return spawnSeam(process.execPath, args, options); +} + +/** + * Run `git`. + * @param {string[]} args - argv passed to git. + * @param {object} [options] - see spawnSeam. + */ +function runGit(args, options = {}) { + return spawnSeam('git', args, options); +} + +/** + * Run a hook script, matching how tests/read-guard.test.cjs and + * tests/workflow-guard.test.cjs invoke hooks/*.js today: + * `execFileSync(process.execPath, [HOOK_PATH, ...args], ...)`. + * + * The hook surface this seam replaces is not node-only: 4 of the 23 local + * wrappers being migrated drive `bash` guard/gate scripts directly + * (tests/execute-phase-worktree-guard.test.cjs:59, + * tests/graphify-auto-update.slow.test.cjs:248, + * tests/worktree-cleanup.test.cjs:1549, tests/worktree-safety.test.cjs:4641). + * Rather than add a fourth spawn primitive for one more binary, the target + * interpreter is a parameter here. It is EXPLICIT, never inferred from + * `target`'s extension — guessing an interpreter from a path fails silently + * on a script whose name does not match its shebang. + * + * @param {string} target - the first argv element handed to the + * interpreter. Normally an absolute path to the hook/guard/gate script + * being run. But for an interpreter invoked with an inline program (e.g. + * `bash -c '