From c5d1861acf24475bf6c10adf53df2c03b4a7278f Mon Sep 17 00:00:00 2001 From: Federico Ruggi Date: Tue, 30 Sep 2025 14:48:39 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=A7=20fix:=20Ensure=20`getServerToolFu?= =?UTF-8?q?nctions`=20Handles=20Errors=20(#9895)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ensure getServerToolFunctions handles errors * remove reduntant test --- packages/api/src/mcp/MCPManager.ts | 34 ++-- .../api/src/mcp/__tests__/MCPManager.test.ts | 169 ++++++++++++++++++ 2 files changed, 190 insertions(+), 13 deletions(-) create mode 100644 packages/api/src/mcp/__tests__/MCPManager.test.ts diff --git a/packages/api/src/mcp/MCPManager.ts b/packages/api/src/mcp/MCPManager.ts index d3966ff008..c6bfe77b8f 100644 --- a/packages/api/src/mcp/MCPManager.ts +++ b/packages/api/src/mcp/MCPManager.ts @@ -97,22 +97,30 @@ export class MCPManager extends UserConnectionManager { userId: string, serverName: string, ): Promise { - if (this.appConnections?.has(serverName)) { - return this.serversRegistry.getToolFunctions( - serverName, - await this.appConnections.get(serverName), + try { + if (this.appConnections?.has(serverName)) { + return this.serversRegistry.getToolFunctions( + serverName, + await this.appConnections.get(serverName), + ); + } + + const userConnections = this.getUserConnections(userId); + if (!userConnections || userConnections.size === 0) { + return null; + } + if (!userConnections.has(serverName)) { + return null; + } + + return this.serversRegistry.getToolFunctions(serverName, userConnections.get(serverName)!); + } catch (error) { + logger.warn( + `[getServerToolFunctions] Error getting tool functions for server ${serverName}`, + error, ); - } - - const userConnections = this.getUserConnections(userId); - if (!userConnections || userConnections.size === 0) { return null; } - if (!userConnections.has(serverName)) { - return null; - } - - return this.serversRegistry.getToolFunctions(serverName, userConnections.get(serverName)!); } /** diff --git a/packages/api/src/mcp/__tests__/MCPManager.test.ts b/packages/api/src/mcp/__tests__/MCPManager.test.ts new file mode 100644 index 0000000000..4d60a16954 --- /dev/null +++ b/packages/api/src/mcp/__tests__/MCPManager.test.ts @@ -0,0 +1,169 @@ +import { logger } from '@librechat/data-schemas'; +import type * as t from '~/mcp/types'; +import { MCPManager } from '~/mcp/MCPManager'; +import { MCPServersRegistry } from '~/mcp/MCPServersRegistry'; +import { ConnectionsRepository } from '~/mcp/ConnectionsRepository'; +import { MCPConnection } from '../connection'; + +// Mock external dependencies +jest.mock('@librechat/data-schemas', () => ({ + logger: { + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + debug: jest.fn(), + }, +})); + +jest.mock('~/mcp/MCPServersRegistry'); +jest.mock('~/mcp/ConnectionsRepository'); + +const mockLogger = logger as jest.Mocked; + +describe('MCPManager', () => { + const userId = 'test-user-123'; + const serverName = 'test_server'; + + beforeEach(() => { + // Reset MCPManager singleton state + (MCPManager as unknown as { instance: null }).instance = null; + jest.clearAllMocks(); + }); + + function mockRegistry( + registryConfig: Partial, + ): jest.MockedClass { + const mock = { + initialize: jest.fn().mockResolvedValue(undefined), + getToolFunctions: jest.fn().mockResolvedValue(null), + ...registryConfig, + }; + return (MCPServersRegistry as jest.MockedClass).mockImplementation( + () => mock as unknown as MCPServersRegistry, + ); + } + + function mockAppConnections( + appConnectionsConfig: Partial, + ): jest.MockedClass { + const mock = { + has: jest.fn().mockReturnValue(false), + get: jest.fn().mockResolvedValue({} as unknown as MCPConnection), + ...appConnectionsConfig, + }; + return ( + ConnectionsRepository as jest.MockedClass + ).mockImplementation(() => mock as unknown as ConnectionsRepository); + } + + function newMCPServersConfig(serverNameOverride?: string): t.MCPServers { + return { + [serverNameOverride ?? serverName]: { + type: 'stdio', + command: 'test', + args: [], + }, + }; + } + + describe('getServerToolFunctions', () => { + it('should catch and handle errors gracefully', async () => { + mockRegistry({ + getToolFunctions: jest.fn(() => { + throw new Error('Connection failed'); + }), + }); + + mockAppConnections({ + has: jest.fn().mockReturnValue(true), + }); + + const manager = await MCPManager.createInstance(newMCPServersConfig()); + + const result = await manager.getServerToolFunctions(userId, serverName); + + expect(result).toBeNull(); + expect(mockLogger.warn).toHaveBeenCalledWith( + `[getServerToolFunctions] Error getting tool functions for server ${serverName}`, + expect.any(Error), + ); + }); + + it('should catch synchronous errors from getUserConnections', async () => { + mockRegistry({ + getToolFunctions: jest.fn().mockResolvedValue({}), + }); + + mockAppConnections({ + has: jest.fn().mockReturnValue(false), + }); + + const manager = await MCPManager.createInstance(newMCPServersConfig()); + + const spy = jest.spyOn(manager, 'getUserConnections').mockImplementation(() => { + throw new Error('Failed to get user connections'); + }); + + const result = await manager.getServerToolFunctions(userId, serverName); + + expect(result).toBeNull(); + expect(mockLogger.warn).toHaveBeenCalledWith( + `[getServerToolFunctions] Error getting tool functions for server ${serverName}`, + expect.any(Error), + ); + expect(spy).toHaveBeenCalled(); + }); + + it('should return tools successfully when no errors occur', async () => { + const expectedTools: t.LCAvailableTools = { + [`test_tool_mcp_${serverName}`]: { + type: 'function', + function: { + name: `test_tool_mcp_${serverName}`, + description: 'Test tool', + parameters: { type: 'object' }, + }, + }, + }; + + mockRegistry({ + getToolFunctions: jest.fn().mockResolvedValue(expectedTools), + }); + + mockAppConnections({ + has: jest.fn().mockReturnValue(true), + }); + + const manager = await MCPManager.createInstance(newMCPServersConfig()); + + const result = await manager.getServerToolFunctions(userId, serverName); + + expect(result).toEqual(expectedTools); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); + + it('should include specific server name in error messages', async () => { + const specificServerName = 'github_mcp_server'; + + mockRegistry({ + getToolFunctions: jest.fn(() => { + throw new Error('Server specific error'); + }), + }); + + mockAppConnections({ + has: jest.fn().mockReturnValue(true), + }); + + const manager = await MCPManager.createInstance(newMCPServersConfig(specificServerName)); + + const result = await manager.getServerToolFunctions(userId, specificServerName); + + expect(result).toBeNull(); + expect(mockLogger.warn).toHaveBeenCalledWith( + `[getServerToolFunctions] Error getting tool functions for server ${specificServerName}`, + expect.any(Error), + ); + }); + }); +});