Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
📝 Walkthrough概述将 Codex 有效载荷规范化逻辑从多个适配器中的内联实现迁移到集中的 更改
估计代码审查工作量🎯 3 (中等) | ⏱️ ~20 分钟 可能相关的 PR
建议的审查者
诗
✨ 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 |
![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)
internal/adapter/provider/codex/adapter_test.go (1)
39-44: 建议补一个“max_tokens与max_output_tokens同时存在”的优先级用例当前断言验证了迁移路径，但还没覆盖冲突输入场景。建议新增用例明确“已存在
max_tokens时不被max_output_tokens覆盖”，可避免后续回归。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/provider/codex/adapter_test.go` around lines 39 - 44, 为避免回归，请新增一个优先级用例（例如 TestTunedMaxTokensPriority）在输入同时包含 max_tokens 和 max_output_tokens 时断言：经过迁移后 max_output_tokens 被移除（使用 gjson.GetBytes(tuned, "max_output_tokens").Exists() 为 false），但已有的 max_tokens 不被覆盖（使用 gjson.GetBytes(tuned, "max_tokens").Int() 等于原始设定值）；在测试中构造包含两者的原始 JSON 并调用当前迁移/处理函数以验证这些断言。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/codexutil/normalize.go`:
- Around line 25-38: 在处理 function_call 的分支中，确保即便 id 已有 "fc_" 前缀但包含前后空白也会被修正：对
item.Get("id").String() 做 strings.TrimSpace(id) 后，如果 strings.HasPrefix(trimmed,
"fc_") 仍需用 sjson.SetBytes 将去空白后的值写回（例如 "fc_"+trimmedWithoutPrefix 或直接
trimmed），并保持现有对空 id 的生成逻辑；在处理 function_call_output 的分支，不仅检查
item.Get("output").Exists()，还要识别 output 为 JSON null（例如通过 item.Get("output").Type
== gjson.Null 或检查 item.Get("output").Raw == "null"）并在这两种情况下把 output 写回为 ""（使用
sjson.SetBytes），以消除 {"output":null} 的边界情况。
In `@internal/converter/claude_to_codex.go`:
- Line 31: The struct field Store is being set to the boolean zero value but is
tagged with `omitempty`, so it is omitted from JSON and cannot force
"store:false"; change the model to distinguish “unset” vs “explicit false” by
making Store a pointer (*bool) or a nullable type, and ensure serialization
emits "store": false when explicitly false (alternatively, after marshaling the
request object, inject the "store": false key into the JSON map/string before
sending). Update all codepaths that construct the request (where Store is
assigned) and any unmarshaling/validation logic to handle the new
pointer/nullable type (look for uses of the Store field and the function that
builds/serializes the Claude->Codex request).
---
Nitpick comments:
In `@internal/adapter/provider/codex/adapter_test.go`:
- Around line 39-44: 为避免回归，请新增一个优先级用例（例如 TestTunedMaxTokensPriority）在输入同时包含
max_tokens 和 max_output_tokens 时断言：经过迁移后 max_output_tokens 被移除（使用
gjson.GetBytes(tuned, "max_output_tokens").Exists() 为 false），但已有的 max_tokens
不被覆盖（使用 gjson.GetBytes(tuned, "max_tokens").Int() 等于原始设定值）；在测试中构造包含两者的原始 JSON
并调用当前迁移/处理函数以验证这些断言。
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/adapter/provider/cliproxyapi_codex/adapter.gointernal/adapter/provider/codex/adapter.gointernal/adapter/provider/codex/adapter_test.gointernal/codexutil/normalize.gointernal/converter/claude_to_codex.go
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (3)
internal/adapter/provider/codex/adapter.go (1)
internal/codexutil/normalize.go (1)
NormalizeCodexInput(12-43)
internal/converter/claude_to_codex.go (1)
internal/converter/types_codex.go (2)
CodexInputItem(23-32)CodexOutput(57-66)
internal/adapter/provider/cliproxyapi_codex/adapter.go (1)
internal/codexutil/normalize.go (1)
NormalizeCodexInput(12-43)
🔇 Additional comments (4)
internal/converter/claude_to_codex.go (2)
94-121:tool_use/tool_result到 Codex 输入项的映射清晰且方向正确这里把
function_call与function_call_output分开构造，并保留CallID，可读性和兼容性都比内联拼装更好。
204-215: 响应侧ID/CallID的双轨映射实现合理
ID统一为fc_前缀、CallID保留调用语义，和请求侧策略保持一致，利于上下游对齐。internal/adapter/provider/codex/adapter.go (1)
565-577: 请求调优与标准化顺序合理先做
max_output_tokens兼容迁移，再统一走NormalizeCodexInput，可避免字段冲突并减少重复逻辑，改动方向正确。internal/adapter/provider/cliproxyapi_codex/adapter.go (1)
231-258: 在执行前统一做 Codex payload 归一化是正确改动这让 CLIProxyAPI 路径与主 Codex 适配器的输入语义保持一致，能减少同类请求在不同通道下的行为差异。
Comment on lines
+25
to
+38
| if itemType == "function_call" { | ||
| id := strings.TrimSpace(item.Get("id").String()) | ||
| switch { | ||
| case strings.HasPrefix(id, "fc_"): | ||
| case id == "": | ||
| body, _ = sjson.SetBytes(body, fmt.Sprintf("input.%d.id", i), "fc_"+uuid.NewString()) | ||
| default: | ||
| body, _ = sjson.SetBytes(body, fmt.Sprintf("input.%d.id", i), "fc_"+id) | ||
| } | ||
| } | ||
| if itemType == "function_call_output" { | ||
| if !item.Get("output").Exists() { | ||
| body, _ = sjson.SetBytes(body, fmt.Sprintf("input.%d.output", i), "") | ||
| } |
There was a problem hiding this comment.
归一化对“带空白的已前缀 ID”和 output:null 仍有边界遗漏
当前逻辑在 id 已是 fc_ 前缀时不会回写去空白值；同时只判断 output 是否存在会漏掉 {"output":null}。这会让“看起来已规范化”的输入仍带异常值。
建议修复示例
if itemType == "function_call" {
- id := strings.TrimSpace(item.Get("id").String())
+ rawID := item.Get("id").String()
+ id := strings.TrimSpace(rawID)
switch {
case strings.HasPrefix(id, "fc_"):
+ if rawID != id {
+ body, _ = sjson.SetBytes(body, fmt.Sprintf("input.%d.id", i), id)
+ }
case id == "":
body, _ = sjson.SetBytes(body, fmt.Sprintf("input.%d.id", i), "fc_"+uuid.NewString())
default:
body, _ = sjson.SetBytes(body, fmt.Sprintf("input.%d.id", i), "fc_"+id)
}
}
if itemType == "function_call_output" {
- if !item.Get("output").Exists() {
+ out := item.Get("output")
+ if !out.Exists() || out.Raw == "null" {
body, _ = sjson.SetBytes(body, fmt.Sprintf("input.%d.output", i), "")
}
}📝 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.
Suggested change
| if itemType == "function_call" { | |
| id := strings.TrimSpace(item.Get("id").String()) | |
| switch { | |
| case strings.HasPrefix(id, "fc_"): | |
| case id == "": | |
| body, _ = sjson.SetBytes(body, fmt.Sprintf("input.%d.id", i), "fc_"+uuid.NewString()) | |
| default: | |
| body, _ = sjson.SetBytes(body, fmt.Sprintf("input.%d.id", i), "fc_"+id) | |
| } | |
| } | |
| if itemType == "function_call_output" { | |
| if !item.Get("output").Exists() { | |
| body, _ = sjson.SetBytes(body, fmt.Sprintf("input.%d.output", i), "") | |
| } | |
| if itemType == "function_call" { | |
| rawID := item.Get("id").String() | |
| id := strings.TrimSpace(rawID) | |
| switch { | |
| case strings.HasPrefix(id, "fc_"): | |
| if rawID != id { | |
| body, _ = sjson.SetBytes(body, fmt.Sprintf("input.%d.id", i), id) | |
| } | |
| case id == "": | |
| body, _ = sjson.SetBytes(body, fmt.Sprintf("input.%d.id", i), "fc_"+uuid.NewString()) | |
| default: | |
| body, _ = sjson.SetBytes(body, fmt.Sprintf("input.%d.id", i), "fc_"+id) | |
| } | |
| } | |
| if itemType == "function_call_output" { | |
| out := item.Get("output") | |
| if !out.Exists() || out.Raw == "null" { | |
| body, _ = sjson.SetBytes(body, fmt.Sprintf("input.%d.output", i), "") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/codexutil/normalize.go` around lines 25 - 38, 在处理 function_call
的分支中，确保即便 id 已有 "fc_" 前缀但包含前后空白也会被修正：对 item.Get("id").String() 做
strings.TrimSpace(id) 后，如果 strings.HasPrefix(trimmed, "fc_") 仍需用 sjson.SetBytes
将去空白后的值写回（例如 "fc_"+trimmedWithoutPrefix 或直接 trimmed），并保持现有对空 id 的生成逻辑；在处理
function_call_output 的分支，不仅检查 item.Get("output").Exists()，还要识别 output 为 JSON
null（例如通过 item.Get("output").Type == gjson.Null 或检查 item.Get("output").Raw ==
"null"）并在这两种情况下把 output 写回为 ""（使用 sjson.SetBytes），以消除 {"output":null} 的边界情况。
| MaxOutputTokens: req.MaxTokens, | ||
| Temperature: req.Temperature, | ||
| TopP: req.TopP, | ||
| Store: false, |
There was a problem hiding this comment.
Store: false 在 omitempty 下不会落到最终 JSON，无法真正“强制关闭存储”
Line 31 仅设置布尔零值；若 Store 字段带 json:",omitempty"，序列化时会被省略。这样上游若默认不是 false，就无法保证行为一致。建议在序列化后显式写入 store:false（或改字段建模为可区分“未设置/显式 false”的类型）。
建议修复示例
import (
"encoding/json"
"strings"
"time"
"github.com/awsl-project/maxx/internal/domain"
+ "github.com/tidwall/sjson"
)
@@
- return json.Marshal(codexReq)
+ encoded, err := json.Marshal(codexReq)
+ if err != nil {
+ return nil, err
+ }
+ encoded, _ = sjson.SetBytes(encoded, "store", false)
+ return encoded, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/converter/claude_to_codex.go` at line 31, The struct field Store is
being set to the boolean zero value but is tagged with `omitempty`, so it is
omitted from JSON and cannot force "store:false"; change the model to
distinguish “unset” vs “explicit false” by making Store a pointer (*bool) or a
nullable type, and ensure serialization emits "store": false when explicitly
false (alternatively, after marshaling the request object, inject the "store":
false key into the JSON map/string before sending). Update all codepaths that
construct the request (where Store is assigned) and any unmarshaling/validation
logic to handle the new pointer/nullable type (look for uses of the Store field
and the function that builds/serializes the Claude->Codex request).
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:
cleanup, function_call id prefixing with fc_, uuid fallback, output defaulting).
defaulted function_call_output output.
Test:
Files touched:
Summary by CodeRabbit
发版说明
重构
测试