diff --git a/crates/pi-shell/src/shell.rs b/crates/pi-shell/src/shell.rs index df419ceef..7e20cf91c 100644 --- a/crates/pi-shell/src/shell.rs +++ b/crates/pi-shell/src/shell.rs @@ -2357,6 +2357,81 @@ mod tests { assert_eq!(plain_status.expect("plain wait").code(), Some(42)); } + /// When clap consumes the `--` marker before `execute` (the default-signal + /// and `-s SIG` forms), a following negative PID is still an operand, not a + /// signal: `kill -- -` defaults to SIGTERM for the group, and + /// `kill -s TERM -- -` sends the named signal to the group. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread")] + async fn kill_builtin_signals_group_when_marker_precedes_negative_pid() { + let dir = tempfile::tempdir().expect("temp dir"); + let default_ready = dir.path().join("default-ready"); + let named_ready = dir.path().join("named-ready"); + let spawn_group_leader = |ready: &std::path::Path| { + Command::new("sh") + .args([ + "-c", + "trap 'exit 42' TERM; : > \"$1\"; while :; do sleep 0.05; done", + "sh", + ready.to_str().expect("utf8 path"), + ]) + .process_group(0) + .spawn() + .expect("group leader") + }; + + let mut default_child = spawn_group_leader(&default_ready); + let mut named_child = spawn_group_leader(&named_ready); + let default_pid = default_child.id().expect("default pid"); + let named_pid = named_child.id().expect("named pid"); + + let ready_result = time::timeout(Duration::from_secs(5), async { + while !default_ready.exists() || !named_ready.exists() { + time::sleep(Duration::from_millis(10)).await; + } + }) + .await; + if ready_result.is_err() { + let _ = default_child.start_kill(); + let _ = named_child.start_kill(); + let _ = default_child.wait().await; + let _ = named_child.wait().await; + panic!("group leaders did not install their SIGTERM traps"); + } + + let (mut session, params) = kill_test_context().await; + let source_info = SourceInfo::from("pi-natives:test"); + // Default signal (SIGTERM) with the marker consumed by clap. + let default_result = session + .shell + .run_string(format!("kill -- -{default_pid}"), &source_info, ¶ms) + .await + .expect("default kill command"); + // Named signal via -s, marker consumed by clap. + let named_result = session + .shell + .run_string(format!("kill -s TERM -- -{named_pid}"), &source_info, ¶ms) + .await + .expect("named kill command"); + + let statuses = time::timeout(Duration::from_secs(5), async { + tokio::join!(default_child.wait(), named_child.wait()) + }) + .await; + if statuses.is_err() { + let _ = default_child.start_kill(); + let _ = named_child.start_kill(); + let _ = default_child.wait().await; + let _ = named_child.wait().await; + panic!("marker-preceded negative PID must signal the process group"); + } + let (default_status, named_status) = statuses.expect("checked timeout"); + assert_eq!(exit_code(&default_result), 0, "`kill -- -` should succeed"); + assert_eq!(exit_code(&named_result), 0, "`kill -s TERM -- -` should succeed"); + assert_eq!(default_status.expect("default wait").code(), Some(42)); + assert_eq!(named_status.expect("named wait").code(), Some(42)); + } + /// A failed target makes `kill` return non-zero without preventing later /// process operands from receiving the selected signal. #[cfg(unix)] diff --git a/crates/vendor/brush-builtins/src/kill.rs b/crates/vendor/brush-builtins/src/kill.rs index 0b03d4c11..4fd48677a 100644 --- a/crates/vendor/brush-builtins/src/kill.rs +++ b/crates/vendor/brush-builtins/src/kill.rs @@ -23,6 +23,12 @@ pub(crate) struct KillCommand { // Interpretation of these depends on whether -l is present. #[arg(allow_hyphen_values = true)] args: Vec, + + /// Process/job operands given after the `--` end-of-options marker. clap + /// consumes `--` before `execute`, so these are captured separately and are + /// always operands — never signal specifications (preserves negative PIDs). + #[arg(last = true, allow_hyphen_values = true)] + post_marker_args: Vec, } impl builtins::Command for KillCommand { @@ -67,45 +73,46 @@ impl builtins::Command for KillCommand { } } - // Parse the remaining args: an optional leading `-sigspec` in the option - // position, an optional `--` end-of-options marker, then pid/jobspec - // operands. A hyphen is only a sigspec while still in the option position; - // once a sigspec or an operand is seen, later hyphen-led args are operands - // so negative PIDs (process groups per `kill(2)`) survive — e.g. - // `kill -TERM -- -10 123` signals process group 10 and PID 123. + // Interpret the pre-`--` args: an optional leading `-sigspec` in the + // option position, then pid/jobspec operands. A hyphen leads a sigspec + // only in the option position — once a signal is chosen (via `-s`/`-n`, a + // leading `-sigspec`, or the `--` marker) or an operand is seen, later + // hyphen-led args are operands, so negative PIDs (process groups per + // `kill(2)`) survive: `kill -TERM -- -10 123` signals process group 10 and + // PID 123, and `kill -s TERM -- -10` signals process group 10. let mut operands: Vec<&String> = Vec::new(); - let mut options_done = false; - let mut consumed_end_of_options = false; + let mut options_done = self.signal_name.is_some() || self.signal_number.is_some(); + let mut consumed_marker = false; for arg in &self.args { - // The first `--` ends option parsing and is not itself an operand. - if !consumed_end_of_options && arg == "--" { - consumed_end_of_options = true; + // Whether clap leaves the `--` marker here depends on whether a + // positional value was already collected; consume the first one and + // close the option position either way. + if !consumed_marker && arg == "--" { + consumed_marker = true; options_done = true; continue; } - if options_done { - operands.push(arg); - continue; - } - match arg.strip_prefix('-') { - Some(possible_sigspec) if !possible_sigspec.is_empty() => { - // Option position: interpret as a signal specification. The - // sigspec may be a signal name (e.g. -TERM) or number (e.g. -9). - if let Ok(parsed_trap_signal) = possible_sigspec.parse::() { - trap_signal = parsed_trap_signal; - options_done = true; - } else { + if !options_done { + if let Some(possible_sigspec) = arg.strip_prefix('-') { + if !possible_sigspec.is_empty() { + // Option position: interpret as a signal specification. The + // sigspec may be a signal name (e.g. -TERM) or number (e.g. -9). + if let Ok(parsed_trap_signal) = possible_sigspec.parse::() { + trap_signal = parsed_trap_signal; + options_done = true; + continue; + } writeln!(context.stderr(), "{}: invalid signal name", context.command_name)?; return Ok(ExecutionExitCode::InvalidUsage.into()); } - }, - _ => { - // First operand ends the option position. - operands.push(arg); - options_done = true; - }, + } + // The first operand ends the option position. + options_done = true; } + operands.push(arg); } + // Operands after the `--` marker are always process/job specs. + operands.extend(self.post_marker_args.iter()); if self.list_signals { return print_signals(&context, self.args.as_ref());