fix(agent): enforce approval gate on allowFinalResponse path and validate predicate args (#54)

[LukasParke]

Summary

Two independent ways the tool-approval gate could be bypassed, letting a tool the user was supposed to approve execute unguarded.

Bug 1 — allowFinalResponse skipped the approval gate entirely

When a stopWhen condition halted the loop on a turn that still carried tool calls, the final-response path called executeToolRound(pendingToolCalls, turnContext) directly, with no handleApprovalCheck — unlike the two in-loop call sites, which gate every round.

Consequences:

• A tool marked requireApproval: true (or gated by a predicate) executed without approval.
• Hook-based deny never fired on this path either: hookDeniedCalls is only populated inside handleApprovalCheck, and neither executeToolRound nor executeSingleToolCall partitions on approval internally. So a PermissionRequest hook returning deny was silently ignored.

This is reachable in ordinary use — any run with a stopWhen (including the default step limit) that halts on a turn carrying a gated call.

Bug 2 — the approval predicate saw different arguments than execute

A function-based requireApproval was invoked with toolCall.arguments, which at that point is only JSON.parsed (see extractToolCallsFromResponse). execute receives the arguments after validateToolInput (z4.parse) runs, which applies the schema's defaults, coercions, and transforms — so the two disagreed whenever the schema does any of those.

Concretely, with inputSchema: z.object({ dangerous: z.boolean().default(true) }) and a model emitting {}:

• predicate saw dangerous: undefined → no approval required
• execute then ran with dangerous: true

The predicate was deciding on values that were never the ones used. A stale comment in conversation-state.ts asserted the arguments were "already parsed and validated against the tool's Zod inputSchema" — that was false, and is corrected here.

The fix

• model-result.ts: added if (await this.handleApprovalCheck(pendingToolCalls, turnNumber, currentResponse)) { return; } before the final-response executeToolRound, mirroring the in-loop call sites. On pause, handleApprovalCheck already persists pendingToolCalls + status: 'awaiting_approval', records auto-approved calls as unsent results, and sets finalResponse, so the early return is consistent: nothing executed, so there is no round to record, and it correctly skips both markStateComplete() and the final text-coercion request. sessionEndReason stays 'max_turns', matching the sibling HITL pause return in the same block.
• model-result.ts (review follow-up): handleApprovalCheck now gates each response at most once per run. The same response object could otherwise be checked twice — the pre-loop gate plus the post-loop allowFinalResponse gate when a stop condition fires on the first loop iteration (and, pre-existing, the pre-loop gate plus the first in-loop iteration) — re-emitting PermissionRequest hooks (duplicate prompts/audit records) and re-running predicates for calls already resolved. A repeat visit means the first pass already partitioned the calls, fired the hooks, and recorded any hook deny in hookDeniedCalls, so skipping is safe.
• conversation-state.ts: the predicate's arguments are now z4.safeParsed against the tool's inputSchema — the same zod entry point validateToolInput uses — so the predicate sees exactly what execute will receive. Zod is imported directly rather than reusing validateToolInput because tool-executor.ts imports conversation-state.ts; sharing the helper would create an import cycle. The false comment is replaced with one explaining the actual invariant.
• conversation-state.ts (review follow-up): schema-invalid arguments are not gated for engine-executed tools. Such a call can never execute — every execute path (regular, generator, HITL onToolCalled, unified run) runs the same schema through validateToolInput and converts the failure into a tool error output the model can recover from — so requiring approval would pause the run for a human to approve a call that can only fail, or throw outright when no state accessor is configured. Fail-closed is kept in two cases: parses that succeed with a non-record payload (which would break the predicate's Record<string, unknown> contract), and manual tools (no execute / onToolCalled / run), which the host application executes without any engine-side validation — the fail-open is gated on isAutoResolvableTool.

Test coverage

packages/agent/tests/unit/approval-gate-regressions.test.ts (14 tests). The regression tests were verified red against unmodified code before implementing, then green after — confirmed by stashing the fixes and re-running.

• predicate sees schema defaults ({} → { dangerous: true }, approval required)
• predicate sees schema coercions ({ amount: '500' } → { amount: 500 }, so > 100 compares numerically rather than lexicographically)
• schema-invalid arguments are not gated for engine-executed tools and the predicate is not called at all (they fall through to the executor's validation error)
• still fails closed when the schema parses to a non-object value (predicate contract is Record<string, unknown>)
• still fails closed on schema-invalid arguments for manual (caller-executed) tools, which receive no engine-side validation
• allowFinalResponse gate: stepCountIs(1) firing on a turn carrying a requireApproval call — asserts the tool does not execute, the run pauses with awaiting_approval, the gated call is on pendingToolCalls, requiresApproval() is true, and no final text-coercion request is made. Structured so the first round completes with an ungated tool, ensuring the break lands on the post-loop path rather than being caught by the pre-loop gate.
• control: non-gated tools still execute normally on the allowFinalResponse path and the final response is still produced (guards against over-blocking).
• a PermissionRequest hook returning deny is honored on the allowFinalResponse path: the denied tool does not execute, the run does not pause, and the rejection is recorded as a function_call_output carrying the hook's reason.
• schema-invalid arguments surface as a tool error end-to-end: a full run through the mocked transport completes with status: 'complete', the tool body never executes, and the validation failure is recorded as the call's output (also covers the no-state-accessor throw).
• a response is gated only once when the stop condition fires on the first iteration: a PermissionRequest hook returning allow is emitted exactly once for a gated call on the initial response, and the tool executes exactly once on the allowFinalResponse path.
• invariant lock: one test per engine execute path (regular, generator, HITL onToolCalled, unified run) asserting schema-invalid calls never reach the tool body — the invariant the gate's fail-open relies on (mutation-checked).

Per this repo's practice, I re-audited consumers after the contract change: both executeToolRound call sites are now gated, and partitionToolCalls / toolRequiresApproval have no other non-test callers.

Verification

pnpm turbo run build typecheck lint test --filter=@openrouter/agent — all 4 tasks pass. Full unit suite: 843 tests / 68 files passing, no type errors.

Judgment call

The call-level requireApproval override (options.requireApproval) was left as-is. It receives the whole ParsedToolCall, not just the arguments — a deliberately different public contract from the tool-level predicate — and normalizing its arguments would change a published signature's semantics. Worth a follow-up decision, but out of scope for a patch fix; flagging it rather than changing it silently.

Fixes #54

🤖 Generated with Claude Code

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 3 additional findings in Devin Review.

[devin-ai-integration]

🟨 Approval gate is skipped for engine-executed tools whose arguments fail schema validation

When a tool's function-based requireApproval predicate is used and the model's arguments fail the tool's Zod inputSchema, the gate now returns false (no approval required) for any engine-executed tool (packages/agent/src/lib/conversation-state.ts:317-329). This is a deliberate fail-open that relies on every engine execute path re-validating with the same schema before running the tool body. The security property therefore depends on an invariant enforced only by convention and tests (validateToolInput in packages/agent/src/lib/tool-executor.ts:142-144 being called by every execute path). If any current or future execution path (custom executors, MCP wrappers, hooks that receive raw arguments such as PreToolUse/PermissionRequest) acts on an unvalidated call, an attacker-influenced model could emit deliberately schema-invalid arguments to bypass the human approval gate entirely.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[LukasParke]

This is a deliberate, documented trade-off, and I want to lay out why the residual risk is smaller than it looks — but it's a judgment call and easy to flip if the maintainers prefer.

What an attacker-influenced model actually gains from emitting schema-invalid arguments: nothing executable. The dangerous call — valid arguments with dangerous values — is still gated exactly as before (the predicate now sees the post-parse values, which is the original Bug 2 fix). A schema-invalid call can only ever produce a validation-error output on every engine path, and that property is now test-locked per execute path (631edb2, mutation-checked). So the bypass buys the model a skipped pause for calls that error out anyway; it cannot get unvalidated arguments executed.

On the specific paths named:

• MCP tools are wrapped as regular client tools and execute through executeRegularTool / prepareUnifiedInvocation, which validate (tool-executor.ts:265, :597) — covered by the invariant tests.
• PreToolUse / PermissionRequest hooks receive the raw ParsedToolCall by design in every flow — gated or not, before and after this PR. The fail-open doesn't hand hooks anything they didn't already receive for ordinary ungated calls; a hook that acts dangerously on raw arguments is a pre-existing hook-authoring concern, orthogonal to the gate.
• Custom executors: the package's executor functions are the only execute paths; the invariant tests now fail CI if any of them stops validating.

Why not fail closed: that was the original implementation here, and it was itself flagged in review — it converts recoverable malformed model output into a run that pauses for a human to approve a call that can only fail, or throws outright when no state accessor is configured. Failing open for engine-executed tools preserves the model's ability to see the validation error and retry, which matches how the executor treated these calls before this PR.

If the maintainers would rather have the conservative stance, it's a one-line flip (return false → return true in the isAutoResolvableTool branch) and the tests pin whichever behavior is chosen. Flagging for a human decision rather than silently churning the security posture back and forth between bot reviews.

[perry-the-pr-reviewer]

Perry's Review

Verdict: 💬 Comments / questions

Risk: 🟢 Low

Note: Perry would approve this PR, but the maintainer app lacks pull_requests:write on OpenRouterTeam. Posting as a comment instead — a human reviewer can approve.

Full review

Summary

This PR fixes two ways the tool-approval gate could be bypassed, plus prevents duplicate permission prompts. I've done a full deep review: read both changed source files end-to-end, traced every handleApprovalCheck call site (pre-loop, in-loop, post-loop), verified the invariant the fail-open relies on against all four execute paths, and checked the blast radius of all changed exports.

Both fixes are correct, well-documented, and comprehensively tested (14 tests including mutation-checked invariant locks). The PR has been iteratively refined through prior review rounds (Perry on the first SHA, Devin across three subsequent SHAs), and all previous findings are addressed in the current head.

Bug 1 — allowFinalResponse approval gate

The post-loop allowFinalResponse path now calls handleApprovalCheck before executeToolRound, mirroring the two in-loop call sites. On pause, handleApprovalCheck persists pendingToolCalls + status: 'awaiting_approval', records auto-approved results as unsent, and sets finalResponse — so the early return is correct: nothing executed, no round to record, no final text-coercion request. sessionEndReason stays 'max_turns', consistent with the sibling HITL pause return in the same block.

The dedup guard (lastApprovalGatedResponse) prevents the same response from being gated twice — the pre-loop gate plus the first in-loop iteration, or the pre-loop gate plus the post-loop gate when stepCountIs(0) stops on the first iteration. Reference equality (===) is correct here: each API response is a distinct object, and ModelResult instances are single-use (created fresh per callModel() call, never reused). The guard is checked and set without an intervening await, so no race is possible in the single-threaded event loop.

Bug 2 — predicate argument parity

toolRequiresApproval now z4.safeParses arguments against the tool's inputSchema before invoking the predicate, so the predicate sees exactly what execute will receive (defaults, coercions, transforms applied). The z4 import matches the executor's entry point (validateToolInput uses z4.parse from the same zod/v4), and the direct import avoids the import cycle (tool-executor.ts → conversation-state.ts).

The fail-open for schema-invalid arguments on engine-executed tools is sound: every execute path (executeRegularTool, executeGeneratorTool, executeHITLTool, prepareUnifiedInvocation) runs validateToolInput before the tool body, so schema-invalid calls produce a tool error the model can recover from — gating them would only pause for a human to approve a call that can only fail. This invariant is locked in with four mutation tests (one per execute path). Manual tools correctly fail-closed since the host application executes them without engine-side validation.

One suggestion

See the inline comment on conversation-state.ts — the callLevelCheck path still receives unvalidated arguments (the same class of bug just fixed for the tool-level predicate). The PR description explicitly scopes this out, but a code comment would prevent future confusion now that the tool-level path below is documented to parse arguments.

Risk assessment:

Dimension	Severity	Risk	Reasoning
Implementation risk	🟩	Low	Minimal, well-targeted changes; 14 tests cover both bugs + edge cases + invariant lock
Premise risk	🟩	Low	Both bugs verified red-then-green; fixes address root causes, not symptoms
Estimated impact	🟩	Low	Fixes a security vulnerability; if wrong, worst case is approval gate not engaging in an edge case — but the fix is correct

Risk Factor	Severity	Risk	Reasoning
Reversibility	🟩	Low	Fully reversible — standard revert restores prior behavior
Detectability	🟩	Low	Regression tests lock both fixes and the invariant they depend on
Blast radius	🟩	Low	Only affects runs with requireApproval or stopWhen + allowFinalResponse
Data integrity	🟩	None	No persisted state is touched beyond the normal approval pause flow
Financial exposure	🟩	None	No billing/payment code is affected
Security and privacy exposure	🟩	Low	Fix tightens the approval gate — the security posture only improves
Propagation	🟩	Low	Changed exports (toolRequiresApproval, partitionToolCalls) have no non-test callers outside model-result.ts and re-exports
Availability	🟩	None	The change does not affect whether anything serves
Recovery cost	🟩	Low	A revert fully restores prior behavior
Time to correct	🟩	Low	Any issue would be caught by the existing test suite

[perry-the-pr-reviewer]

▶ Prompt for agents: The tool-level predicate now correctly parses arguments through inputSchema here. Worth noting in a comment at the callLevelCheck branch above (line ~280, outside this diff) that it still receives the raw JSON-parsed toolCall.arguments — not schema-validated — unlike this path. The PR description explicitly scopes normalizing that out, but a brief note there would prevent a future reader from assuming both paths parse arguments the same way.