Add ValidationIssue::command for structured command routing - #411
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…orced 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 <noreply@anthropic.com>
…-command # Conflicts: # CHANGELOG.md
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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. This adds a
commandfield, filled the same waywith_span/with_suggestionalready are, wherever the command name is genuinely known rather than guessed or scraped back out of a string.ValidationIssueis already#[non_exhaustive], so downstream code cannot construct it by struct literal, and adding a field is not a breaking change.validate_against_schemais the highest-leverage site:schema.nameis always known there, soMissingRequiredArg,UnknownFlag, andInvalidArgTypecarry the field for every builtin that goes through the shared defaultTool::validate, not just the ones with a custom override. Every other construction site in the kernel was walked and set wherever the name was actually in hand —walker.rs'sUndefinedCommandandScatterWithoutGather, the user-tool path, and each builtin whose ownTool::validateoverride pushes issues the shared schema check never sees, such asseq,jq,grep,test,sed, anddiff.The field stays absent, on purpose, where an issue is not about a command at all: assignment-target and for-loop-variable issues,
break/continue/returnoutside their construct, undefined-variable warnings, andMixedScriptNamewherever it fires. A guessed value there would be worse than an honest absence.Two end-to-end tests through
Kernel::executepin both shapes: aseqzero-increment error carriescommand: Some("seq"), andbreakoutside a loop carriescommand: None.🤖 Generated with Claude Code