fix: bridge OpenRouter Codex tools via chat completions#562
Open
awsl233777 wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough该 PR 添加了 OpenRouter 兼容判定并在 Executor.dispatch 中按需将自定义 Codex 请求桥接为 OpenAI Chat Completions：实现角色映射与工具过滤的转换器、OpenRouter 判定函数及其测试，并将转换流程集成到请求分发中（失败时回退原格式）。 变更内容Codex 到 OpenAI 的转换和工具过滤
OpenRouter 桥接判定和兼容性检查
请求分发流程集成
序列图sequenceDiagram
participant Client as Codex CLI
participant Dispatcher as Executor.dispatch
participant BridgeChecker as shouldBridgeCustomCodexViaOpenAI
participant Converter as CodexToOpenAI
participant URIHandler as ConvertRequestURI
participant OpenAI as OpenAI Chat Completions
Client->>Dispatcher: /api/openrouter/responses (Codex 请求)
Dispatcher->>BridgeChecker: 检查 custom OpenRouter provider
BridgeChecker-->>Dispatcher: true/false
alt 需要桥接
Dispatcher->>Converter: TransformRequest (角色+工具转换)
alt 转换成功
Converter-->>Dispatcher: OpenAI 请求体
Dispatcher->>URIHandler: /responses -> /v1/chat/completions
URIHandler-->>Dispatcher: /v1/chat/completions
Dispatcher->>OpenAI: 转换后的 OpenAI 格式请求
else 转换失败
Dispatcher-->>Client: 回退原始请求流程或错误
end
else 无需桥接
Dispatcher->>Converter: 走 NeedConvert 通用路径
end
OpenAI-->>Dispatcher: 响应
Dispatcher-->>Client: 返回结果
审查工作量估计🎯 4 (Complex) | ⏱️ ~45 minutes 建议审查人员
诗歌
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/converter/codex_to_openai.go (1)
140-149: ⚡ Quick win可以优化重复的字符串处理
函数在 141 行已经对
role执行了strings.ToLower(strings.TrimSpace(role))，但在 145 行又重复执行了相同的操作。建议直接返回 switch 语句匹配到的原始值（已经是标准化后的小写形式）。♻️ 优化建议
func codexMessageRoleToOpenAI(role string) string { - switch strings.ToLower(strings.TrimSpace(role)) { + normalized := strings.ToLower(strings.TrimSpace(role)) + switch normalized { case "developer", "system": return "system" case "assistant", "user", "tool", "function": - return strings.ToLower(strings.TrimSpace(role)) + return normalized default: return "user" } }🤖 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 `@internal/converter/codex_to_openai.go` around lines 140 - 149, The function codexMessageRoleToOpenAI calls strings.ToLower(strings.TrimSpace(role)) twice; normalize role once into a local variable (e.g., normalized := strings.ToLower(strings.TrimSpace(role))) then use that variable in the switch and return the matched value directly (for example return normalized for the assistant/user/tool/function cases) to eliminate redundant string processing and keep logic clear.internal/executor/openrouter_bridge.go (1)
52-65: 💤 Low value后备字符串检查可能存在误报风险
64 行的
strings.Contains(trimmed, "openrouter.ai")可以匹配包含该字符串的任意 URL，例如https://evil.com/path/openrouter.ai/fake也会返回 true。虽然这种场景在实际配置中不太可能出现，但如果需要更严格的验证，建议移除此后备检查或增加额外的上下文验证（例如检查是否出现在域名部分）。🤖 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 `@internal/executor/openrouter_bridge.go` around lines 52 - 65, The fallback strings.Contains(trimmed, "openrouter.ai") in isOpenRouterCompatibleURL can yield false positives for paths or query strings; instead, require a successful url.Parse and validate the domain portion only (use parsed.Hostname(), strip "www.", then check equality or hasSuffix ".openrouter.ai"); drop the broad strings.Contains check and return false if parsing fails so only host-based matches pass.internal/converter/codex_openai_more_test.go (1)
62-64: ⚡ Quick win断言条件可以更精确
当前断言
len(got.Messages) < 2只验证至少有 2 条消息，但根据输入（1 条 developer、1 条 user）应该恰好有 2 条消息。建议使用相等性检查以提高测试的准确性。💚 改进建议
- if len(got.Messages) < 2 || got.Messages[0].Role != "system" || got.Messages[1].Role != "user" { + if len(got.Messages) != 2 || got.Messages[0].Role != "system" || got.Messages[1].Role != "user" { t.Fatalf("unexpected converted roles: %#v", got.Messages) }🤖 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 `@internal/converter/codex_openai_more_test.go` around lines 62 - 64, The assertion is too loose—replace the length check with an exact equality so the test verifies there are exactly two converted messages; specifically update the condition that inspects got.Messages (the current if using len(got.Messages) < 2) to assert len(got.Messages) == 2 and still verify the roles order (got.Messages[0].Role == "system" and got.Messages[1].Role == "user"), and adjust the t.Fatalf message accordingly to reflect the stricter expectation.
🤖 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.
Nitpick comments:
In `@internal/converter/codex_openai_more_test.go`:
- Around line 62-64: The assertion is too loose—replace the length check with an
exact equality so the test verifies there are exactly two converted messages;
specifically update the condition that inspects got.Messages (the current if
using len(got.Messages) < 2) to assert len(got.Messages) == 2 and still verify
the roles order (got.Messages[0].Role == "system" and got.Messages[1].Role ==
"user"), and adjust the t.Fatalf message accordingly to reflect the stricter
expectation.
In `@internal/converter/codex_to_openai.go`:
- Around line 140-149: The function codexMessageRoleToOpenAI calls
strings.ToLower(strings.TrimSpace(role)) twice; normalize role once into a local
variable (e.g., normalized := strings.ToLower(strings.TrimSpace(role))) then use
that variable in the switch and return the matched value directly (for example
return normalized for the assistant/user/tool/function cases) to eliminate
redundant string processing and keep logic clear.
In `@internal/executor/openrouter_bridge.go`:
- Around line 52-65: The fallback strings.Contains(trimmed, "openrouter.ai") in
isOpenRouterCompatibleURL can yield false positives for paths or query strings;
instead, require a successful url.Parse and validate the domain portion only
(use parsed.Hostname(), strip "www.", then check equality or hasSuffix
".openrouter.ai"); drop the broad strings.Contains check and return false if
parsing fails so only host-based matches pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d33701b1-ecdd-4ccd-84bf-f5902396905d
📒 Files selected for processing (5)
internal/converter/codex_openai_more_test.gointernal/converter/codex_to_openai.gointernal/executor/middleware_dispatch.gointernal/executor/openrouter_bridge.gointernal/executor/openrouter_bridge_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). (3)
- GitHub Check: Backend Checks
- GitHub Check: e2e
- GitHub Check: playwright
🔇 Additional comments (11)
internal/converter/codex_to_openai.go (2)
83-83: LGTM!
119-126: LGTM!internal/executor/openrouter_bridge.go (3)
17-41: LGTM!
43-50: LGTM!
67-69: LGTM!internal/executor/openrouter_bridge_test.go (5)
9-21: LGTM!
23-35: LGTM!
37-49: LGTM!
51-66: LGTM!
68-75: LGTM!internal/executor/middleware_dispatch.go (1)
78-99: LGTM!
Collaborator
Author
|
Follow-up for head This round addresses the review nits from CodeRabbit:
Validation run locally:
Remaining known status from my side:
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
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.
Summary
Fixes #537 by routing custom OpenRouter-compatible Codex requests through OpenAI Chat Completions when the provider also supports OpenAI format.
This avoids sending Codex Responses API tool schemas directly to OpenRouter-compatible
/responsesendpoints, where non-openrouter:*tool types are rejected.Changes
developermessages to Chat-compatiblesystemmessages during Codex → OpenAI conversion.web_searchthat cannot be represented as Chat Completions function tools./responses→/v1/chat/completionspath, role normalization, and tool filtering.Validation
go test ./internal/converter ./internal/executor ./internal/adapter/provider/customgo test ./internal/.../responses→ telepubDeepSeek-V4-pro, including a function tool payload, returned HTTP 200 andmaxx537-ok.codexmate codex execsmoke test through local maxx using telepubDeepSeek-V4-pro; Codex CLI request included 44 tools includingweb_search, and returnedmaxx537-codexmate-ok.No API keys are included in this PR.
Summary by CodeRabbit
Bug Fixes
New Features
Tests