🔧 fix: Ensure getServerToolFunctions Handles Errors (#9895)

* ensure getServerToolFunctions handles errors

* remove reduntant test
This commit is contained in:
Federico Ruggi 2025-09-30 14:48:39 +02:00 committed by GitHub
parent b8720a9b7a
commit c5d1861acf
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 190 additions and 13 deletions

View file

@ -97,22 +97,30 @@ export class MCPManager extends UserConnectionManager {
userId: string,
serverName: string,
): Promise<t.LCAvailableTools | null> {
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)!);
}
/**

View file

@ -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<typeof logger>;
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<MCPServersRegistry>,
): jest.MockedClass<typeof MCPServersRegistry> {
const mock = {
initialize: jest.fn().mockResolvedValue(undefined),
getToolFunctions: jest.fn().mockResolvedValue(null),
...registryConfig,
};
return (MCPServersRegistry as jest.MockedClass<typeof MCPServersRegistry>).mockImplementation(
() => mock as unknown as MCPServersRegistry,
);
}
function mockAppConnections(
appConnectionsConfig: Partial<ConnectionsRepository>,
): jest.MockedClass<typeof ConnectionsRepository> {
const mock = {
has: jest.fn().mockReturnValue(false),
get: jest.fn().mockResolvedValue({} as unknown as MCPConnection),
...appConnectionsConfig,
};
return (
ConnectionsRepository as jest.MockedClass<typeof ConnectionsRepository>
).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),
);
});
});
});