fix(remove, prune, merge): target the named worktree, retain shared branches - #3533
Conversation
…anches wt remove addressed every argument by branch name, resolving it through worktree_for_branch — which, for a branch checked out in several worktrees (only reachable via git worktree add --force), returns git's first-listed worktree. So wt remove <path-to-duplicate> removed an arbitrary duplicate, deleted the shared branch, and orphaned the surviving worktree at a null OID (git update-ref -d bypasses git's checked-out-elsewhere guard). Remove non-current worktrees by their resolved path instead, so the exact worktree named is dropped. prepare_worktree_removal now detects when the branch is still checked out in another worktree and retains it (forces Keep, the single chokepoint every deletion path honors), surfacing the surviving checkout so the user knows why the branch survived. The RemoveTarget::Path arm gains the missing-directory prune fallback the branch arm already had, and matches worktrees symlink-aware via paths_match.
worktrunk-bot
left a comment
There was a problem hiding this comment.
This touches the data-loss surface (wt remove branch deletion in src/commands/repository_ext.rs), so flagging for a human call rather than self-approving — requesting review from @max-sixty.
The core fix reads well: removing by resolved path is unambiguous, and forcing Keep at the single chokepoint when a sibling checkout exists is the right shape to avoid orphaning. One residual gap in the same data-loss class, noted inline — the missing-directory prune fallback returns BranchOnly before the sibling-checkout guard runs, so it can still delete a branch that's live in a survivor. Narrow (requires the target's dir to be externally removed while a --force duplicate of an integrated branch survives), and it pre-dates this PR in the branch-targeted arm, but it sidesteps exactly the retention this PR adds.
Everything else — the paths_match symlink-aware matching, the JSON field, the output message, and the test coverage — looks correct.
| && !wt.path.exists() | ||
| { | ||
| self.prune_worktrees()?; | ||
| Resolved::BranchOnly { |
There was a problem hiding this comment.
This early return lands before the Phase 5 branch_checked_out_at retention guard, so it isn't protected by it. If the target worktree's directory was externally removed (rm -rf) while a --force duplicate of the same branch survives, this prunes and falls through to RemoveResult::BranchOnly, which deletes the branch when integrated via the CAS git update-ref -d — the very call that bypasses git's checked-out-elsewhere guard and orphans the survivor at a null OID.
Narrow (integrated branch + missing dir + surviving --force sibling), and the branch-targeted arm has the same gap today, so not a blocker for this PR. But since the whole point here is to stop orphaning survivors, worth either detecting the sibling checkout before the BranchOnly fallback deletes the branch, or leaving a note that the missing-directory path is intentionally out of scope.
There was a problem hiding this comment.
Fixed in 6c9fb5b — the branch-only fallback now runs the same sibling-checkout detection before deleting the ref.
When a pruned removal resolves to BranchOnly and a still-intact worktree has the branch checked out (a --force duplicate), the branch is retained (deletion_mode forced to Keep) and the survivor is named in the output, so the CAS git update-ref -d never fires against a ref live in another worktree. Because both the path- and branch-targeted arms funnel through the one Resolved::BranchOnly handler in Phase 4, this closes the pre-existing branch-targeted gap too. A new branch_checked_out_at on RemoveResult::BranchOnly (mirroring the RemovedWorktree variant) carries the survivor path to the message; test_remove_pruned_dir_with_sibling_checkout_retains_branch covers rm -rf of the target dir with a surviving integrated duplicate.
The path- and branch-targeted branch-only fallbacks prune a missing worktree directory and fall through to branch deletion before Phase 5's sibling-checkout retention runs. For an integrated branch with a surviving --force duplicate, that deletes the ref via the CAS git update-ref -d and orphans the survivor at a null OID — the exact data loss the sibling guard prevents on the worktree-removal path. Detect a live sibling checkout in the branch-only path too and retain the branch (forcing Keep), surfacing the survivor so the user knows why the branch was kept. Adds branch_checked_out_at to RemoveResult::BranchOnly mirroring the RemovedWorktree variant.
Resolves the conflicts with #3607 (path resolution) and #3617 (the removal chain's one-check-per-guarantee refactor), and reworks the retention onto their shape: - One `live_sibling_checkout` predicate replaces the two divergent sibling scans. A sibling entry whose directory is already gone no longer retains the branch and names a path that isn't there. - `wt merge` and `wt step prune` ask it too. Both reached the same ref deletion outside the guard; merge asserted the invariant in a comment instead of checking it. - `wt step prune` targets candidates by path, matching `wt remove`. Targeting a stale entry by branch name resolved to a live worktree the same prune had skipped as too young, then removed it. - A `-D` this refuses warns instead of passing quietly. - #3480's duplicate hint points at `wt remove <path>`, now the safe answer, and `wt remove --help` states the retention. Tests: remove (path, name, refused -D, pruned fallback, stale sibling must not retain), prune (stale entry with a live sibling), merge (duplicate checkout). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- `Candidate::remove_target` always takes the path: routing `Prunable` candidates through it left the branch fallback dead, and inlining the `.context` drops the unreachable arm. - The refused-`-D` test runs `--foreground`, so the retention message is exercised on the synchronous removal path as well as the background one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
Reviewed the incremental since my last review (the fix(prune, tests) commit) — the prune refactor holds up: targeting stale/duplicate worktrees by their own path rather than re-resolving the branch to git's first-listed checkout is the right shape, computing the removal plan on the parallel scan (and deferring only Prunable, whose prep mutates metadata under the write lock) is sound, and swapping the per-worktree is_linked() probe for idx == 0 && !is_bare matches list_worktrees()'s documented ordering guarantee (main first, bare entries filtered). No new findings. My earlier data-loss-surface flag for @max-sixty still stands.
The affected tests (windows, advisory) red is not this PR. It's a cargo-affected tooling error, not a test failure:
Error: git diff stdout was not valid UTF-8: invalid utf-8 sequence of 1 bytes from index 112399
The failure fires after compilation, before any test runs, when cargo-affected decodes its git diff output to select affected tests. Two things pin it to the tooling rather than the change: the byte offset (112399) is past the PR's entire 66 KB three-dot diff, which contains no binary files, so the non-UTF-8 byte isn't in what this PR changed; and the string isn't one worktrunk emits. The job is advisory, and the required test (windows) passed, so the PR's own tests are green on Windows. A rerun should clear it — no code change warranted.
…ed (#3633) Three follow-ups from the review of #3533, which landed shared-branch retention across the removal surface. Each is small and independent; they share a branch because they came out of the same pass. ## `wt step prune` counted kinds, not outcomes A candidate's kind is what the scan selected, not what the removal took. A branch a sibling worktree still has checked out is retained, so a worktree candidate can take the worktree and leave the branch standing. The summary counted the kind anyway: ```console ○ Worktree directory missing for feature; pruned ↳ branch checked out at ~/code/repo.feature ✓ Pruned 1 branch ← the branch is right there ``` `prune_summary` now counts the planned outcome, so that reads `✓ Pruned 1 worktree` — the stale entry, which is all that went. Both `--format=json` payloads gained `branch_deleted` for the same reason: a consumer reading `{"branch": "feature", "kind": "branch_only"}` would reasonably conclude the branch is gone. The predicate has one home now. `RemoveResult::to_json` already computed `branch_deleted` inline; it moved to `RemoveResult::deletes_branch()` and both callers share it. That incidentally fixes a detached worktree reporting `"branch_deleted": true` beside a null branch — it has no branch to delete. `wt step prune --dry-run` had the same defect and no per-item line to contradict it, so it now predicts retention too. A `Prunable` item has no plan until `try_remove` prunes its stale entry, so the dry run asks `live_sibling_checkout` — the same predicate the plan would. ## `RemoveTarget::Branch` now means "a branch with no worktree" All three callers resolve first and pass `Path` for anything with a worktree, so the arm handling "the branch turned out to have a worktree" was unreachable at selection time — codecov confirmed it never executed. Deleting it would have been enough, but the arm was a live hazard rather than dead weight: the picker builds a fresh `Repository` and re-lists worktrees between selecting a row and removing it, so a `wt switch` in another terminal can give a branch-only row a worktree mid-flight. The old code would then have removed that worktree, from a row that said "delete this branch" — the same defect class #3533 fixed. ```console ✗ Branch feature gained a worktree @ ~/code/repo.feature since it was selected; to remove that worktree, run wt remove ~/code/repo.feature ``` `test_prepare_removal_refuses_branch_that_gained_a_worktree` covers the picker path. I checked it fails when the guard is neutralized rather than passing trivially. ## `affected tests (windows, advisory)` was permanently red git calls a file binary only when it finds a NUL byte in the first 8000, and two of the committed loose objects under `tests/fixtures/*/_git/` are zlib streams small enough to have none. Those diff as text, so `git diff` writes raw deflate into its output, and `cargo affected` aborts decoding it as a string before running a test. Reproduced with `git show <commit> -- <object>` (invalid UTF-8, byte `0x95` at position 5403) and confirmed fixed — the same command now reports `Binary files … differ`. The rest of `_git/` stays text worth reading. ## Testing `prune_summary_counts_a_retained_branch_as_worktree_only` covers the counting; `test_prune_retains_branch_checked_out_in_another_worktree` gained an assertion on the summary line, and I verified it fails when the guard is removed. The six `.snap` changes are all the same added `branch_deleted` key. > _This was written by Claude Code on behalf of max_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#3533 grew RemoveResult, tripping clippy on the merge commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cuts 0.70.0 — version bump plus the changelog for the 45 commits since v0.69.2. **Minor bump, not patch.** `cargo semver-checks` reports 6 breaking library changes (`set_command_timeout` and `ListConfig::task_timeout_ms` removed, `WorkingTree::stage` arity changed, three enum variants added to exhaustive enums). Worktrunk ships breaking library changes freely, but semver still puts a break at minor while pre-1.0. ## Release validation - Local gate green on the release commit: 4631 tests, lints, doctests. - `nightly` dispatched on the cut-from tip (`a27cbd42`) for the full cross-platform suite — `full-tests` green on linux, macOS, and Windows, plus minimal-versions, nix-flake, crate-build, and the release targets. ## Data-loss surface review The cumulative diff was audited against the deletion surface. One deliberate widening, signed off for this release with follow-ups to file: - **#3602** removes the content check from `wt config shell install`'s legacy cleanup, so `conf.d/{cmd}.fish` and the stranded nushell `{cmd}.nu` are now deleted by path, unread. Only that exact filename is touched and each removal is reported, but the deletion is absent from both `--dry-run` and the confirmation prompt, and the already-configured path skips the prompt entirely. Also noted, none blocking: - The `!path.exists()` check precedes the lock guard on the `Path`/`Current` removal arm, so a locked worktree whose directory is absent loses its branch. This already governed the branch-targeted route in v0.69.2; #3533 unified the other arms onto it. The FAQ's "Neither `git worktree remove` nor `wt remove` (even with `--force`) will delete them" is absolute where the behavior isn't. - On the default background removal path, `ensure_clean` and `stop_fsmonitor_daemon` swapped order, so the safety gate is now answered by the live fsmonitor daemon rather than a full re-stat. Bounded by trash staging with 24-hour retention, and the foreground and picker paths were already daemon-served. Net *improvements* to the same surface: shared-branch retention across remove/prune/merge (#3533), outcome-accurate removal reporting (#3633, #3637), and the removal of the thread-local command timeout that could kill in-flight git commands on worker threads (#3615). ## Changelog accuracy Entries were verified against the actual diffs rather than commit messages, which corrected several drafts: the prune figures were one PR stale (~2.9 s → the real ~0.6 s), `wt merge` takes no worktree argument so it only gained the retention half of #3533, the Azure DevOps report is behind `--full`, #3608 never touched `nightly.yaml`, and #3601 inverted what the FAQ change actually said. Two omissions were added — the `install-statusline` foreign-statusline fix (#3595) and the shipped `/wt-switch-create` skill change (#3636). > _This was written by Claude Code on behalf of Maximilian_
Follow-up to the discussion on #3480: @max-sixty asked to have
wt removehonor an explicit path generally. #3607 has since landed the resolution half,Repository::resolve_worktree, which turns a path into a worktree. This is the removal half, and it is the part resolution cannot decide: which worktree to act on is a naming question, whether the branch may be deleted is not.Problem
wt removethrew the resolved answer away. For any non-current worktree it re-targeted by branch name, andprepare_worktree_removalmapped that branch back to git's first-listed worktree. Two failures followed, both silent.The wrong worktree is removed. With
featurechecked out twice:…/dupis gone.…/first, the one named, is still there.The survivor is left broken. The shared branch is deleted with it. Worktrunk deletes branches with
git update-ref -d, git's compare-and-swap primitive, which unlikegit branch -ddoes not refuse a ref that is checked out somewhere:wt step prunereached the same deletion unattended, andwt mergereached it with a freshly integrated branch, so nothing else declined. A branch gets a second worktree only throughgit worktree add --force; worktrunk never does it itself.Fix
Remove the worktree that was named.
wt removedrops non-current worktrees viaRemoveTarget::Path.wt step prunedoes the same: its candidates already carry a path, and targeting a stale entry by branch name resolved to a live worktree that the same prune had just skipped as too young, then removed it.Retain a branch another worktree holds. One predicate,
live_sibling_checkout, answers "would deleting this ref orphan a checkout?", and every path that can delete a branch asks it:prepare_worktree_removal's worktree and pruned-branch-only arms (coveringwt remove,wt step prune, and the picker, which already targeted by path) andwt merge's finish. A hit forcesBranchDeletionMode::Keep, the single chokepoint every deletion path honors, and names the surviving checkout:A sibling whose directory is already gone is stale metadata, not a checkout with anything to lose, so it does not retain: removing the last live checkout still deletes the branch.
-Dis refused out loud. Everywhere else-Dis the override that wins, so one that cannot be honored warns rather than passing quietly:The ordinary single-checkout case is unchanged, and a retained branch skips the integration check entirely rather than computing a verdict it would discard.
#3480's duplicate-checkout hint now points at
wt remove <path>, which this makes the safe answer.Testing
Full gate green. New coverage, each case asserting the survivor still resolves
HEAD, which is the corruption in question:remove: by path, by name, refused-D, the pruned-directory fallback, and the mirror case where a stale sibling must not retain.step prune: a stale entry whose branch is live in an age-skipped worktree. This test is what surfaced the wrong-worktree bug in prune.merge: merging a branch that a--forceduplicate also holds.Not addressed
wt step prune's summary counts candidates rather than outcomes, so a retained branch still reports✓ Pruned 1 branch. The per-item line above it already says the branch was retained. Fixing the count means threading removal outcomes back through prune's accounting, which is a separate change.