From af60e50c9b434815fab7c4add6db9e1a08318024 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 31 Aug 2026 13:15:50 -0400 Subject: [PATCH] A fault is never converted to a boolean MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The plan was to make `test` abort in a condition like `[[ ]]` does. Probing every position first showed `[[ ]]` was the wrong reference: it aborts inside an `if` and reads as false inside a `||`, so it did not agree with itself, and `(( ))` had the same split. statement if/while/! && / || (left) [[ ]] exit 2 abort reads false test exit 2 reads false reads false (( )) exit 2 abort reads false The `||` row is the one that does damage. All three printed a conclusion drawn from a comparison that never happened: x=abc; [[ "$x" -eq 1 ]] || echo "concluded: not one" That is the silent coercion the number rules exist to refuse, reached through control flow instead of arithmetic. The rule now is one sentence: a fault is never converted to a boolean. Where something consumes the result as a boolean there is no true or false to give it, so the statement aborts. Where nothing does — a standalone statement, or a chain's RIGHT operand, whose value simply becomes the chain's value — it is exit 2 with the message and execution continues. `ExecResult::fault` carries the distinction, because a command cannot say "I could not decide" through an exit code that callers already read as false. It is set by the three fault sources and read at the two places a boolean is demanded: a condition's command, and a chain's left operand. `accumulate_result` assigns it like `code`, so a fault reaching an outer chain through an inner chain's tail is still seen. A command that ran and failed is not a fault, and that boundary is tested: `grep` matching nothing and `false` still select `else` and still drive `||`. Only an operand that cannot be compared aborts. The controls carry the weight here. Every abort test is paired with a sound comparison in the same position, because a rule that aborts too eagerly would pass every abort test and destroy ordinary control flow. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 6 + crates/kaish-kernel/src/kernel.rs | 38 ++- crates/kaish-kernel/src/tools/builtin/test.rs | 10 +- .../fault_never_becomes_boolean_tests.rs | 241 ++++++++++++++++++ crates/kaish-types/src/result.rs | 30 +++ docs/LANGUAGE.md | 19 ++ 6 files changed, 339 insertions(+), 5 deletions(-) create mode 100644 crates/kaish-kernel/tests/fault_never_becomes_boolean_tests.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ef71a7c1..dcb8ece8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,12 @@ breaking entries are marked **BREAKING**. existing implementations keep compiling; not breaking. ### Fixed +- **A comparison kaish cannot make is a fault, not `false`** — `[[ ]]`, `test`, + and `(( ))` now agree: exit 2 where nothing reads the result as a boolean, and + an abort where something does (`if`/`while`, `!`, the left operand of + `&&`/`||`). `[[ $x -eq 1 ]] || echo no` no longer prints a conclusion drawn + from a comparison that never happened. A command that merely failed is + unaffected. - **A `[[ ]]` type error is exit 2, not a false reading** — `[[ $x -eq 1 ]]` with a non-numeric `$x` now reports code 2 with the message, matching `(( ))` and `test`. It was leaving `Kernel::execute` as an error, which diff --git a/crates/kaish-kernel/src/kernel.rs b/crates/kaish-kernel/src/kernel.rs index c081e5f0..a34ca19a 100644 --- a/crates/kaish-kernel/src/kernel.rs +++ b/crates/kaish-kernel/src/kernel.rs @@ -2960,6 +2960,17 @@ impl Kernel { ControlFlow::Normal(mut left_result) => { self.drain_stderr_into(&mut left_result).await; self.update_last_result(&left_result).await; + // The left operand is consumed as a boolean to + // decide whether the right one runs, and a fault has + // no boolean to give. Abort instead of reading it as + // failure — a fallback chosen from a comparison that + // never happened is a wrong conclusion, not a + // recovery. The RIGHT operand is not guarded: its + // value becomes the chain's value, so nothing + // consumes it as a boolean and it reports exit 2. + if left_result.fault { + return Err(anyhow::anyhow!("{}", left_result.err.trim_end())); + } // Pending is not failure (spec §I.5) — see the // `OrChain` twin. The stash check matters here for a // hold swallowed into an apparent success below. @@ -3011,6 +3022,17 @@ impl Kernel { ControlFlow::Normal(mut left_result) => { self.drain_stderr_into(&mut left_result).await; self.update_last_result(&left_result).await; + // The left operand is consumed as a boolean to + // decide whether the right one runs, and a fault has + // no boolean to give. Abort instead of reading it as + // failure — a fallback chosen from a comparison that + // never happened is a wrong conclusion, not a + // recovery. The RIGHT operand is not guarded: its + // value becomes the chain's value, so nothing + // consumes it as a boolean and it reports exit 2. + if left_result.fault { + return Err(anyhow::anyhow!("{}", left_result.err.trim_end())); + } // Pending is not failure (spec §I.5): a fallback // written for failure must not run on a decision // nobody has made yet — and running it would also @@ -3055,7 +3077,7 @@ impl Kernel { let result = match self.eval_test_async(test_expr).await { Ok(true) => ExecResult::success(""), Ok(false) => ExecResult::failure(1, ""), - Err(e) => ExecResult::failure(2, format!("{e:#}")), + Err(e) => ExecResult::failure(2, format!("{e:#}")).into_fault(), }; // A bare test writes `$?` and honors `set -e` like any command // (bash: `[[ 1 = 2 ]]; echo $?` → 1). `&&`/`||` operands stay @@ -3085,7 +3107,7 @@ 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()), + Err(e) => ExecResult::failure(2, e.to_string()).into_fault(), }; self.update_last_result(&result).await; if !result.ok() { @@ -4039,6 +4061,14 @@ impl Kernel { // the spill contract can remap it. A capped `if seq 1 // 100000` succeeded; only its output was too big to keep, // and reading the remapped 3 would send it to `else`. + // A fault has no boolean to give. Aborting here is the + // rule `[[ ]]` and `(( ))` already follow in this position; + // reading it as false would let `else` run on a comparison + // that never happened. + if result.fault { + self.emit_cmdsubst_stderr(&result.err).await; + return Err(anyhow::anyhow!("{}", result.err.trim_end())); + } let truthy = result.code == 0; // Carrying the stdout made this arm one of the surfaces // that produce a raw `ExecResult`, and it reaches @@ -7364,6 +7394,10 @@ fn accumulate_result(accumulated: &mut ExecResult, new: &ExecResult) { // bug this mechanism exists to fix, re-entering through a side door). accumulated.data = new.data.clone(); accumulated.data_is_value = new.data_is_value; + // Assign, like `code`: the combined result IS `new`'s value, so it is a + // fault exactly when `new` is. This lets an outer chain see a fault that + // arrived through an inner chain's right operand. + accumulated.fault = new.fault; // OR, not assign. `did_spill` is a fact about the OUTPUT — this block's // text was truncated — and an ordinary statement running afterwards does // not untruncate it. Assigning let `seq …; echo after` report diff --git a/crates/kaish-kernel/src/tools/builtin/test.rs b/crates/kaish-kernel/src/tools/builtin/test.rs index f22442aa..7f37a2af 100644 --- a/crates/kaish-kernel/src/tools/builtin/test.rs +++ b/crates/kaish-kernel/src/tools/builtin/test.rs @@ -146,13 +146,13 @@ impl Tool for Test { }; let argv = match args.to_argv() { Ok(v) => v, - Err(e) => return ExecResult::failure(2, format!("test: {e}")), + Err(e) => return ExecResult::failure(2, format!("test: {e}")).into_fault(), }; let parsed = match TestArgs::try_parse_from( std::iter::once("test".to_string()).chain(argv), ) { Ok(p) => p, - Err(e) => return ExecResult::failure(2, format!("test: {e}")), + Err(e) => return ExecResult::failure(2, format!("test: {e}")).into_fault(), }; parsed.global.apply(ctx); @@ -160,7 +160,11 @@ impl Tool for Test { match eval_test(ctx, &args.positional).await { Ok(true) => ExecResult::success(""), Ok(false) => ExecResult::failure(1, ""), - Err(msg) => ExecResult::failure(2, msg), + // A fault, not a false reading: an operand that cannot be + // compared leaves nothing to report as a boolean. In a condition, + // or as a chain's left operand, the kernel aborts on this rather + // than reading false — the same rule `[[ ]]` follows. + Err(msg) => ExecResult::failure(2, msg).into_fault(), } } } diff --git a/crates/kaish-kernel/tests/fault_never_becomes_boolean_tests.rs b/crates/kaish-kernel/tests/fault_never_becomes_boolean_tests.rs new file mode 100644 index 00000000..2074ffb5 --- /dev/null +++ b/crates/kaish-kernel/tests/fault_never_becomes_boolean_tests.rs @@ -0,0 +1,241 @@ +//! A fault is never converted to a boolean. +//! +//! `[[ ]]`, `test`, and `(( ))` all refuse a non-numeric operand. The refusal +//! is not in question; what these tests pin is what happens to it next. +//! +//! Where something CONSUMES the result as a boolean — an `if`/`while` +//! condition, a `!`, or the left operand of `&&`/`||` — there is no true or +//! false to hand it, so the statement aborts. Answering `false` there is a +//! silent coercion: `[[ $x -eq 1 ]] || echo "not one"` would print a +//! conclusion drawn from a comparison that never happened. +//! +//! Where NOTHING consumes it as a boolean — a standalone statement, or the +//! right operand of a chain, whose value simply becomes the chain's value — +//! the fault is reported as exit 2 with its message and execution continues. +//! The exit code is the report there, and 2 is distinguishable from a false +//! comparison's 1. +//! +//! All three constructs must agree in every position. Before this rule they +//! did not, and `[[ ]]` did not even agree with itself: it aborted inside an +//! `if` and read as false inside a `||`. + +// Test-fixture code: unwrap/expect on known-good setup is the idiom here. +#![allow(clippy::unwrap_used, clippy::expect_used)] + +use kaish_kernel::Kernel; + +/// The three spellings of the same broken comparison. `x` holds `abc` +/// throughout, so each is a type fault, not a false reading. +const FAULTS: [(&str, &str); 3] = [ + ("[[ ]]", r#"[[ "$x" -eq 1 ]]"#), + ("test", r#"test "$x" -eq 1"#), + ("(( ))", r#"(( x == 1 ))"#), +]; + +/// The same three spellings with an operand that is a real number, so they +/// genuinely evaluate. Used as controls: every abort test needs proof that +/// the position works at all when nothing faults. +const SOUND: [(&str, &str); 3] = [ + ("[[ ]]", r#"[[ "$x" -eq 1 ]]"#), + ("test", r#"test "$x" -eq 1"#), + ("(( ))", r#"(( x == 1 ))"#), +]; + +async fn aborts(script: &str) -> bool { + let kernel = Kernel::transient().unwrap(); + kernel.execute(script).await.is_err() +} + +async fn code_of(script: &str) -> i64 { + let kernel = Kernel::transient().unwrap(); + kernel + .execute(script) + .await + .expect("expected a result, not an abort") + .code +} + +// --- boolean-consuming positions: every construct aborts ------------------- + +#[tokio::test] +async fn fault_in_an_if_condition_aborts() { + for (name, expr) in FAULTS { + let script = format!("x=abc; if {expr}; then echo T; else echo F; fi"); + assert!(aborts(&script).await, "{name} must abort in an if condition"); + } +} + +#[tokio::test] +async fn fault_in_a_while_condition_aborts() { + for (name, expr) in FAULTS { + let script = format!("x=abc; while {expr}; do break; done"); + assert!( + aborts(&script).await, + "{name} must abort in a while condition" + ); + } +} + +#[tokio::test] +async fn fault_under_negation_aborts() { + for (name, expr) in FAULTS { + let script = format!("x=abc; if ! {expr}; then echo T; fi"); + assert!(aborts(&script).await, "{name} must abort under `!`"); + } +} + +#[tokio::test] +async fn fault_as_the_left_operand_of_and_aborts() { + for (name, expr) in FAULTS { + let script = format!("x=abc; {expr} && echo RIGHT_RAN"); + assert!( + aborts(&script).await, + "{name} must abort as the left operand of `&&`" + ); + } +} + +/// The headline case. Reading a fault as false here does not merely lose the +/// error, it prints a wrong conclusion. +#[tokio::test] +async fn fault_as_the_left_operand_of_or_aborts() { + for (name, expr) in FAULTS { + let script = format!(r#"x=abc; {expr} || echo "concluded: not one""#); + assert!( + aborts(&script).await, + "{name} must abort as the left operand of `||`, never conclude" + ); + } +} + +#[tokio::test] +async fn fault_in_a_compound_condition_aborts() { + for (name, expr) in FAULTS { + let script = format!("x=abc; if {expr} && [[ 1 -eq 1 ]]; then echo T; else echo F; fi"); + assert!( + aborts(&script).await, + "{name} must abort inside a compound condition" + ); + } +} + +// --- non-consuming positions: exit 2, execution continues ------------------ + +#[tokio::test] +async fn fault_as_a_standalone_statement_is_code_2() { + for (name, expr) in FAULTS { + let script = format!("x=abc; {expr}"); + assert_eq!(code_of(&script).await, 2, "{name} standalone is exit 2"); + } +} + +/// A standalone fault does not stop the statement list — the exit code is the +/// report, and nothing consumed it as a boolean. +#[tokio::test] +async fn a_standalone_fault_does_not_stop_the_script() { + for (name, expr) in FAULTS { + let kernel = Kernel::transient().unwrap(); + let script = format!("x=abc; {expr}; echo AFTER"); + let result = kernel + .execute(&script) + .await + .expect("a standalone fault is a result"); + assert!( + result.text_out().contains("AFTER"), + "{name} must not stop the statement list" + ); + } +} + +/// The right operand of a chain is not consumed as a boolean — its value +/// becomes the chain's value — so it reports rather than aborting. +#[tokio::test] +async fn fault_as_the_right_operand_of_and_is_code_2() { + for (name, expr) in FAULTS { + let script = format!("x=abc; true && {expr}"); + assert_eq!( + code_of(&script).await, + 2, + "{name} as a right operand reports, it does not abort" + ); + } +} + +// --- controls: the same positions work when nothing faults ---------------- + +#[tokio::test] +async fn control_sound_comparisons_still_decide_every_position() { + for (name, expr) in SOUND { + assert_eq!(code_of(&format!("x=1; {expr}")).await, 0, "{name} true"); + assert_eq!(code_of(&format!("x=2; {expr}")).await, 1, "{name} false"); + + let taken = format!("x=1; if {expr}; then echo T; else echo F; fi"); + let kernel = Kernel::transient().unwrap(); + let out = kernel.execute(&taken).await.expect("sound condition runs"); + assert!(out.text_out().contains('T'), "{name} true takes `then`"); + + let not_taken = format!("x=2; if {expr}; then echo T; else echo F; fi"); + let kernel = Kernel::transient().unwrap(); + let out = kernel + .execute(¬_taken) + .await + .expect("sound condition runs"); + assert!(out.text_out().contains('F'), "{name} false takes `else`"); + } +} + +/// A false comparison must still drive `||`. The abort is for faults only — +/// if this regressed, the rule would have eaten ordinary shell control flow. +#[tokio::test] +async fn control_a_false_comparison_still_runs_the_or_branch() { + for (name, expr) in SOUND { + let kernel = Kernel::transient().unwrap(); + let script = format!(r#"x=2; {expr} || echo "not one""#); + let out = kernel + .execute(&script) + .await + .expect("a false comparison is not a fault"); + assert!( + out.text_out().contains("not one"), + "{name}: a FALSE comparison must still run the `||` branch" + ); + } +} + +/// And a true comparison must still drive `&&`. +#[tokio::test] +async fn control_a_true_comparison_still_runs_the_and_branch() { + for (name, expr) in SOUND { + let kernel = Kernel::transient().unwrap(); + let script = format!(r#"x=1; {expr} && echo "is one""#); + let out = kernel + .execute(&script) + .await + .expect("a true comparison is not a fault"); + assert!( + out.text_out().contains("is one"), + "{name}: a TRUE comparison must still run the `&&` branch" + ); + } +} + +/// A command that merely FAILS is not a fault: `grep` finding nothing, or a +/// missing file, must keep selecting `else` and driving `||` as it always +/// has. The rule is about operands that cannot be compared, not about +/// commands that ran and said no. +#[tokio::test] +async fn control_an_ordinary_command_failure_is_not_a_fault() { + let kernel = Kernel::transient().unwrap(); + let out = kernel + .execute("if false; then echo T; else echo F; fi") + .await + .expect("an ordinary failure is not a fault"); + assert!(out.text_out().contains('F')); + + let kernel = Kernel::transient().unwrap(); + let out = kernel + .execute(r#"false || echo "fell back""#) + .await + .expect("an ordinary failure still drives ||"); + assert!(out.text_out().contains("fell back")); +} diff --git a/crates/kaish-types/src/result.rs b/crates/kaish-types/src/result.rs index 75e6dbaf..afac8ed3 100644 --- a/crates/kaish-types/src/result.rs +++ b/crates/kaish-types/src/result.rs @@ -132,6 +132,20 @@ pub struct ExecResult { /// capture buffer evicted its head with no spill file at all. All cases /// remap the exit code to 3. pub did_spill: bool, + /// The command could not decide, as opposed to deciding `false`. + /// + /// A non-numeric operand in `[[ ]]`, `test`, or `(( ))` is a fault: there + /// is no true or false to report, only a malformed comparison. Where + /// nothing consumes the result as a boolean this rides along with exit 2 + /// and is ignored. Where something DOES — an `if`/`while` condition, a + /// `!`, or the left operand of `&&`/`||` — the kernel aborts rather than + /// coerce a fault into a boolean, which would let a wrong conclusion be + /// drawn from a comparison that never happened. + /// + /// A command that ran and failed is NOT a fault. `grep` matching nothing + /// and a missing file still select `else` and still drive `||`. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub fault: bool, /// The command's original exit code before spill logic overwrote it with 2 or 3. /// Present only when `did_spill` is true and `code` was changed. #[serde(skip_serializing_if = "Option::is_none")] @@ -177,6 +191,7 @@ impl ExecResult { data: None, output: None, did_spill: false, + fault: false, original_code: None, content_type: None, baggage: BTreeMap::new(), @@ -200,6 +215,7 @@ impl ExecResult { data: None, output: Some(Box::new(output)), did_spill: false, + fault: false, original_code: None, content_type: None, baggage: BTreeMap::new(), @@ -237,6 +253,7 @@ impl ExecResult { data: Some(data), output: None, did_spill: false, + fault: false, original_code: None, content_type: None, baggage: BTreeMap::new(), @@ -260,6 +277,7 @@ impl ExecResult { data: Some(data), output: None, did_spill: false, + fault: false, original_code: None, content_type: None, baggage: BTreeMap::new(), @@ -270,6 +288,14 @@ impl ExecResult { /// /// The message is normalized to the stderr line contract: it ends with /// exactly one newline (unless empty), so renderers print it verbatim. + /// Mark this result as a fault: it could not decide, rather than + /// deciding `false`. See [`Self::fault`]. + #[must_use] + pub fn into_fault(mut self) -> Self { + self.fault = true; + self + } + pub fn failure(code: i64, err: impl Into) -> Self { Self { code, @@ -279,6 +305,7 @@ impl ExecResult { data: None, output: None, did_spill: false, + fault: false, original_code: None, content_type: None, baggage: BTreeMap::new(), @@ -299,6 +326,7 @@ impl ExecResult { data: None, output: None, did_spill: false, + fault: false, original_code: None, content_type: None, baggage: BTreeMap::new(), @@ -318,6 +346,7 @@ impl ExecResult { data: None, output: Some(Box::new(output)), did_spill: false, + fault: false, original_code: None, content_type: None, baggage: BTreeMap::new(), @@ -339,6 +368,7 @@ impl ExecResult { data, output: None, did_spill: false, + fault: false, original_code: None, content_type: None, baggage: BTreeMap::new(), diff --git a/docs/LANGUAGE.md b/docs/LANGUAGE.md index f6285c81..f46898d1 100644 --- a/docs/LANGUAGE.md +++ b/docs/LANGUAGE.md @@ -745,6 +745,25 @@ test -f a && test -f b # compound: chain with shell && / || if test -f "$path"; then …; fi # the usual home, an `if`/`while` condition ``` +A comparison kaish cannot make is a **fault**, not a false reading, and the +three spellings — `[[ ]]`, `test`, and `(( ))` — treat one identically. Where +nothing consumes the result as a boolean, a fault is exit `2` with its message +and the script continues. Where something does — an `if`/`while` condition, a +`!`, or the **left** operand of `&&`/`||` — the statement aborts, because there +is no true or false to give it: + +```sh +x=abc +test "$x" -eq 1 # exit 2, message, execution continues +test "$x" -eq 1 || echo "no" # aborts — it never prints "no" +if test "$x" -eq 1; then …; fi # aborts — it never takes `else` +true && test "$x" -eq 1 # exit 2: the right operand is the chain's value +``` + +Reading a fault as `false` would print a conclusion drawn from a comparison +that never happened. A command that ran and *failed* is not a fault: `grep` +matching nothing still selects `else` and still drives `||`. + `test` follows `[[`'s semantics, with a few deliberate, predictable differences: - **Numbers are kaish (JSON) numbers**, so `test 1.5 -gt 1` compares (it does not