diff --git a/docs/src/content/docs/hook.md b/docs/src/content/docs/hook.md index 2ea0aad91..b295b5eab 100644 --- a/docs/src/content/docs/hook.md +++ b/docs/src/content/docs/hook.md @@ -130,8 +130,8 @@ Hooks can use template variables that expand at runtime: | | `{{ base_worktree_path }}` | Base worktree path | | | `{{ target }}` | Target branch name | | | `{{ target_worktree_path }}` | Target worktree path (when target has a worktree) | -| | `{{ pr_number }}` | PR/MR number (post-switch, pre-start, post-start; when creating via `pr:N` / `mr:N`) | -| | `{{ pr_url }}` | PR/MR web URL (post-switch, pre-start, post-start; when creating via `pr:N` / `mr:N`) | +| | `{{ pr_number }}` | PR/MR number (switch and create hooks; when switching via `pr:N` / `mr:N`) | +| | `{{ pr_url }}` | PR/MR web URL (switch and create hooks; when switching via `pr:N` / `mr:N`) | | repo | `{{ repo }}` | Repository directory name | | | `{{ repo_path }}` | Absolute path to repository root | | | `{{ owner }}` | Primary remote owner path (may include subgroups) | @@ -161,7 +161,7 @@ All hooks share the same perspective — `{{ branch | hash_port }}` produces the `cwd` is the worktree root where the hook command runs. It equals `worktree_path` except in three cases: -- `pre-switch`: hook runs in the source worktree; `worktree_path` is the destination +- `pre-switch`: hook runs in the source worktree; `worktree_path` is the destination when that worktree already exists — a switch that creates one has no destination directory yet, so `worktree_path` stays on the source (use `pre-start` to work in the new worktree) - `post-remove`: the active worktree is gone, so the hook runs in the primary worktree - `post-merge` with removal: the active worktree is gone, so the hook runs in the target worktree diff --git a/plugins/worktrunk/skills/worktrunk/reference/hook.md b/plugins/worktrunk/skills/worktrunk/reference/hook.md index d34fdff35..39d2391f2 100644 --- a/plugins/worktrunk/skills/worktrunk/reference/hook.md +++ b/plugins/worktrunk/skills/worktrunk/reference/hook.md @@ -124,8 +124,8 @@ Hooks can use template variables that expand at runtime: | | `{{ base_worktree_path }}` | Base worktree path | | | `{{ target }}` | Target branch name | | | `{{ target_worktree_path }}` | Target worktree path (when target has a worktree) | -| | `{{ pr_number }}` | PR/MR number (post-switch, pre-start, post-start; when creating via `pr:N` / `mr:N`) | -| | `{{ pr_url }}` | PR/MR web URL (post-switch, pre-start, post-start; when creating via `pr:N` / `mr:N`) | +| | `{{ pr_number }}` | PR/MR number (switch and create hooks; when switching via `pr:N` / `mr:N`) | +| | `{{ pr_url }}` | PR/MR web URL (switch and create hooks; when switching via `pr:N` / `mr:N`) | | repo | `{{ repo }}` | Repository directory name | | | `{{ repo_path }}` | Absolute path to repository root | | | `{{ owner }}` | Primary remote owner path (may include subgroups) | @@ -155,7 +155,7 @@ All hooks share the same perspective — `{{ branch | hash_port }}` produces the `cwd` is the worktree root where the hook command runs. It equals `worktree_path` except in three cases: -- `pre-switch`: hook runs in the source worktree; `worktree_path` is the destination +- `pre-switch`: hook runs in the source worktree; `worktree_path` is the destination when that worktree already exists — a switch that creates one has no destination directory yet, so `worktree_path` stays on the source (use `pre-start` to work in the new worktree) - `post-remove`: the active worktree is gone, so the hook runs in the primary worktree - `post-merge` with removal: the active worktree is gone, so the hook runs in the target worktree diff --git a/skills/worktrunk/reference/hook.md b/skills/worktrunk/reference/hook.md index d34fdff35..39d2391f2 100644 --- a/skills/worktrunk/reference/hook.md +++ b/skills/worktrunk/reference/hook.md @@ -124,8 +124,8 @@ Hooks can use template variables that expand at runtime: | | `{{ base_worktree_path }}` | Base worktree path | | | `{{ target }}` | Target branch name | | | `{{ target_worktree_path }}` | Target worktree path (when target has a worktree) | -| | `{{ pr_number }}` | PR/MR number (post-switch, pre-start, post-start; when creating via `pr:N` / `mr:N`) | -| | `{{ pr_url }}` | PR/MR web URL (post-switch, pre-start, post-start; when creating via `pr:N` / `mr:N`) | +| | `{{ pr_number }}` | PR/MR number (switch and create hooks; when switching via `pr:N` / `mr:N`) | +| | `{{ pr_url }}` | PR/MR web URL (switch and create hooks; when switching via `pr:N` / `mr:N`) | | repo | `{{ repo }}` | Repository directory name | | | `{{ repo_path }}` | Absolute path to repository root | | | `{{ owner }}` | Primary remote owner path (may include subgroups) | @@ -155,7 +155,7 @@ All hooks share the same perspective — `{{ branch | hash_port }}` produces the `cwd` is the worktree root where the hook command runs. It equals `worktree_path` except in three cases: -- `pre-switch`: hook runs in the source worktree; `worktree_path` is the destination +- `pre-switch`: hook runs in the source worktree; `worktree_path` is the destination when that worktree already exists — a switch that creates one has no destination directory yet, so `worktree_path` stays on the source (use `pre-start` to work in the new worktree) - `post-remove`: the active worktree is gone, so the hook runs in the primary worktree - `post-merge` with removal: the active worktree is gone, so the hook runs in the target worktree diff --git a/src/cli/mod.rs b/src/cli/mod.rs index b65d3f5cf..ebd65956f 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -1670,8 +1670,8 @@ Hooks can use template variables that expand at runtime: | | `{{ base_worktree_path }}` | Base worktree path | | | `{{ target }}` | Target branch name | | | `{{ target_worktree_path }}` | Target worktree path (when target has a worktree) | -| | `{{ pr_number }}` | PR/MR number (post-switch, pre-start, post-start; when creating via `pr:N` / `mr:N`) | -| | `{{ pr_url }}` | PR/MR web URL (post-switch, pre-start, post-start; when creating via `pr:N` / `mr:N`) | +| | `{{ pr_number }}` | PR/MR number (switch and create hooks; when switching via `pr:N` / `mr:N`) | +| | `{{ pr_url }}` | PR/MR web URL (switch and create hooks; when switching via `pr:N` / `mr:N`) | | repo | `{{ repo }}` | Repository directory name | | | `{{ repo_path }}` | Absolute path to repository root | | | `{{ owner }}` | Primary remote owner path (may include subgroups) | @@ -1701,7 +1701,7 @@ All hooks share the same perspective — `{{ branch | hash_port }}` produces the `cwd` is the worktree root where the hook command runs. It equals `worktree_path` except in three cases: -- `pre-switch`: hook runs in the source worktree; `worktree_path` is the destination +- `pre-switch`: hook runs in the source worktree; `worktree_path` is the destination when that worktree already exists — a switch that creates one has no destination directory yet, so `worktree_path` stays on the source (use `pre-start` to work in the new worktree) - `post-remove`: the active worktree is gone, so the hook runs in the primary worktree - `post-merge` with removal: the active worktree is gone, so the hook runs in the target worktree diff --git a/src/commands/template_vars.rs b/src/commands/template_vars.rs index 99f9296f6..bd390f8c6 100644 --- a/src/commands/template_vars.rs +++ b/src/commands/template_vars.rs @@ -149,7 +149,10 @@ impl TemplateVars { /// /// `target` matches the bare vars (the destination); `base` is the source /// — the branched-from for creates, the source worktree for existing - /// switches. PR/MR identity propagates into post-* hooks. + /// switches. PR/MR identity is not read off the result: it belongs to the + /// argument, not to what the switch did with it, so the caller applies it + /// with [`with_pr`](Self::with_pr) — an `Existing` switch onto a + /// `pr:N` branch has the same identity as the run that created it. pub fn for_post_switch( result: &SwitchResult, branch_info: &SwitchBranchInfo, @@ -164,12 +167,8 @@ impl TemplateVars { SwitchResult::Created { base_branch, base_worktree_path, - pr_number, - pr_url, .. - } => vars - .with_base_strs(base_branch.as_deref(), base_worktree_path.as_deref()) - .with_pr(*pr_number, pr_url.as_deref()), + } => vars.with_base_strs(base_branch.as_deref(), base_worktree_path.as_deref()), SwitchResult::Existing { .. } | SwitchResult::AlreadyAt(_) => { let base = (!source_branch.is_empty()).then_some(source_branch); let path = (!source_path.is_empty()).then_some(source_path); @@ -247,21 +246,24 @@ mod tests { assert!(!pairs.iter().any(|(k, _)| *k == "base_worktree_path")); } + /// The switch pipeline layers the PR/MR identity on afterwards, the same + /// way for a create as for a switch onto an existing worktree. #[test] - fn for_post_switch_created_with_pr() { + fn for_post_switch_created_takes_pr_from_caller() { let result = SwitchResult::Created { path: PathBuf::from("/repo.fork"), created_branch: false, base_branch: Some("main".to_string()), base_worktree_path: Some("/repo".to_string()), from_remote: None, - pr_number: Some(42), - pr_url: Some("https://example.test/pr/42".to_string()), }; let info = SwitchBranchInfo { branch: Some("contributor/feature".to_string()), }; let vars = TemplateVars::for_post_switch(&result, &info, "", ""); + assert!(!vars.as_extra_vars().iter().any(|(k, _)| *k == "pr_number")); + + let vars = vars.with_pr(Some(42), Some("https://example.test/pr/42")); let pairs = vars.as_extra_vars(); assert!(pairs.contains(&("base", "main"))); assert!(pairs.contains(&("base_worktree_path", "/repo"))); diff --git a/src/commands/worktree/switch.rs b/src/commands/worktree/switch.rs index c35565904..e89b3a245 100644 --- a/src/commands/worktree/switch.rs +++ b/src/commands/worktree/switch.rs @@ -33,7 +33,7 @@ use worktrunk::styling::{ }; use super::resolve::{compute_worktree_path, offer_bare_repo_worktree_path_fix}; -use super::types::{CreationMethod, SwitchBranchInfo, SwitchPlan, SwitchResult}; +use super::types::{CreationMethod, RefIdentity, SwitchBranchInfo, SwitchPlan, SwitchResult}; use crate::cli::{SwitchArgs, SwitchFormat}; use crate::commands::backup::back_up_clobbered_path_now; use crate::commands::command_approval::approve_hooks; @@ -58,6 +58,29 @@ struct ResolvedTarget { selector: Selector, /// How to create the worktree method: CreationMethod, + /// Set when the argument was `pr:N` / `mr:N`, for the `pr_number` / + /// `pr_url` hook variables. Independent of `method`: a same-repo PR + /// resolves to `CreationMethod::Regular` and still has an identity. + ref_identity: Option, +} + +impl ResolvedTarget { + /// A target with no PR/MR identity — every form but `pr:N` / `mr:N`. + fn new(selector: Selector, method: CreationMethod) -> Self { + Self { + selector, + method, + ref_identity: None, + } + } + + /// Attach the PR/MR the argument named. Called on the single return path + /// of [`resolve_remote_ref`], so a fork resolution can't reach the plan + /// without an identity the way it could when each arm set the field. + fn with_ref_identity(mut self, identity: RefIdentity) -> Self { + self.ref_identity = Some(identity); + self + } } static GITHUB_PROVIDER: GitHubProvider = GitHubProvider; @@ -216,20 +239,27 @@ fn resolve_remote_ref( .into()); } - if info.is_cross_repo { - return resolve_fork_ref(repo, provider, number, &info); - } + let target = if info.is_cross_repo { + resolve_fork_ref(repo, provider, number, &info)? + } else { + // Same-repo ref: fetch the branch to ensure remote tracking refs exist + fetch_same_repo_branch(repo, &info)?; + ResolvedTarget::new( + Selector::rewritten_to(info.source_branch), + CreationMethod::Regular { + create_branch: false, + base_branch: None, + base_pr_upstream: None, + }, + ) + }; - // Same-repo ref: fetch the branch to ensure remote tracking refs exist - fetch_same_repo_branch(repo, &info)?; - Ok(ResolvedTarget { - selector: Selector::rewritten_to(info.source_branch), - method: CreationMethod::Regular { - create_branch: false, - base_branch: None, - base_pr_upstream: None, - }, - }) + // Every path out of here resolved the same PR/MR, so the identity is set + // once here rather than repeated on each arm of `resolve_fork_ref`. + Ok(target.with_ref_identity(RefIdentity { + number, + url: info.url, + })) } /// Resolve a fork (cross-repo) PR/MR. @@ -266,14 +296,14 @@ fn resolve_fork_ref( ref_type.display(number) )) ); - return Ok(ResolvedTarget { - selector: Selector::rewritten_to(local_branch), - method: CreationMethod::Regular { + return Ok(ResolvedTarget::new( + Selector::rewritten_to(local_branch), + CreationMethod::Regular { create_branch: false, base_branch: None, base_pr_upstream: None, }, - }); + )); } // Branch exists but doesn't track this ref - try prefixed name (GitHub/Gitea) @@ -293,14 +323,14 @@ fn resolve_fork_ref( ref_type.display(number) )) ); - return Ok(ResolvedTarget { - selector: Selector::rewritten_to(prefixed), - method: CreationMethod::Regular { + return Ok(ResolvedTarget::new( + Selector::rewritten_to(prefixed), + CreationMethod::Regular { create_branch: false, base_branch: None, base_pr_upstream: None, }, - }); + )); } // Prefixed branch exists but tracks something else - error return Err(GitError::BranchTracksDifferentRef { @@ -314,17 +344,16 @@ fn resolve_fork_ref( // Use prefixed branch name; push won't work (None for fork_push_url) // This is GitHub-only (GitLab doesn't support prefixed names) let remote = remote_ref::find_remote(repo, info)?; - return Ok(ResolvedTarget { - selector: Selector::rewritten_to(prefixed), - method: CreationMethod::ForkRef { + return Ok(ResolvedTarget::new( + Selector::rewritten_to(prefixed), + CreationMethod::ForkRef { ref_type, number, ref_path: provider.ref_path(number), fork_push_url: None, - ref_url: info.url.clone(), remote, }, - }); + )); } // GitLab doesn't support prefixed branch names - error @@ -376,17 +405,16 @@ fn resolve_fork_ref( } }; - Ok(ResolvedTarget { - selector: Selector::rewritten_to(local_branch), - method: CreationMethod::ForkRef { + Ok(ResolvedTarget::new( + Selector::rewritten_to(local_branch), + CreationMethod::ForkRef { ref_type, number, ref_path: provider.ref_path(number), fork_push_url, - ref_url: info.url.clone(), remote, }, - }) + )) } /// Fetch a same-repo PR/MR's source branch with an explicit refspec so the @@ -518,30 +546,73 @@ fn resolve_remote_ref_as_base( Ok((sha, None)) } -/// Resolve the switch target, handling pr:/mr: syntax and --create/--base flags. +/// Resolve a `pr:N` / `mr:N` argument through the forge, ahead of the rest of +/// planning. +/// +/// Returns `None` for any other argument — this is the one form whose branch +/// name can't be derived locally. +/// +/// Split out of [`resolve_switch_target`] so [`SwitchPipeline::run`] can +/// resolve it *before* `pre-switch` hooks: a hook that receives the raw +/// `pr:3933` token sees a `branch` / `target` naming nothing, and no +/// `target_worktree_path` even when the PR's branch is already checked out +/// (#3934). Symbolic arguments (`-`, `@`, `^`) already resolve ahead of the +/// hook for the same reason (#2310); this extends the rule to the one +/// resolution that costs a forge round-trip. The result is threaded back into +/// [`plan_switch`], so the forge is still queried exactly once. +fn resolve_ref_shortcut_target( + repo: &Repository, + branch: &str, + create: bool, + base: Option<&str>, +) -> anyhow::Result> { + // `pr:N` dispatches to GitHub, Gitea, or Azure DevOps based on remotes; + // `mr:N` to GitLab. Forge PR/MR web URLs normalise to the same shortcuts. + let Some((ref_type, number)) = parse_ref_shortcut(branch) else { + return Ok(None); + }; + // Fail closed on a malformed project config before any provider is chosen: + // `forge.platform` lives there, and `configured_forge_platform` reports a + // config that won't parse as "unset", which would route an intended + // override to the wrong CLI. The hook-approval gate used to be what + // surfaced this, but that no longer runs first. + repo.project_config()?; + // --base is invalid with pr:/mr: syntax (check before provider selection, + // which may invoke a forge CLI to inspect authentication). + if base.is_some() { + return Err(GitError::RefBaseConflict { ref_type, number }.into()); + } + let provider: &dyn RemoteRefProvider = match ref_type { + RefType::Pr => choose_pr_provider(repo)?, + RefType::Mr => &GitLabProvider, + }; + resolve_remote_ref(repo, provider, number, create).map(Some) +} + +/// Resolve the switch target, handling --create/--base flags. /// /// This is the first phase of planning: determine what branch we're switching to -/// and how we'll create the worktree. May involve network calls for PR/MR resolution. +/// and how we'll create the worktree. `pr:`/`mr:` arguments arrive already +/// resolved in `ref_target` — [`resolve_ref_shortcut_target`] runs them before +/// `pre-switch` hooks, and its result is passed straight through here. fn resolve_switch_target( repo: &Repository, branch: &str, + ref_target: Option, create: bool, base: Option<&str>, ) -> anyhow::Result { - // `pr:N` dispatches to GitHub, Gitea, or Azure DevOps based on remotes; - // `mr:N` to GitLab. Forge PR/MR web URLs normalise to the same shortcuts. - if let Some((ref_type, number)) = parse_ref_shortcut(branch) { - // --base is invalid with pr:/mr: syntax (check before provider selection, - // which may invoke a forge CLI to inspect authentication). - if base.is_some() { - return Err(GitError::RefBaseConflict { ref_type, number }.into()); - } - let provider: &dyn RemoteRefProvider = match ref_type { - RefType::Pr => choose_pr_provider(repo)?, - RefType::Mr => &GitLabProvider, - }; - return resolve_remote_ref(repo, provider, number, create); + if let Some(target) = ref_target { + return Ok(target); } + // Everything below treats `branch` as a branch name, so a `pr:N` token + // arriving unresolved would go looking for a literal branch called + // `pr:101`. One caller today, and it always resolves first — this is what + // fails the tests if a second one forgets to. + debug_assert!( + parse_ref_shortcut(branch).is_none(), + "`pr:`/`mr:` must reach plan_switch pre-resolved (resolve_ref_shortcut_target)" + ); // Regular branch switch. `expand_selector` normalizes the token and // expands `@` / `-` / `^`, reporting whether it rewrote anything. @@ -632,21 +703,21 @@ fn resolve_switch_target( None }; - Ok(ResolvedTarget { + Ok(ResolvedTarget::new( // Under `--create` the argument names a branch that does not exist // yet, so it is not a path to look up — stated here, where `create` // lives, rather than re-tested at each arm that consults it. - selector: if create { + if create { selector.branch_only() } else { selector }, - method: CreationMethod::Regular { + CreationMethod::Regular { create_branch: create, base_branch, base_pr_upstream, }, - }) + )) } /// Validate that we can create a worktree at the given path. @@ -802,6 +873,7 @@ fn setup_fork_branch( fn plan_switch( repo: &Repository, branch: &str, + ref_target: Option, create: bool, base: Option<&str>, clobber: bool, @@ -810,8 +882,9 @@ fn plan_switch( // Record current branch for `wt switch -` support let new_previous = repo.current_worktree().branch().ok().flatten(); - // Phase 1: Resolve target (handles pr:, validates --create/--base, may do network) - let target = resolve_switch_target(repo, branch, create, base)?; + // Phase 1: Resolve target (validates --create/--base; `pr:`/`mr:` arrived + // pre-resolved from the caller, ahead of the pre-switch hooks) + let target = resolve_switch_target(repo, branch, ref_target, create, base)?; // Phase 2: the shared worktree ladder — the branch, then the argument as a // worktree's own path (the way to name a detached one, which has no @@ -867,6 +940,7 @@ fn plan_switch( branch: target.selector.token().to_string(), worktree_path: expected_path, method: target.method, + ref_identity: target.ref_identity, needs_clobber_backup, new_previous, }) @@ -927,6 +1001,7 @@ fn execute_switch( branch, worktree_path, method, + ref_identity, needs_clobber_backup, new_previous, } => { @@ -1079,7 +1154,6 @@ fn execute_switch( number, ref_path, fork_push_url, - ref_url: _, remote, } => { let label = ref_type.display(*number); @@ -1150,14 +1224,15 @@ fn execute_switch( }) .map(|p| worktrunk::path::to_posix_path(&p.to_string_lossy())); - // PR/MR identity travels into both the pre-start hook below and the - // SwitchResult — TemplateVars::for_post_switch then forwards it to - // background post-switch / post-start hooks. - let (pr_number, pr_url) = match &method { - CreationMethod::ForkRef { - number, ref_url, .. - } => (Some(*number), Some(ref_url.clone())), - CreationMethod::Regular { .. } => (None, None), + // PR/MR identity for the pre-start hook below. It rides the plan, + // not `method`: a same-repo PR resolves to + // `CreationMethod::Regular`, and reading `method` left the common + // case with no `pr_number`. The post-* hooks get the same value + // from the pipeline's own copy rather than back off the + // `SwitchResult`, which would leave the `Existing` path unserved. + let (pr_number, pr_url) = match &ref_identity { + Some(RefIdentity { number, url }) => (Some(*number), Some(url.clone())), + None => (None, None), }; // Execute pre-start commands. `hook_repo` roots the render context @@ -1170,17 +1245,11 @@ fn execute_switch( CommandContext::new(&hook_repo, config, Some(&branch), &worktree_path, force); let mut vars = TemplateVars::new() .with_target(&branch) - .with_target_worktree_path(&worktree_path); - match &method { - CreationMethod::Regular { base_branch, .. } => { - vars = vars - .with_base_strs(base_branch.as_deref(), base_worktree_path.as_deref()); - } - CreationMethod::ForkRef { - number, ref_url, .. - } => { - vars = vars.with_pr(Some(*number), Some(ref_url)); - } + .with_target_worktree_path(&worktree_path) + .with_pr(pr_number, pr_url.as_deref()); + if let CreationMethod::Regular { base_branch, .. } = &method { + vars = + vars.with_base_strs(base_branch.as_deref(), base_worktree_path.as_deref()); } ctx.execute_pre_create_commands(&vars.as_extra_vars(), hook_plan, &worktree_path)?; } @@ -1195,8 +1264,6 @@ fn execute_switch( base_branch, base_worktree_path, from_remote, - pr_number, - pr_url, }, SwitchBranchInfo { branch: Some(branch), @@ -1345,32 +1412,40 @@ struct SwitchOptions<'a> { format: crate::cli::SwitchFormat, } -/// Run pre-switch hooks before branch resolution or worktree creation. +/// Run pre-switch hooks before worktree creation. /// /// Symbolic arguments (`-`, `@`, `^`) are resolved to concrete branch names /// before building the hook context so `{{ target }}`, `{{ target_worktree_path }}`, /// and the Active overrides point at the real destination. When resolution /// fails (e.g., no previous branch for `-`), the raw argument is used — the /// same error surfaces later from `plan_switch` with the canonical message. +/// `pr:N` / `mr:N` cannot resolve locally at all, so the caller resolves them +/// through the forge first and passes the result in `ref_target`. /// /// Directional vars: /// - `base` / `base_worktree_path`: current (source) branch and worktree /// - `target` / `target_worktree_path`: destination branch and worktree (if it exists) +/// - `pr_number` / `pr_url`: the PR/MR a `pr:N` / `mr:N` argument named fn run_pre_switch_hooks( repo: &Repository, config: &UserConfig, target_branch: &str, + ref_target: Option<&ResolvedTarget>, yes: bool, ) -> anyhow::Result<()> { let current_wt = repo.current_worktree(); let current_path = current_wt.path().to_path_buf(); // `expand_selector`, not the bare shortcut expander: the `target` var a // pre-switch hook receives has to name the same branch the switch goes on - // to resolve, normalization included. - let resolved_target = repo - .expand_selector(target_branch) - .map(|s| s.token().to_string()) - .unwrap_or_else(|_| target_branch.to_string()); + // to resolve, normalization included. A `pr:`/`mr:` argument is already + // past that point — the forge answered with the branch name itself. + let resolved_target = match ref_target { + Some(target) => target.selector.token().to_string(), + None => repo + .expand_selector(target_branch) + .map(|s| s.token().to_string()) + .unwrap_or_else(|_| target_branch.to_string()), + }; let pre_ctx = CommandContext::new(repo, config, Some(&resolved_target), ¤t_path, yes); let pre_switch_approved = approve_hooks(&pre_ctx, &[HookType::PreSwitch])?; @@ -1382,9 +1457,14 @@ fn run_pre_switch_hooks( let base_branch = current_wt.branch().ok().flatten().unwrap_or_default(); let dest_path = repo.worktree_for_branch(&resolved_target).ok().flatten(); + let ref_identity = ref_target.and_then(|t| t.ref_identity.as_ref()); let mut vars = TemplateVars::new() .with_base(&base_branch, ¤t_path) - .with_target(&resolved_target); + .with_target(&resolved_target) + .with_pr( + ref_identity.map(|id| id.number), + ref_identity.map(|id| id.url.as_str()), + ); if let Some(p) = dest_path.as_deref() { vars = vars.with_target_worktree_path(p).with_active_worktree(p); } @@ -1619,6 +1699,20 @@ impl SwitchPipeline<'_> { // (.git, .bare) before anything reads worktree-path config. offer_bare_repo_worktree_path_fix(repo, config, identifier)?; + // Resolve a `pr:N` / `mr:N` argument before the hooks below, so their + // `{{ branch }}` / `{{ target }}` name the PR's branch rather than the + // raw token. This is the one resolution that reaches the forge, and it + // is bounded to the argument form that asked for it; the resolved + // target is handed to `plan_switch`, which queries nothing further. + let ref_target = resolve_ref_shortcut_target(repo, identifier, create, base)?; + // Kept past the move into `plan_switch`: a `pr:N` switch onto a branch + // that already has a worktree produces `SwitchResult::Existing`, which + // carries no PR identity of its own, and the hooks on that path should + // still see the same `pr_number` / `pr_url` as the creating run. + let ref_identity = ref_target + .as_ref() + .and_then(|target| target.ref_identity.clone()); + // Run pre-switch hooks before worktree creation. run_pre_switch_hooks // resolves symbolic args (`-`, `@`, `^`) first, so {{ branch }} and // {{ target }} carry the concrete destination, not the raw token. Skip @@ -1626,7 +1720,7 @@ impl SwitchPipeline<'_> { // against. `yes` is the single switch-wide flag, so the picker (no // `--yes`) and the argument path gate `pre-switch` hooks identically. if verify && !is_recovered { - run_pre_switch_hooks(repo, config, identifier, yes)?; + run_pre_switch_hooks(repo, config, identifier, ref_target.as_ref(), yes)?; } // Capture source (base) worktree identity BEFORE the switch, for @@ -1637,8 +1731,8 @@ impl SwitchPipeline<'_> { let (source_branch, source_path) = capture_switch_source(repo, is_recovered); // Validate and resolve the target branch. - let plan = plan_switch(repo, identifier, create, base, clobber, config).map_err(|err| { - match suggestion_ctx { + let plan = plan_switch(repo, identifier, ref_target, create, base, clobber, config) + .map_err(|err| match suggestion_ctx { Some(ref ctx) => match err.downcast::() { Ok(git_err) => GitError::WithSwitchSuggestion { source: Box::new(git_err), @@ -1648,8 +1742,7 @@ impl SwitchPipeline<'_> { Err(err) => err, }, None => err, - } - })?; + })?; // "Approve at the Gate": collect and approve hooks upfront. Approval // happens once at the command entry point. If the user declines, skip @@ -1716,8 +1809,14 @@ impl SwitchPipeline<'_> { // switches), or the branch they branched from (creates). "target" // matches the bare vars (the destination) — kept symmetric with // pre-switch. - let template_vars = + // `pr_number` / `pr_url` come from the resolved argument, not from + // `result` — a `pr:N` switch onto a branch that already has a worktree + // returns `SwitchResult::Existing`, which knows nothing about the PR. + let mut template_vars = TemplateVars::for_post_switch(&result, &branch_info, &source_branch, &source_path); + if let Some(identity) = &ref_identity { + template_vars = template_vars.with_pr(Some(identity.number), Some(&identity.url)); + } let extra_vars = template_vars.as_extra_vars(); // Spawn background hooks after the success message. diff --git a/src/commands/worktree/types.rs b/src/commands/worktree/types.rs index 47815cd02..eed29f9d6 100644 --- a/src/commands/worktree/types.rs +++ b/src/commands/worktree/types.rs @@ -33,12 +33,6 @@ pub enum SwitchResult { base_worktree_path: Option, /// Remote tracking branch if auto-created from remote (e.g., "origin/feature") from_remote: Option, - /// PR/MR number when created via `pr:N` / `mr:N` (carried into post-* hook - /// templates as `pr_number`). - pr_number: Option, - /// PR/MR web URL when created via `pr:N` / `mr:N` (carried into post-* hook - /// templates as `pr_url`). - pr_url: Option, }, } @@ -94,13 +88,28 @@ pub enum CreationMethod { /// URL to push to (the fork's URL). `None` when using a prefixed branch /// name (e.g., `contributor/main`) because push won't work. fork_push_url: Option, - /// Web URL for the PR/MR. - ref_url: String, /// Resolved remote name where PR/MR refs live (e.g., "origin", "upstream"). remote: String, }, } +/// Identity of the PR/MR a `pr:N` / `mr:N` argument resolved to. +/// +/// Resolved once, before `pre-switch` hooks run, then held by whoever needs it: +/// the `ResolvedTarget` `pre-switch` reads, the plan `pre-start` reads, and the +/// pipeline's own clone behind the post-* hooks — an `Existing` switch builds no +/// `SwitchPlan::Create` at all. So `pr_number` / `pr_url` reach every switch +/// hook whether the PR came from this repo or a fork. Reading it back off +/// [`CreationMethod::ForkRef`] used to be the only source, which silently left +/// same-repo PRs (the common case) with both variables unset. +#[derive(Debug, Clone)] +pub struct RefIdentity { + /// The PR/MR number the user typed. + pub number: u32, + /// Web URL of the PR/MR. + pub url: String, +} + /// Validated plan for a switch operation. /// /// Created by `plan_switch()`, consumed by `execute_switch()`. @@ -122,6 +131,10 @@ pub enum SwitchPlan { worktree_path: PathBuf, /// How to create the worktree method: CreationMethod, + /// The PR/MR this switch resolved from, when the argument was + /// `pr:N` / `mr:N`. Source of the `pr_number` / `pr_url` hook + /// variables. + ref_identity: Option, /// True when a stale path occupies `worktree_path` and `--clobber` was /// given — `execute_switch` backs it up before creating the worktree. needs_clobber_backup: bool, @@ -588,8 +601,6 @@ mod tests { base_branch: Some("main".to_string()), base_worktree_path: Some("/test/main".to_string()), from_remote: None, - pr_number: None, - pr_url: None, }, PathBuf::from("/test/created"), ), @@ -601,8 +612,6 @@ mod tests { base_branch: None, base_worktree_path: None, from_remote: Some("origin/feature".to_string()), - pr_number: None, - pr_url: None, }, PathBuf::from("/test/remote"), ), diff --git a/src/config/expansion.rs b/src/config/expansion.rs index aa9ac3e6c..a2d789733 100644 --- a/src/config/expansion.rs +++ b/src/config/expansion.rs @@ -238,9 +238,9 @@ fn hook_extras(hook_type: HookType) -> &'static [&'static str] { use HookType::*; match hook_type { // Switch: source branch (`base`) and destination (`target`). - // `pr_number`/`pr_url` are populated for `post-switch` when creating - // via `pr:N` / `mr:N`; pre-switch fires before the PR/MR API call, - // so they're never set there but remain accepted for portability. + // `pr_number`/`pr_url` are populated whenever the switch was named by + // `pr:N` / `mr:N` — the forge answers before `pre-switch` runs, so + // both hooks see the same values. PreSwitch | PostSwitch => &[ "base", "base_worktree_path", diff --git a/tests/integration_tests/switch.rs b/tests/integration_tests/switch.rs index 658de792d..8e4e06ded 100644 --- a/tests/integration_tests/switch.rs +++ b/tests/integration_tests/switch.rs @@ -3371,11 +3371,12 @@ fn test_switch_pr_fork(#[from(repo_with_remote)] repo: TestRepo) { }); } -/// `pre-start`, `post-start`, and `post-switch` hooks on PR/MR-created worktrees -/// see `pr_number` and `pr_url` in their template context. Both GitHub PRs and -/// GitLab MRs canonicalize to the same `pr_*` names — hook authors don't need -/// to branch on platform. Pre-switch fires before PR resolution and never sees -/// them. +/// Every hook on a PR/MR-created worktree — `pre-switch`, `pre-start`, +/// `post-start`, `post-switch` — sees `pr_number` and `pr_url` in its template +/// context. Both GitHub PRs and GitLab MRs canonicalize to the same `pr_*` +/// names — hook authors don't need to branch on platform. `pre-switch` fires +/// before the worktree exists but after the forge answered, so it names the +/// PR's branch rather than the raw `pr:N` token (#3934). #[rstest] fn test_switch_pr_hooks_see_pr_vars(#[from(repo_with_remote)] repo: TestRepo) { // Set up the same fork-PR scenario as test_switch_pr_fork: a refs/pull/42/head on the @@ -3404,7 +3405,8 @@ fn test_switch_pr_hooks_see_pr_vars(#[from(repo_with_remote)] repo: TestRepo) { fs::create_dir_all(repo.root_path().join(".config")).unwrap(); fs::write( repo.root_path().join(".config/wt.toml"), - r#"pre-start = "echo 'pr_number={{ pr_number }} pr_url={{ pr_url }}' > {{ repo_path }}/pre_start.txt" + r#"pre-switch = "echo 'pr_number={{ pr_number }} pr_url={{ pr_url }}' > {{ repo_path }}/pre_switch.txt" +pre-start = "echo 'pr_number={{ pr_number }} pr_url={{ pr_url }}' > {{ repo_path }}/pre_start.txt" post-start = "echo 'pr_number={{ pr_number }} pr_url={{ pr_url }}' > {{ repo_path }}/post_start.txt" post-switch = "echo 'pr_number={{ pr_number }} pr_url={{ pr_url }}' > {{ repo_path }}/post_switch.txt" "#, @@ -3462,7 +3464,12 @@ post-switch = "echo 'pr_number={{ pr_number }} pr_url={{ pr_url }}' > {{ repo_pa ); let expected = "pr_number=42 pr_url=https://github.com/owner/test-repo/pull/42"; - for marker in ["pre_start.txt", "post_start.txt", "post_switch.txt"] { + for marker in [ + "pre_switch.txt", + "pre_start.txt", + "post_start.txt", + "post_switch.txt", + ] { let path = repo.root_path().join(marker); // post-* hooks run in the background; poll until the file appears. wait_for_file_content(&path); @@ -3476,6 +3483,169 @@ post-switch = "echo 'pr_number={{ pr_number }} pr_url={{ pr_url }}' > {{ repo_pa } } +/// A **same-repo** `pr:N` switch has to fill the same template variables as a +/// fork one. Two things used to go missing here (#3934): `pre-switch` ran +/// before the forge was queried, so `branch` / `target` carried the literal +/// `pr:101` token; and `pr_number` / `pr_url` were read back off +/// `CreationMethod::ForkRef`, which a same-repo PR never produces — so the +/// common case saw them unset in every hook. +#[rstest] +fn test_switch_pr_same_repo_hooks_see_resolved_branch(#[from(repo_with_remote)] repo: TestRepo) { + // Push the PR's source branch to the remote, then drop the local branch so + // the switch takes the create path (the reporter's case: the PR's worktree + // does not exist yet). + repo.run_git(&["branch", "feature-auth"]); + repo.run_git(&["push", "origin", "feature-auth"]); + repo.run_git(&["branch", "-D", "feature-auth"]); + + set_github_remote_url(&repo); + + // `pre-switch` runs in the invoking worktree, before the destination + // exists — so it reports the branch it is switching to, not a path. + fs::create_dir_all(repo.root_path().join(".config")).unwrap(); + fs::write( + repo.root_path().join(".config/wt.toml"), + r#"pre-switch = "echo 'branch={{ branch }} target={{ target }} pr_number={{ pr_number }} pr_url={{ pr_url }}' > {{ repo_path }}/pre_switch.txt" +post-switch = "echo 'branch={{ branch }} target={{ target }} pr_number={{ pr_number }} pr_url={{ pr_url }}' > {{ repo_path }}/post_switch.txt" +"#, + ) + .unwrap(); + + let gh_response = r#"{ + "title": "Fix authentication bug in login flow", + "user": {"login": "alice"}, + "state": "open", + "draft": false, + "head": { + "ref": "feature-auth", + "repo": {"name": "test-repo", "owner": {"login": "owner"}} + }, + "base": { + "ref": "main", + "repo": {"name": "test-repo", "owner": {"login": "owner"}} + }, + "html_url": "https://github.com/owner/test-repo/pull/101" + }"#; + let mock_bin = setup_mock_gh_for_pr(&repo, gh_response); + + let mut cmd = repo.wt_command(); + cmd.args(["switch", "pr:101", "--yes"]); + configure_mock_cli_env(&mut cmd, &mock_bin); + let output = cmd.output().expect("wt switch pr:101 should run"); + assert!( + output.status.success(), + "wt switch pr:101 failed: stdout={}\nstderr={}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + + let expected = "branch=feature-auth target=feature-auth pr_number=101 pr_url=https://github.com/owner/test-repo/pull/101"; + for marker in ["pre_switch.txt", "post_switch.txt"] { + let path = repo.root_path().join(marker); + // post-switch runs in the background; poll until the file appears. + wait_for_file_content(&path); + let contents = fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("{marker} should have been written: {e}")); + assert_eq!( + contents.trim(), + expected, + "{marker} should name the PR's branch and carry its pr_* variables", + ); + } +} + +/// A second `wt switch pr:N`, onto the worktree the first one created, is the +/// path with nothing else to fall back on. It returns `SwitchResult::Existing`, +/// which carries no PR identity of its own and no base branch — so `pr_number` +/// / `pr_url` reach `post-switch` only because the pipeline keeps the resolved +/// identity past `plan_switch`, and `target_worktree_path` reaches `pre-switch` +/// only because the forge answered before the hook ran (#3934). Both go silently +/// missing again if either is dropped; the create-path tests above stay green. +#[rstest] +fn test_switch_pr_existing_worktree_hooks_see_pr_vars(#[from(repo_with_remote)] repo: TestRepo) { + repo.run_git(&["branch", "feature-auth"]); + repo.run_git(&["push", "origin", "feature-auth"]); + repo.run_git(&["branch", "-D", "feature-auth"]); + + set_github_remote_url(&repo); + + let gh_response = r#"{ + "title": "Fix authentication bug in login flow", + "user": {"login": "alice"}, + "state": "open", + "draft": false, + "head": { + "ref": "feature-auth", + "repo": {"name": "test-repo", "owner": {"login": "owner"}} + }, + "base": { + "ref": "main", + "repo": {"name": "test-repo", "owner": {"login": "owner"}} + }, + "html_url": "https://github.com/owner/test-repo/pull/101" + }"#; + let mock_bin = setup_mock_gh_for_pr(&repo, gh_response); + + let run_switch = |mock_bin: &std::path::Path| { + let mut cmd = repo.wt_command(); + cmd.args(["switch", "pr:101", "--yes"]); + configure_mock_cli_env(&mut cmd, mock_bin); + let output = cmd.output().expect("wt switch pr:101 should run"); + assert!( + output.status.success(), + "wt switch pr:101 failed: stdout={}\nstderr={}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + }; + + // First switch creates the worktree. Hooks are configured only afterwards, + // so the markers below can't be left over from the create path. + run_switch(&mock_bin); + + fs::create_dir_all(repo.root_path().join(".config")).unwrap(); + fs::write( + repo.root_path().join(".config/wt.toml"), + r#"pre-switch = "echo 'target={{ target }} target_worktree_path={{ target_worktree_path }} pr_number={{ pr_number }} pr_url={{ pr_url }}' > {{ repo_path }}/pre_switch.txt" +post-switch = "echo 'target={{ target }} pr_number={{ pr_number }} pr_url={{ pr_url }}' > {{ repo_path }}/post_switch.txt" +"#, + ) + .unwrap(); + + // Second switch lands on the existing worktree (the command runs from the + // primary worktree, so this is `Existing`, not `AlreadyAt`). + run_switch(&mock_bin); + + let post_switch = repo.root_path().join("post_switch.txt"); + wait_for_file_content(&post_switch); + assert_eq!( + fs::read_to_string(&post_switch).unwrap().trim(), + "target=feature-auth pr_number=101 pr_url=https://github.com/owner/test-repo/pull/101", + "post-switch on an existing worktree should still carry the PR identity", + ); + + let pre_switch = fs::read_to_string(repo.root_path().join("pre_switch.txt")) + .expect("pre_switch.txt should have been written"); + let pre_switch = pre_switch.trim(); + let dest = pre_switch + .split("target_worktree_path=") + .nth(1) + .and_then(|rest| rest.split(" pr_number=").next()) + .unwrap_or_else(|| panic!("no target_worktree_path in: {pre_switch}")); + assert!( + std::path::Path::new(dest).is_dir(), + "pre-switch `target_worktree_path` should name the branch's existing worktree, got: {dest}" + ); + assert!( + pre_switch.starts_with("target=feature-auth "), + "pre-switch should name the PR's branch, got: {pre_switch}" + ); + assert!( + pre_switch.ends_with("pr_number=101 pr_url=https://github.com/owner/test-repo/pull/101"), + "pre-switch should carry the PR identity, got: {pre_switch}" + ); +} + /// `wt switch pr:N` resolves `post-start` etc. from the **invoking** worktree's /// `.config/wt.toml` — not the PR's own committed config, even though the new /// worktree is a checkout of the PR head. The approval prompt lists those same