diff --git a/.changeset/reject-duplicate-tool-names.md b/.changeset/reject-duplicate-tool-names.md new file mode 100644 index 00000000..574b11d7 --- /dev/null +++ b/.changeset/reject-duplicate-tool-names.md @@ -0,0 +1,5 @@ +--- +'@openrouter/agent': patch +--- + +`callModel` now throws when two client tools in `tools` share a name. Tool calls are dispatched by name, so with a duplicate the first tool in the list received the call, including a call that was approved while a different tool with that name was registered. Rename one of the tools, or pass `toolNamePrefix` to `createMCPTools` when combining MCP tools with local tools. diff --git a/packages/agent/src/lib/model-result.ts b/packages/agent/src/lib/model-result.ts index 166fb96f..466aaa77 100644 --- a/packages/agent/src/lib/model-result.ts +++ b/packages/agent/src/lib/model-result.ts @@ -98,6 +98,7 @@ import { ToolEventBroadcaster } from './tool-event-broadcaster.js'; import type { AsyncToolInvocation } from './tool-executor.js'; import { applyOnResponseReceivedHooks, + assertUniqueClientToolNames, executeTool, isAsyncToolInvocation, validateToolInput, @@ -798,6 +799,8 @@ export class ModelResult< this.doomLoopMonitor = doomLoopConfig ? new DoomLoopMonitor(doomLoopConfig) : null; this.doomLoopEscalation = doomLoopConfig?.escalation ?? null; + assertUniqueClientToolNames(options.tools); + // Runtime validation: approval decisions require state const hasApprovalDecisions = (options.approveToolCalls && options.approveToolCalls.length > 0) || diff --git a/packages/agent/src/lib/tool-executor.ts b/packages/agent/src/lib/tool-executor.ts index 780917c9..c34bab16 100644 --- a/packages/agent/src/lib/tool-executor.ts +++ b/packages/agent/src/lib/tool-executor.ts @@ -18,6 +18,7 @@ import type { UnifiedTool, } from './tool-types.js'; import { + findDuplicateClientToolName, hasExecuteFunction, isDeferredHandle, isGeneratorTool, @@ -783,6 +784,24 @@ export function findToolByName(tools: Tool[], name: string): ClientTool | undefi ); } +/** + * Throw when two client tools share a name. Calls are dispatched by name, so a + * duplicate would let whichever tool is listed first receive arguments the + * model (or an approver) meant for the other. + */ +export function assertUniqueClientToolNames(tools: readonly Tool[] | undefined): void { + if (!tools) { + return; + } + const name = findDuplicateClientToolName(tools); + if (name !== undefined) { + throw new Error( + `Duplicate tool name "${name}": every tool passed to callModel must have a unique name. ` + + 'Rename one, or use toolNamePrefix when combining MCP tools with local tools.', + ); + } +} + /** * Format tool execution result as a string for sending to the model */ diff --git a/packages/agent/src/lib/tool-types.ts b/packages/agent/src/lib/tool-types.ts index 41217cf8..3abb0cc3 100644 --- a/packages/agent/src/lib/tool-types.ts +++ b/packages/agent/src/lib/tool-types.ts @@ -1052,6 +1052,25 @@ export function isServerTool(tool: Tool): tool is ServerToolBase { return tool._brand === 'server-tool'; } +/** + * Return the first client tool name that appears more than once, or + * `undefined`. Server tools have no client name and are skipped. + */ +export function findDuplicateClientToolName(tools: readonly Tool[]): string | undefined { + const seen = new Set(); + for (const tool of tools) { + if (isServerTool(tool)) { + continue; + } + const name = tool.function.name; + if (seen.has(name)) { + return name; + } + seen.add(name); + } + return undefined; +} + /** * A client tool additionally branded as originating from an MCP server. The * `_mcp` marker is purely informational: it does NOT change how the tool is diff --git a/packages/agent/src/mcp/build-tools.ts b/packages/agent/src/mcp/build-tools.ts index 2eaaa137..f7e8d3ba 100644 --- a/packages/agent/src/mcp/build-tools.ts +++ b/packages/agent/src/mcp/build-tools.ts @@ -1,5 +1,6 @@ import type { Client } from '@modelcontextprotocol/client'; import type { Tool, ToolLoopKey } from '../lib/tool-types.js'; +import { findDuplicateClientToolName } from '../lib/tool-types.js'; import { MCPError } from './errors.js'; import { buildResourceTools } from './resource-tools.js'; import type { UnconvertibleSchemaMode } from './schema/json-schema-to-zod.js'; @@ -93,25 +94,11 @@ export function buildTools(options: BuildToolsOptions): Tool[] { return tools; } -function toolName(tool: Tool): string | undefined { - if ('function' in tool && typeof tool.function.name === 'string') { - return tool.function.name; - } - return undefined; -} - function assertNoDuplicateNames(tools: readonly Tool[]): void { - const seen = new Set(); - for (const tool of tools) { - const name = toolName(tool); - if (name === undefined) { - continue; - } - if (seen.has(name)) { - throw new MCPError( - `Duplicate tool name "${name}". Use toolNamePrefix or excludeTools to disambiguate.`, - ); - } - seen.add(name); + const name = findDuplicateClientToolName(tools); + if (name !== undefined) { + throw new MCPError( + `Duplicate tool name "${name}". Use toolNamePrefix or excludeTools to disambiguate.`, + ); } } diff --git a/packages/agent/tests/unit/duplicate-tool-names.test.ts b/packages/agent/tests/unit/duplicate-tool-names.test.ts new file mode 100644 index 00000000..f510003c --- /dev/null +++ b/packages/agent/tests/unit/duplicate-tool-names.test.ts @@ -0,0 +1,193 @@ +import type { OpenRouterCore } from '@openrouter/sdk/core'; +import type * as models from '@openrouter/sdk/models'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { z } from 'zod/v4'; +import { tool } from '../../src/lib/tool.js'; +import type { ConversationState, StateAccessor, Tool } from '../../src/lib/tool-types.js'; + +const mockBetaResponsesSend = vi.hoisted(() => vi.fn()); + +vi.mock('@openrouter/sdk/funcs/betaResponsesSend', () => ({ + betaResponsesSend: mockBetaResponsesSend, +})); + +const { ModelResult } = await import('../../src/lib/model-result.js'); +const { callModel } = await import('../../src/inner-loop/call-model.js'); + +function makeResponse( + id: string, + output: models.OpenResponsesResult['output'], +): models.OpenResponsesResult { + return { + id, + object: 'response', + createdAt: 0, + model: 'test-model', + status: 'completed', + completedAt: 0, + output, + error: null, + incompleteDetails: null, + temperature: null, + topP: null, + presencePenalty: null, + frequencyPenalty: null, + metadata: null, + instructions: null, + tools: [], + toolChoice: 'auto', + parallelToolCalls: false, + }; +} + +function createMemoryAccessor(): { + accessor: StateAccessor; + get: () => ConversationState | null; +} { + let state: ConversationState | null = null; + return { + accessor: { + load: async () => state, + save: async (next) => { + state = next; + }, + }, + get: () => state, + }; +} + +const request = { + model: 'test-model', + input: 'read the value', +}; + +function makeTool( + name: string, + execute: (input: { slot: string }) => Promise<{ + ok: boolean; + }>, +) { + return tool({ + name, + inputSchema: z.object({ + slot: z.string(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + requireApproval: true, + execute, + }); +} + +describe('duplicate client tool names', () => { + beforeEach(() => { + mockBetaResponsesSend.mockReset(); + }); + + it('rejects a tools list where two client tools share a name', () => { + const first = makeTool('read_value', async () => ({ + ok: true, + })); + const second = makeTool('read_value', async () => ({ + ok: true, + })); + + expect( + () => + new ModelResult({ + request, + client: {} as OpenRouterCore, + tools: [ + first, + second, + ], + }), + ).toThrow(/Duplicate tool name "read_value"/); + expect(() => + callModel({} as OpenRouterCore, { + ...request, + tools: [ + first, + second, + ], + }), + ).toThrow(/Duplicate tool name "read_value"/); + }); + + it('does not run an approved call on a different same-named tool added before resume', async () => { + const approvedExecute = vi.fn(async () => ({ + ok: true, + })); + const otherExecute = vi.fn(async () => ({ + ok: true, + })); + const approved = makeTool('read_value', approvedExecute); + const other = makeTool('read_value', otherExecute); + + mockBetaResponsesSend.mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_pause', [ + { + type: 'function_call', + id: 'fc_call_1', + callId: 'call_1', + name: 'read_value', + arguments: '{"slot":"synthetic-marker"}', + status: 'completed', + }, + ]), + }); + const memory = createMemoryAccessor(); + await new ModelResult({ + request, + client: {} as OpenRouterCore, + tools: [ + approved, + ], + state: memory.accessor, + }).getResponse(); + expect(memory.get()?.status).toBe('awaiting_approval'); + + mockBetaResponsesSend.mockResolvedValue({ + ok: true, + value: makeResponse('resp_done', [ + { + type: 'message', + id: 'msg_1', + role: 'assistant', + status: 'completed', + content: [ + { + type: 'output_text', + text: 'Done.', + annotations: [], + }, + ], + }, + ]), + }); + + let resumeError: unknown; + try { + await new ModelResult({ + request, + client: {} as OpenRouterCore, + tools: [ + other, + approved, + ], + state: memory.accessor, + approveToolCalls: [ + 'call_1', + ], + }).getResponse(); + } catch (error) { + resumeError = error; + } + + expect(otherExecute).not.toHaveBeenCalled(); + expect(String(resumeError)).toMatch(/Duplicate tool name "read_value"/); + expect(memory.get()?.status).toBe('awaiting_approval'); + }); +});