fix(gom-43): remove stream_options injection from Responses API handler#85
Merged
SantiagoDePolonia merged 2 commits intomainfrom Feb 24, 2026
Merged
Conversation
The Responses API does not accept stream_options.include_usage — it's a Chat Completions-only parameter. The Responses API already returns usage data unconditionally in the response.done SSE event. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughRemoves automatic injection of StreamOptions.IncludeUsage for Responses streaming; chat completion streaming still injects usage. Unit and end-to-end tests added to assert the difference and prevent regressions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Provider
participant UsageLogger
Client->>Server: POST /v1/responses (stream=true)
Note over Server: OLD BEHAVIOR (before change)
Server->>Server: EnforceReturningUsageData? -> set req.StreamOptions.IncludeUsage = true
Server->>Provider: StreamResponses(req with StreamOptions)
Provider->>UsageLogger: Emit usage events (via IncludeUsage)
Provider->>Client: SSE stream events ... done
sequenceDiagram
participant Client
participant Server
participant Provider
participant UsageLogger
Client->>Server: POST /v1/responses (stream=true)
Note over Server: NEW BEHAVIOR (after change)
Server->>Server: Does not inject req.StreamOptions for Responses
Server->>Provider: StreamResponses(req without StreamOptions)
Provider-->>UsageLogger: (no automatic usage emission triggered by IncludeUsage)
Provider->>Client: SSE stream events ... done
Note over Server,Provider: Chat completions path still injects StreamOptions.IncludeUsage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/handlers_test.go`:
- Around line 611-617: The test currently uses t.Errorf for the status check and
then dereferences provider.capturedResponsesReq.StreamOptions which can panic if
capturedResponsesReq is nil; change the status assertion to t.Fatalf so the test
stops on non-200, and add a nil guard before inspecting
provider.capturedResponsesReq (e.g., check if provider.capturedResponsesReq ==
nil and fail with t.Fatalf or t.Errorf accordingly) before accessing
StreamOptions; reference the identifiers provider, capturedResponsesReq,
StreamOptions and the status assertion around rec.Code to locate the code to
change.
- Around line 651-661: The status check in TestStreamingResponses (the t.Errorf
that asserts rec.Code == http.StatusOK) must be made fatal or otherwise guarded
so we don't continue when the response is not OK and provider.capturedChatReq
may be nil; change the non-fatal assertion to t.Fatalf (or add an immediate
nil-check on provider.capturedChatReq before accessing StreamOptions) so that
accessing provider.capturedChatReq.StreamOptions cannot panic, and ensure the
test still verifies StreamOptions and IncludeUsage afterward.
In `@tests/e2e/responses_test.go`:
- Around line 143-162: The subtest currently named "streaming does not inject
stream_options" never asserts absence of stream_options; update the test to
actually verify the outgoing request body does not include "stream_options" (or
"include_usage") by either having sendResponsesRequest return the captured HTTP
request (or exposing a mock server helper to fetch the lastRequest) and assert
the request JSON does not contain the stream_options key, or delete this
redundant subtest and update the comment to reference the unit test
TestStreamingResponses_DoesNotInjectStreamOptions in handlers_test.go as the
regression guard; use the payload variable and helpers
sendResponsesRequest/readResponsesStream/hasDoneEvent to locate and adjust the
test accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
internal/server/handlers.gointernal/server/handlers_test.gotests/e2e/responses_test.go
💤 Files with no reviewable changes (1)
- internal/server/handlers.go
Comment on lines
+651
to
+661
| if rec.Code != http.StatusOK { | ||
| t.Errorf("expected status 200, got %d", rec.Code) | ||
| } | ||
|
|
||
| if provider.capturedChatReq.StreamOptions == nil { | ||
| t.Fatal("ChatCompletion streaming should have StreamOptions injected") | ||
| } | ||
|
|
||
| if !provider.capturedChatReq.StreamOptions.IncludeUsage { | ||
| t.Error("ChatCompletion streaming should have IncludeUsage=true") | ||
| } |
Contributor
There was a problem hiding this comment.
Same nil-guard / non-fatal status assertion issue as in TestStreamingResponses_DoesNotInjectStreamOptions.
t.Errorf at line 651 allows execution to continue to line 655, where provider.capturedChatReq.StreamOptions panics if capturedChatReq is nil.
🛡️ Proposed fix
- if rec.Code != http.StatusOK {
- t.Errorf("expected status 200, got %d", rec.Code)
- }
-
- if provider.capturedChatReq.StreamOptions == nil {
- t.Fatal("ChatCompletion streaming should have StreamOptions injected")
- }
-
- if !provider.capturedChatReq.StreamOptions.IncludeUsage {
- t.Error("ChatCompletion streaming should have IncludeUsage=true")
- }
+ if rec.Code != http.StatusOK {
+ t.Fatalf("expected status 200, got %d", rec.Code)
+ }
+
+ if provider.capturedChatReq == nil {
+ t.Fatal("StreamChatCompletion was not called")
+ }
+ if provider.capturedChatReq.StreamOptions == nil {
+ t.Fatal("ChatCompletion streaming should have StreamOptions injected")
+ }
+ if !provider.capturedChatReq.StreamOptions.IncludeUsage {
+ t.Error("ChatCompletion streaming should have IncludeUsage=true")
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/handlers_test.go` around lines 651 - 661, The status check in
TestStreamingResponses (the t.Errorf that asserts rec.Code == http.StatusOK)
must be made fatal or otherwise guarded so we don't continue when the response
is not OK and provider.capturedChatReq may be nil; change the non-fatal
assertion to t.Fatalf (or add an immediate nil-check on provider.capturedChatReq
before accessing StreamOptions) so that accessing
provider.capturedChatReq.StreamOptions cannot panic, and ensure the test still
verifies StreamOptions and IncludeUsage afterward.
Comment on lines
+143
to
+162
| t.Run("streaming does not inject stream_options", func(t *testing.T) { | ||
| // Regression test for GOM-43: streaming Responses API must not include | ||
| // stream_options.include_usage, which is a Chat Completions-only parameter. | ||
| // The Responses API returns usage in the response.done event by default. | ||
| payload := core.ResponsesRequest{ | ||
| Model: "gpt-4.1", | ||
| Input: "Hello", | ||
| Stream: true, | ||
| } | ||
|
|
||
| resp := sendResponsesRequest(t, payload) | ||
| defer closeBody(resp) | ||
|
|
||
| require.Equal(t, http.StatusOK, resp.StatusCode, "Streaming responses should succeed without stream_options") | ||
| assert.Equal(t, "text/event-stream", resp.Header.Get("Content-Type")) | ||
|
|
||
| events := readResponsesStream(t, resp.Body) | ||
| require.Greater(t, len(events), 0, "Should receive at least one SSE event") | ||
| assert.True(t, hasDoneEvent(events), "Should receive done event") | ||
| }) |
Contributor
There was a problem hiding this comment.
The E2E subtest doesn't assert what its name implies.
The subtest is named "streaming does not inject stream_options" and the comment calls it a regression test for GOM-43, but none of the assertions verify the absence of stream_options in the outgoing request. The four checks (status 200, text/event-stream, at least one event, done event) are identical to those in the "basic streaming" sibling test — differing only in the input string. If the in-process mock LLM server doesn't return a non-2xx response when it receives stream_options.include_usage, this test will pass regardless of whether the regression is reintroduced.
The unit test TestStreamingResponses_DoesNotInjectStreamOptions in handlers_test.go directly captures the request and is the real regression guard. Consider either:
- Adding an assertion here that the mock records/rejects the parameter (if the mock server supports it), or
- Removing this subtest as redundant and calling out in the test comment that the unit test covers the regression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/responses_test.go` around lines 143 - 162, The subtest currently
named "streaming does not inject stream_options" never asserts absence of
stream_options; update the test to actually verify the outgoing request body
does not include "stream_options" (or "include_usage") by either having
sendResponsesRequest return the captured HTTP request (or exposing a mock server
helper to fetch the lastRequest) and assert the request JSON does not contain
the stream_options key, or delete this redundant subtest and update the comment
to reference the unit test TestStreamingResponses_DoesNotInjectStreamOptions in
handlers_test.go as the regression guard; use the payload variable and helpers
sendResponsesRequest/readResponsesStream/hasDoneEvent to locate and adjust the
test accordingly.
Use t.Fatalf for status assertion and add nil guard on capturedResponsesReq before accessing StreamOptions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/server/handlers_test.go (1)
654-664:⚠️ Potential issue | 🟡 MinorNil-dereference risk on
capturedChatReqstill present — same issue as previous review.
t.Errorfon line 655 is non-fatal. If the handler short-circuits before callingStreamChatCompletion(e.g., routing/auth fails),capturedChatReqstaysnilandprovider.capturedChatReq.StreamOptionson line 658 panics instead of failing cleanly. Promote the status check tot.Fatalfand add a nil guard mirroring the fix already applied toTestStreamingResponses_DoesNotInjectStreamOptions.🛡️ Proposed fix
if rec.Code != http.StatusOK { - t.Errorf("expected status 200, got %d", rec.Code) + t.Fatalf("expected status 200, got %d", rec.Code) } + if provider.capturedChatReq == nil { + t.Fatal("StreamChatCompletion was not called") + } if provider.capturedChatReq.StreamOptions == nil { t.Fatal("ChatCompletion streaming should have StreamOptions injected") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/handlers_test.go` around lines 654 - 664, Change the non-fatal status assertion to a fatal one and guard against a nil capturedChatReq before dereferencing its StreamOptions: replace the t.Errorf("expected status 200...") with t.Fatalf(...) and add a nil check for provider.capturedChatReq (matching the pattern used in TestStreamingResponses_DoesNotInjectStreamOptions) so the test exits cleanly if StreamChatCompletion was never called before asserting provider.capturedChatReq.StreamOptions and IncludeUsage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/handlers_test.go`:
- Around line 554-578: The capture methods on capturingProvider
(StreamChatCompletion and StreamResponses) ignore the error field defined on
mockProvider (c.err) causing inconsistent mock behavior; update both methods in
capturingProvider to propagate the underlying mock error by checking c.err (or
delegating to mockProvider.StreamChatCompletion/StreamResponses) and returning
that error when set, otherwise capture the request into
capturedChatReq/capturedResponsesReq and return the stream as before.
---
Duplicate comments:
In `@internal/server/handlers_test.go`:
- Around line 654-664: Change the non-fatal status assertion to a fatal one and
guard against a nil capturedChatReq before dereferencing its StreamOptions:
replace the t.Errorf("expected status 200...") with t.Fatalf(...) and add a nil
check for provider.capturedChatReq (matching the pattern used in
TestStreamingResponses_DoesNotInjectStreamOptions) so the test exits cleanly if
StreamChatCompletion was never called before asserting
provider.capturedChatReq.StreamOptions and IncludeUsage.
Comment on lines
+554
to
+578
| // mockUsageLogger implements usage.LoggerInterface for testing. | ||
| type mockUsageLogger struct { | ||
| config usage.Config | ||
| } | ||
|
|
||
| func (m *mockUsageLogger) Write(_ *usage.UsageEntry) {} | ||
| func (m *mockUsageLogger) Config() usage.Config { return m.config } | ||
| func (m *mockUsageLogger) Close() error { return nil } | ||
|
|
||
| // capturingProvider is a mockProvider that captures the request passed to StreamResponses/StreamChatCompletion. | ||
| type capturingProvider struct { | ||
| mockProvider | ||
| capturedChatReq *core.ChatRequest | ||
| capturedResponsesReq *core.ResponsesRequest | ||
| } | ||
|
|
||
| func (c *capturingProvider) StreamChatCompletion(_ context.Context, req *core.ChatRequest) (io.ReadCloser, error) { | ||
| c.capturedChatReq = req | ||
| return io.NopCloser(strings.NewReader(c.streamData)), nil | ||
| } | ||
|
|
||
| func (c *capturingProvider) StreamResponses(_ context.Context, req *core.ResponsesRequest) (io.ReadCloser, error) { | ||
| c.capturedResponsesReq = req | ||
| return io.NopCloser(strings.NewReader(c.streamData)), nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
capturingProvider capture methods silently ignore m.err
StreamChatCompletion and StreamResponses on capturingProvider always return nil error regardless of c.mockProvider.err, while the base mockProvider versions honour it. This is harmless for the current tests (no error is set), but makes the mock inconsistent and could silently mask failures if a future test sets err and expects an error from those paths.
♻️ Optional: propagate err in capture methods
func (c *capturingProvider) StreamChatCompletion(_ context.Context, req *core.ChatRequest) (io.ReadCloser, error) {
c.capturedChatReq = req
+ if c.err != nil {
+ return nil, c.err
+ }
return io.NopCloser(strings.NewReader(c.streamData)), nil
}
func (c *capturingProvider) StreamResponses(_ context.Context, req *core.ResponsesRequest) (io.ReadCloser, error) {
c.capturedResponsesReq = req
+ if c.err != nil {
+ return nil, c.err
+ }
return io.NopCloser(strings.NewReader(c.streamData)), nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/handlers_test.go` around lines 554 - 578, The capture methods
on capturingProvider (StreamChatCompletion and StreamResponses) ignore the error
field defined on mockProvider (c.err) causing inconsistent mock behavior; update
both methods in capturingProvider to propagate the underlying mock error by
checking c.err (or delegating to
mockProvider.StreamChatCompletion/StreamResponses) and returning that error when
set, otherwise capture the request into capturedChatReq/capturedResponsesReq and
return the stream as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Responses API does not accept stream_options.include_usage — it's a Chat Completions-only parameter. The Responses API already returns usage data unconditionally in the response.done SSE event.
Summary by CodeRabbit
Bug Fixes
Tests