feat: add proxy integration tests and fix Codex↔Gemini converter bug#393
Merged
feat: add proxy integration tests and fix Codex↔Gemini converter bug#393
Conversation
…rter bug Add comprehensive end-to-end proxy integration tests covering all 4 protocol types (Claude, OpenAI, Codex, Gemini) with 20 test cases including passthrough, all 12 cross-protocol conversions, error handling, and multi-protocol coexistence. Fix a bug where Codex↔Gemini response transformers were registered with swapped implementations, causing silent data corruption: responses would be returned in the wrong format with null/zero fields instead of being properly converted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthrough该 PR 修复了 Codex 和 Gemini 协议转换器的响应转换函数注册，确保正确的方向映射。同时新增了全面的端到端测试套件，验证多协议路由、跨协议转换和错误处理场景。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 |
ymkiux
approved these changes
Mar 11, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/converter/codex_to_gemini.go (1)
12-14: 建议把 response transformer 按真实数据流重命名。这里能工作，是因为
geminiToCodexResponse实际上消费的是 Codex 响应并产出 Gemini 响应，而另一个文件里的codexToGeminiResponse正好相反。类型名和真实方向仍然是反着的，这次修完后后续维护者还是很容易把注册再写反。建议改成按响应体方向命名，或者把 request/response 的注册拆成两个显式 helper。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/converter/codex_to_gemini.go` around lines 12 - 14, The response transformer name is inverted: RegisterConverter(domain.ClientTypeCodex, domain.ClientTypeGemini, &codexToGeminiRequest{}, &geminiToCodexResponse{}) registers a transformer that actually consumes a Codex response and produces a Gemini response; rename the type to reflect the real data flow (e.g., geminiResponseFromCodex or codexResponseToGemini) and update its declaration and usages so the name matches "response body direction" (also update the counterpart type in the other file, codexToGeminiResponse, to the opposite-direction name), or alternatively replace this call with an explicit helper that registers request and response transformers separately to avoid future confusion; ensure all references to geminiToCodexResponse and codexToGeminiResponse are updated accordingly.tests/e2e/proxy_integration_test.go (1)
563-593: 建议给 Codex↔Gemini 再补一组流式回归用例。这次修的是 response transformer 的注册，而
Transform()和TransformChunk()共用同一条 registry 映射。当前两条回归只覆盖普通 JSON 响应，SSE 路径如果以后再注册反了，这组测试仍然会全绿。建议两个方向各补一个stream=true断言，直接校验返回事件形态。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/proxy_integration_test.go` around lines 563 - 593, Add two additional E2E cases mirroring "Codex_client_to_Gemini_upstream" and "Gemini_client_to_Codex_upstream" but exercising the SSE/streaming path (set stream=true on the client request), because Transform() and TransformChunk() share the same registry mapping; for each new case use the same clientType/clientReq/upstreamMock pairs and expectedUpPath/expectedUpField, and in respAssert assert streaming event shape (e.g., presence of SSE event markers or "event" fields and that body events follow the target format: Codex events contain "object":"response" and Gemini events include "candidates") so the transformer registration for chunks is validated separately from the JSON path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/proxy_integration_test.go`:
- Around line 636-659: In TestProxyUpstreamError (and the similar test around
lines 661-669), tighten assertions to assert the exact expected HTTP status
codes (e.g., expect http.StatusTooManyRequests/429 for the upstream rate-limit
case and the exact code for the "no matching route" case) instead of just
checking resp.StatusCode != http.StatusOK; additionally decode resp.Body (using
the same shape produced by the mock/openai response used in openaiRequest) and
assert key fields (e.g., error.type == "rate_limit_error" and error.message
contains "Rate limit exceeded" for the 429 case) to ensure the error body is
passed through unchanged; locate these changes in the TestProxyUpstreamError
test function and the analogous test that calls env.ProxyPost and uses
createProvider/createRoute.
- Around line 57-61: 在 tests/e2e/proxy_integration_test.go 中处理 JSON 反序列化时应在读取前立即
defer resp.Body.Close() 并检查 Decode 错误：在创建局部结果变量（result）并调用
json.NewDecoder(resp.Body).Decode(&result) 的位置（包括两处出现的代码块）改为先 defer
resp.Body.Close()，然后将 Decode 的返回值赋给 err 并在 err != nil 时使用 t.Fatalf 报告详细错误信息（包含
err 和上下文字符串），以避免在 decode 失败时静默继续导致误导性后续错误。
In `@tests/e2e/proxy_setup_test.go`:
- Around line 87-95: The test setup swallows errors from all cache preloads
(e.g., cachedProviderRepo.Load, cachedRouteRepo.Load,
cachedRetryConfigRepo.Load, cachedRoutingStrategyRepo.Load,
cachedProjectRepo.Load, cachedAPITokenRepo.Load, cachedModelMappingRepo.Load);
change each ignored call to check the returned error and call t.Fatalf with a
clear message including the failing repo name and the error (for example, if err
:= cachedProviderRepo.Load(); err != nil { t.Fatalf("cachedProviderRepo.Load
failed: %v", err) }) so test setup fails fast and surfaces the real preload
failure.
---
Nitpick comments:
In `@internal/converter/codex_to_gemini.go`:
- Around line 12-14: The response transformer name is inverted:
RegisterConverter(domain.ClientTypeCodex, domain.ClientTypeGemini,
&codexToGeminiRequest{}, &geminiToCodexResponse{}) registers a transformer that
actually consumes a Codex response and produces a Gemini response; rename the
type to reflect the real data flow (e.g., geminiResponseFromCodex or
codexResponseToGemini) and update its declaration and usages so the name matches
"response body direction" (also update the counterpart type in the other file,
codexToGeminiResponse, to the opposite-direction name), or alternatively replace
this call with an explicit helper that registers request and response
transformers separately to avoid future confusion; ensure all references to
geminiToCodexResponse and codexToGeminiResponse are updated accordingly.
In `@tests/e2e/proxy_integration_test.go`:
- Around line 563-593: Add two additional E2E cases mirroring
"Codex_client_to_Gemini_upstream" and "Gemini_client_to_Codex_upstream" but
exercising the SSE/streaming path (set stream=true on the client request),
because Transform() and TransformChunk() share the same registry mapping; for
each new case use the same clientType/clientReq/upstreamMock pairs and
expectedUpPath/expectedUpField, and in respAssert assert streaming event shape
(e.g., presence of SSE event markers or "event" fields and that body events
follow the target format: Codex events contain "object":"response" and Gemini
events include "candidates") so the transformer registration for chunks is
validated separately from the JSON path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7896739b-8056-4808-9ae1-f43d3bbeaee3
📒 Files selected for processing (4)
internal/converter/codex_to_gemini.gointernal/converter/gemini_to_codex.gotests/e2e/proxy_integration_test.gotests/e2e/proxy_setup_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: playwright
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
2 participants
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.
Summary
codex_to_gemini.goandgemini_to_codex.go, causing responses to be silently returned in the wrong format with null/zero fieldsBug Details
The
RegisterConverter()calls incodex_to_gemini.goandgemini_to_codex.gohad their response transformers swapped:codexToGeminiResponse.Transform()was readingGeminiResponseand outputtingCodexResponse(Gemini→Codex), but was registered atresponses[codex][gemini]which should convert Codex→GeminigeminiToCodexResponse.Transform()was readingCodexResponseand outputtingGeminiResponse(Codex→Gemini), but was registered atresponses[gemini][codex]which should convert Gemini→CodexThis caused the
ConvertingResponseWriter.Finalize()to unmarshal upstream responses into the wrong struct type, producing zero-valued output that was silently accepted (no error returned).Test plan
go test ./...) passes (only pre-existingTestInviteCodeConsume_Concurrentflake)🤖 Generated with Claude Code
Summary by CodeRabbit
发布说明
测试
改进