From 38e143d7cf2e48a120b6939032abf30d8347a695 Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 27 Jul 2026 09:14:13 +0000 Subject: [PATCH] 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