From 49859c04a2fcb0a48853519ede4912f633db8654 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 13 Jun 2026 11:26:49 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=97=84=EF=B8=8F=20fix:=20Gate=20Request-S?= =?UTF-8?q?coped=20MCP=20Servers=20Out=20of=20Persistent=20Tool=20Cache=20?= =?UTF-8?q?(#13672)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ๐Ÿ—„๏ธ fix: Gate Request-Scoped MCP Servers Out of Persistent Tool Cache PR #13626 established that request-scoped MCP servers (runtime OPENID/GRAPH/BODY placeholders) must not use the persistent 12h tool cache, but only gated three of five touchpoints. The panel endpoint still back-filled the cache and the OAuth callback still wrote to it, while agent loading read those entries ungated โ€” pinning ephemeral model-spec/agent toolsets to stale definitions for up to 12h. Centralize the invariant in createMCPToolCacheService: a getServerConfig resolver dep gates both writers and a new service-owned getMCPServerTools read, so every current and future caller is covered. Callers that already hold the parsed config pass it to skip resolution; the per-call skipCache flag and duplicated call-site gates are removed in favor of the single config-based mechanism. Resolution failures fail open to preserve prior behavior. * ๐Ÿฉน fix: Address Codex Review on Cache Gating - Repair getCachedTools.spec.js, which destructured the relocated getMCPServerTools directly from the module; its coverage now lives in the service-level tools.spec.ts. - Resolve the merged (Config-tier-aware) server config in the OAuth callback before writing tool definitions, so the cache gate detects request-scoped servers supplied via admin Config overlays that the base registry lookup cannot see. - Discover tools actively for request-scoped servers in the panel endpoint via ephemeral reinitialization: such servers have no stored app/user connections, so the previous getServerToolFunctions fallback returned an empty toolset once the cache read was gated. * ๐Ÿงต fix: Address Second Codex Review on Cache Gating - Resolve the merged server config before the OAuth callback reconnects, so the connection itself uses Config-tier overlays rather than only the subsequent cache write. - Pass Config-tier candidates into the panel's request-scoped discovery, matching the reinitialize route: reinitMCPServer forwards configServers (not the provided serverConfig) to its OAuth discovery fallback. - Document the accepted read-path trade-off: the gate resolver sees base configs only, all writers pass merged configs, so a pre-gating or overlay-divergent entry survives at most one cache TTL. * ๐Ÿš chore: Rework Cache Gating for BODY-Only Request Scoping After #13673 narrowed requiresEphemeralUserConnection to BODY placeholders, the central gate follows the predicate unchanged, but the panel's active discovery no longer serves a purpose: the only remaining request-scoped class cannot connect outside a chat turn, so the reinitialization attempt would always fail at the missing-body check. Remove that path; OpenID/Graph servers are persistent user-scoped again and flow through the stored-connection and cache lookups as before. Flip test fixtures that used OPENID placeholders to denote request-scoped configs over to BODY placeholders. * ๐ŸชŸ fix: Check Config Overlays in Agent-Loading Cache Reads The cache service's registry resolver sees only base YAML/DB configs, so a BODY placeholder introduced by a request-tier Config overlay was invisible to the gate on the agent-loading read path: model-spec and ephemeral-agent expansion could read a leftover persistent entry and pin stale concrete tool names instead of the mcp_all fresh-discovery path. Check the raw overlay candidate inline in loadEphemeralAgent and loadAddedAgent โ€” a pure placeholder scan with no extra IO โ€” and skip the cache read when the overlay makes the server request-scoped. Widen UserScopedConnectionConfig so raw (pre-inspection) configs qualify for the scoping predicates, which only check key presence. * ๐Ÿงช test: Guard Run-Scoped MCP Definition Handoff Boundaries The original ClickHouse breaker storm regressed precisely at field pass-through boundaries that unit tests of each end could not see: initializeAgent dropping mcpAvailableTools from its destructure, and the agent tool context losing it on the way into ON_TOOL_EXECUTE. Add direct guards on both hops: the loadTools result must surface on the initialized agent, and the captured toolExecuteOptions closure must forward it to loadToolsForExecution. --- api/app/clients/tools/util/handleTools.js | 5 +- .../clients/tools/util/handleTools.test.js | 8 +- api/server/controllers/mcp.js | 9 +- api/server/routes/__tests__/mcp.spec.js | 63 +++++++ api/server/routes/mcp.js | 17 ++ .../Config/__tests__/getCachedTools.spec.js | 40 +---- api/server/services/Config/getCachedTools.js | 19 -- api/server/services/Config/mcp.js | 14 +- .../Endpoints/agents/initialize.spec.js | 42 +++++ api/server/services/ToolService.js | 4 +- api/server/services/Tools/mcp.js | 2 +- api/server/services/Tools/mcp.spec.js | 13 +- .../services/__tests__/ToolService.spec.js | 34 +++- .../src/agents/__tests__/initialize.test.ts | 51 ++++++ .../api/src/agents/__tests__/load.spec.ts | 39 ++++ packages/api/src/agents/added.ts | 9 +- packages/api/src/agents/load.ts | 9 +- packages/api/src/mcp/tools.spec.ts | 170 +++++++++++++++--- packages/api/src/mcp/tools.ts | 79 ++++++-- packages/api/src/mcp/utils.ts | 22 ++- 20 files changed, 515 insertions(+), 134 deletions(-) diff --git a/api/app/clients/tools/util/handleTools.js b/api/app/clients/tools/util/handleTools.js index 3f0dc8ab9b..adeb9f7ca9 100644 --- a/api/app/clients/tools/util/handleTools.js +++ b/api/app/clients/tools/util/handleTools.js @@ -9,7 +9,6 @@ const { getCodeApiAuthHeaders, buildImageToolContext, buildWebSearchContext, - requiresEphemeralUserConnection, buildWebSearchDynamicContext, } = require('@librechat/api'); const { @@ -494,9 +493,7 @@ const loadTools = async ({ } if (!availableTools) { try { - availableTools = requiresEphemeralUserConnection(config.config) - ? null - : await getMCPServerTools(safeUser.id, serverName); + availableTools = await getMCPServerTools(safeUser.id, serverName, config.config); } catch (error) { logger.error(`Error fetching available tools for MCP server ${serverName}:`, error); } diff --git a/api/app/clients/tools/util/handleTools.test.js b/api/app/clients/tools/util/handleTools.test.js index 7d61a2a3a5..697649e3bd 100644 --- a/api/app/clients/tools/util/handleTools.test.js +++ b/api/app/clients/tools/util/handleTools.test.js @@ -329,7 +329,11 @@ describe('Tool Handlers', () => { }); expect(result.loadedTools).toEqual([{ name: 'loaded-mcp-tool' }]); - expect(mockGetMCPServerTools).not.toHaveBeenCalled(); + expect(mockGetMCPServerTools).toHaveBeenCalledWith( + fakeUser._id.toString(), + serverName, + serverConfig, + ); expect(mockCreateMCPTool).toHaveBeenCalledWith( expect.objectContaining({ requestBody, @@ -435,7 +439,7 @@ describe('Tool Handlers', () => { }); expect(result.loadedTools).toEqual([{ name: 'search-tool' }, { name: 'lookup-tool' }]); - expect(mockGetMCPServerTools).not.toHaveBeenCalled(); + expect(mockGetMCPServerTools).toHaveBeenCalledTimes(1); expect(mockCreateMCPTool).toHaveBeenCalledTimes(2); expect(mockCreateMCPTool).toHaveBeenNthCalledWith( 2, diff --git a/api/server/controllers/mcp.js b/api/server/controllers/mcp.js index 02e29d7596..f5850638b2 100644 --- a/api/server/controllers/mcp.js +++ b/api/server/controllers/mcp.js @@ -96,7 +96,7 @@ const getMCPTools = async (req, res) => { try { return { serverName, - tools: await getMCPServerTools(userId, serverName), + tools: await getMCPServerTools(userId, serverName, mcpConfig[serverName]), }; } catch (error) { logger.error(`[getMCPTools] Error fetching cached tools for ${serverName}:`, error); @@ -125,7 +125,12 @@ const getMCPTools = async (req, res) => { if (Object.keys(serverTools).length > 0) { // Cache asynchronously without blocking - cacheMCPServerTools({ userId, serverName, serverTools }).catch((err) => + cacheMCPServerTools({ + userId, + serverName, + serverTools, + serverConfig: mcpConfig[serverName], + }).catch((err) => logger.error(`[getMCPTools] Failed to cache tools for ${serverName}:`, err), ); } diff --git a/api/server/routes/__tests__/mcp.spec.js b/api/server/routes/__tests__/mcp.spec.js index a55d6ef81d..a1f981843c 100644 --- a/api/server/routes/__tests__/mcp.spec.js +++ b/api/server/routes/__tests__/mcp.spec.js @@ -762,6 +762,69 @@ describe('MCP Routes', () => { expect(response.headers.location).toContain(`${basePath}/oauth/success`); }); + it('should forward the merged server config so the tool cache gate sees request-scoped servers', async () => { + const flowId = 'test-user-id:test-server'; + const mockFlowManager = { + getFlowState: jest.fn().mockResolvedValue({ + status: 'PENDING', + createdAt: Date.now(), + }), + completeFlow: jest.fn().mockResolvedValue(true), + deleteFlow: jest.fn().mockResolvedValue(true), + }; + const mockFlowState = { + serverName: 'test-server', + userId: 'test-user-id', + metadata: {}, + clientInfo: {}, + codeVerifier: 'test-verifier', + }; + const mergedServerConfig = { + type: 'streamable-http', + url: 'https://override.example.com/{{LIBRECHAT_BODY_CONVERSATIONID}}/mcp', + source: 'config', + }; + const fetchedTools = [{ name: 'search', inputSchema: { type: 'object' } }]; + + getLogStores.mockReturnValue({}); + require('~/config').getFlowStateManager.mockReturnValue(mockFlowManager); + MCPOAuthHandler.getFlowState.mockResolvedValue(mockFlowState); + MCPOAuthHandler.completeOAuthFlow.mockResolvedValue({ + access_token: 'test-token', + }); + MCPTokenStorage.storeTokens.mockResolvedValue(); + mockRegistryInstance.getServerConfig.mockResolvedValue({}); + mockResolveAllMcpConfigs.mockResolvedValueOnce({ 'test-server': mergedServerConfig }); + + const mockMcpManager = { + getUserConnection: jest.fn().mockResolvedValue({ + fetchTools: jest.fn().mockResolvedValue(fetchedTools), + }), + }; + require('~/config').getMCPManager.mockReturnValue(mockMcpManager); + require('~/config').getOAuthReconnectionManager.mockReturnValue({ + clearReconnection: jest.fn(), + }); + const { updateMCPServerTools } = require('~/server/services/Config/mcp'); + updateMCPServerTools.mockResolvedValue(); + + const response = await request(app) + .get('/api/mcp/test-server/oauth/callback') + .query({ code: 'test-code', state: flowId }); + + expect(response.status).toBe(302); + expect(mockResolveAllMcpConfigs).toHaveBeenCalledWith('test-user-id'); + expect(mockMcpManager.getUserConnection).toHaveBeenCalledWith( + expect.objectContaining({ serverConfig: mergedServerConfig }), + ); + expect(updateMCPServerTools).toHaveBeenCalledWith({ + userId: 'test-user-id', + serverName: 'test-server', + tools: fetchedTools, + serverConfig: mergedServerConfig, + }); + }); + it('should reject when no PENDING flow exists and no cookies are present', async () => { const flowId = 'test-user-id:test-server'; const mockFlowManager = { diff --git a/api/server/routes/mcp.js b/api/server/routes/mcp.js index a216a897b4..5e9fad5422 100644 --- a/api/server/routes/mcp.js +++ b/api/server/routes/mcp.js @@ -39,6 +39,7 @@ const { } = require('~/config'); const { getServerConnectionStatus, + resolveAllMcpConfigs, resolveConfigServers, getMCPSetupData, } = require('~/server/services/MCP'); @@ -442,10 +443,25 @@ router.get('/:serverName/oauth/callback', async (req, res) => { if (flowState.userId !== 'system') { const user = { id: flowState.userId }; + /** Merged config (incl. Config-tier overlays) so the reconnection and + * the cache gate both see request-scoped servers the base registry + * lookup misses */ + let serverConfig; + try { + const allConfigs = await resolveAllMcpConfigs(flowState.userId); + serverConfig = allConfigs?.[serverName]; + } catch (error) { + logger.warn( + `[MCP OAuth] Could not resolve server config for ${serverName} before reconnecting:`, + error, + ); + } + const userConnection = await mcpManager.getUserConnection({ user, serverName, flowManager, + serverConfig, tokenMethods: { findToken: db.findToken, updateToken: db.updateToken, @@ -466,6 +482,7 @@ router.get('/:serverName/oauth/callback', async (req, res) => { userId: flowState.userId, serverName, tools, + serverConfig, }); } else { logger.debug(`[MCP OAuth] System-level OAuth completed for ${serverName}`); diff --git a/api/server/services/Config/__tests__/getCachedTools.spec.js b/api/server/services/Config/__tests__/getCachedTools.spec.js index 71ae8b5a57..3f85a018f0 100644 --- a/api/server/services/Config/__tests__/getCachedTools.spec.js +++ b/api/server/services/Config/__tests__/getCachedTools.spec.js @@ -1,12 +1,6 @@ const { CacheKeys } = require('librechat-data-provider'); -jest.mock('@librechat/data-schemas', () => ({ - logger: { - error: jest.fn(), - }, -})); jest.mock('~/cache/getLogStores'); -const { logger } = require('@librechat/data-schemas'); const getLogStores = require('~/cache/getLogStores'); const mockCache = { get: jest.fn(), set: jest.fn(), delete: jest.fn() }; @@ -16,7 +10,6 @@ const { ToolCacheKeys, getCachedTools, setCachedTools, - getMCPServerTools, invalidateCachedTools, } = require('../getCachedTools'); @@ -74,41 +67,10 @@ describe('getCachedTools', () => { expect(mockCache.delete).toHaveBeenCalledWith(ToolCacheKeys.GLOBAL); }); - it('getMCPServerTools should use TOOL_CACHE namespace', async () => { - mockCache.get.mockResolvedValue(null); - await getMCPServerTools('user1', 'github'); - expect(getLogStores).toHaveBeenCalledWith(CacheKeys.TOOL_CACHE); - expect(mockCache.get).toHaveBeenCalledWith(ToolCacheKeys.MCP_SERVER('user1', 'github')); - }); - - it('getMCPServerTools should return null when the cache lookup fails', async () => { - const error = new Error('cache unavailable'); - mockCache.get.mockRejectedValue(error); - - await expect(getMCPServerTools('user1', 'github')).resolves.toBeNull(); - expect(logger.error).toHaveBeenCalledWith( - '[getMCPServerTools] Error fetching cached tools for github:', - error, - ); - }); - - it('getMCPServerTools should return null when the cache store is unavailable', async () => { - const error = new Error('cache store unavailable'); - getLogStores.mockImplementationOnce(() => { - throw error; - }); - - await expect(getMCPServerTools('user1', 'github')).resolves.toBeNull(); - expect(logger.error).toHaveBeenCalledWith( - '[getMCPServerTools] Error fetching cached tools for github:', - error, - ); - }); - it('should NOT use CONFIG_STORE namespace', async () => { mockCache.get.mockResolvedValue(null); await getCachedTools(); - await getMCPServerTools('user1', 'github'); + await getCachedTools({ userId: 'user1', serverName: 'github' }); mockCache.set.mockResolvedValue(true); await setCachedTools({ tool1: {} }); mockCache.delete.mockResolvedValue(true); diff --git a/api/server/services/Config/getCachedTools.js b/api/server/services/Config/getCachedTools.js index 083cfae6ba..2877234b58 100644 --- a/api/server/services/Config/getCachedTools.js +++ b/api/server/services/Config/getCachedTools.js @@ -1,5 +1,4 @@ const { CacheKeys, Time } = require('librechat-data-provider'); -const { logger } = require('@librechat/data-schemas'); const getLogStores = require('~/cache/getLogStores'); /** @@ -82,27 +81,9 @@ async function invalidateCachedTools(options = {}) { await Promise.all(keysToDelete.map((key) => cache.delete(key))); } -/** - * Gets MCP tools for a specific server from cache - * @function getMCPServerTools - * @param {string} userId - The user ID - * @param {string} serverName - The MCP server name - * @returns {Promise} The available tools for the server - */ -async function getMCPServerTools(userId, serverName) { - try { - const cache = getLogStores(CacheKeys.TOOL_CACHE); - return (await cache.get(ToolCacheKeys.MCP_SERVER(userId, serverName))) || null; - } catch (error) { - logger.error(`[getMCPServerTools] Error fetching cached tools for ${serverName}:`, error); - return null; - } -} - module.exports = { ToolCacheKeys, getCachedTools, setCachedTools, - getMCPServerTools, invalidateCachedTools, }; diff --git a/api/server/services/Config/mcp.js b/api/server/services/Config/mcp.js index fa37e223f5..2bd64cc31b 100644 --- a/api/server/services/Config/mcp.js +++ b/api/server/services/Config/mcp.js @@ -1,13 +1,17 @@ -const { createMCPToolCacheService } = require('@librechat/api'); +const { createMCPToolCacheService, MCPServersRegistry } = require('@librechat/api'); const { getCachedTools, setCachedTools } = require('./getCachedTools'); -const { mergeAppTools, cacheMCPServerTools, updateMCPServerTools } = createMCPToolCacheService({ - getCachedTools, - setCachedTools, -}); +const { mergeAppTools, cacheMCPServerTools, updateMCPServerTools, getMCPServerTools } = + createMCPToolCacheService({ + getCachedTools, + setCachedTools, + getServerConfig: (serverName, userId) => + MCPServersRegistry.getInstance().getServerConfig(serverName, userId), + }); module.exports = { mergeAppTools, + getMCPServerTools, cacheMCPServerTools, updateMCPServerTools, }; diff --git a/api/server/services/Endpoints/agents/initialize.spec.js b/api/server/services/Endpoints/agents/initialize.spec.js index c2ca150228..1e331fc40d 100644 --- a/api/server/services/Endpoints/agents/initialize.spec.js +++ b/api/server/services/Endpoints/agents/initialize.spec.js @@ -539,6 +539,48 @@ describe('initializeClient โ€” subagent loading', () => { expect(arg.actionsEnabled).toBe(true); }); + it('threads run-scoped MCP tool definitions into ON_TOOL_EXECUTE loading', async () => { + /** Regression guard for the request-scoped MCP/PTC handoff: the + * `mcpAvailableTools` discovered at run start must survive + * `buildAgentToolContext` and reach `loadToolsForExecution`, otherwise + * request-scoped servers reinitialize on every programmatic tool call + * and can trip the MCP circuit breaker under parallel calls. */ + const mcpTool = 'list_tables_mcp_ClickHouse'; + const mcpAvailableTools = { + ClickHouse: { + [mcpTool]: { + type: 'function', + function: { + name: mcpTool, + description: 'List tables', + parameters: { type: 'object', properties: {} }, + }, + }, + }, + }; + const primaryConfig = { + ...makePrimaryConfig({}), + toolRegistry: new Map([[mcpTool, { name: mcpTool }]]), + mcpAvailableTools, + }; + mockInitializeAgent.mockResolvedValue(primaryConfig); + + await initializeClient({ + req: makeSubagentReq(), + res: {}, + signal: new AbortController().signal, + endpointOption: makeEndpointOption(), + }); + + expect(capturedToolExecuteOptions?.loadTools).toBeInstanceOf(Function); + await capturedToolExecuteOptions.loadTools([mcpTool], PRIMARY_ID); + + expect(mockLoadToolsForExecution).toHaveBeenCalledTimes(1); + expect(mockLoadToolsForExecution).toHaveBeenCalledWith( + expect.objectContaining({ mcpAvailableTools }), + ); + }); + it('deduplicates repeated ids in subagents.agent_ids', async () => { const subAgent = await createAgent({ id: DUPLICATE_SUBAGENT_ID, diff --git a/api/server/services/ToolService.js b/api/server/services/ToolService.js index 52d9f713ad..99054b1fca 100644 --- a/api/server/services/ToolService.js +++ b/api/server/services/ToolService.js @@ -16,7 +16,6 @@ const { isActionDomainAllowed, buildWebSearchContext, buildImageToolContext, - requiresEphemeralUserConnection, buildToolClassification, getMissingCustomUserVars, buildWebSearchDynamicContext, @@ -755,12 +754,11 @@ async function loadToolDefinitionsWrapper({ req, res, agent, streamId = null, to return null; } - const requestScoped = requiresEphemeralUserConnection(serverConfig); if (mcpAvailableTools[serverName]) { return mcpAvailableTools[serverName]; } - const cached = requestScoped ? null : await getMCPServerTools(userId, serverName); + const cached = await getMCPServerTools(userId, serverName, serverConfig); if (cached) { rememberMCPAvailableTools(serverName, cached); await addPendingOAuthServer(); diff --git a/api/server/services/Tools/mcp.js b/api/server/services/Tools/mcp.js index 5a2dcf8fcc..c0e32ccd8f 100644 --- a/api/server/services/Tools/mcp.js +++ b/api/server/services/Tools/mcp.js @@ -219,7 +219,7 @@ async function reinitMCPServer({ userId: user.id, serverName, tools, - skipCache: ephemeralServer, + serverConfig, }); } diff --git a/api/server/services/Tools/mcp.spec.js b/api/server/services/Tools/mcp.spec.js index 2dc92ebd9b..ab8cd3f281 100644 --- a/api/server/services/Tools/mcp.spec.js +++ b/api/server/services/Tools/mcp.spec.js @@ -144,6 +144,11 @@ describe('reinitMCPServer โ€” customUserVars gating (issue #10969)', () => { it('disconnects ephemeral BODY-scoped connections after loading tools', async () => { const disconnect = jest.fn().mockResolvedValue(undefined); const tools = [{ name: 'search', inputSchema: { type: 'object', properties: {} } }]; + const serverConfig = { + type: 'streamable-http', + url: 'https://thingy.example.com/messages/{{LIBRECHAT_BODY_MESSAGEID}}/mcp', + source: 'yaml', + }; mockGetConnection.mockResolvedValue({ disconnect, fetchTools: jest.fn().mockResolvedValue(tools), @@ -152,11 +157,7 @@ describe('reinitMCPServer โ€” customUserVars gating (issue #10969)', () => { await reinitMCPServer({ user, serverName, - serverConfig: { - type: 'streamable-http', - url: 'https://thingy.example.com/messages/{{LIBRECHAT_BODY_MESSAGEID}}/mcp', - source: 'yaml', - }, + serverConfig, requestBody: { messageId: 'msg-789' }, userMCPAuthMap: undefined, }); @@ -165,7 +166,7 @@ describe('reinitMCPServer โ€” customUserVars gating (issue #10969)', () => { expect(mockUpdateMCPServerTools).toHaveBeenCalledWith( expect.objectContaining({ tools, - skipCache: true, + serverConfig, }), ); }); diff --git a/api/server/services/__tests__/ToolService.spec.js b/api/server/services/__tests__/ToolService.spec.js index cd8d05478c..0d69fb92ef 100644 --- a/api/server/services/__tests__/ToolService.spec.js +++ b/api/server/services/__tests__/ToolService.spec.js @@ -472,7 +472,11 @@ describe('ToolService - Action Capability Gating', () => { }); expect(result.toolDefinitions).toEqual([mcpTool]); - expect(mockGetMCPServerTools).toHaveBeenCalledWith(req.user.id, serverName); + expect(mockGetMCPServerTools).toHaveBeenCalledWith( + req.user.id, + serverName, + expect.objectContaining({ requiresOAuth: true }), + ); expect(reinitMCPServer).toHaveBeenCalledWith( expect.objectContaining({ serverName, @@ -542,7 +546,11 @@ describe('ToolService - Action Capability Gating', () => { definitionsOnly: true, }); - expect(mockGetMCPServerTools).toHaveBeenCalledWith(req.user.id, serverName); + expect(mockGetMCPServerTools).toHaveBeenCalledWith( + req.user.id, + serverName, + expect.objectContaining({ requiresOAuth: true }), + ); expect(reinitMCPServer).toHaveBeenCalledTimes(1); expect(reinitMCPServer).toHaveBeenCalledWith( expect.objectContaining({ @@ -756,7 +764,13 @@ describe('ToolService - Action Capability Gating', () => { requestBody: req.body, }), ); - expect(mockGetMCPServerTools).not.toHaveBeenCalled(); + expect(mockGetMCPServerTools).toHaveBeenCalledWith( + req.user.id, + serverName, + expect.objectContaining({ + url: expect.stringContaining('LIBRECHAT_BODY_MESSAGEID'), + }), + ); }); it('returns run-scoped MCP tool definitions for request-scoped servers', async () => { @@ -801,7 +815,13 @@ describe('ToolService - Action Capability Gating', () => { expect(result.toolDefinitions).toEqual([mcpTool]); expect(result.mcpAvailableTools).toEqual({ [serverName]: availableTools }); - expect(mockGetMCPServerTools).not.toHaveBeenCalled(); + expect(mockGetMCPServerTools).toHaveBeenCalledWith( + req.user.id, + serverName, + expect.objectContaining({ + url: expect.stringContaining('LIBRECHAT_BODY_MESSAGEID'), + }), + ); }); it('should preserve pending-flow expiry for OAuth URLs captured during discovery', async () => { @@ -897,7 +917,11 @@ describe('ToolService - Action Capability Gating', () => { expect(result.toolDefinitions).toEqual([mcpTool]); expect(mockGetServerConfig).not.toHaveBeenCalled(); - expect(mockGetMCPServerTools).toHaveBeenCalledWith(req.user.id, serverName); + expect(mockGetMCPServerTools).toHaveBeenCalledWith( + req.user.id, + serverName, + expect.objectContaining({ url: 'https://config.example.com/mcp' }), + ); }); }); diff --git a/packages/api/src/agents/__tests__/initialize.test.ts b/packages/api/src/agents/__tests__/initialize.test.ts index 857638f59a..1bec88f837 100644 --- a/packages/api/src/agents/__tests__/initialize.test.ts +++ b/packages/api/src/agents/__tests__/initialize.test.ts @@ -2068,3 +2068,54 @@ describe('initializeAgent โ€” code-generated file thread filter (regression)', ( expect(getUserCodeFiles).not.toHaveBeenCalled(); }); }); + +describe('initializeAgent โ€” run-scoped MCP tool definitions', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('carries mcpAvailableTools from the loadTools result onto the initialized agent', async () => { + /** Regression guard for the request-scoped MCP/PTC handoff: dropping this + * field at the destructure boundary forces per-call reinitialization + * downstream and can storm the MCP circuit breaker. */ + const { agent, req, res, loadTools, db } = createMocks(); + const mcpTool = 'list_tables_mcp_ClickHouse'; + const mcpAvailableTools = { + ClickHouse: { + [mcpTool]: { + type: 'function' as const, + function: { + name: mcpTool, + description: 'List tables', + parameters: { type: 'object' as const, properties: {} }, + }, + }, + }, + }; + loadTools.mockResolvedValue({ + tools: [], + toolContextMap: {}, + dynamicToolContextMap: {}, + userMCPAuthMap: undefined, + toolRegistry: undefined, + toolDefinitions: [], + hasDeferredTools: false, + mcpAvailableTools, + }); + + const result = await initializeAgent( + { + req, + res, + agent, + loadTools, + endpointOption: { endpoint: EModelEndpoint.agents }, + allowedProviders: new Set([Providers.OPENAI]), + isInitialAgent: true, + }, + db, + ); + + expect(result.mcpAvailableTools).toEqual(mcpAvailableTools); + }); +}); diff --git a/packages/api/src/agents/__tests__/load.spec.ts b/packages/api/src/agents/__tests__/load.spec.ts index 332f278873..705c5e8ef7 100644 --- a/packages/api/src/agents/__tests__/load.spec.ts +++ b/packages/api/src/agents/__tests__/load.spec.ts @@ -9,6 +9,7 @@ import type { TEphemeralAgent, TConversation, } from 'librechat-data-provider'; +import type { AppConfig } from '@librechat/data-schemas'; import type { LoadAgentParams, LoadAgentDeps } from '../load'; import { loadAddedAgent } from '../added'; import { loadAgent } from '../load'; @@ -128,6 +129,44 @@ describe('loadAgent', () => { } }); + test('should skip cached tools for servers made request-scoped by a config overlay', async () => { + const { EPHEMERAL_AGENT_ID } = Constants; + + mockGetMCPServerTools.mockResolvedValue({ tool1_mcp_server1: {} }); + + const mockReq = { + user: { id: 'user123' }, + config: { + mcpConfig: { + 'body-scoped': { + type: 'streamable-http' as const, + url: 'https://mcp.example.com/{{LIBRECHAT_BODY_CONVERSATIONID}}/mcp', + }, + }, + } as unknown as AppConfig, + body: { + ephemeralAgent: { + mcp: ['body-scoped', 'server1'], + }, + }, + }; + + const result = await loadAgent( + { + req: mockReq, + agent_id: EPHEMERAL_AGENT_ID as string, + endpoint: 'openai', + model_parameters: { model: 'gpt-4' } as unknown as AgentModelParameters, + }, + deps, + ); + + expect(mockGetMCPServerTools).toHaveBeenCalledTimes(1); + expect(mockGetMCPServerTools).toHaveBeenCalledWith('user123', 'server1'); + expect(result?.tools).toContain(`${Constants.mcp_all}${Constants.mcp_delimiter}body-scoped`); + expect(result?.tools).toContain('tool1_mcp_server1'); + }); + test('should return null for non-existent agent', async () => { const mockReq = { user: { id: 'user123' } }; const result = await loadAgent( diff --git a/packages/api/src/agents/added.ts b/packages/api/src/agents/added.ts index 829f5285d3..85485ad958 100644 --- a/packages/api/src/agents/added.ts +++ b/packages/api/src/agents/added.ts @@ -9,6 +9,7 @@ import { } from 'librechat-data-provider'; import type { Agent, TConversation, TModelSpec } from 'librechat-data-provider'; import type { AppConfig } from '@librechat/data-schemas'; +import { requiresEphemeralUserConnection } from '~/mcp/utils'; import { getCustomEndpointConfig } from '~/app/config'; const { mcp_all, mcp_delimiter } = Constants; @@ -184,7 +185,13 @@ export async function loadAddedAgent( if (addedServers.has(mcpServer)) { continue; } - const serverTools = await deps.getMCPServerTools(userId, mcpServer); + /** Request-tier overlays are invisible to the cache service's registry + * resolver โ€” overlay-scoped servers expand fresh via `mcp_all` instead */ + const overlayConfig = appConfig?.mcpConfig?.[mcpServer]; + const serverTools = + overlayConfig && requiresEphemeralUserConnection(overlayConfig) + ? null + : await deps.getMCPServerTools(userId, mcpServer); if (!serverTools) { tools.push(`${mcp_all}${mcp_delimiter}${mcpServer}`); addedServers.add(mcpServer); diff --git a/packages/api/src/agents/load.ts b/packages/api/src/agents/load.ts index 83e6e2832b..dfd741428e 100644 --- a/packages/api/src/agents/load.ts +++ b/packages/api/src/agents/load.ts @@ -13,6 +13,7 @@ import type { Agent, } from 'librechat-data-provider'; import type { AppConfig } from '@librechat/data-schemas'; +import { requiresEphemeralUserConnection } from '~/mcp/utils'; import { getCustomEndpointConfig } from '~/app/config'; const { mcp_all, mcp_delimiter } = Constants; @@ -79,7 +80,13 @@ export async function loadEphemeralAgent( if (addedServers.has(mcpServer)) { continue; } - const serverTools = await deps.getMCPServerTools(userId, mcpServer); + /** Request-tier overlays are invisible to the cache service's registry + * resolver โ€” overlay-scoped servers expand fresh via `mcp_all` instead */ + const overlayConfig = req.config?.mcpConfig?.[mcpServer]; + const serverTools = + overlayConfig && requiresEphemeralUserConnection(overlayConfig) + ? null + : await deps.getMCPServerTools(userId, mcpServer); if (!serverTools) { tools.push(`${mcp_all}${mcp_delimiter}${mcpServer}`); addedServers.add(mcpServer); diff --git a/packages/api/src/mcp/tools.spec.ts b/packages/api/src/mcp/tools.spec.ts index fe9d87bb3c..f1a630487a 100644 --- a/packages/api/src/mcp/tools.spec.ts +++ b/packages/api/src/mcp/tools.spec.ts @@ -1,12 +1,25 @@ import { Constants } from 'librechat-data-provider'; +import type { LCAvailableTools, ParsedServerConfig } from './types'; import type { MCPToolInput, MCPToolCacheDeps } from './tools'; -import type { LCAvailableTools } from './types'; import { createMCPToolCacheService } from './tools'; +const requestScopedConfig: ParsedServerConfig = { + type: 'streamable-http', + url: 'https://mcp.example.com/{{LIBRECHAT_BODY_CONVERSATIONID}}/mcp', + source: 'yaml', +}; + +const cacheableConfig: ParsedServerConfig = { + type: 'streamable-http', + url: 'https://mcp.example.com/mcp', + source: 'yaml', +}; + function createMockDeps(overrides: Partial = {}): MCPToolCacheDeps { return { getCachedTools: jest.fn().mockResolvedValue(null), setCachedTools: jest.fn().mockResolvedValue(true), + getServerConfig: jest.fn().mockResolvedValue(undefined), ...overrides, }; } @@ -57,8 +70,10 @@ describe('createMCPToolCacheService', () => { }); }); - it('constructs tool names without caching when skipCache is true', async () => { - const deps = createMockDeps(); + it('builds tool names without caching when the resolved config is request-scoped', async () => { + const deps = createMockDeps({ + getServerConfig: jest.fn().mockResolvedValue(requestScopedConfig), + }); const { updateMCPServerTools } = createMCPToolCacheService(deps); const tools: MCPToolInput[] = [ { @@ -72,14 +87,42 @@ describe('createMCPToolCacheService', () => { userId: 'u1', serverName: 'body-scoped', tools, - skipCache: true, }); const expectedKey = `search${Constants.mcp_delimiter}body-scoped`; expect(result[expectedKey]).toBeDefined(); + expect(deps.getServerConfig).toHaveBeenCalledWith('body-scoped', 'u1'); expect(deps.setCachedTools).not.toHaveBeenCalled(); }); + it('uses a provided serverConfig without calling the resolver', async () => { + const deps = createMockDeps(); + const { updateMCPServerTools } = createMCPToolCacheService(deps); + const tools: MCPToolInput[] = [{ name: 'search' }]; + + await updateMCPServerTools({ + userId: 'u1', + serverName: 'body-scoped', + tools, + serverConfig: requestScopedConfig, + }); + + expect(deps.getServerConfig).not.toHaveBeenCalled(); + expect(deps.setCachedTools).not.toHaveBeenCalled(); + }); + + it('fails open and caches when config resolution throws', async () => { + const deps = createMockDeps({ + getServerConfig: jest.fn().mockRejectedValue(new Error('registry not initialized')), + }); + const { updateMCPServerTools } = createMCPToolCacheService(deps); + const tools: MCPToolInput[] = [{ name: 'search' }]; + + await updateMCPServerTools({ userId: 'u1', serverName: 'srv', tools }); + + expect(deps.setCachedTools).toHaveBeenCalled(); + }); + it('propagates setCachedTools errors', async () => { const deps = createMockDeps({ setCachedTools: jest.fn().mockRejectedValue(new Error('Redis down')), @@ -178,6 +221,17 @@ describe('createMCPToolCacheService', () => { }); describe('cacheMCPServerTools', () => { + const serverTools: LCAvailableTools = { + tool: { + type: 'function', + ['function']: { + name: 'tool', + description: '', + parameters: { type: 'object', properties: {} }, + }, + }, + }; + it('no-ops when serverTools is empty', async () => { const deps = createMockDeps(); const { cacheMCPServerTools } = createMCPToolCacheService(deps); @@ -190,16 +244,6 @@ describe('createMCPToolCacheService', () => { it('caches server tools with userId and serverName', async () => { const deps = createMockDeps(); const { cacheMCPServerTools } = createMCPToolCacheService(deps); - const serverTools: LCAvailableTools = { - tool: { - type: 'function', - ['function']: { - name: 'tool', - description: '', - parameters: { type: 'object', properties: {} }, - }, - }, - }; await cacheMCPServerTools({ userId: 'u1', serverName: 'brave', serverTools }); @@ -209,6 +253,17 @@ describe('createMCPToolCacheService', () => { }); }); + it('skips caching for request-scoped servers', async () => { + const deps = createMockDeps({ + getServerConfig: jest.fn().mockResolvedValue(requestScopedConfig), + }); + const { cacheMCPServerTools } = createMCPToolCacheService(deps); + + await cacheMCPServerTools({ userId: 'u1', serverName: 'body-scoped', serverTools }); + + expect(deps.setCachedTools).not.toHaveBeenCalled(); + }); + it('propagates setCachedTools errors', async () => { const deps = createMockDeps({ setCachedTools: jest.fn().mockRejectedValue(new Error('write failed')), @@ -216,21 +271,80 @@ describe('createMCPToolCacheService', () => { const { cacheMCPServerTools } = createMCPToolCacheService(deps); await expect( - cacheMCPServerTools({ - userId: 'u1', - serverName: 'srv', - serverTools: { - t: { - type: 'function', - ['function']: { - name: 't', - description: '', - parameters: { type: 'object', properties: {} }, - }, - }, - }, - }), + cacheMCPServerTools({ userId: 'u1', serverName: 'srv', serverTools }), ).rejects.toThrow('write failed'); }); }); + + describe('getMCPServerTools', () => { + const cachedTools: LCAvailableTools = { + tool: { + type: 'function', + ['function']: { + name: 'tool', + description: '', + parameters: { type: 'object', properties: {} }, + }, + }, + }; + + it('returns cached tools for cacheable servers', async () => { + const deps = createMockDeps({ + getCachedTools: jest.fn().mockResolvedValue(cachedTools), + getServerConfig: jest.fn().mockResolvedValue(cacheableConfig), + }); + const { getMCPServerTools } = createMCPToolCacheService(deps); + + const result = await getMCPServerTools('u1', 'brave'); + + expect(result).toEqual(cachedTools); + expect(deps.getCachedTools).toHaveBeenCalledWith({ userId: 'u1', serverName: 'brave' }); + }); + + it('returns null for request-scoped servers without reading the cache', async () => { + const deps = createMockDeps({ + getCachedTools: jest.fn().mockResolvedValue(cachedTools), + getServerConfig: jest.fn().mockResolvedValue(requestScopedConfig), + }); + const { getMCPServerTools } = createMCPToolCacheService(deps); + + const result = await getMCPServerTools('u1', 'body-scoped'); + + expect(result).toBeNull(); + expect(deps.getCachedTools).not.toHaveBeenCalled(); + }); + + it('uses a provided serverConfig without calling the resolver', async () => { + const deps = createMockDeps({ + getCachedTools: jest.fn().mockResolvedValue(cachedTools), + }); + const { getMCPServerTools } = createMCPToolCacheService(deps); + + const result = await getMCPServerTools('u1', 'body-scoped', requestScopedConfig); + + expect(result).toBeNull(); + expect(deps.getServerConfig).not.toHaveBeenCalled(); + expect(deps.getCachedTools).not.toHaveBeenCalled(); + }); + + it('returns null when the cache is empty', async () => { + const deps = createMockDeps(); + const { getMCPServerTools } = createMCPToolCacheService(deps); + + const result = await getMCPServerTools('u1', 'brave'); + + expect(result).toBeNull(); + }); + + it('returns null instead of throwing when the cache read fails', async () => { + const deps = createMockDeps({ + getCachedTools: jest.fn().mockRejectedValue(new Error('cache unavailable')), + }); + const { getMCPServerTools } = createMCPToolCacheService(deps); + + const result = await getMCPServerTools('u1', 'brave'); + + expect(result).toBeNull(); + }); + }); }); diff --git a/packages/api/src/mcp/tools.ts b/packages/api/src/mcp/tools.ts index fe0f582315..1326bb99f3 100644 --- a/packages/api/src/mcp/tools.ts +++ b/packages/api/src/mcp/tools.ts @@ -1,7 +1,8 @@ import { logger } from '@librechat/data-schemas'; import { Constants } from 'librechat-data-provider'; import type { JsonSchemaType } from '@librechat/agents'; -import type { LCAvailableTools, LCFunctionTool } from './types'; +import type { LCAvailableTools, LCFunctionTool, ParsedServerConfig } from './types'; +import { requiresEphemeralUserConnection } from './utils'; export interface MCPToolInput { name: string; @@ -18,31 +19,66 @@ export interface MCPToolCacheDeps { tools: LCAvailableTools, options?: { userId?: string; serverName?: string }, ) => Promise; + getServerConfig: (serverName: string, userId?: string) => Promise; } -export function createMCPToolCacheService(deps: MCPToolCacheDeps): { +export interface MCPToolCacheService { updateMCPServerTools: (params: { userId: string; serverName: string; tools: MCPToolInput[] | null; - skipCache?: boolean; + serverConfig?: ParsedServerConfig; }) => Promise; mergeAppTools: (appTools: LCAvailableTools) => Promise; cacheMCPServerTools: (params: { userId: string; serverName: string; serverTools: LCAvailableTools; + serverConfig?: ParsedServerConfig; }) => Promise; -} { - const { getCachedTools, setCachedTools } = deps; + getMCPServerTools: ( + userId: string, + serverName: string, + serverConfig?: ParsedServerConfig, + ) => Promise; +} + +export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheService { + const { getCachedTools, setCachedTools, getServerConfig } = deps; + + /** + * Request-scoped servers resolve runtime user/request placeholders per + * connection, so their definitions must never enter the persistent tool + * cache. Fails open: an unresolvable config is treated as cacheable, + * preserving pre-gating behavior for servers the registry cannot see. + * The resolver sees only base registry configs โ€” callers holding merged + * Config-overlay configs must pass them. All writers do, so an entry that + * predates gating or an overlay change survives at most one cache TTL. + */ + async function isRequestScoped( + userId: string, + serverName: string, + serverConfig?: ParsedServerConfig, + ): Promise { + try { + const config = serverConfig ?? (await getServerConfig(serverName, userId)); + return config ? requiresEphemeralUserConnection(config) : false; + } catch (error) { + logger.debug( + `[MCP Cache] Could not resolve config for ${serverName} (user: ${userId}), treating as cacheable:`, + error, + ); + return false; + } + } async function updateMCPServerTools(params: { userId: string; serverName: string; tools: MCPToolInput[] | null; - skipCache?: boolean; + serverConfig?: ParsedServerConfig; }): Promise { - const { userId, serverName, tools, skipCache = false } = params; + const { userId, serverName, tools, serverConfig } = params; try { const serverTools: LCAvailableTools = {}; const mcpDelimiter = Constants.mcp_delimiter; @@ -65,7 +101,7 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): { serverTools[name] = entry; } - if (skipCache) { + if (await isRequestScoped(userId, serverName, serverConfig)) { logger.debug( `[MCP Cache] Built ${tools.length} tools for request-scoped server ${serverName} (user: ${userId}) without caching`, ); @@ -106,13 +142,20 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): { userId: string; serverName: string; serverTools: LCAvailableTools; + serverConfig?: ParsedServerConfig; }): Promise { - const { userId, serverName, serverTools } = params; + const { userId, serverName, serverTools, serverConfig } = params; try { const count = Object.keys(serverTools).length; if (!count) { return; } + if (await isRequestScoped(userId, serverName, serverConfig)) { + logger.debug( + `[MCP Cache] Skipped caching ${count} tools for request-scoped server ${serverName} (user: ${userId})`, + ); + return; + } await setCachedTools(serverTools, { userId, serverName }); logger.debug(`Cached ${count} MCP server tools for ${serverName} (user: ${userId})`); } catch (error) { @@ -121,5 +164,21 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): { } } - return { updateMCPServerTools, mergeAppTools, cacheMCPServerTools }; + async function getMCPServerTools( + userId: string, + serverName: string, + serverConfig?: ParsedServerConfig, + ): Promise { + if (await isRequestScoped(userId, serverName, serverConfig)) { + return null; + } + try { + return (await getCachedTools({ userId, serverName })) ?? null; + } catch (error) { + logger.error(`[getMCPServerTools] Error fetching cached tools for ${serverName}:`, error); + return null; + } + } + + return { updateMCPServerTools, mergeAppTools, cacheMCPServerTools, getMCPServerTools }; } diff --git a/packages/api/src/mcp/utils.ts b/packages/api/src/mcp/utils.ts index 12829a93e1..9b7d25429d 100644 --- a/packages/api/src/mcp/utils.ts +++ b/packages/api/src/mcp/utils.ts @@ -23,15 +23,19 @@ type PlaceholderValue = | readonly PlaceholderValue[] | { readonly [key: string]: PlaceholderValue }; -type UserScopedConnectionConfig = Pick< - ParsedServerConfig, - 'requiresOAuth' | 'customUserVars' | 'obo' | 'source' | 'dbId' -> & { +type UserScopedConnectionConfig = Pick & { args?: string[]; - env?: Record; - headers?: Record; + /** Loosened from the parsed shapes so raw (pre-inspection) configs qualify; + * scoping predicates only check key presence */ + obo?: { scopes?: string } | null; + customUserVars?: Record< + string, + { description?: string; title?: string; sensitive?: boolean } | undefined + >; + env?: Record; + headers?: Record; oauth?: PlaceholderValue; - oauth_headers?: Record; + oauth_headers?: Record; url?: string; }; @@ -67,7 +71,9 @@ export function requiresOAuthMachinery( } /** Checks that `customUserVars` is present AND non-empty (guards against truthy `{}`) */ -export function hasCustomUserVars(config: Pick): boolean { +export function hasCustomUserVars( + config: Pick, +): boolean { return !!config.customUserVars && Object.keys(config.customUserVars).length > 0; }