fix: normalize web_search_preview for codex responses#2223
fix: normalize web_search_preview for codex responses#2223cnrpman wants to merge 3 commits intorouter-for-me:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a compatibility layer within the Codex OpenAI Responses translator. It addresses an issue where the Codex upstream API rejects requests using the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the issue of normalizing web_search_preview to web_search for Codex Responses, ensuring compatibility with the upstream API. The new normalizeCodexBuiltinTools function is well-implemented and the addition of a dedicated test case provides good coverage for the introduced functionality. The changes are concise and directly target the problem described in the pull request.
| for i := 0; i < len(toolArray); i++ { | ||
| typePath := fmt.Sprintf("tools.%d.type", i) | ||
| if gjson.GetBytes(result, typePath).String() == "web_search_preview" { | ||
| result, _ = sjson.SetBytes(result, typePath, "web_search") |
There was a problem hiding this comment.
The sjson.SetBytes function can return an error, which is currently ignored. While the transformation might be expected to always succeed for valid JSON, explicitly handling potential errors (e.g., logging them or returning the original rawJSON if an error occurs) would make the function more robust against unexpected input structures. This could prevent silent failures where the normalization doesn't happen as intended.
result, err := sjson.SetBytes(result, typePath, "web_search")
if err != nil {
// Log the error or handle it appropriately
// For now, we proceed with the original result if setting fails
// log.Printf("Error setting tool type: %v", err)
}| } | ||
|
|
||
| if gjson.GetBytes(result, "tool_choice.type").String() == "web_search_preview" { | ||
| result, _ = sjson.SetBytes(result, "tool_choice.type", "web_search") |
There was a problem hiding this comment.
Similar to the previous comment, the error returned by sjson.SetBytes here is ignored. Consider adding error handling to ensure that any issues during JSON modification are either logged or gracefully managed.
result, err := sjson.SetBytes(result, "tool_choice.type", "web_search")
if err != nil {
// Log the error or handle it appropriately
// For now, we proceed with the original result if setting fails
// log.Printf("Error setting tool_choice type: %v", err)
}There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be2dd60ee7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if gjson.GetBytes(result, typePath).String() == "web_search_preview" { | ||
| result, _ = sjson.SetBytes(result, typePath, "web_search") |
There was a problem hiding this comment.
Normalize the dated web_search_preview alias too
This comparison only catches the undated literal web_search_preview. OpenAI’s current tool schema/SDKs also emit the dated alias web_search_preview_2025_03_11; when a caller uses that value in tools (or in the identical tool_choice.type check below), the translator forwards it unchanged and Codex will still reject the request as an unsupported tool type. That leaves the new compatibility layer incomplete for one of the valid preview names clients send today.
Useful? React with 👍 / 👎.
| if gjson.GetBytes(result, "tool_choice.type").String() == "web_search_preview" { | ||
| result, _ = sjson.SetBytes(result, "tool_choice.type", "web_search") |
There was a problem hiding this comment.
Rewrite preview tools nested under
tool_choice.allowed_tools
Only the top-level tool_choice.type is normalized here. Responses requests can also force a subset of tools with tool_choice: {"type":"allowed_tools","tools":[...]}; if one of those nested entries is {"type":"web_search_preview"}, it survives translation unchanged because this helper only touches the root tools[] array and tool_choice.type. In that supported tool-choice mode, Codex will continue to fail on the same legacy tool value this patch is trying to make compatible.
Useful? React with 👍 / 👎.
|
all review comments above addressed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 793840cdb4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| case "web_search_preview", "web_search_preview_2025_03_11": | ||
| return "web_search" |
There was a problem hiding this comment.
Preserve
external_web_access semantics for preview web search
For requests that still use {"type":"web_search_preview"} (or the dated alias) together with external_web_access: false, this rewrite changes behavior instead of just making the request acceptable. OpenAI’s current web-search guide says preview variants ignore external_web_access and behave as if it were true, but after translating only the type field we forward {"type":"web_search","external_web_access":false} to Codex, which disables live web access. Existing preview clients relying on the documented preview behavior will silently start getting cache-only search results.
Useful? React with 👍 / 👎.
xkonjin
left a comment
There was a problem hiding this comment.
Code Review
Clean, focused PR. The normalization approach is solid. A few observations:
Looks good:
- Smart placement in the translation pipeline right after
convertSystemRoleToDeveloper - Covers both
tools[]andtool_choice(including nestedtool_choice.tools[]) - Good test coverage with both variants (
web_search_previewandweb_search_preview_2025_03_11) - The helper returning empty string for unknown types is a safe default
Suggestions:
-
Future-proofing: The
normalizeCodexBuiltinToolTypeswitch only handlesweb_search_preview*variants. Consider a comment noting this is the single known case, so future maintainers know whether to extend it or use a different pattern if more normalizations are needed. -
Logging: Consider adding a debug log when normalization fires (e.g.
log.Debug("normalized tool type", "from", toolType, "to", "web_search")). This would help diagnose issues if Codex upstream changes behavior again. -
Minor: The second test (
TestConvertOpenAIResponsesRequestToCodex_NormalizesTopLevelToolChoicePreviewAlias) teststool_choice.typedirectly set toweb_search_preview_2025_03_11. This is good, but worth verifying whether any real client sendstool_choice.typeas a raw tool type vs wrapped inallowed_tools— both are covered now though.
No blocking issues. Approve-worthy once CI passes.
Summary
Normalize
web_search_previewtoweb_searchfor Codex Responses requests.Codex upstream currently rejects
web_search_previewwith:This change adds a narrow compatibility layer in the Codex OpenAI Responses translator so existing
clients using the older tool type continue to work.
What changed
Scope
This change is intentionally limited to the Codex Responses translation path and does not affect other
providers or request formats.
Verification
Ran:
go test ./internal/translator/codex/openai/responses
Also verified end-to-end locally by retrying a /v1/responses request that previously failed with
Unsupported tool type: web_search_preview.
Misc
might be related to #1539
but the current PR is a minimal fix towards
web_search_previewcompatibility