From 489bc02d4a9e3364ef8997ddfeffdc86b08d1443 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Wed, 5 Aug 2026 17:30:57 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=AD=20fix:=20Fail=20Closed=20When=20Ex?= =?UTF-8?q?pected=20MCP=20Tools=20Are=20Unavailable=20(#14646)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: fail closed when expected mcp tools are unavailable * test: strengthen MCP handoff coverage * fix: clarify unavailable MCP tool guidance * fix: preserve MCP discovery for empty catalogs --- .../agents/__tests__/openai.spec.js | 27 +++ .../agents/__tests__/responses.unit.spec.js | 32 ++++ api/server/controllers/agents/openai.js | 4 + api/server/controllers/agents/responses.js | 9 +- .../services/Endpoints/agents/addedConvo.js | 4 + .../Endpoints/agents/addedConvo.spec.js | 15 ++ .../services/Endpoints/agents/initialize.js | 7 + .../Endpoints/agents/initialize.spec.js | 123 +++++++++++++ api/server/services/ToolService.js | 130 ++++++++++---- api/server/services/Tools/mcp.js | 2 +- api/server/services/Tools/mcp.spec.js | 18 ++ .../services/__tests__/ToolService.spec.js | 132 ++++++++++++++ e2e/setup/fake-model.js | 50 ++++++ e2e/specs/mock/agent-handoffs.spec.ts | 160 +++++++++++++++++ e2e/specs/real/agent-handoff-mcp.spec.ts | 168 ++++++++++++++++++ packages/api/src/agents/discovery.spec.ts | 33 +++- packages/api/src/agents/discovery.ts | 17 ++ packages/api/src/agents/initialize.ts | 11 +- packages/api/src/mcp/tools.spec.ts | 16 +- packages/api/src/mcp/tools.ts | 15 +- packages/api/src/tools/definitions.spec.ts | 132 ++++++++++++++ packages/api/src/tools/definitions.ts | 51 +++++- 22 files changed, 1112 insertions(+), 44 deletions(-) create mode 100644 e2e/specs/real/agent-handoff-mcp.spec.ts diff --git a/api/server/controllers/agents/__tests__/openai.spec.js b/api/server/controllers/agents/__tests__/openai.spec.js index a50038d5dc..c82053e54e 100644 --- a/api/server/controllers/agents/__tests__/openai.spec.js +++ b/api/server/controllers/agents/__tests__/openai.spec.js @@ -196,6 +196,8 @@ jest.mock('~/cache', () => ({ jest.mock('~/server/services/ToolService', () => ({ loadAgentTools: jest.fn().mockResolvedValue([]), loadToolsForExecution: jest.fn().mockResolvedValue([]), + isExpectedMCPToolsUnavailableError: (error) => + error?.code === 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', })); const mockGetMultiplier = jest.fn().mockReturnValue(1); @@ -393,6 +395,31 @@ describe('OpenAIChatCompletionController', () => { expect.objectContaining({ agentResourceType: ResourceType.REMOTE_AGENT }), ); }); + + it('returns 503 when an agent expects MCP tools but resolves none', async () => { + const { initializeAgent } = require('@librechat/api'); + const { loadAgentTools } = require('~/server/services/ToolService'); + const toolError = Object.assign(new Error('Expected MCP tools are unavailable'), { + code: 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', + status: 503, + statusCode: 503, + }); + loadAgentTools.mockRejectedValueOnce(toolError); + initializeAgent.mockImplementationOnce(async ({ req, res, loadTools, agent }) => { + await loadTools({ + req, + res, + tools: ['run_query_mcp_warehouse'], + model: agent.model, + agentId: agent.id, + provider: agent.provider, + }); + }); + + await OpenAIChatCompletionController(req, res); + + expect(res.status).toHaveBeenCalledWith(503); + }); }); describe('execution envelope', () => { diff --git a/api/server/controllers/agents/__tests__/responses.unit.spec.js b/api/server/controllers/agents/__tests__/responses.unit.spec.js index d54cc0cfb0..621dce6799 100644 --- a/api/server/controllers/agents/__tests__/responses.unit.spec.js +++ b/api/server/controllers/agents/__tests__/responses.unit.spec.js @@ -211,6 +211,8 @@ jest.mock('@librechat/api', () => ({ jest.mock('~/server/services/ToolService', () => ({ loadAgentTools: jest.fn().mockResolvedValue([]), loadToolsForExecution: jest.fn().mockResolvedValue([]), + isExpectedMCPToolsUnavailableError: (error) => + error?.code === 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', })); const mockGetMultiplier = jest.fn().mockReturnValue(1); @@ -343,6 +345,36 @@ describe('createResponse controller', () => { }; }); + it('returns 503 when an agent expects MCP tools but resolves none', async () => { + const { initializeAgent, sendResponsesErrorResponse } = require('@librechat/api'); + const { loadAgentTools } = require('~/server/services/ToolService'); + const toolError = Object.assign(new Error('Expected MCP tools are unavailable'), { + code: 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', + status: 503, + statusCode: 503, + }); + loadAgentTools.mockRejectedValueOnce(toolError); + initializeAgent.mockImplementationOnce(async ({ req, res, loadTools, agent }) => { + await loadTools({ + req, + res, + tools: ['run_query_mcp_warehouse'], + model: agent.model, + agentId: agent.id, + provider: agent.provider, + }); + }); + + await createResponse(req, res); + + expect(sendResponsesErrorResponse).toHaveBeenCalledWith( + res, + 503, + 'Expected MCP tools are unavailable', + 'server_error', + ); + }); + describe('execution envelope', () => { it('creates the portable run input before agent initialization', async () => { req.user = { diff --git a/api/server/controllers/agents/openai.js b/api/server/controllers/agents/openai.js index e014331acf..897e389a88 100644 --- a/api/server/controllers/agents/openai.js +++ b/api/server/controllers/agents/openai.js @@ -49,6 +49,7 @@ const { loadAgentTools, loadToolsForExecution, getAccessibleMcpServerNames, + isExpectedMCPToolsUnavailableError, } = require('~/server/services/ToolService'); const { findAccessibleResources, @@ -103,6 +104,9 @@ function createToolLoader(signal, definitionsOnly = true) { }); } catch (error) { logger.error('Error loading tools for agent ' + agentId, error); + if (isExpectedMCPToolsUnavailableError(error)) { + throw error; + } } }; } diff --git a/api/server/controllers/agents/responses.js b/api/server/controllers/agents/responses.js index 7429359cd6..759166fad8 100644 --- a/api/server/controllers/agents/responses.js +++ b/api/server/controllers/agents/responses.js @@ -57,7 +57,11 @@ const { createToolEndCallback, agentLogHandlerObj, } = require('~/server/controllers/agents/callbacks'); -const { loadAgentTools, loadToolsForExecution } = require('~/server/services/ToolService'); +const { + loadAgentTools, + loadToolsForExecution, + isExpectedMCPToolsUnavailableError, +} = require('~/server/services/ToolService'); const { findAccessibleResources, getEffectivePermissions, @@ -113,6 +117,9 @@ function createToolLoader(signal, definitionsOnly = true) { }); } catch (error) { logger.error('Error loading tools for agent ' + agentId, error); + if (isExpectedMCPToolsUnavailableError(error)) { + throw error; + } } }; } diff --git a/api/server/services/Endpoints/agents/addedConvo.js b/api/server/services/Endpoints/agents/addedConvo.js index d57c656028..eed0df0a84 100644 --- a/api/server/services/Endpoints/agents/addedConvo.js +++ b/api/server/services/Endpoints/agents/addedConvo.js @@ -11,6 +11,7 @@ const { isEphemeralAgentId } = require('librechat-data-provider'); const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions'); const { getMCPServerTools } = require('~/server/services/Config'); const { getAccessibleMcpServerNames } = require('~/server/services/MCP'); +const { isExpectedMCPToolsUnavailableError } = require('~/server/services/ToolService'); const { getSkillDbMethods, canAuthorSkillFiles } = require('./skillDeps'); const db = require('~/models'); @@ -226,6 +227,9 @@ const processAddedConvo = async ({ return { userMCPAuthMap }; } catch (err) { + if (isExpectedMCPToolsUnavailableError(err)) { + throw err; + } logger.error('[processAddedConvo] Error processing addedConvo for parallel agent', err); return { userMCPAuthMap }; } diff --git a/api/server/services/Endpoints/agents/addedConvo.spec.js b/api/server/services/Endpoints/agents/addedConvo.spec.js index de637a4011..3033901e0b 100644 --- a/api/server/services/Endpoints/agents/addedConvo.spec.js +++ b/api/server/services/Endpoints/agents/addedConvo.spec.js @@ -41,6 +41,11 @@ jest.mock('~/server/services/MCP', () => ({ getAccessibleMcpServerNames: jest.fn(async () => []), })); +jest.mock('~/server/services/ToolService', () => ({ + isExpectedMCPToolsUnavailableError: (error) => + error?.code === 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', +})); + jest.mock('./skillDeps', () => ({ canAuthorSkillFiles: (...args) => mockCanAuthorSkillFiles(...args), getSkillDbMethods: () => mockGetSkillDbMethods(), @@ -138,6 +143,16 @@ describe('processAddedConvo', () => { ); }); + it('propagates an expected-MCP-tools failure from an added parallel agent', async () => { + const toolError = Object.assign(new Error('Added agent expected MCP tools are unavailable'), { + code: 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', + statusCode: 503, + }); + mockInitializeAgent.mockRejectedValueOnce(toolError); + + await expect(processAddedConvo(baseParams())).rejects.toBe(toolError); + }); + it('keeps deployment-aware skill metadata on a persisted added-agent config', async () => { const deploymentSkillId = { toString: () => 'deployment-skill' }; const agentConfigs = new Map(); diff --git a/api/server/services/Endpoints/agents/initialize.js b/api/server/services/Endpoints/agents/initialize.js index 20f1a78b2c..a9ad6a8103 100644 --- a/api/server/services/Endpoints/agents/initialize.js +++ b/api/server/services/Endpoints/agents/initialize.js @@ -40,6 +40,7 @@ const { loadAgentTools, loadToolsForExecution, getAccessibleMcpServerNames, + isExpectedMCPToolsUnavailableError, } = require('~/server/services/ToolService'); const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions'); const { @@ -109,6 +110,9 @@ function createToolLoader(signal, streamId = null, definitionsOnly = false, jobC }); } catch (error) { logger.error('Error loading tools for agent ' + agentId, error); + if (isExpectedMCPToolsUnavailableError(error)) { + throw error; + } } }; } @@ -800,6 +804,9 @@ const initializeClient = async ({ agentToolContexts.set(agentId, buildAgentToolContext({ agent, config })); return config; } catch (err) { + if (isExpectedMCPToolsUnavailableError(err)) { + throw err; + } logger.error(`[processAgent] Error processing subagent ${agentId}:`, err); skippedAgentIds.add(agentId); return null; diff --git a/api/server/services/Endpoints/agents/initialize.spec.js b/api/server/services/Endpoints/agents/initialize.spec.js index 871a7ecc3d..59c4b654b0 100644 --- a/api/server/services/Endpoints/agents/initialize.spec.js +++ b/api/server/services/Endpoints/agents/initialize.spec.js @@ -58,6 +58,8 @@ const mockLoadToolsForExecution = jest.fn(); jest.mock('~/server/services/ToolService', () => ({ loadAgentTools: jest.fn(), loadToolsForExecution: (...args) => mockLoadToolsForExecution(...args), + isExpectedMCPToolsUnavailableError: (error) => + error?.code === 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', })); jest.mock('~/server/controllers/ModelController', () => ({ @@ -82,6 +84,7 @@ jest.mock('~/cache', () => ({ const { initializeClient } = require('./initialize'); const { getSkillDbMethods, getSkillToolDeps } = require('./skillDeps'); +const { loadAgentTools } = require('~/server/services/ToolService'); const { getModelsConfig } = require('~/server/controllers/ModelController'); const { logger } = require('@librechat/data-schemas'); const { User, AclEntry } = require('~/db/models'); @@ -199,6 +202,83 @@ describe('initializeClient — processAgent ACL gate', () => { ); }); + it('propagates an expected-MCP-tools failure from the runtime tool loader', async () => { + const toolError = Object.assign(new Error('Expected MCP tools are unavailable'), { + code: 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', + statusCode: 503, + }); + loadAgentTools.mockRejectedValueOnce(toolError); + mockInitializeAgent.mockImplementationOnce(async ({ req, res, loadTools, agent }) => { + await loadTools({ + req, + res, + tools: ['run_query_mcp_warehouse'], + model: agent.model, + agentId: agent.id, + provider: agent.provider, + }); + return makePrimaryConfig([]); + }); + + await expect( + initializeClient({ + req: makeReq(), + res: {}, + signal: new AbortController().signal, + endpointOption: makeEndpointOption(), + }), + ).rejects.toBe(toolError); + }); + + it('aborts the run when a handoff target resolves none of its expected MCP tools', async () => { + const target = await createAgent({ + id: AUTHORIZED_ID, + name: 'Target Agent', + provider: 'openai', + model: 'gpt-4', + author: new mongoose.Types.ObjectId(), + tools: ['run_query_mcp_warehouse'], + }); + await AclEntry.create({ + principalType: PrincipalType.USER, + principalId: testUser._id, + principalModel: PrincipalModel.USER, + resourceType: ResourceType.AGENT, + resourceId: target._id, + permBits: PermissionBits.VIEW, + grantedBy: testUser._id, + }); + + const toolError = Object.assign(new Error('Target Agent expected MCP tools are unavailable'), { + code: 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', + statusCode: 503, + }); + loadAgentTools.mockRejectedValueOnce(toolError); + mockInitializeAgent + .mockResolvedValueOnce( + makePrimaryConfig([{ from: PRIMARY_ID, to: AUTHORIZED_ID, edgeType: 'handoff' }]), + ) + .mockImplementationOnce(async ({ req, res, loadTools, agent }) => { + await loadTools({ + req, + res, + tools: agent.tools, + model: agent.model, + agentId: agent.id, + provider: agent.provider, + }); + }); + + await expect( + initializeClient({ + req: makeReq(), + res: {}, + signal: new AbortController().signal, + endpointOption: makeEndpointOption(), + }), + ).rejects.toBe(toolError); + }); + it('should skip handoff agent and filter its edge when user lacks VIEW access', async () => { await createAgent({ id: TARGET_ID, @@ -588,6 +668,49 @@ describe('initializeClient — subagent loading', () => { return agent; }; + it('aborts the run when a pure subagent resolves none of its expected MCP tools', async () => { + const subAgent = await createAgent({ + id: SUBAGENT_ID, + name: 'Data Subagent', + provider: 'openai', + model: 'gpt-4', + author: new mongoose.Types.ObjectId(), + tools: ['run_query_mcp_warehouse'], + }); + await grantView(subAgent); + + const toolError = Object.assign(new Error('Subagent expected MCP tools are unavailable'), { + code: 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', + statusCode: 503, + }); + loadAgentTools.mockRejectedValueOnce(toolError); + mockInitializeAgent + .mockResolvedValueOnce( + makePrimaryConfig({ + subagents: { enabled: true, allowSelf: true, agent_ids: [SUBAGENT_ID] }, + }), + ) + .mockImplementationOnce(async ({ req, res, loadTools, agent }) => { + await loadTools({ + req, + res, + tools: agent.tools, + model: agent.model, + agentId: agent.id, + provider: agent.provider, + }); + }); + + await expect( + initializeClient({ + req: makeSubagentReq(), + res: {}, + signal: new AbortController().signal, + endpointOption: makeEndpointOption(), + }), + ).rejects.toBe(toolError); + }); + it('loads a configured subagent, populates `subagentAgentConfigs`, and keeps it out of `agentConfigs`', async () => { const subAgent = await createAgent({ id: SUBAGENT_ID, diff --git a/api/server/services/ToolService.js b/api/server/services/ToolService.js index 46b003c631..b29d172c99 100644 --- a/api/server/services/ToolService.js +++ b/api/server/services/ToolService.js @@ -524,6 +524,29 @@ const nativeTools = new Set([ Tools.memory, ]); +const mcpServerPinPrefix = `${Constants.mcp_server}${Constants.mcp_delimiter}`; +const isExpectedMCPTool = (toolName) => + toolName?.includes(Constants.mcp_delimiter) && + !toolName.startsWith(mcpServerPinPrefix) && + !isActionTool(toolName); +const expectedMCPToolsUnavailableCode = 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE'; +const isExpectedMCPToolsUnavailableError = (error) => + error?.code === expectedMCPToolsUnavailableCode; +const createExpectedMCPToolsUnavailableError = (agentName, cause) => { + const subject = agentName ? `Agent "${agentName}"` : 'The agent'; + const error = new Error( + `${subject} is configured to use MCP tools, but none are available. Verify that the MCP server is connected and this agent can access its selected tools, then try again.`, + ); + error.name = 'AgentToolInitializationError'; + error.code = expectedMCPToolsUnavailableCode; + error.status = 503; + error.statusCode = 503; + if (cause != null) { + error.cause = cause; + } + return error; +}; + /** Checks if a tool name is a known built-in tool */ const isBuiltInTool = (toolName) => Boolean( @@ -573,6 +596,7 @@ async function loadToolDefinitionsWrapper({ } const appConfig = req.config; + const hasExpectedMCPTools = agent.tools.some(isExpectedMCPTool); const enabledCapabilities = await resolveAgentCapabilities(req, appConfig, agent.id); const checkCapability = (capability) => enabledCapabilities.has(capability); @@ -616,6 +640,9 @@ async function loadToolDefinitionsWrapper({ }); if (!filteredTools || filteredTools.length === 0) { + if (hasExpectedMCPTools) { + throw createExpectedMCPToolsUnavailableError(agent.name); + } return { toolDefinitions: [] }; } @@ -893,6 +920,33 @@ async function loadToolDefinitionsWrapper({ return result?.availableTools || null; }; + const refreshMCPServerTools = async (_userId, serverName) => { + if (pendingOAuthServers.has(serverName)) { + return null; + } + + const oauthStart = async (authURL, options) => { + pendingOAuthServers.add(serverName); + if (typeof authURL === 'string' && authURL.length > 0) { + pendingOAuthStarts.set(serverName, { authURL, options }); + } + }; + const result = await reinitMCPServer({ + user: req.user, + forceNew: true, + oauthStart, + flowManager, + serverName, + configServers, + userMCPAuthMap, + requestBody: req.body, + requestScopedConnections, + }); + + rememberMCPAvailableTools(serverName, result?.availableTools); + return result?.availableTools || null; + }; + const getActionToolDefinitions = async (agentId, actionToolNames) => { const actionSets = (await loadActionSets({ agent_id: agentId })) ?? []; if (actionSets.length === 0) { @@ -952,26 +1006,28 @@ async function loadToolDefinitionsWrapper({ return definitions; }; - let { toolDefinitions, toolRegistry, hasDeferredTools } = await loadToolDefinitions( - { - userId: req.user.id, - agentId: agent.id, - tools: defsFilteredTools, - toolOptions: agent.tool_options, - deferredToolsEnabled, - programmaticToolsEnabled, - codeExecutionEnabled, - provider: agent.provider, - mcpServerNames, - rawServerNames: mcpRawServerNames, - accessibleServerNames: defsAccessibleServerNames, - }, - { - isBuiltInTool, - getOrFetchMCPServerTools, - getActionToolDefinitions, - }, - ); + let { toolDefinitions, toolRegistry, hasDeferredTools, mcpResolution } = + await loadToolDefinitions( + { + userId: req.user.id, + agentId: agent.id, + tools: defsFilteredTools, + toolOptions: agent.tool_options, + deferredToolsEnabled, + programmaticToolsEnabled, + codeExecutionEnabled, + provider: agent.provider, + mcpServerNames, + rawServerNames: mcpRawServerNames, + accessibleServerNames: defsAccessibleServerNames, + }, + { + isBuiltInTool, + getOrFetchMCPServerTools, + refreshMCPServerTools, + getActionToolDefinitions, + }, + ); /** OAuth discovery must not reconnect (or prompt for) a server whose * definitions the collision filter deliberately rejected. */ @@ -1056,15 +1112,21 @@ async function loadToolDefinitionsWrapper({ { isBuiltInTool, getOrFetchMCPServerTools, + refreshMCPServerTools, getActionToolDefinitions, }, ); toolDefinitions = reloadResult.toolDefinitions; toolRegistry = reloadResult.toolRegistry; hasDeferredTools = reloadResult.hasDeferredTools; + mcpResolution = reloadResult.mcpResolution; } } + if (hasExpectedMCPTools && mcpResolution?.resolvedToolCount === 0) { + throw createExpectedMCPToolsUnavailableError(agent.name); + } + /** @type {Record} */ const toolContextMap = {}; /** @type {Record} */ @@ -1195,16 +1257,23 @@ async function loadAgentTools({ accessibleMcpServerNames, }) { if (definitionsOnly) { - return loadToolDefinitionsWrapper({ - req, - res, - agent, - agentResourceType, - streamId, - jobCreatedAt, - tool_resources, - accessibleMcpServerNames, - }); + try { + return await loadToolDefinitionsWrapper({ + req, + res, + agent, + agentResourceType, + streamId, + jobCreatedAt, + tool_resources, + accessibleMcpServerNames, + }); + } catch (error) { + if (isExpectedMCPToolsUnavailableError(error) || !agent.tools?.some(isExpectedMCPTool)) { + throw error; + } + throw createExpectedMCPToolsUnavailableError(agent.name, error); + } } if (!agent.tools || agent.tools.length === 0) { @@ -1971,6 +2040,7 @@ module.exports = { loadToolsForExecution, processRequiredActions, resolveAgentCapabilities, + isExpectedMCPToolsUnavailableError, /** Re-exported for controllers that already depend on (and mock) this * module, avoiding a fresh heavy `services/MCP` require chain there. */ getAccessibleMcpServerNames, diff --git a/api/server/services/Tools/mcp.js b/api/server/services/Tools/mcp.js index de75c8437c..5475a99173 100644 --- a/api/server/services/Tools/mcp.js +++ b/api/server/services/Tools/mcp.js @@ -255,7 +255,7 @@ async function reinitMCPServer({ tools = await connection.fetchTools(); } - if (tools && tools.length > 0) { + if (tools) { availableTools = await updateMCPServerTools({ userId: user.id, serverName, diff --git a/api/server/services/Tools/mcp.spec.js b/api/server/services/Tools/mcp.spec.js index 8e69ba9a78..7b20cb0d38 100644 --- a/api/server/services/Tools/mcp.spec.js +++ b/api/server/services/Tools/mcp.spec.js @@ -102,6 +102,24 @@ describe('reinitMCPServer — customUserVars gating (issue #10969)', () => { ); }); + it('updates the cache with an empty catalog after a successful connection', async () => { + mockGetConnection.mockResolvedValue({ fetchTools: jest.fn().mockResolvedValue([]) }); + + await reinitMCPServer({ + user, + serverName, + serverConfig: { type: 'streamable-http', url: 'https://thingy.example.com/mcp' }, + userMCPAuthMap: undefined, + }); + + expect(mockUpdateMCPServerTools).toHaveBeenCalledWith({ + userId: user.id, + serverName, + tools: [], + serverConfig: { type: 'streamable-http', url: 'https://thingy.example.com/mcp' }, + }); + }); + it('passes request body and Graph resolver into connection creation', async () => { mockGetConnection.mockResolvedValue({ fetchTools: jest.fn().mockResolvedValue([]) }); const requestBody = { conversationId: 'conv-123', messageId: 'msg-123' }; diff --git a/api/server/services/__tests__/ToolService.spec.js b/api/server/services/__tests__/ToolService.spec.js index d859680587..850a18b4e2 100644 --- a/api/server/services/__tests__/ToolService.spec.js +++ b/api/server/services/__tests__/ToolService.spec.js @@ -359,6 +359,113 @@ describe('ToolService - Action Capability Gating', () => { expect(callArgs.tools).toContain(regularTool); }); + it('fails initialization when an explicitly selected MCP tool cannot be resolved', async () => { + const mcpTool = `search${Constants.mcp_delimiter}warehouse`; + const capabilities = [AgentCapabilities.tools]; + const req = createMockReq(capabilities); + mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities)); + mockResolveConfigServers.mockResolvedValue({ + warehouse: { + type: 'streamable-http', + url: 'https://mcp.example.com/warehouse', + }, + }); + mockLoadToolDefinitions.mockResolvedValueOnce({ + toolDefinitions: [], + toolRegistry: new Map(), + hasDeferredTools: false, + mcpResolution: { expectedToolCount: 1, resolvedToolCount: 0 }, + }); + + await expect( + loadAgentTools({ + req, + res: {}, + agent: { id: 'agent_123', name: 'Target Agent', tools: [mcpTool] }, + definitionsOnly: true, + }), + ).rejects.toMatchObject({ + code: 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', + statusCode: 503, + message: expect.stringContaining('can access its selected tools'), + }); + }); + + it('fails closed when MCP definition loading throws before resolution completes', async () => { + const capabilities = [AgentCapabilities.tools]; + const req = createMockReq(capabilities); + mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities)); + mockLoadToolDefinitions.mockRejectedValueOnce(new Error('MCP registry unavailable')); + + await expect( + loadAgentTools({ + req, + res: {}, + agent: { + id: 'agent_123', + name: 'Target Agent', + tools: [`run_query${Constants.mcp_delimiter}warehouse`], + }, + definitionsOnly: true, + }), + ).rejects.toMatchObject({ + code: 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', + statusCode: 503, + cause: expect.objectContaining({ message: 'MCP registry unavailable' }), + }); + }); + + it('allows a server pin with no explicitly selected MCP tools', async () => { + const serverPin = `${Constants.mcp_server}${Constants.mcp_delimiter}warehouse`; + const capabilities = [AgentCapabilities.tools]; + const req = createMockReq(capabilities); + mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities)); + mockLoadToolDefinitions.mockResolvedValueOnce({ + toolDefinitions: [], + toolRegistry: new Map(), + hasDeferredTools: false, + mcpResolution: { expectedToolCount: 0, resolvedToolCount: 0 }, + }); + + await expect( + loadAgentTools({ + req, + res: {}, + agent: { id: 'agent_123', tools: [serverPin] }, + definitionsOnly: true, + }), + ).resolves.toMatchObject({ toolDefinitions: [] }); + }); + + it('allows partial MCP resolution when at least one expected tool is available', async () => { + const capabilities = [AgentCapabilities.tools]; + const req = createMockReq(capabilities); + mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities)); + mockLoadToolDefinitions.mockResolvedValueOnce({ + toolDefinitions: [{ name: 'list_sources_mcp_warehouse', toolType: 'mcp' }], + toolRegistry: new Map(), + hasDeferredTools: false, + mcpResolution: { expectedToolCount: 2, resolvedToolCount: 1 }, + }); + + await expect( + loadAgentTools({ + req, + res: {}, + agent: { + id: 'agent_123', + tools: [ + `list_sources${Constants.mcp_delimiter}warehouse`, + `run_query${Constants.mcp_delimiter}warehouse`, + ], + }, + definitionsOnly: true, + }), + ).resolves.toMatchObject({ + toolDefinitions: [expect.objectContaining({ name: 'list_sources_mcp_warehouse' })], + }); + }); + it('should filter MCP tool definitions when user lacks MCP server use permission', async () => { const { userCanUseMCPServers } = require('~/server/services/MCP'); userCanUseMCPServers.mockResolvedValueOnce(false); @@ -381,6 +488,31 @@ describe('ToolService - Action Capability Gating', () => { expect(callArgs.tools).not.toContain(mcpTool); }); + it('fails explicitly when MCP permission filtering removes every expected tool', async () => { + const { userCanUseMCPServers } = require('~/server/services/MCP'); + userCanUseMCPServers.mockResolvedValueOnce(false); + + const capabilities = [AgentCapabilities.tools]; + const req = createMockReq(capabilities); + mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities)); + + await expect( + loadAgentTools({ + req, + res: {}, + agent: { + id: 'agent_123', + tools: [`run_query${Constants.mcp_delimiter}warehouse`], + }, + definitionsOnly: true, + }), + ).rejects.toMatchObject({ + code: 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', + statusCode: 503, + }); + expect(mockLoadToolDefinitions).not.toHaveBeenCalled(); + }); + it('should return actionsEnabled in the result', async () => { const capabilities = [AgentCapabilities.tools]; const req = createMockReq(capabilities); diff --git a/e2e/setup/fake-model.js b/e2e/setup/fake-model.js index ba65529786..baade45c28 100644 --- a/e2e/setup/fake-model.js +++ b/e2e/setup/fake-model.js @@ -1340,6 +1340,25 @@ function parseHandoffScript(text) { error: `script.routes[${index}].targetTools must be an array of non-empty strings`, }; } + if (route.targetToolCall != null) { + const targetToolCall = route.targetToolCall; + if ( + typeof targetToolCall !== 'object' || + Array.isArray(targetToolCall) || + typeof targetToolCall.id !== 'string' || + targetToolCall.id === '' || + typeof targetToolCall.name !== 'string' || + targetToolCall.name === '' || + typeof targetToolCall.args !== 'object' || + targetToolCall.args == null || + Array.isArray(targetToolCall.args) || + typeof targetToolCall.outputIncludes !== 'string' + ) { + return { + error: `script.routes[${index}].targetToolCall must contain an id, name, args object, and outputIncludes`, + }; + } + } const args = route.args ?? {}; let inferredReceipt = null; @@ -1358,6 +1377,7 @@ function parseHandoffScript(text) { receipt: route.receipt ?? inferredReceipt, targetInstructions: route.targetInstructions, targetTools: route.targetTools ?? [], + targetToolCall: route.targetToolCall, }); } @@ -1784,6 +1804,36 @@ function buildHandoffResponses(graph, parsed) { receptionFailures.join('; '), }; } + + const targetToolCall = incomingRoute.targetToolCall; + if (targetToolCall) { + const toolResult = findToolMessage(messages, targetToolCall.id); + if (!toolResult) { + return { + response: '', + toolCalls: [ + { + id: targetToolCall.id, + name: targetToolCall.name, + args: targetToolCall.args, + type: 'tool_call', + }, + ], + }; + } + + const output = getContentText(toolResult.content); + if (!output.includes(targetToolCall.outputIncludes)) { + return { + response: + `E2E handoff target tool failed ${script.label}: agent=${agentId}; ` + + `expected=${targetToolCall.outputIncludes}; received=${output || '(empty)'}`, + }; + } + return { + response: `E2E handoff tool complete ${script.label}: agent=${agentId}`, + }; + } } const outgoingRoutes = script.routes.filter((route) => route.from === agentId); diff --git a/e2e/specs/mock/agent-handoffs.spec.ts b/e2e/specs/mock/agent-handoffs.spec.ts index efa419e18c..f007319706 100644 --- a/e2e/specs/mock/agent-handoffs.spec.ts +++ b/e2e/specs/mock/agent-handoffs.spec.ts @@ -19,6 +19,7 @@ const HANDOFF_PROMPT = 'Pass the specialist the exact request and relevant const const HANDOFF_PROMPT_KEY = 'context'; const MCP_SERVER_TOOL_ID = 'sys__server__sys_mcp_e2e-memory'; const MCP_TOOL_ID = 'remember_fact_mcp_e2e-memory'; +const MISSING_MCP_TOOL_ID = 'retired_fact_mcp_e2e-memory'; const MCP_SERVER_NAME = 'e2e-memory'; type HandoffRoute = { @@ -31,6 +32,12 @@ type HandoffRoute = { receipt?: string; targetInstructions?: string; targetTools?: string[]; + targetToolCall?: { + id: string; + name: string; + args: Record; + outputIncludes: string; + }; }; type MCPToolsResponse = { @@ -799,6 +806,159 @@ test.describe('agent handoffs', () => { } }); + test('invokes the target-scoped MCP tool after transfer and persists its output', async ({ + page, + }) => { + test.setTimeout(180000); + + await page.goto('/c/new', { timeout: 10000 }); + const token = await getAccessToken(page); + const targetName = uniqueAgentName('E2E Tool Handoff Target'); + const routerName = uniqueAgentName('E2E Tool Handoff Router'); + const label = `target-tool-${Date.now()}`; + const fact = `delegated fact ${label}`; + const toolCallId = `call_e2e_handoff_target_tool_${label}`; + const toolOutput = `E2E MCP memory noted: ${fact}`; + let targetId: string | undefined; + let routerId: string | undefined; + + try { + await waitForMCPTool(page, token); + const target = await createAgentViaApi(page, token, targetName, undefined, { + tools: [MCP_SERVER_TOOL_ID, MCP_TOOL_ID], + }); + targetId = target.id; + const router = await createAgentViaApi(page, token, routerName, [ + { + from: '', + to: target.id, + edgeType: 'handoff', + description: 'Delegate requests that require the target memory tool.', + }, + ]); + routerId = router.id; + + await selectAgentForChat(page, routerName); + const response = await sendMessage( + page, + handoffMarker(label, [ + { + from: router.id, + to: target.id, + description: 'Delegate requests that require the target memory tool.', + args: {}, + targetTools: [MCP_TOOL_ID], + targetToolCall: { + id: toolCallId, + name: MCP_TOOL_ID, + args: { fact }, + outputIncludes: toolOutput, + }, + }, + ]), + ); + expect(response.ok()).toBeTruthy(); + + const view = messagesView(page); + await expect(view.getByRole('button', { name: `Transferred to ${targetName}` })).toBeVisible({ + timeout: 30000, + }); + const toolCall = view.locator(`[data-testid="tool-call"][data-tool-call-id="${toolCallId}"]`); + await expect(toolCall).toBeVisible({ timeout: 30000 }); + const toolToggle = toolCall.getByRole('button', { name: /remember_fact/ }); + if ((await toolToggle.getAttribute('aria-expanded')) !== 'true') { + await toolToggle.click(); + } + await expect( + view.locator(`[data-tool-call-output-id="${toolCallId}"]`).getByText(toolOutput, { + exact: true, + }), + ).toBeVisible({ timeout: 30000 }); + const finalText = `E2E handoff tool complete ${label}: agent=${target.id}`; + await expect(view.getByText(finalText, { exact: true })).toBeVisible({ timeout: 30000 }); + + await expect(page).toHaveURL(/\/c\/(?!new)/, { timeout: 15000 }); + await page.reload({ waitUntil: 'domcontentloaded' }); + await expect( + messagesView(page).locator(`[data-testid="tool-call"][data-tool-call-id="${toolCallId}"]`), + ).toBeVisible({ timeout: 30000 }); + await expect(messagesView(page).getByText(finalText, { exact: true })).toBeVisible({ + timeout: 30000, + }); + } finally { + await cleanupAgent(page, routerId); + await cleanupAgent(page, targetId); + } + }); + + test('publishes a terminal error before model execution when the handoff target expects an unavailable MCP tool', async ({ + page, + }) => { + test.setTimeout(180000); + + await page.goto('/c/new', { timeout: 10000 }); + const token = await getAccessToken(page); + const targetName = uniqueAgentName('E2E Unavailable Tool Target'); + const routerName = uniqueAgentName('E2E Unavailable Tool Router'); + const label = `unavailable-target-tool-${Date.now()}`; + let targetId: string | undefined; + let routerId: string | undefined; + + try { + await waitForMCPTool(page, token); + const target = await createAgentViaApi(page, token, targetName, undefined, { + tools: [MISSING_MCP_TOOL_ID], + }); + targetId = target.id; + const router = await createAgentViaApi(page, token, routerName, [ + { + from: '', + to: target.id, + edgeType: 'handoff', + description: 'Delegate requests that require the unavailable target tool.', + }, + ]); + routerId = router.id; + + await selectAgentForChat(page, routerName); + const input = page.getByRole('textbox', { name: 'Message input' }); + await input.fill( + handoffMarker(label, [ + { + from: router.id, + to: target.id, + description: 'Delegate requests that require the unavailable target tool.', + args: {}, + }, + ]), + ); + const [response] = await Promise.all([ + page.waitForResponse((candidate) => { + const { pathname } = new URL(candidate.url()); + return ( + candidate.request().method() === 'POST' && + (pathname === '/api/agents/chat' || pathname.startsWith('/api/agents/chat/')) && + !pathname.endsWith('/abort') + ); + }), + input.press('Enter'), + ]); + + expect(response.status()).toBe(200); + await expect( + messagesView(page).getByText( + /is configured to use MCP tools, but none are available\. Verify that the MCP server is connected and this agent can access its selected tools, then try again\./, + ), + ).toBeVisible({ timeout: 30000 }); + await expect( + messagesView(page).getByText(new RegExp(`E2E handoff (continuing|complete) ${label}`)), + ).toHaveCount(0); + } finally { + await cleanupAgent(page, routerId); + await cleanupAgent(page, targetId); + } + }); + test('executes a transitive router-to-specialist-to-reviewer handoff', async ({ page }) => { test.setTimeout(180000); diff --git a/e2e/specs/real/agent-handoff-mcp.spec.ts b/e2e/specs/real/agent-handoff-mcp.spec.ts new file mode 100644 index 0000000000..bd8f75eab5 --- /dev/null +++ b/e2e/specs/real/agent-handoff-mcp.spec.ts @@ -0,0 +1,168 @@ +import { expect, test } from '@playwright/test'; +import type { Page } from '@playwright/test'; +import type { AgentDetail } from '../mock/agents.helpers'; +import { cleanupAgent, openAgentBuilder, uniqueAgentName } from '../mock/agents.helpers'; +import { fetchJson, getAccessToken, requestJson, sendMessage } from '../mock/helpers'; + +/** + * LOCAL-ONLY real-provider verification for agent-scoped MCP tools after a + * handoff. The deterministic suites cover failure semantics; this test proves + * that a real model can transfer to a target and invoke the target's MCP tool. + */ + +const REAL_MODEL = process.env.E2E_REAL_ANTHROPIC_MODEL ?? 'claude-haiku-4-5'; +const MCP_SERVER_NAME = 'e2e-memory'; +const MCP_SERVER_TOOL_ID = `sys__server__sys_mcp_${MCP_SERVER_NAME}`; +const REMEMBER_TOOL_ID = `remember_fact_mcp_${MCP_SERVER_NAME}`; + +type MCPToolsResponse = { + servers?: Record }>; +}; + +type ToolCallRecord = { + name?: string; + args?: unknown; +}; + +type MessageRecord = { + content?: Array<{ type?: string; tool_call?: ToolCallRecord }>; +}; + +async function waitForRememberTool(page: Page) { + const token = await getAccessToken(page); + for (let attempt = 0; attempt < 20; attempt++) { + const tools = await fetchJson(page, '/api/mcp/tools', token); + const serverTools = tools.servers?.[MCP_SERVER_NAME]?.tools ?? []; + if (serverTools.some((tool) => tool.pluginKey === REMEMBER_TOOL_ID)) { + return token; + } + await new Promise((resolve) => setTimeout(resolve, 500)); + } + throw new Error(`Expected ${MCP_SERVER_NAME} to expose ${REMEMBER_TOOL_ID}`); +} + +async function selectAgentForChat(page: Page, agentName: string) { + const form = await openAgentBuilder(page); + await form.getByRole('combobox', { name: 'Agent', exact: true }).click(); + await page.getByRole('option', { name: agentName }).click(); + await expect(form.getByLabel('Agent name')).toHaveValue(agentName); + await form.getByRole('button', { name: 'Select Agent' }).click(); + await expect(page.getByRole('textbox', { name: 'Message input' })).toBeVisible(); +} + +async function readToolCalls(page: Page, conversationId: string): Promise { + const token = await getAccessToken(page); + const messages = await fetchJson(page, `/api/messages/${conversationId}`, token); + return (messages ?? []).flatMap((message) => + (message.content ?? []) + .filter((part) => part.type === 'tool_call' && part.tool_call) + .map((part) => part.tool_call as ToolCallRecord), + ); +} + +function parseArgs(args: unknown): Record | undefined { + if (args != null && typeof args === 'object' && !Array.isArray(args)) { + return args as Record; + } + if (typeof args !== 'string') { + return undefined; + } + try { + const parsed: unknown = JSON.parse(args); + return parsed != null && typeof parsed === 'object' && !Array.isArray(parsed) + ? (parsed as Record) + : undefined; + } catch { + return undefined; + } +} + +test.describe('agent handoff with MCP tools (real provider)', () => { + test('the target invokes its own MCP tool after transfer', async ({ page }) => { + test.setTimeout(180000); + const targetName = uniqueAgentName('Real Handoff Target'); + const primaryName = uniqueAgentName('Real Handoff Primary'); + let targetId: string | undefined; + let primaryId: string | undefined; + + try { + await page.goto('/c/new'); + const token = await waitForRememberTool(page); + + const target = await requestJson(page, { + path: '/api/agents', + token, + method: 'POST', + body: { + name: targetName, + description: 'Handles delegated memory requests.', + instructions: + 'For every request, call remember_fact exactly once with the requested fact before ' + + 'replying. Never claim the fact was stored without calling the tool.', + provider: 'anthropic', + model: REAL_MODEL, + tools: [MCP_SERVER_TOOL_ID, REMEMBER_TOOL_ID], + tool_options: { [REMEMBER_TOOL_ID]: { describe_intent: true } }, + }, + }); + targetId = target.id; + + const primary = await requestJson(page, { + path: '/api/agents', + token, + method: 'POST', + body: { + name: primaryName, + description: 'Routes every request to the target agent.', + instructions: + 'Immediately transfer every user request to the configured target agent. Do not ' + + 'answer the request yourself.', + provider: 'anthropic', + model: REAL_MODEL, + edges: [ + { + from: '', + to: target.id, + edgeType: 'handoff', + description: 'Use this handoff for every user request.', + }, + ], + }, + }); + primaryId = primary.id; + + await selectAgentForChat(page, primaryName); + const response = await sendMessage( + page, + 'Delegate this request and store the fact: the target retained its MCP tool after handoff.', + ); + expect(response.ok()).toBeTruthy(); + await expect(page).toHaveURL(/\/c\/(?!new)/, { timeout: 60000 }); + await expect(page.getByRole('button', { name: `Transferred to ${targetName}` })).toBeVisible({ + timeout: 120000, + }); + + const conversationId = new URL(page.url()).pathname.split('/c/')[1]; + let rememberCall: ToolCallRecord | undefined; + await expect + .poll( + async () => { + const calls = await readToolCalls(page, conversationId); + rememberCall = calls.find((call) => call.name?.startsWith('remember_fact')); + return rememberCall != null; + }, + { timeout: 120000, intervals: [2000] }, + ) + .toBe(true); + + const args = parseArgs(rememberCall?.args); + expect(args).toBeTruthy(); + expect(Object.keys(args as Record)[0]).toBe('intent'); + expect(typeof args?.intent).toBe('string'); + expect((args?.intent as string).trim().length).toBeGreaterThan(0); + } finally { + await cleanupAgent(page, primaryId); + await cleanupAgent(page, targetId); + } + }); +}); diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts index 70a9012478..fdfe163646 100644 --- a/packages/api/src/agents/discovery.spec.ts +++ b/packages/api/src/agents/discovery.spec.ts @@ -1,8 +1,8 @@ import { EModelEndpoint } from 'librechat-data-provider'; import type { Agent, GraphEdge } from 'librechat-data-provider'; import type { Response } from 'express'; -import type { ServerRequest } from '~/types'; import type { InitializedAgent } from './initialize'; +import type { ServerRequest } from '~/types'; jest.mock('@librechat/data-schemas', () => ({ logger: { @@ -1038,6 +1038,37 @@ describe('discoverConnectedAgents', () => { expect(result.agentConfigs.has('B')).toBe(false); }); + it('propagates an expected-MCP-tools failure from a handoff target', async () => { + const toolError = Object.assign(new Error('Target Agent has no available MCP tools'), { + code: 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', + statusCode: 503, + }); + mockInitializeAgent.mockRejectedValueOnce(toolError); + + const primaryConfig = makeConfig('A', [{ from: 'A', to: 'B', edgeType: 'handoff' }]); + const getAgent = jest.fn(async () => makeAgent('B', [])); + const checkPermission = jest.fn().mockResolvedValue(true); + + await expect( + discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ), + ).rejects.toBe(toolError); + }); + it('skips when request has no authenticated user', async () => { const primaryConfig = makeConfig('A', [{ from: 'A', to: 'B', edgeType: 'handoff' }]); diff --git a/packages/api/src/agents/discovery.ts b/packages/api/src/agents/discovery.ts index 8359d7b45a..f093373d8b 100644 --- a/packages/api/src/agents/discovery.ts +++ b/packages/api/src/agents/discovery.ts @@ -14,6 +14,17 @@ import { initializeAgent as defaultInitializeAgent } from './initialize'; import { createEdgeCollector, filterOrphanedEdges } from './edges'; import { createSequentialChainEdges } from './chain'; +const expectedMCPToolsUnavailableCode = 'AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE'; + +function isExpectedMCPToolsUnavailableError(error: unknown): boolean { + return ( + typeof error === 'object' && + error !== null && + 'code' in error && + error.code === expectedMCPToolsUnavailableCode + ); +} + /** * Callback invoked after a sub-agent is successfully initialized. * Used by callers that need to track per-agent tool context (e.g., for @@ -327,6 +338,9 @@ export async function discoverConnectedAgents( collectEdges(agent.edges); } } catch (err) { + if (isExpectedMCPToolsUnavailableError(err)) { + throw err; + } logger.error(`[discoverConnectedAgents] Error processing agent ${agentId}:`, err); markSkipped(agentId); } @@ -341,6 +355,9 @@ export async function discoverConnectedAgents( try { await processAgent(agentId); } catch (err) { + if (isExpectedMCPToolsUnavailableError(err)) { + throw err; + } logger.error(`[discoverConnectedAgents] Error processing chain agent ${agentId}:`, err); markSkipped(agentId); } diff --git a/packages/api/src/agents/initialize.ts b/packages/api/src/agents/initialize.ts index 8ea74b0301..e1f8c7eb22 100644 --- a/packages/api/src/agents/initialize.ts +++ b/packages/api/src/agents/initialize.ts @@ -991,10 +991,11 @@ export async function initializeAgent( * `loadTools` failures take two forms: * 1. The wrapper throws — rare; only when something around the * try/catch in `createToolLoader` itself fails. - * 2. The wrapper returns `undefined` — the typical CJS path: every - * production loader (`createToolLoader` in `initialize.js`, - * `openai.js`, `responses.js`) catches `loadAgentTools` errors and - * returns `undefined`. Without explicit handling, the empty + * 2. The wrapper returns `undefined` — the typical CJS path for errors + * that remain soft-failures. Runtime loaders rethrow invariant + * failures such as an explicitly configured MCP tool set resolving + * to zero, but preserve the legacy `undefined` result for unrelated + * failures. Without explicit handling, the empty * fallback object below would silently drop the agent's baseline * tools for the turn (not just the skill-added extras). * @@ -1034,7 +1035,7 @@ export async function initializeAgent( } } if (initialFailedSilently(loadToolsResult)) { - /* Production loaders swallow errors and return undefined. Treat that + /* Runtime loaders may swallow non-invariant errors and return undefined. Treat that the same as a throw when extras were requested — the agent's own tools must still load. */ logger.warn( diff --git a/packages/api/src/mcp/tools.spec.ts b/packages/api/src/mcp/tools.spec.ts index fac1859262..5e036f483a 100644 --- a/packages/api/src/mcp/tools.spec.ts +++ b/packages/api/src/mcp/tools.spec.ts @@ -36,14 +36,14 @@ describe('createMCPToolCacheService', () => { expect(deps.setCachedTools).not.toHaveBeenCalled(); }); - it('returns empty object for empty tools array', async () => { + it('replaces a stale cache entry when the server returns an empty tools array', async () => { const deps = createMockDeps(); const { updateMCPServerTools } = createMCPToolCacheService(deps); const result = await updateMCPServerTools({ userId: 'u1', serverName: 'srv', tools: [] }); expect(result).toEqual({}); - expect(deps.setCachedTools).not.toHaveBeenCalled(); + expect(deps.setCachedTools).toHaveBeenCalledWith({}, { userId: 'u1', serverName: 'srv' }); }); it('builds MODEL-FACING keys with the normalized server name, store keyed raw', async () => { @@ -326,6 +326,18 @@ describe('createMCPToolCacheService', () => { expect(deps.getCachedTools).toHaveBeenCalledWith({ userId: 'u1', serverName: 'brave' }); }); + it('treats a cached empty catalog as a miss so discovery remains enabled', async () => { + const deps = createMockDeps({ + getCachedTools: jest.fn().mockResolvedValue({}), + getServerConfig: jest.fn().mockResolvedValue(cacheableConfig), + }); + const { getMCPServerTools } = createMCPToolCacheService(deps); + + const result = await getMCPServerTools('u1', 'brave'); + + expect(result).toBeNull(); + }); + it('heals stale raw-keyed cache entries to the normalized key format at read time', async () => { /** Entries written before keys embedded the normalized server name would * otherwise make the server's tools vanish for up to the cache TTL — diff --git a/packages/api/src/mcp/tools.ts b/packages/api/src/mcp/tools.ts index 10f779df44..0cb3255789 100644 --- a/packages/api/src/mcp/tools.ts +++ b/packages/api/src/mcp/tools.ts @@ -83,11 +83,21 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS const serverTools: LCAvailableTools = {}; const mcpDelimiter = Constants.mcp_delimiter; - if (tools == null || tools.length === 0) { + if (tools == null) { logger.debug(`[MCP Cache] No tools to update for server ${serverName} (user: ${userId})`); return serverTools; } + if (tools.length === 0) { + if (!(await isRequestScoped(userId, serverName, serverConfig))) { + await setCachedTools(serverTools, { userId, serverName }); + logger.debug( + `[MCP Cache] Cleared stale tools for server ${serverName} (user: ${userId})`, + ); + } + return serverTools; + } + /** Cache keys are MODEL-FACING: they become builder tool ids, agent.tools * entries, tool_options keys, and definition names, and must equal the * runtime instance name (`createToolInstance` in MCP.js), which embeds @@ -218,6 +228,9 @@ export function createMCPToolCacheService(deps: MCPToolCacheDeps): MCPToolCacheS } try { const cached = (await getCachedTools({ userId, serverName })) ?? null; + if (!cached || Object.keys(cached).length === 0) { + return null; + } return normalizeCachedToolKeys(cached, serverName); } catch (error) { logger.error(`[getMCPServerTools] Error fetching cached tools for ${serverName}:`, error); diff --git a/packages/api/src/tools/definitions.spec.ts b/packages/api/src/tools/definitions.spec.ts index 8366d86447..5df9b16d98 100644 --- a/packages/api/src/tools/definitions.spec.ts +++ b/packages/api/src/tools/definitions.spec.ts @@ -385,6 +385,138 @@ describe('definitions.ts', () => { }); describe('MCP tool definitions with server name variants', () => { + it('treats a server pin with no selected tools as intentionally empty', async () => { + const result = await loadToolDefinitions( + { + userId: 'user-123', + agentId: 'agent-123', + tools: ['sys__server__sys_mcp_warehouse'], + }, + { + getOrFetchMCPServerTools: mockGetOrFetchMCPServerTools, + isBuiltInTool: mockIsBuiltInTool, + }, + ); + + expect(mockGetOrFetchMCPServerTools).not.toHaveBeenCalled(); + expect(result.toolDefinitions).toEqual([]); + expect(result.mcpResolution).toEqual({ expectedToolCount: 0, resolvedToolCount: 0 }); + }); + + it('refreshes a non-empty server catalog once when the selected tool is missing', async () => { + const selectedTool = 'run_query_mcp_warehouse'; + const staleTools = { + list_sources_mcp_warehouse: { + function: { + name: 'list_sources_mcp_warehouse', + description: 'List databases', + parameters: { type: 'object', properties: {} }, + }, + }, + }; + const refreshedTools = { + ...staleTools, + [selectedTool]: { + function: { + name: selectedTool, + description: 'Run a read-only query', + parameters: { type: 'object', properties: {} }, + }, + }, + }; + const refreshMCPServerTools = jest.fn().mockResolvedValue(refreshedTools); + mockGetOrFetchMCPServerTools.mockResolvedValue(staleTools); + + const result = await loadToolDefinitions( + { + userId: 'user-123', + agentId: 'agent-123', + tools: [selectedTool], + }, + { + getOrFetchMCPServerTools: mockGetOrFetchMCPServerTools, + refreshMCPServerTools, + isBuiltInTool: mockIsBuiltInTool, + }, + ); + + expect(refreshMCPServerTools).toHaveBeenCalledTimes(1); + expect(refreshMCPServerTools).toHaveBeenCalledWith('user-123', 'warehouse'); + expect(result.toolDefinitions).toEqual([ + expect.objectContaining({ name: selectedTool, serverName: 'warehouse' }), + ]); + expect(result.mcpResolution).toEqual({ expectedToolCount: 1, resolvedToolCount: 1 }); + }); + + it('refreshes an empty server catalog once when a selected tool is expected', async () => { + const selectedTool = 'run_query_mcp_warehouse'; + const refreshedTools = { + [selectedTool]: { + function: { + name: selectedTool, + description: 'Run a read-only query', + parameters: { type: 'object', properties: {} }, + }, + }, + }; + const refreshMCPServerTools = jest.fn().mockResolvedValue(refreshedTools); + mockGetOrFetchMCPServerTools.mockResolvedValue({}); + + const result = await loadToolDefinitions( + { + userId: 'user-123', + agentId: 'agent-123', + tools: [selectedTool], + }, + { + getOrFetchMCPServerTools: mockGetOrFetchMCPServerTools, + refreshMCPServerTools, + isBuiltInTool: mockIsBuiltInTool, + }, + ); + + expect(refreshMCPServerTools).toHaveBeenCalledTimes(1); + expect(result.toolDefinitions).toEqual([ + expect.objectContaining({ name: selectedTool, serverName: 'warehouse' }), + ]); + expect(result.mcpResolution).toEqual({ expectedToolCount: 1, resolvedToolCount: 1 }); + }); + + it('refreshes an empty server catalog once when all server tools are expected', async () => { + const wildcardTool = 'sys__all__sys_mcp_warehouse'; + const refreshedTool = 'run_query_mcp_warehouse'; + const refreshedTools = { + [refreshedTool]: { + function: { + name: refreshedTool, + description: 'Run a read-only query', + parameters: { type: 'object', properties: {} }, + }, + }, + }; + const refreshMCPServerTools = jest.fn().mockResolvedValue(refreshedTools); + mockGetOrFetchMCPServerTools.mockResolvedValue({}); + + const result = await loadToolDefinitions( + { + userId: 'user-123', + agentId: 'agent-123', + tools: [wildcardTool], + }, + { + getOrFetchMCPServerTools: mockGetOrFetchMCPServerTools, + refreshMCPServerTools, + isBuiltInTool: mockIsBuiltInTool, + }, + ); + + expect(refreshMCPServerTools).toHaveBeenCalledTimes(1); + expect(result.toolDefinitions).toEqual([ + expect.objectContaining({ name: refreshedTool, serverName: 'warehouse' }), + ]); + expect(result.mcpResolution).toEqual({ expectedToolCount: 1, resolvedToolCount: 1 }); + }); + it('should load MCP tools with underscored server names (server_one)', async () => { const mockServerTools = { list_items_mcp_server_one: { diff --git a/packages/api/src/tools/definitions.ts b/packages/api/src/tools/definitions.ts index f77be71ead..8904bf9fcb 100644 --- a/packages/api/src/tools/definitions.ts +++ b/packages/api/src/tools/definitions.ts @@ -6,7 +6,12 @@ */ import { Providers } from '@librechat/agents'; -import { isActionTool, splitMCPToolKey, buildServerNameAliases } from 'librechat-data-provider'; +import { + Constants, + isActionTool, + splitMCPToolKey, + 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'; @@ -72,6 +77,8 @@ export interface ActionToolDefinition { export interface LoadToolDefinitionsDeps { /** Gets MCP server tools - first checks cache, then initializes server if needed */ getOrFetchMCPServerTools: (userId: string, serverName: string) => Promise; + /** Bypasses a non-empty stale catalog when it does not contain a selected tool. */ + refreshMCPServerTools?: (userId: string, serverName: string) => Promise; /** Checks if a tool name is a known built-in tool */ isBuiltInTool: (toolName: string) => boolean; /** Loads action tool definitions (schemas) from OpenAPI specs */ @@ -85,9 +92,14 @@ export interface LoadToolDefinitionsResult { toolDefinitions: (ToolDefinition | LCTool)[]; toolRegistry: LCToolRegistry; hasDeferredTools: boolean; + mcpResolution: { + expectedToolCount: number; + resolvedToolCount: number; + }; } const mcpToolPattern = /_mcp_/; +const mcpServerPinPrefix = `${Constants.mcp_server}${Constants.mcp_delimiter}`; /** * Loads tool definitions without creating tool instances. @@ -110,7 +122,12 @@ export async function loadToolDefinitions( rawServerNames, accessibleServerNames, } = params; - const { getOrFetchMCPServerTools, isBuiltInTool, getActionToolDefinitions } = deps; + const { + getOrFetchMCPServerTools, + refreshMCPServerTools, + isBuiltInTool, + getActionToolDefinitions, + } = deps; const serverNameAliases = buildServerNameAliases(rawServerNames ?? []); const isGoogle = provider === Providers.GOOGLE || provider === Providers.VERTEXAI; @@ -128,6 +145,7 @@ export async function loadToolDefinitions( toolDefinitions: [], toolRegistry: new Map(), hasDeferredTools: false, + mcpResolution: { expectedToolCount: 0, resolvedToolCount: 0 }, }; if (!tools || tools.length === 0) { @@ -135,12 +153,15 @@ export async function loadToolDefinitions( } const mcpServerToolsCache = new Map(); + const refreshedServerNames = new Set(); /** Parsed key segment → the RAW server name it resolved to (direct-first). */ const resolvedServerNames = new Map(); const mcpToolDefs: ToolDefinition[] = []; const builtInToolDefs: ToolDefinition[] = []; let actionToolDefs: ToolDefinition[] = []; const actionToolNames: string[] = []; + let expectedMCPToolCount = 0; + let resolvedMCPToolCount = 0; for (const toolName of tools) { if (isActionTool(toolName)) { @@ -178,6 +199,12 @@ export async function loadToolDefinitions( continue; } + if (toolName.startsWith(mcpServerPinPrefix)) { + continue; + } + + expectedMCPToolCount++; + /** Keys carry the normalized server name (raw in pre-normalization data), * so both spellings resolve the boundary. Resolution is DIRECT-FIRST: a * server that resolves under the parsed name as-is wins (a user-DB @@ -215,11 +242,23 @@ export async function loadToolDefinitions( } const serverName = resolvedServerNames.get(parsed) ?? parsed; - const serverTools = mcpServerToolsCache.get(parsed); + let serverTools = mcpServerToolsCache.get(parsed); if (!serverTools) { continue; } + const selectedToolMissing = isMCPAllPlaceholder(toolName) + ? Object.keys(serverTools).length === 0 + : !serverTools[toolName]?.function; + if (selectedToolMissing && refreshMCPServerTools && !refreshedServerNames.has(serverName)) { + refreshedServerNames.add(serverName); + const refreshedTools = await refreshMCPServerTools(userId, serverName); + if (refreshedTools != null) { + mcpServerToolsCache.set(parsed, refreshedTools); + serverTools = refreshedTools; + } + } + if (isMCPAllPlaceholder(toolName)) { for (const [actualToolName, toolDef] of Object.entries(serverTools)) { if (toolDef?.function) { @@ -229,6 +268,7 @@ export async function loadToolDefinitions( parameters: buildMcpParameters(toolDef.function.parameters), serverName, }); + resolvedMCPToolCount++; } } continue; @@ -242,6 +282,7 @@ export async function loadToolDefinitions( parameters: buildMcpParameters(toolDef.function.parameters), serverName, }); + resolvedMCPToolCount++; } } @@ -309,5 +350,9 @@ export async function loadToolDefinitions( toolDefinitions: allDefinitions, toolRegistry, hasDeferredTools, + mcpResolution: { + expectedToolCount: expectedMCPToolCount, + resolvedToolCount: resolvedMCPToolCount, + }, }; }