fix: preserve OpenAI responses stream metadata - #3528
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR extends response schema structs with optional OpenAI metadata fields ( ChangesOpenAI metadata field support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
|
|
Confidence Score: 4/5Safe to merge; all three new fields are correctly wired through struct definition, WithDefaults(), and deep-copy. The one gap is that SummaryIndex is now stored but not yet used to route multi-summary deltas during accumulation. The struct additions, WithDefaults() propagation, and deep-copy branches are all consistent and tested. The only open question is whether multi-summary streaming responses (multiple distinct SummaryIndex values) would produce correct accumulated output — the accumulator still merges everything into Summary[0]. This is a noted pre-existing limitation rather than a regression, but it means the newly captured SummaryIndex has no behavioral effect yet on the accumulation path. framework/streaming/responses.go — the reasoning summary accumulation loop does not use SummaryIndex to route deltas to the correct summary slot. Important Files Changed
|
|
❤️ for the PR @etnperlong |
## Summary Several fields on the Responses streaming schema (`Phase`, `SummaryIndex`, `Obfuscation`, and the latent leaks `Status` and `Signature`) were either undocumented or silently dropped by the deep-copy helper used during stream processing. This PR fixes the documentation, ensures the deep-copy preserves all of these fields with proper pointer independence, and adds test coverage to prevent regressions. The `Phase` field is particularly important: it is required when replaying history to `gpt-5.3-codex+` models, and omitting it causes significant performance degradation. ## Changes - Expanded the inline doc comment on `ResponsesMessage.Phase` to explain its role (`"commentary"` vs `"final_answer"`), its requirement on `gpt-5.3-codex+` history replay, and the performance impact of dropping it. - Expanded the inline doc comment on `BifrostResponsesStreamResponse.SummaryIndex` to clarify which streaming event types emit it. - Expanded the inline doc comment on `BifrostResponsesStreamResponse.Obfuscation` to explain it is random padding used as a side-channel mitigation, toggled via `StreamOptions.IncludeObfuscation`. - Added `TestDeepCopyResponsesStreamResponsePreservesAllFields` to guard the deep-copy helper against silently dropping `Phase`, `SummaryIndex`, `Obfuscation`, `Status`, and `Signature`, and to assert pointer independence after copying. - Added four new E2E harness cases covering: reasoning `summary_index` and `obfuscation` surviving a stream, `phase` appearing on assistant message items in a stream, and `phase` round-tripping correctly as an input field. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/streaming/... -run TestDeepCopyResponsesStreamResponsePreservesAllFields -v ``` For E2E validation, run the updated provider harness collection against a live environment with a valid `openaiKey` and confirm: - `summary_index` and `obfuscation` appear in the SSE body for the `o3-mini` streaming request. - `phase` (`"final_answer"` or `"commentary"`) appears on assistant message items for the `gpt-5.3-codex` streaming request. - The `phase` input round-trip request returns output items without an `unknown field "phase"` error. ## Breaking changes - [ ] Yes - [x] No ## Related issues Closes #3528 ## Security considerations The `Obfuscation` field is explicitly documented as random padding added to normalize delta event payload sizes as a side-channel mitigation. No new secrets, PII, or auth surfaces are introduced. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Several fields on the Responses streaming schema (`Phase`, `SummaryIndex`, `Obfuscation`, and the latent leaks `Status` and `Signature`) were either undocumented or silently dropped by the deep-copy helper used during stream processing. This PR fixes the documentation, ensures the deep-copy preserves all of these fields with proper pointer independence, and adds test coverage to prevent regressions. The `Phase` field is particularly important: it is required when replaying history to `gpt-5.3-codex+` models, and omitting it causes significant performance degradation. ## Changes - Expanded the inline doc comment on `ResponsesMessage.Phase` to explain its role (`"commentary"` vs `"final_answer"`), its requirement on `gpt-5.3-codex+` history replay, and the performance impact of dropping it. - Expanded the inline doc comment on `BifrostResponsesStreamResponse.SummaryIndex` to clarify which streaming event types emit it. - Expanded the inline doc comment on `BifrostResponsesStreamResponse.Obfuscation` to explain it is random padding used as a side-channel mitigation, toggled via `StreamOptions.IncludeObfuscation`. - Added `TestDeepCopyResponsesStreamResponsePreservesAllFields` to guard the deep-copy helper against silently dropping `Phase`, `SummaryIndex`, `Obfuscation`, `Status`, and `Signature`, and to assert pointer independence after copying. - Added four new E2E harness cases covering: reasoning `summary_index` and `obfuscation` surviving a stream, `phase` appearing on assistant message items in a stream, and `phase` round-tripping correctly as an input field. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/streaming/... -run TestDeepCopyResponsesStreamResponsePreservesAllFields -v ``` For E2E validation, run the updated provider harness collection against a live environment with a valid `openaiKey` and confirm: - `summary_index` and `obfuscation` appear in the SSE body for the `o3-mini` streaming request. - `phase` (`"final_answer"` or `"commentary"`) appears on assistant message items for the `gpt-5.3-codex` streaming request. - The `phase` input round-trip request returns output items without an `unknown field "phase"` error. ## Breaking changes - [ ] Yes - [x] No ## Related issues Closes #3528 ## Security considerations The `Obfuscation` field is explicitly documented as random padding added to normalize delta event payload sizes as a side-channel mitigation. No new secrets, PII, or auth surfaces are introduced. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Several fields on the Responses streaming schema (`Phase`, `SummaryIndex`, `Obfuscation`, and the latent leaks `Status` and `Signature`) were either undocumented or silently dropped by the deep-copy helper used during stream processing. This PR fixes the documentation, ensures the deep-copy preserves all of these fields with proper pointer independence, and adds test coverage to prevent regressions. The `Phase` field is particularly important: it is required when replaying history to `gpt-5.3-codex+` models, and omitting it causes significant performance degradation. ## Changes - Expanded the inline doc comment on `ResponsesMessage.Phase` to explain its role (`"commentary"` vs `"final_answer"`), its requirement on `gpt-5.3-codex+` history replay, and the performance impact of dropping it. - Expanded the inline doc comment on `BifrostResponsesStreamResponse.SummaryIndex` to clarify which streaming event types emit it. - Expanded the inline doc comment on `BifrostResponsesStreamResponse.Obfuscation` to explain it is random padding used as a side-channel mitigation, toggled via `StreamOptions.IncludeObfuscation`. - Added `TestDeepCopyResponsesStreamResponsePreservesAllFields` to guard the deep-copy helper against silently dropping `Phase`, `SummaryIndex`, `Obfuscation`, `Status`, and `Signature`, and to assert pointer independence after copying. - Added four new E2E harness cases covering: reasoning `summary_index` and `obfuscation` surviving a stream, `phase` appearing on assistant message items in a stream, and `phase` round-tripping correctly as an input field. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/streaming/... -run TestDeepCopyResponsesStreamResponsePreservesAllFields -v ``` For E2E validation, run the updated provider harness collection against a live environment with a valid `openaiKey` and confirm: - `summary_index` and `obfuscation` appear in the SSE body for the `o3-mini` streaming request. - `phase` (`"final_answer"` or `"commentary"`) appears on assistant message items for the `gpt-5.3-codex` streaming request. - The `phase` input round-trip request returns output items without an `unknown field "phase"` error. ## Breaking changes - [ ] Yes - [x] No ## Related issues Closes #3528 ## Security considerations The `Obfuscation` field is explicitly documented as random padding added to normalize delta event payload sizes as a side-channel mitigation. No new secrets, PII, or auth surfaces are introduced. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
Summary
Preserve missing OpenAI Responses stream metadata so Bifrost does not drop reasoning-summary and multi-phase message fields during normal streaming.
Changes
summary_indexandobfuscationtoBifrostResponsesStreamResponsephasetoResponsesMessageWithDefaults()and streaming deep-copy helpersType of change
Affected areas
How to test
Expected outcome: both test commands pass, and the regression tests confirm
summary_index,obfuscation, andphasesurvive unmarshal, defaulting, deep-copy, and re-serialization.Screenshots/Recordings
N/A
Breaking changes
Related issues
Related to reasoning summary metadata loss in OpenAI Responses streaming.
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelines