fix(events): tool/before pass-through projects no decision instead of allow (#461) - #481
Merged
Merged
Conversation
…of allow A tool/before route (or config beforeTool hook) that returned outcome 'continue', no value, or only Agent.Context was delivered to Claude Code and Codex as hookSpecificOutput.permissionDecision 'allow', which skips the host permission prompt for every matching tool call. Pass-through now writes no decision (and no output without context or a rewrite); permission/request likewise answers only on an explicit decision. The route vocabulary gains explicit 'allow' and 'ask'. Fixes #461
🦋 Changeset detectedLatest commit: 8ce853f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
A pass-through result carries no decision, so a reason has no channel; the proximity warnings already reach the agent as Agent.Context.
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.
Fixes #461.
Severity
High / security-relevant. Any plugin with a
tool/beforeevent route (or a config-declaredbeforeToolhook) matchingBashturned every matching tool call in the session into an explicitly approved one on Claude Code and Codex —rm -rf,git push --force, anything — because a pass-through result was delivered aspermissionDecision: "allow", which per the Claude hooks contract skips the permission prompt. A plugin could not opt out.examples/host-test(which returns an empty<Agent.Result />for every family "so no host decision channel is touched") was itself auto-approving all tool calls on both hosts.Root cause
packages/agent-bundle/src/events/projection.ts:758(pre-fix) —permissionDecision: parsedValue?.outcome === 'deny' ? 'deny' : 'allow'. Every non-deny result, includingoutcome: 'continue', novalue, and context-only results, becameallow. BecausehookEventNamewas always written, the output object was never empty, so the wrapper always emitted ahookSpecificOutputwithpermissionDecision: "allow".packages/agent-bundle/src/adapters/hook-contract.ts:500(encodeNativeHookPlaygroundOutput) and:1248(the emitted legacybeforeToolwrapper'sencodeOutput) — the same ternary for config-declared hooks.projection.ts:655— the same class of defect onpermission/request:outcome: 'continue'projecteddecision.behavior: 'allow', answering the prompt on the user's behalf.website/docs/{en,zh}/guide/authoring/hooks.mdxdocumented the defect as the rule ("allowunless the route denied, even when the route renders no decision").The result vocabulary was
continue | deny, so "explicit allow" and "no opinion" were indistinguishable.Fix
continue, novalue, orAgent.Contextonly) projects no decision on every host. With no context and no rewrite the wrapper writes nothing and exits 0, so the host proceeds exactly as it would without the hook.outcome: 'allow'andoutcome: 'ask'(backward compatible; pass-through stays the default).allow/askaretool/beforedecisions;allowalso answerspermission/request. Every other family rejects them with aTypeErrorinstead of silently treating them ascontinue.reasonontool/beforenow requires a decision (no channel otherwise) — same rule the other families already enforce.updatedInputis independent of the decision. On Claude/Codex a rewrite withcontinuecarries nopermissionDecision; per the Claude docs the host "evaluates permission rules … against the input your hook returns". Pair withallowto auto-approve,askto show the rewritten input.permission: 'allow', updated_input, because Cursor documentsupdated_inputonly alongside apermission); explicitallow→{ permission: 'allow' };askfails closed because https://cursor.com/docs/hooks#pretooluse says it is "accepted by the schema but not enforced forpreToolUsetoday".beforeToolhooks:continue→ nopermissionDecision;denyunchanged. Their vocabulary is unchanged (continue | deny | stop); explicit allow/ask is an event-route feature. Documented.Per-host before / after (route result → host stdout)
<Agent.Result />or{ outcome: 'continue' }{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"allow"}}<Agent.Context>ctx</Agent.Context>only{"hookSpecificOutput":{"additionalContext":"ctx","hookEventName":"PreToolUse","permissionDecision":"allow"}}{"hookSpecificOutput":{"additionalContext":"ctx","hookEventName":"PreToolUse"}}{ outcome: 'continue', updatedInput }…"permissionDecision":"allow","updatedInput":{…}{"hookSpecificOutput":{"hookEventName":"PreToolUse","updatedInput":{…}}}{"permission":"allow","updated_input":{…}}{ outcome: 'allow', reason?, updatedInput? }…"permissionDecision":"allow","permissionDecisionReason":…,"updatedInput":…{"permission":"allow","updated_input"?:{…}}{ outcome: 'ask', reason?, updatedInput? }…"permissionDecision":"ask",…TypeError(fails closed; documented as unenforced){ outcome: 'deny', reason }permission/request{ outcome: 'continue' }/ empty{"hookSpecificOutput":{"decision":{"behavior":"allow"},…}}/ (none)permission/request{ outcome: 'allow' }{"hookSpecificOutput":{"decision":{"behavior":"allow"},"hookEventName":"PermissionRequest"}}Legacy config hook (
beforeToolhandler returning{ outcome: 'continue' }): Claude/Codex before{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"allow"}}, after (no output); withupdatedInput, after is{"hookSpecificOutput":{"hookEventName":"PreToolUse","updatedInput":{…}}}.Tests
tests/route-unit/event-project.test.ts— newprojects tool/before pass-through as no decision and only explicit allow/ask/deny as one (#461): renders through real Flight against the captured host-testPreToolUse/preToolUseenvelopes (Claude 2.1.257, Codex 0.147.0, Cursor 3.18.25 infixtures/host-lineage) and asserts the exact payload per host for pass-through, context-only, rewrite, allow, ask, deny, reason-without-decision, Cursor ask, and allow/ask on a non-tool family. Fails onorigin/main(expected { hookSpecificOutput: { …(2) } } to be undefined).permission/requestassertions updated:continue→ no decision,allow→behavior: allow,askrejected.tests/hooks.test.ts(emitted legacy wrappers, both codecs): continuingbeforeTool→ empty stdout, exit 0; rewrite →updatedInputwith nopermissionDecision; deny unchanged.tests/hook-playground-service.test.ts: playground native output for a continuing rewrite no longer carriespermissionDecision.examples/host-testwrappers:PreToolUseBashls→ empty stdout, exit 0 on Claude, Codex, and Cursor.pnpm typecheck✅,pnpm lint✅,pnpm test:unit✅ (3026),pnpm test:route-unit✅ (45),pnpm test:projection✅ (145),tests/hooks.test.tsvia the integration config ✅ (27),pnpm docs:site:build✅ (parity ok).Docs / provenance
website/docs/{en,zh}/guide/authoring/hooks.mdx: result vocabulary, pass-through rule,updatedInputsemantics per host, wire item 5 rewritten (it previously described the bug as the rule), legacybeforeToolnote.permissionDecision= normal flow), Codex pinned output schemapermissionDecisiondefaultnull, Cursoraskunenforced → fails closed. Two Cursor deferred-row reasons that said "the canonical deny/continue vocabulary does not express ask" were reworded to stay true.Changeset
agent-bundle: minor(pre-1.0 → breaking). Consumers that relied on the implicit approval must now returnoutcome: 'allow'explicitly, which is a changed default requiring consumer action per.changeset/README.md.Consumer impact (no repos edited)
src/events/tool/before.tsx→handleBeforeShell): returns{ outcome: 'continue' }for non-cargo commands,{ outcome: 'deny', reason }for blocked ones, and{ outcome: 'continue', updatedInput }for thecargo …→ hauler rewrite. After this fix, on Claude Code and Codex: every non-cargo Bash call that was silently auto-approved goes back through the host's normal permission prompt (the correct behavior — this was the bug), and the rewritten cargo command is evaluated by the host's permission rules instead of being auto-approved, so users may now see a prompt for the rewrittenhaulerinvocation. To keep auto-approving the rewrite, cargo-hauler can return{ outcome: 'allow', updatedInput }(oraskto show the rewritten command). Cursor behavior is unchanged. No cargo-hauler issue mentions permissions (gh issue list --repo ScriptedAlchemy/cargo-hauler --search permission→ only the closed test: parallelize the integration pool and stop rebuilding shared artifacts #26 about npm release), so nothing to comment there.src/events/session/start.tsx; not affected.tool/beforeinstead of approving every tool call.tool/beforeroute returned{ outcome: 'continue', reason }for proximity warnings, which the old projection delivered aspermissionDecision: "allow"+permissionDecisionReason— i.e. it too auto-approved every matching call. Since a pass-through result carries no decision,reasonhas no channel there and the projection now rejects it; the example now warns throughAgent.Contextonly (which it already did) and its route-unit tests assert{ outcome: 'continue' }. Caught byworktree-proximity-journeys.test.tson the first CI run.tool/before/beforeTool.Review status
@codex reviewrequests are posted from this PR by instruction.