diff --git a/src/lib/conversation-state.ts b/src/lib/conversation-state.ts index 344856ead..9fb1103d1 100644 --- a/src/lib/conversation-state.ts +++ b/src/lib/conversation-state.ts @@ -9,6 +9,7 @@ import type { UnsentToolResult, } from './tool-types.js'; import { normalizeInputToArray } from './turn-context.js'; +import * as z4 from 'zod/v4'; /** * Type guard to verify an object is a valid UnsentToolResult @@ -102,7 +103,8 @@ 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 schema-normalized arguments when parsing succeeds and the raw call otherwise. */ export async function toolRequiresApproval( toolCall: ParsedToolCall, @@ -110,26 +112,63 @@ export async function toolRequiresApproval( context: TurnContext, callLevelCheck?: (toolCall: ParsedToolCall, context: TurnContext) => boolean | Promise ): Promise { - // Call-level check takes precedence + const tool = tools.find(t => t.function.name === toolCall.name); + + // Call-level checks always take precedence. Pass a normalized copy when + // parsing succeeds (so the check sees the values `execute` will receive), + // or the raw call when it does not. The caller's decision is honored either + // way; the original call is never mutated. if (callLevelCheck) { - return callLevelCheck(toolCall, context); + 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, + context + ); } // Fall back to tool-level setting - const tool = tools.find(t => t.function.name === toolCall.name); if (!tool) return false; const requireApproval = tool.function.requireApproval; - // If it's a function, call it with the tool's arguments and context + // `toolCall.arguments` is only the JSON-parsed wire payload; it has not been + // validated against the tool's inputSchema. The executor validates right + // before `execute`, 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. `{ dangerous: z.boolean().default(true) }` with `{}` on + // the wire: the predicate sees `undefined`, then `execute` runs with `true`). + // Parse with the same schema so the predicate decides on the executed values. if (typeof requireApproval === 'function') { - return requireApproval(toolCall.arguments, context); + const parsed = z4.safeParse(tool.function.inputSchema, toolCall.arguments); + if (!parsed.success) { + // No trustworthy value to judge. Fail closed rather than waive approval + // based on arguments the tool body would never receive. + return true; + } + if (!isRecord(parsed.data)) { + // Valid per the schema but not an object: the predicate contract is + // Record, so fail closed. + return true; + } + return requireApproval(parsed.data, context); } // Otherwise treat as boolean return requireApproval ?? false; } +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + /** * Partition tool calls into those requiring approval and those that can auto-execute * @param toolCalls - Tool calls to partition diff --git a/src/lib/model-result.ts b/src/lib/model-result.ts index ebf1527c5..65db99521 100644 --- a/src/lib/model-result.ts +++ b/src/lib/model-result.ts @@ -227,8 +227,16 @@ export class ModelResult< // Initialize state management this.stateAccessor = options.state ?? null; this.requireApprovalFn = options.requireApproval ?? null; - this.approvedToolCalls = options.approveToolCalls ?? []; - this.rejectedToolCalls = options.rejectToolCalls ?? []; + this.approvedToolCalls = [...new Set(options.approveToolCalls ?? [])]; + this.rejectedToolCalls = [...new Set(options.rejectToolCalls ?? [])]; + + const rejectedIds = new Set(this.rejectedToolCalls); + const conflictingIds = this.approvedToolCalls.filter(id => rejectedIds.has(id)); + if (conflictingIds.length > 0) { + throw new Error( + `Tool calls cannot be both approved and rejected: ${conflictingIds.join(', ')}` + ); + } } /** @@ -1018,6 +1026,12 @@ export class ModelResult< return; // Skip normal initialization, we're resuming } + // Resuming without any decision keeps the conversation paused + if (loadedState.status === 'awaiting_approval') { + this.isResumingFromApproval = true; + return; + } + // Check for interruption flag and handle if (loadedState.interruptedBy) { // Clear interruption flag and continue from saved state diff --git a/tests/unit/conversation-state.test.ts b/tests/unit/conversation-state.test.ts index 9544ce01f..a6f981110 100644 --- a/tests/unit/conversation-state.test.ts +++ b/tests/unit/conversation-state.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; import { z } from 'zod/v4'; import { createInitialState, @@ -131,6 +131,96 @@ describe('Conversation State Utilities', () => { expect(await toolRequiresApproval(toolCall, [toolWithApproval, toolWithoutApproval], context)).toBe(false); }); + it('should evaluate tool-level predicates against schema-parsed arguments', async () => { + const deleteTool = tool({ + name: 'delete_files', + inputSchema: z.object({ recursive: z.boolean().default(true) }), + requireApproval: (args) => args.recursive, + execute: async () => ({}), + }); + const toolCall = { id: '1', name: 'delete_files', arguments: {} }; + + expect(await toolRequiresApproval(toolCall, [deleteTool], context)).toBe(true); + }); + + it('should apply schema coercions before a tool-level predicate', async () => { + const seen: unknown[] = []; + const transfer = tool({ + name: 'transfer', + inputSchema: z.object({ amount: z.coerce.number() }), + requireApproval: (args) => { + seen.push(args); + return args.amount > 100; + }, + execute: async () => ({}), + }); + const toolCall = { id: '1', name: 'transfer', arguments: { amount: '500' } }; + + expect(await toolRequiresApproval(toolCall, [transfer], context)).toBe(true); + expect(seen).toEqual([{ amount: 500 }]); + }); + + it('should fail closed when arguments fail schema validation for a tool-level predicate', async () => { + const predicate = vi.fn(() => false); + const strict = tool({ + name: 'strict_action', + inputSchema: z.object({ target: z.string() }), + requireApproval: predicate, + execute: async () => ({}), + }); + const toolCall = { id: '1', name: 'strict_action', arguments: {} }; + + expect(await toolRequiresApproval(toolCall, [strict], context)).toBe(true); + expect(predicate).not.toHaveBeenCalled(); + }); + + it('should fail closed when the schema parses to a non-object value', async () => { + const predicate = vi.fn(() => false); + const weird = tool({ + name: 'weird_action', + inputSchema: z.object({ target: z.string() }), + requireApproval: predicate, + execute: async () => ({}), + }); + (weird.function as { inputSchema: unknown }).inputSchema = z + .object({ target: z.string() }) + .transform(() => ['not', 'a', 'record']); + const toolCall = { id: '1', name: 'weird_action', arguments: { target: 'x' } }; + + expect(await toolRequiresApproval(toolCall, [weird], context)).toBe(true); + expect(predicate).not.toHaveBeenCalled(); + }); + + it('should pass schema-parsed arguments to a call-level check without mutating the call', async () => { + const defaulted = tool({ + name: 'defaulted_action', + inputSchema: z.object({ destructive: z.boolean().default(true) }), + execute: async () => ({}), + }); + const toolCall = { id: '1', name: 'defaulted_action', arguments: {} }; + const callLevelCheck = vi.fn((tc: { arguments: Record }) => tc.arguments['destructive'] === true); + + expect(await toolRequiresApproval(toolCall, [defaulted], context, callLevelCheck)).toBe(true); + expect(callLevelCheck).toHaveBeenCalledWith({ ...toolCall, arguments: { destructive: true } }, context); + expect(toolCall.arguments).toEqual({}); + }); + + it('should honor a call-level check with the raw call when arguments fail schema validation', async () => { + const strict = tool({ + name: 'strict_action', + inputSchema: z.object({ target: z.string() }), + execute: async () => ({}), + }); + const toolCall = { id: '1', name: 'strict_action', arguments: {} }; + const allow = vi.fn(() => false); + const deny = vi.fn(() => true); + + expect(await toolRequiresApproval(toolCall, [strict], context, allow)).toBe(false); + expect(allow).toHaveBeenCalledWith(toolCall, context); + expect(await toolRequiresApproval(toolCall, [strict], context, deny)).toBe(true); + expect(deny).toHaveBeenCalledWith(toolCall, context); + }); + it('should use call-level check when provided', async () => { const toolCall = { id: '1', name: 'safe_action', arguments: {} }; const alwaysRequire = () => true; diff --git a/tests/unit/model-result-approval.test.ts b/tests/unit/model-result-approval.test.ts new file mode 100644 index 000000000..89c232d8f --- /dev/null +++ b/tests/unit/model-result-approval.test.ts @@ -0,0 +1,201 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { z } from 'zod/v4'; +import type { OpenRouterCore } from '../../src/core.js'; +import type { OpenResponsesResult } from '../../src/models/openresponsesresult.js'; +import { tool } from '../../src/lib/tool.js'; +import type { ConversationState, StateAccessor, Tool } from '../../src/lib/tool-types.js'; + +const mockResponsesSend = vi.hoisted(() => vi.fn()); + +vi.mock('../../src/funcs/responsesSend.js', () => ({ + responsesSend: mockResponsesSend, +})); + +// Import ModelResult AFTER vi.mock so the transport mock is wired in. +const { ModelResult } = await import('../../src/lib/model-result.js'); + +function makeResponse(id: string, output: OpenResponsesResult['output']): OpenResponsesResult { + return { + id, + object: 'response', + createdAt: 0, + completedAt: 0, + model: 'test-model', + status: 'completed', + output, + error: null, + incompleteDetails: null, + temperature: null, + topP: null, + presencePenalty: null, + frequencyPenalty: null, + metadata: null, + instructions: null, + tools: [], + toolChoice: 'auto', + parallelToolCalls: false, + } as OpenResponsesResult; +} + +function doneResponse(id: string): OpenResponsesResult { + return makeResponse(id, [ + { + type: 'message', + id: `msg_${id}`, + role: 'assistant', + status: 'completed', + content: [{ type: 'output_text', text: 'Done.', annotations: [] }], + }, + ] as OpenResponsesResult['output']); +} + +function functionCall(callId: string): OpenResponsesResult['output'][number] { + return { + type: 'function_call', + id: `fc_${callId}`, + callId, + name: 'gated', + arguments: '{"v":"x"}', + status: 'completed', + } as OpenResponsesResult['output'][number]; +} + +function createMemoryAccessor() { + let state: ConversationState | null = null; + const accessor: StateAccessor = { + load: async () => state, + save: async (next) => { + state = next; + }, + }; + return { accessor, get: () => state }; +} + +/** IDs of the function_call_output items sent in the `callIndex`-th request. */ +function sentOutputIds(callIndex: number): string[] { + const input = (mockResponsesSend.mock.calls[callIndex]?.[1]?.responsesRequest?.input ?? []) as Array<{ + type?: string; + callId?: string; + }>; + return input.filter((item) => item?.type === 'function_call_output').map((item) => String(item.callId)); +} + +const request = { model: 'test-model', input: 'run the gated action' }; + +/** Runs one turn that pauses on approval-gated calls with `callIds`. */ +async function pauseOnGatedCalls(callIds: string[]) { + const execute = vi.fn(async (_input: { v: string }) => ({ ok: true })); + const gated = tool({ + name: 'gated', + inputSchema: z.object({ v: z.string() }), + outputSchema: z.object({ ok: z.boolean() }), + requireApproval: true, + execute, + }); + const tools = [gated] as const; + mockResponsesSend.mockResolvedValueOnce({ ok: true, value: makeResponse('resp_pause', callIds.map(functionCall)) }); + const memory = createMemoryAccessor(); + await new ModelResult({ + request, + client: {} as OpenRouterCore, + tools, + state: memory.accessor, + }).getResponse(); + expect(memory.get()?.status).toBe('awaiting_approval'); + expect(execute).not.toHaveBeenCalled(); + mockResponsesSend.mockClear(); + return { execute, tools, ...memory }; +} + +beforeEach(() => { + mockResponsesSend.mockReset(); +}); + +describe('approval decisions are de-duplicated (#518)', () => { + it('executes an approved call once when its ID is listed twice', async () => { + const paused = await pauseOnGatedCalls(['call_1']); + mockResponsesSend.mockResolvedValueOnce({ ok: true, value: doneResponse('resp_done') }); + + const text = await new ModelResult({ + request, + client: {} as OpenRouterCore, + tools: paused.tools, + state: paused.accessor, + approveToolCalls: ['call_1', 'call_1'], + }).getText(); + + expect(text).toBe('Done.'); + expect(paused.execute).toHaveBeenCalledTimes(1); + expect(sentOutputIds(0)).toEqual(['call_1']); + }); + + it('records a rejection once when its ID is listed twice', async () => { + const paused = await pauseOnGatedCalls(['call_1']); + mockResponsesSend.mockResolvedValueOnce({ ok: true, value: doneResponse('resp_done') }); + + await new ModelResult({ + request, + client: {} as OpenRouterCore, + tools: paused.tools, + state: paused.accessor, + rejectToolCalls: ['call_1', 'call_1'], + }).getResponse(); + + expect(paused.execute).not.toHaveBeenCalled(); + expect(sentOutputIds(0)).toEqual(['call_1']); + }); + + it('refuses an ID that is both approved and rejected before anything runs', async () => { + const paused = await pauseOnGatedCalls(['call_1', 'call_2']); + const before = structuredClone(paused.get()); + + expect( + () => + new ModelResult({ + request, + client: {} as OpenRouterCore, + tools: paused.tools, + state: paused.accessor, + approveToolCalls: ['call_1', 'call_2'], + rejectToolCalls: ['call_2'], + }) + ).toThrow(/both approved and rejected: call_2/); + + expect(paused.execute).not.toHaveBeenCalled(); + expect(mockResponsesSend).not.toHaveBeenCalled(); + expect(paused.get()).toEqual(before); + }); +}); + +describe('resuming awaiting_approval without decisions (#517)', () => { + it('stays paused and sends no request', async () => { + const paused = await pauseOnGatedCalls(['call_1']); + const before = structuredClone(paused.get()); + mockResponsesSend.mockResolvedValueOnce({ ok: true, value: doneResponse('resp_should_not_be_requested') }); + + const result = new ModelResult({ + request, + client: {} as OpenRouterCore, + tools: paused.tools, + state: paused.accessor, + }); + + await expect(result.getResponse()).rejects.toThrow(); + expect(await result.requiresApproval()).toBe(true); + expect((await result.getPendingToolCalls()).map((tc) => tc.id)).toEqual(['call_1']); + expect(mockResponsesSend).not.toHaveBeenCalled(); + expect(paused.execute).not.toHaveBeenCalled(); + expect(paused.get()).toEqual(before); + + // The pause still resolves normally once a decision is supplied. + await new ModelResult({ + request, + client: {} as OpenRouterCore, + tools: paused.tools, + state: paused.accessor, + approveToolCalls: ['call_1'], + }).getResponse(); + expect(paused.execute).toHaveBeenCalledTimes(1); + expect(paused.get()?.status).toBe('complete'); + }); +});