From 250aca375a5e5586fd5b9957e7b35305938efacf Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 27 Jul 2026 14:45:38 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=97=20fix:=20Resolve=20MCP=20Tool-Key?= =?UTF-8?q?=20Boundary=20Against=20Configured=20Server=20Names=20(#14448)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: resolve MCP tool-name delimiter collision at invocation time MCP tool keys are identified internally as `${rawToolName}${mcp_delimiter}${serverName}` (delimiter `_mcp_`). Several call sites parsed this back apart with a naive `toolKey.split(Constants.mcp_delimiter)`, assuming the delimiter occurs exactly once. When the raw upstream tool name itself contains the delimiter substring - which happens whenever it's exposed through a gateway that prefixes aggregated tool names by server (e.g. a gateway's own "gitlab-get_mcp_server_version" for GitLab's "get_mcp_server_version" tool) - the combined key has the delimiter more than once. `.split()` then produces more than two segments, and destructuring `[toolName, serverName]` silently keeps only the first two, yielding a bogus server name that matches no configured server. Tool listing still worked (a different code path builds keys directly without re-splitting), but invocation failed with `Tool {name} not found`, and `filterAuthorizedTools` rejected such keys outright as malformed. Add `splitMCPToolKey`, which splits on the *last* occurrence of the delimiter instead: the server-name half is always LibreChat's own normalized suffix (guaranteed not to contain the delimiter), while the raw tool-name half is untrusted and may legitimately contain it. This matches `.split()`'s result whenever the delimiter occurs once, and correctly resolves the collision case. Update the four call sites that parsed this manually (`handleTools.js`, `MCP.js`, `mcp.js` controller, `filterAuthorizedTools` in `v1.js`) plus one in the client (`useVisibleTools.ts`) to use it. Fixes #14440 * fix: resolve MCP tool-key boundary against configured server names splitMCPToolKey moves to librechat-data-provider so the client and backend share one parser, and takes the configured server names when the caller has them: the longest name the key actually ends with wins, which is exact. Position alone cannot identify the boundary because both halves may contain the delimiter. lastIndexOf alone fixes gateway-prefixed tool names but regresses servers whose own name contains it, which ToolService.spec.js already covered; the last-delimiter path now only serves as the fallback for callers with no configured set. Also converts the remaining first-occurrence parsers that the delimiter fix missed - mcp/auth.ts (custom user vars silently unresolved), mcp/oauth/events.ts, agents/initialize.ts, and the three client parsers that labelled tool calls with the wrong server. * fix: keep client tool-call labels on first-delimiter parsing The three client parsers had deliberate, tested first-delimiter semantics (ToolCall.test.tsx asserts the full server name for 'foo_mcp_bar' and the synthetic 'oauth_mcp_server' call), and the client has no configured server list in scope to resolve the boundary exactly, so they are left as they were. Threads the configured names into the event-driven definition loader so it resolves the same boundary as the authorization filter that admits the key, and documents the one case that stays undecidable without provenance. * fix: resolve tool-key boundary against all configured servers resolveConfigServers only returns lazily-initialized config overrides - ensureConfigServers skips unmodified YAML servers - so on a stock deployment the known-name list was empty and suffix resolution never engaged. Adds resolveMcpServerNames, which keeps every configured server in the normalized form tool keys carry, and uses it at the loading, auth-map and definition sites. Background-tool eligibility now resolves against all configured names before testing ephemeral membership, so a non-ephemeral server whose name ends in an ephemeral one is no longer misclassified, and useVisibleTools resolves against the server map it already receives. * fix: use resolved server provenance and one app-config read createMCPTool now uses the serverName loadTools already resolved for the key and only parses as a fallback, so an unmodified YAML server whose name contains the delimiter no longer resolves to the wrong server for auth, reconnection and callTool. resolveMcpServerContext derives config servers and all configured names from a single getAppConfigForRequest, replacing two independent lookups on the chat startup path, and degrades to empty like resolveConfigServers instead of aborting tool loading when the config lookup fails. * chore: drop unused resolveConfigServers import * fix: forward server provenance on the all-tools path and read config once createMCPTools builds each toolKey from the server name it already has but did not forward it, so the sys__all__sys path re-derived it by parsing and bound an unmodified YAML server whose name contains the delimiter to the wrong auth and invocation context. loadAgentTools now resolves the MCP server context once and threads it into loadTools, replacing the second app-config read it had introduced on the non-event-driven chat startup path. * fix: carry resolved MCP server name through tool classification definitions.ts resolves the server for each key and then dropped it when building loadedTools, so buildToolClassification re-derived it with a last-segment split and recorded 'Workspace' for a server configured as 'Google_mcp_Workspace'. The resolved name now rides along on the tool instance and classification prefers it over re-parsing. * fix: consume carried server name when extracting MCP servers extractMCPServers re-derived the name with a last-segment split, so a server configured as Google_mcp_Workspace resolved to Workspace and its instructions were silently omitted. Prefers the name carried on the tool definition instance, falling back to the split. * fix: fail closed on ambiguous MCP keys when persisting server names Persisted mcpServerNames grant agent-scoped access to a DB server by name (ServerConfigsDB.getAccessibleServers), so a wrong guess exposes an unrelated server to everyone who can view the agent. The last-segment split turned search_mcp_Google_mcp_workspace into 'workspace'; such keys were previously rejected outright at agent save, so admitting them opened this path. Derives a name only from unambiguous single-delimiter keys. This is #12250's guard moved to the boundary it was actually protecting, instead of blocking tool admission. * fix: keep DB server access for multi-delimiter tool keys The fail-closed guard was wrong for the case this PR exists to fix. This index only grants DB-backed servers, and DB names are slugs that cannot contain the delimiter (generateServerNameFromTitle strips underscores), so the trailing segment is always the real server for them - dropping it cost every consumer of a gateway-prefixed tool their shared-agent access. Also gates the MCP server-context lookup on the filtered MCP set, so an agent with no MCP tools no longer pays an app-config read on startup. * fix: resolve tool-call display names without breaking OAuth calls The display parsers could not use the shared boundary parser because their tested behavior depends on first-delimiter semantics. That constraint only applies to synthetic MCP OAuth calls, whose tool half is always exactly 'oauth', so everything after the first delimiter is the server even when the server name carries one. splitToolCallName special-cases that form and defers to splitMCPToolKey for real tool keys, so a gateway-prefixed tool now renders its own name and server while oauth_mcp_foo_mcp_bar still resolves to foo_mcp_bar. * fix: persist resolved MCP server provenance on agents Deriving mcpServerNames from the tool key cannot tell a config server's trailing segment from a real DB server name, so a config server named a_mcp_b indexed an unrelated DB server b and shared the agent's viewers into it. Neither string rule works: the suffix guess exposes, and failing closed drops legitimate DB access for gateway-prefixed tools. filterAuthorizedTools already resolves each tool's server against the merged registry config, so it now collects those names and create, update and duplicate persist them. No extra registry queries: the update path unions the newly resolved names with what the agent already had, and duplicate replaces the copied list rather than inheriting the source's servers. Display parsing also takes the configured names, so a real tool call on a delimiter-bearing server renders the right server and icon. * test: teach MCP hook mocks about useMCPServerNames Three specs mock ~/hooks/MCP with a hand-listed factory, so adding the hook to ToolCall made useMCPServerNames undefined under test and every render threw. Returns a stable array so the mock cannot perturb render counts. * fix: rebuild agent MCP server index from surviving tools Unioning the prior names kept a server indexed after its last tool was detached, so viewers of a shared agent retained agent-scoped access to it. The index is now rebuilt from the tools that survive the edit: a prior name carries forward only while some retained tool still resolves to it, using the agent's own persisted names as the candidate set, and the rebuild runs on any tool change rather than only when a new MCP tool is added. * fix: keep duplicate indexes on registry fallback and harden the oauth split Duplication blanked mcpServerNames when the registry was unavailable, because filterAuthorizedTools grandfathers the source's tools without resolving them - the copy kept tools it could no longer resolve. Source names now carry forward for the tools that still point at them. splitToolCallName also treated any oauth_mcp_ prefix as a synthetic OAuth call, so a genuine upstream tool by that name resolved to the wrong server. A configured server name now decides when one matches, since a real key always ends in its server, and the prefix only breaks ties for unconfigured servers. * fix: thread configured server names through display parsing parseToolName and getMCPServerName resolved context-free, so a configured server whose name contains the delimiter showed the wrong server in grouped tool summaries and subagent tool labels, and stacked icons missed its entry in the icon map. Both take the configured names now, supplied by the components that render them. Adds the hook to SubagentCall's mock factory: the spec renders the real component, so an unmocked useMCPServerNames would reach the query with no provider. * test: cover the auth-map boundary, server provenance and context fallback Adds regression coverage for three behaviors this PR changed that no test exercised: customUserVars resolving under the right plugin key for a gateway-prefixed tool name (the failure that made these tools loadable but unusable), the resolved server name reaching createMCPTool instead of being re-parsed, and resolveMcpServerContext degrading to empty rather than aborting tool loading when the config lookup fails. Each was checked against a mutated source to confirm it fails when the behavior is broken. * fix: normalize server-name candidates and cover the boundary guard Tool keys embed normalizeServerName's output while the config is keyed by the raw name, so callers passing raw keys never matched a server whose name needs normalizing and silently fell back to the last delimiter. filterAuthorizedTools now maps normalized names back to their config key, and createMCPTool normalizes its candidates. Adds the cases an audit found surviving mutation: a configured name that is a bare but not delimiter-aligned suffix must not match, an empty candidate list behaves as no list, and splitToolCallName still falls back to the oauth prefix when a list is supplied but nothing in it matches. * fix: keep resolved server names when a non-owner retains MCP tools The shared-agent path keeps an agent's existing MCP tools verbatim but supplied no mcpServerNames, so persistence re-derived them and reduced a configured server like Google_mcp_Workspace to Workspace - which ServerConfigsDB then treats as a DB server, granting the agent's viewers access to an unrelated one. Carries the existing resolved names across instead, and clears the index on the owner path where every MCP tool is removed. * fix: preserve resolved MCP names for every tools update extractMCPServerNames was reachable from any caller that writes tools without mcpServerNames - the Action edit path does exactly that - so a configured Google_mcp_Workspace was reindexed as Workspace and ServerConfigsDB granted shared-agent viewers an unrelated DB server by that name. updateAgent now rebuilds the index from the agent's own resolved names: one carries forward while a retained tool still resolves to it, and only keys matching none of them fall back to derivation. Callers are safe by default rather than by remembering to pass the set. normalizeServerName moves to librechat-data-provider so the client can match its candidates against tool keys, which embed the normalized form; the icon map is keyed the same way since it is looked up with a parsed server name. * refactor: move MCP context resolution into packages/api New backend logic belongs in the TypeScript workspace per CLAUDE.md, with /api kept to a thin wrapper. resolveMCPServerContext now lives in packages/api/src/mcp/context.ts and takes ensureConfigServers by injection, since the registry accessor is still legacy-only; the /api function is reduced to loading the request app config and translating failures into the empty degrade it already promised. * test: teach the MCP service mock about resolveMCPServerContext The spec mocks @librechat/api with a hand-listed factory, so moving the resolver into that package left it undefined and the wrapper degraded into its own catch, returning empty config servers. The stub mirrors the real resolver so these tests still cover what the wrapper owns - loading the request config and degrading on failure - while the resolution logic is unit-tested in packages/api. * fix: only persist an authoritative MCP server index on update Assigning the resolved set unconditionally pinned the index to [] whenever nothing authoritative was available - a legacy agent holding MCP tools with no stored mcpServerNames - which suppressed updateAgent's derivation and stripped agent-scoped access to its DB-backed server. The field is now supplied only when the result is authoritative: names were resolved, or no MCP tool survives so the index genuinely is empty. The retained-tools branch likewise leaves it unset when the agent has none stored. --------- Co-authored-by: Jens Schumann --- api/app/clients/tools/util/handleTools.js | 12 +- .../clients/tools/util/handleTools.test.js | 49 +++++++ .../agents/filterAuthorizedTools.spec.js | 135 +++++++++++++++-- api/server/controllers/agents/v1.js | 86 ++++++++++- api/server/controllers/mcp.js | 3 +- api/server/services/MCP.js | 62 +++++++- api/server/services/ToolService.js | 26 +++- api/server/services/__tests__/MCP.spec.js | 50 ++++++- .../services/__tests__/ToolService.spec.js | 10 ++ .../Messages/Content/Parts/SubagentCall.tsx | 14 +- .../Parts/__tests__/SubagentCall.test.tsx | 5 + .../Chat/Messages/Content/ToolCall.tsx | 10 +- .../Chat/Messages/Content/ToolCallGroup.tsx | 7 +- .../Content/ToolOutput/StackedToolIcons.tsx | 8 +- .../Messages/Content/ToolOutput/ToolIcon.tsx | 11 +- .../ContentParts.integration.test.tsx | 12 +- .../Content/__tests__/ToolCall.test.tsx | 10 +- .../Content/__tests__/ToolCallGroup.test.tsx | 10 +- .../MCP/__tests__/useVisibleTools.test.ts | 43 ++++++ client/src/hooks/MCP/index.ts | 2 +- client/src/hooks/MCP/useMCPIconMap.ts | 14 +- client/src/hooks/MCP/useVisibleTools.ts | 5 +- client/src/utils/toolLabels.ts | 16 +- packages/api/src/agents/context.ts | 5 +- packages/api/src/agents/initialize.ts | 15 +- packages/api/src/index.ts | 1 + packages/api/src/mcp/__tests__/auth.test.ts | 34 +++++ .../api/src/mcp/__tests__/context.test.ts | 27 ++++ packages/api/src/mcp/__tests__/utils.test.ts | 30 ++++ packages/api/src/mcp/auth.ts | 8 +- packages/api/src/mcp/context.ts | 35 +++++ packages/api/src/mcp/oauth/events.ts | 13 +- packages/api/src/mcp/utils.ts | 35 +---- packages/api/src/tools/classification.ts | 4 +- packages/api/src/tools/definitions.ts | 10 +- packages/data-provider/src/config.ts | 101 +++++++++++++ .../data-provider/src/splitMCPToolKey.spec.ts | 137 ++++++++++++++++++ .../data-schemas/src/methods/agent.spec.ts | 59 ++++++++ packages/data-schemas/src/methods/agent.ts | 63 +++++++- 39 files changed, 1052 insertions(+), 125 deletions(-) create mode 100644 client/src/hooks/MCP/__tests__/useVisibleTools.test.ts create mode 100644 packages/api/src/mcp/__tests__/context.test.ts create mode 100644 packages/api/src/mcp/context.ts create mode 100644 packages/data-provider/src/splitMCPToolKey.spec.ts 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; }