feat(hook): a handler may select on the command, not only the tool name - #907
Draft
wenzowski wants to merge 1 commit into
Draft
feat(hook): a handler may select on the command, not only the tool name#907wenzowski wants to merge 1 commit into
wenzowski wants to merge 1 commit into
Conversation
`Handler::matcher` is a regex over the TOOL NAME, and for a shell tool the tool name is `Bash` on every call the agent makes. So a handler about a dozen commands and a handler about every command are indistinguishable to the dispatch: the only two options were fire on all of them or on none. Firing on all of them is not a neutral default. `matcher`'s own row records the measurement — an unnarrowed `pre-tool` handler cost 19.6ms p50 on a `Bash` call against a `wired` path whose whole p50 is 21ms — so "narrow by tool name only" prices a handler about a few commands as if it were about all of them, and pays it in a spawn per call. `command_matcher` is that second narrowing, and `selects_command` is where it decides. Ordered after `selects_tool` in `dispatch`, because a tool-name mismatch is the cheaper rejection and the commoner one. IT NAMES A SHAPE, NEVER A PROGRAM'S OWN VOCABULARY. The engine holds the matcher and the consumer's `batten.toml` holds the expression, which is non-negotiable rule 1: a list of git subcommands in `crates/batten` would be one consumer's vocabulary shipped to every adopter. What is generic is "select on what the call is going to RUN, not on what the host called the tool". COULD-NOT-LOOK READS THE OPPOSITE WAY FROM THE SIBLING COLUMN, and the asymmetry is the decision rather than an inconsistency. `selects_tool` runs on an unparseable matcher because the row was validated at load, so reaching that arm means validation was skipped and a participant in a fail-open contract should run. Here `None` means the ENVELOPE carried no command — every `Read` and every `Edit` in the session — and selecting on those would fire the handler on all of them, which is the unnarrowed cost this column exists to remove. A row declaring this column has said its subject is a command; an envelope with no command is not its subject. An unparseable EXPRESSION still selects, matching the sibling. The empty-to-absent conversion happens once, at the boundary. `Envelope::command` is a `String` spelling "no command" as `""`, and a row must be able to tell that from a command that is empty — an expression like `^$` would otherwise match every non-command call in the session. REFUSED AT LOAD, for the sibling's reason and one of its own: an uncompilable expression falls through to the fail-open arm and runs the handler on every call the tool matcher admitted, which for a `^Bash$` row is every shell command in the session, spawning a program per call to decide nothing. No server-segment probe here — CLOUD-178's trap is a label the HOST rotates per registration episode, and a command string is written by the agent, so there is no rotating name for an expression over it to be pinned to. The new suite asserts a NEGATIVE deliberately. A suite that only checked the advisory fires would stay green with the selector dropped, because a handler that fires on everything fires on the selected call too; what an over-firing row breaks is every call it should have been silent on. `spawned()` is a marker file rather than a reading of the output, because "did not fire" and "fired and said nothing" render identically on the advisory channel and are the two states this suite most needs to keep apart. Refs: CLOUD-1650 BREAKING CHANGE: `handler::dispatch` takes the mediated call's command as a new fourth argument, and `handler::Handler` gains a `command_matcher` field. A consumer constructing either must add it; the argument is `Option<&str>` and `None` reproduces the previous behaviour for every row that declares no selector.
|
Warning Review limit reachedNext included review available in 41 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 (5)
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 |
|
❌ The last analysis has failed. |
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.
Closes CLOUD-1650.
Why
A tree move mid-session leaves the mediator built from the old source, and
nothing re-checks until
doctorhappens to be invoked. CLOUD-1630 landed theverdict (
Mediator::BuildBehindSource,776a1be7); what was missing is atrigger. The row's §1 is explicit that this adds a trigger and not a second
reading.
The gap the mechanism had to close first
Handler::matcheris a regex over the tool name, and for a shell tool thetool name is
Bashon every call the agent makes. So a handler about a dozen gitsubcommands and a handler about every shell command are indistinguishable to the
dispatch — fire on all, or on none.
Firing on all is not a neutral default.
matcher's own row records themeasurement: an unnarrowed
pre-toolhandler cost 19.6ms p50 on aBashcallagainst a
wiredpath whose whole p50 is 21ms.What this changes
handler.rs—command_matcher, a declared regex over the mediated call'scommand, and
selects_commandwhere it decides. Ordered afterselects_toolin
dispatch, since a tool-name mismatch is the cheaper and commonerrejection. Refused at load: an uncompilable expression falls through to the
fail-open arm and would spawn a program per shell call to decide nothing.
lib.rs— the boundary convertsEnvelope::command's""toNoneonce,so a row can tell "no command" from "empty command";
^$would otherwise matchevery
Readin the session.tests/it/handler_dispatch.rs— four cases onpost-tool, plus two unitcases on the selector's readings.
schema/batten.schema.json— regenerated bymise run fix.Two decisions worth review
Could-not-look reads the opposite way from the sibling column.
selects_toolruns on an unparseable matcher, because the row was validated at load and
reaching that arm means validation was skipped. Here
Nonemeans the envelopecarried no command at all, and selecting on that fires the handler on every
ReadandEdit— the unnarrowed cost the column exists to remove. Anunparseable expression still selects, matching the sibling.
The suite asserts a negative deliberately. A suite that only checked the
advisory fires would stay green with the selector dropped, because a handler that
fires on everything fires on the selected call too. What an over-firing row
breaks is every call it should have been silent on, so that is what is asserted —
and
spawned()is a marker file rather than a reading of the output, because"did not fire" and "fired and said nothing" render identically on the advisory
channel.
Outstanding on this branch, before it is readied
The
[[hook.handler]] head-move-mediator-checkrow is not in this diff yet,so the column ships with nothing in this repository using it.
batten.tomlis aprotectedpath; the repository's declared route was taken —batten override requestput three preconditions, they are answered on the record, and admissiona6048b61...was issued — but spending that admission was refused by thesession's harness rather than by the repository. Until the row lands, the
mechanism is here and the trigger the ticket is actually about is not. The row
lands on this branch before the PR leaves draft.
Verification
mise run verifygreen and rebased onorigin/mainbefore ready;batten mutatereddens
head-move-unwatched, whose target case isa_git_call_that_moves_no_head_is_not_selected.Generated by Claude Code