Repository navigation
fix(actions): surface image size limit errors instead of the generic message - #3447
pacocartones wants to merge 1 commit into
Conversation
The catch block in processImageUrl() re-threw size-limit errors only when the message contained "Image size exceeds", but getImageSizeErrorMessage() produces "Image size (32.0MB) exceeds your current limit of 1MB." — the size parenthetical breaks the substring, so the branch never fired. Both remote-URL size checks (Content-Length pre-check and post-download body check) were swallowed and rethrown as the generic "Failed to process image from URL", leaving users with no idea their image was too big, while the data URL branch (which throws outside the try) reported the real message. Replace the message sniffing with a typed ImageSizeLimitError so the passthrough cannot silently rot when the wording changes again, and add regression tests covering both remote paths, data-URL/remote parity, and that non-size failures are still sanitized.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughChangesImage size-limit error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
smakosh
left a comment
There was a problem hiding this comment.
Code review
Typing the error is the right call, but as written the fix lands on /v1/images and /v1/videos only — the chat-completions path still flattens it to a 500. Findings below, most important first.
1. ImageSizeLimitError extends Error instead of RequestError, so chat completions still return a generic 500
llmgateway/packages/actions/src/process-image-url.ts
Lines 14 to 20 in f2377fa
RequestError is already imported in this file. Because the new class isn't one, on /v1/chat/completions:
transformGoogleMessagesonly preservesRequestErrorand otherwise rethrows a plainError:llmgateway/packages/actions/src/transform-google-messages.ts
Lines 382 to 392 in f2377fa
- the chat handler's typed-client-error branch doesn't match, so no
client_errorlog row is written:llmgateway/apps/gateway/src/chat/chat.ts
Lines 6362 to 6373 in f2377fa
app.onErrorfalls through to 500 "Internal Server Error" plus anUnhandled errorlog:llmgateway/apps/gateway/src/app.ts
Lines 223 to 226 in f2377fa
So the user still gets an opaque 500 with the size message discarded, the request never appears in the activity feed, and every oversized user image raises a false server-error alert. extends RequestError with statusCode 400 fixes all of these at once (and also covers the aws-mantle path in prepare-request-body.ts, which has no try/catch).
2. On Anthropic and Bedrock the size error is swallowed into a placeholder, so the fix is a no-op there
llmgateway/packages/actions/src/transform-anthropic-messages.ts
Lines 117 to 127 in f2377fa
Both call sites catch every processImageUrl error and substitute [Image failed to load: <url>] (Bedrock does the same in prepare-request-body.ts). The request returns 200, the image is silently ignored, and the user is billed for the tokens without ever learning the image was too big — the same symptom, on the two chat providers that actually download images. Worth re-throwing ImageSizeLimitError from those catches.
3. The newly-surfaced message states a limit the caller doesn't have on /v1/images and /v1/videos
llmgateway/packages/actions/src/process-image-url.ts
Lines 190 to 193 in f2377fa
Both call sites use the default maxSizeMB = 20 and userPlan = null:
llmgateway/apps/gateway/src/images/images.ts
Lines 1069 to 1073 in f2377fa
llmgateway/apps/gateway/src/videos/videos.ts
Lines 4273 to 4279 in f2377fa
A Pro org (100MB on chat) posting a 30MB image to /v1/images/edits now gets Image size (30.0MB) exceeds your current limit of 20MB. — a number that org doesn't have, and one that contradicts its chat limit. With userPlan null, an enterprise org also gets the non-enterprise upsell sentence. Before this change the generic message hid the wrong number; now it is user-visible, so these call sites should pass the real limit and plan.
4. Enterprise orgs are bucketed into the free cap on chat
llmgateway/apps/gateway/src/chat/chat.ts
Lines 2725 to 2732 in f2377fa
userPlan === "pro" ? proLimitMB : freeLimitMB puts enterprise on the free limit, and both values are forwarded to processImageUrl. Combined with the enterprise branch of getImageSizeErrorMessage, an enterprise org can now be told exceeds your current limit of 50MB. Contact us to increase your Enterprise plan limits. Pre-existing, but this PR is what makes it reach the user.
5. toFixed(1) can print a limit that equals the reported size
llmgateway/packages/actions/src/process-image-url.ts
Lines 32 to 34 in f2377fa
The actual size is rounded to one decimal while the limit prints as a bare integer, so anything in the ~52KB band above the limit reads Image size (1.0MB) exceeds your current limit of 1MB. ((1048577/(1024*1024)).toFixed(1) === "1.0"). Users barely over the threshold — exactly the ones who hit this — get a self-contradictory message. Rounding up would avoid it.
6. logger.error fires for what is now an ordinary client rejection
llmgateway/packages/actions/src/process-image-url.ts
Lines 184 to 193 in f2377fa
The error-severity log runs before the new instanceof passthrough, so every plan-limit rejection lands in error dashboards next to genuine fetch/SSRF/decode failures — and it is already logged at warn at the throw sites. Moving the logger.error below the passthrough would keep it to unexpected failures.
7. Size is checked before content type, so oversized non-images now get the wrong diagnosis
llmgateway/packages/actions/src/process-image-url.ts
Lines 135 to 155 in f2377fa
A 60MB application/pdf URL now reports "Image size … exceeds your current limit" rather than "URL does not point to a valid image", so the user shrinks the file and hits a completely different error on the retry. Previously both collapsed to the generic message, so the misleading specificity is new. Checking content type first resolves it.
Tests
- The fully anchored regex couples the assertion to ambient env:
getImageSizeErrorMessageappends the upsell sentence whenHOSTEDandPAID_MODEare both"true", and neither is pinned by the vitest setup or CI, soHOSTED=true PAID_MODE=true pnpm test:unitturns this red. Dropping the$(or clearing both vars in the test) makes it deterministic.llmgateway/packages/actions/src/process-image-url.spec.ts
Lines 92 to 100 in f2377fa
throw new Error("expected throw")sits inside thetryit guards, so it is caught by its owncatchand a regression whereprocessImageUrlstops throwing reportsexpected 'expected throw' to contain …instead of naming the real failure.await expect(...).rejects.toThrow(...)— used correctly in the last test — avoids the misdirection. Same shape at L68.llmgateway/packages/actions/src/process-image-url.spec.ts
Lines 45 to 58 in f2377fa
- Several comments narrate the pre-fix bug rather than current behaviour ("The catch block used to sniff for…", "this is the line that fails on the unfixed code"), including the class doc comment. AGENTS.md/CLAUDE.md asks for no unnecessary comments; this archaeology already lives in the PR description.
llmgateway/packages/actions/src/process-image-url.spec.ts
Lines 36 to 41 in f2377fa
Pre-existing, not this PR's job — flagging for a follow-up
Two memory hazards in the same function that this PR's tests now pin in place: the body is fully buffered before the post-download size check, so a response with no (or an understated) Content-Length bypasses the cap entirely, and the base64 conversion allocates one JS string per byte before joining.
llmgateway/packages/actions/src/process-image-url.ts
Lines 157 to 178 in f2377fa
Generated with Claude Code
|
Opened #3480 which builds directly on this branch — your commit is preserved there, plus a The main addition is making |
|
Thanks for the fix and the thorough write-up, @pacocartones — the diagnosis was spot on. Superseded by #3481, which carries your commit as-is and additionally addresses the review findings above (the error is now a Generated by Claude Code |
Builds on #3447 by @pacocartones (their commit is preserved here). ## Problem `processImageUrl`'s catch block sniffed the message for `"Image size exceeds"`, which the generated text (`"Image size (32.0MB) exceeds your current limit of 1MB."`) never contains, so every remote-URL size rejection was flattened into the generic `"Failed to process image from URL"`. #3447 fixes that by throwing a typed `ImageSizeLimitError`, but as a plain `Error` it only helps `/v1/images` and `/v1/videos`, which wrap errors themselves. On `/v1/chat/completions` — the path users actually hit — `transformGoogleMessages`, the chat handler's typed-client-error branch and `app.onError` all key on `RequestError`, so the message was still discarded and the user got a 500 with no `client_error` log row and a false server-error alert. ## Approach `ImageSizeLimitError extends RequestError` (400). That single change lands the fix on every path at once, including the `aws-mantle` branch in `prepare-request-body.ts`, which has no try/catch at all. Alongside it, in the same function: - Anthropic and Bedrock caught every `processImageUrl` error and substituted an `[Image failed to load]` placeholder, so an oversized image returned 200 with the image silently ignored and the turn billed. Size rejections are now re-thrown; network flakiness still degrades to the placeholder. - The message no longer claims a plan limit when the caller has no plan context. `/v1/images/edits` and `/v1/videos` call with the default 20MB cap and `userPlan: null`, so a Pro org was told `exceeds your current limit of 20MB` — a limit it does not have — plus a non-enterprise upsell. Those callers now get `exceeds the 20MB limit for image inputs`. - Enterprise orgs were bucketed into the free branch (`userPlan === "pro" ? proLimitMB : freeLimitMB`), so an enterprise org was rejected at the free cap and then told to contact us about raising its Enterprise limits. Enterprise now reads `IMAGE_SIZE_LIMIT_ENTERPRISE_MB`, defaulting to the Pro limit. - The body is read with a running cap instead of `arrayBuffer()`, so a response with no (or an understated) `Content-Length` can no longer be buffered in full before the check runs, and base64 goes through `Buffer` instead of one JS string per byte. - Non-base64 data URLs are percent-decoded instead of `btoa`-ing the escapes, which silently corrupted SVG payloads and threw an uncaught `InvalidCharacterError` on any non-Latin-1 character. - Content type is checked before size, so an oversized PDF is no longer reported as an image that is too big. - The reported size rounds up, so it can never print `Image size (1.0MB) exceeds your current limit of 1MB.` - Size rejections log at `warn` only; they were also hitting `logger.error` on every plan-limit rejection. ## Deliberate omissions The 20MB cap on image/video inputs is left as-is. Threading the org plan into `processVideoImageInput` means touching call sites in the atlas and avalanche upload helpers that have no organization in scope, and raising the cap could push payloads past provider limits — so the message was made accurate rather than the cap made plan-aware. A follow-up can do the plumbing. ## Verification - `packages/actions` suite: 545 tests pass, including 11 in the rewritten `process-image-url.spec.ts` (streaming cap abandons an endless body, `statusCode` is 400, content-type precedence, data-URL round-trip, plan-neutral message). - The spec no longer depends on ambient env: it passes both with and without `HOSTED=true PAID_MODE=true` exported, which turned the previous anchored regex red. - `pnpm build` and lint clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved image upload error messages to clearly identify applicable 20MB or plan-based limits. * Prevented oversized images from being silently replaced with placeholders during processing. * Added safer handling for remote and data URL images, including content validation, download limits, and clearer failure messages. * Improved handling of malformed or unavailable image URLs. * Enterprise image limits can now be configured independently, with a sensible default. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Manuel Sánchez <mpktmpktmpktmpkt@gmail.com> Co-authored-by: pacocartones <pacocartones@users.noreply.github.com>
Summary
processImageUrl()never tells a caller that their image was too big. The size-limit passthrough in thecatchblock sniffs the error message for"Image size exceeds":but the message
getImageSizeErrorMessage()actually produces isThe size parenthetical sits between
Image sizeandexceeds, so the substring never matches. That branch is dead. Both remote-URL size checks — theContent-Lengthpre-check and the post-download body check — are swallowed and rethrown as the generic"Failed to process image from URL".The asymmetry is what gives it away: the data URL branch throws from outside the
try, so it always reported the real message. Same rejection, two different answers depending on how the image arrived.Fix
Replace the message sniffing with a typed
ImageSizeLimitError. The passthrough then keys off the type, so it cannot rot again the next time the wording changes — which is exactly how it broke.The other two passthroughs in the same
catch("Failed to fetch image: HTTP"and"URL does not point to a valid image") also use.includes(), but those substrings do match the messages that are actually thrown, so they work today. I left them alone rather than widen this PR.Tests
New
packages/actions/src/process-image-url.spec.ts, 4 tests:Content-Lengthexceeds the limit — the pre-check path.Content-Length, so the post-download check is the one that rejects.In each size test the message is asserted before the type, on purpose: that is the line that fails on
main, and it fails for the reason a user experiences rather than because a new export is missing.Without the fix (source reverted to
main, tests kept):With the fix:
Tests 4 passed (4).What I did not verify
pnpm exec vitest run packages/actionsis 534 passed, 4 failed; the 4 failures areprovider-key/env-inventory.spec.tshittingECONNREFUSED 127.0.0.1:6379. I re-ran that file on a cleanmainand it fails identically, so it is a missing local Redis and not this change — but I have not run the suite anywhere with the service up.tsc --noEmit,eslintandprettier --checkare clean on both changed files.Summary by CodeRabbit