[GH-ISSUE #5944] [Bug] Derived MCP operations discard the request cancellation reason #3614

New issue

Closed

opened 2026-10-31 00:42:23 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

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

Owner

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

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 compare_artifacts while its terminal progress notification is being delivered produces an internal AnalysisCancelledError with cause === undefined, although the server request's signal.reason is still "Error: Requested terminal stop".

This was executed using the real MCP SDK client, REA's public server registration and linked in-memory transports. The typed result was observed immediately before MCP projection. Direct tests of the shared derived-operation helper also lost the exact supplied object reason at each of its three checkpoints: before computation, after computation and after terminal progress. The non-aborted controls succeeded, and the connection still answered a ping after cancellation.

Reproduction

In a prepared source checkout with locked dependencies, save the following as tests/boundary/mcp/cancellationReason.repro.test.ts and run:

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

The focused run reports four failed reason-identity assertions and one passing helper control. The MCP case also executes a successful normal comparison before cancellation. The transport hook calls the real send method, cancels at the terminal notification and yields to let the SDK process its cancellation notification. The admission and delivery spies only observe the actual request signal and typed result. No provider engine is simulated or launched; this derived tool uses the repository's canonical Evidence fixture.

import { Client, InMemoryTransport } from "@modelcontextprotocol/client";
import { setImmediate } from "node:timers/promises";
import { expect, it, vi } from "vitest";
import { ARTIFACT_COMPARISON_EXAMPLE } from "../../../src/contracts/artifactComparisonExample.js";
import { AnalysisCancelledError } from "../../../src/domain/analysisErrorCore.js";
import { createServer } from "../../../src/server/createServer.js";
import { runDerivedOperation } from "../../../src/server/runDerivedOperation.js";
import { ToolResultDelivery } from "../../../src/server/toolResult.js";
import { createTestBinarySession } from "../../fixtures/binarySession.js";

it.each(["before", "after-compute", "terminal", "control"])("derived reason: %s", async (phase) => {
  const controller = new AbortController();
  const reason = { requested: "stop", phase };
  let computed = false;
  if (phase === "before") controller.abort(reason);
  const result = await runDerivedOperation({ mcpReq: {
    signal: controller.signal,
    _meta: { progressToken: "reason-test" },
    notify: async (notification) => {
      if (phase === "terminal" && notification.params?.message === "complete: Completed compare_artifacts") controller.abort(reason);
    },
  } }, "compare_artifacts", () => {
    computed = true;
    if (phase === "after-compute") controller.abort(reason);
    return 42;
  });
  if (phase === "control") { expect(result).toEqual({ ok: true, value: 42 }); return; }
  expect(result.ok).toBe(false);
  if (result.ok) throw new Error("Expected cancellation");
  console.log(JSON.stringify({ phase, tag: result.error._tag, computed, causeAbsent: result.error.cause === undefined, exactReason: result.error.cause === reason }));
  expect(result.error).toBeInstanceOf(AnalysisCancelledError);
  expect(result.error.cause).toBe(reason);
});

it("MCP compare_artifacts drops its server request signal reason at terminal progress", async () => {
  const session = createTestBinarySession(() => { throw new Error("Provider must not start"); });
  let requestSignal: AbortSignal | undefined;
  const admit = session.withAdmittedAnalysis.bind(session);
  const admissionSpy = vi.spyOn(session, "withAdmittedAnalysis").mockImplementation((name, signal, operation) => {
    requestSignal = signal;
    return admit(name, signal, operation);
  });
  const server = createServer({ kind: "session", session });
  const client = new Client({ name: "cancellation-reason-test", version: "1" });
  const controller = new AbortController();
  const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();
  const send = serverTransport.send.bind(serverTransport);
  const sendSpy = vi.spyOn(serverTransport, "send").mockImplementation(async (message, ...args) => {
    await send(message, ...args);
    if ("method" in message && message.method === "notifications/progress" && message.params?.message === "complete: Completed compare_artifacts") {
      controller.abort(new Error("Requested terminal stop"));
      await setImmediate();
    }
  });
  let resolveSettled!: () => void;
  const settled = new Promise<void>(resolve => { resolveSettled = resolve; });
  let typed: any;
  const deliver = ToolResultDelivery.prototype.toCallToolResult;
  const deliverySpy = vi.spyOn(ToolResultDelivery.prototype, "toCallToolResult").mockImplementation(function(result, contract) {
    if (contract.name === "compare_artifacts") {
      typed = result;
      resolveSettled();
    }
    return deliver.call(this, result, contract);
  });
  try {
    await server.connect(serverTransport);
    await client.connect(clientTransport);
    const control = await client.callTool({ name: "compare_artifacts", arguments: ARTIFACT_COMPARISON_EXAMPLE });
    expect(control.isError).not.toBe(true);
    typed = undefined;
    const pending = client.callTool({ name: "compare_artifacts", arguments: ARTIFACT_COMPARISON_EXAMPLE, _meta: { progressToken: "terminal-stop" } }, { signal: controller.signal }).catch(error => error);
    await pending;
    await settled;
    expect(typed?.ok).toBe(false);
    expect(typed.error).toBeInstanceOf(AnalysisCancelledError);
    expect(requestSignal?.aborted).toBe(true);
    expect(requestSignal?.reason).toBeDefined();
    console.log(JSON.stringify({ phase: "MCP-terminal", tag: typed.error._tag, causeAbsent: typed.error.cause === undefined, serverReason: String(requestSignal?.reason), exactServerReason: typed.error.cause === requestSignal?.reason }));
    await client.ping();
    expect(typed.error.cause).toBe(requestSignal?.reason);
  } finally {
    sendSpy.mockRestore(); deliverySpy.mockRestore(); admissionSpy.mockRestore();
    await client.close(); await server.close(); await session.close();
  }
});

Expected behavior

Each shared derived-operation checkpoint should return AnalysisCancelledError whose cause equals the exact reason on its server request signal. The lifecycle contract requires this even when cancellation is first observed after work completes. The test guidance explicitly requires reason identity assertions.

Impact and scope

P2: the shared producer loses a required diagnostic/provenance value before downstream typed-result consumers can inspect it. Cancellation classification remains correct. The measured public request still cancels, and the server remains responsive. A caller retaining the original signal can still read its reason; the tests observed that the value remained intact there.

The current cancellation projection does not expose the opaque cause, so this report does not establish changed wire fields or a missing client-visible message. Natural occurrence frequency, stdio transport, other tools sharing the helper and other platforms were not measured. No incorrect analysis result, input corruption or persistent resource leak was observed.

REA version

rea-agents 6.7.0; source revision 5241597594.

Runtime and platform

macOS 27.0.1 arm64; Node.js v24.18.0 (repository pin), Vitest 5.0.3. The same focused test also reproduced on supported Node.js v26.12.0.

Target and artifact context

Canonical ARTIFACT_COMPARISON_EXAMPLE Evidence supplied inline. No active binary, external analysis engine or target process.

Interface

MCP

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

Diagnostics from the executed focused test:

{"phase":"before","tag":"AnalysisCancelledError","computed":false,"causeAbsent":true,"exactReason":false}
{"phase":"after-compute","tag":"AnalysisCancelledError","computed":true,"causeAbsent":true,"exactReason":false}
{"phase":"terminal","tag":"AnalysisCancelledError","computed":true,"causeAbsent":true,"exactReason":false}
{"phase":"MCP-terminal","tag":"AnalysisCancelledError","causeAbsent":true,"serverReason":"Error: Requested terminal stop","exactServerReason":false}

Tests  4 failed | 1 passed (5)
AssertionError: expected undefined to be 'Error: Requested terminal stop'

The SDK converts the local client's Error reason to a string for the server signal. The MCP assertion therefore compares against the actual server signal reason, independently of that transport conversion. The three direct helper assertions compare the original object by identity.

Typed outcome or recovery expected

Preserve the existing AnalysisCancelledError classification and set its cause to the exact server request signal.reason. This finding does not require serializing arbitrary causes in the public MCP result.

Analysis or suspected cause

The three checkpoints each construct new AnalysisCancelledError(operation) without options. The error constructor accepts error options. The public artifact-comparison handler passes the SDK context to the helper and forwards its failed typed result to delivery.

These source observations explain the executed loss. No repair was tested.

Additional context

Related reports cover different producing boundaries: #5822 concerns IDA, #5332 concerns EVM artifact wrapping, #5868 concerns historical-capture reply reads, and #5935 concerns the JavaScript reader. The admission fix in #5671 changes reverse, pcap and SQLite services. None changes the shared derived-operation helper.

Fetched upstream main ce1f4b1bba and source-compared the relevant server files; they are unchanged from the tested baseline. That newer revision was not executed.

Originally created by @coygeek on GitHub (Oct 21, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/5944 ### 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 `compare_artifacts` while its terminal progress notification is being delivered produces an internal `AnalysisCancelledError` with `cause === undefined`, although the server request's `signal.reason` is still `"Error: Requested terminal stop"`. This was executed using the real MCP SDK client, REA's public server registration and linked in-memory transports. The typed result was observed immediately before MCP projection. Direct tests of the shared derived-operation helper also lost the exact supplied object reason at each of its three checkpoints: before computation, after computation and after terminal progress. The non-aborted controls succeeded, and the connection still answered a ping after cancellation. ### Reproduction In a prepared source checkout with locked dependencies, save the following as `tests/boundary/mcp/cancellationReason.repro.test.ts` and run: ```sh node node_modules/vitest/vitest.mjs run tests/boundary/mcp/cancellationReason.repro.test.ts --maxWorkers 1 ``` The focused run reports four failed reason-identity assertions and one passing helper control. The MCP case also executes a successful normal comparison before cancellation. The transport hook calls the real send method, cancels at the terminal notification and yields to let the SDK process its cancellation notification. The admission and delivery spies only observe the actual request signal and typed result. No provider engine is simulated or launched; this derived tool uses the repository's canonical Evidence fixture. ```ts import { Client, InMemoryTransport } from "@modelcontextprotocol/client"; import { setImmediate } from "node:timers/promises"; import { expect, it, vi } from "vitest"; import { ARTIFACT_COMPARISON_EXAMPLE } from "../../../src/contracts/artifactComparisonExample.js"; import { AnalysisCancelledError } from "../../../src/domain/analysisErrorCore.js"; import { createServer } from "../../../src/server/createServer.js"; import { runDerivedOperation } from "../../../src/server/runDerivedOperation.js"; import { ToolResultDelivery } from "../../../src/server/toolResult.js"; import { createTestBinarySession } from "../../fixtures/binarySession.js"; it.each(["before", "after-compute", "terminal", "control"])("derived reason: %s", async (phase) => { const controller = new AbortController(); const reason = { requested: "stop", phase }; let computed = false; if (phase === "before") controller.abort(reason); const result = await runDerivedOperation({ mcpReq: { signal: controller.signal, _meta: { progressToken: "reason-test" }, notify: async (notification) => { if (phase === "terminal" && notification.params?.message === "complete: Completed compare_artifacts") controller.abort(reason); }, } }, "compare_artifacts", () => { computed = true; if (phase === "after-compute") controller.abort(reason); return 42; }); if (phase === "control") { expect(result).toEqual({ ok: true, value: 42 }); return; } expect(result.ok).toBe(false); if (result.ok) throw new Error("Expected cancellation"); console.log(JSON.stringify({ phase, tag: result.error._tag, computed, causeAbsent: result.error.cause === undefined, exactReason: result.error.cause === reason })); expect(result.error).toBeInstanceOf(AnalysisCancelledError); expect(result.error.cause).toBe(reason); }); it("MCP compare_artifacts drops its server request signal reason at terminal progress", async () => { const session = createTestBinarySession(() => { throw new Error("Provider must not start"); }); let requestSignal: AbortSignal | undefined; const admit = session.withAdmittedAnalysis.bind(session); const admissionSpy = vi.spyOn(session, "withAdmittedAnalysis").mockImplementation((name, signal, operation) => { requestSignal = signal; return admit(name, signal, operation); }); const server = createServer({ kind: "session", session }); const client = new Client({ name: "cancellation-reason-test", version: "1" }); const controller = new AbortController(); const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); const send = serverTransport.send.bind(serverTransport); const sendSpy = vi.spyOn(serverTransport, "send").mockImplementation(async (message, ...args) => { await send(message, ...args); if ("method" in message && message.method === "notifications/progress" && message.params?.message === "complete: Completed compare_artifacts") { controller.abort(new Error("Requested terminal stop")); await setImmediate(); } }); let resolveSettled!: () => void; const settled = new Promise<void>(resolve => { resolveSettled = resolve; }); let typed: any; const deliver = ToolResultDelivery.prototype.toCallToolResult; const deliverySpy = vi.spyOn(ToolResultDelivery.prototype, "toCallToolResult").mockImplementation(function(result, contract) { if (contract.name === "compare_artifacts") { typed = result; resolveSettled(); } return deliver.call(this, result, contract); }); try { await server.connect(serverTransport); await client.connect(clientTransport); const control = await client.callTool({ name: "compare_artifacts", arguments: ARTIFACT_COMPARISON_EXAMPLE }); expect(control.isError).not.toBe(true); typed = undefined; const pending = client.callTool({ name: "compare_artifacts", arguments: ARTIFACT_COMPARISON_EXAMPLE, _meta: { progressToken: "terminal-stop" } }, { signal: controller.signal }).catch(error => error); await pending; await settled; expect(typed?.ok).toBe(false); expect(typed.error).toBeInstanceOf(AnalysisCancelledError); expect(requestSignal?.aborted).toBe(true); expect(requestSignal?.reason).toBeDefined(); console.log(JSON.stringify({ phase: "MCP-terminal", tag: typed.error._tag, causeAbsent: typed.error.cause === undefined, serverReason: String(requestSignal?.reason), exactServerReason: typed.error.cause === requestSignal?.reason })); await client.ping(); expect(typed.error.cause).toBe(requestSignal?.reason); } finally { sendSpy.mockRestore(); deliverySpy.mockRestore(); admissionSpy.mockRestore(); await client.close(); await server.close(); await session.close(); } }); ``` ### Expected behavior Each shared derived-operation checkpoint should return `AnalysisCancelledError` whose cause equals the exact reason on its server request signal. The [lifecycle contract](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/tool-design.md#L101-L110) requires this even when cancellation is first observed after work completes. The [test guidance](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/testing.md#L268-L275) explicitly requires reason identity assertions. ### Impact and scope P2: the shared producer loses a required diagnostic/provenance value before downstream typed-result consumers can inspect it. Cancellation classification remains correct. The measured public request still cancels, and the server remains responsive. A caller retaining the original signal can still read its reason; the tests observed that the value remained intact there. The current cancellation projection does not expose the opaque cause, so this report does not establish changed wire fields or a missing client-visible message. Natural occurrence frequency, stdio transport, other tools sharing the helper and other platforms were not measured. No incorrect analysis result, input corruption or persistent resource leak was observed. ### REA version rea-agents 6.7.0; source revision 52415975942a4b9c53a817915b313ccc404f280e. ### Runtime and platform macOS 27.0.1 arm64; Node.js v24.18.0 (repository pin), Vitest 5.0.3. The same focused test also reproduced on supported Node.js v26.12.0. ### Target and artifact context Canonical ARTIFACT_COMPARISON_EXAMPLE Evidence supplied inline. No active binary, external analysis engine or target process. ### Interface MCP ### Earliest failure stage Cleanup, cancellation, or lifecycle ### Relevant output or evidence Diagnostics from the executed focused test: ```json {"phase":"before","tag":"AnalysisCancelledError","computed":false,"causeAbsent":true,"exactReason":false} {"phase":"after-compute","tag":"AnalysisCancelledError","computed":true,"causeAbsent":true,"exactReason":false} {"phase":"terminal","tag":"AnalysisCancelledError","computed":true,"causeAbsent":true,"exactReason":false} {"phase":"MCP-terminal","tag":"AnalysisCancelledError","causeAbsent":true,"serverReason":"Error: Requested terminal stop","exactServerReason":false} ``` ```text Tests 4 failed | 1 passed (5) AssertionError: expected undefined to be 'Error: Requested terminal stop' ``` The SDK converts the local client's Error reason to a string for the server signal. The MCP assertion therefore compares against the actual server signal reason, independently of that transport conversion. The three direct helper assertions compare the original object by identity. ### Typed outcome or recovery expected Preserve the existing `AnalysisCancelledError` classification and set its cause to the exact server request `signal.reason`. This finding does not require serializing arbitrary causes in the public MCP result. ### Analysis or suspected cause The [three checkpoints](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/server/runDerivedOperation.ts#L25-L75) each construct `new AnalysisCancelledError(operation)` without options. The [error constructor](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/domain/analysisErrorCore.ts#L151-L160) accepts error options. The [public artifact-comparison handler](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/server/registerArtifactComparisonTool.ts#L27-L65) passes the SDK context to the helper and forwards its failed typed result to delivery. These source observations explain the executed loss. No repair was tested. ### Additional context Related reports cover different producing boundaries: #5822 concerns IDA, #5332 concerns EVM artifact wrapping, #5868 concerns historical-capture reply reads, and #5935 concerns the JavaScript reader. The admission fix in #5671 changes reverse, pcap and SQLite services. None changes the shared derived-operation helper. Fetched upstream main ce1f4b1bbaced57529d609f3007a76de583e11c4 and source-compared the relevant server files; they are unchanged from the tested baseline. That newer revision was not executed.

bsakaguchi closed this issue

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

bsakaguchi referenced this issue

2026-11-01 21:10:58 +00:00

[GH-ISSUE #3796] [Tracker] Open feature requests by area (October 2026) #1471