Conversation
- Request `identity` responses from `AutoRouter` upstreams - Share the `Accept-Encoding` policy across router paths - Add regressions for compressed client request headers
📝 WalkthroughWalkthroughA helper function is added to disable upstream response compression by setting Accept-Encoding to identity. The helper is integrated into the Forward and ForwardStreaming paths, replacing prior explicit header manipulation. Tests verify correct behavior in both paths. ChangesUpstream Compression Disabling
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
autorouter.go (1)
195-197: ⚡ Quick win
disableUpstreamResponseCompressionshould be called afterEnrichto prevent overrideCurrently called before
Enrichin bothForward(line 195) andForwardStreaming(line 330). A provider enricher that setsAccept-Encoding— even inadvertently — will silently overrideidentityand reintroduce the compression bug this PR fixes. Calling the helper afterEnrichmakes the guarantee unconditional.🛠️ Proposed fix for
Forward- disableUpstreamResponseCompression(upstreamReq) - if err := provider.RequestEnricher().Enrich(upstreamReq, meta, body); err != nil { return nil, ResponseMetadata{}, err } + + disableUpstreamResponseCompression(upstreamReq)Apply the same reordering in
ForwardStreaming(around lines 330–332).🤖 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 `@autorouter.go` around lines 195 - 197, The call to disableUpstreamResponseCompression is currently made before provider.RequestEnricher().Enrich(upstreamReq, meta, body), which allows an enricher to set Accept-Encoding and override the enforced identity; move the call to disableUpstreamResponseCompression(upstreamReq) to immediately after the Enrich(...) call in both Forward and ForwardStreaming so enrichment cannot reintroduce compression on upstreamReq.
🤖 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.
Inline comments:
In `@autorouter_test.go`:
- Around line 209-222: The test calls router.Forward(...) and asserts on resp
but never closes resp.Body; add a close to avoid leaking the response body by
deferring resp.Body.Close() (or explicitly closing it after use) immediately
after the nil error check when resp is non-nil so the http response body is
always released; locate the Forward call and ensure resp.Body.Close() is invoked
(e.g., directly after resp, meta, err := router.Forward(...) and err check) to
satisfy bodyclose vet/linter.
---
Nitpick comments:
In `@autorouter.go`:
- Around line 195-197: The call to disableUpstreamResponseCompression is
currently made before provider.RequestEnricher().Enrich(upstreamReq, meta,
body), which allows an enricher to set Accept-Encoding and override the enforced
identity; move the call to disableUpstreamResponseCompression(upstreamReq) to
immediately after the Enrich(...) call in both Forward and ForwardStreaming so
enrichment cannot reintroduce compression on upstreamReq.
🪄 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: 90f8ca09-1c25-4847-ba71-b95a7b043287
📒 Files selected for processing (2)
autorouter.goautorouter_test.go
📜 Review details
🧰 Additional context used
🪛 golangci-lint (2.12.1)
autorouter_test.go
[error] 209-209: response body must be closed
(bodyclose)
[error] 205-205: net/http/httptest.NewRequest must not be called. use net/http/httptest.NewRequestWithContext
(noctx)
🔇 Additional comments (2)
autorouter_test.go (1)
690-752: LGTM — streaming regression test is well-structured.Verifies Accept-Encoding, status code, metadata, and full event-stream body propagation in one test.
autorouter.go (1)
18-22:disableUpstreamResponseCompressionhelper is correct and well-scoped.
- Enforce `Accept-Encoding: identity` after provider enrichment - Cover enricher overrides in streaming and non-streaming tests - Close response bodies and satisfy diff-scoped lint
2 participants
Summary
AutoRouterupstream requests to sendAccept-Encoding: identityForwardandForwardStreamingCloses #12
Context
AutoRouter.Forwardcopied the caller's request headers into the upstream provider request. When the caller sent:Accept-Encoding: gzip, deflate, brthe upstream provider could return compressed JSON. Provider response extractors read
resp.Bodydirectly as JSON, which reproduced the observed failure:AutoRouter.ForwardStreamingalready forced upstreamAccept-Encoding: identity. This PR applies the same policy to non-streamingAutoRouterrequests and keeps both paths on one helper.What Changed
AutoRouterupstream responses stay uncompressedAutoRouter.Forwardnow normalizes the upstream request before provider enrichment so response extractors receive plain provider bodies.The streaming path uses the same helper, preserving the existing behavior while making the shared invariant explicit.
Regression coverage
The new non-streaming regression simulates a compressed-capable client. The test upstream returns gzipped JSON unless llmproxy sends:
Accept-Encoding: identityWithout the fix, the test fails with the same
\x1fJSON parse error. With the fix, extraction succeeds.A streaming regression confirms
ForwardStreamingstill sendsAccept-Encoding: identityupstream.Verification
TestAutoRouter_ForwardForcesIdentityAcceptEncodingfailing without the fix:Forward() error = invalid character '\x1f' looking for beginning of valuegit diff --cached --checkgo vet ./...go test -run 'TestAutoRouter_(ForwardForcesIdentityAcceptEncoding|ForwardStreamingForcesIdentityAcceptEncoding|Forward)$' .go test ./...go test -race ./.../Users/parteek/go/bin/golangci-lint run --new-from-rev=origin/main ./...go run golang.org/x/vuln/cmd/govulncheck@latest -show verbose ./...Notes
make testwas not used as the final verification command because localgovulncheckis not installed onPATH.go run golang.org/x/vuln/cmd/govulncheck@latest.References
AutoRouter.Forwardrequest-header forwarding: https://github.com/agentuity/llmproxy/blob/main/autorouter.go#L180-L187AutoRouter.roundTripprovider extraction: https://github.com/agentuity/llmproxy/blob/main/autorouter.go#L214-L223Accept-Encoding: identitybehavior: https://github.com/agentuity/llmproxy/blob/main/autorouter.go#L318-L323Accept-Encoding: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Accept-EncodingSummary by CodeRabbit