-
Notifications
You must be signed in to change notification settings - Fork 177
fix(mcp): preserve explicit tool_choice for the initial responses request #1963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ use crate::{ | |
| }, | ||
| }, | ||
| error, | ||
| openai::responses::utils, | ||
| }, | ||
| }; | ||
|
|
||
|
|
@@ -348,8 +349,42 @@ pub(crate) fn prepare_mcp_tools_as_functions(payload: &mut Value, session: &McpT | |
| tools_json.extend(session_tools); | ||
|
|
||
| if !tools_json.is_empty() { | ||
| let tool_choice = remap_tool_choice_for_functions(obj.get("tool_choice"), &tools_json); | ||
| obj.insert("tools".to_string(), Value::Array(tools_json)); | ||
| obj.insert("tool_choice".to_string(), Value::String("auto".to_string())); | ||
| obj.insert("tool_choice".to_string(), tool_choice); | ||
| } | ||
| } | ||
|
|
||
| /// Remap an explicit `tool_choice` through the MCP/hosted→function rewrite for | ||
| /// the initial request; resume payloads switch to "auto" so the loop can end. | ||
| /// | ||
| /// String options pass through. Object choices resolve by name against the | ||
| /// rewritten function tools (hosted types like `image_generation` match the | ||
| /// function of the same name); anything unmappable falls back to "auto", since | ||
| /// the referenced tool no longer exists in `tools`. | ||
| fn remap_tool_choice_for_functions(tool_choice: Option<&Value>, tools: &[Value]) -> Value { | ||
| let selected = match tool_choice { | ||
| Some(Value::String(choice)) => return Value::String(choice.clone()), | ||
| Some(choice) if choice.is_object() => { | ||
| choice.get("name").and_then(Value::as_str).or_else(|| { | ||
| choice | ||
| .get("type") | ||
| .and_then(Value::as_str) | ||
| .filter(|choice_type| *choice_type != ItemType::FUNCTION) | ||
| }) | ||
| } | ||
| _ => None, | ||
| }; | ||
|
|
||
| match selected { | ||
| Some(name) | ||
| if tools | ||
| .iter() | ||
| .any(|tool| utils::function_tool_name(tool) == Some(name)) => | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fresh evidence after the earlier comment is that the new remapper only compares the hosted Useful? React with 👍 / 👎. |
||
| { | ||
| json!({ "type": ItemType::FUNCTION, "name": name }) | ||
| } | ||
| _ => Value::String("auto".to_string()), | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -393,6 +428,9 @@ pub(crate) fn build_resume_payload( | |
| if let Some(tools_arr) = tools_json.as_array() { | ||
| if !tools_arr.is_empty() { | ||
| obj.insert("tools".to_string(), tools_json.clone()); | ||
| // The tool call already happened; "auto" lets the model answer | ||
| // instead of looping a preserved "required" until the call limit. | ||
| obj.insert("tool_choice".to_string(), Value::String("auto".to_string())); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1899,4 +1937,131 @@ mod tests { | |
| "mcpr_raw_id" | ||
| ); | ||
| } | ||
|
|
||
| async fn function_tool_session() -> (McpOrchestrator, Vec<McpServerBinding>) { | ||
| let orchestrator = McpOrchestrator::new(McpConfig { | ||
| servers: vec![McpServerConfig { | ||
| name: "wiki-server".to_string(), | ||
| transport: McpTransport::Sse { | ||
| url: "http://localhost:3000/sse".to_string(), | ||
| token: None, | ||
| headers: Default::default(), | ||
| }, | ||
| proxy: None, | ||
| required: false, | ||
| tools: None, | ||
| builtin_type: None, | ||
| builtin_tool_name: None, | ||
| internal: false, | ||
| }], | ||
| ..Default::default() | ||
| }) | ||
| .await | ||
| .expect("orchestrator"); | ||
| orchestrator | ||
| .tool_inventory() | ||
| .insert_entry(ToolEntry::from_server_tool( | ||
| "wiki-server", | ||
| test_tool("ask_wiki"), | ||
| )); | ||
| let bindings = vec![McpServerBinding { | ||
| label: "wiki".to_string(), | ||
| server_key: "wiki-server".to_string(), | ||
| allowed_tools: None, | ||
| }]; | ||
| (orchestrator, bindings) | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn prepare_preserves_explicit_tool_choice() { | ||
| let (orchestrator, bindings) = function_tool_session().await; | ||
| let session = McpToolSession::new(&orchestrator, bindings, "test-request"); | ||
|
|
||
| let mut payload = json!({ | ||
| "model": "m", | ||
| "input": "q", | ||
| "tools": [{"type": "mcp", "server_label": "wiki"}], | ||
| "tool_choice": "required" | ||
| }); | ||
| super::prepare_mcp_tools_as_functions(&mut payload, &session); | ||
|
|
||
| assert_eq!(payload["tool_choice"], json!("required")); | ||
| assert_eq!(payload["tools"][0]["type"], json!("function")); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn prepare_remaps_hosted_tool_choice_to_function() { | ||
| let (orchestrator, bindings) = function_tool_session().await; | ||
| orchestrator | ||
| .tool_inventory() | ||
| .insert_entry(ToolEntry::from_server_tool( | ||
| "wiki-server", | ||
| test_tool("image_generation"), | ||
| )); | ||
| let session = McpToolSession::new(&orchestrator, bindings, "test-request"); | ||
|
|
||
| let mut payload = json!({ | ||
| "model": "m", | ||
| "input": "q", | ||
| "tools": [{"type": "image_generation"}], | ||
| "tool_choice": {"type": "image_generation"} | ||
| }); | ||
| super::prepare_mcp_tools_as_functions(&mut payload, &session); | ||
|
|
||
| assert_eq!( | ||
| payload["tool_choice"], | ||
| json!({"type": "function", "name": "image_generation"}) | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn prepare_downgrades_unmappable_tool_choice_to_auto() { | ||
| let (orchestrator, bindings) = function_tool_session().await; | ||
| let session = McpToolSession::new(&orchestrator, bindings, "test-request"); | ||
|
|
||
| let mut payload = json!({ | ||
| "model": "m", | ||
| "input": "q", | ||
| "tools": [{"type": "mcp", "server_label": "wiki"}], | ||
| "tool_choice": {"type": "web_search_preview"} | ||
| }); | ||
| super::prepare_mcp_tools_as_functions(&mut payload, &session); | ||
|
|
||
| assert_eq!(payload["tool_choice"], json!("auto")); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn prepare_defaults_missing_tool_choice_to_auto() { | ||
| let (orchestrator, bindings) = function_tool_session().await; | ||
| let session = McpToolSession::new(&orchestrator, bindings, "test-request"); | ||
|
|
||
| let mut payload = json!({ | ||
| "model": "m", | ||
| "input": "q", | ||
| "tools": [{"type": "mcp", "server_label": "wiki"}] | ||
| }); | ||
| super::prepare_mcp_tools_as_functions(&mut payload, &session); | ||
|
|
||
| assert_eq!(payload["tool_choice"], json!("auto")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn resume_payload_forces_auto_tool_choice() { | ||
| let base = json!({ | ||
| "model": "m", | ||
| "input": "q", | ||
| "tools": [{"type": "function", "name": "ask_wiki"}], | ||
| "tool_choice": "required" | ||
| }); | ||
| let resumed = super::build_resume_payload( | ||
| &base, | ||
| &[], | ||
| &ResponseInput::Text("q".to_string()), | ||
| &json!([{"type": "function", "name": "ask_wiki"}]), | ||
| false, | ||
| ) | ||
| .expect("resume payload"); | ||
|
|
||
| assert_eq!(resumed["tool_choice"], json!("auto")); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a request mixes a caller-owned
functiontool with any MCP-routed tool and setstool_choiceto that function, this check now preserves the forced function choice because the retained user function is present intools_json. The non-streaming tool loop later extracts everyfunction_calland executes it viasession.execute_tool_result, which only contains MCP-exposed tools, so a forced user function likeget_weatheris converted into an MCP execution error instead of being returned to the client. Either pass user-function calls through or avoid preserving forced choices for functions that are not MCP-exposed.Useful? React with 👍 / 👎.