fix(cursor): clean-EOF terminal gate and real image content in tool results - #1993
Merged
Conversation
Roadmap unit for the cursor-call hardening loop. Records the decode of protobuf-events/protobuf-request/live-transport, two proven High defects (clean-EOF terminal bypass, tool-result images destroyed despite McpImageContent support), three disproved hypotheses, and the external evidence corroborating each.
Root cause: the freeform custom-tool contract is erased on the openai-chat path (parser.ts:184/189, openai-chat.ts:1183/1194), so Grok sees an ordinary function with no guidance to use it. Decoding already works; this is an affordance defect.
…urned FAIL Ten findings, each re-verified against source before acceptance. Load-bearing ones: the 010 retry claim is impossible because committed is set on HTTP/2 connect; 010 had no single terminal owner; state.terminated also covers the synthetic client-tool finalize; 030 conceded that per-property apply_patch guidance already exists, downgrading its root cause; 030 guidance must be xai-scoped. Also: args buffer on partialToolCall, two terminal rows are expectedClose-conditional, the F2 causal claim is withdrawn, 020 needs a data-URL parser and a conversation-level byte budget.
Round 2 confirmed the 001/003 corrections and found 010 coherent and regression-free, then failed the rest. Two round-1 corrections were themselves unimplementable: 030 xai-scoping has no identity seam at the adapter factory (now declared in-scope, with a same-base-URL isolation test), and 020 decoded-byte budgeting cannot bound a serialized ConversationStep (now measured after serialization, degrade and re-serialize, with a near-limit regression and a byte-identical no-image test). Also: the sibling-edit-tool gate is dropped as undefinable, the shared parseDataUrl is no longer tightened, xAI Responses support is restated correctly, F3 is downgraded to lossy-conversion-proven/cause-unproven, and the Connect vs gRPC-web citation is disambiguated.
All five refine round-2 corrections: 020 must take the effective blob limit from the admission authority rather than copying 16 MiB, and its byte-identical test must freeze randomUUID; 030 identity must be mandatory at the route-resolver boundary so it survives the eight adapter-reconstruction sites, its isolation test must vary only identity on an identical OcxProviderConfig, the sibling-demotion claim is downgraded to an accepted live-test-dependent risk with a specified conditional wording shape, and the xAI paragraph is restated as a claim about the documented contract.
…reproduce F3 Both xai/grok-4.6 and cursor/grok-4.6 subagents edited the probe file and reported apply_patch. The cursor agent named code mode (tools.apply_patch inside exec), which is not the top-level freeform surface 030 theorises about, and self-reported tool names are not wire evidence. 030 now leads with a measurement cycle: capture the wire, probe with code mode disabled, probe a larger patch, and ask the user for the failing case. NOOP with evidence is an acceptable outcome.
Blocker: recordToolCall emits unknown-tool and tool-limit errors without setting state.terminated while leaving earlier calls open (pinned by tests/cursor-protobuf-events.test.ts:534), so the proposed EOF branch would have emitted a second terminal via cursor.ts:180. The EOF predicate is now no-terminal-of-any-kind via an explicit emitted-error flag, with a regression test. Also: 010 test 3 fixture clarified, 030 option 2 made conditional on reproduction with measurement exit criteria and a named probe harness, and the index F3 labels corrected.
Round 5 proved the duplicate-terminal defect already ships on dev: a mapper error does not set state.terminated, so a later real turnEnded emits a second terminal (protobuf-events.ts:1231/1361-1376), and a mapper error followed by a transport failure yields the queued error then throws another via cursor.ts:180. Widening only the EOF predicate would have passed this phase tests while leaving both live. 010 is now scoped to the invariant exactly one terminal per turn, sets the flag at the transport push seam, consults it at three sites, and adds tests 7 and 8 which must be demonstrated red on the unmodified tree.
Round 6 disproved the round-5 premise, verified locally: the bridge already enforces terminal singleness (bridge.ts:1248 streaming, :1619 batch) and tests/bridge-terminal-singleness.test.ts passes 3/3. A second adapter terminal never becomes a second protocol terminal, so round-5 tests 7 and 8 would have been red at the adapter boundary and green where users observe. The push seam also cannot carry a turn-wide invariant since adapter errors bypass it. 010 reverts to F1 only, keeping the emittedTerminal flag as a local EOF guard, and adds an end-to-end test for the user-visible symptom. The NGHTTP2_CANCEL zero-terminal path is recorded as a separate follow-up.
Superseded by dev. dev fixed the same clean-EOF defect first, in the opposite shape: finalizeTurnEvents emits a fail-closed truncation ERROR EVENT and CodeRabbit explicitly rejected throwing (1824a01), because a thrown transport error hides the domain-specific message behind a generic adapter_eof. Traced through both the streaming and buffered Responses paths, dev's event sets errorEvent and sawTerminal, so buildResponseJSON already returns failed with no compaction history — the outcome this commit was written to produce. What survives from the original is emittedTerminal, which f145fd5 depends on to tell an unexpected server CANCEL from one we requested, plus one guard: EOF finalization now also returns early when a terminal was already emitted, so a mapper error that failed the turn cannot be followed by a second terminal. The test expectation changes with the shape: it asserts the error EVENT naming the open call rather than a thrown CursorStreamTruncatedError. The requirement was never "throw" — it was "do not finish silently". Rationale: devlog/_plan/260818_cursor_call_integration/010_phase1.md
…ceholder
Every image part of a Codex tool result was replaced with the literal string
"[image input unsupported by Cursor adapter phase 3: ...]", so a Computer Use
screenshot, browser QA capture, or any image-returning MCP tool reached the model
blind. The Cursor schema has always supported it: McpToolResultContentItem.content
has an image case carrying McpImageContent{data, mimeType}, and native-exec-mcp.ts
already emits it for MCP-invoked tools. Only the Codex OcxToolResultMessage path
was left behind.
toolResultContentItems now maps parts in order, decoding data: URLs into
McpImageContent. Decoding uses a strict helper layered on the shared parseDataUrl
rather than tightening it, since Anthropic, Google, and Command Code depend on that
parser, and base64 is charset-validated because Buffer.from accepts invalid input
silently. Remote https URLs stay placeholders: McpImageContent needs bytes and
fetching would put network IO on the encoding path. Images are budgeted against a
fraction of the live per-blob admission ceiling (cursorBlobMaxEntryBytes, not a
copied constant) because a ConversationStep is stored as one blob shared with the
call arguments and text.
Verified red-before-green: with the old text-only encoding 4 of 6 tests fail;
all 6 pass after.
…ot a fraction An adversarial review proved the previous fraction-of-the-ceiling budget could fail a request that works today: a ConversationStep is stored as ONE blob, so its images share an entry with the call's arguments, text, mime strings, and protobuf framing. With the limit at 1024 bytes, a 448-byte argument plus a 460-byte image produced a 1026-byte step where the pre-change text-placeholder encoding produced 607 — admitted before, rejected after. A budget over decoded image bytes alone cannot see the rest of the step. toolCallStep now serializes the step, compares the real byte length against the live cursorBlobMaxEntryBytes(), and re-serializes with fewer images until it fits. Images are dropped oldest-first so the most recent screenshot — the one the model is reasoning about — is the last to go. Added the audit's exact near-limit scenario as a regression test, plus newest-retained and comfortably-fits cases. Also reworded the remaining text-only placeholders: the external-model replay and unmatched-native paths omit images by design, so saying they are "unsupported by Cursor adapter phase 3" is now false — matching native calls do support them.
Round-2 review found the previous fix could still enlarge a step. At a 1024-byte ceiling an 831-char argument serialized to 993 bytes with the legacy placeholder but 1025 with the new, longer one, so the degraded text ITSELF pushed a previously admissible step past admission. Placeholders are now capped to the length of the string they replaced, which makes the zero-image case byte-bounded by the pre-change encoding rather than merely close to it. The same review measured the shrink loop as quadratic: every re-serialization decoded all images again (~3s for 100 images on a synchronous request path). Parts are now decoded once into a DecodedResultPart[] that every attempt reuses; a 40-image forced degrade runs in ~12ms. Tests compare the emitted placeholder against the legacy string directly instead of guessing an absolute ceiling, so they assert the actual invariant: a degraded image never costs more than the text it replaced.
…their size Round-3 review found the remaining way this feature could enlarge a step: the legacy encoding flattened a tool result's text parts into ONE newline-joined content item, while the new code emitted one protobuf item per part. The extra per-item framing was enough to fail admission for a request that previously fit - at a 1024-byte ceiling a 912-character argument went from 1020 bytes to 1025. Consecutive text (including image placeholders) is now joined into a single item again, with images flushing the run and starting a new one. A result with no images therefore serializes exactly as it did before this feature, which the new tests assert by comparing real serialized step bytes rather than decoded fields: multi-part text is byte-equal to the equivalent joined string across several shapes and sizes.
The user-message prompt path still flattens images to text, and that is correct: this builds the plain-text prompt, and the schema slot that could carry them (UserMessage.selectedContext.selectedImages) is not populated by this adapter. But the wording "unsupported by Cursor adapter phase 3" is no longer true of the adapter as a whole now that tool-result images reach Cursor as real McpImageContent, and a stale claim like that is how the tool-result gap survived this long. Same length, so no byte-budgeted prompt path shifts.
3 tasks
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis change documents Cursor tool-call decoding findings and ships two fixes: truncated clean EOFs now produce explicit errors, and inline tool-result images now pass through as MCP image content with size-aware degradation. It adds transport, serialization, and regression-test coverage. ChangesCursor decoding scope and audit
Clean-EOF terminal handling
Tool-result image serialization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CursorHTTP2
participant LiveTransport
participant ResponsesBridge
CursorHTTP2->>LiveTransport: Send framed tool-call data
CursorHTTP2->>LiveTransport: Close stream before turnEnded
LiveTransport->>LiveTransport: Detect open tool calls
LiveTransport->>ResponsesBridge: Emit CursorStreamTruncatedError
ResponsesBridge->>ResponsesBridge: Mark result failed and incomplete
sequenceDiagram
participant ToolResult
participant ProtobufRequest
participant BlobStore
ToolResult->>ProtobufRequest: Provide text and inline image parts
ProtobufRequest->>ProtobufRequest: Decode data URLs and create MCP content
ProtobufRequest->>BlobStore: Serialize the complete step
ProtobufRequest->>BlobStore: Remove oldest images if the step exceeds the limit
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Aug 18, 2026
Closed
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cursor wire hardening: the clean-EOF terminal gate and real image content in tool results.
Clean EOF (
live-transport.ts). A framed Cursor stream that ended at the HTTP/2 layer with no turn terminal, while a client tool call was still open, settled as success — and because a client tool call defers itstool_call_startto completion, the open call emitted nothing at all. Streaming degraded toresponse.incomplete; the non-streaming path returnedcompletedfor a turn whose tool call had silently vanished.dev fixed this first, in the opposite shape, and dev's shape wins.
6a64db19d+1824a0148emit a fail-closed truncation ERROR EVENT viafinalizeTurnEvents, and CodeRabbit explicitly rejected throwing because a thrown transport error hides the domain-specific message behind a genericadapter_eof. What survives from this branch is theemittedTerminalflag — PR2 depends on it to tell an unexpected server CANCEL from one we requested — plus one guard so EOF finalization cannot append a second terminal after a mapper error already failed the turn.Tool-result images (
protobuf-request.ts). Codex tool-result images reached Cursor as placeholder text. The encoder now emits realMcpImageContent. Three corrections came out of review: bounding is post-serialization against the livecursorBlobMaxEntryBytes()rather than a decoded-byte fraction; placeholders are capped to the legacy string length so a degrade cannot enlarge a step; and consecutive text is newline-joined into one item so a no-image result is byte-identical to the pre-feature encoding.Honest limit: the encoder is correct and does not reach production today. Every Cursor model is in
noVisionModels(providers/registry.ts), so the vision sidecar describes or strips images before this adapter runs. Closing that gap needs role-aware vision policy and is tracked as a follow-up, not claimed here.Verification
Full gate run on
ssh lidgeat the stack tipff4b0bb4ein a dedicated worktree pinned to that SHA:Local focused run on the rebased branch:
bun test tests/cursor-eof-terminal.test.ts tests/cursor-hardening.test.ts tests/cursor-cancel-provenance.test.ts tests/cursor-tool-result-image.test.ts tests/cursor-request-builder.test.ts→ 91 pass / 0 fail.Checklist
devlog/_plan/260817_cursor_toolcall_decode/; no user-facing docs change — the image capability does not reach production yet, sodocs-site/stays accurate.)privacy:scangreen.)Summary by CodeRabbit
New Features
Bug Fixes
Tests