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

.changeset/approval-gate-fixes.md

@@ -0,0 +1,34 @@
+---
+'@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.** Tool-level and call-level predicates were 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 them disagree — e.g. with `inputSchema: z.object({ dangerous: z.boolean().default(true) })`, a model emitting `{}` showed a predicate `dangerous: undefined` (no approval required) and then executed with `dangerous: true`. Predicates now see a parsed copy, so they decide on exactly what `execute` will receive without mutating the original executable call or parsing transformed output a second time. `PreToolUse` now runs before every auto-resolvable call is partitioned, so approval hooks and persisted pending calls see its effective arguments. Pending calls record an additive marker when preparation ran, preventing a resumed `ModelResult` from applying the hook twice while legacy state without the marker retains its prior behavior. Call-level checks remain unconditional and receive raw arguments when parsing fails; tool-level checks fail closed when schema parsing fails because a hook may later repair the input.
+
+**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 call occurrence in a response is now gated at most once per run, including responses containing duplicate call IDs and arguments.
+
+```ts
+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 the normalized default: { environment: 'production' }.
+// Persist this additive marker when PreToolUse already produced effective args.
+const pending: PendingToolCall<typeof deploy> = {
+  id: 'call_deploy',
+  name: 'deploy',
+  arguments: { environment: 'production' },
+  preToolUseApplied: true,
+};
+```

packages/agent/src/index.ts

@@ -262,6 +262,7 @@ export type {
   ParsedToolCall,
   PartialResponse,
   PendingAsyncTool,
+  PendingToolCall,
   ResponseStreamEvent,
   ResponseStreamEvent as EnhancedResponseStreamEvent,
   ServerTool,

packages/agent/src/lib/async-params.ts

@@ -67,7 +67,7 @@ type BaseCallModelInput<
   context?: ContextInput<ToolContextMapWithShared<TTools, TShared>>;
   /**
    * Call-level approval check - overrides tool-level requireApproval setting
-   * Receives the tool call and turn context, can be sync or async
+   * Receives normalized arguments when schema parsing succeeds and raw arguments otherwise
    */
   requireApproval?: (
     toolCall: ParsedToolCall<TTools[number]>,

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

@@ -1,4 +1,10 @@
 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,
@@ -260,7 +266,7 @@ export function appendToMessages(
  * @param toolCall - The tool call to check
  * @param tools - Available tools
  * @param context - Turn context for the approval check
- * @param callLevelCheck - Optional call-level approval function (overrides tool-level), can be async
+ * @param callLevelCheck - Optional call-level approval function (overrides tool-level), can be async. Receives normalized arguments when schema parsing succeeds and raw arguments otherwise.
  */
 export async function toolRequiresApproval<TTools extends readonly Tool[]>(
   toolCall: ParsedToolCall<TTools[number]>,
@@ -271,12 +277,6 @@ export async function toolRequiresApproval<TTools extends readonly Tool[]>(
     context: TurnContext,
   ) => boolean | Promise<boolean>,
 ): Promise<boolean> {
-  // Call-level check takes precedence
-  if (callLevelCheck) {
-    return callLevelCheck(toolCall, context);
-  }
-
-  // Fall back to tool-level setting (server tools never require approval)
   const tool = tools.find(
     (
       t,
@@ -287,25 +287,63 @@ export async function toolRequiresApproval<TTools extends readonly Tool[]>(
       }
     > => isClientTool(t) && t.function.name === toolCall.name,
   );
+  // Call-level checks always take precedence. Pass a normalized copy when
+  // parsing succeeds, or the raw call when it does not.
+  if (callLevelCheck) {
+    if (!tool) {
+      return callLevelCheck(toolCall, context);
+    }
+
+    const parsed = z4.safeParse(tool.function.inputSchema, toolCall.arguments);
+    if (!parsed.success) {
+      return callLevelCheck(toolCall, context);
+    }
+
+    return callLevelCheck(
+      {
+        ...toolCall,
+        arguments: parsed.data,
+      } as ParsedToolCall<TTools[number]>,
+      context,
+    );
+  }
+
+  // Fall back to the tool-level setting (server tools never require approval).
   if (!tool) {
     return false;
   }
 
   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) {
+      // There is no trustworthy value to pass to the predicate. Fail closed:
+      // a PreToolUse hook may later replace invalid input with executable
+      // input, so schema-invalid wire arguments cannot safely bypass approval.
+      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/hooks-schemas.ts

@@ -75,13 +75,9 @@ export const PostToolUseFailurePayloadSchema = z4.object({
 });
 
 /**
- * Fired when a tool EXECUTION throws or returns an error.
- *
- * Deliberately NOT fired when a tool never ran: a PermissionRequest 'deny',
- * a user rejection on approval resume, or a PreToolUse block all synthesize
- * a rejected result without execution, so no failure event is emitted.
- * Observe those outcomes via the PermissionRequest / PreToolUse hooks
- * themselves.
+ * Fired when an entered tool lifecycle fails after PreToolUse, including a
+ * mutated-input approval denial/rejection or schema-invalid mutation.
+ * Initial approval denials and rejections do not enter the lifecycle.
  */
 export type PostToolUseFailurePayload = Readonly<z4.infer<typeof PostToolUseFailurePayloadSchema>>;