From 8049526d7895be14d0841d30da96670e70f105d6 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Sun, 23 Aug 2026 11:27:42 -0400 Subject: [PATCH 1/9] Add ValidationIssue::command for structured command routing ValidationIssue told embedders to route on IssueCode rather than message text, but the one piece of structure an issue about a specific command actually needs -- which command -- lived only inside the prose. with_command() fills that gap the same way with_span/with_suggestion already do; ValidationIssue is already #[non_exhaustive] so downstream code cannot construct it by struct literal, and adding a field is not a breaking change. validate_against_schema is the highest-leverage site: schema.name is always known there, so MissingRequiredArg, UnknownFlag, and InvalidArgType (threaded through check_type_compatibility) get the field for every builtin that uses the shared default Tool::validate, not just the ones with a custom override. Co-Authored-By: Claude Opus 5 --- crates/kaish-tool-api/src/issue.rs | 39 ++++++++++++++++++++++++ crates/kaish-tool-api/src/tool.rs | 49 ++++++++++++++++++++++++++++-- 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/crates/kaish-tool-api/src/issue.rs b/crates/kaish-tool-api/src/issue.rs index f14468a9..b29ee774 100644 --- a/crates/kaish-tool-api/src/issue.rs +++ b/crates/kaish-tool-api/src/issue.rs @@ -253,6 +253,17 @@ pub struct ValidationIssue { pub span: Option, /// Optional suggestion for fixing the issue. pub suggestion: Option, + /// The command this issue concerns, when one is genuinely known — + /// `UndefinedCommand`'s unresolved name, or the builtin whose own + /// `Tool::validate` raised the issue (a bad regex, a missing required + /// argument, a zero `seq` increment, ...). + /// + /// Absent, never a placeholder, when an issue is not about a command at + /// all — an assignment target, a bare `break`, an undefined variable. + /// Route on `code` (this crate's own advice), then narrow by `command` + /// when the code can fire for more than one command; don't parse + /// `message` to recover a name this field already gives you. + pub command: Option, } impl ValidationIssue { @@ -264,6 +275,7 @@ impl ValidationIssue { message: message.into(), span: None, suggestion: None, + command: None, } } @@ -275,6 +287,7 @@ impl ValidationIssue { message: message.into(), span: None, suggestion: None, + command: None, } } @@ -290,6 +303,16 @@ impl ValidationIssue { self } + /// Record the command this issue concerns. + /// + /// Call this only where the name is genuinely known at the construction + /// site — the tool being validated, or the unresolved name itself for + /// `UndefinedCommand`. Leave it unset rather than guess. + pub fn with_command(mut self, command: impl Into) -> Self { + self.command = Some(command.into()); + self + } + /// Format the issue for display. /// /// With source provided, includes line:column information and source context. @@ -382,6 +405,22 @@ mod tests { assert!(formatted.contains("did you mean 'for'?")); } + #[test] + fn command_absent_by_default() { + let error = ValidationIssue::error(IssueCode::BreakOutsideLoop, "break outside a loop"); + assert_eq!(error.command, None); + + let warning = ValidationIssue::warning(IssueCode::PossiblyUndefinedVariable, "maybe undefined"); + assert_eq!(warning.command, None); + } + + #[test] + fn with_command_records_the_name() { + let issue = ValidationIssue::error(IssueCode::SeqZeroIncrement, "seq: increment cannot be zero") + .with_command("seq"); + assert_eq!(issue.command.as_deref(), Some("seq")); + } + #[test] fn get_line_at_offset_works() { let source = "line one\nline two\nline three"; diff --git a/crates/kaish-tool-api/src/tool.rs b/crates/kaish-tool-api/src/tool.rs index 2a26275d..96a27e90 100644 --- a/crates/kaish-tool-api/src/tool.rs +++ b/crates/kaish-tool-api/src/tool.rs @@ -78,6 +78,7 @@ pub fn validate_against_schema(args: &ToolArgs, schema: &ToolSchema) -> Vec", param.name, param.name)), + command: Some(schema.name.clone()), }); } } @@ -97,6 +98,7 @@ pub fn validate_against_schema(args: &ToolArgs, schema: &ToolSchema) -> Vec", param.name)), + command: Some(schema.name.clone()), }); } } @@ -130,6 +132,7 @@ pub fn validate_against_schema(args: &ToolArgs, schema: &ToolSchema) -> Vec Vec Vec bool { } /// Check if a value is compatible with a type. -fn check_type_compatibility(name: &str, value: &Value, expected_type: &str) -> Option { +fn check_type_compatibility( + name: &str, + value: &Value, + expected_type: &str, + command: &str, +) -> Option { let compatible = match expected_type { "any" => true, "string" => true, // Everything can be a string @@ -207,6 +215,7 @@ fn check_type_compatibility(name: &str, value: &Value, expected_type: &str) -> O ), span: None, suggestion: None, + command: Some(command.to_string()), }) } } @@ -317,4 +326,38 @@ mod validate_tests { issues ); } + + /// The schema always names the command being checked, so every issue + /// `validate_against_schema` raises should carry it — a caller that + /// calls `Tool::validate` directly (no walker in between) still gets a + /// structured command name, not just message text. + #[test] + fn missing_required_positional_carries_command_name() { + let schema = schema_with_positionals_after_flags(); + let args = ToolArgs::new(); + + let issues = validate_against_schema(&args, &schema); + let issue = issues + .iter() + .find(|i| i.code == IssueCode::MissingRequiredArg) + .expect("expected a MissingRequiredArg issue"); + assert_eq!(issue.command.as_deref(), Some("demo")); + } + + #[test] + fn invalid_arg_type_carries_command_name() { + let schema = ToolSchema::new("demo", "demo").param( + ParamSchema::new("count", "int").with_required(true).positional(), + ); + let mut args = ToolArgs::new(); + // Bool is not int-compatible per check_type_compatibility. + args.positional.push(Value::Bool(true)); + + let issues = validate_against_schema(&args, &schema); + let issue = issues + .iter() + .find(|i| i.code == IssueCode::InvalidArgType) + .expect("expected an InvalidArgType issue"); + assert_eq!(issue.command.as_deref(), Some("demo")); + } } From 1b0a687684a9651021d15c93b28a7d05ae91dc86 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Sun, 23 Aug 2026 11:27:50 -0400 Subject: [PATCH 2/9] Populate ValidationIssue::command at every site it is genuinely known Walked every ValidationIssue construction site in the kernel and set .command wherever the name is actually in hand -- never scraped back out of a message string: - walker.rs: UndefinedCommand (the unresolved cmd.name itself), ScatterWithoutGather ("scatter", the stage the rule is about), and the user-tool MissingRequiredArg path (tool_def.name). - seq/jq/grep/test/sed/diff: each builtin's own Tool::validate override pushes issues validate_against_schema never sees (zero increment, a bad regex/sed/jq expression, a wrong file-operand count, a refused test operator) -- self.name() is right there in every one of them. Left absent, on purpose: assignment-target and for-loop-variable issues, break/continue/return outside their construct, undefined- variable warnings, and MixedScriptName wherever it fires (an assignment target, a for-loop variable, or a name argument like `export`'s) -- none of these are about a command, and a guessed value would be worse than an honest absence. Co-Authored-By: Claude Opus 5 --- crates/kaish-kernel/src/tools/builtin/diff.rs | 3 ++- crates/kaish-kernel/src/tools/builtin/grep.rs | 4 +++- crates/kaish-kernel/src/tools/builtin/jq_native.rs | 3 ++- crates/kaish-kernel/src/tools/builtin/sed.rs | 3 ++- crates/kaish-kernel/src/tools/builtin/seq.rs | 6 +++--- crates/kaish-kernel/src/tools/builtin/test.rs | 3 ++- crates/kaish-kernel/src/validator/walker.rs | 11 ++++++++--- 7 files changed, 22 insertions(+), 11 deletions(-) diff --git a/crates/kaish-kernel/src/tools/builtin/diff.rs b/crates/kaish-kernel/src/tools/builtin/diff.rs index f5e0b85c..dcc9dbc7 100644 --- a/crates/kaish-kernel/src/tools/builtin/diff.rs +++ b/crates/kaish-kernel/src/tools/builtin/diff.rs @@ -105,7 +105,8 @@ impl Tool for Diff { IssueCode::DiffNeedsTwoFiles, format!("diff: needs exactly two file operands (got {operand_count})"), ) - .with_suggestion("usage: diff OLD NEW"), + .with_suggestion("usage: diff OLD NEW") + .with_command(self.name()), ); } diff --git a/crates/kaish-kernel/src/tools/builtin/grep.rs b/crates/kaish-kernel/src/tools/builtin/grep.rs index 50c7b69a..58a18144 100644 --- a/crates/kaish-kernel/src/tools/builtin/grep.rs +++ b/crates/kaish-kernel/src/tools/builtin/grep.rs @@ -189,7 +189,9 @@ impl Tool for Grep { rewrote, Some("-E"), ), - ).with_suggestion("check regex syntax at https://docs.rs/regex")); + ) + .with_suggestion("check regex syntax at https://docs.rs/regex") + .with_command(self.name())); } } diff --git a/crates/kaish-kernel/src/tools/builtin/jq_native.rs b/crates/kaish-kernel/src/tools/builtin/jq_native.rs index 43ff9c12..c641b0e7 100644 --- a/crates/kaish-kernel/src/tools/builtin/jq_native.rs +++ b/crates/kaish-kernel/src/tools/builtin/jq_native.rs @@ -507,7 +507,8 @@ impl Tool for JqNative { IssueCode::InvalidJqFilter, format!("jq: {msg}"), ) - .with_suggestion("check jq filter syntax: https://jqlang.org/manual/"), + .with_suggestion("check jq filter syntax: https://jqlang.org/manual/") + .with_command(self.name()), ); } diff --git a/crates/kaish-kernel/src/tools/builtin/sed.rs b/crates/kaish-kernel/src/tools/builtin/sed.rs index e8209b3b..8f617615 100644 --- a/crates/kaish-kernel/src/tools/builtin/sed.rs +++ b/crates/kaish-kernel/src/tools/builtin/sed.rs @@ -130,7 +130,8 @@ impl Tool for Sed { "commands: s/pat/rep/[gipN], y/abc/xyz/, d, p, q, a/i/c TEXT; \ chain with ; or -e; addresses: N, $, /re/, N,M; regex is ERE \ (egrep-style; GNU BRE \\| \\(…\\) \\{N,M\\} also accepted)", - ), + ) + .with_command(self.name()), ); } } diff --git a/crates/kaish-kernel/src/tools/builtin/seq.rs b/crates/kaish-kernel/src/tools/builtin/seq.rs index 4c0138a1..518c567a 100644 --- a/crates/kaish-kernel/src/tools/builtin/seq.rs +++ b/crates/kaish-kernel/src/tools/builtin/seq.rs @@ -59,13 +59,13 @@ impl Tool for Seq { issues.push(ValidationIssue::error( IssueCode::SeqZeroIncrement, "seq: increment cannot be zero (would cause infinite loop)", - )); + ).with_command(self.name())); } else if let Some(Value::Float(f)) = args.positional.get(1) { if *f == 0.0 { issues.push(ValidationIssue::error( IssueCode::SeqZeroIncrement, "seq: increment cannot be zero (would cause infinite loop)", - )); + ).with_command(self.name())); } } else if let Some(Value::String(s)) = args.positional.get(1) && let Ok(n) = s.parse::() @@ -73,7 +73,7 @@ impl Tool for Seq { issues.push(ValidationIssue::error( IssueCode::SeqZeroIncrement, "seq: increment cannot be zero (would cause infinite loop)", - )); + ).with_command(self.name())); } } diff --git a/crates/kaish-kernel/src/tools/builtin/test.rs b/crates/kaish-kernel/src/tools/builtin/test.rs index ba15c1af..595ba9d5 100644 --- a/crates/kaish-kernel/src/tools/builtin/test.rs +++ b/crates/kaish-kernel/src/tools/builtin/test.rs @@ -132,7 +132,8 @@ impl Tool for Test { IssueCode::TestCompoundOperator, format!("test: '{op}' is not supported — {COMPOUND_HINT}"), ) - .with_suggestion("test EXPR1 && test EXPR2, or [[ EXPR1 && EXPR2 ]]"), + .with_suggestion("test EXPR1 && test EXPR2, or [[ EXPR1 && EXPR2 ]]") + .with_command(self.name()), ], None => Vec::new(), } diff --git a/crates/kaish-kernel/src/validator/walker.rs b/crates/kaish-kernel/src/validator/walker.rs index 6351d6f1..d9adc342 100644 --- a/crates/kaish-kernel/src/validator/walker.rs +++ b/crates/kaish-kernel/src/validator/walker.rs @@ -230,7 +230,9 @@ impl<'a> Validator<'a> { self.issues.push(ValidationIssue::warning( IssueCode::UndefinedCommand, format!("command '{}' not found in builtin registry", cmd.name), - ).with_suggestion("this may be a script in PATH or external command")); + ) + .with_suggestion("this may be a script in PATH or external command") + .with_command(cmd.name.clone())); } // Validate arguments expressions @@ -299,7 +301,9 @@ impl<'a> Validator<'a> { ValidationIssue::error( IssueCode::ScatterWithoutGather, "scatter without gather — parallel results would be lost", - ).with_suggestion("add gather: ... | scatter | cmd | gather") + ) + .with_suggestion("add gather: ... | scatter | cmd | gather") + .with_command("scatter") ); } @@ -692,7 +696,8 @@ impl<'a> Validator<'a> { "'{}' requires {} arguments, got {}", tool_def.name, required_count, positional_count ), - )); + ) + .with_command(tool_def.name.clone())); } } } From 7982248a7341111606a5c4433be24e97928a1110 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Sun, 23 Aug 2026 11:27:53 -0400 Subject: [PATCH 3/9] Pin ValidationIssue::command's has-a-name and absent cases Two end-to-end tests through Kernel::execute, alongside the existing KernelError::Validation pins in this file: a seq zero-increment error (raised by seq's own Tool::validate, not the shared schema check) carries command: Some("seq"), and break outside a loop -- a language-level statement, not a command invocation -- carries command: None rather than an empty string or a guess. Co-Authored-By: Claude Opus 5 --- .../kaish-kernel/tests/kernel_error_tests.rs | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/crates/kaish-kernel/tests/kernel_error_tests.rs b/crates/kaish-kernel/tests/kernel_error_tests.rs index 0146e120..2eca064f 100644 --- a/crates/kaish-kernel/tests/kernel_error_tests.rs +++ b/crates/kaish-kernel/tests/kernel_error_tests.rs @@ -195,3 +195,46 @@ async fn execution_display_walks_the_cause_chain_under_alternate() { "the real cause must survive `{{:#}}`: {alternate:?}" ); } + +// ── 4: ValidationIssue::command — structural command routing, not prose +// parsing ───────────────────────────────────────────────────────── + +/// A validation issue that concerns a specific command exposes that command +/// name structurally, so an embedder can route on it instead of parsing +/// `message` (which also happens to say "seq" here, but a caller must not +/// have to scrape it out). +#[tokio::test] +async fn validation_issue_about_a_command_carries_its_name() { + let kernel = make_kernel(); + // seq's own `Tool::validate` raises SeqZeroIncrement (E004, Error + // severity) directly — not through the generic schema check — so this + // also pins that a builtin's own validate() populates `command`, not + // just the shared `validate_against_schema` path. + let err = kernel.execute("seq 1 0 10").await.expect_err("zero increment must be rejected"); + + let KernelError::Validation { issues, .. } = err else { + panic!("seq with a zero increment must be KernelError::Validation, not {err:?}"); + }; + + assert!( + issues.iter().any(|i| i.code == kaish_kernel::validator::IssueCode::SeqZeroIncrement + && i.command.as_deref() == Some("seq")), + "expected a SeqZeroIncrement issue naming 'seq': {issues:?}" + ); +} + +/// A validation issue that is not about any command — `break` outside a +/// loop is a language-level statement, not a command invocation — reports +/// the command as genuinely absent, never an empty string or a guess. +#[tokio::test] +async fn validation_issue_without_a_command_reports_absence() { + let kernel = make_kernel(); + let err = kernel.execute("break").await.expect_err("break outside a loop must be rejected"); + + let KernelError::Validation { issues, .. } = err else { + panic!("break outside a loop must be KernelError::Validation, not {err:?}"); + }; + + assert_eq!(issues.len(), 1, "expected exactly one validation issue: {issues:?}"); + assert_eq!(issues[0].command, None); +} From 17f7edeb4d34885a2c9da446476b726fdb4c6e45 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Sun, 23 Aug 2026 11:27:56 -0400 Subject: [PATCH 4/9] Changelog entry for ValidationIssue::command Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c549c17d..cee85a97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,15 @@ breaking entries are marked **BREAKING**. ## [Unreleased] +### Added + +- **`ValidationIssue::command`** — the command an issue concerns, when one is + genuinely known (`UndefinedCommand`'s name, a builtin's own regex/schema + failure), so an embedder can route on it instead of parsing `message`. + Absent, not a placeholder, for issues that aren't about a command at all + (a bad assignment target, `break` outside a loop). `#[non_exhaustive]` + already blocked struct-literal construction, so this is not breaking. + ## [0.16.0] - 2026-08-23 ### Added From 9c224e2240b3aa072dd8583398d5fc11649e5a92 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:29:51 -0400 Subject: [PATCH 5/9] Pin ValidationIssue::command at every populated site, not just two A cross-model review confirmed the population rule holds -- no site reconstructs or scrapes a command name out of prose, and every absent case is a decision -- but found the tests thin: two end-to-end paths (seq's Int branch, break's absence) covered by convention, with ten of about a dozen populated construction sites free to lose their `.with_command(...)` call without a single test going red. Closed the gap per layer, matching where each site is actually reachable: a new table-driven kernel_error_tests.rs case per builtin whose own Tool::validate populates the field through a real, parseable script (seq's Float/String branches, grep, sed, jq, diff, test, and walker.rs's own scatter/gather check); direct Validator assertions in walker.rs's own test module for the two sites a real script can never reach (UndefinedCommand is Warning severity, so KernelError::Validation's Error-only filter hides it; user-tool MissingRequiredArg is dead through the parser today since ToolDef params are always empty); and two new kaish-tool-api unit tests for the schema-driven paths the existing ones missed -- the required-flag branch of MissingRequiredArg (only the positional branch had a command pin) and UnknownFlag. Two structural fixes came out of writing these tests. The catalog-hit-vs-fallback comparison's render() helper dropped command from its tuple, so it could not have caught a divergence between the two validation paths on exactly the field this PR added -- included now. And ScatterWithoutGather populated command with a hardcoded "scatter" literal instead of the matched stage's own name; it cannot diverge today since the guard compares against the same constant, but cloning the matched Command's name ties the two together structurally instead of by convention, the same class of fix as the rest of this commit. Also enforced the invariant walker.rs already documented but never checked -- validate_command's schema-driven path assumes schema.name matches the command actually invoked, true by construction on a catalog hit but only by convention on the tool.schema() fallback -- with a debug_assert_eq! at the point schema is selected, so a future mismatch fails loudly in tests instead of silently misattributing an issue's command name. Co-Authored-By: Claude Opus 5 --- crates/kaish-kernel/src/validator/walker.rs | 85 ++++++++++++++++--- .../kaish-kernel/tests/kernel_error_tests.rs | 61 ++++++++++++- crates/kaish-tool-api/src/tool.rs | 42 +++++++++ 3 files changed, 173 insertions(+), 15 deletions(-) diff --git a/crates/kaish-kernel/src/validator/walker.rs b/crates/kaish-kernel/src/validator/walker.rs index d9adc342..147f9e48 100644 --- a/crates/kaish-kernel/src/validator/walker.rs +++ b/crates/kaish-kernel/src/validator/walker.rs @@ -35,7 +35,10 @@ pub struct Validator<'a> { /// it hits, and a hit is what skips the fallback. The kernel cannot go /// stale — the registry is frozen before `set_tool_schemas` snapshots it — /// but an embedder hand-building a catalog can. A hit also assumes - /// `tool.name() == tool.schema().name`, which nothing enforces. + /// `tool.name() == tool.schema().name`; `validate_command` `debug_assert_eq!`s + /// `schema.name == cmd.name` right after selecting `schema`, so a mismatch + /// (catalog or fallback) fails loudly in tests instead of silently + /// misattributing `ValidationIssue::command`. catalog: &'a [ToolSchema], /// Variable scope tracker. scope: ScopeTracker, @@ -261,6 +264,20 @@ impl<'a> Validator<'a> { &owned } }; + // `validate_against_schema` reads `schema.name` — not `cmd.name` — + // to populate `ValidationIssue::command` (E002/W001/E003). A + // catalog hit already guarantees `schema.name == cmd.name` (the + // binary search matched on it); the fallback trusts + // `tool.schema().name == tool.name()`, which the `catalog` field + // doc above calls out as unenforced. Enforce it here so a tool + // whose `schema()` disagrees with its own `name()` fails loudly + // in tests instead of silently misattributing an issue. + debug_assert_eq!( + schema.name, cmd.name, + "schema-driven validation issues for '{}' would carry the wrong command \ + name — schema.name ('{}') must match the command actually invoked", + cmd.name, schema.name + ); let tool_args = build_tool_args_for_validation(&cmd.args, Some(schema)); let tool_issues = tool.validate(&tool_args); self.issues.extend(tool_issues); @@ -287,23 +304,24 @@ impl<'a> Validator<'a> { /// Validate a pipeline. fn validate_pipeline(&mut self, pipe: &Pipeline) { - // Check for scatter without gather - let named = |name: &str| { - pipe.stages - .iter() - .filter_map(|s| s.as_command()) - .any(|c| c.name == name) - }; - let has_scatter = named("scatter"); - let has_gather = named("gather"); - if has_scatter && !has_gather { + // Check for scatter without gather. `command` is cloned off the + // matched stage's own name rather than hardcoded, so it can never + // drift from the guard that found it (both currently read "scatter", + // but nothing ties them together otherwise). + let scatter_stage = + pipe.stages.iter().filter_map(|s| s.as_command()).find(|c| c.name == "scatter"); + let has_gather = + pipe.stages.iter().filter_map(|s| s.as_command()).any(|c| c.name == "gather"); + if let Some(scatter_cmd) = scatter_stage + && !has_gather + { self.issues.push( ValidationIssue::error( IssueCode::ScatterWithoutGather, "scatter without gather — parallel results would be lost", ) .with_suggestion("add gather: ... | scatter | cmd | gather") - .with_command("scatter") + .with_command(scatter_cmd.name.clone()), ); } @@ -1249,6 +1267,17 @@ mod tests { let issues = validator.validate(&program); assert!(!issues.is_empty()); assert!(issues.iter().any(|i| i.code == IssueCode::UndefinedCommand)); + // The unresolved name is the whole point of the issue, so `command` + // must carry it structurally, not just inside `message`. Warning + // severity — this site is unreachable through `KernelError::Validation` + // (Error-only), so this direct `Validator` call is the only way to + // pin it at the field level. + assert!( + issues.iter().any(|i| i.code == IssueCode::UndefinedCommand + && i.command.as_deref() == Some("nonexistent_command")), + "UndefinedCommand must carry the unresolved name: {:?}", + issues + ); } /// `test` is a first-class builtin now, so it validates through the @@ -1352,10 +1381,15 @@ mod tests { got {fallback_issues:?}" ); - fn render(issues: &[ValidationIssue]) -> Vec<(Severity, IssueCode, &str, Option<&str>)> { + // `command` is included on purpose: it is schema-driven the same way + // `message` is (both read `schema.name`), so a catalog-hit/fallback + // divergence on the command name is exactly the class of bug this + // comparison exists to catch — dropping the field here would make + // that divergence invisible to the one test built to find it. + fn render(issues: &[ValidationIssue]) -> Vec<(Severity, IssueCode, &str, Option<&str>, Option<&str>)> { issues .iter() - .map(|i| (i.severity, i.code, i.message.as_str(), i.suggestion.as_deref())) + .map(|i| (i.severity, i.code, i.message.as_str(), i.suggestion.as_deref(), i.command.as_deref())) .collect() } assert_eq!( @@ -1550,6 +1584,14 @@ mod tests { let issues = validator.validate(&program); assert!(issues.iter().any(|i| i.code == IssueCode::ScatterWithoutGather), "should flag scatter without gather: {:?}", issues); + // `command` is cloned off the matched stage, not hardcoded — pin that + // it names the stage that actually triggered the check. + assert!( + issues.iter().any(|i| i.code == IssueCode::ScatterWithoutGather + && i.command.as_deref() == Some("scatter")), + "ScatterWithoutGather must carry the scatter stage's own name: {:?}", + issues + ); } #[test] @@ -1651,5 +1693,20 @@ mod tests { "missing positional should still error; got {:?}", issues ); + // A real script can never reach this site today — every `ToolDef` + // the parser produces has `params: vec![]` (`posix_function_parser` + // and `bash_function_parser` both hardcode it), so `required_count` + // is always 0 and the branch is dead outside a hand-built + // `HashMap` like this fixture's. Pin the field + // anyway: it is still a genuinely populated construction site + // (`walker.rs`'s own `.with_command(tool_def.name.clone())`), and a + // future parser change that lets user tools declare required params + // must not silently regress it. + assert!( + issues.iter().any(|i| i.code == IssueCode::MissingRequiredArg + && i.command.as_deref() == Some("mytool")), + "user-tool MissingRequiredArg must carry the tool's own name: {:?}", + issues + ); } } diff --git a/crates/kaish-kernel/tests/kernel_error_tests.rs b/crates/kaish-kernel/tests/kernel_error_tests.rs index 2eca064f..aecd0e6e 100644 --- a/crates/kaish-kernel/tests/kernel_error_tests.rs +++ b/crates/kaish-kernel/tests/kernel_error_tests.rs @@ -11,6 +11,7 @@ #![allow(clippy::unwrap_used, clippy::expect_used)] use kaish_kernel::{Kernel, KernelConfig, KernelError}; +use rstest::rstest; /// Transient kernel, matching the other error-surface test files. fn make_kernel() -> Kernel { @@ -196,7 +197,7 @@ async fn execution_display_walks_the_cause_chain_under_alternate() { ); } -// ── 4: ValidationIssue::command — structural command routing, not prose +// ── 5: ValidationIssue::command — structural command routing, not prose // parsing ───────────────────────────────────────────────────────── /// A validation issue that concerns a specific command exposes that command @@ -238,3 +239,61 @@ async fn validation_issue_without_a_command_reports_absence() { assert_eq!(issues.len(), 1, "expected exactly one validation issue: {issues:?}"); assert_eq!(issues[0].command, None); } + +// ── 6: ValidationIssue::command — pinned at every populated site a review +// found untested at the field level ────────────────────────────── + +/// Every populated `ValidationIssue::command` construction site that (a) +/// raises an Error-severity issue and (b) is reachable through a real, +/// parseable script, so `Kernel::execute` surfaces it as +/// `KernelError::Validation` without hand-building an AST. +/// +/// The prior two tests in this section pin exactly two paths — seq's Int +/// branch and `break`'s absence — which is a by-convention property, not a +/// falsifiable one: deleting a `.with_command(...)` call at any other +/// populated site left nothing red. This table closes that: each case +/// names the exact site's issue code and the exact command name it must +/// carry, so removing the call at that site turns `Some(name)` into `None` +/// and fails the matching case, not just the two already covered. +/// +/// seq's other two literal-typed increment branches (`seq.rs:68`, `:76`) +/// join the existing Int case (`seq.rs:62`); the rest are one case per +/// builtin's own `Tool::validate` override (grep.rs, sed.rs, jq_native.rs, +/// diff.rs, test.rs) plus the one non-builtin site, `walker.rs`'s own +/// scatter/gather pipeline check. +#[rstest] +#[case("seq 1 0.0 10", kaish_kernel::validator::IssueCode::SeqZeroIncrement, Some("seq"))] +#[case("seq 1 \"0\" 10", kaish_kernel::validator::IssueCode::SeqZeroIncrement, Some("seq"))] +#[case("grep '[' /dev/null", kaish_kernel::validator::IssueCode::InvalidRegex, Some("grep"))] +#[case("sed 's/a/' /dev/null", kaish_kernel::validator::IssueCode::InvalidSedExpr, Some("sed"))] +#[case("jq '.['", kaish_kernel::validator::IssueCode::InvalidJqFilter, Some("jq"))] +#[case("diff a.txt", kaish_kernel::validator::IssueCode::DiffNeedsTwoFiles, Some("diff"))] +#[case("test foo -a bar", kaish_kernel::validator::IssueCode::TestCompoundOperator, Some("test"))] +#[case( + "seq 1 3 | scatter | echo hi", + kaish_kernel::validator::IssueCode::ScatterWithoutGather, + Some("scatter") +)] +#[tokio::test] +async fn validation_issue_command_is_pinned_at_every_populated_site( + #[case] script: &str, + #[case] code: kaish_kernel::validator::IssueCode, + #[case] expected_command: Option<&str>, +) { + let kernel = make_kernel(); + let err = kernel.execute(script).await.expect_err(&format!("`{script}` must be rejected")); + + let KernelError::Validation { issues, .. } = err else { + panic!("`{script}` must be KernelError::Validation, not {err:?}"); + }; + + let issue = issues + .iter() + .find(|i| i.code == code) + .unwrap_or_else(|| panic!("`{script}` must raise {code:?}: {issues:?}")); + assert_eq!( + issue.command.as_deref(), + expected_command, + "`{script}` ({code:?}) command mismatch: {issues:?}" + ); +} diff --git a/crates/kaish-tool-api/src/tool.rs b/crates/kaish-tool-api/src/tool.rs index 96a27e90..f18a8db6 100644 --- a/crates/kaish-tool-api/src/tool.rs +++ b/crates/kaish-tool-api/src/tool.rs @@ -344,6 +344,48 @@ mod validate_tests { assert_eq!(issue.command.as_deref(), Some("demo")); } + /// The positional and flag branches are two separate loops in + /// `validate_against_schema` (see `required_flag_still_errors_when_missing` + /// above) — both push the same `IssueCode::MissingRequiredArg` from their + /// own `ValidationIssue { .. }` literal, so the field has to be pinned on + /// each independently. The positional branch is covered by + /// `missing_required_positional_carries_command_name`; this is the flag one. + #[test] + fn missing_required_flag_carries_command_name() { + let schema = ToolSchema::new("demo", "demo").param( + ParamSchema::new("output", "string") + .with_required(true) + .with_aliases(["o"]), + ); + + let args = ToolArgs::new(); + let issues = validate_against_schema(&args, &schema); + let issue = issues + .iter() + .find(|i| i.code == IssueCode::MissingRequiredArg) + .expect("expected a MissingRequiredArg issue"); + assert_eq!(issue.command.as_deref(), Some("demo")); + } + + /// `UnknownFlag` is its own `ValidationIssue { .. }` literal in the + /// unknown-flags loop, not shared with either `MissingRequiredArg` branch + /// — it needs its own pin. + #[test] + fn unknown_flag_carries_command_name() { + let schema = ToolSchema::new("demo", "demo").param( + ParamSchema::new("verbose", "bool").with_default(Some(Value::Bool(false))), + ); + let mut args = ToolArgs::new(); + args.flags.insert("bogus".to_string()); + + let issues = validate_against_schema(&args, &schema); + let issue = issues + .iter() + .find(|i| i.code == IssueCode::UnknownFlag) + .expect("expected an UnknownFlag issue"); + assert_eq!(issue.command.as_deref(), Some("demo")); + } + #[test] fn invalid_arg_type_carries_command_name() { let schema = ToolSchema::new("demo", "demo").param( From 929e65bf9f91090affec14e1b7307df50b844370 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:32:12 -0400 Subject: [PATCH 6/9] Fix the command field doc's unreachable example, and EMBEDDING.md's gap A review caught a reachability problem in the published doc text: the `command` field doc led with UndefinedCommand's unresolved name as its headline example, but UndefinedCommand is Warning severity and KernelError::Validation carries only Error-severity issues (kernel.rs filters on Severity::Error before building it). An embedder following the doc through KernelError, the documented path, would never see the example that doc leads with -- they would have to drive kaish-kernel's Validator directly, which the doc never mentioned. Reworked it to lead with a reachable example (a builtin's own Tool::validate raising an Error-severity issue about itself -- grep, sed, jq, seq, diff) and state the reachability split plainly, with UndefinedCommand named explicitly as the case that needs Validator instead of Kernel::execute. The same doc's first sentence over-reached too: "the builtin whose own Tool::validate raised the issue" reads as covering MixedScriptName, which also fires from a builtin's own validate() (export, read, unset, push, scatter --as, via the shared mixed_script_issue helper) but is deliberately absent, because the issue names a mis-spelled argument, not the command carrying it. Carved that out as its own paragraph instead of leaving the distinction implied. docs/EMBEDDING.md's KernelError::Validation bullet listed what a ValidationIssue carries without mentioning command at all -- the exact document this field exists to serve. Added it alongside the existing code/message/span mentions. Co-Authored-By: Claude Opus 5 --- crates/kaish-tool-api/src/issue.rs | 27 +++++++++++++++++++++------ docs/EMBEDDING.md | 9 +++++++-- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/crates/kaish-tool-api/src/issue.rs b/crates/kaish-tool-api/src/issue.rs index b29ee774..a213b7e5 100644 --- a/crates/kaish-tool-api/src/issue.rs +++ b/crates/kaish-tool-api/src/issue.rs @@ -253,13 +253,28 @@ pub struct ValidationIssue { pub span: Option, /// Optional suggestion for fixing the issue. pub suggestion: Option, - /// The command this issue concerns, when one is genuinely known — - /// `UndefinedCommand`'s unresolved name, or the builtin whose own - /// `Tool::validate` raised the issue (a bad regex, a missing required - /// argument, a zero `seq` increment, ...). + /// The command this issue concerns, when one is genuinely known. + /// + /// `Some(name)` for an Error-severity issue a builtin's own + /// `Tool::validate` override raises about *itself* — a bad `grep` + /// regex, an invalid `sed`/`jq` expression, a zero `seq` increment, a + /// wrong `diff` operand count, .... These reach an embedder matching + /// `KernelError::Validation`, since kaish-kernel filters that variant's + /// issues to Error severity (see `docs/EMBEDDING.md`). + /// + /// Also `Some(name)` for `UndefinedCommand`'s unresolved name — but that + /// issue is Warning severity, so it never reaches + /// `KernelError::Validation`; reading it means driving kaish-kernel's + /// `Validator` directly rather than going through `Kernel::execute`. + /// + /// `None`, never a placeholder, when an issue is not about a command at + /// all — an assignment target, a bare `break`, an undefined variable — + /// and also when a builtin's own `Tool::validate` raises an issue about + /// one of its *arguments* rather than about the command itself: + /// `MixedScriptName` fires from `export`/`read`/`unset`/`push`/ + /// `scatter --as`'s own `validate()`, but the mis-spelled name is the + /// argument, not the command, so it stays absent there too. /// - /// Absent, never a placeholder, when an issue is not about a command at - /// all — an assignment target, a bare `break`, an undefined variable. /// Route on `code` (this crate's own advice), then narrow by `command` /// when the code can fire for more than one command; don't parse /// `message` to recover a name this field already gives you. diff --git a/docs/EMBEDDING.md b/docs/EMBEDDING.md index 66930bc6..b21fda87 100644 --- a/docs/EMBEDDING.md +++ b/docs/EMBEDDING.md @@ -137,8 +137,13 @@ rather than parsing `Display` text: inventing a distinction the kernel doesn't make internally. - **`KernelError::Validation { issues, message }`** — the pre-execution validator rejected the program. `issues` is every error-severity - `ValidationIssue`, each carrying its `IssueCode`, message, and source span - — route on `code` (an enum), not on substring matches against `message`. + `ValidationIssue`, each carrying its `IssueCode`, message, source span, and + `command` — route on `code` (an enum), not on substring matches against + `message`. `command` names the command an issue concerns, `Some(name)` + when one is genuinely known (a builtin's own regex/schema failure) and + `None` when the issue isn't about a command at all (`break` outside a + loop); narrow by it once `code` alone isn't specific enough, rather than + parsing `message` to recover a name this field already gives you. - **`KernelError::Execution(anyhow::Error)`** — a statement began running and faulted: a builtin, the evaluator, an IO fault, or anything else the interpreter propagated. The original error chain is intact — `{:?}` and From cebcb1f5d0cd2f00e95d79fe6e900c576d96c6bf Mon Sep 17 00:00:00 2001 From: A Tobey Date: Mon, 24 Aug 2026 07:38:04 -0400 Subject: [PATCH 7/9] Replace a no-op release assert with a real test, fix the claim it enforced The debug_assert_eq! added for the schema.name/cmd.name invariant was itself the class of bug the previous commit fixed elsewhere: this workspace sets no [profile.release] override, so debug_assertions is off in every build kaish actually ships, and the assert compiled to nothing there. The catalog field doc's claim that the invariant "is now enforced" was therefore a contract comment describing behavior the shipped code does not have -- exactly the pattern flagged earlier in this same PR for a different site. Replaced the runtime claim with a real test: every_builtin_tool_name_matches_its_own_schema_name walks every tool register_builtins installs and asserts Tool::name() == Tool::schema().name for each, with a failure message that says what a mismatch means (validate_against_schema attributes issues by schema.name, the walker resolves by cmd.name, so a mismatch reports an issue against the wrong command) and what to do about it (make the two names agree, not relax the test). Verified it is discriminating by corrupting seq's schema name to "seq-corrupted" and confirming the test fails with that exact message, then reverting. This costs nothing at runtime, runs in CI on every cargo test, and covers every in-tree builtin at once rather than only whatever a given script happens to exercise. Kept the debug_assert_eq! at the call site -- harmless in debug and still documents the trust at the point of use -- but rewrote both comments to say plainly what holds the invariant (the registry test, for every in-tree tool) and what does not (the assert, since it is compiled out in release) and what isn't covered by either (a third-party tool an embedder registers at runtime). Co-Authored-By: Claude Opus 5 --- crates/kaish-kernel/src/tools/registry.rs | 42 +++++++++++++++++++++ crates/kaish-kernel/src/validator/walker.rs | 26 +++++++++---- 2 files changed, 61 insertions(+), 7 deletions(-) diff --git a/crates/kaish-kernel/src/tools/registry.rs b/crates/kaish-kernel/src/tools/registry.rs index e6d30e49..b8877ef1 100644 --- a/crates/kaish-kernel/src/tools/registry.rs +++ b/crates/kaish-kernel/src/tools/registry.rs @@ -106,6 +106,48 @@ mod tests { assert!(!registry.contains("nonexistent")); } + /// `validate_command` (validator/walker.rs) falls back to `tool.schema()` + /// on a catalog miss, and `validate_against_schema` attributes every + /// `ValidationIssue` it raises to that schema's `.name` — not to the AST + /// `cmd.name` the walker actually resolved and dispatched by (`== + /// tool.name()`, since `register`/`register_arc` key this map on it). + /// Nothing at the `Tool` trait level keeps `name()` and `schema().name` + /// in sync; a tool whose two names disagree would silently misattribute + /// every issue it raises to the wrong command. + /// + /// This test is the actual enforcement of that invariant. A + /// `debug_assert!` at the call site would compile to nothing in a + /// release build — this workspace has no `[profile.release]` override, + /// so `debug_assertions` is off wherever kaish actually ships — while + /// this test runs in CI on every `cargo test` and covers every in-tree + /// builtin at once, not only the ones a particular script happens to + /// exercise. + /// + /// Only covers what `register_builtins` installs — a third-party tool + /// an embedder registers at runtime is not walked here and is not + /// covered by anything else either. + #[test] + fn every_builtin_tool_name_matches_its_own_schema_name() { + let mut registry = ToolRegistry::new(); + crate::tools::register_builtins(&mut registry); + + for name in registry.names() { + let tool = registry.get(name).expect("a name from names() must resolve via get()"); + assert_eq!( + tool.name(), + tool.schema().name.as_str(), + "tool '{name}': Tool::name() == {:?} but Tool::schema().name == {:?} -- \ + these must agree. validate_against_schema attributes every ValidationIssue \ + it raises to schema.name, while the walker resolves and dispatches the \ + command by cmd.name (== tool.name()); a mismatch here means an embedder \ + reading ValidationIssue::command sees the wrong command. Fix the tool so \ + Tool::name() and Tool::schema().name agree -- do not relax this test.", + tool.name(), + tool.schema().name, + ); + } + } + #[test] fn test_names_sorted() { let mut registry = ToolRegistry::new(); diff --git a/crates/kaish-kernel/src/validator/walker.rs b/crates/kaish-kernel/src/validator/walker.rs index 147f9e48..4d630796 100644 --- a/crates/kaish-kernel/src/validator/walker.rs +++ b/crates/kaish-kernel/src/validator/walker.rs @@ -35,10 +35,16 @@ pub struct Validator<'a> { /// it hits, and a hit is what skips the fallback. The kernel cannot go /// stale — the registry is frozen before `set_tool_schemas` snapshots it — /// but an embedder hand-building a catalog can. A hit also assumes - /// `tool.name() == tool.schema().name`; `validate_command` `debug_assert_eq!`s - /// `schema.name == cmd.name` right after selecting `schema`, so a mismatch - /// (catalog or fallback) fails loudly in tests instead of silently - /// misattributing `ValidationIssue::command`. + /// `tool.name() == tool.schema().name` — on the fallback path this is + /// what keeps `validate_against_schema`'s `schema.name` (which populates + /// `ValidationIssue::command`) matching the command the walker actually + /// resolved and dispatched by. Nothing at runtime enforces this; ship + /// builds have no `[profile.release]` override in this workspace, so + /// `debug_assert!` compiles to nothing there. What holds it is + /// `tools::registry::tests::every_builtin_tool_name_matches_its_own_schema_name`, + /// which walks every builtin `register_builtins` installs and is true + /// for all of them today. It does not cover a third-party tool an + /// embedder registers — nothing does. catalog: &'a [ToolSchema], /// Variable scope tracker. scope: ScopeTracker, @@ -269,9 +275,15 @@ impl<'a> Validator<'a> { // catalog hit already guarantees `schema.name == cmd.name` (the // binary search matched on it); the fallback trusts // `tool.schema().name == tool.name()`, which the `catalog` field - // doc above calls out as unenforced. Enforce it here so a tool - // whose `schema()` disagrees with its own `name()` fails loudly - // in tests instead of silently misattributing an issue. + // doc above calls out as unenforced by anything at this call + // site — this `debug_assert_eq!` documents that trust here, but + // it is not the enforcement: `debug_assertions` is off in every + // build this workspace ships (no `[profile.release]` override), + // so it compiles to nothing outside `cargo test`/`cargo build` + // without `--release`. The actual invariant is pinned by + // `tools::registry::tests::every_builtin_tool_name_matches_its_own_schema_name`, + // which walks every in-tree builtin regardless of which script + // happens to exercise it. debug_assert_eq!( schema.name, cmd.name, "schema-driven validation issues for '{}' would carry the wrong command \ From b0027a72a0acd4d9d63a4a2b3147447f19000c7e Mon Sep 17 00:00:00 2001 From: A Tobey Date: Tue, 25 Aug 2026 11:28:10 -0400 Subject: [PATCH 8/9] A wrapped command's refusal left the field it was about empty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merging main in brought wrapped commands alongside this field for the first time, and they did not meet. Every `WrappedError` variant carries a `command`, every message opens with it, and `issue()` mapped none of them — so `git log --output=x` produced "git: unknown flag ..." with `command: None`. Absent is not neutral here. It is this field's way of saying "not about a command," which is the opposite of true for a message that names one, and an embedder routing on the field would have gone back to parsing the string the field exists to replace. `WrappedError::command()` reads the name every variant already holds. Empty stays absent — a path failure before `attributed_to` has run has no name yet, and inventing one would repeat the mistake. The field doc claimed argument-level issues stay absent, which reads as the opposite of what the schema-driven sites do: they record the schema's name. Rewritten around what is on hand, with the schema-name-versus-invoked-name limit stated. --- crates/kaish-kernel/src/tools/wrapped.rs | 10 +++++- .../kaish-kernel/src/tools/wrapped/error.rs | 25 +++++++++++++ .../kaish-kernel/src/tools/wrapped/tests.rs | 8 +++++ crates/kaish-tool-api/src/issue.rs | 36 +++++++++---------- 4 files changed, 58 insertions(+), 21 deletions(-) diff --git a/crates/kaish-kernel/src/tools/wrapped.rs b/crates/kaish-kernel/src/tools/wrapped.rs index 9bff2fee..6daf61a3 100644 --- a/crates/kaish-kernel/src/tools/wrapped.rs +++ b/crates/kaish-kernel/src/tools/wrapped.rs @@ -484,10 +484,18 @@ fn issue(error: &WrappedError, uncertain: bool) -> ValidationIssue { } _ => IssueCode::WrappedCallRejected, }; - if uncertain { + let issue = if uncertain { ValidationIssue::warning(code, error.to_string()) } else { ValidationIssue::error(code, error.to_string()) + }; + // Every variant names its command, so absent here would mean "not about a + // command" for a message that opens with one. Empty only before + // `attributed_to` has run. + if error.command().is_empty() { + issue + } else { + issue.with_command(error.command().to_string()) } } diff --git a/crates/kaish-kernel/src/tools/wrapped/error.rs b/crates/kaish-kernel/src/tools/wrapped/error.rs index 6620c5a5..e7abf2e8 100644 --- a/crates/kaish-kernel/src/tools/wrapped/error.rs +++ b/crates/kaish-kernel/src/tools/wrapped/error.rs @@ -227,6 +227,31 @@ impl WrappedError { 2 } + /// The wrapped command this refusal is about. Empty only for a path + /// failure that has not been through [`Self::attributed_to`] yet. + pub fn command(&self) -> &str { + match self { + WrappedError::UnknownVerb { command, .. } + | WrappedError::MissingVerb { command, .. } + | WrappedError::UnknownFlag { command, .. } + | WrappedError::ClusteredShort { command, .. } + | WrappedError::GluedShortValue { command, .. } + | WrappedError::MissingFlagValue { command, .. } + | WrappedError::UnexpectedFlagValue { command, .. } + | WrappedError::RepeatedFlag { command, .. } + | WrappedError::UnexpectedArgument { command, .. } + | WrappedError::UndeclaredPositional { command, .. } + | WrappedError::MissingRequiredFlag { command, .. } + | WrappedError::MissingRequiredPositional { command, .. } + | WrappedError::NotAnInteger { command, .. } + | WrappedError::NotInChoices { command, .. } + | WrappedError::PathOutsideRoot { command, .. } + | WrappedError::PathRootUnresolvable { command, .. } + | WrappedError::NulByte { command, .. } + | WrappedError::BinaryArgument { command, .. } => command, + } + } + /// Fill in the command and positional a bare path failure belongs to. /// /// [`crate::tools::wrapped::path_is_under`]'s companion diff --git a/crates/kaish-kernel/src/tools/wrapped/tests.rs b/crates/kaish-kernel/src/tools/wrapped/tests.rs index 94d26833..48b263aa 100644 --- a/crates/kaish-kernel/src/tools/wrapped/tests.rs +++ b/crates/kaish-kernel/src/tools/wrapped/tests.rs @@ -483,6 +483,14 @@ fn a_literal_unknown_flag_is_an_error_before_anything_runs() { ); } +#[test] +fn a_refusal_records_the_command_it_is_about() { + // The message opens with the command, so an absent field here would read + // as "not about a command" — the one thing the field exists to mean. + let issues = tool(git()).validate(&args(&["log", "--output=/tmp/x"])); + assert_eq!(issues[0].command.as_deref(), Some("git"), "{issues:?}"); +} + #[test] fn an_opaque_word_cannot_be_an_unknown_flag() { // `git log "$branch"` — the validator has no value, so it has no verdict. diff --git a/crates/kaish-tool-api/src/issue.rs b/crates/kaish-tool-api/src/issue.rs index 01bc4586..9c841b82 100644 --- a/crates/kaish-tool-api/src/issue.rs +++ b/crates/kaish-tool-api/src/issue.rs @@ -264,29 +264,25 @@ pub struct ValidationIssue { pub suggestion: Option, /// The command this issue concerns, when one is genuinely known. /// - /// `Some(name)` for an Error-severity issue a builtin's own - /// `Tool::validate` override raises about *itself* — a bad `grep` - /// regex, an invalid `sed`/`jq` expression, a zero `seq` increment, a - /// wrong `diff` operand count, .... These reach an embedder matching - /// `KernelError::Validation`, since kaish-kernel filters that variant's - /// issues to Error severity (see `docs/EMBEDDING.md`). + /// `Some(name)` when a name is on hand: a builtin's own `Tool::validate` + /// raising about itself, a schema-driven argument issue (the schema's + /// name), a wrapped command's refusal, `UndefinedCommand`'s unresolved + /// name, `scatter` without a gather, and a user tool's arity failure. /// - /// Also `Some(name)` for `UndefinedCommand`'s unresolved name — but that - /// issue is Warning severity, so it never reaches - /// `KernelError::Validation`; reading it means driving kaish-kernel's - /// `Validator` directly rather than going through `Kernel::execute`. + /// `None`, never a placeholder, when the issue is not about a command at + /// all — an assignment target, a bare `break`, an undefined variable, or + /// `MixedScriptName`, where the mis-spelled name is the argument. /// - /// `None`, never a placeholder, when an issue is not about a command at - /// all — an assignment target, a bare `break`, an undefined variable — - /// and also when a builtin's own `Tool::validate` raises an issue about - /// one of its *arguments* rather than about the command itself: - /// `MixedScriptName` fires from `export`/`read`/`unset`/`push`/ - /// `scatter --as`'s own `validate()`, but the mis-spelled name is the - /// argument, not the command, so it stays absent there too. + /// Severity varies: `UndefinedCommand` is a Warning and so never reaches + /// `KernelError::Validation`, which kaish-kernel filters to Error. + /// Reading it means driving the `Validator` directly. /// - /// Route on `code` (this crate's own advice), then narrow by `command` - /// when the code can fire for more than one command; don't parse - /// `message` to recover a name this field already gives you. + /// One limit: schema-driven issues record the SCHEMA's name, which equals + /// the invoked name for every builtin (pinned by a registry test) but is + /// not enforced for a tool an embedder registers. + /// + /// Route on `code`, then narrow by `command`; don't parse `message` to + /// recover a name this field already gives you. pub command: Option, } From 5f5acb54f9d25f97359b744bdd4abb6af13b9b0d Mon Sep 17 00:00:00 2001 From: A Tobey Date: Tue, 25 Aug 2026 11:36:39 -0400 Subject: [PATCH 9/9] Two gate failures the branch introduced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding `command` to the catalog-vs-fallback comparison grew its helper to a five-tuple, which trips `clippy::type_complexity`. A type alias keeps the field in the comparison, which is the point of that test. The new `command()` doc linked `attributed_to`, which is pub(crate), from a pub item — a rustdoc error under `-D warnings`. Named, not linked. --- crates/kaish-kernel/src/tools/wrapped/error.rs | 2 +- crates/kaish-kernel/src/validator/walker.rs | 13 +++++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/crates/kaish-kernel/src/tools/wrapped/error.rs b/crates/kaish-kernel/src/tools/wrapped/error.rs index e7abf2e8..e03249dc 100644 --- a/crates/kaish-kernel/src/tools/wrapped/error.rs +++ b/crates/kaish-kernel/src/tools/wrapped/error.rs @@ -228,7 +228,7 @@ impl WrappedError { } /// The wrapped command this refusal is about. Empty only for a path - /// failure that has not been through [`Self::attributed_to`] yet. + /// failure that `attributed_to` has not filled in yet. pub fn command(&self) -> &str { match self { WrappedError::UnknownVerb { command, .. } diff --git a/crates/kaish-kernel/src/validator/walker.rs b/crates/kaish-kernel/src/validator/walker.rs index 4d630796..54e5d1dd 100644 --- a/crates/kaish-kernel/src/validator/walker.rs +++ b/crates/kaish-kernel/src/validator/walker.rs @@ -1398,10 +1398,19 @@ mod tests { // divergence on the command name is exactly the class of bug this // comparison exists to catch — dropping the field here would make // that divergence invisible to the one test built to find it. - fn render(issues: &[ValidationIssue]) -> Vec<(Severity, IssueCode, &str, Option<&str>, Option<&str>)> { + type Rendered<'a> = (Severity, IssueCode, &'a str, Option<&'a str>, Option<&'a str>); + fn render(issues: &[ValidationIssue]) -> Vec> { issues .iter() - .map(|i| (i.severity, i.code, i.message.as_str(), i.suggestion.as_deref(), i.command.as_deref())) + .map(|i| { + ( + i.severity, + i.code, + i.message.as_str(), + i.suggestion.as_deref(), + i.command.as_deref(), + ) + }) .collect() } assert_eq!(