fix: data-safety fixes across the worktree merge/remove/prune lifecycle#2870
Merged
Conversation
`wt merge` and `wt remove` snapshotted the clean check and branch integration decision during planning, then ran approved `pre-remove` hooks before the actual cleanup. A hook (or concurrent process) could dirty the worktree or advance the branch after planning; the fast path then renamed the directory into trash and background safe-delete used the stale integration result to run `git branch -D`. Drop the precomputed `integration_reason` from `RemoveResult::RemovedWorktree` and add `refresh_removal_safety_after_pre_remove`: after pre-remove hooks and before staging/removal, re-run `ensure_clean` (when not `--force`) and recompute the integration reason from a fresh ref snapshot. Branch-only removals keep the precomputed value since they have no pre-remove hook. Adjacent fix: the picker `do_removal` unit test created an untracked `.config/wt.toml` in the worktree; commit it so the worktree is clean, matching the real picker flow where `prepare_worktree_removal` verifies cleanliness before `do_removal` is queued. Adds merge tests covering a pre-remove hook that dirties the worktree (cleanup aborts, worktree and branch preserved) and one that commits a new file (branch retained with the hook commit). clawpatch-finding: fnd_sig-feat-custom-merge-safety-34a_d3b4e2e71c Co-Authored-By: Claude <noreply@anthropic.com>
The background `wt remove` path passed a precomputed branch-deletion decision into the detached removal command. `execute_instant_removal_or_fallback` deleted the branch with an unconditional `git branch -D` on the fast path and synthesized `git worktree remove --force` whenever `.gitmodules` existed on the fallback path — both decided from pre-hook planning state. A `pre-remove` hook (or concurrent process) could advance the branch or dirty the worktree after planning, and the background removal would still force-delete. - `execute_instant_removal_or_fallback` now re-runs `ensure_clean` (when not `--force`) and, on the fast path, recomputes branch integration from a fresh ref snapshot via `delete_branch_if_safe` instead of a bare `branch -D`. - The fallback no longer synthesizes `--force` for `.gitmodules`: a submodule worktree that hits the cross-filesystem fallback fails closed (the fast-path rename, used for same-filesystem removals, is unaffected and still handles submodules). Safe-delete routes through `build_remove_command_safe_delete` (non-forcing `git branch -d`). - Removal parameters bundled into `BackgroundRemoval` so `spawn_background_removal` stays within the argument-count lint. This also closes the prune background-fallback synthesized-force gap (no `--force` is synthesized for submodules anywhere now). Adds remove tests: a `pre-remove` hook that dirties the worktree blocks foreground removal (worktree preserved), and one that adds an unintegrated commit retains the branch through background removal. clawpatch-finding: fnd_sig-feat-custom-remove-3f0bf299e_24947bcfd7 Co-Authored-By: Claude <noreply@anthropic.com>
`wt step prune` runs integration checks in parallel and serializes them against worktree removal via a `check_lock` RwLock — but only for the synchronous fast-path `git branch -D`. When the fast-path rename failed (cross-filesystem, permissions, Windows file locks), prune spawned a detached `git worktree remove && git branch -d` that ran *after* the write guard dropped, so its `.git/config` rewrite could collide with a still-live integration-check reader (issue #2801's residual gap, which `test_prune_fallback_config_race_canary` documents). Add `BackgroundFallbackMode`: `Detached` (default — `wt remove`, `wt merge`, picker) keeps the existing detached fallback; prune now uses `SynchronousForNonCurrent` via `handle_remove_output_serialized_fallback`. For a non-current worktree that hits the fallback, removal and branch deletion run synchronously inside `try_remove`, under the `check_lock` write guard — no integration reader is alive while `git branch -d` rewrites `.git/config`. The current worktree still defers (it references the branch until `git worktree remove` runs, so in-process deletion would fail). `execute_instant_removal_or_fallback` now returns `BackgroundRemovalPlan` (`Detached` command vs `CompletedSynchronously`); the synchronous path routes through `Repository::remove_worktree`, keeping the submodule clean-recheck backstop. clawpatch-finding: fnd_sig-feat-custom-prune-a546eb9c2e_8e41171a00 Co-Authored-By: Claude <noreply@anthropic.com>
`wt step prune` ran `approve_prune_hooks` before the integration, age, and clean checks had selected the removal set. It fed every linked worktree's `pre-remove`/`post-remove` into the approval gate — including unmerged, too-young, and dirty worktrees prune would skip — and always added the primary `post-switch`. Project approvals are persistent and keyed by command template, so a non-interactive prune could abort on an unapproved hook belonging to a worktree outside the operation, or save approval for a command prune never ran. Restructure `step_prune` into explicit phases: - The parallel integration loop now collects candidates and an ordered `PruneEvent` list (`Candidate` / `SkippedYoung`) instead of removing inline — replaying the events during the removal phase preserves the original interleaved output order. - A `can_attempt_candidate_removal` preflight runs `prepare_worktree_removal` per candidate to drop dirty/locked worktrees before approval. - `approve_prune_hooks` now takes the exact `removable` candidate set; primary `post-switch` is added only when a current-worktree candidate is in that set. Adds tests: an unmerged worktree's `pre-remove` is not gated (prune reports no removable worktrees instead of aborting), and a non-current removal does not request `post-switch` approval. clawpatch-finding: fnd_sig-feat-custom-prune-ef964afb5b_e48262ee0e Co-Authored-By: Claude <noreply@anthropic.com>
`gather_check_items` added a `CheckSource::Prunable` entry only when a prunable worktree still had a branch. A detached worktree whose directory was deleted outside Worktrunk has `prunable` set and `branch == None`, so it was silently dropped — no candidate, no `git worktree prune`, and the stale entry lingered in `git worktree list --porcelain` forever. - `CheckSource::Prunable` now carries `wt_idx` so both branched and detached prunable worktrees are kept; the integration ref falls back to `wt.head` when there is no branch. - New `CandidateKind::StaleDetached`: `try_remove` handles it by running `git worktree prune` (idempotent) with no branch deletion and no `prepare_worktree_removal`; it is excluded from the hook-approval set (the directory is gone, so no `pre-remove` runs). - `prune_summary`, `can_attempt_candidate_removal`, and the candidate builder handle the new kind; JSON output reports `kind: stale_worktree`. Adds `test_prune_stale_detached_worktree`: an integrated detached worktree with its directory deleted is pruned from `git worktree list` and no branch is deleted. Also picks up a `cargo fmt` reflow of the Windows `to_wide` helper. clawpatch-finding: fnd_sig-feat-custom-prune-7a58bab83d_36b1c96953 Co-Authored-By: Claude <noreply@anthropic.com>
…y the rebase The first commit of this chain removed `integration_reason` from `RemoveResult::RemovedWorktree`. `test_prepare_removal_refreshes_stale_detached_worktree_cache` was added to picker/mod.rs after this chain's base (alongside #2845's `prepare_removal`), so the cherry-pick's auto-merge left its constructor passing the now-removed field. Drop it — `RemovedWorktree` no longer carries a precomputed integration reason. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
One observation on the new sync fallback in prune — the rest of the chain looks solid. The post-hook revalidation in refresh_removal_safety_after_pre_remove, the removed .gitmodules-implies-force shortcut (now delegated to remove_worktree's in-process re-check), and the narrowed approval set with can_attempt_candidate_removal all close the planning-time / use-time gaps cleanly. New integration coverage tracks each fix's failure mode.
The inline question on delete_branch_in_synchronous_fallback is about whether prune candidates that the fast path would have force-deleted (worktrunk's broader integration check, e.g. squash-merge / patch-id match) get silently retained when the rename falls back.
Collaborator
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
Required test (windows) failed on two new/updated prune tests — both PR-related, not flakes. Details inline.
test_prune_stale_detached_worktree— straightforward path normalization mismatch on Windows. Fix below.test_prune_fallback_config_race_canary— the newbranch_delete_marker.exists()assertion fails on Windows. The wrappergit.cmddoesn't appear to be invoked. Two plausible causes are worth ruling out; comment inline.
Two Windows-only `test (windows)` failures from this branch's new `step_prune` tests, plus the `codecov/patch` gap they left: - `test_prune_stale_detached_worktree` compared the JSON `path` field against `std::fs::canonicalize`, which on Windows yields a `\\?\` verbatim, backslash-separated path while git (and so `wt`'s `WorktreeInfo::path`) reports a forward-slash one. Derive the expected value from `git worktree list --porcelain` instead — git's own string, byte-for-byte what `wt` emits — and drop the `std::fs::canonicalize` call (also satisfies the `no-std-canonicalize-in-tests` hook). - `test_prune_fallback_config_race_canary` shimmed `git` with a `git.cmd` batch file on Windows, but Rust's `Command` resolves a bare program name through `CreateProcess`, which appends only `.exe` and never finds a `.cmd`/`.bat` (the same reason `mock_commands` ships a real `mock-stub.exe`). The shim was never invoked, so the `branch_delete_marker` assertion was unsatisfiable. Gate the shim and its marker assertion to Unix; Windows still exercises the fallback via the pre-blocked staged path and the synchronous-completion assertion. Refresh the stale docstring that still framed the test as a deliberate Windows flake canary. Coverage for the fallback paths this branch added: - New `test_remove_background_fallback_force_delete_branch` and `test_remove_background_fallback_detached_worktree` cover the `ForceDelete` and detached (`_`) arms of the legacy fallback command builder; the existing sibling test only covered `SafeDelete`. - New `test_prune_detached_worktree_rename_fallback` covers the branch-less arm of `delete_branch_in_synchronous_fallback`. - New `prune_summary` / `remove_target` unit tests cover the `StaleDetached` `CandidateKind` arms. Adjacent cleanup in the same code: - `delete_branch_in_synchronous_fallback` dropped its `deletion_mode` parameter: its sole caller (`wt step prune`) always safe-deletes, so the `should_keep` / `-D` handling was dead. Hardcode `git branch -d`. - `prune_summary` matches on `CandidateKind` alone, removing an `unreachable!()` arm for the impossible `(StaleDetached, Some(branch))` state. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
worktrunk-bot
approved these changes
May 21, 2026
The PR added `handle_remove_output_serialized_fallback` as a second public entry point alongside `handle_remove_output`, both forwarding to a private `handle_remove_output_with_fallback_mode` — three function signatures (6-7 params each) expressing "one function plus a `BackgroundFallbackMode`". Collapse to a single `handle_remove_output` taking `background_fallback_mode` explicitly. The four detached call sites (`wt remove` ×2, picker, `wt merge` finish) pass `BackgroundFallbackMode::Detached`; `wt step prune` passes `SynchronousForNonCurrent`. `BackgroundFallbackMode` becomes `pub` with per-variant docs. Also drop the single-use `impl From<String> for BackgroundRemovalPlan` so both `Detached` construction sites use the explicit variant, and replace a now-stale comment in `try_remove` (the call no longer fits on one line regardless of how its arguments are bound). Net: -63 lines in handlers.rs, one canonical removal-output entry point. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
# Conflicts: # src/commands/picker/mod.rs # src/commands/step/prune.rs # tests/integration_tests/remove.rs # tests/integration_tests/step_prune.rs
The sync fallback at handlers.rs called plain `git branch -d`, which only checks reachability from HEAD. Squash-merged or patch-id-matched branches that prune accepted as candidates (and the fast path correctly deletes via `delete_branch_if_safe`) would error out here, silently retaining the branch while the worktree was already removed. Mirror the fast path: re-capture refs after `remove_worktree`, gate on `Some(branch) && !deletion_mode.should_keep()` at the caller, and call `delete_branch_if_safe` so both paths reach the same outcome. The canary test's git shim was matching only `-d`; widened to `-d`/`-D` because `delete_branch_if_safe` uses `-D` for integrated branches. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
worktrunk-bot
approved these changes
May 23, 2026
max-sixty
added a commit
that referenced
this pull request
May 25, 2026
Three follow-ups to #2870 collapse into one rewrite of `delete_branch_if_safe`: it now performs an atomic compare-and-swap delete via `git update-ref -d refs/heads/<branch> <expected-sha>`, where the expected SHA comes from the snapshot the integration check already consulted. If the ref moves between the integration check and the delete (a hook, a concurrent push), git rejects the CAS — the branch is retained (fail-closed) and we surface the new `BranchDeletionOutcome::RetainedRaced` outcome rather than dropping the unmerged commits silently. Force-delete still uses `git branch -D` (the user's explicit override). This unifies the divergent safe-delete semantics in `execute_instant_removal_or_fallback`: - Fast path and `SynchronousForNonCurrent` fallback already routed through `delete_branch_if_safe`, so they pick up CAS for free. - Detached fallback used to bake `&& git branch -d <branch>` into the shell, which only honored git's reachability-from-HEAD check (so squash-merged / patch-id / ancestor branches the planner accepted as integrated were rejected at delete time). Now `build_cas_branch_delete_tail` runs worktrunk's full integration check in the foreground and, if integrated, appends an atomic `git update-ref -d <ref> <expected-sha>` to the detached shell — same semantics as the other paths, plus CAS safety against tip movement between the foreground check and the detached delete. - `handle_branch_only_output` also switches its safe-delete path from `git branch -D` (an unsafe force-delete after a stale integration check) to `delete_branch_if_safe`, so the CAS protects branch-only deletions the same way. The display layer absorbs the new variant: `flag_note` treats `RetainedRaced` like `NotDeleted` for the success badge, `handle_branch_deletion_result` shows the unmerged hint, and the branch-only output path emits a dedicated "moved during deletion" warning with a `wt remove --force-delete <branch>` recovery hint. CAS protects the TOCTOU window inside `delete_branch_if_safe` (between its own snapshot capture and `update-ref -d`). Hook-driven races are still caught by the existing fresh integration check that runs after pre-remove hooks; CAS narrows the residual unprotected window from \"between check and delete\" to \"never\". Tests: `git update-ref -d <ref> <sha>` is structured — exit 0 on deletion, non-zero on stale-SHA rejection (\"cannot lock ref ... is at X but expected Y\"). The CAS helper re-checks ref presence via `rev-parse --verify --quiet` rather than parsing the error message, keeping with the structured-output policy. Two new unit tests in `git::remove::tests`: - `cas_rejects_delete_when_branch_advances` — captures a snapshot, moves the branch externally, then calls `delete_branch_if_safe` and asserts `RetainedRaced` plus surviving tip at the post-race SHA. Pins the CAS rejection mechanism. - `cas_deletes_when_branch_unchanged` — sanity check the common integrated case still deletes. `test_prune_fallback_config_race_canary`'s wrapper-git script now matches `update-ref -d refs/heads/<branch>` alongside the `branch -d|-D <branch>` shapes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
3 tasks
max-sixty
added a commit
that referenced
this pull request
May 25, 2026
Three follow-ups to #2870 collapse into one rewrite of `delete_branch_if_safe`: it now performs an atomic compare-and-swap delete via `git update-ref -d refs/heads/<branch> <expected-sha>`, where the expected SHA comes from the snapshot the integration check already consulted. If the ref moves between the integration check and the delete (a hook, a concurrent push), git rejects the CAS — the branch is retained (fail-closed) and we surface the new `BranchDeletionOutcome::RetainedRaced` outcome rather than dropping the unmerged commits silently. Force-delete still uses `git branch -D` (the user's explicit override). This unifies the divergent safe-delete semantics in `execute_instant_removal_or_fallback`: - Fast path and `SynchronousForNonCurrent` fallback already routed through `delete_branch_if_safe`, so they pick up CAS for free. - Detached fallback used to bake `&& git branch -d <branch>` into the shell, which only honored git's reachability-from-HEAD check (so squash-merged / patch-id / ancestor branches the planner accepted as integrated were rejected at delete time). Now `build_cas_branch_delete_tail` runs worktrunk's full integration check in the foreground and, if integrated, appends an atomic `git update-ref -d <ref> <expected-sha>` to the detached shell — same semantics as the other paths, plus CAS safety against tip movement between the foreground check and the detached delete. - `handle_branch_only_output` also switches its safe-delete path from `git branch -D` (an unsafe force-delete after a stale integration check) to `delete_branch_if_safe`, so the CAS protects branch-only deletions the same way. The display layer absorbs the new variant: `flag_note` treats `RetainedRaced` like `NotDeleted` for the success badge, `handle_branch_deletion_result` shows the unmerged hint, and the branch-only output path emits a dedicated "moved during deletion" warning with a `wt remove --force-delete <branch>` recovery hint. CAS protects the TOCTOU window inside `delete_branch_if_safe` (between its own snapshot capture and `update-ref -d`). Hook-driven races are still caught by the existing fresh integration check that runs after pre-remove hooks; CAS narrows the residual unprotected window from \"between check and delete\" to \"never\". Tests: `git update-ref -d <ref> <sha>` is structured — exit 0 on deletion, non-zero on stale-SHA rejection (\"cannot lock ref ... is at X but expected Y\"). The CAS helper re-checks ref presence via `rev-parse --verify --quiet` rather than parsing the error message, keeping with the structured-output policy. Two new unit tests in `git::remove::tests`: - `cas_rejects_delete_when_branch_advances` — captures a snapshot, moves the branch externally, then calls `delete_branch_if_safe` and asserts `RetainedRaced` plus surviving tip at the post-race SHA. Pins the CAS rejection mechanism. - `cas_deletes_when_branch_unchanged` — sanity check the common integrated case still deletes. `test_prune_fallback_config_race_canary`'s wrapper-git script now matches `update-ref -d refs/heads/<branch>` alongside the `branch -d|-D <branch>` shapes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
2 participants
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.
Five data-safety / correctness fixes in the worktree-lifecycle paths, plus one rebase-integration follow-up. Each closes a real TOCTOU or scoping gap and carries integration tests; together they form a dependency chain through
output/handlers.rsandstep/prune.rs.merge/remove— revalidate cleanliness afterpre-removehooks.wt merge/wt removesnapshotted the clean check and branch-integration decision during planning, then ranpre-removehooks before cleanup. A hook (or concurrent process) could dirty the worktree or advance the branch in between; the stale decision then trashed the directory or rangit branch -D. Cleanliness and integration are now recomputed from a fresh ref snapshot after hooks run.remove— recompute branch safety in the background path. The background removal path also acted on pre-hook planning state, and synthesizedgit worktree remove --forcewhenever.gitmodulesexisted. It now re-runsensure_clean, recomputes integration viadelete_branch_if_safe, and fails closed for submodule worktrees on the fallback path instead of forcing.prune— serialize the rename-failure fallback. When the fast-path rename failed, prune spawned a detachedgit branch -dthat ran after the lock guard dropped, racing a live integration-check reader on.git/config(wt step prune: Windows .git/config Permission-denied race (parallel integration checks vs background removals) #2801's residual gap). It now runs synchronously under the write guard for non-current worktrees.prune— scope hook approval to the removal set. Approval was built from every linked worktree (including ones prune would skip) plus an always-on primarypost-switch, so a non-interactive prune could abort on an unrelated unapproved hook. Approval is now built only for the worktrees prune will actually remove.prune— prune stale detached worktree metadata. A detached worktree whose directory was deleted outside Worktrunk (branch == None) was silently skipped and lingered ingit worktree listforever; it is now pruned via an idempotentgit worktree prune.The final commit drops a struct field — removed by the first commit — from a test constructor that landed in
picker/mod.rsafter this chain's base (#2845), which the cherry-pick's auto-merge did not catch.