From c2d5b528e573cc69ee1d43d2bfdef461911e40d5 Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 6 Aug 2026 00:32:28 -0400 Subject: [PATCH] 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'