feat(llm): support OpenAI Codex endpoints#62
Conversation
|
Harshit Sharma seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if req.MaxTokens > 0 && useMaxCompletionTokens(model) { | ||
| delete(body, "max_tokens") | ||
| body["max_completion_tokens"] = req.MaxTokens | ||
| } | ||
| for k, v := range c.cfg.ExtraBody { | ||
| body[k] = v | ||
| } |
There was a problem hiding this comment.
Bug: ExtraBody 可能覆盖或撤销 max_completion_tokens 的转换
在此处先将 max_tokens 删除并替换为 max_completion_tokens，但随后在下方遍历 c.cfg.ExtraBody 时，用户配置的 ExtraBody 可能会重新写入 max_tokens 键，导致请求体中同时存在 max_tokens 和 max_completion_tokens。OpenAI API 对这两种字段同时存在时的行为未做明确保证，可能导致调用失败或行为异常。
建议：在 ExtraBody 合并之后再做 max_tokens -> max_completion_tokens 的转换，或者在 ExtraBody 合并后再次清理冲突字段。例如：
for k, v := range c.cfg.ExtraBody {
body[k] = v
}
// 在 ExtraBody 合并之后再处理转换
if req.MaxTokens > 0 && useMaxCompletionTokens(model) {
delete(body, "max_tokens")
body["max_completion_tokens"] = req.MaxTokens
}
| func useMaxCompletionTokens(model string) bool { | ||
| lower := strings.ToLower(model) | ||
| return strings.Contains(lower, "gpt-5") || | ||
| strings.Contains(lower, "codex") || | ||
| strings.Contains(lower, "o1") || | ||
| strings.Contains(lower, "o3") || | ||
| strings.Contains(lower, "o4") | ||
| } |
There was a problem hiding this comment.
DRY 违规：encodingForModel 和 useMaxCompletionTokens 中的模型匹配逻辑完全重复
这两个函数都维护着相同的模型名称匹配条件 (gpt-5, codex, o1, o3, o4)。未来新增模型时，如果只更新了其中一个函数而遗漏另一个，将导致 token 计数与实际请求参数不一致。
建议：将模型匹配逻辑提取为一个共享的判断函数，例如：
func isAdvancedReasoningModel(modelName string) bool {
lower := strings.ToLower(modelName)
return strings.Contains(lower, "gpt-5") ||
strings.Contains(lower, "codex") ||
strings.Contains(lower, "o1") ||
strings.Contains(lower, "o3") ||
strings.Contains(lower, "o4")
}然后在 encodingForModel 和 useMaxCompletionTokens 中复用该函数。
| func isResponsesEndpoint(rawURL string) bool { | ||
| return strings.HasSuffix(strings.TrimRight(rawURL, "/"), "/responses") | ||
| } |
There was a problem hiding this comment.
isResponsesEndpoint 对含查询参数的 URL 匹配可能失败
该函数使用 strings.HasSuffix 检查 URL 是否以 /responses 结尾，但未考虑 URL 可能携带查询参数（如 https://example.com/v1/responses?api-version=2024-01）或片段标识符的情况。这会导致本应路由到 OpenAIResponsesClient 的请求被错误地路由到 OpenAIClient。
建议：使用 net/url 包先解析 URL，基于路径部分进行匹配：
func isResponsesEndpoint(rawURL string) bool {
u, err := url.Parse(rawURL)
if err != nil {
return strings.HasSuffix(strings.TrimRight(rawURL, "/"), "/responses")
}
return strings.HasSuffix(strings.TrimRight(u.Path, "/"), "/responses")
}
| func responseContentAsString(content any) string { | ||
| switch v := content.(type) { | ||
| case string: | ||
| return v | ||
| case []ContentBlock: | ||
| msg := Message{Content: v} | ||
| return msg.ExtractText() | ||
| default: | ||
| return fmt.Sprintf("%v", v) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: 当 content 为 nil 时（Message.Content 类型为 any，完全可能为 nil），fmt.Sprintf("%v", nil) 会返回字符串 "<nil>"，这会被作为工具调用输出或消息内容发送给 API，导致请求内容错误。
注意到 ExtractText() 的 default 分支返回的是空字符串 ""，这里应保持一致。
Suggestion:
| func responseContentAsString(content any) string { | |
| switch v := content.(type) { | |
| case string: | |
| return v | |
| case []ContentBlock: | |
| msg := Message{Content: v} | |
| return msg.ExtractText() | |
| default: | |
| return fmt.Sprintf("%v", v) | |
| } | |
| } | |
| func responseContentAsString(content any) string { | |
| switch v := content.(type) { | |
| case string: | |
| return v | |
| case []ContentBlock: | |
| msg := Message{Content: v} | |
| return msg.ExtractText() | |
| case nil: | |
| return "" | |
| default: | |
| return fmt.Sprintf("%v", v) | |
| } | |
| } |
| finishReason := "stop" | ||
| if len(toolCalls) > 0 { | ||
| finishReason = "tool_calls" | ||
| } |
There was a problem hiding this comment.
Bug: finishReason 仅根据是否存在 tool calls 来判断，完全忽略了 Responses API 实际返回的完成原因。如果响应因 max_output_tokens 限制被截断（API 返回 "length"），此处仍会标记为 "stop"，导致调用方无法区分内容是正常结束还是被截断。
Responses API 在 output 的 message 项中包含 stop_reason 字段，应当解析并使用该值。可参考 Anthropic 客户端的做法（finishReason := resp.StopReason，空时回退到 "stop"）。
| baseURL := strings.TrimRight(cfg.URL, "/") | ||
| if !strings.HasSuffix(baseURL, "/responses") { | ||
| cfg.URL = baseURL + "/responses" | ||
| } |
There was a problem hiding this comment.
潜在问题: NewOpenAIResponsesClient 中修改了 cfg.URL 的值（追加 /responses），但 cfg 是 ClientConfig 的值拷贝，因此 cfg.URL 的修改只影响本结构体内部的副本。然而，isResponsesEndpoint 的检测逻辑和这里的追加逻辑之间存在微妙的不一致：
isResponsesEndpoint检查HasSuffix(TrimRight(url, "/"), "/responses")- 这里检查
HasSuffix(baseURL, "/responses")，其中baseURL = TrimRight(cfg.URL, "/")
两者逻辑一致，所以目前不会出问题。但如果用户的 URL 是类似 https://example.com/v1/responses 的完整路径，这里不会再追加，请求会正确发送到该地址。不过如果 URL 是 https://example.com/v1 这种不包含 /responses 的路径，则会被自动追加。建议在注释中明确说明这种 URL 自动补全行为，方便后续维护。
|
🔍 OpenCodeReview found 8 issue(s) in this PR.
📊 Posting Statistics:
❌ Failed Comments Details
|
2 participants
Summary
/v1/responsesendpointsmax_completion_tokensfor GPT-5/Codex/o-series chat-completions models while preservingmax_tokensfor legacy modelsOCR_LLM_AUTH_TOKEN/OCR_LLM_USE_ANTHROPICnamesTests
git diff --checkgo test ./internal/llmmake buildocr llm testagainst an Azure GPT-5.5 chat-completions deploymentNote:
go test ./...currently fails ininternal/config/rulesbecause existing tests expect Chinese default-rule snippets while the embedded default rules resolve to English text; that failure is unrelated to this LLM endpoint change.