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

API example

import { z } from 'zod/v4';
import { tool, type PendingToolCall } from '@openrouter/agent';

const deploy = tool({
  name: 'deploy',
  inputSchema: z.object({
    environment: z.enum(['staging', 'production']).default('production'),
  }),
  requireApproval: ({ environment }) => environment === 'production',
  execute: async ({ environment }) => deployEnvironment(environment),
});

// `requireApproval` sees normalized schema output:
// { environment: 'production' }.
const pending: PendingToolCall<typeof deploy> = {
  id: 'call_deploy',
  name: 'deploy',
  arguments: { environment: 'production' },
  // Persist this additive marker when PreToolUse already prepared the call.
  preToolUseApplied: true,
};