Skip to content

Commit b32a93e

Browse files
wentaoyuanclaude
andcommitted
fix(review-round-3b): propagate worktreeListDetailed failures via Result
CodeRabbit identified that worktreeListDetailed silently returned [] on `git worktree list` failure, making the try/catch in checkBranchCollisions dead code. The pre-check would then pass as "collision-free" despite never having queried git — violating FR-009 case (b), which requires marking the repo status="failed" and excluding it from subsequent mutation. Changes: - git.ts: worktreeListDetailed now returns Result<WorktreeEntry[], string>. Empty array on parse success, Result.err(gitError) on git failure. - workspace-create.ts checkBranchCollisions: replace try/catch with `if (!result.ok)` branch; cache map now stores WorktreeEntry[] directly. - workspace-create.ts isWorktreeHealthy: treat git failure as unhealthy so reconcile re-runs `git worktree add` (which will surface the underlying git error through normal error paths). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 063f103 commit b32a93e

2 files changed

Lines changed: 23 additions & 17 deletions

File tree

src/plugin/worktree/git.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -225,9 +225,9 @@ interface WorktreeEntry {
225225
* @param repoRoot - Absolute path to the repository root
226226
* @returns Array of WorktreeEntry (includes the main worktree)
227227
*/
228-
async function worktreeListDetailed(repoRoot: string): Promise<WorktreeEntry[]> {
228+
async function worktreeListDetailed(repoRoot: string): Promise<Result<WorktreeEntry[], string>> {
229229
const result = await git(["worktree", "list", "--porcelain"], repoRoot)
230-
if (!result.ok) return []
230+
if (!result.ok) return Result.err(result.error)
231231

232232
const entries: WorktreeEntry[] = []
233233
let currentPath: string | null = null
@@ -253,7 +253,7 @@ async function worktreeListDetailed(repoRoot: string): Promise<WorktreeEntry[]>
253253
entries.push({ path: currentPath, branch: currentBranch })
254254
}
255255

256-
return entries
256+
return Result.ok(entries)
257257
}
258258

259259
// =============================================================================

src/plugin/worktree/workspace-create.ts

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import { loadWorktreeConfig } from "./config"
2424
import {
2525
type Result,
2626
Result as R,
27+
type WorktreeEntry,
2728
branchExists,
2829
createWorktree,
2930
git,
@@ -210,9 +211,9 @@ async function resolveBaseBranch(repoPath: string): Promise<Result<string, strin
210211
* FR-009 mandates per-repo `status="failed"` and exclusion from
211212
* subsequent steps for that repo only; other repos continue.
212213
*
213-
* The function never throws on a per-repo `worktreeListDetailed` failure —
214-
* the failure is captured into `preCheckFailures` and the rest of the
215-
* pre-check continues so all repos get evaluated.
214+
* `worktreeListDetailed` surfaces git errors via its `Result` return (not
215+
* throws), so those failures are captured into `preCheckFailures` and the
216+
* rest of the pre-check continues so all repos get evaluated.
216217
*
217218
* @param plans Planned repo worktree actions (only "create" and
218219
* "retry" plans require pre-check; "reuse" already
@@ -240,7 +241,7 @@ export async function checkBranchCollisions(
240241
// Collect all worktree entries per unique repo root (dedup in case
241242
// multiple plans reference the same repo root — shouldn't happen but
242243
// defensive).
243-
const worktreeCache = new Map<string, Awaited<ReturnType<typeof worktreeListDetailed>>>()
244+
const worktreeCache = new Map<string, WorktreeEntry[]>()
244245
// Repo roots whose pre-check has already failed; skip subsequent plans
245246
// referencing the same repo to avoid duplicate failure entries.
246247
const failedRepoRoots = new Set<string>()
@@ -254,21 +255,22 @@ export async function checkBranchCollisions(
254255
if (failedRepoRoots.has(repoRoot)) continue
255256

256257
// Fetch detailed worktree list (cached per repo root). On failure,
257-
// record it as a per-repo PreCheckFailure rather than throwing —
258-
// FR-009 case (b) requires this repo to be marked failed without
259-
// aborting the whole pre-check.
258+
// record it as a per-repo PreCheckFailure rather than silently
259+
// treating the repo as collision-free — FR-009 case (b) requires
260+
// this repo to be marked failed without aborting the whole
261+
// pre-check. `worktreeListDetailed` returns Result, not throws, so
262+
// the error surfaces via result.ok rather than try/catch.
260263
if (!worktreeCache.has(repoRoot)) {
261-
try {
262-
worktreeCache.set(repoRoot, await worktreeListDetailed(repoRoot))
263-
} catch (error) {
264-
const reason = error instanceof Error ? error.message : String(error)
264+
const result = await worktreeListDetailed(repoRoot)
265+
if (!result.ok) {
265266
preCheckFailures.push({
266267
repoName: plan.repo.name,
267-
reason: `pre-check failed: ${reason}`,
268+
reason: `pre-check failed: ${result.error}`,
268269
})
269270
failedRepoRoots.add(repoRoot)
270271
continue
271272
}
273+
worktreeCache.set(repoRoot, result.value)
272274
}
273275
const entries = worktreeCache.get(repoRoot)!
274276

@@ -327,9 +329,13 @@ async function isWorktreeHealthy(
327329
}
328330

329331
// (c) Is the path listed in `git worktree list`?
330-
const entries = await worktreeListDetailed(repoPath)
332+
// If git fails (e.g. repo corrupted or binary missing), treat the
333+
// worktree as unhealthy — the reconcile path will then retry via
334+
// `git worktree add` which will surface the underlying error.
335+
const result = await worktreeListDetailed(repoPath)
336+
if (!result.ok) return false
331337
const normalizedTarget = path.resolve(worktreePath)
332-
return entries.some((entry) => path.resolve(entry.path) === normalizedTarget)
338+
return result.value.some((entry) => path.resolve(entry.path) === normalizedTarget)
333339
}
334340

335341
// =============================================================================

0 commit comments

Comments
 (0)