From 275af48592ad8f7c9571d12b621f3ed8ab25615d Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Wed, 1 Apr 2026 22:36:21 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=AF=20fix:=20MCP=20Tool=20Misclassific?= =?UTF-8?q?ation=20from=20Action=20Delimiter=20Collision=20(#12512)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: prevent MCP tools with `_action` in name from being misclassified as OpenAPI action tools Add `isActionTool()` helper that checks for the `_action_` delimiter while guarding against cross-delimiter collision with `_mcp_`. Replace all `includes(actionDelimiter)` classification checks with the new helper across backend and frontend. * test: add coverage for MCP/action cross-delimiter collision Verify that `isActionTool` correctly rejects MCP tool names containing `_action` and that `loadAgentTools` does not filter them based on `actionsEnabled`. Add ToolIcon and definitions test cases. * fix: simplify isActionTool to handle all MCP name patterns - Use `!toolName.includes('_mcp_')` instead of checking only after the first `_action_` occurrence, which missed MCP tools with `_action_` in the middle of their name (e.g. `get_action_data_mcp_myserver`). - Reference `Constants.mcp_delimiter` value via a local const to avoid circular import from config.ts, with a comment explaining why. - Remove dead `actionDelimiter` import from definitions.ts. - Replace double-filter with single-pass partition in loadToolsForExecution. - Add test for mid-name `_action_` collision case. * fix: narrow MCP exclusion to delimiter position in isActionTool Only reject when `_mcp_` appears after `_action_` (the MCP suffix position). `_mcp_` before `_action_` is part of the operationId and is valid — e.g. `sync_mcp_state_action_api---example---com` is a legitimate action tool whose operationId happens to contain `_mcp_`. * fix: document positional _mcp_ guard and known RFC-invalid domain limitation Expand JSDoc on isActionTool to explain the action/MCP format disambiguation and the theoretical false negative for non-RFC-compliant domains containing `_mcp_`. Add test documenting this known edge case. --- api/server/services/ToolService.js | 14 +++-- .../services/__tests__/ToolService.spec.js | 56 +++++++++++++++++++ .../Messages/Content/ToolOutput/ToolIcon.tsx | 4 +- .../Content/__tests__/ToolIcon.test.tsx | 6 ++ .../SidePanel/Builder/AssistantPanel.tsx | 4 +- packages/api/src/tools/definitions.spec.ts | 33 +++++++++++ packages/api/src/tools/definitions.ts | 4 +- .../data-provider/src/types/assistants.ts | 29 ++++++++++ 8 files changed, 139 insertions(+), 11 deletions(-) diff --git a/api/server/services/ToolService.js b/api/server/services/ToolService.js index c11843cb69..b4d948eda4 100644 --- a/api/server/services/ToolService.js +++ b/api/server/services/ToolService.js @@ -31,6 +31,7 @@ const { imageGenTools, EModelEndpoint, EToolResources, + isActionTool, actionDelimiter, ImageVisionTool, openapiToFunction, @@ -490,7 +491,7 @@ async function loadToolDefinitionsWrapper({ req, res, agent, streamId = null, to if (tool === Tools.web_search) { return checkCapability(AgentCapabilities.web_search); } - if (tool.includes(actionDelimiter)) { + if (isActionTool(tool)) { return actionsEnabled; } if (!areToolsEnabled) { @@ -871,7 +872,7 @@ async function loadAgentTools({ } else if (tool === Tools.web_search) { includesWebSearch = checkCapability(AgentCapabilities.web_search); return includesWebSearch; - } else if (tool.includes(actionDelimiter)) { + } else if (isActionTool(tool)) { return actionsEnabled; } else if (!areToolsEnabled) { return false; @@ -978,7 +979,7 @@ async function loadAgentTools({ agentTools.push(...additionalTools); - const hasActionTools = _agentTools.some((t) => t.includes(actionDelimiter)); + const hasActionTools = _agentTools.some((t) => isActionTool(t)); if (!hasActionTools) { return { toolRegistry, @@ -1237,8 +1238,11 @@ async function loadToolsForExecution({ ? [...new Set([...requestedNonSpecialToolNames, ...ptcOrchestratedToolNames])] : requestedNonSpecialToolNames; - const actionToolNames = allToolNamesToLoad.filter((name) => name.includes(actionDelimiter)); - const regularToolNames = allToolNamesToLoad.filter((name) => !name.includes(actionDelimiter)); + const actionToolNames = []; + const regularToolNames = []; + for (const name of allToolNamesToLoad) { + (isActionTool(name) ? actionToolNames : regularToolNames).push(name); + } if (regularToolNames.length > 0) { const includesWebSearch = regularToolNames.includes(Tools.web_search); diff --git a/api/server/services/__tests__/ToolService.spec.js b/api/server/services/__tests__/ToolService.spec.js index 6e06804280..740bb06e5a 100644 --- a/api/server/services/__tests__/ToolService.spec.js +++ b/api/server/services/__tests__/ToolService.spec.js @@ -2,6 +2,7 @@ const { Tools, Constants, EModelEndpoint, + isActionTool, actionDelimiter, AgentCapabilities, defaultAgentCapabilities, @@ -143,6 +144,42 @@ describe('ToolService - Action Capability Gating', () => { }); }); + describe('isActionTool — cross-delimiter collision guard', () => { + it('should identify real action tools', () => { + expect(isActionTool(`get_weather${actionDelimiter}api_example_com`)).toBe(true); + expect(isActionTool(`fetch_data${actionDelimiter}my---domain---com`)).toBe(true); + }); + + it('should identify action tools whose operationId contains _mcp_', () => { + expect(isActionTool(`sync_mcp_state${actionDelimiter}api---example---com`)).toBe(true); + expect(isActionTool(`get_mcp_config${actionDelimiter}internal---api---com`)).toBe(true); + }); + + it('should reject MCP tools whose name ends with _action', () => { + expect(isActionTool(`get_action${Constants.mcp_delimiter}myserver`)).toBe(false); + expect(isActionTool(`fetch_action${Constants.mcp_delimiter}server_name`)).toBe(false); + expect(isActionTool(`retrieve_action${Constants.mcp_delimiter}srv`)).toBe(false); + }); + + it('should reject MCP tools with _action_ in the middle of their name', () => { + expect(isActionTool(`get_action_data${Constants.mcp_delimiter}myserver`)).toBe(false); + expect(isActionTool(`create_action_item${Constants.mcp_delimiter}server`)).toBe(false); + }); + + it('should reject tools without the action delimiter', () => { + expect(isActionTool('calculator')).toBe(false); + expect(isActionTool(`web_search${Constants.mcp_delimiter}myserver`)).toBe(false); + }); + + it('known limitation: non-RFC domain with _mcp_ substring yields false negative', () => { + // RFC 952/1123 prohibit underscores in hostnames, so this is not expected in practice. + // Encoded domain `api_mcp_internal_com` places `_mcp_` after `_action_`, which + // the guard interprets as the MCP suffix. + const edgeCaseTool = `getData${actionDelimiter}api_mcp_internal_com`; + expect(isActionTool(edgeCaseTool)).toBe(false); + }); + }); + describe('loadAgentTools (definitionsOnly=true) — action tool filtering', () => { const actionToolName = `get_weather${actionDelimiter}api_example_com`; const regularTool = 'calculator'; @@ -183,6 +220,25 @@ describe('ToolService - Action Capability Gating', () => { expect(callArgs.tools).toContain(actionToolName); }); + it('should not filter MCP tools whose name contains _action (cross-delimiter collision)', async () => { + const mcpToolWithAction = `get_action${Constants.mcp_delimiter}myserver`; + const capabilities = [AgentCapabilities.tools]; + const req = createMockReq(capabilities); + mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities)); + + await loadAgentTools({ + req, + res: {}, + agent: { id: 'agent_123', tools: [regularTool, mcpToolWithAction] }, + definitionsOnly: true, + }); + + expect(mockLoadToolDefinitions).toHaveBeenCalledTimes(1); + const [callArgs] = mockLoadToolDefinitions.mock.calls[0]; + expect(callArgs.tools).toContain(mcpToolWithAction); + expect(callArgs.tools).toContain(regularTool); + }); + it('should return actionsEnabled in the result', async () => { const capabilities = [AgentCapabilities.tools]; const req = createMockReq(capabilities); diff --git a/client/src/components/Chat/Messages/Content/ToolOutput/ToolIcon.tsx b/client/src/components/Chat/Messages/Content/ToolOutput/ToolIcon.tsx index bc8f94c450..4484287dd6 100644 --- a/client/src/components/Chat/Messages/Content/ToolOutput/ToolIcon.tsx +++ b/client/src/components/Chat/Messages/Content/ToolOutput/ToolIcon.tsx @@ -1,4 +1,4 @@ -import { Constants, actionDelimiter } from 'librechat-data-provider'; +import { Constants, isActionTool } from 'librechat-data-provider'; import { Terminal, Globe, ImageIcon, ArrowRightLeft, FileSearch, Zap, Wrench } from 'lucide-react'; import { cn } from '~/utils'; @@ -48,7 +48,7 @@ export function getToolIconType(name: string): ToolIconType { if (name.startsWith(Constants.LC_TRANSFER_TO_)) { return 'agent_handoff'; } - if (name.includes(actionDelimiter)) { + if (isActionTool(name)) { return 'action'; } return 'generic'; diff --git a/client/src/components/Chat/Messages/Content/__tests__/ToolIcon.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/ToolIcon.test.tsx index 7faf18ad8b..4937b547c8 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/ToolIcon.test.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/ToolIcon.test.tsx @@ -12,6 +12,12 @@ describe('getToolIconType - ACTN-01: Action delimiter detection', () => { expect(getToolIconType(toolName)).toBe('mcp'); }); + it('returns "mcp" not "action" for MCP tool whose name ends with _action (cross-delimiter collision)', () => { + const toolName = `get_action${Constants.mcp_delimiter}myserver`; + expect(getToolIconType(toolName)).not.toBe('action'); + expect(getToolIconType(toolName)).toBe('mcp'); + }); + it('returns "generic" for plain tool name without delimiters', () => { expect(getToolIconType('some_plain_tool')).toBe('generic'); }); diff --git a/client/src/components/SidePanel/Builder/AssistantPanel.tsx b/client/src/components/SidePanel/Builder/AssistantPanel.tsx index c47a9aea53..588f55c92f 100644 --- a/client/src/components/SidePanel/Builder/AssistantPanel.tsx +++ b/client/src/components/SidePanel/Builder/AssistantPanel.tsx @@ -5,7 +5,7 @@ import { useForm, FormProvider, Controller, useWatch } from 'react-hook-form'; import { Tools, Capabilities, - actionDelimiter, + isActionTool, ImageVisionTool, defaultAssistantFormValues, } from 'librechat-data-provider'; @@ -139,7 +139,7 @@ export default function AssistantPanel({ const onSubmit = (data: AssistantForm) => { const tools: Array = [...functions].map((functionName) => { - if (!functionName.includes(actionDelimiter)) { + if (!isActionTool(functionName)) { return functionName; } else { const assistant = assistantMap?.[endpoint]?.[assistant_id]; diff --git a/packages/api/src/tools/definitions.spec.ts b/packages/api/src/tools/definitions.spec.ts index dc58327a2e..e7ba2f5ce9 100644 --- a/packages/api/src/tools/definitions.spec.ts +++ b/packages/api/src/tools/definitions.spec.ts @@ -119,6 +119,39 @@ describe('definitions.ts', () => { expect(actionDef?.parameters).toBeUndefined(); }); + it('should not classify MCP tools with _action in name as action tools', async () => { + const mockGetActionToolDefinitions = jest.fn(); + const mcpTool = 'get_action_mcp_myserver'; + + mockGetOrFetchMCPServerTools.mockResolvedValue({ + tools: [ + { + name: 'get_action', + description: 'Gets an action', + inputSchema: { type: 'object', properties: {} }, + }, + ], + }); + + const params: LoadToolDefinitionsParams = { + userId: 'user-123', + agentId: 'agent-123', + tools: [mcpTool], + }; + + const deps: LoadToolDefinitionsDeps = { + getOrFetchMCPServerTools: mockGetOrFetchMCPServerTools, + isBuiltInTool: mockIsBuiltInTool, + loadAuthValues: mockLoadAuthValues, + getActionToolDefinitions: mockGetActionToolDefinitions, + }; + + await loadToolDefinitions(params, deps); + + expect(mockGetActionToolDefinitions).not.toHaveBeenCalled(); + expect(mockGetOrFetchMCPServerTools).toHaveBeenCalled(); + }); + it('should not call getActionToolDefinitions when no action tools present', async () => { const mockGetActionToolDefinitions = jest.fn(); mockIsBuiltInTool.mockReturnValue(true); diff --git a/packages/api/src/tools/definitions.ts b/packages/api/src/tools/definitions.ts index 1598baee70..8ca60e9aaf 100644 --- a/packages/api/src/tools/definitions.ts +++ b/packages/api/src/tools/definitions.ts @@ -5,7 +5,7 @@ * @module packages/api/src/tools/definitions */ -import { Constants, actionDelimiter } from 'librechat-data-provider'; +import { Constants, isActionTool } from 'librechat-data-provider'; import type { AgentToolOptions } from 'librechat-data-provider'; import type { LCToolRegistry, JsonSchemaType, LCTool, GenericTool } from '@librechat/agents'; import type { ToolDefinition } from './classification'; @@ -99,7 +99,7 @@ export async function loadToolDefinitions( const mcpAllPattern = `${Constants.mcp_all}${Constants.mcp_delimiter}`; for (const toolName of tools) { - if (toolName.includes(actionDelimiter)) { + if (isActionTool(toolName)) { actionToolNames.push(toolName); continue; } diff --git a/packages/data-provider/src/types/assistants.ts b/packages/data-provider/src/types/assistants.ts index 690b2e06d2..2ee30490c3 100644 --- a/packages/data-provider/src/types/assistants.ts +++ b/packages/data-provider/src/types/assistants.ts @@ -583,6 +583,35 @@ export type TContentData = StreamContentData & { export const actionDelimiter = '_action_'; export const actionDomainSeparator = '---'; +/** Mirrors `Constants.mcp_delimiter`; duplicated here to avoid a circular import from `config.ts`. */ +const mcpDelimiter = '_mcp_'; + +/** + * Checks whether a tool name is an OpenAPI action tool. + * + * Action format: `operationId_action_normalizedDomain` + * MCP format: `toolName_mcp_serverName` + * + * Cross-delimiter collision: an MCP tool like `get_action_mcp_srv` contains + * `_action_` as a false positive. Guarded by checking whether `_mcp_` appears + * after `_action_`. In the collision case the `_mcp_` suffix always follows + * `_action_`; in a valid action tool whose operationId contains `_mcp_`, the + * `_mcp_` precedes `_action_`. + * + * Theoretical limitation: a non-RFC-compliant domain containing literal + * underscores that form `_mcp_` (e.g. `api_mcp_internal.com`) would produce + * a false negative. RFC 952/1123 prohibit underscores in hostnames, so this + * is not expected in practice. + */ +export function isActionTool(toolName: string): boolean { + const actionIdx = toolName.indexOf(actionDelimiter); + if (actionIdx < 0) { + return false; + } + const mcpIdx = toolName.indexOf(mcpDelimiter); + return mcpIdx < 0 || mcpIdx < actionIdx; +} + export const hostImageIdSuffix = '_host_copy'; export const hostImageNamePrefix = 'host_copy_';