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) 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..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; } @@ -1737,9 +1750,26 @@ 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. 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; + } let pidIsAlive: boolean; try { - pidIsAlive = Number.isNaN(pid) || isPidAliveCheck(pid); + pidIsAlive = isPidAliveCheck(pid); } catch { pidIsAlive = true; // Cannot determine liveness — treat as alive, do not reap. } @@ -1803,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'; } } @@ -1825,21 +1870,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 +1922,23 @@ 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 { + // `...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 }, - { 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' diff --git a/tests/git-base-branch.test.cjs b/tests/git-base-branch.test.cjs index d67ff38bc..1e0d0ed84 100644 --- a/tests/git-base-branch.test.cjs +++ b/tests/git-base-branch.test.cjs @@ -283,9 +283,24 @@ 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. + // + // 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.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).replace(/\\/g, '/'), + 'worktreeRoot must be the resolved worktree root path'); }); test('gitWorktreeInfoInternal(createTempDir()) returns {inside:false, worktreeRoot:null} for a non-git dir', (t) => { @@ -296,11 +311,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 +381,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 +399,482 @@ 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', () => { + // 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'); + 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 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'); + }); + + 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', () => { + 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. + assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', constGit({ stdout: '\n' })), null); + }); + + test('stdout is exactly "" → null via the EARLY guard (a different arm)', () => { + assert.strictEqual(gitBaseBranch.tryLocalBranch('/x', constGit({ stdout: '' })), null); + }); + + // 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 +1193,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)' + ); }); }); }); diff --git a/tests/worktree-safety-reap.test.cjs b/tests/worktree-safety-reap.test.cjs new file mode 100644 index 000000000..93ff4a124 --- /dev/null +++ b/tests/worktree-safety-reap.test.cjs @@ -0,0 +1,992 @@ +'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_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 + // 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); + // 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)); + }); + + 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_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 + // 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.deepStrictEqual( + { status: row.status, reason: row.reason }, + { status: 'skipped', reason: 'lock_age_unknown' } + ); + assert.ok(fs.existsSync(f.wtDir)); + }); + + 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.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', () => { + 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'); + }); + + // ── 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)); + + 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'); + }); + + 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; + + 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('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)); + + 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)); + }); + + // ── 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'); + 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({ + 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 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 = []; + + 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'),