From 8e6e58692b1b4b1582dda038a25bf0d4cd6da1ff Mon Sep 17 00:00:00 2001 From: Ganni Galea Curmi Date: Thu, 25 Jun 2026 22:22:21 -0400 Subject: [PATCH] fix(agent): enforce approval gate on allowFinalResponse path and normalize approval-predicate args The allowFinalResponse path previously executed the pending tool calls left over when stopWhen halted the loop without first running the approval partitioning the main loop applies to every other round. It now runs the normal approval check before executing those pending calls, so an approval-gated tool can no longer run unprompted on the final-response turn. Separately, function-form requireApproval predicates were evaluated against the raw model-produced arguments while the tool's execute() received the Zod-parsed input (defaults applied, values coerced). The predicate now sees the schema-normalized arguments, so the approval decision matches the input the tool actually executes with. --- .changeset/approval-gate-final-response.md | 5 + packages/agent/src/lib/conversation-state.ts | 47 ++++++-- packages/agent/src/lib/model-result.ts | 4 + .../tests/unit/allow-final-response.test.ts | 55 ++++++++++ .../tests/unit/conversation-state.test.ts | 103 ++++++++++++++++++ 5 files changed, 204 insertions(+), 10 deletions(-) create mode 100644 .changeset/approval-gate-final-response.md diff --git a/.changeset/approval-gate-final-response.md b/.changeset/approval-gate-final-response.md new file mode 100644 index 00000000..87a554d8 --- /dev/null +++ b/.changeset/approval-gate-final-response.md @@ -0,0 +1,5 @@ +--- +"@openrouter/agent": patch +--- + +Run the approval check before executing pending tool calls on the `allowFinalResponse` path, and evaluate function-form `requireApproval` predicates against schema-normalized arguments so the approval decision matches the executed input. diff --git a/packages/agent/src/lib/conversation-state.ts b/packages/agent/src/lib/conversation-state.ts index 06bd74d2..401943cb 100644 --- a/packages/agent/src/lib/conversation-state.ts +++ b/packages/agent/src/lib/conversation-state.ts @@ -1,4 +1,5 @@ import type * as models from '@openrouter/sdk/models'; +import * as z4 from 'zod/v4'; import type { ConversationState, ParsedToolCall, @@ -255,6 +256,21 @@ export function appendToMessages( ]; } +function normalizeToolCallArguments< + TTool extends Exclude< + Tool, + { + _brand: 'server-tool'; + } + >, +>(toolCall: ParsedToolCall, tool: TTool): boolean { + const parsed = z4.safeParse(tool.function.inputSchema, toolCall.arguments); + if (parsed.success) { + toolCall.arguments = parsed.data as ParsedToolCall['arguments']; + } + return parsed.success; +} + /** * Check if a tool call requires approval * @param toolCall - The tool call to check @@ -271,12 +287,6 @@ export async function toolRequiresApproval( context: TurnContext, ) => boolean | Promise, ): Promise { - // 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,6 +297,25 @@ export async function toolRequiresApproval( } > => isClientTool(t) && t.function.name === toolCall.name, ); + if (tool) { + const argumentsAreValid = normalizeToolCallArguments( + toolCall as ParsedToolCall, + tool, + ); + if (!argumentsAreValid) { + // Invalid arguments cannot execute. Let executeTool surface its structured + // validation error instead of asking for approval or evaluating a + // predicate against unvalidated input. + return false; + } + } + + // Call-level check takes precedence + if (callLevelCheck) { + return callLevelCheck(toolCall, context); + } + + // Fall back to tool-level setting (server tools never require approval) if (!tool) { return false; } @@ -294,10 +323,8 @@ export async function toolRequiresApproval( 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. + // Arguments were schema-normalized above. The guard keeps this boundary + // explicit if an input schema ever produces a non-record value. if (typeof requireApproval === 'function') { const rawArgs: unknown = toolCall.arguments; if (!isRecord(rawArgs)) { diff --git a/packages/agent/src/lib/model-result.ts b/packages/agent/src/lib/model-result.ts index 7a0402e8..70079419 100644 --- a/packages/agent/src/lib/model-result.ts +++ b/packages/agent/src/lib/model-result.ts @@ -3207,6 +3207,10 @@ export class ModelResult< numberOfTurns: turnNumber, }; + if (await this.handleApprovalCheck(pendingToolCalls, turnNumber, currentResponse)) { + return; + } + await this.options.onTurnStart?.(turnContext); await this.resolveAsyncFunctionsForTurn(turnContext); diff --git a/packages/agent/tests/unit/allow-final-response.test.ts b/packages/agent/tests/unit/allow-final-response.test.ts index b33f116a..dd6a8b12 100644 --- a/packages/agent/tests/unit/allow-final-response.test.ts +++ b/packages/agent/tests/unit/allow-final-response.test.ts @@ -323,6 +323,61 @@ describe('allowFinalResponse', () => { expect(fnCallOutput?.output).toContain('"temperature":99'); }); + it('pauses for approval before executing final-response tool calls', async () => { + const executeSpy = vi.fn(async (_p: { location: string }) => ({ + temperature: 99, + })); + const approvalTool = { + type: ToolType.Function, + function: { + name: 'get_weather', + description: 'Get the weather for a location.', + inputSchema: z.object({ + location: z.string(), + }), + outputSchema: z.object({ + temperature: z.number(), + }), + requireApproval: true, + execute: executeSpy, + }, + } as const; + const saved: Array<{ + status?: string; + pendingToolCalls?: unknown[]; + }> = []; + const stateAccessor = { + load: async () => null, + save: async (state: { status?: string; pendingToolCalls?: unknown[] }) => { + saved.push(state); + }, + }; + + mockBetaResponsesSend.mockResolvedValueOnce({ + ok: true, + value: toolCallResponse(), + }); + + const result = callModel(client, { + model: 'test-model', + input: 'What is the weather?', + tools: [ + approvalTool, + ] as const, + stopWhen: stepCountIs(0), + allowFinalResponse: true, + state: stateAccessor as unknown as Parameters[1]['state'], + }); + + const pending = await result.getPendingToolCalls(); + + expect(pending).toHaveLength(1); + expect(pending[0]?.name).toBe('get_weather'); + expect(executeSpy).not.toHaveBeenCalled(); + expect(mockBetaResponsesSend).toHaveBeenCalledTimes(1); + expect(saved.some((state) => state.status === 'awaiting_approval')).toBe(true); + }); + it('does not trigger when allowFinalResponse is false', async () => { mockBetaResponsesSend.mockResolvedValueOnce({ ok: true, diff --git a/packages/agent/tests/unit/conversation-state.test.ts b/packages/agent/tests/unit/conversation-state.test.ts index 81c60e24..5c6d5652 100644 --- a/packages/agent/tests/unit/conversation-state.test.ts +++ b/packages/agent/tests/unit/conversation-state.test.ts @@ -12,6 +12,7 @@ import { updateState, } from '../../src/lib/conversation-state.js'; import { tool } from '../../src/lib/tool.js'; +import { executeTool } from '../../src/lib/tool-executor.js'; import { hasApprovalRequiredTools, toolHasApprovalConfigured } from '../../src/lib/tool-types.js'; describe('Conversation State Utilities', () => { @@ -308,6 +309,108 @@ describe('Conversation State Utilities', () => { ).toBe(true); }); + it('should apply schema defaults before function-based approval checks', async () => { + const toolWithDefaultedApproval = tool({ + name: 'defaulted_action', + inputSchema: z.object({ + destructive: z.boolean().default(true), + }), + requireApproval: (params) => params.destructive === true, + execute: async () => ({}), + }); + + const toolCall = { + id: '1', + name: 'defaulted_action', + arguments: {}, + }; + + expect( + await toolRequiresApproval( + toolCall, + [ + toolWithDefaultedApproval, + ], + context, + ), + ).toBe(true); + expect(toolCall.arguments).toEqual({ + destructive: true, + }); + }); + + it('should apply schema defaults before call-level requireApproval', async () => { + const toolWithDefaultedInput = tool({ + name: 'call_level_defaulted_action', + inputSchema: z.object({ + destructive: z.boolean().default(true), + }), + execute: async () => ({}), + }); + const toolCall = { + id: '1', + name: 'call_level_defaulted_action', + arguments: {}, + }; + let receivedArguments: unknown; + + await toolRequiresApproval( + toolCall, + [ + toolWithDefaultedInput, + ], + context, + (normalizedToolCall) => { + receivedArguments = normalizedToolCall.arguments; + return false; + }, + ); + + expect(receivedArguments).toEqual({ + destructive: true, + }); + }); + + it('should surface invalid arguments through execution without running approval or tool code', async () => { + let approvalChecked = false; + let toolExecuted = false; + const validatedTool = tool({ + name: 'validated_action', + inputSchema: z.object({ + count: z.number(), + }), + execute: async () => { + toolExecuted = true; + return {}; + }, + }); + const toolCall = { + id: '1', + name: 'validated_action', + arguments: { + count: 'not a number', + }, + }; + + const requiresApproval = await toolRequiresApproval( + toolCall, + [ + validatedTool, + ], + context, + () => { + approvalChecked = true; + return true; + }, + ); + const execution = await executeTool(validatedTool, toolCall, context); + + expect(requiresApproval).toBe(false); + expect(approvalChecked).toBe(false); + expect(toolExecuted).toBe(false); + expect(execution?.error).toBeInstanceOf(z.ZodError); + }); + it('should support async function-based tool-level requireApproval', async () => { // Tool with async function-based approval const toolWithAsyncApproval = tool({