From 15012d7a60f94ac3b8ed274bb9725f1004ab1c0e Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 21 Sep 2025 07:33:56 -0400 Subject: [PATCH] ci: Remove MCP tests from PluginController test file and add comprehensive tests for MCP tools controller --- .../controllers/PluginController.spec.js | 136 +---- api/server/controllers/mcp.spec.js | 561 ++++++++++++++++++ 2 files changed, 579 insertions(+), 118 deletions(-) create mode 100644 api/server/controllers/mcp.spec.js diff --git a/api/server/controllers/PluginController.spec.js b/api/server/controllers/PluginController.spec.js index f7a991490..d7d3f83a8 100644 --- a/api/server/controllers/PluginController.spec.js +++ b/api/server/controllers/PluginController.spec.js @@ -1,4 +1,3 @@ -const { Constants } = require('librechat-data-provider'); const { getCachedTools, getAppConfig } = require('~/server/services/Config'); const { getLogStores } = require('~/cache'); @@ -17,18 +16,10 @@ jest.mock('~/server/services/Config', () => ({ includedTools: [], }), setCachedTools: jest.fn(), - mergeUserTools: jest.fn(), })); // loadAndFormatTools mock removed - no longer used in PluginController - -jest.mock('~/config', () => ({ - getMCPManager: jest.fn(() => ({ - getAllToolFunctions: jest.fn().mockResolvedValue({}), - getRawConfig: jest.fn().mockReturnValue({}), - })), - getFlowStateManager: jest.fn(), -})); +// getMCPManager mock removed - no longer used in PluginController jest.mock('~/app/clients/tools', () => ({ availableTools: [], @@ -183,9 +174,6 @@ describe('PluginController', () => { paths: { structuredTools: '/mock/path' }, }; - // Mock second call to return tool definitions - getCachedTools.mockResolvedValueOnce(mockUserTools); - await getAvailableTools(mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(200); @@ -208,14 +196,7 @@ describe('PluginController', () => { require('~/app/clients/tools').availableTools.push(mockPlugin); mockCache.get.mockResolvedValue(null); - // First call returns null for user tools - getCachedTools.mockResolvedValueOnce(null); - mockReq.config = { - mcpConfig: null, - paths: { structuredTools: '/mock/path' }, - }; - - // Second call (with includeGlobal: true) returns the tool definitions + // getCachedTools returns the tool definitions getCachedTools.mockResolvedValueOnce({ tool1: { type: 'function', @@ -226,6 +207,10 @@ describe('PluginController', () => { }, }, }); + mockReq.config = { + mcpConfig: null, + paths: { structuredTools: '/mock/path' }, + }; await getAvailableTools(mockReq, mockRes); @@ -256,14 +241,7 @@ describe('PluginController', () => { }); mockCache.get.mockResolvedValue(null); - // First call returns null for user tools - getCachedTools.mockResolvedValueOnce(null); - mockReq.config = { - mcpConfig: null, - paths: { structuredTools: '/mock/path' }, - }; - - // Second call (with includeGlobal: true) returns the tool definitions + // getCachedTools returns the tool definitions getCachedTools.mockResolvedValueOnce({ toolkit1_function: { type: 'function', @@ -274,6 +252,10 @@ describe('PluginController', () => { }, }, }); + mockReq.config = { + mcpConfig: null, + paths: { structuredTools: '/mock/path' }, + }; await getAvailableTools(mockReq, mockRes); @@ -285,63 +267,6 @@ describe('PluginController', () => { }); }); - describe('plugin.icon behavior', () => { - const callGetAvailableToolsWithMCPServer = async (serverConfig) => { - mockCache.get.mockResolvedValue(null); - - const functionTools = { - [`test-tool${Constants.mcp_delimiter}test-server`]: { - type: 'function', - function: { - name: `test-tool${Constants.mcp_delimiter}test-server`, - description: 'A test tool', - parameters: { type: 'object', properties: {} }, - }, - }, - }; - - // Mock the MCP manager to return tools and server config - const mockMCPManager = { - getAllToolFunctions: jest.fn().mockResolvedValue(functionTools), - getRawConfig: jest.fn().mockReturnValue(serverConfig), - }; - require('~/config').getMCPManager.mockReturnValue(mockMCPManager); - - // First call returns empty user tools - getCachedTools.mockResolvedValueOnce({}); - - // Mock getAppConfig to return the mcpConfig - mockReq.config = { - mcpConfig: { - 'test-server': serverConfig, - }, - }; - - // Second call (with includeGlobal: true) returns the tool definitions - getCachedTools.mockResolvedValueOnce(functionTools); - - await getAvailableTools(mockReq, mockRes); - const responseData = mockRes.json.mock.calls[0][0]; - return responseData.find( - (tool) => tool.pluginKey === `test-tool${Constants.mcp_delimiter}test-server`, - ); - }; - - it('should set plugin.icon when iconPath is defined', async () => { - const serverConfig = { - iconPath: '/path/to/icon.png', - }; - const testTool = await callGetAvailableToolsWithMCPServer(serverConfig); - expect(testTool.icon).toBe('/path/to/icon.png'); - }); - - it('should set plugin.icon to undefined when iconPath is not defined', async () => { - const serverConfig = {}; - const testTool = await callGetAvailableToolsWithMCPServer(serverConfig); - expect(testTool.icon).toBeUndefined(); - }); - }); - describe('helper function integration', () => { it('should handle error cases gracefully', async () => { mockCache.get.mockRejectedValue(new Error('Cache error')); @@ -364,23 +289,13 @@ describe('PluginController', () => { it('should handle null cachedTools and cachedUserTools', async () => { mockCache.get.mockResolvedValue(null); - // First call returns null for user tools - getCachedTools.mockResolvedValueOnce(null); + // getCachedTools returns empty object instead of null + getCachedTools.mockResolvedValueOnce({}); mockReq.config = { mcpConfig: null, paths: { structuredTools: '/mock/path' }, }; - // Mock MCP manager to return no tools - const mockMCPManager = { - getAllToolFunctions: jest.fn().mockResolvedValue({}), - getRawConfig: jest.fn().mockReturnValue({}), - }; - require('~/config').getMCPManager.mockReturnValue(mockMCPManager); - - // Second call (with includeGlobal: true) returns empty object instead of null - getCachedTools.mockResolvedValueOnce({}); - await getAvailableTools(mockReq, mockRes); // Should handle null values gracefully @@ -395,9 +310,9 @@ describe('PluginController', () => { paths: { structuredTools: '/mock/path' }, }; - // Mock getCachedTools to return undefined for both calls + // Mock getCachedTools to return undefined getCachedTools.mockReset(); - getCachedTools.mockResolvedValueOnce(undefined).mockResolvedValueOnce(undefined); + getCachedTools.mockResolvedValueOnce(undefined); await getAvailableTools(mockReq, mockRes); @@ -416,13 +331,6 @@ describe('PluginController', () => { // Ensure no plugins are available require('~/app/clients/tools').availableTools.length = 0; - // Reset MCP manager to default state - const mockMCPManager = { - getAllToolFunctions: jest.fn().mockResolvedValue({}), - getRawConfig: jest.fn().mockReturnValue({}), - }; - require('~/config').getMCPManager.mockReturnValue(mockMCPManager); - await getAvailableTools(mockReq, mockRes); // With empty tool definitions, no tools should be in the final output @@ -457,16 +365,13 @@ describe('PluginController', () => { require('~/app/clients/tools').availableTools.push(mockToolkit); mockCache.get.mockResolvedValue(null); - // First call returns empty object + // getCachedTools returns empty object to avoid null reference error getCachedTools.mockResolvedValueOnce({}); mockReq.config = { mcpConfig: null, paths: { structuredTools: '/mock/path' }, }; - // Second call (with includeGlobal: true) returns empty object to avoid null reference error - getCachedTools.mockResolvedValueOnce({}); - await getAvailableTools(mockReq, mockRes); // Should handle null toolDefinitions gracefully @@ -487,15 +392,12 @@ describe('PluginController', () => { mockCache.get.mockResolvedValue(null); - // First call returns null for user tools - getCachedTools.mockResolvedValueOnce(null); - mockReq.config = { mcpConfig: null, paths: { structuredTools: '/mock/path' }, }; - // CRITICAL: Second call (with includeGlobal: true) returns undefined + // CRITICAL: getCachedTools returns undefined // This is what causes the bug when trying to access toolDefinitions[plugin.pluginKey] getCachedTools.mockResolvedValueOnce(undefined); @@ -534,9 +436,8 @@ describe('PluginController', () => { { name: 'Tool 2', pluginKey: 'tool2', description: 'Tool 2' }, ); - // First call: Simulate cache cleared state (returns null for both global and user tools) + // Simulate cache cleared state (returns null) mockCache.get.mockResolvedValue(null); - getCachedTools.mockResolvedValueOnce(null); // User tools getCachedTools.mockResolvedValueOnce(null); // Global tools (cache cleared) mockReq.config = { @@ -574,7 +475,6 @@ describe('PluginController', () => { // Cache returns null (cleared state) mockCache.get.mockResolvedValue(null); - getCachedTools.mockResolvedValueOnce(null); // User tools getCachedTools.mockResolvedValueOnce(null); // Global tools (cache cleared) mockReq.config = { diff --git a/api/server/controllers/mcp.spec.js b/api/server/controllers/mcp.spec.js new file mode 100644 index 000000000..6dea3ac34 --- /dev/null +++ b/api/server/controllers/mcp.spec.js @@ -0,0 +1,561 @@ +const { getMCPTools } = require('./mcp'); +const { getAppConfig, getMCPServerTools } = require('~/server/services/Config'); +const { getMCPManager } = require('~/config'); +const { convertMCPToolToPlugin } = require('@librechat/api'); + +jest.mock('@librechat/data-schemas', () => ({ + logger: { + debug: jest.fn(), + error: jest.fn(), + warn: jest.fn(), + }, +})); + +jest.mock('librechat-data-provider', () => ({ + Constants: { + mcp_delimiter: '~~~', + }, +})); + +jest.mock('@librechat/api', () => ({ + convertMCPToolToPlugin: jest.fn(), +})); + +jest.mock('~/server/services/Config', () => ({ + getAppConfig: jest.fn(), + getMCPServerTools: jest.fn(), + cacheMCPServerTools: jest.fn(), +})); + +jest.mock('~/config', () => ({ + getMCPManager: jest.fn(), +})); + +describe('MCP Controller', () => { + let mockReq, mockRes, mockMCPManager; + + beforeEach(() => { + jest.clearAllMocks(); + + mockReq = { + user: { id: 'test-user-id', role: 'user' }, + config: null, + }; + + mockRes = { + status: jest.fn().mockReturnThis(), + json: jest.fn(), + }; + + mockMCPManager = { + getAllToolFunctions: jest.fn().mockResolvedValue({}), + }; + + getMCPManager.mockReturnValue(mockMCPManager); + getAppConfig.mockResolvedValue({ + mcpConfig: {}, + }); + getMCPServerTools.mockResolvedValue(null); + }); + + describe('getMCPTools', () => { + it('should return 401 when user ID is not found', async () => { + mockReq.user = null; + + await getMCPTools(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(401); + expect(mockRes.json).toHaveBeenCalledWith({ message: 'Unauthorized' }); + const { logger } = require('@librechat/data-schemas'); + expect(logger.warn).toHaveBeenCalledWith('[getMCPTools] User ID not found in request'); + }); + + it('should return empty array when no mcpConfig exists', async () => { + getAppConfig.mockResolvedValue({ + // No mcpConfig + }); + + await getMCPTools(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(mockRes.json).toHaveBeenCalledWith([]); + }); + + it('should use cached server tools when available', async () => { + const cachedTools = { + 'tool1~~~server1': { + type: 'function', + function: { + name: 'tool1', + description: 'Tool 1', + parameters: {}, + }, + }, + }; + + getMCPServerTools.mockResolvedValue(cachedTools); + getAppConfig.mockResolvedValue({ + mcpConfig: { + server1: {}, + }, + }); + + const mockPlugin = { + name: 'Tool 1', + pluginKey: 'tool1~~~server1', + description: 'Tool 1', + }; + convertMCPToolToPlugin.mockReturnValue(mockPlugin); + + await getMCPTools(mockReq, mockRes); + + expect(getMCPServerTools).toHaveBeenCalledWith('server1'); + expect(mockMCPManager.getAllToolFunctions).not.toHaveBeenCalled(); + expect(convertMCPToolToPlugin).toHaveBeenCalledWith({ + toolKey: 'tool1~~~server1', + toolData: cachedTools['tool1~~~server1'], + mcpManager: mockMCPManager, + }); + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(mockRes.json).toHaveBeenCalledWith([ + { + ...mockPlugin, + authConfig: [], + authenticated: true, + }, + ]); + }); + + it('should fetch from MCP manager when cache is empty', async () => { + getMCPServerTools.mockResolvedValue(null); + + const allTools = { + 'tool1~~~server1': { + type: 'function', + function: { + name: 'tool1', + description: 'Tool 1', + parameters: {}, + }, + }, + 'tool2~~~server2': { + type: 'function', + function: { + name: 'tool2', + description: 'Tool 2', + parameters: {}, + }, + }, + }; + + mockMCPManager.getAllToolFunctions.mockResolvedValue(allTools); + + getAppConfig.mockResolvedValue({ + mcpConfig: { + server1: {}, + }, + }); + + const mockPlugin = { + name: 'Tool 1', + pluginKey: 'tool1~~~server1', + description: 'Tool 1', + }; + convertMCPToolToPlugin.mockReturnValue(mockPlugin); + + await getMCPTools(mockReq, mockRes); + + expect(getMCPServerTools).toHaveBeenCalledWith('server1'); + expect(mockMCPManager.getAllToolFunctions).toHaveBeenCalledWith('test-user-id'); + + // Should cache the server tools + const { cacheMCPServerTools } = require('~/server/services/Config'); + expect(cacheMCPServerTools).toHaveBeenCalledWith({ + serverName: 'server1', + serverTools: { + 'tool1~~~server1': allTools['tool1~~~server1'], + }, + }); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(mockRes.json).toHaveBeenCalledWith([ + { + ...mockPlugin, + authConfig: [], + authenticated: true, + }, + ]); + }); + + it('should handle custom user variables in server config', async () => { + getMCPServerTools.mockResolvedValue({ + 'tool1~~~server1': { + type: 'function', + function: { + name: 'tool1', + description: 'Tool 1', + parameters: {}, + }, + }, + }); + + getAppConfig.mockResolvedValue({ + mcpConfig: { + server1: { + customUserVars: { + API_KEY: { + title: 'API Key', + description: 'Your API key', + }, + SECRET: { + title: 'Secret Token', + description: 'Your secret token', + }, + }, + }, + }, + }); + + const mockPlugin = { + name: 'Tool 1', + pluginKey: 'tool1~~~server1', + description: 'Tool 1', + }; + convertMCPToolToPlugin.mockReturnValue(mockPlugin); + + await getMCPTools(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(mockRes.json).toHaveBeenCalledWith([ + { + ...mockPlugin, + authConfig: [ + { + authField: 'API_KEY', + label: 'API Key', + description: 'Your API key', + }, + { + authField: 'SECRET', + label: 'Secret Token', + description: 'Your secret token', + }, + ], + authenticated: false, + }, + ]); + }); + + it('should handle empty custom user variables', async () => { + getMCPServerTools.mockResolvedValue({ + 'tool1~~~server1': { + type: 'function', + function: { + name: 'tool1', + description: 'Tool 1', + parameters: {}, + }, + }, + }); + + getAppConfig.mockResolvedValue({ + mcpConfig: { + server1: { + customUserVars: {}, + }, + }, + }); + + const mockPlugin = { + name: 'Tool 1', + pluginKey: 'tool1~~~server1', + description: 'Tool 1', + }; + convertMCPToolToPlugin.mockReturnValue(mockPlugin); + + await getMCPTools(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(mockRes.json).toHaveBeenCalledWith([ + { + ...mockPlugin, + authConfig: [], + authenticated: true, + }, + ]); + }); + + it('should handle multiple servers', async () => { + getMCPServerTools.mockResolvedValue(null); + + const allTools = { + 'tool1~~~server1': { + type: 'function', + function: { + name: 'tool1', + description: 'Tool 1', + parameters: {}, + }, + }, + 'tool2~~~server2': { + type: 'function', + function: { + name: 'tool2', + description: 'Tool 2', + parameters: {}, + }, + }, + }; + + mockMCPManager.getAllToolFunctions.mockResolvedValue(allTools); + + getAppConfig.mockResolvedValue({ + mcpConfig: { + server1: {}, + server2: {}, + }, + }); + + const mockPlugin1 = { + name: 'Tool 1', + pluginKey: 'tool1~~~server1', + description: 'Tool 1', + }; + const mockPlugin2 = { + name: 'Tool 2', + pluginKey: 'tool2~~~server2', + description: 'Tool 2', + }; + + convertMCPToolToPlugin.mockReturnValueOnce(mockPlugin1).mockReturnValueOnce(mockPlugin2); + + await getMCPTools(mockReq, mockRes); + + expect(getMCPServerTools).toHaveBeenCalledWith('server1'); + expect(getMCPServerTools).toHaveBeenCalledWith('server2'); + expect(mockMCPManager.getAllToolFunctions).toHaveBeenCalledTimes(2); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(mockRes.json).toHaveBeenCalledWith([ + { + ...mockPlugin1, + authConfig: [], + authenticated: true, + }, + { + ...mockPlugin2, + authConfig: [], + authenticated: true, + }, + ]); + }); + + it('should handle server-specific errors gracefully', async () => { + getMCPServerTools.mockResolvedValue(null); + mockMCPManager.getAllToolFunctions.mockRejectedValue(new Error('Server connection failed')); + + getAppConfig.mockResolvedValue({ + mcpConfig: { + server1: {}, + server2: {}, + }, + }); + + await getMCPTools(mockReq, mockRes); + + const { logger } = require('@librechat/data-schemas'); + expect(logger.error).toHaveBeenCalledWith( + '[getMCPTools] Error loading tools for server server1:', + expect.any(Error), + ); + expect(logger.error).toHaveBeenCalledWith( + '[getMCPTools] Error loading tools for server server2:', + expect.any(Error), + ); + + // Should still return 200 with empty array + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(mockRes.json).toHaveBeenCalledWith([]); + }); + + it('should skip tools when convertMCPToolToPlugin returns null', async () => { + getMCPServerTools.mockResolvedValue({ + 'tool1~~~server1': { + type: 'function', + function: { + name: 'tool1', + description: 'Tool 1', + parameters: {}, + }, + }, + 'tool2~~~server1': { + type: 'function', + function: { + name: 'tool2', + description: 'Tool 2', + parameters: {}, + }, + }, + }); + + getAppConfig.mockResolvedValue({ + mcpConfig: { + server1: {}, + }, + }); + + const mockPlugin = { + name: 'Tool 1', + pluginKey: 'tool1~~~server1', + description: 'Tool 1', + }; + + // First tool returns plugin, second returns null + convertMCPToolToPlugin.mockReturnValueOnce(mockPlugin).mockReturnValueOnce(null); + + await getMCPTools(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(mockRes.json).toHaveBeenCalledWith([ + { + ...mockPlugin, + authConfig: [], + authenticated: true, + }, + ]); + }); + + it('should use req.config when available', async () => { + const reqConfig = { + mcpConfig: { + server1: {}, + }, + }; + mockReq.config = reqConfig; + + getMCPServerTools.mockResolvedValue({ + 'tool1~~~server1': { + type: 'function', + function: { + name: 'tool1', + description: 'Tool 1', + parameters: {}, + }, + }, + }); + + const mockPlugin = { + name: 'Tool 1', + pluginKey: 'tool1~~~server1', + description: 'Tool 1', + }; + convertMCPToolToPlugin.mockReturnValue(mockPlugin); + + await getMCPTools(mockReq, mockRes); + + // Should not call getAppConfig when req.config is available + expect(getAppConfig).not.toHaveBeenCalled(); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(mockRes.json).toHaveBeenCalledWith([ + { + ...mockPlugin, + authConfig: [], + authenticated: true, + }, + ]); + }); + + it('should handle general error in getMCPTools', async () => { + const error = new Error('Unexpected error'); + getAppConfig.mockRejectedValue(error); + + await getMCPTools(mockReq, mockRes); + + const { logger } = require('@librechat/data-schemas'); + expect(logger.error).toHaveBeenCalledWith('[getMCPTools]', error); + expect(mockRes.status).toHaveBeenCalledWith(500); + expect(mockRes.json).toHaveBeenCalledWith({ message: 'Unexpected error' }); + }); + + it('should handle custom user variables without title or description', async () => { + getMCPServerTools.mockResolvedValue({ + 'tool1~~~server1': { + type: 'function', + function: { + name: 'tool1', + description: 'Tool 1', + parameters: {}, + }, + }, + }); + + getAppConfig.mockResolvedValue({ + mcpConfig: { + server1: { + customUserVars: { + MY_VAR: { + // No title or description + }, + }, + }, + }, + }); + + const mockPlugin = { + name: 'Tool 1', + pluginKey: 'tool1~~~server1', + description: 'Tool 1', + }; + convertMCPToolToPlugin.mockReturnValue(mockPlugin); + + await getMCPTools(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(mockRes.json).toHaveBeenCalledWith([ + { + ...mockPlugin, + authConfig: [ + { + authField: 'MY_VAR', + label: 'MY_VAR', // Falls back to key + description: '', // Empty string + }, + ], + authenticated: false, + }, + ]); + }); + + it('should not cache when no tools are found for a server', async () => { + getMCPServerTools.mockResolvedValue(null); + + const allTools = { + 'tool1~~~otherserver': { + type: 'function', + function: { + name: 'tool1', + description: 'Tool 1', + parameters: {}, + }, + }, + }; + + mockMCPManager.getAllToolFunctions.mockResolvedValue(allTools); + + getAppConfig.mockResolvedValue({ + mcpConfig: { + server1: {}, + }, + }); + + await getMCPTools(mockReq, mockRes); + + const { cacheMCPServerTools } = require('~/server/services/Config'); + expect(cacheMCPServerTools).not.toHaveBeenCalled(); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(mockRes.json).toHaveBeenCalledWith([]); + }); + }); +});