Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .config/clawpatch/features/feat_custom_signal_handling.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"schemaVersion": 1,
"featureId": "feat_custom_signal_handling",
"title": "Signal handling: Ctrl-C aborts every child-process loop",
"summary": "wt installs a signal_hook SIGINT/SIGTERM handler that forwards signals to child process groups, so wt itself does not die from the user's Ctrl-C, only the current child does. Every foreground loop that runs multiple child processes (`wt step for-each`, hook pipelines, alias steps, concurrent groups) MUST abort on a signal-derived child exit instead of charging into the next iteration: otherwise one Ctrl-C against `wt merge` runs the remaining hook steps, with FailureStrategy::Warn silently swallowing each interrupt. Detect via `err.interrupt_exit_code()` (the worktrunk::git::ErrorExt trait); the check happens before any FailureStrategy branch. The structured channel is WorktrunkError::ChildProcessExited { signal }, never sniff code >= 128. See CLAUDE.md > Signal Handling.",
"summary": "wt installs a signal_hook SIGINT/SIGTERM handler that forwards signals to child process groups, so wt itself does not die from the user's Ctrl-C, only the current child does. Every foreground loop that runs multiple child processes (`wt step for-each`, hook pipelines, alias steps, concurrent groups) MUST abort on a signal-derived child exit instead of charging into the next iteration: otherwise one Ctrl-C against `wt merge` runs the remaining hook steps, with FailureStrategy::Warn silently swallowing each interrupt. Detect via `err.interrupt_signal()` (the worktrunk::git::ErrorExt trait); the check happens before any FailureStrategy branch. The structured channel is WorktrunkError::ChildProcessExited { signal }, never sniff code >= 128. See CLAUDE.md > Signal Handling.",
"kind": "library",
"source": "manual",
"confidence": "high",
Expand All @@ -25,7 +25,7 @@
},
{
"path": "src/git/error.rs",
"reason": "ErrorExt::interrupt_exit_code and the WorktrunkError::ChildProcessExited signal channel"
"reason": "ErrorExt::interrupt_signal and the WorktrunkError::ChildProcessExited signal channel"
}
],
"contextFiles": [
Expand Down
7 changes: 4 additions & 3 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,11 @@ When a child process exits from a signal (SIGINT, SIGTERM), every loop in the fo

Why: wt installs a `signal_hook` SIGINT/SIGTERM handler so it can forward signals to child process groups before exiting cleanly. As a side effect wt itself does not die from the user's Ctrl-C — only the current child does. Without this policy a single Ctrl-C against `wt merge` would charge through the remaining hook steps, with `FailureStrategy::Warn` silently swallowing each interrupt.

- Signal-derived child exits surface as `WorktrunkError::ChildProcessExited { signal: Some(sig), .. }`. The `signal` field is the structured channel — never sniff `code >= 128` or parse error messages.
- Detect via `err.interrupt_exit_code()` (the `worktrunk::git::ErrorExt` trait). When it returns `Some(exit_code)`, propagate as `WorktrunkError::AlreadyDisplayed { exit_code }` (`128 + sig` by convention — 130 SIGINT, 143 SIGTERM) and break the loop.
- Signal-derived child exits surface structurally: stream mode (`Cmd::stream`) as `WorktrunkError::ChildProcessExited { signal: Some(sig), .. }`, capture mode (`Cmd::run`) as `CommandError { signal: Some(sig), .. }`. These fields are the structured channel — never sniff `code >= 128` or parse error messages.
- Detect via `err.interrupt_signal()` (the `worktrunk::git::ErrorExt` trait). When it returns `Some(signal)`, propagate as `WorktrunkError::Interrupted { signal, hint }` and break the loop. `Interrupted` exits `128 + signal` (130 SIGINT, 143 SIGTERM) and renders once, at exit, per shell convention: silent for SIGINT (the terminal echoed `^C`), `Terminated` for SIGTERM — the line the shell would print if wt weren't trapping the signal. `hint` carries an optional recovery line for state the interrupt left behind (e.g. a mid-rebase worktree).
- In capture mode only SIGINT/SIGTERM classify as interrupts. Capture children get no forwarding or escalation, and their captured output would be discarded by the silent exit — so a child killed by any other signal (a crash, an OOM kill) surfaces as a visible error instead. Stream mode counts any signal: output already streamed to the terminal, and user-initiated kills are normalized upstream to the originating SIGINT/SIGTERM (`seen_signal` in `shell_exec`, the concurrent runner's originating-signal override).
- The check happens **before** any `FailureStrategy` branch — Warn must NOT swallow signal-derived errors.
- `handle_command_error` in `src/commands/command_executor.rs` enforces this for hook and alias pipelines (foreground and concurrent groups); `for_each.rs` enforces it for the worktree loop. New code that loops over child processes calls `.interrupt_exit_code()` on per-iteration errors and breaks.
- `handle_command_error` in `src/commands/command_executor.rs` enforces this for hook and alias pipelines (foreground and concurrent groups); `for_each.rs` enforces it for the worktree loop. New code that loops over child processes calls `.interrupt_signal()` on per-iteration errors and breaks.

### Project Commands Run Only After Approval

Expand Down
21 changes: 12 additions & 9 deletions src/commands/command_executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ impl PreparedStep {
/// Receives the failing command, the message extracted from the inner error,
/// and the optional exit code (set when the inner error is
/// `WorktrunkError::ChildProcessExited`). Signal-derived errors bypass this
/// wrapper and short-circuit to `AlreadyDisplayed` — see
/// wrapper and short-circuit to `Interrupted` — see
/// [`handle_command_error`] for the rationale.
///
/// The wrapper is invoked on the calling thread after concurrent children
Expand Down Expand Up @@ -668,19 +668,19 @@ pub fn alias_error_wrapper(alias_name: String) -> ErrorWrapper {

/// Handle a command execution error via the step's `ErrorWrapper`.
///
/// Signal-derived child exits (SIGINT/SIGTERM) bypass the wrapper and
/// `failure_strategy`: the error is returned as `AlreadyDisplayed` with the
/// `128 + signal` exit code so the enclosing loop aborts. This enforces the
/// project-wide Ctrl-C cancellation policy — see the "Signal Handling"
/// section of the root `CLAUDE.md` for the rationale.
/// Signal-derived child exits bypass the wrapper and `failure_strategy`:
/// the error is returned as `Interrupted` (exiting `128 + signal`) so the
/// enclosing loop aborts. This enforces the project-wide Ctrl-C cancellation
/// policy — see the "Signal Handling" section of the root `CLAUDE.md` for
/// the rationale.
fn handle_command_error(
err: anyhow::Error,
cmd: &PreparedCommand,
error_wrapper: &ErrorWrapper,
failure_strategy: FailureStrategy,
) -> anyhow::Result<()> {
if let Some(exit_code) = err.interrupt_exit_code() {
return Err(WorktrunkError::AlreadyDisplayed { exit_code }.into());
if let Some(signal) = err.interrupt_signal() {
return Err(WorktrunkError::Interrupted { signal, hint: None }.into());
}

let (err_msg, exit_code) = if let Some(wt_err) = err.downcast_ref::<WorktrunkError>() {
Expand Down Expand Up @@ -895,7 +895,10 @@ mod tests {
let wt_err = err.downcast_ref::<WorktrunkError>().unwrap();
assert!(matches!(
wt_err,
WorktrunkError::AlreadyDisplayed { exit_code: 143 }
WorktrunkError::Interrupted {
signal: 15,
hint: None
}
));
}

Expand Down
32 changes: 20 additions & 12 deletions src/commands/custom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ use crate::enhance_clap_error;
/// for project-config aliases.
///
/// On success (child exit code 0), returns `Ok(())`. On non-zero exit, returns
/// `WorktrunkError::AlreadyDisplayed` with the child's exit code so `main`
/// `WorktrunkError::AlreadyDisplayed` with the child's exit code (or
/// `Interrupted` for a signal kill) so `main`
/// can propagate it without printing an extra error line. When the command
/// isn't found on PATH, returns `AlreadyDisplayed` via `enhance_clap_error`
/// with clap's standard exit code 2.
Expand Down Expand Up @@ -150,15 +151,19 @@ fn run_custom(path: &Path, args: &[OsString], working_dir: Option<&Path>) -> Res
return Ok(());
}

// Propagate the exact exit code — including signal codes on Unix — so
// `wt foo` behaves like running `wt-foo` directly. We use
// `AlreadyDisplayed` (not `ChildProcessExited`) because the custom
// command has already reported its own failure to the user; `wt` should
// just forward the exit code without adding a second error line.
// Propagate the exact exit status so `wt foo` behaves like running
// `wt-foo` directly. A signal kill surfaces as `Interrupted` (exit
// `128 + sig`, rendered per shell convention — a signal-killed child
// usually never got to report anything, and wt's own survival suppresses
// the shell's "Terminated" line). A plain non-zero exit surfaces as
// `AlreadyDisplayed` (not `ChildProcessExited`): the custom command
// already reported its own failure, so `wt` just forwards the code
// without adding a second error line.
#[cfg(unix)]
if let Some(sig) = std::os::unix::process::ExitStatusExt::signal(&status) {
return Err(WorktrunkError::AlreadyDisplayed {
exit_code: 128 + sig,
return Err(WorktrunkError::Interrupted {
signal: sig,
hint: None,
}
.into());
}
Expand Down Expand Up @@ -252,14 +257,17 @@ mod tests {
let sh = Path::new("/bin/sh");
let args = [OsString::from("-c"), OsString::from("kill -TERM $$")];

use worktrunk::git::ErrorExt;

let err = run_custom(sh, &args, None).expect_err("child killed by SIGTERM");
let wt_err = err
.downcast_ref::<WorktrunkError>()
.expect("signal should surface as WorktrunkError::AlreadyDisplayed");
.expect("signal should surface as WorktrunkError::Interrupted");
match wt_err {
WorktrunkError::AlreadyDisplayed { exit_code } => {
// SIGTERM = 15, and the shell-style convention is 128 + signal.
assert_eq!(*exit_code, 128 + 15);
WorktrunkError::Interrupted { signal, hint: None } => {
assert_eq!(*signal, 15);
// The shell-style convention is 128 + signal.
assert_eq!(err.exit_code(), Some(143));
}
other => panic!("unexpected WorktrunkError variant: {other:?}"),
}
Expand Down
16 changes: 8 additions & 8 deletions src/commands/for_each.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ pub fn step_for_each(args: Vec<String>, format: crate::cli::SwitchFormat) -> any

let mut failed: Vec<String> = Vec::new();
let mut json_results: Vec<serde_json::Value> = Vec::new();
// Set when a child dies from a signal (Ctrl-C / SIGTERM). We abort the
// loop and propagate an equivalent exit code rather than visiting the
// remaining worktrees — the user asked for the work to stop.
// The signal a child died from (Ctrl-C / SIGTERM), if any. We abort the
// loop and propagate `Interrupted` rather than visiting the remaining
// worktrees — the user asked for the work to stop.
let mut interrupted: Option<i32> = None;
let total = worktrees.len();

Expand Down Expand Up @@ -113,7 +113,7 @@ pub fn step_for_each(args: Vec<String>, format: crate::cli::SwitchFormat) -> any
}
}
Err(err) => {
let signal_exit = err.interrupt_exit_code();
let signal_exit = err.interrupt_signal();
// Two error strings: a styled block for stderr (preserves
// hints from typed diagnostics) and a plain string for
// JSON (consumers shouldn't see ANSI codes or symbols).
Expand Down Expand Up @@ -153,15 +153,15 @@ pub fn step_for_each(args: Vec<String>, format: crate::cli::SwitchFormat) -> any
"error": json_detail,
}));
}
if let Some(code) = signal_exit {
interrupted = Some(code);
if let Some(signal) = signal_exit {
interrupted = Some(signal);
break;
}
}
}
}

if let Some(exit_code) = interrupted {
if let Some(signal) = interrupted {
if json_mode {
println!("{}", serde_json::to_string_pretty(&json_results)?);
} else {
Expand All @@ -171,7 +171,7 @@ pub fn step_for_each(args: Vec<String>, format: crate::cli::SwitchFormat) -> any
warning_message("Interrupted — skipped remaining worktrees")
);
}
return Err(WorktrunkError::AlreadyDisplayed { exit_code }.into());
return Err(WorktrunkError::Interrupted { signal, hint: None }.into());
}

if json_mode {
Expand Down
10 changes: 5 additions & 5 deletions src/commands/run_pipeline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ fn create_command_log(spec: &PipelineSpec, name: &str) -> anyhow::Result<fs::Fil
/// Signal-killed children surface as `WorktrunkError::ChildProcessExited`
/// with `signal: Some(sig)` and `code: 128 + sig`, matching the foreground
/// convention established by `shell_exec`. That lets `exit_code()` and
/// `interrupt_exit_code()` work consistently and the `wt hook run-pipeline`
/// `interrupt_signal()` work consistently and the `wt hook run-pipeline`
/// process exits 130 on SIGINT and 143 on SIGTERM — the expectation the
/// "Signal Handling" section of the project `CLAUDE.md` sets for every
/// command loop.
Expand Down Expand Up @@ -404,9 +404,9 @@ mod tests {
assert_eq!(code, expected_code, "exit code for {sig}");
assert_eq!(message, expected_msg, "message for {sig}");
assert_eq!(
err.interrupt_exit_code(),
Some(expected_code),
"interrupt_exit_code for {sig}",
err.interrupt_signal(),
Some(sig),
"interrupt_signal for {sig}"
);
}
}
Expand All @@ -421,6 +421,6 @@ mod tests {
assert_eq!(code, 2);
assert_eq!(message, "command failed with exit code 2: my-step");
// Non-signal errors must NOT trip the interrupt abort path.
assert_eq!(err.interrupt_exit_code(), None);
assert_eq!(err.interrupt_signal(), None);
}
}
Loading
Loading