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.
This commit is contained in:
@@ -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 */
|
||||
|
||||
@@ -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<LoadResult<MCPServer>>
|
||||
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<string, string> | undefined,
|
||||
|
||||
@@ -26,6 +26,7 @@ interface MCPConfigFile {
|
||||
{
|
||||
enabled?: boolean;
|
||||
timeout?: number;
|
||||
requestIdFormat?: "string" | "number";
|
||||
command?: string;
|
||||
args?: string[];
|
||||
env?: Record<string, string>;
|
||||
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
|
||||
@@ -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) */
|
||||
|
||||
@@ -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<string, unknown>) {
|
||||
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();
|
||||
});
|
||||
Reference in New Issue
Block a user