fix(responses): tolerate unsupported Codex tools - #625
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGoModel now omits unsupported Responses hosted tools during Responses-to-Chat conversion. Supported function tools remain available. Tool choices and parallel-call settings are cleared when no supported tools remain. Stream output indexes remain unique for out-of-order tool calls. ChangesResponses compatibility and stream conversion
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ResponsesClient
participant ConvertResponsesRequestToChat
participant ChatProvider
ResponsesClient->>ConvertResponsesRequestToChat: submit tools and tool choice
ConvertResponsesRequestToChat->>ConvertResponsesRequestToChat: retain function tools and omit unsupported tools
ConvertResponsesRequestToChat->>ChatProvider: dispatch normalized Chat request
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![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
🤖 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.
Inline comments:
In `@internal/providers/responses_adapter_test.go`:
- Around line 596-662: Add a table-driven test case alongside
TestConvertResponsesRequestToChat_IgnoresUnsupportedTools covering a namespace
child named spawn_agent that is omitted while exec_command is retained, with
tool_choice selecting spawn_agent and ParallelToolCalls enabled. Assert that
only exec_command remains, ToolChoice is nil, and ParallelToolCalls remains set.
In `@internal/providers/responses_adapter.go`:
- Around line 33-39: The normalized tool-choice handling in
internal/providers/responses_adapter.go lines 33-39 must clear a named function
choice when its name is absent from the normalized function tools, while
preserving valid choices and the existing empty-tools behavior; add a
table-driven mixed namespace-and-function case in
internal/providers/responses_adapter_test.go lines 596-662 verifying an omitted
child choice is cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9f058967-d745-4d3d-9755-dd88e0361759
📒 Files selected for processing (4)
docs/advanced/responses-compatibility.mdxdocs/examples/openai-agents-sdk/anthropic_responses_probe.pyinternal/providers/responses_adapter.gointernal/providers/responses_adapter_test.go
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Confidence Score: 5/5
 T-Rex LogsWhat T-Rex did
Reviews (2): Last reviewed commit: "test(responses): assert streamed output ..." | Re-trigger Greptile |
|
Addressed the reproduced out-of-order stream index collision in da1713e. OpenAI-compatible Responses streams now allocate output indexes monotonically when items are actually emitted, instead of deriving them from upstream tool-call slots. The exact reasoning → tool index 1 → assistant text → tool index 0 sequence now has unique indexes, with a regression test. Full go test ./... and all commit hooks, including race tests and the hot-path performance guard, pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@internal/providers/responses_converter_test.go`:
- Around line 312-359: Update
TestOpenAIResponsesStreamConverter_OutOfOrderToolCallsKeepUniqueIndexes to
record each response.output_item.added output_index in emission order and assert
the complete sequence is [0, 1, 2, 3]. Preserve the existing uniqueness,
output-count, and tool-call ID/index assertions, and incorporate this scenario
into the existing table-driven test structure if available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 659365bb-8bec-4dc6-8d79-79b94f3e2061
📒 Files selected for processing (4)
internal/providers/responses_adapter.gointernal/providers/responses_adapter_test.gointernal/providers/responses_converter.gointernal/providers/responses_converter_test.go
a6c0afe to
c484c32
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/providers/responses_adapter.go (1)
292-355: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the new tool_choice branches.
Two new branches in this segment have no visible test coverage:
- The raw-string handling in
normalizeResponsesToolChoiceForChat(lines 293-300) acceptstool_choice: "auto"/"required"/"none"as a bare string and returnsnilfor any other string. Only the map form ({"type": "auto"}) is tested inTestConvertResponsesRequestToChat_NormalizesToolChoiceAliases.- The fail-open guards in
dropUnavailableResponsesToolChoice(lines 339-346) for a missingfunctionmap or a missing/emptynamereturn the choice unchanged. No test exercises a malformed function-choice shape alongside available tools.Add table-driven cases for a bare-string
ToolChoiceand for a malformed function choice, alongside the existing tool-choice tests.As per coding guidelines for
**/*_test.go, "Add or update table-driven tests for behavior changes, covering request translation, response normalization, error handling, default configuration, and provider-specific parameter mapping."🤖 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/providers/responses_adapter.go` around lines 292 - 355, Extend the existing table-driven tool-choice tests around TestConvertResponsesRequestToChat_NormalizesToolChoiceAliases to cover bare-string values: preserve “auto”, “required”, and “none”, and normalize unsupported strings to nil. Add malformed function-choice cases with available tools to exercise dropUnavailableResponsesToolChoice’s missing function map and missing or empty name guards, verifying each choice is returned unchanged.Source: Coding guidelines
🤖 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.
Inline comments:
In `@docs/advanced/responses-compatibility.mdx`:
- Line 38: Update the Responses-only tools table entry to replace the plain
“computer use” text with the backtick-formatted exact identifier
`computer_use_preview`, preserving the existing table structure and other tool
names.
---
Outside diff comments:
In `@internal/providers/responses_adapter.go`:
- Around line 292-355: Extend the existing table-driven tool-choice tests around
TestConvertResponsesRequestToChat_NormalizesToolChoiceAliases to cover
bare-string values: preserve “auto”, “required”, and “none”, and normalize
unsupported strings to nil. Add malformed function-choice cases with available
tools to exercise dropUnavailableResponsesToolChoice’s missing function map and
missing or empty name guards, verifying each choice is returned unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 73421e56-34f3-4019-bc2f-f9251ce5ecb5
📒 Files selected for processing (6)
docs/advanced/responses-compatibility.mdxdocs/examples/openai-agents-sdk/anthropic_responses_probe.pyinternal/providers/responses_adapter.gointernal/providers/responses_adapter_test.gointernal/providers/responses_converter.gointernal/providers/responses_converter_test.go
|
Addressed the remaining outside-diff test coverage feedback in 47d5ce0. The table-driven tool-choice tests now cover bare auto/required/none values, unsupported bare strings, and malformed function choices with a missing function map, missing name, or empty name. Provider tests, race tests, lint, the performance guard, and Mintlify build validation pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@docs/advanced/responses-compatibility.mdx`:
- Line 38: Align the hosted web-search tool type across the documentation and
probe: update the Anthropic responses probe in
internal/providers/responses_adapter_test.go:569-571 to use the supported
web_search type, and ensure the corresponding documentation entry in
docs/advanced/responses-compatibility.mdx:38 remains consistent; alternatively,
explicitly document both accepted native hosted aliases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 09c8b568-6310-48d5-9c70-b14080a07077
📒 Files selected for processing (2)
docs/advanced/responses-compatibility.mdxinternal/providers/responses_adapter_test.go
Summary
namespaceandweb_searchon chat-translated providerstool_choiceandparallel_tool_callswhen every declared tool is filteredWhy
Codex 0.146.0 advertises ordinary function tools together with namespace groups and web search. DeepSeek and other chat-backed providers cannot execute Responses-only tools, but rejecting the entire request prevents supported Codex function tools from working. Filtering only the unsupported declarations follows the gateway ingress policy without pretending a search or namespaced tool ran.
Verification
go test ./...DEEPSEEK_API_KEYfrom the sibling.envdeepseek/deepseek-v4-pro/v1/responsesrequests, both HTTP 200exec_commandcompleted a real tool round trip and Codex returned the exact expected responseRisk
Unsupported hosted tools are intentionally unavailable on chat-translated providers. Requests continue with supported function tools, but web search and namespace-contained tools do not execute. Native Responses providers retain their existing passthrough behavior. No extra provider call or hot-path pass is added.
Summary by CodeRabbit
Bug Fixes
Documentation