mirror of
https://github.com/morluto/rea.git
synced 2026-11-14 10:43:30 +00:00
fix(mcp-doctor): keep the diagnostic child target-free and its stderr visible
The owned launcher merges its host base back under the explicit overrides, so filtering only the override object let an ambient HOPPER_TARGET_* variable through and the diagnostic child could open the target the doctor promises to skip. Pass the filtered environment as the host base too, and opt into the token sweep so a detached descendant that outlives the launcher is still reached by cleanup. Also keep the stderr observer attached until bounded cleanup and draining have settled, report the typed cleanup resources instead of a generic close_binary message, and drop a first-attempt cleanup failure once the owned retry proves release.
This commit is contained in:
1 parent
d427fc05c3
commit
1b019a73d8
4 files changed
+65
-4
No files matched your search
+25
-2
@@ -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<Record<string, unknown>> =>
|
||||
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
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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) {
|
||||
|
||||
+10
@@ -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);
|
||||
Reference in new issue
Block a user