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,