diff --git a/api/app/clients/tools/util/handleTools.js b/api/app/clients/tools/util/handleTools.js index 38a8d8b26b..99f7d2fedc 100644 --- a/api/app/clients/tools/util/handleTools.js +++ b/api/app/clients/tools/util/handleTools.js @@ -6,6 +6,7 @@ const { createSafeUser, mcpToolPattern, loadWebSearchAuth, + splitMCPToolKey, buildInlineMemoryTool, getCodeApiAuthHeaders, buildImageToolContext, @@ -45,7 +46,7 @@ const { createMCPTool, createMCPTools, createMCPPermissionContext, - resolveConfigServers, + resolveMcpServerContext, } = require('~/server/services/MCP'); const { getMCPRequestContext } = require('~/server/services/MCPRequestContext'); const { createFileSearchTool, primeFiles: primeSearchFiles } = require('./fileSearch'); @@ -285,8 +286,13 @@ const loadTools = async ({ /** Resolve config-source servers for the current user/tenant context */ let configServers; + /** All configured names, in the normalized form tool keys carry */ + let mcpServerNames = []; if (hasMCPTools && canUseMCP) { - configServers = await resolveConfigServers(options.req); + /** Reuse the caller's context when it already resolved one, so the chat + * startup path reads the request app config once. */ + ({ configServers, serverNames: mcpServerNames } = + options.mcpServerContext ?? (await resolveMcpServerContext(options.req))); } for (const tool of tools) { @@ -396,7 +402,7 @@ const loadTools = async ({ continue; } - const [toolName, serverName] = tool.split(Constants.mcp_delimiter); + const [toolName, serverName] = splitMCPToolKey(tool, mcpServerNames); if (toolName === Constants.mcp_server) { /** Placeholder used for UI purposes */ continue; diff --git a/api/app/clients/tools/util/handleTools.test.js b/api/app/clients/tools/util/handleTools.test.js index 50be5798ec..6987e61091 100644 --- a/api/app/clients/tools/util/handleTools.test.js +++ b/api/app/clients/tools/util/handleTools.test.js @@ -42,6 +42,7 @@ jest.mock('~/server/services/MCP', () => ({ canUseServers: jest.fn().mockResolvedValue(true), })), resolveConfigServers: jest.fn().mockResolvedValue({}), + resolveMcpServerContext: jest.fn(async () => ({ configServers: {}, serverNames: [] })), })); jest.mock('~/config', () => ({ @@ -357,6 +358,54 @@ describe('Tool Handlers', () => { ); }); + it('resolves an MCP tool whose raw name itself contains the delimiter substring', async () => { + // Regression test for https://github.com/danny-avila/LibreChat/issues/14440: + // gateways that prefix aggregated tool names by server (e.g. LiteLLM's + // MCP proxy) can produce a raw tool name that already contains "_mcp_" + // (e.g. GitLab's own "get_mcp_server_version" tool becomes + // "gitlab-get_mcp_server_version" once gateway-prefixed). Once + // LibreChat appends its own server suffix, the combined key has the + // delimiter twice - a naive split used to silently derive the wrong + // server name ("server_version" instead of "gitlab") and drop the tool. + const serverName = 'gitlab'; + const rawToolName = 'gitlab-get_mcp_server_version'; + const toolKey = `${rawToolName}${Constants.mcp_delimiter}${serverName}`; + const serverConfig = { + type: 'streamable-http', + url: 'https://litellm.example.com/gitlab/mcp', + source: 'yaml', + }; + + mockGetServerConfig.mockResolvedValue(serverConfig); + mockCreateMCPTool.mockResolvedValue({ name: 'loaded-mcp-tool' }); + + const result = await loadTools({ + user: fakeUser._id.toString(), + tools: [toolKey], + options: { + req: { + user: { id: fakeUser._id.toString(), role: 'USER' }, + }, + }, + }); + + expect(result.loadedTools).toEqual([{ name: 'loaded-mcp-tool' }]); + expect(mockGetServerConfig).toHaveBeenCalledWith( + serverName, + expect.anything(), + expect.anything(), + ); + expect(mockCreateMCPTool).toHaveBeenCalledWith( + expect.objectContaining({ + toolKey, + config: serverConfig, + /** The resolved server rides along, so `createMCPTool` uses it for auth, + * reconnection and invocation instead of re-parsing the ambiguous key. */ + serverName, + }), + ); + }); + it('uses run-scoped MCP tool definitions before cache lookup', async () => { const serverName = 'body-scoped'; const toolKey = `search${Constants.mcp_delimiter}${serverName}`; diff --git a/api/server/controllers/agents/filterAuthorizedTools.spec.js b/api/server/controllers/agents/filterAuthorizedTools.spec.js index 677bccdfe0..97e0e28d0c 100644 --- a/api/server/controllers/agents/filterAuthorizedTools.spec.js +++ b/api/server/controllers/agents/filterAuthorizedTools.spec.js @@ -342,23 +342,58 @@ describe('MCP Tool Authorization', () => { expect(result).toEqual(['web_search']); }); - test('should not preserve malformed existing tools when registry is unavailable', async () => { + test('should not preserve a tool key with no delimiter at all when registry is unavailable', async () => { + // A key that isn't a real MCP tool key (no delimiter, so it has no + // resolvable server) is rejected regardless of the existing-tools + // fallback - unlike a key with multiple delimiters, which does have a + // resolvable server (the segment after the last delimiter) and is + // covered separately below. getMCPServersRegistry.mockImplementation(() => { throw new Error('MCPServersRegistry has not been initialized.'); }); - const malformedTool = `a${d}b${d}c`; + // Deliberately not named anything containing "_mcp_" - that would + // ironically make it an MCP tool key itself, exactly the class of + // naming collision this whole regression is about. (Confirmed + // programmatically, not just by eye - it's an easy mistake to repeat.) + const noDelimiterTool = 'regular_web_tool'; const result = await filterAuthorizedTools({ - tools: [malformedTool, `legit${d}serverA`, 'web_search'], + tools: [noDelimiterTool, `legit${d}serverA`, 'web_search'], userId, user: testUser, availableTools, - existingTools: [malformedTool, `legit${d}serverA`], + existingTools: [noDelimiterTool, `legit${d}serverA`], }); expect(result).toContain(`legit${d}serverA`); expect(result).toContain('web_search'); - expect(result).not.toContain(malformedTool); + expect(result).not.toContain(noDelimiterTool); + }); + + test('should preserve an existing MCP tool key with multiple delimiters when registry is unavailable', async () => { + // Regression test for https://github.com/danny-avila/LibreChat/issues/14440: + // a tool key with more than one delimiter occurrence is not inherently + // malformed - it just means the raw tool-name half (everything before + // the *last* delimiter) itself contains the delimiter substring, which + // legitimately happens with some upstream MCP tool names. The + // registry-unavailable fallback should treat it like any other + // previously-persisted tool, not single it out as broken. + getMCPServersRegistry.mockImplementation(() => { + throw new Error('MCPServersRegistry has not been initialized.'); + }); + + const multiDelimiterTool = `a${d}b${d}c`; + const result = await filterAuthorizedTools({ + tools: [multiDelimiterTool, `legit${d}serverA`, 'web_search'], + userId, + user: testUser, + availableTools, + existingTools: [multiDelimiterTool, `legit${d}serverA`], + }); + + expect(result).toContain(multiDelimiterTool); + expect(result).toContain(`legit${d}serverA`); + expect(result).toContain('web_search'); }); test('should gate app-level MCP tools present in the global tool cache', async () => { @@ -398,12 +433,29 @@ describe('MCP Tool Authorization', () => { expect(mockGetAllServerConfigs).not.toHaveBeenCalled(); }); - test('should reject malformed MCP tool keys with multiple delimiters', async () => { + test('should resolve MCP tool keys with multiple delimiters using the last segment as the server name', async () => { + // Regression test for https://github.com/danny-avila/LibreChat/issues/14440. + // A tool key with more than one delimiter occurrence is not inherently + // malformed - it means the raw tool-name half (the part before the + // *last* delimiter, which is always the segment LibreChat itself + // appends) legitimately contains the delimiter substring. Previously + // any key with >2 segments was rejected outright; now the server name + // is always the last segment, matching how the key is actually built. + // + // `multiSegmentTool` below has an unrelated string ("victimServer") + // embedded in its raw-tool-name half purely to prove there's no way to + // spoof a *different* server via that embedded text - only the real + // last segment ("authorizedServer") is ever consulted for + // authorization, so this does not grant access to anything the user + // isn't already allowed to use. + const multiSegmentTool = `attack${d}victimServer${d}authorizedServer`; + const unauthorizedMultiSegmentTool = `a${d}b${d}c${d}forbiddenServer`; + const result = await filterAuthorizedTools({ tools: [ - `attack${d}victimServer${d}authorizedServer`, + multiSegmentTool, `legit${d}authorizedServer`, - `a${d}b${d}c${d}d`, + unauthorizedMultiSegmentTool, 'web_search', ], userId, @@ -411,9 +463,14 @@ describe('MCP Tool Authorization', () => { availableTools, }); - expect(result).toEqual([`legit${d}authorizedServer`, 'web_search']); - expect(result).not.toContainEqual(expect.stringContaining('victimServer')); - expect(result).not.toContainEqual(expect.stringContaining(`a${d}b`)); + expect(result).toContain(multiSegmentTool); + expect(result).toContain(`legit${d}authorizedServer`); + expect(result).toContain('web_search'); + // The unrelated embedded text does not let the key resolve to a + // different, unauthorized server: only the true last segment + // ("forbiddenServer", not in the mocked server configs) is checked, + // and it's correctly rejected. + expect(result).not.toContain(unauthorizedMultiSegmentTool); }); }); @@ -691,6 +748,62 @@ describe('MCP Tool Authorization', () => { expect(updatedAgent.tools).toContain(`newTool${d}anotherServer`); }); + test('should drop mcpServerNames for a server detached in the same edit that adds another', async () => { + mockReq.user.id = existingAgentAuthorId.toString(); + mockReq.params.id = existingAgentId; + /** Swapping servers in one edit: authorizedServer loses its only tool while + * anotherServer gains one. Carrying the prior names forward wholesale would + * leave authorizedServer indexed, so its viewers would keep agent-scoped + * access to a server the agent no longer references. */ + mockReq.body = { tools: ['web_search', `newTool${d}anotherServer`] }; + + await updateAgentHandler(mockReq, mockRes); + + const agentInDb = await Agent.findOne({ id: existingAgentId }); + expect(agentInDb.tools).not.toContain(`existingTool${d}authorizedServer`); + expect(agentInDb.tools).toContain(`newTool${d}anotherServer`); + expect(agentInDb.mcpServerNames).toEqual(['anotherServer']); + }); + + test('should preserve resolved mcpServerNames when a non-owner retains MCP tools', async () => { + /** The shared-agent path keeps the existing MCP tools verbatim; re-deriving the + * index from their keys would turn a delimiter-bearing configured server into + * its trailing segment, which `ServerConfigsDB` then treats as a DB server. */ + await Agent.updateOne( + { id: existingAgentId }, + { + tools: ['web_search', `existingTool${d}Google${d}Workspace`], + mcpServerNames: [`Google${d}Workspace`], + }, + ); + mockUserCanUseMCPServers.mockResolvedValue(false); + mockReq.user.id = new mongoose.Types.ObjectId().toString(); + mockReq.params.id = existingAgentId; + mockReq.body = { tools: ['web_search', `existingTool${d}Google${d}Workspace`] }; + + await updateAgentHandler(mockReq, mockRes); + + const agentInDb = await Agent.findOne({ id: existingAgentId }); + expect(agentInDb.mcpServerNames).toEqual([`Google${d}Workspace`]); + expect(agentInDb.mcpServerNames).not.toContain('Workspace'); + }); + + test('should let persistence derive when an unindexed agent retains MCP tools', async () => { + /** A legacy or partially migrated agent can hold MCP tools with no stored + * mcpServerNames. Pinning the index to [] here would suppress the derivation + * in updateAgent and strip agent-scoped access to its DB-backed server. */ + await Agent.updateOne({ id: existingAgentId }, { $unset: { mcpServerNames: 1 } }); + mockReq.user.id = existingAgentAuthorId.toString(); + mockReq.params.id = existingAgentId; + mockReq.body = { tools: ['web_search', `existingTool${d}authorizedServer`] }; + + await updateAgentHandler(mockReq, mockRes); + + const agentInDb = await Agent.findOne({ id: existingAgentId }); + expect(agentInDb.tools).toContain(`existingTool${d}authorizedServer`); + expect(agentInDb.mcpServerNames).toEqual(['authorizedServer']); + }); + test('should not query MCP registry when no new MCP tools added', async () => { mockReq.user.id = existingAgentAuthorId.toString(); mockReq.params.id = existingAgentId; diff --git a/api/server/controllers/agents/v1.js b/api/server/controllers/agents/v1.js index 79d5527f1e..3fcdf53054 100644 --- a/api/server/controllers/agents/v1.js +++ b/api/server/controllers/agents/v1.js @@ -4,6 +4,8 @@ const { nanoid } = require('nanoid'); const { logger } = require('@librechat/data-schemas'); const { refreshS3Url, + splitMCPToolKey, + normalizeServerName, agentCreateSchema, agentUpdateSchema, refreshListAvatars, @@ -227,9 +229,12 @@ const filterAuthorizedTools = async ({ availableTools, existingTools, configServers, + resolvedServerNames, }) => { const filteredTools = []; let mcpServerConfigs; + /** normalized server name -> the raw key `mcpServerConfigs` is indexed by */ + let configNamesByNormalized = new Map(); let registryUnavailable = false; const existingToolSet = existingTools?.length ? new Set(existingTools) : null; const hasMCPTools = tools.some((tool) => tool?.includes(Constants.mcp_delimiter)); @@ -273,10 +278,18 @@ const filterAuthorizedTools = async ({ mcpServerConfigs = {}; registryUnavailable = true; } + configNamesByNormalized = new Map( + Object.keys(mcpServerConfigs).map((name) => [normalizeServerName(name), name]), + ); } - const parts = tool.split(Constants.mcp_delimiter); - if (parts.length !== 2) { + /** Tool keys embed the normalized server name; the config is keyed by the raw name. */ + const [, normalizedServerName] = splitMCPToolKey( + tool, + Array.from(configNamesByNormalized.keys()), + ); + const serverName = configNamesByNormalized.get(normalizedServerName) ?? normalizedServerName; + if (!serverName) { logger.warn( `[filterAuthorizedTools] Rejected malformed MCP tool key "${tool}" for user ${userId}`, ); @@ -288,14 +301,14 @@ const filterAuthorizedTools = async ({ continue; } - const [, serverName] = parts; - if (!serverName || !Object.hasOwn(mcpServerConfigs, serverName)) { + if (!Object.hasOwn(mcpServerConfigs, serverName)) { logger.warn( `[filterAuthorizedTools] Rejected MCP tool "${tool}" — server "${serverName}" not accessible to user ${userId}`, ); continue; } + resolvedServerNames?.add(serverName); filteredTools.push(tool); } @@ -458,6 +471,9 @@ const createAgentHandler = async (req, res) => { hasMCPTools ? resolveConfigServers(req) : Promise.resolve(undefined), ]); const mcpPermissionContext = createMCPPermissionContext(req); + /** Resolved during authorization, so persistence indexes the real server rather + * than a suffix guess - see the note on `filterAuthorizedTools`. */ + const resolvedServerNames = new Set(); agentData.tools = await filterAuthorizedTools({ tools, userId, @@ -466,7 +482,11 @@ const createAgentHandler = async (req, res) => { mcpPermissionContext, availableTools, configServers, + resolvedServerNames, }); + if (hasMCPTools) { + agentData.mcpServerNames = Array.from(resolvedServerNames); + } const agent = await db.createAgent(agentData); @@ -752,6 +772,8 @@ const updateAgentHandler = async (req, res) => { if (!(await mcpPermissionContext.canUseServers(req.user))) { if (editingOwnAgent) { updateData.tools = effectiveTools.filter((t) => !isMCPTool(t)); + /** Every MCP tool just went away, so nothing should stay indexed. */ + updateData.mcpServerNames = []; } else if (hasToolUpdate) { const existingMCPToolSet = new Set(existingMCPTools); const nextTools = updateData.tools.filter( @@ -764,10 +786,19 @@ const updateAgentHandler = async (req, res) => { } } updateData.tools = nextTools; + /** The agent's MCP tools are retained verbatim here, so carry its resolved + * names across too. Left unset when the agent has none stored, so + * `updateAgent` can still derive rather than being pinned to an empty + * index that would strip agent-scoped access. */ + if (existingAgent.mcpServerNames?.length) { + updateData.mcpServerNames = existingAgent.mcpServerNames; + } } } else if (hasToolUpdate) { const existingToolSet = new Set(existingTools); const newMCPTools = requestedMCPTools.filter((t) => !existingToolSet.has(t)); + /** Names resolved during authorization of the newly added tools. */ + const resolvedServerNames = new Set(); if (newMCPTools.length > 0) { const [availableTools, configServers] = await Promise.all([ @@ -782,12 +813,38 @@ const updateAgentHandler = async (req, res) => { mcpPermissionContext, availableTools, configServers, + resolvedServerNames, }); const rejectedSet = new Set(newMCPTools.filter((t) => !approvedNew.includes(t))); if (rejectedSet.size > 0) { updateData.tools = updateData.tools.filter((t) => !rejectedSet.has(t)); } } + + /** Rebuild the index from the tools that survive this edit: carry a prior name + * forward only while some retained tool still resolves to it, so detaching every + * tool for a server revokes agent-scoped access to it. The agent's own persisted + * names are the candidate set, which needs neither a registry query nor a guess. */ + const priorNames = existingAgent.mcpServerNames ?? []; + if (priorNames.length > 0) { + const priorNameSet = new Set(priorNames); + for (const tool of updateData.tools ?? []) { + if (typeof tool !== 'string' || !tool.includes(Constants.mcp_delimiter)) { + continue; + } + const [, retainedName] = splitMCPToolKey(tool, priorNames); + if (retainedName && priorNameSet.has(retainedName)) { + resolvedServerNames.add(retainedName); + } + } + } + /** Supplying `[]` would pin the index empty and suppress `updateAgent`'s + * derivation, so only assert it when the result is authoritative: either we + * resolved names, or no MCP tool survives and the index genuinely is empty. */ + const retainsMCPTools = (updateData.tools ?? []).some(isMCPTool); + if (resolvedServerNames.size > 0 || !retainsMCPTools) { + updateData.mcpServerNames = Array.from(resolvedServerNames); + } } } @@ -965,6 +1022,9 @@ const duplicateAgentHandler = async (req, res) => { resolveConfigServers(req), ]); const mcpPermissionContext = createMCPPermissionContext(req); + /** The duplicate carries the source agent's `mcpServerNames`; replace it with what + * this user is actually authorized for, or the copy would grant the source's servers. */ + const resolvedServerNames = new Set(); newAgentData.tools = await filterAuthorizedTools({ tools: newAgentData.tools, userId, @@ -974,7 +1034,25 @@ const duplicateAgentHandler = async (req, res) => { availableTools, existingTools: newAgentData.tools, configServers, + resolvedServerNames, }); + /** When the registry is unavailable, `filterAuthorizedTools` grandfathers the + * source's tools without resolving them, so carry forward the source names those + * retained tools still point at rather than blanking the index. */ + const sourceNames = agent.mcpServerNames ?? []; + if (sourceNames.length > 0) { + const sourceNameSet = new Set(sourceNames); + for (const tool of newAgentData.tools ?? []) { + if (typeof tool !== 'string' || !tool.includes(Constants.mcp_delimiter)) { + continue; + } + const [, retainedName] = splitMCPToolKey(tool, sourceNames); + if (retainedName && sourceNameSet.has(retainedName)) { + resolvedServerNames.add(retainedName); + } + } + } + newAgentData.mcpServerNames = Array.from(resolvedServerNames); } if (newAgentData.tool_resources) { diff --git a/api/server/controllers/mcp.js b/api/server/controllers/mcp.js index c3d3fc46df..bc850e08b4 100644 --- a/api/server/controllers/mcp.js +++ b/api/server/controllers/mcp.js @@ -10,6 +10,7 @@ const { checkAccess, isUserSourced, MCPErrorCodes, + splitMCPToolKey, redactServerSecrets, redactAllServerSecrets, isMCPDomainNotAllowedError, @@ -177,7 +178,7 @@ const getMCPTools = async (req, res) => { continue; } - const toolName = toolKey.split(Constants.mcp_delimiter)[0]; + const [toolName] = splitMCPToolKey(toolKey, [serverName]); server.tools.push({ name: toolName, pluginKey: toolKey, diff --git a/api/server/services/MCP.js b/api/server/services/MCP.js index 9ac936f57c..18355d99a9 100644 --- a/api/server/services/MCP.js +++ b/api/server/services/MCP.js @@ -6,7 +6,9 @@ const { PENDING_STALE_MS, MCPOAuthHandler, isMCPDomainAllowed, + splitMCPToolKey, normalizeServerName, + resolveMCPServerContext, normalizeJsonSchema, GenerationJobManager, resolveJsonSchemaRefs, @@ -143,6 +145,50 @@ async function resolveMcpConfigNames(req) { return Object.keys(appConfig?.mcpConfig || {}); } +/** + * All configured server names in the normalized form tool keys are built with. + * Unlike `resolveConfigServers`, this keeps unmodified YAML servers, which + * `ensureConfigServers` skips - those are exactly the ones that must still + * resolve the tool-key boundary. + * @param {import('express').Request} req + * @returns {Promise} + */ +async function resolveMcpServerNames(req) { + try { + const names = await resolveMcpConfigNames(req); + return names.map(normalizeServerName); + } catch (error) { + logger.warn( + '[resolveMcpServerNames] Failed to resolve server names, degrading to empty:', + error, + ); + return []; + } +} + +/** + * Config-source servers and all configured names from a single app-config read, + * so the tool-loading path does not pay two lookups for the same principal. + * Degrades to empty like `resolveConfigServers` rather than aborting tool loading. + * @param {import('express').Request} req + * @returns {Promise<{ configServers: Record, serverNames: string[] }>} + */ +async function resolveMcpServerContext(req) { + try { + const appConfig = await getAppConfigForRequest(req); + return await resolveMCPServerContext({ + mcpConfig: appConfig?.mcpConfig || {}, + ensureConfigServers: (mcpConfig) => getMCPServersRegistry().ensureConfigServers(mcpConfig), + }); + } catch (error) { + logger.warn( + '[resolveMcpServerContext] Failed to resolve MCP servers, degrading to empty:', + error, + ); + return { configServers: {}, serverNames: [] }; + } +} + /** * Resolves config-source servers and merges all server configs (YAML + config + user DB) * for the given user context. Shared helper for controllers needing the full merged config. @@ -613,6 +659,7 @@ async function createMCPTools({ streamId, jobCreatedAt, availableTools: result.availableTools, + serverName, toolKey: `${tool.name}${Constants.mcp_delimiter}${serverName}`, requestBody, requestScopedConnections, @@ -661,11 +708,22 @@ async function createMCPTool({ requestScopedConnections, config, configServers, + serverName: resolvedServerName, onAvailableTools, streamId = null, jobCreatedAt, }) { - const [toolName, serverName] = toolKey.split(Constants.mcp_delimiter); + /** `loadTools` already resolved the server for this key; parsing is the fallback. */ + const [parsedToolName, parsedServerName] = splitMCPToolKey( + toolKey, + /** Tool keys embed the normalized server name, so the candidate list must be + * normalized too or a name needing normalization never matches. */ + resolvedServerName + ? [normalizeServerName(resolvedServerName)] + : Object.keys(configServers ?? {}).map(normalizeServerName), + ); + const serverName = resolvedServerName ?? parsedServerName; + const toolName = parsedToolName; const serverConfig = config ?? (await getMCPServersRegistry().getServerConfig(serverName, user?.id, configServers)); @@ -1094,6 +1152,8 @@ module.exports = { userCanUseMCPServers, getMCPSetupData, resolveConfigServers, + resolveMcpServerNames, + resolveMcpServerContext, resolveMcpConfigNames, resolveAllMcpConfigs, createOAuthStart, diff --git a/api/server/services/ToolService.js b/api/server/services/ToolService.js index ec28d15b55..3226bbe479 100644 --- a/api/server/services/ToolService.js +++ b/api/server/services/ToolService.js @@ -69,7 +69,7 @@ const { primeFiles: primeCodeFiles } = require('~/server/services/Files/Code/pro const { manifestToolMap, toolkits } = require('~/app/clients/tools/manifest'); const { createOnSearchResults } = require('~/server/services/Tools/search'); const { reinitMCPServer } = require('~/server/services/Tools/mcp'); -const { createMCPPermissionContext, resolveConfigServers } = require('~/server/services/MCP'); +const { createMCPPermissionContext, resolveMcpServerContext } = require('~/server/services/MCP'); const { getMCPRequestContext } = require('~/server/services/MCPRequestContext'); const { recordUsage } = require('~/server/services/Threads'); const { loadTools } = require('~/app/clients/tools/util'); @@ -607,19 +607,26 @@ async function loadToolDefinitionsWrapper({ return { toolDefinitions: [] }; } + /** Only MCP tool keys need the server context; a purely non-MCP agent should not + * pay an app-config lookup on startup. */ + const hasFilteredMCPTools = filteredTools.some((t) => t.includes(Constants.mcp_delimiter)); + const { configServers, serverNames: mcpServerNames } = hasFilteredMCPTools + ? await resolveMcpServerContext(req) + : { configServers: {}, serverNames: [] }; + /** @type {Record>} */ let userMCPAuthMap; - if (filteredTools?.some((t) => t.includes(Constants.mcp_delimiter))) { + if (hasFilteredMCPTools) { userMCPAuthMap = await getUserMCPAuthMap({ tools: filteredTools, userId: req.user.id, + serverNames: mcpServerNames, findPluginAuthsByKeys, }); } const flowsCache = getLogStores(CacheKeys.FLOWS); const flowManager = getFlowStateManager(flowsCache); - const configServers = await resolveConfigServers(req); const pendingOAuthServers = new Set(); const pendingOAuthStarts = new Map(); const emittedOAuthStarts = new Map(); @@ -885,6 +892,7 @@ async function loadToolDefinitionsWrapper({ programmaticToolsEnabled, codeExecutionEnabled, provider: agent.provider, + mcpServerNames, }, { isBuiltInTool, @@ -893,7 +901,7 @@ async function loadToolDefinitionsWrapper({ }, ); - for (const serverName of getMCPServerNamesFromTools(filteredTools)) { + for (const serverName of getMCPServerNamesFromTools(filteredTools, mcpServerNames)) { if (pendingOAuthServers.has(serverName)) { continue; } @@ -967,6 +975,7 @@ async function loadToolDefinitionsWrapper({ programmaticToolsEnabled, codeExecutionEnabled, provider: agent.provider, + mcpServerNames, }, { isBuiltInTool, @@ -1181,12 +1190,18 @@ async function loadAgentTools({ webSearchCallbacks = createOnSearchResults(res, streamId, jobCreatedAt); } + /** Resolved once and threaded into `loadTools` so the request app config is read once. */ + const mcpServerContext = _agentTools?.some((t) => t.includes(Constants.mcp_delimiter)) + ? await resolveMcpServerContext(req) + : undefined; + /** @type {Record>} */ let userMCPAuthMap; - if (_agentTools?.some((t) => t.includes(Constants.mcp_delimiter))) { + if (mcpServerContext) { userMCPAuthMap = await getUserMCPAuthMap({ tools: _agentTools, userId: req.user.id, + serverNames: mcpServerContext.serverNames, findPluginAuthsByKeys, }); } @@ -1201,6 +1216,7 @@ async function loadAgentTools({ options: { req, res, + mcpServerContext, jobCreatedAt, openAIApiKey, tool_resources, diff --git a/api/server/services/__tests__/MCP.spec.js b/api/server/services/__tests__/MCP.spec.js index ca3d5eea9a..81848ebadb 100644 --- a/api/server/services/__tests__/MCP.spec.js +++ b/api/server/services/__tests__/MCP.spec.js @@ -32,6 +32,13 @@ jest.mock('@librechat/api', () => ({ GenerationJobManager: jest.fn(), resolveJsonSchemaRefs: jest.fn((schema) => schema), buildOAuthToolCallName: jest.fn((name) => name), + /** Mirrors the real resolver so these tests still exercise the wrapper's own + * plumbing - loading the request config and degrading on failure - rather than + * the resolution logic, which is unit-tested in packages/api. */ + resolveMCPServerContext: jest.fn(async ({ mcpConfig, ensureConfigServers }) => ({ + configServers: await ensureConfigServers(mcpConfig), + serverNames: Object.keys(mcpConfig), + })), })); jest.mock('~/cache', () => ({ getLogStores: jest.fn() })); @@ -54,7 +61,12 @@ jest.mock('~/server/services/Tools/mcp', () => ({ })); const { getAppConfig } = require('~/server/services/Config'); -const { resolveConfigServers, resolveMcpConfigNames, resolveAllMcpConfigs } = require('../MCP'); +const { + resolveConfigServers, + resolveMcpConfigNames, + resolveAllMcpConfigs, + resolveMcpServerContext, +} = require('../MCP'); describe('resolveConfigServers', () => { beforeEach(() => jest.clearAllMocks()); @@ -99,6 +111,42 @@ describe('resolveConfigServers', () => { }); }); +describe('resolveMcpServerContext', () => { + beforeEach(() => jest.clearAllMocks()); + + it('derives config servers and all configured names from a single app-config read', async () => { + /** `ensureConfigServers` intentionally omits unmodified YAML servers, so the name + * list must come from `mcpConfig` itself or boundary resolution goes inert. */ + getAppConfig.mockResolvedValue({ mcpConfig: { unchangedYaml: {}, lazyInit: {} } }); + mockRegistry.ensureConfigServers.mockResolvedValue({ lazyInit: { name: 'lazyInit' } }); + + const result = await resolveMcpServerContext({ user: { id: 'u1' } }); + + expect(result.configServers).toEqual({ lazyInit: { name: 'lazyInit' } }); + expect(result.serverNames.sort()).toEqual(['lazyInit', 'unchangedYaml']); + expect(getAppConfig).toHaveBeenCalledTimes(1); + }); + + it('degrades to empty rather than rejecting when the config lookup fails', async () => { + /** A rejection here would abort tool loading entirely, defeating the + * catch-and-degrade the sibling resolver already provides. */ + getAppConfig.mockRejectedValue(new Error('db timeout')); + + const result = await resolveMcpServerContext({ user: { id: 'u1' } }); + + expect(result).toEqual({ configServers: {}, serverNames: [] }); + }); + + it('degrades to empty when ensureConfigServers throws', async () => { + getAppConfig.mockResolvedValue({ mcpConfig: { srv: {} } }); + mockRegistry.ensureConfigServers.mockRejectedValue(new Error('inspect failed')); + + const result = await resolveMcpServerContext({ user: { id: 'u1' } }); + + expect(result).toEqual({ configServers: {}, serverNames: [] }); + }); +}); + describe('resolveMcpConfigNames', () => { beforeEach(() => jest.clearAllMocks()); diff --git a/api/server/services/__tests__/ToolService.spec.js b/api/server/services/__tests__/ToolService.spec.js index 6e902de5cd..3b4a31d1b5 100644 --- a/api/server/services/__tests__/ToolService.spec.js +++ b/api/server/services/__tests__/ToolService.spec.js @@ -45,6 +45,7 @@ const mockCreateActionTool = jest.fn(); const mockGetServerConfig = jest.fn(); const mockFlowManager = { getFlowState: jest.fn() }; const mockResolveConfigServers = jest.fn(); +const mockResolveMcpServerNames = jest.fn(); const mockUserCanUseMCPServers = jest.fn().mockResolvedValue(true); jest.mock('~/server/services/Tools/credentials', () => ({ loadAuthValues: jest.fn().mockResolvedValue({}), @@ -86,6 +87,11 @@ jest.mock('~/config', () => ({ })); jest.mock('~/server/services/MCP', () => ({ resolveConfigServers: (...args) => mockResolveConfigServers(...args), + resolveMcpServerNames: (...args) => mockResolveMcpServerNames(...args), + resolveMcpServerContext: async (...args) => { + const configServers = (await mockResolveConfigServers(...args)) ?? {}; + return { configServers, serverNames: Object.keys(configServers) }; + }, createMCPPermissionContext: jest.fn((req) => ({ canUseServers: (user) => mockUserCanUseMCPServers(user, req), })), @@ -140,6 +146,7 @@ describe('ToolService - Action Capability Gating', () => { mockGetServerConfig.mockResolvedValue(undefined); mockFlowManager.getFlowState.mockResolvedValue(undefined); mockResolveConfigServers.mockResolvedValue({}); + mockResolveMcpServerNames.mockResolvedValue([]); }); describe('resolveAgentCapabilities', () => { @@ -679,6 +686,9 @@ describe('ToolService - Action Capability Gating', () => { const mcpTool = `search${Constants.mcp_delimiter}${serverName}`; const capabilities = [AgentCapabilities.tools]; const req = createMockReq(capabilities); + /** A server whose own name contains the delimiter is only resolvable + * against the configured set, so the key boundary is unambiguous. */ + mockResolveConfigServers.mockResolvedValue({ [serverName]: {} }); const res = { writableEnded: false }; mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities)); mockFlowManager.getFlowState.mockResolvedValue({ diff --git a/client/src/components/Chat/Messages/Content/Parts/SubagentCall.tsx b/client/src/components/Chat/Messages/Content/Parts/SubagentCall.tsx index 3c58576103..879b48d340 100644 --- a/client/src/components/Chat/Messages/Content/Parts/SubagentCall.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/SubagentCall.tsx @@ -16,6 +16,7 @@ import { MessageContext } from '~/Providers/MessageContext'; import MessageIcon from '~/components/Share/MessageIcon'; import { subagentProgressByToolCallId } from '~/store'; import { useAgentsMapContext } from '~/Providers'; +import { useMCPServerNames } from '~/hooks/MCP'; import { AttachmentGroup } from './Attachment'; import { useLocalize } from '~/hooks'; import Reasoning from './Reasoning'; @@ -704,11 +705,13 @@ function ToolNameBadge({ name }: { name: string }): JSX.Element { function ToolIdentifier({ rawName, localize, + mcpServerNames, }: { rawName: string; localize: ReturnType; + mcpServerNames?: readonly string[]; }): JSX.Element { - const parsed = parseToolName(rawName); + const parsed = parseToolName(rawName, mcpServerNames); if (parsed.mcpServer) { return ( @@ -740,6 +743,7 @@ function ToolIdentifier({ */ function TickerLineView({ line }: { line: SubagentTickerLine }): JSX.Element { const localize = useLocalize(); + const mcpServerNames = useMCPServerNames(); if (line.kind === 'writing' || line.kind === 'reasoning') { const prefix = line.kind === 'writing' @@ -766,7 +770,7 @@ function TickerLineView({ line }: { line: SubagentTickerLine }): JSX.Element { {line.toolNames.map((name, i) => ( {i > 0 && ,} - + ))} {line.argsSnippet && ( @@ -779,7 +783,11 @@ function TickerLineView({ line }: { line: SubagentTickerLine }): JSX.Element { if (line.kind === 'tool_complete') { return (
  • - + ({ ), })); +jest.mock('~/hooks/MCP', () => { + const mcpServerNames: string[] = []; + return { useMCPServerNames: () => mcpServerNames }; +}); + jest.mock('~/utils', () => ({ ...jest.requireActual('~/utils/groupToolCalls'), ...jest.requireActual('~/utils/toolLabels'), diff --git a/client/src/components/Chat/Messages/Content/ToolCall.tsx b/client/src/components/Chat/Messages/Content/ToolCall.tsx index 4eb0a3d0f9..bc3efc4605 100644 --- a/client/src/components/Chat/Messages/Content/ToolCall.tsx +++ b/client/src/components/Chat/Messages/Content/ToolCall.tsx @@ -7,11 +7,12 @@ import { dataService, actionDelimiter, actionDomainSeparator, + splitToolCallName, } from 'librechat-data-provider'; import type { TAttachment } from 'librechat-data-provider'; import { useLocalize, useProgress, useExpandCollapse } from '~/hooks'; import { ToolIcon, getToolIconType, isError } from './ToolOutput'; -import { useMCPIconMap } from '~/hooks/MCP'; +import { useMCPIconMap, useMCPServerNames } from '~/hooks/MCP'; import { AttachmentGroup } from './Parts'; import ToolCallInfo from './ToolCallInfo'; import ProgressText from './ProgressText'; @@ -66,14 +67,13 @@ export default function ToolCall({ } }, [auth]); + const mcpServerNames = useMCPServerNames(); const { function_name, domain, isMCPToolCall, mcpServerName } = useMemo(() => { if (typeof name !== 'string') { return { function_name: '', domain: null, isMCPToolCall: false, mcpServerName: '' }; } if (name.includes(Constants.mcp_delimiter)) { - const parts = name.split(Constants.mcp_delimiter); - const func = parts[0]; - const server = parts.slice(1).join(Constants.mcp_delimiter); + const [func, server = ''] = splitToolCallName(name, mcpServerNames); const displayName = func === 'oauth' ? server : func; return { function_name: displayName || '', @@ -105,7 +105,7 @@ export default function ToolCall({ isMCPToolCall: false, mcpServerName: '', }; - }, [name, parsedAuthUrl]); + }, [name, parsedAuthUrl, mcpServerNames]); const toolIconType = useMemo(() => getToolIconType(name), [name]); const mcpIconMap = useMCPIconMap(); diff --git a/client/src/components/Chat/Messages/Content/ToolCallGroup.tsx b/client/src/components/Chat/Messages/Content/ToolCallGroup.tsx index 1d86ac356d..5c386cc06d 100644 --- a/client/src/components/Chat/Messages/Content/ToolCallGroup.tsx +++ b/client/src/components/Chat/Messages/Content/ToolCallGroup.tsx @@ -10,11 +10,11 @@ import type { } from 'librechat-data-provider'; import type { PartWithIndex } from './ParallelContent'; import { useLocalize, useExpandCollapse, scheduleMessageContentLayoutReconcile } from '~/hooks'; +import { useMCPIconMap, useMCPServerNames } from '~/hooks/MCP'; import { isBashProgrammaticToolCall } from './routing'; import { ASK_USER_QUESTION } from '~/utils/approval'; import { cn, getToolDisplayLabel } from '~/utils'; import { StackedToolIcons } from './ToolOutput'; -import { useMCPIconMap } from '~/hooks/MCP'; import { AttachmentGroup } from './Parts'; import store from '~/store'; @@ -126,6 +126,7 @@ export default function ToolCallGroup({ }: ToolCallGroupProps) { const localize = useLocalize(); const mcpIconMap = useMCPIconMap(); + const mcpServerNames = useMCPServerNames(); const rootRef = useRef(null); const cancelLayoutReconcileRef = useRef<(() => void) | null>(null); const retainedForPendingApprovalRef = useRef(false); @@ -179,7 +180,7 @@ export default function ToolCallGroup({ const labels: string[] = []; for (const rawName of toolNames) { if (!rawName) continue; - const label = getToolDisplayLabel(rawName, localize); + const label = getToolDisplayLabel(rawName, localize, mcpServerNames); if (!seen.has(label)) { seen.add(label); labels.push(label); @@ -189,7 +190,7 @@ export default function ToolCallGroup({ return labels.join(', '); } return `${labels.slice(0, 3).join(', ')}, +${labels.length - 3}`; - }, [toolNames, localize]); + }, [toolNames, localize, mcpServerNames]); const autoExpand = useRecoilValue(store.autoExpandTools); const autoCollapse = !autoExpand && count >= 2 && allCompleted; diff --git a/client/src/components/Chat/Messages/Content/ToolOutput/StackedToolIcons.tsx b/client/src/components/Chat/Messages/Content/ToolOutput/StackedToolIcons.tsx index c7be3adc25..6ffd0d4c9d 100644 --- a/client/src/components/Chat/Messages/Content/ToolOutput/StackedToolIcons.tsx +++ b/client/src/components/Chat/Messages/Content/ToolOutput/StackedToolIcons.tsx @@ -1,6 +1,7 @@ import { useMemo } from 'react'; -import ToolIcon, { getToolIconType, getMCPServerName } from './ToolIcon'; import type { ToolIconType } from './ToolIcon'; +import ToolIcon, { getToolIconType, getMCPServerName } from './ToolIcon'; +import { useMCPServerNames } from '~/hooks/MCP'; import { cn } from '~/utils'; interface ResolvedIcon { @@ -22,12 +23,13 @@ export default function StackedToolIcons({ maxIcons = 3, isAnimating = false, }: StackedToolIconsProps) { + const mcpServerNames = useMCPServerNames(); const uniqueIcons = useMemo(() => { const seen = new Set(); const result: ResolvedIcon[] = []; for (const name of toolNames) { const type = getToolIconType(name); - const serverName = getMCPServerName(name); + const serverName = getMCPServerName(name, mcpServerNames); const iconUrl = serverName ? mcpIconMap?.get(serverName) : undefined; const key = iconUrl ? `mcp-${serverName}` : type; if (!seen.has(key)) { @@ -36,7 +38,7 @@ export default function StackedToolIcons({ } } return result; - }, [toolNames, mcpIconMap]); + }, [toolNames, mcpIconMap, mcpServerNames]); const visibleIcons = uniqueIcons.slice(0, maxIcons); const overflowCount = uniqueIcons.length - visibleIcons.length; diff --git a/client/src/components/Chat/Messages/Content/ToolOutput/ToolIcon.tsx b/client/src/components/Chat/Messages/Content/ToolOutput/ToolIcon.tsx index 9cf7d95306..f90dddc493 100644 --- a/client/src/components/Chat/Messages/Content/ToolOutput/ToolIcon.tsx +++ b/client/src/components/Chat/Messages/Content/ToolOutput/ToolIcon.tsx @@ -1,4 +1,4 @@ -import { Constants, isActionTool } from 'librechat-data-provider'; +import { Constants, isActionTool, splitToolCallName } from 'librechat-data-provider'; import { Terminal, Globe, @@ -91,13 +91,12 @@ export function getToolIconType(name: string): ToolIconType { } /** Extracts the MCP server name from a tool name with format `toolserver`. */ -export function getMCPServerName(toolName: string): string { - const idx = toolName.indexOf(Constants.mcp_delimiter); - if (idx < 0) { +export function getMCPServerName(toolName: string, knownServerNames?: readonly string[]): string { + if (!toolName.includes(Constants.mcp_delimiter)) { return ''; } - const afterDelimiter = toolName.slice(idx + Constants.mcp_delimiter.length); - return afterDelimiter || ''; + const [, serverName] = splitToolCallName(toolName, knownServerNames); + return serverName ?? ''; } interface ToolIconProps { diff --git a/client/src/components/Chat/Messages/Content/__tests__/ContentParts.integration.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/ContentParts.integration.test.tsx index 555bf7c4f6..050804db0a 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/ContentParts.integration.test.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/ContentParts.integration.test.tsx @@ -1,8 +1,8 @@ import React from 'react'; import { RecoilRoot } from 'recoil'; import { ContentTypes } from 'librechat-data-provider'; -import type { TAttachment, TMessageContentParts } from 'librechat-data-provider'; import { fireEvent, render, screen } from '@testing-library/react'; +import type { TAttachment, TMessageContentParts } from 'librechat-data-provider'; import ContentParts from '../ContentParts'; jest.mock('~/hooks', () => ({ @@ -20,9 +20,13 @@ jest.mock('~/hooks', () => ({ scheduleMessageContentLayoutReconcile: jest.fn(() => jest.fn()), })); -jest.mock('~/hooks/MCP', () => ({ - useMCPIconMap: () => new Map(), -})); +jest.mock('~/hooks/MCP', () => { + const mcpServerNames: string[] = []; + return { + useMCPIconMap: () => new Map(), + useMCPServerNames: () => mcpServerNames, + }; +}); jest.mock('../ToolOutput', () => ({ StackedToolIcons: () => , diff --git a/client/src/components/Chat/Messages/Content/__tests__/ToolCall.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/ToolCall.test.tsx index 81622392a3..1e9bd72273 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/ToolCall.test.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/ToolCall.test.tsx @@ -33,9 +33,13 @@ jest.mock('~/hooks', () => ({ }), })); -jest.mock('~/hooks/MCP', () => ({ - useMCPIconMap: () => new Map(), -})); +jest.mock('~/hooks/MCP', () => { + const mcpServerNames: string[] = []; + return { + useMCPIconMap: () => new Map(), + useMCPServerNames: () => mcpServerNames, + }; +}); jest.mock('~/components/Chat/Messages/Content/MessageContent', () => ({ __esModule: true, diff --git a/client/src/components/Chat/Messages/Content/__tests__/ToolCallGroup.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/ToolCallGroup.test.tsx index 10d61e4482..2328d916f5 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/ToolCallGroup.test.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/ToolCallGroup.test.tsx @@ -32,9 +32,13 @@ jest.mock('~/hooks', () => ({ scheduleMessageContentLayoutReconcile: jest.fn(() => jest.fn()), })); -jest.mock('~/hooks/MCP', () => ({ - useMCPIconMap: () => new Map(), -})); +jest.mock('~/hooks/MCP', () => { + const mcpServerNames: string[] = []; + return { + useMCPIconMap: () => new Map(), + useMCPServerNames: () => mcpServerNames, + }; +}); jest.mock('../ToolOutput', () => ({ StackedToolIcons: ({ toolNames }: { toolNames: string[] }) => ( diff --git a/client/src/hooks/MCP/__tests__/useVisibleTools.test.ts b/client/src/hooks/MCP/__tests__/useVisibleTools.test.ts new file mode 100644 index 0000000000..724b3980b5 --- /dev/null +++ b/client/src/hooks/MCP/__tests__/useVisibleTools.test.ts @@ -0,0 +1,43 @@ +import { renderHook } from '@testing-library/react'; +import { Constants } from 'librechat-data-provider'; +import type { TPlugin } from 'librechat-data-provider'; +import type { MCPServerInfo } from '~/common'; +import { useVisibleTools } from '../useVisibleTools'; + +const d = Constants.mcp_delimiter; + +describe('useVisibleTools', () => { + const regularTools: TPlugin[] = [{ name: 'Web Search', pluginKey: 'web_search' }] as TPlugin[]; + const mcpServersMap = new Map([ + ['gitlab', {} as MCPServerInfo], + ['myserver', {} as MCPServerInfo], + ]); + + it('resolves a normal single-delimiter MCP tool id to its server name', () => { + const { result } = renderHook(() => + useVisibleTools([`search${d}myserver`], regularTools, mcpServersMap), + ); + expect(result.current.mcpServerNames).toEqual(['myserver']); + expect(result.current.toolIds).toEqual([]); + }); + + it('resolves an MCP tool id whose raw tool name itself contains the delimiter substring', () => { + // Regression test for https://github.com/danny-avila/LibreChat/issues/14440: + // a raw MCP tool name that already contains "_mcp_" (e.g. one exposed + // through a gateway that prefixes tool names by server) must still + // resolve to the real server name - the *last* segment, not + // `.split(delimiter)[1]`, which would grab the wrong (middle) segment + // once there's more than one occurrence. + const toolId = `gitlab-get${d}server_version${d}gitlab`; + const { result } = renderHook(() => useVisibleTools([toolId], regularTools, mcpServersMap)); + expect(result.current.mcpServerNames).toEqual(['gitlab']); + }); + + it('keeps regular (non-MCP) tools separate from MCP server names', () => { + const { result } = renderHook(() => + useVisibleTools(['web_search'], regularTools, mcpServersMap), + ); + expect(result.current.toolIds).toEqual(['web_search']); + expect(result.current.mcpServerNames).toEqual([]); + }); +}); diff --git a/client/src/hooks/MCP/index.ts b/client/src/hooks/MCP/index.ts index b53003f3ce..b32bca419b 100644 --- a/client/src/hooks/MCP/index.ts +++ b/client/src/hooks/MCP/index.ts @@ -3,5 +3,5 @@ export * from './useVisibleTools'; export * from './useMCPServerManager'; export * from './useMCPConnectionStatus'; -export { useMCPIconMap } from './useMCPIconMap'; +export { useMCPIconMap, useMCPServerNames } from './useMCPIconMap'; export { useRemoveMCPTool } from './useRemoveMCPTool'; diff --git a/client/src/hooks/MCP/useMCPIconMap.ts b/client/src/hooks/MCP/useMCPIconMap.ts index 43f109b68c..00cc7a75e1 100644 --- a/client/src/hooks/MCP/useMCPIconMap.ts +++ b/client/src/hooks/MCP/useMCPIconMap.ts @@ -1,4 +1,5 @@ import { useMemo } from 'react'; +import { normalizeServerName } from 'librechat-data-provider'; import { useMCPServersQuery } from '~/data-provider'; export function useMCPIconMap(): Map { @@ -11,9 +12,20 @@ export function useMCPIconMap(): Map { } for (const [serverName, config] of Object.entries(servers)) { if (config.iconPath) { - map.set(serverName, config.iconPath); + /** Looked up with a server name parsed out of a tool key, which carries the + * normalized form, so key the map the same way. */ + map.set(normalizeServerName(serverName), config.iconPath); } } return map; }, [servers]); } + +/** + * Configured MCP server names in the normalized form tool keys are built from, + * so they can be matched against a key. The config is keyed by the raw name. + */ +export function useMCPServerNames(): string[] { + const { data: servers } = useMCPServersQuery(); + return useMemo(() => (servers ? Object.keys(servers).map(normalizeServerName) : []), [servers]); +} diff --git a/client/src/hooks/MCP/useVisibleTools.ts b/client/src/hooks/MCP/useVisibleTools.ts index 1e48d08914..015248d112 100644 --- a/client/src/hooks/MCP/useVisibleTools.ts +++ b/client/src/hooks/MCP/useVisibleTools.ts @@ -1,5 +1,5 @@ import { useMemo } from 'react'; -import { Constants } from 'librechat-data-provider'; +import { Constants, splitMCPToolKey } from 'librechat-data-provider'; import type { TPlugin } from 'librechat-data-provider'; import type { MCPServerInfo } from '~/common'; @@ -23,13 +23,14 @@ export function useVisibleTools( mcpServersMap: Map, ): VisibleToolsResult { return useMemo(() => { + const knownServerNames = Array.from(mcpServersMap.keys()); const mcpServers = new Set(); const regularToolIds: string[] = []; for (const toolId of selectedToolIds ?? []) { // MCP tools/servers if (toolId.includes(Constants.mcp_delimiter)) { - const serverName = toolId.split(Constants.mcp_delimiter)[1]; + const [, serverName] = splitMCPToolKey(toolId, knownServerNames); if (serverName) { mcpServers.add(serverName); } diff --git a/client/src/utils/toolLabels.ts b/client/src/utils/toolLabels.ts index ffb1c74630..58757b9ce8 100644 --- a/client/src/utils/toolLabels.ts +++ b/client/src/utils/toolLabels.ts @@ -1,4 +1,4 @@ -import { Constants } from 'librechat-data-provider'; +import { Constants, splitToolCallName } from 'librechat-data-provider'; import type { TranslationKeys } from '~/hooks'; /** @@ -46,11 +46,12 @@ export interface ParsedToolName { * - `web_search` → `{ mcpServer: '', toolName: 'web_search', friendlyKey: 'com_ui_tool_name_web_search' }` * - `some_custom_tool` → `{ mcpServer: '', toolName: 'some_custom_tool' }` */ -export function parseToolName(rawName: string): ParsedToolName { - const idx = rawName.indexOf(Constants.mcp_delimiter); - if (idx >= 0) { - const mcpServer = rawName.slice(idx + Constants.mcp_delimiter.length); - const toolName = rawName.slice(0, idx); +export function parseToolName( + rawName: string, + knownServerNames?: readonly string[], +): ParsedToolName { + if (rawName.includes(Constants.mcp_delimiter)) { + const [toolName, mcpServer = ''] = splitToolCallName(rawName, knownServerNames); return { raw: rawName, mcpServer, toolName }; } const friendlyKey = TOOL_FRIENDLY_NAME_KEYS[rawName]; @@ -74,8 +75,9 @@ export function parseToolName(rawName: string): ParsedToolName { export function getToolDisplayLabel( rawName: string, localize: (key: TranslationKeys) => string, + knownServerNames?: readonly string[], ): string { - const parsed = parseToolName(rawName); + const parsed = parseToolName(rawName, knownServerNames); if (parsed.mcpServer) return parsed.mcpServer; if (parsed.friendlyKey) return localize(parsed.friendlyKey); return parsed.toolName; diff --git a/packages/api/src/agents/context.ts b/packages/api/src/agents/context.ts index 01a6fbcce9..5cfed56c70 100644 --- a/packages/api/src/agents/context.ts +++ b/packages/api/src/agents/context.ts @@ -30,7 +30,8 @@ export function extractMCPServers(agent: AgentWithTools): string[] { if (agent?.tools?.length) { for (const tool of agent.tools) { if (tool instanceof DynamicStructuredTool && tool.name.includes(Constants.mcp_delimiter)) { - const serverName = tool.name.split(Constants.mcp_delimiter).pop(); + const carried = (tool as { mcpRawServerName?: string }).mcpRawServerName; + const serverName = carried ?? tool.name.split(Constants.mcp_delimiter).pop(); if (serverName) { mcpServers.add(serverName); } @@ -42,7 +43,7 @@ export function extractMCPServers(agent: AgentWithTools): string[] { if (agent?.toolDefinitions?.length) { for (const toolDef of agent.toolDefinitions) { if (toolDef.name?.includes(Constants.mcp_delimiter)) { - const serverName = toolDef.name.split(Constants.mcp_delimiter).pop(); + const serverName = toolDef.serverName ?? toolDef.name.split(Constants.mcp_delimiter).pop(); if (serverName) { mcpServers.add(serverName); } diff --git a/packages/api/src/agents/initialize.ts b/packages/api/src/agents/initialize.ts index 6c52107427..c554e24d68 100644 --- a/packages/api/src/agents/initialize.ts +++ b/packages/api/src/agents/initialize.ts @@ -50,7 +50,7 @@ import { registerFileAuthoringTools, isFileAuthoringToolDefinition, } from './tools'; -import { normalizeServerName, requiresEphemeralUserConnection } from '~/mcp/utils'; +import { normalizeServerName, requiresEphemeralUserConnection, splitMCPToolKey } from '~/mcp/utils'; import { registerMemoryTools, memoryToolUsageGuard } from './memory'; import { applyBackgroundToolCalls } from './background'; import { filterFilesByEndpointConfig } from '~/files'; @@ -1187,6 +1187,10 @@ export async function initializeAgent( ephemeralServerNames.add(normalizeServerName(serverName)); } } + /** Resolve the boundary against every configured server, not just the + * ephemeral subset: a non-ephemeral name ending in an ephemeral one would + * otherwise be misread as ephemeral. */ + const allServerNames = Object.keys(req.config?.mcpConfig ?? {}).map(normalizeServerName); const backgroundResult = applyBackgroundToolCalls({ toolDefinitions, toolRegistry, @@ -1197,13 +1201,8 @@ export async function initializeAgent( * Unknown servers stay eligible — the executor's per-instance tag is * the fail-safe for those. */ excludeTool: (toolName) => { - const delimiterIndex = toolName.indexOf(Constants.mcp_delimiter); - if (delimiterIndex < 0) { - return false; - } - return ephemeralServerNames.has( - toolName.slice(delimiterIndex + Constants.mcp_delimiter.length), - ); + const [, serverName] = splitMCPToolKey(toolName, allServerNames); + return serverName != null && ephemeralServerNames.has(serverName); }, }); toolDefinitions = backgroundResult.toolDefinitions; diff --git a/packages/api/src/index.ts b/packages/api/src/index.ts index fff580d06f..769ca417ee 100644 --- a/packages/api/src/index.ts +++ b/packages/api/src/index.ts @@ -22,6 +22,7 @@ export * from './mcp/tools'; export * from './mcp/request'; /* Utilities */ export * from './mcp/utils'; +export * from './mcp/context'; export * from './utils'; export { default as Tokenizer, countTokens } from './utils/tokenizer'; export type { EncodingName } from './utils/tokenizer'; diff --git a/packages/api/src/mcp/__tests__/auth.test.ts b/packages/api/src/mcp/__tests__/auth.test.ts index 04f7d5c81f..2a06d5dd6a 100644 --- a/packages/api/src/mcp/__tests__/auth.test.ts +++ b/packages/api/src/mcp/__tests__/auth.test.ts @@ -67,6 +67,40 @@ describe('getUserMCPAuthMap', () => { }); }); + describe('tool-key boundary', () => { + it('resolves the plugin key from the last delimiter for a gateway-prefixed tool name', async () => { + /** The raw upstream name carries the delimiter, so first-occurrence extraction + * asked for `mcp_server_version_mcp_gitlab` and silently resolved no + * customUserVars, leaving API-key/header placeholders unfilled. */ + mockGetPluginAuthMap.mockResolvedValue({}); + + await getUserMCPAuthMap({ + userId: 'user123', + tools: ['gitlab-get_mcp_server_version_mcp_gitlab'], + findPluginAuthsByKeys: mockFindPluginAuthsByKeys, + }); + + expect(mockGetPluginAuthMap).toHaveBeenCalledWith( + expect.objectContaining({ pluginKeys: ['mcp_gitlab'] }), + ); + }); + + it('resolves a configured server whose own name contains the delimiter', async () => { + mockGetPluginAuthMap.mockResolvedValue({}); + + await getUserMCPAuthMap({ + userId: 'user123', + tools: ['search_mcp_Google_mcp_Workspace'], + serverNames: ['Google_mcp_Workspace'], + findPluginAuthsByKeys: mockFindPluginAuthsByKeys, + }); + + expect(mockGetPluginAuthMap).toHaveBeenCalledWith( + expect.objectContaining({ pluginKeys: ['mcp_Google_mcp_Workspace'] }), + ); + }); + }); + describe('Edge Cases', () => { it('should return empty object when no tools have mcpRawServerName', async () => { const toolInstances = [ diff --git a/packages/api/src/mcp/__tests__/context.test.ts b/packages/api/src/mcp/__tests__/context.test.ts new file mode 100644 index 0000000000..811a4b8409 --- /dev/null +++ b/packages/api/src/mcp/__tests__/context.test.ts @@ -0,0 +1,27 @@ +import { resolveMCPServerContext } from '../context'; + +describe('resolveMCPServerContext', () => { + it('returns every configured name, not just the lazily-initialized ones', async () => { + /** `ensureConfigServers` skips unmodified YAML servers, so its keys are not the + * configured set; boundary resolution needs the full list or it goes inert. */ + const ensureConfigServers = jest.fn().mockResolvedValue({ lazyInit: { name: 'lazyInit' } }); + + const result = await resolveMCPServerContext({ + mcpConfig: { lazyInit: {}, unchangedYaml: {} } as never, + ensureConfigServers, + }); + + expect(result.configServers).toEqual({ lazyInit: { name: 'lazyInit' } }); + expect(result.serverNames.sort()).toEqual(['lazyInit', 'unchangedYaml']); + expect(ensureConfigServers).toHaveBeenCalledTimes(1); + }); + + it('normalizes names into the form tool keys embed', async () => { + const result = await resolveMCPServerContext({ + mcpConfig: { 'Google MCP Workspace': {} } as never, + ensureConfigServers: jest.fn().mockResolvedValue({}), + }); + + expect(result.serverNames).toEqual(['Google_MCP_Workspace']); + }); +}); diff --git a/packages/api/src/mcp/__tests__/utils.test.ts b/packages/api/src/mcp/__tests__/utils.test.ts index cbed141d0f..1da821fc1c 100644 --- a/packages/api/src/mcp/__tests__/utils.test.ts +++ b/packages/api/src/mcp/__tests__/utils.test.ts @@ -2,6 +2,7 @@ import type { ParsedServerConfig } from '~/mcp/types'; import { buildOAuthToolCallName, normalizeServerName, + splitMCPToolKey, redactAllServerSecrets, redactServerSecrets, requiresUserScopedConnection, @@ -45,6 +46,35 @@ describe('normalizeServerName', () => { }); }); +describe('splitMCPToolKey', () => { + it('should return the tool name unchanged with an undefined server name when there is no delimiter', () => { + expect(splitMCPToolKey('plainToolName')).toEqual(['plainToolName', undefined]); + }); + + it('should split a normal single-occurrence key the same way String.split would', () => { + expect(splitMCPToolKey('search_mcp_myserver')).toEqual(['search', 'myserver']); + }); + + it('should resolve a raw tool name that itself contains the delimiter substring by using the last occurrence', () => { + // Regression test: a tool whose own (possibly gateway-prefixed) name + // already contains "_mcp_" - e.g. LiteLLM's MCP gateway prefixes + // aggregated tool names with "{server}-", so GitLab's own + // "get_mcp_server_version" tool becomes "gitlab-get_mcp_server_version" + // before LibreChat appends its own "_mcp_gitlab" suffix. A naive + // `.split(delimiter)` produces 3 segments here and silently drops the + // 3rd, yielding a bogus server name ("server_version" instead of + // "gitlab"). See https://github.com/danny-avila/LibreChat/issues/14440 + expect(splitMCPToolKey('gitlab-get_mcp_server_version_mcp_gitlab')).toEqual([ + 'gitlab-get_mcp_server_version', + 'gitlab', + ]); + }); + + it('should handle a raw tool name with multiple delimiter occurrences by always taking the last segment as the server name', () => { + expect(splitMCPToolKey('a_mcp_b_mcp_c_mcp_server')).toEqual(['a_mcp_b_mcp_c', 'server']); + }); +}); + describe('buildOAuthToolCallName', () => { it('should prefix a simple server name with oauth_mcp_', () => { expect(buildOAuthToolCallName('my-server')).toBe('oauth_mcp_my-server'); diff --git a/packages/api/src/mcp/auth.ts b/packages/api/src/mcp/auth.ts index b25e0ce8ee..912e0fad41 100644 --- a/packages/api/src/mcp/auth.ts +++ b/packages/api/src/mcp/auth.ts @@ -3,18 +3,22 @@ import { Constants } from 'librechat-data-provider'; import type { PluginAuthMethods } from '@librechat/data-schemas'; import type { GenericTool } from '@librechat/agents'; import { getPluginAuthMap } from '~/agents/auth'; +import { splitMCPToolKey } from './utils'; export async function getUserMCPAuthMap({ userId, tools, servers, toolInstances, + serverNames, findPluginAuthsByKeys, }: { userId: string; tools?: (string | undefined)[]; servers?: (string | undefined)[]; toolInstances?: (GenericTool | null)[]; + /** Configured server names, used to resolve the tool-key boundary exactly */ + serverNames?: readonly string[]; findPluginAuthsByKeys: PluginAuthMethods['findPluginAuthsByKeys']; }): Promise>> { let allMcpCustomUserVars: Record> = {}; @@ -34,9 +38,7 @@ export async function getUserMCPAuthMap({ if (!toolName) { continue; } - const delimiterIndex = toolName.indexOf(Constants.mcp_delimiter); - if (delimiterIndex === -1) continue; - const mcpServer = toolName.slice(delimiterIndex + Constants.mcp_delimiter.length); + const [, mcpServer] = splitMCPToolKey(toolName, serverNames); if (!mcpServer) continue; uniqueMcpServers.add(`${Constants.mcp_prefix}${mcpServer}`); } diff --git a/packages/api/src/mcp/context.ts b/packages/api/src/mcp/context.ts new file mode 100644 index 0000000000..8abecc2be6 --- /dev/null +++ b/packages/api/src/mcp/context.ts @@ -0,0 +1,35 @@ +import { normalizeServerName } from 'librechat-data-provider'; +import type { MCPOptions } from 'librechat-data-provider'; +import type { ParsedServerConfig } from '~/mcp/types'; + +export interface MCPServerContext { + /** Config-source servers that needed lazy initialization. */ + configServers: Record; + /** Every configured server, in the normalized form tool keys are built from. */ + serverNames: string[]; +} + +export interface ResolveMCPServerContextParams { + mcpConfig: Record; + ensureConfigServers: ( + mcpConfig: Record, + ) => Promise>; +} + +/** + * Resolves the MCP server context for one request from a single config snapshot. + * + * `ensureConfigServers` deliberately skips unmodified YAML servers, so its keys are + * not the configured set. Tool-key boundary resolution needs every configured name, + * normalized the way keys embed it, or a server absent from the lazy-init result + * silently falls back to positional parsing. + */ +export async function resolveMCPServerContext({ + mcpConfig, + ensureConfigServers, +}: ResolveMCPServerContextParams): Promise { + return { + configServers: await ensureConfigServers(mcpConfig), + serverNames: Object.keys(mcpConfig).map(normalizeServerName), + }; +} diff --git a/packages/api/src/mcp/oauth/events.ts b/packages/api/src/mcp/oauth/events.ts index 4aec0db7ea..79515097aa 100644 --- a/packages/api/src/mcp/oauth/events.ts +++ b/packages/api/src/mcp/oauth/events.ts @@ -1,7 +1,7 @@ import { Constants, Time } from 'librechat-data-provider'; import { GraphEvents, StepTypes } from '@librechat/agents'; import type * as t from '~/types'; -import { buildOAuthToolCallName } from '~/mcp/utils'; +import { buildOAuthToolCallName, splitMCPToolKey } from '~/mcp/utils'; export type OAuthPromptOptions = { expiresAt?: number; @@ -24,7 +24,10 @@ export function getOAuthPromptExpiresAt( : now + Time.TWO_MINUTES; } -export function getMCPServerNamesFromTools(tools?: unknown[] | null): Set { +export function getMCPServerNamesFromTools( + tools?: unknown[] | null, + knownServerNames?: readonly string[], +): Set { const serverNames = new Set(); for (const tool of tools ?? []) { @@ -32,12 +35,12 @@ export function getMCPServerNamesFromTools(tools?: unknown[] | null): Set` @@ -439,3 +408,5 @@ export function generateServerNameFromTitle(title: string): string { return slug || 'mcp-server'; // Fallback if empty } + +export { splitMCPToolKey, normalizeServerName } from 'librechat-data-provider'; diff --git a/packages/api/src/tools/classification.ts b/packages/api/src/tools/classification.ts index b461c85990..d211eba0a5 100644 --- a/packages/api/src/tools/classification.ts +++ b/packages/api/src/tools/classification.ts @@ -101,6 +101,8 @@ interface MCPToolInstance { mcp?: boolean; /** Original JSON schema attached at MCP tool creation time */ mcpJsonSchema?: JsonSchemaType; + /** Server this tool came from, carried from resolution instead of re-parsed */ + mcpRawServerName?: string; } /** @@ -121,7 +123,7 @@ export function extractMCPToolDefinition(tool: MCPToolInstance): ToolDefinition def.parameters = tool.mcpJsonSchema; } - const serverName = getServerNameFromTool(tool.name); + const serverName = tool.mcpRawServerName ?? getServerNameFromTool(tool.name); if (serverName) { def.serverName = serverName; } diff --git a/packages/api/src/tools/definitions.ts b/packages/api/src/tools/definitions.ts index 16d689807b..b6e6340f78 100644 --- a/packages/api/src/tools/definitions.ts +++ b/packages/api/src/tools/definitions.ts @@ -6,7 +6,7 @@ */ import { Providers } from '@librechat/agents'; -import { Constants, isActionTool } from 'librechat-data-provider'; +import { Constants, isActionTool, splitMCPToolKey } 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'; @@ -42,6 +42,8 @@ export interface LoadToolDefinitionsParams { codeExecutionEnabled?: boolean; /** Agent provider — Gemini/Vertex tool schemas get union-flattened for compatibility */ provider?: Providers; + /** Configured server names, used to resolve the tool-key boundary exactly */ + mcpServerNames?: readonly string[]; } export interface ActionToolDefinition { @@ -87,6 +89,7 @@ export async function loadToolDefinitions( programmaticToolsEnabled = false, codeExecutionEnabled = false, provider, + mcpServerNames, } = params; const { getOrFetchMCPServerTools, isBuiltInTool, getActionToolDefinitions } = deps; @@ -155,8 +158,8 @@ export async function loadToolDefinitions( continue; } - const parts = toolName.split(Constants.mcp_delimiter); - const serverName = parts[parts.length - 1]; + const [, parsedServerName] = splitMCPToolKey(toolName, mcpServerNames); + const serverName = parsedServerName ?? toolName; if (!mcpServerToolsCache.has(serverName)) { const serverTools = await getOrFetchMCPServerTools(userId, serverName); @@ -207,6 +210,7 @@ export async function loadToolDefinitions( description: def.description, mcp: true as const, mcpJsonSchema: def.parameters, + mcpRawServerName: def.serverName, })) as unknown as GenericTool[]; const classificationResult = await buildToolClassification({ diff --git a/packages/data-provider/src/config.ts b/packages/data-provider/src/config.ts index 97607f99c0..07c47be50b 100644 --- a/packages/data-provider/src/config.ts +++ b/packages/data-provider/src/config.ts @@ -2755,6 +2755,107 @@ export enum Constants { CHECK_BACKGROUND_TASK = 'check_background_task', } +/** + * Normalizes a server name into the character set tool keys are built from. + * Tool keys embed this output, so any candidate list matched against a key must + * be normalized the same way. + */ +export function normalizeServerName(serverName: string): string { + if (/^[a-zA-Z0-9_.-]+$/.test(serverName)) { + return serverName; + } + + const normalized = serverName.replace(/[^a-zA-Z0-9_.-]/g, '_').replace(/^_+|_+$/g, ''); + if (normalized) { + return normalized; + } + + /** All characters were stripped; hash the original so the name stays unique. */ + let hash = 0; + for (let i = 0; i < serverName.length; i++) { + hash = (hash << 5) - hash + serverName.charCodeAt(i); + hash |= 0; + } + return `server_${Math.abs(hash)}`; +} + +/** + * Splits a combined MCP tool key (`${rawToolName}${mcp_delimiter}${serverName}`) + * back into its two parts. + * + * Both halves can legitimately contain the delimiter, so position alone cannot + * identify the boundary. Raw tool names come from the upstream server and are + * untrusted (`get_mcp_server_version`, or a gateway-prefixed + * `gitlab-get_mcp_server_version`), and `normalizeServerName` preserves + * underscores, so a configured server may be named `Google_mcp_Workspace`. + * + * When `knownServerNames` is supplied the boundary is resolved against it: the + * longest configured name the key actually ends with wins. Otherwise this falls + * back to the last delimiter, which is correct whenever only the tool half + * contains one and matches `.split()` when neither does. + * + * One case stays undecidable from the key alone: if both `bar` and `foo_mcp_bar` + * are configured, `tool_mcp_foo_mcp_bar` is a valid key for either. Longest match + * is the deterministic tiebreak; resolving it properly needs the tool/server + * mapping carried alongside the key rather than re-derived from the string. + */ +export function splitMCPToolKey( + toolKey: string, + knownServerNames?: readonly string[], +): [string, string | undefined] { + if (knownServerNames?.length) { + let matched: string | undefined; + for (let i = 0; i < knownServerNames.length; i++) { + const serverName = knownServerNames[i]; + if (!serverName || serverName.length <= (matched?.length ?? 0)) { + continue; + } + if (toolKey.endsWith(`${Constants.mcp_delimiter}${serverName}`)) { + matched = serverName; + } + } + if (matched != null) { + return [ + toolKey.slice(0, toolKey.length - matched.length - Constants.mcp_delimiter.length), + matched, + ]; + } + } + + const idx = toolKey.lastIndexOf(Constants.mcp_delimiter); + if (idx === -1) { + return [toolKey, undefined]; + } + return [toolKey.slice(0, idx), toolKey.slice(idx + Constants.mcp_delimiter.length)]; +} + +/** + * Splits a tool-call name for display, where the key may be a synthetic MCP OAuth + * call (`oauth${mcp_delimiter}${serverName}`) rather than a real tool key. + * + * A configured server name is authoritative when one matches, because a real tool key + * always ends in its server. Only when none matches does the `oauth` prefix decide, + * which keeps a genuine upstream tool named `oauth${mcp_delimiter}...` from being read + * as a synthetic call while still resolving OAuth prompts for unconfigured servers. + */ +export function splitToolCallName( + toolCallName: string, + knownServerNames?: readonly string[], +): [string, string | undefined] { + if (knownServerNames?.length) { + const [toolName, serverName] = splitMCPToolKey(toolCallName, knownServerNames); + if (serverName != null && knownServerNames.includes(serverName)) { + return [toolName, serverName]; + } + } + + const oauthPrefix = `oauth${Constants.mcp_delimiter}`; + if (toolCallName.startsWith(oauthPrefix)) { + return ['oauth', toolCallName.slice(oauthPrefix.length)]; + } + return splitMCPToolKey(toolCallName, knownServerNames); +} + /** Maximum explicit subagent hops allowed from any root agent at runtime. */ export const MAX_SUBAGENT_DEPTH = 5; diff --git a/packages/data-provider/src/splitMCPToolKey.spec.ts b/packages/data-provider/src/splitMCPToolKey.spec.ts new file mode 100644 index 0000000000..eab45b65fd --- /dev/null +++ b/packages/data-provider/src/splitMCPToolKey.spec.ts @@ -0,0 +1,137 @@ +import { Constants, splitMCPToolKey, splitToolCallName } from './config'; + +describe('splitMCPToolKey', () => { + it('splits a normal single-delimiter key like String.split would', () => { + expect(splitMCPToolKey('search_mcp_myserver')).toEqual(['search', 'myserver']); + }); + + it('returns an undefined server name when there is no delimiter', () => { + expect(splitMCPToolKey('plainToolName')).toEqual(['plainToolName', undefined]); + }); + + it('resolves a raw tool name containing the delimiter via the last occurrence', () => { + expect(splitMCPToolKey('gitlab-get_mcp_server_version_mcp_gitlab')).toEqual([ + 'gitlab-get_mcp_server_version', + 'gitlab', + ]); + }); + + it('resolves a server name containing the delimiter when known names are supplied', () => { + expect(splitMCPToolKey('search_mcp_Google_mcp_Workspace', ['Google_mcp_Workspace'])).toEqual([ + 'search', + 'Google_mcp_Workspace', + ]); + }); + + it('prefers the longest matching configured server name', () => { + expect( + splitMCPToolKey('search_mcp_Google_mcp_Workspace', ['Workspace', 'Google_mcp_Workspace']), + ).toEqual(['search', 'Google_mcp_Workspace']); + }); + + it('falls back to the last delimiter when no configured name matches', () => { + expect(splitMCPToolKey('gitlab-get_mcp_server_version_mcp_gitlab', ['other'])).toEqual([ + 'gitlab-get_mcp_server_version', + 'gitlab', + ]); + }); + + it('still resolves the tool half when both halves contain the delimiter', () => { + expect(splitMCPToolKey('a_mcp_b_mcp_Google_mcp_Workspace', ['Google_mcp_Workspace'])).toEqual([ + 'a_mcp_b', + 'Google_mcp_Workspace', + ]); + }); +}); + +describe('splitToolCallName', () => { + const d = Constants.mcp_delimiter; + + it('treats a synthetic OAuth call as oauth plus the full server name', () => { + expect(splitToolCallName(`oauth${d}foo${d}bar`)).toEqual(['oauth', `foo${d}bar`]); + }); + + it('keeps a normalized server name that itself contains the delimiter', () => { + expect(splitToolCallName(`oauth${d}oauth${d}server`)).toEqual(['oauth', `oauth${d}server`]); + }); + + it('resolves a real tool key whose raw name contains the delimiter', () => { + expect(splitToolCallName(`gitlab-get${d}server_version${d}gitlab`)).toEqual([ + `gitlab-get${d}server_version`, + 'gitlab', + ]); + }); + + it('resolves a real tool key against configured server names when supplied', () => { + expect(splitToolCallName(`search${d}Google${d}Workspace`, [`Google${d}Workspace`])).toEqual([ + 'search', + `Google${d}Workspace`, + ]); + }); +}); + +describe('splitToolCallName with configured server names', () => { + const d = Constants.mcp_delimiter; + + it('reads a real tool whose own name starts with the oauth prefix', () => { + expect(splitToolCallName(`oauth${d}reset${d}github`, ['github'])).toEqual([ + `oauth${d}reset`, + 'github', + ]); + }); + + it('still resolves a synthetic OAuth call for a configured server', () => { + expect(splitToolCallName(`oauth${d}github`, ['github'])).toEqual(['oauth', 'github']); + }); + + it('resolves a synthetic OAuth call for a delimiter-bearing configured server', () => { + expect(splitToolCallName(`oauth${d}foo${d}bar`, [`foo${d}bar`])).toEqual([ + 'oauth', + `foo${d}bar`, + ]); + }); +}); + +describe('splitMCPToolKey boundary alignment', () => { + const d = Constants.mcp_delimiter; + + it('ignores a configured name that is not delimiter-aligned in the key', () => { + /** `server` is a bare suffix of `myserver`, not a segment. Matching on + * `endsWith(name)` instead of `endsWith(delimiter + name)` would route the + * call to a different configured server than the agent authorized. */ + expect(splitMCPToolKey(`search${d}myserver`, ['server'])).toEqual(['search', 'myserver']); + }); + + it('treats an empty known-name list the same as no list', () => { + expect(splitMCPToolKey(`gitlab-get${d}server_version${d}gitlab`, [])).toEqual([ + `gitlab-get${d}server_version`, + 'gitlab', + ]); + }); + + it('requires the configured name to be normalized to match the key', () => { + /** Keys embed `normalizeServerName`'s output, so callers must normalize their + * candidate list; a raw name with spaces can never align. */ + expect(splitMCPToolKey(`search${d}Google_mcp_Workspace`, ['Google_mcp_Workspace'])).toEqual([ + 'search', + 'Google_mcp_Workspace', + ]); + }); +}); + +describe('splitToolCallName oauth precedence', () => { + const d = Constants.mcp_delimiter; + + it('falls back to the oauth prefix when a list is supplied but nothing matches', () => { + /** Pins the precedence rule: the configured branch must not return its + * last-delimiter result when no configured name actually matched. */ + expect(splitToolCallName(`oauth${d}foo${d}bar`, ['github'])).toEqual(['oauth', `foo${d}bar`]); + }); + + it('prefers a matching configured server over the oauth prefix', () => { + expect(splitToolCallName(`oauth${d}reset${d}github`, ['github'])).toEqual([ + `oauth${d}reset`, + 'github', + ]); + }); +}); diff --git a/packages/data-schemas/src/methods/agent.spec.ts b/packages/data-schemas/src/methods/agent.spec.ts index 035d37e3ca..c48759ee72 100644 --- a/packages/data-schemas/src/methods/agent.spec.ts +++ b/packages/data-schemas/src/methods/agent.spec.ts @@ -549,6 +549,65 @@ describe('Agent Methods', () => { expect(newAgent.mcpServerNames).toEqual(['authorizedServer']); }); + test('should derive the server from a key whose raw tool name contains the delimiter', async () => { + const { agentId, authorId } = createTestIds(); + /** DB server names are slugs and cannot contain the delimiter, so the trailing + * segment is the real server even when the raw tool name carries one. Shared-agent + * access is keyed off this field, so it must not be dropped. */ + const gatewayTool = `get${Constants.mcp_delimiter}server_version${Constants.mcp_delimiter}gitlab`; + + const newAgent = await createAgent({ + id: agentId, + name: 'Gateway MCP Agent', + provider: 'test', + model: 'test-model', + author: authorId, + tools: [gatewayTool], + }); + + expect(newAgent.mcpServerNames).toEqual(['gitlab']); + }); + + test('should preserve a resolved server name across an update that omits it', async () => { + const { agentId, authorId } = createTestIds(); + /** Any caller that writes `tools` without `mcpServerNames` — the Action edit + * path, for one — must not have a configured `Google_mcp_Workspace` reduced to + * `Workspace`, which ServerConfigsDB would resolve as an unrelated DB server. */ + const mcpTool = `search${Constants.mcp_delimiter}Google${Constants.mcp_delimiter}Workspace`; + await createAgent({ + id: agentId, + name: 'Provenance Agent', + provider: 'test', + model: 'test-model', + author: authorId, + tools: [mcpTool], + mcpServerNames: [`Google${Constants.mcp_delimiter}Workspace`], + }); + + const updated = await updateAgent({ id: agentId }, { tools: [mcpTool, 'web_search'] }); + + expect(updated!.mcpServerNames).toEqual([`Google${Constants.mcp_delimiter}Workspace`]); + expect(updated!.mcpServerNames).not.toContain('Workspace'); + }); + + test('should drop a resolved name once its last tool is gone', async () => { + const { agentId, authorId } = createTestIds(); + const mcpTool = `search${Constants.mcp_delimiter}Google${Constants.mcp_delimiter}Workspace`; + await createAgent({ + id: agentId, + name: 'Provenance Agent 2', + provider: 'test', + model: 'test-model', + author: authorId, + tools: [mcpTool], + mcpServerNames: [`Google${Constants.mcp_delimiter}Workspace`], + }); + + const updated = await updateAgent({ id: agentId }, { tools: ['web_search'] }); + + expect(updated!.mcpServerNames).toEqual([]); + }); + test('should derive mcpServerNames only from MCP tools on update', async () => { const { agentId, authorId } = createTestIds(); const actionTool = `sync${Constants.mcp_delimiter}state${actionDelimiter}api---example---com`; diff --git a/packages/data-schemas/src/methods/agent.ts b/packages/data-schemas/src/methods/agent.ts index d88b8443fd..f2fe72d11a 100644 --- a/packages/data-schemas/src/methods/agent.ts +++ b/packages/data-schemas/src/methods/agent.ts @@ -137,6 +137,12 @@ function extractMCPServerNames(tools: string[] | undefined | null): string[] { continue; } const parts = tool.split(mcp_delimiter); + /** This index only grants DB-backed servers (`ServerConfigsDB.getAccessibleServers`), + * and DB server names are slugs that cannot contain the delimiter + * (`generateServerNameFromTitle` strips underscores), so the last segment is always + * the real server for those. A config server whose own name contains the delimiter + * yields a trailing segment that is not its name; resolving that needs the configured + * server list, which is unavailable here - see #14449. */ if (parts.length >= 2) { serverNames.add(parts[parts.length - 1]); } @@ -144,6 +150,43 @@ function extractMCPServerNames(tools: string[] | undefined | null): string[] { return Array.from(serverNames); } +/** + * Rebuilds an agent's MCP server index across a tools update without re-deriving + * names from the keys. + * + * A name already on the agent was resolved against the registry when it was + * stored, so it is authoritative; it carries forward while some retained tool + * still resolves to it. Only keys that match none of them fall back to the + * ambiguous trailing-segment derivation, which cannot tell a config server's + * suffix from a real DB server name. + */ +function rebuildMCPServerNames(tools: string[] | undefined | null, priorNames: string[]): string[] { + if (priorNames.length === 0) { + return extractMCPServerNames(tools); + } + + const retained = new Set(); + const unmatched: string[] = []; + for (const tool of tools ?? []) { + if (!tool || !tool.includes(mcp_delimiter) || isActionTool(tool)) { + continue; + } + const match = priorNames + .filter((name) => tool.endsWith(`${mcp_delimiter}${name}`)) + .sort((a, b) => b.length - a.length)[0]; + if (match) { + retained.add(match); + } else { + unmatched.push(tool); + } + } + + for (const name of extractMCPServerNames(unmatched)) { + retained.add(name); + } + return Array.from(retained); +} + /** * Check if a version already exists in the versions array, excluding timestamp and author fields. */ @@ -440,7 +483,11 @@ export function createAgentMethods( }, ], category: (agentData.category as string) || 'general', - mcpServerNames: extractMCPServerNames(agentData.tools as string[] | undefined), + /** Callers that authorized the tools pass resolved names; deriving from the key + * alone cannot tell a config server's suffix from a real DB server name. */ + mcpServerNames: + (agentData.mcpServerNames as string[] | undefined) ?? + extractMCPServerNames(agentData.tools as string[] | undefined), }; return (await Agent.create(initialAgentData)).toObject() as IAgent; @@ -595,9 +642,17 @@ export function createAgentMethods( // Sync mcpServerNames when tools are updated if ((directUpdates as Record).tools !== undefined) { - const mcpServerNames = extractMCPServerNames( - (directUpdates as Record).tools as string[], - ); + /** Callers that authorized the tools pass resolved names; deriving from the key + * alone cannot tell a config server's suffix from a real DB server name. */ + const supplied = (directUpdates as Record).mcpServerNames as + | string[] + | undefined; + const mcpServerNames = + supplied ?? + rebuildMCPServerNames( + (directUpdates as Record).tools as string[], + (currentAgent.mcpServerNames as string[] | undefined) ?? [], + ); (directUpdates as Record).mcpServerNames = mcpServerNames; updateData.mcpServerNames = mcpServerNames; }