Repository: reubeno/brush
Pull request #221 (2024-10-21 13:38:02)

feat: implement `kill -l`
Utilize `nix::signal::Signal` to print the all signals.

Note that `0 - EXIT` is missing for now because it is not posix, and I didn't find any documentation for the `EXIT` signal, except for the `trap` builtin https://www.gnu.org/software/bash/manual/html_node/Bourne-Shell-Builtins.html

> If a sigspec is 0 or EXIT, arg is executed when the shell exits. 

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `expand_one_string                   ` | `    3.42 μs` | `    3.42 μs` | `    0.00 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   60.15 μs` | `   59.88 μs` | `   -0.27 μs` | `⚪  Unchanged` |
| `instantiate_shell_with_init_scripts ` | `30965.04 μs` | `31345.54 μs` | `  380.50 μs` | `⚪  Unchanged` |
| `parse_bash_completion               ` | ` 2759.51 μs` | ` 2806.69 μs` | `   47.18 μs` | `⚪  Unchanged` |
| `parse_sample_script                 ` | `    4.21 μs` | `    4.30 μs` | `    0.09 μs` | `🟠 +2.16%    ` |
| `run_echo_builtin_command            ` | `   90.21 μs` | `   90.92 μs` | `    0.71 μs` | `⚪  Unchanged` |
| `run_one_builtin_command             ` | `  108.81 μs` | `  108.45 μs` | `   -0.36 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 2015.47 μs` | ` 1935.26 μs` | `  -80.21 μs` | `🟢 -3.98%    ` |
| `run_one_external_command_directly   ` | ` 1013.51 μs` | ` 1018.67 μs` | `    5.16 μs` | `⚪  Unchanged` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/builtins/kill.rs | 🔴 0% | 🟠 71.91% | 🟢 71.91% |
| brush-core/src/jobs.rs | 🔴 37.23% | 🔴 42.42% | 🟢 5.19% |
| **Overall Coverage** | **🟢 73.33%** | **🟢 73.61%** | **🟢 0.28%** |

_Minimum allowed coverage is_ `70%`_, this run produced_ `73.61%`
<!-- Sticky Pull Request Comment -->
39555: Cool thanks! I will look at that
reubeno: > 1. I've noticed that in bash `kill -l`can print selected signals e.g., `kill -l HUP 2 TERM` would print
> _[snipped]_
> 
> while `trap -l` always prints all the signals. Should the `trap -l` also be able to print selected signals?

Yes, I think it would be best for `trap -l` to behave like `bash`'s implementation does. In theory, I suppose that some script *could* use it to decode signal numbers / names. Seems unlikely, but couldn't hurt to be consistent there.

> 2. Maybe we should also print `EXIT`, `DEBUG` and `ERR`? Bash is highly inconsistent in this regard. For example, when you print all with `kill -l`, Bash doesn't show these signals, but if you type `kill -l 0`, it prints `EXIT`. Furthermore, when you type `kill -l 33` it shows nothing but when you type `kill -l ERR` it shows `33`

I see the inconsistencies as well; I'm assuming some of it maybe intentional, and other aspects may come down to the way the shared code happened to be structured.

For `kill`, `EXIT`, `DEBUG`, and `ERR` don't really make sense. They aren't really signals that can be sent to a job or process. They're virtual signals that can be trapped via `trap`. I'd vote for us avoiding duplicating much logic between `trap` and `kill`, and where possible without too much effort, hiding these 3 virtual signals from `kill`.

> 3. `trap` is case insensitive  (e.g `trap int` works), while `kill -l` is not. Should `kill` also be case insensitive?

I see `trap` call out its case insensitivity; did you see `kill -l` being case *sensitive*? From my quick tests on a few versions of `bash`, I saw it behave in a similar case _insensitive_ fashion:

```bash
bash$ kill -l int
2
bash$ kill -l iNt
2
bash$ kill -l INT
2
bash$ kill -l SiGiNT
2
bash$ kill -l SIGINT
2
bash$ kill -l sigint
2
```

> 4. when `kill -l` bash prints full names (e.g `SIGINT`) but when `kill -l 2`, it prints `INT`. Should it be always `SIGINT` or should I mimic the bash behavior?

I think it makes sense to mimic the bash behavior for `kill -l <signal>`, just on the extreme off-chance that some caller would notice the difference. For `kill -l` (listing all of them), I don't think I have a strong preference one way or the other.

39555: Interesting, the value of ERR and DEBUG is based on `NSIG` macro. On macos Err = 33, DEBUG = 32, on linux Err = 65, Debug = 64. 

Also I found a line in the bash change log for the version 2.01.1

> s.  A fix was made so that you can no longer trap `SIGEXIT' or `SIGDEBUG' --
    only `EXIT' and `DEBUG' are accepted.
39555: @reubeno Should I try to use `libc::SIGRTMAX` and `libc::SIGN` where possible and fallback to the `32` or leave DEBUG, ERR without numbers (than it would be `kill -l ERR` -> `ERR: invalid signal specification`)?
reubeno: What would your recommendation be? I think the requirements here are that the numbers for these virtual signals don't conflict with any of the *real* signals. They're not expected to be portable across systems, to my knowledge.
39555: > What would your recommendation be? I think the requirements here are that the numbers for these virtual signals don't conflict with any of the _real_ signals. They're not expected to be portable across systems, to my knowledge.

Fоr now, I made it without numbers. It is pretty accurate, `kill` just prints a name instead:
```
kill -l INT
2
kill -l DEBUG
DEBUG
```
in `kill -l` and `trap -l` ERR and DEBUG are not listed, similar to Bash.
reubeno: Sounds good to me. I'll try to review this in the next day or so. (I'd gotten a bit busy looking at an incoming issue report that seemed more severe (syntax parse failures).)
39555: There should be additional signals for linux that are not listed in nix::Signal...
39555: There is ongoing pr for realtime signals in nix https://github.com/nix-rust/nix/pull/2451
reubeno: > There is ongoing pr for realtime signals in nix [nix-rust/nix#2451](https://github.com/nix-rust/nix/pull/2451)

Is that something we could integrate with later (without preventing us from moving forward with this set of changes now)?
39555: Yeah. For now just realtime signals are missing (the linux failed test shows them)
reubeno: > Yeah. For now just realtime signals are missing (the linux failed test shows them)

Sounds like something we could track with a TODO follow-up and exclude from the diff (via grep) for now?

Patch:
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
index bfa8204..94b9dd5 100644
--- a/.github/workflows/ci.yaml
+++ b/.github/workflows/ci.yaml
@@ -93,7 +93,7 @@ jobs:
 
       - name: "Build (native)"
         if: ${{ matrix.target == '' }}
-        run: cargo build --release
+        run: cargo build --release --all-targets
 
       - name: "Build (cross)"
         if: ${{ matrix.target != '' }}
@@ -105,6 +105,15 @@ jobs:
           name: binaries-${{ matrix.arch }}-${{ matrix.os }}
           path: target/${{ matrix.target }}/release/${{ matrix.binary_name }}
 
+      - name: "Upload integration test binaries"
+        if: ${{ matrix.target == '' }}
+        uses: actions/upload-artifact@v4
+        with:
+          name: integration-tests-${{ matrix.arch }}-${{ matrix.os }}
+          path: |
+            target/${{ matrix.target }}/release/deps/brush_*_tests-*
+            !**/*.d
+
   # Test functional correctness
   test:
     strategy:
@@ -317,3 +326,76 @@ jobs:
             pr/benchmarks.txt
             main/benchmarks.txt
             benchmark-results.md
+
+  # Test release binary on a variety of OS platforms.
+  os-tests:
+    strategy:
+      fail-fast: false
+      matrix:
+        include:
+          # N.B. We don't include Ubuntu because it's already covered by the initial test job.
+          - container: "fedora:latest"
+            description: "Fedora/latest"
+            prereqs_command: "dnf install -y bash-completion iputils grep less sed util-linux"
+          - container: "debian:latest"
+            description: "Debian/latest"
+            prereqs_command: "apt-get update -y && apt-get install -y bash-completion bsdmainutils iputils-ping grep less sed"
+          - container: "archlinux:latest"
+            description: "Arch Linux/latest"
+            prereqs_command: "pacman -Sy --noconfirm bash-completion iputils grep less sed util-linux"
+
+    name: "OS target tests (${{ matrix.description }})"
+    runs-on: ubuntu-latest
+    container: ${{ matrix.container }}
+    needs: build
+    steps:
+      # Checkout sources for YAML-based test cases
+      - name: Checkout
+        uses: actions/checkout@v4
+        with:
+          path: sources
+
+      - name: Download binaries
+        uses: actions/download-artifact@v4
+        with:
+          name: binaries-x86_64-linux
+          path: binaries
+
+      - name: Download integration test binaries
+        uses: actions/download-artifact@v4
+        with:
+          name: integration-tests-x86_64-linux
+          path: binaries
+
+      - name: Setup downloads
+        run: |
+          # N.B. Can't use -o pipefail because it's not supported on Debian.
+          set -eux
+          chmod +x binaries/*
+          ls -l -R sources/brush-shell/tests
+          ls -l binaries
+
+      - name: Install prerequisites
+        if: ${{ matrix.prereqs_command != '' }}
+        run: ${{ matrix.prereqs_command }}
+
+      - name: Run tests
+        run: |
+          export BRUSH_PATH=$PWD/binaries/brush
+          export BRUSH_COMPAT_TEST_CASES=$PWD/sources/brush-shell/tests/cases
+          export BRUSH_VERBOSE=true
+
+          result=0
+          for test_name in binaries/*tests*; do
+            # TODO: Re-enable interactive tests.
+            if [[ ${test_name} == *interactive* ]]; then
+              echo "WARNING: skipping interactive test: ${test_name}"
+              continue
+            fi
+
+            echo "Running test: ${test_name}"
+            chmod +x ${test_name}
+            ${test_name} || result=$?
+          done
+
+          exit ${result}
diff --git a/brush-core/src/builtins/kill.rs b/brush-core/src/builtins/kill.rs
index f94b978..3a13410 100644
--- a/brush-core/src/builtins/kill.rs
+++ b/brush-core/src/builtins/kill.rs
@@ -1,6 +1,7 @@
 use clap::Parser;
 use std::io::Write;
 
+use crate::traps::TrapSignal;
 use crate::{builtins, commands, error};
 
 /// Signal a job or process.
@@ -37,15 +38,13 @@ impl builtins::Command for KillCommand {
         }
 
         if self.list_signals {
-            error::unimp("kill -l")
+            return print_signals(&context, self.args.as_ref());
         } else {
             if self.args.len() != 1 {
                 writeln!(context.stderr(), "{}: invalid usage", context.command_name)?;
                 return Ok(builtins::ExitCode::InvalidUsage);
             }
 
-            let exit_code = builtins::ExitCode::Success;
-
             let pid_or_job_spec = &self.args[0];
             if pid_or_job_spec.starts_with('%') {
                 // It's a job spec.
@@ -67,8 +66,56 @@ impl builtins::Command for KillCommand {
                 nix::sys::signal::kill(nix::unistd::Pid::from_raw(pid), nix::sys::signal::SIGKILL)
                     .map_err(|_errno| error::Error::FailedToSendSignal)?;
             }
+        }
+        Ok(builtins::ExitCode::Success)
+    }
+}
 
-            Ok(exit_code)
+fn print_signals(
+    context: &commands::ExecutionContext<'_>,
+    signals: &[String],
+) -> Result<builtins::ExitCode, error::Error> {
+    let mut exit_code = builtins::ExitCode::Success;
+    if !signals.is_empty() {
+        for s in signals {
+            // If the user gives us a code, we print the name; if they give a name, we print its
+            // code.
+            enum PrintSignal {
+                Name(&'static str),
+                Num(i32),
+            }
+
+            let signal = if let Ok(n) = s.parse::<i32>() {
+                // bash compatibility. `SIGHUP` -> `HUP`
+                TrapSignal::try_from(n).map(|s| {
+                    PrintSignal::Name(s.as_str().strip_prefix("SIG").unwrap_or(s.as_str()))
+                })
+            } else {
+                TrapSignal::try_from(s.as_str()).map(|sig| {
+                    i32::try_from(sig).map_or(PrintSignal::Name(sig.as_str()), PrintSignal::Num)
+                })
+            };
+
+            match signal {
+                Ok(PrintSignal::Num(n)) => {
+                    writeln!(context.stdout(), "{n}")?;
+                }
+                Ok(PrintSignal::Name(s)) => {
+                    writeln!(context.stdout(), "{s}")?;
+                }
+                Err(e) => {
+                    writeln!(context.stderr(), "{e}")?;
+                    exit_code = builtins::ExitCode::Custom(1);
+                }
+            }
         }
+    } else {
+        return crate::traps::format_signals(
+            context.stdout(),
+            TrapSignal::iterator().filter(|s| !matches!(s, TrapSignal::Exit)),
+        )
+        .map(|()| builtins::ExitCode::Success);
     }
+
+    Ok(exit_code)
 }
diff --git a/brush-core/src/builtins/trap.rs b/brush-core/src/builtins/trap.rs
index 1a9cc71..33cff63 100644
--- a/brush-core/src/builtins/trap.rs
+++ b/brush-core/src/builtins/trap.rs
@@ -1,7 +1,8 @@
 use clap::Parser;
 use std::io::Write;
 
-use crate::{builtins, commands, error, sys, traps};
+use crate::traps::TrapSignal;
+use crate::{builtins, commands, error};
 
 /// Manage signal traps.
 #[derive(Parser)]
@@ -23,13 +24,12 @@ impl builtins::Command for TrapCommand {
         mut context: commands::ExecutionContext<'_>,
     ) -> Result<builtins::ExitCode, crate::error::Error> {
         if self.list_signals {
-            Self::display_signals(&context)?;
-            Ok(builtins::ExitCode::Success)
+            crate::traps::format_signals(context.stdout(), TrapSignal::iterator())
+                .map(|()| builtins::ExitCode::Success)
         } else if self.print_trap_commands || self.args.is_empty() {
             if !self.args.is_empty() {
                 for signal_type in &self.args {
-                    let signal_type = parse_signal(signal_type)?;
-                    Self::display_handlers_for(&context, signal_type)?;
+                    Self::display_handlers_for(&context, signal_type.parse()?)?;
                 }
             } else {
                 Self::display_all_handlers(&context)?;
@@ -37,15 +37,14 @@ impl builtins::Command for TrapCommand {
             Ok(builtins::ExitCode::Success)
         } else if self.args.len() == 1 {
             let signal = self.args[0].as_str();
-            let signal_type = parse_signal(signal)?;
-            Self::remove_all_handlers(&mut context, signal_type);
+            Self::remove_all_handlers(&mut context, signal.parse()?);
             Ok(builtins::ExitCode::Success)
         } else {
             let handler = &self.args[0];
 
             let mut signal_types = vec![];
             for signal in &self.args[1..] {
-                signal_types.push(parse_signal(signal)?);
+                signal_types.push(signal.parse()?);
             }
 
             Self::register_handler(&mut context, signal_types, handler.as_str());
@@ -56,16 +55,6 @@ impl builtins::Command for TrapCommand {
 
 #[allow(unused_variables)]
 impl TrapCommand {
-    #[allow(clippy::unnecessary_wraps)]
-    fn display_signals(context: &commands::ExecutionContext<'_>) -> Result<(), error::Error> {
-        #[cfg(unix)]
-        for signal in nix::sys::signal::Signal::iterator() {
-            writeln!(context.stdout(), "{}: {signal}", signal as i32)?;
-        }
-
-        Ok(())
-    }
-
     fn display_all_handlers(context: &commands::ExecutionContext<'_>) -> Result<(), error::Error> {
         for signal in context.shell.traps.handlers.keys() {
             Self::display_handlers_for(context, *signal)?;
@@ -75,7 +64,7 @@ impl TrapCommand {
 
     fn display_handlers_for(
         context: &commands::ExecutionContext<'_>,
-        signal_type: traps::TrapSignal,
+        signal_type: TrapSignal,
     ) -> Result<(), error::Error> {
         if let Some(handler) = context.shell.traps.handlers.get(&signal_type) {
             writeln!(context.stdout(), "trap -- '{handler}' {signal_type}")?;
@@ -85,14 +74,14 @@ impl TrapCommand {
 
     fn remove_all_handlers(
         context: &mut crate::commands::ExecutionContext<'_>,
-        signal: traps::TrapSignal,
+        signal: TrapSignal,
     ) {
         context.shell.traps.remove_handlers(signal);
     }
 
     fn register_handler(
         context: &mut crate::commands::ExecutionContext<'_>,
-        signals: Vec<traps::TrapSignal>,
+        signals: Vec<TrapSignal>,
         handler: &str,
     ) {
         for signal in signals {
@@ -103,26 +92,3 @@ impl TrapCommand {
         }
     }
 }
-
-fn parse_signal(signal: &str) -> Result<traps::TrapSignal, error::Error> {
-    if signal.chars().all(|c| c.is_ascii_digit()) {
-        let digits = signal
-            .parse::<i32>()
-            .map_err(|_| error::Error::InvalidSignal)?;
-
-        sys::signal::parse_numeric_signal(digits)
-    } else {
-        let mut signal_to_parse = signal.to_ascii_uppercase();
-
-        if !signal_to_parse.starts_with("SIG") {
-            signal_to_parse.insert_str(0, "SIG");
-        }
-
-        match signal_to_parse {
-            s if s == "SIGDEBUG" => Ok(traps::TrapSignal::Debug),
-            s if s == "SIGERR" => Ok(traps::TrapSignal::Err),
-            s if s == "SIGEXIT" => Ok(traps::TrapSignal::Exit),
-            s => sys::signal::parse_os_signal_name(s.as_str()),
-        }
-    }
-}
diff --git a/brush-core/src/completion.rs b/brush-core/src/completion.rs
index 450a661..5a73cee 100644
--- a/brush-core/src/completion.rs
+++ b/brush-core/src/completion.rs
@@ -487,10 +487,9 @@ impl Spec {
                     }
                 }
                 CompleteAction::Signal => {
-                    for signal in traps::TrapSignal::all_values() {
-                        let signal_str = signal.to_string();
-                        if signal_str.starts_with(token) {
-                            candidates.insert(signal_str);
+                    for signal in traps::TrapSignal::iterator() {
+                        if signal.as_str().starts_with(token) {
+                            candidates.insert(signal.as_str().to_string());
                         }
                     }
                 }
diff --git a/brush-core/src/error.rs b/brush-core/src/error.rs
index d2e890d..946e082 100644
--- a/brush-core/src/error.rs
+++ b/brush-core/src/error.rs
@@ -148,8 +148,8 @@ pub enum Error {
     ThreadingError(#[from] tokio::task::JoinError),
 
     /// An invalid signal was referenced.
-    #[error("invalid signal")]
-    InvalidSignal,
+    #[error("{0}: invalid signal specification")]
+    InvalidSignal(String),
 
     /// A system error occurred.
     #[cfg(unix)]
diff --git a/brush-core/src/shell.rs b/brush-core/src/shell.rs
index c102f20..a8f33c1 100644
--- a/brush-core/src/shell.rs
+++ b/brush-core/src/shell.rs
@@ -135,6 +135,8 @@ pub struct CreateOptions {
     pub no_profile: bool,
     /// Whether to skip sourcing the user's rc file.
     pub no_rc: bool,
+    /// Whether to skip inheriting environment variables from the calling process.
+    pub do_not_inherit_env: bool,
     /// Whether the shell is in POSIX compliance mode.
     pub posix: bool,
     /// Whether to print commands and arguments as they are read.
@@ -210,11 +212,13 @@ impl Shell {
     fn initialize_vars(options: &CreateOptions) -> Result<ShellEnvironment, error::Error> {
         let mut env = ShellEnvironment::new();
 
-        // Seed parameters from environment.
-        for (k, v) in std::env::vars() {
-            let mut var = ShellVariable::new(ShellValue::String(v));
-            var.export();
-            env.set_global(k, var)?;
+        // Seed parameters from environment (unless requested not to do so).
+        if !options.do_not_inherit_env {
+            for (k, v) in std::env::vars() {
+                let mut var = ShellVariable::new(ShellValue::String(v));
+                var.export();
+                env.set_global(k, var)?;
+            }
         }
 
         // Set some additional ones.
diff --git a/brush-core/src/sys/stubs/signal.rs b/brush-core/src/sys/stubs/signal.rs
index 22c8932..6b816cf 100644
--- a/brush-core/src/sys/stubs/signal.rs
+++ b/brush-core/src/sys/stubs/signal.rs
@@ -1,13 +1,5 @@
 use crate::{error, sys, traps};
 
-pub(crate) fn parse_numeric_signal(_signal: i32) -> Result<traps::TrapSignal, error::Error> {
-    Err(error::Error::InvalidSignal)
-}
-
-pub(crate) fn parse_os_signal_name(_signal: &str) -> Result<traps::TrapSignal, error::Error> {
-    Err(error::Error::InvalidSignal)
-}
-
 pub(crate) fn continue_process(_pid: sys::process::ProcessId) -> Result<(), error::Error> {
     error::unimp("continue process")
 }
diff --git a/brush-core/src/sys/unix/signal.rs b/brush-core/src/sys/unix/signal.rs
index 281f2f5..67cb1a6 100644
--- a/brush-core/src/sys/unix/signal.rs
+++ b/brush-core/src/sys/unix/signal.rs
@@ -1,18 +1,4 @@
-use std::str::FromStr;
-
-use crate::{error, sys, traps};
-
-pub(crate) fn parse_numeric_signal(signal: i32) -> Result<traps::TrapSignal, error::Error> {
-    Ok(traps::TrapSignal::Signal(
-        nix::sys::signal::Signal::try_from(signal).map_err(|_| error::Error::InvalidSignal)?,
-    ))
-}
-
-pub(crate) fn parse_os_signal_name(signal: &str) -> Result<traps::TrapSignal, error::Error> {
-    Ok(traps::TrapSignal::Signal(
-        nix::sys::signal::Signal::from_str(signal).map_err(|_| error::Error::InvalidSignal)?,
-    ))
-}
+use crate::{error, sys};
 
 pub(crate) fn continue_process(pid: sys::process::ProcessId) -> Result<(), error::Error> {
     #[allow(clippy::cast_possible_wrap)]
diff --git a/brush-core/src/traps.rs b/brush-core/src/traps.rs
index f4004ca..25729f2 100644
--- a/brush-core/src/traps.rs
+++ b/brush-core/src/traps.rs
@@ -1,5 +1,10 @@
+use std::str::FromStr;
 use std::{collections::HashMap, fmt::Display};
 
+use itertools::Itertools as _;
+
+use crate::error;
+
 /// Type of signal that can be trapped in the shell.
 #[derive(Clone, Copy, Eq, Hash, PartialEq)]
 pub enum TrapSignal {
@@ -16,28 +21,128 @@ pub enum TrapSignal {
 
 impl Display for TrapSignal {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str(self.as_str())
+    }
+}
+
+impl TrapSignal {
+    /// Returns all possible values of [`TrapSignal`].
+    pub fn iterator() -> impl Iterator<Item = TrapSignal> {
+        const SIGNALS: &[TrapSignal] = &[TrapSignal::Debug, TrapSignal::Err, TrapSignal::Exit];
+        let iter = SIGNALS.iter().copied();
+
+        #[cfg(unix)]
+        let iter = itertools::chain!(
+            iter,
+            nix::sys::signal::Signal::iterator().map(TrapSignal::Signal)
+        );
+
+        iter
+    }
+
+    /// Converts [`TrapSignal`] into its corresponding signal name as a [`&'static str`](str)
+    pub const fn as_str(self) -> &'static str {
         match self {
             #[cfg(unix)]
-            TrapSignal::Signal(s) => s.fmt(f),
-            TrapSignal::Debug => write!(f, "DEBUG"),
-            TrapSignal::Err => write!(f, "ERR"),
-            TrapSignal::Exit => write!(f, "EXIT"),
+            TrapSignal::Signal(s) => s.as_str(),
+            TrapSignal::Debug => "DEBUG",
+            TrapSignal::Err => "ERR",
+            TrapSignal::Exit => "EXIT",
         }
     }
 }
 
-impl TrapSignal {
-    /// Returns all possible values of `TrapSignal`.
-    #[allow(unused_mut)]
-    pub fn all_values() -> Vec<TrapSignal> {
-        let mut signals = vec![TrapSignal::Debug, TrapSignal::Err, TrapSignal::Exit];
+/// Formats [`Iterator<Item = TrapSignal>`](TrapSignal)  to the provided writer.
+///
+/// # Arguments
+///
+/// * `f` - Any type that implements [`std::io::Write`].
+/// * `it` - An iterator over the signals that will be formatted into the `f`.
+pub fn format_signals(
+    mut f: impl std::io::Write,
+    it: impl Iterator<Item = TrapSignal>,
+) -> Result<(), error::Error> {
+    let it = it
+        .filter_map(|s| i32::try_from(s).ok().map(|n| (s, n)))
+        .sorted_by(|a, b| Ord::cmp(&a.1, &b.1))
+        .format_with("\n", |s, f| f(&format_args!("{}) {}", s.1, s.0)));
+    write!(f, "{it}")?;
+    Ok(())
+}
 
-        #[cfg(unix)]
-        for signal in nix::sys::signal::Signal::iterator() {
-            signals.push(TrapSignal::Signal(signal));
+// implement s.parse::<TrapSignal>()
+impl FromStr for TrapSignal {
+    type Err = error::Error;
+    fn from_str(s: &str) -> Result<Self, <TrapSignal as FromStr>::Err> {
+        if let Ok(n) = s.parse::<i32>() {
+            TrapSignal::try_from(n)
+        } else {
+            TrapSignal::try_from(s)
         }
+    }
+}
+
+// from a signal number
+impl TryFrom<i32> for TrapSignal {
+    type Error = error::Error;
+    fn try_from(value: i32) -> Result<Self, Self::Error> {
+        // NOTE: DEBUG and ERR are real-time signals, defined based on NSIG or SIGRTMAX (is not
+        // available on bsd-like systems),
+        // and don't have persistent numbers across platforms, so we skip them here.
+        Ok(match value {
+            0 => TrapSignal::Exit,
+            #[cfg(unix)]
+            value => TrapSignal::Signal(
+                nix::sys::signal::Signal::try_from(value)
+                    .map_err(|_| error::Error::InvalidSignal(value.to_string()))?,
+            ),
+            #[cfg(not(unix))]
+            _ => return Err(error::Error::InvalidSignal(value.to_string())),
+        })
+    }
+}
+
+// from a signal name
+impl TryFrom<&str> for TrapSignal {
+    type Error = error::Error;
+    fn try_from(value: &str) -> Result<Self, Self::Error> {
+        #[allow(unused_mut)] // on not unix platforms
+        let mut s = value.to_ascii_uppercase();
 
-        signals
+        Ok(match s.as_str() {
+            "DEBUG" => TrapSignal::Debug,
+            "ERR" => TrapSignal::Err,
+            "EXIT" => TrapSignal::Exit,
+
+            #[cfg(unix)]
+            _ => {
+                // Bash compatibility:
+                // support for signal names without the `SIG` prefix, for example `HUP` -> `SIGHUP`
+                if !s.starts_with("SIG") {
+                    s.insert_str(0, "SIG");
+                }
+                nix::sys::signal::Signal::from_str(s.as_str())
+                    .map(TrapSignal::Signal)
+                    .map_err(|_| error::Error::InvalidSignal(value.into()))?
+            }
+            #[cfg(not(unix))]
+            _ => return Err(error::Error::InvalidSignal(value.into())),
+        })
+    }
+}
+
+#[derive(Debug, Clone, Copy)]
+pub struct DoesntHaveANumber;
+
+impl TryFrom<TrapSignal> for i32 {
+    type Error = DoesntHaveANumber;
+    fn try_from(value: TrapSignal) -> Result<Self, Self::Error> {
+        Ok(match value {
+            #[cfg(unix)]
+            TrapSignal::Signal(s) => s as i32,
+            TrapSignal::Exit => 0,
+            _ => return Err(DoesntHaveANumber),
+        })
     }
 }
 
diff --git a/brush-parser/src/parser.rs b/brush-parser/src/parser.rs
index 734d363..5979ce5 100644
--- a/brush-parser/src/parser.rs
+++ b/brush-parser/src/parser.rs
@@ -254,8 +254,22 @@ peg::parser! {
             bang:bang()? seq:pipe_sequence() { ast::Pipeline { bang: bang.is_some(), seq } }
         rule bang() -> bool = specific_word("!") { true }
 
-        rule pipe_sequence() -> Vec<ast::Command> =
-            c:command() ++ (specific_operator("|") linebreak()) { c }
+        pub(crate) rule pipe_sequence() -> Vec<ast::Command> =
+            c:(c:command() r:&pipe_extension_redirection()? {? // check for `|&` without consuming the stream.
+                let mut c = c;
+                if r.is_some() {
+                    add_pipe_extension_redirection(&mut c)?;
+                }
+                Ok(c)
+            }) ++ (pipe_operator() linebreak()) {
+            c
+        }
+        rule pipe_operator() =
+            specific_operator("|") /
+            pipe_extension_redirection()
+
+        rule pipe_extension_redirection() -> &'input Token  =
+            non_posix_extensions_enabled() p:specific_operator("|&") { p }
 
         // N.B. We needed to move the function definition branch up to avoid conflicts with array assignment syntax.
         rule command() -> ast::Command =
@@ -826,6 +840,37 @@ fn parse_assignment_word(word: &str) -> Result<ast::Assignment, &'static str> {
     parse_result.map_err(|_| "not assignment word")
 }
 
+// add `2>&1` to the command if the pipeline is `|&`
+fn add_pipe_extension_redirection(c: &mut ast::Command) -> Result<(), &'static str> {
+    let r = ast::IoRedirect::File(
+        Some(2),
+        ast::IoFileRedirectKind::DuplicateOutput,
+        ast::IoFileRedirectTarget::Fd(1),
+    );
+    match c {
+        ast::Command::Simple(c) => {
+            let r = ast::CommandPrefixOrSuffixItem::IoRedirect(r);
+            if let Some(suffix) = &mut c.suffix {
+                suffix.0.push(r);
+            } else {
+                let v = vec![r];
+                c.suffix = Some(ast::CommandSuffix(v));
+            }
+        }
+        ast::Command::Compound(_, l) => {
+            if let Some(r_list) = l {
+                r_list.0.push(r);
+            } else {
+                let v = vec![r];
+                *l = Some(ast::RedirectList(v));
+            }
+        }
+        ast::Command::ExtendedTest(_) => return Err("|& unimplemented for extended tests"),
+        ast::Command::Function(_) => return Err("|& unimplemented for functions"),
+    };
+    Ok(())
+}
+
 fn parse_array_assignment(
     word: &str,
     elements: &[&String],
@@ -858,9 +903,11 @@ fn parse_array_assignment(
 
 #[cfg(test)]
 mod tests {
+
     use super::*;
     use crate::tokenizer::tokenize_str;
     use anyhow::Result;
+    use assert_matches::assert_matches;
 
     #[test]
     fn parse_case() -> Result<()> {
@@ -911,4 +958,33 @@ esac\
 
         Ok(())
     }
+
+    #[test]
+    fn parse_redirection() -> Result<()> {
+        let input = r"echo |& wc";
+
+        let tokens = tokenize_str(input)?;
+        let seq = super::token_parser::pipe_sequence(
+            &Tokens {
+                tokens: tokens.as_slice(),
+            },
+            &ParserOptions::default(),
+            &SourceInfo::default(),
+        )?;
+
+        assert_eq!(seq.len(), 2);
+        assert_matches!(seq[0], ast::Command::Simple(..));
+        if let ast::Command::Simple(c) = &seq[0] {
+            let c = c.suffix.as_ref().unwrap();
+            assert_matches!(
+                c.0[0],
+                ast::CommandPrefixOrSuffixItem::IoRedirect(ast::IoRedirect::File(
+                    Some(2),
+                    ast::IoFileRedirectKind::DuplicateOutput,
+                    ast::IoFileRedirectTarget::Fd(1)
+                ))
+            )
+        }
+        Ok(())
+    }
 }
diff --git a/brush-parser/src/tokenizer.rs b/brush-parser/src/tokenizer.rs
index 5017047..79ee1d1 100644
--- a/brush-parser/src/tokenizer.rs
+++ b/brush-parser/src/tokenizer.rs
@@ -936,7 +936,7 @@ impl<'a, R: ?Sized + std::io::BufRead> Tokenizer<'a, R> {
 
     fn is_operator(&self, s: &str) -> bool {
         // Handle non-POSIX operators.
-        if !self.options.posix_mode && matches!(s, "<<<" | "&>" | "&>>" | ";;&" | ";&") {
+        if !self.options.posix_mode && matches!(s, "<<<" | "&>" | "&>>" | ";;&" | ";&" | "|&") {
             return true;
         }
 
diff --git a/brush-shell/Cargo.toml b/brush-shell/Cargo.toml
index f4a44f7..8dbd52b 100644
--- a/brush-shell/Cargo.toml
+++ b/brush-shell/Cargo.toml
@@ -42,7 +42,7 @@ async-trait = "0.1.83"
 brush-parser = { version = "^0.2.9", path = "../brush-parser" }
 brush-core = { version = "^0.2.11", path = "../brush-core" }
 cfg-if = "1.0.0"
-clap = { version = "4.5.17", features = ["derive", "wrap_help"] }
+clap = { version = "4.5.17", features = ["derive", "env", "wrap_help"] }
 const_format = "0.2.33"
 git-version = "0.3.9"
 lazy_static = "1.5.0"
diff --git a/brush-shell/src/args.rs b/brush-shell/src/args.rs
index 6e1ddbc..0284ec7 100644
--- a/brush-shell/src/args.rs
+++ b/brush-shell/src/args.rs
@@ -73,6 +73,10 @@ pub struct CommandLineArgs {
     #[clap(long = "norc")]
     pub no_rc: bool,
 
+    /// Don't inherit environment variables from the calling process.
+    #[clap(long = "noenv")]
+    pub do_not_inherit_env: bool,
+
     /// Enable shell option.
     #[clap(short = 'O', value_name = "OPTION")]
     pub enabled_shopt_options: Vec<String>,
diff --git a/brush-shell/src/main.rs b/brush-shell/src/main.rs
index 858fe8d..e3faee8 100644
--- a/brush-shell/src/main.rs
+++ b/brush-shell/src/main.rs
@@ -205,6 +205,7 @@ async fn instantiate_shell(
             no_editing: args.no_editing,
             no_profile: args.no_profile,
             no_rc: args.no_rc,
+            do_not_inherit_env: args.do_not_inherit_env,
             posix: args.posix || args.sh_mode,
             print_commands_and_arguments: args.print_commands_and_arguments,
             read_commands_from_stdin,


Test patch:
diff --git a/brush-shell/tests/cases/builtins/command.yaml b/brush-shell/tests/cases/builtins/command.yaml
index 8bfe949..573e7e0 100644
--- a/brush-shell/tests/cases/builtins/command.yaml
+++ b/brush-shell/tests/cases/builtins/command.yaml
@@ -20,13 +20,26 @@ cases:
 
   - name: "command -v"
     stdin: |
+      echo "PATH: $PATH"
+
+      echo "[echo]"
       command -v echo
-      command -v cat
-      command -v $(command -v cat)
 
+      echo "[non-existent]"
       command -v non-existent || echo "1. Not found"
+
+      echo "[/usr/bin/non-existent]"
       command -v /usr/bin/non-existent || echo "2. Not found"
 
+  - name: "command -v with full paths"
+    skip: true # TODO: investigate why this fails on arch linux
+    stdin: |
+      echo "[cat]"
+      command -v cat
+
+      echo "[\$(command -v cat)]"
+      command -v $(command -v cat)
+
   - name: "command -V"
     ignore_stderr: true
     stdin: |
diff --git a/brush-shell/tests/cases/builtins/kill.yaml b/brush-shell/tests/cases/builtins/kill.yaml
new file mode 100644
index 0000000..3645ab6
--- /dev/null
+++ b/brush-shell/tests/cases/builtins/kill.yaml
@@ -0,0 +1,17 @@
+name: "Builtins: kill"
+cases:
+  - name: "kill -l"
+    ignore_stderr: true
+    stdin: |
+      for i in $(seq 1 31); do kill -l $i; done
+      # limit the number of signals to 31. Realtime signals are not implemented yet.
+      for i in $(kill -l | sed -e "s/[[:digit:]]*)//g"); do echo $i; done | head -31
+      # invalid option
+      kill -l 9999
+      kill -l HUP
+      kill -l iNt
+      kill -l int
+      kill -l SIGHUP
+      kill -l EXIT
+
+
diff --git a/brush-shell/tests/cases/pipeline.yaml b/brush-shell/tests/cases/pipeline.yaml
index 4a60272..6b7a39d 100644
--- a/brush-shell/tests/cases/pipeline.yaml
+++ b/brush-shell/tests/cases/pipeline.yaml
@@ -36,3 +36,8 @@ cases:
       echo "var: ${var}"
       echo hi | { var=3; cat; }
       echo "var: ${var}"
+
+  - name: "pipeline extension"
+    stdin: |
+      echo -e "hello" |& wc -l
+      cat dfdfgdfgdf |& wc -l
diff --git a/brush-shell/tests/compat_tests.rs b/brush-shell/tests/compat_tests.rs
index d1d4c04..402fdaf 100644
--- a/brush-shell/tests/compat_tests.rs
+++ b/brush-shell/tests/compat_tests.rs
@@ -1,5 +1,9 @@
 //! The test harness for brush shell integration tests.
 
+// Only compile this for Unix-like platforms (Linux, macOS) because they have an oracle to compare
+// against.
+#![cfg(unix)]
+
 use anyhow::{Context, Result};
 use assert_fs::fixture::{FileWriteStr, PathChild};
 use clap::Parser;
@@ -35,6 +39,9 @@ impl TestConfig {
     pub fn for_bash_testing(options: &TestOptions) -> Result<Self> {
         // Check for bash version.
         let bash_version_str = get_bash_version_str(Path::new(&options.bash_path))?;
+        if options.verbose {
+            eprintln!("Detected bash version: {bash_version_str}");
+        }
 
         // Skip rc file and profile for deterministic behavior across systems/distros.
         Ok(Self {
@@ -45,7 +52,7 @@ impl TestConfig {
             },
             oracle_version_str: Some(bash_version_str),
             test_shell: ShellConfig {
-                which: WhichShell::ShellUnderTest(String::from("brush")),
+                which: WhichShell::ShellUnderTest(PathBuf::from(&options.brush_path)),
                 // Disable a few fancy UI options for shells under test.
                 default_args: vec![
                     "--norc".into(),
@@ -65,12 +72,12 @@ impl TestConfig {
         Ok(Self {
             name: String::from(SH_CONFIG_NAME),
             oracle_shell: ShellConfig {
-                which: WhichShell::NamedShell(String::from("sh")),
+                which: WhichShell::NamedShell(PathBuf::from("sh")),
                 default_args: vec![],
             },
             oracle_version_str: None,
             test_shell: ShellConfig {
-                which: WhichShell::ShellUnderTest(String::from("brush")),
+                which: WhichShell::ShellUnderTest(PathBuf::from(&options.brush_path)),
                 // Disable a few fancy UI options for shells under test.
                 default_args: vec![
                     String::from("--sh"),
@@ -85,9 +92,7 @@ impl TestConfig {
 }
 
 #[allow(clippy::too_many_lines)]
-async fn cli_integration_tests(options: TestOptions) -> Result<()> {
-    let dir = env!("CARGO_MANIFEST_DIR");
-
+async fn cli_integration_tests(mut options: TestOptions) -> Result<()> {
     let mut success_count = 0;
     let mut skip_count = 0;
     let mut known_failure_count = 0;
@@ -98,18 +103,46 @@ async fn cli_integration_tests(options: TestOptions) -> Result<()> {
         test: std::time::Duration::default(),
     };
 
-    let mut test_configs = vec![];
+    // Resolve paths.
+    let test_cases_dir = options.test_cases_path.as_deref().map_or_else(
+        || PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/cases"),
+        |p| p.to_owned(),
+    );
+
+    // Resolve path to the shell-under-test.
+    if options.brush_path.is_empty() {
+        options.brush_path = assert_cmd::cargo::cargo_bin("brush")
+            .to_string_lossy()
+            .to_string();
+    }
+    if !Path::new(&options.brush_path).exists() {
+        return Err(anyhow::anyhow!(
+            "brush binary not found: {}",
+            options.brush_path
+        ));
+    }
 
+    // Set up test configs
+    let mut test_configs = vec![];
     if options.should_enable_config(BASH_CONFIG_NAME) {
         test_configs.push(TestConfig::for_bash_testing(&options)?);
     }
-
     if options.should_enable_config(SH_CONFIG_NAME) {
         test_configs.push(TestConfig::for_sh_testing(&options)?);
     }
 
+    // Generate a glob pattern to find all the YAML test case files.
+    let glob_pattern = test_cases_dir
+        .join("**/*.yaml")
+        .to_string_lossy()
+        .to_string();
+
+    if options.verbose {
+        eprintln!("Running test cases: {glob_pattern}");
+    }
+
     // Spawn each test case set separately.
-    for entry in glob::glob(format!("{dir}/tests/cases/**/*.yaml").as_ref()).unwrap() {
+    for entry in glob::glob(glob_pattern.as_ref()).unwrap() {
         let entry = entry.unwrap();
 
         let yaml_file = std::fs::File::open(entry.as_path())?;
@@ -472,8 +505,8 @@ impl Default for ShellInvocation {
 
 #[derive(Clone)]
 enum WhichShell {
-    ShellUnderTest(String),
-    NamedShell(String),
+    ShellUnderTest(PathBuf),
+    NamedShell(PathBuf),
 }
 
 struct TestCaseResult {
@@ -943,10 +976,7 @@ impl TestCase {
                     let target_dir = std::env::var("CARGO_TARGET_DIR")
                         .ok()
                         .map_or_else(default_target_dir, PathBuf::from);
-                    (
-                        std::process::Command::new(assert_cmd::cargo::cargo_bin(name)),
-                        Some(target_dir),
-                    )
+                    (std::process::Command::new(name), Some(target_dir))
                 }
                 WhichShell::NamedShell(name) => (std::process::Command::new(name), None),
             },
@@ -964,6 +994,11 @@ impl TestCase {
         test_cmd.env("PS1", "test$ ");
         // Try to get decent backtraces when problems get hit.
         test_cmd.env("RUST_BACKTRACE", "1");
+        // Hard-code a well-known PATH that isn't dependent on the system's configuration.
+        test_cmd.env(
+            "PATH",
+            "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
+        );
 
         // Set up any env vars needed for collecting coverage data.
         if let Some(coverage_target_dir) = &coverage_target_dir {
@@ -1278,7 +1313,7 @@ struct TestOptions {
     pub display_known_failure_details: bool,
 
     /// Display details regarding successful test cases
-    #[clap(short = 'v', long = "verbose")]
+    #[clap(short = 'v', long = "verbose", env = "BRUSH_VERBOSE")]
     pub verbose: bool,
 
     /// Enable a specific configuration
@@ -1294,8 +1329,16 @@ struct TestOptions {
     pub exact_match: bool,
 
     /// Optionaly specify a non-default path for bash
-    #[clap(long = "bash-path", default_value = "bash")]
-    pub bash_path: String,
+    #[clap(long = "bash-path", default_value = "bash", env = "BASH_PATH")]
+    pub bash_path: PathBuf,
+
+    /// Optionally specify a non-default path for brush
+    #[clap(long = "brush-path", default_value = "", env = "BRUSH_PATH")]
+    pub brush_path: String,
+
+    /// Optionally specify path to test cases
+    #[clap(long = "test-cases-path", env = "BRUSH_COMPAT_TEST_CASES")]
+    pub test_cases_path: Option<PathBuf>,
 
     //
     // Compat-only options
@@ -1421,14 +1464,7 @@ fn get_bash_version_str(bash_path: &Path) -> Result<String> {
 
 fn main() -> Result<()> {
     let unparsed_args: Vec<_> = std::env::args().collect();
-    let mut options = TestOptions::parse_from(unparsed_args);
-
-    // Allow overriding the bash path for invocations where custom arguments
-    // can't be used (because other tests get run too and they don't support
-    // those arguments).
-    if let Ok(value) = std::env::var("BASH_PATH") {
-        options.bash_path = value;
-    }
+    let options = TestOptions::parse_from(unparsed_args);
 
     tokio::runtime::Builder::new_multi_thread()
         .enable_all()
diff --git a/brush-shell/tests/completion_tests.rs b/brush-shell/tests/completion_tests.rs
index bc779cd..7cf98e0 100644
--- a/brush-shell/tests/completion_tests.rs
+++ b/brush-shell/tests/completion_tests.rs
@@ -1,3 +1,5 @@
+//! Completion integration tests for brush shell.
+
 // For now, only compile this for Linux.
 #![cfg(target_os = "linux")]
 #![allow(clippy::panic_in_result_fn)]
diff --git a/brush-shell/tests/interactive_tests.rs b/brush-shell/tests/interactive_tests.rs
index 95e8e30..6a4973f 100644
--- a/brush-shell/tests/interactive_tests.rs
+++ b/brush-shell/tests/interactive_tests.rs
@@ -1,3 +1,7 @@
+//! Interactive integration tests for brush shell
+
+// For now, only compile this for Unix-like platforms (Linux, macOS).
+#![cfg(unix)]
 #![allow(clippy::panic_in_result_fn)]
 
 use anyhow::Context;