From 38e143d7cf2e48a120b6939032abf30d8347a695 Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 27 Jul 2026 09:14:13 +0000 Subject: [PATCH 1/4] fix(shell): corrected kill signal handling Accepted numeric signal specifications, signaled every process operand, and restored SIGTERM as the default. Added process-level regressions for multi-target SIGKILL and graceful default termination. Fixes #6779 --- crates/pi-shell/src/shell.rs | 112 +++++++++++++++++++++++ crates/vendor/brush-builtins/src/kill.rs | 34 +++---- packages/coding-agent/CHANGELOG.md | 4 + 3 files changed, 129 insertions(+), 21 deletions(-) diff --git a/crates/pi-shell/src/shell.rs b/crates/pi-shell/src/shell.rs index 99b332000..72601a333 100644 --- a/crates/pi-shell/src/shell.rs +++ b/crates/pi-shell/src/shell.rs @@ -2174,8 +2174,120 @@ fn uutils_env_disabled(config: &ShellConfig, key: &str) -> bool { #[cfg(test)] mod tests { + #[cfg(unix)] + use std::os::unix::process::ExitStatusExt as _; + + #[cfg(unix)] + use tokio::process::Command; + use super::*; + #[cfg(unix)] + async fn kill_test_context() -> (ShellSessionCore, ExecutionParameters) { + let config = ShellConfig { session_env: None, snapshot_path: None, minimizer: None }; + let mut session = create_session(&config).await.expect("create_session"); + let mut params = session.shell.default_exec_params(); + params.set_fd(OpenFiles::STDIN_FD, null_file().expect("null stdin")); + params.set_fd(OpenFiles::STDOUT_FD, null_file().expect("null stdout")); + params.set_fd(OpenFiles::STDERR_FD, null_file().expect("null stderr")); + (session, params) + } + + /// The kill builtin accepts a numeric signal and applies it to every process + /// operand. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread")] + async fn kill_builtin_accepts_numeric_signal_for_multiple_processes() { + let mut first = Command::new("sleep") + .arg("30") + .spawn() + .expect("first sleep"); + let mut second = Command::new("sleep") + .arg("30") + .spawn() + .expect("second sleep"); + let first_pid = first.id().expect("first pid"); + let second_pid = second.id().expect("second pid"); + let (mut session, params) = kill_test_context().await; + let source_info = SourceInfo::from("pi-natives:test"); + + let result = session + .shell + .run_string(format!("kill -9 {first_pid} {second_pid}"), &source_info, ¶ms) + .await + .expect("kill command"); + let code = exit_code(&result); + if code != 0 { + let _ = first.kill().await; + let _ = second.kill().await; + let _ = first.wait().await; + let _ = second.wait().await; + assert_eq!(code, 0, "numeric multi-process kill should succeed"); + } + + let statuses = + time::timeout(Duration::from_secs(5), async { tokio::join!(first.wait(), second.wait()) }) + .await; + if statuses.is_err() { + let _ = first.kill().await; + let _ = second.kill().await; + let _ = first.wait().await; + let _ = second.wait().await; + panic!("kill must signal every process operand"); + } + let (first_status, second_status) = statuses.expect("checked timeout"); + assert_eq!(first_status.expect("first wait").signal(), Some(libc::SIGKILL)); + assert_eq!(second_status.expect("second wait").signal(), Some(libc::SIGKILL)); + } + + /// The kill builtin defaults to SIGTERM so processes can shut down + /// gracefully. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread")] + async fn kill_builtin_defaults_to_sigterm() { + let dir = tempfile::tempdir().expect("temp dir"); + let ready = dir.path().join("ready"); + let mut child = Command::new("sh") + .args([ + "-c", + "trap 'exit 42' TERM; : > \"$1\"; while :; do :; done", + "sh", + ready.to_str().expect("utf8 path"), + ]) + .spawn() + .expect("trapping child"); + let pid = child.id().expect("child pid"); + let ready_result = time::timeout(Duration::from_secs(5), async { + while !ready.exists() { + time::sleep(Duration::from_millis(10)).await; + } + }) + .await; + if ready_result.is_err() { + let _ = child.kill().await; + let _ = child.wait().await; + panic!("child did not install its SIGTERM trap"); + } + + let (mut session, params) = kill_test_context().await; + let source_info = SourceInfo::from("pi-natives:test"); + let result = session + .shell + .run_string(format!("kill {pid}"), &source_info, ¶ms) + .await + .expect("kill command"); + let code = exit_code(&result); + + let status = time::timeout(Duration::from_secs(5), child.wait()).await; + if status.is_err() { + let _ = child.kill().await; + let _ = child.wait().await; + panic!("default kill signal did not terminate the child"); + } + assert_eq!(code, 0, "default kill should succeed"); + assert_eq!(status.expect("checked timeout").expect("child wait").code(), Some(42)); + } + /// `cmp` remains available with no executable search path, proving the shell /// dispatches the in-process builtin rather than a platform binary. #[tokio::test(flavor = "multi_thread")] diff --git a/crates/vendor/brush-builtins/src/kill.rs b/crates/vendor/brush-builtins/src/kill.rs index 82c747bc6..7ce3b4008 100644 --- a/crates/vendor/brush-builtins/src/kill.rs +++ b/crates/vendor/brush-builtins/src/kill.rs @@ -32,8 +32,8 @@ impl builtins::Command for KillCommand { &self, context: brush_core::ExecutionContext<'_, SE>, ) -> Result { - // Default signal is SIGKILL. - let mut trap_signal = TrapSignal::Signal(nix::sys::signal::Signal::SIGKILL); + // Match shell and POSIX defaults by allowing graceful termination. + let mut trap_signal = TrapSignal::Signal(nix::sys::signal::Signal::SIGTERM); // Try parsing the signal name (if specified). if let Some(signal_name) = &self.signal_name { @@ -67,39 +67,31 @@ impl builtins::Command for KillCommand { } } - // Look through the remaining args for a pid/job spec or a -sigspec style - // option. - let mut pid_or_job_spec = None; + // Look through the remaining args for process operands or a -sigspec option. + let mut saw_pid_or_job_spec = false; for arg in &self.args { - // See if this is -sigspec syntax. if let Some(possible_sigspec) = arg.strip_prefix("-") { - // See if this is -sigspec syntax. - if let Ok(parsed_trap_signal) = TrapSignal::try_from(possible_sigspec) { + // 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; } else { writeln!(context.stderr(), "{}: invalid signal name", context.command_name)?; return Ok(ExecutionExitCode::InvalidUsage.into()); } - } else if pid_or_job_spec.is_none() { - pid_or_job_spec = Some(arg); } else { - writeln!( - context.stderr(), - "{}: too many jobs or processes specified", - context.command_name - )?; - return Ok(ExecutionExitCode::InvalidUsage.into()); + saw_pid_or_job_spec = true; } } if self.list_signals { return print_signals(&context, self.args.as_ref()); - } else { - let Some(pid_or_job_spec) = pid_or_job_spec else { - writeln!(context.stderr(), "{}: invalid usage", context.command_name)?; - return Ok(ExecutionExitCode::InvalidUsage.into()); - }; + } + if !saw_pid_or_job_spec { + writeln!(context.stderr(), "{}: invalid usage", context.command_name)?; + return Ok(ExecutionExitCode::InvalidUsage.into()); + } + for pid_or_job_spec in self.args.iter().filter(|arg| !arg.starts_with('-')) { if pid_or_job_spec.starts_with('%') { // It's a job spec. if let Some(job) = context.shell.jobs_mut().resolve_job_spec(pid_or_job_spec) { diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 8278d1171..9c9e5f79e 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed the bash tool's `kill` builtin rejecting numeric signals and multiple process operands, and changed its default signal from `SIGKILL` to the standard `SIGTERM` ([#6779](https://github.com/can1357/oh-my-pi/issues/6779)). + ## [17.1.5] - 2026-07-27 ### Added From fb6a62c227c65b87c5804d59ddc07d601331e215 Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 27 Jul 2026 09:27:57 +0000 Subject: [PATCH 2/4] fix(shell): preserved negative pid operands in kill Restricted -sigspec parsing to the option position and consumed the -- end-of-options marker, so negative PIDs (process groups) and post-marker operands are signaled rather than parsed as signals. Added a process-group regression covering kill -TERM -- - . Fixes #6779 --- crates/pi-shell/src/shell.rs | 69 ++++++++++++++++++++++++ crates/vendor/brush-builtins/src/kill.rs | 52 +++++++++++++----- packages/coding-agent/CHANGELOG.md | 2 +- 3 files changed, 108 insertions(+), 15 deletions(-) diff --git a/crates/pi-shell/src/shell.rs b/crates/pi-shell/src/shell.rs index 72601a333..011b95dfd 100644 --- a/crates/pi-shell/src/shell.rs +++ b/crates/pi-shell/src/shell.rs @@ -2288,6 +2288,75 @@ mod tests { assert_eq!(status.expect("checked timeout").expect("child wait").code(), Some(42)); } + /// A negative PID after `--` targets a process group per `kill(2)` instead + /// of being parsed as a numeric signal, and a plain PID in the same command + /// is still signaled. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread")] + async fn kill_builtin_preserves_negative_pid_process_group_operands() { + let dir = tempfile::tempdir().expect("temp dir"); + let group_ready = dir.path().join("group-ready"); + let plain_ready = dir.path().join("plain-ready"); + let spawn_trapping = |ready: &std::path::Path, own_group: bool| { + let mut cmd = Command::new("sh"); + cmd.args([ + "-c", + "trap 'exit 42' TERM; : > \"$1\"; while :; do sleep 0.05; done", + "sh", + ready.to_str().expect("utf8 path"), + ]); + if own_group { + // pgid becomes this child's own pid, so `-pid` addresses the group. + cmd.process_group(0); + } + cmd.spawn().expect("trapping child") + }; + + let mut group_child = spawn_trapping(&group_ready, true); + let mut plain_child = spawn_trapping(&plain_ready, false); + let group_pid = group_child.id().expect("group pid"); + let plain_pid = plain_child.id().expect("plain pid"); + + let ready_result = time::timeout(Duration::from_secs(5), async { + while !group_ready.exists() || !plain_ready.exists() { + time::sleep(Duration::from_millis(10)).await; + } + }) + .await; + if ready_result.is_err() { + let _ = group_child.start_kill(); + let _ = plain_child.start_kill(); + let _ = group_child.wait().await; + let _ = plain_child.wait().await; + panic!("children did not install their SIGTERM traps"); + } + + let (mut session, params) = kill_test_context().await; + let source_info = SourceInfo::from("pi-natives:test"); + let result = session + .shell + .run_string(format!("kill -TERM -- -{group_pid} {plain_pid}"), &source_info, ¶ms) + .await + .expect("kill command"); + let code = exit_code(&result); + + let statuses = time::timeout(Duration::from_secs(5), async { + tokio::join!(group_child.wait(), plain_child.wait()) + }) + .await; + if statuses.is_err() { + let _ = group_child.start_kill(); + let _ = plain_child.start_kill(); + let _ = group_child.wait().await; + let _ = plain_child.wait().await; + panic!("negative-PID kill must signal the process group and the plain PID"); + } + let (group_status, plain_status) = statuses.expect("checked timeout"); + assert_eq!(code, 0, "negative-PID kill should succeed"); + assert_eq!(group_status.expect("group wait").code(), Some(42)); + assert_eq!(plain_status.expect("plain wait").code(), Some(42)); + } + /// `cmp` remains available with no executable search path, proving the shell /// dispatches the in-process builtin rather than a platform binary. #[tokio::test(flavor = "multi_thread")] diff --git a/crates/vendor/brush-builtins/src/kill.rs b/crates/vendor/brush-builtins/src/kill.rs index 7ce3b4008..24e4f9625 100644 --- a/crates/vendor/brush-builtins/src/kill.rs +++ b/crates/vendor/brush-builtins/src/kill.rs @@ -67,31 +67,55 @@ impl builtins::Command for KillCommand { } } - // Look through the remaining args for process operands or a -sigspec option. - let mut saw_pid_or_job_spec = false; + // 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. + let mut operands: Vec<&String> = Vec::new(); + let mut options_done = false; + let mut consumed_end_of_options = false; for arg in &self.args { - if let Some(possible_sigspec) = arg.strip_prefix("-") { - // 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; - } else { - writeln!(context.stderr(), "{}: invalid signal name", context.command_name)?; - return Ok(ExecutionExitCode::InvalidUsage.into()); - } - } else { - saw_pid_or_job_spec = true; + // The first `--` ends option parsing and is not itself an operand. + if !consumed_end_of_options && arg == "--" { + consumed_end_of_options = 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 { + 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; + }, } } if self.list_signals { return print_signals(&context, self.args.as_ref()); } - if !saw_pid_or_job_spec { + if operands.is_empty() { writeln!(context.stderr(), "{}: invalid usage", context.command_name)?; return Ok(ExecutionExitCode::InvalidUsage.into()); } - for pid_or_job_spec in self.args.iter().filter(|arg| !arg.starts_with('-')) { + for pid_or_job_spec in operands { if pid_or_job_spec.starts_with('%') { // It's a job spec. if let Some(job) = context.shell.jobs_mut().resolve_job_spec(pid_or_job_spec) { diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 9c9e5f79e..9d3771346 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed the bash tool's `kill` builtin rejecting numeric signals and multiple process operands, and changed its default signal from `SIGKILL` to the standard `SIGTERM` ([#6779](https://github.com/can1357/oh-my-pi/issues/6779)). +- Fixed the bash tool's `kill` builtin rejecting numeric signals and multiple process operands, and changed its default signal from `SIGKILL` to the standard `SIGTERM`. Negative PID operands (process groups per `kill(2)`) and the `--` end-of-options marker are now handled instead of being misparsed as signals ([#6779](https://github.com/can1357/oh-my-pi/issues/6779)). ## [17.1.5] - 2026-07-27 From 0e0fa77a5197fe482760ecd1e2269b3a65c18615 Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 27 Jul 2026 09:32:21 +0000 Subject: [PATCH 3/4] fix(shell): continued kill after target failures Recorded per-target PID and jobspec errors while continuing through every remaining operand, then returned a non-zero aggregate status. Added a regression with a stale PID between two live processes. Fixes #6779 --- crates/pi-shell/src/shell.rs | 48 ++++++++++++++++++++++++ crates/vendor/brush-builtins/src/kill.rs | 30 +++++++++++---- packages/coding-agent/CHANGELOG.md | 2 +- 3 files changed, 72 insertions(+), 8 deletions(-) diff --git a/crates/pi-shell/src/shell.rs b/crates/pi-shell/src/shell.rs index 011b95dfd..df419ceef 100644 --- a/crates/pi-shell/src/shell.rs +++ b/crates/pi-shell/src/shell.rs @@ -2357,6 +2357,54 @@ mod tests { assert_eq!(plain_status.expect("plain wait").code(), Some(42)); } + /// A failed target makes `kill` return non-zero without preventing later + /// process operands from receiving the selected signal. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread")] + async fn kill_builtin_continues_after_target_failure() { + let mut first = Command::new("sleep") + .arg("30") + .spawn() + .expect("first sleep"); + let mut second = Command::new("sleep") + .arg("30") + .spawn() + .expect("second sleep"); + let mut stale = Command::new("true").spawn().expect("stale process"); + let first_pid = first.id().expect("first pid"); + let second_pid = second.id().expect("second pid"); + let stale_pid = stale.id().expect("stale pid"); + stale.wait().await.expect("stale wait"); + + let (mut session, params) = kill_test_context().await; + let source_info = SourceInfo::from("pi-natives:test"); + let result = session + .shell + .run_string( + format!("kill -KILL {first_pid} {stale_pid} {second_pid}"), + &source_info, + ¶ms, + ) + .await + .expect("kill command"); + let code = exit_code(&result); + + let statuses = + time::timeout(Duration::from_secs(5), async { tokio::join!(first.wait(), second.wait()) }) + .await; + if statuses.is_err() { + let _ = first.start_kill(); + let _ = second.start_kill(); + let _ = first.wait().await; + let _ = second.wait().await; + panic!("kill must continue signaling after an intermediate target fails"); + } + let (first_status, second_status) = statuses.expect("checked timeout"); + assert_ne!(code, 0, "a failed target should make kill return non-zero"); + assert_eq!(first_status.expect("first wait").signal(), Some(libc::SIGKILL)); + assert_eq!(second_status.expect("second wait").signal(), Some(libc::SIGKILL)); + } + /// `cmp` remains available with no executable search path, proving the shell /// dispatches the in-process builtin rather than a platform binary. #[tokio::test(flavor = "multi_thread")] diff --git a/crates/vendor/brush-builtins/src/kill.rs b/crates/vendor/brush-builtins/src/kill.rs index 24e4f9625..0b03d4c11 100644 --- a/crates/vendor/brush-builtins/src/kill.rs +++ b/crates/vendor/brush-builtins/src/kill.rs @@ -115,11 +115,12 @@ impl builtins::Command for KillCommand { return Ok(ExecutionExitCode::InvalidUsage.into()); } + let mut had_failure = false; for pid_or_job_spec in operands { - if pid_or_job_spec.starts_with('%') { + let signal_result = if pid_or_job_spec.starts_with('%') { // It's a job spec. if let Some(job) = context.shell.jobs_mut().resolve_job_spec(pid_or_job_spec) { - job.kill(trap_signal)?; + job.kill(trap_signal) } else { writeln!( context.stderr(), @@ -127,16 +128,31 @@ impl builtins::Command for KillCommand { context.command_name, pid_or_job_spec )?; - return Ok(ExecutionResult::general_error()); + had_failure = true; + continue; } } else { - let pid = brush_core::int_utils::parse(pid_or_job_spec.as_str(), 10)?; + brush_core::int_utils::parse(pid_or_job_spec.as_str(), 10) + .and_then(|pid| sys::signal::kill_process(pid, trap_signal)) + }; - // It's a pid. - sys::signal::kill_process(pid, trap_signal)?; + if let Err(err) = signal_result { + writeln!( + context.stderr(), + "{}: {}: {}", + context.command_name, + pid_or_job_spec, + err + )?; + had_failure = true; } } - Ok(ExecutionResult::success()) + + if had_failure { + Ok(ExecutionResult::general_error()) + } else { + Ok(ExecutionResult::success()) + } } } diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 9d3771346..92c7a4124 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed the bash tool's `kill` builtin rejecting numeric signals and multiple process operands, and changed its default signal from `SIGKILL` to the standard `SIGTERM`. Negative PID operands (process groups per `kill(2)`) and the `--` end-of-options marker are now handled instead of being misparsed as signals ([#6779](https://github.com/can1357/oh-my-pi/issues/6779)). +- Fixed the bash tool's `kill` builtin rejecting numeric signals and multiple process operands, stopping after the first failed target, and defaulting to `SIGKILL` instead of the standard `SIGTERM`. Negative PID operands (process groups per `kill(2)`) and the `--` end-of-options marker are now handled instead of being misparsed as signals ([#6779](https://github.com/can1357/oh-my-pi/issues/6779)). ## [17.1.5] - 2026-07-27 From 0c1fd7f5d84f4b54bc6e87d70c40c2484e2df0d5 Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 27 Jul 2026 09:42:32 +0000 Subject: [PATCH 4/4] fix(shell): preserved negative pid when clap eats -- clap consumes the -- marker before execute for the default-signal and -s/-n forms, so kill -- -10 and kill -s TERM -- -10 previously misread the negative PID as a signal. Captured post-marker operands via a dedicated last=true field and treated a preselected -s/-n signal as closing the option position. Added a regression covering both marker-consumed forms. Fixes #6779 --- crates/pi-shell/src/shell.rs | 75 ++++++++++++++++++++++++ crates/vendor/brush-builtins/src/kill.rs | 65 +++++++++++--------- 2 files changed, 111 insertions(+), 29 deletions(-) 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());