fix(hook): refuse a call whose declaration nothing could read - #934
Draft
wenzowski wants to merge 2 commits into
Draft
fix(hook): refuse a call whose declaration nothing could read#934wenzowski wants to merge 2 commits into
wenzowski wants to merge 2 commits into
Conversation
…ing non-zero `load_policy` failed with `?`, which raises a `UsageError` — exit `1` — and `exit.rs` makes only `2` a denial precisely so no failure path can block a call. The harness therefore read a config this build could not load as a non-blocking hook error and ran the mediated tool anyway. Measured over one 5-day session: 1,149 calls proceeded unjudged through seven windows of a mid-edit `batten.toml`, and ~456 more through a preset the installed build did not ship — `policy.rs`'s unknown-preset arm raises exactly this error. The discrimination is CLOUD-1572's, one level up. Where the engine is guessing about the call — unreadable stdin, an undecodable payload, an event the host does not declare — allowing is right, because nothing is known. Here the engine has read its own authority and been told it cannot enforce it, so proceeding reports a clean allow over rules that never ran. It renders rather than propagates, because `render` owns the per-harness deny channel: Claude Code answers in its JSON decision object at exit `0`, where the document is the deny, and the neutral adapter answers `Violation`. A `Denial` raised here would send `2` to a host that reads the document instead. The bypass is honoured first, which is what keeps a container recoverable: a stale binary meeting a newer config denies every call until one of them moves. Refs: CLOUD-1688
…an outage Four cases over the compiled binary, because the defect is not in `adjudicate` — which is pure and whose unit cases passed throughout — but in what the boundary does with a load that failed. `mediated_admission.rs` records the same lesson from the other side. The pairing is the point. Under the declared mutation `unloadable-config-allows`, which restores the old fall-through, the two deny cases redden and the two allow cases stay green: FAIL a_config_this_build_cannot_load_denies_rather_than_failing_open FAIL on_claude_code_the_refusal_is_the_document_rather_than_the_number PASS a_loadable_config_still_allows_an_ordinary_call PASS the_declared_hatch_still_reaches_a_clone_whose_config_will_not_load Proved by hand rather than left to the nightly. The mirror is what stops the change being satisfied by an adjudicator that denies every call in the fleet, which is an outage wearing a fix's clothes; the hatch case is what keeps a container recoverable when a stale binary meets a newer config. The fixture is a `batten.toml` mid-edit, which is the largest measured bucket: seven windows across one 5-day session, 1,149 calls, none of them judged. Refs: CLOUD-1688
|
Warning Review limit reachedNext included review available in 7 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
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.
The defect
run_hookloaded the config with?. A failure raises aUsageError— exit1— andexit.rsmakes only2a denial so that no failure path can block a call. A harness reads1as a non-blocking hook error, so the gate neither allowed nor denied: it was absent, and the mediated tool ran.Measured over one 5-day session (CLOUD-1677): 1,149 calls proceeded unjudged through seven windows of a mid-edit
batten.toml— which is itself a protected path, so the gate guarding the config stopped guarding it during the one operation that changes it.Why this refuses one fault and not three
A first pass refused on every load failure. Eleven existing cases across three suites went red, two of them named
hook_fails_open_and_loud_…, and that was the tree objecting correctly.Gates are registered fail-open, and a gate that fails open is INERT — it does not allow and does not deny, it is absent. So a config fault is never a choice between refusing and allowing. It is a choice between keeping the enforcement surface we still have and losing it entirely.
An unknown key costs its own row (CLOUD-1428). A table whose validator refuses names that table. A version this build is too old for still says so. Each leaves every other row readable and enforceable, and leaves an agent that can still be told to repair the broken one. Refusing there trades a working partial gate for nothing, and takes with it the surface that would have carried the repair instruction.
A file that is not TOML has no partial function to preserve: zero rows readable, nothing enforced. Then the refusal is the only signal left, and the declared hatch is the recovery path.
Native::ConfigUnreadableThat argument is not a PR note. It lives on the
Native::ConfigUnreadabledoc, theconfig_errorarm and therun_hookcall site, because a decision recorded only in a review is one nobody inherits.What the gate does NOT see, stated rather than left green
Rule 2: a green gate says "nothing it can SEE is wrong", never "nothing is wrong". One case escapes this one.
A batten-governed repo whose
batten.tomlhas been deleted.unreadable_declarationkeys onconfig_error's syntax arm, which only fires when a file exists and will not parse. An absent authority never reaches it, so nothing is enforced and the call proceeds — the same "no partial function to preserve" condition as a file that is not TOML.It is owned rather than unowned: CLOUD-1688 extends this row "from its invalid-config arm to skew and absence". Naming it here so "green" is read as the file-exists case is covered, not as unreadability is covered.
Three things the shape is forced by
min_batten_versionrefusals carry none either and both leave the file readable. Keying on absence would refuse those, and would silently widen what denies with every future unclassed error.toml::de::Erroris one type for two unlike faults and renders both asTOML parse error at line N— measured on the[[fact]]-with-no-returnsfixture, a schema fault the message alone classed as unreadable. Atoml::Tableparse answers it exactly and costs nothing until a parse has already failed, which is the probeparse_ungatedrecords as removed for costing one on the hot path.renderalready denies through each host's own channel, so its return says which one carried it: a document harness getspermissionDecision:"deny"and a3that honestly says nothing was judged; the neutral adapter's only channel is the number, so it keeps2.exit.rs's guarantee —UsageandInternalare the only codes a Batten failure produces — is never spent, andno_failure_code_can_deny_a_mediated_callstays green.Also: the hatch is read before the refusal, so a stale binary meeting a newer config leaves a container recoverable rather than bricked.
Fix::None, because the repair is rebuild/reinstall/fix-the-config and each is the consumer's own command (rule 1). AndNative::ConfigUnreadableis appended, not grouped — the enum carries norepr, so placing it beside its siblings moved eighteen discriminants andsemver checkread the tail as broken; the reason is written onto the variant.Shown able to fail
crates/batten/tests/it/adjudicate_absent.rs, over the compiled binary — the defect is not inadjudicate, which is pure and whose unit cases passed throughout, but in what the boundary does with a load that failed.Under the declared mutation
unloadable-config-allows:Proved by hand rather than left to the nightly. The mirror is the half that matters most: without it this is satisfied by an adjudicator that denies every call in the fleet, which is an outage wearing a fix's clothes.
The rule-4 case earned its place. The un-truncated
tomlerror renders a multi-line span including the offending source line, so the first draft put bytes of the unreadable config into a refusal that reaches the model, the host log and the transcript. Only the first line — the pointer — is carried now.The nine assertions the first pass moved are reverted to their originals; only two genuine syntax sites change.
cli.rs's loop now asserts what it can prove for all six harnesses — that none fails open — with the per-protocol channel detail left where it can be stated exactly, rather than testing Claude Code's spelling five times and calling it coverage.Closes CLOUD-1677
DO-NOT-CLOSE CLOUD-1688
That row extends this one to skew and absence, and its other parts are untouched here. Three findings from this session belong to it:
doctorruns the build mise resolves via_.path; the hook runs~/.local/bin. A self-check indoctorwould faithfully interrogate the wrong binary and report green, so part 4 has to spawn the installed artifact — a declared-effect change under house-style §5.install:localis what repaired this container three times while landing this PR; removing it without a replacement would have made the session unrecoverable, which is the row's own bootstrap concern made concrete.