diff --git a/api/server/controllers/assistants/v1.js b/api/server/controllers/assistants/v1.js index 926ab7db4d..b27e5530b1 100644 --- a/api/server/controllers/assistants/v1.js +++ b/api/server/controllers/assistants/v1.js @@ -7,7 +7,11 @@ const validateAuthor = require('~/server/middleware/assistants/validateAuthor'); const { getStrategyFunctions } = require('~/server/services/Files/strategies'); const { deleteAssistantActions } = require('~/server/services/ActionService'); const { getOpenAIClient, fetchAssistants } = require('./helpers'); -const { healMcpToolNames, getAssistantToolDefinitions } = require('~/server/services/MCP'); +const { + healMcpToolNames, + getAssistantToolDefinitions, + toProviderToolDefinition, +} = require('~/server/services/MCP'); const { manifestToolMap, isAgentsOnlyTool } = require('~/app/clients/tools'); /** @@ -30,8 +34,16 @@ const createAssistant = async (req, res) => { delete assistantData.conversation_starters; delete assistantData.append_current_datetime; - const toolDefinitions = await getAssistantToolDefinitions({ req, tools }); - const healedTools = await healMcpToolNames({ req, tools, toolDefinitions }); + const { toolDefinitions, accessibleServerNames } = await getAssistantToolDefinitions({ + req, + tools, + }); + const healedTools = await healMcpToolNames({ + req, + tools, + toolDefinitions, + accessibleServerNames, + }); assistantData.tools = healedTools .map((tool) => { @@ -59,7 +71,8 @@ const createAssistant = async (req, res) => { return toolDef; }) .filter((tool) => tool) - .flat(); + .flat() + .map(toProviderToolDefinition); let azureModelIdentifier = null; if (openai.locals?.azureOptions) { @@ -145,8 +158,16 @@ const patchAssistant = async (req, res) => { ...updateData } = req.body; - const toolDefinitions = await getAssistantToolDefinitions({ req, tools: updateData.tools }); - const healedTools = await healMcpToolNames({ req, tools: updateData.tools, toolDefinitions }); + const { toolDefinitions, accessibleServerNames } = await getAssistantToolDefinitions({ + req, + tools: updateData.tools, + }); + const healedTools = await healMcpToolNames({ + req, + tools: updateData.tools, + toolDefinitions, + accessibleServerNames, + }); updateData.tools = healedTools .map((tool) => { @@ -174,7 +195,8 @@ const patchAssistant = async (req, res) => { return toolDef; }) .filter((tool) => tool) - .flat(); + .flat() + .map(toProviderToolDefinition); if (openai.locals?.azureOptions && updateData.model) { updateData.model = openai.locals.azureOptions.azureOpenAIApiDeploymentName; diff --git a/api/server/controllers/assistants/v2.js b/api/server/controllers/assistants/v2.js index a436ed611d..ec0a3f9309 100644 --- a/api/server/controllers/assistants/v2.js +++ b/api/server/controllers/assistants/v2.js @@ -2,7 +2,11 @@ const { logger } = require('@librechat/data-schemas'); const { ToolCallTypes } = require('librechat-data-provider'); const validateAuthor = require('~/server/middleware/assistants/validateAuthor'); const { validateAndUpdateTool } = require('~/server/services/ActionService'); -const { healMcpToolNames, getAssistantToolDefinitions } = require('~/server/services/MCP'); +const { + healMcpToolNames, + getAssistantToolDefinitions, + toProviderToolDefinition, +} = require('~/server/services/MCP'); const { manifestToolMap, isAgentsOnlyTool } = require('~/app/clients/tools'); const { updateAssistantDoc } = require('~/models'); const { getOpenAIClient } = require('./helpers'); @@ -28,8 +32,16 @@ const createAssistant = async (req, res) => { delete assistantData.conversation_starters; delete assistantData.append_current_datetime; - const toolDefinitions = await getAssistantToolDefinitions({ req, tools }); - const healedTools = await healMcpToolNames({ req, tools, toolDefinitions }); + const { toolDefinitions, accessibleServerNames } = await getAssistantToolDefinitions({ + req, + tools, + }); + const healedTools = await healMcpToolNames({ + req, + tools, + toolDefinitions, + accessibleServerNames, + }); assistantData.tools = healedTools .map((tool) => { @@ -57,7 +69,8 @@ const createAssistant = async (req, res) => { return toolDef; }) .filter((tool) => tool) - .flat(); + .flat() + .map(toProviderToolDefinition); let azureModelIdentifier = null; if (openai.locals?.azureOptions) { @@ -134,8 +147,16 @@ const updateAssistant = async ({ req, openai, assistant_id, updateData }) => { } let hasFileSearch = false; - const toolDefinitions = await getAssistantToolDefinitions({ req, tools: updateData.tools }); - const healedTools = await healMcpToolNames({ req, tools: updateData.tools, toolDefinitions }); + const { toolDefinitions, accessibleServerNames } = await getAssistantToolDefinitions({ + req, + tools: updateData.tools, + }); + const healedTools = await healMcpToolNames({ + req, + tools: updateData.tools, + toolDefinitions, + accessibleServerNames, + }); for (const tool of healedTools) { /** Agents-runtime-only tools (e.g. ask_user_question) cannot execute on * the assistants runtime — drop them even when posted directly, since @@ -201,7 +222,7 @@ const updateAssistant = async ({ req, openai, assistant_id, updateData }) => { }; } - updateData.tools = tools; + updateData.tools = tools.map(toProviderToolDefinition); if (openai.locals?.azureOptions && updateData.model) { updateData.model = openai.locals.azureOptions.azureOpenAIApiDeploymentName; diff --git a/api/server/controllers/mcp.js b/api/server/controllers/mcp.js index 1eb1b6eb3d..19ea8d90b5 100644 --- a/api/server/controllers/mcp.js +++ b/api/server/controllers/mcp.js @@ -302,6 +302,10 @@ const getMCPTools = async (req, res) => { name: toolName, pluginKey: toolKey, description: toolData.function.description || '', + /** Upstream identity for keys that stripped a redundant + * server-name prefix — the agent editor migrates legacy + * persisted ids only when this proves the same tool. */ + ...(toolData.serverToolName != null && { serverToolName: toolData.serverToolName }), }); } } diff --git a/api/server/services/Config/__tests__/getCachedTools.lock.spec.js b/api/server/services/Config/__tests__/getCachedTools.lock.spec.js index 6302d20cad..50e99ecfd0 100644 --- a/api/server/services/Config/__tests__/getCachedTools.lock.spec.js +++ b/api/server/services/Config/__tests__/getCachedTools.lock.spec.js @@ -248,7 +248,7 @@ describe('global tool cache write lock', () => { expect.objectContaining({ keys: [ `${CacheKeys.TOOL_CACHE}:tools:metadata:mcp:user-generation:{user-1:server-1}`, - `${CacheKeys.TOOL_CACHE}:tools:mcp:user:{user-1:server-1}:config-current`, + `${CacheKeys.TOOL_CACHE}:tools:mcp:user:{user-1:server-1}:v2:config-current`, ], arguments: [ 'generation-current', @@ -342,7 +342,7 @@ describe('global tool cache write lock', () => { `tools:mcp:write-fence:{user-1:server-1}`, `${CacheKeys.TOOL_CACHE}:tools:metadata:mcp:user-legacy-fence:{user-1:server-1}`, `${CacheKeys.TOOL_CACHE}:tools:metadata:mcp:user-generation:{user-1:server-1}`, - `${CacheKeys.TOOL_CACHE}:tools:mcp:user:{user-1:server-1}:config-current`, + `${CacheKeys.TOOL_CACHE}:tools:mcp:user:{user-1:server-1}:v2:config-current`, ], }), ); diff --git a/api/server/services/Config/__tests__/getCachedTools.spec.js b/api/server/services/Config/__tests__/getCachedTools.spec.js index 6d3947392f..dd5e231f02 100644 --- a/api/server/services/Config/__tests__/getCachedTools.spec.js +++ b/api/server/services/Config/__tests__/getCachedTools.spec.js @@ -29,10 +29,10 @@ describe('MCP tool cache', () => { it('uses collision-safe configuration-addressed keys', () => { expect(ToolCacheKeys.MCP_APP_SERVER('server:name', 'config/a')).toBe( - 'tools:mcp:app:server%3Aname:config%2Fa', + 'tools:mcp:app:v2:server%3Aname:config%2Fa', ); expect(ToolCacheKeys.MCP_SERVER('tenant:user', 'server:name', 'config/a')).toBe( - 'tools:mcp:user:{tenant%3Auser:server%3Aname}:config%2Fa', + 'tools:mcp:user:{tenant%3Auser:server%3Aname}:v2:config%2Fa', ); expect(ToolCacheKeys.MCP_SERVER('tenant:user', 'server:name', 'config/a')).not.toBe( ToolCacheKeys.MCP_SERVER('tenant', 'user:server:name', 'config/a'), @@ -49,7 +49,7 @@ describe('MCP tool cache', () => { }); it('keeps the legacy user key available for non-generation callers', () => { - expect(ToolCacheKeys.MCP_SERVER('user123', 'github')).toBe('tools:mcp:user123:github'); + expect(ToolCacheKeys.MCP_SERVER('user123', 'github')).toBe('tools:mcp:v2:user123:github'); }); it('gets and sets static global tools without touching MCP slices', async () => { diff --git a/api/server/services/MCP.js b/api/server/services/MCP.js index 0c351996fa..15bbecc025 100644 --- a/api/server/services/MCP.js +++ b/api/server/services/MCP.js @@ -10,9 +10,12 @@ const { splitMCPToolKey, normalizeServerName, normalizeMCPToolKey, + stripServerNamePrefix, + stripServerNamePrefixes, buildServerNameAliases, findShadowedServerNames, getAssistantToolDefinitions: loadAssistantToolDefinitions, + toProviderToolDefinition, resolveMCPServerContext, normalizeJsonSchema, GenerationJobManager, @@ -216,8 +219,10 @@ async function resolveMcpServerContext(req) { */ /** * Names of every MCP server the user can reach (operator config + user DB), - * for the legacy-key heal's collision detection in `initializeAgent`. Only - * consulted when a configured server name needs normalization. + * for legacy-key healing: collision detection in `initializeAgent` (consulted + * when a configured server name needs normalization) and the assistants heal + * in `healMcpToolNames` (always, since assistants reference user-owned + * servers too). * @param {string} [userId] * @param {string} [role] * @returns {Promise} @@ -251,7 +256,7 @@ async function getAccessibleMcpServerNames(userId, role) { * @param {Record} params.toolDefinitions * @returns {Promise>} */ -async function healMcpToolNames({ req, tools, toolDefinitions }) { +async function healMcpToolNames({ req, tools, toolDefinitions, accessibleServerNames }) { const list = tools ?? []; const needsHeal = list.some( (tool) => @@ -262,21 +267,36 @@ async function healMcpToolNames({ req, tools, toolDefinitions }) { if (!needsHeal) { return list; } - const rawServerNames = await resolveMcpConfigNames(req); /** Cross-tier shadowing (DB `foo` vs operator `foo!`) is invisible to * operator names alone — the shadow set must come from the FULL - * accessible audit. Every rewrite candidate here is normalization- - * sensitive by construction, so an incomplete audit skips healing - * entirely (the raw key stays raw and fails closed). */ - const audit = await resolveCollisionAuditNames({ - rawServerNames, - userId: req.user?.id, - role: req.user?.role, - }); - if (!audit.complete) { - return list; + * accessible audit: assistants reference user-owned servers too (the + * definitions loader resolves them), so their pre-strip keys must heal + * against the same catalog. Callers holding the loader's snapshot pass + * it to avoid repeating the app-config and registry reads on the write + * path; without one, the audit is fetched here, and when it cannot + * complete healing is skipped entirely (the raw key stays raw and fails + * closed). */ + let auditNames = accessibleServerNames; + if (auditNames == null) { + const rawServerNames = await resolveMcpConfigNames(req); + try { + const accessible = await getAccessibleMcpServerNames(req.user?.id, req.user?.role); + auditNames = [...new Set([...accessible, ...rawServerNames])]; + } catch (error) { + logger.warn( + '[healMcpToolNames] Accessible-server audit unavailable; skipping legacy-key healing:', + error, + ); + return list; + } } - const shadowed = findShadowedServerNames(audit.names); + const shadowed = findShadowedServerNames(auditNames); + /** A pre-strip key persisted AFTER server-name normalization carries the + * NORMALIZED suffix, which the raw config names cannot match — the + * boundary must resolve against both spellings and map back to the raw + * name for the shadow and membership guards. */ + const serverNameAliases = buildServerNameAliases(auditNames); + const boundaryNames = [...new Set([...auditNames, ...serverNameAliases.keys()])]; const seen = new Set(); const healedList = []; for (const tool of list) { @@ -286,15 +306,47 @@ async function healMcpToolNames({ req, tools, toolDefinitions }) { tool.includes(Constants.mcp_delimiter) && toolDefinitions[tool] == null ) { - const [, parsedServerName] = splitMCPToolKey(tool, rawServerNames); - if ( - parsedServerName != null && - rawServerNames.includes(parsedServerName) && - !shadowed.has(parsedServerName) - ) { - const healed = normalizeMCPToolKey(tool, rawServerNames); + const [, parsedServerName] = splitMCPToolKey(tool, boundaryNames); + let rawServerName; + if (parsedServerName != null && auditNames.includes(parsedServerName)) { + rawServerName = parsedServerName; + } else if (parsedServerName != null) { + const aliased = serverNameAliases.get(parsedServerName); + /** A normalized spelling on a CONTESTED slot is ambiguous between the + * tie-break winner and its shadowed rivals — rewriting persisted + * data must fail closed here, mirroring the raw-spelling shadow + * guard, rather than bind the reference to the winner. */ + const contested = + aliased != null && + auditNames.some( + (name) => name !== aliased && normalizeServerName(name) === parsedServerName, + ); + rawServerName = contested ? undefined : aliased; + } + if (rawServerName != null && !shadowed.has(rawServerName)) { + const healed = normalizeMCPToolKey(tool, auditNames); if (toolDefinitions[healed] != null) { healedTool = healed; + } else { + /** Catalog keys built after redundant-prefix stripping no longer + * match a pre-strip persisted key — without this second candidate + * the exact-lookup below silently drops the tool from the + * assistant. The rewrite only lands when the stripped key actually + * exists in the loaded definitions, so an unstripped catalog + * (collision guard kept the raw name) never heals into a phantom. */ + const keyServerName = normalizeServerName(rawServerName); + const [healedToolName] = splitMCPToolKey(healed, [keyServerName]); + const strippedName = stripServerNamePrefix(healedToolName, keyServerName); + const strippedKey = `${strippedName}${Constants.mcp_delimiter}${keyServerName}`; + /** Rewrite only when the stripped entry PROVES the same upstream + * identity — a stale key for a removed tool must not be healed + * onto a different sibling that kept its raw name. */ + if ( + strippedName !== healedToolName && + toolDefinitions[strippedKey]?.serverToolName === healedToolName + ) { + healedTool = strippedKey; + } } } } @@ -807,6 +859,11 @@ async function createMCPTools({ } const serverTools = []; + const keyServerName = normalizeServerName(serverName); + const keyToolNames = stripServerNamePrefixes( + result.tools.map((tool) => tool.name), + keyServerName, + ); for (const tool of result.tools) { const toolInstance = await createMCPTool({ res, @@ -821,7 +878,7 @@ async function createMCPTools({ serverName, /** Model-facing key: matches the normalized `availableTools` keys and * the instance name `createToolInstance` will assign. */ - toolKey: `${tool.name}${Constants.mcp_delimiter}${normalizeServerName(serverName)}`, + toolKey: `${keyToolNames.get(tool.name) ?? tool.name}${Constants.mcp_delimiter}${keyServerName}`, requestBody, requestScopedConnections, config: serverConfig, @@ -936,18 +993,49 @@ async function createMCPTool({ /** Legacy keys persisted pre-normalization (assistants, direct tool * calls) carry the RAW server name, while `availableTools` is keyed by - * the canonical normalized key — look up both spellings. */ + * the canonical normalized key — look up both spellings. Keys are also + * built after redundant server-name-prefix stripping now, so a persisted + * pre-strip key (`acme_foo_mcp_acme`) must additionally try + * its stripped spelling or the tool degrades to an unavailable stub. */ + const keyServerName = serverName != null ? normalizeServerName(serverName) : undefined; const canonicalToolKey = - serverName != null - ? `${toolName}${Constants.mcp_delimiter}${normalizeServerName(serverName)}` - : toolKey; - const findToolDefinition = (tools) => - tools?.[toolKey]?.function ?? - (canonicalToolKey !== toolKey ? tools?.[canonicalToolKey]?.function : undefined); + keyServerName != null ? `${toolName}${Constants.mcp_delimiter}${keyServerName}` : toolKey; + const strippedToolName = + keyServerName != null ? stripServerNamePrefix(toolName, keyServerName) : toolName; + const strippedToolKey = + strippedToolName !== toolName + ? `${strippedToolName}${Constants.mcp_delimiter}${keyServerName}` + : null; + const candidateToolKeys = [toolKey]; + if (canonicalToolKey !== toolKey) { + candidateToolKeys.push(canonicalToolKey); + } + if (strippedToolKey != null && !candidateToolKeys.includes(strippedToolKey)) { + candidateToolKeys.push(strippedToolKey); + } + let matchedToolKey = toolKey; + const findToolEntry = (tools) => { + for (const key of candidateToolKeys) { + const entry = tools?.[key]; + if (!entry?.function) { + continue; + } + /** The stripped-spelling candidate is only a legacy match when the + * entry PROVES the same upstream identity — without this, a stale + * reference to a removed tool could strip onto a DIFFERENT sibling + * that kept its raw name and silently call the wrong tool. */ + if (key === strippedToolKey && entry.serverToolName !== toolName) { + continue; + } + matchedToolKey = key; + return entry; + } + return undefined; + }; - /** @type {LCTool | undefined} */ - let toolDefinition = findToolDefinition(availableTools); - if (!toolDefinition) { + /** @type {LCFunctionTool | undefined} */ + let toolEntry = findToolEntry(availableTools); + if (!toolEntry) { const cachedAt = useMissingToolCache ? missingToolCache.get(toolKey) : undefined; if (cachedAt && Date.now() - cachedAt < MISSING_TOOL_TTL_MS) { logger.debug( @@ -976,15 +1064,15 @@ async function createMCPTool({ if (result?.availableTools) { onAvailableTools?.(result.availableTools); } - toolDefinition = findToolDefinition(result?.availableTools); + toolEntry = findToolEntry(result?.availableTools); - if (!toolDefinition && useMissingToolCache) { + if (!toolEntry && useMissingToolCache) { missingToolCache.set(toolKey, Date.now()); evictStale(missingToolCache, MISSING_TOOL_TTL_MS); } } - if (!toolDefinition) { + if (!toolEntry) { logger.warn( `[MCP][${serverName}][${toolName}] Tool definition not found, returning unavailable stub.`, ); @@ -998,10 +1086,20 @@ async function createMCPTool({ requestBody, requestScopedConnections, provider, + /** A legacy pre-strip key that resolves to the stripped entry KEEPS its + * persisted spelling as the instance name: `agent.tools` entries and + * `tool_options` keys reference that spelling, and renaming the instance + * would silently detach those per-tool settings. The upstream call name + * still comes from the MATCHED entry — its recorded raw name, or the + * matched key's own tool half when the entry was never stripped. */ toolName, + serverToolName: + toolEntry.serverToolName ?? + (matchedToolKey === strippedToolKey ? strippedToolName : toolName), + currentToolName: matchedToolKey === strippedToolKey ? strippedToolName : undefined, serverName, serverConfig, - toolDefinition, + toolDefinition: toolEntry['function'], streamId, jobCreatedAt, }); @@ -1014,6 +1112,8 @@ function createToolInstance({ requestBody: capturedRequestBody, requestScopedConnections: capturedRequestScopedConnections, toolName, + serverToolName = toolName, + currentToolName, serverName, serverConfig: capturedServerConfig, toolDefinition, @@ -1091,7 +1191,9 @@ function createToolInstance({ const result = await mcpManager.callTool({ serverName, serverConfig: capturedServerConfig, - toolName, + /** The upstream server never sees stripped names — a key that dropped + * a redundant server-name prefix calls the ORIGINAL tool. */ + toolName: serverToolName, provider, toolArguments, options: { @@ -1170,6 +1272,17 @@ function createToolInstance({ }); toolInstance.mcp = true; toolInstance.mcpRawServerName = serverName; + if (serverToolName !== toolName) { + /** Upstream identity for stripped keys — lets the options aliasing in + * `buildToolClassification` heal legacy `tool_options` spellings. */ + toolInstance.mcpServerToolName = serverToolName; + } + if (currentToolName != null && currentToolName !== toolName) { + /** Current catalog spelling for a LEGACY-named instance, so approval + * policies and hook matchers written against the current name still + * reach it (see `collectMCPToolAliases`). */ + toolInstance.mcpCurrentToolName = currentToolName; + } // Ephemeral request-scoped servers (runtime body placeholders) tear their // connection down at request end, so they must never be backgrounded. A // missing/stale config means the server's lifetime is unknowable, so fail @@ -1430,6 +1543,7 @@ async function getServerConnectionStatus( module.exports = { createMCPTool, createMCPTools, + toProviderToolDefinition, createMCPPermissionContext, userCanUseMCPServers, getMCPSetupData, diff --git a/api/server/services/MCP.spec.js b/api/server/services/MCP.spec.js index 976e6f81e8..c73fa2968b 100644 --- a/api/server/services/MCP.spec.js +++ b/api/server/services/MCP.spec.js @@ -1830,6 +1830,146 @@ describe('User parameter passing tests', () => { expect(mockReinitMCPServer).not.toHaveBeenCalled(); }); + it('rejects a stripped-spelling entry without matching upstream identity', async () => { + /** A stale key for a removed tool must degrade to the unavailable stub, + * not resolve onto a DIFFERENT sibling whose key coincides with the + * stripped spelling. */ + const mockUser = { id: 'stale-identity-user', role: 'USER' }; + const mockRes = { write: jest.fn(), flush: jest.fn() }; + mockReinitMCPServer.mockResolvedValue(null); + + const staleKey = `acme_acme_foo${D}acme`; + const mcpTool = await createMCPTool({ + res: mockRes, + user: mockUser, + toolKey: staleKey, + provider: 'openai', + userMCPAuthMap: {}, + availableTools: { + [`acme_foo${D}acme`]: { + function: { + name: `acme_foo${D}acme`, + description: 'Different tool', + parameters: { type: 'object', properties: {} }, + }, + }, + }, + }); + + expect(mockReinitMCPServer).toHaveBeenCalled(); + expect(mcpTool.description).toBe( + "This tool's MCP server is temporarily unavailable. Please try again shortly.", + ); + }); + + it('sends the raw upstream tool name when the key stripped a redundant server-name prefix', async () => { + const mockUser = { id: 'stripped-prefix-user', role: 'USER' }; + const mockRes = { write: jest.fn(), flush: jest.fn() }; + const { getRoleByName } = require('~/models'); + getRoleByName.mockResolvedValue({ + permissions: { + [PermissionTypes.MCP_SERVERS]: { + [Permissions.USE]: true, + }, + }, + }); + const callTool = jest.fn().mockResolvedValue(['ok', null]); + mockGetMCPManager.mockReturnValue({ callTool }); + + const strippedKey = `trace_top_time_consuming_operations${D}acme`; + const mcpTool = await createMCPTool({ + res: mockRes, + user: mockUser, + toolKey: strippedKey, + provider: 'openai', + userMCPAuthMap: {}, + availableTools: { + [strippedKey]: { + serverToolName: 'acme_trace_top_time_consuming_operations', + function: { + name: strippedKey, + description: 'Trace', + parameters: { type: 'object', properties: {} }, + }, + }, + }, + }); + + await mcpTool.invoke( + {}, + { + configurable: { user: mockUser }, + metadata: { provider: 'openai', thread_id: 'thread-1', run_id: 'run-1' }, + toolCall: {}, + }, + ); + + expect(mcpTool.name).toBe(strippedKey); + expect(callTool).toHaveBeenCalledWith( + expect.objectContaining({ + serverName: 'acme', + toolName: 'acme_trace_top_time_consuming_operations', + }), + ); + }); + + it('resolves a legacy pre-strip tool key to the stripped definition without reinit', async () => { + const mockUser = { id: 'legacy-prefix-user', role: 'USER' }; + const mockRes = { write: jest.fn(), flush: jest.fn() }; + const { getRoleByName } = require('~/models'); + getRoleByName.mockResolvedValue({ + permissions: { + [PermissionTypes.MCP_SERVERS]: { + [Permissions.USE]: true, + }, + }, + }); + const callTool = jest.fn().mockResolvedValue(['ok', null]); + mockGetMCPManager.mockReturnValue({ callTool }); + + const strippedKey = `trace_top_time_consuming_operations${D}acme`; + const legacyKey = `acme_trace_top_time_consuming_operations${D}acme`; + const mcpTool = await createMCPTool({ + res: mockRes, + user: mockUser, + toolKey: legacyKey, + provider: 'openai', + userMCPAuthMap: {}, + availableTools: { + [strippedKey]: { + serverToolName: 'acme_trace_top_time_consuming_operations', + function: { + name: strippedKey, + description: 'Trace', + parameters: { type: 'object', properties: {} }, + }, + }, + }, + }); + + expect(mockReinitMCPServer).not.toHaveBeenCalled(); + + await mcpTool.invoke( + {}, + { + configurable: { user: mockUser }, + metadata: { provider: 'openai', thread_id: 'thread-1', run_id: 'run-1' }, + toolCall: {}, + }, + ); + + /** The persisted spelling stays the instance name so `agent.tools` and + * `tool_options` keyed by it keep applying; only the upstream call + * uses the recorded raw name. */ + expect(mcpTool.name).toBe(legacyKey); + expect(callTool).toHaveBeenCalledWith( + expect.objectContaining({ + serverName: 'acme', + toolName: 'acme_trace_top_time_consuming_operations', + }), + ); + }); + it('should reject tool execution when user lacks MCP server use permission', async () => { const mockUser = { id: 'mcp-denied-user', role: 'USER' }; const mockRes = { write: jest.fn(), flush: jest.fn() }; diff --git a/api/server/services/ToolService.js b/api/server/services/ToolService.js index f03b6e8933..81aae5aef9 100644 --- a/api/server/services/ToolService.js +++ b/api/server/services/ToolService.js @@ -1021,7 +1021,7 @@ async function loadToolDefinitionsWrapper({ return definitions; }; - let { toolDefinitions, toolRegistry, hasDeferredTools, mcpResolution } = + let { toolDefinitions, toolRegistry, hasDeferredTools, mcpToolAliases, mcpResolution } = await loadToolDefinitions( { userId: req.user.id, @@ -1134,6 +1134,7 @@ async function loadToolDefinitionsWrapper({ toolDefinitions = reloadResult.toolDefinitions; toolRegistry = reloadResult.toolRegistry; hasDeferredTools = reloadResult.hasDeferredTools; + mcpToolAliases = reloadResult.mcpToolAliases; mcpResolution = reloadResult.mcpResolution; } } @@ -1242,6 +1243,7 @@ async function loadToolDefinitionsWrapper({ dynamicToolContextMap, toolDefinitions, hasDeferredTools, + mcpToolAliases, actionsEnabled, primedCodeFiles, }; @@ -1434,7 +1436,7 @@ async function loadAgentTools({ /** Build tool registry from MCP tools and create PTC/tool search tools if configured */ const deferredToolsEnabled = checkCapability(AgentCapabilities.deferred_tools); const programmaticToolsEnabled = enabledCapabilities.has(AgentCapabilities.programmatic_tools); - const { toolRegistry, toolDefinitions, additionalTools, hasDeferredTools } = + const { toolRegistry, toolDefinitions, additionalTools, hasDeferredTools, mcpToolAliases } = await buildToolClassification({ loadedTools, userId: req.user.id, @@ -1504,6 +1506,7 @@ async function loadAgentTools({ dynamicToolContextMap, toolDefinitions, hasDeferredTools, + mcpToolAliases, actionsEnabled, tools: agentTools, primedCodeFiles, @@ -1523,6 +1526,7 @@ async function loadAgentTools({ dynamicToolContextMap, toolDefinitions, hasDeferredTools, + mcpToolAliases, actionsEnabled, tools: agentTools, primedCodeFiles, @@ -1653,6 +1657,7 @@ async function loadAgentTools({ userMCPAuthMap, toolDefinitions, hasDeferredTools, + mcpToolAliases, actionsEnabled, tools: agentTools, primedCodeFiles, diff --git a/api/server/services/__tests__/MCP.spec.js b/api/server/services/__tests__/MCP.spec.js index aa4ee0c080..0fae7a3058 100644 --- a/api/server/services/__tests__/MCP.spec.js +++ b/api/server/services/__tests__/MCP.spec.js @@ -115,8 +115,11 @@ describe('getAssistantToolDefinitions', () => { }); expect(definitions).toEqual({ - code_interpreter: { type: 'code_interpreter' }, - [toolKey]: mcpDefinition, + toolDefinitions: { + code_interpreter: { type: 'code_interpreter' }, + [toolKey]: mcpDefinition, + }, + accessibleServerNames: ['app-server'], }); expect(getMCPServerTools).toHaveBeenCalledWith('u1', 'app-server', serverConfig); }); @@ -135,7 +138,8 @@ describe('getAssistantToolDefinitions', () => { require('~/config').getMCPManager.mockReturnValue({ getServerToolFunctionsSnapshot }); await expect(getAssistantToolDefinitions({ req, tools: [toolKey] })).resolves.toEqual({ - [toolKey]: mcpDefinition, + toolDefinitions: { [toolKey]: mcpDefinition }, + accessibleServerNames: ['app-server'], }); expect(cacheMCPServerTools).toHaveBeenCalledWith({ userId: 'u1', @@ -159,7 +163,8 @@ describe('getAssistantToolDefinitions', () => { reinitMCPServer.mockResolvedValue({ availableTools: { [toolKey]: mcpDefinition } }); await expect(getAssistantToolDefinitions({ req, tools: [toolKey] })).resolves.toEqual({ - [toolKey]: mcpDefinition, + toolDefinitions: { [toolKey]: mcpDefinition }, + accessibleServerNames: ['app-server'], }); expect(reinitMCPServer).toHaveBeenCalledWith({ user: req.user, @@ -398,6 +403,140 @@ describe('healMcpToolNames', () => { expect(healed).toEqual([`search${Constants.mcp_delimiter}foo!`]); }); + it('heals a pre-strip prefixed key to the stripped catalog key', async () => { + /** Catalog keys drop a redundant leading server-name prefix now; an + * assistant saved before that resubmits the prefixed key and the exact + * lookup would silently drop the tool. */ + getAppConfig.mockResolvedValue({ mcpConfig: { acme: {} } }); + mockRegistry.ensureConfigServers.mockResolvedValue({}); + mockRegistry.getAllServerConfigs.mockResolvedValue({ acme: {} }); + const strippedKey = `search${Constants.mcp_delimiter}acme`; + const toolDefinitions = { + [strippedKey]: { type: 'function', serverToolName: 'acme_search' }, + }; + + const healed = await healMcpToolNames({ + req, + tools: [`acme_search${Constants.mcp_delimiter}acme`], + toolDefinitions, + }); + + expect(healed).toEqual([strippedKey]); + }); + + it('heals a pre-strip key whose server suffix is already normalized', async () => { + /** Keys persisted after server-name normalization carry the NORMALIZED + * suffix, which the raw config names cannot match — the strip heal must + * resolve the boundary against both spellings. */ + getAppConfig.mockResolvedValue({ mcpConfig: { 'My Server': {} } }); + mockRegistry.ensureConfigServers.mockResolvedValue({}); + mockRegistry.getAllServerConfigs.mockResolvedValue({ 'My Server': {} }); + const strippedKey = `search${Constants.mcp_delimiter}My_Server`; + const toolDefinitions = { + [strippedKey]: { type: 'function', serverToolName: 'my_server_search' }, + }; + + const healed = await healMcpToolNames({ + req, + tools: [`my_server_search${Constants.mcp_delimiter}My_Server`], + toolDefinitions, + }); + + expect(healed).toEqual([strippedKey]); + }); + + it('reuses a provided accessible-server snapshot without re-reading config', async () => { + /** The controllers pass the definitions loader's snapshot so the write + * path does not repeat the app-config and registry round trips. */ + const strippedKey = `search${Constants.mcp_delimiter}acme`; + const toolDefinitions = { + [strippedKey]: { type: 'function', serverToolName: 'acme_search' }, + }; + + const healed = await healMcpToolNames({ + req, + tools: [`acme_search${Constants.mcp_delimiter}acme`], + toolDefinitions, + accessibleServerNames: ['acme'], + }); + + expect(healed).toEqual([strippedKey]); + expect(getAppConfig).not.toHaveBeenCalled(); + expect(mockRegistry.getAllServerConfigs).not.toHaveBeenCalled(); + }); + + it('heals a pre-strip key for a USER-OWNED server absent from the operator config', async () => { + /** Assistants reference user DB servers too — the definitions loader + * resolves them, so the heal's audit must include them or the legacy + * key stays unhealed and the controllers drop the tool on edit. */ + getAppConfig.mockResolvedValue({ mcpConfig: {} }); + mockRegistry.ensureConfigServers.mockResolvedValue({}); + mockRegistry.getAllServerConfigs.mockResolvedValue({ acme: {} }); + const strippedKey = `search${Constants.mcp_delimiter}acme`; + const toolDefinitions = { + [strippedKey]: { type: 'function', serverToolName: 'acme_search' }, + }; + + const healed = await healMcpToolNames({ + req, + tools: [`acme_search${Constants.mcp_delimiter}acme`], + toolDefinitions, + }); + + expect(healed).toEqual([strippedKey]); + }); + + it('does not heal a stale key onto a sibling that lacks matching upstream identity', async () => { + /** With `acme_acme_foo` removed upstream while `acme_foo` kept its raw + * name, the stale key's stripped spelling exists but belongs to a + * DIFFERENT tool — the identity check must reject the rewrite. */ + getAppConfig.mockResolvedValue({ mcpConfig: { acme: {} } }); + mockRegistry.ensureConfigServers.mockResolvedValue({}); + mockRegistry.getAllServerConfigs.mockResolvedValue({ acme: {} }); + const staleKey = `acme_acme_foo${Constants.mcp_delimiter}acme`; + const toolDefinitions = { + [`acme_foo${Constants.mcp_delimiter}acme`]: { type: 'function' }, + [`foo${Constants.mcp_delimiter}acme`]: { type: 'function', serverToolName: 'acme_foo' }, + }; + + const healed = await healMcpToolNames({ req, tools: [staleKey], toolDefinitions }); + + expect(healed).toEqual([staleKey]); + }); + + it('fails closed on a normalized-suffix key whose slot is CONTESTED', async () => { + /** `My Server` and `My_Server!` both normalize to `My_Server`, so a + * normalized-suffix reference is ambiguous between them — rewriting + * persisted data must not bind it to the tie-break winner. */ + getAppConfig.mockResolvedValue({ mcpConfig: { 'My Server': {}, 'My_Server!': {} } }); + mockRegistry.ensureConfigServers.mockResolvedValue({}); + mockRegistry.getAllServerConfigs.mockResolvedValue({ 'My Server': {}, 'My_Server!': {} }); + const legacyKey = `my_server_search${Constants.mcp_delimiter}My_Server`; + const toolDefinitions = { [`search${Constants.mcp_delimiter}My_Server`]: { type: 'function' } }; + + const healed = await healMcpToolNames({ req, tools: [legacyKey], toolDefinitions }); + + expect(healed).toEqual([legacyKey]); + }); + + it('keeps a prefixed key whose stripped spelling is not in the loaded definitions', async () => { + /** When the catalog kept the raw name (bare-sibling collision), the + * prefixed key IS canonical and must not be rewritten into a key owned + * by the bare tool. */ + getAppConfig.mockResolvedValue({ mcpConfig: { acme: {} } }); + mockRegistry.ensureConfigServers.mockResolvedValue({}); + mockRegistry.getAllServerConfigs.mockResolvedValue({ acme: {} }); + const prefixedKey = `acme_search${Constants.mcp_delimiter}acme`; + const toolDefinitions = { + [prefixedKey]: { type: 'function' }, + [`search${Constants.mcp_delimiter}acme`]: { type: 'function' }, + }; + + const healed = await healMcpToolNames({ req, tools: [prefixedKey], toolDefinitions }); + + expect(healed).toEqual([prefixedKey]); + }); + it('skips the config read entirely when every delimiter-bearing name resolves', async () => { const key = `search${Constants.mcp_delimiter}srv`; const healed = await healMcpToolNames({ diff --git a/client/src/components/SidePanel/Agents/Tools/ItemDialog/sections/McpSection.tsx b/client/src/components/SidePanel/Agents/Tools/ItemDialog/sections/McpSection.tsx index d03ed99630..95248599d6 100644 --- a/client/src/components/SidePanel/Agents/Tools/ItemDialog/sections/McpSection.tsx +++ b/client/src/components/SidePanel/Agents/Tools/ItemDialog/sections/McpSection.tsx @@ -8,6 +8,7 @@ import { splitMCPToolKey, normalizeServerName, buildServerNameAliases, + stripServerNamePrefix, } from 'librechat-data-provider'; import type { MouseEvent } from 'react'; import type { TranslationKeys } from '~/hooks/useLocalize'; @@ -136,7 +137,7 @@ export default function McpSection({ item }: Props) { * runtime heal keeps them active, and per-tool updates could never replace * the legacy entry. Tokens and other servers' entries pass through. */ - const toCurrentToolId = useCallback( + const toNormalizedToolId = useCallback( (entry: string): string => { const normalizedName = normalizeServerName(serverName); if ( @@ -172,6 +173,44 @@ export default function McpSection({ item }: Props) { [serverName, serverToken, serverAllToken, mcpServersMap], ); + /** + * Second migration stage: catalog keys drop a redundant leading server-name + * prefix, so a pre-strip persisted id would show its tool unchecked and a + * per-tool toggle could silently drop it from the selection. The rewrite is + * identity-verified — it only lands when the stripped catalog entry records + * this exact raw name as its upstream tool — so a stale id for a removed + * tool can never migrate onto a different sibling. + */ + /** Constant-time lookups for the migration below — the form heal calls it + * per persisted key, so linear catalog scans go O(options × tools). */ + const toolsById = useMemo(() => new Map(tools.map((tool) => [tool.tool_id, tool])), [tools]); + + const toStrippedToolId = useCallback( + (entry: string): string => { + if (entry === serverToken || entry === serverAllToken || toolsById.has(entry)) { + return entry; + } + const normalizedName = normalizeServerName(serverName); + const [toolPart, parsed] = splitMCPToolKey(entry, [normalizedName]); + if (parsed !== normalizedName) { + return entry; + } + const strippedPart = stripServerNamePrefix(toolPart, normalizedName); + if (strippedPart === toolPart) { + return entry; + } + const strippedId = `${strippedPart}${Constants.mcp_delimiter}${normalizedName}`; + const target = toolsById.get(strippedId); + return target?.metadata.serverToolName === toolPart ? strippedId : entry; + }, + [serverName, serverToken, serverAllToken, toolsById], + ); + + const toCurrentToolId = useCallback( + (entry: string): string => toStrippedToolId(toNormalizedToolId(entry)), + [toNormalizedToolId, toStrippedToolId], + ); + const isServerSelection = useCallback( (token: string): boolean => { const allServerNames = Array.from(new Set([...mcpServersMap.keys(), serverName])); diff --git a/packages/api/src/agents/hitl/policy.spec.ts b/packages/api/src/agents/hitl/policy.spec.ts index 1a43846d54..e95f3b034a 100644 --- a/packages/api/src/agents/hitl/policy.spec.ts +++ b/packages/api/src/agents/hitl/policy.spec.ts @@ -2,6 +2,9 @@ import type { Agents, TToolApprovalPolicy } from 'librechat-data-provider'; import { resolveToolApprovalPolicy, isHITLEnabled, + healToolApprovalPolicy, + collectAliasMatcherNames, + buildAliasMatcherPattern, mapToolApprovalPolicy, buildToolApprovalPayload, buildAskUserQuestionPayload, @@ -773,3 +776,87 @@ describe('exemptAskUserQuestionFromApproval', () => { expect(exemptAskUserQuestionFromApproval(undefined, NAME)).toBeUndefined(); }); }); + +describe('healToolApprovalPolicy', () => { + const aliases = [ + { name: 'delete_thing_mcp_acme', aliasName: 'acme_delete_thing_mcp_acme' }, + { name: 'search_mcp_acme', aliasName: 'acme_search_mcp_acme' }, + ]; + + it('appends current names to lists whose patterns match only the legacy spelling', () => { + /** Admin YAML written against upstream naming must keep applying — a + * non-matching deny fails OPEN. */ + const healed = healToolApprovalPolicy( + { enabled: true, deny: ['acme_delete_*'], ask: ['acme_search_mcp_acme'] }, + aliases, + ); + + expect(healed?.deny).toEqual(['acme_delete_*', 'delete_thing_mcp_acme']); + expect(healed?.ask).toEqual(['acme_search_mcp_acme', 'search_mcp_acme']); + }); + + it('heals list-level so allow semantics are preserved, not tightened', () => { + const healed = healToolApprovalPolicy({ enabled: true, allow: ['acme_search_*'] }, aliases); + + expect(healed?.allow).toEqual(['acme_search_*', 'search_mcp_acme']); + }); + + it('skips names the list already matches and leaves non-matching lists untouched', () => { + const healed = healToolApprovalPolicy( + { enabled: true, deny: ['*_mcp_acme'], allow: ['unrelated_tool'] }, + aliases, + ); + + expect(healed?.deny).toEqual(['*_mcp_acme']); + expect(healed?.allow).toEqual(['unrelated_tool']); + }); + + it('passes through without aliases or policy', () => { + expect(healToolApprovalPolicy(undefined, aliases)).toBeUndefined(); + const policy: TToolApprovalPolicy = { enabled: true, deny: ['x'] }; + expect(healToolApprovalPolicy(policy, [])).toBe(policy); + }); +}); + +describe('healToolApprovalPolicy reverse direction', () => { + it('appends a legacy-named instance when the pattern targets the current catalog name', () => { + /** An unedited agent retains the pre-strip instance name — a deny written + * against the current catalog name must still reach it. */ + const aliases = [{ name: 'acme_search_mcp_acme', aliasName: 'search_mcp_acme' }]; + const healed = healToolApprovalPolicy( + { enabled: true, mode: 'bypass', deny: ['search_mcp_acme'] }, + aliases, + ); + + expect(healed?.deny).toEqual(['search_mcp_acme', 'acme_search_mcp_acme']); + }); +}); + +describe('collectAliasMatcherNames', () => { + const aliases = [ + { name: 'search_mcp_acme', aliasName: 'acme_search_mcp_acme' }, + { name: 'acme_list_mcp_acme', aliasName: 'list_mcp_acme' }, + ]; + + it('returns names whose alias matches the regex while the name does not', () => { + expect(collectAliasMatcherNames('^acme_search_mcp_acme$', aliases)).toEqual([ + 'search_mcp_acme', + ]); + expect(collectAliasMatcherNames('^list_mcp_acme$', aliases)).toEqual(['acme_list_mcp_acme']); + }); + + it('skips names the matcher already matches and invalid patterns', () => { + expect(collectAliasMatcherNames('_mcp_acme$', aliases)).toEqual([]); + expect(collectAliasMatcherNames('(unclosed', aliases)).toEqual([]); + expect(collectAliasMatcherNames(undefined, aliases)).toEqual([]); + }); + + it('builds an anchored exact-name pattern with escaped names', () => { + const pattern = buildAliasMatcherPattern(['a.b_mcp_acme', 'c_mcp_acme']); + const regex = new RegExp(pattern); + expect(regex.test('a.b_mcp_acme')).toBe(true); + expect(regex.test('axb_mcp_acme')).toBe(false); + expect(regex.test('c_mcp_acme')).toBe(true); + expect(regex.test('xc_mcp_acme')).toBe(false); + }); +}); diff --git a/packages/api/src/agents/hitl/policy.ts b/packages/api/src/agents/hitl/policy.ts index 535ce37d72..d3350ec063 100644 --- a/packages/api/src/agents/hitl/policy.ts +++ b/packages/api/src/agents/hitl/policy.ts @@ -2,6 +2,7 @@ import { randomUUID, createHash } from 'crypto'; import { openAIBaseSchema, googleBaseSchema, anthropicBaseSchema } from 'librechat-data-provider'; import type { Agents, TToolApprovalPolicy } from 'librechat-data-provider'; import type { ToolPolicyConfig } from '@librechat/agents'; +import type { MCPToolAlias } from '~/tools/classification'; /** * Default decisions offered to the user for a paused tool call. @@ -86,6 +87,93 @@ export function isHITLEnabled(policy: TToolApprovalPolicy | undefined): boolean * defaults apply). The `enabled` field is LibreChat-only and stripped here — * it's consumed separately via {@link isHITLEnabled} to gate the SDK opt-out. */ +/** Anchored-glob matcher mirroring the SDK's `createToolPolicyHook` semantics exactly. */ +function globToRegex(pattern: string): RegExp { + const escaped = pattern.replace(/[.+?^${}()|[\]\\]/g, '\\$&'); + return new RegExp('^' + escaped.replace(/\*/g, '.*') + '$'); +} + +/** + * Extends each `toolApproval` pattern list with the names of tools whose + * OTHER spelling matches, so admin YAML keeps applying when a tool's key + * spelling changed in either direction: patterns written against pre-strip + * upstream naming reach the stripped instances (a non-matching `deny` would + * otherwise FAIL OPEN), and patterns written against the current catalog + * naming reach legacy-named instances retained by unedited agents. Healing + * is list-level (literal names appended, patterns never rewritten), so + * `deny`/`ask`/`allow` precedence semantics are unchanged, and a name + * already matched by its own list is skipped. + */ +export function healToolApprovalPolicy( + policy: TToolApprovalPolicy | undefined, + aliases: readonly MCPToolAlias[], +): TToolApprovalPolicy | undefined { + if (!policy || aliases.length === 0) { + return policy; + } + const healList = (patterns: string[] | undefined): string[] | undefined => { + if (!patterns || patterns.length === 0) { + return patterns; + } + const regexes = patterns.map(globToRegex); + const appended: string[] = []; + for (const { name, aliasName } of aliases) { + if (name === aliasName || regexes.some((regex) => regex.test(name))) { + continue; + } + if (regexes.some((regex) => regex.test(aliasName))) { + appended.push(name); + } + } + return appended.length > 0 ? [...patterns, ...appended] : patterns; + }; + return { + ...policy, + allow: healList(policy.allow), + deny: healList(policy.deny), + ask: healList(policy.ask), + }; +} + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +/** + * Names whose OTHER spelling matches a programmatic hook's regex matcher + * while their own name does not — the hook must also fire for these or its + * argument-, user-, or tenant-specific deny/ask decisions are silently + * skipped for renamed tools. Mirrors the SDK's unanchored `new RegExp(pattern)` + * matcher semantics; an invalid pattern matches nothing there, so it aliases + * nothing here. + */ +export function collectAliasMatcherNames( + matcher: string | undefined, + aliases: readonly MCPToolAlias[], +): string[] { + if (!matcher || aliases.length === 0) { + return []; + } + let regex: RegExp; + try { + regex = new RegExp(matcher); + } catch { + return []; + } + const names: string[] = []; + for (const { name, aliasName } of aliases) { + if (name !== aliasName && !regex.test(name) && regex.test(aliasName)) { + names.push(name); + } + } + return names; +} + +/** Anchored exact-name pattern for the alias-matched names of one hook matcher. */ +export function buildAliasMatcherPattern(names: readonly string[]): string { + return `^(?:${names.map(escapeRegExp).join('|')})$`; +} + export function mapToolApprovalPolicy( policy: TToolApprovalPolicy | undefined, ): ToolPolicyConfig | undefined { diff --git a/packages/api/src/agents/hitl/runtime.ts b/packages/api/src/agents/hitl/runtime.ts index b7c7404ad8..5d96ae05a3 100644 --- a/packages/api/src/agents/hitl/runtime.ts +++ b/packages/api/src/agents/hitl/runtime.ts @@ -1,7 +1,13 @@ import { HookRegistry, createToolPolicyHook } from '@librechat/agents'; import type { TToolApprovalPolicy } from 'librechat-data-provider'; +import type { MCPToolAlias } from '~/tools/classification'; import type { ToolApprovalHookContext } from './hooks'; -import { isHITLEnabled, mapToolApprovalPolicy } from './policy'; +import { + isHITLEnabled, + mapToolApprovalPolicy, + collectAliasMatcherNames, + buildAliasMatcherPattern, +} from './policy'; import { buildToolApprovalHooks } from './hooks'; /** @@ -33,6 +39,7 @@ export interface HITLRunWiring { export function buildHITLRunWiring( policy: TToolApprovalPolicy | undefined, context: ToolApprovalHookContext = {}, + mcpToolAliases: readonly MCPToolAlias[] = [], ): HITLRunWiring | undefined { if (!isHITLEnabled(policy)) { return undefined; @@ -52,6 +59,20 @@ export function buildHITLRunWiring( 'PreToolUse', matcher ? { pattern: matcher, hooks: [hook] } : { hooks: [hook] }, ); + /** A matcher written against a tool's OTHER key spelling (pre-strip or + * current) would silently never fire for the renamed instance, skipping + * its argument/user/tenant-specific deny or ask. The SAME hook is + * registered again under an exact-name pattern for those aliased names + * — a separate entry keeps the admin's regex semantics and the SDK's + * pattern-length cap intact, and the name sets are disjoint so the hook + * never fires twice for one call. */ + const aliasNames = matcher ? collectAliasMatcherNames(matcher, mcpToolAliases) : []; + if (aliasNames.length > 0) { + registry.register('PreToolUse', { + pattern: buildAliasMatcherPattern(aliasNames), + hooks: [hook], + }); + } } return { humanInTheLoop: { enabled: true }, hooks: registry }; diff --git a/packages/api/src/agents/initialize.ts b/packages/api/src/agents/initialize.ts index 7e49a30a91..9c429832d2 100644 --- a/packages/api/src/agents/initialize.ts +++ b/packages/api/src/agents/initialize.ts @@ -40,6 +40,7 @@ import type { } from '~/types'; import type { LCAvailableTools, RequestScopedMCPConnectionStore } from '../mcp/types'; import type { TFilterFilesByAgentAccess } from './resources'; +import type { MCPToolAlias } from '~/tools/classification'; import { injectSkillCatalog, resolveManualSkills, @@ -279,6 +280,8 @@ export type InitializedAgent = Agent & { requestScopedConnections?: RequestScopedMCPConnectionStore; /** Serializable tool definitions for event-driven execution */ toolDefinitions?: LCTool[]; + /** Both-direction identity aliases for MCP tools whose key spelling changed */ + mcpToolAliases?: MCPToolAlias[]; /** Precomputed flag indicating if any tools have defer_loading enabled (for efficient runtime checks) */ hasDeferredTools?: boolean; /** @@ -444,6 +447,7 @@ export interface InitializeAgentParams { /** Serializable tool definitions for event-driven mode */ toolDefinitions?: LCTool[]; hasDeferredTools?: boolean; + mcpToolAliases?: MCPToolAlias[]; actionsEnabled?: boolean; /** * Pre-uploaded code-env file refs for the agent's @@ -1106,6 +1110,7 @@ export async function initializeAgent( mcpAvailableTools, requestScopedConnections, hasDeferredTools, + mcpToolAliases, actionsEnabled, tools: structuredTools, primedCodeFiles, @@ -1119,6 +1124,7 @@ export async function initializeAgent( requestScopedConnections: undefined, toolDefinitions: [], hasDeferredTools: false, + mcpToolAliases: [], actionsEnabled: undefined, primedCodeFiles: undefined, }; @@ -1557,6 +1563,7 @@ export async function initializeAgent( userMCPAuthMap, toolDefinitions, hasDeferredTools, + mcpToolAliases, backgroundToolNames, intentToolNames, actionsEnabled, diff --git a/packages/api/src/agents/run.ts b/packages/api/src/agents/run.ts index e198b13095..8e7b940084 100644 --- a/packages/api/src/agents/run.ts +++ b/packages/api/src/agents/run.ts @@ -41,6 +41,7 @@ import type { BaseMessage } from '@librechat/agents/langchain/messages'; import type { AppConfig, IUser } from '@librechat/data-schemas'; import type { ToolInputValidationError } from '~/agents/toolValidation'; import type { ResolvedAlwaysApplySkill } from '~/agents/skills'; +import type { MCPToolAlias } from '~/tools/classification'; import type { SubagentUsageEvent } from '~/agents/usage'; import type * as t from '~/types'; import { @@ -49,6 +50,11 @@ import { stripBackgroundFromToolRegistry, stripBackgroundFromToolDefinitions, } from '~/agents/background'; +import { + resolveToolApprovalPolicy, + healToolApprovalPolicy, + exemptAskUserQuestionFromApproval, +} from '~/agents/hitl/policy'; import { ASK_USER_QUESTION_TOOL_NAME, createAskUserQuestionTool, @@ -57,7 +63,6 @@ import { createSubagentWakeupHandleHook, usesSubagentCompletionWakeups, } from '~/agents/subagentDelivery'; -import { resolveToolApprovalPolicy, exemptAskUserQuestionFromApproval } from '~/agents/hitl/policy'; import { applyCustomHandoffPromptKeyCompatibility } from '~/agents/handoffPromptKeyCompatibility'; import { stripIntentFromToolRegistry, stripIntentFromToolDefinitions } from '~/agents/intent'; import { isSteeringSupported, isSteerPreemptSupported } from '~/agents/steering/runtime'; @@ -378,6 +383,8 @@ type RunAgent = Omit & { toolDefinitions?: LCTool[]; /** Precomputed flag indicating if any tools have defer_loading enabled */ hasDeferredTools?: boolean; + /** Both-direction identity aliases for MCP tools whose key spelling changed */ + mcpToolAliases?: MCPToolAlias[]; /** Names of tools injected with the `run_in_background` param (excluded from eager execution). */ backgroundToolNames?: string[]; /** Names of tools with the host-injected `intent` param (stripped from self-spawn inputs). */ @@ -1694,18 +1701,29 @@ export async function createRun({ // would pause with no approval surface or resume endpoint, and the route would emit a // normal final response / `[DONE]` with the tool call dangling. Only AgentClient (chat + // resume) passes `hitlCapable`; without it the run is identical to the no-HITL path. + /** Both-direction key-spelling aliases collected at tool classification — + * identical in instance and event-driven loading modes. */ + const mcpToolAliases = agents.flatMap((agent) => agent.mcpToolAliases ?? []); const hitl = hitlCapable ? buildHITLRunWiring( // The ask tool is exempt from the approval prompt (unless explicitly // listed by the admin) — approving the right to ask a question is a - // pure double-pause; the tool has no side effects to gate. - exemptAskUserQuestionFromApproval(toolApprovalPolicy, ASK_USER_QUESTION_TOOL_NAME), + // pure double-pause; the tool has no side effects to gate. Pattern + // lists are healed against the tools' other key spellings first, so + // admin globs written for pre-strip upstream names keep applying (a + // non-matching deny would fail OPEN), and rules written against + // current catalog names reach legacy-named instances. + exemptAskUserQuestionFromApproval( + healToolApprovalPolicy(toolApprovalPolicy, mcpToolAliases), + ASK_USER_QUESTION_TOOL_NAME, + ), { userId: user?.id, conversationId: requestBody?.conversationId, tenantId: tenantId ?? user?.tenantId, appConfig, }, + mcpToolAliases, ) : undefined; /** diff --git a/packages/api/src/mcp/assistants.spec.ts b/packages/api/src/mcp/assistants.spec.ts index a326911404..38eafdd204 100644 --- a/packages/api/src/mcp/assistants.spec.ts +++ b/packages/api/src/mcp/assistants.spec.ts @@ -1,7 +1,7 @@ import { Constants } from 'librechat-data-provider'; import type { LCAvailableTools, ParsedServerConfig } from './types'; import type { AssistantToolDefinitionsDeps } from './assistants'; -import { getAssistantToolDefinitions } from './assistants'; +import { getAssistantToolDefinitions, toProviderToolDefinition } from './assistants'; const serverConfig: ParsedServerConfig = { type: 'streamable-http', @@ -48,12 +48,47 @@ describe('getAssistantToolDefinitions', () => { const deps = createDeps(); await expect(getAssistantToolDefinitions(params, deps)).resolves.toEqual({ - ...params.staticTools, - ...catalog, + toolDefinitions: { ...params.staticTools, ...catalog }, + accessibleServerNames: ['app-server'], }); expect(deps.getMCPServerTools).toHaveBeenCalledWith('user-1', 'app-server', serverConfig); }); + it('retains serverToolName for the heal; toProviderToolDefinition strips it at submission', async () => { + /** The heal verifies legacy rewrites against the recorded upstream + * identity, so the loader keeps the field; assistant writers submit + * entries verbatim, so the controllers sanitize each entry through + * toProviderToolDefinition before the provider sees it. */ + const strippedKey = `search${Constants.mcp_delimiter}app-server`; + const strippedCatalog: LCAvailableTools = { + [strippedKey]: { + type: 'function', + serverToolName: 'app-server_search', + ['function']: { + name: strippedKey, + description: '', + parameters: { type: 'object', properties: {} }, + }, + }, + }; + const deps = createDeps({ getMCPServerTools: jest.fn().mockResolvedValue(strippedCatalog) }); + + const { toolDefinitions } = await getAssistantToolDefinitions(params, deps); + + expect(toolDefinitions[strippedKey]?.serverToolName).toBe('app-server_search'); + + const sanitized = toProviderToolDefinition(toolDefinitions[strippedKey]); + expect(sanitized).toEqual({ + type: 'function', + ['function']: strippedCatalog[strippedKey]['function'], + }); + expect(sanitized).not.toHaveProperty('serverToolName'); + expect(toProviderToolDefinition('code_interpreter')).toBe('code_interpreter'); + expect(toProviderToolDefinition(params.staticTools.code_interpreter)).toBe( + params.staticTools.code_interpreter, + ); + }); + it('reconnects a user server when neither cache nor local snapshot has a catalog', async () => { const recoveredCatalog = { ...catalog }; const recoverServerTools = jest.fn().mockResolvedValue(recoveredCatalog); @@ -64,8 +99,8 @@ describe('getAssistantToolDefinitions', () => { }); await expect(getAssistantToolDefinitions(params, deps)).resolves.toEqual({ - ...params.staticTools, - ...recoveredCatalog, + toolDefinitions: { ...params.staticTools, ...recoveredCatalog }, + accessibleServerNames: ['app-server'], }); expect(recoverServerTools).toHaveBeenCalledWith('app-server', serverConfig); }); @@ -118,7 +153,10 @@ describe('getAssistantToolDefinitions', () => { cacheMCPServerTools, }); - await expect(getAssistantToolDefinitions(params, deps)).resolves.toEqual(params.staticTools); + await expect(getAssistantToolDefinitions(params, deps)).resolves.toEqual({ + toolDefinitions: params.staticTools, + accessibleServerNames: ['app-server'], + }); expect(cacheMCPServerTools).toHaveBeenCalledWith({ userId: 'user-1', serverName: 'app-server', @@ -192,7 +230,7 @@ describe('getAssistantToolDefinitions', () => { }, deps, ), - ).resolves.toBe(staticTools); + ).resolves.toEqual({ toolDefinitions: staticTools }); expect(deps.ensureConfigServers).not.toHaveBeenCalled(); expect(deps.getMCPServerTools).not.toHaveBeenCalled(); }); diff --git a/packages/api/src/mcp/assistants.ts b/packages/api/src/mcp/assistants.ts index 6b0160ce4a..78906210b4 100644 --- a/packages/api/src/mcp/assistants.ts +++ b/packages/api/src/mcp/assistants.ts @@ -6,7 +6,7 @@ import { splitMCPToolKey, } from 'librechat-data-provider'; import type { MCPOptions } from 'librechat-data-provider'; -import type { LCAvailableTools, ParsedServerConfig } from '~/mcp/types'; +import type { LCAvailableTools, LCFunctionTool, ParsedServerConfig } from '~/mcp/types'; import { createConcurrencyLimiter } from '~/utils/promise'; import { findShadowedServerNames } from '~/mcp/utils'; @@ -154,11 +154,22 @@ async function loadServerCatalog( throw new Error(`MCP tool definitions unavailable for assistant server "${serverName}"`); } +export interface AssistantToolDefinitionsResult { + toolDefinitions: LCAvailableTools; + /** + * Every server name the principal can reach, from the same merged registry + * read that resolved the catalogs — the legacy-key heal reuses it instead + * of repeating the app-config and registry round trips on the write path. + * `undefined` when the payload references no MCP tools (nothing to heal). + */ + accessibleServerNames?: string[]; +} + /** Loads the static catalog with the configuration-addressed MCP slices referenced by an assistant. */ export async function getAssistantToolDefinitions( params: AssistantToolDefinitionsParams, deps: AssistantToolDefinitionsDeps, -): Promise { +): Promise { const mcpToolNames = params.tools?.filter( (tool): tool is string => @@ -166,7 +177,7 @@ export async function getAssistantToolDefinitions( ) ?? []; const userId = params.user?.id; if (mcpToolNames.length === 0 || !userId) { - return params.staticTools; + return { toolDefinitions: params.staticTools }; } const configs = await resolveAssistantMcpConfigs( @@ -182,5 +193,31 @@ export async function getAssistantToolDefinitions( (serverName) => loadServerCatalog(userId, serverName, configs[serverName], deps, recover), ), ); - return Object.assign({}, params.staticTools, ...serverCatalogs); + /** Entries keep `serverToolName` here: the assistants heal verifies legacy + * key rewrites against that upstream identity. The controllers sanitize + * through {@link toProviderToolDefinition} at the submission boundary. */ + return { + toolDefinitions: Object.assign({}, params.staticTools, ...serverCatalogs), + accessibleServerNames: [ + ...new Set([...Object.keys(configs), ...Object.keys(params.mcpConfig)]), + ], + }; +} + +/** + * Assistant writers submit tool entries VERBATIM as provider tool definitions + * (`assistantData.tools` in the v1/v2 controllers), and providers reject + * unknown fields — the internal `serverToolName` mapping must never leave the + * catalog. Strings and entries without the mapping pass through by reference; + * the cached catalog keeps the mapping for the runtime call path. + */ +export function toProviderToolDefinition(tool: T): T | LCFunctionTool { + if (tool == null || typeof tool !== 'object') { + return tool; + } + const entry = tool as Partial; + if (entry.serverToolName == null || entry.type !== 'function' || entry['function'] == null) { + return tool; + } + return { type: entry.type, ['function']: entry['function'] }; } diff --git a/packages/api/src/mcp/catalog/store.ts b/packages/api/src/mcp/catalog/store.ts index 96cae3c75c..574a71f4ad 100644 --- a/packages/api/src/mcp/catalog/store.ts +++ b/packages/api/src/mcp/catalog/store.ts @@ -116,14 +116,23 @@ redis.call('DEL', KEYS[2]) return 1 `; +/** + * Catalog entries can carry `serverToolName` (redundant server-name prefix + * stripping): an older replica reading a stripped entry ignores the mapping + * and calls the stripped key segment upstream. Versioning the MCP catalog + * slices keeps mixed-version replicas on their own representation during a + * rolling deploy; stale slices simply expire. + */ +const CATALOG_VERSION = 'v2'; + export const ToolCacheKeys = { GLOBAL: 'tools:global', MCP_APP_SERVER: (serverName: string, configGeneration: string): string => - `tools:mcp:app:${encodeURIComponent(serverName)}:${encodeURIComponent(configGeneration)}`, + `tools:mcp:app:${CATALOG_VERSION}:${encodeURIComponent(serverName)}:${encodeURIComponent(configGeneration)}`, MCP_SERVER: (userId: string, serverName: string, configGeneration?: string): string => configGeneration - ? `tools:mcp:user:{${encodeURIComponent(userId)}:${encodeURIComponent(serverName)}}:${encodeURIComponent(configGeneration)}` - : `tools:mcp:${userId}:${serverName}`, + ? `tools:mcp:user:{${encodeURIComponent(userId)}:${encodeURIComponent(serverName)}}:${CATALOG_VERSION}:${encodeURIComponent(configGeneration)}` + : `tools:mcp:${CATALOG_VERSION}:${userId}:${serverName}`, MCP_SERVER_GENERATION: (userId: string, serverName: string): string => `tools:metadata:mcp:user-generation:{${encodeURIComponent(userId)}:${encodeURIComponent(serverName)}}`, MCP_SERVER_LEGACY_FENCE: (userId: string, serverName: string): string => diff --git a/packages/api/src/mcp/registry/MCPServerInspector.ts b/packages/api/src/mcp/registry/MCPServerInspector.ts index b0c1b436e0..fe05d41e6c 100644 --- a/packages/api/src/mcp/registry/MCPServerInspector.ts +++ b/packages/api/src/mcp/registry/MCPServerInspector.ts @@ -1,5 +1,5 @@ import { logger } from '@librechat/data-schemas'; -import { Constants, normalizeServerName } from 'librechat-data-provider'; +import { Constants, normalizeServerName, stripServerNamePrefixes } from 'librechat-data-provider'; import type { JsonSchemaType } from '@librechat/data-schemas'; import type { MCPConnection } from '~/mcp/connection'; import type * as t from '~/mcp/types'; @@ -188,10 +188,16 @@ export class MCPServerInspector { /** Model-facing key: must match the runtime instance name, which embeds * the normalized server name (see `createToolInstance` in MCP.js). */ const keyServerName = normalizeServerName(serverName); + const keyToolNames = stripServerNamePrefixes( + tools.map((tool) => tool.name), + keyServerName, + ); tools.forEach((tool) => { - const name = `${tool.name}${Constants.mcp_delimiter}${keyServerName}`; + const keyToolName = keyToolNames.get(tool.name) ?? tool.name; + const name = `${keyToolName}${Constants.mcp_delimiter}${keyServerName}`; toolFunctions[name] = { type: 'function', + ...(keyToolName !== tool.name && { serverToolName: tool.name }), ['function']: { name, description: tool.description, diff --git a/packages/api/src/mcp/registry/MCPServersInitializer.ts b/packages/api/src/mcp/registry/MCPServersInitializer.ts index 4cdd148383..143d14c699 100644 --- a/packages/api/src/mcp/registry/MCPServersInitializer.ts +++ b/packages/api/src/mcp/registry/MCPServersInitializer.ts @@ -22,9 +22,13 @@ const DEFAULT_FOLLOWER_RETRY_MS = 3000; * followers short-circuit on the stale status and never re-tag entries written * by the previous version. Bumped to 3 so cached entries whose `serverInstructions` still holds * inspector-fetched text are rewritten with the declaration preserved and the text moved to - * `resolvedInstructions`. + * `resolvedInstructions`. Bumped to 4 so persisted `toolFunctions` are rebuilt + * with redundant server-name prefixes stripped and `serverToolName` recorded — + * otherwise a follower accepts the previous deployment's config hash and + * republishes pre-strip definitions into the current catalog namespace + * indefinitely. */ -const REGISTRY_STORAGE_SCHEMA_VERSION = 3; +const REGISTRY_STORAGE_SCHEMA_VERSION = 4; const parseDurationMs = ( value: string | undefined, diff --git a/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts b/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts index 5299060699..c77b1b32e7 100644 --- a/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts +++ b/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts @@ -597,6 +597,33 @@ describe('MCPServerInspector', () => { expect(result[key]['function'].name).toBe(key); }); + it('strips a redundant server-name prefix from keys and records the raw name', async () => { + mockConnection.fetchOrderedToolsSnapshot = jest.fn().mockResolvedValue({ + complete: true, + tools: [ + { + name: 'acme_trace_top_time_consuming_operations', + description: 'Trace', + inputSchema: { type: 'object', properties: {} }, + }, + { + name: 'list_services', + description: 'List', + inputSchema: { type: 'object', properties: {} }, + }, + ], + }); + + const { tools: result } = await MCPServerInspector.getToolCatalog('acme', mockConnection); + + const strippedKey = 'trace_top_time_consuming_operations_mcp_acme'; + const plainKey = 'list_services_mcp_acme'; + expect(Object.keys(result).sort()).toEqual([plainKey, strippedKey].sort()); + expect(result[strippedKey]['function'].name).toBe(strippedKey); + expect(result[strippedKey].serverToolName).toBe('acme_trace_top_time_consuming_operations'); + expect(result[plainKey].serverToolName).toBeUndefined(); + }); + it('rejects an incomplete snapshot before it can replace cached tools', async () => { mockConnection.fetchOrderedToolsSnapshot = jest.fn().mockResolvedValue({ tools: [{ name: 'partial', inputSchema: { type: 'object' } }], diff --git a/packages/api/src/mcp/tools.spec.ts b/packages/api/src/mcp/tools.spec.ts index 03726be831..56fad5f666 100644 --- a/packages/api/src/mcp/tools.spec.ts +++ b/packages/api/src/mcp/tools.spec.ts @@ -495,6 +495,49 @@ describe('createMCPToolCacheService', () => { }); }); + it('strips a redundant server-name prefix from keys and records the raw name', async () => { + /** `acme_trace..._mcp_acme` carries the server twice and can push the + * model-facing name past provider function-name limits (64). */ + const deps = createMockDeps(); + const tools: MCPToolInput[] = [ + { name: 'acme_trace_top_time_consuming_operations', description: 'Trace' }, + { name: 'list_services', description: 'List' }, + ]; + const result = await createMCPToolCacheService(deps).updateMCPServerTools({ + userId: 'u1', + serverName: 'acme', + tools, + }); + + const strippedKey = toolName('trace_top_time_consuming_operations', 'acme'); + const plainKey = toolName('list_services', 'acme'); + expect(Object.keys(result ?? {}).sort()).toEqual([plainKey, strippedKey].sort()); + expect(result?.[strippedKey]?.['function'].name).toBe(strippedKey); + expect(result?.[strippedKey]?.serverToolName).toBe( + 'acme_trace_top_time_consuming_operations', + ); + expect(result?.[plainKey]?.serverToolName).toBeUndefined(); + }); + + it('keeps the prefixed key when stripping would collide with a sibling tool', async () => { + const deps = createMockDeps(); + const tools: MCPToolInput[] = [ + { name: 'search', description: 'Plain' }, + { name: 'acme_search', description: 'Prefixed' }, + ]; + const result = await createMCPToolCacheService(deps).updateMCPServerTools({ + userId: 'u1', + serverName: 'acme', + tools, + }); + + const plainKey = toolName('search', 'acme'); + const prefixedKey = toolName('acme_search', 'acme'); + expect(Object.keys(result ?? {}).sort()).toEqual([prefixedKey, plainKey].sort()); + expect(result?.[plainKey]?.serverToolName).toBeUndefined(); + expect(result?.[prefixedKey]?.serverToolName).toBeUndefined(); + }); + it('builds request-scoped tools without caching them', async () => { const deps = createMockDeps({ getServerConfig: jest.fn().mockResolvedValue(requestScopedConfig), diff --git a/packages/api/src/mcp/tools.ts b/packages/api/src/mcp/tools.ts index b1b801e1ca..d043d2cad1 100644 --- a/packages/api/src/mcp/tools.ts +++ b/packages/api/src/mcp/tools.ts @@ -1,5 +1,10 @@ import { logger } from '@librechat/data-schemas'; -import { Constants, buildServerNameAliases, normalizeServerName } from 'librechat-data-provider'; +import { + Constants, + buildServerNameAliases, + normalizeServerName, + stripServerNamePrefixes, +} from 'librechat-data-provider'; import type { Tool } from '@modelcontextprotocol/sdk/types.js'; import type { JsonSchemaType } from '@librechat/agents'; import type { LCAvailableTools, LCFunctionTool, ParsedServerConfig } from './types'; @@ -234,8 +239,13 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS * `normalizeServerName(serverName)`. The cache STORE itself stays keyed * by the raw config name. */ const keyServerName = normalizeServerName(serverName); + const keyToolNames = stripServerNamePrefixes( + tools.map((tool) => tool.name), + keyServerName, + ); for (const tool of tools) { - const name = `${tool.name}${mcpDelimiter}${keyServerName}`; + const keyToolName = keyToolNames.get(tool.name) ?? tool.name; + const name = `${keyToolName}${mcpDelimiter}${keyServerName}`; const entry: LCFunctionTool = { type: 'function', ['function']: { @@ -246,6 +256,9 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS : ({ type: 'object', properties: {} } as JsonSchemaType), }, }; + if (keyToolName !== tool.name) { + entry.serverToolName = tool.name; + } serverTools[name] = entry; } diff --git a/packages/api/src/mcp/types/index.ts b/packages/api/src/mcp/types/index.ts index 4585ac1bd6..ae59ed4af4 100644 --- a/packages/api/src/mcp/types/index.ts +++ b/packages/api/src/mcp/types/index.ts @@ -49,6 +49,9 @@ export interface MCPResource { export interface LCFunctionTool { type: 'function'; ['function']: LCTool; + /** Raw upstream tool name when the model-facing key stripped a redundant + * server-name prefix — tool calls must send THIS name to the server. */ + serverToolName?: string; } export type LCAvailableTools = Record; diff --git a/packages/api/src/mcp/utils.ts b/packages/api/src/mcp/utils.ts index a715a1df09..0abbe46034 100644 --- a/packages/api/src/mcp/utils.ts +++ b/packages/api/src/mcp/utils.ts @@ -631,4 +631,6 @@ export { normalizeServerName, normalizeMCPToolKey, buildServerNameAliases, + stripServerNamePrefix, + stripServerNamePrefixes, } from 'librechat-data-provider'; diff --git a/packages/api/src/tools/classification.spec.ts b/packages/api/src/tools/classification.spec.ts index b543d2bad1..642bbc1e4a 100644 --- a/packages/api/src/tools/classification.spec.ts +++ b/packages/api/src/tools/classification.spec.ts @@ -4,8 +4,10 @@ import type { GenericTool } from '@librechat/agents'; import type { LCToolRegistry } from './classification'; import { buildToolRegistryFromAgentOptions, + aliasMCPToolOptions, agentHasProgrammaticTools, buildToolClassification, + collectMCPToolAliases, getServerNameFromTool, agentHasDeferredTools, } from './classification'; @@ -28,6 +30,111 @@ describe('classification.ts', () => { }); }); + describe('collectMCPToolAliases', () => { + it('collects both alias directions from definitions', () => { + const defs = [ + { name: 'search_mcp_acme', serverName: 'acme', serverToolName: 'acme_search' }, + { + name: 'acme_list_mcp_acme', + serverName: 'acme', + serverToolName: 'acme_list', + currentToolName: 'list', + }, + { name: 'plain_mcp_acme', serverName: 'acme' }, + ]; + + expect(collectMCPToolAliases(defs)).toEqual([ + { name: 'search_mcp_acme', aliasName: 'acme_search_mcp_acme' }, + { name: 'acme_list_mcp_acme', aliasName: 'list_mcp_acme' }, + ]); + }); + + it('normalizes the server name when reconstructing alias keys', () => { + const defs = [ + { + name: 'search_mcp_My_Server', + serverName: 'My Server', + serverToolName: 'my_server_search', + }, + ]; + + expect(collectMCPToolAliases(defs)).toEqual([ + { name: 'search_mcp_My_Server', aliasName: 'my_server_search_mcp_My_Server' }, + ]); + }); + }); + + describe('aliasMCPToolOptions', () => { + it('aliases pre-strip option keys onto the current instance name, identity-gated', () => { + /** Wildcard-expanded catalogs rename stripped tools without any + * `agent.tools` entry to preserve the spelling — persisted defer, + * programmatic, background, and intent settings must follow. */ + const defs = [ + { + name: 'search_mcp_acme', + serverName: 'acme', + serverToolName: 'acme_search', + }, + { name: 'list_items_mcp_acme', serverName: 'acme' }, + ]; + const agentToolOptions: AgentToolOptions = { + acme_search_mcp_acme: { defer_loading: true }, + }; + + aliasMCPToolOptions(collectMCPToolAliases(defs), agentToolOptions); + + expect(agentToolOptions['search_mcp_acme']).toEqual({ defer_loading: true }); + const registry = buildToolRegistryFromAgentOptions(defs, agentToolOptions); + expect(registry.get('search_mcp_acme')?.defer_loading).toBe(true); + }); + + it('aliases current-keyed options back onto a legacy-named instance', () => { + /** The editor migrates `tool_options` keys to the current catalog + * spelling, while an unedited `agent.tools` entry keeps the legacy + * instance name — options must follow the reverse direction too. */ + const defs = [ + { + name: 'acme_search_mcp_acme', + serverName: 'acme', + serverToolName: 'acme_search', + currentToolName: 'search', + }, + ]; + const agentToolOptions: AgentToolOptions = { + search_mcp_acme: { defer_loading: true }, + }; + + aliasMCPToolOptions(collectMCPToolAliases(defs), agentToolOptions); + + expect(agentToolOptions['acme_search_mcp_acme']).toEqual({ defer_loading: true }); + const registry = buildToolRegistryFromAgentOptions(defs, agentToolOptions); + expect(registry.get('acme_search_mcp_acme')?.defer_loading).toBe(true); + }); + + it('never overrides an explicit entry under the instance name', () => { + const defs = [{ name: 'search_mcp_acme', serverName: 'acme', serverToolName: 'acme_search' }]; + const agentToolOptions: AgentToolOptions = { + search_mcp_acme: { defer_loading: false }, + acme_search_mcp_acme: { defer_loading: true }, + }; + + aliasMCPToolOptions(collectMCPToolAliases(defs), agentToolOptions); + + expect(agentToolOptions['search_mcp_acme']).toEqual({ defer_loading: false }); + }); + + it('does nothing without recorded upstream identity', () => { + const defs = [{ name: 'search_mcp_acme', serverName: 'acme' }]; + const agentToolOptions: AgentToolOptions = { + acme_search_mcp_acme: { defer_loading: true }, + }; + + aliasMCPToolOptions(collectMCPToolAliases(defs), agentToolOptions); + + expect(agentToolOptions['search_mcp_acme']).toBeUndefined(); + }); + }); + describe('buildToolRegistryFromAgentOptions', () => { it('should use agent tool options for defer_loading', () => { const tools = [ diff --git a/packages/api/src/tools/classification.ts b/packages/api/src/tools/classification.ts index 2ede26ffcb..be83fa08ef 100644 --- a/packages/api/src/tools/classification.ts +++ b/packages/api/src/tools/classification.ts @@ -6,7 +6,7 @@ */ import { logger } from '@librechat/data-schemas'; -import { Constants } from 'librechat-data-provider'; +import { Constants, normalizeServerName } from 'librechat-data-provider'; import { Providers, createToolSearch, @@ -33,6 +33,47 @@ export interface ToolDefinition { parameters?: JsonSchemaType; /** MCP server name extracted from tool name */ serverName?: string; + /** Raw upstream tool name when the model-facing key stripped a redundant server-name prefix */ + serverToolName?: string; + /** Current catalog tool name when a LEGACY persisted key kept its pre-strip spelling */ + currentToolName?: string; +} + +/** An MCP tool name plus its OTHER spelling (legacy for stripped instances, current for legacy-named ones). */ +export interface MCPToolAlias { + name: string; + aliasName: string; +} + +/** + * Collects both directions of identity aliases from MCP tool definitions, so + * approval policies and hook matchers written against EITHER spelling keep + * applying: a stripped instance aliases its pre-strip name, and a + * legacy-named instance (persisted key retained) aliases its current catalog + * name. Works in both loading modes because both funnel their definitions + * through {@link buildToolClassification}. + */ +export function collectMCPToolAliases(mcpToolDefs: ToolDefinition[]): MCPToolAlias[] { + const aliases: MCPToolAlias[] = []; + for (const def of mcpToolDefs) { + if (!def.serverName) { + continue; + } + const keySuffix = `${Constants.mcp_delimiter}${normalizeServerName(def.serverName)}`; + if (def.serverToolName) { + const aliasName = `${def.serverToolName}${keySuffix}`; + if (aliasName !== def.name) { + aliases.push({ name: def.name, aliasName }); + } + } + if (def.currentToolName) { + const aliasName = `${def.currentToolName}${keySuffix}`; + if (aliasName !== def.name) { + aliases.push({ name: def.name, aliasName }); + } + } + } + return aliases; } /** @@ -49,6 +90,31 @@ export function getServerNameFromTool(toolName: string): string | undefined { return undefined; } +/** + * Aliases persisted `tool_options` keys onto the instance names IN PLACE, so + * every downstream reader of `agent.tool_options` (the registry build for + * defer/programmatic, the background and intent passes) sees the healed keys + * in BOTH loading modes and BOTH spelling directions: options keyed by a + * pre-strip spelling follow a renamed (wildcard-expanded) instance, and + * options the editor migrated to the CURRENT catalog spelling still reach a + * legacy-named instance an unedited `agent.tools` entry retained. + * Identity-gated through {@link collectMCPToolAliases}, and an explicit + * entry under the instance's own name always wins. + */ +export function aliasMCPToolOptions( + aliases: readonly MCPToolAlias[], + agentToolOptions?: AgentToolOptions, +): void { + if (!agentToolOptions || Object.keys(agentToolOptions).length === 0) { + return; + } + for (const { name, aliasName } of aliases) { + if (agentToolOptions[name] == null && agentToolOptions[aliasName] != null) { + agentToolOptions[name] = agentToolOptions[aliasName]; + } + } +} + /** * Builds a tool registry from agent-level tool_options. * @@ -104,6 +170,10 @@ interface MCPToolInstance { mcpJsonSchema?: JsonSchemaType; /** Server this tool came from, carried from resolution instead of re-parsed */ mcpRawServerName?: string; + /** Raw upstream tool name when the instance name stripped a redundant server-name prefix */ + mcpServerToolName?: string; + /** Current catalog tool name when a legacy persisted key kept its pre-strip spelling */ + mcpCurrentToolName?: string; } /** @@ -129,6 +199,14 @@ export function extractMCPToolDefinition(tool: MCPToolInstance): ToolDefinition def.serverName = serverName; } + if (tool.mcpServerToolName) { + def.serverToolName = tool.mcpServerToolName; + } + + if (tool.mcpCurrentToolName) { + def.currentToolName = tool.mcpCurrentToolName; + } + return def; } @@ -214,6 +292,8 @@ export interface BuildToolClassificationResult { additionalTools: GenericTool[]; /** Whether any tools have defer_loading enabled (precomputed for efficiency) */ hasDeferredTools: boolean; + /** Both-direction identity aliases for MCP tools whose key spelling changed (see {@link collectMCPToolAliases}) */ + mcpToolAliases: MCPToolAlias[]; } /** @@ -282,10 +362,13 @@ export async function buildToolClassification( toolDefinitions: [], toolRegistry: undefined, hasDeferredTools: false, + mcpToolAliases: [], }; } const mcpToolDefs = mcpTools.map(extractMCPToolDefinition); + const mcpToolAliases = collectMCPToolAliases(mcpToolDefs); + aliasMCPToolOptions(mcpToolAliases, agentToolOptions); const toolRegistry: LCToolRegistry = buildToolRegistry(mcpToolDefs, agentToolOptions); /** Clean up temporary mcpJsonSchema property from tools now that registry is populated */ @@ -318,7 +401,13 @@ export async function buildToolClassification( logger.debug( `[buildToolClassification] Agent ${agentId} has no programmatic or deferred tools, skipping PTC/ToolSearch`, ); - return { toolRegistry, toolDefinitions, additionalTools, hasDeferredTools: false }; + return { + toolRegistry, + toolDefinitions, + additionalTools, + hasDeferredTools: false, + mcpToolAliases, + }; } /** Tool search uses local mode (no API key needed) */ @@ -357,7 +446,7 @@ export async function buildToolClassification( } if (!hasProgrammaticTools) { - return { toolRegistry, toolDefinitions, additionalTools, hasDeferredTools }; + return { toolRegistry, toolDefinitions, additionalTools, hasDeferredTools, mcpToolAliases }; } /** In definitions-only mode, add PTC definition without creating the tool instance */ @@ -374,7 +463,7 @@ export async function buildToolClassification( logger.debug( `[buildToolClassification] PTC definition added for agent ${agentId} (definitions only)`, ); - return { toolRegistry, toolDefinitions, additionalTools, hasDeferredTools }; + return { toolRegistry, toolDefinitions, additionalTools, hasDeferredTools, mcpToolAliases }; } try { @@ -408,5 +497,5 @@ export async function buildToolClassification( logger.error('[buildToolClassification] Error creating PTC tool:', error); } - return { toolRegistry, toolDefinitions, additionalTools, hasDeferredTools }; + return { toolRegistry, toolDefinitions, additionalTools, hasDeferredTools, mcpToolAliases }; } diff --git a/packages/api/src/tools/definitions.spec.ts b/packages/api/src/tools/definitions.spec.ts index 5df9b16d98..9e7fc239e1 100644 --- a/packages/api/src/tools/definitions.spec.ts +++ b/packages/api/src/tools/definitions.spec.ts @@ -575,6 +575,72 @@ describe('definitions.ts', () => { expect(getItemDef?.description).toBe('Get a specific item'); }); + it('resolves a pre-strip persisted key against the stripped catalog, keeping the persisted name', async () => { + /** Catalog keys drop a redundant leading server-name prefix; an agent + * saved before that must still resolve, and the definition keeps the + * persisted spelling so it matches the runtime instance name. */ + const mockServerTools = { + search_mcp_acme: { + serverToolName: 'acme_search', + function: { + name: 'search_mcp_acme', + description: 'Search things', + parameters: { type: 'object', properties: {} }, + }, + }, + }; + + mockGetOrFetchMCPServerTools.mockResolvedValue(mockServerTools); + + const params: LoadToolDefinitionsParams = { + userId: 'user-123', + agentId: 'agent-123', + tools: ['acme_search_mcp_acme'], + }; + + const deps: LoadToolDefinitionsDeps = { + getOrFetchMCPServerTools: mockGetOrFetchMCPServerTools, + isBuiltInTool: mockIsBuiltInTool, + }; + + const result = await loadToolDefinitions(params, deps); + + expect(result.toolDefinitions).toHaveLength(1); + expect(result.toolDefinitions[0]?.name).toBe('acme_search_mcp_acme'); + expect(result.toolDefinitions[0]?.description).toBe('Search things'); + }); + + it('rejects a stripped-spelling match without matching upstream identity', async () => { + /** A stale key for a removed tool must not resolve onto a DIFFERENT + * sibling whose key merely coincides with the stripped spelling. */ + const mockServerTools = { + acme_foo_mcp_acme: { + function: { + name: 'acme_foo_mcp_acme', + description: 'Different tool', + parameters: { type: 'object', properties: {} }, + }, + }, + }; + + mockGetOrFetchMCPServerTools.mockResolvedValue(mockServerTools); + + const params: LoadToolDefinitionsParams = { + userId: 'user-123', + agentId: 'agent-123', + tools: ['acme_acme_foo_mcp_acme'], + }; + + const deps: LoadToolDefinitionsDeps = { + getOrFetchMCPServerTools: mockGetOrFetchMCPServerTools, + isBuiltInTool: mockIsBuiltInTool, + }; + + const result = await loadToolDefinitions(params, deps); + + expect(result.toolDefinitions).toHaveLength(0); + }); + it('union-flattens MCP tool schemas for Google, but preserves unions otherwise', async () => { const mockServerTools = { issue_write_mcp_github: { diff --git a/packages/api/src/tools/definitions.ts b/packages/api/src/tools/definitions.ts index 8904bf9fcb..d4b5967b1d 100644 --- a/packages/api/src/tools/definitions.ts +++ b/packages/api/src/tools/definitions.ts @@ -10,11 +10,13 @@ import { Constants, isActionTool, splitMCPToolKey, + normalizeServerName, + stripServerNamePrefix, buildServerNameAliases, } from 'librechat-data-provider'; import type { LCToolRegistry, JsonSchemaType, LCTool, GenericTool } from '@librechat/agents'; import type { AgentToolOptions } from 'librechat-data-provider'; -import type { ToolDefinition } from './classification'; +import type { MCPToolAlias, ToolDefinition } from './classification'; import { resolveJsonSchemaRefs, normalizeJsonSchema, sanitizeGeminiSchema } from '~/mcp/zod'; import { buildToolClassification } from './classification'; import { getToolDefinition } from './registry/definitions'; @@ -27,6 +29,7 @@ export interface MCPServerTool { description?: string; parameters?: JsonSchemaType; }; + serverToolName?: string; } export type MCPServerTools = Record; @@ -92,6 +95,8 @@ export interface LoadToolDefinitionsResult { toolDefinitions: (ToolDefinition | LCTool)[]; toolRegistry: LCToolRegistry; hasDeferredTools: boolean; + /** Both-direction identity aliases for MCP tools whose key spelling changed */ + mcpToolAliases: MCPToolAlias[]; mcpResolution: { expectedToolCount: number; resolvedToolCount: number; @@ -145,6 +150,7 @@ export async function loadToolDefinitions( toolDefinitions: [], toolRegistry: new Map(), hasDeferredTools: false, + mcpToolAliases: [], mcpResolution: { expectedToolCount: 0, resolvedToolCount: 0 }, }; @@ -247,9 +253,38 @@ export async function loadToolDefinitions( continue; } + /** Catalog keys are built after redundant server-name-prefix stripping — + * a pre-strip persisted key (`acme_search_mcp_acme`) must also try its + * stripped spelling or the agent fails initialization with its expected + * tools "unavailable". The definition keeps the PERSISTED name so it + * matches the runtime instance `createMCPTool` builds for the same key, + * and the stripped entry is accepted only when its recorded raw name + * PROVES the same upstream identity. */ + const findToolMatch = ( + tools: Record, + ): { def: MCPServerTool; currentToolName?: string } | undefined => { + const direct = tools[toolName]; + if (direct?.function) { + return { def: direct }; + } + const keyServerName = normalizeServerName(serverName); + const [toolPart] = splitMCPToolKey(toolName, [parsed]); + const strippedPart = stripServerNamePrefix(toolPart, keyServerName); + if (strippedPart === toolPart) { + return undefined; + } + const entry = tools[`${strippedPart}${Constants.mcp_delimiter}${keyServerName}`]; + /** `currentToolName` records the catalog spelling so approval policies + * and hook matchers written against it still reach this legacy-named + * definition (see `collectMCPToolAliases`). */ + return entry?.serverToolName === toolPart + ? { def: entry, currentToolName: strippedPart } + : undefined; + }; + const selectedToolMissing = isMCPAllPlaceholder(toolName) ? Object.keys(serverTools).length === 0 - : !serverTools[toolName]?.function; + : !findToolMatch(serverTools)?.def.function; if (selectedToolMissing && refreshMCPServerTools && !refreshedServerNames.has(serverName)) { refreshedServerNames.add(serverName); const refreshedTools = await refreshMCPServerTools(userId, serverName); @@ -267,6 +302,7 @@ export async function loadToolDefinitions( description: toolDef.function.description || undefined, parameters: buildMcpParameters(toolDef.function.parameters), serverName, + serverToolName: toolDef.serverToolName, }); resolvedMCPToolCount++; } @@ -274,13 +310,15 @@ export async function loadToolDefinitions( continue; } - const toolDef = serverTools[toolName]; - if (toolDef?.function) { + const toolMatch = findToolMatch(serverTools); + if (toolMatch?.def.function) { mcpToolDefs.push({ name: toolName, - description: toolDef.function.description || undefined, - parameters: buildMcpParameters(toolDef.function.parameters), + description: toolMatch.def.function.description || undefined, + parameters: buildMcpParameters(toolMatch.def.function.parameters), serverName, + serverToolName: toolMatch.def.serverToolName, + currentToolName: toolMatch.currentToolName, }); resolvedMCPToolCount++; } @@ -301,6 +339,8 @@ export async function loadToolDefinitions( mcp: true as const, mcpJsonSchema: def.parameters, mcpRawServerName: def.serverName, + mcpServerToolName: def.serverToolName, + mcpCurrentToolName: def.currentToolName, })) as unknown as GenericTool[]; const classificationResult = await buildToolClassification({ @@ -350,6 +390,7 @@ export async function loadToolDefinitions( toolDefinitions: allDefinitions, toolRegistry, hasDeferredTools, + mcpToolAliases: classificationResult.mcpToolAliases, mcpResolution: { expectedToolCount: expectedMCPToolCount, resolvedToolCount: resolvedMCPToolCount, diff --git a/packages/data-provider/src/config.ts b/packages/data-provider/src/config.ts index c7661f3976..7ef02184bc 100644 --- a/packages/data-provider/src/config.ts +++ b/packages/data-provider/src/config.ts @@ -12,6 +12,7 @@ import { ComponentTypes, SettingTypes, OptionTypes } from './generate'; import { MAX_SUBAGENTS, MAX_SUBAGENTS_CEILING } from './limits'; import { STATEFUL_CODE_ENVIRONMENTS } from './stateful-code'; import { specsConfigSchema, TSpecsConfig } from './models'; +import { isActionTool } from './types/assistants'; import { REFILL_INTERVAL_UNITS } from './balance'; import { fileConfigSchema } from './file-config'; import { apiBaseUrl } from './api-endpoints'; @@ -3177,6 +3178,120 @@ export function normalizeMCPToolKey(toolKey: string, rawServerNames: readonly st return `${toolKey.slice(0, toolKey.length - matched.length)}${normalized}`; } +/** + * Strips a redundant leading server-name prefix from a raw upstream tool name + * before it is embedded into a model-facing key, so the key doesn't carry the + * server twice (`acme_trace_..._mcp_acme`) and push long tool names + * past provider function-name limits (64 chars). The match is case-insensitive + * because display-cased server names ("Acme") conventionally prefix their + * tools in lowercase. Ingestion that strips must record the original name + * (`serverToolName` on the cached definition) — tool calls send THAT name back + * to the server, never the stripped one. Catalog producers must not call this + * directly: only {@link stripServerNamePrefixes} sees the whole sibling set and + * can keep colliding results apart. + */ +export function stripServerNamePrefix(toolName: string, normalizedServerName: string): string { + const prefixLength = normalizedServerName.length + 1; + if (toolName.length <= prefixLength) { + return toolName; + } + const prefix = toolName.slice(0, prefixLength).toLowerCase(); + if (prefix !== `${normalizedServerName.toLowerCase()}_`) { + return toolName; + } + const stripped = toolName.slice(prefixLength); + if (isReservedMCPToolName(stripped)) { + return toolName; + } + /** `isActionTool` classifies keys by the RELATIVE position of `_action_` + * and `_mcp_`; stripping moves the first `_mcp_` earlier, so a server + * whose normalized name contains `_action_` could see a real MCP tool + * reclassified as an OpenAPI action (bypassing MCP authorization). Never + * produce a key whose classification differs from the raw key's. */ + const keySuffix = `${Constants.mcp_delimiter}${normalizedServerName}`; + if (isActionTool(`${stripped}${keySuffix}`) !== isActionTool(`${toolName}${keySuffix}`)) { + return toolName; + } + return stripped; +} + +/** + * Synthetic markers consumed by prefix (`isMCPAllPlaceholder`, the server-pin + * skip, the client's OAuth stream classification), so each reserves BOTH its + * exact name and its `${marker}${mcp_delimiter}` namespace: a stripped + * remainder inside any of them would turn a real upstream tool into the + * server-wide wildcard, the UI pin placeholder, or a synthetic OAuth call. + */ +const RESERVED_MCP_TOOL_MARKERS: readonly string[] = [ + `${Constants.mcp_all}`, + `${Constants.mcp_server}`, + 'oauth', +]; + +function isReservedMCPToolName(toolName: string): boolean { + /** `mcp_` opens the server-scoped pluginKey namespace (`mcp_${serverName}`), + * and `lc_transfer_to_` opens the agent-handoff namespace (the client + * renders such calls as handoffs; the background and intent passes exclude + * them) — pre-strip tool keys could never enter either, since they always + * began with the server name itself. */ + if ( + toolName.startsWith(`${Constants.mcp_prefix}`) || + toolName.startsWith(`${Constants.LC_TRANSFER_TO_}`) + ) { + return true; + } + return RESERVED_MCP_TOOL_MARKERS.some( + (marker) => toolName === marker || toolName.startsWith(`${marker}${Constants.mcp_delimiter}`), + ); +} + +/** + * Maps every raw tool name in a server's catalog to its model-facing name, + * stripping redundant server-name prefixes collision-free: when two names + * yield the same result — a bare `foo` next to `_foo`, or the + * case-variant pair `_Foo` / `_Foo` under the case-insensitive + * prefix match — every collider keeps its raw name, so two distinct upstream + * tools can never collapse onto one key. Unprefixed names count against the + * result set through their identity mapping, which is what makes the bare-name + * case fall out of the same counter. + */ +export function stripServerNamePrefixes( + toolNames: readonly string[], + normalizedServerName: string, +): Map { + const rawNames = new Set(toolNames); + const finalNames = new Map( + toolNames.map((name) => { + const stripped = stripServerNamePrefix(name, normalizedServerName); + /** Every sibling's RAW name is reserved even when that sibling itself + * strips away: keys persisted BEFORE stripping embed raw names, so a + * stripped result landing on another sibling's raw name would route + * that sibling's legacy references to the wrong upstream tool. */ + return [name, stripped !== name && rawNames.has(stripped) ? name : stripped]; + }), + ); + /** Reverting a collider to its raw name can itself collide with ANOTHER + * sibling's stripped result (`foo` / `acme_foo` / `acme_acme_foo`), so the + * guard iterates to a fixpoint. Each pass converts at least one stripped + * result back to its unique raw name, so it terminates within the catalog + * size. */ + let changed = true; + while (changed) { + changed = false; + const counts = new Map(); + finalNames.forEach((result) => { + counts.set(result, (counts.get(result) ?? 0) + 1); + }); + finalNames.forEach((result, raw) => { + if (result !== raw && (counts.get(result) ?? 0) > 1) { + finalNames.set(raw, raw); + changed = true; + } + }); + } + return finalNames; +} + export function splitMCPToolKey( toolKey: string, knownServerNames?: readonly string[], diff --git a/packages/data-provider/src/schemas.ts b/packages/data-provider/src/schemas.ts index ba40501687..68f435a948 100644 --- a/packages/data-provider/src/schemas.ts +++ b/packages/data-provider/src/schemas.ts @@ -765,6 +765,9 @@ export const tPluginSchema = z.object({ chatMenu: z.boolean().optional(), isButton: z.boolean().optional(), toolkit: z.boolean().optional(), + /** Raw upstream tool name when the model-facing key stripped a redundant + * server-name prefix — proves upstream identity for legacy id migration. */ + serverToolName: z.string().optional(), }); export type TPlugin = z.infer; diff --git a/packages/data-provider/src/splitMCPToolKey.spec.ts b/packages/data-provider/src/splitMCPToolKey.spec.ts index 1b05ac404d..e39d55afc9 100644 --- a/packages/data-provider/src/splitMCPToolKey.spec.ts +++ b/packages/data-provider/src/splitMCPToolKey.spec.ts @@ -4,6 +4,8 @@ import { splitToolCallName, normalizeMCPToolKey, buildServerNameAliases, + stripServerNamePrefix, + stripServerNamePrefixes, } from './config'; describe('splitMCPToolKey', () => { @@ -208,3 +210,127 @@ describe('splitToolCallName oauth precedence', () => { ]); }); }); + +describe('stripServerNamePrefix', () => { + it('strips a leading server-name prefix from the tool name', () => { + expect(stripServerNamePrefix('acme_trace_top_time_consuming_operations', 'acme')).toBe( + 'trace_top_time_consuming_operations', + ); + }); + + it('matches the prefix case-insensitively', () => { + /** Display-cased server names ("Acme") conventionally prefix their + * tools in lowercase — the redundancy is the same either way. */ + expect(stripServerNamePrefix('acme_list_services', 'Acme')).toBe('list_services'); + }); + + it('returns the name unchanged when the prefix does not match', () => { + expect(stripServerNamePrefix('github_create_issue', 'acme')).toBe('github_create_issue'); + }); + + it('requires the underscore separator, not a bare substring match', () => { + expect(stripServerNamePrefix('acmecorp_tool', 'acme')).toBe('acmecorp_tool'); + }); + + it('keeps a name that is exactly the server name or would strip to empty', () => { + expect(stripServerNamePrefix('acme', 'acme')).toBe('acme'); + expect(stripServerNamePrefix('acme_', 'acme')).toBe('acme_'); + }); +}); + +describe('stripServerNamePrefixes', () => { + it('maps every raw name to its stripped model-facing name', () => { + const map = stripServerNamePrefixes(['acme_search', 'list_services'], 'acme'); + expect(map.get('acme_search')).toBe('search'); + expect(map.get('list_services')).toBe('list_services'); + }); + + it('keeps the prefixed name when stripping would collide with a bare sibling', () => { + /** A server exposing BOTH `search` and `acme_search` must keep two + * distinct keys — stripping would collapse them into one. */ + const map = stripServerNamePrefixes(['search', 'acme_search'], 'acme'); + expect(map.get('search')).toBe('search'); + expect(map.get('acme_search')).toBe('acme_search'); + }); + + it('keeps both raw names when case-variant prefixed siblings strip to the same result', () => { + /** The prefix match is case-insensitive, so `acme_Foo` and `Acme_Foo` are + * distinct upstream tools with the SAME stripped remainder — both must + * fall back to their raw names or one silently overwrites the other. */ + const map = stripServerNamePrefixes(['acme_Foo', 'Acme_Foo'], 'acme'); + expect(map.get('acme_Foo')).toBe('acme_Foo'); + expect(map.get('Acme_Foo')).toBe('Acme_Foo'); + }); + + it('collisions do not suppress stripping of unrelated siblings', () => { + const map = stripServerNamePrefixes(['search', 'acme_search', 'acme_trace'], 'acme'); + expect(map.get('acme_trace')).toBe('trace'); + }); + + it('reserves every sibling raw name, even when that sibling itself strips', () => { + /** Keys persisted BEFORE stripping embed raw names: if `acme_acme_foo` + * stripped to `acme_foo`, a pre-rollout reference to the REAL `acme_foo` + * would exact-match the wrong tool in the same snapshot. */ + const map = stripServerNamePrefixes(['acme_foo', 'acme_acme_foo'], 'acme'); + expect(map.get('acme_foo')).toBe('foo'); + expect(map.get('acme_acme_foo')).toBe('acme_acme_foo'); + }); + + it('resolves secondary collisions introduced by a fallback to a raw name', () => { + /** `acme_foo` falls back to raw because of the bare `foo`, which then + * collides with `acme_acme_foo`'s stripped result — the guard must + * iterate until no two final names coincide. */ + const map = stripServerNamePrefixes(['foo', 'acme_foo', 'acme_acme_foo'], 'acme'); + expect(map.get('foo')).toBe('foo'); + expect(map.get('acme_foo')).toBe('acme_foo'); + expect(map.get('acme_acme_foo')).toBe('acme_acme_foo'); + expect(new Set(map.values()).size).toBe(3); + }); + + it('never strips a remainder that equals a synthetic MCP marker', () => { + /** `sys__all__sys` keys expand to every server tool, `sys__server__sys` + * keys are skipped as UI placeholders, and `oauth${mcp_delimiter}` names + * get OAuth-only handling in the client stream handlers — a real + * upstream tool must not be renamed onto any of them. */ + expect(stripServerNamePrefix(`acme_${Constants.mcp_all}`, 'acme')).toBe( + `acme_${Constants.mcp_all}`, + ); + expect(stripServerNamePrefix(`acme_${Constants.mcp_server}`, 'acme')).toBe( + `acme_${Constants.mcp_server}`, + ); + expect(stripServerNamePrefix('acme_oauth', 'acme')).toBe('acme_oauth'); + /** Each marker is consumed by PREFIX (`isMCPAllPlaceholder`, the + * server-pin skip, the client's OAuth classification), so the whole + * `${marker}${mcp_delimiter}` namespace stays raw, not just the exact + * name. */ + expect(stripServerNamePrefix(`acme_oauth${Constants.mcp_delimiter}reset`, 'acme')).toBe( + `acme_oauth${Constants.mcp_delimiter}reset`, + ); + expect( + stripServerNamePrefix(`acme_${Constants.mcp_all}${Constants.mcp_delimiter}reset`, 'acme'), + ).toBe(`acme_${Constants.mcp_all}${Constants.mcp_delimiter}reset`); + expect( + stripServerNamePrefix(`acme_${Constants.mcp_server}${Constants.mcp_delimiter}reset`, 'acme'), + ).toBe(`acme_${Constants.mcp_server}${Constants.mcp_delimiter}reset`); + /** `mcp_` opens the server-scoped pluginKey namespace and + * `lc_transfer_to_` the agent-handoff namespace — pre-strip tool keys + * could never enter either. */ + expect(stripServerNamePrefix('acme_mcp_status', 'acme')).toBe('acme_mcp_status'); + expect(stripServerNamePrefix('acme_lc_transfer_to_status', 'acme')).toBe( + 'acme_lc_transfer_to_status', + ); + }); + + it('never flips isActionTool classification for the produced key', () => { + /** `isActionTool` compares the FIRST `_action_` and `_mcp_` positions; + * stripping moves `_mcp_` earlier, so a server whose normalized name + * contains `_action_` (e.g. "svc action v1") would see a real MCP tool + * reclassified as an OpenAPI action and bypass MCP authorization. */ + expect(stripServerNamePrefix('svc_action_v1_report', 'svc_action_v1')).toBe( + 'svc_action_v1_report', + ); + /** A remainder containing `_action_` in the tool half does not flip and + * still strips. */ + expect(stripServerNamePrefix('acme_do_action_thing', 'acme')).toBe('do_action_thing'); + }); +}); diff --git a/packages/data-provider/src/types/queries.ts b/packages/data-provider/src/types/queries.ts index 8d95a94a91..2a481d3df5 100644 --- a/packages/data-provider/src/types/queries.ts +++ b/packages/data-provider/src/types/queries.ts @@ -130,6 +130,9 @@ export type MCPTool = { name: string; pluginKey: string; description: string; + /** Raw upstream tool name when the model-facing key stripped a redundant + * server-name prefix — gates the agent editor's legacy id migration. */ + serverToolName?: string; }; export type MCPServer = {