fix(relayconvert): improve Responses→Chat tool compatibility - #5939
fix(relayconvert): improve Responses→Chat tool compatibility#5939orangeboyChen wants to merge 4 commits into
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:
WalkthroughThe PR updates Responses-to-Chat conversion to flatten MCP and namespace tools into Chat function tools, proxy tool search, normalize inner parameters, and translate related tool choices. Unit tests cover the new conversion behavior. ChangesTool conversion flattening
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Sequence Diagram(s)sequenceDiagram
participant ResponsesRequest
participant responsesRequestToolsToChat
participant ChatToolList
ResponsesRequest->>responsesRequestToolsToChat: provide MCP, namespace, function, or tool_search tools
responsesRequestToolsToChat->>ChatToolList: emit flattened function tools
responsesRequestToolsToChat->>ChatToolList: emit tool_search proxy function
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
@codex review |
…at conversion Responses→Chat conversion (responsesRequestToolsToChat) only handled type:"function" tools. Tools with type:"mcp_server" or type:"namespace" were dumped into a Custom field that the upstream Chat Completions API does not understand, causing the model to lose all tool definitions inside those blocks. Fix: - Flatten mcp_server/namespace inner tools[] into individual type:"function" entries (name, description, parameters preserved) - Skip tool_search entirely (no Chat Completions equivalent) - defer_loading is implicitly stripped since it is not copied into the FunctionRequest output Add tests covering mcp_server flattening, namespace flattening, tool_search skipping, mixed tool types, and nil parameters fallback. Closes QuantumNous#5938
3c92426 to
f562b77
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
service/relayconvert/responses_request_to_chat.go (2)
361-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnreachable
[]map[string]anybranch.
raworiginates fromtool["tools"], itself decoded viacommon.Unmarshal(raw, &tools)intomap[string]any. JSON-unmarshaling a nested array into anany-typed field always yields[]interface{}, never[]map[string]any, so the secondswitchcase can never be hit in the real request path (only test code constructing native Go values without going through JSON could trigger it, and the tests here all go throughmustRawMessage). Not harmful, but dead code.🤖 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 `@service/relayconvert/responses_request_to_chat.go` around lines 361 - 374, The responsesFlattenInnerTools type switch has a dead branch for []map[string]any that is never reached in the real request path because tool["tools"] comes from JSON unmarshaling into any and will be []any. Remove the unreachable case from responsesFlattenInnerTools and keep only the []any handling, using the existing function name to verify the cleanup.
364-369: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMalformed inner tool entries are silently dropped.
If an item in the inner
toolsarray isn't amap[string]any(e.g., malformed client payload), it's silently skipped with no error or log, which can make missing-tool issues hard to diagnose downstream.♻️ Optional: surface a warning for skipped entries
case []any: for _, item := range v { if m, ok := item.(map[string]any); ok { inner = append(inner, m) + } else { + common.SysLog(fmt.Sprintf("responsesFlattenInnerTools: skipping malformed inner tool entry: %v", item)) } }🤖 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 `@service/relayconvert/responses_request_to_chat.go` around lines 364 - 369, The inner tools parsing in responses_request_to_chat.go currently drops non-map entries from the []any case without any visibility. Update the tool-collection logic in the relevant conversion function to surface a warning or error when an item in the inner tools array is not a map[string]any, so malformed client payloads are detectable; keep the existing valid-path behavior for maps and log enough context to identify the skipped entry.
🤖 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 `@service/relayconvert/responses_request_to_chat.go`:
- Around line 361-374: The responsesFlattenInnerTools type switch has a dead
branch for []map[string]any that is never reached in the real request path
because tool["tools"] comes from JSON unmarshaling into any and will be []any.
Remove the unreachable case from responsesFlattenInnerTools and keep only the
[]any handling, using the existing function name to verify the cleanup.
- Around line 364-369: The inner tools parsing in responses_request_to_chat.go
currently drops non-map entries from the []any case without any visibility.
Update the tool-collection logic in the relevant conversion function to surface
a warning or error when an item in the inner tools array is not a
map[string]any, so malformed client payloads are detectable; keep the existing
valid-path behavior for maps and log enough context to identify the skipped
entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0168a688-59a4-450d-9f72-8e22c3a4f184
📒 Files selected for processing (2)
service/relayconvert/responses_request_to_chat.goservice/relayconvert/responses_request_to_chat_test.go
|
@codex review |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
service/relayconvert/responses_request_to_chat_test.go (2)
348-348: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicate assertion.
Line 348 re-asserts
got.Tools[0].Type == "function", which is already checked on line 339.♻️ Proposed fix
assert.Equal(t, "string", query["type"]) - assert.Equal(t, "function", got.Tools[0].Type) assert.Equal(t, "lookup", got.Tools[1].Function.Name)🤖 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 `@service/relayconvert/responses_request_to_chat_test.go` at line 348, Remove the duplicate got.Tools[0].Type assertion from the relevant test, keeping the earlier assertion near line 339 as the sole validation of the tool type.
431-449: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider expanding tool-choice test coverage.
The implementation in
responsesRequestToolChoiceToChathandles three choice types (mcp,mcp_server,namespace) and two namespace source fields (server_label,namespace). This test only coverstype: "mcp"withserver_label. Adding cases formcp_server/namespacetypes and thenamespacefield path would guard against regressions in those branches.🤖 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 `@service/relayconvert/responses_request_to_chat_test.go` around lines 431 - 449, Expand TestResponsesRequestToChatCompletionsRequestMcpToolChoiceUsesFlattenedName with table-driven cases covering mcp_server and namespace tool-choice types, including the namespace source field as well as server_label, and assert each produces the expected flattened function name. Ensure all cases continue validating successful conversion and the resulting Chat Completions tool choice.
🤖 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 `@service/relayconvert/responses_request_to_chat_test.go`:
- Line 348: Remove the duplicate got.Tools[0].Type assertion from the relevant
test, keeping the earlier assertion near line 339 as the sole validation of the
tool type.
- Around line 431-449: Expand
TestResponsesRequestToChatCompletionsRequestMcpToolChoiceUsesFlattenedName with
table-driven cases covering mcp_server and namespace tool-choice types,
including the namespace source field as well as server_label, and assert each
produces the expected flattened function name. Ensure all cases continue
validating successful conversion and the resulting Chat Completions tool choice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4ca404c-9006-4f22-850f-ee4337179a39
📒 Files selected for processing (2)
service/relayconvert/responses_request_to_chat.goservice/relayconvert/responses_request_to_chat_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- service/relayconvert/responses_request_to_chat.go
orangeboyChen
changed the title
…tool_search proxy + tool_choice namespace QuantumNous#6593 剩余方向: - OpenAI→Claude 请求: thinking 块 replay (非最新 assistant 轮) - OpenAI→Responses 请求: reasoning_content → reasoning input item - OpenAI→Gemini 请求: reasoning_content → thought part QuantumNous#5939 补充: - tool_search → function 代理 (Codex MCP 兼容) - tool_choice: server_label/namespace → flattened name - tool_choice: mcp/mcp_server/namespace → function choice
51fdfc5 to
2b6f1df
Compare
2 participants
📝 变更描述 / Description
解决Response->Chat转换后mcp tool会丢失的问题。
在Response->Chat转换时，会转换mcp工具向上传递，并且添加tool_search代理。
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。