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: () => {},