Skip to content

Commit caa47cf

Browse files
max-sixtyclaude
andauthored
fix(hooks): approve post-switch against the removal's destination worktree (#2748)
When a worktree is removed, the `post-switch` hook runs in the *destination* worktree — where the user lands — which `prepare_worktree_removal` records as `RemoveResult.main_path` (the primary worktree, except cwd when the primary worktree is itself the removal target). But `wt remove`'s and `wt step prune`'s approval gates collected the `post-switch` commands to prompt for from `repo.home_path()` instead. They agree in the common case (`main_path == home_path()`), so nobody noticed — but in a bare repo, removing the default-branch worktree from a *different* worktree, `main_path == current_path` while the gate read `home_path()`, so the gate approved the primary's `post-switch` while the executor ran cwd's — an unapproved project command could run. (`wt merge` and the picker's `removal_hooks_approved`, added in #2746, already passed the destination correctly.) ## What changed `collect_remove_hook_commands` now takes `(removed_worktree_paths: &[&Path], destination_paths: &[&Path])` instead of `(primary_repo: &Repository, removed_worktree_paths)` — it collects `pre-remove` / `post-remove` from each removed worktree and `post-switch` from each destination (path-deduped, since the common case is the same primary repeated). New `RemoveResult::destination_path() -> Option<&Path>` returns `main_path` for `RemovedWorktree`, `None` for `BranchOnly` — a sibling of the existing `removed_worktree_path()`. Callers updated: - `wt remove` (single): `approve_remove(result.removed_worktree_path().as_slice(), result.destination_path().as_slice(), …)`. (Multi: the same projections over the plan list.) - `wt merge`: passes `&[destination_path]` — matches `finish_after_merge`'s `RemoveResult { main_path: destination_path, … }`. - `wt step prune`: passes `&[home_path()]` — a prune candidate is never the primary worktree (`gather_check_items` filters non-linked worktrees and the default-branch worktree), so each candidate's removal destination is always `home_path()`. - The picker's `removal_hooks_approved` gains a `main_path` param and passes `&[main_path]`. The `commands::hooks` "Which `.config/wt.toml` a hook reads" table row for `post-switch after a removal` now points at `RemoveResult::destination_path` and notes the bare-repo cwd case. ## Testing Behavior-neutral in every case the test suite exercises — `post-switch` only fires when `changed_directory` (the removed worktree was cwd), and in all of those sub-cases `main_path` resolves to `home_path()` anyway, so the old and new gates collect the same commands. The change closes the latent decoupling. No new test added: the only observable difference is the bare-repo-remove-the-primary-from-elsewhere corner, and a dedicated integration test for it would be disproportionate; the new code paths (`destination_path()` on both variants, the destination loop in `collect_remove_hook_commands`, the `main.rs` plumbing) are covered by the existing `wt remove` / `wt merge` / `wt step prune` integration tests and the `test_do_removal_*` picker unit tests. `cargo run -- hook pre-merge --yes` is green (3693 tests, lints, fmt, doctests). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 761c293 commit caa47cf

7 files changed

Lines changed: 108 additions & 57 deletions

File tree

src/commands/hooks.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
//! |---|---|---|
2626
//! | `pre-remove`, `post-remove` | the worktree being removed (`pre-remove` reads it on disk; `post-remove` reads a snapshot taken before removal, stashed on [`super::worktree::RemoveResult::RemovedWorktree::removed_project_config`]) | `output::handlers::execute_pre_remove_hooks_if_needed` and `output::handlers::spawn_hooks_after_remove`; the three approval call sites (`main.rs`'s `approve_remove` for `wt remove`, `merge::collect_merge_commands` for `wt merge`, `step::prune::approve_prune_hooks` for `wt step prune`) all delegate to [`super::project_config::collect_remove_hook_commands`] |
2727
//! | `post-merge` | the merge destination (target branch's worktree if it has one, otherwise the primary) | `worktree::finish::finish_after_merge`; approval in `merge::collect_merge_commands` |
28-
//! | `post-switch` after a removal | the post-removal working directory (where the user landed) — for `wt remove` / `wt step prune` the primary worktree, for `wt merge` the merge destination | `output::handlers::spawn_hooks_after_remove`; approval through `collect_remove_hook_commands` |
28+
//! | `post-switch` after a removal | the post-removal working directory the user lands in ([`super::worktree::RemoveResult::destination_path`]) — usually the primary worktree, cwd when the primary worktree is itself removed (bare repo), the merge destination for `wt merge` | `output::handlers::spawn_hooks_after_remove`; approval through `collect_remove_hook_commands` |
2929
//! | `pre-commit` / `post-commit` | the worktree being committed — the cwd worktree, or `<b>`'s worktree for `wt step commit --branch <b>` | `commands::commit`, `step::commit` (via `CommandEnv::for_branch`) |
3030
//! | `wt switch`'s `pre-start` / `post-start` / `post-switch` | the new (`--create`) or destination (`wt switch <existing>`) worktree | `worktree::switch` — `hook_repo_for_worktree` (execution); `switch_hook_project_config` (approval / pre-flight, see Snapshot paths below) |
3131
//! | `pre-switch`, `pre-merge`, `wt hook <type>`, aliases | the worktree `wt` was invoked in | the respective command entry points |

src/commands/merge.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,11 @@ fn collect_merge_commands(
127127
if verify && will_remove {
128128
let current_wt = repo.current_worktree();
129129
let feature_path = current_wt.path();
130+
// The feature worktree is removed; the user lands in the merge
131+
// destination, so `post-switch` reads `destination_path`'s config.
130132
all_commands.extend(collect_remove_hook_commands(
131-
&destination_repo,
132133
&[feature_path],
134+
&[destination_path],
133135
)?);
134136
}
135137
}

src/commands/picker/mod.rs

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,8 @@ impl PickerCollector {
204204
..
205205
} => {
206206
let main_repo = Repository::at(main_path)?;
207-
let verify = removal_hooks_approved(&main_repo, worktree_path, approvals)?;
207+
let verify =
208+
removal_hooks_approved(&main_repo, main_path, worktree_path, approvals)?;
208209
let mut announcer = HookAnnouncer::new(&main_repo, main_repo.user_config(), false);
209210
handle_remove_output(
210211
result,
@@ -353,17 +354,21 @@ impl CommandCollector for PickerCollector {
353354
/// Whether every `pre-remove` / `post-remove` / `post-switch` command this
354355
/// removal would run is already approved — a read-only check, no prompt.
355356
///
356-
/// `wt remove` / `wt merge` collect the same command set and prompt for it at
357-
/// the gate. The picker can't prompt mid-render, so it runs the removal's hooks
358-
/// only when they're already approved (e.g. from a prior `wt remove` /
359-
/// `wt merge`) and skips them otherwise — unapproved project commands must
360-
/// never run. See CLAUDE.md → "Project Commands Run Only After Approval".
357+
/// `main_path` is the post-removal destination (where `post-switch` reads its
358+
/// config, same as the executor); `worktree_path` is the worktree being
359+
/// removed (where `pre-remove` / `post-remove` read theirs). `wt remove` /
360+
/// `wt merge` collect the same command set and prompt for it at the gate; the
361+
/// picker can't prompt mid-render, so it runs the removal's hooks only when
362+
/// they're already approved (e.g. from a prior `wt remove` / `wt merge`) and
363+
/// skips them otherwise — unapproved project commands must never run. See
364+
/// CLAUDE.md → "Project Commands Run Only After Approval".
361365
fn removal_hooks_approved(
362366
main_repo: &Repository,
367+
main_path: &Path,
363368
worktree_path: &Path,
364369
approvals: &Approvals,
365370
) -> anyhow::Result<bool> {
366-
let commands = collect_remove_hook_commands(main_repo, &[worktree_path])?;
371+
let commands = collect_remove_hook_commands(&[worktree_path], &[main_path])?;
367372
if commands.is_empty() {
368373
return Ok(true); // nothing to run — `verify` is moot
369374
}
@@ -1176,7 +1181,7 @@ pub mod tests {
11761181
// Empty approvals → the hook is unapproved.
11771182
let approvals = Approvals::default();
11781183
assert!(
1179-
!removal_hooks_approved(&repo, &wt_path, &approvals).unwrap(),
1184+
!removal_hooks_approved(&repo, test.path(), &wt_path, &approvals).unwrap(),
11801185
"an unapproved pre-remove hook is not approved"
11811186
);
11821187

src/commands/project_config.rs

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -49,25 +49,31 @@ pub fn collect_commands_for_hooks(
4949
///
5050
/// Each `pre-remove` and `post-remove` reads the removed worktree's own
5151
/// `.config/wt.toml` — both hooks are *about* that worktree, and at approval
52-
/// time it's still on disk. `post-switch` reads `primary_repo`'s config (the
53-
/// post-removal working directory, where the user lands). For `wt merge`,
54-
/// `primary_repo` is the merge destination — same rule, different "primary".
52+
/// time it's still on disk. `post-switch` reads the destination worktree's
53+
/// config — the post-removal working directory the user lands in, which
54+
/// `prepare_worktree_removal` records as [`super::worktree::RemoveResult`]'s
55+
/// `main_path` (the primary worktree, except cwd when the primary worktree is
56+
/// itself being removed; the merge destination for `wt merge`). Pass those
57+
/// `destination_path()`s in — the gate must read the same config the executor
58+
/// will (`output::handlers::spawn_hooks_after_remove`).
5559
///
5660
/// No fallback to another worktree's config, mirroring the executors
5761
/// (`output::handlers::execute_pre_remove_hooks_if_needed` and the
5862
/// `post-remove` snapshot path in `spawn_hooks_after_remove`). A
5963
/// present-but-malformed worktree config surfaces as an error so the user
6064
/// fixes it rather than silently running a different one.
6165
///
62-
/// Templates are deduped so the approval prompt shows each command once.
66+
/// Templates are deduped so the approval prompt shows each command once — so
67+
/// `destination_paths` may contain duplicates (the common case: every removal
68+
/// lands in the same primary worktree).
6369
///
6470
/// Callers feed the result into [`super::command_approval::approve_command_batch`].
6571
/// `wt merge` prepends its own `pre-commit` / `post-commit` / `pre-merge` /
6672
/// `post-merge` commands to the same batch; `wt remove` and `wt step prune`
6773
/// approve the helper's output on its own.
6874
pub fn collect_remove_hook_commands(
69-
primary_repo: &Repository,
7075
removed_worktree_paths: &[&Path],
76+
destination_paths: &[&Path],
7177
) -> anyhow::Result<Vec<ApprovableCommand>> {
7278
let mut commands: Vec<ApprovableCommand> = Vec::new();
7379

@@ -83,8 +89,19 @@ pub fn collect_remove_hook_commands(
8389
}
8490
}
8591

86-
if let Some(cfg) = primary_repo.load_project_config()? {
87-
commands.extend(collect_commands_for_hooks(&cfg, &[HookType::PostSwitch]));
92+
// `destination_paths` is usually one worktree repeated (every removal lands
93+
// in the same primary) — read each one's config at most once.
94+
let mut seen_dests: HashSet<&Path> = HashSet::new();
95+
for &dest_path in destination_paths {
96+
if !seen_dests.insert(dest_path) {
97+
continue;
98+
}
99+
// Propagate a `Repository::at` failure rather than silently skipping
100+
// — same as the removed-worktree loop above. See #2708.
101+
let dest_repo = Repository::at(dest_path)?;
102+
if let Some(cfg) = dest_repo.load_project_config()? {
103+
commands.extend(collect_commands_for_hooks(&cfg, &[HookType::PostSwitch]));
104+
}
88105
}
89106

90107
let mut seen = HashSet::new();

src/commands/step/prune.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -389,8 +389,9 @@ fn render_dry_run(
389389
/// metadata and orphan-branch removals delete just the branch — no
390390
/// `pre-remove`/`post-remove`/`post-switch`). The integration checks haven't
391391
/// run yet, so every linked worktree is fed to the helper — its `pre-remove`
392-
/// approval is a superset of what executes. `post-remove` / `post-switch`
393-
/// resolve from the primary worktree's config (the same helper input). No
392+
/// approval is a superset of what executes. `post-switch` resolves from the
393+
/// primary worktree's config: a prune candidate is never the primary worktree,
394+
/// so each removal's `RemoveResult::destination_path()` is `home_path()`. No
394395
/// fallback between worktrees — each `.config/wt.toml` stands alone.
395396
///
396397
/// Returns `true` when hooks should run, `false` when the user declined.
@@ -401,7 +402,6 @@ fn approve_prune_hooks(
401402
yes: bool,
402403
) -> anyhow::Result<bool> {
403404
let primary_path = repo.home_path()?;
404-
let primary_repo = Repository::at(&primary_path)?;
405405

406406
let removed_worktree_paths: Vec<&Path> = check_items
407407
.iter()
@@ -410,7 +410,8 @@ fn approve_prune_hooks(
410410
_ => None,
411411
})
412412
.collect();
413-
let all_commands = collect_remove_hook_commands(&primary_repo, &removed_worktree_paths)?;
413+
let all_commands =
414+
collect_remove_hook_commands(&removed_worktree_paths, &[primary_path.as_path()])?;
414415

415416
if all_commands.is_empty() {
416417
return Ok(true);

src/commands/worktree/types.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,17 @@ impl RemoveResult {
227227
}
228228
}
229229

230+
/// Post-removal working directory — where the user lands, and the worktree
231+
/// whose `.config/wt.toml` `post-switch` reads. `None` for branch-only
232+
/// deletions (no worktree was removed, so nothing was switched away from).
233+
/// See the `main_path` field docs on [`RemoveResult::RemovedWorktree`].
234+
pub fn destination_path(&self) -> Option<&Path> {
235+
match self {
236+
RemoveResult::RemovedWorktree { main_path, .. } => Some(main_path),
237+
RemoveResult::BranchOnly { .. } => None,
238+
}
239+
}
240+
230241
/// Convert to a JSON value for structured output.
231242
pub fn to_json(&self) -> serde_json::Value {
232243
match self {

src/main.rs

Lines changed: 51 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -894,30 +894,33 @@ fn handle_remove_command(args: RemoveArgs, yes: bool) -> anyhow::Result<()> {
894894
}
895895

896896
// Helper: approve every command the removal will run, in one batch.
897-
// `pre-remove` runs in — and resolves its `.config/wt.toml` from —
898-
// each worktree being removed (no fallback to the primary worktree's
899-
// config, same rule as `execute_pre_remove_hooks_if_needed`);
900-
// `post-remove` and `post-switch` run in the primary worktree
901-
// afterwards and resolve their config from there. The shared helper
902-
// assembles both, dedup'd by template. Returns `true` when the prompt
903-
// was accepted or there was nothing to approve.
904-
let approve_remove = |removed_worktree_paths: &[&Path], yes: bool| -> anyhow::Result<bool> {
905-
let primary_path = repo.home_path()?;
906-
let primary_repo = Repository::at(&primary_path)?;
907-
let commands =
908-
collect_remove_hook_commands(&primary_repo, removed_worktree_paths)?;
909-
if commands.is_empty() {
910-
return Ok(true);
911-
}
912-
let project_id = repo.project_identifier()?;
913-
let approvals = Approvals::load().context("Failed to load approvals")?;
914-
let approved =
915-
approve_command_batch(&commands, &project_id, &approvals, yes, false)?;
916-
if !approved {
917-
eprintln!("{}", info_message("Commands declined, continuing removal"));
918-
}
919-
Ok(approved)
920-
};
897+
// `pre-remove` / `post-remove` resolve their `.config/wt.toml` from
898+
// each worktree being removed (`removed_worktree_paths`); `post-switch`
899+
// resolves from each removal's post-removal destination
900+
// (`destination_paths` — `RemoveResult::destination_path()`, normally
901+
// the primary worktree, cwd when the primary worktree is itself the
902+
// removal target). No fallback between worktrees, same rule as the
903+
// executors. The shared helper assembles them all, dedup'd by template.
904+
// Returns `true` when the prompt was accepted or there was nothing to
905+
// approve.
906+
let approve_remove =
907+
|removed_worktree_paths: &[&Path], destination_paths: &[&Path], yes: bool| -> anyhow::Result<bool> {
908+
let commands = collect_remove_hook_commands(
909+
removed_worktree_paths,
910+
destination_paths,
911+
)?;
912+
if commands.is_empty() {
913+
return Ok(true);
914+
}
915+
let project_id = repo.project_identifier()?;
916+
let approvals = Approvals::load().context("Failed to load approvals")?;
917+
let approved =
918+
approve_command_batch(&commands, &project_id, &approvals, yes, false)?;
919+
if !approved {
920+
eprintln!("{}", info_message("Commands declined, continuing removal"));
921+
}
922+
Ok(approved)
923+
};
921924

922925
let branches = args.branches;
923926

@@ -941,8 +944,12 @@ fn handle_remove_command(args: RemoveArgs, yes: bool) -> anyhow::Result<()> {
941944
}
942945

943946
// "Approve at the Gate": approval happens AFTER validation passes
944-
let removed_path = result.removed_worktree_path();
945-
let run_hooks = verify && approve_remove(removed_path.as_slice(), yes)?;
947+
let run_hooks = verify
948+
&& approve_remove(
949+
result.removed_worktree_path().as_slice(),
950+
result.destination_path().as_slice(),
951+
yes,
952+
)?;
946953

947954
let mut announcer = HookAnnouncer::new(&repo, &config, false);
948955
handle_remove_output(
@@ -984,16 +991,24 @@ fn handle_remove_command(args: RemoveArgs, yes: bool) -> anyhow::Result<()> {
984991
}
985992

986993
// Approve hooks (only if we have valid plans). Each removed
987-
// worktree's `pre-remove` is approved against that worktree's
988-
// own config — see `approve_remove` above.
989-
let pre_remove_targets: Vec<&Path> = plans
990-
.others
991-
.iter()
992-
.chain(&plans.branch_only)
993-
.chain(plans.current.iter())
994-
.filter_map(|r| r.removed_worktree_path())
995-
.collect();
996-
let run_hooks = verify && approve_remove(&pre_remove_targets, yes)?;
994+
// worktree's `pre-remove` / `post-remove` is approved against
995+
// that worktree's config, and its `post-switch` against the
996+
// worktree the user lands in — see `approve_remove` above.
997+
// (`destination_targets` is mostly the primary worktree
998+
// repeated; the helper dedups by template.)
999+
let all_plans = || {
1000+
plans
1001+
.others
1002+
.iter()
1003+
.chain(&plans.branch_only)
1004+
.chain(plans.current.iter())
1005+
};
1006+
let removed_targets: Vec<&Path> =
1007+
all_plans().filter_map(|r| r.removed_worktree_path()).collect();
1008+
let destination_targets: Vec<&Path> =
1009+
all_plans().filter_map(|r| r.destination_path()).collect();
1010+
let run_hooks =
1011+
verify && approve_remove(&removed_targets, &destination_targets, yes)?;
9971012

9981013
// Execute all validated plans: others first, branch-only next, current last
9991014
let show_branch =

0 commit comments

Comments
 (0)