From 73c72d5745ae982b789074d1069480b9cc850bbd Mon Sep 17 00:00:00 2001 From: Rudy Mizrahi Celekli <47457359+rudycelekli@users.noreply.github.com> Date: Thu, 15 Oct 2026 03:05:44 -0400 Subject: [PATCH] fix(mcp): keep a snapshot open ahead of later calls on the same connection (#3661) open_binary read snapshot_path before it entered the session's lifecycle order, so a current_document or close_binary dispatched after it could run first: the read returned Evidence for the previous target and the close left the snapshot target open. The session now loads the snapshot inside the open's own turn. Co-authored-by: Claude Sonnet 5.5 --- src/application/binary/BinarySession.ts | 35 ++++++++++++++---- src/application/binary/BinarySessionOpen.ts | 10 ++++++ src/application/binary/BinarySessionPort.ts | 2 ++ src/server/registerSessionTools.ts | 26 +++++++------- tests/boundary/mcp/sessionMcp.test.ts | 39 +++++++++++++++++++++ 5 files changed, 92 insertions(+), 20 deletions(-) diff --git a/src/application/binary/BinarySession.ts b/src/application/binary/BinarySession.ts index 62f40363d6..151e36ac59 100644 --- a/src/application/binary/BinarySession.ts +++ b/src/application/binary/BinarySession.ts @@ -216,12 +216,12 @@ export class BinarySession options: BinarySessionOpenOptions = {}, ): Promise> { return this.#open( - () => + (admitted) => resolveSessionOpen({ router: this.#providerRouter, current: this.#active, path, - options, + options: admitted, stagedSnapshotMatches: (target, profile) => this.matchesSnapshot(target, profile), }), @@ -250,12 +250,33 @@ export class BinarySession } #open( - resolve: () => Promise>, - options: Pick, + resolve: ( + admitted: BinarySessionOpenOptions, + ) => Promise>, + options: BinarySessionOpenOptions, ): Promise> { - return this.#serialize((admittedThrough) => - this.#openWithinTransition(admittedThrough, resolve, options), - ); + return this.#serialize(async (admittedThrough) => { + const admitted = await this.#withLoadedSnapshot(options); + if (!admitted.ok) return admitted; + return this.#openWithinTransition( + admittedThrough, + () => resolve(admitted.value), + admitted.value, + ); + }); + } + + /** Read the staged snapshot only after earlier lifecycle transitions have settled. */ + async #withLoadedSnapshot( + options: BinarySessionOpenOptions, + ): Promise> { + if (options.loadSnapshot === undefined) return ok(options); + if (isAborted(options.signal)) + return err(new AnalysisCancelledError("open_binary")); + const loaded = await options.loadSnapshot(); + if (!loaded.ok) return loaded; + const { loadSnapshot: _loaded, ...rest } = options; + return ok({ ...rest, snapshot: loaded.value }); } async #openWithinTransition( diff --git a/src/application/binary/BinarySessionOpen.ts b/src/application/binary/BinarySessionOpen.ts index cd5f6be4a6..bb1631f350 100644 --- a/src/application/binary/BinarySessionOpen.ts +++ b/src/application/binary/BinarySessionOpen.ts @@ -19,6 +19,11 @@ import { err, type Result } from "../../domain/result.js"; import type { SessionProviderRoute } from "./SessionProviderRouter.js"; import { SessionProviderRouter } from "./SessionProviderRouter.js"; +/** Read a staged snapshot once the open holds its place in the session order. */ +export type AnalysisSnapshotLoader = () => Promise< + Result +>; + export interface BinarySessionOpenOptions { readonly signal?: AbortSignal; readonly targetKind?: BinaryTarget["kind"]; @@ -26,6 +31,11 @@ export interface BinarySessionOpenOptions { readonly architecture?: BinaryArchitecture; readonly entryPoint?: string; readonly snapshot?: AnalysisSnapshot; + /** + * Load the snapshot inside the open's turn instead of before it, so calls + * dispatched after the open cannot overtake its file read. + */ + readonly loadSnapshot?: AnalysisSnapshotLoader; readonly providerId?: AnalysisProviderSelector; } diff --git a/src/application/binary/BinarySessionPort.ts b/src/application/binary/BinarySessionPort.ts index 49f513c69d..e66dfc1afb 100644 --- a/src/application/binary/BinarySessionPort.ts +++ b/src/application/binary/BinarySessionPort.ts @@ -11,6 +11,7 @@ import type { AnalysisProviderSelector } from "../../contracts/providerSelection import type { AnalysisError } from "../../domain/analysisErrorBase.js"; import type { Result } from "../../domain/result.js"; import type { EvidenceIntegrityError } from "../../domain/evidenceErrors.js"; +import type { AnalysisSnapshotLoader } from "./BinarySessionOpen.js"; import type { WorkflowSnapshotRecordInput } from "./BinarySessionRecords.js"; import type { @@ -71,6 +72,7 @@ export interface BinarySessionPort readonly architecture?: BinaryArchitecture; readonly entryPoint?: string; readonly snapshot?: AnalysisSnapshot; + readonly loadSnapshot?: AnalysisSnapshotLoader; readonly providerId?: AnalysisProviderSelector; }, ): Promise>; diff --git a/src/server/registerSessionTools.ts b/src/server/registerSessionTools.ts index 58a47176e0..8053abf660 100644 --- a/src/server/registerSessionTools.ts +++ b/src/server/registerSessionTools.ts @@ -12,7 +12,6 @@ import { captureProcessScenario } from "../process/capture/ProcessHarness.js"; import { ProcessCaptureResourceScope } from "../process/capture/ProcessCaptureLifecycle.js"; import { ProcessCaptureError } from "../process/capture/ProcessCaptureError.js"; import { toolContract } from "../contracts/toolContracts.js"; -import type { AnalysisSnapshot } from "../domain/analysisSnapshot.js"; import { binaryTargetSummary } from "../application/binary/BinaryTargetSummary.js"; import { UnknownRegistryError } from "../domain/unknownRegistryError.js"; import type { AnalysisError } from "../domain/analysisErrorBase.js"; @@ -178,17 +177,7 @@ const registerOpenLifecycleTool = ( openContract.name, toolRegistrationOptions(openContract), async (input, context) => { - let snapshot: AnalysisSnapshot | undefined; - if (input.snapshot_path !== undefined) { - const loaded = await readAnalysisSnapshot( - input.snapshot_path, - context.mcpReq.signal, - artifactResources, - ); - if (!loaded.ok) - return server.delivery.toCallToolResult(loaded, openContract); - snapshot = loaded.value; - } + const snapshotPath = input.snapshot_path; const opened = await logToolExecution(logger, openContract.name, () => session.open(input.path, { signal: context.mcpReq.signal, @@ -202,7 +191,18 @@ const registerOpenLifecycleTool = ( ...(input.provider_id === undefined ? {} : { providerId: input.provider_id }), - ...(snapshot === undefined ? {} : { snapshot }), + ...(snapshotPath === undefined + ? {} + : { + // Read inside the open's turn: calls issued after it must not + // overtake a file read that precedes its place in the order. + loadSnapshot: () => + readAnalysisSnapshot( + snapshotPath, + context.mcpReq.signal, + artifactResources, + ), + }), }), ); return opened.ok diff --git a/tests/boundary/mcp/sessionMcp.test.ts b/tests/boundary/mcp/sessionMcp.test.ts index 0bad24e576..9a173a159f 100644 --- a/tests/boundary/mcp/sessionMcp.test.ts +++ b/tests/boundary/mcp/sessionMcp.test.ts @@ -481,6 +481,45 @@ describe("target-free MCP workflow", () => { }, 10_000); }); +describe("MCP snapshot open ordering", () => { + it("keeps a snapshot open ahead of later reads and closes on the same connection", async () => { + directory = await createTestTempDirectory("rea-mcp-snapshot-open-order-"); + const { mcp, first, second } = await createSessionMcpHarness( + directory, + provider, + resources, + ); + const snapshotPath = join(directory, "analysis.json"); + await mcp.callTool({ name: "open_binary", arguments: { path: second } }); + expect( + ( + await mcp.callTool({ + name: "close_binary", + arguments: { snapshot_path: snapshotPath }, + }) + ).isError, + ).not.toBe(true); + await mcp.callTool({ name: "open_binary", arguments: { path: first } }); + + // Dispatched without awaiting, like a client issuing one turn's calls. + const opening = mcp.callTool({ + name: "open_binary", + arguments: { path: second, snapshot_path: snapshotPath }, + }); + const reading = mcp.callTool({ name: "current_document", arguments: {} }); + const closing = mcp.callTool({ name: "close_binary", arguments: {} }); + + expect((await opening).isError, JSON.stringify(await opening)).not.toBe( + true, + ); + expect(text(await reading)).toContain("second.hop"); + expect((await closing).isError).not.toBe(true); + expect( + text(await mcp.callTool({ name: "binary_session", arguments: {} })), + ).toContain('"open":false'); + }, 10_000); +}); + describe("session filesystem path boundaries over MCP", () => { it("rejects relative snapshot and export paths with an absolute-path error", async () => { directory = await createTestTempDirectory("rea-mcp-path-boundary-");