fix(switch): resolve pr:N before pre-switch hooks, and set pr_* for same-repo PRs - #3941
Conversation
…ame-repo PRs `wt switch pr:<n>` resolved the PR through the forge inside `plan_switch`, which runs after `pre-switch`. The hook therefore received the raw `pr:3933` token as `branch` and `target`, and no `target_worktree_path` even when the PR's branch was already checked out. Symbolic arguments (`-`, `@`, `^`) already resolve ahead of the hook for the same reason (#2310); this extends the rule to the one form whose branch name can't be derived locally. `pr_number` / `pr_url` were read back off `CreationMethod::ForkRef`, which a same-repo PR never produces, so the common case saw both unset in every hook despite the documented behaviour. The identity now rides its own field from resolution through the plan, so it reaches `pre-switch`, `pre-start`, `post-start`, and `post-switch` alike — including a `pr:N` switch onto a branch that already has a worktree. Resolution now precedes the pre-switch hook, so a malformed project config is loaded explicitly before provider selection rather than surfacing through the hook-approval gate.
worktrunk-bot
left a comment
There was a problem hiding this comment.
Self-review. Three observations — the first is the one I'd act on; the other two are cleanup this PR's own principle argues for.
The SwitchResult::Existing path is the one behavior nothing pins. Both the updated test_switch_pr_hooks_see_pr_vars and the new test_switch_pr_same_repo_hooks_see_resolved_branch take the create path — the branch has no worktree yet. But the two things the description promises for an already-checked-out branch are exactly the ones on the other branch: target_worktree_path in pre-switch ("unset even when that branch had a worktree → set when it does"), and the ref_identity deliberately kept alive past the move into plan_switch so an Existing result still carries pr_number / pr_url. Drop the pipeline's if let Some(identity) = &ref_identity re-application, or revert run_pre_switch_hooks to expand_selector for pr: args, and the suite is still green. Worth a third case: wt switch pr:101 twice — once to create, once to land on the existing worktree — asserting pre_switch.txt carries target_worktree_path and post_switch.txt still carries pr_number.
RefIdentity is constructed at five sites with the same value. resolve_remote_ref builds it once for the same-repo return; resolve_fork_ref repeats Some(RefIdentity { number, url: info.url.clone() }) on each of its four Ok returns. Every fork return path wants the same identity, so resolve_remote_ref can set it once on what resolve_fork_ref hands back (info.url is still owned there — the fork branch only borrows &info), and resolve_fork_ref stops carrying the field at all. That makes "a fork return path that forgets the identity" a non-representable state rather than a review check — which is the same bug class this PR is fixing, one level down.
SwitchResult::Created { pr_number, pr_url } is now a second channel for the same value. TemplateVars::for_post_switch reads them off the result, and the pipeline then re-applies with_pr from ref_identity on the very next line — and the two can never disagree, since execute_switch derives the result's fields from plan.ref_identity, which is the same value the pipeline cloned. Nothing else reads them (SwitchJsonOutput::from_result doesn't; for_post_switch has one production caller). This PR removed CreationMethod::ForkRef::ref_url for precisely this reason once the identity got its own field — the SwitchResult pair is the same leftover, and dropping it plus for_post_switch's .with_pr(...) leaves one source of truth.
Checked and clear
- Error enrichment. Moving
pr:/mr:resolution out ofplan_switchmoves it out of themap_errthat wrapsGitErrorinWithSwitchSuggestion. OnlyBranchAlreadyExists,BranchNotFound, andWorktreePathExistsconsume that ctx (write_render_with_ctxinsrc/git/error.rs); thepr:path raisesRefBaseConflict,RefCreateConflict, andBranchTracksDifferentRef, none of which do. No hint regressed. - Help snapshots. Nothing under
tests/snapshots/contains the editedafter_long_helpprose, andhook_verbose_variable_tablerenders a non-PR switch wherepr_numberstays(unset)— so the absent snapshot churn is right, not a missedcargo insta test --accept. - The
worktree_pathdoc caveat is accurate.run_pre_switch_hookspassescurrent_pathtoCommandContext::newand only overrides viawith_active_worktreewhenworktree_for_branchresolves, andwith_active_worktreeis what setsworktree_path. So it genuinely stays on the source when the switch creates. - Approval gate. The forge round-trip added ahead of
pre-switchruns no project-defined command, so the "Project Commands Run Only After Approval" ordering inCLAUDE.mdis intact.repo.project_config()is aget_or_try_initcell, so the explicit load costs nothing the later gate wasn't already paying. - Data safety. No deletion surface in the diff;
switch.rscontains none of the force-delete forms the review policy holds on.
One structural note in passing: resolve_switch_target now falls through to regular-branch handling when branch is a pr:N token but ref_target is None — it would look for a literal branch named pr:101. There's one caller today so it can't happen, but the invariant "a pr: token always arrives pre-resolved" is held by convention rather than by the types.
…nnel Self-review follow-ups. Cover `SwitchResult::Existing`. Both PR-hook tests took the create path, so the two things the fix promises for an already-checked-out branch — `target_worktree_path` in `pre-switch`, and `pr_number` / `pr_url` surviving into `post-switch` — were unpinned. The new test switches to `pr:101` twice and asserts both; dropping either half of the fix fails it. Set the PR identity once. `resolve_fork_ref` repeated the same `RefIdentity` on each of its four `Ok` returns; `resolve_remote_ref` now attaches it on its single return path, so a fork arm can't forget it. Drop `SwitchResult::Created`'s `pr_number` / `pr_url`. The pipeline re-applies the same values from the resolved argument on the next line, and nothing else read them — the result is the wrong home for identity that belongs to the argument, which is why the `Existing` path had none.
worktrunk-bot
left a comment
There was a problem hiding this comment.
Self-review of 100a272. All three follow-ups land, and the new Existing-path test earns its keep — I checked that repo.wt_command() runs from root_path(), so the second wt switch pr:101 really is Existing rather than AlreadyAt, and that fetch_same_repo_branch's refspec targets refs/remotes/<remote>/<branch>, so re-fetching a branch that is now checked out in a linked worktree doesn't trip git's "refusing to fetch into branch" guard. Two things to act on, plus a correction to my last review.
Correction: this PR does touch the deletion surface, and I said it didn't. My previous review's "Checked and clear" block claimed "no deletion surface in the diff; switch.rs contains none of the force-delete forms the review policy holds on." That was wrong. execute_switch's CreationMethod::ForkRef arm — the arm this PR edits, to drop ref_url: _ from the destructuring — ends its if let Err(e) = setup_result branch with let _ = repo.run_command(&["branch", "-D", "--", &branch]);. Nothing in the diff widens what that deletes or changes when it fires; the branch it removes is one wt created seconds earlier in the same operation. But .claude/skills/running-tend/references/review-pr.md → Data-Loss Surface: Hold for Human Review keys on reach, not on whether the destructive line itself changed — "In source, a change near the force-delete path holds even when the destructive line isn't in the diff" — and editing that match arm is reach. So: requesting review from @max-sixty, and not authorizing the merge. Flagging it late is worse than flagging it on the first pass, which is the actual failure here.
The two inline notes are ordinary cleanup — a test helper this file already has, and a type docstring the increment quietly outdated.
…t the RefIdentity docstring Review follow-ups on the two tests this PR adds: the 21-line origin/insteadOf setup they inlined is `set_github_remote_url`'s body verbatim, which 16 other tests in the file already call. `RefIdentity`'s docstring said the identity is "carried through the plan", which the previous commit outdated: an `Existing` switch builds no `SwitchPlan::Create`, so `pre-switch` reads the `ResolvedTarget` and the post-* hooks read the pipeline's own clone.
Fixes #3934.
wt switch pr:<n>resolved the PR through the forge insideplan_switch, which runs afterpre-switch— so the hook got the rawpr:3933token asbranchandtarget, and notarget_worktree_patheven when the PR's branch was already checked out. Separately,pr_number/pr_urlwere read back offCreationMethod::ForkRef, which a same-repo PR never produces, so the reporter's output shows both unset in every hook although the help text promises them. This resolvespr:/mr:before the hooks (the rule-/@/^already follow, #2310) and gives the PR identity its own field, carried from resolution through the plan intopre-switch,pre-start,post-start, andpost-switchalike.Verified with two integration tests that fail on
main(thepre-switchhook aborts the switch withundefined value) and pass here; full unit + integration suites, clippy--all-targets --all-features, rustdoc, and pre-commit are green locally.What each variable does now
For
wt switch pr:3933where the PR's branch isfeat/issue-3922-list-sort:branch/targetinpre-switchpr:3933feat/issue-3922-list-sorttarget_worktree_pathinpre-switchpr_number/pr_url(same-repo PR, all hooks)3933/ the PR URLpr_number/pr_urlinpre-switch(fork PR)worktree_pathinpre-switchis deliberately unchanged: on a switch that creates a worktree there is no destination directory yet, so it stays on the source. The help text claimed it was "the destination" without that caveat —src/cli/mod.rsnow says which case is which and points atpre-startfor work that needs the new worktree (which is what the reporter's ownmise-trusthook already does).Ordering consequences
The forge lookup and its
git fetchnow happen beforepre-switchruns, so a hook that aborts the switch aborts it after the PR was fetched. That is inherent to the fix — the branch name is the forge's answer. The lookup is still bounded to the argument form that asked for it (resolve_ref_shortcut_targetreturnsNonefor anything that isn'tpr:/mr:), and the resolved target is threaded intoplan_switch, so the forge is queried exactly once as before.The hook-approval gate used to be what surfaced a malformed
.config/wt.tomlbefore provider selection —configured_forge_platformreports an unparsable config as "unset", which would route an intendedforge.platformoverride to the wrong CLI. With the resolution now first, that config load is explicit at the top ofresolve_ref_shortcut_targetrather than an accident of ordering (test_switch_pr_malformed_project_config_bails_before_provider_selectioncovers it and caught the regression).CreationMethod::ForkRef::ref_urlbecame dead once the identity moved to its own field, so it is removed rather than left with an#[allow].SwitchResult::Created'spr_number/pr_urlare removed for the same reason: the pipeline applies the identity from the resolved argument, which is the only channel that also serves a switch onto an existing worktree.