diff --git a/docs/testing.md b/docs/testing.md index 1ab5184aad..7dc7705ca1 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -146,6 +146,17 @@ the fixture cache. These subprocesses are outside aggregate Vitest coverage; subprocess coverage attachment remains disabled. Cold-start import checks launch independently without this fixture cache. +`analyzeJavaScriptDirectory` stdio MCP cases in one `cliTest` file share one +server and that compile cache. The first case pays startup; later cases reuse +the process. File teardown waits for startup still in flight, closes a server +registered during that wait, and does not open a server after close. A failed +handshake stays the caller error when its cleanup also fails, including a +rejection without a reason. That cleanup stays owned until a later close or +call releases it. Another server does not open while the cleanup remains, and +a cleanup that already succeeded is not run again. Pass +`mcpCall` when a case needs its own server, environment, or lifecycle. CLI +cases still start a fresh process. + The Apple artifact verifier step shares a job-temporary bytecode cache among its fresh CLI/MCP children via `REA_ARTIFACT_NODE_COMPILE_CACHE`. The artifact helper maps this setting to child `NODE_COMPILE_CACHE`; npm, Turbo, and nested diff --git a/tests/boundary/cli/sharedJavaScriptMcpSession.test.ts b/tests/boundary/cli/sharedJavaScriptMcpSession.test.ts new file mode 100644 index 0000000000..ce281a7431 --- /dev/null +++ b/tests/boundary/cli/sharedJavaScriptMcpSession.test.ts @@ -0,0 +1,264 @@ +import { expect, test } from "vitest"; + +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { + cliTest, + createSharedMcpSession, + type SharedMcpCall, + type TestCli, +} from "../../support/cli/cliFixture.js"; +import { + analyzeJavaScriptDirectory, + resolveJavaScriptApplicationMcpCall, +} from "../../support/cli/javascriptApplication.js"; +import { writeFixtureFiles } from "../../support/workspace/writeFixtureFiles.js"; + +const unusedCli = { + run: async () => { + throw new Error("CLI was not requested"); + }, +} as TestCli; + +test("prefers an explicit JavaScript MCP caller over the file session", async () => { + const explicit = (async () => ({ + isError: false, + })) as unknown as SharedMcpCall; + let sharedCalls = 0; + const resolved = await resolveJavaScriptApplicationMcpCall( + { + ...unusedCli, + sharedMcpCall: async () => { + sharedCalls += 1; + return explicit; + }, + }, + explicit, + ); + expect(resolved).toBe(explicit); + expect(sharedCalls).toBe(0); +}); + +test("uses the file session when the case does not own a server", async () => { + const shared = (async () => ({ isError: false })) as unknown as SharedMcpCall; + const resolved = await resolveJavaScriptApplicationMcpCall( + { + ...unusedCli, + sharedMcpCall: async () => shared, + }, + undefined, + ); + expect(resolved).toBe(shared); +}); + +const writeApplication = async (prefix: string): Promise => { + const root = await createTestTempDirectory(prefix); + await writeFixtureFiles(root, { + "package.json": JSON.stringify({ main: "main.cjs" }), + "main.cjs": + 'const { BrowserWindow } = require("electron"); new BrowserWindow().loadFile("index.html");', + "index.html": "page", + }); + return root; +}; + +let fileSession: SharedMcpCall | undefined; + +/** Record the first selected case and compare later cases when they also run. */ +const useFileSession = async (cli: TestCli): Promise => { + const open = cli.sharedMcpCall; + if (open === undefined) + throw new Error("cliTest did not provide a shared MCP session"); + const current = await open(); + if (fileSession === undefined) fileSession = current; + else expect(current).toBe(fileSession); + return current; +}; + +const held = (): { gate: Promise; release: () => void } => { + let release: () => void = () => {}; + const gate = new Promise((resolve) => { + release = resolve; + }); + return { gate, release }; +}; + +const fakeCall = (async () => ({ + isError: false, +})) as unknown as SharedMcpCall; + +test("does not open a shared MCP server after file cleanup starts", async () => { + const startup = held(); + let opened = false; + const session = createSharedMcpSession(async (_registerCleanup, isClosed) => { + await startup.gate; + if (isClosed()) throw new Error("Shared MCP session is closed"); + opened = true; + return fakeCall; + }); + const opening = session.session.call(); + const closing = session.close(); + startup.release(); + await closing; + await expect(opening).rejects.toThrow("Shared MCP session is closed"); + expect(opened).toBe(false); + await expect(session.session.call()).rejects.toThrow( + "Shared MCP session is closed", + ); +}); + +test("keeps a failed MCP handshake when its cleanup also fails", async () => { + const startup = new Error("handshake failed"); + const cleanup = new Error("teardown failed"); + let released = 0; + let retained = 0; + const session = createSharedMcpSession(async (registerCleanup) => { + registerCleanup(async () => { + retained += 1; + if (retained === 1) throw cleanup; + }); + registerCleanup(async () => { + released += 1; + }); + throw startup; + }); + const caught = await session.session.call().then( + () => { + throw new Error("startup succeeded"); + }, + (error: unknown) => error, + ); + expect(caught).toBeInstanceOf(Error); + if (!(caught instanceof Error)) throw new Error("expected startup failure"); + expect(caught.message).toBe(startup.message); + expect(caught.cause).toBeInstanceOf(AggregateError); + const cause = caught.cause as AggregateError; + expect(cause.cause).toBe(startup); + expect([...cause.errors]).toEqual([startup, cleanup]); + expect(released).toBe(1); + expect(retained).toBe(1); + await session.close(); + expect(released).toBe(1); + expect(retained).toBe(2); +}); + +test("retries a reasonless MCP cleanup rejection on a later close", async () => { + const startup = new Error("handshake failed"); + let attempts = 0; + let fail = true; + const session = createSharedMcpSession(async (registerCleanup) => { + registerCleanup(() => { + attempts += 1; + return fail ? Promise.reject() : Promise.resolve(); + }); + throw startup; + }); + const caught = await session.session.call().then( + () => { + throw new Error("startup succeeded"); + }, + (error: unknown) => error, + ); + expect(caught).toBeInstanceOf(Error); + if (!(caught instanceof Error)) throw new Error("expected startup failure"); + expect(caught.message).toBe(startup.message); + expect(caught.cause).toBeInstanceOf(AggregateError); + const cause = caught.cause as AggregateError; + expect(cause.cause).toBe(startup); + expect([...cause.errors]).toEqual([startup, undefined]); + await expect(session.close()).rejects.toBeUndefined(); + expect(attempts).toBe(2); + fail = false; + await session.close(); + expect(attempts).toBe(3); +}); + +test("does not open another MCP server while failed cleanup is retained", async () => { + const startup = new Error("handshake failed"); + const cleanup = new Error("teardown failed"); + let openings = 0; + let attempts = 0; + let failCleanup = true; + const session = createSharedMcpSession(async (registerCleanup) => { + openings += 1; + registerCleanup(async () => { + attempts += 1; + if (failCleanup) throw cleanup; + }); + throw startup; + }); + const first = await session.session.call().then( + () => { + throw new Error("startup succeeded"); + }, + (error: unknown) => error, + ); + expect(first).toBeInstanceOf(Error); + if (!(first instanceof Error)) throw new Error("expected startup failure"); + expect(first.message).toBe(startup.message); + expect(openings).toBe(1); + expect(attempts).toBe(1); + + const blocked = await session.session.call().then( + () => { + throw new Error("startup succeeded"); + }, + (error: unknown) => error, + ); + expect(blocked).toBe(cleanup); + expect(openings).toBe(1); + expect(attempts).toBe(2); + + failCleanup = false; + const reopened = await session.session.call().then( + () => { + throw new Error("startup succeeded"); + }, + (error: unknown) => error, + ); + expect(reopened).toBe(startup); + expect(openings).toBe(2); + expect(attempts).toBe(4); +}); + +test("closes a shared MCP server registered after file cleanup starts", async () => { + const startup = held(); + let closedServer = false; + const session = createSharedMcpSession(async (registerCleanup) => { + await startup.gate; + registerCleanup(async () => { + closedServer = true; + }); + return fakeCall; + }); + const opening = session.session.call(); + const closing = session.close(); + startup.release(); + await closing; + await expect(opening).rejects.toThrow("Shared MCP session is closed"); + expect(closedServer).toBe(true); +}); + +cliTest( + "opens one stdio MCP session for JavaScript application cases", + async ({ cli }) => { + const first = await useFileSession(cli); + expect(await useFileSession(cli)).toBe(first); + const root = await writeApplication("rea-shared-mcp-"); + const analyzed = await analyzeJavaScriptDirectory("stdio MCP", cli, root); + expect( + analyzed.graph.nodes.some(({ kind }) => kind === "electron-renderer"), + ).toBe(true); + }, +); + +cliTest( + "reuses that stdio MCP session for a later case in the file", + async ({ cli }) => { + await useFileSession(cli); + const root = await writeApplication("rea-shared-mcp-later-"); + const analyzed = await analyzeJavaScriptDirectory("stdio MCP", cli, root); + expect( + analyzed.graph.nodes.some(({ kind }) => kind === "electron-renderer"), + ).toBe(true); + }, +); diff --git a/tests/support/cli/cliFixture.ts b/tests/support/cli/cliFixture.ts index e41f6ea951..3ee43177a5 100644 --- a/tests/support/cli/cliFixture.ts +++ b/tests/support/cli/cliFixture.ts @@ -25,11 +25,174 @@ export interface CliResult extends ProcessResult { readonly json: unknown | undefined; } +/** One compiled stdio MCP tool call. */ +export type SharedMcpCall = Awaited< + ReturnType< + typeof import("../../fixtures/localToolsMcp.js").connectLocalToolsMcp + > +>["call"]; + /** Compiled REA CLI driver owned by one test. */ export interface TestCli { run(invocation: CliInvocation): Promise; + /** + * File-scoped stdio MCP caller. Present on `cliTest` drivers and absent for + * drivers created with `createTestCli` alone. + */ + sharedMcpCall?(): Promise; } +/** One file-scoped stdio MCP session. */ +export interface SharedMcpSession { + call(): Promise; +} + +const sharedMcpSessionClosed = (): Error => + new Error("Shared MCP session is closed"); + +/** Keep the handshake as the caller error and retain the cleanup failure beside it. */ +const startupFailureWithCleanup = ( + startup: unknown, + cleanup: unknown, +): Error => { + const primary = + startup instanceof Error ? startup : new Error(String(startup)); + return new Error(primary.message, { + cause: new AggregateError([primary, cleanup], primary.message, { + cause: primary, + }), + }); +}; + +/** + * Start at most one server for the file. Close waits for startup still in + * flight. A connector that sees `isClosed` can skip the server; a server + * registered after close is still cleaned up. A failed cleanup stays owned, + * and the next call does not open another server until that cleanup succeeds. + */ +export const createSharedMcpSession = ( + connect: ( + registerCleanup: (cleanup: () => Promise) => void, + isClosed: () => boolean, + ) => Promise, +): { session: SharedMcpSession; close(): Promise } => { + let pending: Promise | undefined; + let closed = false; + let closing: Promise | undefined; + let closeGeneration: symbol | undefined; + let cleanupQueue: Promise = Promise.resolve(); + const cleanups: Array<() => Promise> = []; + const drain = (): Promise => { + const run = cleanupQueue + .then( + () => undefined, + () => undefined, + ) + .then(async () => { + const failures: unknown[] = []; + const retained: Array<() => Promise> = []; + while (cleanups.length > 0) { + const cleanup = cleanups.pop(); + if (cleanup === undefined) break; + try { + await cleanup(); + } catch (error) { + failures.push(error); + retained.push(cleanup); + } + } + for (const cleanup of retained.reverse()) cleanups.push(cleanup); + // A reasonless rejection is `undefined` and still owns the cleanup. + if (failures.length > 1) { + throw new AggregateError( + failures, + "Shared MCP session cleanup failed", + ); + } + if (failures.length === 1) throw failures[0]; + }); + cleanupQueue = run; + return run; + }; + const registerCleanup = (cleanup: () => Promise): void => { + cleanups.push(cleanup); + }; + return { + session: { + call: () => { + if (closed) return Promise.reject(sharedMcpSessionClosed()); + pending ??= (async () => { + try { + if (cleanups.length > 0) await drain(); + } catch (cleanupError) { + // Close owns cleanup once shutdown has started, so a failed + // handshake cannot clear the startup close is awaiting. + if (!closed) pending = undefined; + throw cleanupError; + } + if (closed) throw sharedMcpSessionClosed(); + try { + const opened = await connect(registerCleanup, () => closed); + if (closed) throw sharedMcpSessionClosed(); + return opened; + } catch (error) { + if (!closed) { + pending = undefined; + try { + await drain(); + } catch (cleanupError) { + throw startupFailureWithCleanup(error, cleanupError); + } + } + throw error; + } + })(); + return pending; + }, + }, + close: () => { + closed = true; + if (closing !== undefined) return closing; + const starting = pending; + const generation = Symbol("shared-mcp-close"); + closeGeneration = generation; + const attempt = (async () => { + try { + if (starting !== undefined) { + await starting.catch(() => undefined); + } + await drain(); + } catch (error) { + // A failed close stays retryable. Completion is cached only after + // every retained cleanup has succeeded. + if (closeGeneration === generation) { + closing = undefined; + closeGeneration = undefined; + } + throw error; + } + })(); + closing = attempt; + return attempt; + }, + }; +}; + +const createFileMcpSession = ( + compileCache: string, +): { session: SharedMcpSession; close(): Promise } => + createSharedMcpSession(async (registerCleanup, isClosed) => { + const { connectLocalToolsMcp } = await import( + "../../fixtures/localToolsMcp.js" + ); + if (isClosed()) throw sharedMcpSessionClosed(); + const mcp = await connectLocalToolsMcp( + { NODE_COMPILE_CACHE: compileCache }, + registerCleanup, + ); + return mcp.call; + }); + const parseJson = (stdout: string): unknown | undefined => { const document = stdout.trim(); if (document.length === 0) return undefined; @@ -92,6 +255,7 @@ export const createTestCli = ( /** Vitest API extended with an owned process registry and compiled CLI driver. */ export const cliTest = processTest.extend<{ $test: { cli: TestCli }; + $file: { mcpSession: SharedMcpSession }; $worker: { cliCompileCache: string }; }>({ cliCompileCache: [ @@ -106,7 +270,22 @@ export const cliTest = processTest.extend<{ }, { scope: "worker" }, ], - cli: async ({ processes, cliCompileCache }, use) => { - await use(createTestCli(processes, cliCompileCache)); + mcpSession: [ + async ({ cliCompileCache }, use) => { + const opened = createFileMcpSession(cliCompileCache); + try { + await use(opened.session); + } finally { + await opened.close(); + } + }, + { scope: "file" }, + ], + cli: async ({ processes, cliCompileCache, mcpSession }, use) => { + const cli = createTestCli(processes, cliCompileCache); + await use({ + ...cli, + sharedMcpCall: () => mcpSession.call(), + }); }, }); diff --git a/tests/support/cli/javascriptApplication.ts b/tests/support/cli/javascriptApplication.ts index 59bb317f2d..6a04b9592c 100644 --- a/tests/support/cli/javascriptApplication.ts +++ b/tests/support/cli/javascriptApplication.ts @@ -6,7 +6,7 @@ import { z } from "zod"; import { javascriptApplicationAnalysisResultSchema } from "../../../src/domain/javascript/javascriptApplicationAnalysis.js"; import { connectLocalToolsMcp } from "../../fixtures/localToolsMcp.js"; -import type { TestCli } from "./cliFixture.js"; +import type { SharedMcpCall, TestCli } from "./cliFixture.js"; const execute = promisify(execFile); @@ -21,6 +21,20 @@ export const runNodeJsonControl = async ( return JSON.parse(stdout) as unknown; }; +/** + * Use an explicit caller when the test owns the server. Otherwise reuse the + * file-scoped session from `cliTest`, and start a fresh server only for + * drivers created outside that fixture. + */ +export const resolveJavaScriptApplicationMcpCall = async ( + cli: TestCli, + mcpCall: SharedMcpCall | undefined, +): Promise => { + if (mcpCall !== undefined) return mcpCall; + if (cli.sharedMcpCall !== undefined) return cli.sharedMcpCall(); + return (await connectLocalToolsMcp()).call; +}; + /** Analyze a directory; a supplied MCP caller reuses its test-owned connection. */ export const analyzeJavaScriptDirectory = async ( surface: "CLI" | "stdio MCP", @@ -31,9 +45,7 @@ export const analyzeJavaScriptDirectory = async ( mcpCall, }: { readonly sourcePaths?: readonly string[]; - readonly mcpCall?: - | Awaited>["call"] - | undefined; + readonly mcpCall?: SharedMcpCall | undefined; } = {}, ) => { let document: unknown; @@ -51,7 +63,7 @@ export const analyzeJavaScriptDirectory = async ( expect(response.exitCode, response.stderr).toBe(0); document = response.json; } else { - const call = mcpCall ?? (await connectLocalToolsMcp()).call; + const call = await resolveJavaScriptApplicationMcpCall(cli, mcpCall); const response = await call("analyze_javascript_application", { input_path: root, format: "directory",