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