From b1fea8fc923e997b507189fac3bf3074fa5db7ab Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Wed, 7 Oct 2026 12:41:08 +0000 Subject: [PATCH 1/2] fix(builtins): usage errors fail the command, not the script grep -E '(' or an awk syntax error returned Err from the builtin, which aborted the whole exec. Bundled builtins' Execution/Regex errors now become exit 2 with the message on stderr, as in bash. Err from custom builtins (builder, registry, resolver) still aborts. Claude-Session: https://claude.ai/code/session_019aFikmptPc91Fj4N2iDXQA --- crates/bashkit/docs/custom_builtins.md | 4 ++ crates/bashkit/src/builtins/grep.rs | 9 +++- crates/bashkit/src/interpreter/mod.rs | 40 +++++++++++++++ .../tests/integration/threat_model_tests.rs | 23 +++++++-- .../bash/builtin-usage-errors.test.sh | 49 +++++++++++++++++++ knowledge/foundations/builtins.md | 15 ++++++ knowledge/log.md | 1 + 7 files changed, 136 insertions(+), 5 deletions(-) create mode 100644 crates/bashkit/tests/spec_cases/bash/builtin-usage-errors.test.sh diff --git a/crates/bashkit/docs/custom_builtins.md b/crates/bashkit/docs/custom_builtins.md index 5b27b8815..60de52992 100644 --- a/crates/bashkit/docs/custom_builtins.md +++ b/crates/bashkit/docs/custom_builtins.md @@ -352,6 +352,10 @@ ExecResult::ok("output\n".to_string()); ExecResult::err("error message\n".to_string(), 1); ``` +Return `Ok(ExecResult::err(..))` for a failure the script should see as a +non-zero exit. Returning `Err(..)` from a custom builtin aborts the whole +`exec()` call; use it only for failures that must stop the script. + ## Examples ### Database Query Builtin diff --git a/crates/bashkit/src/builtins/grep.rs b/crates/bashkit/src/builtins/grep.rs index ed92d9971..86643fe24 100644 --- a/crates/bashkit/src/builtins/grep.rs +++ b/crates/bashkit/src/builtins/grep.rs @@ -587,7 +587,14 @@ impl Builtin for Grep { } } Err(e) => { - return Err(Error::Execution(format!("grep: {}: {}", pattern_file, e))); + return Ok(ExecResult::err( + format!( + "grep: {}: {}\n", + pattern_file, + crate::error::io_error_reason(&e) + ), + 2, + )); } } } diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index d4caccf3c..5a1c50abe 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -1361,6 +1361,10 @@ pub struct Interpreter { /// override baked-in commands. Survives `reset_transient_state` because /// it lives behind an `Arc` shared with the embedder. host_builtins: Option, + /// Names registered through `BashBuilder::builtin`. Their `Err` results + /// keep aborting execution (public contract); bundled builtins' usage + /// errors fail only the command (see `builtin_usage_error`). + custom_builtin_names: Arc>, /// Optional last-chance name resolver. Consulted only after every other /// dispatch route has missed, immediately before `command not found`, so /// it can never shadow a function, builtin, or `$PATH` script. @@ -1904,6 +1908,8 @@ impl Interpreter { builtins.retain(|name, _| filter(name)); } + let custom_builtin_names: Arc> = + Arc::new(custom_builtins.keys().cloned().collect()); // Merge custom builtins (override defaults if same name). // `Arc::from(Box)` reuses the existing allocation. for (name, builtin) in custom_builtins { @@ -1951,6 +1957,7 @@ impl Interpreter { last_exit_code: 0, builtins, host_builtins, + custom_builtin_names, command_resolver: None, call_stack: Vec::new(), bash_source_stack: Vec::new(), @@ -2108,6 +2115,7 @@ impl Interpreter { last_exit_code: self.last_exit_code, builtins: self.builtins.clone(), host_builtins: self.host_builtins.clone(), + custom_builtin_names: Arc::clone(&self.custom_builtin_names), command_resolver: self.command_resolver.clone(), call_stack: self.call_stack.clone(), bash_source_stack: self.bash_source_stack.clone(), @@ -7260,6 +7268,13 @@ impl Interpreter { stdin: Option<&'a crate::StreamData>, redirects: &'a [Redirect], ) -> std::pin::Pin> + Send + 'a>> { + // Bundled = one of bashkit's own builtins (not host, resolver or + // `BashBuilder::builtin` entries, whose `Err` stays fatal). + let bundled = !self.custom_builtin_names.contains(name) + && self + .builtins + .get(name) + .is_some_and(|b| std::ptr::addr_eq(Arc::as_ptr(b), Arc::as_ptr(&builtin))); Box::pin(async move { // Fire before_tool hooks — may modify args or cancel the invocation let args = if !self.hooks.before_tool.is_empty() { @@ -7402,6 +7417,16 @@ impl Interpreter { let mut result = match result { Ok(Ok(exec_result)) => exec_result, + // A builtin's own usage error (bad regex, awk syntax error, + // missing operand) fails only that command, as in bash: + // exit 2 with the message on stderr, and the script goes on. + // Limits, cancellation and I/O errors still abort. + Ok(Err(crate::error::Error::Execution(msg))) if bundled => { + builtin_usage_error(&msg) + } + Ok(Err(crate::error::Error::Regex(e))) if bundled => { + builtin_usage_error(&format!("{name}: {e}")) + } Ok(Err(e)) => return Err(e), Err(_panic) => { ExecResult::err(format!("bash: {}: builtin failed unexpectedly\n", name), 1) @@ -11295,6 +11320,21 @@ impl Interpreter { } } +/// Turn a builtin's usage error into a failed command (exit 2) instead of +/// aborting the script. THREAT[TM-INF-022]: the message is capped at 1 KB. +fn builtin_usage_error(msg: &str) -> ExecResult { + let mut msg = msg.trim_end().to_string(); + if msg.len() > 1024 { + let mut cut = 1024; + while !msg.is_char_boundary(cut) { + cut -= 1; + } + msg.truncate(cut); + } + msg.push('\n'); + ExecResult::err(msg, 2) +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/bashkit/tests/integration/threat_model_tests.rs b/crates/bashkit/tests/integration/threat_model_tests.rs index 9f87e126a..502db105f 100644 --- a/crates/bashkit/tests/integration/threat_model_tests.rs +++ b/crates/bashkit/tests/integration/threat_model_tests.rs @@ -4424,10 +4424,25 @@ mod trace_events { #[tokio::test] async fn trace_failed_exec_does_not_leak_events_to_next_exec() { - let mut bash = Bash::builder().trace_mode(TraceMode::Full).build(); + // A custom builtin's `Err` aborts the exec (bundled builtins' usage + // errors no longer do), which is the failure path under test. + struct Fatal; + #[async_trait::async_trait] + impl bashkit::Builtin for Fatal { + async fn execute( + &self, + _ctx: bashkit::BuiltinContext<'_>, + ) -> bashkit::Result { + Err(bashkit::Error::Execution("fatal".to_string())) + } + } + let mut bash = Bash::builder() + .trace_mode(TraceMode::Full) + .builtin("fatal", Box::new(Fatal)) + .build(); - let failed = bash.exec(r#"grep -E "(" tenant-a-private-arg"#).await; - assert!(failed.is_err(), "invalid regex should fail execution"); + let failed = bash.exec("fatal tenant-a-private-arg").await; + assert!(failed.is_err(), "fatal builtin should fail execution"); let r = bash.exec("echo tenant-b").await.unwrap(); assert_eq!(r.exit_code, 0); @@ -4435,7 +4450,7 @@ mod trace_events { for event in &r.events { if let TraceEventDetails::CommandStart { command, argv, .. } = &event.details { assert_ne!( - command, "grep", + command, "fatal", "stale failed command leaked into next exec" ); assert!( diff --git a/crates/bashkit/tests/spec_cases/bash/builtin-usage-errors.test.sh b/crates/bashkit/tests/spec_cases/bash/builtin-usage-errors.test.sh new file mode 100644 index 000000000..69773b208 --- /dev/null +++ b/crates/bashkit/tests/spec_cases/bash/builtin-usage-errors.test.sh @@ -0,0 +1,49 @@ +# A builtin's usage error (bad regex, awk syntax error, missing operand) +# fails that one command with exit 2; the script keeps running. + +### grep_invalid_regex_continues +echo a | grep -E "(" 2>/dev/null; echo "after=$?" +echo a | grep -E "(" 2>&1 | grep -c '^grep: ' +### expect +after=2 +1 +### end + +### grep_missing_pattern_continues +grep 2>/dev/null; echo "after=$?" +### expect +after=2 +### end + +### grep_missing_pattern_file_continues +grep -f /tmp/no-such-patterns a 2>&1; echo "after=$?" +### expect +grep: /tmp/no-such-patterns: No such file or directory +after=2 +### end + +### awk_syntax_error_continues +awk 'BEGIN{,}' 2>/dev/null; echo "after=$?" +awk 'BEGIN{,}' 2>&1 | grep -c '^awk: ' +### expect +after=2 +1 +### end + +### rg_invalid_regex_continues +### bash_diff: rg is not installed on the reference host +echo a | rg "(" 2>/dev/null; echo "after=$?" +### expect +after=2 +### end + +### usage_error_in_pipeline_and_errexit +echo a | grep -E "(" 2>/dev/null | cat; echo "pipe=$?" +set -e +(echo a | grep -E "(" 2>/dev/null) || echo "caught=$?" +echo done +### expect +pipe=0 +caught=2 +done +### end diff --git a/knowledge/foundations/builtins.md b/knowledge/foundations/builtins.md index 481db910b..482da10b4 100644 --- a/knowledge/foundations/builtins.md +++ b/knowledge/foundations/builtins.md @@ -318,6 +318,21 @@ continuation frames for shell control flow, pipelines, substitutions, redirects, accumulated output, and budgets; see [Snapshot History](snapshot-history.md). +### Errors: Failed Command vs Aborted Execution + +A builtin reports an ordinary failure as `Ok(ExecResult::err(msg, code))`. +For bashkit's own (bundled) builtins, an `Err(Error::Execution(_))` or +`Err(Error::Regex(_))` is treated the same way: the dispatcher turns it into +exit 2 with the message on stderr (capped at 1 KB, TM-INF-022), and the +script keeps running. That matches bash, where `grep -E '('` or an awk syntax +error fails one command, not the script. Before this, such errors aborted the +whole `exec()`. + +`Err` from a custom builtin (`BashBuilder::builtin`, a `BuiltinRegistry` +entry or a `CommandResolver`) still aborts execution: that is the public +contract custom builtins rely on. Resource limits, cancellation and I/O +errors abort for every builtin. + ### Adding Internal Builtins Simple builtins (zero-arg unit structs) are registered via the `register_builtins!` diff --git a/knowledge/log.md b/knowledge/log.md index 80c7fce69..ad2951726 100644 --- a/knowledge/log.md +++ b/knowledge/log.md @@ -2,6 +2,7 @@ ## 2026-10-07 +* **Fix**: A usage error in a bundled builtin no longer aborts the script. `grep -E '('`, `grep -P` with a bad pattern, `rg` with a bad regex, an awk syntax error, `grep` with no pattern and `grep -f missing` now fail that one command with exit 2 and the message on stderr, and the script continues (bash semantics; `set -e` still stops). `Err` from custom builtins (`BashBuilder::builtin`, registry, resolver) keeps aborting, per the public contract. See foundations/builtins "Errors: Failed Command vs Aborted Execution". * **Feature**: `gawk`, `mawk` and `nawk` run the awk builtin (same engine, including gawk extensions it already had such as `gensub`, `toupper`). Gap telemetry showed agents probing `gawk` by name. `--version` still identifies as bashkit awk. * **Fix**: `tar -P`/`--absolute-names` keeps the leading `/` (no notice), `-T FILE`/`--files-from` reads member names (blank lines skipped, `-` is stdin), and `--` ends options. `ls` implements the quoting styles: `-b`/`--escape`, `-N`, `-Q`, `--quoting-style=literal|escape|c|shell|shell-always|shell-escape|shell-escape-always` (GNU rules, including `"it's"` and headers left unescaped under `-b`); `locale`/`clocale` still report not implemented. Found by gap telemetry. * **Fix**: Arithmetic assigns to array elements. `((a[i]++))`, `((c[$k]+=3))`, `((a[2]=5))`, `((--a[0]))`, `let a[1]++` and `$((c[x]--))` read and write the element (indexed subscripts are arithmetic and accept negative indexes; associative keys are literal, quotes dropped as in `((c["x y"]++))`). Before, the update was lost or written to a scalar. Writes go through the budgeted `${a[i]:=v}` writer, so array memory limits still apply. Found by gap telemetry. From 15afca9f022ea5915a8cc2b9685ebc50ee9365ff Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Wed, 7 Oct 2026 13:01:53 +0000 Subject: [PATCH 2/2] test(builtins): awk usage-error spec independent of awk flavor Claude-Session: https://claude.ai/code/session_019aFikmptPc91Fj4N2iDXQA --- .../spec_cases/bash/builtin-usage-errors.test.sh | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/bashkit/tests/spec_cases/bash/builtin-usage-errors.test.sh b/crates/bashkit/tests/spec_cases/bash/builtin-usage-errors.test.sh index 69773b208..094647354 100644 --- a/crates/bashkit/tests/spec_cases/bash/builtin-usage-errors.test.sh +++ b/crates/bashkit/tests/spec_cases/bash/builtin-usage-errors.test.sh @@ -23,11 +23,15 @@ after=2 ### end ### awk_syntax_error_continues -awk 'BEGIN{,}' 2>/dev/null; echo "after=$?" -awk 'BEGIN{,}' 2>&1 | grep -c '^awk: ' +# Exit status (2 mawk, 1 gawk) and message layout vary by awk; only check +# that the command fails, reports on stderr, and the script continues. +awk 'BEGIN{,}' 2>/dev/null || echo failed +awk 'BEGIN{,}' 2>&1 >/dev/null | grep -q 'awk' && echo reported +echo after ### expect -after=2 -1 +failed +reported +after ### end ### rg_invalid_regex_continues