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 06125187..af516689 100644 --- a/packages/agent/src/lib/model-result.ts +++ b/packages/agent/src/lib/model-result.ts @@ -3223,6 +3223,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 34515407..85d59247 100644 --- a/packages/agent/tests/unit/allow-final-response.test.ts +++ b/packages/agent/tests/unit/allow-final-response.test.ts @@ -367,6 +367,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({