From b29db895c5c1b30b560a39fb9e4664508f1683de Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Wed, 30 Sep 2026 18:39:38 +0200 Subject: [PATCH] fix(coding-agent): align MCP tool names with codemode identifiers MCP tool and namespace names now replace - with _ like Codex, so the pi tool name is also the codemode identifier. Tools of a server whose names collide all get a hash suffix, and server names that differ only in - and _ are rejected. closes #10239 --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/docs/mcp.md | 2 +- .../src/core/extensions/loader.ts | 7 ++- packages/coding-agent/src/core/mcp-servers.ts | 5 ++ .../coding-agent/src/extensions/mcp/config.ts | 13 ++++- .../coding-agent/src/extensions/mcp/index.ts | 35 +++++++------ .../coding-agent/src/extensions/mcp/tools.ts | 9 ++-- .../coding-agent/test/mcp-extension.test.ts | 12 ++++- .../test/suite/agent-session-mcp.test.ts | 51 +++++++++++++++---- 9 files changed, 101 insertions(+), 34 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index b9d744f1c..f625369ca 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -24,6 +24,7 @@ - Fixed extension commands registered without a string name or handler crashing pi when typing `/`; the extension now fails to load with an error instead ([#10054](https://github.com/earendil-works/pi/issues/10054)). - Fixed collapsed `codemode` and MCP tool results filling the screen when the output is one long line, such as minified JSON. Like bash output, the preview is now limited to wrapped lines instead of logical lines. - Fixed `codemode.mode: "only"` listing `read`, `bash`, `edit`, and `write` in the system prompt's tool list although requests only declare `codemode` ([#10192](https://github.com/earendil-works/pi/issues/10192)). +- Fixed codemode scripts calling the wrong MCP tool when two tool names differ only in `-` and `_`, such as `read-file` and `read_file`. Like in Codex, MCP tool and namespace names now replace `-` with `_` (`mcp__my-server__x` is now `mcp__my_server__x`), colliding tools of a server all get a hash suffix, and server names that differ only in `-` and `_` are rejected ([#10239](https://github.com/earendil-works/pi/issues/10239)). ## [0.99.1] - 2026-09-29 diff --git a/packages/coding-agent/docs/mcp.md b/packages/coding-agent/docs/mcp.md index 073481d55..327cacfb4 100644 --- a/packages/coding-agent/docs/mcp.md +++ b/packages/coding-agent/docs/mcp.md @@ -64,7 +64,7 @@ Keep personal servers and servers with credentials in the user-level file. Use t ### Configuration rules -- Server names may contain only letters, digits, `_`, and `-`. Tools are named `mcp____`. +- Server names may contain only letters, digits, `_`, and `-`. Tools are named `mcp____`, with every character other than letters, digits, and `_` replaced by `_`; tools of a server whose names then collide all get a hash suffix. Server names that differ only in `-` and `_` count as the same server: a second one is rejected, and a `mcp.json` server overrides a registered one. - `type` is optional. A `command` selects stdio and a `url` selects streamable HTTP. When present, `type` must be `stdio`, `http`, or `streamable-http`. - `sse` is rejected. Servers that document an SSE endpoint often also provide streamable HTTP, commonly at `/mcp` instead of `/sse`. - `command` is one executable and `args` contains its arguments. It is not a shell command string. diff --git a/packages/coding-agent/src/core/extensions/loader.ts b/packages/coding-agent/src/core/extensions/loader.ts index 82511f913..a35adbb86 100644 --- a/packages/coding-agent/src/core/extensions/loader.ts +++ b/packages/coding-agent/src/core/extensions/loader.ts @@ -15,7 +15,7 @@ import { resolvePath } from "../../utils/paths.ts"; import { createEventBus, type EventBus } from "../event-bus.ts"; import type { ExecOptions } from "../exec.ts"; import { execCommand } from "../exec.ts"; -import { type McpServerConfig, McpServerRegistry, validateMcpServerConfig } from "../mcp-servers.ts"; +import { type McpServerConfig, McpServerRegistry, mcpNamespace, validateMcpServerConfig } from "../mcp-servers.ts"; import { readPiManifest } from "../pi-manifest.ts"; import { createSyntheticSourceInfo, getSyntheticPathSource, isSyntheticPath } from "../source-info.ts"; import { time } from "../timings.ts"; @@ -471,6 +471,11 @@ function createExtensionAPI( if (owner !== undefined && owner !== extension.path) { throw new Error(`MCP server "${name}" is already registered by extension "${owner}"`); } + // Names that differ only in `-` and `_` would share a namespace. + const clash = runtime.mcpServers + .list() + .find((server) => server.name !== name && mcpNamespace(server.name) === mcpNamespace(name)); + if (clash) throw new Error(`MCP server "${name}" conflicts with registered server "${clash.name}"`); const server = { name, config: structuredClone(validated), extensionPath: extension.path }; applyRuntimeChange(() => runtime.mcpServers.register(server)); }, diff --git a/packages/coding-agent/src/core/mcp-servers.ts b/packages/coding-agent/src/core/mcp-servers.ts index 70e1b190b..858d8f2fa 100644 --- a/packages/coding-agent/src/core/mcp-servers.ts +++ b/packages/coding-agent/src/core/mcp-servers.ts @@ -99,6 +99,11 @@ export type McpServerConfig = McpStdioServerConfig | McpHttpServerConfig; const SERVER_NAME = /^[A-Za-z0-9_-]+$/; +/** Namespace of a server's tools: `mcp__` with `-` replaced by `_`, like the tool names. */ +export function mcpNamespace(server: string): string { + return `mcp__${server.replace(/-/g, "_")}`; +} + function isRecord(value: unknown): value is Record { return typeof value === "object" && value !== null && !Array.isArray(value); } diff --git a/packages/coding-agent/src/extensions/mcp/config.ts b/packages/coding-agent/src/extensions/mcp/config.ts index b2da3cd6f..b1b685779 100644 --- a/packages/coding-agent/src/extensions/mcp/config.ts +++ b/packages/coding-agent/src/extensions/mcp/config.ts @@ -25,7 +25,12 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { CONFIG_DIR_NAME } from "../../config.ts"; -import { type McpExposure, type McpServerConfig, validateMcpServerConfig } from "../../core/mcp-servers.ts"; +import { + type McpExposure, + type McpServerConfig, + mcpNamespace, + validateMcpServerConfig, +} from "../../core/mcp-servers.ts"; export type { McpExposure, @@ -87,6 +92,12 @@ function readConfigFile(path: string, scope: "global" | "project", state: McpCon errors.push(`${path}: ${config}`); continue; } + // Names that differ only in `-` and `_` would share a namespace. + const clash = [...servers.keys()].find((other) => other !== name && mcpNamespace(other) === mcpNamespace(name)); + if (clash) { + errors.push(`${path}: server "${name}" conflicts with "${clash}"`); + continue; + } servers.set(name, { name, config, source: path, scope }); } } diff --git a/packages/coding-agent/src/extensions/mcp/index.ts b/packages/coding-agent/src/extensions/mcp/index.ts index 07abfadce..14b81a82c 100644 --- a/packages/coding-agent/src/extensions/mcp/index.ts +++ b/packages/coding-agent/src/extensions/mcp/index.ts @@ -27,7 +27,6 @@ */ import { join, resolve } from "node:path"; -import { toCodemodeIdentifier } from "@earendil-works/pi-codemode/declarations"; import type { SelectItem } from "@earendil-works/pi-tui"; import type { TSchema } from "typebox"; import { getAgentDir } from "../../config.ts"; @@ -38,6 +37,7 @@ import type { ExtensionFactory, ToolDefinition, } from "../../core/extensions/types.ts"; +import { mcpNamespace } from "../../core/mcp-servers.ts"; import { openBrowser } from "../../utils/open-browser.ts"; import { CODEMODE_TOOL_NAME, isCodemodeTool } from "../codemode/tool.ts"; import { isToolSearchTool, TOOL_SEARCH_TOOL_NAME } from "../tool-search/tool.ts"; @@ -144,10 +144,6 @@ function hasIndirectTools(entry: McpServerEntry): boolean { return exposures.has("codemode") || exposures.has("deferred"); } -function namespaceName(server: string): string { - return `mcp__${server}`; -} - /** Name of the system prompt section that lists the servers whose tools are not declared. */ export const MCP_SERVERS_SECTION = "mcp_servers"; /** Characters of a server description in the section, as Codex allows for deferred namespaces. */ @@ -191,7 +187,7 @@ export function renderServersSection(servers: readonly McpServerListing[]): stri const heads = listed.map((server) => { const exposures = configuredExposures(server.entry); const reach = exposures.has("codemode") ? "codemode" : "tool_search"; - return `- ${namespaceName(server.entry.name)} (${reach})`; + return `- ${mcpNamespace(server.entry.name)} (${reach})`; }); const omitted = (count: number) => count > 0 ? [`- … ${count} more server${count === 1 ? "" : "s"}; find their tools with searchTools()`] : []; @@ -213,14 +209,12 @@ export function renderServersSection(servers: readonly McpServerListing[]): stri } /** - * Whether a codemode script needs the server: it names the server's namespace, as written or as the - * script identifier codemode derives from it, or searches, enumerates, or describes tools or - * namespaces, which may name the server in other forms. + * Whether a codemode script needs the server: it names the server's namespace, or searches, + * enumerates, or describes tools or namespaces, which may name the server in other forms. */ function scriptNeedsServer(code: string, server: string): boolean { if (/\b(searchTools|describeNamespace|describeTool|ALL_TOOLS)\b/.test(code)) return true; - const name = namespaceName(server); - return code.includes(name) || code.includes(toCodemodeIdentifier(name)); + return code.includes(mcpNamespace(server)); } /** Short state for lists and the startup report. `withError` appends the first line of a failure. */ @@ -317,9 +311,11 @@ export function createMcpExtension(options: McpExtensionOptions = {}): Extension const registered: McpServer[] = []; const overriddenNames: string[] = []; for (const { name, config, extensionPath } of pi.getMcpServers()) { - const configured = configuredEntries.find((entry) => entry.name === name); + const configured = configuredEntries.find((entry) => mcpNamespace(entry.name) === mcpNamespace(name)); if (configured) { - overriddenNames.push(`"${name}" registered by ${extensionPath} is overridden by ${configured.source}`); + overriddenNames.push( + `"${name}" registered by ${extensionPath} is overridden by "${configured.name}" in ${configured.source}`, + ); continue; } registered.push({ @@ -352,16 +348,25 @@ export function createMcpExtension(options: McpExtensionOptions = {}): Extension const entry = findServer(server)?.entry ?? connection.entry; const description = entry.config.description?.trim(); const namespace = { - name: namespaceName(server), + name: mcpNamespace(server), ...(description ? { description } : {}), ...(connection.instructions ? { instructions: connection.instructions } : {}), }; const previous = serverTools.get(server) ?? new Set(); const current = new Set(); + // Like Codex, all tools whose names sanitize to the same name get the hash suffix, so which + // one would keep the plain name does not depend on the order of the list. + const plain = [...new Set(connection.tools.map((tool) => tool.name))].map((tool) => + createMcpToolName(server, tool), + ); const assignName = (tool: string, owner: string) => { const name = createMcpToolName(server, tool, (candidate) => { const existing = toolOwners.get(candidate); - return (existing !== undefined && existing !== owner) || current.has(candidate); + return ( + (existing !== undefined && existing !== owner) || + current.has(candidate) || + plain.indexOf(candidate) !== plain.lastIndexOf(candidate) + ); }); toolOwners.set(name, owner); current.add(name); diff --git a/packages/coding-agent/src/extensions/mcp/tools.ts b/packages/coding-agent/src/extensions/mcp/tools.ts index b8d4296d2..979a7330c 100644 --- a/packages/coding-agent/src/extensions/mcp/tools.ts +++ b/packages/coding-agent/src/extensions/mcp/tools.ts @@ -74,16 +74,17 @@ export interface McpToolCaller { } /** - * `mcp____`, sanitized and shortened with a hash suffix when too long. `isTaken` - * reports names already used by a different MCP tool: sanitizing can map two tools to one name - * (`a.b` and `a_b`), and the second then gets the hash suffix too. + * `mcp____`, sanitized and shortened with a hash suffix when too long. Like Codex, + * everything but `[A-Za-z0-9_]` becomes `_`, so the name is also the identifier codemode scripts + * call it by. `isTaken` reports names used by a different MCP tool: sanitizing can map two tools to + * one name (`a-b` and `a_b`), which then get the hash suffix. */ export function createMcpToolName( server: string, tool: string, isTaken: (name: string) => boolean = () => false, ): string { - const name = `mcp__${server}__${tool}`.replace(/[^A-Za-z0-9_-]/g, "_"); + const name = `mcp__${server}__${tool}`.replace(/[^A-Za-z0-9_]/g, "_"); if (name.length <= MAX_TOOL_NAME_LENGTH && !isTaken(name)) return name; const hash = createHash("sha256").update(`${server}\0${tool}`).digest("hex").slice(0, 8); return `${name.slice(0, MAX_TOOL_NAME_LENGTH - hash.length - 1)}_${hash}`; diff --git a/packages/coding-agent/test/mcp-extension.test.ts b/packages/coding-agent/test/mcp-extension.test.ts index dda0a1919..4afc5a909 100644 --- a/packages/coding-agent/test/mcp-extension.test.ts +++ b/packages/coding-agent/test/mcp-extension.test.ts @@ -78,6 +78,14 @@ describe("MCP config", () => { expect(untrusted.servers.find((server) => server.name === "shared")?.config).toEqual({ command: "global-cmd" }); }); + // Regression: #10239. + it("rejects server names that differ only in - and _", () => { + const paths = setup({ mcpServers: { "work-files": { command: "a" }, work_files: { command: "b" } } }, {}); + const { servers, errors } = loadMcpConfig({ ...paths, projectTrusted: false }); + expect(servers.map((server) => server.name)).toEqual(["work-files"]); + expect(errors).toEqual([expect.stringContaining('server "work_files" conflicts with "work-files"')]); + }); + it("validates exposure and reads autoEnableCodemode with project precedence", () => { const paths = setup( { @@ -176,14 +184,14 @@ describe("MCP config", () => { describe("MCP tools", () => { it("creates provider-safe tool names", () => { expect(createMcpToolName("docs", "search")).toBe("mcp__docs__search"); - expect(createMcpToolName("my-server", "get.item/v2")).toBe("mcp__my-server__get_item_v2"); + expect(createMcpToolName("my-server", "get.item/v2")).toBe("mcp__my_server__get_item_v2"); const long = createMcpToolName("server", "x".repeat(100)); expect(long).toHaveLength(64); expect(long).toMatch(/^mcp__server__x+_[0-9a-f]{8}$/); expect(createMcpToolName("server", `${"x".repeat(100)}y`)).not.toBe(long); // Names that sanitize to one already taken by another tool get a hash suffix. const taken = createMcpToolName("s", "a_b"); - const second = createMcpToolName("s", "a.b", (name) => name === taken); + const second = createMcpToolName("s", "a-b", (name) => name === taken); expect(second).toMatch(/^mcp__s__a_b_[0-9a-f]{8}$/); }); diff --git a/packages/coding-agent/test/suite/agent-session-mcp.test.ts b/packages/coding-agent/test/suite/agent-session-mcp.test.ts index f127e496e..0e2bb894f 100644 --- a/packages/coding-agent/test/suite/agent-session-mcp.test.ts +++ b/packages/coding-agent/test/suite/agent-session-mcp.test.ts @@ -283,6 +283,25 @@ describe("AgentSession MCP integration", () => { expect(nestedToolNames(harness)).toContain(searchName); }); + // Regression: #10239. + it.each([false, true])("routes tools whose names differ only in - and _ (reverse: %s)", async (reverse) => { + const tools = [ + { name: "read-file", description: "dashed", inputSchema: { type: "object", properties: {} } }, + { name: "read_file", description: "underscored", inputSchema: { type: "object", properties: {} } }, + ]; + if (reverse) tools.reverse(); + const { harness, calls } = await setup("codemode", () => [...tools, ...SERVER_TOOLS]); + const code = `for (const query of ["dashed", "underscored"]) await tools[(await searchTools(query))[0].name]({});`; + harness.setResponses([ + fauxAssistantMessage([fauxToolCall("codemode", { code })], { stopReason: "toolUse" }), + fauxAssistantMessage("done"), + ]); + + await harness.session.prompt("go"); + + expect(calls).toEqual(["read-file:{}", "read_file:{}"]); + }); + it("rejects direct model calls to codemode-only MCP tools", async () => { const { harness, calls } = await setup("codemode"); harness.setResponses([ @@ -696,7 +715,7 @@ return { docs, sameForAliases: aliases.every((alias) => JSON.stringify(alias) == await harness.session.prompt("go"); expect(toolResult(harness, "codemode").isError).toBe(false); - expect(getMessageText(toolResult(harness, "codemode")).split("\n").at(-1)).toBe("mcp__slow-docs"); + expect(getMessageText(toolResult(harness, "codemode")).split("\n").at(-1)).toBe("mcp__slow_docs"); expect(calls).toEqual(['search:{"query":"q"}']); }); @@ -913,11 +932,16 @@ describe("AgentSession MCP servers registered by extensions", () => { await vi.waitFor(() => expect(harness.session.getCallableToolNames()).not.toContain("mcp__late__search")); }); - it("prefers the mcp.json server over a registered server of the same name", async () => { - const configured: McpServerEntry = { name: "docs", config: { url: "http://config.invalid" }, source: "mcp.json" }; + // "my_docs" shares the namespace of "my-docs" (#10239). + it.each(["my-docs", "my_docs"])("prefers the mcp.json server over a registered %s", async (name) => { + const configured: McpServerEntry = { + name: "my-docs", + config: { url: "http://config.invalid" }, + source: "mcp.json", + }; const { connected } = await setup( (pi) => { - pi.registerMcpServer("docs", { url: "http://plugin.invalid" }); + pi.registerMcpServer(name, { url: "http://plugin.invalid" }); }, [configured], ); @@ -927,22 +951,29 @@ describe("AgentSession MCP servers registered by extensions", () => { }); it("rejects names another extension registered", async () => { - let error: unknown; + const errors: string[] = []; await setup([ (pi) => { pi.registerMcpServer("taken", { url: "http://x.invalid" }); // Registering again replaces the extension's own registration. pi.registerMcpServer("taken", { url: "http://y.invalid" }); + pi.registerMcpServer("my-server", { url: "http://x.invalid" }); }, (pi) => { - try { - pi.registerMcpServer("taken", { url: "http://z.invalid" }); - } catch (caught) { - error = caught; + for (const name of ["taken", "my_server"]) { + try { + pi.registerMcpServer(name, { url: "http://z.invalid" }); + } catch (caught) { + errors.push(String(caught)); + } } }, ]); - expect(String(error)).toMatch(/MCP server "taken" is already registered by extension/); + expect(errors).toEqual([ + expect.stringMatching(/MCP server "taken" is already registered by extension/), + // Names that differ only in - and _ share a namespace (#10239). + 'Error: MCP server "my_server" conflicts with registered server "my-server"', + ]); }); it("reports registered servers when no extension connects them", async () => {