From e043a83f74ad639933ba7b110e9575c417425e88 Mon Sep 17 00:00:00 2001 From: Matt Apperson Date: Wed, 4 Nov 2026 15:08:16 -0500 Subject: [PATCH 1/2] fix: harden tool approval decisions and predicates - #520: run tool-call arguments through the tool's inputSchema before calling a call-level or tool-level requireApproval predicate, so the predicate sees the same defaults and coercions the executor runs with. The original call is not modified, so the executor's own parse does not apply transforms twice. - #518: reject duplicate IDs within approveToolCalls or rejectToolCalls, and IDs present in both, when ModelResult is constructed, before any tool runs. - #517: resuming an awaiting_approval conversation always goes through decision processing. With no decisions every call stays pending, so the conversation stays paused instead of sending a new model request. Fixes #517 Fixes #518 Fixes #520 --- src/lib/conversation-state.ts | 68 ++++++++++- src/lib/model-result.ts | 10 +- tests/unit/approval-hardening.test.ts | 165 ++++++++++++++++++++++++++ 3 files changed, 237 insertions(+), 6 deletions(-) create mode 100644 tests/unit/approval-hardening.test.ts diff --git a/src/lib/conversation-state.ts b/src/lib/conversation-state.ts index 344856ead..0b5e74c35 100644 --- a/src/lib/conversation-state.ts +++ b/src/lib/conversation-state.ts @@ -3,11 +3,13 @@ import type { InputsUnion, InputsUnion1 } from '../models/inputsunion.js'; import type { OpenResponsesResult } from '../models/openresponsesresult.js'; import type { ConversationState, + InferToolInput, ParsedToolCall, Tool, TurnContext, UnsentToolResult, } from './tool-types.js'; +import * as z4 from 'zod/v4'; import { normalizeInputToArray } from './turn-context.js'; /** @@ -110,26 +112,86 @@ 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 normalizedCall = tool ? withNormalizedArguments(toolCall, tool) : toolCall; + // Call-level check takes precedence if (callLevelCheck) { - return callLevelCheck(toolCall, context); + return callLevelCheck(normalizedCall, 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); + return requireApproval(normalizedCall.arguments, context); } // Otherwise treat as boolean return requireApproval ?? false; } +/** + * Return the tool call with its arguments run through the tool's inputSchema, + * applying defaults and coercions the same way the executor does. If the + * arguments fail validation, the executor rejects the call before `execute` + * runs, so the raw arguments are passed through unchanged. + * + * The original call is not modified: the executor parses `toolCall.arguments` + * itself, and parsing already-parsed values would apply transforms twice. + */ +function withNormalizedArguments( + toolCall: ParsedToolCall, + tool: T, +): ParsedToolCall { + const parsed = z4.safeParse(tool.function.inputSchema, toolCall.arguments); + if (!parsed.success || !conformsToToolInput(tool, parsed.data)) return toolCall; + return { ...toolCall, arguments: parsed.data }; +} + +/** + * Runtime check that a value is valid input for the tool. Parsed output that + * no longer matches the schema (for example after a type-changing transform) + * fails it, and the caller falls back to the raw arguments. + */ +function conformsToToolInput(tool: T, value: unknown): value is InferToolInput { + return z4.safeParse(tool.function.inputSchema, value).success; +} + +/** + * 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..058c6f2d3 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 }; diff --git a/tests/unit/approval-hardening.test.ts b/tests/unit/approval-hardening.test.ts new file mode 100644 index 000000000..890dad756 --- /dev/null +++ b/tests/unit/approval-hardening.test.ts @@ -0,0 +1,165 @@ +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('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); + }); +}); + +/** 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, + }; +} From 9b3eb1d64fe6496e040f43d708474b56c85d26f3 Mon Sep 17 00:00:00 2001 From: Matt Apperson Date: Thu, 5 Nov 2026 12:07:28 -0500 Subject: [PATCH 2/2] fix: pass transformed tool input to approval predicates The re-validation guard parsed the already-parsed arguments a second time. A schema with a type-changing transform (e.g. a string transformed to a boolean) fails that second parse, so predicates fell back to the raw arguments and could allow a call the executor then ran with the transformed value. Pass the inputSchema output whenever the first parse succeeds. Also explain a pending approval when getText() or getResponse() is called on a resumed conversation that is still awaiting a decision, instead of the generic "Response not available". --- src/lib/conversation-state.ts | 58 +++++++++++++++------------ src/lib/model-result.ts | 21 +++++++++- tests/unit/approval-hardening.test.ts | 33 +++++++++++++++ 3 files changed, 84 insertions(+), 28 deletions(-) diff --git a/src/lib/conversation-state.ts b/src/lib/conversation-state.ts index 0b5e74c35..d3c3aa7f5 100644 --- a/src/lib/conversation-state.ts +++ b/src/lib/conversation-state.ts @@ -3,7 +3,6 @@ import type { InputsUnion, InputsUnion1 } from '../models/inputsunion.js'; import type { OpenResponsesResult } from '../models/openresponsesresult.js'; import type { ConversationState, - InferToolInput, ParsedToolCall, Tool, TurnContext, @@ -114,11 +113,11 @@ export async function toolRequiresApproval( ): Promise { const tool = tools.find(t => t.function.name === toolCall.name); // Predicates see the arguments the executor will run with (#520) - const normalizedCall = tool ? withNormalizedArguments(toolCall, tool) : toolCall; + const args = tool ? normalizedArguments(toolCall.arguments, tool) : toolCall.arguments; // Call-level check takes precedence - if (callLevelCheck) { - return callLevelCheck(normalizedCall, context); + if (isCallApprovalCheck(callLevelCheck)) { + return callLevelCheck({ ...toolCall, arguments: args }, context); } // Fall back to tool-level setting @@ -128,38 +127,45 @@ export async function toolRequiresApproval( // If it's a function, call it with the tool's arguments and context if (typeof requireApproval === 'function') { - return requireApproval(normalizedCall.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; } -/** - * Return the tool call with its arguments run through the tool's inputSchema, - * applying defaults and coercions the same way the executor does. If the - * arguments fail validation, the executor rejects the call before `execute` - * runs, so the raw arguments are passed through unchanged. - * - * The original call is not modified: the executor parses `toolCall.arguments` - * itself, and parsing already-parsed values would apply transforms twice. - */ -function withNormalizedArguments( - toolCall: ParsedToolCall, - tool: T, -): ParsedToolCall { - const parsed = z4.safeParse(tool.function.inputSchema, toolCall.arguments); - if (!parsed.success || !conformsToToolInput(tool, parsed.data)) return toolCall; - return { ...toolCall, arguments: parsed.data }; +/** 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'; } /** - * Runtime check that a value is valid input for the tool. Parsed output that - * no longer matches the schema (for example after a type-changing transform) - * fails it, and the caller falls back to the raw arguments. + * 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 conformsToToolInput(tool: T, value: unknown): value is InferToolInput { - return z4.safeParse(tool.function.inputSchema, value).success; +function normalizedArguments(args: unknown, tool: Tool): unknown { + const parsed = z4.safeParse(tool.function.inputSchema, args); + return parsed.success ? parsed.data : args; } /** diff --git a/src/lib/model-result.ts b/src/lib/model-result.ts index 058c6f2d3..0c907f627 100644 --- a/src/lib/model-result.ts +++ b/src/lib/model-result.ts @@ -1366,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 */ @@ -1373,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); @@ -1401,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 index 890dad756..473ac1eae 100644 --- a/tests/unit/approval-hardening.test.ts +++ b/tests/unit/approval-hardening.test.ts @@ -52,6 +52,23 @@ describe('approval predicates see normalized arguments (#520)', () => { 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); @@ -117,6 +134,22 @@ describe('resuming a paused conversation without a decision (#517)', () => { 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. */