Reserve exit 1 for results, exit 2 for mistakes - #444
Merged
Conversation
An agent ran `grep -v '[cast:' lines.txt` and read the failure as "no
lines matched". The quoting was correct: the pattern reached the regex
engine intact and failed to compile there, and grep reported that with
exit 1 — the code it also uses for a search that found nothing. A caller
branching on $? cannot tell those apart.
The survey showed the rule already existed, half-applied. cmp and diff
spend exit 1 on "the inputs differ" and report every error as 2. grep did
not: seven sites (missing pattern, file read errors, invalid UTF-8)
returned 1, as did two operand checks in diff sitting next to siblings
returning 2.
The rule now in force: a builtin that answers a question spends exit 1 on
the negative answer and nothing else, so every error in grep, test, cmp,
and diff exits 2. A builtin where 1 is free keeps the familiar split — 2
for usage, 1 for an operational failure — so `cat missing.txt` is
unchanged.
The same split was missing a layer up. `kaish --plan 'if'` exited 2 and
documented it as "the same usage-error code a builtin returns for bad
argv", while `kaish -c 'if'` exited 1 for the identical rejection. Both
now exit 2, keyed off KernelError::is_rejected, which already existed to
name this class.
Probing that boundary found a worse gap: --plan never validated, so it
printed a clean plan and exited 0 for a program the kernel then refused.
A dry run that green-lights an unrunnable command is worse than none.
--plan now runs the validator through a new pure
validator::validate_program, filtered to Error to match what the kernel
refuses on.
The error text pointed at docs.rs/regex — our implementation crate, and
nothing a model can act on. It now names the escape: `\[` for a literal
`[`, and `[(]` / `[{]` where a backslash would instead be read as a BRE
operator, a distinction bre_metas_to_ere creates and the hint respects.
Last, 98 builtins opened by downcasting to ExecContext and returned exit
1 with "internal error: kernel builtin requires ExecContext" if it
failed. The builtin types are private to the crate and reachable only
through the kernel's registry, which always dispatches an ExecContext, so
the branch cannot be taken. An impossible error must still panic rather
than hand a script a number it reads as data; they now call
tools::exec_context, which does.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three holes, found by auditing the change rather than the diff. grep still returned 1 for an uncompilable pattern. validate() compiles the pattern before the run, but it skips any pattern holding a `<dynamic>` marker, which is what a variable or command substitution becomes. So `p='[cast:'; grep "$p" f` sailed past validation and failed inside execute(), at two builders the first pass never touched — they spell the code on the line after `failure(`, where a single-line search for `failure(1` cannot see it. Both now exit 2, with a test that drives the pattern through a variable. stream_grep discarded a read error: `Err(_) => break` fell through to the same "no matches" exit 1 that a clean empty search returns. A read failure now exits 2. A *write* failure still breaks quietly, because a closed downstream pipe is ordinary — `grep x | head -1` must not become an error. regex_fix_hint could name a fix that does not compile. `[)` opens a class and leaves a group unopened; naming `\[` sends the reader back with `\[)`, still broken. Rather than enumerate the combinations, the hint now applies its own spelling at the site the scan found and compiles the result, and offers nothing when that still fails. The scanner tracks byte offsets so the fix lands where the fault is, not at the first matching character. The audit that found these also corrected the reachability claim behind the exec_context panic. Builtin types are private, but ToolRegistry::get hands out an Arc<dyn Tool>, Tool::execute is public, and ToolCtx was implementable, so an embedder could dispatch a builtin with a context of its own and hit the panic where an exit code used to be. ToolCtx is now sealed. ExecContext is its only implementor, the branch is unreachable by construction, and the assertion is honest. The in-crate test double has to opt into the seal to exist, which is the seal written as code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
An agent ran a correctly quoted
grep -v '[cast:' lines.txtand read the failure as "no lines matched". The quoting was fine — the pattern reached the regex engine intact and failed to compile there, and grep reported that with exit 1, the code it also uses for a search that found nothing.cmpanddiffalready got this right. The rule now holds everywhere it applies, anddocs/LANGUAGE.mdstates it: a builtin that answers a question spends exit 1 on the negative answer and nothing else, so every error ingrep,test,cmp, anddiffexits 2. Where 1 is free the familiar split stands, socat missing.txtis unchanged. A computed pattern needed the same fix — the validator only sees literals, sop='[cast:'; grep "$p" ffailed further in — and a pipe read error no longer arrives as "no match".The split was missing a layer up too:
kaish -c 'if'exited 1 where--plan 'if'exited 2. Both are 2 now.--planalso never validated, so it green-lit programs the kernel refuses.The refusal names the fix rather than linking our regex crate:
\[for a literal[,[(]and[{]where a backslash would be a BRE operator. It compiles its own suggestion first, so a two-fault pattern gets no hint instead of a bad one.Finally, 98 builtins returned exit 1 with an internal message when handed a context that was not the kernel's.
ToolCtxis sealed now, making that branch unreachable so the builtins can assert.ToolRegistry::getandTool::executeare public, so type privacy alone had left it open.BREAKING (
kaish-tool-api): an out-of-treeToolCtximpl no longer compiles. Tool authors receive aToolCtxand never implement one, so no supported use changes.🤖 Generated with Claude Code