diff --git a/crates/pi-shell/src/shell.rs b/crates/pi-shell/src/shell.rs index 99b332000..7e20cf91c 100644 --- a/crates/pi-shell/src/shell.rs +++ b/crates/pi-shell/src/shell.rs @@ -2174,8 +2174,312 @@ 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)); + } + + /// 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)); + } + + /// 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)] + #[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 82c747bc6..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 { @@ -32,8 +38,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,43 +73,61 @@ 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; + // 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 = self.signal_name.is_some() || self.signal_number.is_some(); + let mut consumed_marker = 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) { - 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()); + // 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 { + 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()); + } + } + // 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()); - } 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 operands.is_empty() { + writeln!(context.stderr(), "{}: invalid usage", context.command_name)?; + return Ok(ExecutionExitCode::InvalidUsage.into()); + } - if pid_or_job_spec.starts_with('%') { + let mut had_failure = false; + for pid_or_job_spec in operands { + 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(), @@ -111,16 +135,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 9417c4594..e548d08a4 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -32,6 +32,9 @@ ### Fixed - Fixed `/live` sideband WebSockets ignoring standard proxy environment variables and `NO_PROXY`, which left proxied sessions stuck while the rest of the Codex connection succeeded ([#6770](https://github.com/can1357/oh-my-pi/issues/6770)). +### Fixed + +- 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