diff --git a/devlog/_plan/260817_cursor_toolcall_decode/040_phase4-server-cancel-terminal.md b/devlog/_plan/260817_cursor_toolcall_decode/040_phase4-server-cancel-terminal.md new file mode 100644 index 0000000000..6f76ced5df --- /dev/null +++ b/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. diff --git a/src/adapters/cursor/cursor-errors.ts b/src/adapters/cursor/cursor-errors.ts index ee2ab7e0e9..294adc4a7c 100644 --- a/src/adapters/cursor/cursor-errors.ts +++ b/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; diff --git a/src/adapters/cursor/live-transport.ts b/src/adapters/cursor/live-transport.ts index a36144b881..b05acd29d7 100644 --- a/src/adapters/cursor/live-transport.ts +++ b/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(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); } } diff --git a/tests/cursor-cancel-provenance.test.ts b/tests/cursor-cancel-provenance.test.ts new file mode 100644 index 0000000000..548bbd3f05 --- /dev/null +++ b/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(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); + }); +});