mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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>
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.
<img width="1593" height="488" alt="Screenshot 2026-04-15 at 10 55 55
AM"
src="https://github.com/user-attachments/assets/bab668ba-bf17-4c71-be4e-b102202fce57"
/>
---------
Co-authored-by: Sayan Sisodiya <sayan@openai.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
aca781b3a7
commit
78ce61c78e
@@ -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;
|
||||
|
||||
@@ -55,6 +55,10 @@ pub struct ResponsesApiNamespace {
|
||||
pub tools: Vec<ResponsesApiNamespaceTool>,
|
||||
}
|
||||
|
||||
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 {
|
||||
|
||||
@@ -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,
|
||||
}));
|
||||
}
|
||||
|
||||
@@ -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");
|
||||
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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();
|
||||
|
||||
Reference in New Issue
Block a user