Skip to content

fix(prune, remove, ci): count what a removal took, not what it selected - #3633

Merged
max-sixty merged 2 commits into
mainfrom
feat/remove-honor-path
Jul 28, 2026
Merged

fix(prune, remove, ci): count what a removal took, not what it selected#3633
max-sixty merged 2 commits into
mainfrom
feat/remove-honor-path

Conversation

@max-sixty

Copy link
Copy Markdown
Owner

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:

○ 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.

✗ 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

max-sixty and others added 2 commits July 28, 2026 10:46
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 any tool decoding that output as a
string fails on it — which is how `cargo affected` came to abort before
running a test, leaving `affected tests (windows, advisory)` red.

Reproduced with `git show <commit> -- <object>`: invalid UTF-8 at byte
0x95, position 5403. With the attribute set, the same command reports
`Binary files … differ`. The rest of `_git/` (HEAD, config, refs) stays
text worth reading.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
A candidate's kind is a plan, not an outcome. A branch a sibling worktree
still has checked out is retained, so a worktree candidate can take the
worktree and leave the branch standing — and `wt step prune` still
announced `Pruned 1 branch` for a branch that is right there, contradicting
the retention line printed directly above it.

The summary and both `--format=json` payloads now read a removal's planned
outcome through `RemoveResult::deletes_branch`, which `to_json` already
computed inline and now shares. That also fixes a detached worktree
reporting `branch_deleted: true` next to a null branch.

`RemoveTarget::Branch` now means what its three callers already pass: a
branch with no worktree. The arm handling "the branch turned out to have a
worktree" was unreachable at selection time, and the one caller that can
race there — the picker, which re-lists worktrees between selecting a row
and removing it — would have removed a worktree nobody named. It errors and
points at `wt remove <path>` instead.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the removal-surface logic closely (RemoveTarget::Branch in src/commands/repository_ext.rs, the prune counting in src/commands/step/prune.rs, RemoveResult in src/commands/worktree/types.rs). Per the repo's data-loss-surface policy I don't auto-approve changes to the deletion path — the merge is yours — but flagging that I traced it independently and found nothing to fix:

  • RemoveTarget::Branch race guard is sound. wt remove resolves via resolve_worktreeworktree_for_branch, which returns Some for a stale (directory-gone) entry too, so any branch with an entry routes through RemoveTarget::Path; only a genuinely entry-less branch reaches the Branch arm. The deleted !wt.path.exists() prune-and-continue branch was therefore unreachable, and a worktree materializing in the resolve→prepare window is exactly the live race the new bail! refuses. test_prepare_removal_refuses_branch_that_gained_a_worktree covers it.
  • Dry-run vs. actual parity holds. The Prunable predicate wt.branch.is_some() && live_sibling_checkout(...).is_none() matches what prepare_worktree_removal computes on the live path — live_sibling_checkout requires wt.path.exists(), so the stale entry itself is correctly excluded and a retained branch reads "1 worktree", consistent with the per-item retention line.
  • deletes_branch() gate is correct. Adding branch_name.is_some() for RemovedWorktree fixes the detached-worktree "branch_deleted": true beside a null branch, and both JSON callers now share one predicate.

The .gitattributes fix is right too — a small enough loose object has no NUL in the first 8000 bytes, so git diffs it as text and emits raw deflate; marking _git/objects/** and _git/index binary keeps cargo affected from aborting on it.

Nothing actionable — leaving the merge call to you as the removal-surface owner.

@max-sixty
max-sixty merged commit 4c84559 into main Jul 28, 2026
39 checks passed
@max-sixty
max-sixty deleted the feat/remove-honor-path branch July 28, 2026 18:05
max-sixty added a commit that referenced this pull request Jul 28, 2026
…nt (#3637)

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 — `RemoveResult` was produced by
`prepare_worktree_removal` before anything ran, and the codebase
couldn't agree on what it was (`RemovePlans` in remove.rs, `plan` in
prune, `result` in the handler).

## The plan/outcome split

`RemoveResult` → `RemovalPlan` (variant `RemovedWorktree` → `Worktree`),
and `handle_remove_output` now returns a `BranchFate`: `Deleted` /
`Retained` / `NotAttempted` observed synchronously, `Deferred` only for
the detached legacy fallback whose outcome this process never sees.
Consumers report the fate:

- `wt remove --format=json` said `"branch_deleted": true` while 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. Now `false`.
- `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 detached fallback emits the "kept branch" correction in the one
case it can know (no CAS tail to append), instead of letting the
progress message's "worktree & branch" stand uncorrected.

The picker's `foreground: true, silent: true` contradiction collapses
into one `RemovalExecution::{Foreground, Background(fallback), Silent}`
axis, and the four hand-rolled capture-refs-then-CAS-delete sites share
one `worktrunk::git::execute_branch_deletion` — the single spelling of
"the plan said delete; do it now, against fresh refs".

## For the reviewer

`src/output/handlers.rs` carries 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 from `Result<bool>` to `Result<Option<bool>>` so fates flow back
through the done-channel and the collector stamps
`candidate.deletes_branch` before the summary and JSON read it.
Deliberately *not* done: reusing the planner from `wt 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/patch` to flag those
few lines.

> _This was written by Claude Code on behalf of max_

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@max-sixty max-sixty mentioned this pull request Jul 29, 2026
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_
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants