Repository navigation
Conversation
When image generation forwards to chat completions and the provider returns an upstream error (e.g. google-vertex 429), images.ts re-threw it as an HTTPException(500), which app.onError logged at ERROR severity and tripped error alerting. Mark these forwarded upstream/gateway provider errors via the exception cause so the global handler surfaces them as warnings. They are already recorded in the log table with the correct unifiedFinishReason by the internal chat-completions call, so no DB classification change is needed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughThis PR adds upstream error cause detection to distinguish provider failures from internal errors. It introduces a new ChangesUpstream Error Cause Detection
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR reduces noisy ERROR-level logging for expected upstream/provider failures (e.g., provider rate limits) when the images endpoint internally forwards to chat-completions and surfaces those failures via HTTPException.
Changes:
- Introduces an
HTTPException.causemarker (upstreamErrorCause/getUpstreamErrorCause) to identify forwarded upstream/gateway provider errors. - Updates the images forwarder to attach the marker when it detects
upstream_error/gateway_errorin the internal chat-completions error payload. - Adjusts the global
app.onErrorhandler to log these marked errors at WARN instead of ERROR, while preserving the client response behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/gateway/src/lib/error-response.ts | Adds helpers for creating/reading a marker used to classify forwarded upstream/gateway provider errors. |
| apps/gateway/src/lib/error-response.spec.ts | Adds unit tests covering marker creation and detection behavior. |
| apps/gateway/src/images/images.ts | Attaches the marker as HTTPException cause when forwarding internal chat-completions upstream/gateway provider errors. |
| apps/gateway/src/app.ts | Detects the marker in onError and downgrades log severity to WARN for these expected provider failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( | ||
| cause && | ||
| typeof cause === "object" && | ||
| (cause as Partial<UpstreamErrorCause>).upstreamError === true && | ||
| typeof (cause as Partial<UpstreamErrorCause>).unifiedFinishReason === | ||
| "string" | ||
| ) { | ||
| return cause as UpstreamErrorCause; | ||
| } | ||
| return null; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@apps/gateway/src/images/images.ts`:
- Around line 347-350: The code derives errorType using parsed?.error?.type ??
parsed?.error?.code which ignores parsed.error.code when parsed.error.type
exists but is a non-marker; update the logic around the errorType assignment
(the block that sets upstreamFinishReason) to check parsed?.error?.type and
parsed?.error?.code independently and set upstreamFinishReason if either equals
"upstream_error" or "gateway_error" (do the same fix for the similar logic later
in the file that mirrors this behavior). Ensure you reference parsed.error.type
and parsed.error.code separately when deciding to assign upstreamFinishReason.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 80404366-b412-4393-b3ab-a2064314ae90
📒 Files selected for processing (4)
apps/gateway/src/app.tsapps/gateway/src/images/images.tsapps/gateway/src/lib/error-response.spec.tsapps/gateway/src/lib/error-response.ts
| const errorType = parsed?.error?.type ?? parsed?.error?.code; | ||
| if (errorType === "upstream_error" || errorType === "gateway_error") { | ||
| upstreamFinishReason = errorType; | ||
| } |
There was a problem hiding this comment.
Check both error.type and error.code independently when deriving upstream markers.
Line 347 currently uses parsed?.error?.type ?? parsed?.error?.code, so if error.type exists but is non-marker (e.g. "api_error"), a marker in error.code is ignored. That drops cause, and Line 181 in apps/gateway/src/app.ts falls back to 5xx ERROR logging.
Suggested fix
- const errorType = parsed?.error?.type ?? parsed?.error?.code;
- if (errorType === "upstream_error" || errorType === "gateway_error") {
- upstreamFinishReason = errorType;
- }
+ const errorType = parsed?.error?.type;
+ const errorCode = parsed?.error?.code;
+ if (errorType === "upstream_error" || errorType === "gateway_error") {
+ upstreamFinishReason = errorType;
+ } else if (
+ errorCode === "upstream_error" ||
+ errorCode === "gateway_error"
+ ) {
+ upstreamFinishReason = errorCode;
+ }Also applies to: 357-359
🤖 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 `@apps/gateway/src/images/images.ts` around lines 347 - 350, The code derives
errorType using parsed?.error?.type ?? parsed?.error?.code which ignores
parsed.error.code when parsed.error.type exists but is a non-marker; update the
logic around the errorType assignment (the block that sets upstreamFinishReason)
to check parsed?.error?.type and parsed?.error?.code independently and set
upstreamFinishReason if either equals "upstream_error" or "gateway_error" (do
the same fix for the similar logic later in the file that mirrors this
behavior). Ensure you reference parsed.error.type and parsed.error.code
separately when deciding to assign upstreamFinishReason.
|
Closing as superseded. Main has since fixed the same issue more broadly:
Rebasing this branch conflicts in both |
Problem
A google-vertex
429 Too Many Requests(an expected, transient upstream rate limit) was being surfaced in logs at ERROR severity, tripping error alerting:These are expected upstream errors and should not be reported as logger errors — they just need to be marked as
upstream_error/gateway_errorin the log table (which already happens).Root cause
The
"Error from provider ..."string is only ever built as a response body inchat.ts(returned viac.json/SSE, never thrown). The image-generation endpoint forwards to/v1/chat/completions(images.ts:forwardToChatCompletions); on an upstream errorchat.tswraps it as HTTP 500, andimages.tsre-threw it asHTTPException(500). The global handlerapp.onErrorlogs allHTTPExceptionwith status ≥ 500 at ERROR (app.ts:170), so the forwarded provider 429 became an ERROR log.The sibling forwarders (
responses.ts,anthropic.ts) return the upstream error viac.jsonand never hit this path —images.tswas the outlier.Fix
images.tsnow tags forwarded upstream/gateway provider errors (error.type/codeofupstream_errororgateway_error) via theHTTPExceptioncause.app.onErrorrecognizes that marker and logs at warn ("Provider error", with theunifiedFinishReason) instead of ERROR, while still returning the same error response to the client.upstreamErrorCause/getUpstreamErrorCausehelpers inerror-response.ts, with unit tests.No DB change needed: the internal chat-completions call already inserts the log row with the correct
unifiedFinishReason(429 →upstream_error). The client-facing response is unchanged.Testing
pnpm build— full monorepo build passes (17/17).pnpm exec vitest run apps/gateway/src/lib/error-response.spec.ts— 6/6 pass.pnpm formatclean.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
Chores