diff --git a/src/lib/model-result.ts b/src/lib/model-result.ts index ebf1527c5..9de1a4177 100644 --- a/src/lib/model-result.ts +++ b/src/lib/model-result.ts @@ -559,6 +559,29 @@ export class ModelResult< }); } + /** + * Refuse a batch the loop would execute when two of its `function_call` + * items share a `call_id`. Tool outputs, approval decisions and pending + * calls are all matched by that id, so an approval meant for one call could + * run another. Batches with no executable calls are left to the caller, who + * owns correlation for manual tools. + */ + private assertUniqueCallIds(toolCalls: ParsedToolCall[]): void { + if (!this.hasExecutableToolCalls(toolCalls)) { + return; + } + const seen = new Set(); + for (const toolCall of toolCalls) { + if (seen.has(toolCall.id)) { + throw new Error( + `Response contains more than one function_call with call_id "${toolCall.id}"; ` + + 'refusing to execute the batch because tool outputs are matched to calls by call_id.', + ); + } + seen.add(toolCall.id); + } + } + /** * Execute tools that can auto-execute (don't require approval) in parallel. * @@ -1111,6 +1134,20 @@ export class ModelResult< const pendingCalls = this.currentState.pendingToolCalls ?? []; const unsentResults = [...(this.currentState.unsentToolResults ?? [])]; + // Saved state can predate the per-round check, so re-check before any + // decision runs a tool: a call id must name exactly one row. + const savedCallIds = new Set(unsentResults.map(r => r.callId)); + for (const tc of pendingCalls) { + if (savedCallIds.has(tc.id)) { + throw new Error( + `Saved state has more than one tool call with call_id "${tc.id}"; ` + + 'refusing to apply approval decisions because they are matched to calls by call_id.', + ); + } + savedCallIds.add(tc.id); + } + const handled = new Set>(); + // Build turn context - numberOfTurns represents the current turn (1-indexed after initial) const turnContext: TurnContext = { numberOfTurns: this.allToolExecutionRounds.length + 1, @@ -1119,8 +1156,9 @@ export class ModelResult< // Process approvals - execute the approved tools for (const callId of this.approvedToolCalls) { - const toolCall = pendingCalls.find(tc => tc.id === callId); + const toolCall = pendingCalls.find(tc => tc.id === callId && !handled.has(tc)); if (!toolCall) continue; + handled.add(toolCall); const tool = this.options.tools?.find(t => t.function.name === toolCall.name); if (!tool || !hasExecuteFunction(tool)) { @@ -1140,15 +1178,15 @@ export class ModelResult< // Process rejections for (const callId of this.rejectedToolCalls) { - const toolCall = pendingCalls.find(tc => tc.id === callId); + const toolCall = pendingCalls.find(tc => tc.id === callId && !handled.has(tc)); if (!toolCall) continue; + handled.add(toolCall); unsentResults.push(createRejectedResult(callId, String(toolCall.name), 'Rejected by user')); } - // Remove processed calls from pending - const processedIds = new Set([...this.approvedToolCalls, ...this.rejectedToolCalls]); - const remainingPending = pendingCalls.filter(tc => !processedIds.has(tc.id)); + // Remove only the rows that were handled above + const remainingPending = pendingCalls.filter(tc => !handled.has(tc)); // Update state - conditionally include optional properties only if they have values const stateUpdates: Partial, 'id' | 'createdAt' | 'updatedAt'>> = { @@ -1277,6 +1315,7 @@ export class ModelResult< // Extract and check tool calls const toolCalls = extractToolCallsFromResponse(currentResponse); + this.assertUniqueCallIds(toolCalls); // Check for approval requirements if (await this.handleApprovalCheck(toolCalls, 0, currentResponse)) { @@ -1307,6 +1346,7 @@ export class ModelResult< if (currentToolCalls.length === 0) { break; } + this.assertUniqueCallIds(currentToolCalls); // Check for approval requirements if (await this.handleApprovalCheck(currentToolCalls, currentRound + 1, currentResponse)) { diff --git a/tests/unit/duplicate-call-ids.test.ts b/tests/unit/duplicate-call-ids.test.ts new file mode 100644 index 000000000..ff58a178a --- /dev/null +++ b/tests/unit/duplicate-call-ids.test.ts @@ -0,0 +1,295 @@ +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: true, + } 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, + name: string, + args: Record, + index: number, +): OpenResponsesResult['output'][number] { + return { + type: 'function_call', + id: `fc_${index}`, + callId, + name, + arguments: JSON.stringify(args), + status: 'completed', + } as OpenResponsesResult['output'][number]; +} + +function createMemoryAccessor(initial: ConversationState | null = null) { + let state = initial; + const accessor: StateAccessor = { + load: async () => state, + save: async (next) => { + state = next; + }, + }; + return { accessor, get: () => state }; +} + +const request = { model: 'test-model', input: 'tidy up the repo' }; + +let executed: string[]; + +function makeTools(options: { harmlessNeedsApproval: boolean }) { + const deleteRepo = tool({ + name: 'delete_repo', + inputSchema: z.object({ repo: z.string() }), + requireApproval: true, + execute: async ({ repo }) => { + executed.push(`delete_repo:${repo}`); + return { ok: true }; + }, + }); + const readFile = tool({ + name: 'read_file', + inputSchema: z.object({ path: z.string() }), + requireApproval: options.harmlessNeedsApproval, + execute: async ({ path }) => { + executed.push(`read_file:${path}`); + return { ok: true }; + }, + }); + return [deleteRepo, readFile] as const; +} + +/** A protected call and a harmless call that share one call_id. */ +function sharedIdResponse(id: string, callId: string): OpenResponsesResult { + return makeResponse(id, [ + functionCall(callId, 'delete_repo', { repo: 'prod' }, 0), + functionCall(callId, 'read_file', { path: 'README.md' }, 1), + ]); +} + +beforeEach(() => { + mockResponsesSend.mockReset(); + executed = []; +}); + +describe('duplicate call ids in one model response', () => { + it('approving the harmless call cannot run a protected call that shares its call_id', async () => { + const tools = makeTools({ harmlessNeedsApproval: true }); + mockResponsesSend + .mockResolvedValueOnce({ ok: true, value: sharedIdResponse('resp_1', 'call_x') }) + .mockResolvedValue({ ok: true, value: doneResponse('resp_done') }); + const memory = createMemoryAccessor(); + + const first = new ModelResult({ + request, + client: {} as OpenRouterCore, + tools, + state: memory.accessor, + }); + // Fails closed before anything is saved as pending. + await expect(first.getResponse()).rejects.toThrow(/call_x/); + expect(memory.get()?.status).not.toBe('awaiting_approval'); + expect(memory.get()?.pendingToolCalls).toBeUndefined(); + + // The caller approves the call it was shown as harmless. + const resumed = new ModelResult({ + request, + client: {} as OpenRouterCore, + tools, + state: memory.accessor, + approveToolCalls: ['call_x'], + }); + await resumed.getResponse().catch(() => undefined); + + expect(executed).not.toContain('delete_repo:prod'); + }); + + it('runs no tool when an auto-executed call and an approval-gated call share a call_id', async () => { + const tools = makeTools({ harmlessNeedsApproval: false }); + mockResponsesSend.mockResolvedValueOnce({ ok: true, value: sharedIdResponse('resp_1', 'call_x') }); + const memory = createMemoryAccessor(); + + await expect( + new ModelResult({ + request, + client: {} as OpenRouterCore, + tools, + state: memory.accessor, + }).getResponse(), + ).rejects.toThrow(/call_x/); + + expect(executed).toEqual([]); + expect(mockResponsesSend).toHaveBeenCalledTimes(1); + }); + + it('rejects a duplicate-id batch that arrives on a later turn', async () => { + const tools = makeTools({ harmlessNeedsApproval: false }); + mockResponsesSend + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_1', [functionCall('call_a', 'read_file', { path: 'a.md' }, 0)]), + }) + .mockResolvedValueOnce({ ok: true, value: sharedIdResponse('resp_2', 'call_c') }); + + await expect( + new ModelResult({ request, client: {} as OpenRouterCore, tools }).getResponse(), + ).rejects.toThrow(/call_c/); + + expect(executed).toEqual(['read_file:a.md']); + expect(mockResponsesSend).toHaveBeenCalledTimes(2); + }); + + it('leaves a duplicate-id batch of manual tools to the caller', async () => { + const manual = tool({ + name: 'manual', + inputSchema: z.object({ n: z.number() }), + execute: false, + }); + const tools = [manual] as const; + mockResponsesSend.mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_1', [ + functionCall('call_m', 'manual', { n: 1 }, 0), + functionCall('call_m', 'manual', { n: 2 }, 1), + ]), + }); + + const result = new ModelResult({ request, client: {} as OpenRouterCore, tools }); + const calls = await result.getToolCalls(); + expect(calls.map((c) => c.arguments)).toEqual([{ n: 1 }, { n: 2 }]); + }); +}); + +describe('resuming saved approval state', () => { + function savedState( + overrides: Partial>, + ): ConversationState { + return { + id: 'conv_1', + messages: [], + status: 'awaiting_approval', + createdAt: 0, + updatedAt: 0, + ...overrides, + }; + } + + it('refuses saved pending calls that share a call_id', async () => { + const tools = makeTools({ harmlessNeedsApproval: true }); + const memory = createMemoryAccessor( + savedState({ + pendingToolCalls: [ + { id: 'call_x', name: 'delete_repo', arguments: { repo: 'prod' } }, + { id: 'call_x', name: 'read_file', arguments: { path: 'README.md' } }, + ], + }), + ); + mockResponsesSend.mockResolvedValue({ ok: true, value: doneResponse('resp_done') }); + + await expect( + new ModelResult({ + request, + client: {} as OpenRouterCore, + tools, + state: memory.accessor, + approveToolCalls: ['call_x'], + }).getResponse(), + ).rejects.toThrow(/call_x/); + + expect(executed).toEqual([]); + expect(mockResponsesSend).not.toHaveBeenCalled(); + expect(memory.get()?.pendingToolCalls).toHaveLength(2); + }); + + it('refuses a saved pending call that shares a call_id with an unsent result', async () => { + const tools = makeTools({ harmlessNeedsApproval: false }); + const memory = createMemoryAccessor( + savedState({ + pendingToolCalls: [{ id: 'call_x', name: 'delete_repo', arguments: { repo: 'prod' } }], + unsentToolResults: [{ callId: 'call_x', name: 'read_file', output: { ok: true } }], + }), + ); + + await expect( + new ModelResult({ + request, + client: {} as OpenRouterCore, + tools, + state: memory.accessor, + approveToolCalls: ['call_x'], + }).getResponse(), + ).rejects.toThrow(/call_x/); + + expect(executed).toEqual([]); + expect(mockResponsesSend).not.toHaveBeenCalled(); + }); + + it('runs only the approved call and keeps the other one pending', async () => { + const tools = makeTools({ harmlessNeedsApproval: true }); + const memory = createMemoryAccessor( + savedState({ + pendingToolCalls: [ + { id: 'call_delete', name: 'delete_repo', arguments: { repo: 'prod' } }, + { id: 'call_read', name: 'read_file', arguments: { path: 'README.md' } }, + ], + }), + ); + + await new ModelResult({ + request, + client: {} as OpenRouterCore, + tools, + state: memory.accessor, + approveToolCalls: ['call_read'], + }).getResponse().catch(() => undefined); + + expect(executed).toEqual(['read_file:README.md']); + expect(memory.get()?.status).toBe('awaiting_approval'); + expect(memory.get()?.pendingToolCalls?.map((tc) => tc.id)).toEqual(['call_delete']); + expect(memory.get()?.unsentToolResults?.map((r) => r.callId)).toEqual(['call_read']); + expect(mockResponsesSend).not.toHaveBeenCalled(); + }); +});