fix(git): clean up the worktree+branch on any failure after worktree add (P6-GIT-05)#41
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds cleanup for freshly created worktrees and branches when post-add steps fail, extends CLI regression coverage, and updates related specs, work log, and audit ledger entries. The agent run failure path also now deletes the created branch after worktree cleanup. ChangesFresh worktree cleanup implementation
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/cli/worktree_test.go (1)
76-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant/tautological assertion.
wtPathis extracted fromstderrbylfsFailureWorktreePath, sostrings.Contains(stderr, wtPath)is true by construction and never catches a real regression. Either drop it or replace with a check that the message doesn't imply the worktree survives (once the message wording issue onworktree.goline 241 is addressed).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/worktree_test.go` around lines 76 - 83, The assertion in worktree_test.go is tautological because lfsFailureWorktreePath extracts wtPath from stderr, so checking strings.Contains(stderr, wtPath) adds no value. Update the test around executeForTest and lfsFailureWorktreePath to either remove that check or replace it with a meaningful assertion about the LFS failure message from worktree new, ensuring the test verifies the intended wording/behavior instead of reusing a value parsed from the same output.internal/cli/worktree.go (1)
174-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate worktree/branch cleanup logic vs.
internal/cli/agent.go.The same "
WorktreeRemovethen force-delete branch" sequence is duplicated between this closure and the inline cleanup innewAgentRunCommand'sRunE(internal/cli/agent.go, Lines 79-80). Extracting a shared helper (e.g.,cleanupOrphanWorktree(ctx, r, repoPath, wtPath, branch, stdout)) would fix both call sites at once for the error-swallowing/context-reuse concerns raised above, and prevent the two copies from drifting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/worktree.go` around lines 174 - 188, The orphan worktree cleanup logic is duplicated here and in newAgentRunCommand’s RunE, so refactor the repeated WorktreeRemove plus force-delete branch sequence into a shared helper such as cleanupOrphanWorktree that takes the context, runner, repo path, worktree path, branch, and stdout; update this closure in internal/cli/worktree.go and the inline cleanup in internal/cli/agent.go to call that helper so both sites stay consistent and share the same error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/agent.go`:
- Around line 79-81: The cleanup in the worktree removal path is still silently
discarding errors from gitRunner(opts).WorktreeRemove and gitRunner(opts).Run
for branch deletion, and it reuses cmd.Context() even when that context may
already be canceled. Update this flow to capture and log or return cleanup
errors instead of ignoring them, and invoke the git cleanup using a fresh
background/derived context rather than cmd.Context(). If possible, extract the
cleanup logic into the same shared helper used by createFreshWorktree so both
call sites handle WorktreeRemove, branch -D, and store.MarkWorktreeRemoved
consistently.
In `@internal/cli/worktree.go`:
- Line 241: The error returned from createFreshWorktree is now misleading
because cleanupOrphan removes the worktree after an LFS pull failure, so the
message should no longer imply the worktree remains or that pointer-file cleanup
is needed. Update the fmt.Errorf text in createFreshWorktree to reflect that the
worktree was cleaned up after the LFS failure, and keep it consistent with any
test parsing in
lfsFailureWorktreePath/TestCreateFreshWorktreeCleansUpAfterLFSPullFailure by
adjusting the fixed markers there at the same time.
- Around line 174-188: The cleanup in worktree creation is swallowing failures
from both WorktreeRemove and branch deletion, so orphaned checkouts can remain
without any visibility. Update cleanupOrphan in internal/cli/worktree.go to
capture and surface errors from r.WorktreeRemove and r.Run("branch", "-D",
branch), using stdout to emit a warning if either cleanup step fails. Keep the
existing call sites in the applyWorktreeLFSPolicy and store.CurrentDevice error
paths, but make sure cleanupOrphan reports any manual cleanup needed instead of
discarding the result.
- Around line 174-188: The orphan cleanup in worktree setup can be cancelled
along with the main request, so cleanup may never run to completion and leave
the checkout/branch behind. Update the cleanupOrphan logic in worktree.go to use
a detached background context, ideally with a short timeout, for both
r.WorktreeRemove and r.Run so cleanup can proceed even after the original ctx is
interrupted. Keep the rest of the error handling in the worktree creation flow
the same.
---
Nitpick comments:
In `@internal/cli/worktree_test.go`:
- Around line 76-83: The assertion in worktree_test.go is tautological because
lfsFailureWorktreePath extracts wtPath from stderr, so checking
strings.Contains(stderr, wtPath) adds no value. Update the test around
executeForTest and lfsFailureWorktreePath to either remove that check or replace
it with a meaningful assertion about the LFS failure message from worktree new,
ensuring the test verifies the intended wording/behavior instead of reusing a
value parsed from the same output.
In `@internal/cli/worktree.go`:
- Around line 174-188: The orphan worktree cleanup logic is duplicated here and
in newAgentRunCommand’s RunE, so refactor the repeated WorktreeRemove plus
force-delete branch sequence into a shared helper such as cleanupOrphanWorktree
that takes the context, runner, repo path, worktree path, branch, and stdout;
update this closure in internal/cli/worktree.go and the inline cleanup in
internal/cli/agent.go to call that helper so both sites stay consistent and
share the same error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4719e4e5-e478-4957-bc79-24754f79d2f6
📒 Files selected for processing (8)
docs/audits/README.mdinternal/cli/agent.gointernal/cli/worktree.gointernal/cli/worktree_test.gospec/08_GIT_MATERIALIZATION_AND_WORKTREES.mdspec/10_AGENT_WORKSPACES_AND_POLICIES.mdspec/16_TEST_PLAN.mdspec/18_WORK_LOG.md
…add (P6-GIT-05) After a successful `git worktree add`, three failure returns in createFreshWorktree (LFS pull, CurrentDevice, InsertWorktree) leaked a live checkout under ~/.devstrap/worktrees/ plus an agent/... branch that SQLite never saw — invisible to `worktree list`/`cleanup` and to the planned DB-driven GC, compounding on every retry. A cleanupOrphan closure now removes both on each of those returns; the agent-run file-policy-denial cleanup deletes the branch too, and the LFS error names the worktree path. Pinned by TestCreateFreshWorktreeCleansUpAfterLFSPullFailure (fake-git PATH shim forces `git lfs pull` to fail) and TestCreateFreshWorktreeCleansUpAfterInsertWorktreeFailure (SQLite BEFORE INSERT trigger forces the DB insert to fail): in both, the checkout directory is gone and `git branch --list 'agent/*'` is empty. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ntext (review) Dual-review hardening (opus-4.8, Minor): cleanupOrphan reused the failing ctx, so when the post-add failure was itself a cancellation (Ctrl-C mid-LFS-pull), both cleanup git commands no-op'd on the already-cancelled ctx and the orphan leaked anyway — the exact leak the fix targets. The cleanup now runs under context.WithoutCancel with a 2-minute cap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…th sites (CodeRabbit) Both cleanup call sites (createFreshWorktree failure paths and the agent-run policy-denial M2 path) now go through removeOrphanWorktree: detached bounded context (WithoutCancel + 2m — already applied to the first site, now to both) and WorktreeRemove/branch -D/MarkWorktreeRemoved failures surfaced as warnings instead of silently swallowed, so an operator knows when manual cleanup is needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Pass 6 P2 quick-win wave, PR 4 of 4 (
P6-GIT-05, docs/audits/AUDIT_RECOMMENDATIONS_2026-07-01_PASS6.md).createFreshWorktreeleaked a live, DB-invisible worktree +agent/...branch whenever a step failed aftergit worktree add: the LFS pull (lfs_policy=agent|alwayson a flaky network), theCurrentDevicelookup, or theInsertWorktreeDB write. Because nothing was recorded in SQLite,worktree list/cleanupcould never see or reap the orphan, and everyagent run/worktree newretry minted a fresh random-suffixed branch — compounding the leak.Fix:
cleanupOrphanclosure (WorktreeRemove +branch -D) runs on each of the three post-add failure returns.agent runfile-policy-denial cleanup (M2) now deletes the branch too, not just the worktree.Tests
TestCreateFreshWorktreeCleansUpAfterLFSPullFailure— a PATH-shim fake git forcesgit lfs pullto exit 42; asserts the error contains the worktree path, the checkout is gone, and noagent/*branch remains.TestCreateFreshWorktreeCleansUpAfterInsertWorktreeFailure— a SQLiteBEFORE INSERTtrigger forces the DB insert to fail; same post-conditions.Validation
gofmt -w cmd internalgolangci-lint run(0 issues)go run ./cmd/spec-drift --base origin/main --head HEAD(pass)GOCACHE=/tmp/devstrap-gocache go test -race ./...(green)Specs: 08 + 10 (cleanup guarantee documented), 16 (test inventory + e2e scenario steps), 18 work log; ledger reconciled (open-count line harmonized with sibling wave PRs at merge time).
Implementation delegated to gpt-5.5 (Codex) per the model policy; diff reviewed line-by-line.
🤖 Generated with Claude Code
Summary by CodeRabbit
agent/*branches or worktree directories.