fix(openai): process custom responses stream chunks - #4497
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors ChangesResponses Streaming Raw Field Propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/openai/openai.go (1)
1794-1802:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid nil-clobbering custom raw fields on handler errors.
If
customResponseHandlerattachesRawRequest/RawResponsedirectly tobifrostErr.ExtraFieldsbut returns nil raw values, Lines 1795-1800 overwrite those fields before enrichment. Preserve existing error metadata and only prefer non-nil returned raw values.🐛 Proposed fix
if bifrostErr != nil { - if sendBackRawRequest { - bifrostErr.ExtraFields.RawRequest = rawRequest - } - if sendBackRawResponse { - bifrostErr.ExtraFields.RawResponse = rawResponse - } + customRawRequest := bifrostErr.ExtraFields.RawRequest + if rawRequest != nil { + customRawRequest = rawRequest + } + customRawResponse := bifrostErr.ExtraFields.RawResponse + if rawResponse != nil { + customRawResponse = rawResponse + } ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) - providerUtils.ProcessAndSendBifrostError(ctx, postHookRunner, preserveCustomRawErrorFields(providerUtils.EnrichError(ctx, bifrostErr, jsonBody, nil, sendBackRawRequest, sendBackRawResponse)), responseChan, logger, postHookSpanFinalizer) + enrichedErr := providerUtils.EnrichError(ctx, bifrostErr, jsonBody, nil, sendBackRawRequest, sendBackRawResponse) + if sendBackRawRequest && customRawRequest != nil { + enrichedErr.ExtraFields.RawRequest = customRawRequest + } + if sendBackRawResponse && customRawResponse != nil { + enrichedErr.ExtraFields.RawResponse = customRawResponse + } + providerUtils.ProcessAndSendBifrostError(ctx, postHookRunner, enrichedErr, responseChan, logger, postHookSpanFinalizer) return }🤖 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 `@core/providers/openai/openai.go` around lines 1794 - 1802, When processing bifrostErr and the sendBackRawRequest or sendBackRawResponse flags are true, the code unconditionally overwrites bifrostErr.ExtraFields.RawRequest and bifrostErr.ExtraFields.RawResponse with the returned rawRequest and rawResponse values, even if those returned values are nil. This clobbers any custom raw fields that customResponseHandler may have already attached to bifrostErr.ExtraFields. Fix this by only setting RawRequest and RawResponse if the corresponding returned values are not nil, ensuring that custom values set by customResponseHandler are preserved while still honoring non-nil returned raw values.
🤖 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.
Outside diff comments:
In `@core/providers/openai/openai.go`:
- Around line 1794-1802: When processing bifrostErr and the sendBackRawRequest
or sendBackRawResponse flags are true, the code unconditionally overwrites
bifrostErr.ExtraFields.RawRequest and bifrostErr.ExtraFields.RawResponse with
the returned rawRequest and rawResponse values, even if those returned values
are nil. This clobbers any custom raw fields that customResponseHandler may have
already attached to bifrostErr.ExtraFields. Fix this by only setting RawRequest
and RawResponse if the corresponding returned values are not nil, ensuring that
custom values set by customResponseHandler are preserved while still honoring
non-nil returned raw values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d3c364e-1990-44e4-bbe3-afeb3981378d
📒 Files selected for processing (2)
core/providers/openai/openai.gocore/providers/openai/responsesstream_test.go
The merge-base changed after approval.
fa15f50 to
ca190fc
Compare
ac30a53 to
7c66b20
Compare
|
recheck |
a0ec4f5 to
47c3fcd
Compare
44564de to
493bff0
Compare
244a01d to
ce1b2a6
Compare
Summary
Fixes
HandleOpenAIResponsesStreamingwhen acustomResponseHandleris provided.Previously, the custom handler branch parsed the SSE payload but skipped the normal Responses stream post-processing path. That meant successful custom-handled Responses stream chunks were never emitted. The branch also passed
nil, false, falseinto the handler, so custom raw request/response handling could not work correctly.Changes
customResponseHandler.response.error/response.failedconversionEnrichErrorruns.response.errorstream chunkVerification