fix(cursor): surface a cancel we did not request instead of dropping the turn

devlog/_plan/260817_cursor_toolcall_decode/040_phase4-server-cancel-terminal.md

@@ -0,0 +1,138 @@
+# 040 — Phase 4: a server-side cancel must not vanish
+
+Discovered by the round-6 audit of `010` and recorded there as a follow-up.
+Its own work-phase (LOOP-UNIT-CHAIN-01). **Revised after an audit returned FAIL**
+with two blockers; the original plan is corrected below.
+
+## The defect
+
+An unexpected `NGHTTP2_CANCEL` produces a turn with **zero adapter events**.
+
+The transport already gets this right. Both failure exits
+(`live-transport.ts:629`, `:638`) check provenance before swallowing:
+
+```ts
+if (this.expectedClose && isCursorBenignCancelError(failure)) return;
+throw attachPartialUsage(summarizeFailure(failure), state);
+```
+
+So an unexpected cancel is thrown, correctly. It is the **adapter** that loses it
+(`cursor.ts:181`):
+
+```ts
+if (isCursorBenignCancelError(err)) return;
+```
+
+No `expectedClose` in scope, so it re-decides from the error **code** alone
+(`cursor-errors.ts:49`) and returns silently. The transport's provenance check is
+overruled one layer up by a weaker test of the same question.
+
+Downstream: streaming synthesizes `response.incomplete` / `adapter_eof`
+(`bridge.ts:1283`); non-streaming defaults to `"completed"` (`:1829`) — the same
+silent-success shape `010` closed for EOF.
+
+## Audit corrections
+
+**Blocker 1 — do not re-emit after a terminal.** Tagging on `!expectedClose`
+alone would fire even when a `done`/`error` was already queued, adding a second
+terminal (`emittedTerminal` is set at `live-transport.ts:540`). Buffered JSON
+processes both and flips a completed turn to failed. **Tag only when
+`!expectedClose && !emittedTerminal`**, at both exits.
+
+**Blocker 2 — a marker alone produces a lying message.** Making
+`isCursorBenignCancelError` false is not enough: `safeCursorTransportError`
+(`cursor.ts:50`) passes only `err.message` to `safeCursorErrorMessage`, and
+`classifyCursorError:105` re-matches the untagged string and labels it
+`"Cursor stream suspended"`. The turn would fail with a message claiming an
+intentional suspension — worse than the silent drop, because it misdirects
+diagnosis. **The tagged error must carry its own message**, not be reclassified
+from the raw text.
+
+**Blocker 3 — the fixture cannot do a literal server RST.** A server-side
+`stream.close(NGHTTP2_CANCEL)` surfaces as a clean `end` in the local h2 fixture.
+Test 1 uses the existing fault-injection seam
+(`tests/cursor-live-transport.test.ts:207`) and is named for a **cancel-shaped
+transport failure**, not a literal RST reproduction. It is genuinely red today
+under that seam.
+
+## Contract
+
+| Situation | Behavior |
+|-----------|----------|
+| We cancelled (`expectedClose`) | silent — unchanged |
+| Cancel with a terminal already emitted | silent — no duplicate |
+| Cancel we did not request, no terminal yet | one `error`, with an honest message |
+
+## Diff-level plan
+
+**`src/adapters/cursor/cursor-errors.ts`**
+
+- Add `CursorUnexpectedCancelError` carrying a message that names what happened
+  ("Cursor cancelled the stream before the turn completed"), so the adapter's
+  existing `safeCursorTransportError` path renders it correctly instead of
+  matching `"nghttp2_cancel"` and calling it a suspension.
+- `isCursorBenignCancelError` returns **false** for this class. Untagged errors
+  keep the current code/message matching, so nothing that returns silently today
+  starts erroring without evidence.
+- Verify `classifyCursorError` maps the new message to a connection failure, not
+  `"Cursor stream suspended"`.
+
+**`src/adapters/cursor/live-transport.ts`**
+
+- At both failure exits, when `!this.expectedClose && !this.emittedTerminal` and
+  the failure is cancel-shaped, throw `CursorUnexpectedCancelError` (preserving
+  `attachPartialUsage`) instead of the raw failure.
+- `cancelCursorRun` is unchanged; its `expectedClose` write is the provenance.
+
+**`src/adapters/cursor.ts`** — unchanged.
+
+## Tests (`tests/cursor-cancel-provenance.test.ts`)
+
+1. Cancel-shaped transport failure, no `expectedClose`, no prior terminal ->
+   exactly one `error` event, **and** its message does not say "suspended".
+   Red today. Uses the fault-injection seam.
+2. Client-tool suspend (`expectedClose`) -> still silent. The regression that
+   matters: every working multi-turn tool cycle takes this path.
+3. Cancel after a terminal was already emitted -> still silent, no duplicate.
+4. Unit: `isCursorBenignCancelError` false for the new class, still true for an
+   untagged `NGHTTP2_CANCEL` and for `"cursor stream suspended"`;
+   `classifyCursorError` does not label the new message as a suspension.
+
+## Scope
+
+Not in scope: the non-streaming `completed` default (`bridge.ts:1829`), still the
+open follow-up in `000_index.md`. This removes one route to it, not the default.
+
+## Done when
+
+All four pass, `bun run typecheck` clean, cursor suite green on `ssh lidge`,
+pushed. Test 1 demonstrated red on the pre-fix tree.
+
+
+## Shipped
+
+Commits `f145fd513` (fix + tests) and `c9681d043` (review follow-up).
+
+The plan's tagging design survived implementation, with one correction the audit
+forced and one it surfaced afterwards:
+
+- **The guard is `!expectedClose && !emittedTerminal`,** not `!expectedClose`
+  alone. Tagging after a terminal was already queued would have added a second
+  one, and buffered JSON processes both — flipping a completed turn to failed.
+- **The typed error carries its own message**, because a raw `NGHTTP2_CANCEL`
+  string is re-matched by `classifyCursorError` and reported as an intentional
+  "Cursor stream suspended". A turn that failed unexpectedly would have claimed a
+  deliberate suspension, which is worse than the silent drop it replaced.
+- **It also re-exposes the originating transport code.** Wrapping hid it from the
+  `turn-failed` diagnostic, leaving the one summary that exists to explain this
+  failure without an error code. The regression test pins the non-obvious
+  consequence: carrying `NGHTTP2_CANCEL` back onto the wrapper must not make
+  `isCursorBenignCancelError` match it again — provenance is checked first.
+
+Implementation note: `classifyTurnFailure` is a closure beside `summarizeFailure`
+rather than a method, because `summarizeFailure` is itself a per-run closure over
+the turn's state. A method could not reach it.
+
+Verified on `ssh lidge` at `c9681d0435`: typecheck clean, 630 pass / 0 fail.
+Red-before-green demonstrated, and independently reproduced by the reviewer as
+4 pass / 1 fail under a mutation that disables the classification.

src/adapters/cursor/cursor-errors.ts

@@ -46,7 +46,37 @@ export class CursorStreamTruncatedError extends Error {
   }
 }
 
+/**
+ * A cancel-shaped stream failure that WE did not request. `cancelCursorRun` is the only place
+ * that cancels our own stream, and it sets `expectedClose` first, so a cancel arriving without it
+ * came from Cursor or the network and is a real transport failure.
+ *
+ * It carries its own message on purpose. Left as a raw `NGHTTP2_CANCEL` error, the text is
+ * re-matched downstream (`classifyCursorError`) and labelled "Cursor stream suspended" — a turn
+ * that failed unexpectedly would report an intentional suspension and misdirect diagnosis.
+ */
+export class CursorUnexpectedCancelError extends Error {
+  /**
+   * The originating error's transport code (typically `NGHTTP2_CANCEL`), re-exposed so the
+   * per-turn `turn-failed` diagnostic still records how the stream actually died. Wrapping
+   * without this made the summary for exactly this failure the one with no code.
+   */
+  public readonly code?: string;
+
+  constructor(public readonly cause?: unknown) {
+    super("Cursor connection was cancelled by the server before the turn completed");
+    this.name = "CursorUnexpectedCancelError";
+    const causeCode = errorCode(cause);
+    if (causeCode) this.code = causeCode;
+  }
+}
+
 export function isCursorBenignCancelError(value: unknown): boolean {
+  // An unexpected cancel is never benign, however it is spelled. This class is raised only when
+  // the transport knows WE did not request the cancel, so its provenance outranks the code match
+  // below — otherwise the adapter would re-decide the same question from the error code alone
+  // and swallow a real transport failure (cursor.ts:181).
+  if (value instanceof CursorUnexpectedCancelError) return false;
   const message = errorMessage(value).toLowerCase();
   const code = errorCode(value).toUpperCase();
   if (code === "NGHTTP2_CANCEL") return true;

src/adapters/cursor/live-transport.ts

@@ -48,7 +48,7 @@ import {
   type InteractionResponse,
 } from "./gen/agent_pb";
 import { debugProviderDiagnostic } from "../../lib/debug";
-import { classifyCursorError, isCursorBenignCancelError, safeCursorErrorMessage } from "./cursor-errors";
+import { classifyCursorError, CursorUnexpectedCancelError, isCursorBenignCancelError, safeCursorErrorMessage } from "./cursor-errors";
 import { mcpArgsFromToolCall } from "./protobuf-events";
 import { OCX_RESPONSES_TOOL_PROVIDER } from "./tool-definitions";
 import {
@@ -529,6 +529,22 @@ class LiveCursorTransport implements CursorTransport {
       }
       return err;
     };
+    /**
+     * A cancel we did not request is a real transport failure, but as a raw `NGHTTP2_CANCEL` it
+     * gets swallowed twice over: the adapter re-decides "benign" from the error code alone
+     * (`cursor.ts:181`) and drops the turn, and any message that survives is re-matched
+     * downstream and labelled an intentional "Cursor stream suspended". Raising a typed error
+     * carries the provenance this class already holds.
+     *
+     * Suppressed once a terminal was emitted: the turn already ended, and a second terminal flips
+     * a completed buffered response to failed.
+     */
+    const classifyTurnFailure = (err: Error): Error => {
+      if (!this.expectedClose && !this.emittedTerminal && isCursorBenignCancelError(err)) {
+        return summarizeFailure(new CursorUnexpectedCancelError(err));
+      }
+      return summarizeFailure(err);
+    };
     const wake = () => {
       const fn = notify;
       notify = undefined;
@@ -628,7 +644,7 @@ class LiveCursorTransport implements CursorTransport {
         // A CANCEL is benign only on the client-tool suspend path (expectedClose); an
         // unexpected server-side NGHTTP2_CANCEL must surface as a real transport error.
         if (this.expectedClose && isCursorBenignCancelError(failure)) return;
-        throw attachPartialUsage(summarizeFailure(failure), state);
+        throw attachPartialUsage(classifyTurnFailure(failure), state);
       }
       if (done) break;
       await new Promise<void>(resolve => {
@@ -637,7 +653,7 @@ class LiveCursorTransport implements CursorTransport {
     }
     if (failure) {
       if (this.expectedClose && isCursorBenignCancelError(failure)) return;
-      throw attachPartialUsage(summarizeFailure(failure), state);
+      throw attachPartialUsage(classifyTurnFailure(failure), state);
     }
   }
 

tests/cursor-cancel-provenance.test.ts

@@ -0,0 +1,138 @@
+import { describe, expect, test } from "bun:test";
+import { createLiveCursorTransport } from "../src/adapters/cursor/live-transport";
+import {
+  classifyCursorError,
+  CursorUnexpectedCancelError,
+  isCursorBenignCancelError,
+} from "../src/adapters/cursor/cursor-errors";
+import { resetCursorBlobStateForTests } from "../src/adapters/cursor/native-exec";
+import { createTestTranslatorBudget } from "./helpers/translator-budget";
+import type { CursorServerMessage } from "../src/adapters/cursor/types";
+
+type OpenFn = (
+  encoded: Uint8Array,
+  signal: AbortSignal | undefined,
+  state: unknown,
+  push: (message: CursorServerMessage) => void,
+  fail: (error: Error) => void,
+  finish: () => void,
+) => void;
+
+function cancelError(): Error {
+  const err = new Error("stream closed with NGHTTP2_CANCEL");
+  (err as { code?: string }).code = "NGHTTP2_CANCEL";
+  return err;
+}
+
+/**
+ * Drive a turn through the transport's fault-injection seam. A literal server-side
+ * `stream.close(NGHTTP2_CANCEL)` surfaces as a clean HTTP/2 `end` in a local fixture, so the
+ * cancel-shaped FAILURE is injected directly — that is the state the production socket handler
+ * reaches when Cursor resets the stream.
+ */
+async function runCancelTurn(opts: {
+  emitTerminalFirst?: boolean;
+  suspendFirst?: boolean;
+}): Promise<{ messages: CursorServerMessage[]; failure?: Error }> {
+  resetCursorBlobStateForTests();
+  const transport = createLiveCursorTransport({
+    provider: { adapter: "cursor", baseUrl: "https://api2.cursor.sh", apiKey: "test-token" },
+    translatorBudget: createTestTranslatorBudget(),
+    headers: new Headers(),
+  });
+  let onOpened!: () => void;
+  const opened = new Promise<void>(resolve => { onOpened = resolve; });
+  let failTurn!: (error: Error) => void;
+  let pushEvent!: (message: CursorServerMessage) => void;
+  (transport as unknown as { open: OpenFn }).open = (_encoded, _signal, _state, push, fail) => {
+    failTurn = fail;
+    pushEvent = push;
+    onOpened();
+  };
+
+  const messages: CursorServerMessage[] = [];
+  let failure: Error | undefined;
+  const iterator = transport.run({
+    modelId: "composer-2.5",
+    conversationId: "cursor_cancel_provenance",
+    system: ["system"],
+    messages: [{ role: "user", content: "hi" }],
+  })[Symbol.asyncIterator]();
+
+  const drain = (async () => {
+    try {
+      for (let next = await iterator.next(); !next.done; next = await iterator.next()) {
+        messages.push(next.value);
+      }
+    } catch (err) {
+      failure = err instanceof Error ? err : new Error(String(err));
+    }
+  })();
+
+  await opened;
+  if (opts.emitTerminalFirst) pushEvent({ type: "done", usage: { inputTokens: 0, outputTokens: 0, totalTokens: 0 } });
+  // The client-tool suspend path cancels our own stream, which sets expectedClose.
+  if (opts.suspendFirst) (transport as unknown as { cancelCursorRun(): void }).cancelCursorRun();
+  failTurn(cancelError());
+  await drain;
+  transport.close?.();
+  return { messages, failure };
+}
+
+describe("Cursor cancel provenance", () => {
+  test("a cancel we did not request surfaces as a real transport failure", async () => {
+    const { failure } = await runCancelTurn({});
+
+    // Before this change the adapter treated ANY NGHTTP2_CANCEL as benign and returned silently,
+    // leaving the turn with zero adapter events — reported as `completed` on the non-streaming path.
+    expect(failure).toBeDefined();
+    expect(failure).toBeInstanceOf(CursorUnexpectedCancelError);
+    // The message must not claim an intentional suspension: that would misdirect diagnosis of a
+    // turn that actually failed.
+    expect(failure?.message.toLowerCase()).not.toContain("suspend");
+    expect(classifyCursorError(failure!.message)).not.toBe("Cursor stream suspended");
+  });
+
+  test("a cancel from our own client-tool suspend stays silent", async () => {
+    const { failure } = await runCancelTurn({ suspendFirst: true });
+
+    // The regression that matters: every working multi-turn tool cycle ends this way.
+    expect(failure).toBeUndefined();
+  });
+
+  test("a cancel after a terminal was already emitted does not add a second one", async () => {
+    const { messages, failure } = await runCancelTurn({ emitTerminalFirst: true });
+
+    expect(messages.some(m => m.type === "done")).toBe(true);
+    // The transport still throws, but as the RAW cancel rather than the typed unexpected-cancel.
+    // That distinction is the whole contract: the adapter's benign check (cursor.ts:181) swallows
+    // a raw cancel, so no second terminal reaches the bridge and an already-completed buffered
+    // response is not flipped to failed.
+    expect(failure).toBeDefined();
+    expect(failure).not.toBeInstanceOf(CursorUnexpectedCancelError);
+    expect(isCursorBenignCancelError(failure)).toBe(true);
+  });
+});
+
+describe("isCursorBenignCancelError provenance", () => {
+  test("an unexpected-cancel error is never benign, but an untagged one still is", () => {
+    expect(isCursorBenignCancelError(new CursorUnexpectedCancelError())).toBe(false);
+    // Fallback preserved: nothing that returns silently today starts erroring without evidence.
+    expect(isCursorBenignCancelError(cancelError())).toBe(true);
+    expect(isCursorBenignCancelError(new Error("Cursor stream suspended"))).toBe(true);
+  });
+
+  test("the unexpected-cancel message is not classified as a suspension", () => {
+    const classified = classifyCursorError(new CursorUnexpectedCancelError().message);
+    expect(classified).not.toBe("Cursor stream suspended");
+  });
+
+  test("the wrapper keeps the originating transport code for diagnostics", () => {
+    // Wrapping must not blind the per-turn `turn-failed` summary for exactly the failure it
+    // exists to explain: without this the one diagnostic that matters has no error code.
+    const wrapped = new CursorUnexpectedCancelError(cancelError());
+    expect((wrapped as { code?: string }).code).toBe("NGHTTP2_CANCEL");
+    // Carrying the code must NOT make it benign again — provenance still wins.
+    expect(isCursorBenignCancelError(wrapped)).toBe(false);
+  });
+});