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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* chore(#3050): backfill changeset pr number

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-08-04 16:18:21 -04:00
committed by GitHub
parent 8c1962200d
commit 24066e536e
5 changed files with 267 additions and 20 deletions

View File

@@ -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)

View File

@@ -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 };

View File

@@ -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 <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha>
* Usage: worktree create --manifest <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha> --root <dir>
*
* #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 <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha> [--root <dir>]\n');
writeErr('Usage: worktree create --manifest <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha> --root <dir>\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 <orchestrator-root-dir> 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);

View File

@@ -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');
});
});

View File

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