From 24066e536ef2e3672df0ab7fc471bb0459fff289 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 4 Aug 2026 16:18:21 -0400 Subject: [PATCH] fix(#3050): fail closed when a worktree guard cannot verify safety (#3054) * fix(#3050): fail closed when a worktree guard cannot verify safety Three places answered "safe" when they had not actually checked. The base-divergence gate held the clearest evidence against itself: within one function, an unresolvable fork ref correctly degrades, while an unresolvable HEAD twenty-five lines earlier returned "proceed". Because a timeout collapsed into the same branch as "not a git repository", a locked index or a stalled mount produced a green gate that had never resolved the fork base -- and that value decides parallel versus sequential dispatch. Timeouts are now distinguished from a genuine absence of a repository. A timeout degrades with its own reason and message; not-a-git-repo keeps today's non-degrading behavior, because there is no worktree concern there. The same conflation in worktree-context resolution is surfaced rather than silently falling back to the current directory. Worktree creation's root confinement was opt-in: omitting the root skipped the check entirely, leaving only the leading-dash and parent-segment guards. The sole caller always passed it, so nothing was exploitable -- it is now mandatory so a future caller cannot inherit an unconfined path by forgetting. The timeout predicate was checked against what Node actually emits on a spawnSync timeout, not only against the fixtures, so it cannot be a guard that fires solely in tests. Coverage is deliberately behavioral. The existing worktree suites -- 134 tests across two files -- require no production module and call no production function; they assert against prose and would pass with the implementation deleted. That is how three fail-open guards survived in a heavily-tested module, so the new tests drive the real resolvers through an injected git seam, with five of them pinning the paths that must NOT change. One existing test asserted the opt-in confinement behavior and was rewritten rather than left green against the corrected code. Co-Authored-By: Claude Opus 5 * test(#3050): stop a CLI exit code leaking into the test process The runner reported the new file as failed while the file's own summary said nine tests passed and none failed. That signature is a non-zero process exit after a green run, not a failing assertion. Cause: the confinement test calls the worktree-create command function directly, and that function sets process.exitCode on its failure path as a CLI would. In process, that exit code became the test file's own exit status. The sibling suite already guards this with a save/restore wrapper and a comment naming the hazard; the new file simply did not follow the convention. It does now. Root cause is in the test, not the production code -- setting an exit code is correct behavior for a command entry point, and the existing convention exists precisely because tests call these functions in process. Verified by exit-code and active-handle probes rather than by re-running: exit was 1, is now 0, with zero lingering handles and all nine tests still passing. Co-Authored-By: Claude Opus 5 * chore(#3050): backfill changeset pr number Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- .changeset/calm-cats-greet.md | 5 + src/worktree-base-ref.cts | 20 +++ src/worktree-safety.cts | 50 +++++-- tests/worktree-safety-degrade.test.cjs | 186 +++++++++++++++++++++++++ tests/worktree-safety.test.cjs | 26 ++-- 5 files changed, 267 insertions(+), 20 deletions(-) create mode 100644 .changeset/calm-cats-greet.md create mode 100644 tests/worktree-safety-degrade.test.cjs diff --git a/.changeset/calm-cats-greet.md b/.changeset/calm-cats-greet.md new file mode 100644 index 000000000..1384b7698 --- /dev/null +++ b/.changeset/calm-cats-greet.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3054 +--- +**Worktree safety gates no longer report success when they could not check** — a git command that timed out (a locked index, a stalled network mount) was treated the same as "this is not a git repository", so the base-divergence gate answered "safe to run parallel worktrees" without ever resolving the fork base, and worktree-context resolution silently fell back to the current directory. Both now degrade instead. Worktree creation also no longer skips its root-confinement check when the caller omits the root. (#3050) diff --git a/src/worktree-base-ref.cts b/src/worktree-base-ref.cts index d18ddfff2..da28ffb11 100644 --- a/src/worktree-base-ref.cts +++ b/src/worktree-base-ref.cts @@ -98,6 +98,20 @@ function buildMsgDiverged(headSha: string | null, forkRef: string | null, forkSh const MSG_UNKNOWN = `⚠ Cannot determine the worktree fork base (origin/HEAD unresolved). Running this phase sequentially on the main working tree to avoid a base mismatch. To keep parallel worktrees, set worktree.baseRef:"head" in .claude/settings.local.json (or run: gsd-tools worktree set-baseref). See #683.`; +const MSG_HEAD_UNRESOLVABLE = `⚠ Cannot determine the worktree base (git rev-parse HEAD timed out or could not complete). Running this phase sequentially on the main working tree to avoid an unverified base mismatch. To keep parallel worktrees, set worktree.baseRef:"head" in .claude/settings.local.json (or run: gsd-tools worktree set-baseref). See #683, #3050.`; + +/** + * Returns true when an execGit result indicates the subprocess was killed by + * a timeout (SIGTERM + ETIMEDOUT), mirroring the idiom already established in + * worktree-safety.cts's execGitDefault. A timeout means the command genuinely + * could not complete — it must never be treated the same as a clean non-zero + * exit (e.g. "not a git repository"), which DID complete and reported a real + * answer. + */ +function isExecGitTimeout(result: { signal: string | null; error: unknown }): boolean { + return result.signal === 'SIGTERM' && (result.error as NodeJS.ErrnoException | null | undefined)?.code === 'ETIMEDOUT'; +} + // ─── Exports ────────────────────────────────────────────────────────────────── /** @@ -351,6 +365,12 @@ export function evaluateWorktreeBaseDegrade(deps?: { // b. Resolve HEAD sha. const headResult = execGit(['rev-parse', 'HEAD'], cwdOpts); + // A TIMEOUT means the command never completed — it is not evidence of "not a + // git repository" and must fail closed (distinct from the clean-exit-128 + // "no-head" case below, which genuinely completed and reported no HEAD). + if (isExecGitTimeout(headResult)) { + return { shouldDegrade: true, reason: 'head-unresolvable', message: MSG_HEAD_UNRESOLVABLE, headSha: null, forkRef: null, forkSha: null }; + } const headStdout = headResult.stdout ? headResult.stdout.trim() : ''; if (headResult.exitCode !== 0 || !headStdout) { return { shouldDegrade: false, reason: 'no-head', message: null, headSha: null, forkRef: null, forkSha: null }; diff --git a/src/worktree-safety.cts b/src/worktree-safety.cts index 7430942b5..6c144b3ee 100644 --- a/src/worktree-safety.cts +++ b/src/worktree-safety.cts @@ -163,6 +163,19 @@ function resolveWorktreeContext(cwd: string, deps: WorktreeDeps = {}): WorktreeC const gitDir = execGit(['rev-parse', '--git-dir'], { cwd }); const commonDir = execGit(['rev-parse', '--git-common-dir'], { cwd }); + // A TIMEOUT means the command never completed — it is not evidence of "not a + // git repository" (which completes fast, with a clean non-zero exit). Surface + // it under a distinct reason so callers can tell "genuinely not a repo" apart + // from "could not determine" (#3050). effectiveRoot still degrades to cwd + // (there is no safer default without a resolved git-dir), but the reason is + // no longer indistinguishable from the benign case. + if (gitDir.timedOut || commonDir.timedOut) { + return { + effectiveRoot: cwd, + mode: 'current_directory', + reason: 'git_timed_out', + }; + } if (gitDir.exitCode !== 0 || commonDir.exitCode !== 0) { return { effectiveRoot: cwd, @@ -1341,7 +1354,7 @@ interface WorktreeCreateCmdResult { * validated manifest entry so the worktree is immediately manageable by * `worktree cleanup-wave` / `worktree reap-orphans`. * - * Usage: worktree create --manifest --agent-id --path --branch --base + * Usage: worktree create --manifest --agent-id --path --branch --base --root * * #2584 FIX 1 — ORDERING CONTRACT: every manifest read/parse/shape-validate/ * plan step runs BEFORE the git side effect (step 5). The ONLY manifest @@ -1362,7 +1375,7 @@ function cmdWorktreeCreate(cwd: string, args: string[] = [], deps: RecordAgentCm const manifestPath = flag('--manifest'); if (!manifestPath) { - writeErr('Usage: worktree create --manifest --agent-id --path --branch --base [--root ]\n'); + writeErr('Usage: worktree create --manifest --agent-id --path --branch --base --root \n'); process.exitCode = 2; return { ok: false, reason: 'usage' }; } @@ -1435,25 +1448,38 @@ function cmdWorktreeCreate(cwd: string, args: string[] = [], deps: RecordAgentCm return { ok: false, reason: plan.reason, hint: plan.hint }; } - // 3b. Optional root confinement (#2627, Phase 3 — the confinement Phase 2 - // deferred here from planWorktreeCreate's path-traversal guard). - // planWorktreeCreate rejects a literal ".." SEGMENT, but a plain absolute - // path outside the project contains no ".." and passes. Phase 3 makes the - // orchestrator SPAWN executor processes into these paths, so an - // unconfined --path is a write primitive aimed anywhere on the filesystem. + // 3b. Mandatory root confinement (#2627 Phase 3 introduced it; #3050 made it + // mandatory — the confinement Phase 2 deferred here from + // planWorktreeCreate's path-traversal guard). planWorktreeCreate rejects a + // literal ".." SEGMENT, but a plain absolute path outside the project + // contains no ".." and passes. The orchestrator SPAWNS executor processes + // into these paths, so an unconfined --path is a write primitive aimed + // anywhere on the filesystem. // // The root is DECLARED by the caller (`--root`) rather than inferred: agent // worktrees legitimately live outside the orchestrator's own root (a lane // orchestrator creates siblings under the repo's .claude/worktrees/), so - // there is no layout this module could derive without guessing. Absent - // `--root` the behavior is exactly as shipped in Phase 2 — the - // orchestrator-worktree scheduler path always passes it. + // there is no layout this module could derive without guessing. // // Lexical by design: the worktree does not exist yet, so there is nothing // to realpath, and resolving only the root would not close a symlinked-leaf // hole. Pairs with the leading-dash and ".."-segment guards above. + // + // #3050: confinement does not depend on the caller remembering to pass + // `--root` — it used to be silently skippable, so a caller that forgot the + // flag got an unconfined `--path` with no warning. Fail closed instead: + // absent `--root`, this verb refuses to create anything. The one current + // caller (execute-phase's orchestrator-worktree dispatch) always passes + // `--root`, so this closes the gap without breaking it. const rootFlag = flag('--root'); - if (rootFlag) { + if (!rootFlag) { + const hint = '--root is required (fail-closed root confinement, #3050). Pass --root so worktree.create can verify --path resolves inside it before creating anything.'; + writeErr(`[gsd] worktree.create: root_required — ${hint}\n`); + write(`${JSON.stringify({ ok: false, reason: 'root_required', hint }, null, 2)}\n`); + process.exitCode = 1; + return { ok: false, reason: 'root_required', hint }; + } + { const absRoot = path.resolve(cwd, rootFlag); const absWorktree = path.resolve(cwd, plan.entry.worktree_path); const rel = path.relative(absRoot, absWorktree); diff --git a/tests/worktree-safety-degrade.test.cjs b/tests/worktree-safety-degrade.test.cjs new file mode 100644 index 000000000..51c0c94ce --- /dev/null +++ b/tests/worktree-safety-degrade.test.cjs @@ -0,0 +1,186 @@ +'use strict'; + +/** + * Worktree fail-open-guards regression tests (#3050). + * + * Covers three defects where a git TIMEOUT was silently collapsed into a + * benign "not a git repo" / "current directory" outcome instead of failing + * closed: + * + * 1. evaluateWorktreeBaseDegrade (worktree-base-ref.cjs) — a `rev-parse + * HEAD` timeout must degrade (shouldDegrade:true), not be treated the + * same as "not a git repository". + * 2. resolveWorktreeContext (worktree-safety.cjs) — a `rev-parse + * --git-dir`/`--git-common-dir` timeout must be distinguishable from + * the genuine not-a-git-repo case, not silently fall back to + * current_directory with an identical reason. + * 3. cmdWorktreeCreate (worktree-safety.cjs) — root confinement must not + * be skippable by omitting `--root`; it must fail closed. + * + * All tests drive the real production functions through the injected + * execGit/deps seam — no real filesystem or git subprocess is touched. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); + +const BASE_REF_MODULE_PATH = path.join( + __dirname, '..', 'gsd-core', 'bin', 'lib', 'worktree-base-ref.cjs' +); +const WORKTREE_SAFETY_MODULE_PATH = path.join( + __dirname, '..', 'gsd-core', 'bin', 'lib', 'worktree-safety.cjs' +); + +const { evaluateWorktreeBaseDegrade } = require(BASE_REF_MODULE_PATH); +const { + resolveWorktreeContext, + cmdWorktreeCreate, +} = require(WORKTREE_SAFETY_MODULE_PATH); + +// ─── helpers ──────────────────────────────────────────────────────────────── + +/** + * Build a mock execGit result mirroring the real shape spawnSync-derived + * results carry: {exitCode, stdout, stderr, signal, error}. + */ +function gitResult({ exitCode = 0, stdout = '', stderr = '', signal = null, error = null } = {}) { + return { exitCode, stdout, stderr, signal, error }; +} + +/** A timed-out git result: SIGTERM + ETIMEDOUT, per the established idiom. */ +function timedOutResult() { + const err = new Error('spawnSync git ETIMEDOUT'); + err.code = 'ETIMEDOUT'; + return gitResult({ exitCode: null, signal: 'SIGTERM', error: err }); +} + +/** A genuine "not a git repository" result (git exits 128). */ +function notAGitRepoResult() { + return gitResult({ exitCode: 128, stderr: 'fatal: not a git repository (or any of the parent directories): .git' }); +} + +// ─── DEFECT 1: evaluateWorktreeBaseDegrade must fail closed on HEAD timeout ── + +describe('evaluateWorktreeBaseDegrade — HEAD resolution timeout (#3050 DEFECT 1)', () => { + test('rev-parse HEAD TIMES OUT → shouldDegrade:true (fail closed, distinct reason)', () => { + const execGit = (args) => { + assert.deepStrictEqual(args, ['rev-parse', 'HEAD']); + return timedOutResult(); + }; + const result = evaluateWorktreeBaseDegrade({ execGit, effectiveBaseRef: null, cwd: '/repo' }); + assert.strictEqual(result.shouldDegrade, true); + assert.notStrictEqual(result.reason, 'no-head'); + assert.ok(result.message, 'a fail-closed degrade must carry a non-null explanatory message'); + assert.strictEqual(result.headSha, null); + }); + + test('genuine not-a-git-repo (exit 128) → shouldDegrade:false, reason "no-head" (regression guard — unchanged)', () => { + const execGit = () => notAGitRepoResult(); + const result = evaluateWorktreeBaseDegrade({ execGit, effectiveBaseRef: null, cwd: '/repo' }); + assert.strictEqual(result.shouldDegrade, false); + assert.strictEqual(result.reason, 'no-head'); + assert.strictEqual(result.message, null); + }); + + test('HEAD resolves but fork ref is unresolvable → reason "fork-ref-unknown" (regression guard — unchanged)', () => { + const execGit = (args) => { + if (args[0] === 'rev-parse' && args[1] === 'HEAD') return gitResult({ stdout: 'aaaa111\n' }); + // origin/HEAD direct rev-parse and symbolic-ref fallback both fail. + return gitResult({ exitCode: 1 }); + }; + const result = evaluateWorktreeBaseDegrade({ execGit, effectiveBaseRef: null, cwd: '/repo' }); + assert.strictEqual(result.shouldDegrade, true); + assert.strictEqual(result.reason, 'fork-ref-unknown'); + assert.strictEqual(result.headSha, 'aaaa111'); + }); + + test('HEAD == fork sha → shouldDegrade:false, reason "head-matches-fork" (regression guard — unchanged)', () => { + const sha = 'deadbeef00000000000000000000000000000000'; + const execGit = (args) => { + if (args[0] === 'rev-parse' && args[1] === 'HEAD') return gitResult({ stdout: `${sha}\n` }); + if (args.includes('origin/HEAD')) return gitResult({ stdout: `${sha}\n` }); + return gitResult({ exitCode: 1 }); + }; + const result = evaluateWorktreeBaseDegrade({ execGit, effectiveBaseRef: null, cwd: '/repo' }); + assert.strictEqual(result.shouldDegrade, false); + assert.strictEqual(result.reason, 'head-matches-fork'); + }); + + test('HEAD diverged from fork → shouldDegrade:true, reason "head-diverged-from-fork" (regression guard — unchanged)', () => { + const headSha = 'aaaaaaaa00000000000000000000000000000000'; + const forkSha = 'bbbbbbbb00000000000000000000000000000000'; + const execGit = (args) => { + if (args[0] === 'rev-parse' && args[1] === 'HEAD') return gitResult({ stdout: `${headSha}\n` }); + if (args.includes('origin/HEAD')) return gitResult({ stdout: `${forkSha}\n` }); + return gitResult({ exitCode: 1 }); + }; + const result = evaluateWorktreeBaseDegrade({ execGit, effectiveBaseRef: null, cwd: '/repo' }); + assert.strictEqual(result.shouldDegrade, true); + assert.strictEqual(result.reason, 'head-diverged-from-fork'); + }); + + test('baseRef:"head" short-circuits before any git call → shouldDegrade:false (regression guard — unchanged)', () => { + const execGit = () => { throw new Error('execGit must not be called when baseRef is "head"'); }; + const result = evaluateWorktreeBaseDegrade({ execGit, effectiveBaseRef: 'head', cwd: '/repo' }); + assert.strictEqual(result.shouldDegrade, false); + assert.strictEqual(result.reason, 'baseref-head'); + }); +}); + +// ─── DEFECT 2: resolveWorktreeContext must distinguish timeout from not-a-repo ── + +describe('resolveWorktreeContext — git-dir resolution timeout (#3050 DEFECT 2)', () => { + test('git rev-parse --git-dir TIMES OUT → reason is distinguishable from not_git_repo', () => { + const execGit = (args) => { + if (args.includes('--git-dir')) return { ...timedOutResult(), timedOut: true }; + return { ...notAGitRepoResult(), timedOut: false }; + }; + const context = resolveWorktreeContext('/repo', { execGit, existsSync: () => false }); + assert.notStrictEqual(context.reason, 'not_git_repo'); + }); + + test('genuine not-a-git-repo (both calls exit 128, no timeout) → reason "not_git_repo" (regression guard — unchanged)', () => { + const execGit = () => ({ ...notAGitRepoResult(), timedOut: false }); + const context = resolveWorktreeContext('/repo', { execGit, existsSync: () => false }); + assert.strictEqual(context.effectiveRoot, '/repo'); + assert.strictEqual(context.mode, 'current_directory'); + assert.strictEqual(context.reason, 'not_git_repo'); + }); +}); + +// ─── DEFECT 3: worktree create root confinement must not be skippable ─────── + +describe('cmdWorktreeCreate — root confinement is mandatory (#3050 DEFECT 3)', () => { + // process.exitCode is global; cmdWorktreeCreate sets it as a side effect on + // its failure paths (it doubles as the CLI entry point). Calling it directly + // in-process — rather than through a spawned subprocess — means that side + // effect leaks into THIS test file's own process.exitCode, which makes the + // whole file exit non-zero even though every assertion passes (mirrors the + // documented hazard + `withExitCode` guard in tests/worktree-safety.test.cjs + // around cmdWorktreeRecordAgent). Save/restore it so this test can assert + // on the failure path without poisoning the file's own exit status. + function withExitCode(fn) { + const saved = process.exitCode; + try { return fn(); } finally { process.exitCode = saved; } + } + + test('omitting --root fails closed instead of silently skipping confinement', () => { + const deps = { + readFile: () => JSON.stringify({ orchestrator_root: '/repo', worktrees: [] }), + writeFile: () => { throw new Error('writeFile must not be called — confinement must fail before any write'); }, + execGit: () => { throw new Error('execGit must not be called — confinement must fail before any git side effect'); }, + write: () => {}, + writeErr: () => {}, + }; + const result = withExitCode(() => cmdWorktreeCreate('/repo', [ + '--manifest', 'manifest.json', + '--agent-id', 'agent-1', + '--path', '/repo/.claude/worktrees/agent-1', + '--branch', 'worktree-agent-agent-1', + '--base', 'abc1234', + ], deps)); + assert.strictEqual(result.ok, false); + assert.notStrictEqual(result.reason, 'created'); + }); +}); diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index af996da7b..7d6809251 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -1295,6 +1295,11 @@ describe('cmdWorktreeCreate', () => { '--path', '/repo/.claude/worktrees/agent-a1', '--branch', 'worktree-agent-a1', '--base', 'abc123', + // #3050: --root is now mandatory (fail-closed confinement) — every test + // below that isn't specifically exercising the missing-root case must + // supply one. '/repo' confines every okArgs-derived --path used in this + // describe block (all live under /repo/.claude/worktrees/...). + '--root', '/repo', ]; function okExecGit() { @@ -1330,10 +1335,10 @@ describe('cmdWorktreeCreate', () => { assert.match(out.join(''), /"ok": true/); }); - // #2627 Phase 3: --root confines the created worktree. Absent the flag the - // behavior is exactly Phase 2's (every test above passes unchanged); the - // orchestrator-worktree scheduler path always passes it, because Phase 3 is - // what starts SPAWNING processes into these directories. + // #2627 Phase 3 / #3050: --root confines the created worktree. #3050 + // hardened this from opt-in to MANDATORY — confinement must not depend on + // the caller remembering to pass the flag; omitting it now fails closed + // instead of silently creating an unconfined worktree. describe('--root confinement', () => { const rootedArgs = (wtPath, root) => [ '--manifest', 'manifest.json', @@ -1389,15 +1394,17 @@ describe('cmdWorktreeCreate', () => { assert.equal(result.reason, 'path_outside_root'); }); - test('omitting --root preserves Phase-2 behavior (no confinement)', () => { - const { result } = run([ + test('omitting --root fails closed (#3050 — confinement is mandatory, not opt-in)', () => { + const { result, gitCalled } = run([ '--manifest', 'manifest.json', '--agent-id', 'a1', '--path', '/repo/.claude/worktrees/agent-a1', '--branch', 'worktree-agent-a1', '--base', 'abc123', ]); - assert.equal(result.ok, true, 'no --root → unchanged Phase-2 acceptance'); + assert.equal(result.ok, false, 'no --root → fail closed, never silently unconfined'); + assert.equal(result.reason, 'root_required'); + assert.equal(gitCalled, false, 'must fail before any git side effect'); }); }); @@ -1622,9 +1629,12 @@ describe('cmdWorktreeCreate / cmdWorktreeRecordAgent — on-disk entry parity (# '--branch', 'worktree-agent-a1', '--base', 'abc123', ]; + // cmdWorktreeCreate now requires --root (#3050); cmdWorktreeRecordAgent has + // no --root concept at all, so it's appended only to the create-side args. + const createArgs = [...argsFor('--manifest'), '--root', '/repo']; let createdContent = null; - cmdWorktreeCreate('/repo/main', argsFor('--manifest'), { + cmdWorktreeCreate('/repo/main', createArgs, { readFile: () => '{"worktrees":[]}', writeFile: (_p, c) => { createdContent = c; }, write: () => {},