From 78ce61c78eaf1eb596b5b78031a47443a4706e44 Mon Sep 17 00:00:00 2001 From: Shijie Rao Date: Wed, 15 Apr 2026 14:14:43 -0400 Subject: [PATCH] Fix empty tool descriptions (#17946) ## Summary - Ensure direct namespaced MCP tool groups are emitted with a non-empty namespace description even when namespace metadata is missing or blank. - Add regression coverage for missing MCP namespace descriptions. ## Cause Latest `main` can serialize a direct namespaced MCP tool group with an empty top-level `description`. The namespace description path used `unwrap_or_default()` when `tool_namespaces` did not include metadata for that namespace, so the outbound Responses API payload could contain a tool like `{"type":"namespace","description":""}`. The Responses API rejects that because namespace tool descriptions must be a non-empty string. ## Fix - Add a fallback namespace description: `Tools in the namespace.` - Preserve provided namespace descriptions after trimming, but treat blank descriptions as missing. ### Issue I am seeing This is what I am seeing on the local build. Screenshot 2026-04-15 at 10 55 55 AM --------- Co-authored-by: Sayan Sisodiya --- codex-rs/tools/src/lib.rs | 1 + codex-rs/tools/src/responses_api.rs | 4 ++ codex-rs/tools/src/tool_discovery.rs | 12 +++--- codex-rs/tools/src/tool_discovery_tests.rs | 6 +-- codex-rs/tools/src/tool_registry_plan.rs | 18 +++++++-- .../tools/src/tool_registry_plan_tests.rs | 40 +++++++++++++++++++ 6 files changed, 67 insertions(+), 14 deletions(-) diff --git a/codex-rs/tools/src/lib.rs b/codex-rs/tools/src/lib.rs index b30e0bffc..4a3ccc01a 100644 --- a/codex-rs/tools/src/lib.rs +++ b/codex-rs/tools/src/lib.rs @@ -85,6 +85,7 @@ pub use responses_api::ResponsesApiNamespace; pub use responses_api::ResponsesApiNamespaceTool; pub use responses_api::ResponsesApiTool; pub use responses_api::ToolSearchOutputTool; +pub(crate) use responses_api::default_namespace_description; pub use responses_api::dynamic_tool_to_responses_api_tool; pub use responses_api::mcp_tool_to_deferred_responses_api_tool; pub use responses_api::mcp_tool_to_responses_api_tool; diff --git a/codex-rs/tools/src/responses_api.rs b/codex-rs/tools/src/responses_api.rs index 205c07510..5317a082f 100644 --- a/codex-rs/tools/src/responses_api.rs +++ b/codex-rs/tools/src/responses_api.rs @@ -55,6 +55,10 @@ pub struct ResponsesApiNamespace { pub tools: Vec, } +pub(crate) fn default_namespace_description(namespace_name: &str) -> String { + format!("Tools in the {namespace_name} namespace.") +} + #[derive(Debug, Clone, Serialize, PartialEq)] #[serde(tag = "type")] pub enum ResponsesApiNamespaceTool { diff --git a/codex-rs/tools/src/tool_discovery.rs b/codex-rs/tools/src/tool_discovery.rs index e35da5735..b773623ed 100644 --- a/codex-rs/tools/src/tool_discovery.rs +++ b/codex-rs/tools/src/tool_discovery.rs @@ -5,6 +5,7 @@ use crate::ResponsesApiTool; use crate::ToolName; use crate::ToolSearchOutputTool; use crate::ToolSpec; +use crate::default_namespace_description; use crate::mcp_tool_to_deferred_responses_api_tool; use codex_app_server_protocol::AppInfo; use serde::Deserialize; @@ -225,6 +226,8 @@ pub fn collect_tool_search_output_tools<'a>( let description = first_tool .connector_description + .map(str::trim) + .filter(|description| !description.is_empty()) .map(str::to_string) .or_else(|| { first_tool @@ -232,12 +235,6 @@ pub fn collect_tool_search_output_tools<'a>( .map(str::trim) .filter(|connector_name| !connector_name.is_empty()) .map(|connector_name| format!("Tools for working with {connector_name}.")) - }) - .or_else(|| { - Some(format!( - "Tools from the {} MCP server.", - first_tool.server_name - )) }); let tools = tools @@ -251,7 +248,8 @@ pub fn collect_tool_search_output_tools<'a>( results.push(ToolSearchOutputTool::Namespace(ResponsesApiNamespace { name: tool_namespace.to_string(), - description: description.unwrap_or_default(), + description: description + .unwrap_or_else(|| default_namespace_description(tool_namespace)), tools, })); } diff --git a/codex-rs/tools/src/tool_discovery_tests.rs b/codex-rs/tools/src/tool_discovery_tests.rs index 10e108fe0..b745f756c 100644 --- a/codex-rs/tools/src/tool_discovery_tests.rs +++ b/codex-rs/tools/src/tool_discovery_tests.rs @@ -253,7 +253,7 @@ fn collect_tool_search_output_tools_preserves_search_order_while_grouping_by_nam }), ToolSearchOutputTool::Namespace(ResponsesApiNamespace { name: "mcp__docs__".to_string(), - description: "Tools from the docs MCP server.".to_string(), + description: "Tools in the mcp__docs__ namespace.".to_string(), tools: vec![ResponsesApiNamespaceTool::Function(ResponsesApiTool { name: "search".to_string(), description: "Search docs.".to_string(), @@ -272,7 +272,7 @@ fn collect_tool_search_output_tools_preserves_search_order_while_grouping_by_nam } #[test] -fn collect_tool_search_output_tools_falls_back_to_connector_name_description() { +fn collect_tool_search_output_tools_ignores_blank_connector_description() { let gmail_batch_read_email = mcp_tool("gmail-batch-read-email", "Read multiple emails."); let tools = collect_tool_search_output_tools([ToolSearchResultSource { @@ -281,7 +281,7 @@ fn collect_tool_search_output_tools_falls_back_to_connector_name_description() { tool_name: "_batch_read_email", tool: &gmail_batch_read_email, connector_name: Some("Gmail"), - connector_description: None, + connector_description: Some(" "), }]) .expect("collect tool search output tools"); diff --git a/codex-rs/tools/src/tool_registry_plan.rs b/codex-rs/tools/src/tool_registry_plan.rs index cec938962..b5da9afb2 100644 --- a/codex-rs/tools/src/tool_registry_plan.rs +++ b/codex-rs/tools/src/tool_registry_plan.rs @@ -55,6 +55,7 @@ use crate::create_wait_agent_tool_v2; use crate::create_wait_tool; use crate::create_web_search_tool; use crate::create_write_stdin_tool; +use crate::default_namespace_description; use crate::dynamic_tool_to_responses_api_tool; use crate::mcp_tool_to_responses_api_tool; use crate::request_permissions_tool_description; @@ -485,11 +486,20 @@ pub fn build_tool_registry_plan( for (namespace, mut entries) in namespace_entries { entries.sort_by_key(|tool| tool.name.name.clone()); - let description = params + let tool_namespace = params .tool_namespaces - .and_then(|namespaces| namespaces.get(&namespace)) - .and_then(|namespace| namespace.description.clone()) - .unwrap_or_default(); + .and_then(|namespaces| namespaces.get(&namespace)); + let description = tool_namespace + .and_then(|namespace| namespace.description.as_deref()) + .map(str::trim) + .filter(|description| !description.is_empty()) + .map(str::to_string) + .unwrap_or_else(|| { + let namespace_name = tool_namespace + .map(|namespace| namespace.name.as_str()) + .unwrap_or(namespace.as_str()); + default_namespace_description(namespace_name) + }); let mut tools = Vec::new(); for tool in entries { match mcp_tool_to_responses_api_tool(&tool.name, tool.tool) { diff --git a/codex-rs/tools/src/tool_registry_plan_tests.rs b/codex-rs/tools/src/tool_registry_plan_tests.rs index 7b9e8ed9a..a643543ee 100644 --- a/codex-rs/tools/src/tool_registry_plan_tests.rs +++ b/codex-rs/tools/src/tool_registry_plan_tests.rs @@ -1151,6 +1151,46 @@ fn test_build_specs_mcp_tools_converted() { ); } +#[test] +fn test_build_specs_mcp_namespace_description_falls_back_when_missing() { + let model_info = model_info(); + let mut features = Features::with_defaults(); + features.enable(Feature::UnifiedExec); + let available_models = Vec::new(); + let tools_config = ToolsConfig::new(&ToolsConfigParams { + model_info: &model_info, + available_models: &available_models, + features: &features, + image_generation_tool_auth_allowed: true, + web_search_mode: Some(WebSearchMode::Cached), + session_source: SessionSource::Cli, + sandbox_policy: &SandboxPolicy::DangerFullAccess, + windows_sandbox_level: WindowsSandboxLevel::Disabled, + }); + let (tools, _) = build_specs( + &tools_config, + Some(HashMap::from([( + ToolName::namespaced("test_server/", "do_something_cool"), + mcp_tool( + "do_something_cool", + "Do something cool", + serde_json::json!({"type": "object"}), + ), + )])), + /*deferred_mcp_tools*/ None, + &[], + ); + + let namespace_tool = find_tool(&tools, "test_server/"); + let ToolSpec::Namespace(namespace) = &namespace_tool.spec else { + panic!("expected namespace tool"); + }; + assert_eq!( + namespace.description, + "Tools in the test_server/ namespace." + ); +} + #[test] fn test_build_specs_mcp_tools_sorted_by_name() { let model_info = model_info();