morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[GH-ISSUE #5968] [Bug] Snapshot-read cancellation attributes open_binary to read_evidence_file #3639

New issue

Closed

opened 2026-10-31 00:42:37 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-31 00:42:37 +00:00

Owner

Originally created by @coygeek on GitHub (Oct 21, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/5968

Before submitting

I searched the open and closed issues for a related report.
I removed credentials, tokens, private paths, and sensitive target data.

Area

MCP or agent integration

Observed behavior

Cancelling open_binary during the read of its selected analysis snapshot constructs a cancelled error with details.operation: "read_evidence_file". The same lifecycle request uses open_binary at its surrounding cancellation checkpoints.

A focused integration test exercised the public MCP registration through linked SDK transports, the actual snapshot reader and a simulated analysis provider. The non-cancelled snapshot import succeeded. Cancellation retained the exact server request reason and reported complete cleanup; only operation attribution differed.

The SDK rejected the cancelled client wait. The eventual error was observed at REA's delivery boundary before SDK reply suppression; this does not claim that the cancelled client received it.

Reproduction

In a prepared checkout with locked dependencies and built artifacts, save the test below as tests/boundary/mcp/snapshotCancellationOperation.repro.test.ts. Set REA_VERIFY_ROOT to an existing writable directory for the fixture, then run with the repository's pinned Node.js:

node node_modules/vitest/vitest.mjs run tests/boundary/mcp/snapshotCancellationOperation.repro.test.ts --maxWorkers 1

The test opens /bin/ls through the repository's simulated analysis provider, exports a matching snapshot and successfully reimports it. During a second import, the physical FileHandle.read completes and its return is gated until an actual SDK cancellation notification has reached the server. The read and cancellation results are not substituted. The expected-operation assertion fails. Remove the test after the run; it removes its generated snapshot directory in finally.

import { mkdtemp, open, rm, writeFile } from "node:fs/promises";
import { join } from "node:path";
import { setImmediate } from "node:timers/promises";
import { Client, InMemoryTransport } from "@modelcontextprotocol/client";
import { expect, it, vi } from "vitest";
import { createServer } from "../../../src/server/createServer.js";
import { ToolResultDelivery } from "../../../src/server/toolResult.js";
import { projectAnalysisError } from "../../../src/domain/analysisErrorProjection.js";
import { readAnalysisSnapshot } from "../../../src/application/binary/AnalysisSnapshotFiles.js";
import { createCacheProvider, createDeferred, createTestBinarySession } from "../../fixtures/binarySession.js";

it("attributes cancellation during a real snapshot read to open_binary", async () => {
  const root = await mkdtemp(join(process.env.REA_VERIFY_ROOT!, "snapshot-"));
  const snapshotPath = join(root, "snapshot.json");
  const session = createTestBinarySession(createCacheProvider([]));
  const server = createServer({ kind: "session", session });
  const client = new Client({ name: "snapshot-operation-test", version: "1" });
  const [ct, st] = InMemoryTransport.createLinkedPair();
  const release = createDeferred<void>();
  let readSpy: ReturnType<typeof vi.spyOn> | undefined;
  let admissionSpy: ReturnType<typeof vi.spyOn> | undefined;
  let deliverySpy: ReturnType<typeof vi.spyOn> | undefined;
  try {
    await server.connect(st);
    await client.connect(ct);
    const first = await client.callTool({ name: "open_binary", arguments: { path: "/bin/ls" } });
    expect(first.isError).not.toBe(true);
    const snapshot = session.exportAnalysisSnapshot();
    if (!snapshot.ok) throw snapshot.error;
    await writeFile(snapshotPath, JSON.stringify(snapshot.value));
    expect((await readAnalysisSnapshot(snapshotPath)).ok).toBe(true);
    expect((await client.callTool({ name: "close_binary", arguments: {} })).isError).not.toBe(true);
    const control = await client.callTool({ name: "open_binary", arguments: { path: "/bin/ls", snapshot_path: snapshotPath } });
    expect(control.isError).not.toBe(true);
    expect((await client.callTool({ name: "close_binary", arguments: {} })).isError).not.toBe(true);
    console.log(JSON.stringify({ phase: "control", snapshotOpenSucceeded: true }));
    const probe = await open(snapshotPath, "r");
    const prototype = Object.getPrototypeOf(probe);
    await probe.close();
    const read = prototype.read;
    const reached = createDeferred<void>();
    let blocked = false;
    readSpy = vi.spyOn(prototype, "read").mockImplementation(async function(this: unknown, ...args: unknown[]) {
      const result = await read.apply(this, args);
      if (!blocked) {
        blocked = true;
        reached.resolve();
        await release.promise;
      }
      return result;
    });
    let requestSignal: AbortSignal | undefined;
    const openSession = session.open.bind(session);
    admissionSpy = vi.spyOn(session, "open").mockImplementation((path, options) => {
      requestSignal = options?.signal;
      return openSession(path, options);
    });
    const delivered = createDeferred<unknown>();
    let typed: any;
    const deliver = ToolResultDelivery.prototype.toCallToolResult;
    deliverySpy = vi.spyOn(ToolResultDelivery.prototype, "toCallToolResult").mockImplementation(function(result, contract) {
      const projected = deliver.call(this, result, contract);
      if (contract.name === "open_binary") { typed = result; delivered.resolve(projected); }
      return projected;
    });
    const controller = new AbortController();
    const pending = client.callTool({ name: "open_binary", arguments: { path: "/bin/ls", snapshot_path: snapshotPath } }, { signal: controller.signal }).catch(error => error);
    await reached.promise;
    controller.abort(new Error("Stop snapshot read"));
    await pending;
    for (let turn = 0; turn < 20 && !requestSignal?.aborted; turn++) await setImmediate();
    expect(requestSignal?.aborted).toBe(true);
    release.resolve();
    const projected = await delivered.promise;
    console.log(JSON.stringify({ phase: "cancelled-read", projected, exactReason: typed.error.cause === requestSignal?.reason }));
    expect(typed.ok).toBe(false);
    expect(typed.error.cause).toBe(requestSignal?.reason);
    expect(projectAnalysisError(typed.error)).toMatchObject({ code: "cancelled", details: { operation: "open_binary", cleanup: "complete" } });
  } finally {
    release.resolve();
    readSpy?.mockRestore(); admissionSpy?.mockRestore(); deliverySpy?.mockRestore();
    await client.close(); await server.close(); await session.close();
    await rm(root, { recursive: true, force: true });
  }
}, 20000);

Expected behavior

The cancellation should identify open_binary, retaining its existing cause and cleanup status. This follows the surrounding lifecycle checkpoints and the existing convention of reattributing nested file-read cancellations. The error's operation is a caller-visible field, serialized by the standard projection.

Impact and scope

P3: inconsistent cancellation diagnostics. The observed operation field names an internal JSON reader, although the accepted request was open_binary. The cancellation classification, exact abort reason and complete-cleanup state remain correct. The caller can retain its original request identity; no wrong analysis, data loss, failed retry or resource leak was observed.

The controlled read case reproduced once. Natural race frequency, stdio delivery, CLI cancellation and the loader's later parse/yield cancellation timing were not measured.

REA version

rea-agents 6.7.0; source revision 5241597594.

Runtime and platform

Node.js v24.18.0; macOS 27.0.1 arm64; Vitest 5.0.3. The analysis provider is simulated using the repository fixture; no external reverse-engineering engine or target execution was used.

Target and artifact context

/bin/ls read as a native artifact and a matching persisted analysis snapshot exported from the simulated session.

Interface

MCP

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

Observed test diagnostics, reduced to the relevant fields:

{"phase":"control","snapshotOpenSucceeded":true}
{"phase":"cancelled-read","error":{"code":"cancelled","category":"cancelled","details":{"operation":"read_evidence_file","cleanup":"complete"}},"exactReason":true}

Tests  1 failed (1)
Expected details.operation: open_binary
Received details.operation: read_evidence_file

The full test logs the actual constructed MCP result. Its final assertion compares the standard projection against open_binary; the reason-identity assertion passes first.

Typed outcome or recovery expected

AnalysisCancelledError with operation open_binary, unchanged exact server signal.reason, and truthful cleanup state.

Analysis or suspected cause

The public lifecycle handler passes the selected snapshot and request signal into readAnalysisSnapshot. The snapshot loader propagates the JSON reader's cancellation unchanged, and its later catch also hard-codes read_evidence_file. The JSON reader boundary constructs that operation label. The enclosing session returns a failed load unchanged. These source observations explain the reproduced attribution; no repair was tested.

The CLI also calls the same loader; that reachability is source-derived and was not separately executed.

Additional context

Reports #5935 and #5944 concern loss of cancellation reasons in different producing boundaries. This case retains its exact reason and instead misattributes the operation. #4752 concerns native Ghidra snapshot descriptor ownership; #1680 concerns large analysis-snapshot memory expansion.

Fetched upstream main 25e1a24a3e and compared the relevant read/handler lines; the attribution remains unchanged. That newer revision was not executed.

Originally created by @coygeek on GitHub (Oct 21, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/5968 ### Before submitting - [x] I searched the open and closed issues for a related report. - [x] I removed credentials, tokens, private paths, and sensitive target data. ### Area MCP or agent integration ### Observed behavior Cancelling `open_binary` during the read of its selected analysis snapshot constructs a `cancelled` error with `details.operation: "read_evidence_file"`. The same lifecycle request uses `open_binary` at its surrounding cancellation checkpoints. A focused integration test exercised the public MCP registration through linked SDK transports, the actual snapshot reader and a simulated analysis provider. The non-cancelled snapshot import succeeded. Cancellation retained the exact server request reason and reported complete cleanup; only operation attribution differed. The SDK rejected the cancelled client wait. The eventual error was observed at REA's delivery boundary before SDK reply suppression; this does not claim that the cancelled client received it. ### Reproduction In a prepared checkout with locked dependencies and built artifacts, save the test below as `tests/boundary/mcp/snapshotCancellationOperation.repro.test.ts`. Set `REA_VERIFY_ROOT` to an existing writable directory for the fixture, then run with the repository's pinned Node.js: ```sh node node_modules/vitest/vitest.mjs run tests/boundary/mcp/snapshotCancellationOperation.repro.test.ts --maxWorkers 1 ``` The test opens `/bin/ls` through the repository's simulated analysis provider, exports a matching snapshot and successfully reimports it. During a second import, the physical `FileHandle.read` completes and its return is gated until an actual SDK cancellation notification has reached the server. The read and cancellation results are not substituted. The expected-operation assertion fails. Remove the test after the run; it removes its generated snapshot directory in `finally`. ```ts import { mkdtemp, open, rm, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { setImmediate } from "node:timers/promises"; import { Client, InMemoryTransport } from "@modelcontextprotocol/client"; import { expect, it, vi } from "vitest"; import { createServer } from "../../../src/server/createServer.js"; import { ToolResultDelivery } from "../../../src/server/toolResult.js"; import { projectAnalysisError } from "../../../src/domain/analysisErrorProjection.js"; import { readAnalysisSnapshot } from "../../../src/application/binary/AnalysisSnapshotFiles.js"; import { createCacheProvider, createDeferred, createTestBinarySession } from "../../fixtures/binarySession.js"; it("attributes cancellation during a real snapshot read to open_binary", async () => { const root = await mkdtemp(join(process.env.REA_VERIFY_ROOT!, "snapshot-")); const snapshotPath = join(root, "snapshot.json"); const session = createTestBinarySession(createCacheProvider([])); const server = createServer({ kind: "session", session }); const client = new Client({ name: "snapshot-operation-test", version: "1" }); const [ct, st] = InMemoryTransport.createLinkedPair(); const release = createDeferred<void>(); let readSpy: ReturnType<typeof vi.spyOn> | undefined; let admissionSpy: ReturnType<typeof vi.spyOn> | undefined; let deliverySpy: ReturnType<typeof vi.spyOn> | undefined; try { await server.connect(st); await client.connect(ct); const first = await client.callTool({ name: "open_binary", arguments: { path: "/bin/ls" } }); expect(first.isError).not.toBe(true); const snapshot = session.exportAnalysisSnapshot(); if (!snapshot.ok) throw snapshot.error; await writeFile(snapshotPath, JSON.stringify(snapshot.value)); expect((await readAnalysisSnapshot(snapshotPath)).ok).toBe(true); expect((await client.callTool({ name: "close_binary", arguments: {} })).isError).not.toBe(true); const control = await client.callTool({ name: "open_binary", arguments: { path: "/bin/ls", snapshot_path: snapshotPath } }); expect(control.isError).not.toBe(true); expect((await client.callTool({ name: "close_binary", arguments: {} })).isError).not.toBe(true); console.log(JSON.stringify({ phase: "control", snapshotOpenSucceeded: true })); const probe = await open(snapshotPath, "r"); const prototype = Object.getPrototypeOf(probe); await probe.close(); const read = prototype.read; const reached = createDeferred<void>(); let blocked = false; readSpy = vi.spyOn(prototype, "read").mockImplementation(async function(this: unknown, ...args: unknown[]) { const result = await read.apply(this, args); if (!blocked) { blocked = true; reached.resolve(); await release.promise; } return result; }); let requestSignal: AbortSignal | undefined; const openSession = session.open.bind(session); admissionSpy = vi.spyOn(session, "open").mockImplementation((path, options) => { requestSignal = options?.signal; return openSession(path, options); }); const delivered = createDeferred<unknown>(); let typed: any; const deliver = ToolResultDelivery.prototype.toCallToolResult; deliverySpy = vi.spyOn(ToolResultDelivery.prototype, "toCallToolResult").mockImplementation(function(result, contract) { const projected = deliver.call(this, result, contract); if (contract.name === "open_binary") { typed = result; delivered.resolve(projected); } return projected; }); const controller = new AbortController(); const pending = client.callTool({ name: "open_binary", arguments: { path: "/bin/ls", snapshot_path: snapshotPath } }, { signal: controller.signal }).catch(error => error); await reached.promise; controller.abort(new Error("Stop snapshot read")); await pending; for (let turn = 0; turn < 20 && !requestSignal?.aborted; turn++) await setImmediate(); expect(requestSignal?.aborted).toBe(true); release.resolve(); const projected = await delivered.promise; console.log(JSON.stringify({ phase: "cancelled-read", projected, exactReason: typed.error.cause === requestSignal?.reason })); expect(typed.ok).toBe(false); expect(typed.error.cause).toBe(requestSignal?.reason); expect(projectAnalysisError(typed.error)).toMatchObject({ code: "cancelled", details: { operation: "open_binary", cleanup: "complete" } }); } finally { release.resolve(); readSpy?.mockRestore(); admissionSpy?.mockRestore(); deliverySpy?.mockRestore(); await client.close(); await server.close(); await session.close(); await rm(root, { recursive: true, force: true }); } }, 20000); ``` ### Expected behavior The cancellation should identify `open_binary`, retaining its existing cause and cleanup status. This follows the [surrounding lifecycle checkpoints](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/application/binary/BinarySession.ts#L511-L545) and the existing convention of [reattributing nested file-read cancellations](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/application/EvidenceBundleFiles.ts#L59-L81). The error's operation is a caller-visible field, [serialized by the standard projection](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/domain/analysisErrorProjection.ts#L507-L514). ### Impact and scope P3: inconsistent cancellation diagnostics. The observed operation field names an internal JSON reader, although the accepted request was `open_binary`. The cancellation classification, exact abort reason and complete-cleanup state remain correct. The caller can retain its original request identity; no wrong analysis, data loss, failed retry or resource leak was observed. The controlled read case reproduced once. Natural race frequency, stdio delivery, CLI cancellation and the loader's later parse/yield cancellation timing were not measured. ### REA version rea-agents 6.7.0; source revision 52415975942a4b9c53a817915b313ccc404f280e. ### Runtime and platform Node.js v24.18.0; macOS 27.0.1 arm64; Vitest 5.0.3. The analysis provider is simulated using the repository fixture; no external reverse-engineering engine or target execution was used. ### Target and artifact context `/bin/ls` read as a native artifact and a matching persisted analysis snapshot exported from the simulated session. ### Interface MCP ### Earliest failure stage Cleanup, cancellation, or lifecycle ### Relevant output or evidence Observed test diagnostics, reduced to the relevant fields: ```json {"phase":"control","snapshotOpenSucceeded":true} {"phase":"cancelled-read","error":{"code":"cancelled","category":"cancelled","details":{"operation":"read_evidence_file","cleanup":"complete"}},"exactReason":true} ``` ```text Tests 1 failed (1) Expected details.operation: open_binary Received details.operation: read_evidence_file ``` The full test logs the actual constructed MCP result. Its final assertion compares the standard projection against `open_binary`; the reason-identity assertion passes first. ### Typed outcome or recovery expected `AnalysisCancelledError` with operation `open_binary`, unchanged exact server `signal.reason`, and truthful cleanup state. ### Analysis or suspected cause The [public lifecycle handler](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/server/registerSessionTools.ts#L184-L213) passes the selected snapshot and request signal into `readAnalysisSnapshot`. The [snapshot loader](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/application/binary/AnalysisSnapshotFiles.ts#L55-L86) propagates the JSON reader's cancellation unchanged, and its later catch also hard-codes `read_evidence_file`. The [JSON reader boundary](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/application/JsonFiles.ts#L127-L145) constructs that operation label. The enclosing session returns a failed load unchanged. These source observations explain the reproduced attribution; no repair was tested. The CLI [also calls the same loader](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/application/DirectAnalysis.ts#L577-L583); that reachability is source-derived and was not separately executed. ### Additional context Reports #5935 and #5944 concern loss of cancellation reasons in different producing boundaries. This case retains its exact reason and instead misattributes the operation. #4752 concerns native Ghidra snapshot descriptor ownership; #1680 concerns large analysis-snapshot memory expansion. Fetched upstream main 25e1a24a3ed3671a0148757555f18d16c29d936a and compared the relevant read/handler lines; the attribution remains unchanged. That newer revision was not executed.

bsakaguchi closed this issue

2026-10-31 00:42:37 +00:00

bsakaguchi commented

2026-10-31 00:42:38 +00:00

Author

Owner

@coygeek commented on GitHub (Oct 24, 2026):

Checked the reported fix on main at d87e73f2db.

AnalysisSnapshotFiles defines READ_OPERATION as open_binary, passes it into the streaming JSON reader and uses it in its local cancellation constructor. Existing test holds a real file read, cancels with an exact reason, asserts operation open_binary and accepts the next complete read. The SDK suppression behavior remains intentionally outside this claim.

Implementation: AnalysisSnapshotFiles.ts, AnalysisSnapshotFiles.ts.

The relevant regression checks in analysisSnapshot.test.ts passed in the focused test run on this revision. This closure is based on the current source and those checks; provider fixtures do not establish a real-engine run. Closing as completed on main.

@coygeek commented on GitHub (Oct 24, 2026): Checked the reported fix on `main` at d87e73f2db50be72d33a4b3c7199af35518b441c. AnalysisSnapshotFiles defines READ_OPERATION as open_binary, passes it into the streaming JSON reader and uses it in its local cancellation constructor. Existing test holds a real file read, cancels with an exact reason, asserts operation open_binary and accepts the next complete read. The SDK suppression behavior remains intentionally outside this claim. Implementation: [AnalysisSnapshotFiles.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/application/binary/AnalysisSnapshotFiles.ts#L23), [AnalysisSnapshotFiles.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/application/binary/AnalysisSnapshotFiles.ts#L63). The relevant regression checks in [analysisSnapshot.test.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/tests/boundary/filesystem/analysisSnapshot.test.ts) passed in the focused test run on this revision. This closure is based on the current source and those checks; provider fixtures do not establish a real-engine run. Closing as completed on main.

bsakaguchi referenced this issue

2026-10-31 01:10:00 +00:00

[PR #3639] [CLOSED] fix(setup): preserve overridden runner variables and test isolation on Windows #5861

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#3639

No description provided.

Rows
Columns