Fix AI Gateway model regressions#14
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📜 Recent review details🧰 Additional context used🪛 golangci-lint (2.12.1)autorouter.go[error] 228-228: Error return value of (errcheck) 🔇 Additional comments (4)
📝 WalkthroughWalkthroughThe PR short-circuits upstream HTTP error responses in router and proxy, adds OpenAI-compatible response content normalization, includes deepseek provider prefix stripping, and adds/updates unit and gated live integration tests covering these behaviors. ChangesError Handling & Provider Compatibility
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🧹 Nitpick comments (1)
autorouter_test.go (1)
263-263: 💤 Low valueStatic analysis suggests using
httptest.NewRequestWithContext.The
noctxlinter flagshttptest.NewRequestcalls. Whilehttptest.NewRequestusescontext.Background()internally, usingNewRequestWithContextexplicitly makes context handling clear.Note: This pattern is used throughout the existing tests in this file, so this is a low-priority cleanup that could be addressed project-wide.
Example fix (if adopting project-wide)
- req := httptest.NewRequest("POST", "/v1/chat/completions", bytes.NewReader([]byte(`{"model":"gpt-4"}`))) + req := httptest.NewRequestWithContext(context.Background(), "POST", "/v1/chat/completions", bytes.NewReader([]byte(`{"model":"gpt-4"}`)))🤖 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_test.go` at line 263, Replace the call to httptest.NewRequest in the test (the variable req in autorouter_test.go) with httptest.NewRequestWithContext and pass an explicit context (e.g., context.Background()) so the request has an explicit context; update the test to import context if missing and follow the same replacement for other httptest.NewRequest calls in this file to satisfy the noctx linter.
🤖 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.go`:
- Around line 226-232: The error-handling branch in autorouter.go reads
resp.Body but never closes it; update the block that handles resp.StatusCode >=
http.StatusBadRequest to ensure resp.Body.Close() is called in all returns (both
when io.ReadAll returns data and when it returns readErr) — e.g., call
resp.Body.Close() immediately after io.ReadAll (and also before returning on
readErr) so the response body is always closed (same fix pattern as used in
proxy.go).
In `@proxy.go`:
- Around line 129-135: The error branch reads resp.Body into rawBody but never
closes the original response body, leaking the connection; in the block that
handles resp.StatusCode >= http.StatusBadRequest (the code that reads via
io.ReadAll(resp.Body) and returns rawBody and ResponseMetadata{}), ensure you
call resp.Body.Close() after successfully reading (or use defer immediately
after checking resp.Body != nil) and before returning the wrapped NopCloser so
the original body is released back to the pool; update the error-return path
around resp and rawBody to close resp.Body reliably.
---
Nitpick comments:
In `@autorouter_test.go`:
- Line 263: Replace the call to httptest.NewRequest in the test (the variable
req in autorouter_test.go) with httptest.NewRequestWithContext and pass an
explicit context (e.g., context.Background()) so the request has an explicit
context; update the test to import context if missing and follow the same
replacement for other httptest.NewRequest calls in this file to satisfy the
noctx linter.
🪄 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: a227cfba-0bc9-4b15-8770-49892adbd4f7
📒 Files selected for processing (6)
aigateway_regression_live_test.goautorouter.goautorouter_test.goproviders/openai_compatible/extractor.goproviders/openai_compatible/extractor_test.goproxy.go
📜 Review details
🧰 Additional context used
🪛 golangci-lint (2.12.1)
autorouter_test.go
[error] 263-263: net/http/httptest.NewRequest must not be called. use net/http/httptest.NewRequestWithContext
(noctx)
🔇 Additional comments (9)
providers/openai_compatible/extractor.go (1)
139-180: LGTM! Clean implementation of multi-format content handling.The
UnmarshalJSONcorrectly normalizes OpenAI-compatible message content from both string and array-of-parts formats. The fallback to raw string on line 170 provides graceful degradation for unexpected formats.providers/openai_compatible/extractor_test.go (1)
137-182: LGTM! Good test coverage for the new array content parsing.The test validates the key scenario (concatenating multiple text parts from an array) and verifies raw body preservation.
autorouter.go (1)
582-582: LGTM! DeepSeek prefix added correctly.The addition of
"deepseek": truetoknownProviderPrefixesenables proper stripping ofdeepseek/prefixes before forwarding requests.aigateway_regression_live_test.go (1)
1-102: LGTM! Well-structured live integration test.The test is properly guarded with environment variables and build tags. The table-driven approach with per-provider skip conditions provides good flexibility for selective testing.
autorouter_test.go (5)
230-283: LGTM! Comprehensive test for upstream error pass-through.The test correctly verifies that:
- The extractor is not called for error responses
- The upstream error body is preserved
- Metadata remains empty
- No error is returned to the caller
285-346: LGTM! Good test for DeepSeek prefix stripping.The test verifies that
deepseek/deepseek-v4-prois correctly stripped todeepseek-v4-probefore being forwarded upstream.
348-413: LGTM! Good edge case coverage for empty error bodies.The test confirms that an empty upstream error body (Cohere scenario) doesn't trigger extractor errors.
415-458: LGTM! Proxy error pass-through test matches AutoRouter behavior.Good coverage ensuring both
ProxyandAutoRouterhandle upstream errors consistently.
603-603: LGTM! Test case added for deepseek prefix stripping.
jhaynie
force-pushed
the
fix-aigateway-model-regressions
branch
from
1d7ecb7 to
536df15
Compare
1 participant
Summary
Validation
Summary by CodeRabbit
New Features
Bug Fixes
Tests