Repository navigation
fix(gateway): keep google's original finish reason - #3516
Conversation
A Google response with no candidates set the finish reason to the generic "content_filter" before the block-reason assignment ran, so the actual reason Google returned was discarded and every such request was logged as "content_filter". Keep the reported reason and only fall back to "content_filter" when Google gives none. Also stop classifying NO_IMAGE, IMAGE_OTHER and OTHER as content filters. NO_IMAGE means generation finished without emitting an image and the other two are Google's unspecified buckets, so none of them is a policy block. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughGoogle finish-reason parsing now preserves provider block reasons before fallback. Non-policy image reasons map to ChangesGoogle finish reason normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
apps/gateway/src/lib/logs.ts (1)
28-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct coverage for the new expected unknown reasons.
isExpectedUnknownFinishReasonnow suppresses logging forIMAGE_OTHERandNO_IMAGE. The supplied tests cover onlyOTHERandMALFORMED_RESPONSE. Add assertions for both new reasons so a later change does not re-enable erroneous unknown-reason logging.🤖 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/lib/logs.ts` around lines 28 - 39, Add direct test assertions for IMAGE_OTHER and NO_IMAGE in the isExpectedUnknownFinishReason test coverage, verifying both return the expected unknown-result behavior for supported providers. Keep the existing OTHER and MALFORMED_RESPONSE assertions unchanged.
🤖 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.
Nitpick comments:
In `@apps/gateway/src/lib/logs.ts`:
- Around line 28-39: Add direct test assertions for IMAGE_OTHER and NO_IMAGE in
the isExpectedUnknownFinishReason test coverage, verifying both return the
expected unknown-result behavior for supported providers. Keep the existing
OTHER and MALFORMED_RESPONSE assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3934d21d-8bf4-463d-b9b8-98df96bbfb05
📒 Files selected for processing (5)
apps/gateway/src/chat/tools/map-finish-reason-to-openai.tsapps/gateway/src/chat/tools/parse-provider-response.spec.tsapps/gateway/src/chat/tools/parse-provider-response.tsapps/gateway/src/lib/logs.spec.tsapps/gateway/src/lib/logs.ts
Follow-up to #3516. That PR stopped the generic `content_filter` label from overwriting the reason Google actually returned, but it kept a fallback: when a Google response has no candidates, no `promptFeedback.blockReason` and no candidate finish reason, it still recorded `content_filter`. That is the same guess the rest of the change removes, made with even less evidence — Google reported nothing at all, so there is nothing to suggest a policy block. The finish reason is now left unset, which `getUnifiedFinishReason` already maps to `unknown`. No other behaviour changes: - The existing `logger.warn` for this case still fires and carries the full response body. - `hasEmptyNonStreamingResponse` is gated on a truthy finish reason, so an empty response here is treated exactly as before (`content_filter` was explicitly excluded from that check). - The "Unknown finish reason encountered" error log is gated on a truthy finish reason, so it does not fire. The only client-visible difference is that `finish_reason` becomes `stop` instead of `content_filter` for a response that carried no information at all. 🤖 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 handling of blocked Google responses when no candidates or block reason are provided. * Finish reasons now accurately reflect available response information instead of defaulting to a content-filter status. * Diagnostic warnings remain available for affected responses. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Google requests were always logged with finish reason
content_filter, hiding the reason Google actually returned.Two causes:
parse-provider-response.ts, a response with no candidates setfinishReason = "content_filter"before thepromptFeedback.blockReasonassignment, which is guarded byif (!finishReason). The real reason was therefore always discarded. It is now preserved. When Google reports no reason at all — no candidates, noblockReason, no candidate finish reason — the finish reason is left unset (logged asunknown) rather than guessing at a block; the existing warning still carries the full response body for diagnosis.NO_IMAGE,IMAGE_OTHERandOTHERwere mapped tocontent_filter.NO_IMAGEmeans generation finished without emitting an image, and the other two are Google's unspecified / forward-compatibility buckets — none is a policy block. They now map tostopon the OpenAI surface and tounknownin the logs, alongside the existingOTHERhandling.This also removes a disagreement between
map-finish-reason-to-openai.tsandlogs.ts, which classifiedOTHERdifferently. Genuine blocks (SAFETY,PROHIBITED_CONTENT,IMAGE_SAFETY,IMAGE_PROHIBITED_CONTENT,BLOCKLIST,SPII,RECITATION,IMAGE_RECITATION,LANGUAGE) are unchanged.NO_IMAGEandIMAGE_OTHERare added toisExpectedUnknownFinishReasonso the newunknownmapping does not log an error on every occurrence.🤖 Generated with Claude Code