diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a6608fa75..741946003 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -41,6 +41,10 @@ - Fixed eval startup messaging to report `eval` as unavailable when Python is unreachable and JavaScript backend is disabled +### Fixed +- Stabilized MCP tool ordering so reconnects and refreshes no longer reorder the tools array sent to the model. Anthropic prompt caching is keyed on byte-identical tool definitions; previously, the order depended on connection sequence and a single MCP server reconnect could shuffle tools across servers and invalidate the tools cache breakpoint. +- Skipped redundant system-prompt rebuilds in `AgentSession.refreshMCPTools` when the active tool set is unchanged. MCP transport flapping (e.g. routine 5-minute SSE reconnects) used to call `rebuildSystemPrompt` on every reconnect even though the resulting prompt was byte-identical, eating CPU and risking cache misses if the rebuild ever became non-deterministic. The applied-tool signature also covers `customWireName` so a wire-name flip with the rest of the tool metadata constant still forces a rebuild. + ## [14.5.12] - 2026-04-30 ### Breaking Changes diff --git a/packages/coding-agent/src/mcp/manager.ts b/packages/coding-agent/src/mcp/manager.ts index 5cba6d1dc..6c08f9966 100644 --- a/packages/coding-agent/src/mcp/manager.ts +++ b/packages/coding-agent/src/mcp/manager.ts @@ -78,6 +78,21 @@ function delay(ms: number): Promise { return Bun.sleep(ms); } +/** + * Stable, total ordering on MCP tools by name. + * + * Anthropic prompt caching keys on byte-identical tool definitions: any reorder + * of the tools array invalidates the tools cache breakpoint and forces a full + * prefix rebuild on the next request. MCP servers connect/reconnect at arbitrary + * times, so the natural "insertion order" of `#tools` is non-deterministic. + * Sorting after every mutation makes the array bytes independent of connection + * sequence. + */ +export function sortMCPToolsByName(tools: T[]): T[] { + tools.sort((a, b) => (a.name < b.name ? -1 : a.name > b.name ? 1 : 0)); + return tools; +} + export function resolveSubscriptionPostAction( notificationsEnabled: boolean, currentEpoch: number, @@ -459,6 +474,10 @@ export class MCPManager { } } + // Stable sort by name so the order is independent of connection completion. + // See `sortMCPToolsByName` for the cache-stability rationale. + sortMCPToolsByName(allTools); + // Update cached tools this.#tools = allTools; allowBackgroundLogging = true; @@ -474,6 +493,9 @@ export class MCPManager { #replaceServerTools(name: string, tools: CustomTool[]): void { this.#tools = this.#tools.filter(t => !t.name.startsWith(`mcp__${name}_`)); this.#tools.push(...tools); + // Stable sort by name so reconnect order does not perturb the array. + // See `sortMCPToolsByName` for the cache-stability rationale. + sortMCPToolsByName(this.#tools); } #triggerNotificationRefresh(serverName: string, kind: "tools" | "resources" | "prompts"): void { diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index 5c368ef53..f8cb4e7b0 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -428,6 +428,9 @@ function isCustomTool(tool: CustomTool | ToolDefinition): tool is CustomTool { const TOOL_DEFINITION_MARKER = Symbol("__isToolDefinition"); +/** Matches the truncation applied to per-server instructions inside `rebuildSystemPrompt`. */ +const MAX_MCP_INSTRUCTIONS_LENGTH = 4000; + let sshCleanupRegistered = false; async function cleanupSshResources(): Promise { @@ -1337,7 +1340,6 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} const serverInstructions = mcpManager?.getServerInstructions(); let appendPrompt: string | undefined = memoryInstructions ?? undefined; if (serverInstructions && serverInstructions.size > 0) { - const MAX_INSTRUCTIONS_LENGTH = 4000; const parts: string[] = []; if (appendPrompt) parts.push(appendPrompt); parts.push( @@ -1345,8 +1347,8 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} ); for (const [srvName, srvInstructions] of serverInstructions) { const truncated = - srvInstructions.length > MAX_INSTRUCTIONS_LENGTH - ? `${srvInstructions.slice(0, MAX_INSTRUCTIONS_LENGTH)}\n[truncated]` + srvInstructions.length > MAX_MCP_INSTRUCTIONS_LENGTH + ? `${srvInstructions.slice(0, MAX_MCP_INSTRUCTIONS_LENGTH)}\n[truncated]` : srvInstructions; parts.push(`### ${srvName}\n${truncated}`); } @@ -1626,6 +1628,20 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} onResponse, convertToLlm: convertToLlmFinal, rebuildSystemPrompt, + getMcpServerInstructions: mcpManager + ? () => { + const raw = mcpManager.getServerInstructions(); + if (!raw || raw.size === 0) return raw; + const out = new Map(); + for (const [name, text] of raw) { + out.set( + name, + text.length > MAX_MCP_INSTRUCTIONS_LENGTH ? text.slice(0, MAX_MCP_INSTRUCTIONS_LENGTH) : text, + ); + } + return out; + } + : undefined, mcpDiscoveryEnabled, initialSelectedMCPToolNames, defaultSelectedMCPToolNames, diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 5056f51e4..851597c06 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -244,6 +244,13 @@ export interface AgentSessionConfig { convertToLlm?: (messages: AgentMessage[]) => Message[] | Promise; /** System prompt builder that can consider tool availability */ rebuildSystemPrompt?: (toolNames: string[], tools: Map) => Promise; + /** + * Optional accessor for live MCP server instructions. Read by the session's + * `rebuildSystemPrompt`-skip optimization to detect server-side instruction + * changes (e.g. an MCP server upgrade) that would otherwise pass the tool-set + * signature comparison and silently keep a stale prompt cached. + */ + getMcpServerInstructions?: () => Map | undefined; /** Enable hidden-by-default MCP tool discovery for this session. */ mcpDiscoveryEnabled?: boolean; /** MCP tool names to activate for the current session when discovery mode is enabled. */ @@ -511,7 +518,15 @@ export class AgentSession { #onResponse: SimpleStreamOptions["onResponse"] | undefined; #convertToLlm: (messages: AgentMessage[]) => Message[] | Promise; #rebuildSystemPrompt: ((toolNames: string[], tools: Map) => Promise) | undefined; + #getMcpServerInstructions: (() => Map | undefined) | undefined; #baseSystemPrompt: string; + /** + * Signature of the (toolNames, tool descriptions) tuple passed to the most + * recent successful `rebuildSystemPrompt` call. Used to skip redundant rebuilds + * when MCP servers reconnect without changing their tool definitions, which is + * the dominant cause of prompt-cache invalidation in long sessions. + */ + #lastAppliedToolSignature: string | undefined; #mcpDiscoveryEnabled = false; #discoverableMCPTools = new Map(); #discoverableMCPSearchIndex: DiscoverableMCPSearchIndex | null = null; @@ -595,6 +610,7 @@ export class AgentSession { this.#onResponse = config.onResponse; this.#convertToLlm = config.convertToLlm ?? convertToLlm; this.#rebuildSystemPrompt = config.rebuildSystemPrompt; + this.#getMcpServerInstructions = config.getMcpServerInstructions; this.#baseSystemPrompt = this.agent.state.systemPrompt; this.#mcpDiscoveryEnabled = config.mcpDiscoveryEnabled ?? false; this.#setDiscoverableMCPTools(this.#collectDiscoverableMCPToolsFromRegistry()); @@ -2211,10 +2227,18 @@ export class AgentSession { } this.agent.setTools(tools); - // Rebuild base system prompt with new tool set + // Rebuild base system prompt with new tool set, but only when the tool set + // actually changed. MCP servers can reconnect at arbitrary times and call + // `refreshMCPTools` -> `#applyActiveToolsByName` even though the resulting + // tool list is byte-identical. Skipping the rebuild keeps the system prompt + // stable, which is required for Anthropic prompt caching to keep hitting. if (this.#rebuildSystemPrompt) { - this.#baseSystemPrompt = await this.#rebuildSystemPrompt(validToolNames, this.#toolRegistry); - this.agent.setSystemPrompt(this.#baseSystemPrompt); + const signature = this.#computeAppliedToolSignature(validToolNames, tools); + if (signature !== this.#lastAppliedToolSignature) { + this.#baseSystemPrompt = await this.#rebuildSystemPrompt(validToolNames, this.#toolRegistry); + this.agent.setSystemPrompt(this.#baseSystemPrompt); + this.#lastAppliedToolSignature = signature; + } } if (options?.persistMCPSelection !== false) { this.#persistSelectedMCPToolNamesIfChanged(previousSelectedMCPToolNames); @@ -2256,6 +2280,86 @@ export class AgentSession { const activeToolNames = this.getActiveToolNames(); this.#baseSystemPrompt = await this.#rebuildSystemPrompt(activeToolNames, this.#toolRegistry); this.agent.setSystemPrompt(this.#baseSystemPrompt); + // Refresh the cached signature so a subsequent `#applyActiveToolsByName` with + // the same tool set does not re-rebuild on top of the explicit refresh we + // just performed (and conversely, a different set forces a fresh rebuild). + const activeTools = activeToolNames + .map(name => this.#toolRegistry.get(name)) + .filter((tool): tool is AgentTool => tool != null); + this.#lastAppliedToolSignature = this.#computeAppliedToolSignature(activeToolNames, activeTools); + } + + /** + * Compose a stable signature for the inputs that `rebuildSystemPrompt` reads. + * Two calls producing identical signatures are guaranteed to produce identical + * system prompt bytes, so the rebuild can be skipped. + * + * The signature covers: + * 1. Active tool names in order (the prompt renders them in this order). + * 2. Active tool labels, descriptions, and wire-visible names — all are + * rendered into the prompt body (see `system-prompt.md` `{{label}}: \`{{name}}\`` + * and `toolPromptNames` in `buildSystemPrompt`). The wire name comes from + * `tool.customWireName` and overrides the internal name on the model wire + * (e.g. `edit` exposes itself as `apply_patch` to GPT-5 in apply_patch mode); + * a stale wire name would desync prompt guidance from actual tool routing. + * 3. When MCP discovery is on, every registry tool's name+label+description+ + * customWireName, since `rebuildSystemPrompt` summarizes discoverable MCP + * tools that are not in the active set. + * 4. MCP server instructions text (per server), since `rebuildSystemPrompt` + * embeds these in the appended prompt under "## MCP Server Instructions". + * A server upgrade can change instructions while keeping tools identical. + * + * Settings-driven tool metadata is covered automatically: built-in tools that + * depend on settings expose `description`/`label` via getters (see `TaskTool`, + * `SearchToolBm25Tool`, `EditTool`), and the signature reads them live on every + * call - so a settings flip that mutates the rendered string differs the signature + * the next time `#applyActiveToolsByName` runs. Do not refactor `describeTool` to + * cache per-tool strings without preserving this property. + * + * Inputs NOT covered: tool input schemas; memory instructions read from disk; + * and SDK-init-time closure constants in `sdk.ts` (`repeatToolDescriptions`, + * `eagerTasks`, `intentField`, `mcpDiscoveryEnabled`, `secretsEnabled`). The + * closure-captured ones cannot change at runtime regardless of skip behavior. + * For everything else, callers must explicitly call `refreshBaseSystemPrompt()` + * after side-effecting changes; see e.g. the memory hooks and + * `#syncEditToolModeAfterModelChange`. + * + * The current calendar date IS covered (appended as a segment) because + * `buildSystemPrompt` injects it into the prompt body (`Today is '{{date}}'`). + * Without this, a session spanning midnight with only tool-stable MCP + * reconnects would keep yesterday's date indefinitely. + */ + #computeAppliedToolSignature(toolNames: string[], tools: AgentTool[]): string { + // Order-preserving join: any reorder must produce a different signature so + // the rebuild fires and the new tool list reaches the API. + const nameSegment = toolNames.join("\u0001"); + const describeTool = (tool: AgentTool): string => + `${tool.name}=${tool.label ?? ""}|${tool.description ?? ""}|${tool.customWireName ?? ""}`; + const descriptionSegment = tools.map(describeTool).join("\u0002"); + let registrySegment = ""; + if (this.#mcpDiscoveryEnabled) { + // Registry iteration order is not load-bearing for the prompt content, so we + // sort to keep the signature insensitive to incidental insertion order. + const entries: string[] = []; + for (const tool of this.#toolRegistry.values()) { + entries.push(describeTool(tool)); + } + entries.sort(); + registrySegment = entries.join("\u0004"); + } + let instructionsSegment = ""; + const serverInstructions = this.#getMcpServerInstructions?.(); + if (serverInstructions && serverInstructions.size > 0) { + // Sort by server name so transport flap order does not perturb the signature. + const entries: string[] = []; + for (const [server, instructions] of serverInstructions) { + entries.push(`${server}=${instructions}`); + } + entries.sort(); + instructionsSegment = entries.join("\u0006"); + } + const date = new Date().toISOString().slice(0, 10); + return `${nameSegment}\u0003${descriptionSegment}\u0005${registrySegment}\u0007${instructionsSegment}|${date}`; } /** diff --git a/packages/coding-agent/test/agent-session-tool-rebuild-skip.test.ts b/packages/coding-agent/test/agent-session-tool-rebuild-skip.test.ts new file mode 100644 index 000000000..5a8d09e83 --- /dev/null +++ b/packages/coding-agent/test/agent-session-tool-rebuild-skip.test.ts @@ -0,0 +1,463 @@ +import { afterEach, describe, expect, it, setSystemTime } from "bun:test"; +import { Agent, type AgentTool } from "@oh-my-pi/pi-agent-core"; +import type { Model } from "@oh-my-pi/pi-ai"; +import { Type } from "@sinclair/typebox"; +import { Settings } from "../src/config/settings"; +import type { CustomTool } from "../src/extensibility/custom-tools/types"; +import { AgentSession } from "../src/session/agent-session"; +import { SessionManager } from "../src/session/session-manager"; + +// Cache-stability invariant: when MCP servers reconnect with byte-identical tool +// definitions, `refreshMCPTools` must not rebuild the system prompt. A rebuild +// invalidates the Anthropic prompt-cache breakpoint placed on the system block +// and forces a full prefix re-encode on the next request. + +function createModel(): Model<"openai-responses"> { + return { + id: "mock", + name: "mock", + api: "openai-responses", + provider: "openai", + baseUrl: "https://example.invalid", + reasoning: false, + input: ["text"], + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, + contextWindow: 8192, + maxTokens: 2048, + }; +} + +function createBasicTool(name: string, label: string, description = `${label} tool`): AgentTool { + return { + name, + label, + description, + parameters: Type.Object({ value: Type.String() }), + strict: true, + async execute() { + return { content: [{ type: "text", text: `${name} executed` }] }; + }, + }; +} + +function createMcpCustomTool(name: string, serverName: string, mcpToolName: string, description: string): CustomTool { + return { + name, + label: `${serverName}/${mcpToolName}`, + description, + parameters: Type.Object({ q: Type.String() }), + strict: true, + mcpServerName: serverName, + mcpToolName, + async execute() { + return { content: [{ type: "text", text: `${name} executed` }] }; + }, + } as CustomTool; +} + +describe("AgentSession refreshMCPTools rebuild skipping", () => { + const sessions: AgentSession[] = []; + + afterEach(async () => { + for (const session of sessions.splice(0)) { + await session.dispose(); + } + }); + + interface NewSessionOptions { + mcpDiscoveryEnabled?: boolean; + getMcpServerInstructions?: () => Map | undefined; + } + + function newSession( + rebuildSystemPrompt: (toolNames: string[]) => Promise, + options: NewSessionOptions = {}, + ): { + session: AgentSession; + } { + const readTool = createBasicTool("read", "Read"); + const initialMcp = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "Search nucleus"); + const toolRegistry = new Map([ + [readTool.name, readTool], + [initialMcp.name, initialMcp as unknown as AgentTool], + ]); + const agent = new Agent({ + initialState: { + model: createModel(), + systemPrompt: "initial", + tools: [readTool, initialMcp as unknown as AgentTool], + messages: [], + }, + }); + const session = new AgentSession({ + agent, + sessionManager: SessionManager.inMemory(), + settings: Settings.isolated({ "compaction.enabled": false }), + modelRegistry: {} as never, + toolRegistry, + rebuildSystemPrompt, + mcpDiscoveryEnabled: options.mcpDiscoveryEnabled, + getMcpServerInstructions: options.getMcpServerInstructions, + }); + sessions.push(session); + return { session }; + } + + it("skips rebuild when an MCP refresh produces an identical tool set", async () => { + let rebuildCount = 0; + const { session } = newSession(async toolNames => { + rebuildCount++; + return `tools:${toolNames.join(",")}`; + }); + // The session constructor does not run rebuildSystemPrompt; baseline=0. + expect(rebuildCount).toBe(0); + + const initialMcp = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "Search nucleus"); + + // First refresh: no signature recorded yet, must rebuild. + await session.refreshMCPTools([initialMcp]); + expect(rebuildCount).toBe(1); + + // Second refresh with byte-identical metadata: must NOT rebuild. + await session.refreshMCPTools([initialMcp]); + expect(rebuildCount).toBe(1); + + // Third refresh, again identical: still no rebuild. + await session.refreshMCPTools([initialMcp]); + expect(rebuildCount).toBe(1); + }); + + it("rebuilds when an MCP tool's description changes", async () => { + let rebuildCount = 0; + const { session } = newSession(async toolNames => { + rebuildCount++; + return `tools:${toolNames.join(",")}`; + }); + + const v1 = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "Search v1"); + await session.refreshMCPTools([v1]); + expect(rebuildCount).toBe(1); + + const v2 = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "Search v2"); + await session.refreshMCPTools([v2]); + expect(rebuildCount).toBe(2); + }); + + it("rebuilds when the active tool list changes via setActiveToolsByName", async () => { + let rebuildCount = 0; + const { session } = newSession(async toolNames => { + rebuildCount++; + return `tools:${toolNames.join(",")}`; + }); + + const a = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "Search"); + const b = createMcpCustomTool("mcp__nucleus_explain", "nucleus", "explain", "Explain"); + + // Bring both into the registry; only `mcp__nucleus_search` becomes active per + // the agent's initial tool set. + await session.refreshMCPTools([a, b]); + const baseline = rebuildCount; + expect(baseline).toBeGreaterThanOrEqual(1); + + // Activate the previously-inactive tool: the active list grew, signature must + // change, rebuild must fire. + await session.setActiveToolsByName(["read", "mcp__nucleus_search", "mcp__nucleus_explain"]); + expect(rebuildCount).toBe(baseline + 1); + + // Same list again: skip. + await session.setActiveToolsByName(["read", "mcp__nucleus_search", "mcp__nucleus_explain"]); + expect(rebuildCount).toBe(baseline + 1); + + // Drop one: rebuild fires again. + await session.setActiveToolsByName(["read", "mcp__nucleus_search"]); + expect(rebuildCount).toBe(baseline + 2); + }); + + it("does not skip when refreshBaseSystemPrompt is called explicitly", async () => { + let rebuildCount = 0; + const { session } = newSession(async toolNames => { + rebuildCount++; + return `tools:${toolNames.join(",")}`; + }); + + const tool = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "Search"); + await session.refreshMCPTools([tool]); + expect(rebuildCount).toBe(1); + + // Explicit refresh must always rebuild (callers use it to pick up env-side changes + // such as edit mode toggles, which are invisible to our tool signature). + await session.refreshBaseSystemPrompt(); + expect(rebuildCount).toBe(2); + + // Subsequent identical MCP refresh should still skip after the explicit refresh + // freshens the cached signature. + await session.refreshMCPTools([tool]); + expect(rebuildCount).toBe(2); + }); + + it("ignores incidental insertion order in the refresh argument", async () => { + let rebuildCount = 0; + const { session } = newSession(async toolNames => { + rebuildCount++; + return `tools:${toolNames.join(",")}`; + }); + + const a = createMcpCustomTool("mcp__nucleus_a", "nucleus", "a", "A"); + const b = createMcpCustomTool("mcp__nucleus_b", "nucleus", "b", "B"); + + // `refreshMCPTools` re-derives the active tool set from the registry using the + // session's existing ordering, so passing the same content in a different order + // must not mutate the signature. + await session.refreshMCPTools([a, b]); + expect(rebuildCount).toBe(1); + + await session.refreshMCPTools([b, a]); + expect(rebuildCount).toBe(1); + }); + + it("rebuilds when an MCP tool's label changes", async () => { + // Tool labels are rendered into the prompt body (`{{label}}: \`{{name}}\``), + // so a label change \u2014 even with name and description constant \u2014 must force + // a rebuild. Otherwise we'd serve a stale label after an MCP server upgrade. + let rebuildCount = 0; + const { session } = newSession(async toolNames => { + rebuildCount++; + return `tools:${toolNames.join(",")}`; + }); + + const v1 = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "Search"); + // Override the auto-derived label so the test mutates only the label. + const v1WithLabel = { ...v1, label: "old label" } as typeof v1; + await session.refreshMCPTools([v1WithLabel]); + expect(rebuildCount).toBe(1); + + const v2WithLabel = { ...v1, label: "new label" } as typeof v1; + await session.refreshMCPTools([v2WithLabel]); + expect(rebuildCount).toBe(2); + }); + + it("rebuilds when MCP server instructions text changes", async () => { + // `rebuildSystemPrompt` embeds per-server `instructions` text into the appended + // prompt. The signature must include this so a server upgrade that changes + // instructions while keeping tools constant still triggers a rebuild. + let rebuildCount = 0; + const instructions = new Map([["nucleus", "v1 instructions"]]); + const { session } = newSession( + async toolNames => { + rebuildCount++; + return `tools:${toolNames.join(",")}`; + }, + { getMcpServerInstructions: () => instructions }, + ); + + const tool = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "Search"); + await session.refreshMCPTools([tool]); + expect(rebuildCount).toBe(1); + + // Same tools, same instructions: skip. + await session.refreshMCPTools([tool]); + expect(rebuildCount).toBe(1); + + // Mutate the live instructions map (callers return the live reference). + instructions.set("nucleus", "v2 instructions"); + await session.refreshMCPTools([tool]); + expect(rebuildCount).toBe(2); + + // Adding a new server's instructions also triggers rebuild. + instructions.set("glean", "glean instructions"); + await session.refreshMCPTools([tool]); + expect(rebuildCount).toBe(3); + }); + + it("rebuilds when a discoverable (inactive) registry tool's metadata changes", async () => { + // With MCP discovery on, the rebuilt prompt summarizes ALL discoverable MCP + // tools \u2014 including ones not in the active set. The signature must capture the + // full registry; otherwise a description change to a discoverable-but-inactive + // tool would silently leave a stale summary in the cached prompt. + let rebuildCount = 0; + const { session } = newSession( + async toolNames => { + rebuildCount++; + return `tools:${toolNames.join(",")}`; + }, + { mcpDiscoveryEnabled: true }, + ); + + const active = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "Search"); + const discoverable = createMcpCustomTool("mcp__nucleus_explain", "nucleus", "explain", "Explain v1"); + + await session.refreshMCPTools([active, discoverable]); + const baseline = rebuildCount; + expect(baseline).toBeGreaterThanOrEqual(1); + + // Same registry: skip. + await session.refreshMCPTools([active, discoverable]); + expect(rebuildCount).toBe(baseline); + + // Mutate the discoverable (NOT active) tool's description: signature must + // differ via the registrySegment branch and force a rebuild. + const discoverableV2 = createMcpCustomTool("mcp__nucleus_explain", "nucleus", "explain", "Explain v2"); + await session.refreshMCPTools([active, discoverableV2]); + expect(rebuildCount).toBe(baseline + 1); + }); + it("rebuilds when an MCP tool's customWireName changes", async () => { + // `customWireName` overrides the model-facing tool name (e.g. `edit` exposes + // itself as `apply_patch` to GPT-5). The wire name is rendered into the prompt + // body via `toolPromptNames`, so a wire-name flip with the rest of the metadata + // constant would otherwise leave a stale system prompt that advertises the wrong + // callable name to the model. The signature must catch this. + let rebuildCount = 0; + const { session } = newSession(async toolNames => { + rebuildCount++; + return `tools:${toolNames.join(",")}`; + }); + + // Attach a custom wire name to the MCP tool. `applyToolProxy` forwards arbitrary + // properties from the underlying CustomTool to the wrapper, so the AgentTool the + // signature inspects exposes `customWireName` as if it were declared on the type. + const v1 = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "Search"); + const v1WithWire = { ...v1, customWireName: "wire_v1" } as typeof v1 & { customWireName: string }; + await session.refreshMCPTools([v1WithWire]); + expect(rebuildCount).toBe(1); + + // Same wire name: skip. + await session.refreshMCPTools([v1WithWire]); + expect(rebuildCount).toBe(1); + + // Wire name changes while name/label/description stay constant: must rebuild. + const v2WithWire = { ...v1, customWireName: "wire_v2" } as typeof v1 & { customWireName: string }; + await session.refreshMCPTools([v2WithWire]); + expect(rebuildCount).toBe(2); + + // Drop wire name entirely: must rebuild (signature must differ from `wire_v2`). + await session.refreshMCPTools([v1]); + expect(rebuildCount).toBe(3); + }); + + it("rebuilds when a tool's getter-based description reflects new settings state", async () => { + // Built-in tools whose prompt-rendered metadata depends on settings expose + // `description` via getters that re-evaluate on every access (TaskTool reads + // task.disabledAgents/maxConcurrency/isolation.mode/simple/async.enabled, + // SearchToolBm25Tool reads the discoverable MCP tool count, EditTool resolves + // through the current edit-mode definition). The signature reads `tool.description` + // live each call, so a settings flip that mutates the rendered string MUST differ + // the signature on the next `#applyActiveToolsByName`. Defending this contract + // against a future refactor that caches per-tool description strings. + let rebuildCount = 0; + const { session } = newSession( + async toolNames => { + rebuildCount++; + return `tools:${toolNames.join(",")}`; + }, + // Discovery-on so the dynamic tool participates in the registry segment too, + // matching the production pattern where `TaskTool` and friends are always + // reachable via the registry. + { mcpDiscoveryEnabled: true }, + ); + + // Reuse the initially-active MCP name so the tool stays in the active list + // across refreshes - we want to defend the path where `tool.description` is read + // for the active descriptionSegment, not just the registrySegment. + const settingState = { disabled: "none" }; + const dynamicTool = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "placeholder"); + Object.defineProperty(dynamicTool, "description", { + get: () => `dynamic disabled=${settingState.disabled}`, + enumerable: true, + configurable: true, + }); + + await session.refreshMCPTools([dynamicTool]); + const baseline = rebuildCount; + expect(baseline).toBeGreaterThanOrEqual(1); + + // Same underlying state, same tool object identity: skip. + await session.refreshMCPTools([dynamicTool]); + expect(rebuildCount).toBe(baseline); + + // Mutate the settings-backed state. The tool object identity does not change, + // but its `description` getter now returns a new string. The signature must + // pick this up live (no per-tool caching) and force a rebuild. + settingState.disabled = "plan,explore"; + await session.refreshMCPTools([dynamicTool]); + expect(rebuildCount).toBe(baseline + 1); + + // Same state again: skip. + await session.refreshMCPTools([dynamicTool]); + expect(rebuildCount).toBe(baseline + 1); + }); + it("rebuilds when the calendar date rolls over between tool-stable MCP refreshes", async () => { + // `buildSystemPrompt` injects today's date into the prompt body. + // A session spanning midnight must not serve yesterday's date after an MCP + // reconnect that happens to bring an identical tool set. + setSystemTime(new Date("2025-01-01T23:59:58Z")); + try { + let rebuildCount = 0; + const { session } = newSession(async toolNames => { + rebuildCount++; + return `tools:${toolNames.join(",")}`; + }); + const tool = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "Search"); + + // First refresh: no signature yet, must rebuild. + await session.refreshMCPTools([tool]); + expect(rebuildCount).toBe(1); + + // Same tools, same day: signature matches, skip. + await session.refreshMCPTools([tool]); + expect(rebuildCount).toBe(1); + + // Advance past midnight. + setSystemTime(new Date("2025-01-02T00:00:01Z")); + + // Same tools, new calendar day: date segment changed, must rebuild. + await session.refreshMCPTools([tool]); + expect(rebuildCount).toBe(2); + + // Same tools, same new day: skip again. + await session.refreshMCPTools([tool]); + expect(rebuildCount).toBe(2); + } finally { + setSystemTime(); // restore real time + } + }); + it("does not rebuild when MCP server instructions change only beyond the 4000-char truncation boundary", async () => { + // `rebuildSystemPrompt` (sdk.ts) truncates each server instruction to 4000 chars + // before embedding it. The `getMcpServerInstructions` callback must therefore + // return pre-truncated strings so the signature hashes exactly what the prompt + // builder uses. Changes beyond char 4000 cannot affect rendered prompt bytes + // and must NOT trigger a rebuild. + const prefix = "A".repeat(4000); + const instructions = new Map([["nucleus", `${prefix}_tail_v1`]]); + let rebuildCount = 0; + const { session } = newSession( + async toolNames => { + rebuildCount++; + return `tools:${toolNames.join(",")}`; + }, + { + getMcpServerInstructions: () => { + // Mirror what sdk.ts does: truncate to 4000 chars before returning. + const out = new Map(); + for (const [name, text] of instructions) { + out.set(name, text.length > 4000 ? text.slice(0, 4000) : text); + } + return out; + }, + }, + ); + const tool = createMcpCustomTool("mcp__nucleus_search", "nucleus", "search", "Search"); + + await session.refreshMCPTools([tool]); + expect(rebuildCount).toBe(1); + + // Mutate only the text beyond char 4000: truncated string is identical → skip. + instructions.set("nucleus", `${prefix}_tail_v2`); + await session.refreshMCPTools([tool]); + expect(rebuildCount).toBe(1); + + // Mutate within the first 4000 chars: truncated string differs → rebuild. + instructions.set("nucleus", `${"B".repeat(4000)}_tail_v2`); + await session.refreshMCPTools([tool]); + expect(rebuildCount).toBe(2); + }); +}); diff --git a/packages/coding-agent/test/mcp-tool-ordering.test.ts b/packages/coding-agent/test/mcp-tool-ordering.test.ts new file mode 100644 index 000000000..040f1a343 --- /dev/null +++ b/packages/coding-agent/test/mcp-tool-ordering.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it } from "bun:test"; +import { sortMCPToolsByName } from "../src/mcp/manager"; + +// `sortMCPToolsByName` is the cache-stability invariant: Anthropic prompt caching +// keys on byte-identical tool definitions, so the tools array sent to the API +// must be byte-stable across MCP server connect / reconnect / refresh cycles. +// These tests defend that invariant directly because the production call sites +// (initial discovery and `#replaceServerTools`) are heavy to exercise without +// mock transports. + +describe("sortMCPToolsByName", () => { + it("orders tools lexicographically by name", () => { + const tools = [ + { name: "mcp__nucleus_searchCode" }, + { name: "mcp__glean_chat" }, + { name: "mcp__atlassian_get_issue" }, + ]; + sortMCPToolsByName(tools); + expect(tools.map(t => t.name)).toEqual([ + "mcp__atlassian_get_issue", + "mcp__glean_chat", + "mcp__nucleus_searchCode", + ]); + }); + + it("produces identical output regardless of insertion order", () => { + // Simulates the multi-server reconnect bug: depending on which MCP server + // reconnected most recently, the same set of tools could land in different + // orders in `#tools`. After sorting, the array bytes are identical. + const orderA = sortMCPToolsByName([ + { name: "mcp__nucleus_a" }, + { name: "mcp__nucleus_b" }, + { name: "mcp__glean_x" }, + { name: "mcp__glean_y" }, + ]); + const orderB = sortMCPToolsByName([ + { name: "mcp__glean_x" }, + { name: "mcp__glean_y" }, + { name: "mcp__nucleus_a" }, + { name: "mcp__nucleus_b" }, + ]); + expect(orderA.map(t => t.name)).toEqual(orderB.map(t => t.name)); + }); + + it("mutates the input array in place and returns the same reference", () => { + const tools = [{ name: "b" }, { name: "a" }]; + const result = sortMCPToolsByName(tools); + expect(result).toBe(tools); + expect(tools.map(t => t.name)).toEqual(["a", "b"]); + }); + + it("preserves total order under repeated sorts", () => { + // Reconnects re-sort an already-sorted array. The output must be byte-stable + // across repeated sorts so the tools cache breakpoint keeps hitting; ES2019+ + // guarantees a stable sort, and MCP tool names are globally unique within a + // session, so the comparator's strict total order yields one canonical result. + const tools = sortMCPToolsByName([{ name: "c" }, { name: "a" }, { name: "b" }]); + const before = tools.map(t => t.name); + sortMCPToolsByName(tools); + expect(tools.map(t => t.name)).toEqual(before); + }); + + it("handles empty arrays and single-element arrays", () => { + expect(sortMCPToolsByName([])).toEqual([]); + expect(sortMCPToolsByName([{ name: "only" }]).map(t => t.name)).toEqual(["only"]); + }); +});