Implement kill -0 by pepa65 · Pull Request #1367 · reubeno/brush

pepa65 · 2026-09-10T16:30:35Z

Implement the kill -0 PID (and kill -n 0 PID) command, to mirror bash's behavior.
This would close Issue #1206.
Assisted-by: ChatGPT

Copilot

🟡 Changes recommended

Job targets may be killed instead of checked, later signal specifications are ignored, and compatibility tests are missing.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds kill -0 PID and kill -n 0 PID process-existence checks.

Changes:

Adds platform-specific process-check helpers.
Routes zero-signal PID requests through the helper.
Supports zero in numeric signal parsing.

File summaries

File	Description
`brush-core/src/sys/unix/signal.rs`	Implements signal-zero checks on Unix.
`brush-core/src/sys/stubs/signal.rs`	Adds unsupported-platform fallback.
`brush-builtins/src/kill.rs`	Parses and executes zero-signal requests.

Review details

Suppressed comments (2)

brush-builtins/src/kill.rs:86

A later nonzero sigspec updates trap_signal but leaves signal_zero set, so kill -0 -TERM PID still performs only the existence check instead of sending SIGTERM. Clear zero mode when a subsequent valid signal is parsed, or reject multiple signal specifications.

                } else if let Ok(parsed_trap_signal) = possible_sigspec.parse::<TrapSignal>() {
                    trap_signal = parsed_trap_signal;

brush-builtins/src/kill.rs:134

signal_zero is only consulted for PID targets. For a job target such as kill -0 %1, the resolved job still executes job.kill(trap_signal) with the default SIGKILL, so this supposedly non-signaling check can terminate the job. Apply the zero-signal check to the job's process-group ID as well.

                if signal_zero {
                    sys::signal::check_process(pid)?;

Files reviewed: 3/3 changed files
Comments generated: 2
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+                #[expect(clippy::cast_possible_truncation)]
+                #[expect(clippy::cast_possible_wrap)]
+                if let Ok(parsed_trap_signal) = TrapSignal::try_from(*signal_number as i32) {
+                   trap_signal = parsed_trap_signal;


github-actions · 2026-09-10T19:49:49Z

Public API changes for crate: brush-core

Added items

+pub brush_core::error::ErrorKind::NoSuchProcess
+pub brush_core::error::ErrorKind::PermissionDenied
+pub fn brush_core::jobs::Job::check_signalable(&self) -> core::result::Result<(), brush_core::error::Error>
+pub fn brush_core::sys::signal::check_signalable(i32) -> core::result::Result<(), brush_core::error::Error>
+pub brush_core::ErrorKind::NoSuchProcess
+pub brush_core::ErrorKind::PermissionDenied

Performance Benchmark Report

Benchmark name	Baseline (μs)	Test/PR (μs)	Delta (μs)	Delta %
`clone_shell_object`	`17.31 μs`	`17.59 μs`	`0.28 μs`	`🟠 +1.59%`
`eval_arithmetic`	`0.15 μs`	`0.15 μs`	`0.00 μs`	`⚪ Unchanged`
`expand_one_string`	`1.87 μs`	`1.84 μs`	`-0.04 μs`	`⚪ Unchanged`
`for_loop`	`31.40 μs`	`32.89 μs`	`1.49 μs`	`🟠 +4.75%`
`full_peg_complex`	`59.30 μs`	`59.21 μs`	`-0.09 μs`	`⚪ Unchanged`
`full_peg_for_loop`	`6.32 μs`	`6.30 μs`	`-0.01 μs`	`⚪ Unchanged`
`full_peg_nested_expansions`	`16.56 μs`	`16.31 μs`	`-0.26 μs`	`⚪ Unchanged`
`full_peg_pipeline`	`4.38 μs`	`4.34 μs`	`-0.04 μs`	`🟢 -1.03%`
`full_peg_simple`	`1.83 μs`	`1.84 μs`	`0.00 μs`	`⚪ Unchanged`
`function_call`	`3.72 μs`	`3.78 μs`	`0.06 μs`	`⚪ Unchanged`
`instantiate_shell`	`54.66 μs`	`55.31 μs`	`0.64 μs`	`🟠 +1.18%`
`instantiate_shell_with_init_scripts`	`27859.11 μs`	`27731.30 μs`	`-127.81 μs`	`⚪ Unchanged`
`parse_peg_bash_completion`	`2123.02 μs`	`2113.54 μs`	`-9.48 μs`	`⚪ Unchanged`
`parse_peg_complex`	`19.81 μs`	`19.68 μs`	`-0.13 μs`	`⚪ Unchanged`
`parse_peg_for_loop`	`2.06 μs`	`2.05 μs`	`-0.02 μs`	`⚪ Unchanged`
`parse_peg_pipeline`	`2.21 μs`	`2.18 μs`	`-0.02 μs`	`🟢 -1.13%`
`parse_peg_simple`	`1.22 μs`	`1.17 μs`	`-0.04 μs`	`⚪ Unchanged`
`run_echo_builtin_command`	`17.18 μs`	`16.69 μs`	`-0.49 μs`	`⚪ Unchanged`
`tokenize_sample_script`	`3.39 μs`	`3.37 μs`	`-0.02 μs`	`⚪ Unchanged`

Code Coverage Report: Only Changed Files listed

Package	Base Coverage	New Coverage	Difference
brush-builtins/src/kill.rs	🟠 59.52%	🟠 65.26%	🟢 5.74%
brush-core/src/jobs.rs	🟠 51.13%	🟠 50%	🔴 -1.13%
brush-core/src/sys/unix/signal.rs	🟠 55.13%	🟠 56.98%	🟢 1.85%
brush-test-harness/src/comparison.rs	🟢 80.13%	🟢 83.97%	🟢 3.84%
brush-test-harness/src/reporting.rs	🔴 11.62%	🔴 29.93%	🟢 18.31%
brush-test-harness/src/runner.rs	🟢 83.98%	🟢 84.58%	🟢 0.6%
brush-test-harness/src/util.rs	🔴 47.76%	🟠 71.64%	🟢 23.88%
Overall Coverage	🟢 76.31%	🟢 76.61%	🟢 0.3%

Minimum allowed coverage is 70%, this run produced 76.61%
Maximum allowed coverage difference is -5%, this run produced 0.3%

Test Summary: bash-completion test suite

Outcome	Count	Percentage
✅ Pass	1589	75.34
❗️ Error	16	0.76
❌ Fail	150	7.11
⏩ Skip	339	16.07
❎ Expected Fail	13	0.62
✔️ Unexpected Pass	2	0.09
📊 Total	2109	100.00

reubeno

Thanks for the contribution, @pepa65! This looks like a great start.

I've left a couple of comments in addition to the ones that copilot left (which I'd agree with). In particular, it would be ideal to add some YAML-based test cases to our compatibility test suite; that's the best way to demonstrate that the functionality as implemented now matches bash.

There should be plenty of examples of tests like that, and tests for kill in particular. Holler if you could use some pointers.

reubeno · 2026-09-10T20:39:06Z

                // See if this is -sigspec syntax. The sigspec may be a signal name
                // (e.g., -TERM) or a signal number (e.g., -9).
-                if let Ok(parsed_trap_signal) = possible_sigspec.parse::<TrapSignal>() {
+                if possible_sigspec == "0" {


issue(blocking): We probably shouldn't just be doing a direct string comparison against "0". From some quick testing, it looks like any string that would parse to the integer 0 should fall into this case, e.g.:

reubeno · 2026-09-10T20:44:24Z

+    pid: sys::process::ProcessId,
+) -> Result<(), error::Error> {
+    nix::sys::signal::kill(nix::unistd::Pid::from_raw(pid), None)
+        .map_err(|_errno| error::ErrorKind::FailedToSendSignal)?;


question(blocking): Since kill -0 is typically used to check for the existence of a process, can we differentiate the return values and faithfully bubble them up? In other words, if the underlying call fails because the process couldn't be found -- ideally we'd be able to report via the kill builtin that the process doesn't exist. This is in contrast with, for example, a permissions issue.

To do the same as bash and as kill in procps, where only 0 (signalable) and 1 (insufficient permissions or non-existing process) are returned, brush should do the same in order to not break compatibility for existing scripts.

pepa65 · 2026-09-11T02:23:09Z

What do I do with such a test result:

* Test case: [kill -0 (numeric, non-existent)]... 
    Oracle comparison:
      status matches (exit status: 1) ✔️
      stdout matches ✔️
      stderr DIFFERS:
          ------ Oracle <> Test: stderr ---------------------------------
          - /usr/bin/bash: line 1: kill: (99999999) - No such process
          + error: kill: failed to send signal to process
            
          ---------------------------------------------------------------
      temp dir matches ✔️
    FAILED.

Firstly, the output differs, but that seems to be common for brush vs. bash..?

But I guess here is differentiation, even if the return code is always 1 for non-signalable processes: the error text is different for the non-existent and lacking-permissions cases. This is what should be implemented, with the error texts corresponding to the bash output in both cases.

reubeno

Thanks for iterating on the changes and for adding the tests; it's looking in pretty good shape.

I've left a few additional comments/requests -- much smaller this time.

reubeno · 2026-09-12T06:16:51Z

-                        possible_sigspec
-                    )?;
-                    return Ok(ExecutionResult::general_error());
+                if let Ok(signal_number) = possible_sigspec.parse::<i32>() {


suggestion: This can be slightly simplified. Instead of the if let Ok(...) ... followed by the comparison against 0, we can instead do something like:

if let Ok(0) = possible_sigspec.parse::<i32>() { signal_zero = true; } else if let Ok(parsed_trap_signal) = possible_sigspec.parse::<TrapSignal>() { ... } else { ... }

pepa65 · 2026-09-12T09:40:58Z

Thank you for your encouraging and helpful comments. I think brush is great, I just bumped into it last week, and it's basically ready to replace bash in most cases already..! (I am building it as a musl static binary, and that works great, and isn't too big.)

….toml

pepa65 · 2026-09-13T06:25:21Z

Sorry for wasting everybody's time with the untested push...

pepa65 · 2026-09-13T12:23:18Z

Not sure what to do about this (below). The oracle included the pid that doesn't exist in the error, should I mirror this? If you do a normal kill 99999999 the error is "failed to send signal to process", but if you do kill -0 99999999 the error is "no such process".

* Test case: [kill -0 (numeric, non-existent)]...                                                                              
    Oracle comparison:                                                                                                         
      status matches (exit status: 1) ✔️                                                                                        
      stdout matches ✔️                                                                                                         
      stderr DIFFERS:                                                                                                          
          ------ Oracle <> Test: stderr ---------------------------------                                                      
          - /usr/bin/bash: line 1: kill: (99999999) - No such process                                                          
          + error: kill: no such process                                                                                       
                                                                                                                               
          ---------------------------------------------------------------                                                      
      temp dir matches ✔️                                                                                                       
    FAILED.

reubeno · 2026-09-13T17:08:42Z

Not sure what to do about this (below). The oracle included the pid that doesn't exist in the error, should I mirror this? If you do a normal kill 99999999 the error is "failed to send signal to process", but if you do kill -0 99999999 the error is "no such process".

@pepa65 -- good question. We probably could be doing something better about this in the project, but for now, we enable the ignore_stderr option on these kinds of negative tests where the error messages diverge. If you search nearby you should see some examples of that.

With that open set (just on the test that fails in that way), then exit code, stdout, file output, etc. are all verified -- just stderr is ignored.

pepa65 · 2026-09-13T23:41:03Z

Okay, fixed that. On my test locally I also got:

* Test case: [Pathname expansion: extglob disabled]... 
    Oracle comparison:                                         
      status mismatch: exit status: 2 from oracle vs. exit status: 126 from test                                               
      stdout matches ✔️                                                                                                        
      stderr ignored                                                                                                           
      temp dir matches ✔️                                      
    FAILED.

(But I didn't 'fix' that.)

Implement kill -0

64d2f83

pepa65 marked this pull request as draft September 10, 2026 16:36

pepa65 marked this pull request as ready for review September 10, 2026 16:36

reubeno requested a balanced review from Copilot September 10, 2026 19:31

Copilot started reviewing on behalf of reubeno September 10, 2026 19:31 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Comment thread brush-builtins/src/kill.rs Outdated

#[expect(clippy::cast_possible_truncation)]

#[expect(clippy::cast_possible_wrap)]

if let Ok(parsed_trap_signal) = TrapSignal::try_from(*signal_number as i32) {

trap_signal = parsed_trap_signal;

Comment thread brush-builtins/src/kill.rs

reubeno requested changes Sep 10, 2026

View reviewed changes

pepa65 added 2 commits September 11, 2026 10:16

Fix the kill -0 implementation

d2a4524

Merge branch 'reubeno:main' into main

bd296b0

reubeno requested changes Sep 12, 2026

View reviewed changes

pepa65 added 2 commits September 12, 2026 16:34

reubeno/pull/1367/review/5185541154

f9f5c7e

Merge branch 'reubeno:main' into main

f4eaa54

Fix missing = and testing all, and cargo fmt -- --config-path rustfmt…

8ec33cb

….toml

Disable stderr on kill -0 99999999

98ea5c5

Conversation

pepa65 commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Uh oh!

github-actions Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Public API changes for crate: brush-core

Added items

Performance Benchmark Report

Code Coverage Report: Only Changed Files listed

Test Summary: bash-completion test suite

Uh oh!

reubeno left a comment

Choose a reason for hiding this comment

Uh oh!

reubeno Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

pepa65 Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

reubeno Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

pepa65 Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

pepa65 commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reubeno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reubeno Sep 12, 2026

Choose a reason for hiding this comment

Uh oh!

pepa65 commented Sep 12, 2026

Uh oh!

pepa65 commented Sep 13, 2026

Uh oh!

pepa65 commented Sep 13, 2026

Uh oh!

reubeno commented Sep 13, 2026

Uh oh!

pepa65 commented Sep 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pepa65 commented Sep 10, 2026 •

edited

Loading

github-actions Bot commented Sep 10, 2026 •

edited

Loading

pepa65 commented Sep 11, 2026 •

edited

Loading