From 998faa41189c6924bdb42d18dc584ec5a8775d71 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sun, 15 Nov 2026 19:38:13 +0800 Subject: [PATCH] refactor(ida): launch stdio registrations through the owned MCP transport (#10739) * refactor(ida): launch stdio registrations through the owned MCP transport IDA stdio servers now run as an owned process group (POSIX) or job (Windows) through OwnedMcpStdioTransport instead of the SDK's unowned child. Windows bare commands resolve against the registration PATH/PATHEXT and .cmd/.bat launchers run through the shared WindowsBatchCommand cmd.exe invocation. The frame limit is classified from the transport's typed OwnedMcpOutputLimitError instead of parsing SDK error text, detach no longer reaches into the SDK's private _process, and the per-operation byte budget is optional. * fix(ida): keep owned stdio cleanup observable and resolve Windows interpreter paths - StructuredContentMcpClient observes SDK-initiated closes like OwnedMcpClient, so a failed handshake with incomplete owned cleanup cannot reject unhandled. - OwnedMcpStdioTransport attaches an owner-redacted captured output to cleanup failures; IDA passes its retained stderr tail. - findWindowsExecutable resolves path-qualified commands at their own location with PATHEXT; IDA routes every Windows registration through it and refuses non-drive paths with an actionable remedy before the owned launcher. * fix(process): end a stalled stdio launch when the owned MCP transport closes OwnedMcpStdioTransport passes the launcher a signal it aborts with OwnerCancellation.ownerClosed when it stops, so IDA's Windows PATH/PATHEXT lookup stalled on an unreachable directory no longer holds close() open; IDA reports the interrupted lookup by its abort reason. The IDA environment tests use createTestTempDirectory for owned temporary roots. * fix(ida): bound the Windows command lookup by the registration timeout The PATH/PATHEXT search for a stdio registration now runs under a ProviderStartupDeadline of the registration's timeoutMs that also follows the transport's close signal. A lookup stalled on an unreachable PATH directory ends as an AnalysisTimeoutError (phase windows_path_lookup) instead of leaving connect() pending, matching Apktool's Windows lookup. * test(ida): stub REA's PATH again now that process inspection runs ps from fixed locations * test(ida): pass the required registration protocol in owned-stdio fixtures * fix(ida): report an uninspectable Windows command candidate with its path and code * refactor(ida): take the host platform from composition, not an ambient read * fix(verify): pass the host platform to the real IDA upstream check * fix(process): keep a failed launch's preparation cleanup owned by the stdio transport's close OwnedMcpStdioTransport discarded the launch rejection before a supervisor existed, so an ownership-helper preparation that transferred a cleanup owner (or reported a cleanup failure without one) was dropped and close() reported success. The transport's close now closes each transferred owner from processInspectionCleanupFailures, and reports a ProviderCleanupError naming the preparation resource until it does; a later close retries it. --- scripts/verify-real-ida.mjs | 4 +- src/composition/binary.ts | 7 +- src/ida/IdaMcpConnection.ts | 219 +++++++++++++++----- src/ida/IdaMcpEnvironment.test.ts | 75 +++++-- src/ida/IdaMcpOwnedCleanup.test.ts | 85 ++++++++ src/ida/IdaMcpWindowsLaunch.test.ts | 199 ++++++++++++++++++ src/process/OwnedMcpStdioTransport.test.ts | 84 ++++++++ src/process/OwnedMcpStdioTransport.ts | 139 +++++++++++-- src/process/WindowsExecutable.test.ts | 17 ++ src/process/WindowsExecutable.ts | 13 +- src/process/executableSearch.ts | 31 ++- src/providers/StructuredContentMcpClient.ts | 24 ++- 12 files changed, 777 insertions(+), 120 deletions(-) create mode 100644 src/ida/IdaMcpOwnedCleanup.test.ts create mode 100644 src/ida/IdaMcpWindowsLaunch.test.ts create mode 100644 src/process/OwnedMcpStdioTransport.test.ts diff --git a/scripts/verify-real-ida.mjs b/scripts/verify-real-ida.mjs index 60331e9ce5..ef77f5bda5 100644 --- a/scripts/verify-real-ida.mjs +++ b/scripts/verify-real-ida.mjs @@ -411,7 +411,9 @@ const mcp = async (cliDossier) => { }; const cleanup = async () => { phase = "upstream lifecycle verification"; - const upstream = createIdaMcpConnection(config, process.env); + const upstream = createIdaMcpConnection(config, process.env, { + platform: process.platform, + }); try { await upstream.connect(); if (config.mode === "attached") { diff --git a/src/composition/binary.ts b/src/composition/binary.ts index d7056f2dc0..c96d161f42 100644 --- a/src/composition/binary.ts +++ b/src/composition/binary.ts @@ -6,6 +6,7 @@ import type { AppConfig } from "../config/types.js"; import type { BinarySession } from "../application/binary/BinarySession.js"; import { HopperProvider } from "../hopper/HopperProvider.js"; import { GhidraProvider } from "../ghidra/GhidraProvider.js"; +import { createIdaMcpConnection } from "../ida/IdaMcpConnection.js"; import { IdaProvider } from "../ida/IdaProvider.js"; import { KunaProvider } from "../kuna/KunaProvider.js"; import type { Logger } from "pino"; @@ -64,7 +65,11 @@ const composeDeepBinarySession = ( const environment = snapshotEnvironment(selectedEnvironment, platform); const hopper = new HopperProvider(config, logger, environment); const ghidra = new GhidraProvider(config, logger, environment); - const ida = new IdaProvider(config, environment); + const ida = new IdaProvider(config, environment, (registration, selected) => + createIdaMcpConnection(registration, selected, { + platform: HOST_PLATFORM.platform, + }), + ); const kuna = new KunaProvider(environment, { regexMatchingDeadline: config.regexMatchingDeadline, regexWorkerHeapRequest: config.regexWorkerHeapRequest, diff --git a/src/ida/IdaMcpConnection.ts b/src/ida/IdaMcpConnection.ts index 4c906f8afd..051c96df66 100644 --- a/src/ida/IdaMcpConnection.ts +++ b/src/ida/IdaMcpConnection.ts @@ -19,9 +19,22 @@ import { } from "./IdaDiagnosticRedactor.js"; import { IdaDiagnosticResourceError } from "./IdaDiagnosticWriter.js"; import { MIMEType } from "node:util"; -import { ChildProcess } from "node:child_process"; -import { Socket } from "node:net"; -import { snapshotEnvironment } from "../process/snapshotEnvironment.js"; +import { + mergeEnvironmentOverrides, + snapshotEnvironment, +} from "../process/snapshotEnvironment.js"; +import { + OwnedMcpOutputLimitError, + OwnedMcpStdioTransport, + type OwnedMcpLauncher, +} from "../process/OwnedMcpStdioTransport.js"; +import { spawnOwnedProviderProcess } from "../process/ProviderProcess.js"; +import { findWindowsExecutable } from "../process/WindowsExecutable.js"; +import { ProviderStartupDeadline } from "../process/ProviderDeadline.js"; +import { + isWindowsBatchFile, + windowsBatchCommand, +} from "../process/WindowsBatchCommand.js"; import { ProtocolError, SdkError, @@ -29,10 +42,6 @@ import { SdkHttpError, StreamableHTTPClientTransport, } from "@modelcontextprotocol/client"; -import { - DEFAULT_INHERITED_ENV_VARS, - StdioClientTransport, -} from "@modelcontextprotocol/client/stdio"; import { z } from "zod"; import { idaRegistrationTimeoutGuidance, @@ -87,6 +96,15 @@ export interface IdaMcpConnection { export type IdaMcpConnectionDependencies = Readonly<{ createDiagnosticRedactor?: typeof createIdaDiagnosticRedactor; + /** + * Host platform that selects the Windows launch resolution. Composition + * passes the running host; omitted, stdio registrations launch directly. + */ + platform?: NodeJS.Platform; + /** Owned process acquisition for a stdio registration. */ + launcher?: OwnedMcpLauncher; + /** Largest stdio JSON-RPC message accepted from the upstream server. */ + maxStdioMessageBytes?: number; }>; const toolResultSchema = mcpToolResultSchema.extend({ @@ -105,6 +123,74 @@ const toolResultSchema = mcpToolResultSchema.extend({ }); const IDA_STDIO_MAX_MESSAGE_BYTES = 64 * 1024 * 1024; +const IDA_STDERR_TAIL_BYTES = 16_384; + +/** + * Windows creates owned processes from an absolute drive-path executable only, + * without appending an extension, and runs `.bat`/`.cmd` launchers only through + * cmd.exe. Resolve the registration command against the child's PATH/PATHEXT + * and route batch launchers through the shared non-AutoRun cmd.exe invocation. + */ +const idaStdioLauncher = + ( + platform: NodeJS.Platform | undefined, + launch: OwnedMcpLauncher, + timeoutMs: number, + ): OwnedMcpLauncher => + async (options) => { + if (platform !== "win32") return launch(options); + const environment = mergeEnvironmentOverrides( + options.hostEnvironment ?? {}, + options.env ?? {}, + "win32", + ); + // The registration timeout bounds the PATH search, and the owned + // transport aborts its signal when it closes, so a lookup that stalls on + // an unreachable PATH directory holds neither connect nor shutdown open. + const deadline = new ProviderStartupDeadline(timeoutMs, options.signal); + const found = await findWindowsExecutable(options.command, environment, { + signal: deadline.signal, + allowBatch: true, + }).finally(() => deadline.dispose()); + if (!found.ok && found.error.kind === "interrupted") + throw deadline.interruption === "timeout" + ? new AnalysisTimeoutError("ida_mcp_connect", timeoutMs, { + cause: found.error, + diagnostics: { phase: "windows_path_lookup" }, + ...idaRegistrationTimeoutGuidance(timeoutMs, "request"), + }) + : (options.signal?.reason ?? + new Error("IDA MCP command lookup was interrupted")); + if (!found.ok) { + const failure = found.error; + // An unreadable candidate is not a missing command: keep the path and + // filesystem code so a denied install is distinguishable from none. + throw new Error( + failure.kind === "invalid_extension" + ? "Windows PATHEXT must contain filename extensions, not paths" + : failure.kind === "inspection" + ? `IDA MCP command candidate ${JSON.stringify(failure.path)} could not be inspected (${failure.code ?? "unknown error"})` + : `IDA MCP command ${JSON.stringify(options.command)} was not found at its path or on the registration PATH/PATHEXT`, + { cause: failure.kind === "inspection" ? failure.cause : failure }, + ); + } + const command = found.value; + // The owned Windows launcher creates processes from drive paths only. + if (!/^[a-z]:\\/iu.test(command)) + throw new Error( + `IDA MCP command ${JSON.stringify(command)} is not on a drive path; map its network share to a drive letter or install the server locally`, + ); + if (!isWindowsBatchFile(command)) return launch({ ...options, command }); + return launch({ + ...options, + ...windowsBatchCommand({ + environment, + batchPath: command, + arguments: options.arguments, + }), + windowsVerbatimArguments: true, + }); + }; const parseIdaTextReply = ( text: string, @@ -487,12 +573,18 @@ export const createIdaMcpConnection = ( operation = "ida_mcp_response", requestTimeoutMs = config.timeoutMs, ): never => { - const bufferLimit = - cause instanceof Error - ? /ReadBuffer exceeded maximum size of (\d+) bytes/u.exec(cause.message) - : null; + // The owned transport records the exceeded frame limit before it closes + // the connection, so the SDK's closed-request error is not the reason. + const observed = stdio?.observedFailure(); + const frameLimit = + observed?.kind === "protocol" && + observed.cause instanceof OwnedMcpOutputLimitError + ? observed.cause.limitBytes + : undefined; if (cause instanceof IdaDiagnosticResourceError) throw cause; if (cause instanceof McpHttpUtf8Error) throw cause; + // The Windows command lookup already classified its own deadline. + if (cause instanceof AnalysisTimeoutError) throw cause; if ( httpUtf8Failure !== undefined && cause instanceof SdkError && @@ -536,21 +628,21 @@ export const createIdaMcpConnection = ( config.timeoutMs, ), }); - if (bufferLimit !== null) + if (frameLimit !== undefined) throw new AnalysisResourceConstraintError( operation, "transport", - `Upstream IDA MCP reply exceeded the ${bufferLimit[1]}-byte transport limit; no partial Evidence was returned.`, - { max_response_bytes: Number(bufferLimit[1]) }, + `Upstream IDA MCP reply exceeded the ${String(frameLimit)}-byte transport limit; no partial Evidence was returned.`, + { max_response_bytes: frameLimit }, { - cause, + cause: observed?.kind === "protocol" ? observed.cause : cause, capturedOutput, - // The upstream MCP SDK owns this buffer; REA cannot raise it. + // REA bounds each stdio message at a fixed size with no runtime override. ...fixedAllowanceOptions( - `The upstream IDA MCP reply allowance of ${bufferLimit[1]} bytes`, + `The upstream IDA MCP reply allowance of ${String(frameLimit)} bytes`, "bytes", { - limit: Number(bufferLimit[1]), + limit: frameLimit, unit: "bytes", narrowing: "Request a smaller result from IDA, such as a narrower address range or fewer items.", @@ -565,30 +657,54 @@ export const createIdaMcpConnection = ( capturedOutput, }); }; - const transport: StdioClientTransport | StreamableHTTPClientTransport = + // Keep a bounded diagnostic tail for genuine failures; never persist ambient variables. + const retainStderr = (chunk: unknown): void => { + const bytes = Buffer.isBuffer(chunk) ? chunk : Buffer.from(String(chunk)); + stderrBytes += bytes.length; + const combined = Buffer.concat([stderr, bytes]); + stderr = Buffer.from( + combined.subarray(Math.max(0, combined.length - IDA_STDERR_TAIL_BYTES)), + ); + }; + const transport: OwnedMcpStdioTransport | StreamableHTTPClientTransport = "command" in config - ? new StdioClientTransport({ - command: config.command, - args: config.args, - env: { - // The SDK always merges ambient defaults. Its string-only environment - // API requires empty values to suppress omitted caller-selected keys. - ...Object.fromEntries( - DEFAULT_INHERITED_ENV_VARS.map((key) => [key, ""]), + ? new OwnedMcpStdioTransport( + { + command: config.command, + arguments: config.args, + // Only the selected snapshot reaches the child, never REA's ambient environment. + hostEnvironment: snapshotEnvironment( + environment, + HOST_PLATFORM.platform, ), - ...Object.fromEntries( - Object.entries( - snapshotEnvironment(environment, HOST_PLATFORM.platform), - ).filter( - (entry): entry is [string, string] => entry[1] !== undefined, - ), - ), - IDA_MCP_MAX_WORKERS: "1", - ...snapshotEnvironment(config.env, HOST_PLATFORM.platform), + env: { + IDA_MCP_MAX_WORKERS: "1", + ...snapshotEnvironment(config.env, HOST_PLATFORM.platform), + }, + // Registrations commonly launch interpreter-driven scripts. + expectedCommand: null, }, - stderr: "pipe", - maxBufferSize: IDA_STDIO_MAX_MESSAGE_BYTES, - }) + { + provider: "ida", + operation: "ida_mcp_close", + label: "IDA MCP", + maxFrameBytes: + dependencies.maxStdioMessageBytes ?? IDA_STDIO_MAX_MESSAGE_BYTES, + // Upstream servers release IDA databases on end of input; keep the + // SDK's two-second grace before the owned group is stopped. + gracefulInputCloseMs: 2_000, + // IDA keeps its own redacted tail; the supervisor retains none. + maxDiagnosticBytes: { stdout: 0, stderr: 0 }, + onStderr: retainStderr, + // Cleanup failures carry the same redacted tail as other transport failures. + capturedOutput: capturedTransportOutput, + }, + idaStdioLauncher( + dependencies.platform, + dependencies.launcher ?? spawnOwnedProviderProcess, + config.timeoutMs, + ), + ) : new StreamableHTTPClientTransport(new URL(config.url), { requestInit: { headers: config.headers, redirect: "error" }, fetch: diagnosticFetch( @@ -602,15 +718,8 @@ export const createIdaMcpConnection = ( }, ), }); - // Keep a bounded diagnostic tail for genuine failures; never persist ambient variables. - if (transport instanceof StdioClientTransport) - transport.stderr?.on("data", (chunk: Buffer) => { - stderrBytes += chunk.length; - const combined = Buffer.concat([stderr, chunk]); - stderr = Buffer.from( - combined.subarray(Math.max(0, combined.length - 16_384)), - ); - }); + const stdio = + transport instanceof OwnedMcpStdioTransport ? transport : undefined; return { async connect() { await client @@ -672,7 +781,11 @@ export const createIdaMcpConnection = ( const failures: unknown[] = []; if (!transportClosed) { try { - await client.close().catch(transportFailure); + // Owned stdio teardown notifies the SDK client itself and reports + // incomplete process cleanup as its own ProviderCleanupError. + if (stdio === undefined) + await client.close().catch(transportFailure); + else await stdio.close(); transportClosed = true; } catch (cause: unknown) { failures.push(cause); @@ -708,13 +821,7 @@ export const createIdaMcpConnection = ( return attempt; }, detach() { - if (!(transport instanceof StdioClientTransport)) return; - // The SDK exposes only the child's pid, not the handles it holds. - const child: unknown = Reflect.get(transport, "_process"); - if (!(child instanceof ChildProcess)) return; - child.unref(); - for (const stream of [child.stdin, child.stdout, child.stderr]) - if (stream instanceof Socket) stream.unref(); + stdio?.detach(); }, }; }; diff --git a/src/ida/IdaMcpEnvironment.test.ts b/src/ida/IdaMcpEnvironment.test.ts index 15a66af875..630220c980 100644 --- a/src/ida/IdaMcpEnvironment.test.ts +++ b/src/ida/IdaMcpEnvironment.test.ts @@ -1,20 +1,13 @@ -import { mkdtemp, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; +import { writeFile } from "node:fs/promises"; import { join } from "node:path"; import { afterEach, expect, it, vi } from "vitest"; -import { DEFAULT_INHERITED_ENV_VARS } from "@modelcontextprotocol/client/stdio"; import { createIdaMcpConnection } from "./IdaMcpConnection.js"; +import { createTestTempDirectory } from "../../tests/fixtures/temporaryDirectory.js"; -const roots: string[] = []; -afterEach(async () => { - vi.unstubAllEnvs(); - for (const root of roots.splice(0)) - await rm(root, { recursive: true, force: true }); -}); +afterEach(() => vi.unstubAllEnvs()); -it("launches the upstream SDK transport with the selected snapshot and suppresses ambient SDK defaults", async () => { - const root = await mkdtemp(join(tmpdir(), "rea-ida-environment-")); - roots.push(root); +it("launches the owned stdio server with only the selected snapshot, never ambient variables", async () => { + const root = await createTestTempDirectory("rea-ida-environment-"); const script = join(root, "producer.mjs"); await writeFile( script, @@ -73,16 +66,60 @@ input.on('line', (line) => { selected: selected === environment ? "selected" : null, registration: "registration", ambient: null, - home: - selected === environment - ? "/selected-home" - : DEFAULT_INHERITED_ENV_VARS.includes("HOME") - ? "" - : null, - path: selected === environment ? "/selected-path" : "", + home: selected === environment ? "/selected-home" : null, + path: selected === environment ? "/selected-path" : null, }); } finally { await connection.close(); } } }); + +it("refuses a stdio reply over the owned frame limit as a fixed transport allowance", async () => { + const root = await createTestTempDirectory("rea-ida-frame-limit-"); + const script = join(root, "producer.mjs"); + await writeFile( + script, + ` +import { createInterface } from 'node:readline'; +const input = createInterface({ input: process.stdin }); +input.on('line', (line) => { + const request = JSON.parse(line); + if (request.id === undefined) return; + const result = request.method === 'initialize' + ? { protocolVersion: request.params.protocolVersion, capabilities: { tools: {} }, serverInfo: { name: 'large-reply', version: '1' } } + : request.method === 'tools/list' + ? { tools: [{ name: 'large', inputSchema: { type: 'object' } }] } + : { content: [], structuredContent: { result: 'x'.repeat(8192) } }; + process.stdout.write(JSON.stringify({ jsonrpc: '2.0', id: request.id, result }) + '\\n'); +}); +`, + ); + const connection = createIdaMcpConnection( + { + command: process.execPath, + args: [script], + env: {}, + protocol: "legacy", + mode: "attached", + timeoutMs: 5000, + }, + {}, + { maxStdioMessageBytes: 4096 }, + ); + try { + expect(await connection.connect()).toEqual(["large"]); + const failure: unknown = await connection.call("large", {}).then( + () => undefined, + (cause: unknown) => cause, + ); + expect(failure).toMatchObject({ + _tag: "AnalysisResourceConstraintError", + operation: "large", + resource: "transport", + reportedLimits: { max_response_bytes: 4096 }, + }); + } finally { + await connection.close(); + } +}); diff --git a/src/ida/IdaMcpOwnedCleanup.test.ts b/src/ida/IdaMcpOwnedCleanup.test.ts new file mode 100644 index 0000000000..1537253649 --- /dev/null +++ b/src/ida/IdaMcpOwnedCleanup.test.ts @@ -0,0 +1,85 @@ +import { EventEmitter } from "node:events"; +import { PassThrough } from "node:stream"; +import { createInterface } from "node:readline"; +import { expect, it } from "vitest"; + +import { ProviderCleanupError } from "../domain/providerCleanupError.js"; +import type { OwnedMcpLauncher } from "../process/OwnedMcpStdioTransport.js"; +import type { ProviderProcessHandle } from "../process/ProviderProcess.js"; +import { createIdaMcpConnection } from "./IdaMcpConnection.js"; + +class FixtureProcess extends EventEmitter implements ProviderProcessHandle { + readonly pid = 1; + readonly stdin = new PassThrough(); + readonly stdout = new PassThrough(); + readonly stderr = new PassThrough(); + readonly exitCode = null; + readonly signalCode = null; + kill(): boolean { + return true; + } +} + +it("keeps a failed handshake's incomplete cleanup observable with the redacted server tail", async () => { + const child = new FixtureProcess(); + let cleanupFails = true; + // The server answers initialize with an unsupported protocol version, so the + // SDK starts close without awaiting it while owned cleanup stays incomplete. + createInterface({ input: child.stdin }).on("line", (line) => { + const request = JSON.parse(line) as { id?: number; method?: string }; + if (request.method !== "initialize") return; + child.stderr.write("server stopped: missing database\n"); + child.stdout.write( + `${JSON.stringify({ + jsonrpc: "2.0", + id: request.id, + result: { + protocolVersion: "1999-01-01", + capabilities: {}, + serverInfo: { name: "old", version: "1" }, + }, + })}\n`, + ); + }); + const launcher: OwnedMcpLauncher = async ({ runId }) => ({ + process: child, + ownership: { + runId, + leaderPid: child.pid, + processGroupId: child.pid, + expectedParentPid: child.pid, + }, + cleanup: async () => { + if (cleanupFails) + return { cleaned: false, reason: "ownership verification unavailable" }; + child.emit("exit", 0, null); + child.emit("close", 0, null); + return { cleaned: true, signaled: false }; + }, + }); + const connection = createIdaMcpConnection( + { + command: "/opt/ida-mcp/server", + args: [], + env: {}, + protocol: "legacy", + mode: "attached", + timeoutMs: 5000, + }, + {}, + { platform: "darwin", launcher }, + ); + await expect(connection.connect()).rejects.toThrow(); + const failure: unknown = await connection.close().then( + () => undefined, + (cause: unknown) => cause, + ); + if (!(failure instanceof ProviderCleanupError)) throw failure; + expect(failure.cleanupIncomplete).toBe(true); + expect(failure.capturedOutput?.stderr).toContain( + "server stopped: missing database", + ); + cleanupFails = false; + await connection.close(); + expect(failure.cleanupIncomplete).toBe(false); +}); diff --git a/src/ida/IdaMcpWindowsLaunch.test.ts b/src/ida/IdaMcpWindowsLaunch.test.ts new file mode 100644 index 0000000000..26c7f4adb3 --- /dev/null +++ b/src/ida/IdaMcpWindowsLaunch.test.ts @@ -0,0 +1,199 @@ +import { beforeEach, expect, it, vi } from "vitest"; + +import { createIdaMcpConnection } from "./IdaMcpConnection.js"; +import { AnalysisTimeoutError } from "../domain/analysisErrorCore.js"; +import type { OwnedMcpLauncher } from "../process/OwnedMcpStdioTransport.js"; +import { waitFor } from "../../tests/support/testWait.js"; + +// The real PATH/PATHEXT lookup runs; only the Windows file system is simulated. +const files = vi.hoisted(() => ({ + present: new Set(), + /** Paths whose stat never settles, like an unreachable network directory. */ + stalled: new Set(), + /** Paths the caller may not inspect. */ + denied: new Set(), + statted: [] as string[], +})); +vi.mock("node:fs/promises", async (original) => ({ + ...(await original()), + stat: async (path: string) => { + files.statted.push(path); + if (files.stalled.has(path)) return new Promise(() => undefined); + if (files.present.has(path)) return { isFile: () => true }; + if (files.denied.has(path)) + throw Object.assign(new Error("denied"), { code: "EACCES" }); + throw Object.assign(new Error("missing"), { code: "ENOENT" }); + }, +})); +beforeEach(() => { + files.present.clear(); + files.stalled.clear(); + files.denied.clear(); + files.statted.length = 0; +}); + +const observeLaunch = async ( + command: string, + environment: NodeJS.ProcessEnv = { SYSTEMROOT: "C:\\Windows" }, +): Promise<{ launches: unknown[]; failure: unknown }> => { + const launches: unknown[] = []; + const launcher: OwnedMcpLauncher = async (options) => { + launches.push(options); + throw new Error("launch observed"); + }; + const connection = createIdaMcpConnection( + { + command, + args: ["--stdio"], + env: {}, + protocol: "legacy", + mode: "attached", + timeoutMs: 5000, + }, + environment, + { platform: "win32", launcher }, + ); + try { + const failure: unknown = await connection.connect().then( + () => undefined, + (cause: unknown) => cause, + ); + return { launches, failure }; + } finally { + await connection.close(); + } +}; + +it("launches a Windows batch registration through the shared cmd.exe invocation", async () => { + files.present.add("C:\\Tools\\ida-mcp.cmd"); + const { launches } = await observeLaunch("C:\\Tools\\ida-mcp.cmd"); + expect(launches).toEqual([ + expect.objectContaining({ + command: "C:\\Windows\\System32\\cmd.exe", + arguments: [ + "/d", + "/e:on", + "/v:off", + "/s", + "/c", + '""C:\\Tools\\ida-mcp.cmd" "--stdio""', + ], + windowsVerbatimArguments: true, + stdin: "pipe", + }), + ]); +}); + +it("appends PATHEXT to an extensionless path-qualified interpreter", async () => { + files.present.add("C:\\Python313\\python.EXE"); + const { launches } = await observeLaunch("C:\\Python313\\python", { + SYSTEMROOT: "C:\\Windows", + PATHEXT: ".COM;.EXE", + }); + expect(launches).toEqual([ + expect.objectContaining({ command: "C:\\Python313\\python.EXE" }), + ]); +}); + +it("refuses a network-share executable before launch with a drive-letter remedy", async () => { + files.present.add("\\\\server\\tools\\python.exe"); + const { launches, failure } = await observeLaunch( + "\\\\server\\tools\\python.exe", + ); + expect(launches).toEqual([]); + expect(String(failure)).toContain("drive letter"); +}); + +it("reports an uninspectable Windows command candidate with its path and code, not as missing", async () => { + files.denied.add("C:\\Tools\\ida-mcp.EXE"); + const { launches, failure } = await observeLaunch("ida-mcp", { + SYSTEMROOT: "C:\\Windows", + PATH: "C:\\Tools", + PATHEXT: ".EXE", + }); + expect(launches).toEqual([]); + expect(String(failure)).toContain( + 'candidate "C:\\\\Tools\\\\ida-mcp.EXE" could not be inspected (EACCES)', + ); + expect(String(failure)).not.toContain("was not found"); +}); + +it("closes while a Windows command lookup is stalled on an unreachable PATH directory", async () => { + files.stalled.add("\\\\offline\\tools\\ida-mcp.EXE"); + const launches: unknown[] = []; + const connection = createIdaMcpConnection( + { + command: "ida-mcp", + args: [], + env: {}, + protocol: "legacy", + mode: "attached", + timeoutMs: 5000, + }, + { + SYSTEMROOT: "C:\\Windows", + PATH: "\\\\offline\\tools", + PATHEXT: ".EXE", + }, + { + platform: "win32", + launcher: async (options) => { + launches.push(options); + throw new Error("launch observed"); + }, + }, + ); + const connecting = connection.connect().then( + () => undefined, + (cause: unknown) => cause, + ); + await waitFor(() => + expect(files.statted).toContain("\\\\offline\\tools\\ida-mcp.EXE"), + ); + // Closing ends the lookup instead of waiting on the stalled directory. + await connection.close(); + await expect(connecting).resolves.toBeDefined(); + expect(launches).toEqual([]); +}); + +it("ends a stalled Windows command lookup at the registration timeout", async () => { + files.stalled.add("\\\\offline\\tools\\ida-mcp.EXE"); + const launches: unknown[] = []; + const connection = createIdaMcpConnection( + { + command: "ida-mcp", + args: [], + env: {}, + protocol: "legacy", + mode: "attached", + timeoutMs: 50, + }, + { + SYSTEMROOT: "C:\\Windows", + PATH: "\\\\offline\\tools", + PATHEXT: ".EXE", + }, + { + platform: "win32", + launcher: async (options) => { + launches.push(options); + throw new Error("launch observed"); + }, + }, + ); + try { + const failure: unknown = await connection.connect().then( + () => undefined, + (cause: unknown) => cause, + ); + expect(failure).toBeInstanceOf(AnalysisTimeoutError); + expect(failure).toMatchObject({ + operation: "ida_mcp_connect", + timeoutMs: 50, + diagnostics: { phase: "windows_path_lookup" }, + }); + expect(launches).toEqual([]); + } finally { + await connection.close(); + } +}); diff --git a/src/process/OwnedMcpStdioTransport.test.ts b/src/process/OwnedMcpStdioTransport.test.ts new file mode 100644 index 0000000000..65f15fbabf --- /dev/null +++ b/src/process/OwnedMcpStdioTransport.test.ts @@ -0,0 +1,84 @@ +import { expect, it } from "vitest"; + +import { ProviderCleanupError } from "../domain/providerCleanupError.js"; +import { DarwinProcessOwnershipInspectionError } from "./DarwinProcessRunTokenReader.js"; +import { + OwnedMcpStdioTransport, + type OwnedMcpLauncher, +} from "./OwnedMcpStdioTransport.js"; + +const transport = (launcher: OwnedMcpLauncher) => + new OwnedMcpStdioTransport( + { command: "/fixture/server", arguments: [] }, + { provider: "fixture", label: "Fixture MCP", maxFrameBytes: 1024 }, + launcher, + ); + +it("keeps a failed launch's transferred preparation cleanup until a close releases it", async () => { + let closeFails = true; + let closes = 0; + const preparation = new DarwinProcessOwnershipInspectionError( + "ownership-helper preparation was cancelled", + { + cleanupOwner: { + close: async () => { + closes += 1; + if (closeFails) throw new Error("helper directory still in use"); + }, + }, + }, + ); + const owned = transport(async () => { + throw preparation; + }); + await expect(owned.start()).rejects.toBe(preparation); + + const first = await owned.close().then( + () => undefined, + (cause: unknown) => cause, + ); + expect(first).toBeInstanceOf(ProviderCleanupError); + const failure = first as ProviderCleanupError; + expect(failure.cleanupIncomplete).toBe(true); + expect(failure.cleanupResources).toEqual(["process ownership inspection"]); + expect(failure.diagnostics).toMatchObject({ + reason: "helper directory still in use", + }); + + closeFails = false; + await expect(owned.close()).resolves.toBeUndefined(); + expect(failure.cleanupIncomplete).toBe(false); + // A released owner is not closed again. + await expect(owned.close()).resolves.toBeUndefined(); + expect(closes).toBe(2); +}); + +it("reports a failed launch's preparation cleanup failure that names no owner", async () => { + const owned = transport(async () => { + throw new DarwinProcessOwnershipInspectionError( + "ownership-helper preparation failed", + { + cleanupFailure: new ProviderCleanupError("fixture", ["helper root"], { + reason: "unremovable", + }), + }, + ); + }); + await expect(owned.start()).rejects.toBeInstanceOf( + DarwinProcessOwnershipInspectionError, + ); + const failure = await owned.close().then( + () => undefined, + (cause: unknown) => cause, + ); + expect(failure).toBeInstanceOf(ProviderCleanupError); + expect((failure as ProviderCleanupError).cleanupIncomplete).toBe(true); +}); + +it("closes cleanly after a launch failure that transferred no cleanup", async () => { + const owned = transport(async () => { + throw new Error("command not found"); + }); + await expect(owned.start()).rejects.toThrow("command not found"); + await expect(owned.close()).resolves.toBeUndefined(); +}); diff --git a/src/process/OwnedMcpStdioTransport.ts b/src/process/OwnedMcpStdioTransport.ts index 0be6c4582f..baaa4fa134 100644 --- a/src/process/OwnedMcpStdioTransport.ts +++ b/src/process/OwnedMcpStdioTransport.ts @@ -1,3 +1,4 @@ +import { OwnerCancellation } from "../domain/analysisErrorCore.js"; import { deserializeMessage, serializeMessage, @@ -17,6 +18,11 @@ import { spawnOwnedProviderProcess, } from "./ProviderProcess.js"; import { cleanupOwnedProcessGroup } from "./ProcessOwnership.js"; +import { DarwinProcessOwnershipInspectionError } from "./DarwinProcessRunTokenReader.js"; +import { + processInspectionCleanupFailures, + type ProcessInspectionCleanupFailure, +} from "./ProcessInspectionCleanup.js"; import { ProviderCleanupError } from "../domain/providerCleanupError.js"; import { AnalysisCleanupLease, @@ -35,6 +41,18 @@ export type OwnedMcpTransportFailure = | { readonly kind: "protocol"; readonly cause: Error } | { readonly kind: "interrupted"; readonly reason: unknown }; +/** A frame or per-operation byte budget the protocol process exceeded. */ +export class OwnedMcpOutputLimitError extends Error { + constructor( + readonly scope: "frame" | "operation", + readonly limitBytes: number, + message: string, + ) { + super(message); + this.name = "OwnedMcpOutputLimitError"; + } +} + /** SDK-framed stdio with REA-owned process cleanup rather than an unowned child. */ export class OwnedMcpStdioTransport implements Transport { onclose: Transport["onclose"]; @@ -53,6 +71,10 @@ export class OwnedMcpStdioTransport implements Transport { [this.runId], ); #starting: Promise | undefined; + /** Preparation cleanup owners a failed launch transferred and that closed. */ + readonly #releasedPreparations = new Set(); + /** Ends a launch still resolving or spawning when the transport closes. */ + readonly #acquisition = new AbortController(); #failure: OwnedMcpTransportFailure | undefined; constructor( @@ -63,11 +85,14 @@ export class OwnedMcpStdioTransport implements Transport { readonly operation?: string; readonly label: string; readonly maxFrameBytes: number; - readonly maxOperationBytes: number; + /** Bytes one serialized operation may produce; omit when only frames are bounded. */ + readonly maxOperationBytes?: number; readonly gracefulInputCloseMs?: number; readonly maxDiagnosticBytes?: ProcessDiagnosticBudget; /** Observe the owned child's stderr from acquisition onward, including when it closes early. */ readonly onStderr?: (chunk: unknown) => void; + /** Owner-redacted output attached to cleanup failures when the owner retains its own tail. */ + readonly capturedOutput?: () => AnalysisCapturedOutput; readonly notifyOnExit?: boolean; }, readonly launcher: OwnedMcpLauncher = spawnOwnedProviderProcess, @@ -90,6 +115,10 @@ export class OwnedMcpStdioTransport implements Transport { ...this.options, runId: this.runId, stdin: "pipe", + signal: + this.options.signal === undefined + ? this.#acquisition.signal + : AbortSignal.any([this.options.signal, this.#acquisition.signal]), }); this.#spawned = spawned; this.#supervisor = new ProviderProcessSupervisor( @@ -186,6 +215,14 @@ export class OwnedMcpStdioTransport implements Transport { return this.#failure; } + /** + * Stop the retained process from holding the host's event loop after its + * cleanup failed; a later close restores the references for the retry. + */ + detach(): void { + this.#supervisor?.unref(); + } + /** Scope both the byte budget and diagnostic retention to the next serialized request. */ beginOperation(): void { this.#bytes = 0; @@ -195,8 +232,15 @@ export class OwnedMcpStdioTransport implements Transport { async #stop(): Promise { this.#frameBuffer = Buffer.alloc(0); this.#frameLength = 0; - // A cancellation may race acquisition. Join it before inspecting ownership. - await this.#starting?.catch(() => undefined); + // A cancellation may race acquisition: end a launch that is still + // resolving its command, then join it before inspecting ownership. + this.#acquisition.abort(OwnerCancellation.ownerClosed); + // A failed launch may transfer ownership-preparation cleanup (a helper + // directory or file handle it could not remove); this close owns it. + const startFailure = await this.#starting?.then( + () => undefined, + (cause: unknown) => cause, + ); const process = this.#spawned?.process; process?.stdout?.off("data", this.#read); process?.stdout?.off("error", this.#error); @@ -235,13 +279,59 @@ export class OwnedMcpStdioTransport implements Transport { reason: stopped.reason, stderr: this.diagnostics()?.stderr ?? "", }, - this.settings.operation === undefined - ? undefined - : { operation: this.settings.operation }, + { + ...(this.settings.operation === undefined + ? {} + : { operation: this.settings.operation }), + ...(this.settings.capturedOutput === undefined + ? {} + : { capturedOutput: this.settings.capturedOutput() }), + }, ); new AnalysisCleanupLeaseGroup(failure, [this.#cleanup]); throw failure; } + for (const preparation of processInspectionCleanupFailures(startFailure)) + await this.#releasePreparation(preparation); + } + + /** + * Close one transferred preparation owner. Until it closes (or when the + * launch reported a failure without an owner) the close fails and a later + * close retries it. + */ + async #releasePreparation( + preparation: ProcessInspectionCleanupFailure, + ): Promise { + const darwin = preparation instanceof DarwinProcessOwnershipInspectionError; + const owner = darwin ? preparation.cleanupOwner : preparation.owner; + if (owner !== undefined && this.#releasedPreparations.has(owner)) return; + let reason = preparation.message; + if (owner !== undefined) + try { + await owner.close(); + this.#releasedPreparations.add(owner); + return; + } catch (cause: unknown) { + reason = cause instanceof Error ? cause.message : String(cause); + } + const failure = new ProviderCleanupError( + this.settings.provider, + [darwin ? "process ownership inspection" : preparation.path], + { reason }, + { + cause: preparation, + ...(this.settings.operation === undefined + ? {} + : { operation: this.settings.operation }), + }, + ); + // The transport's close retries the owner; the failure keeps naming the + // preparation resource rather than the process run. + new AnalysisCleanupLeaseGroup(failure, [this.#cleanup], { + preserveResourceAttribution: true, + }); + throw failure; } readonly #error = (error: Error): void => { @@ -260,26 +350,27 @@ export class OwnedMcpStdioTransport implements Transport { if (this.settings.notifyOnExit !== true) this.onclose?.(); }; readonly #countStderr = (chunk: Buffer | string): void => { - this.#bytes += Buffer.byteLength(chunk); - if (this.#bytes > this.settings.maxOperationBytes) - this.#fail( - new Error( - `${this.settings.label} operation output exceeds ${this.settings.maxOperationBytes} bytes; no complete result is available`, - ), - ); + this.#countOperationBytes(Buffer.byteLength(chunk)); }; + /** Charge output to the operation budget; false once it is exceeded. */ + #countOperationBytes(bytes: number): boolean { + const limit = this.settings.maxOperationBytes; + if (limit === undefined) return true; + this.#bytes += bytes; + if (this.#bytes <= limit) return true; + this.#fail( + new OwnedMcpOutputLimitError( + "operation", + limit, + `${this.settings.label} operation output exceeds ${limit} bytes; no complete result is available`, + ), + ); + return false; + } readonly #read = (chunk: Buffer | string): void => { if (this.#closed) return; const bytes = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); - this.#bytes += bytes.length; - if (this.#bytes > this.settings.maxOperationBytes) { - this.#fail( - new Error( - `${this.settings.label} operation output exceeds ${this.settings.maxOperationBytes} bytes; no complete result is available`, - ), - ); - return; - } + if (!this.#countOperationBytes(bytes.length)) return; try { let offset = 0; while (offset < bytes.length) { @@ -288,7 +379,9 @@ export class OwnedMcpStdioTransport implements Transport { const fragment = bytes.subarray(offset, end); const frameBytes = this.#frameLength + fragment.length; if (frameBytes > this.settings.maxFrameBytes) - throw new Error( + throw new OwnedMcpOutputLimitError( + "frame", + this.settings.maxFrameBytes, `${this.settings.label} MCP frame exceeds ${this.settings.maxFrameBytes} bytes`, ); this.#appendFrame(fragment, frameBytes); diff --git a/src/process/WindowsExecutable.test.ts b/src/process/WindowsExecutable.test.ts index a98cbde47a..4389d2df65 100644 --- a/src/process/WindowsExecutable.test.ts +++ b/src/process/WindowsExecutable.test.ts @@ -114,3 +114,20 @@ it("stops a pending filesystem lookup when its owner aborts", async () => { complete({ isFile: () => true }); expect(lookup.stat).toHaveBeenCalledTimes(1); }); + +it("resolves a path-qualified command at its own location with PATHEXT, never on PATH", async () => { + lookup.stat.mockImplementation(async (path: string) => ({ + isFile: () => path === "C:\\project\\.venv\\Scripts\\python.EXE", + })); + expect( + await findWindowsExecutable( + ".venv\\Scripts\\python", + { PATH: "C:\\tools", PATHEXT: ".COM;.EXE" }, + options(), + ), + ).toEqual({ ok: true, value: "C:\\project\\.venv\\Scripts\\python.EXE" }); + expect(lookup.stat.mock.calls.map(([path]) => path)).toEqual([ + "C:\\project\\.venv\\Scripts\\python.COM", + "C:\\project\\.venv\\Scripts\\python.EXE", + ]); +}); diff --git a/src/process/WindowsExecutable.ts b/src/process/WindowsExecutable.ts index 777f876b2b..d964578365 100644 --- a/src/process/WindowsExecutable.ts +++ b/src/process/WindowsExecutable.ts @@ -16,10 +16,15 @@ export type WindowsExecutableFailure = }; /** - * Resolve a default bare command against PATH/PATHEXT only, probing the - * search owner's candidates (src/process/executableSearch) in order. Never return a bare - * name on failure: handing it to Windows would restart implicit CWD lookup. - * Callers own the lookup deadline and translation to their operation errors. + * Resolve a command the way Windows users name it: a bare name against + * PATH/PATHEXT only, and a path-qualified name (`C:\Python\python`, + * `.venv\Scripts\python`) at that location with the same PATHEXT suffixes, + * probing the search owner's candidates (src/process/executableSearch) in + * order. Owned launches pass an explicit application name to + * `CreateProcessW`, which never appends an extension itself. Never return a + * bare name on failure: handing it to Windows would restart implicit CWD + * lookup. Callers own the lookup deadline and translation to their operation + * errors. */ export const findWindowsExecutable = async ( executable: string, diff --git a/src/process/executableSearch.ts b/src/process/executableSearch.ts index 3ea455a2d4..f4cce40662 100644 --- a/src/process/executableSearch.ts +++ b/src/process/executableSearch.ts @@ -115,10 +115,12 @@ export interface WindowsPathSearchFailure { } /** - * The files Windows' PATH/PATHEXT search tries for bare commands, in order: - * for each PATH entry (relative ones resolve against `cwd`, or against this - * process's directory, read only when such an entry is reached and skipped - * once removed), each command with every PATHEXT extension, + * The files Windows' PATH/PATHEXT search tries for commands, in order: a + * path-qualified command (`C:\Python\python`, `.venv\Scripts\python`) only at + * its own location, resolved against `cwd`, and never on PATH; then for each + * PATH entry (relative ones resolve against `cwd`, or against this process's + * directory, read only when such an entry is reached and skipped once + * removed), each bare command with every PATHEXT extension, * or as written when it already names an extension. Only `.com` and `.exe` * images are searched unless `allowBatch`; a `.bat` or `.cmd` launcher must * then be started through src/process/WindowsBatchCommand. An invalid @@ -147,16 +149,33 @@ export const windowsPathCandidates = ( const runnable = (extension: string) => search.allowBatch === true || WINDOWS_IMAGE_EXTENSIONS.has(extension.toLowerCase()); - const names = listed.flatMap((command) => { + const spelled = (command: string): string[] => { const explicit = win32.extname(command); if (explicit !== "") return runnable(explicit) ? [command] : []; return extensions .filter(runnable) .map((extension) => `${command}${extension}`); - }); + }; + const qualified = (command: string): boolean => + /[\\/]/u.test(command) || win32.parse(command).root !== ""; + const located = listed.filter(qualified).flatMap(spelled); + const names = listed + .filter((command) => !qualified(command)) + .flatMap(spelled); const entries = windowsPathEntries(selected.PATH ?? ""); return ok({ *[Symbol.iterator]() { + for (const name of located) { + const workingDirectory = win32.isAbsolute(name) + ? name + : (search.cwd ?? currentDirectory()); + if (workingDirectory === undefined) continue; + yield { + path: win32.resolve(workingDirectory, name), + relativeEntry: false, + }; + } + if (names.length === 0) return; for (const directory of entries) { // Only an entry without a drive or UNC root reads the working // directory; a removed one holds nothing, and later entries count. diff --git a/src/providers/StructuredContentMcpClient.ts b/src/providers/StructuredContentMcpClient.ts index 6a0d6f6aa0..4ecf67ecf7 100644 --- a/src/providers/StructuredContentMcpClient.ts +++ b/src/providers/StructuredContentMcpClient.ts @@ -1,20 +1,24 @@ -import { - Client, - type CallToolResult, - type Request, - type RequestMethod, - type RequestOptions, - type ResultTypeMap, - type StandardSchemaV1, +import type { + CallToolResult, + Request, + RequestMethod, + RequestOptions, + ResultTypeMap, + StandardSchemaV1, } from "@modelcontextprotocol/client"; +import { OwnedMcpClient } from "../process/OwnedMcpClient.js"; const isSchema = ( value: RequestOptions | StandardSchemaV1 | undefined, ): value is StandardSchemaV1 => typeof value === "object" && value !== null && "~standard" in value; -/** Preserve observed structured dictionaries across the SDK's era-specific result validation. */ -export class StructuredContentMcpClient extends Client { +/** + * Preserve observed structured dictionaries across the SDK's era-specific + * result validation. Owned transports can reject close with a cleanup failure, + * so SDK-initiated closes stay observed as in every owned MCP client. + */ +export class StructuredContentMcpClient extends OwnedMcpClient { readonly #toolResult: StandardSchemaV1 = { "~standard": { version: 1,