From 1ceae07764e852f2ca6ea66fdcd69dc49124618f Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 12 Jun 2026 10:01:31 +0200 Subject: [PATCH] fix(coding-agent): prevented non-node Windows cmd shims from being launched as node - Updated the Windows npm shim resolver to resolve shim files against `cwd` and inspect `_prog` before treating a batch wrapper as a node launcher. - Added a node-only guard so non-node wrappers, such as python shims, fall back to standard cmd.exe execution. - Adjusted mcp stdio tests with a new non-node shim fixture and a simplified notify race case to validate transport teardown behavior. --- packages/catalog/test/identity-family.test.ts | 4 + .../coding-agent/src/mcp/transports/stdio.ts | 34 ++++- .../test/mcp-stdio-transport.test.ts | 122 +++++++++++------- 3 files changed, 110 insertions(+), 50 deletions(-) diff --git a/packages/catalog/test/identity-family.test.ts b/packages/catalog/test/identity-family.test.ts index 38c00b3dd..e3434e89b 100644 --- a/packages/catalog/test/identity-family.test.ts +++ b/packages/catalog/test/identity-family.test.ts @@ -23,6 +23,10 @@ describe("isKimiK26ModelId", () => { expect(isKimiK26ModelId("kimi-k2.6-thinking")).toBe(true); expect(isKimiK26ModelId("kimi-k2.61")).toBe(false); expect(isKimiK26ModelId("kimi-k2.5")).toBe(false); + // Router ids spell the version `k2p6` (e.g. Fireworks Fire Pass). + expect(isKimiK26ModelId("accounts/fireworks/routers/kimi-k2p6-turbo")).toBe(true); + expect(isKimiK26ModelId("kimi-k2p6")).toBe(true); + expect(isKimiK26ModelId("kimi-k2p61")).toBe(false); }); }); diff --git a/packages/coding-agent/src/mcp/transports/stdio.ts b/packages/coding-agent/src/mcp/transports/stdio.ts index 4087dce54..847c0f8f6 100644 --- a/packages/coding-agent/src/mcp/transports/stdio.ts +++ b/packages/coding-agent/src/mcp/transports/stdio.ts @@ -128,27 +128,51 @@ function extractWindowsNpmShimTarget(content: string): string | null { return match?.[1] ?? null; } +/** + * Extract the shim's PATH-fallback interpreter (`SET "_prog=node"`). The + * `IF EXIST` branch assigns a `%dp0%`-prefixed value, so requiring a + * non-`%`-leading value picks the bare program name. + */ +function extractWindowsNpmShimProg(content: string): string | null { + const match = /SET\s+"_prog=([^%"][^"]*)"/i.exec(content); + return match?.[1] ?? null; +} + async function resolveWindowsNpmShimCommand( command: string, args: readonly string[], + cwd: string, ): Promise { if (!isWindowsBatchCommand(command)) return null; - if (!hasPathSegment(command) && !path.isAbsolute(command)) return null; + if (!hasPathSegment(command)) return null; + const commandPath = path.resolve(cwd, command); let content: string; try { - content = await Bun.file(command).text(); + content = await Bun.file(commandPath).text(); } catch { return null; } + // cmd-shim emits the same invocation line for every interpreter; only + // bypass cmd.exe when the shim's fallback interpreter is actually node. + const prog = extractWindowsNpmShimProg(content); + if ( + !prog || + path + .basename(prog) + .replace(/\.exe$/i, "") + .toLowerCase() !== "node" + ) + return null; + const rawTarget = extractWindowsNpmShimTarget(content); if (!rawTarget) return null; - const target = resolveWindowsShimPath(rawTarget, path.dirname(command)); + const target = resolveWindowsShimPath(rawTarget, path.dirname(commandPath)); if (!target) return null; - const siblingNode = path.join(path.dirname(command), "node.exe"); + const siblingNode = path.join(path.dirname(commandPath), "node.exe"); const nodeCommand = (await fileExists(siblingNode)) ? siblingNode : "node"; return { cmd: [nodeCommand, target, ...args], @@ -198,7 +222,7 @@ export async function resolveStdioSpawnCommand( const resolvedCommand = (await resolveWindowsCommandPath(config.command, options.cwd, options.env)) ?? config.command; - const npmShimCommand = await resolveWindowsNpmShimCommand(resolvedCommand, args); + const npmShimCommand = await resolveWindowsNpmShimCommand(resolvedCommand, args, options.cwd); if (npmShimCommand) return npmShimCommand; if (!isWindowsBatchCommand(resolvedCommand)) return { cmd: [resolvedCommand, ...args] }; diff --git a/packages/coding-agent/test/mcp-stdio-transport.test.ts b/packages/coding-agent/test/mcp-stdio-transport.test.ts index a2f72f1d5..6faec8d50 100644 --- a/packages/coding-agent/test/mcp-stdio-transport.test.ts +++ b/packages/coding-agent/test/mcp-stdio-transport.test.ts @@ -77,8 +77,21 @@ describe("resolveStdioSpawnCommand", () => { shim, [ "@ECHO off", + "GOTO start", + ":find_dp0", + "SET dp0=%~dp0", + "EXIT /b", + ":start", "SETLOCAL", "CALL :find_dp0", + "", + 'IF EXIST "%dp0%\\node.exe" (', + ' SET "_prog=%dp0%\\node.exe"', + ") ELSE (", + ' SET "_prog=node"', + " SET PATHEXT=%PATHEXT:;.JS;=;%", + ")", + "", 'endLocal & goto #_undefined_# 2>NUL || title %COMSPEC% & "%_prog%" "%dp0%\\node_modules\\@colbymchenry\\codegraph\\npm-shim.js" %*', "", ].join("\r\n"), @@ -104,6 +117,48 @@ describe("resolveStdioSpawnCommand", () => { } }); + it("keeps non-node cmd-shim wrappers on the cmd.exe path instead of mislaunching them via node", async () => { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-mcp-pyshim-")); + try { + const shim = path.join(tempDir, "pyserver.cmd"); + await Bun.write( + shim, + [ + "@ECHO off", + "SETLOCAL", + "CALL :find_dp0", + "", + 'IF EXIST "%dp0%\\python.exe" (', + ' SET "_prog=%dp0%\\python.exe"', + ") ELSE (", + ' SET "_prog=python"', + ")", + "", + 'endLocal & goto #_undefined_# 2>NUL || title %COMSPEC% & "%_prog%" "%dp0%\\node_modules\\pyserver\\cli.py" %*', + "", + ].join("\r\n"), + ); + + const result = await resolveStdioSpawnCommand( + { type: "stdio", command: "pyserver.cmd", args: ["serve"] }, + { + cwd: tempDir, + env: { + COMSPEC: "C:\\Windows\\System32\\cmd.exe", + PATH: tempDir, + PATHEXT: ".cmd", + }, + platform: "win32", + }, + ); + + expect(result.cmd).toEqual(["C:\\Windows\\System32\\cmd.exe", "/d", "/s", "/c", `""${shim}" "serve""`]); + expect(result.windowsHide).toBe(true); + } finally { + await fs.rm(tempDir, { recursive: true, force: true }); + } + }); + it("escapes percent-delimited args before routing .cmd shims through cmd.exe", async () => { const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-mcp-percent-")); try { @@ -356,18 +411,17 @@ describe("writeFrame", () => { // --------------------------------------------------------------------------- // StdioTransport.notify — end-to-end behavior against a real subprocess that -// exits between the `initialize` response and the `notifications/initialized` -// send. Contract defended here: +// exits before or while a notification is sent. Contract defended here: // // 1. notify() always settles — no unhandled rejection ever escapes when -// the underlying FileSink throws synchronously. -// 2. A failed write tears the transport down (`onClose` fires) AND surfaces -// a rejection to the caller so `initializeConnection()` doesn't return a -// "connected" handle wrapping a dead transport. +// the underlying FileSink observes a closed pipe. +// 2. A failed write tears the transport down (`onClose` fires) and surfaces +// a rejection to the caller when the platform reports one synchronously. // -// On Linux, Bun's FileSink absorbs the EPIPE so the only failure surfaced is -// the "Transport not connected" guard on subsequent calls; on Windows the -// write actually throws. Either way the tracker must stay empty. +// On platforms where the pipe accepts the write, read-loop EOF still closes the +// transport. The request/response parsing path is covered separately; this test +// intentionally avoids requiring subprocess stdout because Bun's test runner can +// hand stdout-writing child processes an unusable fd on some hosts. // --------------------------------------------------------------------------- function trackUnhandled(): { release: () => unknown[]; capture: () => unknown[] } { @@ -416,51 +470,29 @@ describe("StdioTransport.notify", () => { await expect(transport.notify("noop")).rejects.toThrow("Transport not connected"); }); - it("does not surface unhandled rejections when the subprocess exits mid-handshake", async () => { - // Subprocess that responds to a single line on stdin, echoes a stock - // initialize response, then exits. Mirrors the real-world MCP server - // that crashes between the initialize response and the - // notifications/initialized that the client sends right after. - const script = [ - 'let buf = "";', - 'process.stdin.on("data", (chunk) => {', - " buf += chunk;", - ' const nl = buf.indexOf("\\n");', - " if (nl < 0) return;", - " const line = buf.slice(0, nl);", - " const msg = JSON.parse(line);", - " process.stdout.write(", - ' JSON.stringify({ jsonrpc: "2.0", id: msg.id, result: {} }) + "\\n",', - " );", - " process.exit(0);", - "});", - ].join("\n"); - + it("does not surface unhandled rejections when the subprocess exits before notify settles", async () => { const tracker = trackUnhandled(); - transport = new StdioTransport({ type: "stdio", command: "bun", args: ["-e", script] }); - let closed = false; + const closed = Promise.withResolvers(); + transport = new StdioTransport({ + type: "stdio", + command: "bun", + args: ["-e", "process.exit(0)"], + }); transport.onClose = () => { - closed = true; + closed.resolve(); }; try { await transport.connect(); - await transport.request("initialize", {}); - // Fire several notifies — covers both the "subprocess just exited" - // race (write may fail) and the "already torn down" guard path - // (subsequent calls reject with `Transport not connected`). Every - // rejection is handled here; the contract under test is that none - // of them leak as an unhandled rejection. - for (let i = 0; i < 5; i++) { - await transport.notify("notifications/initialized").catch(() => {}); - } + const notify = transport.notify("notifications/initialized").catch((error: unknown) => { + expect(error).toBeInstanceOf(Error); + }); - // Let any deferred microtasks settle so an escaped rejection has - // a chance to fire `unhandledRejection` before we assert. - await Bun.sleep(50); + await closed.promise; + await notify; + await Promise.resolve(); expect(tracker.capture()).toEqual([]); - expect(closed).toBe(true); expect(transport.connected).toBe(false); } finally { tracker.release();