🔐 fix: MCP OAuth Tool Discovery and Event Emission (#11599)

* fix: MCP OAuth tool discovery and event emission in event-driven mode

- Add discoverServerTools method to MCPManager for tool discovery when OAuth is required
- Fix OAuth event emission to send both ON_RUN_STEP and ON_RUN_STEP_DELTA events
- Fix hasSubscriber flag reset in GenerationJobManager for proper event buffering
- Add ToolDiscoveryOptions and ToolDiscoveryResult types
- Update reinitMCPServer to use new discovery method and propagate OAuth URLs

* refactor: Update ToolService and MCP modules for improved functionality

- Reintroduced Constants in ToolService for better reference management.
- Enhanced loadToolDefinitionsWrapper to handle both response and streamId scenarios.
- Updated MCP module to correct type definitions for oauthStart parameter.
- Improved MCPConnectionFactory to ensure proper disconnection handling during tool discovery.
- Adjusted tests to reflect changes in mock implementations and ensure accurate behavior during OAuth handling.

* fix: Refine OAuth handling in MCPConnectionFactory and related tests

- Updated the OAuth URL assignment logic in reinitMCPServer to prevent overwriting existing URLs.
- Enhanced error logging to provide clearer messages when tool discovery fails.
- Adjusted tests to reflect changes in OAuth handling, ensuring accurate detection of OAuth requirements without generating URLs in discovery mode.

* refactor: Clean up OAuth URL assignment in reinitMCPServer

- Removed redundant OAuth URL assignment logic in the reinitMCPServer function to streamline the tool discovery process.
- Enhanced error logging for tool discovery failures, improving clarity in debugging and monitoring.

* fix: Update response handling in ToolService for event-driven mode

- Changed the condition in loadToolDefinitionsWrapper to check for writableEnded instead of headersSent, ensuring proper event emission when the response is still writable.
- This adjustment enhances the reliability of event handling during tool execution, particularly in streaming scenarios.
This commit is contained in:
Danny Avila 2026-02-01 19:37:04 -05:00 committed by GitHub
parent 5af1342dbb
commit d13037881a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
12 changed files with 667 additions and 40 deletions

View file

@ -1,6 +1,5 @@
import { logger } from '@librechat/data-schemas';
import type { TokenMethods } from '@librechat/data-schemas';
import type { TUser } from 'librechat-data-provider';
import type { TokenMethods, IUser } from '@librechat/data-schemas';
import type { FlowStateManager } from '~/flow/manager';
import type { MCPOAuthTokens } from '~/mcp/oauth';
import type * as t from '~/mcp/types';
@ -27,7 +26,7 @@ const mockMCPConnection = MCPConnection as jest.MockedClass<typeof MCPConnection
const mockMCPOAuthHandler = MCPOAuthHandler as jest.Mocked<typeof MCPOAuthHandler>;
describe('MCPConnectionFactory', () => {
let mockUser: TUser;
let mockUser: IUser | undefined;
let mockServerConfig: t.MCPOptions;
let mockFlowManager: jest.Mocked<FlowStateManager<MCPOAuthTokens | null>>;
let mockConnectionInstance: jest.Mocked<MCPConnection>;
@ -37,7 +36,7 @@ describe('MCPConnectionFactory', () => {
mockUser = {
id: 'user123',
email: 'test@example.com',
} as TUser;
} as IUser;
mockServerConfig = {
command: 'node',
@ -275,7 +274,7 @@ describe('MCPConnectionFactory', () => {
user: mockUser,
};
const oauthOptions = {
const oauthOptions: t.OAuthConnectionOptions = {
user: mockUser,
useOAuth: true,
returnOnOAuth: true,
@ -424,4 +423,116 @@ describe('MCPConnectionFactory', () => {
);
});
});
describe('discoverTools static method', () => {
const mockTools = [
{ name: 'tool1', description: 'First tool', inputSchema: { type: 'object' } },
{ name: 'tool2', description: 'Second tool', inputSchema: { type: 'object' } },
];
it('should discover tools from a successfully connected server', async () => {
const basicOptions = {
serverName: 'test-server',
serverConfig: mockServerConfig,
};
mockConnectionInstance.connect.mockResolvedValue(undefined);
mockConnectionInstance.isConnected.mockResolvedValue(true);
mockConnectionInstance.fetchTools = jest.fn().mockResolvedValue(mockTools);
const result = await MCPConnectionFactory.discoverTools(basicOptions);
expect(result.tools).toEqual(mockTools);
expect(result.oauthRequired).toBe(false);
expect(result.oauthUrl).toBeNull();
expect(result.connection).toBe(mockConnectionInstance);
});
it('should detect OAuth required without generating URL in discovery mode', async () => {
const basicOptions = {
serverName: 'test-server',
serverConfig: {
...mockServerConfig,
url: 'https://api.example.com',
type: 'sse' as const,
} as t.SSEOptions,
};
const mockOAuthStart = jest.fn().mockResolvedValue(undefined);
const oauthOptions = {
useOAuth: true as const,
user: mockUser as unknown as IUser,
flowManager: mockFlowManager,
oauthStart: mockOAuthStart,
tokenMethods: {
findToken: jest.fn(),
createToken: jest.fn(),
updateToken: jest.fn(),
deleteTokens: jest.fn(),
},
};
mockConnectionInstance.isConnected.mockResolvedValue(false);
mockConnectionInstance.disconnect = jest.fn().mockResolvedValue(undefined);
let oauthHandler: (() => Promise<void>) | undefined;
mockConnectionInstance.on.mockImplementation((event, handler) => {
if (event === 'oauthRequired') {
oauthHandler = handler as () => Promise<void>;
}
return mockConnectionInstance;
});
mockConnectionInstance.connect.mockImplementation(async () => {
if (oauthHandler) {
await oauthHandler();
}
throw new Error('OAuth required');
});
const result = await MCPConnectionFactory.discoverTools(basicOptions, oauthOptions);
expect(result.connection).toBeNull();
expect(result.tools).toBeNull();
expect(result.oauthRequired).toBe(true);
expect(result.oauthUrl).toBeNull();
expect(mockOAuthStart).not.toHaveBeenCalled();
});
it('should return null tools when discovery fails completely', async () => {
const basicOptions = {
serverName: 'test-server',
serverConfig: mockServerConfig,
};
mockConnectionInstance.connect.mockRejectedValue(new Error('Connection failed'));
mockConnectionInstance.isConnected.mockResolvedValue(false);
mockConnectionInstance.disconnect = jest.fn().mockResolvedValue(undefined);
const result = await MCPConnectionFactory.discoverTools(basicOptions);
expect(result.tools).toBeNull();
expect(result.connection).toBeNull();
expect(result.oauthRequired).toBe(false);
});
it('should handle disconnect errors gracefully during cleanup', async () => {
const basicOptions = {
serverName: 'test-server',
serverConfig: mockServerConfig,
};
mockConnectionInstance.connect.mockRejectedValue(new Error('Connection failed'));
mockConnectionInstance.isConnected.mockResolvedValue(false);
mockConnectionInstance.disconnect = jest
.fn()
.mockRejectedValue(new Error('Disconnect failed'));
const result = await MCPConnectionFactory.discoverTools(basicOptions);
expect(result.tools).toBeNull();
expect(mockLogger.debug).toHaveBeenCalled();
});
});
});