feat(worktree): accept a worktree path wherever a branch is accepted - #3607
Conversation
wt had three answers to "what does this token mean?". `wt switch` took shortcuts, `pr:`/`mr:`, and a path — but only an absolute one or one with a separator. `wt remove` took shortcuts and any path. Everything else took a bare branch name, so `wt step diff --branch @` failed and `wt config state marker --branch @` silently keyed state under `@`. Fold the two path routes into one canonicalizer, `Repository::resolve_worktree`, and route every worktree-naming argument through it: branch first, then the worktree's own path, so a directory never shadows a branch that shares its name and a path names what a branch cannot — a detached worktree, or one of two checkouts of the same branch. `require_worktree` and `require_selected_branch` cover the two shapes callers need; the merge and rebase target validators fall through to the same path lookup, so a target can be named by the worktree it is checked out in. `resolve_input_path` now expands a leading `~`, making the tilde form worktrunk prints its own paths in a form it also reads back. Two silent no-ops go with it: `wt step relocate <unmatched>` reported "all worktrees are at expected paths" for work that never happened, and a selector matching nothing was reported as a branch without a worktree, hinting at a `wt switch` that would only fail again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.
Read the whole change end-to-end; the canonicalizer design (resolve_worktree branch-first, with require_worktree/require_selected_branch splitting the detached-worktree cases) is clean and the coverage is thorough. No defects found on this pass.
One note per the repo's data-safety convention, since the diff touches the removal surface (src/commands/remove.rs swaps resolve_worktree_arg → repo.resolve_worktree, and the new resolver lives alongside remove_worktree/--force in worktrees.rs): I traced the removal path specifically, and wt remove's resolution is behaviorally identical to before — the old resolve_worktree_arg logic (branch-first, then worktree_at_path(resolve_input_path(name))) is exactly what resolve_worktree now does, and the new branch == name guard only changes the shortcut arms (-/^), which never reached the path fallback anyway. So the widening doesn't alter which worktree wt remove <token> selects. Flagging it only so the deletion-surface change is visible for the eventual full review.
# Conflicts: # src/git/repository/tests.rs
The first pass added "or worktree path" to sixteen argument descriptions, which asserts a co-equal addressing scheme sixteen times — the opposite of the model, where a branch names a worktree and its path is the alias for what a branch cannot name. It also pushed several first lines past the 3-6 word guidance in src/commands/CLAUDE.md, and rewrote nine `wt config state` lines that were about a *target* branch. Restore every argument description, and state the alias where the model already lives: one paragraph in `wt switch`'s help, one sentence on the addressing line in worktrunk.md. `wt switch`'s own argument keeps the mention — its description is already the catalogue of accepted forms — as does `wt remove`, which has named the path since before this branch. The Worktree Model section now says which way to document it, so the next argument doesn't grow its own copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eight lines of the diff were unhit; two of the four causes are real and deterministically reachable: - `wt step relocate <prunable>` — the arm that rejects a worktree whose directory is gone. The detached arm beside it had a test; this one was only verified by hand. - `wt step push <worktree-path>` — `require_target_branch`'s path fallback. Its sibling `require_target_ref` was covered through `wt step diff`, so the merge/push half went untested. The other two aren't worth contorting for: `home_dir()` returning `None` (twice, including a test's own skip guard) has no deterministic trigger, and the `@`-resolution `NotInWorktree` arm is unchanged logic that only re-indented when its `match` became an `if`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…p guard
`resolve_worktree` gained its path fallback by turning the `match` into an
early-return `if`, which re-indented the untouched `@` arm and so entered
it into the diff as new, uncovered lines. It is a backstop no CLI route
reaches — `wt remove @`, `wt step diff --branch @`, and `wt config state
marker --branch @` in a bare repo all fail earlier — so the honest fix is
to leave the arm alone rather than test around it. The `match` returns;
the new code lives in the `_` arm.
The `expand_tilde` test's `let Some(home) = home_dir() else { return }`
guard is likewise unhit, and a skipped assertion is worse than a failed
one: every platform the suite runs on sets HOME or USERPROFILE, so it
expects instead.
That leaves `expand_tilde`'s own no-home fallback as the single unhit
line, which has no deterministic trigger in-process.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
Full review now that it's out of draft — read the whole change end-to-end. No defects found.
The consolidation is clean: one canonicalizer, branch-first, path as an alias. I traced the paths that carry the load:
- The
branch == namepath-fallback guard inresolve_worktreeholds becauseresolve_worktree_namereturns non-shortcut tokens unchanged, so the guard excludes exactly@/-/^. The decoy-directory tests (^and same-nameddocs/) pin that a directory can't shadow a branch or a shortcut. require_selected_branchvsrequire_worktreesplit the detached (branch: None) and no-checkout cases into the right errors, andWorktreeSelectorNotFoundvsWorktreeNotFoundcorrectly gates the "create a worktree" hint — a mistyped path no longer gets awt switchsuggestion that would only fail again.expand_tildeis component-wise (~user,repo~,sub/~/repostay literal;~and~/…expand), andresolve_input_pathexpanding before the-Cjoin is safe since an expanded~is absolute.- The relocate loop genuinely fixes the silent no-op — unmatched / detached / prunable now error instead of rendering "all worktrees are at expected paths."
resolve_base_ref/target_branch_at_pathrun the path fallback only after ref (and single-remote) lookups fail, so refs still win.
One governance note, not a request for changes: the diff touches the deletion surface — wt remove in src/commands/remove.rs now resolves via repo.resolve_worktree, and the new resolver sits alongside remove_worktree in src/git/repository/worktrees.rs. I traced wt remove's selection and it's behaviorally identical to the old resolve_worktree_arg (branch-first, then the same resolve_input_path path fallback); the widening only changes the shortcut arms, which never reached the path lookup anyway. Per the repo's data-safety convention I'm holding the approval rather than rubber-stamping a removal-surface change — it's yours to merge.
…ir own, and make a PTY failure explain itself (#3619) Chasing the Windows-only failure of `test_switch_picker_runs_execute_command` that @max-sixty flagged after #3607 merged: it failed the advisory `affected tests (windows)` leg while the required `test (windows)` leg **passed the same test on the same SHA** in the same run. ## The failure message was the whole problem ``` assertion `left == right` failed: Expected exit code 0 for picker switch with --execute left: 1 right: 0 ``` That is everything CI had. wt prints an error before exiting non-zero, so there was a message — it just never reached the captured frame, because of two harness gaps: - **The post-exit drain was a single non-blocking sweep**, taken the instant `try_wait` reported the child reaped. The child's last writes can still be in the PTY at that moment, so the tail — wt's `✗` line — was dropped. This is also why #3427 concluded a picker-create "self-exited 1 after fully-correct work with no error on screen": the error was there, just not drained. - **`child.kill()` ran unconditionally after the exit poll.** On Windows that is `TerminateProcess(proc, 1)`, so a hung picker and a picker that failed on its own both report exit 1 — the ambiguity that made #3427 reason from wall-clock time to rule out a kill. Both are fixed here: drain until the output goes quiet, and panic on a hang rather than killing silently, so every exit code a caller asserts on is one the child chose. The four directive tests now also dump the screen (and the EXEC file) in their exit-code assertions. ## What reproducing it turned up Loop-based reproduction of the reported symptom is hopeless by the numbers: across the last 38 PR runs, exactly one advisory job failed — this one — i.e. ~1 failure per 160,000 test executions. What *did* reproduce was a different Windows failure in the same five tests, at a rate 1000× higher, once the loop ran with realistic neighbours (the full suite running alongside, as the failing leg had at test 3111/4204): ``` thread '…switch_picker::test_switch_picker_emits_cd_directive_by_default' panicked at tests\integration_tests\switch_picker.rs:2531:45: failed to create cd temp file: … PathError { path: "D:\\tmp\\.tmp3XI9wb", err: Os { code: 5, kind: PermissionDenied, message: "Access is denied." } } ``` **4 of 80 runs of the five directive tests.** `directive_files()` called `NamedTempFile::new()` in the shared temp directory; `tempfile` retries a name collision only when it surfaces as `AlreadyExists`, and on Windows `create_new` against a name held by a *directory* — or by a file in delete-pending state — returns `PermissionDenied`, which it hands back to the caller. A full suite run fills that directory with `.tmpXXXXXX` entries (every `TestRepo` makes one), so the collision is routine rather than astronomical. The helpers now take a `TempDir` and put fixed-named files inside it. `TempDir::new` isn't exposed to the same problem — a directory collision surfaces as `AlreadyExists`, which tempfile retries — and fixed names inside a private directory can't collide with anything. That covers all 75 call sites of `directive_files()` / `legacy_directive_file()`, not just the picker's five, and the switch_picker copy of the helper is gone. ## The reported exit-1 is not proven fixed Stated plainly: the panic above is a *different symptom* from the reported exit 1, so this PR does not prove that failure is gone. What the evidence does say, and what a reader should check first if it recurs: - The failing run took 3.34s where the same test passing on the required leg took 3.11s and the same switch *without* `--execute` takes 2.42s. The `--execute` tail — `build_hook_context`'s git lookups, ~0.7s of the difference — had therefore run, so the failure was at the end of the pipeline, not in the picker or the switch. - After that tail, the only fallible step left is the EXEC directive append. Every other step is infallible or `.ok()`-swallowed. - That write's error was context-free, which is why nobody could place it. `wt` rendered `✗ No such file or directory (os error 2)` — no file, no operation, and positioned right under `◎ Executing (--execute):`, where it reads as if the *command* were missing. It now reads: ``` ✗ Failed to write the command to the directive file /tmp/tmp.aBc No such file or directory (os error 2) ``` The message lives in `append_line` / `write_cd_path` rather than at their four call sites, so every directive write says which file and what didn't get written — including the legacy-protocol arms, which no test can drive to failure. So the next occurrence prints its own diagnosis, in one look, instead of `left: 1, right: 0`. ## Also here **The four directive tests select their row by cursor, not by a typed query.** They typed `target` and waited for `target-branch`'s preview pane before Enter. That gate can only assert what skim *painted*: the filtered item list is swapped into `ItemList` during a render, while `Accept` reads `item_list.selected()` — the cursor's slot in whatever list is current at that moment. The list-pane `>` pointer comes from the same render state the accept reads, which is why this file's own `wait_for_cursor_on_row` calls it the race-free signal. One Down reaches `target-branch` (the picker sorts the current worktree first) and the arrow is re-issued until the pointer settles, so no query — and no matcher — is in the accept path at all. It also drops the wait on the preview pane rendering. ## Testing - Reproducer and confirmation both on Windows CI, via a temporary workflow (deleted in the last commit): 80 runs of the five directive tests with the full suite running alongside. **4 failures before the fix, 0 after.** The concurrent full suite passed 4208/4208 in both. - Local: `switch_picker` (68), plus every suite that calls these helpers — `directives`, `switch`, `remove`, `security`, `bare_repository`, `step_alias`, `user_hooks`, `step_relocate` (861 tests) — green, plus clippy `-Dwarnings` on all targets with `--features shell-integration-tests`. - The two directive-write messages are pinned by tests of their own (`test_exec_directive_write_failure_names_the_file`, `test_cd_directive_write_failure_names_the_file`): an unwritable directive path fails the same way on every platform, so the paths that produce these messages are covered rather than checked by hand. ## Follow-ups, not in here - `build_hook_context` is called with `referenced: None` on the `--execute` path, so a `-x 'echo hi'` with no template variables still runs the `default_branch`, `primary_worktree`, `commit`, `short_commit`, `remote`, and `upstream` lookups — roughly doubling the command's subprocess count (30ms → 60ms locally, 2.42s → 3.11s in the Windows test timings). The alias path already passes its referenced set; a `--execute` command receives no JSON context, so the reason hooks pass `None` doesn't apply to it. - `cancel_background_commands()` is a no-op on Windows (`signal_background_pid`), so the picker's "the picker's exit cancels the unfinished request" promise isn't kept there: preview git children keep reading `.git/config` while the accept path writes `worktrunk.history` and `worktrunk.default-branch` to it. Issue #2564 is the same shape of concurrent-config-access failure on Windows. - `assert_valid_create_exit_code`'s Windows tolerance (#3427) and `assert_valid_abort_exit_code`'s acceptance of `1` both rest on the kill-vs-self-exit ambiguity this PR removes; they can be tightened once a Windows run confirms nothing else produces a stray 1. > _This was written by Claude Code on behalf of Maximilian Roos_
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>
…ranches (#3533) Follow-up to the discussion on #3480: [@max-sixty asked](#3480 (comment)) to have `wt remove` honor 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 remove` threw the resolved answer away. For any non-current worktree it re-targeted by branch name, and `prepare_worktree_removal` mapped that branch back to git's *first-listed* worktree. Two failures followed, both silent. **The wrong worktree is removed.** With `feature` checked out twice: ```console $ git worktree list …/dup [feature] …/first [feature] $ wt remove …/first ◎ Removing feature worktree & branch in background (same commit as main, _) ``` `…/dup` is 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 unlike `git branch -d` does not refuse a ref that is checked out somewhere: ```console $ git worktree list …/first 0000000 [feature] $ git -C …/first rev-parse HEAD fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree. ``` `wt step prune` reached the same deletion unattended, and `wt merge` reached it with a freshly integrated branch, so nothing else declined. A branch gets a second worktree only through `git worktree add --force`; worktrunk never does it itself. ## Fix **Remove the worktree that was named.** `wt remove` drops non-current worktrees via `RemoveTarget::Path`. `wt step prune` does 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 (covering `wt remove`, `wt step prune`, and the picker, which already targeted by path) and `wt merge`'s finish. A hit forces `BranchDeletionMode::Keep`, the single chokepoint every deletion path honors, and names the surviving checkout: ```console $ wt remove …/dup ◎ Removing feature worktree in background ○ Branch feature retained; still checked out @ …/first ``` 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. **`-D` is refused out loud.** Everywhere else `-D` is the override that wins, so one that cannot be honored warns rather than passing quietly: ```console $ wt remove …/dup -D ◎ Removing feature worktree in background ▲ Branch feature retained despite -D; still checked out @ …/first ``` 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 `--force` duplicate 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. > _This was written by Claude Code on behalf of Maximilian Roos_ --------- Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Co-authored-by: Maximilian Roos <m@maxroos.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the
wt remove <path>discussion on #3480, widened from that one command to the whole surface. #3480 has since landed and is merged in here — its duplicate-checkout warning composes with this: the warning names the shadowed worktrees, and a path is how you then address one.Audit
Verified against the built binary. wt had three answers to "what does this token mean?":
@-^pr:Nwt switch(resolve_switch_target)--createwt remove(resolve_worktree_arg)worktree_for_branch)Same token, same cwd, two answers:
And outside switch/remove the shortcuts didn't work at all —
wt step diff --branch @was✗ Branch @ has no worktree, whilewt config state marker set --branch @silently wrote state under the literal key@. Separately, wt prints paths as~/…but wouldn't accept that form back.Change
One canonicalizer in the lib,
Repository::resolve_worktree, absorbing the path fallback that lived in the bin crate'sresolve_worktree_arg(now deleted). Resolution order is documented once, on that function:@, then-/^, then a branch with a worktree, then a path naming a registered worktree, then the branch alone.Branch-first, everywhere. A directory never shadows a branch that shares its name; a path answers only what a branch cannot — a detached worktree, or one of two checkouts of the same branch (#3480's case). The
looks_like_pathshape gate is gone, so a single-component path resolves like any other.Two shapes cover what callers need:
require_worktreefor commands that need a worktree to operate in,require_selected_branchfor arguments that key by branch. The merge/rebase target validators fall through to the same path lookup, so a target can be named by the worktree it's checked out in.Routed through it:
switch(including--base),remove,step commit --branch,step diff --branchand its target,step copy-ignored --from/--to,step promote,step relocate,config state --branch(9 sites), andmerge/step rebase/step squash/step pushtargets.resolve_input_path— already documented as the one resolution point for user-supplied paths — now expands a leading~, so the tilde form worktrunk prints is a form it reads back.~userstays literal; wt doesn't reimplement that shell feature.Documentation
A path is an alias, not a second addressing scheme, so it is stated once rather than on every argument: one paragraph in
wt switch's help and one sentence on the addressing line inworktrunk.md. Argument descriptions still read as branches. The two exceptions are the arguments whose descriptions are already catalogues of accepted forms —wt switch's (Branch, worktree path, shortcut, or PR/MR URL) andwt remove's, which has named the path since before this branch. The Worktree Model section ofCLAUDE.mdrecords which way to document it, so the next argument doesn't grow its own copy.Two silent no-ops fixed along the way
wt step relocate <unmatched>matched arguments against branch names by string equality, so a typo filtered everything out and the empty result rendered as○ All worktrees are at expected paths— a success message for work that never happened. Every way an argument can fail to land on a relocatable worktree now errors, including the detached and prunable cases the new path route makes reachable.wt switch <token>— which creates a worktree only when the branch exists, so for a mistyped path it would just fail again.WorktreeSelectorNotFoundnow saysNo branch or worktree named X; a branch that genuinely exists without a checkout keeps the create hint.Testing
Full gate green: 4596 tests, lints, docs sync,
--features shell-integration-testsclippy.codecov/patchis 99.25% of diff hit against a 97.93% target. New coverage:require_selected_branchrefusal), shortcuts never treated as paths, branch-only fallthrough, and the two distinct not-found errors. Plusexpand_tilderound-trippingformat_path_for_display.switchby relative/single-component/absolute/tilde path,--baseby path,step diff --branchby path and@(asserted equal to the by-branch output),config state --branchset via@and read via the worktree path, and both new relocate errors.wt remove's resolution is unchanged — it already had this rule; it now shares the implementation. The 106-testremove::suite is untouched and green.wt step push <worktree-path>(therequire_target_branchhalf of the target fallback), andwt step relocateagainst a prunable worktree.One diff line is unhit:
expand_tilde's fallback whenhome_dir()returnsNone, which has no deterministic trigger. The@-resolution backstop inresolve_worktreeis untested for the same reason — no CLI route reaches it — so it kept its originalmatcharm rather than being re-indented into the diff.Left out
wt config state default-branch setandprevious-branch settake a branch name as a value to store rather than a selector, so they still take it literally.🤖 Generated with Claude Code