From ae33e210d1ae721c5837bd57b83317cf2dddf67b Mon Sep 17 00:00:00 2001 From: TomasPalsson Date: Thu, 18 Jun 2026 12:34:19 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=A2=20fix:=20Paginate=20MCP=20tools/li?= =?UTF-8?q?st=20to=20load=20all=20tools?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MCP `tools/list` is cursor-paginated, but LibreChat only ever read the first page. `MCPConnection.fetchTools()` called `client.listTools()` once and discarded `nextCursor`, and `MCPServerInspector` — which builds the agent-facing tool registry at startup and per request — called the raw `client.listTools()` directly. Servers that paginate (e.g. an aggregating gateway exposing hundreds of tools) only ever exposed page one; tools on later pages were never registered, and invoking one returned "This tool's MCP server is temporarily unavailable." - `MCPConnection.fetchTools()` now follows `nextCursor` across pages and concatenates every page's tools, bounded by a configurable page cap (`MCP_TOOLS_LIST_MAX_PAGES`, default 50) and a repeated-cursor guard so a misbehaving server cannot loop forever. Tools already fetched are returned if a later page fails, and the no-throw error contract is unchanged. - `MCPServerInspector.getToolFunctions()` and `fetchServerCapabilities()` now route through `fetchTools()`, so the canonical startup and per-request tool registry is fully paginated too. --- .../MCPConnectionAgentLifecycle.test.ts | 2 +- ...ectionFactory.oauthSdk.integration.test.ts | 6 +- .../__tests__/MCPConnectionFetchTools.test.ts | 183 ++++++++++++++++++ .../mcp/__tests__/MCPConnectionSSRF.test.ts | 2 +- .../__tests__/dbSourced.integration.test.ts | 2 +- packages/api/src/mcp/connection.ts | 99 +++++----- packages/api/src/mcp/mcpConfig.ts | 5 + .../src/mcp/registry/MCPServerInspector.ts | 6 +- .../MCPReinitRecovery.integration.test.ts | 2 +- .../__tests__/MCPServerInspector.test.ts | 42 ++-- .../__tests__/mcpConnectionsMock.helper.ts | 28 +-- 11 files changed, 286 insertions(+), 91 deletions(-) create mode 100644 packages/api/src/mcp/__tests__/MCPConnectionFetchTools.test.ts diff --git a/packages/api/src/mcp/__tests__/MCPConnectionAgentLifecycle.test.ts b/packages/api/src/mcp/__tests__/MCPConnectionAgentLifecycle.test.ts index 95274f699fc..c9b92aeef69 100644 --- a/packages/api/src/mcp/__tests__/MCPConnectionAgentLifecycle.test.ts +++ b/packages/api/src/mcp/__tests__/MCPConnectionAgentLifecycle.test.ts @@ -45,7 +45,7 @@ jest.mock('~/auth', () => ({ })); jest.mock('~/mcp/mcpConfig', () => ({ - mcpConfig: { CONNECTION_CHECK_TTL: 0 }, + mcpConfig: { CONNECTION_CHECK_TTL: 0, TOOLS_LIST_MAX_PAGES: 50 }, })); const mockLogger = logger as jest.Mocked; diff --git a/packages/api/src/mcp/__tests__/MCPConnectionFactory.oauthSdk.integration.test.ts b/packages/api/src/mcp/__tests__/MCPConnectionFactory.oauthSdk.integration.test.ts index 618bcbf02e3..ea5b3ec788e 100644 --- a/packages/api/src/mcp/__tests__/MCPConnectionFactory.oauthSdk.integration.test.ts +++ b/packages/api/src/mcp/__tests__/MCPConnectionFactory.oauthSdk.integration.test.ts @@ -40,7 +40,11 @@ jest.mock('~/auth', () => ({ })); jest.mock('~/mcp/mcpConfig', () => ({ - mcpConfig: { CONNECTION_CHECK_TTL: 0, USER_CONNECTION_IDLE_TIMEOUT: 30 * 60 * 1000 }, + mcpConfig: { + CONNECTION_CHECK_TTL: 0, + USER_CONNECTION_IDLE_TIMEOUT: 30 * 60 * 1000, + TOOLS_LIST_MAX_PAGES: 50, + }, })); const SERVER_NAME = 'sdk-oauth-server'; diff --git a/packages/api/src/mcp/__tests__/MCPConnectionFetchTools.test.ts b/packages/api/src/mcp/__tests__/MCPConnectionFetchTools.test.ts new file mode 100644 index 00000000000..095c2f36eab --- /dev/null +++ b/packages/api/src/mcp/__tests__/MCPConnectionFetchTools.test.ts @@ -0,0 +1,183 @@ +/** + * Unit tests for MCPConnection.fetchTools pagination. + * + * MCP `tools/list` is a paginated method: the server may return a page of tools + * plus a `nextCursor` that the client must follow to retrieve the rest. These + * tests verify that fetchTools walks every page, passes the cursor back + * unchanged, and is bounded against misbehaving servers (page cap + repeated + * cursor guard) while preserving the original single-page and error behavior. + */ + +import { logger } from '@librechat/data-schemas'; +import { MCPConnection } from '~/mcp/connection'; + +jest.mock('@librechat/data-schemas', () => ({ + logger: { + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + debug: jest.fn(), + }, +})); + +jest.mock('~/auth', () => ({ + createSSRFSafeUndiciConnect: jest.fn(() => undefined), + isOAuthUrlAllowed: jest.fn(() => false), + isSSRFTarget: jest.fn(() => false), + resolveHostnameSSRF: jest.fn(async () => false), +})); + +/** Pin the page cap to a small value so the cap path is cheap to exercise. */ +jest.mock('~/mcp/mcpConfig', () => ({ + mcpConfig: { TOOLS_LIST_MAX_PAGES: 3, CONNECTION_CHECK_TTL: 0 }, +})); + +const mockLogger = logger as jest.Mocked; + +const makeTool = (name: string) => ({ + name, + description: `${name} description`, + inputSchema: { type: 'object' as const, properties: {} }, +}); + +/** Build a bare MCPConnection (no real transport) with an injected, controllable client. */ +function createConnectionWithListTools(listTools: jest.Mock): MCPConnection { + const conn = new MCPConnection({ + serverName: 'pagination-test', + serverConfig: { type: 'streamable-http', url: 'http://localhost/mcp' }, + useSSRFProtection: false, + }); + conn.client = { listTools } as unknown as MCPConnection['client']; + return conn; +} + +describe('MCPConnection.fetchTools pagination', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('returns the tools from a single page and makes one request when there is no nextCursor', async () => { + const listTools = jest.fn().mockResolvedValue({ tools: [makeTool('a'), makeTool('b')] }); + const conn = createConnectionWithListTools(listTools); + + const tools = await conn.fetchTools(); + + expect(tools.map((t) => t.name)).toEqual(['a', 'b']); + expect(listTools).toHaveBeenCalledTimes(1); + expect(listTools).toHaveBeenNthCalledWith(1, undefined); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); + + it('follows nextCursor across pages, concatenating every tool and passing the cursor back', async () => { + const listTools = jest.fn(async (params?: { cursor?: string }) => { + switch (params?.cursor) { + case undefined: + return { tools: [makeTool('a'), makeTool('b')], nextCursor: 'c1' }; + case 'c1': + return { tools: [makeTool('c'), makeTool('d')], nextCursor: 'c2' }; + case 'c2': + return { tools: [makeTool('e')] }; + default: + throw new Error(`unexpected cursor: ${params?.cursor}`); + } + }); + const conn = createConnectionWithListTools(listTools); + + const tools = await conn.fetchTools(); + + expect(tools.map((t) => t.name)).toEqual(['a', 'b', 'c', 'd', 'e']); + expect(listTools).toHaveBeenCalledTimes(3); + expect(listTools).toHaveBeenNthCalledWith(1, undefined); + expect(listTools).toHaveBeenNthCalledWith(2, { cursor: 'c1' }); + expect(listTools).toHaveBeenNthCalledWith(3, { cursor: 'c2' }); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); + + it('stops at the page cap and warns when a server keeps returning new cursors', async () => { + let page = 0; + const listTools = jest.fn(async () => { + page += 1; + return { tools: [makeTool(`t${page}`)], nextCursor: `cursor-${page}` }; + }); + const conn = createConnectionWithListTools(listTools); + + const tools = await conn.fetchTools(); + + // mcpConfig.TOOLS_LIST_MAX_PAGES is mocked to 3. + expect(listTools).toHaveBeenCalledTimes(3); + expect(tools.map((t) => t.name)).toEqual(['t1', 't2', 't3']); + expect(mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('pagination limit')); + }); + + it('stops and warns when the server repeats a cursor instead of looping forever', async () => { + const listTools = jest.fn().mockResolvedValue({ tools: [makeTool('x')], nextCursor: 'same' }); + const conn = createConnectionWithListTools(listTools); + + const tools = await conn.fetchTools(); + + expect(listTools).toHaveBeenCalledTimes(2); + // The second page's tools are collected before the repeated cursor is detected, hence two copies. + expect(tools.map((t) => t.name)).toEqual(['x', 'x']); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('repeated tools/list cursor'), + ); + }); + + it('continues paginating across an empty intermediate page', async () => { + const listTools = jest.fn(async (params?: { cursor?: string }) => { + if (params?.cursor == null) { + return { tools: [], nextCursor: 'c1' }; + } + return { tools: [makeTool('a')] }; + }); + const conn = createConnectionWithListTools(listTools); + + const tools = await conn.fetchTools(); + + expect(tools.map((t) => t.name)).toEqual(['a']); + expect(listTools).toHaveBeenCalledTimes(2); + }); + + it('treats an empty-string nextCursor as a valid cursor, not end-of-list', async () => { + const listTools = jest.fn(async (params?: { cursor?: string }) => { + if (params?.cursor == null) { + return { tools: [makeTool('a')], nextCursor: '' }; + } + return { tools: [makeTool('b')] }; + }); + const conn = createConnectionWithListTools(listTools); + + const tools = await conn.fetchTools(); + + expect(tools.map((t) => t.name)).toEqual(['a', 'b']); + expect(listTools).toHaveBeenCalledTimes(2); + expect(listTools).toHaveBeenNthCalledWith(2, { cursor: '' }); + }); + + it('returns the pages already fetched when a later page fails, without throwing', async () => { + const listTools = jest.fn(async (params?: { cursor?: string }) => { + if (params?.cursor == null) { + return { tools: [makeTool('a'), makeTool('b')], nextCursor: 'c1' }; + } + throw new Error('page 2 boom'); + }); + const conn = createConnectionWithListTools(listTools); + + const tools = await conn.fetchTools(); + + expect(tools.map((t) => t.name)).toEqual(['a', 'b']); + expect(listTools).toHaveBeenCalledTimes(2); + expect(mockLogger.error).toHaveBeenCalledWith(expect.stringContaining('Failed to fetch tools')); + }); + + it('returns an empty array when the first page request rejects', async () => { + const listTools = jest.fn().mockRejectedValue(new Error('boom')); + const conn = createConnectionWithListTools(listTools); + + const tools = await conn.fetchTools(); + + expect(tools).toEqual([]); + expect(listTools).toHaveBeenCalledTimes(1); + expect(mockLogger.error).toHaveBeenCalledWith(expect.stringContaining('Failed to fetch tools')); + }); +}); diff --git a/packages/api/src/mcp/__tests__/MCPConnectionSSRF.test.ts b/packages/api/src/mcp/__tests__/MCPConnectionSSRF.test.ts index 87190ea03f6..1fe735d10f1 100644 --- a/packages/api/src/mcp/__tests__/MCPConnectionSSRF.test.ts +++ b/packages/api/src/mcp/__tests__/MCPConnectionSSRF.test.ts @@ -71,7 +71,7 @@ jest.mock('~/auth', () => ({ })); jest.mock('~/mcp/mcpConfig', () => ({ - mcpConfig: { CONNECTION_CHECK_TTL: 0 }, + mcpConfig: { CONNECTION_CHECK_TTL: 0, TOOLS_LIST_MAX_PAGES: 50 }, })); const mockedResolveHostnameSSRF = resolveHostnameSSRF as jest.MockedFunction< diff --git a/packages/api/src/mcp/__tests__/dbSourced.integration.test.ts b/packages/api/src/mcp/__tests__/dbSourced.integration.test.ts index 79241f1d6b7..4ee078795c8 100644 --- a/packages/api/src/mcp/__tests__/dbSourced.integration.test.ts +++ b/packages/api/src/mcp/__tests__/dbSourced.integration.test.ts @@ -43,7 +43,7 @@ jest.mock('~/auth', () => ({ })); jest.mock('~/mcp/mcpConfig', () => ({ - mcpConfig: { CONNECTION_CHECK_TTL: 0 }, + mcpConfig: { CONNECTION_CHECK_TTL: 0, TOOLS_LIST_MAX_PAGES: 50 }, })); /** Track all Agents for cleanup */ diff --git a/packages/api/src/mcp/connection.ts b/packages/api/src/mcp/connection.ts index 23661816840..38d99e18cb1 100644 --- a/packages/api/src/mcp/connection.ts +++ b/packages/api/src/mcp/connection.ts @@ -1095,6 +1095,9 @@ interface MCPConnectionParams { ephemeralConnection?: boolean; } +/** Result of an MCP `tools/list` request: one page of tools plus an optional pagination cursor. */ +type MCPListToolsResult = Awaited>; + export class MCPConnection extends EventEmitter { public client: Client; private options: t.MCPOptions; @@ -2191,56 +2194,58 @@ export class MCPConnection extends EventEmitter { } } - async fetchTools(): Promise< - { - inputSchema: { - [x: string]: unknown; - type: 'object'; - properties?: Record | undefined; - required?: string[] | undefined; - }; - name: string; - description?: string | undefined; - outputSchema?: - | { - [x: string]: unknown; - type: 'object'; - properties?: Record | undefined; - required?: string[] | undefined; - } - | undefined; - annotations?: - | { - title?: string | undefined; - readOnlyHint?: boolean | undefined; - destructiveHint?: boolean | undefined; - idempotentHint?: boolean | undefined; - openWorldHint?: boolean | undefined; - } - | undefined; - execution?: - | { - taskSupport?: 'optional' | 'required' | 'forbidden' | undefined; - } - | undefined; - _meta?: Record | undefined; - icons?: - | { - src: string; - mimeType?: string | undefined; - sizes?: string[] | undefined; - theme?: 'light' | 'dark' | undefined; - }[] - | undefined; - title?: string | undefined; - }[] - > { + /** + * Fetches the server's tools, following MCP `tools/list` cursor pagination so a + * server that spans multiple pages (e.g. an aggregating gateway exposing many + * tools) is loaded in full instead of being truncated to the first page. + * + * Pagination is bounded by {@link mcpConfig.TOOLS_LIST_MAX_PAGES} and a + * repeated-cursor guard. On error, the tools already fetched are returned rather + * than discarded, and the method never throws. + */ + async fetchTools(): Promise { + const maxPages = mcpConfig.TOOLS_LIST_MAX_PAGES; + const allTools: MCPListToolsResult['tools'] = []; + const seenCursors = new Set(); + let cursor: string | undefined; + + for (let page = 1; page <= maxPages; page++) { + const result = await this.listToolsPage(cursor); + if (result == null) { + /** Request failed mid-pagination: return the pages already fetched instead of discarding them. */ + return allTools; + } + + allTools.push(...result.tools); + + const { nextCursor } = result; + if (nextCursor == null) { + return allTools; + } + if (seenCursors.has(nextCursor)) { + logger.warn( + `${this.getLogPrefix()} MCP server returned a repeated tools/list cursor; stopping pagination after ${page} page(s).`, + ); + return allTools; + } + + seenCursors.add(nextCursor); + cursor = nextCursor; + } + + logger.warn( + `${this.getLogPrefix()} Reached the tools/list pagination limit of ${maxPages} page(s); some tools may be omitted. Set MCP_TOOLS_LIST_MAX_PAGES higher if this server legitimately exposes more.`, + ); + return allTools; + } + + /** Fetches a single `tools/list` page, returning null (and logging) on failure so pagination can stop gracefully. */ + private async listToolsPage(cursor: string | undefined): Promise { try { - const { tools } = await this.client.listTools(); - return tools; + return await this.client.listTools(cursor != null ? { cursor } : undefined); } catch (error) { this.emitError(error, 'Failed to fetch tools'); - return []; + return null; } } diff --git a/packages/api/src/mcp/mcpConfig.ts b/packages/api/src/mcp/mcpConfig.ts index 68e22a81c28..ea75220958f 100644 --- a/packages/api/src/mcp/mcpConfig.ts +++ b/packages/api/src/mcp/mcpConfig.ts @@ -23,6 +23,9 @@ export const mcpConfig: { /** TTL (ms) for OAuth flow state. Must outlive OAUTH_HANDLING_TIMEOUT so the state survives the wait. Default: 15 minutes */ OAUTH_FLOW_TTL: number; CONNECTION_CHECK_TTL: number; + /** Max number of `tools/list` pages to request when an MCP server paginates its tool list. + * Bounds the pagination loop so a misbehaving server cannot stall tool discovery. Default: 50 */ + TOOLS_LIST_MAX_PAGES: number; /** Idle timeout (ms) after which user connections are disconnected. Default: 15 minutes */ USER_CONNECTION_IDLE_TIMEOUT: number; /** Max connect/disconnect cycles before the circuit breaker trips. Default: 7 */ @@ -47,6 +50,8 @@ export const mcpConfig: { /** TTL (ms) for OAuth flow state. Clamped to never fall below OAUTH_HANDLING_TIMEOUT. Default: 15 minutes */ OAUTH_FLOW_TTL: oauthFlowTtl, CONNECTION_CHECK_TTL: math(process.env.MCP_CONNECTION_CHECK_TTL ?? 60000), + /** Max number of `tools/list` pages to request when an MCP server paginates its tool list. Clamped to >= 1. Default: 50 */ + TOOLS_LIST_MAX_PAGES: Math.max(1, math(process.env.MCP_TOOLS_LIST_MAX_PAGES ?? 50)), /** Idle timeout (ms) after which user connections are disconnected. Default: 15 minutes */ USER_CONNECTION_IDLE_TIMEOUT: math( process.env.MCP_USER_CONNECTION_IDLE_TIMEOUT ?? 15 * 60 * 1000, diff --git a/packages/api/src/mcp/registry/MCPServerInspector.ts b/packages/api/src/mcp/registry/MCPServerInspector.ts index 2f41678d981..4e0b9438e45 100644 --- a/packages/api/src/mcp/registry/MCPServerInspector.ts +++ b/packages/api/src/mcp/registry/MCPServerInspector.ts @@ -150,8 +150,8 @@ export class MCPServerInspector { private async fetchServerCapabilities(): Promise { const capabilities = this.connection!.client.getServerCapabilities(); this.config.capabilities = JSON.stringify(capabilities); - const tools = await this.connection!.client.listTools(); - this.config.tools = tools.tools.map((tool) => tool.name).join(', '); + const tools = await this.connection!.fetchTools(); + this.config.tools = tools.map((tool) => tool.name).join(', '); } private async fetchToolFunctions(): Promise { @@ -171,7 +171,7 @@ export class MCPServerInspector { serverName: string, connection: MCPConnection, ): Promise { - const { tools }: t.MCPToolListResponse = await connection.client.listTools(); + const tools = await connection.fetchTools(); const toolFunctions: t.LCAvailableTools = {}; tools.forEach((tool) => { diff --git a/packages/api/src/mcp/registry/__tests__/MCPReinitRecovery.integration.test.ts b/packages/api/src/mcp/registry/__tests__/MCPReinitRecovery.integration.test.ts index 00a485cd859..6dc8f7f603f 100644 --- a/packages/api/src/mcp/registry/__tests__/MCPReinitRecovery.integration.test.ts +++ b/packages/api/src/mcp/registry/__tests__/MCPReinitRecovery.integration.test.ts @@ -56,7 +56,7 @@ jest.mock('~/cluster', () => ({ })); jest.mock('~/mcp/mcpConfig', () => ({ - mcpConfig: { CONNECTION_CHECK_TTL: 0 }, + mcpConfig: { CONNECTION_CHECK_TTL: 0, TOOLS_LIST_MAX_PAGES: 50 }, })); jest.mock('~/mcp/registry/db/ServerConfigsDB', () => ({ diff --git a/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts b/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts index dd64c3e5bd0..f6af7b52b85 100644 --- a/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts +++ b/packages/api/src/mcp/registry/__tests__/MCPServerInspector.test.ts @@ -376,7 +376,7 @@ describe('MCPServerInspector', () => { }); // Mock server with no tools - mockConnection.client.listTools = jest.fn().mockResolvedValue({ tools: [] }); + mockConnection.fetchTools = jest.fn().mockResolvedValue([]); const result = await MCPServerInspector.inspect('test_server', rawConfig, mockConnection); @@ -462,29 +462,27 @@ describe('MCPServerInspector', () => { describe('getToolFunctions()', () => { it('should convert MCP tools to LibreChat tool functions format', async () => { - mockConnection.client.listTools = jest.fn().mockResolvedValue({ - tools: [ - { - name: 'file_read', - description: 'Read a file', - inputSchema: { - type: 'object', - properties: { path: { type: 'string' } }, - }, + mockConnection.fetchTools = jest.fn().mockResolvedValue([ + { + name: 'file_read', + description: 'Read a file', + inputSchema: { + type: 'object', + properties: { path: { type: 'string' } }, }, - { - name: 'file_write', - description: 'Write a file', - inputSchema: { - type: 'object', - properties: { - path: { type: 'string' }, - content: { type: 'string' }, - }, + }, + { + name: 'file_write', + description: 'Write a file', + inputSchema: { + type: 'object', + properties: { + path: { type: 'string' }, + content: { type: 'string' }, }, }, - ], - }); + }, + ]); const result = await MCPServerInspector.getToolFunctions('my_server', mockConnection); @@ -518,7 +516,7 @@ describe('MCPServerInspector', () => { }); it('should handle empty tools list', async () => { - mockConnection.client.listTools = jest.fn().mockResolvedValue({ tools: [] }); + mockConnection.fetchTools = jest.fn().mockResolvedValue([]); const result = await MCPServerInspector.getToolFunctions('my_server', mockConnection); diff --git a/packages/api/src/mcp/registry/__tests__/mcpConnectionsMock.helper.ts b/packages/api/src/mcp/registry/__tests__/mcpConnectionsMock.helper.ts index 74bc83425d2..5e028c4c81e 100644 --- a/packages/api/src/mcp/registry/__tests__/mcpConnectionsMock.helper.ts +++ b/packages/api/src/mcp/registry/__tests__/mcpConnectionsMock.helper.ts @@ -7,6 +7,18 @@ import type { MCPConnection } from '~/mcp/connection'; * @returns Mocked MCPConnection instance */ export function createMockConnection(serverName: string): jest.Mocked { + const tools = [ + { + name: 'listFiles', + description: `Description for ${serverName}'s listFiles tool`, + inputSchema: { + type: 'object', + properties: { + input: { type: 'string' }, + }, + }, + }, + ]; const mockClient = { getInstructions: jest.fn().mockReturnValue(`instructions for ${serverName}`), getServerCapabilities: jest.fn().mockReturnValue({ @@ -14,24 +26,12 @@ export function createMockConnection(serverName: string): jest.Mocked; }