fix(worktree): prune refused every squash-merged worktree - #41
Merged
Conversation
Contributor
📝 WalkthroughWalkthrough
ChangesSquash-aware worktree pruning
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant prune_worktrees
participant Git
participant remove_worktree
CLI->>prune_worktrees: request prune
prune_worktrees->>Git: assess ancestry and upstream state
Git-->>prune_worktrees: containment and tracking evidence
prune_worktrees->>remove_worktree: remove directory and branch_is_spent
remove_worktree-->>CLI: cleanup status
CLI-->>CLI: append verdict notes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Shipped an hour ago, and its first real run refused to clean up its own worktree: PR #40 was squash-merged, so the branch's commit is not an ancestor of `main` and the strict ancestry check read it as unfinished work. In a repo that squash-merges every PR — this one — that is *every* worktree, which is exactly the accumulation the command exists to stop. A squash merge lands the branch's content on the base as one new commit, so the originals never become ancestors of it. Ancestry cannot see the merge; the forge can. `%(upstream:track)` renders `[gone]` for a branch that was pushed and then had its remote ref deleted, which is what a merged PR leaves behind. That evidence is weaker than ancestry, so it buys less. It clears the *worktree* only: the branch is not marked spent, the ref survives, and the commits stay reachable. If the PR was closed rather than merged, nothing is lost but disk — which was the whole complaint. The distinction is now explicit rather than implied. `disposable` governs the directory, `branch_is_spent` governs the ref, and only ancestry sets the latter. An empty `%(upstream:track)` — a branch that never left the machine — is absence of evidence, not evidence of a merge, and still blocks. Verified end-to-end against a real squash-merge: a branch pushed to a remote, squash-merged to main, and its remote ref deleted. The worktree was removed, the branch preserved, and `git log nightly/squashed` still resolved afterward. 1563 tests pass (6 new); `nightly verify` clean on all five checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ulmentflam
force-pushed
the
nightly/worktree-prune
branch
from
July 29, 2026 03:01
ab80beb to
6345873
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@packages/nightly-core/src/nightly_core/cli.py`:
- Around line 1695-1696: Update the prune command’s documentation near the
referenced help text to reflect that commits ahead of --base may be pruned when
[gone] upstream squash-merge evidence exists, and that pruning such a worktree
does not necessarily delete its branch. Preserve the existing documentation for
cases without this evidence and align the wording with the verdict.notes
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: 4b12d9e8-e3ab-43c1-8e8b-481b5ad73eee
📒 Files selected for processing (4)
README.mdpackages/nightly-core/src/nightly_core/cli.pypackages/nightly-core/src/nightly_core/worktree.pypackages/nightly-core/tests/test_worktree_prune.py
Comment on lines
+1695
to
+1696
| elif verdict.notes: | ||
| note += f" — {' '.join(verdict.notes)}" |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the command’s documented prune contract.
Line 1658 still says commits ahead of --base always keep a worktree, and Lines 1663-1666 imply every removed worktree’s branch is deleted. Both are now false for [gone] upstream squash-merge evidence.
Suggested documentation update
- A worktree is removed only when losing it cannot lose work — clean
- tree, and no commits `--base` does not already have. Anything else is
- kept and the reason printed. A check that cannot be *run* counts as a
- blocker, never as consent.
+ A non-forced worktree is removed only when it is clean and either all
+ of its commits are already in `--base`, or its tracked remote branch
+ is gone (covering squash merges). The latter removes only the
+ worktree; its branch is retained.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| elif verdict.notes: | |
| note += f" — {' '.join(verdict.notes)}" | |
| A non-forced worktree is removed only when it is clean and either all | |
| of its commits are already in `--base`, or its tracked remote branch | |
| is gone (covering squash merges). The latter removes only the | |
| worktree; its branch is retained. |
🤖 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 `@packages/nightly-core/src/nightly_core/cli.py` around lines 1695 - 1696,
Update the prune command’s documentation near the referenced help text to
reflect that commits ahead of --base may be pruned when [gone] upstream
squash-merge evidence exists, and that pruning such a worktree does not
necessarily delete its branch. Preserve the existing documentation for cases
without this evidence and align the wording with the verdict.notes handling.
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.
Follow-up to #40, which shipped an hour ago and then refused to clean up its own worktree.
PR #40 was squash-merged, so its branch's commit is not an ancestor of
main, and the strict ancestry check read it as unfinished work:In a repo that squash-merges every PR — this one — that is every worktree. The command would have kept accumulating exactly the state it exists to clear.
Why ancestry can't see it
A squash merge lands the branch's content on the base as one new commit, so the originals never become ancestors of it. No ancestry check can detect that merge. The forge can:
%(upstream:track)renders[gone]for a branch that was pushed and then had its remote ref deleted, which is what a merged PR leaves behind.That evidence is weaker, so it buys less
A
[gone]upstream is not proof of a merge — a PR closed without merging leaves the same trace. So it clears the worktree only. The branch is not marked spent, the ref survives, and the commits stay reachable. If the PR was closed rather than merged, nothing is lost but disk, which was the entire complaint.The distinction is now explicit rather than implied:
disposablegoverns the directorybranch_is_spentgoverns the ref, and only ancestry sets itAn empty
%(upstream:track)— a branch that never left the machine — is absence of evidence, not evidence of a merge, and still blocks.Output states which evidence was used, so an operator can tell this case from an ordinary merged one:
Verification
Verified end-to-end against a genuine squash merge, not a fake: a branch pushed to a real remote, squash-merged to
main, and its remote ref deleted.git merge-base --is-ancestorconfirmed the commit is not reachable frommain. The worktree was removed, the branch preserved, andgit log nightly/squashedstill resolved afterward.1563 tests pass (6 new).
nightly verifyclean on all five checks.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
worktree prunehandling for squash-merged branches by recognizing deleted remote branches as supporting evidence.Improvements