diff --git a/CHANGELOG.md b/CHANGELOG.md index 27810010..191ed654 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,12 @@ breaking entries are marked **BREAKING**. register an external program as a tool with a declared grammar. Verbs and flags are deny-by-default, refused with exit 2 before any spawn; the kernel renders argv. Runs with `allow_external_commands` off. See `docs/wrapped_command.md`. +- **`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. - **Plan JSON carries build identity** — `kaish_version`, `kaish_git_hash`, and `kaish_build_date` ride along on every `--plan`/`--plan-file`/`plan` 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/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/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..e03249dc 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 `attributed_to` has not filled in 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-kernel/src/validator/walker.rs b/crates/kaish-kernel/src/validator/walker.rs index 6351d6f1..54e5d1dd 100644 --- a/crates/kaish-kernel/src/validator/walker.rs +++ b/crates/kaish-kernel/src/validator/walker.rs @@ -35,7 +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`, which nothing enforces. + /// `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, @@ -230,7 +239,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 @@ -259,6 +270,26 @@ 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 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 \ + 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); @@ -285,21 +316,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_suggestion("add gather: ... | scatter | cmd | gather") + .with_command(scatter_cmd.name.clone()), ); } @@ -692,7 +726,8 @@ impl<'a> Validator<'a> { "'{}' requires {} arguments, got {}", tool_def.name, required_count, positional_count ), - )); + ) + .with_command(tool_def.name.clone())); } } } @@ -1244,6 +1279,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 @@ -1347,10 +1393,24 @@ 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. + 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())) + .map(|i| { + ( + i.severity, + i.code, + i.message.as_str(), + i.suggestion.as_deref(), + i.command.as_deref(), + ) + }) .collect() } assert_eq!( @@ -1545,6 +1605,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] @@ -1646,5 +1714,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 0146e120..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 { @@ -195,3 +196,104 @@ async fn execution_display_walks_the_cause_chain_under_alternate() { "the real cause must survive `{{:#}}`: {alternate:?}" ); } + +// ── 5: 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); +} + +// ── 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/issue.rs b/crates/kaish-tool-api/src/issue.rs index 0313a595..9c841b82 100644 --- a/crates/kaish-tool-api/src/issue.rs +++ b/crates/kaish-tool-api/src/issue.rs @@ -262,6 +262,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. + /// + /// `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. + /// + /// `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. + /// + /// 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. + /// + /// 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, } impl ValidationIssue { @@ -273,6 +295,7 @@ impl ValidationIssue { message: message.into(), span: None, suggestion: None, + command: None, } } @@ -284,6 +307,7 @@ impl ValidationIssue { message: message.into(), span: None, suggestion: None, + command: None, } } @@ -299,6 +323,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. @@ -391,6 +425,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..f18a8db6 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,80 @@ 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")); + } + + /// 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( + 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")); + } } diff --git a/docs/EMBEDDING.md b/docs/EMBEDDING.md index 194a201b..0a23d0b6 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