Fix anthropic stream and turn off Encoding when SSE#11
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an internal header-copy helper that omits Content-Encoding and Content-Length, forces upstream ChangesHeader forwarding & Anthropic stream flag
Dependency Updates
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 2/5 reviews remaining, refill in 29 minutes and 36 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@autorouter.go`:
- Around line 16-27: The copyResponseHeaders function is currently removing
headers in skipHeaders (which always includes "Content-Encoding") causing
encoded responses to be forwarded without the encoding header; update the logic
so Content-Encoding is not stripped unless the proxy actually decoded the body:
modify the skipHeaders usage or list so it does not include "Content-Encoding"
by default, or add a condition in copyResponseHeaders that preserves
"Content-Encoding" unless a body-decoding flag/state (e.g., a decodedBody
boolean passed into copyResponseHeaders or a response-processing function like
the upstream decoder) indicates the payload was decompressed—ensure you adjust
references to skipHeaders and the call sites of copyResponseHeaders accordingly.
- Around line 21-24: Remove the debugging stdout print from the header
forwarding loop: in the section where the code iterates over v and calls
header.Add(k, val) (the loop containing fmt.Println("SETTING HEADER", k, "=>",
val)), delete the fmt.Println call so only header.Add(k, val) remains; ensure no
other stdout logging remains in that proxy header forwarding path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02227d8d-58cc-41ac-b24b-c584c7709041
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
autorouter.gogo.modproviders/anthropic/parser.goproviders/anthropic/parser_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
providers/anthropic/parser.go (1)
44-49: Good stream propagation.This keeps
BodyMetadata.Streamaligned with Anthropic requests and avoids leakingstreamintoCustom.Also applies to: 87-93, 151-155
providers/anthropic/parser_test.go (1)
48-59: Nice regression test for the stream bit.This covers the behavior added in the parser and would catch the original miss quickly.
| func copyResponseHeaders(w http.ResponseWriter, headers http.Header) { | ||
| header := w.Header() | ||
|
|
||
| for k, v := range headers { | ||
| if !slice.Contains(skipHeaders, k, slice.WithCaseInsensitive()) { | ||
| for _, val := range v { | ||
| header.Add(k, val) | ||
| fmt.Println("SETTING HEADER", k, "=>", val) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't strip Content-Encoding unless this proxy has already decoded the body.
These paths forward upstream bytes back to the client, but the shared skip list now always removes Content-Encoding. If any upstream response still comes back encoded, clients will receive compressed bytes labeled as a plain payload.
Suggested change
-var skipHeaders = []string{"Content-Encoding", "Content-Length"}
+var skipHeaders = []string{"Content-Length"}Also applies to: 440-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@autorouter.go` around lines 16 - 27, The copyResponseHeaders function is
currently removing headers in skipHeaders (which always includes
"Content-Encoding") causing encoded responses to be forwarded without the
encoding header; update the logic so Content-Encoding is not stripped unless the
proxy actually decoded the body: modify the skipHeaders usage or list so it does
not include "Content-Encoding" by default, or add a condition in
copyResponseHeaders that preserves "Content-Encoding" unless a body-decoding
flag/state (e.g., a decodedBody boolean passed into copyResponseHeaders or a
response-processing function like the upstream decoder) indicates the payload
was decompressed—ensure you adjust references to skipHeaders and the call sites
of copyResponseHeaders accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
autorouter_test.go (1)
826-894: ⚡ Quick winAdd regression cases for multi-value and preexisting destination headers.
Current coverage validates filtering, but it does not protect against repeated headers (
Set-Cookie) or pre-set response headers (e.g.,Trailer) being overwritten. Adding those two cases will lock in the expected proxy semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autorouter_test.go` around lines 826 - 894, The test TestAutoRouter_copyResponseHeaders lacks regression cases for multi-value headers and for not overwriting preexisting destination headers; add two subcases that call copyResponseHeaders: one where the source header includes a multi-valued header like "Set-Cookie" (ensure all values are copied, not collapsed or lost), and one where the destination recorder already has a header such as "Trailer" pre-set (ensure copyResponseHeaders does not overwrite or drop the existing value on the ResponseRecorder). Locate the test function TestAutoRouter_copyResponseHeaders and extend it with assertions that verify all Set-Cookie values appear and that preexisting Trailer (or similar) header values remain intact after calling copyResponseHeaders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@autorouter.go`:
- Around line 23-28: The copyResponseHeaders loop currently uses header.Add for
the first value and header.Set for subsequent values causing multi-value headers
to be collapsed and existing response headers (e.g., Trailer) to be overwritten;
update the loop in copyResponseHeaders to always call header.Add for every value
v in range v (for each k, v := range ... and for _, val := range v) so that all
upstream header values are preserved and existing w.Header() entries are not
clobbered.
---
Nitpick comments:
In `@autorouter_test.go`:
- Around line 826-894: The test TestAutoRouter_copyResponseHeaders lacks
regression cases for multi-value headers and for not overwriting preexisting
destination headers; add two subcases that call copyResponseHeaders: one where
the source header includes a multi-valued header like "Set-Cookie" (ensure all
values are copied, not collapsed or lost), and one where the destination
recorder already has a header such as "Trailer" pre-set (ensure
copyResponseHeaders does not overwrite or drop the existing value on the
ResponseRecorder). Locate the test function TestAutoRouter_copyResponseHeaders
and extend it with assertions that verify all Set-Cookie values appear and that
preexisting Trailer (or similar) header values remain intact after calling
copyResponseHeaders.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9b33a3c-0ba0-4483-bec4-326e4758eeee
📒 Files selected for processing (2)
autorouter.goautorouter_test.go
📜 Review details
🔇 Additional comments (2)
autorouter.go (2)
16-16:Content-Encodingis still being stripped globally (previously raised).Line 16 still excludes
Content-Encodingfor all proxied responses, while Line 327 only forcesAccept-Encoding: identityin the streaming path. Non-stream responses can still carry encoded payloads, so this remains the same unresolved risk noted earlier.Also applies to: 366-366, 491-491
326-327: Good SSE safeguard withAccept-Encoding: identity.This is a solid change for stream stability and parser consistency when proxying SSE.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@autorouter_test.go`:
- Around line 894-895: The test contains a dead reassignment "w =
httptest.NewRecorder()" after "sw.Reset()" where the variable w is never used
later (SA4006); remove the redundant line assigning to w (or if intended,
replace with a real use of the new recorder) so the unused reassignment is
eliminated and lint stops flagging w as unused.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91cf0150-63b1-44dd-b3fb-46911181ea8e
📒 Files selected for processing (2)
autorouter.goautorouter_test.go
✅ Files skipped from review due to trivial changes (1)
- autorouter.go
📜 Review details
🧰 Additional context used
🪛 golangci-lint (2.11.4)
autorouter_test.go
[error] 895-895: SA4006: this value of w is never used
(staticcheck)
🔇 Additional comments (1)
autorouter_test.go (1)
826-893: Nice coverage of header-copy behavior.This test cleanly covers empty headers, normal header pass-through, and case-insensitive filtering of reserved headers.
| sw.Reset() | ||
| w = httptest.NewRecorder() |
There was a problem hiding this comment.
Remove dead code at the end of the test.
Line 895 reassigns w but it is never read afterward (SA4006), which can fail lint.
✂️ Proposed fix
- sw.Reset()
- w = httptest.NewRecorder()
+ sw.Reset()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sw.Reset() | |
| w = httptest.NewRecorder() | |
| sw.Reset() |
🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 895-895: SA4006: this value of w is never used
(staticcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@autorouter_test.go` around lines 894 - 895, The test contains a dead
reassignment "w = httptest.NewRecorder()" after "sw.Reset()" where the variable
w is never used later (SA4006); remove the redundant line assigning to w (or if
intended, replace with a real use of the new recorder) so the unused
reassignment is eliminated and lint stops flagging w as unused.
1 participant
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores