mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Use MCP server instructions in deferred namespace descriptions (#21053)
## Why MCP servers can provide `instructions` that explain what their tools are for. Directly exposed MCP namespaces already use those instructions when a connector description is not available, but deferred `tool_search` results did not preserve that fallback. The direct path falls back from connector metadata to server instructions, while the deferred path only carried `connector_description` and otherwise fell back to generic namespace text. That meant a plain MCP server could provide useful model-facing guidance and still appear as `Tools in the X namespace.` whenever it was discovered lazily through `tool_search`. ## What changed - Store one model-facing `namespace_description` on `ToolInfo`, using connector descriptions for connector-backed tools and server instructions for plain MCP servers. - Thread that namespace description through the `tool_search` source list, search indexing, and returned namespace metadata. - Add an end-to-end regression test for deferred non-app MCP search results exposing server instructions as the namespace description. ## Verification - `cargo test -p codex-tools search_tool_description_lists_each_mcp_source_once --lib` - `cargo test -p codex-core --test all tool_search_uses_non_app_mcp_server_instructions_as_namespace_description`
This commit is contained in:
committed by
GitHub
Unverified
parent
48402be6fa
commit
b9e8df47da
@@ -40,7 +40,7 @@ fn create_test_tool(server_name: &str, tool_name: &str) -> ToolInfo {
|
||||
server_name: server_name.to_string(),
|
||||
callable_name: tool_name.to_string(),
|
||||
callable_namespace: tool_namespace,
|
||||
server_instructions: None,
|
||||
namespace_description: None,
|
||||
tool: Tool {
|
||||
name: tool_name.to_string().into(),
|
||||
title: None,
|
||||
@@ -55,7 +55,6 @@ fn create_test_tool(server_name: &str, tool_name: &str) -> ToolInfo {
|
||||
connector_id: None,
|
||||
connector_name: None,
|
||||
plugin_display_names: Vec::new(),
|
||||
connector_description: None,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -362,16 +362,23 @@ pub(crate) async fn list_tools_for_client_uncached(
|
||||
tool_def.title = Some(normalized_title);
|
||||
}
|
||||
}
|
||||
let has_connector_metadata = connector_id.is_some()
|
||||
|| connector_name.is_some()
|
||||
|| connector_description.is_some();
|
||||
let namespace_description = if has_connector_metadata {
|
||||
connector_description
|
||||
} else {
|
||||
server_instructions.map(str::to_string)
|
||||
};
|
||||
ToolInfo {
|
||||
server_name: server_name.to_owned(),
|
||||
callable_name,
|
||||
callable_namespace,
|
||||
server_instructions: server_instructions.map(str::to_string),
|
||||
namespace_description,
|
||||
tool: tool_def,
|
||||
connector_id,
|
||||
connector_name,
|
||||
plugin_display_names: Vec::new(),
|
||||
connector_description,
|
||||
}
|
||||
})
|
||||
.collect();
|
||||
|
||||
@@ -35,16 +35,16 @@ pub struct ToolInfo {
|
||||
/// Model-visible namespace used for deferred tool loading.
|
||||
#[serde(rename = "tool_namespace", alias = "callable_namespace")]
|
||||
pub callable_namespace: String,
|
||||
/// Instructions from the MCP server initialize result.
|
||||
#[serde(default)]
|
||||
pub server_instructions: Option<String>,
|
||||
/// Model-visible namespace description.
|
||||
// Keep the old serialized field name readable for cached ToolInfo values.
|
||||
#[serde(default, alias = "connector_description")]
|
||||
pub namespace_description: Option<String>,
|
||||
/// Raw MCP tool definition; `tool.name` is sent back to the MCP server.
|
||||
pub tool: Tool,
|
||||
pub connector_id: Option<String>,
|
||||
pub connector_name: Option<String>,
|
||||
#[serde(default)]
|
||||
pub plugin_display_names: Vec<String>,
|
||||
pub connector_description: Option<String>,
|
||||
}
|
||||
|
||||
impl ToolInfo {
|
||||
|
||||
@@ -525,7 +525,7 @@ pub(crate) fn accessible_connectors_from_mcp_tools(
|
||||
Some(codex_connectors::accessible::AccessibleConnectorTool {
|
||||
connector_id: connector_id.to_string(),
|
||||
connector_name: tool.connector_name.clone(),
|
||||
connector_description: tool.connector_description.clone(),
|
||||
connector_description: tool.namespace_description.clone(),
|
||||
plugin_display_names: tool.plugin_display_names.clone(),
|
||||
})
|
||||
});
|
||||
|
||||
@@ -119,11 +119,10 @@ fn codex_app_tool(
|
||||
server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
callable_name: tool_name.to_string(),
|
||||
callable_namespace: tool_namespace,
|
||||
server_instructions: None,
|
||||
namespace_description: None,
|
||||
tool: test_tool_definition(tool_name),
|
||||
connector_id: Some(connector_id.to_string()),
|
||||
connector_name: connector_name.map(ToOwned::to_owned),
|
||||
connector_description: None,
|
||||
plugin_display_names: plugin_names(plugin_display_names),
|
||||
}
|
||||
}
|
||||
@@ -198,11 +197,10 @@ fn accessible_connectors_from_mcp_tools_carries_plugin_display_names() {
|
||||
server_name: "sample".to_string(),
|
||||
callable_name: "echo".to_string(),
|
||||
callable_namespace: "sample".to_string(),
|
||||
server_instructions: None,
|
||||
namespace_description: None,
|
||||
tool: test_tool_definition("echo"),
|
||||
connector_id: None,
|
||||
connector_name: None,
|
||||
connector_description: None,
|
||||
plugin_display_names: plugin_names(&["ignored"]),
|
||||
},
|
||||
),
|
||||
@@ -323,7 +321,7 @@ fn accessible_connectors_from_mcp_tools_preserves_description() {
|
||||
server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
callable_name: "calendar_create_event".to_string(),
|
||||
callable_namespace: "mcp__codex_apps__calendar".to_string(),
|
||||
server_instructions: None,
|
||||
namespace_description: Some("Plan events".to_string()),
|
||||
tool: Tool {
|
||||
name: "calendar_create_event".to_string().into(),
|
||||
title: None,
|
||||
@@ -337,7 +335,6 @@ fn accessible_connectors_from_mcp_tools_preserves_description() {
|
||||
},
|
||||
connector_id: Some("calendar".to_string()),
|
||||
connector_name: Some("Calendar".to_string()),
|
||||
connector_description: Some("Plan events".to_string()),
|
||||
plugin_display_names: Vec::new(),
|
||||
},
|
||||
)]);
|
||||
|
||||
@@ -58,7 +58,7 @@ fn make_mcp_tool(
|
||||
server_name: server_name.to_string(),
|
||||
callable_name: tool_name.to_string(),
|
||||
callable_namespace: tool_namespace,
|
||||
server_instructions: None,
|
||||
namespace_description: None,
|
||||
tool: Tool {
|
||||
name: tool_name.to_string().into(),
|
||||
title: None,
|
||||
@@ -73,7 +73,6 @@ fn make_mcp_tool(
|
||||
connector_id: connector_id.map(str::to_string),
|
||||
connector_name: connector_name.map(str::to_string),
|
||||
plugin_display_names: Vec::new(),
|
||||
connector_description: None,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -392,7 +392,7 @@ mod tests {
|
||||
server_name: server_name.to_string(),
|
||||
callable_name: tool_name.to_string(),
|
||||
callable_namespace: format!("mcp__{server_name}__"),
|
||||
server_instructions: None,
|
||||
namespace_description: None,
|
||||
tool: Tool {
|
||||
name: tool_name.to_string().into(),
|
||||
title: None,
|
||||
@@ -411,7 +411,6 @@ mod tests {
|
||||
connector_id: None,
|
||||
connector_name: None,
|
||||
plugin_display_names: Vec::new(),
|
||||
connector_description: None,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -57,10 +57,7 @@ fn map_mcp_tools_for_plan(mcp_tools: &HashMap<String, ToolInfo>) -> McpToolPlanI
|
||||
tool.callable_namespace.clone(),
|
||||
ToolNamespace {
|
||||
name: tool.callable_namespace.clone(),
|
||||
description: tool
|
||||
.connector_description
|
||||
.clone()
|
||||
.or_else(|| tool.server_instructions.clone()),
|
||||
description: tool.namespace_description.clone(),
|
||||
},
|
||||
)
|
||||
})
|
||||
@@ -118,7 +115,7 @@ pub(crate) fn build_specs_with_discoverable_tools(
|
||||
name: tool.canonical_tool_name(),
|
||||
server_name: tool.server_name.as_str(),
|
||||
connector_name: tool.connector_name.as_deref(),
|
||||
connector_description: tool.connector_description.as_deref(),
|
||||
description: tool.namespace_description.as_deref(),
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
});
|
||||
|
||||
@@ -62,12 +62,11 @@ fn mcp_tool_info(tool: rmcp::model::Tool) -> ToolInfo {
|
||||
server_name: "test_server".to_string(),
|
||||
callable_name: tool.name.to_string(),
|
||||
callable_namespace: "mcp__test_server__".to_string(),
|
||||
server_instructions: None,
|
||||
namespace_description: None,
|
||||
tool,
|
||||
connector_id: None,
|
||||
connector_name: None,
|
||||
plugin_display_names: Vec::new(),
|
||||
connector_description: None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -81,12 +80,11 @@ fn mcp_tool_info_with_display_name(display_name: &str, tool: rmcp::model::Tool)
|
||||
server_name: "test_server".to_string(),
|
||||
callable_name,
|
||||
callable_namespace,
|
||||
server_instructions: None,
|
||||
namespace_description: None,
|
||||
tool,
|
||||
connector_id: None,
|
||||
connector_name: None,
|
||||
plugin_display_names: Vec::new(),
|
||||
connector_description: None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -898,7 +896,7 @@ async fn search_tool_description_falls_back_to_connector_name_without_descriptio
|
||||
server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
callable_name: "_create_event".to_string(),
|
||||
callable_namespace: "mcp__codex_apps__calendar".to_string(),
|
||||
server_instructions: None,
|
||||
namespace_description: None,
|
||||
tool: mcp_tool(
|
||||
"calendar_create_event",
|
||||
"Create calendar event",
|
||||
@@ -907,7 +905,6 @@ async fn search_tool_description_falls_back_to_connector_name_without_descriptio
|
||||
connector_id: Some("calendar".to_string()),
|
||||
connector_name: Some("Calendar".to_string()),
|
||||
plugin_display_names: Vec::new(),
|
||||
connector_description: None,
|
||||
},
|
||||
)])),
|
||||
&[],
|
||||
@@ -950,7 +947,7 @@ async fn search_tool_registers_namespaced_mcp_tool_aliases() {
|
||||
server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
callable_name: "_create_event".to_string(),
|
||||
callable_namespace: "mcp__codex_apps__calendar".to_string(),
|
||||
server_instructions: None,
|
||||
namespace_description: None,
|
||||
tool: mcp_tool(
|
||||
"calendar-create-event",
|
||||
"Create calendar event",
|
||||
@@ -958,7 +955,6 @@ async fn search_tool_registers_namespaced_mcp_tool_aliases() {
|
||||
),
|
||||
connector_id: Some("calendar".to_string()),
|
||||
connector_name: Some("Calendar".to_string()),
|
||||
connector_description: None,
|
||||
plugin_display_names: Vec::new(),
|
||||
},
|
||||
),
|
||||
@@ -968,7 +964,7 @@ async fn search_tool_registers_namespaced_mcp_tool_aliases() {
|
||||
server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
callable_name: "_list_events".to_string(),
|
||||
callable_namespace: "mcp__codex_apps__calendar".to_string(),
|
||||
server_instructions: None,
|
||||
namespace_description: None,
|
||||
tool: mcp_tool(
|
||||
"calendar-list-events",
|
||||
"List calendar events",
|
||||
@@ -976,7 +972,6 @@ async fn search_tool_registers_namespaced_mcp_tool_aliases() {
|
||||
),
|
||||
connector_id: Some("calendar".to_string()),
|
||||
connector_name: Some("Calendar".to_string()),
|
||||
connector_description: None,
|
||||
plugin_display_names: Vec::new(),
|
||||
},
|
||||
),
|
||||
@@ -986,11 +981,10 @@ async fn search_tool_registers_namespaced_mcp_tool_aliases() {
|
||||
server_name: "rmcp".to_string(),
|
||||
callable_name: "echo".to_string(),
|
||||
callable_namespace: "mcp__rmcp__".to_string(),
|
||||
server_instructions: None,
|
||||
namespace_description: None,
|
||||
tool: mcp_tool("echo", "Echo", serde_json::json!({"type": "object"})),
|
||||
connector_id: None,
|
||||
connector_name: None,
|
||||
connector_description: None,
|
||||
plugin_display_names: Vec::new(),
|
||||
},
|
||||
),
|
||||
|
||||
@@ -80,7 +80,7 @@ fn mcp_tool_search_entry(info: &ToolInfo) -> Result<ToolSearchEntry, serde_json:
|
||||
tool_name: info.callable_name.as_str(),
|
||||
tool: &info.tool,
|
||||
connector_name: info.connector_name.as_deref(),
|
||||
connector_description: info.connector_description.as_deref(),
|
||||
description: info.namespace_description.as_deref(),
|
||||
})?,
|
||||
limit_bucket: Some(info.server_name.clone()),
|
||||
})
|
||||
@@ -120,10 +120,10 @@ fn build_mcp_search_text(info: &ToolInfo) -> String {
|
||||
parts.push(connector_name.to_string());
|
||||
}
|
||||
|
||||
if let Some(connector_description) = info.connector_description.as_deref()
|
||||
&& !connector_description.trim().is_empty()
|
||||
if let Some(description) = info.namespace_description.as_deref()
|
||||
&& !description.trim().is_empty()
|
||||
{
|
||||
parts.push(connector_description.to_string());
|
||||
parts.push(description.to_string());
|
||||
}
|
||||
|
||||
parts.extend(
|
||||
|
||||
@@ -1029,3 +1029,92 @@ async fn tool_search_indexes_only_enabled_non_app_mcp_tools() -> Result<()> {
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn tool_search_uses_non_app_mcp_server_instructions_as_namespace_description() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let apps_server = AppsTestServer::mount_searchable(&server).await?;
|
||||
let search_call_id = "tool-search-echo";
|
||||
let mock = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_tool_search_call(
|
||||
search_call_id,
|
||||
&json!({
|
||||
"query": "Echo back the provided message and include environment data.",
|
||||
"limit": 8,
|
||||
}),
|
||||
),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-2"),
|
||||
ev_assistant_message("msg-1", "done"),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
let rmcp_test_server_bin = stdio_server_bin()?;
|
||||
let mut builder =
|
||||
configured_builder(apps_server.chatgpt_base_url.clone()).with_config(move |config| {
|
||||
let mut servers = config.mcp_servers.get().clone();
|
||||
servers.insert(
|
||||
"rmcp".to_string(),
|
||||
McpServerConfig {
|
||||
transport: McpServerTransportConfig::Stdio {
|
||||
command: rmcp_test_server_bin,
|
||||
args: Vec::new(),
|
||||
env: None,
|
||||
env_vars: Vec::new(),
|
||||
cwd: None,
|
||||
},
|
||||
experimental_environment: None,
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(Duration::from_secs(10)),
|
||||
tool_timeout_sec: None,
|
||||
default_tools_approval_mode: None,
|
||||
enabled_tools: Some(vec!["echo".to_string()]),
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
supports_parallel_tool_calls: false,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
config
|
||||
.mcp_servers
|
||||
.set(servers)
|
||||
.expect("test mcp servers should accept any configuration");
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
test.submit_turn_with_approval_and_permission_profile(
|
||||
"Find the rmcp echo tool.",
|
||||
AskForApproval::Never,
|
||||
PermissionProfile::Disabled,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let requests = mock.requests();
|
||||
assert_eq!(requests.len(), 2);
|
||||
|
||||
let tools = tool_search_output_tools(&requests[1], search_call_id);
|
||||
let rmcp_namespace = tools
|
||||
.iter()
|
||||
.find(|tool| tool.get("name").and_then(Value::as_str) == Some("mcp__rmcp__"))
|
||||
.expect("tool_search should return the rmcp namespace");
|
||||
assert_eq!(
|
||||
rmcp_namespace.get("description").and_then(Value::as_str),
|
||||
Some("Use these tools to exercise the rmcp test server.")
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -401,6 +401,7 @@ impl ServerHandler for TestToolServer {
|
||||
)]));
|
||||
|
||||
ServerInfo {
|
||||
instructions: Some("Use these tools to exercise the rmcp test server.".to_string()),
|
||||
capabilities,
|
||||
..ServerInfo::default()
|
||||
}
|
||||
|
||||
@@ -27,7 +27,7 @@ pub struct ToolSearchSourceInfo {
|
||||
pub struct ToolSearchSource<'a> {
|
||||
pub server_name: &'a str,
|
||||
pub connector_name: Option<&'a str>,
|
||||
pub connector_description: Option<&'a str>,
|
||||
pub description: Option<&'a str>,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, PartialEq)]
|
||||
@@ -37,7 +37,7 @@ pub struct ToolSearchResultSource<'a> {
|
||||
pub tool_name: &'a str,
|
||||
pub tool: &'a rmcp::model::Tool,
|
||||
pub connector_name: Option<&'a str>,
|
||||
pub connector_description: Option<&'a str>,
|
||||
pub description: Option<&'a str>,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, Deserialize, Serialize, PartialEq, Eq)]
|
||||
@@ -215,7 +215,7 @@ pub fn tool_search_result_source_to_loadable_tool_spec(
|
||||
|
||||
fn tool_search_result_source_namespace_description(source: ToolSearchResultSource<'_>) -> String {
|
||||
source
|
||||
.connector_description
|
||||
.description
|
||||
.map(str::trim)
|
||||
.filter(|description| !description.is_empty())
|
||||
.map(str::to_string)
|
||||
@@ -251,7 +251,7 @@ pub fn collect_tool_search_source_infos<'a>(
|
||||
return Some(ToolSearchSourceInfo {
|
||||
name: name.to_string(),
|
||||
description: tool
|
||||
.connector_description
|
||||
.description
|
||||
.map(str::trim)
|
||||
.filter(|description| !description.is_empty())
|
||||
.map(str::to_string),
|
||||
@@ -265,7 +265,11 @@ pub fn collect_tool_search_source_infos<'a>(
|
||||
|
||||
Some(ToolSearchSourceInfo {
|
||||
name: name.to_string(),
|
||||
description: None,
|
||||
description: tool
|
||||
.description
|
||||
.map(str::trim)
|
||||
.filter(|description| !description.is_empty())
|
||||
.map(str::to_string),
|
||||
})
|
||||
})
|
||||
.collect()
|
||||
|
||||
@@ -282,7 +282,7 @@ pub fn build_tool_registry_plan(
|
||||
ToolSearchSource {
|
||||
server_name: tool.server_name,
|
||||
connector_name: tool.connector_name,
|
||||
connector_description: tool.connector_description,
|
||||
description: tool.description,
|
||||
}
|
||||
}))
|
||||
})
|
||||
|
||||
@@ -1414,7 +1414,7 @@ fn search_tool_description_lists_each_mcp_source_once() {
|
||||
"mcp__rmcp__",
|
||||
"rmcp",
|
||||
/*connector_name*/ None,
|
||||
/*connector_description*/ None,
|
||||
Some("Remote memory tools."),
|
||||
),
|
||||
]),
|
||||
&[],
|
||||
@@ -1433,7 +1433,7 @@ fn search_tool_description_lists_each_mcp_source_once() {
|
||||
.count(),
|
||||
1
|
||||
);
|
||||
assert!(description.contains("- rmcp"));
|
||||
assert!(description.contains("- rmcp: Remote memory tools."));
|
||||
assert!(!description.contains("mcp__rmcp__echo"));
|
||||
|
||||
assert!(handlers.contains(&ToolHandlerSpec {
|
||||
@@ -1454,7 +1454,7 @@ fn search_tool_requires_model_capability_and_enabled_feature() {
|
||||
"mcp__codex_apps__calendar",
|
||||
CODEX_APPS_MCP_SERVER_NAME,
|
||||
Some("Calendar"),
|
||||
/*connector_description*/ None,
|
||||
/*description*/ None,
|
||||
)]);
|
||||
|
||||
let features = Features::with_defaults();
|
||||
@@ -2355,13 +2355,13 @@ fn deferred_mcp_tool<'a>(
|
||||
tool_namespace: &'a str,
|
||||
server_name: &'a str,
|
||||
connector_name: Option<&'a str>,
|
||||
connector_description: Option<&'a str>,
|
||||
description: Option<&'a str>,
|
||||
) -> ToolRegistryPlanDeferredTool<'a> {
|
||||
ToolRegistryPlanDeferredTool {
|
||||
name: ToolName::namespaced(tool_namespace, tool_name),
|
||||
server_name,
|
||||
connector_name,
|
||||
connector_description,
|
||||
description,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -85,7 +85,7 @@ pub struct ToolRegistryPlanDeferredTool<'a> {
|
||||
pub name: ToolName,
|
||||
pub server_name: &'a str,
|
||||
pub connector_name: Option<&'a str>,
|
||||
pub connector_description: Option<&'a str>,
|
||||
pub description: Option<&'a str>,
|
||||
}
|
||||
|
||||
impl ToolRegistryPlan {
|
||||
|
||||
Reference in New Issue
Block a user