From 1d208e5af68a39b98d2e3d9282075c50417f38f5 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 7 Aug 2026 10:58:34 -0400 Subject: [PATCH] test(#3144): bound the git/worktree cluster onto the process seam (#3152) * test(#3144): bound the git/worktree cluster onto the process seam Migrates 180 unbounded sync spawn sites across 19 files. Every previously unbounded call now carries an explicit timeout with a comment giving the number and why. The migration is not a callee swap. execSync and execFileSync throw on a non-zero exit and the seam never does, so each site was classified first: sites that rely on the throw route to gitOrThrow, and sites that already read .status to detect an EXPECTED non-zero -- an intended cherry-pick conflict, a rev-parse outside a repo driving a skip -- route to the never-throwing runGit instead, which would otherwise throw on exactly the exit being probed for. Two same-named git() helpers in worktree-cleanup.test.cjs have different return contracts, one trimmed and one raw; both are preserved rather than unified. Collapses five hand-rolled throw wrappers onto one throwIfFailed in git-fixture.cjs, which gitOrThrow now also uses so the shape cannot drift. Allowlist drops 139 to 120; BASELINE lowered to match. * test(#3144): fix pre-PR review findings Documents throwIfFailed in the CONTEXT.md glossary and CONTRIBUTING.md -- it became the shared throw mechanism without either doc naming it. Routes the sixth and seventh hand-rolled copies of the throw shape through throwIfFailed (worktree-baseref-install, worktree-safety-reap); the first consolidation missed both. Converts ci-rebase-check's 8 fixture-setup calls from unchecked runGit to gitOrThrow so a failed setup step aborts where it fails rather than surfacing later as a confusing failure against the wrong subject. Adds 12 direct unit tests for throwIfFailed, which until now was only exercised transitively. Splits verify.test.cjs's non-git grep/sed bound off GIT_TIMEOUT_MS. --------- Co-authored-by: sim --- CONTEXT.md | 2 +- CONTRIBUTING.md | 13 ++ .../no-unbounded-spawn.allowlist.json | 21 +--- tests/ci-rebase-check.test.cjs | 28 +++-- tests/commit-docs-bypass.test.cjs | 8 +- tests/commit-files-deletion.test.cjs | 17 +-- tests/commit-files-pathspec.test.cjs | 55 +++++---- tests/execute-phase-worktree-guard.test.cjs | 10 +- .../fix-2608-commit-staging-failure.test.cjs | 31 +++-- tests/git-base-branch.test.cjs | 69 ++++++----- tests/git-fixture.test.cjs | 74 +++++++++++- tests/gsd-write-guard.property.test.cjs | 14 ++- tests/helpers/git-fixture.cjs | 91 +++++++++----- tests/no-unbounded-spawn-allowlist.test.cjs | 5 +- tests/prune-orphaned-worktrees.test.cjs | 49 ++++---- tests/quick-branching.test.cjs | 26 ++-- tests/reapply-verify-hunks.test.cjs | 17 ++- .../release-hotfix-empty-cherry-pick.test.cjs | 36 +++--- tests/verification-status.test.cjs | 27 +++-- tests/verify.test.cjs | 65 ++++++---- tests/workspace.test.cjs | 46 ++++---- tests/worktree-baseref-install.test.cjs | 13 +- tests/worktree-cleanup.test.cjs | 61 ++++++---- tests/worktree-safety-reap.test.cjs | 5 +- tests/worktree-safety.test.cjs | 111 ++++++++++++------ tests/worktree.test.cjs | 66 ++++++----- 26 files changed, 598 insertions(+), 362 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index 447371a3f..39ff2df76 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -424,7 +424,7 @@ An injectable time abstraction accepted as an optional parameter by production c 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. ### Git fixture wrapper -The throw-preserving companion to the process seam (`tests/helpers/git-fixture.cjs`, #3143): `gitOrThrow(args, options)` runs `runGit` and returns `stdout` as a string on a clean exit, but throws on any other outcome. It exists because the seam **deliberately never throws** while `execSync` and `execFileSync` — the two forms 237 migrating call sites use — both throw on a non-zero exit. Migrating those mechanically onto `runGit` would convert a loud failure into a silent one: fixture setup that failed would return an empty string and surface as a baffling assertion failure further down. The thrown error carries `status` **and** `exitCode` as deliberate aliases (`status` is what the legacy `execSync` catch idiom reads, e.g. `tests/worktree-safety.test.cjs:1361`), plus `stdout`, `stderr`, `signal`, `timedOut` and `outcome`. Use `runGit` when every outcome is data you branch on; use `gitOrThrow` for fixture setup that must abort loudly. The seam module is **not** modified to add this — a throwing export would falsify the never-throws contract stated in its own header and in the `### Process seam` entry above. +The throw-preserving companion to the process seam (`tests/helpers/git-fixture.cjs`, #3143): `gitOrThrow(args, options)` runs `runGit` and returns `stdout` as a string on a clean exit, but throws on any other outcome. It exists because the seam **deliberately never throws** while `execSync` and `execFileSync` — the two forms 237 migrating call sites use — both throw on a non-zero exit. Migrating those mechanically onto `runGit` would convert a loud failure into a silent one: fixture setup that failed would return an empty string and surface as a baffling assertion failure further down. The thrown error carries `status` **and** `exitCode` as deliberate aliases (`status` is what the legacy `execSync` catch idiom reads, e.g. `tests/worktree-safety.test.cjs:1361`), plus `stdout`, `stderr`, `signal`, `timedOut` and `outcome`. Use `runGit` when every outcome is data you branch on; use `gitOrThrow` for fixture setup that must abort loudly. The seam module is **not** modified to add this — a throwing export would falsify the never-throws contract stated in its own header and in the `### Process seam` entry above. The module also exports `throwIfFailed(result, displayName)`, the single implementation of that throw shape: `gitOrThrow` itself is `throwIfFailed` specialized to `runGit`, so it routes through the same code path and the two cannot drift apart. Per-suite wrappers driving non-git targets — a node CLI via `runNode`, a bash snippet via `runHook` — call `throwIfFailed` directly rather than hand-rolling their own copy of this shape, which is exactly how five call sites had drifted from each other before this module exported it (#3144). ### Unbounded-spawn guard The lint rule enforcing `DEFECT.UNBOUNDED-SUBPROCESS` across the test suite (`eslint-rules/no-unbounded-spawn.cjs`, #3143, wired into the `tests/**/*.cjs` block of `eslint.config.mjs`). Flags `spawnSync` / `execFileSync` / `execSync` whose options carry no usable `timeout`. It resolves renamed destructures (`const { execSync: exec } = require('node:child_process')`) and chained requires (`require('node:child_process').execSync(...)`) rather than matching literal callee names — both forms exist in the suite today and a name-only matcher leaves them permanently invisible. It resolves an options object held in a single-write `const`, which is what keeps `process-seam.cjs` — the bounded reference implementation — from flagging itself. Two values are rejected as *nominally* bounded: `timeout: 0` (Node reads zero as no timeout) and anything above the 600000 ms ceiling (effectively unbounded); a non-literal value is trusted, since the target shape is one named constant with a comment. `no-unbounded-spawn.allowlist.json` grandfathers pre-existing violations and ratchets **down only** — a listed file with zero violations reports its own entry as stale, so the list cannot go quiet while the class survives. Companion tests assert the list never grows, carries no dead entries, and that no `eslint-disable` for this rule exists anywhere under `tests/`. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 74ec62abe..f0869f3ec 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -447,6 +447,19 @@ site keeps working. `process-seam.cjs` itself is untouched by this — it still never throws. +If your per-suite wrapper spawns something that is **not** git — a node CLI via `runNode`, a bash +snippet via `runHook` — and its callers depend on a throw, call `throwIfFailed(result, displayName)` +directly instead of hand-rolling the same `outcome !== EXITED || exitCode !== 0` check. `gitOrThrow` +is itself just `throwIfFailed` bound to `runGit`, so every thrown error — git or not — carries the +same `status`/`exitCode`/`stdout`/`stderr`/`signal`/`timedOut`/`outcome` shape: + +```javascript +const { throwIfFailed } = require('./helpers/git-fixture.cjs'); + +const r = runNode([BUILD_SCRIPT], { timeoutMs: BUILD_TIMEOUT_MS }); +throwIfFailed(r, 'build-hooks.js (before install tests)'); +``` + #### The lint rule that enforces it `local/no-unbounded-spawn` (`eslint-rules/no-unbounded-spawn.cjs`) fails any `spawnSync`, diff --git a/eslint-rules/no-unbounded-spawn.allowlist.json b/eslint-rules/no-unbounded-spawn.allowlist.json index c142c9ca8..9ecf64299 100644 --- a/eslint-rules/no-unbounded-spawn.allowlist.json +++ b/eslint-rules/no-unbounded-spawn.allowlist.json @@ -19,16 +19,12 @@ "tests/check-tdd-review-checkpoint-e2e.test.cjs", "tests/check-ui-safety-gate.test.cjs", "tests/check-update-config-dir.test.cjs", - "tests/ci-rebase-check.test.cjs", "tests/ci-test-scope.test.cjs", "tests/cli-exit.test.cjs", "tests/close-phase-todos-padded-resolves.test.cjs", "tests/code-review-pipeline-regression.test.cjs", "tests/codex-config.test.cjs", "tests/commands.test.cjs", - "tests/commit-docs-bypass.test.cjs", - "tests/commit-files-deletion.test.cjs", - "tests/commit-files-pathspec.test.cjs", "tests/commonjs-marker.test.cjs", "tests/config-get-default.test.cjs", "tests/config-loader.test.cjs", @@ -47,11 +43,9 @@ "tests/emitted-caps-gate.test.cjs", "tests/emitted-sizes.test.cjs", "tests/ensure-runtime-build.test.cjs", - "tests/execute-phase-worktree-guard.test.cjs", "tests/execute-wave-post-gate-pipeline-e2e.test.cjs", "tests/fix-2136-clock-local-today.test.cjs", "tests/fix-2590-workflow-script-contract.test.cjs", - "tests/fix-2608-commit-staging-failure.test.cjs", "tests/fix-2650-plan-phase-stall-detection.test.cjs", "tests/fix-2657-untrack-compiled-artifacts.test.cjs", "tests/fix-3045-cursor-subagent-isolation.test.cjs", @@ -62,13 +56,11 @@ "tests/frontmatter-cli.test.cjs", "tests/gemini-runtime-removed.test.cjs", "tests/gen-registry.test.cjs", - "tests/git-base-branch.test.cjs", "tests/golden-install-tree.test.cjs", "tests/graphify-auto-update.slow.test.cjs", "tests/graphify-visualization.test.cjs", "tests/gsd-agent-isolation-guard.test.cjs", "tests/gsd-statusline.test.cjs", - "tests/gsd-write-guard.property.test.cjs", "tests/helpers.cjs", "tests/helpers/graphify.cjs", "tests/helpers/install-shared.cjs", @@ -111,10 +103,6 @@ "tests/process-seam.test.cjs", "tests/prohibition-enforcement.test.cjs", "tests/project-instruction-file-parity.test.cjs", - "tests/prune-orphaned-worktrees.test.cjs", - "tests/quick-branching.test.cjs", - "tests/reapply-verify-hunks.test.cjs", - "tests/release-hotfix-empty-cherry-pick.test.cjs", "tests/repo-layout.test.cjs", "tests/reviewer-docs-parity.test.cjs", "tests/roadmap-upgrade.test.cjs", @@ -128,14 +116,7 @@ "tests/state.test.cjs", "tests/tsconfig-noemit.test.cjs", "tests/validate-registry.test.cjs", - "tests/verification-status.test.cjs", - "tests/verify.test.cjs", "tests/windsurf-hooks-bridge.test.cjs", "tests/workflow-fragments-emission.install.test.cjs", - "tests/workflow-guard.test.cjs", - "tests/workspace.test.cjs", - "tests/worktree-baseref-install.test.cjs", - "tests/worktree-cleanup.test.cjs", - "tests/worktree-safety.test.cjs", - "tests/worktree.test.cjs" + "tests/workflow-guard.test.cjs" ] diff --git a/tests/ci-rebase-check.test.cjs b/tests/ci-rebase-check.test.cjs index 07aafdb77..da7772ccf 100644 --- a/tests/ci-rebase-check.test.cjs +++ b/tests/ci-rebase-check.test.cjs @@ -25,6 +25,11 @@ const ROOT = path.resolve(__dirname, '..'); const SCRIPT = path.join(ROOT, 'scripts', 'ci-rebase-check.cjs'); const NODE = process.execPath; const { cleanup } = require('./helpers.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); + +// 15000ms: git plumbing (init/clone/config/checkout/add/commit) on a small +// mkdtemp fixture repo — far over any observed duration for that class of call. +const GIT_TIMEOUT_MS = 15000; // --------------------------------------------------------------------------- // Helper: run a small inline Node snippet that requires the run() helper @@ -129,19 +134,24 @@ describe('ci-rebase-check: fetch-retry loop resolves when git fetch succeeds', ( const workDir = path.join(tmpDir, 'work'); try { - // Build a bare remote with a `main` branch containing one commit. + // Build a bare remote with a `main` branch containing one commit. Each + // setup step uses gitOrThrow (not the bare seam) so a failure here aborts + // loudly at the point of failure instead of surfacing as a baffling + // assertion mismatch against the script's exit code further down — + // matching the throw-on-failure behavior the pre-migration `spawnSync` + // calls this replaced never had either. fs.mkdirSync(remoteDir, { recursive: true }); - spawnSync('git', ['init', '--bare', remoteDir], { encoding: 'utf8' }); + gitOrThrow(['init', '--bare', remoteDir], { timeoutMs: GIT_TIMEOUT_MS }); // Create a working clone to push an initial commit. - spawnSync('git', ['clone', remoteDir, workDir], { encoding: 'utf8' }); + gitOrThrow(['clone', remoteDir, workDir], { timeoutMs: GIT_TIMEOUT_MS }); fs.writeFileSync(path.join(workDir, 'seed.txt'), 'init\n'); - spawnSync('git', ['-C', workDir, 'config', 'user.email', 'ci@test'], { encoding: 'utf8' }); - spawnSync('git', ['-C', workDir, 'config', 'user.name', 'CI Test'], { encoding: 'utf8' }); - spawnSync('git', ['-C', workDir, 'checkout', '-b', 'main'], { encoding: 'utf8' }); - spawnSync('git', ['-C', workDir, 'add', 'seed.txt'], { encoding: 'utf8' }); - spawnSync('git', ['-C', workDir, 'commit', '-m', 'init'], { encoding: 'utf8' }); - spawnSync('git', ['-C', workDir, 'push', 'origin', 'main'], { encoding: 'utf8' }); + gitOrThrow(['-C', workDir, 'config', 'user.email', 'ci@test'], { timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['-C', workDir, 'config', 'user.name', 'CI Test'], { timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['-C', workDir, 'checkout', '-b', 'main'], { timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['-C', workDir, 'add', 'seed.txt'], { timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['-C', workDir, 'commit', '-m', 'init'], { timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['-C', workDir, 'push', 'origin', 'main'], { timeoutMs: GIT_TIMEOUT_MS }); // Run the script from `workDir` with origin pointing at our bare remote. // GITHUB_BASE_REF=main so it fetches `origin main`. diff --git a/tests/commit-docs-bypass.test.cjs b/tests/commit-docs-bypass.test.cjs index 809fb8419..7719a192d 100644 --- a/tests/commit-docs-bypass.test.cjs +++ b/tests/commit-docs-bypass.test.cjs @@ -229,8 +229,12 @@ const { describe, test, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); -const { execFileSync } = require('node:child_process'); const { createTempGitProject, cleanup, runGsdTools } = require('./helpers.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); + +// 15000ms: git plumbing (diff/rev-parse) on a small mkdtemp fixture repo — +// far over any observed duration for that class of call. +const GIT_TIMEOUT_MS = 15000; // Repo root resolution. This test file lives in `/tests/`. Use a single // parent reference (the established repo-wide pattern, e.g. tests/helpers.cjs @@ -249,7 +253,7 @@ const COMMIT_REASON = Object.freeze({ }); function git(args, cwd) { - return execFileSync('git', args, { cwd, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] }); + return gitOrThrow(args, { cwd, timeoutMs: GIT_TIMEOUT_MS }); } describe('bug #3678 — executor must respect commit_docs:false', () => { diff --git a/tests/commit-files-deletion.test.cjs b/tests/commit-files-deletion.test.cjs index 86e4c18c6..b8faf9235 100644 --- a/tests/commit-files-deletion.test.cjs +++ b/tests/commit-files-deletion.test.cjs @@ -13,9 +13,12 @@ const { describe, test, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); -const { execSync } = require('child_process'); - const { createTempGitProject, cleanup, runGsdTools } = require('./helpers.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); + +// 15000ms: git plumbing (add/commit/diff) on a small mkdtemp fixture repo — +// far over any observed duration for that class of call. +const GIT_TIMEOUT_MS = 15000; describe('commit --files: missing files must not stage deletions (#2014)', () => { let tmpDir; @@ -24,8 +27,8 @@ describe('commit --files: missing files must not stage deletions (#2014)', () => tmpDir = createTempGitProject(); // Commit STATE.md so it exists in git history fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), '# State\n\nInitial state.\n'); - execSync('git add .planning/STATE.md', { cwd: tmpDir, stdio: 'pipe' }); - execSync('git commit -m "add STATE.md"', { cwd: tmpDir, stdio: 'pipe' }); + gitOrThrow(['add', '.planning/STATE.md'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'add STATE.md'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); // Delete STATE.md from disk -- now missing but tracked in git fs.unlinkSync(path.join(tmpDir, '.planning', 'STATE.md')); }); @@ -46,7 +49,7 @@ describe('commit --files: missing files must not stage deletions (#2014)', () => // git diff HEAD~1 HEAD --name-status shows what changed between commits. let diffOutput = ''; try { - diffOutput = execSync('git diff HEAD~1 HEAD --name-status', { cwd: tmpDir, encoding: 'utf-8' }); + diffOutput = gitOrThrow(['diff', 'HEAD~1', 'HEAD', '--name-status'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); } catch (e) { // If nothing to commit, there is no HEAD~1 -- that's also acceptable return; @@ -70,7 +73,7 @@ describe('commit --files: missing files must not stage deletions (#2014)', () => assert.strictEqual(parsed.committed, true, 'should have committed when file exists'); // Verify ROADMAP.md was added in the commit - const diffOutput = execSync('git diff HEAD~1 HEAD --name-status', { cwd: tmpDir, encoding: 'utf-8' }); + const diffOutput = gitOrThrow(['diff', 'HEAD~1', 'HEAD', '--name-status'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); assert.ok( diffOutput.includes('A\t.planning/ROADMAP.md'), 'ROADMAP.md should appear as added in the commit' @@ -89,7 +92,7 @@ describe('commit --files: missing files must not stage deletions (#2014)', () => // The commit must not include a deletion of STATE.md let diffOutput = ''; try { - diffOutput = execSync('git diff HEAD~1 HEAD --name-status', { cwd: tmpDir, encoding: 'utf-8' }); + diffOutput = gitOrThrow(['diff', 'HEAD~1', 'HEAD', '--name-status'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); } catch (e) { return; // nothing committed is fine } diff --git a/tests/commit-files-pathspec.test.cjs b/tests/commit-files-pathspec.test.cjs index 2dc81f09e..7c7a8f506 100644 --- a/tests/commit-files-pathspec.test.cjs +++ b/tests/commit-files-pathspec.test.cjs @@ -15,9 +15,12 @@ const { describe, test, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); -const { execSync } = require('child_process'); - const { createTempGitProject, cleanup, runGsdTools } = require('./helpers.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); + +// 15000ms: git plumbing (add/commit/diff/status/rev-list) on a small mkdtemp +// fixture repo — far over any observed duration for that class of call. +const GIT_TIMEOUT_MS = 15000; describe('commit --files: pathspec honors declared scope (#2112)', () => { let tmpDir; @@ -33,7 +36,7 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { test('commit --files does not absorb unrelated staged files', () => { // Developer stages a WIP file via git add (not via --files). fs.writeFileSync(path.join(tmpDir, 'src-wip.txt'), 'work in progress\n'); - execSync('git add src-wip.txt', { cwd: tmpDir, stdio: 'pipe' }); + gitOrThrow(['add', 'src-wip.txt'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); // GSD writes and commits a planning artifact, naming ONLY that file. fs.writeFileSync(path.join(tmpDir, '.planning', 'PLAN.md'), '# Plan\n'); @@ -43,9 +46,9 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { ); // The commit must contain ONLY .planning/PLAN.md. - const diffOutput = execSync('git diff HEAD~1 HEAD --name-only', { + const diffOutput = gitOrThrow(['diff', 'HEAD~1', 'HEAD', '--name-only'], { cwd: tmpDir, - encoding: 'utf-8', + timeoutMs: GIT_TIMEOUT_MS, }).trim(); assert.strictEqual( diffOutput, @@ -54,9 +57,9 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { ); // The WIP file must still be staged, not committed. - const statusOutput = execSync('git status --porcelain', { + const statusOutput = gitOrThrow(['status', '--porcelain'], { cwd: tmpDir, - encoding: 'utf-8', + timeoutMs: GIT_TIMEOUT_MS, }).trim(); assert.ok( statusOutput.includes('A src-wip.txt') || statusOutput.includes('A\tsrc-wip.txt'), @@ -73,9 +76,9 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { tmpDir, ); - const diffOutput = execSync('git diff HEAD~1 HEAD --name-only', { + const diffOutput = gitOrThrow(['diff', 'HEAD~1', 'HEAD', '--name-only'], { cwd: tmpDir, - encoding: 'utf-8', + timeoutMs: GIT_TIMEOUT_MS, }); const files = diffOutput.trim().split('\n').sort(); assert.deepEqual( @@ -88,18 +91,18 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { test('commit without --files still commits the entire .planning/ index (default path)', () => { // Write a planning artifact and stage it. fs.writeFileSync(path.join(tmpDir, '.planning', 'PLAN.md'), '# Plan\n'); - execSync('git add .planning/PLAN.md', { cwd: tmpDir, stdio: 'pipe' }); + gitOrThrow(['add', '.planning/PLAN.md'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); // Also stage an unrelated file. fs.writeFileSync(path.join(tmpDir, 'extra.txt'), 'extra\n'); - execSync('git add extra.txt', { cwd: tmpDir, stdio: 'pipe' }); + gitOrThrow(['add', 'extra.txt'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); runGsdTools(['commit', 'docs: default commit'], tmpDir); // Default path (no --files) commits everything staged. - const diffOutput = execSync('git diff HEAD~1 HEAD --name-only', { + const diffOutput = gitOrThrow(['diff', 'HEAD~1', 'HEAD', '--name-only'], { cwd: tmpDir, - encoding: 'utf-8', + timeoutMs: GIT_TIMEOUT_MS, }); const files = diffOutput.trim().split('\n').sort(); assert.ok( @@ -111,8 +114,8 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { test('missing tracked file in --files is still not committed as deletion (#2014 guard)', () => { // Create and commit STATE.md, then remove it from disk. fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), '# State\n'); - execSync('git add .planning/STATE.md', { cwd: tmpDir, stdio: 'pipe' }); - execSync('git commit -m "add STATE.md"', { cwd: tmpDir, stdio: 'pipe' }); + gitOrThrow(['add', '.planning/STATE.md'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'add STATE.md'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); fs.unlinkSync(path.join(tmpDir, '.planning', 'STATE.md')); // Also create a valid file to commit. @@ -123,9 +126,9 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { tmpDir, ); - const diffOutput = execSync('git diff HEAD~1 HEAD --name-status', { + const diffOutput = gitOrThrow(['diff', 'HEAD~1', 'HEAD', '--name-status'], { cwd: tmpDir, - encoding: 'utf-8', + timeoutMs: GIT_TIMEOUT_MS, }); assert.ok( !diffOutput.includes('D\t.planning/STATE.md'), @@ -140,13 +143,13 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { test('commit --files with only missing files returns nothing_to_commit', () => { // Create and commit STATE.md, then remove it from disk. fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), '# State\n'); - execSync('git add .planning/STATE.md', { cwd: tmpDir, stdio: 'pipe' }); - execSync('git commit -m "add STATE.md"', { cwd: tmpDir, stdio: 'pipe' }); + gitOrThrow(['add', '.planning/STATE.md'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'add STATE.md'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); fs.unlinkSync(path.join(tmpDir, '.planning', 'STATE.md')); // Stage an unrelated file so the index is non-empty. fs.writeFileSync(path.join(tmpDir, 'extra.txt'), 'extra\n'); - execSync('git add extra.txt', { cwd: tmpDir, stdio: 'pipe' }); + gitOrThrow(['add', 'extra.txt'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); const result = runGsdTools( ['commit', 'docs: try', '--files', '.planning/STATE.md'], @@ -164,9 +167,9 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { ); // The unrelated staged file must still be staged, not committed. - const statusOutput = execSync('git status --porcelain', { + const statusOutput = gitOrThrow(['status', '--porcelain'], { cwd: tmpDir, - encoding: 'utf-8', + timeoutMs: GIT_TIMEOUT_MS, }).trim(); assert.ok( statusOutput.includes('extra.txt'), @@ -185,7 +188,7 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { assert.strictEqual(parsed.committed, true, `absolute path must commit, not nothing_to_commit: ${res.output}`); // The absolute path must land in the commit, normalized to repo-relative. - const diff = execSync('git diff HEAD~1 HEAD --name-only', { cwd: tmpDir, encoding: 'utf-8' }).trim(); + const diff = gitOrThrow(['diff', 'HEAD~1', 'HEAD', '--name-only'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(); assert.strictEqual(diff, '.planning/A.md', `absolute --files path must be committed (normalized to relative); got: ${diff}`); }); @@ -202,7 +205,7 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { const parsed = JSON.parse(res.output); assert.strictEqual(parsed.committed, true, `mixed list must commit: ${res.output}`); - const diff = execSync('git diff HEAD~1 HEAD --name-only', { cwd: tmpDir, encoding: 'utf-8' }) + const diff = gitOrThrow(['diff', 'HEAD~1', 'HEAD', '--name-only'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }) .trim().split('\n').sort(); assert.deepStrictEqual( diff, @@ -243,10 +246,10 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { assert.match(parsed.error, /outside repository/, "git's own rejection message must be preserved (#2608)"); // No new commit created (still at the single initial commit). - const logCount = execSync('git rev-list --count HEAD', { cwd: tmpDir, encoding: 'utf-8' }).trim(); + const logCount = gitOrThrow(['rev-list', '--count', 'HEAD'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(); assert.strictEqual(logCount, '1', 'no new commit must be created for an out-of-repo path'); // Index stays clean (git add failed → nothing staged). - const status = execSync('git status --porcelain', { cwd: tmpDir, encoding: 'utf-8' }).trim(); + const status = gitOrThrow(['status', '--porcelain'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(); assert.strictEqual(status, '', `index must be clean (no pollution): ${status}`); }); }); diff --git a/tests/execute-phase-worktree-guard.test.cjs b/tests/execute-phase-worktree-guard.test.cjs index 9c3756149..9935716bb 100644 --- a/tests/execute-phase-worktree-guard.test.cjs +++ b/tests/execute-phase-worktree-guard.test.cjs @@ -10,14 +10,19 @@ 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 { runHook } = require('./helpers/process-seam.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); const ROOT = path.resolve(__dirname, '..'); const WORKFLOW = path.join(ROOT, 'gsd-core', 'workflows', 'execute-phase.md'); const GUARD_MARKER = 'gsd:guard=orchestrator-cwd-drift'; +// 30s: git plumbing (init/config/add/commit/checkout/rev-parse) against a +// small mkdtemp fixture repo — already the file's calibrated bound +// pre-migration (see runGuard below), reused here for consistency. +const GIT_TIMEOUT_MS = 30_000; + /** * Pull the guard's bash block out of the workflow. Anchored on a stable marker * comment rather than a line range so the test does not rot when the file moves. @@ -35,8 +40,7 @@ function guardScript() { ); } -const git = (cwd, ...args) => - execFileSync('git', args, { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); +const git = (cwd, ...args) => gitOrThrow(args, { cwd, timeoutMs: GIT_TIMEOUT_MS }); /** A real repo with a base branch and one commit. */ function makeRepo() { diff --git a/tests/fix-2608-commit-staging-failure.test.cjs b/tests/fix-2608-commit-staging-failure.test.cjs index db5d50d03..c80a290fa 100644 --- a/tests/fix-2608-commit-staging-failure.test.cjs +++ b/tests/fix-2608-commit-staging-failure.test.cjs @@ -43,9 +43,14 @@ 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 { spawnSync } = require('node:child_process'); const { createTempGitProject, cleanup } = require('./helpers.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); + +// 5000ms: git plumbing (add/commit/status/rev-parse/rev-list/diff) on a small +// mkdtemp fixture repo — well over any observed duration for that class of call. +const GIT_TIMEOUT_MS = 5000; const LIB = path.join(__dirname, '..', 'gsd-core', 'bin', 'lib'); @@ -117,11 +122,11 @@ cmdCommit(${JSON.stringify(cwd)}, 'docs: map existing codebase', ${JSON.stringif } function headCount(cwd) { - return Number(execFileSync('git', ['rev-list', '--count', 'HEAD'], { cwd, encoding: 'utf-8' }).trim()); + return Number(gitOrThrow(['rev-list', '--count', 'HEAD'], { cwd, timeoutMs: GIT_TIMEOUT_MS }).trim()); } function committedFiles(cwd) { - return execFileSync('git', ['diff', 'HEAD~1', 'HEAD', '--name-only'], { cwd, encoding: 'utf-8' }) + return gitOrThrow(['diff', 'HEAD~1', 'HEAD', '--name-only'], { cwd, timeoutMs: GIT_TIMEOUT_MS }) .trim().split('\n').filter(Boolean).sort(); } @@ -183,11 +188,11 @@ describe('#2608: commit-to-subrepo fails closed when git add fails', () => { subDir = path.join(rootDir, 'backend'); fs.mkdirSync(subDir, { recursive: true }); for (const [cmd, args] of [['init', []], ['config', ['user.email', 'test@example.com']], ['config', ['user.name', 'Test']]]) { - execFileSync('git', [cmd, ...args], { cwd: subDir, stdio: 'pipe' }); + gitOrThrow([cmd, ...args], { cwd: subDir, timeoutMs: GIT_TIMEOUT_MS }); } fs.writeFileSync(path.join(subDir, 'seed.js'), '// seed\n'); - execFileSync('git', ['add', 'seed.js'], { cwd: subDir, stdio: 'pipe' }); - execFileSync('git', ['commit', '-m', 'seed'], { cwd: subDir, stdio: 'pipe' }); + gitOrThrow(['add', 'seed.js'], { cwd: subDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'seed'], { cwd: subDir, timeoutMs: GIT_TIMEOUT_MS }); fs.writeFileSync(path.join(subDir, 'a.js'), '// a\n'); fs.writeFileSync(path.join(subDir, 'b.js'), '// b\n'); }); @@ -211,7 +216,7 @@ describe('#2608: commit-to-subrepo fails closed when git add fails', () => { "git's original stderr must be preserved"); assert.equal(headCount(subDir), before, 'no partial sub-repo commit may be created'); - const status = execFileSync('git', ['status', '--porcelain'], { cwd: subDir, encoding: 'utf-8' }); + const status = gitOrThrow(['status', '--porcelain'], { cwd: subDir, timeoutMs: GIT_TIMEOUT_MS }); assert.deepEqual(status.split('\n').filter((l) => /^A[ \t]/.test(l)), [], `the sub-repo index must be rolled back, status:\n${status}`); }); @@ -397,7 +402,7 @@ describe('#2608: commit --files fails closed when git add fails', () => { test('successful staging still commits exactly the declared scope', () => { const before = headCount(tmpDir); fs.writeFileSync(path.join(tmpDir, 'unrelated-wip.txt'), 'wip\n'); - execFileSync('git', ['add', 'unrelated-wip.txt'], { cwd: tmpDir, stdio: 'pipe' }); + gitOrThrow(['add', 'unrelated-wip.txt'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); const { result } = commitWithFailingAdd({ cwd: tmpDir, @@ -443,7 +448,7 @@ describe('#2608: commit --files fails closed when git add fails', () => { failFor: ['.planning/CONCERNS.md'], }); - const status = execFileSync('git', ['status', '--porcelain'], { cwd: tmpDir, encoding: 'utf-8' }); + const status = gitOrThrow(['status', '--porcelain'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); const stagedAdds = status.split('\n').filter((l) => /^A[ \t]/.test(l)); assert.deepEqual(stagedAdds, [], `no path may remain staged after a staging failure, status:\n${status}`); @@ -453,7 +458,7 @@ describe('#2608: commit --files fails closed when git add fails', () => { // Boundary: the reset must touch only what THIS call staged. Unstaging a // path the caller staged themselves would destroy their work. fs.writeFileSync(path.join(tmpDir, 'caller-staged.txt'), 'mine\n'); - execFileSync('git', ['add', 'caller-staged.txt'], { cwd: tmpDir, stdio: 'pipe' }); + gitOrThrow(['add', 'caller-staged.txt'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); commitWithFailingAdd({ cwd: tmpDir, @@ -461,7 +466,7 @@ describe('#2608: commit --files fails closed when git add fails', () => { failFor: ['.planning/CONCERNS.md'], }); - const status = execFileSync('git', ['status', '--porcelain'], { cwd: tmpDir, encoding: 'utf-8' }); + const status = gitOrThrow(['status', '--porcelain'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); assert.match(status, /^A[ \t]+caller-staged\.txt$/m, `the caller's own staged file must survive the rollback, status:\n${status}`); }); @@ -486,7 +491,7 @@ describe('#2608: commit --files fails closed when git add fails', () => { test('a failed default-mode git add blocks --amend too', () => { // --amend has no carve-out: amending on top of a failed staging would // rewrite the tip without the changes the caller asked for. - const before = execFileSync('git', ['rev-parse', 'HEAD'], { cwd: tmpDir, encoding: 'utf-8' }).trim(); + const before = gitOrThrow(['rev-parse', 'HEAD'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(); const { result, gitCalls } = commitWithFailingAdd({ cwd: tmpDir, files: undefined, @@ -497,7 +502,7 @@ describe('#2608: commit --files fails closed when git add fails', () => { assert.equal(result.reason, 'staging_failed'); assert.ok(!gitCalls.some((a) => a[0] === 'commit'), 'git commit --amend must not run'); assert.equal( - execFileSync('git', ['rev-parse', 'HEAD'], { cwd: tmpDir, encoding: 'utf-8' }).trim(), + gitOrThrow(['rev-parse', 'HEAD'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(), before, 'HEAD must not be rewritten when staging failed', ); diff --git a/tests/git-base-branch.test.cjs b/tests/git-base-branch.test.cjs index 1e0d0ed84..9f0b6ffc4 100644 --- a/tests/git-base-branch.test.cjs +++ b/tests/git-base-branch.test.cjs @@ -28,10 +28,19 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); -const { execSync } = require('node:child_process'); const { runGsdTools, cleanup, readFileNormalized } = require('./helpers.cjs'); const { makeFaultyGit } = require('./helpers/faulty-deps.cjs'); +const { gitOrThrow, throwIfFailed } = require('./helpers/git-fixture.cjs'); +const { runHook } = require('./helpers/process-seam.cjs'); + +/** + * Bound for every subprocess in this file: git plumbing (init/config/add/ + * commit/clone/symbolic-ref/remote/branch) against small mkdtemp fixture + * repos, plus the one short handle_branching bash-script run below — all + * orders of magnitude under this. #3144. + */ +const GIT_TIMEOUT_MS = 15000; // ─── helpers ────────────────────────────────────────────────────────────────── @@ -42,14 +51,14 @@ const { makeFaultyGit } = require('./helpers/faulty-deps.cjs'); function createGitRepo(opts = {}) { const { prefix = 'gsd-1146-', defaultBranch = 'master' } = opts; const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); - execSync(`git init -b ${defaultBranch}`, { cwd: dir, stdio: 'pipe' }); - execSync('git config user.email "test@test.com"', { cwd: dir, stdio: 'pipe' }); - execSync('git config user.name "Test"', { cwd: dir, stdio: 'pipe' }); - execSync('git config commit.gpgsign false', { cwd: dir, stdio: 'pipe' }); + gitOrThrow(['init', '-b', defaultBranch], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'user.email', 'test@test.com'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'user.name', 'Test'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'commit.gpgsign', 'false'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); // Need at least one commit so branches exist fs.writeFileSync(path.join(dir, 'README.md'), '# test\n'); - execSync('git add README.md', { cwd: dir, stdio: 'pipe' }); - execSync('git commit -m "init"', { cwd: dir, stdio: 'pipe' }); + gitOrThrow(['add', 'README.md'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'init'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); return dir; } @@ -125,13 +134,13 @@ describe('#1146: git.base-branch resolver', () => { t.after(() => { cleanup(originDir); cleanup(worktreeDir); }); // Clone from origin — this sets origin/HEAD - execSync(`git clone "${originDir}" "${worktreeDir}"`, { stdio: 'pipe' }); - execSync('git config user.email "test@test.com"', { cwd: worktreeDir, stdio: 'pipe' }); - execSync('git config user.name "Test"', { cwd: worktreeDir, stdio: 'pipe' }); + gitOrThrow(['clone', originDir, worktreeDir], { timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'user.email', 'test@test.com'], { cwd: worktreeDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'user.name', 'Test'], { cwd: worktreeDir, timeoutMs: GIT_TIMEOUT_MS }); addPlanning(worktreeDir); // Verify origin/HEAD is set (it should be after clone) - const symref = execSync('git symbolic-ref refs/remotes/origin/HEAD', { cwd: worktreeDir, encoding: 'utf8' }).trim(); + const symref = gitOrThrow(['symbolic-ref', 'refs/remotes/origin/HEAD'], { cwd: worktreeDir, timeoutMs: GIT_TIMEOUT_MS }).trim(); assert.ok(symref.includes('origin/main'), `Expected origin/HEAD→origin/main, got: ${symref}`); const result = runGsdTools(['query', 'git.base-branch'], worktreeDir); @@ -149,22 +158,22 @@ describe('#1146: git.base-branch resolver', () => { t.after(() => { cleanup(originDir); cleanup(cloneDir); }); // Manually add remote WITHOUT cloning (so origin/HEAD is never set) - execSync('git init', { cwd: cloneDir, stdio: 'pipe' }); - execSync('git config user.email "test@test.com"', { cwd: cloneDir, stdio: 'pipe' }); - execSync('git config user.name "Test"', { cwd: cloneDir, stdio: 'pipe' }); - execSync('git config commit.gpgsign false', { cwd: cloneDir, stdio: 'pipe' }); - execSync(`git remote add origin "${originDir}"`, { cwd: cloneDir, stdio: 'pipe' }); - execSync('git fetch origin', { cwd: cloneDir, stdio: 'pipe' }); + gitOrThrow(['init'], { cwd: cloneDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'user.email', 'test@test.com'], { cwd: cloneDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'user.name', 'Test'], { cwd: cloneDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'commit.gpgsign', 'false'], { cwd: cloneDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['remote', 'add', 'origin', originDir], { cwd: cloneDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['fetch', 'origin'], { cwd: cloneDir, timeoutMs: GIT_TIMEOUT_MS }); // Explicitly delete origin/HEAD in case git fetch auto-set it (newer git versions may do this) try { - execSync('git remote set-head origin --delete', { cwd: cloneDir, stdio: 'pipe' }); + gitOrThrow(['remote', 'set-head', 'origin', '--delete'], { cwd: cloneDir, timeoutMs: GIT_TIMEOUT_MS }); } catch (_) { /* ignore — may not exist */ } addPlanning(cloneDir); // Confirm origin/HEAD is unset let hasSymref = true; try { - execSync('git symbolic-ref refs/remotes/origin/HEAD', { cwd: cloneDir, stdio: 'pipe' }); + gitOrThrow(['symbolic-ref', 'refs/remotes/origin/HEAD'], { cwd: cloneDir, timeoutMs: GIT_TIMEOUT_MS }); } catch (_) { hasSymref = false; } @@ -237,8 +246,7 @@ describe('#1146: git.base-branch resolver', () => { t.after(() => cleanup(dir)); addPlanning(dir); // Create a "main" branch alongside the existing "master" - const { execSync: exec } = require('node:child_process'); - exec('git branch main', { cwd: dir, stdio: 'pipe' }); + gitOrThrow(['branch', 'main'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); // No remote configured — falls to tier-4 (local branch existence) const result = runGsdTools(['query', 'git.base-branch'], dir); @@ -768,7 +776,7 @@ describe('#3057 W3: gitWorktreeInfoInternal — no work tree, and git failing mi // and it is reachable without any injection. const dir = createTempDir('gsd-3057-w3-bare-'); t.after(() => cleanup(dir)); - execSync('git init --bare', { cwd: dir, stdio: 'pipe' }); + gitOrThrow(['init', '--bare'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); assert.deepStrictEqual( gitBaseBranch.gitWorktreeInfoInternal(dir), @@ -1040,7 +1048,6 @@ describe('bug #2004: pr-branch preserves structural planning commits', () => { const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); -const { execFileSync } = require('node:child_process'); const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); @@ -1064,13 +1071,7 @@ const GIT_ENV = Object.freeze({ }); function git(cwd, ...args) { - return execFileSync('git', args, { - cwd, - env: GIT_ENV, - stdio: ['pipe', 'pipe', 'pipe'], - }) - .toString() - .trim(); + return gitOrThrow(args, { cwd, env: GIT_ENV, timeoutMs: GIT_TIMEOUT_MS }).trim(); } /** @@ -1178,11 +1179,9 @@ function runHandleBranchingStep(bash, cwd, branchName) { const script = `#!/usr/bin/env bash\nset -uo pipefail\nBRANCH_NAME="${branchName}"\n${bash}\n`; fs.writeFileSync(scriptPath, script, { mode: 0o755 }); try { - return execFileSync('bash', [scriptPath], { - cwd, - env: GIT_ENV, - stdio: ['pipe', 'pipe', 'pipe'], - }).toString(); + const r = runHook(scriptPath, [], { interpreter: 'bash', cwd, env: GIT_ENV, timeoutMs: GIT_TIMEOUT_MS }); + throwIfFailed(r, `runHandleBranchingStep: bash ${scriptPath}`); + return r.stdout; } finally { cleanup(scriptDir); } diff --git a/tests/git-fixture.test.cjs b/tests/git-fixture.test.cjs index 8102eac14..2572040af 100644 --- a/tests/git-fixture.test.cjs +++ b/tests/git-fixture.test.cjs @@ -30,7 +30,7 @@ const { OUTCOME } = processSeam; const runGitSpy = mock.method(processSeam, 'runGit'); after(() => mock.restoreAll()); -const { gitOrThrow, DEFAULT_GIT_TIMEOUT_MS } = require('./helpers/git-fixture.cjs'); +const { gitOrThrow, throwIfFailed, DEFAULT_GIT_TIMEOUT_MS } = require('./helpers/git-fixture.cjs'); const { createTempDir, cleanup } = require('./helpers.cjs'); /** @@ -211,3 +211,75 @@ describe('git-fixture: E — gitOrThrow', () => { assert.equal(author, 'Env Author'); }); }); + +/** + * `throwIfFailed` is exercised only indirectly above (via `gitOrThrow`) and by + * six other per-suite wrappers elsewhere in the tree. It takes a plain + * process-seam result object, so it is tested directly here with literal + * result objects — no subprocess needed. + */ +describe('throwIfFailed', () => { + /** A minimal clean-exit result, spread over in each test below. */ + const BASE = { stdout: '', stderr: '', signal: null, timedOut: false }; + + test('does not throw for outcome=EXITED, exitCode=0', () => { + assert.doesNotThrow(() => throwIfFailed({ ...BASE, outcome: OUTCOME.EXITED, exitCode: 0 }, 'ok')); + }); + + test('throws when outcome=EXITED but exitCode=1', () => { + assert.throws(() => throwIfFailed({ ...BASE, outcome: OUTCOME.EXITED, exitCode: 1 }, 'bad')); + }); + + test('throws when outcome=EXITED but exitCode=128', () => { + assert.throws(() => throwIfFailed({ ...BASE, outcome: OUTCOME.EXITED, exitCode: 128 }, 'bad')); + }); + + for (const outcome of [OUTCOME.TIMED_OUT, OUTCOME.KILLED, OUTCOME.BUFFER_OVERFLOW, OUTCOME.SPAWN_FAILED]) { + test(`throws for non-EXITED outcome: ${outcome}`, () => { + assert.throws(() => throwIfFailed({ ...BASE, outcome, exitCode: null }, 'bad')); + }); + } + + test('boundary: exitCode=0 with a non-EXITED outcome still throws (0 alone is not success)', () => { + assert.throws(() => throwIfFailed({ ...BASE, outcome: OUTCOME.KILLED, exitCode: 0 }, 'bad')); + }); + + test('thrown error carries .status and .exitCode as equal aliases of the same value', () => { + const caught = captureThrown(() => throwIfFailed({ ...BASE, outcome: OUTCOME.EXITED, exitCode: 42 }, 'bad')); + assert.equal(caught.status, 42); + assert.equal(caught.exitCode, 42); + assert.equal(caught.status, caught.exitCode); + }); + + test('thrown error carries stdout, stderr, signal, timedOut, outcome from the input result', () => { + const input = { + outcome: OUTCOME.KILLED, + exitCode: null, + stdout: 'input stdout', + stderr: 'input stderr', + signal: 'SIGTERM', + timedOut: false, + }; + const caught = captureThrown(() => throwIfFailed(input, 'bad')); + assert.equal(caught.stdout, input.stdout); + assert.equal(caught.stderr, input.stderr); + assert.equal(caught.signal, input.signal); + assert.equal(caught.timedOut, input.timedOut); + assert.equal(caught.outcome, input.outcome); + }); + + test('exitCode: null propagates to .status as null, not coerced', () => { + const caught = captureThrown(() => + throwIfFailed({ ...BASE, outcome: OUTCOME.TIMED_OUT, exitCode: null, timedOut: true }, 'bad') + ); + assert.equal(caught.status, null); + assert.notEqual(caught.status, undefined); + }); + + test('displayName appears in the thrown error message', () => { + const caught = captureThrown(() => + throwIfFailed({ ...BASE, outcome: OUTCOME.EXITED, exitCode: 1 }, 'my distinctive display name') + ); + assert.ok(caught.message.includes('my distinctive display name')); + }); +}); diff --git a/tests/gsd-write-guard.property.test.cjs b/tests/gsd-write-guard.property.test.cjs index 47809dbb8..3e64fe7ee 100644 --- a/tests/gsd-write-guard.property.test.cjs +++ b/tests/gsd-write-guard.property.test.cjs @@ -22,12 +22,18 @@ 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 fc = require('./helpers/fast-check-setup.cjs'); const { createTempDir, cleanup } = require('./helpers.cjs'); +const { runHook } = require('./helpers/process-seam.cjs'); const HOOK_PATH = path.join(__dirname, '..', 'hooks', 'gsd-write-guard.js'); +// 5000ms: this hook does a handful of sync fs reads/stat calls on a small +// fixture file and exits — a hang here would stall every fast-check sample +// (40+ per run), so the bound is kept low rather than reused from a slower +// class of call. +const HOOK_TIMEOUT_MS = 5000; + // Mirror the hook's published contract (hooks/gsd-write-guard.js). const SHRINK_RATIO = 0.4; const FLOOR_LINES = 40; @@ -53,16 +59,16 @@ function guardVerdict(oldLines, newLines) { fs.writeFileSync(roadmapPath, lines(oldLines)); const env = { ...process.env }; delete env.GSD_ALLOW_PLANNING_SHRINK; - const r = spawnSync(process.execPath, [HOOK_PATH], { + const r = runHook(HOOK_PATH, [], { input: JSON.stringify({ hook_event_name: 'PreToolUse', tool_name: 'Write', tool_input: { file_path: roadmapPath, content: lines(newLines) }, }), - encoding: 'utf8', env, + timeoutMs: HOOK_TIMEOUT_MS, }); - return r.status === 2 ? 'blocked' : 'passed'; + return r.exitCode === 2 ? 'blocked' : 'passed'; } describe('gsd-write-guard.js: SHRINK_RATIO/FLOOR_LINES budget contract (property)', () => { diff --git a/tests/helpers/git-fixture.cjs b/tests/helpers/git-fixture.cjs index 55880c1d0..c480f7543 100644 --- a/tests/helpers/git-fixture.cjs +++ b/tests/helpers/git-fixture.cjs @@ -1,10 +1,11 @@ 'use strict'; /** - * git-fixture — a throw-preserving wrapper over the process seam's `runGit`. + * git-fixture — the shared throw-on-failure mechanism for process-seam + * results, plus a throw-preserving wrapper over the seam's `runGit`. * * Why this exists: `execSync`/`execFileSync` throw on any non-zero exit, - * and 237 sites in this repo's test suite are written against that throw — + * and 237+ sites in this repo's test suite are written against that throw — * they read `err.status`, `err.stdout`, `err.stderr`. `tests/helpers/ * process-seam.cjs` deliberately never throws (see its own header): every * outcome, including a non-zero exit, a timeout, or a spawn failure, comes @@ -14,11 +15,18 @@ * a quiet one (a result object nobody checked) — exactly the kind of * regression a migration must not introduce. * - * `gitOrThrow` is that bridge: it calls the seam's `runGit` and re-throws in - * the shape the old idiom produced, with the seam's typed fields attached - * alongside it. `tests/helpers/process-seam.cjs` itself is NOT modified by - * this module — its never-throws contract is intact; this is a layer on - * top, not a change underneath. + * `throwIfFailed` is that mechanism: given any process-seam result and a + * human-readable name for what ran, it throws in the shape the legacy + * `execSync`/`execFileSync` idiom produced, with the seam's typed fields + * attached alongside it — or returns quietly on a clean exit. `gitOrThrow` + * is `throwIfFailed` specialized to `runGit`. Every other local test helper + * that needs the same throw-on-failure bridge (over `runNode`, `runHook`, + * etc.) calls `throwIfFailed` directly instead of hand-rolling its own copy + * of this shape — five call sites did exactly that before this module + * exported it, and drifted from each other in the process (#3144). + * `tests/helpers/process-seam.cjs` itself is NOT modified by this module — + * its never-throws contract is intact; this is a layer on top, not a change + * underneath. */ const { runGit, OUTCOME } = require('./process-seam.cjs'); @@ -35,14 +43,18 @@ const { runGit, OUTCOME } = require('./process-seam.cjs'); const DEFAULT_GIT_TIMEOUT_MS = 15000; /** - * Run `git` via the process seam and throw on anything other than a clean - * exit, preserving the legacy `execSync`/`execFileSync` throw-on-failure - * idiom that existing test code is written against. + * Throw on anything other than a clean (exit 0) process-seam result, + * preserving the legacy `execSync`/`execFileSync` throw-on-failure idiom + * that existing test code is written against. Returns quietly (no return + * value) on a clean exit — callers that need `stdout` read it off `result` + * themselves; this only decides whether to throw. * - * @param {string[]} args - argv passed to git (never shell-interpreted). - * @param {object} [options] - forwarded to `runGit`; see process-seam.cjs. - * `options.timeoutMs`, if provided, overrides `DEFAULT_GIT_TIMEOUT_MS`. - * @returns {string} `stdout` on a clean (exit 0) run. + * @param {object} result - a process-seam result: `{outcome, exitCode, + * stdout, stderr, timedOut, signal}` (plus any seam-specific fields, + * e.g. `code`, which are ignored here). + * @param {string} displayName - human string naming what ran, e.g. + * `'git commit -m seed'` or `'bash '`. Embedded in + * the thrown message so failures are attributable at a glance. * @throws {Error} On any non-zero exit, timeout, kill, or spawn failure. * The thrown error carries, as own properties: * - `status` — the exit code (the legacy `execSync`/`execFileSync` name; @@ -56,6 +68,36 @@ const DEFAULT_GIT_TIMEOUT_MS = 15000; * - `timedOut` — the seam's `timedOut` field. * - `outcome` — the seam's `OUTCOME` discriminant. */ +function throwIfFailed(result, displayName) { + if (result.outcome === OUTCOME.EXITED && result.exitCode === 0) { + return; + } + + const err = new Error( + `${displayName} failed — outcome=${result.outcome} exitCode=${result.exitCode} ` + + `stderr=${result.stderr.trim()}` + ); + err.status = result.exitCode; + err.exitCode = result.exitCode; + err.stdout = result.stdout; + err.stderr = result.stderr; + err.signal = result.signal; + err.timedOut = result.timedOut; + err.outcome = result.outcome; + throw err; +} + +/** + * Run `git` via the process seam and throw on anything other than a clean + * exit, preserving the legacy `execSync`/`execFileSync` throw-on-failure + * idiom that existing test code is written against. + * + * @param {string[]} args - argv passed to git (never shell-interpreted). + * @param {object} [options] - forwarded to `runGit`; see process-seam.cjs. + * `options.timeoutMs`, if provided, overrides `DEFAULT_GIT_TIMEOUT_MS`. + * @returns {string} `stdout` on a clean (exit 0) run. + * @throws {Error} See `throwIfFailed` for the exact shape thrown. + */ function gitOrThrow(args, options = {}) { // Destructure (not spread-after) so an explicit `timeoutMs: undefined` in // `options` still resolves to the default: a destructure default applies @@ -65,23 +107,8 @@ function gitOrThrow(args, options = {}) { const { timeoutMs = DEFAULT_GIT_TIMEOUT_MS, ...rest } = options; const r = runGit(args, { ...rest, timeoutMs }); - if (r.outcome === OUTCOME.EXITED && r.exitCode === 0) { - return r.stdout; - } - - const argvDisplay = ['git', ...args].join(' '); - const err = new Error( - `gitOrThrow: \`${argvDisplay}\` failed — outcome=${r.outcome} exitCode=${r.exitCode} ` + - `stderr=${r.stderr.trim()}` - ); - err.status = r.exitCode; - err.exitCode = r.exitCode; - err.stdout = r.stdout; - err.stderr = r.stderr; - err.signal = r.signal; - err.timedOut = r.timedOut; - err.outcome = r.outcome; - throw err; + throwIfFailed(r, `gitOrThrow: \`${['git', ...args].join(' ')}\``); + return r.stdout; } -module.exports = { gitOrThrow, DEFAULT_GIT_TIMEOUT_MS }; +module.exports = { gitOrThrow, throwIfFailed, DEFAULT_GIT_TIMEOUT_MS }; diff --git a/tests/no-unbounded-spawn-allowlist.test.cjs b/tests/no-unbounded-spawn-allowlist.test.cjs index 4ab2083e7..441fb7b35 100644 --- a/tests/no-unbounded-spawn-allowlist.test.cjs +++ b/tests/no-unbounded-spawn-allowlist.test.cjs @@ -30,8 +30,9 @@ const REPO_ROOT = path.join(__dirname, '..'); // The allowlist only ratchets DOWN. This baseline is the length observed at // the time this guard was written (139 entries) — each future migration // wave lowers it as files are moved off the allowlist by adding real -// timeouts; it must never grow back up. -const BASELINE = 139; +// timeouts; it must never grow back up. Lowered to 120 by the #3144 Wave-1 +// process-seam migration (19 files' unbounded spawns bounded). +const BASELINE = 120; function readAllowlist() { const raw = fs.readFileSync(ALLOWLIST_PATH, 'utf8'); diff --git a/tests/prune-orphaned-worktrees.test.cjs b/tests/prune-orphaned-worktrees.test.cjs index 514f4f062..a3aba2105 100644 --- a/tests/prune-orphaned-worktrees.test.cjs +++ b/tests/prune-orphaned-worktrees.test.cjs @@ -11,8 +11,13 @@ const { describe, test, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); -const { execSync } = require('child_process'); const { createTempDir, cleanup } = require('./helpers.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); + +// 15000ms: git plumbing (init/config/add/commit/branch/worktree/merge/checkout) +// on a small mkdtemp fixture repo — far over any observed duration for that +// class of call. +const GIT_TIMEOUT_MS = 15000; // Lazy-loaded so tests can fail clearly when the export doesn't exist yet. function getPruneOrphanedWorktrees() { @@ -30,7 +35,7 @@ function canonicalPath(p) { } function listedWorktreePaths(repoDir) { - const out = execSync('git worktree list --porcelain', { cwd: repoDir, encoding: 'utf8' }); + const out = gitOrThrow(['worktree', 'list', '--porcelain'], { cwd: repoDir, timeoutMs: GIT_TIMEOUT_MS }); return new Set( out .split('\n') @@ -41,16 +46,16 @@ function listedWorktreePaths(repoDir) { function createGitRepo(dir) { fs.mkdirSync(dir, { recursive: true }); - execSync('git init', { cwd: dir, stdio: 'pipe' }); - execSync('git config user.email "test@test.com"', { cwd: dir, stdio: 'pipe' }); - execSync('git config user.name "Test"', { cwd: dir, stdio: 'pipe' }); - execSync('git config commit.gpgsign false', { cwd: dir, stdio: 'pipe' }); + gitOrThrow(['init'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'user.email', 'test@test.com'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'user.name', 'Test'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'commit.gpgsign', 'false'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); fs.writeFileSync(path.join(dir, 'README.md'), '# Test\n'); - execSync('git add -A', { cwd: dir, stdio: 'pipe' }); - execSync('git commit -m "initial commit"', { cwd: dir, stdio: 'pipe' }); + gitOrThrow(['add', '-A'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'initial commit'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); // Rename to main if it isn't already (handles older git defaults) try { - execSync('git branch -m master main', { cwd: dir, stdio: 'pipe' }); + gitOrThrow(['branch', '-m', 'master', 'main'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); } catch { /* already named main */ } } @@ -75,16 +80,16 @@ describe('pruneOrphanedWorktrees', () => { createGitRepo(repoDir); // Create worktree on a new branch (main is checked out in repoDir) - execSync('git worktree add "' + worktreeDir + '" -b fix/old-work', { cwd: repoDir, stdio: 'pipe' }); + gitOrThrow(['worktree', 'add', worktreeDir, '-b', 'fix/old-work'], { cwd: repoDir, timeoutMs: GIT_TIMEOUT_MS }); assert.ok(fs.existsSync(worktreeDir), 'worktree dir should exist before prune'); // Add a commit in the worktree fs.writeFileSync(path.join(worktreeDir, 'feature.txt'), 'work\n'); - execSync('git add -A', { cwd: worktreeDir, stdio: 'pipe' }); - execSync('git commit -m "old work"', { cwd: worktreeDir, stdio: 'pipe' }); + gitOrThrow(['add', '-A'], { cwd: worktreeDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'old work'], { cwd: worktreeDir, timeoutMs: GIT_TIMEOUT_MS }); // Merge the branch into main from repoDir - execSync('git merge fix/old-work --no-ff -m "merge old-work"', { cwd: repoDir, stdio: 'pipe' }); + gitOrThrow(['merge', 'fix/old-work', '--no-ff', '-m', 'merge old-work'], { cwd: repoDir, timeoutMs: GIT_TIMEOUT_MS }); // Act const pruneOrphanedWorktrees = getPruneOrphanedWorktrees(); @@ -112,12 +117,12 @@ describe('pruneOrphanedWorktrees', () => { createGitRepo(repoDir); // Create the worktree on a new branch (main is checked out in repoDir) - execSync('git worktree add "' + worktreeDir + '" -b fix/active-work', { cwd: repoDir, stdio: 'pipe' }); + gitOrThrow(['worktree', 'add', worktreeDir, '-b', 'fix/active-work'], { cwd: repoDir, timeoutMs: GIT_TIMEOUT_MS }); // Add a commit in the worktree (NOT merged into main) fs.writeFileSync(path.join(worktreeDir, 'active.txt'), 'active\n'); - execSync('git add -A', { cwd: worktreeDir, stdio: 'pipe' }); - execSync('git commit -m "active work"', { cwd: worktreeDir, stdio: 'pipe' }); + gitOrThrow(['add', '-A'], { cwd: worktreeDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'active work'], { cwd: worktreeDir, timeoutMs: GIT_TIMEOUT_MS }); // main stays at its original commit — no merge // Act @@ -139,12 +144,12 @@ describe('pruneOrphanedWorktrees', () => { createGitRepo(repoDir); // Create a worktree, add a commit, merge it into main - execSync('git worktree add "' + wtDir + '" -b fix/another-merged', { cwd: repoDir, stdio: 'pipe' }); + gitOrThrow(['worktree', 'add', wtDir, '-b', 'fix/another-merged'], { cwd: repoDir, timeoutMs: GIT_TIMEOUT_MS }); fs.writeFileSync(path.join(wtDir, 'more.txt'), 'more\n'); - execSync('git add -A', { cwd: wtDir, stdio: 'pipe' }); - execSync('git commit -m "another merged"', { cwd: wtDir, stdio: 'pipe' }); - execSync('git checkout main', { cwd: repoDir, stdio: 'pipe' }); - execSync('git merge fix/another-merged --no-ff -m "merge another"', { cwd: repoDir, stdio: 'pipe' }); + gitOrThrow(['add', '-A'], { cwd: wtDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'another merged'], { cwd: wtDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['checkout', 'main'], { cwd: repoDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['merge', 'fix/another-merged', '--no-ff', '-m', 'merge another'], { cwd: repoDir, timeoutMs: GIT_TIMEOUT_MS }); // Run pruning const pruneOrphanedWorktrees = getPruneOrphanedWorktrees(); @@ -168,7 +173,7 @@ describe('pruneOrphanedWorktrees', () => { createGitRepo(repoDir); // Create a worktree - execSync('git worktree add "' + worktreeDir + '" -b fix/stale-ref', { cwd: repoDir, stdio: 'pipe' }); + gitOrThrow(['worktree', 'add', worktreeDir, '-b', 'fix/stale-ref'], { cwd: repoDir, timeoutMs: GIT_TIMEOUT_MS }); assert.ok(fs.existsSync(worktreeDir), 'worktree dir should exist before manual deletion'); // Use the canonicalPath helper so Windows 8.3 short-name (RUNNER~1) vs diff --git a/tests/quick-branching.test.cjs b/tests/quick-branching.test.cjs index 966764cb6..a98dab5af 100644 --- a/tests/quick-branching.test.cjs +++ b/tests/quick-branching.test.cjs @@ -15,14 +15,22 @@ const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); -const { execFileSync } = require('node:child_process'); const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); const { cleanup, readFileNormalized } = require('./helpers.cjs'); +const { gitOrThrow, throwIfFailed } = require('./helpers/git-fixture.cjs'); +const { runHook } = require('./helpers/process-seam.cjs'); const QUICK_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'quick.md'); +/** + * Bound for every subprocess in this file: git plumbing against small mkdtemp + * fixture repos, plus the Step 2.5 bash-script run below — both orders of + * magnitude under this. #3144. + */ +const GIT_TIMEOUT_MS = 20000; + const GIT_ENV = Object.freeze({ ...process.env, GIT_AUTHOR_NAME: 'Test', @@ -32,13 +40,7 @@ const GIT_ENV = Object.freeze({ }); function git(cwd, ...args) { - return execFileSync('git', args, { - cwd, - env: GIT_ENV, - stdio: ['pipe', 'pipe', 'pipe'], - }) - .toString() - .trim(); + return gitOrThrow(args, { cwd, env: GIT_ENV, timeoutMs: GIT_TIMEOUT_MS }).trim(); } /** @@ -144,11 +146,9 @@ function runStep(bash, cwd, branchName) { const script = `#!/usr/bin/env bash\nset -uo pipefail\nbranch_name="${branchName}"\n${bash}\n`; fs.writeFileSync(scriptPath, script, { mode: 0o755 }); try { - return execFileSync('bash', [scriptPath], { - cwd, - env: GIT_ENV, - stdio: ['pipe', 'pipe', 'pipe'], - }).toString(); + const r = runHook(scriptPath, [], { interpreter: 'bash', cwd, env: GIT_ENV, timeoutMs: GIT_TIMEOUT_MS }); + throwIfFailed(r, `runStep: bash ${scriptPath}`); + return r.stdout; } finally { cleanup(scriptDir); } diff --git a/tests/reapply-verify-hunks.test.cjs b/tests/reapply-verify-hunks.test.cjs index 553078914..785c1ee47 100644 --- a/tests/reapply-verify-hunks.test.cjs +++ b/tests/reapply-verify-hunks.test.cjs @@ -20,6 +20,11 @@ const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); +// 30000ms: shared by both folded `runVerifier()` blocks below — each runs a +// single deterministic verifier pass over a small mkdtemp fixture tree, well +// over any observed duration for this class of call. +const VERIFIER_TIMEOUT_MS = 30_000; + const WORKFLOW_PATH = path.join( __dirname, '..', 'gsd-core', 'workflows', 'reapply-patches.md' ); @@ -142,8 +147,8 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); -const cp = require('node:child_process'); const { cleanup } = require('./helpers.cjs'); +const { runNode } = require('./helpers/process-seam.cjs'); const ROOT = path.join(__dirname, '..'); // Script lives at gsd-core/bin/ so the installer ships it under @@ -180,9 +185,9 @@ function runVerifier({ includePristine = true } = {}) { ...(includePristine ? ['--pristine-dir', pristineDir] : []), '--json', ]; - const r = cp.spawnSync(process.execPath, args, { encoding: 'utf8' }); + const r = runNode(args, { timeoutMs: VERIFIER_TIMEOUT_MS }); return { - status: r.status, + status: r.exitCode, report: r.stdout && r.stdout.length ? JSON.parse(r.stdout) : null, }; } @@ -409,8 +414,8 @@ const fs = require('node:fs'); const crypto = require('node:crypto'); const os = require('node:os'); const path = require('node:path'); -const cp = require('node:child_process'); const { cleanup } = require('./helpers.cjs'); +const { runNode } = require('./helpers/process-seam.cjs'); const ROOT = path.join(__dirname, '..'); const SCRIPT = path.join(ROOT, 'gsd-core', 'bin', 'verify-reapply-patches.cjs'); @@ -457,9 +462,9 @@ function runVerifier({ pristine = true } = {}) { ...(pristine ? ['--pristine-dir', pristineDir] : []), '--json', ]; - const r = cp.spawnSync(process.execPath, args, { encoding: 'utf8' }); + const r = runNode(args, { timeoutMs: VERIFIER_TIMEOUT_MS }); return { - status: r.status, + status: r.exitCode, report: r.stdout && r.stdout.length ? JSON.parse(r.stdout) : null, }; } diff --git a/tests/release-hotfix-empty-cherry-pick.test.cjs b/tests/release-hotfix-empty-cherry-pick.test.cjs index 1f9d8e0a0..913964427 100644 --- a/tests/release-hotfix-empty-cherry-pick.test.cjs +++ b/tests/release-hotfix-empty-cherry-pick.test.cjs @@ -16,30 +16,36 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); const os = require('node:os'); -const { spawnSync } = require('node:child_process'); +const { runGit } = require('./helpers/process-seam.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); const { cleanup } = require('./helpers.cjs'); const RELEASE_WORKFLOW = path.join(__dirname, '..', '.github', 'workflows', 'release.yml'); +// 15000ms: git plumbing (config, add, commit, rev-parse, checkout, cherry-pick) +// against a small mkdtemp fixture repo — gitOrThrow's own documented default, +// reused here as the named constant for this file's calls. +const GIT_TIMEOUT_MS = 15000; + // ─── git helpers ──────────────────────────────────────────────────────────── +// Migrated from a hand-rolled throw-on-non-zero over spawnSync's `-C cwd` +// argv form to gitOrThrow's `{ cwd }` option — same external behavior (throws +// on non-zero exit, returns trimmed stdout on success), no caller reads the +// old custom error message so the throw-shape swap is safe. function git(cwd, ...args) { - const r = spawnSync('git', ['-C', cwd, ...args], { encoding: 'utf8', stdio: ['pipe', 'pipe', 'pipe'] }); - if (r.status !== 0) { - throw new Error(`git ${args.join(' ')} failed (status ${r.status}) in ${cwd}:\n${r.stderr || r.stdout}`); - } - return r.stdout.trim(); + return gitOrThrow(args, { cwd, timeoutMs: GIT_TIMEOUT_MS }).trim(); } function makeRepo(dir) { fs.mkdirSync(dir, { recursive: true }); - spawnSync('git', ['init', dir], { encoding: 'utf8' }); + gitOrThrow(['init', dir], { timeoutMs: GIT_TIMEOUT_MS }); git(dir, 'config', 'user.email', 'test@test'); git(dir, 'config', 'user.name', 'Test'); git(dir, 'config', 'commit.gpgsign', 'false'); // Ensure the default branch is 'main' regardless of the system's // init.defaultBranch setting (older Git defaults to 'master'). - spawnSync('git', ['-C', dir, 'symbolic-ref', 'HEAD', 'refs/heads/main'], { encoding: 'utf8' }); + gitOrThrow(['symbolic-ref', 'HEAD', 'refs/heads/main'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); return dir; } @@ -110,8 +116,8 @@ describe('#2913 — cherry-pick empty-vs-conflict discrimination (real git)', () git(repo, 'checkout', 'next'); // Attempt the cherry-pick — it exits non-zero (empty). - const r = spawnSync('git', ['-C', repo, 'cherry-pick', '-x', choreSha], { encoding: 'utf8' }); - assert.notStrictEqual(r.status, 0, 'cherry-pick of an already-applied commit must exit non-zero'); + const r = runGit(['cherry-pick', '-x', choreSha], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }); + assert.notStrictEqual(r.exitCode, 0, 'cherry-pick of an already-applied commit must exit non-zero'); // Apply the discrimination logic. const result = discriminateCherryPick(repo); @@ -130,9 +136,9 @@ describe('#2913 — cherry-pick empty-vs-conflict discrimination (real git)', () git(repo, 'checkout', '-b', 'throwaway2'); const realSha = commitFile(repo, 'other.txt', 'real change\n', 'fix: a real fix'); git(repo, 'checkout', 'next'); - const realPick = spawnSync('git', ['-C', repo, 'cherry-pick', '-x', realSha], { encoding: 'utf8' }); - assert.strictEqual(realPick.status, 0, - `after --skip, the sequencer must be clean so the next cherry-pick succeeds; got status ${realPick.status}:\n${realPick.stderr || realPick.stdout}`); + const realPick = runGit(['cherry-pick', '-x', realSha], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }); + assert.strictEqual(realPick.exitCode, 0, + `after --skip, the sequencer must be clean so the next cherry-pick succeeds; got status ${realPick.exitCode}:\n${realPick.stderr || realPick.stdout}`); }); test('a genuine cherry-pick conflict is detected as conflict and aborted', () => { @@ -149,8 +155,8 @@ describe('#2913 — cherry-pick empty-vs-conflict discrimination (real git)', () commitFile(repo, 'file.txt', 'main version\n', 'fix: change to main version'); // Attempt to cherry-pick the next commit — it conflicts (same line, different content). - const r = spawnSync('git', ['-C', repo, 'cherry-pick', '-x', conflictingSha], { encoding: 'utf8' }); - assert.notStrictEqual(r.status, 0, 'cherry-pick of a conflicting commit must exit non-zero'); + const r = runGit(['cherry-pick', '-x', conflictingSha], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }); + assert.notStrictEqual(r.exitCode, 0, 'cherry-pick of a conflicting commit must exit non-zero'); // Apply the discrimination logic. const result = discriminateCherryPick(repo); diff --git a/tests/verification-status.test.cjs b/tests/verification-status.test.cjs index aa16604cb..6d3f78661 100644 --- a/tests/verification-status.test.cjs +++ b/tests/verification-status.test.cjs @@ -28,6 +28,8 @@ const path = require('node:path'); const os = require('node:os'); const { cleanup } = require('./helpers.cjs'); +const { runGit: seamRunGit, OUTCOME } = require('./helpers/process-seam.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); const { VERIFIER_STATUSES, @@ -36,6 +38,11 @@ const { readVerificationStatus, } = require('../gsd-core/bin/lib/verification.cjs'); +// 15000ms: git plumbing (init/config/add/commit) against a small mkdtemp +// fixture repo, plus a `git --version` availability probe — gitOrThrow's own +// documented default, reused here as this file's named constant. +const GIT_TIMEOUT_MS = 15000; + // ─── Helpers ───────────────────────────────────────────────────────────────── /** @@ -407,12 +414,10 @@ describe('verification-status', () => { // git availability for the real-subprocess integration test below. const GIT_AVAILABLE = (() => { - try { - require('node:child_process').execFileSync('git', ['--version'], { stdio: 'ignore' }); - return true; - } catch { - return false; - } + // Soft probe — a missing/broken git binary must resolve to `false`, not + // throw, so seamRunGit is used directly rather than gitOrThrow. + const r = seamRunGit(['--version'], { timeoutMs: GIT_TIMEOUT_MS }); + return r.outcome === OUTCOME.EXITED && r.exitCode === 0; })(); test('committed passed verification is NOT stale from mtime skew alone when the summary was not committed later (#2348)', () => { @@ -605,12 +610,11 @@ describe('verification-status', () => { 'real git: a summary committed after the verification reads stale via the real git clock, even for a dash-named file (#2348 end-to-end + `--` argv guard)', { skip: GIT_AVAILABLE ? false : 'git binary not available' }, () => { - const { execFileSync } = require('node:child_process'); const repo = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2348-realgit-')); const runGit = (args, extraEnv) => - execFileSync('git', args, { + gitOrThrow(args, { cwd: repo, - stdio: 'pipe', + timeoutMs: GIT_TIMEOUT_MS, env: { ...process.env, GIT_TERMINAL_PROMPT: '0', ...(extraEnv || {}) }, }); const commitEnvAt = (iso) => ({ GIT_AUTHOR_DATE: iso + '+00:00', GIT_COMMITTER_DATE: iso + '+00:00' }); @@ -658,12 +662,11 @@ describe('verification-status', () => { 'real git: a committed summary edited on disk (dirty) reads stale via mtime, not shadowed by its commit time (#2348 dirty regression, end-to-end)', { skip: GIT_AVAILABLE ? false : 'git binary not available' }, () => { - const { execFileSync } = require('node:child_process'); const repo = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2348-realgit-dirty-')); const runGit = (args, extraEnv) => - execFileSync('git', args, { + gitOrThrow(args, { cwd: repo, - stdio: 'pipe', + timeoutMs: GIT_TIMEOUT_MS, env: { ...process.env, GIT_TERMINAL_PROMPT: '0', ...(extraEnv || {}) }, }); const commitEnvAt = (iso) => ({ GIT_AUTHOR_DATE: iso + '+00:00', GIT_COMMITTER_DATE: iso + '+00:00' }); diff --git a/tests/verify.test.cjs b/tests/verify.test.cjs index 96e06ac74..b12c66ec7 100644 --- a/tests/verify.test.cjs +++ b/tests/verify.test.cjs @@ -7,7 +7,23 @@ const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); const { runGsdTools, createTempProject, createTempGitProject, cleanup } = require('./helpers.cjs'); -const { execSync } = require('child_process'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); +const { runHook } = require('./helpers/process-seam.cjs'); + +/** + * Bound for every git subprocess in this file: plumbing (init/config/add/ + * commit/rev-parse) against small fixture repos — well under this. #3144. + */ +const GIT_TIMEOUT_MS = 15000; + +/** + * Bound for the grep/sed availability probes and region-extraction calls in + * the region-scoped negative-gate proof below (#3144). These are not git — + * reusing `GIT_TIMEOUT_MS` for them would tie an unrelated tool's budget to + * git's, so they get their own named constant even though the value happens + * to match; a `grep -Eq`/`sed -n` over a small temp file is well under this. + */ +const TEXT_TOOL_TIMEOUT_MS = 15000; // ─── helpers ────────────────────────────────────────────────────────────────── @@ -787,10 +803,10 @@ describe('verify summary command', () => { // Create a source file and commit it fs.mkdirSync(path.join(tmpDir, 'src'), { recursive: true }); fs.writeFileSync(path.join(tmpDir, 'src', 'app.js'), 'console.log("hello");\n'); - execSync('git add -A', { cwd: tmpDir, stdio: 'pipe' }); - execSync('git commit -m "add app.js"', { cwd: tmpDir, stdio: 'pipe' }); + gitOrThrow(['add', '-A'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'add app.js'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); - const hash = execSync('git rev-parse --short HEAD', { cwd: tmpDir, encoding: 'utf-8' }).trim(); + const hash = gitOrThrow(['rev-parse', '--short', 'HEAD'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(); // Write SUMMARY.md referencing the file and commit hash const summaryPath = path.join(tmpDir, '.planning', 'phases', '01-test', '01-01-SUMMARY.md'); @@ -1082,7 +1098,7 @@ describe('verify commits command', () => { }); test('validates real commit hashes', () => { - const hash = execSync('git rev-parse --short HEAD', { cwd: tmpDir, encoding: 'utf-8' }).trim(); + const hash = gitOrThrow(['rev-parse', '--short', 'HEAD'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(); const result = runGsdTools(`verify commits ${hash}`, tmpDir); assert.ok(result.success, `Command failed: ${result.error}`); @@ -1105,7 +1121,7 @@ describe('verify commits command', () => { }); test('handles mixed valid and invalid hashes', () => { - const hash = execSync('git rev-parse --short HEAD', { cwd: tmpDir, encoding: 'utf-8' }).trim(); + const hash = gitOrThrow(['rev-parse', '--short', 'HEAD'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(); const result = runGsdTools(`verify commits ${hash} abcdef1234567`, tmpDir); assert.ok(result.success, `Command failed: ${result.error}`); @@ -1914,7 +1930,6 @@ const { test, describe, before, beforeEach, afterEach } = 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 os = require('node:os'); const { createTempProject, cleanup, runGsdTools } = require('./helpers.cjs'); @@ -2650,9 +2665,9 @@ describe('doc-contract: guidance prose is in place', () => { describe('AC3: executable proof — file-wide ban vs region-scoped simultaneously satisfiable', () => { test('case 22 — grep/sed proof: both gates simultaneously satisfiable', () => { // Check if grep and sed are available - const grepAvail = spawnSync('grep', ['--version']).status === 0; - const sedAvail = spawnSync('sed', ['--version']).status === 0 || - spawnSync('sed', ['-n', '1p', '/dev/null']).status === 0; + const grepAvail = runHook('--version', [], { interpreter: 'grep', timeoutMs: TEXT_TOOL_TIMEOUT_MS }).exitCode === 0; + const sedAvail = runHook('--version', [], { interpreter: 'sed', timeoutMs: TEXT_TOOL_TIMEOUT_MS }).exitCode === 0 || + runHook('-n', ['1p', '/dev/null'], { interpreter: 'sed', timeoutMs: TEXT_TOOL_TIMEOUT_MS }).exitCode === 0; if (!grepAvail || !sedAvail) { // Skip gracefully if tools are unavailable @@ -2679,39 +2694,39 @@ describe('AC3: executable proof — file-wide ban vs region-scoped simultaneousl try { // (a) File-wide: grep -Eq 'await .*refresh' — should EXIT 0 (pattern found) // This means a file-wide ban (! grep -Eq ...) WOULD FAIL - const fileWide = spawnSync('grep', ['-Eq', 'await .*refresh', tmpFile]); + const fileWide = runHook('-Eq', ['await .*refresh', tmpFile], { interpreter: 'grep', timeoutMs: TEXT_TOOL_TIMEOUT_MS }); assert.strictEqual( - fileWide.status, + fileWide.exitCode, 0, 'grep file-wide should find the pattern (exits 0) — proving the file-wide ban would fail', ); // (b) Region-scoped (make_page only): sed extracts lines 1-3, piped to grep → pattern NOT found // The factory region is clean: ban PASSES - const makePageLines = spawnSync('sed', ['-n', '1,3p', tmpFile]); - assert.strictEqual(makePageLines.status, 0, 'sed should succeed'); + const makePageLines = runHook('-n', ['1,3p', tmpFile], { interpreter: 'sed', timeoutMs: TEXT_TOOL_TIMEOUT_MS }); + assert.strictEqual(makePageLines.exitCode, 0, 'sed should succeed'); const makePageRegion = makePageLines.stdout.toString(); // Write to a temp file and grep it const regionFile = path.join(os.tmpdir(), `gsd-968-region-${process.pid}.py`); fs.writeFileSync(regionFile, makePageRegion); try { - const regionBan = spawnSync('grep', ['-Eq', 'await .*refresh', regionFile]); + const regionBan = runHook('-Eq', ['await .*refresh', regionFile], { interpreter: 'grep', timeoutMs: TEXT_TOOL_TIMEOUT_MS }); assert.strictEqual( - regionBan.status, + regionBan.exitCode, 1, 'grep in make_page region should NOT find pattern (exits 1) — ban PASSES in factory region', ); // (c) Region-scoped (reindex_handler): grep should FIND the pattern → requirement met - const reindexLines = spawnSync('sed', ['-n', '6,9p', tmpFile]); + const reindexLines = runHook('-n', ['6,9p', tmpFile], { interpreter: 'sed', timeoutMs: TEXT_TOOL_TIMEOUT_MS }); const reindexRegion = reindexLines.stdout.toString(); const reindexFile = path.join(os.tmpdir(), `gsd-968-reindex-${process.pid}.py`); fs.writeFileSync(reindexFile, reindexRegion); try { - const reindexCheck = spawnSync('grep', ['-Eq', 'await .*refresh', reindexFile]); + const reindexCheck = runHook('-Eq', ['await .*refresh', reindexFile], { interpreter: 'grep', timeoutMs: TEXT_TOOL_TIMEOUT_MS }); assert.strictEqual( - reindexCheck.status, + reindexCheck.exitCode, 0, 'grep in reindex_handler region MUST find pattern (exits 0) — requirement met', ); @@ -2736,7 +2751,6 @@ describe('verifySummaryCore — reusable structured contract (#2572)', () => { const os = require('node:os'); const path = require('node:path'); const { after } = require('node:test'); - const { execSync } = require('node:child_process'); const { cleanup } = require('./helpers.cjs'); const { verifySummaryCore } = require('../gsd-core/bin/lib/verify.cjs'); @@ -2746,16 +2760,17 @@ describe('verifySummaryCore — reusable structured contract (#2572)', () => { function repo(summaryBody, extraFiles = {}) { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2572-')); dirs.push(dir); - execSync('git init -q', { cwd: dir, stdio: 'pipe' }); - execSync('git config user.email "t@t.com"', { cwd: dir, stdio: 'pipe' }); - execSync('git config user.name "T"', { cwd: dir, stdio: 'pipe' }); - execSync('git config commit.gpgsign false', { cwd: dir, stdio: 'pipe' }); + gitOrThrow(['init', '-q'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'user.email', 't@t.com'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'user.name', 'T'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'commit.gpgsign', 'false'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); for (const [rel, body] of Object.entries(extraFiles)) { fs.mkdirSync(path.dirname(path.join(dir, rel)), { recursive: true }); fs.writeFileSync(path.join(dir, rel), body); } fs.writeFileSync(path.join(dir, 'SUMMARY.md'), summaryBody); - execSync('git add -A && git commit -q -m seed', { cwd: dir, stdio: 'pipe' }); + gitOrThrow(['add', '-A'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-q', '-m', 'seed'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); return dir; } diff --git a/tests/workspace.test.cjs b/tests/workspace.test.cjs index 044ffa4b8..44710bab5 100644 --- a/tests/workspace.test.cjs +++ b/tests/workspace.test.cjs @@ -9,9 +9,13 @@ const { test, describe, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); -const { execSync } = require('child_process'); const { runGsdTools, createTempProject, createTempDir, cleanup } = require('./helpers.cjs'); const { detectChildRepos } = require('../gsd-core/bin/lib/init.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); + +// 15000ms: git plumbing (init/config/add/commit/worktree/clone) on a small +// mkdtemp fixture repo — far over any observed duration for that class of call. +const GIT_TIMEOUT_MS = 15000; // ─── detectChildRepos ──────────────────────────────────────────────────────── @@ -32,8 +36,8 @@ describe('detectChildRepos', () => { const repo2 = path.join(tmpDir, 'repo-b'); fs.mkdirSync(repo1); fs.mkdirSync(repo2); - execSync('git init', { cwd: repo1, stdio: 'pipe' }); - execSync('git init', { cwd: repo2, stdio: 'pipe' }); + gitOrThrow(['init'], { cwd: repo1, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['init'], { cwd: repo2, timeoutMs: GIT_TIMEOUT_MS }); const repos = detectChildRepos(tmpDir); assert.strictEqual(repos.length, 2); @@ -46,7 +50,7 @@ describe('detectChildRepos', () => { const notRepo = path.join(tmpDir, 'just-a-dir'); fs.mkdirSync(gitRepo); fs.mkdirSync(notRepo); - execSync('git init', { cwd: gitRepo, stdio: 'pipe' }); + gitOrThrow(['init'], { cwd: gitRepo, timeoutMs: GIT_TIMEOUT_MS }); const repos = detectChildRepos(tmpDir); assert.strictEqual(repos.length, 1); @@ -56,7 +60,7 @@ describe('detectChildRepos', () => { test('skips hidden directories', () => { const hiddenRepo = path.join(tmpDir, '.hidden-repo'); fs.mkdirSync(hiddenRepo); - execSync('git init', { cwd: hiddenRepo, stdio: 'pipe' }); + gitOrThrow(['init'], { cwd: hiddenRepo, timeoutMs: GIT_TIMEOUT_MS }); const repos = detectChildRepos(tmpDir); assert.strictEqual(repos.length, 0); @@ -103,7 +107,7 @@ describe('init new-workspace', () => { test('detects child git repos in cwd', () => { const repo = path.join(tmpDir, 'my-repo'); fs.mkdirSync(repo); - execSync('git init', { cwd: repo, stdio: 'pipe' }); + gitOrThrow(['init'], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }); const result = runGsdTools('init new-workspace', tmpDir); const data = JSON.parse(result.output); @@ -272,18 +276,18 @@ describe('workspace worktree integration', () => { // Create a source git repo with a commit sourceRepo = path.join(tmpDir, 'source-repo'); fs.mkdirSync(sourceRepo); - execSync('git init', { cwd: sourceRepo, stdio: 'pipe' }); - execSync('git config user.email "test@test.com"', { cwd: sourceRepo, stdio: 'pipe' }); - execSync('git config user.name "Test"', { cwd: sourceRepo, stdio: 'pipe' }); + gitOrThrow(['init'], { cwd: sourceRepo, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'user.email', 'test@test.com'], { cwd: sourceRepo, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['config', 'user.name', 'Test'], { cwd: sourceRepo, timeoutMs: GIT_TIMEOUT_MS }); fs.writeFileSync(path.join(sourceRepo, 'README.md'), '# Test Repo\n'); - execSync('git add -A', { cwd: sourceRepo, stdio: 'pipe' }); - execSync('git commit -m "initial"', { cwd: sourceRepo, stdio: 'pipe' }); + gitOrThrow(['add', '-A'], { cwd: sourceRepo, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'initial'], { cwd: sourceRepo, timeoutMs: GIT_TIMEOUT_MS }); }); afterEach(() => { // Clean up worktrees before removing tmp dir try { - execSync('git worktree prune', { cwd: sourceRepo, stdio: 'pipe' }); + gitOrThrow(['worktree', 'prune'], { cwd: sourceRepo, timeoutMs: GIT_TIMEOUT_MS }); } catch { /* best-effort */ } cleanup(tmpDir); }); @@ -294,9 +298,9 @@ describe('workspace worktree integration', () => { fs.mkdirSync(path.join(wsPath, '.planning')); // Create worktree - execSync(`git worktree add "${path.join(wsPath, 'source-repo')}" -b workspace/test`, { + gitOrThrow(['worktree', 'add', path.join(wsPath, 'source-repo'), '-b', 'workspace/test'], { cwd: sourceRepo, - stdio: 'pipe', + timeoutMs: GIT_TIMEOUT_MS, }); // Verify worktree was created @@ -314,8 +318,8 @@ describe('workspace worktree integration', () => { fs.mkdirSync(wsPath); // Clone repo - execSync(`git clone "${sourceRepo}" "${path.join(wsPath, 'source-repo')}"`, { - stdio: 'pipe', + gitOrThrow(['clone', sourceRepo, path.join(wsPath, 'source-repo')], { + timeoutMs: GIT_TIMEOUT_MS, }); // Verify clone @@ -332,24 +336,24 @@ describe('workspace worktree integration', () => { fs.mkdirSync(wsPath); // Create worktree - execSync(`git worktree add "${path.join(wsPath, 'source-repo')}" -b workspace/removable`, { + gitOrThrow(['worktree', 'add', path.join(wsPath, 'source-repo'), '-b', 'workspace/removable'], { cwd: sourceRepo, - stdio: 'pipe', + timeoutMs: GIT_TIMEOUT_MS, }); assert.ok(fs.existsSync(path.join(wsPath, 'source-repo', 'README.md'))); // Remove worktree - execSync(`git worktree remove "${path.join(wsPath, 'source-repo')}"`, { + gitOrThrow(['worktree', 'remove', path.join(wsPath, 'source-repo')], { cwd: sourceRepo, - stdio: 'pipe', + timeoutMs: GIT_TIMEOUT_MS, }); // Verify worktree is gone assert.ok(!fs.existsSync(path.join(wsPath, 'source-repo'))); // Verify worktree list doesn't include it - const worktrees = execSync('git worktree list', { cwd: sourceRepo, encoding: 'utf8' }); + const worktrees = gitOrThrow(['worktree', 'list'], { cwd: sourceRepo, timeoutMs: GIT_TIMEOUT_MS }); assert.ok(!worktrees.includes('removable-ws')); }); }); diff --git a/tests/worktree-baseref-install.test.cjs b/tests/worktree-baseref-install.test.cjs index 90715477c..3939895a7 100644 --- a/tests/worktree-baseref-install.test.cjs +++ b/tests/worktree-baseref-install.test.cjs @@ -22,19 +22,22 @@ const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); const os = require('os'); -const { execFileSync } = require('child_process'); +const { runNode } = require('./helpers/process-seam.cjs'); +const { throwIfFailed } = require('./helpers/git-fixture.cjs'); const INSTALL_SRC = path.join(__dirname, '..', 'bin', 'install.js'); const BUILD_SCRIPT = path.join(__dirname, '..', 'scripts', 'build-hooks.js'); const { install, finishInstall } = require(INSTALL_SRC); const { cleanup } = require('./helpers.cjs'); +// 60000ms: matches the process seam's own default for a Node CLI run — this +// is a real build step (scripts/build-hooks.js), not fixture plumbing. +const BUILD_TIMEOUT_MS = 60000; + // ─── Ensure hooks/dist/ is populated before install tests ──────────────────── before(() => { - execFileSync(process.execPath, [BUILD_SCRIPT], { - encoding: 'utf-8', - stdio: 'pipe', - }); + const r = runNode([BUILD_SCRIPT], { timeoutMs: BUILD_TIMEOUT_MS }); + throwIfFailed(r, 'build-hooks.js (before install tests)'); }); // ─── Helper: run both install phases (mirrors installAllRuntimes two-phase) ── diff --git a/tests/worktree-cleanup.test.cjs b/tests/worktree-cleanup.test.cjs index 4525e2f67..1c40724e1 100644 --- a/tests/worktree-cleanup.test.cjs +++ b/tests/worktree-cleanup.test.cjs @@ -1309,11 +1309,18 @@ 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 } = require('./helpers/process-seam.cjs'); +const { gitOrThrow, throwIfFailed } = require('./helpers/git-fixture.cjs'); const EXECUTE_PHASE_MD = path.join(__dirname, '..', 'gsd-core', 'workflows', 'execute-phase.md'); +// 30000ms: git plumbing (init/config/add/commit/worktree/rev-parse) against a +// small mkdtemp fixture repo, and a `node -e ` one-liner +// extracted from the shipped workflow — well over any observed duration for +// either class of call in this bug-630 block. +const BUG_630_TIMEOUT_MS = 30_000; + function readMd() { return fs.readFileSync(EXECUTE_PHASE_MD, 'utf8'); } @@ -1330,11 +1337,17 @@ function extractManifestReaderScript() { } function git(cwd, args) { - return execFileSync('git', args, { - cwd, - encoding: 'utf8', - stdio: ['ignore', 'pipe', 'pipe'], - }).trim(); + return gitOrThrow(args, { cwd, timeoutMs: BUG_630_TIMEOUT_MS }).trim(); +} + +// runNode never throws; the shipped manifest-reader one-liner is expected to +// exit cleanly (it emits either a resolved path or nothing — never an +// error), so a non-clean exit here is a genuine defect and must still abort +// the test loudly, matching the pre-migration execFileSync throw. +function runManifestReaderOrThrow(args, opts) { + const r = runNode(args, opts); + throwIfFailed(r, `node ${args.join(' ')}`); + return r.stdout; } // Canonicalize a path the way the OS does. On Windows, os.tmpdir() can yield an 8.3 @@ -1418,10 +1431,10 @@ describe('bug #630 — wave-cleanup pins to the orchestrator root, not git-workt // Run the EXACT shipped reader one-liner. const script = extractManifestReaderScript(); - const resolved = execFileSync('node', ['-e', script], { + const resolved = runManifestReaderOrThrow(['-e', script], { cwd: laneDir, env: { ...process.env, MANIFEST: manifest }, - encoding: 'utf8', + timeoutMs: BUG_630_TIMEOUT_MS, }).trim(); // The buggy first-entry resolution (run from the lane) yields the MAIN checkout. @@ -1453,9 +1466,9 @@ describe('bug #630 — wave-cleanup pins to the orchestrator root, not git-workt // Pre-#630 manifest shape: no orchestrator_root. fs.writeFileSync(manifest, JSON.stringify({ worktrees: [] }) + '\n'); const script = extractManifestReaderScript(); - const out = execFileSync('node', ['-e', script], { + const out = runManifestReaderOrThrow(['-e', script], { env: { ...process.env, MANIFEST: manifest }, - encoding: 'utf8', + timeoutMs: BUG_630_TIMEOUT_MS, }); assert.equal(out, '', 'reader must emit nothing for a manifest without orchestrator_root so the first-entry fallback engages'); } finally { @@ -1480,16 +1493,21 @@ describe('bug #630 — wave-cleanup pins to the orchestrator root, not git-workt const { describe, test, before, after } = require('node:test'); const assert = require('node:assert/strict'); -const { execSync, spawnSync } = require('node:child_process'); const fs = require('node:fs'); const path = require('node:path'); const os = require('node:os'); const { cleanup, readFileNormalized } = require('./helpers.cjs'); -const { runHook } = require('./helpers/process-seam.cjs'); +const { runHook, runGit } = require('./helpers/process-seam.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); const REPO_ROOT = path.join(__dirname, '..'); const EXECUTE_PHASE_PATH = path.join(REPO_ROOT, 'gsd-core', 'workflows', 'execute-phase.md'); +// 15000ms: git plumbing (init/config/add/commit/worktree) against a small +// mkdtemp fixture repo — gitOrThrow's own documented default; the guard +// itself keeps its separately-justified 30000ms (see runGuard below). +const GIT_TIMEOUT_MS = 15000; + // --------------------------------------------------------------------------- // Extract the cwd-drift guard bash block from execute-phase.md // --------------------------------------------------------------------------- @@ -1593,12 +1611,15 @@ let agentSubdir; // subdirectory inside agentWtDir let legitUnderClaude; // non-agent worktree whose PATH is under .claude/worktrees/ const dirsToCleanup = []; +// Migrated off a hand-rolled shell string (naive per-arg double-quoting, +// run through execSync) onto gitOrThrow's argv form directly — every call +// site below passes a plain args array with no shell metacharacters, so +// running it as direct argv is behaviorally identical and drops the +// quoting hazard. Return value stays the RAW, un-trimmed stdout string +// (gitOrThrow does not trim), matching what execSync returned and what +// this block's ~10 callers expect. function git(cwd, args) { - return execSync(`git ${args.map(a => `"${a}"`).join(' ')}`, { - cwd, - encoding: 'utf-8', - stdio: ['ignore', 'pipe', 'pipe'], - }); + return gitOrThrow(args, { cwd, timeoutMs: GIT_TIMEOUT_MS }); } before(() => { @@ -1705,11 +1726,11 @@ describe('bug #48: orchestrator cwd-drift guard — executable e2e', () => { // Verify that git rev-parse --show-toplevel actually fails here. // On some systems /tmp itself might be inside a git repo (e.g. if the // user's HOME is a git repo). If it resolves, we must skip this test. - const check = spawnSync('git', ['rev-parse', '--show-toplevel'], { + const check = runGit(['rev-parse', '--show-toplevel'], { cwd: nonRepoDir, - encoding: 'utf-8', + timeoutMs: GIT_TIMEOUT_MS, }); - if (check.status === 0) { + if (check.exitCode === 0) { t.skip('nonRepoDir unexpectedly resolved to a git repo — skipping'); return; } diff --git a/tests/worktree-safety-reap.test.cjs b/tests/worktree-safety-reap.test.cjs index 93ff4a124..4aac62946 100644 --- a/tests/worktree-safety-reap.test.cjs +++ b/tests/worktree-safety-reap.test.cjs @@ -35,6 +35,7 @@ const path = require('node:path'); const os = require('node:os'); const { cleanup } = require('./helpers.cjs'); const { runGit } = require('./helpers/process-seam.cjs'); +const { throwIfFailed } = require('./helpers/git-fixture.cjs'); const { makeFaultyGit, withFaultyFs } = require('./helpers/faulty-deps.cjs'); const { @@ -70,9 +71,7 @@ function resolvedTmpDir() { /** Run git for FIXTURE SETUP; throws on anything but a clean exit. */ function git(args, cwd) { const r = runGit(args, { cwd, timeoutMs: GIT_TIMEOUT_MS }); - if (r.exitCode !== 0) { - throw new Error(`git ${args.join(' ')} failed (${r.outcome}/${r.exitCode}): ${r.stderr}`); - } + throwIfFailed(r, `git ${args.join(' ')}`); return r.stdout; } diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index b65e8515e..01df5f94b 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -24,6 +24,12 @@ const { createTempDir, cleanup } = require('./helpers.cjs'); const { createFixture } = require('./fixtures/index.cjs'); const { makeFaultyGit } = require('./helpers/faulty-deps.cjs'); +// 30000ms: this file's single named bound for every migrated subprocess call +// below (git plumbing on small mkdtemp fixtures, gsd-tools.cjs/hook CLI runs, +// and bash guard snippets) — well over any observed duration for any of +// those classes of call on this file's fixtures. +const SUBPROCESS_TIMEOUT_MS = 30_000; + const WORKTREE_SAFETY_PATH = path.join( __dirname, '..', 'gsd-core', 'bin', 'lib', 'worktree-safety.cjs' ); @@ -1315,22 +1321,34 @@ describe('cmdWorktreeRecordAgent', () => { describe('worktree record-agent — real CLI dispatch (#1298)', () => { const fs = require('node:fs'); - const { execFileSync } = require('node:child_process'); + const { runNode } = require('./helpers/process-seam.cjs'); + const { throwIfFailed } = require('./helpers/git-fixture.cjs'); const GSD_TOOLS = path.join(__dirname, '..', 'gsd-core', 'bin', 'gsd-tools.cjs'); + // runNode never throws; this describe's tests are written against the + // legacy execFileSync throw (an implicit dependency at the success-path + // call, and an explicit `err.status`/`err.stderr` read in the failure-path + // catch below) — this helper re-throws in that shape via the shared + // tests/helpers/git-fixture.cjs mechanism, for a non-git target. + function runGsdToolsOrThrow(args, opts) { + const r = runNode(args, opts); + throwIfFailed(r, `node ${args.join(' ')}`); + return r.stdout; + } + test('the dotted `query worktree.record-agent` path writes an entry the cleanup reader accepts', () => { const dir = createTempDir(); try { const manifest = path.join(dir, 'wave-manifest.json'); fs.writeFileSync(manifest, `${JSON.stringify({ orchestrator_root: dir, worktrees: [] })}\n`); - const out = execFileSync(process.execPath, [ + const out = runGsdToolsOrThrow([ GSD_TOOLS, 'query', 'worktree.record-agent', '--manifest', manifest, '--agent-id', 'a1', '--path', path.join(dir, 'wt-a1'), '--branch', 'worktree-agent-a1', '--base', 'abc123', - ], { encoding: 'utf8' }); + ], { timeoutMs: SUBPROCESS_TIMEOUT_MS }); assert.match(out, /"ok": true/); const written = JSON.parse(fs.readFileSync(manifest, 'utf8')); assert.equal(written.worktrees.length, 1); @@ -1351,11 +1369,11 @@ describe('worktree record-agent — real CLI dispatch (#1298)', () => { fs.writeFileSync(manifest, `${JSON.stringify({ worktrees: [] })}\n`); let threw = false; try { - execFileSync(process.execPath, [ + runGsdToolsOrThrow([ GSD_TOOLS, 'query', 'worktree.record-agent', '--manifest', manifest, '--path', path.join(dir, 'wt'), '--branch', 'worktree-agent-x', '--base', 'abc123', - ], { encoding: 'utf8', stdio: 'pipe' }); + ], { timeoutMs: SUBPROCESS_TIMEOUT_MS }); } catch (err) { threw = true; assert.equal(err.status, 1); @@ -3611,8 +3629,9 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); const os = require('node:os'); -const { execFileSync, spawnSync } = require('node:child_process'); +const { spawnSync } = require('node:child_process'); const { cleanup } = require('./helpers.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); const { executeWorktreeWaveCleanupPlan, @@ -3657,8 +3676,11 @@ const FRESH_MTIME = new Date(8640000000000000); // max safe JS Date (year ~27576 */ function deadPid() { // Use the shortest possible no-op: `node -e ""` on all platforms. + // Bounded directly (not routed through the process-seam) because the + // point of this call is `result.pid` — the seam's discriminated-union + // result does not expose the child's pid, only outcome/exitCode/etc. const nodeExe = process.execPath; - const result = spawnSync(nodeExe, ['-e', ''], { stdio: 'ignore' }); + const result = spawnSync(nodeExe, ['-e', ''], { stdio: 'ignore', timeout: SUBPROCESS_TIMEOUT_MS }); if (result.pid == null || result.status === null) { // Fallback: use a PID above the system max — 2^31-1 always exceeds any // real OS limit (Linux max: 4194304, macOS max: 99998, Windows: variable). @@ -3687,7 +3709,7 @@ function resolvedTmpDir() { } function git(args, cwd) { - return execFileSync('git', args, { cwd, stdio: 'pipe', encoding: 'utf8' }); + return gitOrThrow(args, { cwd, timeoutMs: SUBPROCESS_TIMEOUT_MS }); } function initRepo(dir) { @@ -4550,9 +4572,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 { runHook: seamRunHook } = require('./helpers/process-seam.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); const HOOK_PATH = path.join(__dirname, '..', 'hooks', 'gsd-worktree-path-guard.js'); const INSTALL_SRC = path.join(__dirname, '..', 'bin', 'install.js'); @@ -4573,7 +4595,7 @@ function realp(p) { // --------------------------------------------------------------------------- function git(cwd, args) { - return execFileSync('git', args, { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); + return gitOrThrow(args, { cwd, timeoutMs: SUBPROCESS_TIMEOUT_MS }); } /** @@ -5491,14 +5513,14 @@ 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 { cleanup } = require('./helpers.cjs'); +const { runHook: seamRunHook } = require('./helpers/process-seam.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); const HOOK_PATH = path.join(__dirname, '..', 'hooks', 'gsd-workflow-guard.js'); function git(cwd, args) { - return execFileSync('git', args, { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); + return gitOrThrow(args, { cwd, timeoutMs: SUBPROCESS_TIMEOUT_MS }); } function makeRepo(branch) { @@ -5524,11 +5546,12 @@ function setWorkflowGuard(dir, enabled) { } function runHookInput(cwd, input) { - return spawnSync(process.execPath, [HOOK_PATH], { + const r = seamRunHook(HOOK_PATH, [], { cwd, - encoding: 'utf8', input: JSON.stringify({ cwd, ...input }), + timeoutMs: SUBPROCESS_TIMEOUT_MS, }); + return { status: r.exitCode, stdout: r.stdout, stderr: r.stderr }; } function runBashHook(cwd, command) { @@ -5693,9 +5716,20 @@ const { describe, test, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); -const { execFileSync } = require('child_process'); const { createTempGitProject, cleanup } = require('./helpers.cjs'); const { runHook: seamRunHookGate } = require('./helpers/process-seam.cjs'); +const { gitOrThrow, throwIfFailed } = require('./helpers/git-fixture.cjs'); + +// Bash guard snippets in this block are exercised via `bash -c`, matching +// the pre-migration execFileSync('bash', ...) throw-on-non-zero idiom the +// tests below are written against (some catch and read err.status/ +// err.stderr explicitly). Uses the shared tests/helpers/git-fixture.cjs +// throw mechanism, for a non-git (`bash`) target. +function runBashOrThrow(script, opts) { + const r = seamRunHookGate('-c', [script], { interpreter: 'bash', ...opts }); + throwIfFailed(r, 'bash -c