feat(gate): argument-conditional approval cards (gate.ask_when) - #296
feat(gate): argument-conditional approval cards (gate.ask_when)#296CrazyWillBear wants to merge 2 commits into
Conversation
One tool can be two actions: hound's smart_fetch reads a page, but the same call carrying `actions` clicks and submits on it. The gate could only decide on a tool name, so the owner had to either approve every page read or accept that approving fetch also approved clicking. `gate.ask_when` maps a tool to argument names that pull it back into the card path however it was approved — it outranks `"*"`, an explicit approve, and the read-only fast path; `gate.never` still wins. Matching is on presence, not value. "always" stays available on these cards like any other, but persists `tool:argument` rather than the bare name, so one tap never approves the tool's other watched arguments. Splits GatePolicy and the approved-set persistence into gate_policy.py — gate.py was at the 200-line cap, and the decision rules and the enforcement path are separate concerns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EP49QtT1zxHvm1PPCmCpUz
|
Blocked on #297 — do not merge yet. Review found two highs:
Also outstanding from the review: |
Review high-1 on #296, owner-approved. `decide` checked `pending_arguments` (which returns () once every carried watched argument holds a grant), then fell through to the bare-name check. So a composite grant only ever lifted the ask_when veto — it never approved. For a tool not independently in `approved` / `"*"` / `read_only`, answering "always" to an ask_when card was a no-op that re-carded forever: p = GatePolicy(approved={'peek:actions'}, ask_when={'peek': ('actions',)}) p.pending_arguments('peek', {'actions': []}) # () p.decide('peek', {'actions': []}) # was ASK, now APPROVED A call carrying a watched argument is now decided by its grants alone, before the bare-name branch: all granted → APPROVED, any pending → ASK. The bare name is neither required nor sufficient there, so the grant stays narrow — it never approves a call without that argument, and never covers a sibling argument. Both existing policy tests seeded the bare name alongside the grant, which is why this slipped through; the two new tests do not. Docs the review also flagged: - SECURITY.md documented `decide(name)` with no ask_when step at all. Rewritten with the real order, the read_only override, and why grants are checked first. - CONFIG.md never said that presence-only matching makes one "always" cover every future value of the argument, nor that the argument name is unvalidated against the tool's schema (a typo silently disables the rule). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EP49QtT1zxHvm1PPCmCpUz
Both denial texts contradicted the PR's own argument, in opposite directions. card_denied said "do not retry, do not re-ask" absolutely, which erases the distinction the change exists to draw: the body argues a card decline is a decision the owner can revisit, and the string forbade the retry revisiting needs. The model tells the owner, the owner says go ahead, and the model holds a standing instruction not to. Scoped to "on your own", with the owner's go-ahead named as the exception. never_denied claimed "no approval can lift it", which is false here — gate.never is config, and self-edit/SKILL.md teaches the model to edit gate.never/gate.approved and restart as routine. So the single highest-leverage route around a never-list denial was the one workaround the string did not name. It now says what is true (only the owner, in config.yaml) and forbids that route explicitly. Not an enforcement hole (editing config still cards), but a false absolute is worse than a scoped truth in a string whose whole job is to be believed. The perseveration breaker (agent/loop.py) fires on exactly this path and said "take a different approach" — the workaround both denials forbid, from a second string the model reads at the same moment. Reworded to respect a refusal while keeping the advice for the ordinary repeated-failure case. Lows: SECURITY.md documents the two texts and that they are behavioural, not enforcement; cron/tools.py dropped the same "the owner declined" overclaim on the sibling path. Rationale had been restated in four places — module docstring is now the single home. Left: the split vs #296 (gate_policy.py) — orthogonal concerns out of the same overfull file, mechanical rebase whichever lands second.
Why
One tool can be two actions. hound's
smart_fetchreads a page, but the same call carryingactionsclicks, fills, and submits on it — and hound ships no read-only variant and no flag to disable it.GatePolicy.decide(tool_name)only ever saw the name, so the owner's options were: leave the tool offgate.approvedand approve every page read by hand, or accept that approving fetch also approves clicking. Neither is what they wanted.What
gate.ask_whenmaps a tool to argument names that pull it back into the card path however it was approved:"*", an explicit approve, and theread_onlyfast path.gate.neverstill wins.actions.mcp_hound_smart_fetch:actionsrather than the bare tool name, so one tap grants the tool+argument the rule named and never the tool's other watched arguments.A call carrying a watched argument is decided by its grants alone — the bare tool name is neither required nor sufficient. So a
tool:argumentgrant genuinely approves that call on its own, while a tool nothing else approves still cards for every call not carrying a granted argument.The two approved-set stores are unchanged: config.yaml is declared intent,
gate_approved.jsonaccretes taps, unioned at boot. An argument-scoped grant is just another entry in the same file, so a tap still never rewrites config.Two consequences of presence-matching, both now documented in
docs/CONFIG.md: one "always" covers every future value of that argument (the gate does not read what's inside), and the argument name is not validated against the tool's schema — a typo silently leaves the tool on whatever its ordinary listing says.Structure
GatePolicy+load_approved/save_approvedmove togate_policy.py.gate.pywas at the 200-line hard cap, and the decision rules and the enforcement path (cards, announce, audit) are separate concerns. One drive-by: a 5-line import inwiring.pycollapsed to one line (86 chars) to stay under the cap without an escape-hatch comment.Review fixes (commit 2)
high-1 — a composite grant approved nothing.
decidecheckedpending_arguments(which empties once every carried watched argument holds a grant) and then fell through to the bare-name check, so a grant only ever lifted theask_whenveto. For a tool not independently inapproved/"*"/read_only, "always" was a no-op that re-carded forever:Grants are now checked before the bare name. Both pre-existing policy tests seeded the bare name alongside the grant, which is why this slipped through; the two new tests do not.
Docs the review also flagged:
SECURITY.mddocumenteddecide(name)with noask_whenstep at all (rewritten with the real order and theread_onlyoverride), andCONFIG.mdwas missing the presence-matching consequences above.Tests
Precedence matrix in
tests/test_gate.py— watched argument vs."*", vs. an explicit approve, vs.read_only, vs.never— plus what "always" persists on each card type, that a grant covers only the argument it names, and that a grant approves without the bare name.Done-check:
799 passed,ruff checkclean,mypyclean.Still open from the review
grant_key's:namespace is unenforced against remote-supplied MCP tool names.ask_whenargument disables the rule with nothing logged — now documented, not yet detected.ask_whencards a subagent call. It needs no new code (subagents route through the sameGatedTools), but it's the criterion Subagent tool calls bypass the gate entirely (blocks #296) #297 could not cover on its own.Follow-up (not in this PR)
chief-packages
feat/houndcurrently hard-refusesactionsvia a stdio shim — the stopgap this replaces. It should only be stripped once this is merged and installed, and a card is verified firing against a realsmart_fetch; removing it earlier leavesactionsungated on any box running current core.🤖 Generated with Claude Code
https://claude.ai/code/session_01EP49QtT1zxHvm1PPCmCpUz