From d4f021fe64f00938c28c1a72af8098f2c484b17e Mon Sep 17 00:00:00 2001 From: A Tobey Date: Sat, 12 Sep 2026 15:14:26 -0400 Subject: [PATCH 1/2] fix: show the full anyhow cause chain at three flattening sites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: an `anyhow::Error` built with `.context(...)` shows only its outermost frame under `{}`/`.to_string()`; `{:#}` walks the whole chain. `Stmt::Test`'s fault arm already uses `{e:#}`, but its sibling `Stmt::Arith` (bare `(( expr ))` as a statement), `timeout`'s re-dispatch fault arm, and `eval_redirect_target`'s command-dispatch evaluation still folded the error with the outermost-frame-only form, permanently baking the terse text into `ExecResult.err` and losing the real cause forever. Evidence: `(( $(x=$((1/0))) ))` reported only "failed to evaluate assignment" and hid "arithmetic error: `1 / 0` divides by zero" underneath. The same pattern reproduced through `echo hi > $(x=$((1/0)))` (redirect target evaluation) and through `timeout 5 boom` where `boom` is a user tool whose body faults the same way. Decision: use `format!("{e:#}")` at all three sites, matching the `Stmt::Test` precedent already in the codebase. Four new tests in `error_cause_chain_tests.rs` pin the fix (red before, green after) and one pins that the plain single-frame case (`(( 1/0 ))`) is unchanged. Two other call sites named in the same sweep — `scheduler::pipeline::run_single` and the concurrent pipeline stage path, both `Err(e) => ExecResult::failure(1, e.to_string())` — are left untouched: a sibling PR changes both to also keep the output that ran before the fault, and touching them here would conflict. Swept `to_string())`/`"{e}"` near `Err(e)` across `crates/kaish-kernel/src` and confirmed every other candidate site converts a flat, non-chaining error type (`EvalError`, `BackendError`, `WalkerError`, `TrashError`, `std::io::Error`) whose Display never drops a chain, so `{}` and `{:#}` are always identical there — no change needed. `spawn::hermetic_env`'s callers (`tools/wrapped.rs`) and `scheduler::pipeline::build_tool_args` do wrap a real `anyhow::Error`, but neither ever gains a second `.context()` frame in the current code, so there is no reachable input that makes `{}` and `{:#}` differ — left unchanged rather than "fixed" with no test able to prove it matters. Also found: `y=$(echo inner; x=$((1/0)))` doubles the same context text ("failed to evaluate assignment: failed to evaluate assignment: arithmetic error: ..."), because `Stmt::Assignment`'s `.context("failed to evaluate assignment")` (kernel.rs) fires once for the inner `x=` inside the command substitution and again for the outer `y=` around it — the same call site, same literal string, at two nesting levels. A one-line fix (naming the variable in the context text) would change the message `kernel_error_tests.rs` pins exactly ("failed to evaluate assignment") for the plain single-assignment case, so it is reported here rather than changed. Co-Authored-By: Claude Sonnet 5 --- crates/kaish-kernel/src/kernel.rs | 6 +- crates/kaish-kernel/src/scheduler/pipeline.rs | 6 +- .../kaish-kernel/src/tools/builtin/timeout.rs | 5 +- .../tests/error_cause_chain_tests.rs | 94 +++++++++++++++++++ 4 files changed, 108 insertions(+), 3 deletions(-) create mode 100644 crates/kaish-kernel/tests/error_cause_chain_tests.rs diff --git a/crates/kaish-kernel/src/kernel.rs b/crates/kaish-kernel/src/kernel.rs index 59fe5fe1..bdd178f7 100644 --- a/crates/kaish-kernel/src/kernel.rs +++ b/crates/kaish-kernel/src/kernel.rs @@ -3305,7 +3305,11 @@ impl Kernel { let result = match self.eval_arithmetic_async(expr_str).await { Ok(n) if n != 0 => ExecResult::success(""), Ok(_) => ExecResult::failure(1, ""), - Err(e) => ExecResult::failure(2, e.to_string()).into_fault(), + // Same fix as the `Stmt::Test` arm above: `e` is the + // evaluator's `anyhow::Error`, and a `$(...)` operand that + // faults (e.g. `(( $(x=$((1/0))) ))`) carries a real cause + // chain. `e.to_string()` showed only the outermost frame. + Err(e) => ExecResult::failure(2, format!("{e:#}")).into_fault(), }; self.update_last_result(&result).await; if !result.ok() { diff --git a/crates/kaish-kernel/src/scheduler/pipeline.rs b/crates/kaish-kernel/src/scheduler/pipeline.rs index 03a182d9..825f6ecb 100644 --- a/crates/kaish-kernel/src/scheduler/pipeline.rs +++ b/crates/kaish-kernel/src/scheduler/pipeline.rs @@ -253,10 +253,14 @@ async fn eval_redirect_target( if let Expr::NumericLiteral { raw, .. } = expr { return Ok(raw.clone()); } + // `e` here is the dispatch chain's `anyhow::Error` (`Kernel::eval_expr` + // runs the full async evaluator, including `$(...)`), so a nested fault + // (`> $(x=$((1/0)))`) carries a real cause chain. `{:#}` walks it instead + // of `to_string()`'s outermost-frame-only Display. let value = dispatcher .eval_expr(expr, ctx) .await - .map_err(|e| e.to_string())?; + .map_err(|e| format!("{e:#}"))?; // Decision D: a bare collection can't be a redirect target either — same // process-boundary guard as external argv (see `structured_boundary_error`). if let Some(msg) = crate::interpreter::structured_boundary_error("a redirect target", &value) { diff --git a/crates/kaish-kernel/src/tools/builtin/timeout.rs b/crates/kaish-kernel/src/tools/builtin/timeout.rs index e1568996..4d24cea3 100644 --- a/crates/kaish-kernel/src/tools/builtin/timeout.rs +++ b/crates/kaish-kernel/src/tools/builtin/timeout.rs @@ -174,7 +174,10 @@ impl Tool for Timeout { } result } - Err(e) => ExecResult::failure(1, format!("timeout: {}", e)), + // `e` is the re-dispatched command's `anyhow::Error` — a fault + // inside it (e.g. a user tool body's `x=$((1/0))`) carries a real + // cause chain. `{:#}` walks it; `{}` showed only the outer frame. + Err(e) => ExecResult::failure(1, format!("timeout: {e:#}")), } } } diff --git a/crates/kaish-kernel/tests/error_cause_chain_tests.rs b/crates/kaish-kernel/tests/error_cause_chain_tests.rs new file mode 100644 index 00000000..7b55588d --- /dev/null +++ b/crates/kaish-kernel/tests/error_cause_chain_tests.rs @@ -0,0 +1,94 @@ +//! An `anyhow::Error` chain built with `.context(...)` shows only its +//! outermost frame under `{}`/`.to_string()`; `{:#}` walks the whole chain. +//! Several sites folded a chained fault into `ExecResult.err` via +//! `.to_string()`, permanently baking in the terse form and discarding the +//! real cause forever (unlike `Kernel::execute`'s own `KernelError`, which +//! keeps the `anyhow::Error` alive so a caller can still choose `{:#}` later +//! — see `kernel_error_tests.rs`). +//! +//! Each test below reaches a fault two `.context()`/`anyhow!()` frames deep +//! — a nested `$(...)` around `x=$((1/0))` — and pins that the innermost +//! cause (`divides by zero`) survives to `ExecResult.err`. + +#![allow(clippy::unwrap_used, clippy::expect_used)] + +use kaish_kernel::{Kernel, KernelConfig}; + +fn kernel() -> Kernel { + Kernel::new(KernelConfig::isolated().with_skip_validation(true)).expect("failed to create kernel") +} + +/// `eval_redirect_target` (`scheduler/pipeline.rs`) ran a redirect target's +/// `$(...)` through the full async evaluator and folded a fault via +/// `.map_err(|e| e.to_string())`. A redirect target that is itself a command +/// substitution containing a failing assignment carries a two-frame chain +/// ("failed to evaluate assignment" wrapping "arithmetic error: ... divides +/// by zero"); `to_string()` showed only the outer frame. +#[tokio::test] +async fn redirect_target_fault_keeps_its_cause_chain() { + let kernel = kernel(); + let result = kernel + .execute("echo hi > $(x=$((1/0)))") + .await + .expect("redirect evaluation fails into an ExecResult, not a KernelError"); + + assert!(!result.ok(), "a failing redirect target must not succeed: {result:?}"); + assert!( + result.err.contains("divides by zero"), + "the redirect target's real cause must survive: {:?}", + result.err + ); +} + +/// `timeout`'s dispatch-error arm (`tools/builtin/timeout.rs`) formatted the +/// re-dispatched command's `anyhow::Error` with `{}` instead of `{:#}`. A +/// user tool body that faults through a chain of `.context()` calls (calling +/// it re-dispatches through the full statement executor) lost every frame +/// but the outermost. +#[tokio::test] +async fn timeout_dispatch_fault_keeps_its_cause_chain() { + let kernel = kernel().into_arc(); + let script = "function boom { x=$((1/0)) }\ntimeout 5 boom"; + let result = kernel.execute(script).await.expect("timeout's fault path returns an ExecResult"); + + assert!(!result.ok(), "a faulting tool body under timeout must not succeed: {result:?}"); + assert!( + result.err.contains("divides by zero"), + "the tool body's real cause must survive through timeout: {:?}", + result.err + ); +} + +/// `Stmt::Arith`'s fault arm (`kernel.rs`) is the sibling of `Stmt::Test`'s +/// (already fixed to use `{:#}`) but still folded its `anyhow::Error` with +/// `.to_string()`. A bare `(( $(...) ))` whose command substitution contains +/// a failing assignment carries the same two-frame chain as the redirect +/// case above. +#[tokio::test] +async fn bare_arith_statement_fault_keeps_its_cause_chain() { + let kernel = kernel(); + let result = kernel + .execute("(( $(x=$((1/0))) ))") + .await + .expect("a bare (( )) fault returns an ExecResult, not a KernelError"); + + assert!(!result.ok(), "a faulting (( )) command substitution must not succeed: {result:?}"); + assert!( + result.err.contains("divides by zero"), + "the arithmetic statement's real cause must survive: {:?}", + result.err + ); +} + +/// The plain, single-frame case must be unaffected by the `{:#}` fix: a bare +/// `(( 1/0 ))` has no outer `.context()` to add a second frame, so its +/// message is unchanged (this pins that the fix does not start showing +/// duplicate or unexpected text on the common case). +#[tokio::test] +async fn bare_arith_statement_simple_fault_is_unchanged() { + let kernel = kernel(); + let result = kernel.execute("(( 1/0 ))").await.expect("a bare (( )) fault returns an ExecResult"); + + assert!(!result.ok()); + assert_eq!(result.err.trim_end(), "arithmetic error: `1 / 0` divides by zero"); +} From 2ed5a3dba23d6673974ccf0144e0b0be910d4446 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Sat, 12 Sep 2026 15:25:42 -0400 Subject: [PATCH 2/2] style: drop explanatory comments from the cause-chain fixes `{e:#}` states its own intent, as the existing `Stmt::Test` arm does without a comment; the reasoning lives in the previous commit. Co-Authored-By: Claude Opus 5 --- crates/kaish-kernel/src/kernel.rs | 4 ---- crates/kaish-kernel/src/scheduler/pipeline.rs | 4 ---- crates/kaish-kernel/src/tools/builtin/timeout.rs | 3 --- 3 files changed, 11 deletions(-) diff --git a/crates/kaish-kernel/src/kernel.rs b/crates/kaish-kernel/src/kernel.rs index bdd178f7..e5cbb24d 100644 --- a/crates/kaish-kernel/src/kernel.rs +++ b/crates/kaish-kernel/src/kernel.rs @@ -3305,10 +3305,6 @@ impl Kernel { let result = match self.eval_arithmetic_async(expr_str).await { Ok(n) if n != 0 => ExecResult::success(""), Ok(_) => ExecResult::failure(1, ""), - // Same fix as the `Stmt::Test` arm above: `e` is the - // evaluator's `anyhow::Error`, and a `$(...)` operand that - // faults (e.g. `(( $(x=$((1/0))) ))`) carries a real cause - // chain. `e.to_string()` showed only the outermost frame. Err(e) => ExecResult::failure(2, format!("{e:#}")).into_fault(), }; self.update_last_result(&result).await; diff --git a/crates/kaish-kernel/src/scheduler/pipeline.rs b/crates/kaish-kernel/src/scheduler/pipeline.rs index 825f6ecb..d511fe3d 100644 --- a/crates/kaish-kernel/src/scheduler/pipeline.rs +++ b/crates/kaish-kernel/src/scheduler/pipeline.rs @@ -253,10 +253,6 @@ async fn eval_redirect_target( if let Expr::NumericLiteral { raw, .. } = expr { return Ok(raw.clone()); } - // `e` here is the dispatch chain's `anyhow::Error` (`Kernel::eval_expr` - // runs the full async evaluator, including `$(...)`), so a nested fault - // (`> $(x=$((1/0)))`) carries a real cause chain. `{:#}` walks it instead - // of `to_string()`'s outermost-frame-only Display. let value = dispatcher .eval_expr(expr, ctx) .await diff --git a/crates/kaish-kernel/src/tools/builtin/timeout.rs b/crates/kaish-kernel/src/tools/builtin/timeout.rs index 4d24cea3..ff1ce535 100644 --- a/crates/kaish-kernel/src/tools/builtin/timeout.rs +++ b/crates/kaish-kernel/src/tools/builtin/timeout.rs @@ -174,9 +174,6 @@ impl Tool for Timeout { } result } - // `e` is the re-dispatched command's `anyhow::Error` — a fault - // inside it (e.g. a user tool body's `x=$((1/0))`) carries a real - // cause chain. `{:#}` walks it; `{}` showed only the outer frame. Err(e) => ExecResult::failure(1, format!("timeout: {e:#}")), } }