fix(cursor): Add native image support for Cursor - #1228
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds Cursor vision support for image resolution, selected-image protobuf encoding, tool-result promotion, native and sidecar model metadata, and request replay. It also updates Grok 4.5 effort-tier wire IDs and continuation handling. ChangesCursor model capabilities and Grok mapping
Cursor vision request flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant LiveTransport
participant CursorImages
participant ProtobufRequest
participant Cursor
Client->>LiveTransport: submit raw messages
LiveTransport->>CursorImages: prepare and resolve images
CursorImages-->>LiveTransport: prepared messages and selected images
LiveTransport->>ProtobufRequest: build Cursor request
ProtobufRequest->>Cursor: send selected context and MCP image content
Cursor-->>ProtobufRequest: return continuation or tool result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
|
✅ Deterministic PR hygiene checks passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/adapters/cursor/images.ts`:
- Around line 271-305: Update resolveCursorImages so an image whose prepared
data exceeds MAX_CURSOR_IMAGE_BYTES is omitted with continue instead of throwing
CursorImageError. Preserve the existing behavior for other validation failures
and continue processing subsequent images, aligning this path with
prepareCursorImageDataUrl’s omission policy.
- Around line 459-475: Update the JPEG marker scan in the dimension-detection
function to recognize standalone markers 0xD0–0xD9 and 0x01 before reading a
length, advancing past them without consuming length bytes. Preserve the
existing SOF0/SOF2 dimension extraction and malformed-length handling for
markers that carry a length.
- Around line 564-593: Restrict isTransparentCursorVisionSuffix to developer
messages containing the known Codex Desktop <multi_agent_mode> guidance block,
rather than any image-free developer message. Preserve intentional developer
instructions, while keeping stripTrailingTransparentDeveloperMessages and
cursorIsTrailingToolResultContinuation behavior unchanged for the recognized
guidance suffix.
In `@src/adapters/cursor/live-transport.ts`:
- Around line 573-579: Eliminate duplicate image preparation across both sites:
in src/adapters/cursor/live-transport.ts lines 573-579, make
resolveActiveCursorImages reuse the output of prepareCursorRawMessages and
enforce one deadline for the complete image phase; in
src/adapters/cursor/images.ts lines 664-709, update prepareCursorContentParts to
bypass parts already marked image/jpeg and within the soft cap, preventing
repeated processing of accumulated view_image history.
In `@src/adapters/cursor/protobuf-request.ts`:
- Around line 364-418: Export the existing decodeDataUrl helper from images.ts,
then replace the inline data-URL parsing and Buffer.from logic in
toolResultContentItems with a call to decodeDataUrl inside the existing try
block. Preserve the current omission behavior by returning no image item when
decoding fails or the shared size limits reject the payload.
- Around line 534-538: Update the flush function to skip pending external-model
MCP tool calls that have no result, while preserving image-path pairing and
existing handling for answered calls. Do not emit result-less external entries
through toolCallStep; only append valid completed calls to current.steps.
In `@tests/cursor-blob.test.ts`:
- Around line 838-1196: Add focused regression coverage for the live-transport
image phase, targeting the wiring around prepareCursorRawMessages,
resolveActiveCursorImages, and preparedRequest in the live transport flow.
Exercise the production path end to end so the resolver receives the prepared
rawMessages and produces selectedImages, rather than injecting selectedImages
directly through encodeCursorRunRequest; place the test with the existing cursor
image or live-transport tests.
In `@tests/cursor-request-builder.test.ts`:
- Around line 223-254: Add a focused regression test alongside the existing
createCursorRequest tests for a conversation containing an assistant message
whose content consists only of a toolCall part. Assert that this assistant
message converts to undefined and is omitted from request.messages, and verify
the resulting final message role remains the expected user role for
protobuf-request lastRole handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a4299e02-aacb-425b-bb01-2dfefc5c30fb
⛔ Files ignored due to path filters (1)
tests/helpers/cursor-grumpy-fixture.pngis excluded by!**/*.png
📒 Files selected for processing (22)
docs-site/src/content/docs/reference/adapters.mddocs-site/src/content/docs/reference/configuration/providers.mdsrc/adapters/cursor/discovery.tssrc/adapters/cursor/effort-map.tssrc/adapters/cursor/images.tssrc/adapters/cursor/live-transport.tssrc/adapters/cursor/protobuf-request.tssrc/adapters/cursor/request-builder.tssrc/adapters/cursor/types.tssrc/providers/registry.tssrc/types.tstests/catalog-vision-sidecar-modalities.test.tstests/cursor-blob.test.tstests/cursor-discovery.test.tstests/cursor-effort-suffix.test.tstests/cursor-images.test.tstests/cursor-request-builder.test.tstests/cursor-static-catalog.test.tstests/cursor-vision-wire-harness.test.tstests/model-in-list.test.tstests/oauth-provider-reconcile.test.tstests/provider-registry-parity.test.ts
| // JPEG: scan for SOF0/SOF2 marker with dimensions | ||
| if (data.byteLength >= 4 && data[0] === 0xff && data[1] === 0xd8) { | ||
| let offset = 2; | ||
| while (offset + 8 < data.byteLength) { | ||
| if (data[offset] !== 0xff) break; | ||
| const marker = data[offset + 1]!; | ||
| const length = (data[offset + 2]! << 8) | data[offset + 3]!; | ||
| if (marker === 0xc0 || marker === 0xc2) { | ||
| const height = (data[offset + 5]! << 8) | data[offset + 6]!; | ||
| const width = (data[offset + 7]! << 8) | data[offset + 8]!; | ||
| if (width > 0 && height > 0) return { width, height }; | ||
| break; | ||
| } | ||
| if (length < 2) break; | ||
| offset += 2 + length; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard the JPEG marker scan against standalone markers.
The scan at lines 460-475 assumes every marker after SOI carries a two-byte length. Standalone markers (0xD0-0xD9, 0x01) carry no length. If one appears before SOF0/SOF2, the loop reads the next two bytes as a length and jumps to a wrong offset, so the function returns undefined or, in a crafted file, a wrong width/height. A wrong dimension is written into SelectedImage.dimension by buildSelectedImages at line 497.
The function is documented as best-effort, so the fallback is safe, but the fix is one condition.
🐛 Proposed fix for standalone JPEG markers
let offset = 2;
while (offset + 8 < data.byteLength) {
if (data[offset] !== 0xff) break;
const marker = data[offset + 1]!;
+ // Standalone markers (RSTn, SOI, EOI, TEM) carry no length payload.
+ if (marker === 0x01 || (marker >= 0xd0 && marker <= 0xd9)) {
+ offset += 2;
+ continue;
+ }
const length = (data[offset + 2]! << 8) | data[offset + 3]!;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // JPEG: scan for SOF0/SOF2 marker with dimensions | |
| if (data.byteLength >= 4 && data[0] === 0xff && data[1] === 0xd8) { | |
| let offset = 2; | |
| while (offset + 8 < data.byteLength) { | |
| if (data[offset] !== 0xff) break; | |
| const marker = data[offset + 1]!; | |
| const length = (data[offset + 2]! << 8) | data[offset + 3]!; | |
| if (marker === 0xc0 || marker === 0xc2) { | |
| const height = (data[offset + 5]! << 8) | data[offset + 6]!; | |
| const width = (data[offset + 7]! << 8) | data[offset + 8]!; | |
| if (width > 0 && height > 0) return { width, height }; | |
| break; | |
| } | |
| if (length < 2) break; | |
| offset += 2 + length; | |
| } | |
| } | |
| // JPEG: scan for SOF0/SOF2 marker with dimensions | |
| if (data.byteLength >= 4 && data[0] === 0xff && data[1] === 0xd8) { | |
| let offset = 2; | |
| while (offset + 8 < data.byteLength) { | |
| if (data[offset] !== 0xff) break; | |
| const marker = data[offset + 1]!; | |
| // Standalone markers (RSTn, SOI, EOI, TEM) carry no length payload. | |
| if (marker === 0x01 || (marker >= 0xd0 && marker <= 0xd9)) { | |
| offset += 2; | |
| continue; | |
| } | |
| const length = (data[offset + 2]! << 8) | data[offset + 3]!; | |
| if (marker === 0xc0 || marker === 0xc2) { | |
| const height = (data[offset + 5]! << 8) | data[offset + 6]!; | |
| const width = (data[offset + 7]! << 8) | data[offset + 8]!; | |
| if (width > 0 && height > 0) return { width, height }; | |
| break; | |
| } | |
| if (length < 2) break; | |
| offset += 2 + length; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/adapters/cursor/images.ts` around lines 459 - 475, Update the JPEG marker
scan in the dimension-detection function to recognize standalone markers
0xD0–0xD9 and 0x01 before reading a length, advancing past them without
consuming length bytes. Preserve the existing SOF0/SOF2 dimension extraction and
malformed-length handling for markers that carry a length.
| // JPEG soft-cap rewrite for attach + view_image tool-result data URLs before encode. | ||
| const rawMessages = await prepareCursorRawMessages(request.rawMessages); | ||
| const selectedImages = await resolveActiveCursorImages(rawMessages, signal); | ||
| const preparedRequest = { ...request, rawMessages, selectedImages }; | ||
| // Build the payload once. The estimate is only worth deriving when there is no | ||
| // carry-forward to fall back on — with a carry present it would never be used (#373). | ||
| const prepared = prepareCursorRunRequest(request, { | ||
| const prepared = prepareCursorRunRequest(preparedRequest, { |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Redundant image preparation on the request path. Both sites run the same expensive prep without memoization: prepareCursorImageDataUrl performs a validating Bun.Image decode plus a JPEG quality ladder, and nothing records that a given payload was already prepared. The result is repeated work per turn, serially, before the HTTP/2 stream opens. One memoized preparation step, keyed by the source data URL, removes both costs.
src/adapters/cursor/live-transport.ts#L573-L579: reuse the output ofprepareCursorRawMessagessoresolveActiveCursorImagesdoes not decode and re-encode the active images a second time, and bound the whole image phase with a single deadline instead of relying on the per-imageIMAGE_FETCH_TIMEOUT_MS.src/adapters/cursor/images.ts#L664-L709: inprepareCursorContentParts, skip parts that are alreadyimage/jpegand at or under the soft cap, so accumulatedview_imagehistory is not re-prepared on every subsequent turn.
📍 Affects 2 files
src/adapters/cursor/live-transport.ts#L573-L579(this comment)src/adapters/cursor/images.ts#L664-L709
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/adapters/cursor/live-transport.ts` around lines 573 - 579, Eliminate
duplicate image preparation across both sites: in
src/adapters/cursor/live-transport.ts lines 573-579, make
resolveActiveCursorImages reuse the output of prepareCursorRawMessages and
enforce one deadline for the complete image phase; in
src/adapters/cursor/images.ts lines 664-709, update prepareCursorContentParts to
bypass parts already marked image/jpeg and within the soft cap, preventing
repeated processing of accumulated view_image history.
| function toolResultContentItems( | ||
| content: OcxToolResultMessage["content"], | ||
| options?: { omitImages?: boolean }, | ||
| ) { | ||
| if (typeof content === "string") { | ||
| return [create(McpToolResultContentItemSchema, { | ||
| content: { case: "text", value: create(McpTextContentSchema, { text: content }) }, | ||
| })]; | ||
| } | ||
|
|
||
| const items = content.flatMap(part => { | ||
| if (part.type === "text" && part.text.length > 0) { | ||
| return [create(McpToolResultContentItemSchema, { | ||
| content: { case: "text", value: create(McpTextContentSchema, { text: part.text }) }, | ||
| })]; | ||
| } | ||
| if (part.type === "image" && typeof part.imageUrl === "string" && part.imageUrl.length > 0) { | ||
| if (options?.omitImages) { | ||
| return [create(McpToolResultContentItemSchema, { | ||
| content: { case: "text", value: create(McpTextContentSchema, { text: CURSOR_VISION_MCP_IMAGE_OMITTED }) }, | ||
| })]; | ||
| } | ||
| try { | ||
| if (!part.imageUrl.toLowerCase().startsWith("data:")) return []; | ||
| const comma = part.imageUrl.indexOf(","); | ||
| if (comma < 0) return []; | ||
| const header = part.imageUrl.slice(5, comma); | ||
| const payload = part.imageUrl.slice(comma + 1).replace(/\s/g, ""); | ||
| if (!/;base64/i.test(header) || payload.length === 0) return []; | ||
| const mimeType = (header.split(";")[0] || "").trim().toLowerCase() || "image/png"; | ||
| if (!mimeType.startsWith("image/")) return []; | ||
| const data = Buffer.from(payload, "base64"); | ||
| if (data.byteLength === 0) return []; | ||
| return [create(McpToolResultContentItemSchema, { | ||
| content: { | ||
| case: "image", | ||
| value: create(McpImageContentSchema, { | ||
| data, | ||
| mimeType, | ||
| }), | ||
| }, | ||
| })]; | ||
| } catch { | ||
| return []; | ||
| } | ||
| } | ||
| return []; | ||
| }); | ||
|
|
||
| return items.length > 0 | ||
| ? items | ||
| : [create(McpToolResultContentItemSchema, { | ||
| content: { case: "text", value: create(McpTextContentSchema, { text: "" }) }, | ||
| })]; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reuse decodeDataUrl here; the inline parser has no size guard and duplicates the resolver.
Lines 386-396 re-implement the data-URL parsing that decodeDataUrl already performs in src/adapters/cursor/images.ts lines 121-157: same slice(5, comma) header split, same ;base64 test, same whitespace strip, same MIME defaulting. The copy omits the two limits the original enforces, MAX_CURSOR_IMAGE_DECODE_BYTES and the estimated-decoded-bytes precheck.
The failure mode is concrete. Buffer.from(payload, "base64") at line 395 materializes an unbounded buffer, the bytes land in the ConversationStep blob, and storeCursorBlob then throws CursorBlobAdmissionError when admission rejects the oversized entry. prepareCursorRunRequest propagates that, so the whole turn fails instead of degrading to a text-only tool result. prepareCursorRawMessages does cap this path, but only for callers that route through src/adapters/cursor/live-transport.ts; encodeCursorRunRequest has no such guarantee.
Export decodeDataUrl from images.ts and call it inside the try, so the limits and the omission behavior stay in one place.
♻️ Proposed reuse of the shared decoder
try {
if (!part.imageUrl.toLowerCase().startsWith("data:")) return [];
- const comma = part.imageUrl.indexOf(",");
- if (comma < 0) return [];
- const header = part.imageUrl.slice(5, comma);
- const payload = part.imageUrl.slice(comma + 1).replace(/\s/g, "");
- if (!/;base64/i.test(header) || payload.length === 0) return [];
- const mimeType = (header.split(";")[0] || "").trim().toLowerCase() || "image/png";
- if (!mimeType.startsWith("image/")) return [];
- const data = Buffer.from(payload, "base64");
- if (data.byteLength === 0) return [];
+ // Shared decoder enforces MAX_CURSOR_IMAGE_DECODE_BYTES and MIME validation.
+ const { data, mimeType } = decodeCursorImageDataUrl(part.imageUrl);
+ if (data.byteLength === 0) return [];
return [create(McpToolResultContentItemSchema, {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/adapters/cursor/protobuf-request.ts` around lines 364 - 418, Export the
existing decodeDataUrl helper from images.ts, then replace the inline data-URL
parsing and Buffer.from logic in toolResultContentItems with a call to
decodeDataUrl inside the existing try block. Preserve the current omission
behavior by returning no image item when decoding fails or the shared size
limits reject the payload.
| const flush = () => { | ||
| if (!current) return; | ||
| for (const part of pendingToolCalls.values()) current.steps.push(toolCallStep(part, requestScope)); | ||
| for (const part of pendingToolCalls.values()) { | ||
| current.steps.push(toolCallStep(part, requestScope, undefined, options)); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for coverage of unanswered tool calls on external Cursor models.
set -euo pipefail
rg -n -C8 'pendingToolCalls|invalid_argument' --type=ts -g '!src/adapters/cursor/gen/**'
rg -n -C10 'parallel|two tool calls|call_b' tests/cursor-blob.test.tsRepository: lidge-jun/opencodex
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'src/adapters/cursor|tests/cursor|protobuf-request' || true
echo
echo "Target file outline/size:"
wc -l src/adapters/cursor/protobuf-request.ts 2>/dev/null || true
ast-grep outline src/adapters/cursor/protobuf-request.ts 2>/dev/null | sed -n '1,220p' || true
echo
echo "Relevant source lines:"
sed -n '480,620p' src/adapters/cursor/protobuf-request.ts
echo
echo "Search relevant terms repository-wide:"
rg -n -C6 --glob '!src/adapters/cursor/gen/**' --glob '!node_modules/**' --glob '!dist/**' \
'pendingToolCalls|native mcpToolCall|mcpToolCall|invalid_argument|toolResult|toolCall|externalModel|toolCallStep' \
.Repository: lidge-jun/opencodex
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Target file lines 480-620:"
sed -n '480,620p' src/adapters/cursor/protobuf-request.ts
echo
echo "Target file lines 510-560 with only matching nearby identifiers:"
rg -n -C4 'externalModel|pendingToolCalls|pendingToolCalls|toolResult|toolCallStep|invalid_argument|current??.*steps|flush|message' src/adapters/cursor/protobuf-request.ts
echo
echo "Imports and type definitions in target file:"
sed -n '1,80p' src/adapters/cursor/protobuf-request.ts
echo
echo "Tool result image helper / toolCallStep definitions:"
rg -n -C10 'function toolCallStep|toolCallStep|toolResultImage|invalid_argument|mcpToolCall' src/adapters/cursor/protobuf-request.ts src/adapters/cursor tests -g '*.ts'Repository: lidge-jun/opencodex
Length of output: 50376
Do not flush result-less external-model MCP tool calls.
In src/adapters/cursor/protobuf-request.ts, external-tool-call parts enter pendingToolCalls at line 558, and flush() at line 536-537 emits every remaining entry as a native toolCall step with no result. External workers replay only assistant text; these dangling mcpToolCall steps can hydrate blobs, reach step completion without a result, and reject the turn with invalid_argument. Keep the image-path pairing, but drop unanswered external calls at flush time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/adapters/cursor/protobuf-request.ts` around lines 534 - 538, Update the
flush function to skip pending external-model MCP tool calls that have no
result, while preserving image-path pairing and existing handling for answered
calls. Do not emit result-less external entries through toolCallStep; only
append valid completed calls to current.steps.
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a76c091700
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // JPEG soft-cap rewrite for attach + view_image tool-result data URLs before encode. | ||
| const rawMessages = await prepareCursorRawMessages(request.rawMessages); | ||
| const selectedImages = await resolveActiveCursorImages(rawMessages, signal); | ||
| const preparedRequest = { ...request, rawMessages, selectedImages }; |
There was a problem hiding this comment.
When prepareCursorRawMessages() converts an active user/developer image part into text (for example an unsupported or corrupt image), only rawMessages is replaced here while request.messages still contains the pre-rewrite text that deliberately dropped image parts. activePromptText() then reads the stale message first, so the omission marker is lost; for an image-only turn this even falls through to resumeAction with no user text or SelectedImage. Recompute/sync the prepared messages from the rewritten raw messages before encoding.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| return { | ||
| parts: kept.map(entry => entry.part), | ||
| omittedOlder: Math.max(0, entries.length - kept.length), | ||
| promotedCallIds: new Set(kept.map(entry => entry.callId)), |
There was a problem hiding this comment.
When a single trailing tool result contains more than MAX_CURSOR_IMAGES images, kept contains only the newest images but this set records just the call id. The protobuf builder later treats that call id as fully promoted and replaces every image in that tool result with the SelectedImage marker, so older overflow images are neither promoted nor kept on MCP as intended. Track the promoted image parts/indices instead of the whole call id.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| export function cursorIsTrailingToolResultContinuation( | ||
| messages: readonly OcxMessage[] | undefined, | ||
| ): boolean { | ||
| if (!messages?.length) return false; | ||
| return stripTrailingTransparentDeveloperMessages(messages).at(-1)?.role === "toolResult"; |
There was a problem hiding this comment.
For view_image continuations followed by Desktop's trailing multi-agent developer message, this helper now correctly reports a tool-result continuation after stripping that suffix, but createCursorAdapter still uses the raw last-message role for its invalid-argument retry guard (src/adapters/cursor.ts:114). In that Desktop shape the guard is false, so a pre-output Cursor invalid_argument can be replayed in a fresh conversation even though tool-result resumes are explicitly excluded from retry; use this helper there too.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| return [create(McpToolResultContentItemSchema, { | ||
| content: { | ||
| case: "image", | ||
| value: create(McpImageContentSchema, { | ||
| data, |
There was a problem hiding this comment.
When a later user turn follows an earlier view_image, that prior tool result is no longer the active image turn, but this branch still decodes the historical data URL into McpImageContent while building retained conversation turns. The root replay budget only accounts for text because contentToText drops images, so several retained screenshots can silently blow the Cursor request/hydration payload even though SelectedImage is active-turn-only; gate image MCP content to the trailing active tool-result block or budget these bytes.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| } | ||
| const resolved = url.toLowerCase().startsWith("data:") | ||
| ? decodeDataUrl(url) | ||
| : await fetchHttpsImageBytes(url, signal); |
There was a problem hiding this comment.
If the active Cursor turn contains an HTTPS image URL that fails DNS, times out, returns a non-image content type, or otherwise cannot be fetched, this await propagates CursorImageError through resolveActiveCursorImages() and the live transport aborts the whole request before Cursor sees the text. Data URLs in the same path are safely converted to omission text when they cannot be prepared, so remote image failures should degrade the same way rather than fail an otherwise valid turn.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
|
Addressed the high-signal automated review items:
Verification: focused unit suite (157 pass) + typecheck; collab-on headless attach + |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/reference/configuration/providers.md`:
- Around line 275-278: Update the Vision section in the provider configuration
documentation to explicitly include the curated GLM entries from noVisionModels
as vision-sidecar users, or clarify that auto and composer-* are examples rather
than an exhaustive list. Keep the documented model behavior aligned with the
actual noVisionModels configuration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8e25a38a-1fb3-4959-80ba-33269f6ad9c0
📒 Files selected for processing (8)
docs-site/src/content/docs/reference/configuration/providers.mdsrc/adapters/cursor.tssrc/adapters/cursor/images.tssrc/adapters/cursor/live-transport.tssrc/adapters/cursor/request-builder.tstests/cursor-adapter.test.tstests/cursor-images.test.tstests/cursor-request-builder.test.ts
| the curated `noVisionModels` list and use the vision describe sidecar instead. Trailing | ||
| `<multi_agent_mode>` developer injections (Codex Desktop collab guidance after `view_image`) | ||
| are transparent for SelectedImage promotion so the continuation still carries the image and | ||
| promote nudge. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include GLM in the vision-sidecar description.
Line 282 identifies GLM as part of the curated noVisionModels set. Models in this set use the vision describe sidecar. The Vision section names only auto and composer-*, which can imply that GLM uses native SelectedImage vision.
Include the curated GLM entries, or state that auto and composer-* are examples.
Proposed fix
- the curated `noVisionModels` list and use the vision describe sidecar instead.
+ the curated `noVisionModels` list and use the vision describe sidecar instead. This also includes the curated GLM entries.As per path instructions, user-facing documentation must stay synchronized with actual behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| the curated `noVisionModels` list and use the vision describe sidecar instead. Trailing | |
| `<multi_agent_mode>` developer injections (Codex Desktop collab guidance after `view_image`) | |
| are transparent for SelectedImage promotion so the continuation still carries the image and | |
| promote nudge. | |
| the curated `noVisionModels` list and use the vision describe sidecar instead. This also includes the curated GLM entries. Trailing | |
| `<multi_agent_mode>` developer injections (Codex Desktop collab guidance after `view_image`) | |
| are transparent for SelectedImage promotion so the continuation still carries the image and | |
| promote nudge. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/reference/configuration/providers.md` around lines
275 - 278, Update the Vision section in the provider configuration documentation
to explicitly include the curated GLM entries from noVisionModels as
vision-sidecar users, or clarify that auto and composer-* are examples rather
than an exhaustive list. Keep the documented model behavior aligned with the
actual noVisionModels configuration.
Source: Path instructions
|
Also landed the low-risk optional review cleanups:
Verification: focused unit suite + typecheck green. |
Resolve active-turn images before prepare/encode, attach selectedContext on the active UserMessage, omit image placeholders from text, and remove Cursor from noVisionModels so native vision replaces the sidecar path.
Add focused unit tests for images.ts caps, data URL validation, HTTPS/SSRF rejection, and active-turn selection. Extend cursor-blob coverage to assert history/root UserMessages omit selectedContext while the active turn keeps inline SelectedImage data.
Assert Cursor is absent from noVisionModels, advertises image through modelInputModalities, keeps catalog hints sidecar-free, and skips the vision sidecar when requests carry images.
Keep image-only user/developer turns in request-builder as empty-string active messages, and choose userMessageAction when selectedImages are present even if prompt text is empty. Add regression tests for first-turn and follow-up image-only encodes.
External Cursor models flattened tool results to text via contentToText(), which dropped image parts from view_image continuations. Route image-bearing tool results through the native mcpToolCall/McpImageContent path while keeping text-only external replay flattened. Adds grok-4.5 wire regression coverage and a reusable vision harness test.
Prep before the 1 MiB hard cap, promote consecutive view_image tool results with nudge text and MCP dedupe, fail-closed on undecodable MIME, and restore curated Auto/Composer sidecar noVisionModels. Co-authored-by: Cursor <cursoragent@cursor.com>
Treat trailing non-image developer injections as transparent for SelectedImage resolution and tool-continuation gates so Codex Desktop collab guidance no longer drops vision after view_image. Also enforce the JPEG soft-cap with edge shrink and map Grok none/minimal to medium with the cursor- wire prefix. Co-authored-by: Cursor <cursoragent@cursor.com>
Scope transparent developer suffixes to multi_agent_mode, reuse that continuation helper for invalid_argument retry gating, omit oversized prepared images instead of failing the turn, and rebuild text messages after JPEG prep so omission markers reach activePromptText. Co-authored-by: Cursor <cursoragent@cursor.com>
Skip standalone JPEG markers before SOF so SelectedImage dimensions stay correct, reuse decodeCursorImageDataUrl for MCP tool-result encode size limits, and cover the live-transport prepare→resolve→SelectedImage wiring. Co-authored-by: Cursor <cursoragent@cursor.com>
6c43f20 to
869a4b0
Compare
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
I would not merge this yet. The overall SelectedImage/SSRF approach looks reasonable, but there are several correctness/availability blockers in the current head.
Merge blockers
-
HTTPS image failures can abort or erase the active user turn.
prepareCursorRawMessages()only rewritesdata:images. Remote images are resolved later. DNS failures, timeouts, non-image responses, etc. currently propagate and abort the whole request. Worse, if an HTTPS image fetch succeeds butprepareCursorImageForWire()later omits it, an image-only user turn can end up withselectedImages=[]and empty text, causing protobuf construction to selectresumeAction. Cursor then receives no new user input. Remote failures should degrade consistently to an omission marker/text-only turn, never silently become resume. -
Historical
view_imagepixels are replayed after the active image turn.
conversationTurns()still serializes old image-bearing tool results asMcpImageContent. Those bytes are outside the normal root replay text budget, so accumulated screenshots can materially inflate the request/hydration payload and keep getting retransmitted to Cursor after they are no longer needed. Gate MCP image bytes to the active trailing tool-result block, or explicitly budget/omit historical image bytes. -
Result-less external-model MCP tool calls are emitted during replay.
External assistant tool calls go intopendingToolCalls, butflush()emits every remaining entry throughtoolCallStep()even without a result. This conflicts with the surrounding external-model handling that intentionally avoids native MCP tool-call replay because those structures can causeinvalid_argument. Unanswered/interrupted calls should be dropped for external models; completed image-call/result pairs still need to be preserved. -
Promotion overflow is tracked at call-ID granularity instead of image granularity.
extractTrailingToolResultImagePromotion()keeps the newestMAX_CURSOR_IMAGESentries but returns onlypromotedCallIds. If one tool result contains more than 12 images, that call ID is marked promoted and protobuf omits all images from that result. The older overflow images are therefore neither promoted nor retained on MCP, despite the stated behavior. Track the promoted image parts/indices rather than the entire call ID. -
Image preprocessing is duplicated and has no aggregate deadline.
The live path runsprepareCursorRawMessages()and thenresolveActiveCursorImages(), causing activedata:images to be decoded/re-encoded again. Historical data URLs are also prepared again on subsequent turns. HTTPS processing is serial with a per-image timeout, so 12 stalled images can delay stream creation for a very long time. Reuse prepared results and bound the entire image phase with one deadline/budget.
Security/resource hardening
- The SSRF design itself looks good: HTTPS-only, destination checks, public DNS resolution, and pinned connection/SNI behavior. I did not find an obvious auth bypass, path traversal, command injection, or network-pivot issue in this diff.
- The 16 MiB encoded-byte ceiling does not bound decoded pixel memory.
prepareCursorImageForWire()forces a fullBun.Imagedecode before resize/dimension limiting, so a highly compressed image with huge dimensions can still create a decompression/memory-DoS path. Add max dimensions / total-pixel limits before expensive processing where possible. Buffer.from(..., "base64")is lenient, so malformed base64 is not reliably rejected by the current try/catch. Also, accepting any undecodable <=64-byte payload for PNG/JPEG/GIF/WebP is a production validation bypass added for unit-test stubs. Tests should use real minimal images instead; production should stay fail-closed.
Required regression coverage
- image-only HTTPS unsupported/corrupt image must produce a user action with an omission marker, never
resumeAction - failed HTTPS image fetch plus valid text must continue text-only
- one tool result with >12 images must promote exactly 12 and preserve overflow correctly
- external assistant tool call without a result must not produce a result-less historical
mcpToolCall - prior
view_imagefollowed by a new user turn must not replay historicalMcpImageContent - malformed base64 must be rejected strictly; remove the <=64-byte production bypass
- add pixel/dimension limits and a bounded aggregate image-processing deadline
Some existing bot comments are already stale on this head (JPEG standalone-marker handling, shared capped data-URL decoder use, message rebuild after prep, retry continuation helper), so those should not be fixed twice.
Please address the blockers above and get the PR checks green before merge.
|
@yansigit Status check on this one, and what it needs to move. It is currently Deliberately not rebasing this for you. The other stale PRs I picked up in this pass were small, mechanical rebases where the author's intent was unambiguous. This one adds native image support across eight files of the Cursor adapter, including the protobuf request builder and live transport; resolving those conflicts means re-deciding your own design against a moved base, and I would be guessing at your intent rather than preserving it. To move it forward:
If you would rather not carry it, say so and I will close it as stale with the work preserved in the record — no judgement either way. If you do want to keep going, ping me after the rebase and I will review promptly rather than leaving it to age again. |
3 participants
Summary
SelectedImage(blobIdWithData, JPEG soft-cap with edge shrink,view_image→ SelectedImage promotion + MCP image omit).developerinjections as transparent for vision/tool-continuation so Codex Desktop no longer drops SelectedImage afterview_imageand hallucinates.cursor/grok-4.5, map Codex effortnone/minimalto wiremediumand send live-discoverycursor-grok-4.5-{low,medium,high}ids (Fast still uses parameterizedgrok-4.5).Verification
bun test tests/cursor-images.test.ts tests/cursor-blob.test.ts tests/cursor-effort-suffix.test.ts(110 pass)bun run typecheckorigin/devview_imageon Grumpy): exact captionsI HATE WHEN/PEOPLE USE ME TO ILLUSTRATE A POINT(no geometry/Vite hallucinations)bun run src/cli/index.ts ensurebefore smokeChecklist
Made with Cursor
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes