fix(cursor): Add native image support for Cursor - #1228
Conversation
📝 WalkthroughWalkthroughCursor now supports native and sidecar-backed vision flows. It validates and prepares active-turn images, encodes selected context and MCP image content, promotes tool-result images, preserves image-only turns, updates model capability metadata, and maps Grok 4.5 reasoning tiers. ChangesCursor vision and request handling
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant LiveTransport
participant CursorImages
participant ProtobufRequest
participant Cursor
Client->>LiveTransport: submit messages with image parts
LiveTransport->>CursorImages: prepare active images
CursorImages->>CursorImages: validate, fetch, resize, and store bytes
CursorImages->>ProtobufRequest: provide rewritten messages and selected images
ProtobufRequest->>Cursor: encode selected context and MCP image content
Cursor-->>Client: return Cursor response
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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
yansigit
changed the title
|
|
![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 👍 / 👎.
There was a problem hiding this comment.
Already fixed on current head: live-transport rebuilds messages via cursorRequestMessagesFromRaw after prepare. Closing as stale.
| 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 👍 / 👎.
There was a problem hiding this comment.
Already fixed on current head: promote omit uses per-part keys (callId#index); overflow in the same toolResult stays on MCP. Closing as stale.
| 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 👍 / 👎.
There was a problem hiding this comment.
Already fixed on current head: retry gating uses cursorIsTrailingToolResultContinuation. Closing as stale.
| 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 👍 / 👎.
There was a problem hiding this comment.
Already fixed on current head: historical MCP images outside the trailing promote window are omitted. Closing as stale.
| } | ||
| 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 👍 / 👎.
There was a problem hiding this comment.
Already fixed on current head: HTTPS/decode failures soft-omit to omission text instead of aborting the turn. Closing as stale.
|
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
|
Also landed the low-risk optional review cleanups:
Verification: focused unit suite + typecheck green. |
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. |
869a4b0 to
0f04a8c
Compare
|
@lidge-jun @Wibias — rebased onto current Wibias blockers
Security / docs
Intentionally not expanded
Please re-review when CI settles. (Linux |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Updated full review on current head 0f04a8c487b73c352328050f777b248a9faed56a.
The prior blockers are substantially improved: HTTPS failures now soft-omit and rebuild the text channel; historical MCP image bytes are omitted in replay; external result-less MCP calls are dropped; promotion is tracked per image part; retry gating uses the continuation helper; base64 validation is strict; and PNG/JPEG/GIF dimension/pixel caps plus an aggregate image-phase signal were added. The HTTPS SSRF boundary also still looks sound: HTTPS-only, destination/DNS validation, pinned peer, original Host/SNI, and certificate verification. I did not find an obvious secret leak, auth bypass, path traversal, command injection, or SSRF pivot in this diff.
I still would not merge this head. Remaining blockers:
-
P1/P2 correctness + validation bypass: the small-JPEG fast path trusts only
image/jpeg+FF D8.
InprepareCursorImageForWire,alreadySmallJpegreturnsreadybefore the fullBun.Imagedecode when the declared MIME is JPEG, the payload is under the soft cap, and the first two bytes areFF D8. A truncated/corrupt payload such asFF D8 00 00therefore reachesSelectedImageeven though this path is documented fail-closed. There is a second mismatch case after the generic decode: a valid small PNG/WebP/GIF declared asimage/jpegis returned unchanged as JPEG, so bytes, MIME and attachment extension disagree. The existing fake-JPEG regression uses0xFFbytes and does not hit theFF D8shortcut. Validate the actual format before passthrough, or carry explicit provenance for an image already produced by the JPEG prep stage. Add regressions for truncatedFF D8...and valid PNG bytes mislabeled as JPEG. -
P1/P2 resource hardening: WebP still bypasses the pre-decode pixel/edge guard.
WebP is accepted byCURSOR_VISION_PASSTHROUGH_MIME, butsniffCursorImageDimensionsonly parses PNG/GIF/JPEG. For WebP,prepareCursorImageForWireenters a fullBun.Image(...).resize(...).jpeg(...)decode before dimensions are checked viametadata(). A small compressed image with huge decoded dimensions can therefore allocate/decode far beyond the intendedMAX_CURSOR_IMAGE_DECODE_EDGE/MAX_CURSOR_IMAGE_PIXELSlimits. Preflight WebP dimensions (and reject over-budget images) before the expensive decode. Add a regression with an oversized WebP header/container. -
P2 privacy/performance: historical image URLs are still fetched and historical pixels are reprocessed on every later turn.
createCursorRequeststores the completeparsed.context.messagesasrawMessages; live transport runsprepareCursorRawMessages(request.rawMessages, ...); andprepareCursorRawMessagesprepares images in every user/developer/toolResult message. But the protobuf replay layer now intentionally strips historical image pixels and keeps only active/trailing promotion bytes. So an old HTTPS image can cause a new DNS/HTTP request on each subsequent turn, leaking later turn timing/activity to that remote host and adding avoidable latency; historical data URLs are repeatedly decoded/re-encoded too. Scope byte preparation/fetching to the active user/developer image turn and the trailing tool-result promotion block. Historical replay should omit image parts without touching the source URL or pixels. -
P2 availability: the 30 s image-phase deadline is not an aggregate CPU deadline for local images.
createCursorImagePhaseSignalcan abort HTTPS I/O, but data-URL decode/JPEG work does not consult the signal, and the content loop does not stop starting local image work after the signal fires. The current deadline test only proves parent-abort propagation; it does not prove that local image preprocessing stops at the deadline. At minimum check the signal before/after each image and do not start further decode/encode jobs after expiry. Combined with blocker 3, old local images can currently consume CPU beyond the advertised phase bound.
Maintainer/review status
Several still-unresolved CodeRabbit/Codex threads are stale on this head and should be closed with a short explanation rather than fixed twice: shared capped data-URL decode, result-less external flush, per-part promotion, retry continuation handling, historical MCP serialization, JPEG standalone markers, and prepared-message rebuild are already addressed. The CodeRabbit performance concern remains substantively relevant because historical preprocessing/refetch is still happening.
Usefulness
The feature is useful and the overall architecture is much better than the first reviewed head: native SelectedImage for capable Cursor models plus sidecar fallback for curated blind models is a sensible split, and the view_image continuation handling solves a real failure mode. The remaining issues are boundary/safety problems, not a reason to abandon the feature.
Verdict: REQUEST CHANGES. Fix the four items above and add focused regressions before merge. CI is mostly green on this SHA; Cross-platform CI was still running test shard 3/4 when this review was submitted.
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>
Soft-omit HTTPS/image failures onto userMessageAction text, strip historical McpImageContent outside the trailing promote window, drop result-less external mcpToolCalls, omit promote at image-part granularity, and add a shared image-phase deadline plus decode hardening with regression coverage. Co-authored-by: Cursor <cursoragent@cursor.com>
Require JPEG SOF before soft-cap skip, sniff WebP dims before Bun decode, scope prepare/fetch to the active vision window, and honor the image-phase AbortSignal across local encode work. Co-authored-by: Cursor <cursoragent@cursor.com>
0f04a8c to
f44619a
Compare
|
@Wibias @lidge-jun — addressed the four remaining blockers from review on Round-2 blockers
Verification
Please re-review when CI settles. Readiness checklist remains 4/4 after local green. (Linux |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/adapters/cursor/live-transport.ts (1)
545-598: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDerive the Cursor tool filter from the same prepared request text as the wire payload.
At
src/adapters/cursor/live-transport.ts:547-550,activeText,cursorVisibleTools, andclientToolDefsare built from the originalrequest.preparedRequestis created later, andsrc/adapters/cursor/protobuf-request.ts:732readsactivePromptText(request)again forrawTextbefore buildingmcpTools.
cursorToolsForActivePromptcan change the advertised set depending on whether active text is empty and matches generic tool-count prompts, soclientToolNamesandmcpToolscan become inconsistent on those turns. Move the image raw-message preparation before these reads and deriveactiveText, the tool filter, and the event-state tool names frompreparedRequest.🤖 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 545 - 598, The tool filter and advertised tool names are derived from the unprepared request while the wire payload uses prepared messages, causing inconsistencies when image preparation changes active text. Move the raw-message/image preparation and `preparedRequest` construction before `activePromptText`, `cursorToolsForActivePrompt`, and related event-state setup, then derive those values—including `clientToolDefs`, synthetic tool tracking, and `rejectNativeFileMutations`—from `preparedRequest` while preserving the existing image-phase cancellation behavior.
🤖 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 289-292: Replace the incomplete stale-configuration sentence in
the documentation with wording that clearly states stale
`providers.cursor.noVisionModels` values marking every Cursor model are healed
to the curated Auto/Composer/GLM set during OAuth reconciliation.
In `@src/adapters/cursor/images.ts`:
- Around line 244-290: Update the image-fetch error handling around the local
timeout controller in the fetch helper: distinguish aborts caused by the
per-fetch timer from aborts propagated by the caller’s phase signal. Re-throw
only caller-driven aborts; convert local IMAGE_FETCH_TIMEOUT_MS aborts into
CursorImageError so resolveCursorImages can omit the image without failing the
whole turn. Preserve the existing handling for other errors.
- Around line 464-483: Remove the separate full-decode resize/encode validation
from the try block, and make `Bun.Image(image.data).metadata()` the first image
validation after `throwIfImagePhaseAborted(signal)`. Preserve the subsequent
width/height limits, then retain the declared MIME and JPEG magic check before
returning the passthrough `ready` result.
In `@tests/cursor-images.test.ts`:
- Around line 697-706: Rename the existing test to describe parent-signal
propagation and remove the unused phase setup/cancellation from it. Add a
separate focused test for the deadline behavior in createCursorImagePhaseSignal,
using fake timers or an injectable timeout to advance past
CURSOR_IMAGE_PHASE_TIMEOUT_MS and assert the returned signal aborts, covering
the timer path in src/adapters/cursor/images.ts.
- Around line 96-101: Fix the oversized data-URL test around the existing
oversized payload setup so its base64 length is divisible by four and its
decoded size exceeds estimatedBase64DecodedBytes, ensuring it reaches the
decode-bomb ceiling rather than strict-base64 validation. Add coverage through
decodeCursorImageDataUrl that asserts the expected size-limit error message,
while preserving resolveCursorImages’s soft-omit assertion for mixed-turn
handling.
---
Outside diff comments:
In `@src/adapters/cursor/live-transport.ts`:
- Around line 545-598: The tool filter and advertised tool names are derived
from the unprepared request while the wire payload uses prepared messages,
causing inconsistencies when image preparation changes active text. Move the
raw-message/image preparation and `preparedRequest` construction before
`activePromptText`, `cursorToolsForActivePrompt`, and related event-state setup,
then derive those values—including `clientToolDefs`, synthetic tool tracking,
and `rejectNativeFileMutations`—from `preparedRequest` while preserving the
existing image-phase cancellation behavior.
🪄 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: edff428c-9c8c-408c-8fb3-b96771641759
⛔ Files ignored due to path filters (1)
tests/helpers/cursor-grumpy-fixture.pngis excluded by!**/*.png
📒 Files selected for processing (23)
docs-site/src/content/docs/reference/configuration/providers.mdsrc/adapters/cursor.tssrc/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-adapter.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
| After pulling Cursor vision fixes, run `ocx ensure` so the proxy PID is the workspace `src/cli` | ||
| binary rather than a stale install. Stale `providers.cursor.noVisionModels` stamps that list every | ||
| Cursor model are healed back to the curated Auto/Composer/GLM set on OAuth reconcile. For | ||
| `cursor/grok-4.5`, Codex effort `none`/`minimal` maps to wire tier `medium` (some plans reject |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the stale-configuration sentence.
Lines 290-291 are grammatically incomplete. The sentence does not clearly identify the stale configuration that OAuth reconciliation replaces.
Proposed fix
- Cursor model are healed back to the curated Auto/Composer/GLM set on OAuth reconcile. For
+ Cursor model are healed back to the curated Auto/Composer/GLM set on OAuth reconcile. ForReplace the full sentence with:
Stale `providers.cursor.noVisionModels` values that mark every Cursor model are healed back to the curated Auto/Composer/GLM set during OAuth reconciliation.As per path instructions, user-facing documentation must stay synchronized with actual CLI/API behavior.
🤖 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
289 - 292, Replace the incomplete stale-configuration sentence in the
documentation with wording that clearly states stale
`providers.cursor.noVisionModels` values marking every Cursor model are healed
to the curated Auto/Composer/GLM set during OAuth reconciliation.
Source: Path instructions
| const timer = setTimeout(() => controller.abort(), IMAGE_FETCH_TIMEOUT_MS); | ||
|
|
||
| try { | ||
| const response = await pinnedHttpsGet(url, pickPinnedAddress(resolved.addresses), controller.signal, { | ||
| maxBytes: MAX_CURSOR_IMAGE_DECODE_BYTES, | ||
| }); | ||
| const contentType = (response.headers.get("content-type") || "").toLowerCase(); | ||
| const mimeType = contentType.split(";")[0]?.trim() || ""; | ||
| if (!mimeType.startsWith("image/")) { | ||
| throw new CursorImageError("Image URL did not return an image content type."); | ||
| } | ||
| if (!response.body) throw new CursorImageError("Image URL returned no body."); | ||
|
|
||
| const reader = response.body.getReader(); | ||
| const chunks: Uint8Array[] = []; | ||
| let total = 0; | ||
| try { | ||
| for (;;) { | ||
| const { value, done } = await reader.read(); | ||
| if (done) break; | ||
| total += value.byteLength; | ||
| if (total > MAX_CURSOR_IMAGE_DECODE_BYTES) { | ||
| throw new CursorImageError("Image input is too large to process safely."); | ||
| } | ||
| chunks.push(value); | ||
| } | ||
| } finally { | ||
| try { await reader.cancel(); } catch { /* ignore */ } | ||
| reader.releaseLock(); | ||
| } | ||
|
|
||
| const data = new Uint8Array(total); | ||
| let offset = 0; | ||
| for (const chunk of chunks) { | ||
| data.set(chunk, offset); | ||
| offset += chunk.byteLength; | ||
| } | ||
| return { data, mimeType }; | ||
| } catch (error) { | ||
| if (error instanceof CursorImageError) throw error; | ||
| // Preserve AbortError so the image-phase deadline is not soft-omitted as a fetch failure. | ||
| if (signal?.aborted || (error instanceof Error && error.name === "AbortError")) throw error; | ||
| throw new CursorImageError("Could not fetch the image URL."); | ||
| } finally { | ||
| clearTimeout(timer); | ||
| signal?.removeEventListener("abort", onAbort); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A per-fetch timeout escalates to a whole-turn failure.
Line 244 arms IMAGE_FETCH_TIMEOUT_MS (default 15 s) on the local controller, which is separate from the caller's phase signal. When only that timer fires, the parent signal is not aborted, so line 285 evaluates signal?.aborted as false but error.name === "AbortError" as true. The raw AbortError is rethrown.
resolveCursorImages line 393 rethrows any AbortError, and resolveActiveCursorImages is awaited without a try/catch in src/adapters/cursor/live-transport.ts line 588. One slow image host therefore fails the entire Cursor turn at 15 s, even though the 30 s phase deadline has not expired. The file header states the policy is "Unsupported / undecodable images are omitted (fail-closed)", and every other failure in this function degrades to CursorImageError.
Track which side aborted and convert the local timeout into a CursorImageError.
🐛 Proposed fix to keep the per-fetch timeout soft
const controller = new AbortController();
const onAbort = () => controller.abort();
if (signal) {
if (signal.aborted) controller.abort();
else signal.addEventListener("abort", onAbort, { once: true });
}
- const timer = setTimeout(() => controller.abort(), IMAGE_FETCH_TIMEOUT_MS);
+ let timedOut = false;
+ const timer = setTimeout(() => {
+ timedOut = true;
+ controller.abort();
+ }, IMAGE_FETCH_TIMEOUT_MS); } catch (error) {
if (error instanceof CursorImageError) throw error;
+ // A local fetch timeout is not the phase deadline; omit this image instead.
+ if (timedOut && !signal?.aborted) {
+ throw new CursorImageError("Image URL timed out.");
+ }
// Preserve AbortError so the image-phase deadline is not soft-omitted as a fetch failure.
if (signal?.aborted || (error instanceof Error && error.name === "AbortError")) throw error;
throw new CursorImageError("Could not fetch the image URL.");📝 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.
| const timer = setTimeout(() => controller.abort(), IMAGE_FETCH_TIMEOUT_MS); | |
| try { | |
| const response = await pinnedHttpsGet(url, pickPinnedAddress(resolved.addresses), controller.signal, { | |
| maxBytes: MAX_CURSOR_IMAGE_DECODE_BYTES, | |
| }); | |
| const contentType = (response.headers.get("content-type") || "").toLowerCase(); | |
| const mimeType = contentType.split(";")[0]?.trim() || ""; | |
| if (!mimeType.startsWith("image/")) { | |
| throw new CursorImageError("Image URL did not return an image content type."); | |
| } | |
| if (!response.body) throw new CursorImageError("Image URL returned no body."); | |
| const reader = response.body.getReader(); | |
| const chunks: Uint8Array[] = []; | |
| let total = 0; | |
| try { | |
| for (;;) { | |
| const { value, done } = await reader.read(); | |
| if (done) break; | |
| total += value.byteLength; | |
| if (total > MAX_CURSOR_IMAGE_DECODE_BYTES) { | |
| throw new CursorImageError("Image input is too large to process safely."); | |
| } | |
| chunks.push(value); | |
| } | |
| } finally { | |
| try { await reader.cancel(); } catch { /* ignore */ } | |
| reader.releaseLock(); | |
| } | |
| const data = new Uint8Array(total); | |
| let offset = 0; | |
| for (const chunk of chunks) { | |
| data.set(chunk, offset); | |
| offset += chunk.byteLength; | |
| } | |
| return { data, mimeType }; | |
| } catch (error) { | |
| if (error instanceof CursorImageError) throw error; | |
| // Preserve AbortError so the image-phase deadline is not soft-omitted as a fetch failure. | |
| if (signal?.aborted || (error instanceof Error && error.name === "AbortError")) throw error; | |
| throw new CursorImageError("Could not fetch the image URL."); | |
| } finally { | |
| clearTimeout(timer); | |
| signal?.removeEventListener("abort", onAbort); | |
| } | |
| const timer = setTimeout(() => controller.abort(), IMAGE_FETCH_TIMEOUT_MS); | |
| try { | |
| const response = await pinnedHttpsGet(url, pickPinnedAddress(resolved.addresses), controller.signal, { | |
| maxBytes: MAX_CURSOR_IMAGE_DECODE_BYTES, | |
| }); | |
| const contentType = (response.headers.get("content-type") || "").toLowerCase(); | |
| const mimeType = contentType.split(";")[0]?.trim() || ""; | |
| if (!mimeType.startsWith("image/")) { | |
| throw new CursorImageError("Image URL did not return an image content type."); | |
| } | |
| if (!response.body) throw new CursorImageError("Image URL returned no body."); | |
| const reader = response.body.getReader(); | |
| const chunks: Uint8Array[] = []; | |
| let total = 0; | |
| try { | |
| for (;;) { | |
| const { value, done } = await reader.read(); | |
| if (done) break; | |
| total += value.byteLength; | |
| if (total > MAX_CURSOR_IMAGE_DECODE_BYTES) { | |
| throw new CursorImageError("Image input is too large to process safely."); | |
| } | |
| chunks.push(value); | |
| } | |
| } finally { | |
| try { await reader.cancel(); } catch { /* ignore */ } | |
| reader.releaseLock(); | |
| } | |
| const data = new Uint8Array(total); | |
| let offset = 0; | |
| for (const chunk of chunks) { | |
| data.set(chunk, offset); | |
| offset += chunk.byteLength; | |
| } | |
| return { data, mimeType }; | |
| } catch (error) { | |
| if (error instanceof CursorImageError) throw error; | |
| // A local fetch timeout is not the phase deadline; omit this image instead. | |
| if (timedOut && !signal?.aborted) { | |
| throw new CursorImageError("Image URL timed out."); | |
| } | |
| // Preserve AbortError so the image-phase deadline is not soft-omitted as a fetch failure. | |
| if (signal?.aborted || (error instanceof Error && error.name === "AbortError")) throw error; | |
| throw new CursorImageError("Could not fetch the image URL."); | |
| } finally { | |
| clearTimeout(timer); | |
| signal?.removeEventListener("abort", onAbort); | |
| } |
🤖 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 244 - 290, Update the image-fetch
error handling around the local timeout controller in the fetch helper:
distinguish aborts caused by the per-fetch timer from aborts propagated by the
caller’s phase signal. Re-throw only caller-driven aborts; convert local
IMAGE_FETCH_TIMEOUT_MS aborts into CursorImageError so resolveCursorImages can
omit the image without failing the whole turn. Preserve the existing handling
for other errors.
| try { | ||
| throwIfImagePhaseAborted(signal); | ||
| // Force a full decode before accepting passthrough / encode (Anthropic-style validate). | ||
| await new Bun.Image(image.data).resize(1, 1).jpeg({ quality: 1 }).toBuffer(); | ||
|
|
||
| // Passthrough only when declared MIME matches actual JPEG magic (never PNG-as-JPEG). | ||
| if (declaredJpeg && format === "jpeg" && image.data.byteLength <= softMax) { | ||
| return { status: "ready", image }; | ||
| } | ||
|
|
||
| throwIfImagePhaseAborted(signal); | ||
| const meta = await new Bun.Image(image.data).metadata(); | ||
| const width = typeof meta.width === "number" ? meta.width : 0; | ||
| const height = typeof meta.height === "number" ? meta.height : 0; | ||
| if (width > 0 && height > 0) { | ||
| const edge = Math.max(width, height); | ||
| if (edge > MAX_CURSOR_IMAGE_DECODE_EDGE || width * height > MAX_CURSOR_IMAGE_PIXELS) { | ||
| return { status: "omitted", reason: CURSOR_VISION_IMAGE_OMITTED }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Bun.Image metadata() throw on invalid or corrupt image bytes in Bun 1.3?
💡 Result:
In Bun 1.3, Bun.Image.metadata does not necessarily throw an error when provided with corrupt image bytes, provided the image header can be successfully parsed [1]. The metadata method is designed to decode only enough of the image data to identify its dimensions (width and height) and format [2][3]. As a result, it may return valid metadata for an image file even if the underlying pixel data is corrupt or otherwise unreadable by the full decoding process [1]. Errors in Bun.Image typically occur during terminal operations—such as when resizing, transforming, or encoding—which require full pixel decoding [1][4]. If the input is completely unidentifiable as a valid image format, or if there is a severe issue during the initial header parsing, the call may reject with an error code such as ERR_IMAGE_UNKNOWN_FORMAT [4][5]. For more comprehensive error handling, you should anticipate potential rejections from Bun.Image methods, which use defined error codes like ERR_IMAGE_DECODE_FAILED [4][5].
Citations:
- 1: Bun.Image: CMYK JPEG decode fails with "Image: decode failed" oven-sh/bun#31895
- 2: https://bun.com/reference/bun/Image/metadata
- 3: https://bun.sh/docs/runtime/image
- 4: https://bun.com/reference/bun/Image
- 5: https://bun.sh/reference/bun/Image/ErrorCode
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant source and tests without running repository code.
printf 'Files:\n'
fd -a 'images\.ts$|cursor-images\.test\.ts$' . | sed 's#^\./##'
printf '\nRelevant source lines:\n'
sed -n '430,560p' src/adapters/cursor/images.ts
printf '\nRelevant test sections:\n'
sed -n '420,480p' tests/cursor-images.test.ts
sed -n '650,690p' tests/cursor-images.test.ts
printf '\nSearch for corrupt image assertions around these functions:\n'
rg -n "corrupt|invalid|fake|signature|ERR_IMAGE|onMetadata|metadata\\(" tests/cursor-images.test.ts src/adapters/cursor/images.tsRepository: lidge-jun/opencodex
Length of output: 11970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect imports and symbol usages to understand test helper setup before running a behavioral check.
printf 'Outline around prepareCursorImageForWire:\n'
ast-grep outline src/adapters/cursor/images.ts --match prepareCursorImageForWire --view expanded || true
printf '\nPrepare tests helper usage:\n'
ast-grep outline tests/cursor-images.test.ts --view expanded || true
printf '\nCheck if Bun is available and inspect runtime promise behavior for metadata vs encode on corrupt PNG bytes:\n'
node - <<'JS'
console.log('node unavailable_for_bun_replay')
JS
printf '\nLook at package/bun version declarations:\n'
for f in package.json bun.lock bun.lockb; do
[ -f "$f" ] && { printf '\n--- %s ---\n'; if [ "$f" = "package.json" ]; then jq '.packageManager // .engines // {}' "$f"; else sed -n '1,20p' "$f"; fi; }
doneRepository: lidge-jun/opencodex
Length of output: 1472
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Bun runtime check and metadata vs encode behavior for corrupt PNG signature bytes:\n'
if command -v bun >/dev/null 2>&1; then
bun node - <<'JS'
async function probe(label, data) {
try {
const meta = await new Bun.Image(data).metadata();
console.log(JSON.stringify({ label, metadataSuccess: true, meta }));
} catch (metaErr) {
console.log(JSON.stringify({ label, metadataSuccess: false, metadataErrorName: metaErr?.name ?? '', metadataErrorMessage: metaErr?.message ?? '' }));
}
try {
const buf = await new Bun.Image(data).resize(1,1).jpeg({ quality: 1 }).toBuffer();
console.log(JSON.stringify({ label, encodeSuccess: true, size: buf.length }));
} catch (encodeErr) {
console.log(JSON.stringify({ label, encodeSuccess: false, encodeErrorName: encodeErr?.name ?? '', encodeErrorMessage: encodeErr?.message ?? '' }));
}
}
probe('8-byte PNG signature header only', new Uint8Array([137, 80, 78, 71, 13, 10, 26, 10]));
probe('128 bytes A corrupt PNG', new Uint8Array(128).fill(0x41));
JS
else
printf 'bun runtime unavailable\n'
fiRepository: lidge-jun/opencodex
Length of output: 264
🌐 Web query:
Bun API reference Bun.Image constructor errors metadata resize jpeg decode
💡 Result:
The Bun.Image API provides a chainable pipeline for processing images, including decoding, resizing, and encoding, with support for JPEG, PNG, WebP, HEIC, and AVIF [1][2]. Metadata You can retrieve image metadata (width, height, and format) without decoding the full pixel data using the metadata method [1][3]. const { width, height, format } = await new Bun.Image(input).metadata; Resizing The resize method allows you to set target dimensions [1][4]. - If height is omitted, the aspect ratio is preserved [4]. - Options like { fit: 'inside' } preserve aspect ratio within a box, while { withoutEnlargement: true } prevents upscaling [1][4]. - Resampling filters (e.g., 'lanczos3', 'mitchell') can be specified, with 'lanczos3' being the default [2]. Errors Bun.Image operations can fail with specific error codes. You should branch on these stable codes rather than parsing error messages [5][6][7]: - ERR_IMAGE_FORMAT_UNSUPPORTED: Requested format is unavailable on the current machine (e.g., HEIC/AVIF missing OS codecs, or TIFF on Linux) [5][6][7]. - ERR_IMAGE_TOO_MANY_PIXELS: The image dimensions or resize output exceed the configured maxPixels, or the input size exceeds the 256 MiB limit [5][6][7]. - ERR_IMAGE_DECODE_FAILED / ERR_IMAGE_ENCODE_FAILED: General codec errors [5][6][7]. - ERR_IMAGE_UNKNOWN_FORMAT: The input data could not be identified [5][6][7]. - ERR_INVALID_STATE: The input ArrayBuffer was transferred before the terminal method was called [5][6][7]. - System errors (e.g., ENOENT, EACCES) are surfaced directly for file-backed inputs [5][6][7]. Decoding/Encoding Bun.Image supports decoding JPEG, PNG, WebP, HEIC, AVIF, and supports decode-only for BMP, TIFF, and GIF [5][7]. For JPEG, the pipeline can skip to a lower resolution during decoding if the target size is small enough, improving performance [1]. Output encoding is performed via terminal methods like.jpeg,.png, or.webp, which are awaited to execute the pipeline [1][2].
Citations:
- 1: https://bun.sh/docs/runtime/image
- 2: https://bun.com/docs/runtime/image
- 3: https://bun.com/reference/bun/Image/metadata
- 4: https://bun.com/reference/bun/Image/resize
- 5: https://bun.com/reference/bun/Image/constructor
- 6: https://bun.sh/reference/bun/Image/ErrorCode
- 7: https://bun.sh/reference/bun/Image
🌐 Web query:
Bun 1.3.14 Image metadata corrupt PNG signature only ERR_IMAGE_DECODE_FAILED
💡 Result:
The error ERR_IMAGE_DECODE_FAILED in Bun is a generic code returned by the Bun.Image API when an image codec fails to process the provided input [1][2][3]. When this error occurs in the context of an "invalid PNG signature" or "corrupt" PNG metadata, it is typically caused by one of the following scenarios: 1. File Format Mismatch: The most common cause is that the file does not actually contain PNG data, even if it has a.png file extension [4][5]. For example, a file might be a JPEG or another image format saved with a.png extension [4][5]. Because Bun's PNG codec expects a valid PNG file signature (the first 8 bytes of a PNG file), it will reject files that contain other headers [3][4][5]. You can verify the true format of the file using the command-line tool file: $ file your-image.png 2. Corrupt or Truncated Data: If the file is a legitimate PNG but the header or metadata is truncated or malformed, the decoder will fail during the initial validation phase [3]. To troubleshoot and resolve this: - Validate the File: Confirm the file type using the file command mentioned above. If it is not a PNG, convert it to the correct format or rename the extension to match its actual content [4][5]. - Check for Integrity: If the file is reported as a PNG but still fails, it may be corrupted. Try opening it in a standard image viewer (like a web browser or system previewer) to see if it can be repaired or if it is genuinely invalid [5]. - Update/Verify Bun Version: While Bun 1.3.14 includes improvements for handling image signatures and decoding, ensure your environment is not dealing with edge cases by testing with a known-good PNG file [6][7]. In Bun's implementation, the error code ERR_IMAGE_DECODE_FAILED is used to differentiate codec-level failures from other issues like unsupported formats (ERR_IMAGE_FORMAT_UNSUPPORTED) or exceeding memory/pixel limits (ERR_IMAGE_TOO_MANY_PIXELS) [1][2][3].
Citations:
- 1: https://bun.sh/reference/bun/Image
- 2: https://bun.sh/reference/bun/Image/ErrorCode
- 3: https://github.com/oven-sh/bun/blob/88417471/src/runtime/image/Image.rs
- 4: Format error decoding Png: Invalid PNG signature. typst/typst#3079
- 5: FormatError: PNG file has invalid signature. Scondo/purepng#23
- 6: Bun.markdown.ansi: show image URL and cap Kitty image width oven-sh/bun#29131
- 7: Bun.Image: preserve 16-bit-per-channel PNG through decode/encode oven-sh/bun#30463
🌐 Web query:
Bun github Image.rs constructor metadata jpeg resize decode buffer
💡 Result:
The image crate (often referred to in the context of image.rs) is the primary Rust library for image decoding, encoding, and processing [1][2]. To perform the tasks mentioned: 1. Decoding and Metadata: The library uses an ImageDecoder trait for decoding [1][3]. You can access metadata, such as dimensions, color type, and orientation, directly through this trait [3][4][5]. For example, after initializing a decoder for a JPEG, you can call.dimensions to get the image size or.orientation to retrieve orientation metadata [3][4]. 2. Working with Buffers: The central structure for image data is ImageBuffer, which acts as a generic container for image data [6][7]. You can create an ImageBuffer from an existing raw buffer (like a Vec or slice) using the ImageBuffer::from_raw method, which returns an Option [7]. 3. Resizing: Image resizing is handled by the DynamicImage type or through specific operations provided by the crate [1][4]. Once you have loaded an image (e.g., using ImageReader::open or by decoding into a DynamicImage), you can call the.resize or.resize_exact methods [4]. Example workflow: - Load/Decode: Use ImageReader to open a file or create a reader from a buffer. - Metadata: Access metadata via the decoder. - Resize/Process: Convert to DynamicImage if necessary, then call resize [4]. - Buffer: Use ImageBuffer to manipulate the raw pixel data directly if needed [7]. Official documentation can be found on docs.rs/image [6].
Citations:
- 1: https://github.com/image-rs/image
- 2: https://www.image-rs.org/projects/image.html
- 3: https://rin.rs/doc/image/trait.ImageDecoder.html
- 4: https://docs.rs/crate/image/latest/source/CHANGES.md
- 5: https://codebrowser.dev/slint/crates/image/src/image.rs.html
- 6: https://docs.rs/image/latest/image/
- 7: https://docs.rs/image/latest/image/struct.ImageBuffer.html
Drop the separate validation decode before metadata().
metadata() already rejects the PNG stub and junk JPEG inputs covered by the tests, including the 8-byte PNG signature, 128-byte corrupt PNG, and 128-byte fake JPEG payloads. Keep it first so it carries the validation role, then keep the MIME/magic check before the passthrough ready path.
🤖 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 464 - 483, Remove the separate
full-decode resize/encode validation from the try block, and make
`Bun.Image(image.data).metadata()` the first image validation after
`throwIfImagePhaseAborted(signal)`. Preserve the subsequent width/height limits,
then retain the declared MIME and JPEG magic check before returning the
passthrough `ready` result.
| test("omits data URLs above the inbound decode bomb ceiling", async () => { | ||
| const oversized = "A".repeat(Math.ceil((MAX_CURSOR_IMAGE_DECODE_BYTES + 1) * 4 / 3)); | ||
| // Soft-omit: one bad URL must not abort a mixed turn. | ||
| const resolved = await resolveCursorImages([`data:image/png;base64,${oversized}`]); | ||
| expect(resolved).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This test does not exercise the decode-bomb ceiling it names.
Line 97 builds "A".repeat(Math.ceil((MAX_CURSOR_IMAGE_DECODE_BYTES + 1) * 4 / 3)), which is 22369623 characters. That is below the payload.length > MAX_CURSOR_IMAGE_DECODE_BYTES * 2 guard at src/adapters/cursor/images.ts line 176, so execution reaches the strict-base64 check at line 185. 22369623 % 4 is 3, so the payload is rejected as invalid base64, not as oversize.
The assertion still passes, so the test would keep passing if the estimatedBase64DecodedBytes ceiling at line 188 were deleted. The sibling test at lines 115-121 already pads for this reason.
💚 Proposed fix to reach the size guard
test("omits data URLs above the inbound decode bomb ceiling", async () => {
- const oversized = "A".repeat(Math.ceil((MAX_CURSOR_IMAGE_DECODE_BYTES + 1) * 4 / 3));
+ const raw = "A".repeat(Math.ceil((MAX_CURSOR_IMAGE_DECODE_BYTES + 1) * 4 / 3));
+ // Pad to a valid base64 length so the strict-alphabet check passes and the
+ // estimated-decoded-bytes ceiling is the rule that rejects this payload.
+ const oversized = raw + "=".repeat((4 - (raw.length % 4)) % 4);
// Soft-omit: one bad URL must not abort a mixed turn.
const resolved = await resolveCursorImages([`data:image/png;base64,${oversized}`]);
expect(resolved).toEqual([]);
});Padding with = beyond two characters is itself rejected by the regex at line 185, so also assert the thrown message through decodeCursorImageDataUrl to pin the exact guard.
📝 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.
| test("omits data URLs above the inbound decode bomb ceiling", async () => { | |
| const oversized = "A".repeat(Math.ceil((MAX_CURSOR_IMAGE_DECODE_BYTES + 1) * 4 / 3)); | |
| // Soft-omit: one bad URL must not abort a mixed turn. | |
| const resolved = await resolveCursorImages([`data:image/png;base64,${oversized}`]); | |
| expect(resolved).toEqual([]); | |
| }); | |
| test("omits data URLs above the inbound decode bomb ceiling", async () => { | |
| const raw = "A".repeat(Math.ceil((MAX_CURSOR_IMAGE_DECODE_BYTES + 1) * 4 / 3)); | |
| // Pad to a valid base64 length so the strict-alphabet check passes and the | |
| // estimated-decoded-bytes ceiling is the rule that rejects this payload. | |
| const oversized = raw + "=".repeat((4 - (raw.length % 4)) % 4); | |
| // Soft-omit: one bad URL must not abort a mixed turn. | |
| const resolved = await resolveCursorImages([`data:image/png;base64,${oversized}`]); | |
| expect(resolved).toEqual([]); | |
| }); |
🤖 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 `@tests/cursor-images.test.ts` around lines 96 - 101, Fix the oversized
data-URL test around the existing oversized payload setup so its base64 length
is divisible by four and its decoded size exceeds estimatedBase64DecodedBytes,
ensuring it reaches the decode-bomb ceiling rather than strict-base64
validation. Add coverage through decodeCursorImageDataUrl that asserts the
expected size-limit error message, while preserving resolveCursorImages’s
soft-omit assertion for mixed-turn handling.
| test("createCursorImagePhaseSignal aborts after deadline", async () => { | ||
| const phase = createCursorImagePhaseSignal(); | ||
| // Use a short local timeout by aborting the parent immediately. | ||
| const parent = new AbortController(); | ||
| const child = createCursorImagePhaseSignal(parent.signal); | ||
| parent.abort(); | ||
| expect(child.signal.aborted).toBe(true); | ||
| phase.cancel(); | ||
| child.cancel(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The test name promises deadline coverage that the body does not provide.
The test is named "createCursorImagePhaseSignal aborts after deadline", but it never advances time or waits for CURSOR_IMAGE_PHASE_TIMEOUT_MS. Line 698 creates phase and line 704 cancels it without a single assertion against it. The only assertion, at line 703, checks parent-signal propagation, which is a different branch of createCursorImagePhaseSignal.
The 30 s timer path at src/adapters/cursor/images.ts line 103 is therefore untested. Rename the test to describe what it verifies, and add a separate case for the timer by making the timeout injectable or by asserting the constant is wired.
💚 Proposed rename and split
- test("createCursorImagePhaseSignal aborts after deadline", async () => {
- const phase = createCursorImagePhaseSignal();
- // Use a short local timeout by aborting the parent immediately.
- const parent = new AbortController();
- const child = createCursorImagePhaseSignal(parent.signal);
- parent.abort();
- expect(child.signal.aborted).toBe(true);
- phase.cancel();
- child.cancel();
- });
+ test("createCursorImagePhaseSignal propagates a parent abort", () => {
+ const parent = new AbortController();
+ const child = createCursorImagePhaseSignal(parent.signal);
+ expect(child.signal.aborted).toBe(false);
+ parent.abort();
+ expect(child.signal.aborted).toBe(true);
+ child.cancel();
+ });
+
+ test("createCursorImagePhaseSignal starts unaborted and honors an already-aborted parent", () => {
+ const standalone = createCursorImagePhaseSignal();
+ expect(standalone.signal.aborted).toBe(false);
+ standalone.cancel();
+
+ const aborted = new AbortController();
+ aborted.abort();
+ const child = createCursorImagePhaseSignal(aborted.signal);
+ expect(child.signal.aborted).toBe(true);
+ child.cancel();
+ });Based on path instructions: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
🤖 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 `@tests/cursor-images.test.ts` around lines 697 - 706, Rename the existing test
to describe parent-signal propagation and remove the unused phase
setup/cancellation from it. Add a separate focused test for the deadline
behavior in createCursorImagePhaseSignal, using fake timers or an injectable
timeout to advance past CURSOR_IMAGE_PHASE_TIMEOUT_MS and assert the returned
signal aborts, covering the timer path in src/adapters/cursor/images.ts.
Source: Path instructions
|
This PR changes an intentional Cursor contract rather than fixing a regression. On current The diff also reaches well past that contract. There is real protocol investigation and test work here and I do not want it to vanish into a vague rejection. We would merge a resubmission limited to native Closing this version because its size and mixed contracts cannot be safely reviewed or absorbed as a Cursor image bug fix. A focused native-image feature PR along that boundary gives us a clear path to evaluate the useful core. |
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-vision-wire-harness.test.ts tests/cursor-effort-suffix.test.tsbun run typecheckorigin/dev(f44619aa)HEAD=f44619aawithmulti_agent_v2ON (cursor/grok-4.5, efforthigh):I HATE WHEN/PEOPLE USE ME TO ILLUSTRATE A POINTview_image(collab on): same exact captions after tool resultbun 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.
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
Bug Fixes
Documentation