diff --git a/src/mcpDoctor.ts b/src/mcpDoctor.ts index 60cd3a9b72..6e73dec136 100644 --- a/src/mcpDoctor.ts +++ b/src/mcpDoctor.ts @@ -26,6 +26,7 @@ import { import { contractsForToolFamilies } from "./contracts/toolFamilies.js"; import { promptsForToolFamilies } from "./contracts/promptToolFamilyScope.js"; import { err, ok } from "./domain/result.js"; +import { AnalysisError } from "./domain/analysisErrorBase.js"; import { PRODUCT_IDENTITY } from "./identity.js"; import { parseMcpStartupBudgets, @@ -120,12 +121,18 @@ export const runProductionMcpDoctor = async (options: McpDoctorOptions) => { Math.max(1, Math.ceil(deadline - performance.now())), ); const stderr = captureStderr(); + // The launcher merges its host base back under the explicit overrides, so the + // filtered environment must also be the base; otherwise an ambient target + // variable returns and the child opens the target the doctor promised to skip. + const targetFree = targetFreeEnvironment(options.environment); const transport = new DoctorStdioTransport( { command: options.command, arguments: [...options.args], ...(options.cwd === undefined ? {} : { cwd: options.cwd }), - env: targetFreeEnvironment(options.environment), + hostEnvironment: targetFree, + env: targetFree, + sweepTokenOwnedProcesses: true, }, stderr.append, ); @@ -171,6 +178,9 @@ export const runProductionMcpDoctor = async (options: McpDoctorOptions) => { // An interrupted connect can clear the client's transport reference // before its startup child is closed. The transport retains that child. await transport.close(); + // The transport reports only after it proved the owned family is gone, so + // an earlier SDK close that merely left cleanup to this retry is resolved. + cleanupFailure = undefined; } catch (cause: unknown) { cleanupFailure = { cause: @@ -192,7 +202,7 @@ export const runProductionMcpDoctor = async (options: McpDoctorOptions) => { { name: "cleanup", ok: false, - detail: `MCP stdio cleanup failed: ${errorMessage(cleanupFailure.cause)}`, + detail: cleanupFailureDetail(cleanupFailure.cause), }, ], }; @@ -679,6 +689,19 @@ const isRecord = (value: unknown): value is Readonly> => const errorMessage = (cause: unknown): string => cause instanceof Error ? cause.message : String(cause); +/** Keep the typed cleanup ownership facts a generic message would discard. */ +const cleanupFailureDetail = (cause: unknown): string => { + const detail = `MCP stdio cleanup failed: ${errorMessage(cause)}`; + if (!(cause instanceof AnalysisError) || !cause.cleanupIncomplete) return detail; + const resources = cause.cleanupResources; + if (resources.length === 0) return detail; + const reason = + isRecord(cause.diagnostics) && typeof cause.diagnostics.reason === "string" + ? ` (${cause.diagnostics.reason})` + : ""; + return `${detail}${reason}; retained resources: ${resources.join(", ")}`; +}; + /** * Name the applied doctor deadline when an owned abort or SDK request timeout * fires. Remote failure text is not a deadline. Codex's 30 second startup diff --git a/src/process/OwnedMcpStdioTransport.ts b/src/process/OwnedMcpStdioTransport.ts index 0e66a90d20..9d2029a420 100644 --- a/src/process/OwnedMcpStdioTransport.ts +++ b/src/process/OwnedMcpStdioTransport.ts @@ -199,8 +199,6 @@ export class OwnedMcpStdioTransport implements Transport { process?.stdout?.off("data", this.#read); process?.stdout?.off("error", this.#error); process?.stderr?.off("data", this.#countStderr); - if (this.settings.onStderr !== undefined) - process?.stderr?.off("data", this.settings.onStderr); process?.off("error", this.#error); process?.off("exit", this.#exit); process?.off("close", this.#outputClosed); @@ -222,6 +220,10 @@ export class OwnedMcpStdioTransport implements Transport { input?.destroy(); await inputClosed; input?.off("error", this.#error); + // Detach the stderr observer only after bounded cleanup and draining have + // settled, so shutdown diagnostics still reach the caller's report. + if (this.settings.onStderr !== undefined) + process?.stderr?.off("data", this.settings.onStderr); this.onclose?.(); if (stopped?.status === "incomplete") { const failure = new ProviderCleanupError( diff --git a/tests/boundary/mcp/mcpDoctor.test.ts b/tests/boundary/mcp/mcpDoctor.test.ts index 5b4299e58c..32c5133a37 100644 --- a/tests/boundary/mcp/mcpDoctor.test.ts +++ b/tests/boundary/mcp/mcpDoctor.test.ts @@ -678,6 +678,32 @@ describe("production MCP doctor child lifecycle", () => { }); }); + it("does not hand an ambient target variable to the diagnostic child", async () => { + const root = await createTestTempDirectory("rea-mcp-doctor-target-free-"); + const seenPath = join(root, "env.json"); + const result = await runProductionMcpDoctor({ + command: process.execPath, + args: [resolve("tests/fixtures/mcpDoctorEnvironment.mjs")], + cwd: root, + environment: { + ...process.env, + HOPPER_TARGET_PATH: "/tmp/ambient-target", + HOPPER_TARGET_KIND: "hop", + REA_MCP_DOCTOR_SEEN_PATH: seenPath, + REA_MCP_DOCTOR_DEADLINE_MS: "5000", + }, + }); + expect(result.healthy).toBe(false); + const seen = JSON.parse(await readFile(seenPath, "utf8")) as Record< + string, + unknown + >; + expect(seen).not.toHaveProperty("HOPPER_TARGET_PATH"); + expect(seen).not.toHaveProperty("HOPPER_TARGET_KIND"); + // The rest of the caller's environment still reaches the child. + expect(seen.REA_MCP_DOCTOR_SEEN_PATH).toBe(seenPath); + }, 10_000); + it("retains only the stderr prefix while draining and counting the full child stream", async () => { const result = await runNoisyDoctorChild(` for (let index = 0; index < 64; index += 1) { diff --git a/tests/fixtures/mcpDoctorEnvironment.mjs b/tests/fixtures/mcpDoctorEnvironment.mjs new file mode 100644 index 0000000000..3ae197285e --- /dev/null +++ b/tests/fixtures/mcpDoctorEnvironment.mjs @@ -0,0 +1,10 @@ +import { writeFile } from "node:fs/promises"; + +// Publish the environment the doctor handed this child, then exit before any +// protocol work so the doctor reports the startup failure under test. +if (process.env.REA_MCP_DOCTOR_SEEN_PATH !== undefined) + await writeFile( + process.env.REA_MCP_DOCTOR_SEEN_PATH, + JSON.stringify(process.env), + ); +process.exit(0);