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