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

.changeset/approval-gate-fixes.md

@@ -0,0 +1,11 @@
+---
+'@openrouter/agent': patch
+---
+
+Fix two ways the tool-approval gate could be bypassed.
+
+**`allowFinalResponse` executed pending tool calls with no approval check.** When a `stopWhen` condition halted the loop on a turn that still carried tool calls, the final-response path ran those calls directly — skipping the approval gate the normal loop applies on every round. A tool marked `requireApproval: true` (or gated by a predicate) would execute unguarded, and because the `PermissionRequest` hook's deny bookkeeping lives inside the approval check, hook-based `deny` never fired on this path either. That path now runs the same check as the in-loop call sites, so the run pauses with `status: 'awaiting_approval'` and the gated calls on `pendingToolCalls` instead of executing them.
+
+**Function-based `requireApproval` received unvalidated arguments.** The predicate was called with the raw JSON-parsed tool arguments, while `execute` receives the arguments *after* the tool's Zod `inputSchema` runs. Any default, coercion, or transform in the schema made the two disagree — e.g. with `inputSchema: z.object({ dangerous: z.boolean().default(true) })`, a model emitting `{}` showed the predicate `dangerous: undefined` (no approval required) and then executed with `dangerous: true`. The predicate now sees the parsed value, so it decides on exactly what `execute` will receive. Arguments that don't satisfy the schema are not gated for engine-executed tools: such a call can never run (every execute path validates with the same schema first and turns the failure into a tool error output the model can recover from), so pausing for a human to approve it would only stall the run. Manual tools — which the host application executes without any engine-side validation — still fail closed on schema-invalid arguments.
+
+**Duplicate approval prompts for the same tool call.** The approval gate could run more than once over the same response — e.g. the pre-loop check plus the post-loop `allowFinalResponse` gate when a stop condition fired on the first iteration — re-emitting the `PermissionRequest` hook and re-running `requireApproval` predicates for calls that were already resolved. Each response is now gated at most once per run.

packages/agent/src/lib/conversation-state.ts

@@ -1,12 +1,18 @@
 import type * as models from '@openrouter/sdk/models';
+// Same zod entry point the executor validates through (see
+// `validateToolInput` in tool-executor.ts) so the approval predicate and
+// `execute` agree on parse semantics. Imported directly rather than reusing
+// that helper because tool-executor.ts imports this module — sharing it would
+// create an import cycle.
+import * as z4 from 'zod/v4';
 import type {
   ConversationState,
   ParsedToolCall,
   Tool,
   TurnContext,
   UnsentToolResult,
 } from './tool-types.js';
-import { isClientTool } from './tool-types.js';
+import { isAutoResolvableTool, isClientTool } from './tool-types.js';
 
 import { normalizeInputToArray } from './turn-context.js';
 
@@ -294,18 +300,46 @@ export async function toolRequiresApproval<TTools extends readonly Tool[]>(
   const requireApproval = tool.function.requireApproval;
 
   // If it's a function, call it with the tool's arguments and context.
-  // Arguments have already been parsed and validated against the tool's
-  // Zod inputSchema (a ZodObject), so the runtime shape is always a
-  // record here. A non-record value signals a real upstream bug — surface
-  // it rather than substituting an empty object.
+  //
+  // `toolCall.arguments` at this point is only the JSON-parsed wire payload
+  // (see extractToolCallsFromResponse) — it has NOT been validated against
+  // the tool's Zod inputSchema. The executor validates separately, right
+  // before calling `execute` (see validateToolInput in tool-executor.ts), so
+  // handing the raw payload to the predicate would let the two see different
+  // values whenever the schema applies a default, coercion, or transform
+  // (e.g. schema `{ dangerous: z.boolean().default(true) }` + model emits
+  // `{}`: the predicate sees `undefined` and waves the call through, then
+  // `execute` runs with `dangerous: true`).
+  //
+  // Parse with the same schema the executor uses so the predicate decides on
+  // exactly the values `execute` will receive.
   if (typeof requireApproval === 'function') {
-    const rawArgs: unknown = toolCall.arguments;
-    if (!isRecord(rawArgs)) {
-      throw new Error(
-        `toolCall.arguments for "${toolCall.name}" must be an object after Zod validation, got ${rawArgs === null ? 'null' : Array.isArray(rawArgs) ? 'array' : typeof rawArgs}`,
-      );
+    const parsed = z4.safeParse(tool.function.inputSchema, toolCall.arguments);
+    if (!parsed.success) {
+      if (isAutoResolvableTool(tool)) {
+        // Engine-executed tools validate before running: every execute path
+        // (regular, generator, HITL onToolCalled, unified run) runs the same
+        // schema through validateToolInput first and converts the failure
+        // into a tool error output the model can recover from. Arguments
+        // that don't satisfy the schema can never execute, so gating them
+        // would pause the run so a human can approve a call that can only
+        // fail (or throw outright when no state accessor is configured) —
+        // let them through the gate to the executor's validation error.
+        return false;
+      }
+      // Manual tools (no execute / onToolCalled / run) are surfaced to the
+      // host application via pendingToolCalls and executed WITHOUT any
+      // engine-side validation, so the "can never execute" argument does not
+      // hold — fail closed rather than wave a malformed call past the gate.
+      return true;
+    }
+    if (!isRecord(parsed.data)) {
+      // Valid per the schema but not an object — the predicate contract is
+      // Record<string, unknown>, so there is no trustworthy value to judge.
+      // Fail closed.
+      return true;
     }
-    return requireApproval(rawArgs, context);
+    return requireApproval(parsed.data, context);
   }
 
   // Otherwise treat as boolean

packages/agent/src/lib/model-result.ts

@@ -645,6 +645,14 @@ export class ModelResult<
   // normal tool round consults this to synthesize rejected outputs instead of
   // executing the calls.
   private readonly hookDeniedCalls = new Map<string, string>();
+  // The response most recently passed through handleApprovalCheck on this
+  // run. The same response object can reach the gate more than once — the
+  // pre-loop check plus the first loop iteration, or the pre-loop check plus
+  // the post-loop allowFinalResponse gate when a stop condition fires before
+  // any follow-up request. Re-gating would re-emit PermissionRequest hooks
+  // (duplicate prompts/audit records) and re-run requireApproval predicates
+  // for calls already resolved, so the gate runs at most once per response.
+  private lastApprovalGatedResponse: models.OpenResponsesResult | null = null;
   // Telemetry for the PostModelCall hook: the initial/resume request is
   // dispatched in initStream but its response is materialized later (stream
   // consumption), so the dispatch time and turn labeling are parked here
@@ -2840,6 +2848,17 @@ export class ModelResult<
       return false;
     }
 
+    // Each response is gated at most once per run (see the field doc). A
+    // repeat visit for the same response object means the calls were already
+    // partitioned, the hooks already fired, and any hook 'deny' results are
+    // already recorded in hookDeniedCalls — and it did not pause (a pause
+    // returns out of the run). Skipping is therefore both safe and required
+    // to avoid duplicate permission prompts.
+    if (currentResponse === this.lastApprovalGatedResponse) {
+      return false;
+    }
+    this.lastApprovalGatedResponse = currentResponse;
+
     const turnContext: TurnContext = {
       numberOfTurns: currentRound,
       // context is handled via contextStore, not on TurnContext
@@ -6132,6 +6151,26 @@ export class ModelResult<
           this.hasExecutableToolCalls(pendingToolCalls)
         ) {
           const turnNumber = currentRound + 1;
+
+          // Gate these calls exactly like a normal round would. This path
+          // executes real tools, so it needs the same approval check as the
+          // in-loop call sites above — without it, `stopWhen` firing on a turn
+          // that carries a `requireApproval` call would run that call
+          // unguarded, and hook-based 'deny' would never fire either (the
+          // deny bookkeeping lives inside handleApprovalCheck).
+          //
+          // On pause, handleApprovalCheck persists `pendingToolCalls` +
+          // status 'awaiting_approval', executes any auto-approved calls as
+          // unsent results, and sets `finalResponse` — so returning here is
+          // safe: nothing executed, so there is no round to record, and we
+          // must NOT fall through to markStateComplete() or the final
+          // text-coercion request. `sessionEndReason` stays 'max_turns' —
+          // accurate (the loop did stop on the stop condition) and consistent
+          // with the HITL pause return further down this same block.
+          if (await this.handleApprovalCheck(pendingToolCalls, turnNumber, currentResponse)) {
+            return;
+          }
+
           const turnContext: TurnContext = {
             numberOfTurns: turnNumber,
           };