From c2d5b528e573cc69ee1d43d2bfdef461911e40d5 Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 6 Aug 2026 00:32:28 -0400 Subject: [PATCH 1/7] refactor(#3103): give the reap and worktree-info paths the seam their callers already have MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Twenty-two branches in the orphan-reaping path are unreachable from the test suite, and the reason is not that they are hard to reach — it is that nothing can reach them. The reaper already accepts an injectable dependency bag, but every test drives real git and injects only the clock and the liveness probe, so each fail-closed return inside it has never executed under test. The two entry points above it took no dependencies at all, so a test could not drive them even if it wanted to. Both now accept the same bag and thread it down, with stdout and stderr writers defaulting to the process streams. The worktree-info probe in the base-branch resolver called its git seam directly while a sibling function in the same file already modelled the injectable form; it now follows that sibling rather than inventing a second convention. Every parameter defaults to today's real implementation, so no existing caller changes behavior. This is a testability seam, not a redesign. Two guards are removed as genuinely dead, each excluded by a check a few lines above it. A NaN test on a value captured by a digits-only pattern cannot fire, because parseInt of digits is never NaN. An emptiness test on a capture group that matched one-or-more non-space characters cannot fire either. Each site keeps a one-line note naming the guard that excludes it, so neither gets restored by a future reader. A third guard was proposed for deletion on the same grounds and is NOT removed, because the claim was wrong. The local-branch fallback returns null when git prints output that is non-empty but names neither branch — the emptiness check above it only catches the empty string, so a single newline reaches the fallback with both flags false. Deleting it would have changed which branch the resolver reports. It stays, and it gets a test. Refs #3057 Co-Authored-By: Claude Opus 5 --- src/git-base-branch.cts | 13 +++++++++---- src/worktree-safety.cts | 24 ++++++++++++++---------- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/git-base-branch.cts b/src/git-base-branch.cts index 8bcd1529f..cef69d005 100644 --- a/src/git-base-branch.cts +++ b/src/git-base-branch.cts @@ -136,7 +136,8 @@ export function tryRemoteShow( const branch = m[1]; // git emits "(unknown)" when the remote is offline but the local cache // resolved it; treat that as non-authoritative and fall through. - if (!branch || branch === '(unknown)') return null; + // No `!branch ||` guard: m[1] comes from the `(\S+)` capture group above, so it is never empty. + if (branch === '(unknown)') return null; return branch; } catch { return null; @@ -271,9 +272,13 @@ export interface GitWorktreeInfo { * Detect whether `cwd` sits inside a git worktree, and if so, return the * absolute path of the worktree root. */ -export function gitWorktreeInfoInternal(cwd: string): GitWorktreeInfo { +export function gitWorktreeInfoInternal( + cwd: string, + deps?: Pick +): GitWorktreeInfo { + const execGit: ExecGitFn = deps?.execGit ?? execGitSeam; try { - const insideResult = execGitSeam(['rev-parse', '--is-inside-work-tree'], { cwd, timeout: 5000 }); + const insideResult = execGit(['rev-parse', '--is-inside-work-tree'], { cwd, timeout: 5000 }); if (insideResult.exitCode !== 0) { return { inside: false, worktreeRoot: null }; } @@ -281,7 +286,7 @@ export function gitWorktreeInfoInternal(cwd: string): GitWorktreeInfo { if (insideStdout !== 'true') { return { inside: false, worktreeRoot: null }; } - const rootResult = execGitSeam(['rev-parse', '--show-toplevel'], { cwd, timeout: 5000 }); + const rootResult = execGit(['rev-parse', '--show-toplevel'], { cwd, timeout: 5000 }); if (rootResult.exitCode !== 0) { return { inside: true, worktreeRoot: null }; } diff --git a/src/worktree-safety.cts b/src/worktree-safety.cts index 857439fa8..8f12e3d0f 100644 --- a/src/worktree-safety.cts +++ b/src/worktree-safety.cts @@ -1739,7 +1739,8 @@ function reapOrphanWorktrees(repoRoot: string, deps: WorktreeDeps = {}): ReapRes const pid = parseInt(pidStr, 10); let pidIsAlive: boolean; try { - pidIsAlive = Number.isNaN(pid) || isPidAliveCheck(pid); + // No `Number.isNaN(pid) ||` guard: pidStr is captured by /^\d+/ above, so pid is never NaN. + pidIsAlive = isPidAliveCheck(pid); } catch { pidIsAlive = true; // Cannot determine liveness — treat as alive, do not reap. } @@ -1825,21 +1826,23 @@ function defaultMtimeSafe(file: string): Date | null { try { return fs.statSync(file).mtime; } catch { return null; } } -function cmdWorktreeReapOrphans(cwd: string): void { +function cmdWorktreeReapOrphans(cwd: string, deps: RecordAgentCmdDeps & WorktreeDeps = {}): void { + const write = deps.write || ((s: string) => process.stdout.write(s)); + const writeErr = deps.writeErr || ((s: string) => process.stderr.write(s)); let result: ReapResult[]; try { - result = reapOrphanWorktrees(cwd); + result = reapOrphanWorktrees(cwd, deps); } catch (err) { // Surface failure as a one-line warning; keep exit-zero so workflows don't break. - process.stderr.write(`[gsd] worktree.reap-orphans failed: ${err && (err as Error).message ? (err as Error).message : String(err)}\n`); + writeErr(`[gsd] worktree.reap-orphans failed: ${err && (err as Error).message ? (err as Error).message : String(err)}\n`); result = []; } const skippedCount = result.filter((r) => r.status === 'skipped').length; if (skippedCount > 0) { // Surface skipped entries so operators are aware of unresolved orphans. - process.stderr.write(`[gsd] worktree.reap-orphans: ${skippedCount} orphan(s) skipped (run with DEBUG=1 for details)\n`); + writeErr(`[gsd] worktree.reap-orphans: ${skippedCount} orphan(s) skipped (run with DEBUG=1 for details)\n`); } - process.stdout.write(`${JSON.stringify({ ok: true, reaped: result.filter((r) => r.status === 'reaped').length, entries: result }, null, 2)}\n`); + write(`${JSON.stringify({ ok: true, reaped: result.filter((r) => r.status === 'reaped').length, entries: result }, null, 2)}\n`); } // Unused exports kept for API compatibility @@ -1875,16 +1878,17 @@ function resolveWorktreeRoot(cwd: string, deps: WorktreeDeps = {}): { root: stri * the repository; used as `cwd` for git commands. * @returns list of worktree paths that were removed (always empty) */ -function pruneOrphanedWorktrees(repoRoot: string): string[] { +function pruneOrphanedWorktrees(repoRoot: string, deps: WorktreeDeps & { writeErr?: (s: string) => void } = {}): string[] { + const writeErr = deps.writeErr || ((s: string) => process.stderr.write(s)); try { const plan = planWorktreePrune( repoRoot, { allowDestructive: false }, - { parseWorktreePorcelain } + { parseWorktreePorcelain, ...deps } ); - const pruneResult = executeWorktreePrunePlan(plan) as { timedOut?: boolean } | null; + const pruneResult = executeWorktreePrunePlan(plan, deps) as { timedOut?: boolean } | null; if (pruneResult && pruneResult.timedOut) { - process.stderr.write( + writeErr( '[gsd-tools] WARNING: worktree health check degraded' + ' — git worktree prune timed out after 10s.' + ' Orphaned worktree metadata may remain until the next successful run.\n' From 63404ca45b1b40775af8a576d6158b04b6270b07 Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 6 Aug 2026 00:43:18 -0400 Subject: [PATCH 2/7] test(#3103): pin every branch in the base-branch resolver to the value it returns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thirty-eight tests, covering every previously unreached branch in this module: malformed and non-object config at each nesting level, the origin/HEAD path returning a bare remote prefix, the remote-show parse missing its HEAD line and its "(unknown)" sentinel, three catch arms, the spawn-failure path that reports unverified, the worktree probe answering something other than "true", a non-zero toplevel query, a blank toplevel, and the default diagnostic sink on both arms. The one worth naming is the local-branch fallback that was deleted earlier today as unreachable and restored. It is hard to pin because it and the empty-stdout guard above it both return null, so the return value cannot tell them apart. The test drives it through a result object whose stdout is a getter that counts reads: one read means the guard fired, two means the guard passed and the fallback ran. Deleting the line again fails the value assertion — the function would return undefined — and changing the guard to swallow whitespace fails the read count, which is the other way that branch dies. Tier attribution is now pinned. Four tests rig the lower tiers to answer with DIFFERENT branch names and assert the recorded call sequence, so a result from symref, remote-show, local-branch, or config can no longer be mistaken for another. Previously any of them could have produced the answer and the test would not have noticed. Each tier's argv and timeout are asserted exactly. The boundary trio applies to the branch listing rather than a numeric limit: zero matching lines, one, and two. Three assertions already in this file were removed rather than left beside the new ones. A doesNotThrow around the worktree probe became an exact-value test of the catch arm; a typeof-string-and-non-empty shape check on the resolved root became an equality check against the realpath; and the unverified-fallback diagnostic was asserted only to be non-empty, so it now asserts the exact sentence, which fails if the reason changes rather than only if it vanishes. Two folded bodies used try/finally for teardown and now use t.after. Refs #3057 Co-Authored-By: Claude Opus 5 --- tests/git-base-branch.test.cjs | 621 +++++++++++++++++++++++++++++---- 1 file changed, 556 insertions(+), 65 deletions(-) diff --git a/tests/git-base-branch.test.cjs b/tests/git-base-branch.test.cjs index d67ff38bc..7a0936407 100644 --- a/tests/git-base-branch.test.cjs +++ b/tests/git-base-branch.test.cjs @@ -283,9 +283,14 @@ describe('#1268 gitWorktreeInfoInternal: relocation to git-base-branch', () => { const dir = createTempGitProject('gsd-wt-info-'); t.after(() => cleanup(dir)); const result = gitBaseBranch.gitWorktreeInfoInternal(dir); + // `git rev-parse --show-toplevel` reports the resolved (symlink-free) path, + // which on macOS differs from the mkdtemp path (/var → /private/var). Pin the + // exact value rather than "a non-empty string": a resolver that returned the + // .git dir, the cwd, or any other plausible-looking path would pass the weaker + // shape check while being wrong. assert.strictEqual(result.inside, true, 'inside must be true for a git project dir'); - assert.ok(typeof result.worktreeRoot === 'string' && result.worktreeRoot.length > 0, - `worktreeRoot must be a non-empty string, got: ${JSON.stringify(result.worktreeRoot)}`); + assert.strictEqual(result.worktreeRoot, fs.realpathSync.native(dir), + 'worktreeRoot must be the resolved worktree root path'); }); test('gitWorktreeInfoInternal(createTempDir()) returns {inside:false, worktreeRoot:null} for a non-git dir', (t) => { @@ -296,11 +301,12 @@ describe('#1268 gitWorktreeInfoInternal: relocation to git-base-branch', () => { assert.strictEqual(result.worktreeRoot, null, 'worktreeRoot must be null for a non-git dir'); }); - test('gitWorktreeInfoInternal never throws (non-git dir)', (t) => { - const dir = createTempDir('gsd-wt-info-nothrow-'); - t.after(() => cleanup(dir)); - assert.doesNotThrow(() => gitBaseBranch.gitWorktreeInfoInternal(dir)); - }); + // NOTE: the former "never throws" liveness test that sat here was replaced + // (#3057 W3). "It did not throw" is satisfied by a function that returns + // undefined, the wrong branch, or nothing useful at all. The + // `execGit throws → {inside:false, worktreeRoot:null}` test below asserts the + // exact value the catch arm is contracted to produce, which is what the old + // test was gesturing at. }); // ─── #3057 B4: last-resort "main" — verified vs unverified ─────────────────── @@ -365,7 +371,11 @@ describe('#3057 B4: resolveBaseBranchDiagnostics — verified vs unverified last writeDiagnostic: (s) => { stderrText += s; }, }); assert.strictEqual(stdoutText, 'main\n'); - assert.ok(stderrText.length > 0, 'the unverified fallback must write a stderr diagnostic'); + assert.strictEqual( + stderrText, + `⚠ git-base-branch: defaulted to 'main' WITHOUT verifying against this repository — ` + + `a git query timed out or could not run. See #3057.\n`, + ); stdoutText = ''; stderrText = ''; @@ -379,6 +389,491 @@ describe('#3057 B4: resolveBaseBranchDiagnostics — verified vs unverified last }); }); +// ─── #3057 W3: negative-space coverage for the resolver's failure arms ─────── +// +// Everything below drives the *unhappy* halves of git-base-branch: malformed +// config, git output that parses but says nothing useful, git that cannot run +// at all, and a repository with no work tree. Each test asserts the exact value +// the arm is contracted to produce — never "it did not throw", never a shape +// check — because an arm that silently returns `undefined` instead of `null` +// changes the precedence ladder's behaviour while passing any weaker assertion. + +/** + * Build a result object shaped exactly like `execGit`'s (see `_spawnResult` in + * shell-command-projection). Defaults are a benign, completed, zero-exit call. + */ +function gitResult(overrides) { + return { + exitCode: 0, + stdout: '', + stderr: '', + signal: null, + error: null, + timedOut: false, + ...overrides, + }; +} + +/** An `execGit` stand-in that always returns the same shaped result. */ +function constGit(overrides) { + return () => gitResult(overrides); +} + +/** + * An `execGit` stand-in that throws. `makeFaultyGit` deliberately never throws + * (it returns a shaped failure result), so the resolver's `catch` arms need + * this instead. + */ +function throwingGit(message) { + return () => { throw new Error(message); }; +} + +describe('#3057 W3: readConfigBaseBranch — config present but unusable', () => { + const PLANNING_DIR = path.join(path.sep, 'gsd-3057-w3', '.planning'); + + /** Read a config whose raw text is `raw`, recording the paths requested. */ + function readWith(raw, seenPaths) { + return gitBaseBranch.readConfigBaseBranch(PLANNING_DIR, { + readFile: (p) => { if (seenPaths) seenPaths.push(p); return raw; }, + }); + } + + test('config.json exists but is not valid JSON → null (parse failure swallowed)', () => { + const seen = []; + assert.strictEqual(readWith('{ not json', seen), null); + assert.deepStrictEqual(seen, [path.join(PLANNING_DIR, 'config.json')], + 'the resolver must look for config.json inside the planning dir it was given'); + }); + + test('config.json parses to a non-object → null for null / string / number / array', () => { + assert.strictEqual(readWith('null'), null, 'JSON null must not be treated as a config'); + assert.strictEqual(readWith('"master"'), null, 'a bare JSON string must not be treated as a config'); + assert.strictEqual(readWith('42'), null, 'a bare JSON number must not be treated as a config'); + assert.strictEqual(readWith('[]'), null, 'a JSON array must not be treated as a config'); + }); + + test('"git" section present but base_branch missing / non-string / blank → null', () => { + assert.strictEqual(readWith('{"git":{}}'), null); + assert.strictEqual(readWith('{"git":{"base_branch":42}}'), null); + assert.strictEqual(readWith('{"git":{"base_branch":null}}'), null); + assert.strictEqual(readWith('{"git":{"base_branch":""}}'), null); + assert.strictEqual(readWith('{"git":{"base_branch":" "}}'), null, + 'a whitespace-only override must not win the precedence ladder'); + }); + + test('"git" key present but not an object → skipped, flat legacy key still consulted', () => { + assert.strictEqual(readWith('{"git":"main","base_branch":"release"}'), 'release'); + assert.strictEqual(readWith('{"git":[],"base_branch":"release"}'), 'release'); + assert.strictEqual(readWith('{"git":null,"base_branch":"release"}'), 'release'); + }); + + test('flat base_branch present but non-string / blank → null', () => { + assert.strictEqual(readWith('{"base_branch":true}'), null); + assert.strictEqual(readWith('{"base_branch":["main"]}'), null); + assert.strictEqual(readWith('{"base_branch":""}'), null); + assert.strictEqual(readWith('{"base_branch":" "}'), null); + }); + + test('config parses cleanly but carries neither key → null (distinct from an absent file)', () => { + // The absent-file path returns null after reading an empty string and never + // reaches JSON.parse. This one parses a real object and falls all the way + // through both key lookups to the final return. + assert.strictEqual(readWith('{"other":1}'), null); + assert.strictEqual(readWith('{}'), null); + assert.strictEqual(readWith(''), null, 'absent file (empty read) also yields null'); + }); + + test('positive controls: values are trimmed, and the nested key outranks the flat one', () => { + assert.strictEqual(readWith('{"git":{"base_branch":" develop "}}'), 'develop'); + assert.strictEqual(readWith('{"base_branch":" release\\n"}'), 'release'); + assert.strictEqual(readWith('{"git":{"base_branch":"nested"},"base_branch":"flat"}'), 'nested'); + }); +}); + +describe('#3057 W3: trySymbolicRef — tier-2 output that resolves to nothing', () => { + test('stdout is exactly "origin/" → null (prefix strip leaves an empty name)', () => { + assert.strictEqual(gitBaseBranch.trySymbolicRef('/x', constGit({ stdout: 'origin/' })), null); + assert.strictEqual(gitBaseBranch.trySymbolicRef('/x', constGit({ stdout: 'origin/\n' })), null); + }); + + test('only ONE leading "origin/" is stripped — slashes inside the name survive', () => { + assert.strictEqual( + gitBaseBranch.trySymbolicRef('/x', constGit({ stdout: 'origin/feature/long-name\n' })), + 'feature/long-name'); + assert.strictEqual( + gitBaseBranch.trySymbolicRef('/x', constGit({ stdout: 'origin/origin/main\n' })), + 'origin/main'); + }); + + test('execGit THROWS → null (catch arm; makeFaultyGit cannot reach this)', () => { + assert.strictEqual(gitBaseBranch.trySymbolicRef('/x', throwingGit('symbolic-ref exploded')), null); + }); + + test('the tier-2 subprocess is bounded (argv + timeout are pinned)', () => { + const seen = []; + gitBaseBranch.trySymbolicRef('/some/cwd', (args, opts) => { + seen.push({ args, opts }); + return gitResult({ stdout: 'origin/main\n' }); + }); + assert.deepStrictEqual(seen, [{ + args: ['symbolic-ref', '--quiet', '--short', 'refs/remotes/origin/HEAD'], + opts: { cwd: '/some/cwd', timeout: 5_000 }, + }]); + }); +}); + +describe('#3057 W3: tryRemoteShow — tier-3 output that is present but not authoritative', () => { + const REMOTE_SHOW_NO_HEAD = [ + '* remote origin', + ' Fetch URL: /tmp/origin.git', + ' Push URL: /tmp/origin.git', + ' Remote branch:', + ' main tracked', + '', + ].join('\n'); + + test('stdout has no "HEAD branch:" line → null', () => { + assert.strictEqual( + gitBaseBranch.tryRemoteShow('/x', constGit({ stdout: REMOTE_SHOW_NO_HEAD })), null); + }); + + test('"HEAD branch:" with no value on the line → null (no capture, no guess)', () => { + assert.strictEqual( + gitBaseBranch.tryRemoteShow('/x', constGit({ stdout: ' HEAD branch: \n' })), null); + }); + + test('"HEAD branch: (unknown)" → null — the documented offline-remote case', () => { + // git prints "(unknown)" when it could not reach the remote. Returning it + // verbatim would hand a literal branch named "(unknown)" to five workflows; + // returning null lets tier 4 answer instead. + assert.strictEqual( + gitBaseBranch.tryRemoteShow('/x', constGit({ stdout: ' HEAD branch: (unknown)\n' })), null); + }); + + test('execGit THROWS → null (catch arm)', () => { + assert.strictEqual(gitBaseBranch.tryRemoteShow('/x', throwingGit('remote show exploded')), null); + }); + + test('positive control: the HEAD branch line is found mid-output and returned verbatim', () => { + const stdout = [ + '* remote origin', + ' Fetch URL: /tmp/origin.git', + ' HEAD branch: master', + ' Remote branch:', + ' master tracked', + '', + ].join('\n'); + assert.strictEqual(gitBaseBranch.tryRemoteShow('/x', constGit({ stdout })), 'master'); + }); + + test('the tier-3 subprocess is bounded (argv + timeout are pinned)', () => { + const seen = []; + gitBaseBranch.tryRemoteShow('/some/cwd', (args, opts) => { + seen.push({ args, opts }); + return gitResult({ stdout: ' HEAD branch: main\n' }); + }); + assert.deepStrictEqual(seen, [{ + args: ['remote', 'show', 'origin'], + opts: { cwd: '/some/cwd', timeout: 15_000 }, + }]); + }); +}); + +describe('#3057 W3: tryLocalBranch — non-empty stdout that names neither main nor master', () => { + /** + * An `execGit` stand-in whose result counts how many times `stdout` is read. + * `tryLocalBranch` reads it once in the `!r.stdout` guard and a second time to + * split it into lines. The read count therefore identifies WHICH null the + * function returned: 1 read = the early guard fired; 2 reads = the guard was + * passed and the final `return null` after the main/master checks ran. + */ + function countingGit(stdoutValue) { + const state = { reads: 0 }; + const git = () => ({ + exitCode: 0, + stderr: '', + signal: null, + error: null, + timedOut: false, + get stdout() { state.reads += 1; return stdoutValue; }, + }); + git.state = state; + return git; + } + + test('stdout is exactly "\\n" → null, reached PAST the empty-stdout guard', () => { + // This is the branch that was once deleted as "unreachable". The guard is + // `if (r.exitCode !== 0 || !r.stdout) return null` — `"\n"` is a truthy + // string, so the guard does NOT fire; `split('\n')` yields ["", ""], both + // main/master checks are false, and the FINAL `return null` executes. + // Deleting that line makes this function return `undefined`, which + // strictEqual(null) catches. The read count proves which null we got. + const git = countingGit('\n'); + assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', git), null); + assert.strictEqual(git.state.reads, 2, + 'stdout must be read twice: once by the guard (which passes) and once to split into lines'); + }); + + test('stdout is exactly "" → null via the EARLY guard (a different arm)', () => { + const git = countingGit(''); + assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', git), null); + assert.strictEqual(git.state.reads, 1, + 'an empty stdout must short-circuit in the guard, never reaching the line split'); + }); + + // Boundary trio over the number of branch lines `git branch --list main master` + // can emit: 0 (below the smallest useful listing), 1, and 2 (the maximum this + // argv can produce). + test('0 branch lines → null', () => { + assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', constGit({ stdout: '\n' })), null); + }); + + test('1 branch line → that branch', () => { + assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', constGit({ stdout: ' main\n' })), 'main'); + assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', constGit({ stdout: ' master\n' })), 'master'); + assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', constGit({ stdout: '* master\n' })), 'master', + 'the checked-out marker "* " must be stripped before matching'); + }); + + test('2 branch lines → "main" wins the tie-break', () => { + assert.strictEqual( + gitBaseBranch.tryLocalBranch('/x', constGit({ stdout: ' main\n master\n' })), 'main'); + assert.strictEqual( + gitBaseBranch.tryLocalBranch('/x', constGit({ stdout: '* master\n main\n' })), 'main'); + }); + + test('execGit THROWS → null (catch arm)', () => { + assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', throwingGit('branch --list exploded')), null); + }); + + test('the tier-4 subprocess is bounded (argv + timeout are pinned)', () => { + const seen = []; + gitBaseBranch.tryLocalBranch('/some/cwd', (args, opts) => { + seen.push({ args, opts }); + return gitResult({ stdout: ' main\n' }); + }); + assert.deepStrictEqual(seen, [{ + args: ['branch', '--list', 'main', 'master'], + opts: { cwd: '/some/cwd', timeout: 5_000 }, + }]); + }); +}); + +describe('#3057 W3: resolveBaseBranchDiagnostics — which tier actually answered', () => { + // Every tier can produce a plausible-looking branch name, so asserting the + // returned string alone cannot tell a tier-2 answer from a tier-3 or tier-4 + // one. Each test below rigs the LOWER tiers to answer with a DIFFERENT branch + // than the tier under test, so a resolver that consulted them in the wrong + // order returns the wrong string, and additionally pins the recorded argv so + // an early return is provably an early return. + + /** A passthrough answering each tier with a distinct, recognisable branch. */ + function tieredPassthrough({ symref, remote, local }) { + return (args) => { + if (args[0] === 'symbolic-ref') { + return symref === null ? gitResult({ exitCode: 1 }) : gitResult({ stdout: `origin/${symref}\n` }); + } + if (args[0] === 'remote') { + return remote === null ? gitResult({ exitCode: 128 }) : gitResult({ stdout: ` HEAD branch: ${remote}\n` }); + } + if (args[0] === 'branch') { + return local === null ? gitResult({ stdout: '\n' }) : gitResult({ stdout: ` ${local}\n` }); + } + return gitResult({}); + }; + } + + const NO_CONFIG = { readFile: () => null }; + + test('tier 2 answers → tiers 3 and 4 are never consulted', () => { + const git = makeFaultyGit({ + passthrough: tieredPassthrough({ symref: 'from-symref', remote: 'from-remote', local: 'master' }), + }); + const result = gitBaseBranch.resolveBaseBranchDiagnostics('/x', { ...NO_CONFIG, execGit: git }); + assert.deepStrictEqual(result, { branch: 'from-symref', verified: true }); + assert.deepStrictEqual(git.calls.map((c) => c.args[0]), ['symbolic-ref'], + 'a tier-2 hit must stop the ladder before `remote show` and `branch --list`'); + }); + + test('tier 3 answers → tier 4 is never consulted, even though it WOULD answer "master"', () => { + const git = makeFaultyGit({ + passthrough: tieredPassthrough({ symref: null, remote: 'from-remote', local: 'master' }), + }); + const result = gitBaseBranch.resolveBaseBranchDiagnostics('/x', { ...NO_CONFIG, execGit: git }); + assert.deepStrictEqual(result, { branch: 'from-remote', verified: true }); + assert.deepStrictEqual(git.calls.map((c) => c.args[0]), ['symbolic-ref', 'remote'], + 'a tier-3 hit must stop the ladder before `branch --list`'); + }); + + test('tier 4 answers only after tiers 2 and 3 both decline', () => { + const git = makeFaultyGit({ + passthrough: tieredPassthrough({ symref: null, remote: null, local: 'master' }), + }); + const result = gitBaseBranch.resolveBaseBranchDiagnostics('/x', { ...NO_CONFIG, execGit: git }); + assert.deepStrictEqual(result, { branch: 'master', verified: true }); + assert.deepStrictEqual(git.calls.map((c) => c.args[0]), ['symbolic-ref', 'remote', 'branch']); + }); + + test('a config override answers before ANY git subprocess runs', () => { + const git = makeFaultyGit({ + passthrough: tieredPassthrough({ symref: 'from-symref', remote: 'from-remote', local: 'master' }), + }); + const result = gitBaseBranch.resolveBaseBranchDiagnostics('/x', { + readFile: () => '{"git":{"base_branch":"from-config"}}', + execGit: git, + }); + assert.deepStrictEqual(result, { branch: 'from-config', verified: true }); + assert.deepStrictEqual(git.calls, [], 'tier 1 must not spawn git at all'); + }); + + test('git cannot be SPAWNED at all (exit 127 + error) → "main", verified:false', () => { + // Distinct from the timeout case already covered by #3057 B4: here every + // call returns exitCode 127 with `error` set and `timedOut:false`, which is + // the `r.error` disjunct of the failure detector rather than `r.timedOut`. + const git = makeFaultyGit({ faults: [{ kind: 'spawnFail' }] }); + const result = gitBaseBranch.resolveBaseBranchDiagnostics('/x', { ...NO_CONFIG, execGit: git }); + assert.deepStrictEqual(result, { branch: 'main', verified: false }); + assert.deepStrictEqual(git.calls.map((c) => c.args[0]), ['symbolic-ref', 'remote', 'branch'], + 'all three tiers must still be attempted before the unverified default'); + }); + + test('a spawn failure on ONE tier alone is enough to mark the default unverified', () => { + // Tiers 2 and 3 complete cleanly with "no answer"; only tier 4 fails to run. + const git = makeFaultyGit({ + faults: [{ kind: 'spawnFail', when: ['branch', '--list'] }], + passthrough: tieredPassthrough({ symref: null, remote: null, local: null }), + }); + const result = gitBaseBranch.resolveBaseBranchDiagnostics('/x', { ...NO_CONFIG, execGit: git }); + assert.deepStrictEqual(result, { branch: 'main', verified: false }); + }); + + test('tier-4 stdout of "\\n" (no branches) is a VERIFIED "no candidate", not a failure', () => { + // The counterpart to the tryLocalBranch "\n" test, one level up: git ran, + // answered, and the answer was "neither branch exists". That must still be + // verified:true — collapsing it into verified:false would re-fail-open the + // exact distinction #3057 B4 introduced. + const git = makeFaultyGit({ + passthrough: tieredPassthrough({ symref: null, remote: null, local: null }), + }); + const result = gitBaseBranch.resolveBaseBranchDiagnostics('/x', { ...NO_CONFIG, execGit: git }); + assert.deepStrictEqual(result, { branch: 'main', verified: true }); + }); +}); + +describe('#3057 W3: gitWorktreeInfoInternal — no work tree, and git failing mid-sequence', () => { + test('a REAL bare repository reports {inside:false, worktreeRoot:null}', (t) => { + // `git rev-parse --is-inside-work-tree` exits 0 in a bare repo and prints + // "false" — the exitCode guard does NOT fire, so this is the stdout check, + // 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' }); + + assert.deepStrictEqual( + gitBaseBranch.gitWorktreeInfoInternal(dir), + { inside: false, worktreeRoot: null }); + }); + + test('is-inside-work-tree prints "false" with exit 0 → no second git call is made', () => { + const git = makeFaultyGit({ passthrough: () => gitResult({ stdout: 'false\n' }) }); + assert.deepStrictEqual( + gitBaseBranch.gitWorktreeInfoInternal('/x', { execGit: git }), + { inside: false, worktreeRoot: null }); + assert.deepStrictEqual(git.calls.map((c) => c.args), [['rev-parse', '--is-inside-work-tree']], + '--show-toplevel must not be queried once we know there is no work tree'); + }); + + test('inside a work tree but --show-toplevel FAILS → {inside:true, worktreeRoot:null}', () => { + // inside is still reported truthfully; only the root is unknown. Reporting + // inside:false here would be a lie about a repository we just confirmed. + const git = makeFaultyGit({ + faults: [{ + kind: 'exit', + exitCode: 128, + stderr: 'fatal: no work tree', + when: ['rev-parse', '--show-toplevel'], + }], + passthrough: () => gitResult({ stdout: 'true\n' }), + }); + assert.deepStrictEqual( + gitBaseBranch.gitWorktreeInfoInternal('/x', { execGit: git }), + { inside: true, worktreeRoot: null }); + assert.deepStrictEqual(git.calls.map((c) => c.args[1]), + ['--is-inside-work-tree', '--show-toplevel']); + }); + + test('--show-toplevel succeeds with blank stdout → {inside:true, worktreeRoot:null}', () => { + const git = makeFaultyGit({ + passthrough: (args) => gitResult({ stdout: args[1] === '--show-toplevel' ? ' \n' : 'true\n' }), + }); + assert.deepStrictEqual( + gitBaseBranch.gitWorktreeInfoInternal('/x', { execGit: git }), + { inside: true, worktreeRoot: null }); + }); + + test('--show-toplevel succeeds → the trimmed path is returned', () => { + const git = makeFaultyGit({ + passthrough: (args) => gitResult({ stdout: args[1] === '--show-toplevel' ? ' /repo/root \n' : 'true\n' }), + }); + assert.deepStrictEqual( + gitBaseBranch.gitWorktreeInfoInternal('/x', { execGit: git }), + { inside: true, worktreeRoot: '/repo/root' }); + }); + + test('execGit THROWS → {inside:false, worktreeRoot:null} (catch arm)', () => { + assert.deepStrictEqual( + gitBaseBranch.gitWorktreeInfoInternal('/x', { execGit: throwingGit('git is gone') }), + { inside: false, worktreeRoot: null }); + }); + + test('both worktree probes are bounded and receive the caller cwd', () => { + const git = makeFaultyGit({ + passthrough: (args) => gitResult({ stdout: args[1] === '--show-toplevel' ? '/repo/root\n' : 'true\n' }), + }); + gitBaseBranch.gitWorktreeInfoInternal('/some/cwd', { execGit: git }); + assert.deepStrictEqual(git.calls, [ + { args: ['rev-parse', '--is-inside-work-tree'], opts: { cwd: '/some/cwd', timeout: 5000 } }, + { args: ['rev-parse', '--show-toplevel'], opts: { cwd: '/some/cwd', timeout: 5000 } }, + ]); + }); +}); + +describe('#3057 W3: cmdGitBaseBranch — the DEFAULT diagnostic sink', () => { + test('with no writeDiagnostic injected, the unverified warning goes to process.stderr', (t) => { + const written = []; + t.mock.method(process.stderr, 'write', (chunk) => { written.push(String(chunk)); return true; }); + + const stdout = []; + const branch = gitBaseBranch.cmdGitBaseBranch('/x', [], { + readFile: () => null, + execGit: makeFaultyGit({ faults: [{ kind: 'timeout' }] }), + write: (s) => { stdout.push(s); }, + // writeDiagnostic deliberately omitted → the process.stderr default arm. + }); + + assert.strictEqual(branch, 'main'); + assert.deepStrictEqual(stdout, ['main\n'], 'the stdout contract five workflows parse is unchanged'); + assert.strictEqual(written.length, 1, 'exactly one diagnostic must reach the default stderr sink'); + assert.match(written[0], /WITHOUT verifying/); + }); + + test('with no writeDiagnostic injected and a VERIFIED answer, process.stderr is untouched', (t) => { + const written = []; + t.mock.method(process.stderr, 'write', (chunk) => { written.push(String(chunk)); return true; }); + + const stdout = []; + const branch = gitBaseBranch.cmdGitBaseBranch('/x', [], { + readFile: () => '{"git":{"base_branch":"develop"}}', + execGit: makeFaultyGit(), + write: (s) => { stdout.push(s); }, + }); + + assert.strictEqual(branch, 'develop'); + assert.deepStrictEqual(stdout, ['develop\n']); + assert.deepStrictEqual(written, [], 'a verified answer must write nothing to the default stderr sink'); + }); +}); + // ─── setGsdConfig prototype-pollution guard (#1406) ─────────────────────────── describe('#1406: setGsdConfig prototype-pollution guard', () => { @@ -697,76 +1192,72 @@ describe('handle_branching branches off origin/HEAD, not current HEAD (#2916)', // exercising the symbolic-ref code path) so a regression that hard-codes // `main` instead of consulting origin/HEAD will fail the trunk variant. for (const defaultBranch of ['main', 'trunk']) { - test(`new phase branch branches off origin/${defaultBranch} with 0 inherited commits`, () => { + test(`new phase branch branches off origin/${defaultBranch} with 0 inherited commits`, (t) => { const bash = extractHandleBranchingBash(); const { root, clonePath } = setupFixture(defaultBranch); + // Teardown via t.after, not try/finally — CONTRIBUTING.md "Setup and + // Cleanup" reserves try/finally for context-free helper functions. + t.after(() => cleanup(root)); - try { - const upstream = `origin/${defaultBranch}`; + const upstream = `origin/${defaultBranch}`; - assert.equal( - git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), - 'feature/phase-01-foundation' - ); - assert.equal( - git(clonePath, 'rev-list', '--count', `${upstream}..HEAD`), - '1', - `fixture should be 1 commit ahead of ${upstream}` - ); - - runHandleBranchingStep(bash, clonePath, 'feature/phase-02-content-sync'); - - assert.equal( - git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), - 'feature/phase-02-content-sync', - 'handle_branching should switch to the new phase branch' - ); - - const inherited = git(clonePath, 'rev-list', '--count', `${upstream}..HEAD`); - assert.equal( - inherited, - '0', - `new phase branch must branch off ${upstream}, but inherited ${inherited} commit(s) from previous-phase HEAD` - ); - assert.equal( - git(clonePath, 'rev-parse', 'HEAD'), - git(clonePath, 'rev-parse', upstream), - `new phase branch tip must equal ${upstream} tip` - ); - } finally { - cleanup(root); - } - }); - } - - test('handle_branching reuses an existing branch instead of forking again', () => { - const bash = extractHandleBranchingBash(); - const { root, clonePath } = setupFixture(); - - try { - // Pre-create the target branch off origin/main with its own commit, then - // walk away to a different branch — the step must switch back to it. - git(clonePath, 'checkout', '-B', 'feature/phase-02-content-sync', 'origin/main'); - fs.writeFileSync(path.join(clonePath, 'phase02.txt'), 'phase 2 work\n'); - git(clonePath, 'add', 'phase02.txt'); - git(clonePath, 'commit', '-m', 'phase 02 wip'); - const phase02Sha = git(clonePath, 'rev-parse', 'HEAD'); - git(clonePath, 'checkout', 'feature/phase-01-foundation'); + assert.equal( + git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), + 'feature/phase-01-foundation' + ); + assert.equal( + git(clonePath, 'rev-list', '--count', `${upstream}..HEAD`), + '1', + `fixture should be 1 commit ahead of ${upstream}` + ); runHandleBranchingStep(bash, clonePath, 'feature/phase-02-content-sync'); assert.equal( git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), - 'feature/phase-02-content-sync' + 'feature/phase-02-content-sync', + 'handle_branching should switch to the new phase branch' + ); + + const inherited = git(clonePath, 'rev-list', '--count', `${upstream}..HEAD`); + assert.equal( + inherited, + '0', + `new phase branch must branch off ${upstream}, but inherited ${inherited} commit(s) from previous-phase HEAD` ); assert.equal( git(clonePath, 'rev-parse', 'HEAD'), - phase02Sha, - 'existing-branch tip must be preserved (no rebase/reset)' + git(clonePath, 'rev-parse', upstream), + `new phase branch tip must equal ${upstream} tip` ); - } finally { - cleanup(root); - } + }); + } + + test('handle_branching reuses an existing branch instead of forking again', (t) => { + const bash = extractHandleBranchingBash(); + const { root, clonePath } = setupFixture(); + t.after(() => cleanup(root)); + + // Pre-create the target branch off origin/main with its own commit, then + // walk away to a different branch — the step must switch back to it. + git(clonePath, 'checkout', '-B', 'feature/phase-02-content-sync', 'origin/main'); + fs.writeFileSync(path.join(clonePath, 'phase02.txt'), 'phase 2 work\n'); + git(clonePath, 'add', 'phase02.txt'); + git(clonePath, 'commit', '-m', 'phase 02 wip'); + const phase02Sha = git(clonePath, 'rev-parse', 'HEAD'); + git(clonePath, 'checkout', 'feature/phase-01-foundation'); + + runHandleBranchingStep(bash, clonePath, 'feature/phase-02-content-sync'); + + assert.equal( + git(clonePath, 'rev-parse', '--abbrev-ref', 'HEAD'), + 'feature/phase-02-content-sync' + ); + assert.equal( + git(clonePath, 'rev-parse', 'HEAD'), + phase02Sha, + 'existing-branch tip must be preserved (no rebase/reset)' + ); }); }); }); From a7fdedac6a9a5666947f07fb66324288af8d0864 Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 6 Aug 2026 00:59:05 -0400 Subject: [PATCH 3/7] test(#3103): drive the orphan reaper through its injected dependencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thirty-four tests covering every branch in the reaping path that no test reached, which was all of the fail-closed ones. The function has always accepted an injectable dependency bag; nothing used it. Every existing test drove real git and injected only the clock and the liveness probe, so each guard that exists for a failure — an unreadable git dir, a null directory listing, an unresolvable remote ref, a missing pointer file, an unlocked sibling, an ambiguous remote — had never executed. They are now driven by injecting exactly the fault that selects them, and each asserts its specific status and reason rather than that something happened. The last one needed no new mechanism, only the right one. It was reported as unreachable without a cross-user PID, but the default liveness helper is reachable by not injecting over it and patching process.kill, which is the deterministic injection this repo requires over real OS conditions. Its three outcomes — EPERM, ESRCH, and a clean return — now assert their verdicts. The assertions were kill-tested rather than assumed. Against mutated copies of the built module, renaming the six reason strings fails eighteen tests, neutralising the fail-closed returns fails seven more, dropping the ambiguous-remote guard fails one, and removing the prune catch and its timeout guard fails both prune tests. Flipping the EPERM arm to false turns a skip into a reap and fails that test. Four places where production folds distinct causes into one verdict are recorded in the tests rather than papered over. A lock is too fresh whether its mtime is unreadable or merely recent; a branch tip fails to resolve for three different reasons; a PID reads as alive whether the owner lives or the probe threw. Where the return value cannot separate them the tests assert the git call sequence instead, and where even that cannot, the test says so. Six weak assertions already in the older file are replaced rather than left beside the new ones: five guarded their assertions behind `if (entry)`, so a missing entry skipped the check and passed, and one asserted only that the reaper returned a non-empty array. Three JSON parses wrapped in doesNotThrow now parse directly, so a malformed payload reports its own syntax error instead of a generic message. Refs #3057 Co-Authored-By: Claude Opus 5 --- tests/worktree-safety-reap.test.cjs | 821 ++++++++++++++++++++++++++++ tests/worktree-safety.test.cjs | 66 +-- 2 files changed, 855 insertions(+), 32 deletions(-) create mode 100644 tests/worktree-safety-reap.test.cjs diff --git a/tests/worktree-safety-reap.test.cjs b/tests/worktree-safety-reap.test.cjs new file mode 100644 index 000000000..d390136c6 --- /dev/null +++ b/tests/worktree-safety-reap.test.cjs @@ -0,0 +1,821 @@ +'use strict'; + +/** + * `reapOrphanWorktrees` — fault-injected verdict coverage (#3057, wave 3). + * + * Seam: gsd-core/bin/lib/worktree-safety.cjs + * Interface: reapOrphanWorktrees, cmdWorktreeReapOrphans, pruneOrphanedWorktrees + * + * WHY A SECOND FILE FOR THIS MODULE + * `tests/worktree-safety.test.cjs` is ~6.4k lines and its `reapOrphanWorktrees` + * suites live inside a folded block with their own local fixture helpers. The + * negative-space work below needs a different fixture shape (an injected + * `execGit` that delegates to real git, plus per-test mutation of the + * `.git/worktrees//` admin directory), so it gets its own module-bucketed + * file rather than a third set of helpers wedged into the folded block. + * + * WHAT THIS FILE PINS THAT NOTHING ELSE DID + * Every pre-existing test drove the DEFAULT `execGit` against real git and + * injected only `mtimeSafe` / `nowMs` / `isPidAlive`. `reapOrphanWorktrees` + * accepts `execGit`, `readDirSafe` and `readFileSafe` in the same `deps` bag, + * and nothing used them — so every fail-closed `return` inside the function was + * unreachable from the suite. Each test here injects exactly the one fault that + * selects one branch and asserts the SPECIFIC `{status, reason}` verdict that + * branch produces, never merely that the call returned an array. + * + * Determinism: no wall clock is read (`mtimeSafe`/`nowMs` are injected), and no + * live PID is probed (`isPidAlive` is injected), so the only real-world + * dependency is git itself. + */ + +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 os = require('node:os'); +const { cleanup } = require('./helpers.cjs'); +const { runGit } = require('./helpers/process-seam.cjs'); +const { makeFaultyGit, withFaultyFs } = require('./helpers/faulty-deps.cjs'); + +const { + reapOrphanWorktrees, + cmdWorktreeReapOrphans, + pruneOrphanedWorktrees, +} = require('../gsd-core/bin/lib/worktree-safety.cjs'); + +// ─── Fixed clock values (ADR-456 clock seam) ───────────────────────────────── + +/** Older than any staleness threshold, at any real point in time. */ +const STALE_MTIME = new Date(0); + +/** The lock-owner PID written into every fixture; liveness is always injected. */ +const LOCK_OWNER_PID = '4242'; + +const GIT_TIMEOUT_MS = 30000; + +// ─── Path + git helpers ────────────────────────────────────────────────────── + +function canonicalPath(p) { + try { return fs.realpathSync.native(path.resolve(p)); } catch { return path.resolve(p); } +} + +/** + * Long-form os.tmpdir(). Windows CI reports 8.3 short names that git does not + * echo back, so every fixture path is built from the resolved form. + */ +function resolvedTmpDir() { + try { return fs.realpathSync.native(os.tmpdir()); } catch { return os.tmpdir(); } +} + +/** 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}`); + } + return r.stdout; +} + +/** + * An `execGit`-shaped delegate that runs REAL git. Used as `makeFaultyGit`'s + * `passthrough` so a test can fault one argv and leave every other call intact. + */ +function realExecGit(args, opts = {}) { + const r = runGit(args, { cwd: opts.cwd, timeoutMs: GIT_TIMEOUT_MS }); + return { + exitCode: r.exitCode, + stdout: r.stdout, + stderr: r.stderr, + signal: r.signal, + error: r.code === null ? null : Object.assign(new Error(r.code), { code: r.code }), + timedOut: r.timedOut, + }; +} + +/** A benign zero-exit result carrying `stdout`. */ +function okResult(stdout) { + return { exitCode: 0, stdout, stderr: '', signal: null, error: null, timedOut: false }; +} + +function argvOf(faultyGit) { + return faultyGit.calls.map((c) => c.args.join(' ')); +} + +function calledWith(faultyGit, prefix) { + return faultyGit.calls.some((c) => prefix.every((token, i) => c.args[i] === token)); +} + +// ─── Fixture construction ──────────────────────────────────────────────────── + +function initRepo(dir) { + fs.mkdirSync(dir, { recursive: true }); + git(['init'], dir); + git(['config', 'user.email', 'test@test.com'], dir); + git(['config', 'user.name', 'Test'], dir); + git(['config', 'commit.gpgsign', 'false'], dir); + fs.writeFileSync(path.join(dir, 'README.md'), '# Test\n'); + git(['add', '-A'], dir); + git(['commit', '-m', 'initial commit'], dir); + // Exit code deliberately unchecked: the rename fails harmlessly when the + // repo was already initialised on `main`. + runGit(['branch', '-m', 'master', 'main'], { cwd: dir, timeoutMs: GIT_TIMEOUT_MS }); +} + +/** Locate `.git/worktrees//` for a linked worktree. */ +function adminDirFor(repoDir, wtDir) { + const commonDir = path.resolve(repoDir, git(['rev-parse', '--git-common-dir'], repoDir).trim()); + const worktreesDir = path.join(commonDir, 'worktrees'); + const wanted = canonicalPath(wtDir); + for (const entry of fs.readdirSync(worktreesDir)) { + const gitdirFile = path.join(worktreesDir, entry, 'gitdir'); + if (!fs.existsSync(gitdirFile)) continue; + const pointer = fs.readFileSync(gitdirFile, 'utf8').trim(); + const root = path.resolve(worktreesDir, entry, pointer).replace(/[/\\]\.git$/, ''); + if (canonicalPath(root) === wanted) return path.join(worktreesDir, entry); + } + throw new Error(`no .git/worktrees/ admin dir for ${wtDir}`); +} + +/** + * Build a repo with one linked, locked worktree whose branch is merged into + * `main` unless `merge:false`. The lock owner is a fixed PID string; liveness is + * always supplied through `deps.isPidAlive`, never probed against the OS. + */ +function makeFixture(tmpBase, name, options = {}) { + const repoDir = path.join(tmpBase, `repo-${name}`); + const wtDir = path.join(tmpBase, `wt-${name}`); + const branch = `worktree-agent-${name}`; + + initRepo(repoDir); + git(['worktree', 'add', wtDir, '-b', branch], repoDir); + fs.writeFileSync(path.join(wtDir, 'work.txt'), 'content\n'); + git(['add', '-A'], wtDir); + git(['commit', '-m', `work in ${name}`], wtDir); + if (options.merge !== false) { + git(['merge', branch, '--no-ff', '-m', `merge ${branch}`], repoDir); + } + + const adminDir = adminDirFor(repoDir, wtDir); + if (options.lock !== false) { + fs.writeFileSync(path.join(adminDir, 'locked'), LOCK_OWNER_PID); + } + return { repoDir, wtDir, branch, adminDir }; +} + +/** Deps every "owner is dead, lock is stale" test shares. */ +function deadOwnerDeps(extra = {}) { + return { isPidAlive: () => false, mtimeSafe: () => STALE_MTIME, ...extra }; +} + +/** Assert exactly one result row, and return it. */ +function onlyRow(result) { + assert.strictEqual(result.length, 1, `expected exactly one result row, got ${JSON.stringify(result)}`); + return result[0]; +} + +// ─── Suite: default-branch discovery — fail-closed verdicts ────────────────── + +describe('#3057 reapOrphanWorktrees: default-branch discovery verdicts', () => { + let tmpBase; + + beforeEach(() => { + tmpBase = fs.mkdtempSync(path.join(resolvedTmpDir(), 'gsd-3057-reap-disc-')); + }); + + afterEach(() => { + cleanup(tmpBase); + }); + + test('returns no rows and never reads the admin directory when git cannot resolve --git-dir', () => { + const f = makeFixture(tmpBase, 'nogitdir'); + const probed = []; + const faultyGit = makeFaultyGit({ + faults: [{ kind: 'exit', exitCode: 128, when: ['rev-parse', '--git-dir'] }], + passthrough: realExecGit, + }); + + const result = reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ + execGit: faultyGit, + readDirSafe: (dir) => { probed.push(dir); return fs.readdirSync(dir); }, + })); + + assert.deepStrictEqual(result, []); + assert.deepStrictEqual(probed, [], 'admin directory must not be read once --git-dir failed'); + assert.deepStrictEqual(argvOf(faultyGit), ['rev-parse --git-dir']); + assert.ok(fs.existsSync(f.wtDir), 'the worktree must survive a fail-closed bail-out'); + }); + + test('returns no rows when the worktrees admin directory cannot be listed', () => { + const f = makeFixture(tmpBase, 'nodir'); + const probed = []; + const faultyGit = makeFaultyGit({ passthrough: realExecGit }); + + const result = reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ + execGit: faultyGit, + readDirSafe: (dir) => { probed.push(dir); return null; }, + })); + + assert.deepStrictEqual(result, []); + assert.strictEqual(probed.length, 1, 'the admin directory must be probed exactly once'); + assert.strictEqual(path.basename(probed[0]), 'worktrees'); + // Distinguishes this bail-out from the --git-dir one above: --git-dir DID + // run and succeed, and nothing after the admin listing was attempted. + assert.deepStrictEqual(argvOf(faultyGit), ['rev-parse --git-dir']); + }); + + test('returns no rows for a repo that has no linked worktrees at all', () => { + // Exercises the real `defaultReadDirSafe` catch: `.git/worktrees/` does not + // exist, so readdirSync throws and the helper returns null. + const repoDir = path.join(tmpBase, 'repo-bare-of-worktrees'); + initRepo(repoDir); + + assert.deepStrictEqual(reapOrphanWorktrees(repoDir), []); + }); + + test('reaps from origin/HEAD alone and never consults local branch candidates', () => { + const f = makeFixture(tmpBase, 'remotehead'); + const mainTip = git(['rev-parse', 'main'], f.repoDir).trim(); + const faultyGit = makeFaultyGit({ + passthrough: (args, opts) => { + if (args[0] === 'symbolic-ref' && args[args.length - 1] === 'refs/remotes/origin/HEAD') { + return okResult('origin/main\n'); + } + if (args[0] === 'rev-parse' && args[1] === 'refs/remotes/origin/main') { + return okResult(`${mainTip}\n`); + } + return realExecGit(args, opts); + }, + }); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ execGit: faultyGit }))); + + assert.strictEqual(row.status, 'reaped'); + assert.strictEqual(row.reason, 'pid_dead_and_merged'); + // The remote-exclusive arm is what makes this distinguishable from the + // local-candidate arm that every other fixture in the tree takes. + assert.strictEqual(calledWith(faultyGit, ['remote']), false, 'must not fall back to remote enumeration'); + assert.strictEqual( + calledWith(faultyGit, ['config', '--get', 'init.defaultBranch']), + false, + 'must not build a local candidate list when origin/HEAD resolved' + ); + }); + + test('returns no rows when origin/HEAD names a remote ref that will not resolve', () => { + const f = makeFixture(tmpBase, 'badremoteref'); + const faultyGit = makeFaultyGit({ + faults: [{ kind: 'exit', exitCode: 128, when: ['rev-parse', 'refs/remotes/origin/main'] }], + passthrough: (args, opts) => ( + args[0] === 'symbolic-ref' && args[args.length - 1] === 'refs/remotes/origin/HEAD' + ? okResult('origin/main\n') + : realExecGit(args, opts) + ), + }); + + const result = reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ execGit: faultyGit })); + + assert.deepStrictEqual(result, []); + assert.strictEqual( + calledWith(faultyGit, ['worktree', 'list']), + false, + 'must fail closed before building the canonical index' + ); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('returns no rows when a remote exists but origin/HEAD is unset', () => { + const f = makeFixture(tmpBase, 'ambiguousremote'); + const originSrc = path.join(tmpBase, 'origin-src'); + initRepo(originSrc); + git(['remote', 'add', 'origin', originSrc], f.repoDir); + const faultyGit = makeFaultyGit({ passthrough: realExecGit }); + + const result = reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ execGit: faultyGit })); + + assert.deepStrictEqual(result, [], 'an ambiguous default branch must not be guessed'); + assert.strictEqual(calledWith(faultyGit, ['remote']), true); + assert.strictEqual( + calledWith(faultyGit, ['config', '--get', 'init.defaultBranch']), + false, + 'the candidate list must not be built once a remote is known to exist' + ); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('returns no rows when not one default-branch candidate resolves', () => { + const f = makeFixture(tmpBase, 'nocandidate'); + const faultyGit = makeFaultyGit({ + faults: [{ + kind: 'exit', + exitCode: 128, + when: (args) => args[0] === 'rev-parse' && args[1] !== '--git-dir', + }], + passthrough: realExecGit, + }); + + const result = reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ execGit: faultyGit })); + + assert.deepStrictEqual(result, []); + assert.strictEqual(calledWith(faultyGit, ['rev-parse', 'main']), true); + assert.strictEqual(calledWith(faultyGit, ['rev-parse', 'master']), true); + assert.strictEqual( + calledWith(faultyGit, ['worktree', 'list']), + false, + 'must fail closed before building the canonical index' + ); + }); +}); + +// ─── Suite: canonical-index construction ───────────────────────────────────── + +describe('#3057 reapOrphanWorktrees: canonical-index degradation verdicts', () => { + let tmpBase; + + beforeEach(() => { + tmpBase = fs.mkdtempSync(path.join(resolvedTmpDir(), 'gsd-3057-reap-idx-')); + }); + + afterEach(() => { + cleanup(tmpBase); + }); + + test('still reaps when git worktree list fails and the canonical index stays empty', () => { + const f = makeFixture(tmpBase, 'listfails'); + const wtCanonical = canonicalPath(f.wtDir); + const faultyGit = makeFaultyGit({ + faults: [{ kind: 'timeout', when: ['worktree', 'list'] }], + passthrough: realExecGit, + }); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ execGit: faultyGit }))); + + // A failed listing must degrade to the gitdir-derived path, NOT abort the + // sweep — an empty index is not "there is nothing to reap". + assert.strictEqual(row.status, 'reaped'); + assert.strictEqual(row.reason, 'pid_dead_and_merged'); + assert.strictEqual(canonicalPath(row.path), wtCanonical); + assert.strictEqual(calledWith(faultyGit, ['worktree', 'list']), true); + assert.strictEqual(fs.existsSync(f.wtDir), false); + }); + + test('still reaps when a porcelain block carries no worktree line', () => { + const f = makeFixture(tmpBase, 'headlessblock'); + const realPorcelain = git(['worktree', 'list', '--porcelain'], f.repoDir); + const faultyGit = makeFaultyGit({ + passthrough: (args, opts) => ( + args[0] === 'worktree' && args[1] === 'list' + ? okResult(`bare\n\n${realPorcelain}`) + : realExecGit(args, opts) + ), + }); + + // Without the `continue`, `wtLine.slice(...)` would throw on the leading + // block and the whole sweep would die. + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ execGit: faultyGit }))); + + assert.strictEqual(row.status, 'reaped'); + assert.strictEqual(row.reason, 'pid_dead_and_merged'); + }); + + test('still reaps when the porcelain lists a path that no longer exists on disk', () => { + const f = makeFixture(tmpBase, 'ghostpath'); + const realPorcelain = git(['worktree', 'list', '--porcelain'], f.repoDir); + const ghost = path.join(tmpBase, 'ghost-worktree'); + const faultyGit = makeFaultyGit({ + passthrough: (args, opts) => ( + args[0] === 'worktree' && args[1] === 'list' + ? okResult(`worktree ${ghost}\nHEAD 0000000000000000000000000000000000000000\n\n${realPorcelain}`) + : realExecGit(args, opts) + ), + }); + + // realpathSync.native throws for the ghost block; the catch must skip that + // one entry and keep indexing the rest. + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ execGit: faultyGit }))); + + assert.strictEqual(row.status, 'reaped'); + assert.strictEqual(row.reason, 'pid_dead_and_merged'); + }); +}); + +// ─── Suite: admin-directory shape ──────────────────────────────────────────── + +describe('#3057 reapOrphanWorktrees: admin-entry verdicts', () => { + let tmpBase; + + beforeEach(() => { + tmpBase = fs.mkdtempSync(path.join(resolvedTmpDir(), 'gsd-3057-reap-admin-')); + }); + + afterEach(() => { + cleanup(tmpBase); + }); + + test('reports no row at all for a linked worktree that carries no lock file', () => { + const f = makeFixture(tmpBase, 'locked'); + const unlockedDir = path.join(tmpBase, 'wt-unlocked'); + git(['worktree', 'add', unlockedDir, '-b', 'worktree-agent-unlocked'], f.repoDir); + + const result = reapOrphanWorktrees(f.repoDir, deadOwnerDeps()); + + const row = onlyRow(result); + assert.strictEqual(canonicalPath(row.path), canonicalPath(f.wtDir)); + assert.strictEqual(row.status, 'reaped'); + assert.ok(fs.existsSync(unlockedDir), 'an unlocked worktree is not the reaper concern'); + }); + + test('reports no row for a locked admin entry whose gitdir pointer is missing', () => { + const f = makeFixture(tmpBase, 'nopointer'); + fs.unlinkSync(path.join(f.adminDir, 'gitdir')); + + // The lock file is present and stale and the owner is dead, so a row WOULD + // be emitted if the missing pointer were not a hard skip. + assert.deepStrictEqual(reapOrphanWorktrees(f.repoDir, deadOwnerDeps()), []); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('reports no row when an injected readFileSafe reports the gitdir pointer as empty', () => { + const f = makeFixture(tmpBase, 'blankpointer'); + const gitdirFile = path.join(f.adminDir, 'gitdir'); + + // Covers the `deps.readFileSafe` seam arm AND the empty-string half of the + // falsy-pointer guard (the missing-file half returns null, not ''). + const result = reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ + readFileSafe: (file) => { + if (path.resolve(file) === path.resolve(gitdirFile)) return ''; + try { return fs.readFileSync(file, 'utf8'); } catch { return null; } + }, + })); + + assert.deepStrictEqual(result, []); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('reports lock_too_fresh when the real mtime helper cannot stat the lock file', () => { + const f = makeFixture(tmpBase, 'statfails'); + + // No `mtimeSafe` injection: this drives the module's own default helper and + // pins its catch arm. nowMs is the far future, so a readable mtime would + // read as stale and reap. + const result = withFaultyFs( + { statSync: () => { throw Object.assign(new Error('EIO'), { code: 'EIO' }); } }, + () => reapOrphanWorktrees(f.repoDir, { isPidAlive: () => false, nowMs: 8640000000000000 }) + ); + + const row = onlyRow(result); + assert.strictEqual(row.status, 'skipped'); + assert.strictEqual(row.reason, 'lock_too_fresh'); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('reports remove_failed against the raw gitdir pointer when its basename is not .git', () => { + const f = makeFixture(tmpBase, 'oddpointer'); + const pointerTarget = path.join(f.wtDir, 'notgit'); + fs.writeFileSync(path.join(f.adminDir, 'gitdir'), `${pointerTarget}\n`); + const faultyGit = makeFaultyGit({ + faults: [{ kind: 'exit', exitCode: 1, when: ['worktree', 'remove'] }], + passthrough: realExecGit, + }); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ execGit: faultyGit }))); + + // `path` is the load-bearing assertion: a pointer that does not end in + // `/.git` is used verbatim (no dirname()), and because it does not exist, + // the canonical lookup throws and the raw path is what reaches git. + assert.strictEqual(row.path, pointerTarget); + assert.strictEqual(row.status, 'skipped'); + assert.strictEqual(row.reason, 'remove_failed'); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('reports lock_too_fresh when the lock file cannot be stat-ed', () => { + const f = makeFixture(tmpBase, 'nomtime'); + + // nowMs is the far future, so a REAL mtime would read as stale and the + // entry would be reaped. Only the null-mtime arm can produce this verdict. + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { + isPidAlive: () => false, + mtimeSafe: () => null, + nowMs: 8640000000000000, + })); + + assert.strictEqual(row.status, 'skipped'); + assert.strictEqual(row.reason, 'lock_too_fresh'); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('reports lock_too_fresh for a zero-age lock under the default guard', () => { + const f = makeFixture(tmpBase, 'defaultguard'); + const now = 1000000; + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { + isPidAlive: () => false, + mtimeSafe: () => new Date(now), + nowMs: now, + })); + + assert.strictEqual(row.status, 'skipped'); + assert.strictEqual(row.reason, 'lock_too_fresh'); + }); + + test('reaps the same zero-age lock when an injected reapMtimeGuardMs of 0 retires the guard', () => { + const f = makeFixture(tmpBase, 'zeroguard'); + const now = 1000000; + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { + isPidAlive: () => false, + mtimeSafe: () => new Date(now), + nowMs: now, + reapMtimeGuardMs: 0, + })); + + assert.strictEqual(row.status, 'reaped'); + assert.strictEqual(row.reason, 'pid_dead_and_merged'); + }); +}); + +// ─── Suite: liveness and ancestry verdicts ─────────────────────────────────── + +describe('#3057 reapOrphanWorktrees: liveness and ancestry verdicts', () => { + let tmpBase; + + beforeEach(() => { + tmpBase = fs.mkdtempSync(path.join(resolvedTmpDir(), 'gsd-3057-reap-live-')); + }); + + afterEach(() => { + cleanup(tmpBase); + }); + + test('reports pid_alive when the lock owner is alive', () => { + const f = makeFixture(tmpBase, 'alive'); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { + isPidAlive: () => true, + mtimeSafe: () => STALE_MTIME, + })); + + assert.strictEqual(row.status, 'skipped'); + assert.strictEqual(row.reason, 'pid_alive'); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('reports pid_alive when the liveness probe throws', () => { + const f = makeFixture(tmpBase, 'probethrows'); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { + isPidAlive: () => { throw Object.assign(new Error('EPERM'), { code: 'EPERM' }); }, + mtimeSafe: () => STALE_MTIME, + })); + + // An undeterminable owner is treated as alive — same verdict as a genuinely + // live owner, which is the intended fail-closed conflation. + assert.strictEqual(row.status, 'skipped'); + assert.strictEqual(row.reason, 'pid_alive'); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('reports pid_alive from the default isPidAlive helper when process.kill throws EPERM', (t) => { + // No `isPidAlive` injection: this drives the module's OWN default helper + // (`defaultIsPidAlive`), whose EPERM arm every other test in this tree + // bypasses by injecting `isPidAlive` directly. `process.kill` is + // monkeypatched per CONTRIBUTING's cross-platform IO-fault-injection rule + // rather than run against a real cross-user PID. + const f = makeFixture(tmpBase, 'defaultkill-eperm'); + const originalKill = process.kill; + t.after(() => { process.kill = originalKill; }); + process.kill = () => { throw Object.assign(new Error('EPERM'), { code: 'EPERM' }); }; + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { mtimeSafe: () => STALE_MTIME })); + + assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'skipped', reason: 'pid_alive' }); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('reports pid_dead_and_merged from the default isPidAlive helper when process.kill throws ESRCH', (t) => { + // Same default helper as above, but its dead-owner arm: ESRCH means "no + // such process", so `defaultIsPidAlive` returns false and the sweep falls + // through to the (merged, by fixture default) ancestry check and reaps. + const f = makeFixture(tmpBase, 'defaultkill-esrch'); + const originalKill = process.kill; + t.after(() => { process.kill = originalKill; }); + process.kill = () => { throw Object.assign(new Error('ESRCH'), { code: 'ESRCH' }); }; + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { mtimeSafe: () => STALE_MTIME })); + + assert.deepStrictEqual( + { status: row.status, reason: row.reason }, + { status: 'reaped', reason: 'pid_dead_and_merged' } + ); + assert.strictEqual(fs.existsSync(f.wtDir), false); + }); + + test('reports pid_alive from the default isPidAlive helper when process.kill returns without throwing', (t) => { + // The non-throwing arm of `defaultIsPidAlive`: a live owner's `kill(pid, + // 0)` returns normally, so the helper returns true directly, with no + // catch block involved at all. + const f = makeFixture(tmpBase, 'defaultkill-alive'); + const originalKill = process.kill; + t.after(() => { process.kill = originalKill; }); + process.kill = () => true; + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { mtimeSafe: () => STALE_MTIME })); + + assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'skipped', reason: 'pid_alive' }); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('reports cannot_resolve_branch_tip when the admin HEAD file is absent', () => { + const f = makeFixture(tmpBase, 'noheadfile'); + fs.unlinkSync(path.join(f.adminDir, 'HEAD')); + const faultyGit = makeFaultyGit({ passthrough: realExecGit }); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ execGit: faultyGit }))); + + assert.strictEqual(row.status, 'skipped'); + assert.strictEqual(row.reason, 'cannot_resolve_branch_tip'); + assert.strictEqual( + calledWith(faultyGit, ['merge-base']), + false, + 'ancestry must not be probed once the tip is unknown' + ); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('reports cannot_resolve_branch_tip when the admin HEAD names an unresolvable branch', () => { + const f = makeFixture(tmpBase, 'deadsymref'); + fs.writeFileSync(path.join(f.adminDir, 'HEAD'), 'ref: refs/heads/does-not-exist\n'); + const faultyGit = makeFaultyGit({ passthrough: realExecGit }); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ execGit: faultyGit }))); + + assert.strictEqual(row.status, 'skipped'); + assert.strictEqual(row.reason, 'cannot_resolve_branch_tip'); + // Distinguishes the symbolic-ref arm from the missing-file and + // unrecognised-content arms, which all share this one reason string. + assert.strictEqual(calledWith(faultyGit, ['rev-parse', 'refs/heads/does-not-exist']), true); + }); + + test('reports cannot_resolve_branch_tip for an admin HEAD that is neither a symref nor a sha', () => { + const f = makeFixture(tmpBase, 'garbagehead'); + const headFile = path.join(f.adminDir, 'HEAD'); + fs.writeFileSync(headFile, 'not-a-ref\n'); + const faultyGit = makeFaultyGit({ passthrough: realExecGit }); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ execGit: faultyGit }))); + + assert.strictEqual(row.status, 'skipped'); + assert.strictEqual(row.reason, 'cannot_resolve_branch_tip'); + assert.ok(fs.existsSync(headFile), 'the HEAD file is present — this is not the missing-file arm'); + assert.strictEqual( + faultyGit.calls.some((c) => c.args[0] === 'rev-parse' && String(c.args[1]).startsWith('refs/heads/')), + false, + 'unrecognised HEAD content must not be handed to rev-parse' + ); + }); + + test('reaps a detached admin HEAD without resolving any branch ref', () => { + const f = makeFixture(tmpBase, 'detached'); + const branchTip = git(['rev-parse', f.branch], f.repoDir).trim(); + fs.writeFileSync(path.join(f.adminDir, 'HEAD'), `${branchTip}\n`); + const faultyGit = makeFaultyGit({ passthrough: realExecGit }); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ execGit: faultyGit }))); + + assert.strictEqual(row.status, 'reaped'); + assert.strictEqual(row.reason, 'pid_dead_and_merged'); + assert.strictEqual( + faultyGit.calls.some((c) => c.args[0] === 'rev-parse' && String(c.args[1]).startsWith('refs/heads/')), + false, + 'a bare 40-hex HEAD is the tip; no ref resolution is needed' + ); + assert.strictEqual(fs.existsSync(f.wtDir), false); + }); + + test('reports branch_not_merged for an unmerged branch whose lock owner is dead', () => { + const f = makeFixture(tmpBase, 'unmerged', { merge: false }); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps())); + + assert.strictEqual(row.status, 'skipped'); + assert.strictEqual(row.reason, 'branch_not_merged'); + assert.ok(fs.existsSync(f.wtDir), 'unmerged work must survive the sweep'); + }); + + test('reports remove_failed and leaves the worktree on disk when git worktree remove fails', () => { + const f = makeFixture(tmpBase, 'removefails'); + const faultyGit = makeFaultyGit({ + faults: [{ kind: 'exit', exitCode: 1, when: ['worktree', 'remove'] }], + passthrough: realExecGit, + }); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ execGit: faultyGit }))); + + assert.strictEqual(canonicalPath(row.path), canonicalPath(f.wtDir)); + assert.strictEqual(row.status, 'skipped'); + assert.strictEqual(row.reason, 'remove_failed'); + assert.ok(fs.existsSync(f.wtDir)); + assert.strictEqual(calledWith(faultyGit, ['worktree', 'unlock']), true, 'unlock precedes remove'); + }); +}); + +// ─── Suite: CLI wrappers ───────────────────────────────────────────────────── + +describe('#3057 cmdWorktreeReapOrphans / pruneOrphanedWorktrees output verdicts', () => { + let tmpBase; + + beforeEach(() => { + tmpBase = fs.mkdtempSync(path.join(resolvedTmpDir(), 'gsd-3057-reap-cli-')); + }); + + afterEach(() => { + cleanup(tmpBase); + }); + + test('cmdWorktreeReapOrphans reports ok with zero entries after the reaper throws', () => { + const out = []; + const err = []; + + cmdWorktreeReapOrphans(tmpBase, { + write: (s) => out.push(s), + writeErr: (s) => err.push(s), + execGit: () => { throw new Error('boom'); }, + }); + + assert.deepStrictEqual(err, ['[gsd] worktree.reap-orphans failed: boom\n']); + assert.deepStrictEqual(JSON.parse(out.join('')), { ok: true, reaped: 0, entries: [] }); + }); + + test('cmdWorktreeReapOrphans warns with the skipped count and emits the skipped row as JSON', () => { + const f = makeFixture(tmpBase, 'cliskip', { merge: false }); + const out = []; + const err = []; + + cmdWorktreeReapOrphans(f.repoDir, { + write: (s) => out.push(s), + writeErr: (s) => err.push(s), + ...deadOwnerDeps(), + }); + + assert.deepStrictEqual(err, [ + '[gsd] worktree.reap-orphans: 1 orphan(s) skipped (run with DEBUG=1 for details)\n', + ]); + const payload = JSON.parse(out.join('')); + assert.strictEqual(payload.ok, true); + assert.strictEqual(payload.reaped, 0); + assert.strictEqual(payload.entries.length, 1); + assert.strictEqual(payload.entries[0].status, 'skipped'); + assert.strictEqual(payload.entries[0].reason, 'branch_not_merged'); + }); + + test('cmdWorktreeReapOrphans stays silent on stderr when nothing is skipped', () => { + const f = makeFixture(tmpBase, 'cliclean'); + const out = []; + const err = []; + + cmdWorktreeReapOrphans(f.repoDir, { + write: (s) => out.push(s), + writeErr: (s) => err.push(s), + ...deadOwnerDeps(), + }); + + assert.deepStrictEqual(err, []); + const payload = JSON.parse(out.join('')); + assert.strictEqual(payload.reaped, 1); + assert.strictEqual(payload.entries[0].reason, 'pid_dead_and_merged'); + }); + + test('pruneOrphanedWorktrees warns that the health check degraded when git worktree prune times out', () => { + const f = makeFixture(tmpBase, 'prunetimeout'); + const err = []; + const faultyGit = makeFaultyGit({ + faults: [{ kind: 'timeout', when: ['worktree', 'prune'] }], + passthrough: realExecGit, + }); + + const removed = pruneOrphanedWorktrees(f.repoDir, { + execGit: faultyGit, + writeErr: (s) => err.push(s), + }); + + assert.deepStrictEqual(removed, []); + assert.deepStrictEqual(err, [ + '[gsd-tools] WARNING: worktree health check degraded' + + ' — git worktree prune timed out after 10s.' + + ' Orphaned worktree metadata may remain until the next successful run.\n', + ]); + }); + + test('pruneOrphanedWorktrees returns an empty list and warns nothing when git throws', () => { + const f = makeFixture(tmpBase, 'prunethrows'); + const err = []; + + const removed = pruneOrphanedWorktrees(f.repoDir, { + execGit: () => { throw new Error('boom'); }, + writeErr: (s) => err.push(s), + }); + + assert.deepStrictEqual(removed, [], 'a throwing git must never crash the caller'); + assert.deepStrictEqual(err, [], 'the degraded-health warning belongs to the timeout arm only'); + }); +}); diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index 917bf8774..29cbae989 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -3582,10 +3582,7 @@ describe('worktree-safety: pruneOrphanedWorktrees behaviour', () => { test('pruneOrphanedWorktrees(temp dir) returns [] and does not throw', (t) => { const dir = createTempDir('gsd-prune-'); t.after(() => cleanup(dir)); - let result; - assert.doesNotThrow(() => { - result = worktreeSafety.pruneOrphanedWorktrees(dir); - }); + const result = worktreeSafety.pruneOrphanedWorktrees(dir); assert.deepStrictEqual(result, []); }); }); @@ -3912,10 +3909,14 @@ describe('bug-3707: reapOrphanWorktrees', () => { // ensuring the live-PID check is the only reason the entry is skipped. const result = reapOrphanWorktrees(repoDir, { mtimeSafe: () => STALE_MTIME }); + // #3057: assert the SPECIFIC verdict unconditionally. The previous form + // guarded on `if (skipped)`, so it passed vacuously whenever the entry was + // absent from the results entirely — the exact failure this test exists to + // catch. const skipped = result.find((r) => canonicalPath(r.path) === canonicalPath(wtDir)); - if (skipped) { - assert.notEqual(skipped.status, 'reaped', 'live-pid worktree must not be reaped'); - } + assert.ok(skipped, 'live-pid worktree must appear in the results'); + assert.equal(skipped.status, 'skipped', 'live-pid worktree must not be reaped'); + assert.equal(skipped.reason, 'pid_alive', 'reason must be pid_alive'); assert.ok(fs.existsSync(wtDir), 'worktree directory must still exist for live-pid worktree'); }); @@ -3938,10 +3939,12 @@ describe('bug-3707: reapOrphanWorktrees', () => { // ensuring the unmerged-branch check is the only reason the entry is skipped. const result = reapOrphanWorktrees(repoDir, { mtimeSafe: () => STALE_MTIME }); + // #3057: unconditional verdict assertion — the old `if (entry)` form passed + // vacuously when no row was produced at all. const entry = result.find((r) => canonicalPath(r.path) === canonicalPath(wtDir)); - if (entry) { - assert.notEqual(entry.status, 'reaped', 'unmerged worktree must not be reaped (data loss guard)'); - } + assert.ok(entry, 'unmerged worktree must appear in the results'); + assert.equal(entry.status, 'skipped', 'unmerged worktree must not be reaped (data loss guard)'); + assert.equal(entry.reason, 'branch_not_merged', 'reason must be branch_not_merged'); assert.ok(fs.existsSync(wtDir), 'unmerged worktree directory must still exist'); }); @@ -3967,10 +3970,12 @@ describe('bug-3707: reapOrphanWorktrees', () => { // within 5 minutes) which is fragile on heavily-loaded CI hosts. const result = reapOrphanWorktrees(repoDir, { mtimeSafe: () => FRESH_MTIME }); + // #3057: unconditional verdict assertion — the old `if (entry)` form passed + // vacuously when no row was produced at all. const entry = result.find((r) => canonicalPath(r.path) === canonicalPath(wtDir)); - if (entry) { - assert.notEqual(entry.status, 'reaped', 'fresh-mtime worktree must not be reaped (race guard)'); - } + assert.ok(entry, 'fresh-mtime worktree must appear in the results'); + assert.equal(entry.status, 'skipped', 'fresh-mtime worktree must not be reaped (race guard)'); + assert.equal(entry.reason, 'lock_too_fresh', 'reason must be lock_too_fresh'); assert.ok(fs.existsSync(wtDir), 'fresh-lock worktree directory must still exist'); }); @@ -4189,16 +4194,12 @@ describe('bug-3707: reapOrphanWorktrees — adversarial edge cases', () => { // ensuring the non-numeric content check is the only reason the entry is skipped. const result = reapOrphanWorktrees(repoDir, { mtimeSafe: () => STALE_MTIME }); + // #3057: unconditional verdict assertion — the old `if (entry)` form passed + // vacuously when no row was produced at all. const entry = result.find((r) => canonicalPath(r.path) === canonicalPath(wtDir)); - if (entry) { - assert.notEqual( - entry.status, - 'reaped', - 'non-numeric Claude Code lock must NOT be reaped (fail-closed: owner unknown)' - ); - assert.equal(entry.status, 'skipped', 'non-numeric lock entry should have status=skipped'); - assert.equal(entry.reason, 'lock_owner_unknown', 'reason must be lock_owner_unknown'); - } + assert.ok(entry, 'Claude-Code-locked worktree must appear in the results'); + assert.equal(entry.status, 'skipped', 'non-numeric lock entry should have status=skipped'); + assert.equal(entry.reason, 'lock_owner_unknown', 'reason must be lock_owner_unknown'); assert.ok(fs.existsSync(wtDir), 'worktree with Claude Code lock must NOT be removed'); }); @@ -4232,10 +4233,13 @@ describe('bug-3707: reapOrphanWorktrees — adversarial edge cases', () => { mtimeSafe: () => STALE_MTIME, }); + // #3057: unconditional verdict assertion. An undeterminable owner takes the + // same fail-closed exit as a genuinely live one, so the reason string is + // pid_alive in both cases — production deliberately conflates them. const entry = result.find((r) => canonicalPath(r.path) === canonicalPath(wtDir)); - if (entry) { - assert.notEqual(entry.status, 'reaped', 'EPERM from isPidAlive must be treated as ALIVE — must not reap'); - } + assert.ok(entry, 'EPERM worktree must appear in the results'); + assert.equal(entry.status, 'skipped', 'EPERM from isPidAlive must be treated as ALIVE — must not reap'); + assert.equal(entry.reason, 'pid_alive', 'reason must be pid_alive'); assert.ok(fs.existsSync(wtDir), 'worktree must still exist when isPidAlive throws EPERM'); }); @@ -4282,11 +4286,12 @@ describe('bug-3707: reapOrphanWorktrees — adversarial edge cases', () => { // OR skip it for a safe reason — it must NOT return an empty result (which // would mean it bailed out entirely, silently skipping all orphan detection). assert.ok(Array.isArray(result), 'reapOrphanWorktrees must return an array'); - assert.ok(result.length > 0, 'reaper must not bail out entirely for trunk-default repos — must inspect the worktree'); + assert.equal(result.length, 1, 'reaper must inspect exactly the one worktree in this trunk-default repo — not bail out entirely, and not report extras'); const entry = result.find((r) => canonicalPath(r.path) === wtDirCanonical); assert.ok(entry, 'worktree must appear in results (reaped or skipped with reason)'); // The branch IS merged into trunk, and the PID is dead, so it should be reaped. assert.equal(entry.status, 'reaped', 'worktree with dead pid merged into trunk must be reaped'); + assert.equal(entry.reason, 'pid_dead_and_merged', 'reason must be pid_dead_and_merged (using trunk as the default branch)'); }); }); }); @@ -4755,8 +4760,7 @@ describe('bug #260: gsd-worktree-path-guard.js', () => { }; const result = runHook(worktreeDir, payload); assert.strictEqual(result.status, 2, `Expected exit 2 (block), got ${result.status}. stderr: ${result.stderr}`); - let parsed; - assert.doesNotThrow(() => { parsed = JSON.parse(result.stdout); }, 'stdout must be valid JSON'); + const parsed = JSON.parse(result.stdout); assert.strictEqual(parsed.decision, 'block', 'Expected decision:"block" in output'); }); @@ -5341,8 +5345,7 @@ describe('#1342 — GSD-activity gate + fail-open for no-repo targets', () => { `GSD-managed worktree targeting main repo root must be blocked (exit 2). ` + `Got exit ${result.status}. stderr: ${result.stderr}` ); - let parsed; - assert.doesNotThrow(() => { parsed = JSON.parse(result.stdout); }, 'stdout must be valid JSON'); + const parsed = JSON.parse(result.stdout); assert.strictEqual(parsed.decision, 'block', 'Expected decision:"block" in output'); }); @@ -5407,8 +5410,7 @@ describe('#1342 — GSD-activity gate + fail-open for no-repo targets', () => { `GSD-managed worktree targeting .git/config of another repo must be blocked (exit 2). ` + `Got exit ${result.status}. stderr: ${result.stderr}` ); - let parsed; - assert.doesNotThrow(() => { parsed = JSON.parse(result.stdout); }, 'stdout must be valid JSON'); + const parsed = JSON.parse(result.stdout); assert.strictEqual(parsed.decision, 'block', 'Expected decision:"block" in output'); assert.ok( parsed.reason && parsed.reason.includes('.git'), From 573d39ea602fb912f44b544198c94a4ebbd8ab92 Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 6 Aug 2026 01:08:15 -0400 Subject: [PATCH 4/7] fix(#3103): refuse to reap on a PID the parse could not represent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A lock file whose PID is a digit string longer than 308 characters parses to Infinity, not NaN. The default liveness helper then calls process.kill with it, which throws a TypeError rather than an errno error, and that helper's catch only recognises EPERM — so it returns false, meaning "the owner is dead", and the worktree becomes eligible to be removed. The reaper already fails closed for this exact situation. It wraps the liveness call in a catch that sets alive and does not reap, with a comment saying liveness could not be determined. That protection never fires here, because the inner catch swallowed the error first and answered confidently instead of admitting it did not know. A guard that cannot verify safety reporting success is the defect this whole epic is named for, and it was sitting inside the one function in the tree that deletes things. The guard deleted earlier on this branch tested for NaN. That test really was dead — a digits-only capture cannot parseInt to NaN — but the reachable failure is non-finite, so removing it without correcting the predicate left the hole open. The check is now for a finite value, and a malformed PID reports the same lock_owner_unknown skip as an unparseable one, since both mean the same thing: the owner is unknown, so nothing is removed. Measured, not assumed: 308 nines still parse finite, 309 are Infinity. All three of that boundary are covered, along with a 400-digit case that asserts the worktree is still on disk afterwards — the consequence, not just the verdict. Against a build with the guard removed, that case reports pid_dead_and_merged and the worktree is gone. The liveness helper's own catch still maps every non-EPERM error to "dead". The fix belongs where the value stops being trustworthy rather than at the far end of it, but that helper is worth revisiting on its own terms. Refs #3057 Co-Authored-By: Claude Opus 5 --- src/worktree-safety.cts | 11 ++++++- tests/worktree-safety-reap.test.cjs | 51 +++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/src/worktree-safety.cts b/src/worktree-safety.cts index 8f12e3d0f..cf5fae60d 100644 --- a/src/worktree-safety.cts +++ b/src/worktree-safety.cts @@ -1737,9 +1737,18 @@ function reapOrphanWorktrees(repoRoot: string, deps: WorktreeDeps = {}): ReapRes continue; } const pid = parseInt(pidStr, 10); + // Number.isFinite, not Number.isNaN: pidStr is captured by /^\d+/ above, so + // pid can never be NaN. But a >309-digit string parses to Infinity, and + // process.kill(Infinity, 0) throws a TypeError that defaultIsPidAlive's + // errno-only catch maps to "dead" (no `.code` on a TypeError) — which + // would reap a worktree whose owner may well be alive. Fail closed here, + // before that value is ever trusted. + if (!Number.isFinite(pid)) { + results.push({ path: worktreePath, status: 'skipped', reason: 'lock_owner_unknown' }); + continue; + } let pidIsAlive: boolean; try { - // No `Number.isNaN(pid) ||` guard: pidStr is captured by /^\d+/ above, so pid is never NaN. pidIsAlive = isPidAliveCheck(pid); } catch { pidIsAlive = true; // Cannot determine liveness — treat as alive, do not reap. diff --git a/tests/worktree-safety-reap.test.cjs b/tests/worktree-safety-reap.test.cjs index d390136c6..eeed3a899 100644 --- a/tests/worktree-safety-reap.test.cjs +++ b/tests/worktree-safety-reap.test.cjs @@ -702,6 +702,57 @@ describe('#3057 reapOrphanWorktrees: liveness and ancestry verdicts', () => { assert.ok(fs.existsSync(f.wtDir), 'unmerged work must survive the sweep'); }); + test('reports lock_owner_unknown and leaves the worktree on disk for a 400-digit lock PID', () => { + const f = makeFixture(tmpBase, 'giantpid', { lock: false }); + fs.writeFileSync(path.join(f.adminDir, 'locked'), '9'.repeat(400)); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps())); + + assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'skipped', reason: 'lock_owner_unknown' }); + assert.ok(fs.existsSync(f.wtDir), 'a lock PID that overflows to Infinity must never be reaped'); + }); + + // Parse-cliff boundary, measured empirically: `parseInt('9'.repeat(N), 10)` + // is finite through N=308 and becomes Infinity at N=309 (probed via + // `node -e`, reported in the task return). + test('reaches the liveness check for a 308-digit lock PID (last finite length)', () => { + const f = makeFixture(tmpBase, 'cliffminus1', { lock: false }); + fs.writeFileSync(path.join(f.adminDir, 'locked'), '9'.repeat(308)); + let seenPid; + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ + isPidAlive: (pid) => { seenPid = pid; return false; }, + }))); + + assert.strictEqual(seenPid, Number('9'.repeat(308)), 'a finite 308-digit PID must reach isPidAlive unchanged'); + assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'reaped', reason: 'pid_dead_and_merged' }); + assert.strictEqual(fs.existsSync(f.wtDir), false); + }); + + test('reports lock_owner_unknown for a 309-digit lock PID (first Infinity length)', () => { + const f = makeFixture(tmpBase, 'cliffexact', { lock: false }); + fs.writeFileSync(path.join(f.adminDir, 'locked'), '9'.repeat(309)); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps())); + + assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'skipped', reason: 'lock_owner_unknown' }); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('reaches the liveness check for an ordinary small lock PID', () => { + const f = makeFixture(tmpBase, 'ordinarypid', { lock: false }); + fs.writeFileSync(path.join(f.adminDir, 'locked'), '4242'); + let seenPid; + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, deadOwnerDeps({ + isPidAlive: (pid) => { seenPid = pid; return false; }, + }))); + + assert.strictEqual(seenPid, 4242, 'an ordinary PID must reach isPidAlive unchanged'); + assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'reaped', reason: 'pid_dead_and_merged' }); + assert.strictEqual(fs.existsSync(f.wtDir), false); + }); + test('reports remove_failed and leaves the worktree on disk when git worktree remove fails', () => { const f = makeFixture(tmpBase, 'removefails'); const faultyGit = makeFaultyGit({ From 1e844b942ce9591cbab9446b1a9fe1f390e55b40 Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 6 Aug 2026 01:26:15 -0400 Subject: [PATCH 5/7] fix(#3103): only "no such process" means the owner is dead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the previous commit fixed the wrong cliff. process.kill accepts a pid up to 2147483647 and throws a TypeError above it, so 2147483648 — an ordinary finite number — sailed past the finite check, threw, and was classified dead. Measured here: 2147483646 and 2147483647 raise ESRCH, 2147483648 and above raise ERR_INVALID_ARG_TYPE. The digit-length boundary the last commit pinned was a different, earlier gate, and its tests implied it was the meaningful one. The liveness helper now treats only ESRCH as dead. EPERM, a type error from an out-of-range pid, an error with no code at all — every outcome it does not recognise returns alive, because the value feeds a forced worktree removal and an unrecognised failure must never read as permission to delete. Against a build with the old catch, a lock holding 2147483648 reaps the worktree; with this one it is skipped and the directory survives. The finite check stays. It no longer carries the safety, but it still names garbage input accurately rather than reporting it as a live owner, and its comment now says that so it is not removed as redundant. The freshness verdict is split. An unreadable lock mtime and a genuinely recent lock both reported lock_too_fresh, which tells an operator to wait when waiting cannot help — the same conflation this module already separated for parse failures. The unreadable case is now lock_age_unknown. Only this module and its tests read these strings; no workflow or command consumes them. The dependency spread in the prune path is kept as it is, with a test that kills it. A caller-supplied porcelain parser displacing the default is the intended seam, not an accident: the parser is a declared member of the dependency type and the planner already prefers an injected one, so the hard-coded key restates the same default rather than guarding against override. Previously nothing exercised that line at all. Refs #3057 Co-Authored-By: Claude Opus 5 --- src/worktree-safety.cts | 57 +++++++++-- tests/git-base-branch.test.cjs | 53 ++++------ tests/worktree-safety-reap.test.cjs | 150 +++++++++++++++++++++++++--- 3 files changed, 206 insertions(+), 54 deletions(-) diff --git a/src/worktree-safety.cts b/src/worktree-safety.cts index cf5fae60d..2b48498d5 100644 --- a/src/worktree-safety.cts +++ b/src/worktree-safety.cts @@ -1724,8 +1724,21 @@ function reapOrphanWorktrees(repoRoot: string, deps: WorktreeDeps = {}): ReapRes } // 4a. Stale-lock guard: skip if lock is too fresh (PID recycling / race). + // + // The two causes are reported SEPARATELY (#3057). A lock whose mtime could + // not be read is not "fresh" in any sense: `lock_too_fresh` tells an + // operator that waiting will resolve the skip, and waiting never resolves a + // stat failure — the lock could be seconds or months old and the sweep has + // no way to tell. Conflating them is the same defect this module already + // fixed for `parse_failed` vs `no_worktrees` in planWorktreePrune: a + // decision made on unread data must not be indistinguishable from one made + // on real data. const lockMtime = mtimeSafe(lockedFile); - if (!lockMtime || nowMs - lockMtime.getTime() < reapMtimeGuardMs) { + if (!lockMtime) { + results.push({ path: worktreePath, status: 'skipped', reason: 'lock_age_unknown' }); + continue; + } + if (nowMs - lockMtime.getTime() < reapMtimeGuardMs) { results.push({ path: worktreePath, status: 'skipped', reason: 'lock_too_fresh' }); continue; } @@ -1738,11 +1751,18 @@ function reapOrphanWorktrees(repoRoot: string, deps: WorktreeDeps = {}): ReapRes } const pid = parseInt(pidStr, 10); // Number.isFinite, not Number.isNaN: pidStr is captured by /^\d+/ above, so - // pid can never be NaN. But a >309-digit string parses to Infinity, and - // process.kill(Infinity, 0) throws a TypeError that defaultIsPidAlive's - // errno-only catch maps to "dead" (no `.code` on a TypeError) — which - // would reap a worktree whose owner may well be alive. Fail closed here, - // before that value is ever trusted. + // pid can never be NaN. A 309-or-more-digit string parses to Infinity. + // + // NOT LOAD-BEARING FOR SAFETY — do not delete it as redundant. Fail-closed + // liveness now lives in defaultIsPidAlive, which treats every non-ESRCH + // outcome (including the TypeError process.kill throws for Infinity) as + // ALIVE. This guard survives because it produces a more ACCURATE verdict + // for garbage input: `lock_owner_unknown` says "the lock names a PID this + // parse could not represent", whereas falling through would report + // `pid_alive` — an assertion about an owner that was never probed. + // Note this is an EARLIER, DIFFERENT gate than the process.kill range + // limit: process.kill accepts up to 2147483647 and rejects 2147483648 + // (measured), far below the parse cliff this guard catches. if (!Number.isFinite(pid)) { results.push({ path: worktreePath, status: 'skipped', reason: 'lock_owner_unknown' }); continue; @@ -1813,13 +1833,28 @@ function reapOrphanWorktrees(repoRoot: string, deps: WorktreeDeps = {}): ReapRes // ─── reapOrphanWorktrees deps helpers ───────────────────────────────────────── +/** + * Liveness probe for a lock-owner PID — FAILS CLOSED (#3057). + * + * `ESRCH` ("no such process") is the ONLY outcome that proves the owner is + * gone. Every other failure means the probe could not determine liveness: + * - `EPERM` — the process exists, we just may not signal it; + * - `TypeError` / `ERR_INVALID_ARG_TYPE` — `process.kill` accepts a pid up + * to 2147483647 and REJECTS 2147483648 and above (measured), so a finite + * but out-of-range pid never reaches the OS at all; + * - anything else — an outcome this helper does not recognise. + * + * The return value feeds a DESTRUCTIVE decision (`git worktree remove + * --force`), so an unrecognised failure must never read as "dead". Hence the + * inversion: only ESRCH returns false; everything else returns true (alive, + * do not reap). + */ function defaultIsPidAlive(pid: number): boolean { try { process.kill(pid, 0); return true; } catch (err) { - if (err && (err as NodeJS.ErrnoException).code === 'EPERM') return true; - return false; + return (err as NodeJS.ErrnoException | null | undefined)?.code !== 'ESRCH'; } } @@ -1890,6 +1925,12 @@ function resolveWorktreeRoot(cwd: string, deps: WorktreeDeps = {}): { root: stri function pruneOrphanedWorktrees(repoRoot: string, deps: WorktreeDeps & { writeErr?: (s: string) => void } = {}): string[] { const writeErr = deps.writeErr || ((s: string) => process.stderr.write(s)); try { + // `...deps` comes LAST deliberately: `parseWorktreePorcelain` is a declared + // member of WorktreeDeps and planWorktreePrune already reads + // `deps.parseWorktreePorcelain` before falling back to the module function, + // so a caller-supplied parser is an intended override, not an accident. + // The hard-coded key is only a restatement of that same default. Do not + // reorder the two — `tests/worktree-safety-reap.test.cjs` pins the override. const plan = planWorktreePrune( repoRoot, { allowDestructive: false }, diff --git a/tests/git-base-branch.test.cjs b/tests/git-base-branch.test.cjs index 7a0936407..1fd9f7978 100644 --- a/tests/git-base-branch.test.cjs +++ b/tests/git-base-branch.test.cjs @@ -446,6 +446,17 @@ describe('#3057 W3: readConfigBaseBranch — config present but unusable', () => }); test('config.json parses to a non-object → null for null / string / number / array', () => { + // NOTE on the `[]` case: this documents observed behaviour only. It does + // NOT pin the `Array.isArray(cfg)` guard in readConfigBaseBranch — that + // guard is unreachable (and therefore unkillable) through this readFile + // entry point. `cfg` is always the result of `JSON.parse(raw)` on a + // string, and a JSON array can never carry a `.git` or `.base_branch` + // own-property the way a hand-built JS array could; with the guard + // deleted entirely, `top.git`/`top.base_branch` on an array are still + // `undefined`, so the result is `null` either way. Verified by mutation: + // deleting `|| Array.isArray(cfg)` from the built lib does not change any + // output for any JSON-string input. The guard is real defense-in-depth + // for a future non-JSON-string caller, not something this suite can pin. assert.strictEqual(readWith('null'), null, 'JSON null must not be treated as a config'); assert.strictEqual(readWith('"master"'), null, 'a bare JSON string must not be treated as a config'); assert.strictEqual(readWith('42'), null, 'a bare JSON number must not be treated as a config'); @@ -461,7 +472,14 @@ describe('#3057 W3: readConfigBaseBranch — config present but unusable', () => 'a whitespace-only override must not win the precedence ladder'); }); - test('"git" key present but not an object → skipped, flat legacy key still consulted', () => { + test('"git" key present but not a usable object (string/array/null) → nested lookup finds nothing, flat legacy key still consulted', () => { + // NOTE on the `"git":[]` case: like the sibling note above, this does NOT + // pin `!Array.isArray(gitSection)`. `gitSection` here is a JSON-parsed + // array with no `.base_branch` own-property, so `gitSection.base_branch` + // is `undefined` whether or not the guard runs — the flat key is + // consulted either way. Verified by mutation: deleting + // `&& !Array.isArray(gitSection)` from the built lib does not change this + // output for any JSON-string input. assert.strictEqual(readWith('{"git":"main","base_branch":"release"}'), 'release'); assert.strictEqual(readWith('{"git":[],"base_branch":"release"}'), 'release'); assert.strictEqual(readWith('{"git":null,"base_branch":"release"}'), 'release'); @@ -580,45 +598,18 @@ describe('#3057 W3: tryRemoteShow — tier-3 output that is present but not auth }); describe('#3057 W3: tryLocalBranch — non-empty stdout that names neither main nor master', () => { - /** - * An `execGit` stand-in whose result counts how many times `stdout` is read. - * `tryLocalBranch` reads it once in the `!r.stdout` guard and a second time to - * split it into lines. The read count therefore identifies WHICH null the - * function returned: 1 read = the early guard fired; 2 reads = the guard was - * passed and the final `return null` after the main/master checks ran. - */ - function countingGit(stdoutValue) { - const state = { reads: 0 }; - const git = () => ({ - exitCode: 0, - stderr: '', - signal: null, - error: null, - timedOut: false, - get stdout() { state.reads += 1; return stdoutValue; }, - }); - git.state = state; - return git; - } - test('stdout is exactly "\\n" → null, reached PAST the empty-stdout guard', () => { // This is the branch that was once deleted as "unreachable". The guard is // `if (r.exitCode !== 0 || !r.stdout) return null` — `"\n"` is a truthy // string, so the guard does NOT fire; `split('\n')` yields ["", ""], both // main/master checks are false, and the FINAL `return null` executes. // Deleting that line makes this function return `undefined`, which - // strictEqual(null) catches. The read count proves which null we got. - const git = countingGit('\n'); - assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', git), null); - assert.strictEqual(git.state.reads, 2, - 'stdout must be read twice: once by the guard (which passes) and once to split into lines'); + // strictEqual(null) catches. + assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', constGit({ stdout: '\n' })), null); }); test('stdout is exactly "" → null via the EARLY guard (a different arm)', () => { - const git = countingGit(''); - assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', git), null); - assert.strictEqual(git.state.reads, 1, - 'an empty stdout must short-circuit in the guard, never reaching the line split'); + assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', constGit({ stdout: '' })), null); }); // Boundary trio over the number of branch lines `git branch --list main master` diff --git a/tests/worktree-safety-reap.test.cjs b/tests/worktree-safety-reap.test.cjs index eeed3a899..93ff4a124 100644 --- a/tests/worktree-safety-reap.test.cjs +++ b/tests/worktree-safety-reap.test.cjs @@ -451,7 +451,7 @@ describe('#3057 reapOrphanWorktrees: admin-entry verdicts', () => { assert.ok(fs.existsSync(f.wtDir)); }); - test('reports lock_too_fresh when the real mtime helper cannot stat the lock file', () => { + test('reports lock_age_unknown when the real mtime helper cannot stat the lock file', () => { const f = makeFixture(tmpBase, 'statfails'); // No `mtimeSafe` injection: this drives the module's own default helper and @@ -463,8 +463,12 @@ describe('#3057 reapOrphanWorktrees: admin-entry verdicts', () => { ); const row = onlyRow(result); - assert.strictEqual(row.status, 'skipped'); - assert.strictEqual(row.reason, 'lock_too_fresh'); + // NOT `lock_too_fresh` (#3057): an unreadable mtime is not an age at all. + // Freshness tells an operator to wait; waiting never clears an EIO. + assert.deepStrictEqual( + { status: row.status, reason: row.reason }, + { status: 'skipped', reason: 'lock_age_unknown' } + ); assert.ok(fs.existsSync(f.wtDir)); }); @@ -488,7 +492,7 @@ describe('#3057 reapOrphanWorktrees: admin-entry verdicts', () => { assert.ok(fs.existsSync(f.wtDir)); }); - test('reports lock_too_fresh when the lock file cannot be stat-ed', () => { + test('reports lock_age_unknown, not lock_too_fresh, when mtimeSafe returns null', () => { const f = makeFixture(tmpBase, 'nomtime'); // nowMs is the far future, so a REAL mtime would read as stale and the @@ -499,23 +503,30 @@ describe('#3057 reapOrphanWorktrees: admin-entry verdicts', () => { nowMs: 8640000000000000, })); - assert.strictEqual(row.status, 'skipped'); - assert.strictEqual(row.reason, 'lock_too_fresh'); + assert.deepStrictEqual( + { status: row.status, reason: row.reason }, + { status: 'skipped', reason: 'lock_age_unknown' } + ); assert.ok(fs.existsSync(f.wtDir)); }); - test('reports lock_too_fresh for a zero-age lock under the default guard', () => { + test('reports lock_too_fresh, not lock_age_unknown, for a readable zero-age lock under the default guard', () => { const f = makeFixture(tmpBase, 'defaultguard'); const now = 1000000; + // The other half of the split: the mtime IS readable, the lock genuinely is + // recent, and waiting out the guard genuinely would change the outcome. const row = onlyRow(reapOrphanWorktrees(f.repoDir, { isPidAlive: () => false, mtimeSafe: () => new Date(now), nowMs: now, })); - assert.strictEqual(row.status, 'skipped'); - assert.strictEqual(row.reason, 'lock_too_fresh'); + assert.deepStrictEqual( + { status: row.status, reason: row.reason }, + { status: 'skipped', reason: 'lock_too_fresh' } + ); + assert.ok(fs.existsSync(f.wtDir)); }); test('reaps the same zero-age lock when an injected reapMtimeGuardMs of 0 retires the guard', () => { @@ -702,7 +713,14 @@ describe('#3057 reapOrphanWorktrees: liveness and ancestry verdicts', () => { assert.ok(fs.existsSync(f.wtDir), 'unmerged work must survive the sweep'); }); - test('reports lock_owner_unknown and leaves the worktree on disk for a 400-digit lock PID', () => { + // ── The Number.isFinite PARSE gate ──────────────────────────────────────── + // This gate is NOT the process.kill range limit (pinned in the next block). + // It fires far later, where `parseInt('9'.repeat(N), 10)` stops being + // representable: finite through N=308, Infinity from N=309 (measured). + // Reaching it means the reaper never learned a usable PID at all, so the + // verdict is `lock_owner_unknown`, not a liveness claim. + + test('reports lock_owner_unknown for a 400-digit lock PID (parse overflows past the Number.isFinite gate)', () => { const f = makeFixture(tmpBase, 'giantpid', { lock: false }); fs.writeFileSync(path.join(f.adminDir, 'locked'), '9'.repeat(400)); @@ -712,10 +730,7 @@ describe('#3057 reapOrphanWorktrees: liveness and ancestry verdicts', () => { assert.ok(fs.existsSync(f.wtDir), 'a lock PID that overflows to Infinity must never be reaped'); }); - // Parse-cliff boundary, measured empirically: `parseInt('9'.repeat(N), 10)` - // is finite through N=308 and becomes Infinity at N=309 (probed via - // `node -e`, reported in the task return). - test('reaches the liveness check for a 308-digit lock PID (last finite length)', () => { + test('passes a 308-digit lock PID through the Number.isFinite gate (last representable length)', () => { const f = makeFixture(tmpBase, 'cliffminus1', { lock: false }); fs.writeFileSync(path.join(f.adminDir, 'locked'), '9'.repeat(308)); let seenPid; @@ -729,7 +744,7 @@ describe('#3057 reapOrphanWorktrees: liveness and ancestry verdicts', () => { assert.strictEqual(fs.existsSync(f.wtDir), false); }); - test('reports lock_owner_unknown for a 309-digit lock PID (first Infinity length)', () => { + test('stops a 309-digit lock PID at the Number.isFinite gate (first unrepresentable length)', () => { const f = makeFixture(tmpBase, 'cliffexact', { lock: false }); fs.writeFileSync(path.join(f.adminDir, 'locked'), '9'.repeat(309)); @@ -739,6 +754,87 @@ describe('#3057 reapOrphanWorktrees: liveness and ancestry verdicts', () => { assert.ok(fs.existsSync(f.wtDir)); }); + // ── The process.kill RANGE cliff — the one that actually decides a reap ─── + // Measured with the real `process.kill(pid, 0)` on this platform: + // 2147483647 → Error, code ESRCH (accepted; asks the OS) + // 2147483648 → TypeError ERR_INVALID_ARG_TYPE (rejected before the OS) + // Both tests drive the module's OWN `defaultIsPidAlive` (no `isPidAlive` + // injection) so the verdict is produced by the real errno classification. + // Each asserts the throw shape first: if a future Node moved the cliff, the + // probe fails loudly instead of the verdict flipping silently. + + const PID_KILL_MAX = 2147483647; + + test('treats the largest PID process.kill accepts as dead when the OS answers ESRCH', () => { + const f = makeFixture(tmpBase, 'killmax', { lock: false }); + fs.writeFileSync(path.join(f.adminDir, 'locked'), String(PID_KILL_MAX)); + + // Measured cliff, lower side: this value reaches the OS, which has no such + // process (every platform's max PID is orders of magnitude below it). + assert.throws( + () => process.kill(PID_KILL_MAX, 0), + (err) => err.code === 'ESRCH', + `process.kill(${PID_KILL_MAX}, 0) must reach the OS and report ESRCH` + ); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { mtimeSafe: () => STALE_MTIME })); + + assert.deepStrictEqual( + { status: row.status, reason: row.reason }, + { status: 'reaped', reason: 'pid_dead_and_merged' } + ); + assert.strictEqual(fs.existsSync(f.wtDir), false); + }); + + test('treats the first PID process.kill rejects as ALIVE and leaves the worktree on disk', () => { + const f = makeFixture(tmpBase, 'killmaxplus1', { lock: false }); + const overRange = PID_KILL_MAX + 1; + fs.writeFileSync(path.join(f.adminDir, 'locked'), String(overRange)); + + // Measured cliff, upper side: one past the accepted range, `process.kill` + // throws a TypeError with NO errno. That is "could not determine", not + // "dead" — the old errno-only catch read it as dead and REAPED here. + assert.throws( + () => process.kill(overRange, 0), + (err) => err instanceof TypeError && err.code === 'ERR_INVALID_ARG_TYPE', + `process.kill(${overRange}, 0) must throw TypeError ERR_INVALID_ARG_TYPE` + ); + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { mtimeSafe: () => STALE_MTIME })); + + assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'skipped', reason: 'pid_alive' }); + assert.ok(fs.existsSync(f.wtDir), 'an unclassifiable liveness probe must never reap'); + }); + + test('treats an unrecognised errno from process.kill as ALIVE (only ESRCH means dead)', (t) => { + // EPERM has its own test above; this pins the GENERAL rule for a code the + // helper has never heard of, which an `=== EPERM ? true : false` catch + // would classify as dead. + const f = makeFixture(tmpBase, 'defaultkill-einval'); + const originalKill = process.kill; + t.after(() => { process.kill = originalKill; }); + process.kill = () => { throw Object.assign(new Error('EINVAL'), { code: 'EINVAL' }); }; + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { mtimeSafe: () => STALE_MTIME })); + + assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'skipped', reason: 'pid_alive' }); + assert.ok(fs.existsSync(f.wtDir)); + }); + + test('treats a codeless throw from process.kill as ALIVE', (t) => { + // A thrown value with no `.code` at all (the TypeError case in the + // abstract): `undefined !== 'ESRCH'`, so it must still read as alive. + const f = makeFixture(tmpBase, 'defaultkill-bare'); + const originalKill = process.kill; + t.after(() => { process.kill = originalKill; }); + process.kill = () => { throw new Error('no errno on this one'); }; + + const row = onlyRow(reapOrphanWorktrees(f.repoDir, { mtimeSafe: () => STALE_MTIME })); + + assert.deepStrictEqual({ status: row.status, reason: row.reason }, { status: 'skipped', reason: 'pid_alive' }); + assert.ok(fs.existsSync(f.wtDir)); + }); + test('reaches the liveness check for an ordinary small lock PID', () => { const f = makeFixture(tmpBase, 'ordinarypid', { lock: false }); fs.writeFileSync(path.join(f.adminDir, 'locked'), '4242'); @@ -857,6 +953,30 @@ describe('#3057 cmdWorktreeReapOrphans / pruneOrphanedWorktrees output verdicts' ]); }); + test('pruneOrphanedWorktrees hands the porcelain to a caller-supplied parseWorktreePorcelain', () => { + const f = makeFixture(tmpBase, 'pruneparser'); + const realPorcelain = git(['worktree', 'list', '--porcelain'], f.repoDir); + const seen = []; + const faultyGit = makeFaultyGit({ passthrough: realExecGit }); + + // `parseWorktreePorcelain` is a declared member of the deps bag and + // `planWorktreePrune` reads `deps.parseWorktreePorcelain` first, defaulting + // to the module function only when absent. pruneOrphanedWorktrees therefore + // spreads `...deps` AFTER its own hard-coded default so the caller's parser + // wins. Ordering the two the other way round is invisible to every other + // test in the tree; this one fails if the spread moves. + const removed = pruneOrphanedWorktrees(f.repoDir, { + execGit: faultyGit, + parseWorktreePorcelain: (porcelain) => { seen.push(porcelain); return []; }, + writeErr: () => { throw new Error('no degradation warning expected'); }, + }); + + assert.deepStrictEqual(removed, []); + assert.strictEqual(seen.length, 1, 'the injected parser must be the one that ran, exactly once'); + assert.strictEqual(seen[0], realPorcelain, 'it must receive the porcelain readWorktreeList obtained'); + assert.strictEqual(calledWith(faultyGit, ['worktree', 'prune']), true, 'the metadata prune still runs'); + }); + test('pruneOrphanedWorktrees returns an empty list and warns nothing when git throws', () => { const f = makeFixture(tmpBase, 'prunethrows'); const err = []; From 96fe8a81b4d27fe32ff1e9807c9f87eaa51820e5 Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 6 Aug 2026 02:03:23 -0400 Subject: [PATCH 6/7] chore(#3103): add the Fixed changeset now that the PR number exists The fragment carries the real PR number because changeset-lint rejects the pr: 0 placeholder, so it can only be written once the PR is open. Refs #3057 Co-Authored-By: Claude Opus 5 --- .changeset/plucky-hawks-fly.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/plucky-hawks-fly.md diff --git a/.changeset/plucky-hawks-fly.md b/.changeset/plucky-hawks-fly.md new file mode 100644 index 000000000..7bc16640a --- /dev/null +++ b/.changeset/plucky-hawks-fly.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3106 +--- +**A worktree whose owner could not be probed is no longer deleted** — an orphan lock holding a process id above 2147483647 made the liveness check throw a type error rather than an errno error, which read as "owner is dead" and removed the worktree. Only "no such process" now means dead; every unrecognized outcome leaves the worktree alone. An unreadable lock timestamp also reported "too fresh", advising a wait that could never help, and now reports its own reason. (#3103) From 2134d9739651ae136252f06809b46641be4870b7 Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 6 Aug 2026 02:16:13 -0400 Subject: [PATCH 7/7] test(#3103): expect git's separators, not the ones this machine happens to use MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Windows shard failed on the worktree-root assertion: actual C:/Users/runneradmin/AppData/Local/Temp/gsd-wt-info-Nn4hj6 expected C:\Users\runneradmin\AppData\Local\Temp\gsd-wt-info-Nn4hj6 The value comes straight from `git rev-parse --show-toplevel`, and git reports POSIX separators on every platform. The expected side was built with the native realpath, so the test encoded the separator convention of the machine it was written on. Normalising the expected side keeps the assertion exact — on POSIX the replacement is a no-op, so nothing is weakened where it already passed. This is the same assertion that was strengthened earlier today from a typeof-string-and-non-empty shape check. Pinning the exact path was right; the weaker version would have passed on Windows precisely because it asserted almost nothing. Getting a real assertion wrong on one platform is the better failure, and the Linux-only matrix could not see it — the platform shards caught it, as they did twice in the previous wave. Every other path comparison in the two new test files was swept for the same mistake. The remaining ones are safe: the base-branch tests compare against literal POSIX strings supplied to a mocked git, and the reap tests put both sides through one canonicalising helper, so they cannot disagree on separators. Drive-letter case can differ in principle at the fixed site; it did not here and no case-folding was added on speculation. Refs #3057 Co-Authored-By: Claude Opus 5 --- tests/git-base-branch.test.cjs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/git-base-branch.test.cjs b/tests/git-base-branch.test.cjs index 1fd9f7978..1e0d0ed84 100644 --- a/tests/git-base-branch.test.cjs +++ b/tests/git-base-branch.test.cjs @@ -288,8 +288,18 @@ describe('#1268 gitWorktreeInfoInternal: relocation to git-base-branch', () => { // exact value rather than "a non-empty string": a resolver that returned the // .git dir, the cwd, or any other plausible-looking path would pass the weaker // shape check while being wrong. + // + // git always reports POSIX forward slashes, on every platform including + // Windows, while `fs.realpathSync.native` returns the platform's native + // form (backslashes on Windows). The expected side must therefore be + // normalized to git's convention rather than compared to the raw native + // realpath, or the assertion just encodes the separator convention of + // whatever platform it was written on. This is separators only — POSIX's + // `replace` is a no-op there, so the assertion keeps its full strength on + // POSIX. The remote gsd-test matrix is Linux-only and cannot exercise this + // path; it only surfaced on the Windows GitHub Actions shard. assert.strictEqual(result.inside, true, 'inside must be true for a git project dir'); - assert.strictEqual(result.worktreeRoot, fs.realpathSync.native(dir), + assert.strictEqual(result.worktreeRoot, fs.realpathSync.native(dir).replace(/\\/g, '/'), 'worktreeRoot must be the resolved worktree root path'); });