diff --git a/src/commands/remove.rs b/src/commands/remove.rs index c4a84d1b3..bfb6de958 100644 --- a/src/commands/remove.rs +++ b/src/commands/remove.rs @@ -1,5 +1,8 @@ //! The `wt remove` command: validate removal targets, approve hooks, and //! dispatch each removal to the output handler. +//! +//! Target failures are reported independently; later targets still run and +//! the batch exits unsuccessfully. An interrupt cancels the batch immediately. use std::collections::HashSet; use std::path::Path; @@ -7,7 +10,9 @@ use std::path::Path; use anyhow::Context; use worktrunk::HookType; use worktrunk::config::UserConfig; -use worktrunk::git::{BranchDeletionMode, ErrorExt, GitError, Repository, ResolvedWorktree}; +use worktrunk::git::{ + BranchDeletionMode, ErrorExt, GitError, Repository, ResolvedWorktree, WorktrunkError, +}; use worktrunk::styling::{eprintln, info_message}; use crate::cli::{RemoveArgs, SwitchFormat}; @@ -548,27 +553,26 @@ pub fn handle_remove_command(args: RemoveArgs, yes: bool) -> anyhow::Result<()> announcer.flush()?; Ok(fate) }; - // Fates in execution order, which is also the JSON order below. - let mut fates = Vec::new(); - for result in &plans.others { - fates.push(run(result)?); - } - for result in &plans.branch_only { - fates.push(run(result)?); - } - if let Some(ref result) = plans.current { - fates.push(run(result)?); + let mut failed = !plans.errors.is_empty(); + let mut json_items = Vec::new(); + for result in all_plans() { + match run(result) { + Ok(fate) => { + if json_mode { + json_items.push(result.to_json(fate)); + } + } + Err(e) => { + if let Some(signal) = e.interrupt_signal() { + return Err(WorktrunkError::Interrupted { signal, hint: None }.into()); + } + crate::print_command_error(&e); + failed = true; + } + } } if json_mode { - let json_items: Vec = plans - .others - .iter() - .chain(&plans.branch_only) - .chain(plans.current.as_ref()) - .zip(fates) - .map(|(removal, fate)| removal.to_json(fate)) - .collect(); print_json(&json_items)?; } @@ -577,7 +581,7 @@ pub fn handle_remove_command(args: RemoveArgs, yes: bool) -> anyhow::Result<()> // it never delays the user-visible progress/success messages. super::process::run_internal_sweep(&repo); - if !plans.errors.is_empty() { + if failed { anyhow::bail!(""); } diff --git a/src/git/error.rs b/src/git/error.rs index 84d9321a3..b18859341 100644 --- a/src/git/error.rs +++ b/src/git/error.rs @@ -123,8 +123,8 @@ pub trait ErrorExt { /// short single-line label. /// /// Use this when embedding a sub-error's text inside another typed error's - /// message field (e.g., `GitError::WorktreeRemovalFailed::error`, - /// `GitError::PushFailed::error`) so the user sees git's real reason + /// message field (e.g., `GitError::PushFailed::error`) so the user sees git's + /// real reason /// rather than just the [`CommandError`] single-line summary. fn display_message(&self) -> String; @@ -382,7 +382,7 @@ impl SwitchSuggestionCtx { /// println!("branch {branch} already exists"); /// } /// ``` -#[derive(Debug, Clone)] +#[derive(Debug)] pub enum GitError { // Git state errors /// A worktree is not on a branch, so a command needing one refuses. @@ -539,7 +539,7 @@ pub enum GitError { WorktreeRemovalFailed { branch: String, path: PathBuf, - error: String, + error: anyhow::Error, /// Top-level entries remaining in the directory (for "Directory not empty" diagnostics) remaining_entries: Option>, }, @@ -720,7 +720,14 @@ pub enum GitError { }, } -impl std::error::Error for GitError {} +impl std::error::Error for GitError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + Self::WorktreeRemovalFailed { error, .. } => Some(error.as_ref()), + _ => None, + } + } +} /// `"1 path with unresolved conflicts"` — the shared tail of every message /// about an unmerged index. [`GitError::UnmergedPaths`] refuses outright; @@ -1335,8 +1342,9 @@ impl GitError { remaining_entries, .. } => { + let error = error.display_message(); let title = self.title(); - write!(f, "{}", format_error_block(error_message(&title), error))?; + write!(f, "{}", format_error_block(error_message(&title), &error))?; if let Some(entries) = remaining_entries { const MAX_SHOWN: usize = 10; let listing = if entries.len() > MAX_SHOWN { @@ -2336,7 +2344,7 @@ mod tests { GitError::WorktreeRemovalFailed { branch: "feature".into(), path: PathBuf::from("/tmp/repo.feature"), - error: "fatal: …".into(), + error: anyhow::anyhow!("fatal: …"), remaining_entries: None, }.to_string(), @"Failed to remove worktree for feature @ /tmp/repo.feature" @@ -2361,14 +2369,15 @@ mod tests { let inner = GitError::BranchAlreadyExists { branch: "feature".into(), }; + let expected = inner.to_string(); let wrapped = GitError::WithSwitchSuggestion { - source: Box::new(inner.clone()), + source: Box::new(inner), ctx: SwitchSuggestionCtx { extra_flags: vec!["--execute=claude".into()], trailing_args: vec![], }, }; - assert_eq!(inner.to_string(), wrapped.to_string()); + assert_eq!(expected, wrapped.to_string()); // WorktrunkError variants assert_snapshot!( @@ -2882,15 +2891,16 @@ mod tests { action: Some("merge".into()), worktree: None, }; + let expected = inner.to_string(); let wrapped = GitError::WithSwitchSuggestion { - source: Box::new(inner.clone()), + source: Box::new(inner), ctx: SwitchSuggestionCtx { extra_flags: vec!["--execute=claude".into()], trailing_args: vec!["Check my emails".into()], }, }; // Errors without switch suggestions should render identically - assert_eq!(inner.to_string(), wrapped.to_string()); + assert_eq!(expected, wrapped.to_string()); } fn sample_command_error() -> CommandError { diff --git a/src/main.rs b/src/main.rs index 56db44556..411d6527c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1338,8 +1338,8 @@ mod tests { assert!(out.contains("git fetch failed")); } - /// Codex P2: typed `GitError` wrappers (e.g., `WorktreeRemovalFailed`, - /// `PushFailed`) embed a stringified sub-error into their `error` + /// Typed `GitError` wrappers (e.g., `PushFailed`) embed a stringified + /// sub-error into their `error` /// field. With `display_message`, that field carries git's stderr /// rather than our `CommandError` summary. #[test] diff --git a/src/output/handlers.rs b/src/output/handlers.rs index 997acff0e..f8e4d374d 100644 --- a/src/output/handlers.rs +++ b/src/output/handlers.rs @@ -1916,7 +1916,7 @@ fn handle_detached_removed_worktree_output( branch: path_dir_name(ctx.worktree_path).to_string(), path: ctx.worktree_path.to_path_buf(), remaining_entries: list_remaining_entries(ctx.worktree_path), - error: err.display_message(), + error: err, })?; let (files, bytes) = output .staged_path @@ -1995,7 +1995,7 @@ fn handle_named_removed_worktree_foreground( branch: branch_name.into(), path: ctx.worktree_path.to_path_buf(), remaining_entries: list_remaining_entries(ctx.worktree_path), - error: err.display_message(), + error: err, })?; let stats = output .staged_path diff --git a/tests/integration_tests/git_error_display.rs b/tests/integration_tests/git_error_display.rs index 7feec1298..b7692aea8 100644 --- a/tests/integration_tests/git_error_display.rs +++ b/tests/integration_tests/git_error_display.rs @@ -42,7 +42,9 @@ fn worktree_errors_render() { GitError::WorktreeRemovalFailed { branch: "feature-x".into(), path: PathBuf::from("/tmp/repo.feature-x"), - error: "fatal: worktree is dirty\nerror: could not remove worktree".into(), + error: anyhow::anyhow!( + "fatal: worktree is dirty\nerror: could not remove worktree" + ), remaining_entries: None, } .render(), @@ -52,7 +54,9 @@ fn worktree_errors_render() { GitError::WorktreeRemovalFailed { branch: "feature-x".into(), path: PathBuf::from("/tmp/repo.feature-x"), - error: "error: failed to delete '/tmp/repo.feature-x': Directory not empty".into(), + error: anyhow::anyhow!( + "error: failed to delete '/tmp/repo.feature-x': Directory not empty" + ), remaining_entries: Some(vec![ ".vite/".into(), "node_modules/".into(), @@ -66,8 +70,9 @@ fn worktree_errors_render() { GitError::WorktreeRemovalFailed { branch: "feature-x".into(), path: PathBuf::from("/tmp/repo.feature-x"), - error: "error: failed to remove '/tmp/repo.feature-x/target': Permission denied" - .into(), + error: anyhow::anyhow!( + "error: failed to remove '/tmp/repo.feature-x/target': Permission denied" + ), remaining_entries: Some(vec!["target/".into()]), } .render(), @@ -77,7 +82,9 @@ fn worktree_errors_render() { GitError::WorktreeRemovalFailed { branch: "feature-x".into(), path: PathBuf::from("/tmp/repo.feature-x"), - error: "error: failed to delete '/tmp/repo.feature-x': Directory not empty".into(), + error: anyhow::anyhow!( + "error: failed to delete '/tmp/repo.feature-x': Directory not empty" + ), remaining_entries: Some((0..15).map(|i| format!("dir-{i:02}/")).collect()), } .render(), diff --git a/tests/integration_tests/remove.rs b/tests/integration_tests/remove.rs index f81e66ee0..6a5e00777 100644 --- a/tests/integration_tests/remove.rs +++ b/tests/integration_tests/remove.rs @@ -1071,6 +1071,196 @@ fn test_remove_partial_success(mut repo: TestRepo) { ); } +/// Execution failures belong to their target, just like validation failures. +/// Successful JSON entries must still name the right targets after failures, +/// including branch-only removal and the current worktree executed last. +#[rstest] +#[case::hook_foreground("exit 7", true, "hook_foreground")] +#[case::hook_background("exit 7", false, "hook_background")] +#[case::dirty_foreground("printf uncommitted > dirty.txt", true, "dirty_foreground")] +#[case::dirty_background("printf uncommitted > dirty.txt", false, "dirty_background")] +fn test_remove_continues_after_execution_failures( + mut repo: TestRepo, + #[case] failure: &str, + #[case] foreground: bool, + #[case] snapshot_name: &str, +) { + let hook = format!("case '{{{{ branch }}}}' in failed-*) {failure} ;; esac"); + repo.write_project_config(&format!("pre-remove = {hook:?}")); + repo.commit("Add conditional pre-remove hook"); + let failed_a = repo.add_worktree("failed-a"); + let valid = repo.add_worktree("valid"); + let failed_b = repo.add_worktree("failed-b"); + let current = repo.add_worktree("current"); + repo.create_branch("branch-only"); + + let mut cmd = repo.wt_command(); + // Select the logical current worktree without holding its directory open: + // Windows cannot remove a live process's physical working directory. + cmd.arg("-C").arg(¤t).args([ + "remove", + "current", + "failed-a", + "valid", + "failed-b", + "branch-only", + "--format=json", + "--yes", + ]); + if foreground { + cmd.arg("--foreground"); + } + let output = cmd.output().unwrap(); + let stderr = String::from_utf8_lossy(&output.stderr); + assert_eq!(output.status.code(), Some(1), "stderr:\n{stderr}"); + assert!( + failed_a.exists() && failed_b.exists(), + "failed targets must survive" + ); + assert!( + !valid.exists(), + "a later valid worktree must be removed; stderr:\n{stderr}" + ); + crate::common::wait_for_worktree_removed(¤t); + assert_branch_exists(&repo, "branch-only", false, &stderr); + + let json: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + let branches: Vec<_> = json + .as_array() + .unwrap() + .iter() + .map(|item| item["branch"].as_str().unwrap()) + .collect(); + assert_eq!(branches, ["valid", "branch-only", "current"]); + setup_snapshot_settings(&repo).bind(|| { + assert_snapshot!(format!("remove_continues_{snapshot_name}"), stderr); + }); +} + +/// Cancellation stops the batch before its next worktree. The hook signals +/// its own shell, exercising child signal identity without signaling cargo. +#[cfg(unix)] +#[rstest] +#[case::sigint("INT", 130)] +#[case::sigterm("TERM", 143)] +fn test_remove_interrupt_stops_batch( + mut repo: TestRepo, + #[case] signal: &str, + #[case] exit_code: i32, +) { + let hook = format!("kill -{signal} $$"); + repo.write_project_config(&format!("pre-remove = {hook:?}")); + repo.commit("Add interrupting pre-remove hook"); + let interrupted = repo.add_worktree("interrupted"); + let later = repo.add_worktree("later"); + let output = repo + .wt_command() + .args(["remove", "interrupted", "later", "--foreground", "--yes"]) + .output() + .unwrap(); + assert_eq!( + output.status.code(), + Some(exit_code), + "stderr:\n{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!( + interrupted.exists() && later.exists(), + "interrupt must stop all removals" + ); +} + +/// Signals from execution-time Git commands cancel removal just like hook +/// signals. The hook arms the shim only after validation has succeeded; +/// everything except the selected Git boundary delegates to real Git. +#[cfg(unix)] +#[rstest] +#[case::status_foreground("status", true, false, "INT", 130)] +#[case::status_background("status", false, false, "TERM", 143)] +#[case::delete_foreground("delete", true, false, "TERM", 143)] +#[case::delete_background("delete", false, false, "INT", 130)] +#[case::detached_status_foreground("status", true, true, "TERM", 143)] +fn test_remove_git_interrupt_stops_batch( + mut repo: TestRepo, + #[case] boundary: &str, + #[case] foreground: bool, + #[case] detached: bool, + #[case] signal: &str, + #[case] exit_code: i32, +) { + use std::os::unix::fs::PermissionsExt; + + repo.write_project_config("pre-remove = 'touch \"$WORKTRUNK_TEST_INTERRUPT_ARMED\"'"); + repo.commit("Add hook arming the Git interrupt shim"); + let interrupted = repo.add_worktree("interrupted"); + let later = repo.add_worktree("later"); + if detached { + repo.detach_head_in_worktree("interrupted"); + } + let armed = repo.home_path().join("interrupt-armed"); + let triggered = repo.home_path().join("interrupt-triggered"); + assert!(!armed.exists() && !triggered.exists()); + + let bin_dir = repo.home_path().join("git-wrapper"); + fs::create_dir_all(&bin_dir).unwrap(); + let git = bin_dir.join("git"); + let real_git = which::which("git").unwrap(); + let real_git = shell_escape::unix::escape(real_git.to_string_lossy()); + fs::write( + &git, + format!( + r#"#!/bin/sh +if [ -f "$WORKTRUNK_TEST_INTERRUPT_ARMED" ]; then + if {{ [ "$WORKTRUNK_TEST_INTERRUPT_BOUNDARY" = status ] && [ "$1" = status ] && [ "$PWD" = "$WORKTRUNK_TEST_INTERRUPT_WORKTREE" ]; }} || + {{ [ "$WORKTRUNK_TEST_INTERRUPT_BOUNDARY" = delete ] && [ "$1" = update-ref ] && [ "$3" = refs/heads/interrupted ]; }}; then + touch "$WORKTRUNK_TEST_INTERRUPT_TRIGGERED" + kill "-$WORKTRUNK_TEST_INTERRUPT_SIGNAL" "$$" + fi +fi +exec {real_git} "$@" +"# + ), + ) + .unwrap(); + fs::set_permissions(&git, fs::Permissions::from_mode(0o755)).unwrap(); + + let mut cmd = repo.wt_command(); + let mut paths: Vec<_> = std::env::split_paths(&std::env::var_os("PATH").unwrap()).collect(); + paths.insert(0, bin_dir); + cmd.env("PATH", std::env::join_paths(paths).unwrap()); + cmd.arg("remove") + .arg(&interrupted) + .args(["later", "--yes", "--format=json"]) + .env("WORKTRUNK_TEST_INTERRUPT_ARMED", &armed) + .env("WORKTRUNK_TEST_INTERRUPT_TRIGGERED", &triggered) + .env("WORKTRUNK_TEST_INTERRUPT_BOUNDARY", boundary) + .env("WORKTRUNK_TEST_INTERRUPT_SIGNAL", signal) + .env("WORKTRUNK_TEST_INTERRUPT_WORKTREE", &interrupted); + if foreground { + cmd.arg("--foreground"); + } + let output = cmd.output().unwrap(); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + armed.exists() && triggered.exists(), + "the selected execution-time Git command must receive the signal; stderr:\n{stderr}" + ); + assert!( + later.exists(), + "cancellation must preserve the later worktree; stderr:\n{stderr}" + ); + assert_branch_exists(&repo, "later", true, &stderr); + assert_branch_exists(&repo, "interrupted", true, &stderr); + if boundary == "status" { + assert!(interrupted.exists(), "the clean check precedes removal"); + } + assert_eq!(output.status.code(), Some(exit_code), "stderr:\n{stderr}"); + assert!( + output.stdout.is_empty(), + "a canceled batch must not publish success JSON" + ); +} + #[rstest] fn test_remove_by_name_dirty_target(mut repo: TestRepo) { let worktree_path = repo.add_worktree("feature-dirty"); diff --git a/tests/snapshots/integration__integration_tests__remove__remove_continues_dirty_background.snap b/tests/snapshots/integration__integration_tests__remove__remove_continues_dirty_background.snap new file mode 100644 index 000000000..5b0ae7f0b --- /dev/null +++ b/tests/snapshots/integration__integration_tests__remove__remove_continues_dirty_background.snap @@ -0,0 +1,27 @@ +--- +source: tests/integration_tests/remove.rs +expression: stderr +--- +◎ Running pre-remove project hook @ _REPO_.failed-a +  case 'failed-a' in failed-*) printf uncommitted > dirty.txt ;; esac +◎ Removing failed-a worktree & branch in background (same commit as main, _) +✗ Cannot remove worktree: failed-a has uncommitted changes +  ?? dirty.txt +↳ Commit or stash changes first, or to lose uncommitted changes, run wt remove --force failed-a +◎ Running pre-remove project hook @ _REPO_.valid +  case 'valid' in failed-*) printf uncommitted > dirty.txt ;; esac +◎ Removing valid worktree & branch in background (same commit as main, _) +◎ Running pre-remove project hook @ _REPO_.failed-b +  case 'failed-b' in failed-*) printf uncommitted > dirty.txt ;; esac +◎ Removing failed-b worktree & branch in background (same commit as main, _) +✗ Cannot remove worktree: failed-b has uncommitted changes +  ?? dirty.txt +↳ Commit or stash changes first, or to lose uncommitted changes, run wt remove --force failed-b +○ No worktree found for branch branch-only +✓ Removed branch branch-only (same commit as main, _) +◎ Running pre-remove project hook +  case 'current' in failed-*) printf uncommitted > dirty.txt ;; esac +◎ Removing current worktree & branch in background (same commit as main, _) +▲ Worktree for main @ _REPO_, but cannot change directory — shell integration not installed +↳ To enable automatic cd, run wt config shell install +↳ Current directory was removed; to see worktrees, run wt list diff --git a/tests/snapshots/integration__integration_tests__remove__remove_continues_dirty_foreground.snap b/tests/snapshots/integration__integration_tests__remove__remove_continues_dirty_foreground.snap new file mode 100644 index 000000000..068202f05 --- /dev/null +++ b/tests/snapshots/integration__integration_tests__remove__remove_continues_dirty_foreground.snap @@ -0,0 +1,29 @@ +--- +source: tests/integration_tests/remove.rs +expression: stderr +--- +◎ Running pre-remove project hook @ _REPO_.failed-a +  case 'failed-a' in failed-*) printf uncommitted > dirty.txt ;; esac +◎ Removing failed-a worktree... +✗ Failed to remove worktree for failed-a @ _REPO_.failed-a +  Cannot remove worktree: failed-a has uncommitted changes +↳ Remaining in directory: .config/, .git, .gitattributes, dirty.txt, file.txt +◎ Running pre-remove project hook @ _REPO_.valid +  case 'valid' in failed-*) printf uncommitted > dirty.txt ;; esac +◎ Removing valid worktree... +✓ Removed valid worktree & branch (same commit as main, _) (4 files · [BYTES] B) +◎ Running pre-remove project hook @ _REPO_.failed-b +  case 'failed-b' in failed-*) printf uncommitted > dirty.txt ;; esac +◎ Removing failed-b worktree... +✗ Failed to remove worktree for failed-b @ _REPO_.failed-b +  Cannot remove worktree: failed-b has uncommitted changes +↳ Remaining in directory: .config/, .git, .gitattributes, dirty.txt, file.txt +○ No worktree found for branch branch-only +✓ Removed branch branch-only (same commit as main, _) +◎ Running pre-remove project hook +  case 'current' in failed-*) printf uncommitted > dirty.txt ;; esac +◎ Removing current worktree... +✓ Removed current worktree & branch (same commit as main, _) (4 files · [BYTES] B) +▲ Worktree for main @ _REPO_, but cannot change directory — shell integration not installed +↳ To enable automatic cd, run wt config shell install +↳ Current directory was removed; to see worktrees, run wt list diff --git a/tests/snapshots/integration__integration_tests__remove__remove_continues_hook_background.snap b/tests/snapshots/integration__integration_tests__remove__remove_continues_hook_background.snap new file mode 100644 index 000000000..670517264 --- /dev/null +++ b/tests/snapshots/integration__integration_tests__remove__remove_continues_hook_background.snap @@ -0,0 +1,23 @@ +--- +source: tests/integration_tests/remove.rs +expression: stderr +--- +◎ Running pre-remove project hook @ _REPO_.failed-a +  case 'failed-a' in failed-*) exit 7 ;; esac +✗ pre-remove command failed: exit status: 7 +↳ To skip pre-remove hooks, re-run with --no-hooks +◎ Running pre-remove project hook @ _REPO_.valid +  case 'valid' in failed-*) exit 7 ;; esac +◎ Removing valid worktree & branch in background (same commit as main, _) +◎ Running pre-remove project hook @ _REPO_.failed-b +  case 'failed-b' in failed-*) exit 7 ;; esac +✗ pre-remove command failed: exit status: 7 +↳ To skip pre-remove hooks, re-run with --no-hooks +○ No worktree found for branch branch-only +✓ Removed branch branch-only (same commit as main, _) +◎ Running pre-remove project hook +  case 'current' in failed-*) exit 7 ;; esac +◎ Removing current worktree & branch in background (same commit as main, _) +▲ Worktree for main @ _REPO_, but cannot change directory — shell integration not installed +↳ To enable automatic cd, run wt config shell install +↳ Current directory was removed; to see worktrees, run wt list diff --git a/tests/snapshots/integration__integration_tests__remove__remove_continues_hook_foreground.snap b/tests/snapshots/integration__integration_tests__remove__remove_continues_hook_foreground.snap new file mode 100644 index 000000000..cea41b4b6 --- /dev/null +++ b/tests/snapshots/integration__integration_tests__remove__remove_continues_hook_foreground.snap @@ -0,0 +1,25 @@ +--- +source: tests/integration_tests/remove.rs +expression: stderr +--- +◎ Running pre-remove project hook @ _REPO_.failed-a +  case 'failed-a' in failed-*) exit 7 ;; esac +✗ pre-remove command failed: exit status: 7 +↳ To skip pre-remove hooks, re-run with --no-hooks +◎ Running pre-remove project hook @ _REPO_.valid +  case 'valid' in failed-*) exit 7 ;; esac +◎ Removing valid worktree... +✓ Removed valid worktree & branch (same commit as main, _) (4 files · [BYTES] B) +◎ Running pre-remove project hook @ _REPO_.failed-b +  case 'failed-b' in failed-*) exit 7 ;; esac +✗ pre-remove command failed: exit status: 7 +↳ To skip pre-remove hooks, re-run with --no-hooks +○ No worktree found for branch branch-only +✓ Removed branch branch-only (same commit as main, _) +◎ Running pre-remove project hook +  case 'current' in failed-*) exit 7 ;; esac +◎ Removing current worktree... +✓ Removed current worktree & branch (same commit as main, _) (4 files · [BYTES] B) +▲ Worktree for main @ _REPO_, but cannot change directory — shell integration not installed +↳ To enable automatic cd, run wt config shell install +↳ Current directory was removed; to see worktrees, run wt list