diff --git a/src/lib/conversation-state.ts b/src/lib/conversation-state.ts index 344856ead..d3c3aa7f5 100644 --- a/src/lib/conversation-state.ts +++ b/src/lib/conversation-state.ts @@ -8,6 +8,7 @@ import type { TurnContext, UnsentToolResult, } from './tool-types.js'; +import * as z4 from 'zod/v4'; import { normalizeInputToArray } from './turn-context.js'; /** @@ -110,26 +111,93 @@ export async function toolRequiresApproval( context: TurnContext, callLevelCheck?: (toolCall: ParsedToolCall, context: TurnContext) => boolean | Promise ): Promise { + const tool = tools.find(t => t.function.name === toolCall.name); + // Predicates see the arguments the executor will run with (#520) + const args = tool ? normalizedArguments(toolCall.arguments, tool) : toolCall.arguments; + // Call-level check takes precedence - if (callLevelCheck) { - return callLevelCheck(toolCall, context); + if (isCallApprovalCheck(callLevelCheck)) { + return callLevelCheck({ ...toolCall, arguments: args }, 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 if (typeof requireApproval === 'function') { - return requireApproval(toolCall.arguments, context); + const check: unknown = requireApproval; + // Always a function here; the guard only widens its parameter type. + return isArgumentsApprovalCheck(check) ? check(args, context) : true; } // Otherwise treat as boolean return requireApproval ?? false; } +/** A call-level approval check, seen with the arguments as parsed values. */ +type CallApprovalCheck = ( + toolCall: { id: string; name: string; arguments: unknown }, + context: TurnContext, +) => boolean | Promise; + +/** A tool-level approval check, seen with the arguments as parsed values. */ +type ArgumentsApprovalCheck = (params: unknown, context: TurnContext) => boolean | Promise; + +function isCallApprovalCheck(value: unknown): value is CallApprovalCheck { + return typeof value === 'function'; +} + +function isArgumentsApprovalCheck(value: unknown): value is ArgumentsApprovalCheck { + return typeof value === 'function'; +} + +/** + * Return the tool-call arguments as the executor will see them: the output of + * the tool's inputSchema, with defaults, coercions, and transforms applied. + * Arguments that fail validation are returned unchanged; the executor rejects + * those calls before `execute` runs. + * + * The tool call itself is not modified: the executor parses + * `toolCall.arguments` again, and parsing already-parsed values would apply + * transforms twice. + */ +function normalizedArguments(args: unknown, tool: Tool): unknown { + const parsed = z4.safeParse(tool.function.inputSchema, args); + return parsed.success ? parsed.data : args; +} + +/** + * Validate approval decisions before any tool runs (#518). + * @throws Error if an ID is listed twice, or is both approved and rejected + */ +export function assertValidApprovalDecisions( + approveToolCalls: readonly string[], + rejectToolCalls: readonly string[], +): void { + const problems = [ + ...duplicateIds(approveToolCalls).map(id => `"${id}" is listed more than once in approveToolCalls`), + ...duplicateIds(rejectToolCalls).map(id => `"${id}" is listed more than once in rejectToolCalls`), + ...[...new Set(approveToolCalls)] + .filter(id => rejectToolCalls.includes(id)) + .map(id => `"${id}" is in both approveToolCalls and rejectToolCalls`), + ]; + if (problems.length > 0) { + throw new Error(`Invalid approval decisions: ${problems.join('; ')}`); + } +} + +function duplicateIds(ids: readonly string[]): string[] { + const seen = new Set(); + const duplicates = new Set(); + for (const id of ids) { + if (seen.has(id)) duplicates.add(id); + seen.add(id); + } + return [...duplicates]; +} + /** * 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..0c907f627 100644 --- a/src/lib/model-result.ts +++ b/src/lib/model-result.ts @@ -40,6 +40,7 @@ import { } from './async-params.js'; import { appendToMessages, + assertValidApprovalDecisions, createInitialState, createRejectedResult, createUnsentResult, @@ -224,6 +225,8 @@ export class ModelResult< ); } + assertValidApprovalDecisions(options.approveToolCalls ?? [], options.rejectToolCalls ?? []); + // Initialize state management this.stateAccessor = options.state ?? null; this.requireApprovalFn = options.requireApproval ?? null; @@ -1003,9 +1006,10 @@ export class ModelResult< if (loadedState) { this.currentState = loadedState; - // Check if we're resuming from awaiting_approval with decisions - if (loadedState.status === 'awaiting_approval' && - (this.approvedToolCalls.length > 0 || this.rejectedToolCalls.length > 0)) { + // Resuming from awaiting_approval always goes through decision + // processing. With no decisions every call stays pending, so the + // conversation stays paused instead of advancing (#517). + if (loadedState.status === 'awaiting_approval') { // Initialize context store before resuming so tools have access if (this.options.context !== undefined) { const approvalContext: TurnContext = { numberOfTurns: 0 }; @@ -1362,6 +1366,23 @@ export class ModelResult< return this.toolExecutionPromise; } + /** + * Error for a missing final response. A conversation resumed while still + * awaiting approval has no response until a decision is passed. + */ + private responseUnavailableError(): Error { + if (this.currentState?.status !== 'awaiting_approval') { + return new Error('Response not available'); + } + const pending = (this.currentState.pendingToolCalls ?? []) + .map(tc => `${String(tc.name)} (${tc.id})`) + .join(', '); + return new Error( + `Conversation is awaiting approval for tool calls: ${pending}. ` + + 'Pass approveToolCalls or rejectToolCalls to continue.' + ); + } + /** * Internal helper to get the text after tool execution */ @@ -1369,7 +1390,7 @@ export class ModelResult< await this.executeToolsIfNeeded(); if (!this.finalResponse) { - throw new Error('Response not available'); + throw this.responseUnavailableError(); } return extractTextFromResponse(this.finalResponse); @@ -1397,7 +1418,7 @@ export class ModelResult< await this.executeToolsIfNeeded(); if (!this.finalResponse) { - throw new Error('Response not available'); + throw this.responseUnavailableError(); } return this.finalResponse; diff --git a/tests/unit/approval-hardening.test.ts b/tests/unit/approval-hardening.test.ts new file mode 100644 index 000000000..473ac1eae --- /dev/null +++ b/tests/unit/approval-hardening.test.ts @@ -0,0 +1,198 @@ +import type { Fetcher } from '../../src/lib/http.js'; +import type { ConversationState, ParsedToolCall, StateAccessor } from '../../src/lib/tool-types.js'; + +import { describe, expect, it } from 'vitest'; +import * as z from 'zod/v4'; +import { OpenRouterCore } from '../../src/core.js'; +import { + assertValidApprovalDecisions, + toolRequiresApproval, +} from '../../src/lib/conversation-state.js'; +import { HTTPClient } from '../../src/lib/http.js'; +import { ModelResult } from '../../src/lib/model-result.js'; +import { tool } from '../../src/lib/tool.js'; + +const context = { numberOfTurns: 1 }; + +// A tool whose schema defaults `destructive` to true, and whose approval +// predicate gates on that flag. +const deleteFiles = tool({ + name: 'delete_files', + inputSchema: z.object({ + path: z.string().default('/tmp'), + destructive: z.boolean().default(true), + }), + requireApproval: (params) => params.destructive === true, + execute: async () => ({ deleted: true }), +}); + +const tools: readonly [typeof deleteFiles] = [deleteFiles]; + +function deleteCall(args: Record): ParsedToolCall { + return { id: 'call_1', name: 'delete_files', arguments: args }; +} + +describe('approval predicates see normalized arguments (#520)', () => { + it('applies schema defaults before the tool-level predicate', async () => { + // The model omitted `destructive`; the executor would run with the default `true`. + expect(await toolRequiresApproval(deleteCall({}), tools, context)).toBe(true); + }); + + it('applies schema defaults before the call-level predicate', async () => { + const seen: unknown[] = []; + await toolRequiresApproval(deleteCall({}), tools, context, (call) => { + seen.push(call.arguments); + return false; + }); + + expect(seen).toEqual([{ path: '/tmp', destructive: true }]); + }); + + it('respects an explicit value over the default', async () => { + expect(await toolRequiresApproval(deleteCall({ destructive: false }), tools, context)).toBe(false); + }); + + it('passes type-changing transform output to the predicate', async () => { + const forceTool = tool({ + name: 'force_push', + inputSchema: z.object({ force: z.string().transform((value) => value === 'yes') }), + requireApproval: (params) => params.force === true, + execute: async () => ({ pushed: true }), + }); + const call: ParsedToolCall = { + id: 'call_2', + name: 'force_push', + arguments: { force: 'yes' }, + }; + + // The executor runs with force === true, so approval must be required. + expect(await toolRequiresApproval(call, [forceTool], context)).toBe(true); + }); + + it('does not modify the original tool call', async () => { + const call = deleteCall({}); + await toolRequiresApproval(call, tools, context); + + expect(call.arguments).toEqual({}); + }); +}); + +describe('approval decisions are validated (#518)', () => { + it('accepts distinct decisions', () => { + expect(() => assertValidApprovalDecisions(['call_1', 'call_2'], ['call_3'])).not.toThrow(); + }); + + it('rejects an ID approved twice', () => { + expect(() => assertValidApprovalDecisions(['call_1', 'call_1'], [])).toThrow( + /"call_1" is listed more than once in approveToolCalls/, + ); + }); + + it('rejects an ID rejected twice', () => { + expect(() => assertValidApprovalDecisions([], ['call_1', 'call_1'])).toThrow( + /"call_1" is listed more than once in rejectToolCalls/, + ); + }); + + it('rejects an ID that is both approved and rejected', () => { + expect(() => assertValidApprovalDecisions(['call_1'], ['call_1'])).toThrow( + /"call_1" is in both approveToolCalls and rejectToolCalls/, + ); + }); + + it('throws from ModelResult before any tool runs', () => { + const { client } = recordingClient(); + const { accessor } = memoryState(pausedState()); + + expect( + () => + new ModelResult({ + request: { model: 'openai/gpt-5-mini', input: 'hi' }, + client, + tools, + state: accessor, + approveToolCalls: ['call_1', 'call_1'], + }), + ).toThrow(/listed more than once/); + }); +}); + +describe('resuming a paused conversation without a decision (#517)', () => { + it('stays awaiting_approval and sends no model request', async () => { + const { client, requests } = recordingClient(); + const { accessor, saved } = memoryState(pausedState()); + + const result = new ModelResult({ + request: { model: 'openai/gpt-5-mini', input: 'continue' }, + client, + tools, + state: accessor, + }); + + expect(await result.requiresApproval()).toBe(true); + expect(await result.getPendingToolCalls()).toHaveLength(1); + expect(requests).toHaveLength(0); + expect(saved.every((state) => state.status === 'awaiting_approval')).toBe(true); + }); + + it('explains that approval is pending when the response is requested', async () => { + const { client } = recordingClient(); + const { accessor } = memoryState(pausedState()); + + const result = new ModelResult({ + request: { model: 'openai/gpt-5-mini', input: 'continue' }, + client, + tools, + state: accessor, + }); + + await expect(result.getResponse()).rejects.toThrow( + /awaiting approval for tool calls: delete_files \(call_1\)/, + ); + }); +}); + +/** A client whose fetcher records requests and answers with a 500. */ +function recordingClient(): { client: OpenRouterCore; requests: Request[] } { + const requests: Request[] = []; + const fetcher: Fetcher = async (input) => { + if (input instanceof Request) requests.push(input); + return new Response('{}', { status: 500 }); + }; + const client = new OpenRouterCore({ + apiKey: 'test', + httpClient: new HTTPClient({ fetcher }), + retryConfig: { strategy: 'none' }, + }); + return { client, requests }; +} + +/** A conversation paused on one approval-gated call. */ +function pausedState(): ConversationState { + return { + id: 'conv_test', + messages: [{ role: 'user', content: 'delete the temp files' }], + pendingToolCalls: [deleteCall({})], + status: 'awaiting_approval', + createdAt: 0, + updatedAt: 0, + }; +} + +function memoryState(initial: ConversationState): { + accessor: StateAccessor; + saved: ConversationState[]; +} { + let current = initial; + const saved: ConversationState[] = []; + return { + accessor: { + load: async () => current, + save: async (state) => { + current = state; + saved.push(state); + }, + }, + saved, + }; +}