feat(worktree): cleanup reaps squash/rebase-merged worktrees (P4-GIT-04)#138
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR adds offline squash/rebase-merge detection, wires it into ChangesWorktree cleanup reaps squash/rebase-merged branches
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reederey87
force-pushed
the
feat/p4-git-04-worktree-gc
branch
from
00c8dc5 to
8ea724e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/cli/worktree.go (2)
505-505: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWarnings mix output channels.
The existing skip warnings ("stat failed", "dirty-state check failed") go through
logging.Logger(cmd.Context()).Warn(...), but the two new warnings (refresh failure, branch-delete failure) are written directly tostdoutviafmt.Fprintf. This mixes structured, filterable logs with ad-hoc user-facing text in the same command, making both harder to consume consistently (e.g. scripts scraping stdout for "Removed worktree" lines will also see these warnings interleaved).♻️ Suggested consistency fix
if err := refreshWorktreeBase(cmd.Context(), opts, r, project, repoPath, wt.BaseRef); err != nil { - _, _ = fmt.Fprintf(stdout, "warning: could not refresh %s for worktree %s: %v; using local ref\n", wt.BaseRef, wt.ID, err) + logging.Logger(cmd.Context()).Warn("worktree cleanup: base refresh failed, using local ref", "worktree", wt.ID, "base_ref", wt.BaseRef, "error", err.Error()) } ... if _, err := r.Run(cmd.Context(), repoPath, "branch", "-D", wt.Branch); err != nil { - _, _ = fmt.Fprintf(stdout, "warning: failed to delete branch %s for removed worktree %s: %v\n", wt.Branch, wt.ID, err) + logging.Logger(cmd.Context()).Warn("worktree cleanup: branch delete failed", "worktree", wt.ID, "branch", wt.Branch, "error", err.Error()) }Also applies to: 511-511, 521-521, 547-547
🤖 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` at line 505, The worktree cleanup warnings are inconsistent because `cleanupRemovedWorktree` logs some failures with `logging.Logger(cmd.Context()).Warn(...)` but writes the newer refresh and branch-delete failures directly to stdout. Update those warning paths to use the same structured logger as the existing “stat failed” and “dirty-state check failed” messages so all warnings are emitted consistently through `logging.Logger(cmd.Context())` and not mixed with ad-hoc stdout text.
518-557: 🩺 Stability & Availability | 🔵 TrivialRepo lock only guards the fetch, not the branch delete/removal.
refreshWorktreeBaseacquiresacquireRepoLockfor the fetch, but the subsequentbranch --merged,IsSquashMerged,WorktreeRemove, andbranch -Dcalls on the samerepoPathrun unlocked. If another process (daemon materialization, concurrentworktree new) mutates the same repo between the fetch and the branch deletion, there's a narrow window for surprising interactions. This mirrors the existing pattern innewWorktreeRemoveCommand(also unlocked), so it isn't a regression introduced here, but worth confirming that repo-level git mutations outside of fetch are expected to be safe unlocked in this codebase.🤖 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 518 - 557, The worktree removal flow in the command handling logic is only locking the fetch path via refreshWorktreeBase, while branch inspection and deletion in the same repoPath (`branch --merged`, `IsSquashMerged`, `WorktreeRemove`, and `branch -D`) still run unlocked. Update the removal sequence in the worktree command to either hold the same repo lock across the full git mutation window or explicitly confirm and document that these repo-level operations are safe to run without locking, keeping the behavior consistent with `newWorktreeRemoveCommand` and the rest of the git repo access pattern.
🤖 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 `@spec/13_CLI_DAEMON_API.md`:
- Line 356: Remove the retired merged-ness fallback chain from the CLI spec
paragraph. Update the `P4-GIT-04` description to match the shipped
`git.Runner.IsSquashMerged` behavior: it should describe the `merge-tree`-based
check, and state that older Git versions or any error/ambiguity fail closed as
not merged. Keep the rest of the `worktree cleanup --merged` text intact, but
delete references to `git cherry`, the synthetic-commit recipe, and `patch-id
--stable`.
---
Nitpick comments:
In `@internal/cli/worktree.go`:
- Line 505: The worktree cleanup warnings are inconsistent because
`cleanupRemovedWorktree` logs some failures with
`logging.Logger(cmd.Context()).Warn(...)` but writes the newer refresh and
branch-delete failures directly to stdout. Update those warning paths to use the
same structured logger as the existing “stat failed” and “dirty-state check
failed” messages so all warnings are emitted consistently through
`logging.Logger(cmd.Context())` and not mixed with ad-hoc stdout text.
- Around line 518-557: The worktree removal flow in the command handling logic
is only locking the fetch path via refreshWorktreeBase, while branch inspection
and deletion in the same repoPath (`branch --merged`, `IsSquashMerged`,
`WorktreeRemove`, and `branch -D`) still run unlocked. Update the removal
sequence in the worktree command to either hold the same repo lock across the
full git mutation window or explicitly confirm and document that these
repo-level operations are safe to run without locking, keeping the behavior
consistent with `newWorktreeRemoveCommand` and the rest of the git repo access
pattern.
🪄 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: 8d0086ef-5472-4e30-be6c-2cafe443ce1a
📒 Files selected for processing (9)
docs/audits/README.mdinternal/cli/worktree.gointernal/cli/worktree_test.gointernal/git/git.gointernal/git/git_test.gospec/08_GIT_MATERIALIZATION_AND_WORKTREES.mdspec/13_CLI_DAEMON_API.mdspec/16_TEST_PLAN.mdspec/18_WORK_LOG.md
Reederey87
force-pushed
the
feat/p4-git-04-worktree-gc
branch
from
8ea724e to
62e3f81
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@spec/08_GIT_MATERIALIZATION_AND_WORKTREES.md`:
- Around line 300-302: The cleanup flow in the worktree mergedness logic is too
permissive when refreshing the recorded base ref fails, because it continues
using the stale local ref and may delete worktrees that are no longer merged
upstream. Update the `devstrap worktree cleanup --merged` behavior described in
this spec so the fetch-failure path fails closed (treat as not merged and skip
reaping) or otherwise align the text with the real fallback behavior. Make sure
the wording around the mergedness check, the base-ref refresh step, and the
“never reaps on doubt” guarantee is consistent with the actual implementation.
🪄 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: b93ccd90-4da1-450c-aa54-abf070db5e28
📒 Files selected for processing (9)
docs/audits/README.mdinternal/cli/worktree.gointernal/cli/worktree_test.gointernal/git/git.gointernal/git/git_test.gospec/08_GIT_MATERIALIZATION_AND_WORKTREES.mdspec/13_CLI_DAEMON_API.mdspec/16_TEST_PLAN.mdspec/18_WORK_LOG.md
✅ Files skipped from review due to trivial changes (4)
- spec/18_WORK_LOG.md
- spec/13_CLI_DAEMON_API.md
- docs/audits/README.md
- spec/16_TEST_PLAN.md
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/cli/worktree_test.go
- internal/git/git_test.go
- internal/cli/worktree.go
- internal/git/git.go
worktree cleanup --merged only tested ancestry, so squash- and rebase-merged branches were skipped forever. New git.Runner.IsSquashMerged uses a current-tree merge probe (git merge-tree --write-tree, git >= 2.38): merged iff the simulated merge's tree equals the current base tree. The first-draft git cherry/patch-id chain was replaced in dual review — historical patch-id equivalence matches merged-then-reverted changes and would reap unmerged work. Conservative on conflict/error; the inherent coincidental-identical-diff limitation is tripwire-pinned and every reap prints the deleted branch's tip SHA as the recovery breadcrumb. Base fetched under the repo lock (deduped per project/ref); reaped branches get git branch -D. Ledger: P4-GIT-04 -> Recently shipped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reederey87
force-pushed
the
feat/p4-git-04-worktree-gc
branch
from
62e3f81 to
4568ea3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ships audit finding P4-GIT-04 (worktree GC that reaps squash/rebase-merged worktrees); ledger row moved to Recently shipped.
What
worktree cleanup --mergedtested merged-ness withgit branch --merged— ancestry only — so a branch merged via GitHub "Squash and merge" (or a rebase-merge) was never an ancestor of base and its worktree was skipped forever.How
git.Runner.IsSquashMerged(ctx, dir, branch, baseRef)— a current-tree merge probe:git merge-tree --write-tree <base> <branch>(git ≥ 2.38) reports merged only when the simulated merge's tree is identical to the current base tree — the branch would contribute nothing, which is exactly the effect of a squash/rebase/cherry-pick merge.git cherry+ synthetic-commit +patch-id --stablechain was replaced in dual review: both reviewers independently reproduced that historical patch-id equivalence matches a change that was merged then reverted on base — reaping genuinely-unmerged work. The merge-tree probe compares against the current tree and is immune to the revert class.--write-tree, or any error → not merged. Never reap on doubt.IsSquashMerged. Reaps are labeledmergedvsmerged (squash)and print the deleted branch's tip SHA (branch <name> was at <sha>) sogit branch <name> <sha>restores it until gc.refreshWorktreeBasebest-effort fetches the recordedremote/branchbase under the repo lock, deduped per (project, base ref) per run (review finding); warn + continue offline. Reaped worktrees also getgit branch -D(warn-only). Dirty worktrees still skip unless--force.gh pr list --state merged) cross-checks are a documented non-goal — cleanup works offline.Tests
Real-git:
TestIsSquashMergedDetectsSquashMerge,...DetectsRebaseMerge,...FalseForUnmerged,...ConservativeOnContentDivergence,...FalseAfterRevertOnBase(the dual-review repro),...MatchesCoincidentallyIdenticalDiff(documented-limitation tripwire); CLI e2eTestWorktreeCleanupReapsSquashMergedWorktree.Specs
spec/08 (cleanup semantics + conservative rule + limitation + breadcrumb), spec/16, spec/18 work log,
docs/audits/README.mdledger reconciliation.Validation
gofmtclean ·golangci-lint run0 issues ·go test -race ./...ok ·spec-drift --base origin/mainpass · dual review (coordinator line-by-line + gpt-5.5 implementation + opus reviewer + Codex review; the shared Major redesigned pre-merge).🤖 Generated with Claude Code
Summary by CodeRabbit
worktree cleanup --mergednow recognizes squash/rebase-style merges via an offline content-equivalence check, with ancestry as the first path.