diff --git a/packages/api/src/agents/__tests__/load.spec.ts b/packages/api/src/agents/__tests__/load.spec.ts index 41deaf4fcca..1a4ba4adb65 100644 --- a/packages/api/src/agents/__tests__/load.spec.ts +++ b/packages/api/src/agents/__tests__/load.spec.ts @@ -315,7 +315,7 @@ describe('loadAgent', () => { /** `applyContextToAgent` reads this straight off the body and prefers it * over the agent's tools when loading `serverInstructions`: the hidden * pick is gone, the spec's pin stays. */ - expect(body.ephemeralAgent.mcp).toEqual(['visible', 'hidden']); + expect(body.ephemeralAgent.mcp).toEqual(['hidden', 'visible']); }); }); diff --git a/packages/api/src/agents/__tests__/toolOrderStability.test.ts b/packages/api/src/agents/__tests__/toolOrderStability.test.ts new file mode 100644 index 00000000000..0984741215a --- /dev/null +++ b/packages/api/src/agents/__tests__/toolOrderStability.test.ts @@ -0,0 +1,472 @@ +/** + * Provider payload stability of the `tools` array and the MCP instructions. + * + * Providers cache from the start of the request, and `tools` is serialized + * before the system prompt, so the same MCP servers selected in a different + * order, or a server whose tool map keys come back in a different order after + * a reconnect, must not change a single byte of either. + * + * Each case assembles a realistic mixed tool set through the real code paths: + * `loadEphemeralAgent` (picker selection and spec servers), `loadToolDefinitions` + * (MCP catalogs, deferred tools plus `tool_search`, built-ins, an action tool), + * the capability registrars `initializeAgent` runs (code execution, memory, file + * authoring), the real `MCPManager` instruction formatter, and `createRun` with + * self-spawn subagents. The serialized request is captured right before it would + * leave the process (mocked `fetch`, or an injected Bedrock client) and aborted + * there; nothing reaches the network. + * + * Not driven through the real path: skill tools (they need the skill catalog and + * file storage that `initializeAgent` hydrates) are covered by the registrar + * seam only, and saved-agent graph handoff tools by the dedicated graph specs. + */ +import { HumanMessage } from '@langchain/core/messages'; +import { Providers } from '@librechat/agents'; +import { Tools } from 'librechat-data-provider'; +import type { LCTool } from '@librechat/agents'; +import type { LoadAgentDeps } from '../load'; +import { registerCodeExecutionTools, registerFileAuthoringTools } from '../tools'; +import { getToolDefinition } from '~/tools/registry/definitions'; +import { loadToolDefinitions } from '~/tools/definitions'; +import { applyContextToAgent } from '../context'; +import { registerMemoryTools } from '../memory'; +import { formatMCPServerTools } from '~/mcp/tools'; +import { loadEphemeralAgent } from '../load'; +import { MCPManager } from '~/mcp/MCPManager'; +import { createRun } from '../run'; + +const mockServerConfigs: Record = {}; + +jest.mock('~/mcp/registry/MCPServersRegistry', () => ({ + MCPServersRegistry: { + getInstance: () => ({ + getAllServerConfigs: async () => mockServerConfigs, + }), + }, +})); + +jest.setTimeout(60000); + +const CAPTURED = 'request captured before network'; +const ACTION_TOOL = 'getForecast_action_weather---example---com'; + +const SERVERS = ['alpha', 'beta', 'gamma', 'a_b'] as const; +type Server = (typeof SERVERS)[number]; + +const TOOL_NAMES: Record = { + alpha: ['search_docs', 'fetch_page', 'list_sources'], + beta: ['run_query', 'describe_table', 'list_tables'], + gamma: ['create_ticket', 'close_ticket'], + a_b: ['ping'], +}; + +/** One tool per server is deferred, so `tool_search` is part of every payload. */ +const DEFERRED = new Set(['fetch_page_mcp_alpha', 'list_tables_mcp_beta']); + +for (const server of SERVERS) { + mockServerConfigs[server] = { serverInstructions: `Rules for ${server}.` }; +} + +function catalogFor(server: Server, reverseKeys: boolean): Record { + const catalog = formatMCPServerTools( + server, + TOOL_NAMES[server].map((name) => ({ + name, + description: `${name} on ${server}`, + inputSchema: { + type: 'object', + properties: { input: { type: 'string', description: `Input for ${name}` } }, + required: ['input'], + }, + })), + ) as Record; + const entries = Object.entries(catalog); + return Object.fromEntries(reverseKeys ? entries.reverse() : entries); +} + +interface Scenario { + picker: string[]; + reverseKeys?: boolean; + specServers?: string[]; + failServer?: Server; +} + +interface Assembled { + agent: Record; + toolNames: string[]; + instructions: string; +} + +async function assemble(provider: Providers, scenario: Scenario): Promise { + const { picker, reverseKeys = false, specServers, failServer } = scenario; + const deps: LoadAgentDeps = { + getAgent: async () => null, + getMCPServerTools: async (_userId, serverName) => + serverName === failServer ? null : catalogFor(serverName as Server, reverseKeys), + }; + const req = { + user: { id: 'user-1' }, + config: specServers + ? { modelSpecs: { list: [{ name: 'pinned', mcpServers: specServers }] } } + : {}, + body: { + ephemeralAgent: { + mcp: picker, + web_search: true, + execute_code: true, + file_search: true, + memory: true, + }, + }, + } as unknown as Parameters[0]['req']; + + const ephemeral = await loadEphemeralAgent( + { + req, + spec: specServers ? 'pinned' : undefined, + endpoint: 'custom', + model_parameters: { model: 'm' } as never, + }, + deps, + ); + const tools = [...(ephemeral?.tools ?? []), ACTION_TOOL]; + + const toolOptions = Object.fromEntries( + [...DEFERRED].map((name) => [name, { defer_loading: true }]), + ); + const loaded = await loadToolDefinitions( + { + userId: 'user-1', + agentId: 'agent_stable', + tools, + provider, + toolOptions, + deferredToolsEnabled: true, + mcpServerNames: [...SERVERS], + rawServerNames: [...SERVERS], + }, + { + getOrFetchMCPServerTools: async (_userId, serverName) => + serverName === failServer ? null : (catalogFor(serverName as Server, reverseKeys) as never), + isBuiltInTool: (name) => getToolDefinition(name) != null, + getActionToolDefinitions: async () => [ + { + name: ACTION_TOOL, + description: 'Gets a forecast', + parameters: { type: 'object', properties: { city: { type: 'string' } } }, + }, + ], + }, + ); + + /** Same registrars, same order as `initializeAgent`. */ + let toolDefinitions = loaded.toolDefinitions as LCTool[]; + const code = registerCodeExecutionTools({ + toolRegistry: loaded.toolRegistry, + toolDefinitions, + includeBash: true, + includeSkillFileInstructions: false, + enableToolOutputReferences: true, + }); + toolDefinitions = code.toolDefinitions; + toolDefinitions = registerMemoryTools({ + toolRegistry: loaded.toolRegistry, + toolDefinitions, + }).toolDefinitions; + toolDefinitions = registerFileAuthoringTools({ + toolRegistry: loaded.toolRegistry, + toolDefinitions, + includeSkillFileInstructions: false, + }).toolDefinitions; + + const agent: Record = { + id: 'agent_stable', + name: 'Stable Agent', + instructions: 'You are a careful assistant.', + additional_instructions: 'Answer in the language of the user.', + tools: [], + toolDefinitions, + toolRegistry: loaded.toolRegistry, + hasDeferredTools: loaded.hasDeferredTools, + subagents: { enabled: true, allowSelf: true }, + maxContextTokens: 100000, + }; + const manager = Object.create(MCPManager.prototype) as MCPManager; + await applyContextToAgent({ + agent: agent as never, + sharedRunContext: '', + mcpManager: manager, + ephemeralAgent: req.body?.ephemeralAgent, + }); + + return { + agent, + toolNames: toolDefinitions.map((def) => def.name), + instructions: String(agent.instructions), + }; +} + +type ProviderCase = { + name: string; + provider: Providers; + modelParameters: Record; +}; + +type Captured = { system: unknown; tools: unknown }; + +async function capture(providerCase: ProviderCase, assembled: Assembled): Promise { + let captured: Captured | undefined; + const modelParameters: Record = { ...providerCase.modelParameters }; + const agent = { + ...assembled.agent, + provider: providerCase.provider, + endpoint: providerCase.provider, + model_parameters: modelParameters, + }; + + const fetchMock = jest.spyOn(global, 'fetch').mockImplementation(async (input, init) => { + const request = input instanceof Request ? input : undefined; + const raw = request ? await request.clone().text() : String(init?.body ?? '{}'); + const body = JSON.parse(raw) as Record; + if (providerCase.provider === Providers.GOOGLE) { + captured = { system: body.systemInstruction, tools: body.tools }; + } else if (providerCase.provider === Providers.ANTHROPIC) { + captured = { system: body.system, tools: body.tools }; + } else { + const messages = body.messages as Array<{ role: string }>; + captured = { + system: messages.filter((message) => message.role === 'system'), + tools: body.tools, + }; + } + throw new Error(CAPTURED); + }); + if (providerCase.provider === Providers.BEDROCK) { + modelParameters.client = { + send: async (command: { input: Record }) => { + captured = { system: command.input.system, tools: command.input.toolConfig }; + throw new Error(CAPTURED); + }, + config: {}, + destroy: () => undefined, + middlewareStack: { add: () => undefined, use: () => undefined }, + }; + } + + try { + const messages = [new HumanMessage('Hello')]; + const runId = `tool-order-${providerCase.name}-${Math.random()}`; + const run = await createRun({ + agents: [agent] as never, + messages: [] as never, + runId, + signal: new AbortController().signal, + streaming: false, + streamUsage: false, + }); + await run + .processStream( + { messages: messages as never }, + { configurable: { thread_id: runId }, version: 'v2' }, + ) + .catch(() => undefined); + } finally { + fetchMock.mockRestore(); + } + if (captured == null) { + throw new Error(`No request captured for ${providerCase.name}`); + } + return captured; +} + +const toolNamesIn = (tools: unknown): string[] => + [...JSON.stringify(tools).matchAll(/"name":"([^"]+)"/g)].map((match) => match[1]); + +const MCP_ALPHA = ['fetch_page_mcp_alpha', 'list_sources_mcp_alpha', 'search_docs_mcp_alpha']; +const MCP_BETA = ['describe_table_mcp_beta', 'list_tables_mcp_beta', 'run_query_mcp_beta']; +const MCP_GAMMA = ['close_ticket_mcp_gamma', 'create_ticket_mcp_gamma']; + +/** + * Explicit group order of the final definitions list: MCP tools (sorted by + * server, then tool), `tool_search`, actions, ephemeral built-ins in their + * existing order, then the capability registrars in `initializeAgent` order. + * Only the MCP group is sorted; every other group keeps the order it had before. + */ +const EXPECTED_ORDER = [ + 'ping_mcp_a_b', + ...MCP_ALPHA, + ...MCP_BETA, + ...MCP_GAMMA, + 'tool_search', + ACTION_TOOL, + Tools.file_search, + Tools.web_search, + 'read_file', + 'bash_tool', + 'set_memory', + 'delete_memory', + 'create_file', + 'edit_file', +]; + +/** Deferred tools are discoverable through `tool_search` and are not sent; `subagent` is added by `createRun`. */ +const PAYLOAD_ORDER = [...EXPECTED_ORDER.filter((name) => !DEFERRED.has(name)), 'subagent']; + +const PICKER_ORDERS: string[][] = [ + ['alpha', 'beta', 'gamma', 'a_b'], + ['gamma', 'a_b', 'beta', 'alpha'], + ['beta', 'alpha', 'a_b', 'gamma', 'beta'], +]; + +const PROVIDERS: ProviderCase[] = [ + { + name: 'openAI', + provider: Providers.OPENAI, + modelParameters: { model: 'gpt-4.1', apiKey: 'test-key', maxRetries: 0 }, + }, + { + name: 'custom OpenAI-compatible', + provider: Providers.OPENAI, + modelParameters: { + model: 'local-model', + apiKey: 'test-key', + maxRetries: 0, + configuration: { baseURL: 'https://compat.invalid/v1' }, + }, + }, + { + name: 'anthropic with promptCache', + provider: Providers.ANTHROPIC, + modelParameters: { + model: 'claude-sonnet-4-5', + apiKey: 'test-key', + maxRetries: 0, + promptCache: true, + }, + }, + { + name: 'bedrock with promptCache', + provider: Providers.BEDROCK, + modelParameters: { + model: 'anthropic.claude-sonnet-4-5-20250929-v1:0', + region: 'us-east-1', + credentials: { accessKeyId: 'test', secretAccessKey: 'test' }, + maxRetries: 0, + promptCache: true, + }, + }, + { + name: 'google', + provider: Providers.GOOGLE, + modelParameters: { model: 'gemini-2.5-flash', apiKey: 'test-key', maxRetries: 0 }, + }, +]; + +describe('definition assembly order', () => { + it('sorts the MCP group and keeps the built-in group order', async () => { + const { toolNames } = await assemble(Providers.OPENAI, { picker: PICKER_ORDERS[1] }); + expect(toolNames).toEqual(EXPECTED_ORDER); + }); + + it('is identical for every picker order and tool map key order', async () => { + const baseline = (await assemble(Providers.OPENAI, { picker: PICKER_ORDERS[0] })).toolNames; + for (const picker of PICKER_ORDERS) { + for (const reverseKeys of [false, true]) { + const result = await assemble(Providers.OPENAI, { picker, reverseKeys }); + expect(result.toolNames).toEqual(baseline); + } + } + }); + + it('keeps a failed server as an ordered placeholder without moving other tools', async () => { + const result = await assemble(Providers.OPENAI, { + picker: PICKER_ORDERS[1], + failServer: 'beta', + }); + expect(result.toolNames.filter((name) => name.includes('_mcp_'))).toEqual([ + 'ping_mcp_a_b', + ...MCP_ALPHA, + ...MCP_GAMMA, + ]); + const baseline = await assemble(Providers.OPENAI, { + picker: PICKER_ORDERS[0], + failServer: 'beta', + }); + expect(result.toolNames).toEqual(baseline.toolNames); + }); + + it('orders spec-pinned servers together with the picker selection', async () => { + const merged = await assemble(Providers.OPENAI, { + picker: ['gamma', 'alpha'], + specServers: ['beta', 'a_b'], + }); + const plain = await assemble(Providers.OPENAI, { picker: PICKER_ORDERS[0] }); + expect(merged.toolNames).toEqual(plain.toolNames); + expect(merged.instructions).toBe(plain.instructions); + }); + + it('orders MCP instructions by server name', async () => { + const first = await assemble(Providers.OPENAI, { picker: PICKER_ORDERS[0] }); + const second = await assemble(Providers.OPENAI, { picker: PICKER_ORDERS[1] }); + expect(second.instructions).toBe(first.instructions); + const positions = ['a_b', 'alpha', 'beta', 'gamma'].map((name) => + first.instructions.indexOf(`## ${name} MCP Server Instructions`), + ); + expect(positions.every((position) => position >= 0)).toBe(true); + expect([...positions].sort((a, b) => a - b)).toEqual(positions); + }); +}); + +describe('provider payload tool order stability', () => { + describe.each(PROVIDERS.map((providerCase) => [providerCase.name, providerCase] as const))( + '%s', + (_name, providerCase) => { + let baseline: Captured; + + beforeAll(async () => { + baseline = await capture( + providerCase, + await assemble(providerCase.provider, { picker: PICKER_ORDERS[0] }), + ); + }); + + it('serializes the expected tool group order', () => { + const names = toolNamesIn(baseline.tools); + expect(names).toEqual(PAYLOAD_ORDER); + expect(JSON.stringify(baseline.system)).toContain('## alpha MCP Server Instructions'); + }); + + it.each(PICKER_ORDERS.slice(1).map((order, i) => [i + 1, order] as const))( + 'keeps tools and system identical for picker order %i', + async (_index, picker) => { + const next = await capture( + providerCase, + await assemble(providerCase.provider, { picker }), + ); + expect(JSON.stringify(next.tools)).toBe(JSON.stringify(baseline.tools)); + expect(JSON.stringify(next.system)).toBe(JSON.stringify(baseline.system)); + }, + ); + + it('keeps tools and system identical when tool map keys are shuffled', async () => { + const next = await capture( + providerCase, + await assemble(providerCase.provider, { + picker: PICKER_ORDERS[1], + reverseKeys: true, + }), + ); + expect(JSON.stringify(next.tools)).toBe(JSON.stringify(baseline.tools)); + expect(JSON.stringify(next.system)).toBe(JSON.stringify(baseline.system)); + }); + + it('keeps tools and system identical on a repeated turn', async () => { + const next = await capture( + providerCase, + await assemble(providerCase.provider, { picker: PICKER_ORDERS[0] }), + ); + expect(JSON.stringify(next.tools)).toBe(JSON.stringify(baseline.tools)); + expect(JSON.stringify(next.system)).toBe(JSON.stringify(baseline.system)); + }); + }, + ); +}); diff --git a/packages/api/src/agents/added.ts b/packages/api/src/agents/added.ts index 970072af929..193a92971b9 100644 --- a/packages/api/src/agents/added.ts +++ b/packages/api/src/agents/added.ts @@ -17,6 +17,7 @@ import { requiresEphemeralUserConnection, filterChatSelectableMCPServers, validateMCPServerConfig, + sortNames, } from '~/mcp/utils'; import { ASK_USER_QUESTION_TOOL_NAME } from '~/agents/hitl/askUserQuestionTool'; import { resolveMCPClientCapabilityProfile } from '~/mcp/capabilities'; @@ -245,7 +246,7 @@ export async function loadAddedAgent( } const addedServers = new Set(); - for (const mcpServer of mcpServers) { + for (const mcpServer of sortNames(mcpServers)) { if (addedServers.has(mcpServer)) { continue; } @@ -262,7 +263,7 @@ export async function loadAddedAgent( addedServers.add(mcpServer); continue; } - tools.push(...Object.keys(serverTools)); + tools.push(...sortNames(Object.keys(serverTools))); addedServers.add(mcpServer); } diff --git a/packages/api/src/agents/load.spec.ts b/packages/api/src/agents/load.spec.ts index 48620851e22..87c610eac06 100644 --- a/packages/api/src/agents/load.spec.ts +++ b/packages/api/src/agents/load.spec.ts @@ -100,3 +100,153 @@ describe('loadEphemeralAgent → resolveSender parity', () => { expect(sender).toBe(parseEphemeralAgentId(agent?.id ?? '')?.sender); }); }); + +describe('loadEphemeralAgent MCP ordering', () => { + const catalogs: Record> = { + alpha: { b_mcp_alpha: {}, a_mcp_alpha: {} }, + beta: { z_mcp_beta: {}, c_mcp_beta: {}, m_mcp_beta: {} }, + gamma: { only_mcp_gamma: {} }, + }; + const orderDeps: LoadAgentDeps = { + getAgent: async () => null, + getMCPServerTools: async (_userId, serverName) => catalogs[serverName] ?? null, + }; + + async function load(mcp: string[], deps: LoadAgentDeps = orderDeps) { + const req = { + user: { id: 'user-1' }, + config: {}, + body: { ephemeralAgent: { mcp } }, + } as unknown as Parameters[0]['req']; + const agent = await loadEphemeralAgent( + { req, endpoint: 'my-custom-endpoint', model_parameters: { model: 'm' } as never }, + deps, + ); + return { agent, published: req.body?.ephemeralAgent?.mcp }; + } + + test('different picker orders of the same servers yield identical tools and published servers', async () => { + const first = await load(['gamma', 'alpha', 'beta']); + const second = await load(['beta', 'gamma', 'alpha']); + expect(first.agent?.tools).toEqual([ + 'a_mcp_alpha', + 'b_mcp_alpha', + 'c_mcp_beta', + 'm_mcp_beta', + 'z_mcp_beta', + 'only_mcp_gamma', + ]); + expect(second.agent?.tools).toEqual(first.agent?.tools); + expect(first.published).toEqual(['alpha', 'beta', 'gamma']); + expect(second.published).toEqual(first.published); + }); + + test('shuffled tool map key order yields the same tool list', async () => { + const reordered: LoadAgentDeps = { + ...orderDeps, + getMCPServerTools: async (_userId, serverName) => { + const catalog = catalogs[serverName]; + if (catalog == null) { + return null; + } + return Object.fromEntries(Object.entries(catalog).reverse()); + }, + }; + const baseline = await load(['alpha', 'beta']); + const shuffled = await load(['alpha', 'beta'], reordered); + expect(shuffled.agent?.tools).toEqual(baseline.agent?.tools); + }); + + test('every permutation of the picker selection yields the same tools', async () => { + const servers = ['alpha', 'beta', 'gamma']; + const permutations = (items: string[]): string[][] => + items.length <= 1 + ? [items] + : items.flatMap((item, i) => + permutations([...items.slice(0, i), ...items.slice(i + 1)]).map((rest) => [ + item, + ...rest, + ]), + ); + const baseline = (await load(servers)).agent?.tools; + for (const order of permutations(servers)) { + expect((await load(order)).agent?.tools).toEqual(baseline); + } + }); + + test('similarly named servers are ordered by plain code-unit comparison', async () => { + const similar: LoadAgentDeps = { + ...orderDeps, + getMCPServerTools: async (_userId, serverName) => ({ [`t_mcp_${serverName}`]: {} }), + }; + const first = await load(['a_b', 'a', 'A'], similar); + const second = await load(['A', 'a', 'a_b'], similar); + expect(first.published).toEqual(['A', 'a', 'a_b']); + expect(first.agent?.tools).toEqual(['t_mcp_A', 't_mcp_a', 't_mcp_a_b']); + expect(second.agent?.tools).toEqual(first.agent?.tools); + }); + + test('duplicates collapse and empty servers contribute no tools', async () => { + const withEmpty: LoadAgentDeps = { + ...orderDeps, + getMCPServerTools: async (_userId, serverName) => + serverName === 'empty' ? {} : (catalogs[serverName] ?? null), + }; + const result = await load(['gamma', 'empty', 'gamma', 'alpha', 'empty'], withEmpty); + expect(result.published).toEqual(['alpha', 'empty', 'gamma']); + expect(result.agent?.tools).toEqual(['a_mcp_alpha', 'b_mcp_alpha', 'only_mcp_gamma']); + }); + + test('a failing server keeps its placeholder in name order among resolved servers', async () => { + const partial: LoadAgentDeps = { + ...orderDeps, + getMCPServerTools: async (_userId, serverName) => + serverName === 'beta' ? null : (catalogs[serverName] ?? null), + }; + const result = await load(['gamma', 'beta', 'alpha'], partial); + expect(result.agent?.tools).toEqual([ + 'a_mcp_alpha', + 'b_mcp_alpha', + 'sys__all__sys_mcp_beta', + 'only_mcp_gamma', + ]); + }); + + test('spec servers merge with the picker selection in one name-ordered set', async () => { + const req = { + user: { id: 'user-1' }, + config: { modelSpecs: { list: [{ name: 'pinned', mcpServers: ['gamma', 'alpha'] }] } }, + body: { ephemeralAgent: { mcp: ['beta', 'alpha'] } }, + } as unknown as Parameters[0]['req']; + const agent = await loadEphemeralAgent( + { + req, + spec: 'pinned', + endpoint: 'my-custom-endpoint', + model_parameters: { model: 'm' } as never, + }, + orderDeps, + ); + expect(req.body?.ephemeralAgent?.mcp).toEqual(['alpha', 'beta', 'gamma']); + expect(agent?.tools).toEqual([ + 'a_mcp_alpha', + 'b_mcp_alpha', + 'c_mcp_beta', + 'm_mcp_beta', + 'z_mcp_beta', + 'only_mcp_gamma', + ]); + }); + + test('placeholders for unresolved servers follow server name order', async () => { + const result = await load(['beta', 'unknown_b', 'unknown_a'], { + ...orderDeps, + getMCPServerTools: async () => null, + }); + expect(result.agent?.tools).toEqual([ + 'sys__all__sys_mcp_beta', + 'sys__all__sys_mcp_unknown_a', + 'sys__all__sys_mcp_unknown_b', + ]); + }); +}); diff --git a/packages/api/src/agents/load.ts b/packages/api/src/agents/load.ts index 889a1cf5cfa..fd4e8232533 100644 --- a/packages/api/src/agents/load.ts +++ b/packages/api/src/agents/load.ts @@ -22,6 +22,7 @@ import { requiresEphemeralUserConnection, filterChatSelectableMCPServers, validateMCPServerConfig, + sortNames, } from '~/mcp/utils'; import { ASK_USER_QUESTION_TOOL_NAME } from '~/agents/hitl/askUserQuestionTool'; import { resolveMCPClientCapabilityProfile } from '~/mcp/capabilities'; @@ -105,8 +106,9 @@ export async function loadEphemeralAgent( * instruction path reads `req.body.ephemeralAgent.mcp` directly and prefers * it over the agent's tools, so it would otherwise both inject a hidden * server's `serverInstructions` and omit a spec-pinned server's. */ + const sortedMcpServers = sortNames(mcpServers); if (ephemeralAgent != null && Array.isArray(ephemeralAgent.mcp)) { - ephemeralAgent.mcp = [...mcpServers]; + ephemeralAgent.mcp = sortedMcpServers; } const tools: string[] = []; if (ephemeralAgent?.execute_code === true || modelSpec?.executeCode === true) { @@ -129,8 +131,8 @@ export async function loadEphemeralAgent( } const addedServers = new Set(); - if (mcpServers.size > 0) { - for (const mcpServer of mcpServers) { + if (sortedMcpServers.length > 0) { + for (const mcpServer of sortedMcpServers) { if (addedServers.has(mcpServer)) { continue; } @@ -149,7 +151,7 @@ export async function loadEphemeralAgent( addedServers.add(mcpServer); continue; } - tools.push(...Object.keys(serverTools)); + tools.push(...sortNames(Object.keys(serverTools))); addedServers.add(mcpServer); } } diff --git a/packages/api/src/mcp/MCPManager.ts b/packages/api/src/mcp/MCPManager.ts index b8edb984553..d1916ea7cfc 100644 --- a/packages/api/src/mcp/MCPManager.ts +++ b/packages/api/src/mcp/MCPManager.ts @@ -33,6 +33,7 @@ import { requiresOAuthMachinery, requiresUserScopedConnection, resolveServerInstructions, + sortNames, } from './utils'; import { getMCPConnectionPoolKey, @@ -898,8 +899,10 @@ export class MCPManager extends UserConnectionManager { instructions[serverName] = resolved; } } - if (!serverNames) return instructions; - return pick(instructions, serverNames); + const selected = serverNames ? pick(instructions, serverNames) : instructions; + return Object.fromEntries( + sortNames(Object.keys(selected)).map((name) => [name, selected[name]]), + ); } /** diff --git a/packages/api/src/mcp/__tests__/MCPManager.test.ts b/packages/api/src/mcp/__tests__/MCPManager.test.ts index e06fddf5f8f..54f0fae6c3e 100644 --- a/packages/api/src/mcp/__tests__/MCPManager.test.ts +++ b/packages/api/src/mcp/__tests__/MCPManager.test.ts @@ -577,6 +577,22 @@ describe('MCPManager', () => { expect(result).not.toContain('database'); }); + it('should produce identical text regardless of the requested server order', async () => { + (mockRegistryInstance.getAllServerConfigs as jest.Mock).mockResolvedValue({ + files: { type: 'stdio', command: 'node', args: ['f.js'], serverInstructions: 'Files rule' }, + github: { type: 'sse', url: 'https://api.github.com', serverInstructions: 'GitHub rule' }, + database: { type: 'sse', url: 'https://db.example', serverInstructions: 'DB rule' }, + }); + + const manager = await MCPManager.createInstance(newMCPServersConfig()); + const first = await manager.formatInstructionsForContext(['github', 'files', 'database']); + const second = await manager.formatInstructionsForContext(['database', 'github', 'files']); + + expect(second).toBe(first); + expect(first.indexOf('## database')).toBeLessThan(first.indexOf('## files')); + expect(first.indexOf('## files')).toBeLessThan(first.indexOf('## github')); + }); + it('should return empty string when filtered servers have no instructions', async () => { (mockRegistryInstance.getAllServerConfigs as jest.Mock).mockResolvedValue({ github: { diff --git a/packages/api/src/mcp/__tests__/utils.test.ts b/packages/api/src/mcp/__tests__/utils.test.ts index ea39bc18d89..b3b62d54447 100644 --- a/packages/api/src/mcp/__tests__/utils.test.ts +++ b/packages/api/src/mcp/__tests__/utils.test.ts @@ -28,9 +28,25 @@ import { isChatSelectableMCPServer, filterChatSelectableMCPServers, waitUntilDeadline, + sortNames, } from '~/mcp/utils'; import { usesDirectOpenIDBearerRecovery } from '~/mcp/openid'; +describe('sortNames', () => { + it('orders by code unit independent of input order, without mutating the input', () => { + const input = ['a_b', 'b', 'A', 'a', 'B', 'ä']; + const sorted = sortNames(input); + expect(sorted).toEqual(['A', 'B', 'a', 'a_b', 'b', 'ä']); + expect(sortNames([...input].reverse())).toEqual(sorted); + expect(input[0]).toBe('a_b'); + }); + + it('accepts any iterable and handles empty input', () => { + expect(sortNames(new Set(['y', 'x']))).toEqual(['x', 'y']); + expect(sortNames([])).toEqual([]); + }); +}); + describe('normalizeServerName', () => { it('should not modify server names that already match the pattern', () => { const result = normalizeServerName('valid-server_name.123'); diff --git a/packages/api/src/mcp/utils.ts b/packages/api/src/mcp/utils.ts index 5396dbcc94a..1f7ce49455e 100644 --- a/packages/api/src/mcp/utils.ts +++ b/packages/api/src/mcp/utils.ts @@ -15,6 +15,20 @@ import { isDirectOpenIDBearerRecoveryEnabled } from '~/mcp/openid'; import { ALLOWED_BODY_FIELDS, isPluginSourced } from '~/utils/env'; import { isEnabled } from '~/utils/common'; +/** + * Code-unit ascending order, independent of locale. Provider prompt caches key on the + * `tools` array and system prompt, so MCP servers and tools are materialized in this + * order to keep the prefix stable across selection order and catalog key order. + */ +export function sortNames(names: Iterable): string[] { + return [...names].sort((a, b) => { + if (a === b) { + return 0; + } + return a < b ? -1 : 1; + }); +} + export const mcpToolPattern: RegExp = new RegExp(`^.+${Constants.mcp_delimiter}.+$`); function isMCPServerConfig(config: unknown): config is ParsedServerConfig { diff --git a/packages/api/src/tools/definitions.spec.ts b/packages/api/src/tools/definitions.spec.ts index fa87c3c6e25..8bb1bba8c42 100644 --- a/packages/api/src/tools/definitions.spec.ts +++ b/packages/api/src/tools/definitions.spec.ts @@ -522,6 +522,68 @@ describe('definitions.ts', () => { expect(result.mcpResolution).toEqual({ expectedToolCount: 0, resolvedToolCount: 0 }); }); + it('expands an mcp_all placeholder in tool name order regardless of catalog key order', async () => { + const makeEntry = (name: string) => ({ + function: { + name, + description: name, + parameters: { type: 'object', properties: {} }, + }, + }); + const names = ['zeta_mcp_warehouse', 'alpha_mcp_warehouse', 'mid_mcp_warehouse']; + const load = async (keys: string[]) => { + const catalog = Object.fromEntries(keys.map((k) => [k, makeEntry(k)])); + const result = await loadToolDefinitions( + { userId: 'user-123', agentId: 'agent-123', tools: ['sys__all__sys_mcp_warehouse'] }, + { + getOrFetchMCPServerTools: jest.fn().mockResolvedValue(catalog), + isBuiltInTool: mockIsBuiltInTool, + }, + ); + return result.toolDefinitions.map((d) => d.name); + }; + + const forward = await load(names); + const reversed = await load([...names].reverse()); + expect(forward).toEqual(['alpha_mcp_warehouse', 'mid_mcp_warehouse', 'zeta_mcp_warehouse']); + expect(reversed).toEqual(forward); + }); + + it('keeps server groups in the requested order and sorts tools only inside each placeholder', async () => { + const entry = (name: string) => ({ + function: { name, description: name, parameters: { type: 'object', properties: {} } }, + }); + const catalogs: Record> = { + one: { b_mcp_one: entry('b_mcp_one'), a_mcp_one: entry('a_mcp_one') }, + two: { d_mcp_two: entry('d_mcp_two'), c_mcp_two: entry('c_mcp_two') }, + empty: {}, + }; + const result = await loadToolDefinitions( + { + userId: 'user-123', + agentId: 'agent-123', + tools: [ + 'sys__all__sys_mcp_two', + 'sys__all__sys_mcp_empty', + 'sys__all__sys_mcp_one', + 'z_mcp_one', + ], + }, + { + getOrFetchMCPServerTools: jest + .fn() + .mockImplementation(async (_u: string, server: string) => catalogs[server] ?? null), + isBuiltInTool: mockIsBuiltInTool, + }, + ); + expect(result.toolDefinitions.map((d) => d.name)).toEqual([ + 'c_mcp_two', + 'd_mcp_two', + 'a_mcp_one', + 'b_mcp_one', + ]); + }); + it('refreshes a non-empty server catalog once when the selected tool is missing', async () => { const selectedTool = 'run_query_mcp_warehouse'; const staleTools = { diff --git a/packages/api/src/tools/definitions.ts b/packages/api/src/tools/definitions.ts index ebbcfa3b0f7..1e44a12f266 100644 --- a/packages/api/src/tools/definitions.ts +++ b/packages/api/src/tools/definitions.ts @@ -24,7 +24,7 @@ import { resolveJsonSchemaRefs, normalizeJsonSchema, sanitizeGeminiSchema } from import { buildToolClassification } from './classification'; import { getToolDefinition } from './registry/definitions'; import { toolkitExpansion } from './toolkits/mapping'; -import { isMCPAllPlaceholder } from '~/mcp/utils'; +import { isMCPAllPlaceholder, sortNames } from '~/mcp/utils'; export interface MCPServerTool { function?: { @@ -315,7 +315,8 @@ export async function loadToolDefinitions( } if (isMCPAllPlaceholder(toolName)) { - for (const [actualToolName, toolDef] of Object.entries(serverTools)) { + for (const actualToolName of sortNames(Object.keys(serverTools))) { + const toolDef = serverTools[actualToolName]; if (toolDef?.function) { mcpToolDefs.push( bindToolApprovalIdentity(