From 3cb6e5df9c6967b16fa03efb68efcdaf2a78df65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Marchand?= <4249097+kodlian@users.noreply.github.com> Date: Thu, 30 Jul 2026 21:53:28 +0200 Subject: [PATCH] fix(mcp): carry requestIdFormat through MCP config discovery The option was only present on the transport-facing `MCPServerConfig`, so a value written in `.omp/mcp.json` or a standalone `.mcp.json` never reached the transports: discovery normalizes config into the canonical `MCPServer` shape and `convertToLegacyConfig()` rebuilds the transport config from it, and neither step knew about the field. Setting `"number"` in the documented config path silently kept the snowflake-string default, which is the hang the option exists to avoid. Wire it through the same four places `timeout` already uses: the canonical `MCPServer` shape, the two OMP-native loaders (with validate-and-warn on an unrecognized value), and the legacy conversion. Foreign-format providers are untouched, since the key is OMP-specific. Also point the `MCPServerConfigBase` doc comment at `RequestIdAllocator` rather than a helper name that never existed. --- packages/coding-agent/src/capability/mcp.ts | 4 + .../coding-agent/src/discovery/builtin.ts | 14 ++++ .../coding-agent/src/discovery/mcp-json.ts | 14 ++++ packages/coding-agent/src/mcp/config.ts | 1 + packages/coding-agent/src/mcp/types.ts | 2 +- .../test/mcp/config-request-id-format.test.ts | 79 +++++++++++++++++++ 6 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 packages/coding-agent/test/mcp/config-request-id-format.test.ts diff --git a/packages/coding-agent/src/capability/mcp.ts b/packages/coding-agent/src/capability/mcp.ts index 2a1212464..9ac016a44 100644 --- a/packages/coding-agent/src/capability/mcp.ts +++ b/packages/coding-agent/src/capability/mcp.ts @@ -4,6 +4,8 @@ * Canonical shape for MCP server configurations, regardless of source format. * All providers translate their native format to this shape. */ + +import type { MCPRequestIdFormat } from "../mcp/types"; import { defineCapability } from "."; import type { SourceMeta } from "./types"; @@ -17,6 +19,8 @@ export interface MCPServer { enabled?: boolean; /** Connection timeout in milliseconds */ timeout?: number; + /** Encoding for outgoing JSON-RPC request ids (default: `"string"`) */ + requestIdFormat?: MCPRequestIdFormat; /** Command to run (for stdio transport) */ command?: string; /** Command arguments */ diff --git a/packages/coding-agent/src/discovery/builtin.ts b/packages/coding-agent/src/discovery/builtin.ts index 42abac1d5..3633fa710 100644 --- a/packages/coding-agent/src/discovery/builtin.ts +++ b/packages/coding-agent/src/discovery/builtin.ts @@ -23,6 +23,7 @@ import { type SlashCommand, slashCommandCapability } from "../capability/slash-c import { type SystemPrompt, systemPromptCapability } from "../capability/system-prompt"; import { type CustomTool, toolCapability } from "../capability/tool"; import type { LoadContext, LoadResult } from "../capability/types"; +import type { MCPRequestIdFormat } from "../mcp/types"; import { expandTilde } from "../tools/path-utils"; import { buildRuleFromMarkdown, @@ -154,10 +155,23 @@ async function loadMCPServers(ctx: LoadContext): Promise> timeout = undefined; } + // Validate requestIdFormat: only the two documented encodings + let requestIdFormat: MCPRequestIdFormat | undefined; + if (serverConfig.requestIdFormat !== undefined && serverConfig.requestIdFormat !== null) { + if (serverConfig.requestIdFormat === "string" || serverConfig.requestIdFormat === "number") { + requestIdFormat = serverConfig.requestIdFormat; + } else { + logger.warn( + `MCP server "${serverName}": invalid requestIdFormat ${JSON.stringify(serverConfig.requestIdFormat)}, ignoring`, + ); + } + } + result.push({ name: serverName, enabled, timeout, + requestIdFormat, command: serverConfig.command as string | undefined, args: serverConfig.args as string[] | undefined, env: serverConfig.env as Record | undefined, diff --git a/packages/coding-agent/src/discovery/mcp-json.ts b/packages/coding-agent/src/discovery/mcp-json.ts index fe5de944b..1585f7835 100644 --- a/packages/coding-agent/src/discovery/mcp-json.ts +++ b/packages/coding-agent/src/discovery/mcp-json.ts @@ -26,6 +26,7 @@ interface MCPConfigFile { { enabled?: boolean; timeout?: number; + requestIdFormat?: "string" | "number"; command?: string; args?: string[]; env?: Record; @@ -83,10 +84,23 @@ function transformMCPConfig(config: MCPConfigFile, source: SourceMeta): MCPServe } } + let requestIdFormat: "string" | "number" | undefined; + if (serverConfig.requestIdFormat !== undefined) { + if (serverConfig.requestIdFormat === "string" || serverConfig.requestIdFormat === "number") { + requestIdFormat = serverConfig.requestIdFormat; + } else { + logger.warn("MCP server has invalid 'requestIdFormat' value, ignoring", { + name, + value: serverConfig.requestIdFormat, + }); + } + } + const server: MCPServer = { name, enabled, timeout, + requestIdFormat, command: serverConfig.command, args: serverConfig.args, env: serverConfig.env, diff --git a/packages/coding-agent/src/mcp/config.ts b/packages/coding-agent/src/mcp/config.ts index 852c23619..e24ff6b10 100644 --- a/packages/coding-agent/src/mcp/config.ts +++ b/packages/coding-agent/src/mcp/config.ts @@ -41,6 +41,7 @@ function convertToLegacyConfig(server: MCPServer): MCPServerConfig { const shared = { enabled: server.enabled, timeout: server.timeout, + requestIdFormat: server.requestIdFormat, auth: server.auth, oauth: server.oauth, }; diff --git a/packages/coding-agent/src/mcp/types.ts b/packages/coding-agent/src/mcp/types.ts index 9f873ba6d..2da2467cf 100644 --- a/packages/coding-agent/src/mcp/types.ts +++ b/packages/coding-agent/src/mcp/types.ts @@ -72,7 +72,7 @@ interface MCPServerConfigBase { * Encoding for outgoing JSON-RPC request ids (default: `"string"`). * * Set `"number"` for servers whose decoder accepts integers only, such as - * Apple's `xcrun mcpbridge`. See `createRequestIdAllocator`. + * Apple's `xcrun mcpbridge`. See `RequestIdAllocator` in `./request-id`. */ requestIdFormat?: MCPRequestIdFormat; /** Authentication configuration (optional) */ diff --git a/packages/coding-agent/test/mcp/config-request-id-format.test.ts b/packages/coding-agent/test/mcp/config-request-id-format.test.ts new file mode 100644 index 000000000..9462660c1 --- /dev/null +++ b/packages/coding-agent/test/mcp/config-request-id-format.test.ts @@ -0,0 +1,79 @@ +/** + * `requestIdFormat` must survive the documented config path. + * + * The option is only useful if a value written in config actually reaches the + * transport: discovery parses config into the canonical `MCPServer` shape and + * `convertToLegacyConfig()` turns that back into the `MCPServerConfig` the + * transports read. A field missing from either step silently degrades to the + * snowflake-string default, which is the hang the option exists to avoid. + * + * Both OMP-native loaders are covered: `.omp/mcp.json` (native provider) and a + * standalone project-root `.mcp.json` (mcp-json provider). + */ +import { afterEach, beforeEach, expect, test } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { clearCache as clearFsCache } from "@oh-my-pi/pi-coding-agent/capability/fs"; +import { loadAllMCPConfigs } from "@oh-my-pi/pi-coding-agent/mcp/config"; +import { getConfigRootDir, removeWithRetries, setAgentDir } from "@oh-my-pi/pi-utils"; + +const originalAgentDirEnv = process.env.PI_CODING_AGENT_DIR; +const fallbackAgentDir = path.join(getConfigRootDir(), "agent"); + +let tempAgentDir = ""; +let tempCwd = ""; + +beforeEach(async () => { + tempAgentDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-mcp-reqid-agent-")); + tempCwd = await fs.mkdtemp(path.join(os.tmpdir(), "omp-mcp-reqid-cwd-")); + setAgentDir(tempAgentDir); + clearFsCache(); +}); + +afterEach(async () => { + if (originalAgentDirEnv) { + setAgentDir(originalAgentDirEnv); + } else { + setAgentDir(fallbackAgentDir); + delete process.env.PI_CODING_AGENT_DIR; + } + clearFsCache(); + await removeWithRetries(tempAgentDir); + await removeWithRetries(tempCwd); +}); + +async function loadFrom(file: string, mcpServers: Record) { + await Bun.write(path.join(tempCwd, file), JSON.stringify({ mcpServers })); + clearFsCache(); + const { configs } = await loadAllMCPConfigs(tempCwd); + return configs; +} + +test("requestIdFormat from .omp/mcp.json reaches the transport config", async () => { + const configs = await loadFrom(path.join(".omp", "mcp.json"), { + xcode: { type: "stdio", command: "/usr/bin/xcrun", args: ["mcpbridge"], requestIdFormat: "number" }, + plain: { type: "stdio", command: "/bin/echo" }, + }); + + expect(configs.xcode?.requestIdFormat).toBe("number"); + // Unset stays unset so the allocator keeps its snowflake-string default. + expect(configs.plain?.requestIdFormat).toBeUndefined(); +}); + +test("requestIdFormat from a standalone .mcp.json reaches the transport config", async () => { + const configs = await loadFrom(".mcp.json", { + xcode: { type: "stdio", command: "/usr/bin/xcrun", args: ["mcpbridge"], requestIdFormat: "number" }, + }); + + expect(configs.xcode?.requestIdFormat).toBe("number"); +}); + +test("an unrecognized requestIdFormat is dropped rather than passed through", async () => { + const configs = await loadFrom(path.join(".omp", "mcp.json"), { + bogus: { type: "stdio", command: "/bin/echo", requestIdFormat: "integer" }, + }); + + expect(configs.bogus).toBeDefined(); + expect(configs.bogus?.requestIdFormat).toBeUndefined(); +});