Files
msd-core/tests/helpers/process-seam.cjs
Tom Boucher 9faacc0c15 test(#3148): bound the long tail and delete the unbounded-spawn allowlist (#3192)
* test(#3148): bound the long tail and delete the allowlist

Migrates the final 170 unbounded sync spawn sites across 49 files, then
removes the allowlist entirely. local/no-unbounded-spawn now runs with no
exemption surface across tests/**: there is no file to add a name to.

drift-detection's throw-native git() helper routes to gitOrThrow -- bare
runGit would have taken 16 call sites quiet on failure. commands.test.cjs
has two independently-scoped runGsdTools/runCli helpers, one already bounded
and one not; they are kept distinct rather than unified, the same trap as the
two same-named git() helpers in Wave 1.

runNpm's bound was erasable. Its options spread callerOptions after the
defaults, so an explicit timeout:undefined silently dropped the 180000ms
bound -- the rule flagged it and was right; it was not a false positive. Fixed
by destructuring with a default, with a test that fails when the default is
removed.

Two sites stay on a raw spawn with an explicit timeout because the seam
cannot express them: one needs shell:true for npm.cmd on Windows, one
redirects stdout to a real fd. Both are the rule's own documented second
option, not an escape from it.

Closure verified rather than asserted: the derivation scan reports 0 unbounded
spawn helpers and 0 unbounded direct git call sites, and a temporary file
carrying an unbounded spawn still errors with the allowlist gone.

Closes #3064.

* test(#3148): close a hole in the guard's own eslint-disable ban

The ban listed only the top level of tests/, so it was blind to 37 .cjs
files under tests/helpers, qa, observability, fixtures and dispatch. With the
allowlist deleted this test is the sole remaining way to detect someone
silencing the rule inline, so the gap was load-bearing: a nested file could
carry an unbounded spawn plus an eslint-disable and pass everything.

Proven before and after. A probe planted under tests/helpers with both was
invisible to the guard and clean under eslint; after making the listing
recursive the guard fails on it. The scanned set goes from 771 files to 808.

Pre-existing since the guard shipped, but this wave is what promoted it to
sole defense, so it is fixed here rather than filed.

Also converts the last hand-rolled throw check to throwIfFailed and the last
re-derived legacy shape to compose toLegacyResult, which makes the epic's
none-remain claim true rather than nearly true. toLegacyResult itself is not
widened -- eight callers depend on its shape and one consumer does not
justify changing a shared contract.

* fix(#3148): correct seam incoherence at the bound and a slow review-lane error path

Two real failures from the remote runner, both fixed at the cause.

The seam could return outcome TIMED_OUT together with exitCode 0. At the
exact bound spawnSync reports ETIMEDOUT while the child has already exited
with a real status, and toSeamResult classified on the error code while
passing status straight through -- an incoherent pair its own boundary test
was written to catch, and did. A status that is not null is direct evidence
the child exited on its own, so it now decides the outcome before the
error-code branches run. process-seam.cjs was deliberately untouched by every
earlier wave; this is a defect in the module itself, kept surgical, with a
unit test that fails against the old logic.

review-lane with an unknown subcommand fell through to its usage error only
after loading the capability registry and building a per-lane plan, which
spawns one child process per lane -- up to twelve. The error path took
~1288ms instead of ~119ms, and under bench load it outran a caller's spawn
timeout and was killed before writing anything, which is the empty stdout and
stderr CI saw. It now fails fast before any of that work begins.

This is the epic's first production change. It is user-facing, so it carries
a changeset rather than a no-changelog label.

* test(#3148): replace a real-race timeout test with a deterministic one

E9 raced git rev-parse against a 1ms bound and assumed git always lost. On a
warm container git finishes first, spawnSync returns status 0 with no error
at all, the seam correctly classifies EXITED, and gitOrThrow correctly does
not throw -- so the test failed on both lanes. A probe confirms a genuine
timeout always carries status null, so this was never the seam misbehaving.

Raising the bound would only lengthen the odds, which is the same defect with
better luck. The test now drives gitOrThrow against a stubbed runGit that
returns a synthetic TIMED_OUT result, so it asserts exactly what it always
meant to -- that a timeout propagates as a throw -- with no timing
dependence. Five consecutive runs are identical where the old one varied.

I wrote this test in Wave 0; it is a real-race test by construction and
CLAUDE.md says to replace those rather than re-run them.

* chore(#3148): backfill changeset PR number 3192

---------

Co-authored-by: sim <sim@local>
2026-08-07 21:03:50 -04:00

245 lines
11 KiB
JavaScript

'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.
* - `status !== null` with a populated `result.error` -> EXITED, evidence
* over classification, checked before any error-code branch below. At
* the exact timeout boundary, spawnSync can report `error.code ===
* 'ETIMEDOUT'` (the timer fired) on a result that ALSO carries
* `status: 0` (the child finished on its own first) — verified
* empirically via `tests/process-seam.test.cjs`'s at-the-bound case.
* `status` is only ever populated by a real exit, so it outranks an
* attached error: a process that returned a real exit code did not time
* out in any sense the caller cares about, and reporting TIMED_OUT while
* passing that exit code through as `exitCode` would be an incoherent
* shape. This also means every branch below may assume `status ===
* null`, which is exactly the assumption the BUFFER_OVERFLOW vs
* TIMED_OUT ordering (next) already relies on.
* - `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 (status !== null) {
// Evidence over classification: `status` is only ever populated by a
// real exit, so it outranks an attached `error` — see the header
// comment's at-the-timeout-boundary case.
outcome = OUTCOME.EXITED;
} 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 '<script text>'`), this is that interpreter's own flag —
* `'-c'` — with the actual program text supplied as the first element of
* `args`, not as `target` itself.
* @param {string[]} [args] - extra argv for the hook. When `target` is an
* interpreter flag like `'-c'`, this is where the inline program text and
* its own argv go.
* @param {object} [options] - see spawnSeam.
* @param {string} [options.interpreter] - binary used to run `target`.
* Defaults to `process.execPath` (matching read-guard/workflow-guard
* today); pass `'bash'` to run a shell script instead.
*/
function runHook(target, args = [], options = {}) {
const { interpreter = process.execPath, ...spawnOptions } = options;
return spawnSeam(interpreter, [target, ...args], spawnOptions);
}
module.exports = { runNode, runGit, runHook, OUTCOME, toSeamResult };