refactor(remove): report a removal's branch fate, not its plan's intent - #3637
Merged
Conversation
`prepare_worktree_removal` returns this type before anything has run, and the executor consumes it to do the work — it is a plan, and the codebase already couldn't agree: `remove.rs` stored it in `RemovePlans`, prune named the field `plan`, the output handler received it as `result`. The past-tense name is what let plan intent pass for outcome (the prune summary counting kinds, `branch_deleted` reported from a plan that hadn't run). Name it what it is, including the `RemovedWorktree` variant, which becomes `Worktree`. Mechanical; no behavior change. The library's `RemovalOutput` (what `remove_worktree_with_cleanup` actually did) is untouched — the pair now reads plan in, output back. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
The executor ran a plan and returned `()`, so everything downstream reported intent as if it were outcome: `wt remove --format=json` said `branch_deleted: true` for an unmerged branch SafeDelete then declined, and a CAS-refused deletion mid-`wt step prune --foreground` was still counted in the summary. The picker meanwhile spelled "inline but quiet" as `foreground: true, silent: true`. `handle_remove_output` now takes one `RemovalExecution` (Foreground / Background(fallback) / Silent) in place of the three flags, and returns a `BranchFate`: `Deleted` / `Retained` / `NotAttempted` observed synchronously, `Deferred` only for the detached legacy fallback, where `deleted()` falls back to the plan's intent. Prune's summary and both commands' JSON read the fate, so they state what happened; the dry run keeps predicting, which is its job. The four synchronous capture-refs-then-CAS-delete sites (background fast path, prune's synchronous fallback, branch-only removal, the picker row) now share one primitive, `execute_branch_deletion` — the single spelling of "the plan said delete; do it now, against fresh refs". The detached fallback keeps its `update-ref -d <ref> <sha>` shell tail, and its fate is sharper than before: a tail the integration check declined to build means the branch is known retained, not unknown. No output or JSON changes for any plan whose execution matches its intent — the existing suite passes untouched; the divergent cases gain tests in the follow-up commit. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
With outcomes threaded, an orphan branch (no worktree entry) whose SafeDelete was declined mid-run — a concurrent writer advanced it between scan and delete — would have counted as "Pruned 1 worktree": a candidate where nothing was removed at all. (Intent-counting told a different lie, "1 branch".) `try_remove` now excludes the no-op from the removed list; the per-item retention warning already narrates it. A stale-entry candidate that kept its branch still counts as a worktree — the entry really was pruned. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ts a deletion The deterministic divergence between a removal plan and its execution: a `pre-remove` hook that commits a file on the branch, so the SafeDelete's re-check against fresh refs finds it unmerged and declines while the worktree still goes. (An empty commit doesn't work — `NoAddedChanges` reads it as still integrated — and since the executor captures refs after the hook, this exercises the re-check half of the divergence, not the CAS half, which the library's snapshot-skew unit tests cover.) `wt remove --format=json` must report `"branch_deleted": false` with the "kept branch" warning on stderr; `wt step prune` must summarize `Pruned 1 worktree`, not `worktree & branch`. Both tests assert the hook's commit is the branch tip, so a hook that never fired fails the test instead of passing on a coincidentally retained branch. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
When the rename-into-trash fast path fails and the SafeDelete re-check declines to build a CAS tail, the detached process won't touch the branch — its survival is known in the foreground. The progress message may already have promised "worktree & branch" (planner predicted deletion), and `--format=json` reports `branch_deleted: false`, so stderr now gets the same correction the fast path emits, through the same `warn_if_branch_retained` emitter. Also, from review of the outcome-threading change: - `BranchFate::deleted()` drops its plan parameter: `Deferred` occurs only where a deletion was planned (every fallback arm maps `Keep` and branchless plans to `NotAttempted`), so the intent fallback was constant-true and the parameter coupled the types for nothing. The invariant moves to the variant doc. - The executor spec overstated "a surviving branch is a fate, not an error": that holds for `Worktree` plans, where the removal is the primary operation; a `BranchOnly` plan's deletion is the whole operation, so a hard command failure still propagates. Said so. - `execute_branch_deletion`'s doc claimed every synchronous site routes through it; the foreground path captures its own fresh snapshot. The doc now names the invariant that is universal — no deletion runs against the planning-time snapshot — and where each path gets it. - Stale references from the rename (`plan` → `hook_plan`, a `RemovedWorktree` mention) and rustfmt reflow of the prior commits. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # src/commands/step/prune.rs # tests/integration_tests/step_prune.rs
worktrunk-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the full diff and traced the plan/outcome split end-to-end — no correctness issues found. Notes for the record:
- The four hand-rolled capture-refs-then-CAS sites collapse into
execute_branch_deletionequivalently; the.and_then→?swap preserves the "capture failure ⇒Err, never a lost commit" contract. The one behavior nudge is the picker'sdo_removal, where acapture_refs()failure now routes into the row's error arm instead of being silently skipped — a strict improvement. - The reporting change is the substance:
to_json/prune summary now readBranchFate(observed) rather thandeletes_branch()(intent).Deferred.deleted() == truekeeps the pre-existing detached-fallback behavior (intent, since the outcome is unobservable), so no regression there; the SafeDelete-no-tail arm correctly flips toRetained. The multi-remove.zip(fates)aligns because the fateVecis pushed in the same others→branch-only→current order the JSON chains. - Divergence cases are pinned by the two new tests whose hook must land as the branch tip to pass — good coverage for exactly the intent≠outcome path this PR exists to fix.
Holding the merge decision for a human rather than auto-approving: this lands on worktrunk's data-loss surface — src/output/handlers.rs (std::fs::remove_dir_all, git branch -D, git worktree remove) and src/git/remove.rs (the removal engine, new execute_branch_deletion). Per the repo's review policy that surface isn't an agent's to sign off on, even when the change reads as reporting-only. The behavior of the actual deletions looks unchanged to me, but a human should confirm before merge.
Three of the patch's missed regions have deterministic triggers after all:
- The detached-fallback warning: plant a *file* where `.git/wt/trash`
belongs, so `stage_worktree_removal`'s rename cannot stage on any OS
and the legacy fallback runs; the surviving file afterwards is what
discriminates the fallback from the fast path, which prints the same
warning. Combined with the hook-divergence setup, this pins the
no-CAS-tail arm: warned on stderr, `branch_deleted: false` in JSON.
- The declined-orphan exclusion: a carrier worktree's `pre-remove` hook
points the orphan branch at a commit main lacks, and
`RAYON_NUM_THREADS=1` makes the ordering causal (serial scan queues the
carrier first; the FIFO worker runs its hook before the orphan's
deletion). The exact summary line asserts the no-op contributes
nothing, in either currency it could be miscounted in.
- The picker's branch-only arm: `do_removal` on a `BranchOnly` plan
deletes through `execute_branch_deletion`.
Still uncovered, by design: `background_fallback()`'s Foreground/Silent
default arm (unreachable — the dispatcher returns Silent early and
branches Foreground away before any background path) and the two
`panic!("Expected Worktree variant")` arms in test helpers.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
The pre-merge gate's fmt hook (pinned 1.96.0) reformatted this after the tree was staged, so the commit carried the pre-hook form while the working tree held the fix — the hook-modifies-after-add trap. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Merged
max-sixty
added a commit
that referenced
this pull request
Jul 29, 2026
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_
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
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 #3633, which patched the most visible symptom (prune's summary counting candidate kinds). This lands the structural fix behind that class: the removal chain returned nothing from execution, so everything downstream reported the plan's intent as if it were the outcome. The type even said so —
RemoveResultwas produced byprepare_worktree_removalbefore anything ran, and the codebase couldn't agree on what it was (RemovePlansin remove.rs,planin prune,resultin the handler).The plan/outcome split
RemoveResult→RemovalPlan(variantRemovedWorktree→Worktree), andhandle_remove_outputnow returns aBranchFate:Deleted/Retained/NotAttemptedobserved synchronously,Deferredonly for the detached legacy fallback whose outcome this process never sees. Consumers report the fate:wt remove --format=jsonsaid"branch_deleted": truewhile stderr said▲ Removed worktree but kept branch feature (not integrated)— a pre-remove hook had advanced the branch, the SafeDelete re-check declined, and stdout reported the plan anyway. Nowfalse.wt step prune's summary counts executed outcomes, so a deletion declined mid-run reads✓ Pruned 1 worktree, matching the per-item line above it. A declined orphan deletion (no worktree entry, nothing removed at all) now drops out of the removed list entirely instead of being counted as either lie.The picker's
foreground: true, silent: truecontradiction collapses into oneRemovalExecution::{Foreground, Background(fallback), Silent}axis, and the four hand-rolled capture-refs-then-CAS-delete sites share oneworktrunk::git::execute_branch_deletion— the single spelling of "the plan said delete; do it now, against fresh refs".For the reviewer
src/output/handlers.rscarries the bulk: each execution path constructs its fate where it observes the result, and rendering decisions (foreground propagates deletion errors, background warns, silent does neither) are unchanged. The merge with #3631's concurrent prune rework is the diff's riskiest region — the worker result channel widened fromResult<bool>toResult<Option<bool>>so fates flow back through the done-channel and the collector stampscandidate.deletes_branchbefore the summary and JSON read it. Deliberately not done: reusing the planner fromwt merge's finish path (its divergences — clean-check strictness, preserve-vs-error on primary, merge-target — are intentional), and threading the fallback mode structurally (it just relocates an unreachable arm).Zero snapshot churn: the change is provably invisible wherever intent matched outcome. The divergence cases are pinned end-to-end by tests whose hook must be the branch tip to pass. The one deterministically unreachable arm is the detached-fallback warning (needs a cross-filesystem rename failure); expect
codecov/patchto flag those few lines.