From ddf33ea802f787f800e2a3691cc6e7379e6bb695 Mon Sep 17 00:00:00 2001 From: jif Date: Wed, 29 Jul 2026 11:44:59 +0000 Subject: [PATCH] Bound MCP namespace descriptions (#35941) ## What changed - Limit model-facing MCP namespace descriptions to 1,000 bytes without splitting UTF-8 characters or mutating the stored server metadata. - Cap the aggregate source-description list in `tool_search` at 4 KiB while retaining source names and discovery instructions. - Cover direct and deferred MCP tool exposure, including multibyte descriptions and tool visibility. GitOrigin-RevId: 92736ec39475a1d6b3b03f5e3063e28225d8a5f6 --- codex-rs/core/src/tools/handlers/mcp.rs | 9 +- .../src/tools/handlers/mcp_search_tests.rs | 26 ++++ .../src/tools/handlers/tool_search_spec.rs | 88 +++++++++++-- codex-rs/core/tests/suite/rmcp_client.rs | 120 +++++++++++++++++- .../rmcp-client/src/bin/test_stdio_server.rs | 7 +- 5 files changed, 235 insertions(+), 15 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 1b1ecf361a..2bce827ff0 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -23,11 +23,13 @@ use codex_tools::ToolSearchInfo; use codex_tools::ToolSearchSourceInfo; use codex_tools::ToolSpec; use codex_tools::mcp_tool_to_responses_api_tool; +use codex_utils_string::take_bytes_at_char_boundary; use serde_json::Map; use serde_json::Value; const LEGACY_MCP_TOOL_NAME_PREFIX: &str = "mcp__"; const MCP_TOOL_NAME_DELIMITER: &str = "__"; +const MAX_MCP_NAMESPACE_DESCRIPTION_BYTES: usize = 1_000; pub struct McpHandler { tool_info: ToolInfo, @@ -102,6 +104,7 @@ impl ToolExecutor for McpHandler { .as_deref() .map(str::trim) .filter(|description| !description.is_empty()) + .map(bounded_mcp_namespace_description) .map(str::to_string), }); @@ -254,11 +257,15 @@ fn create_tool_spec(tool_info: &ToolInfo) -> Result Ok(ToolSpec::Namespace(ResponsesApiNamespace { name: tool_info.callable_namespace.clone(), - description, + description: bounded_mcp_namespace_description(&description).to_string(), tools: vec![ResponsesApiNamespaceTool::Function(tool)], })) } +fn bounded_mcp_namespace_description(description: &str) -> &str { + take_bytes_at_char_boundary(description, MAX_MCP_NAMESPACE_DESCRIPTION_BYTES) +} + fn mcp_hook_tool_input(raw_arguments: &str) -> Value { if raw_arguments.trim().is_empty() { return Value::Object(Map::new()); diff --git a/codex-rs/core/src/tools/handlers/mcp_search_tests.rs b/codex-rs/core/src/tools/handlers/mcp_search_tests.rs index b78f8c5205..335750a7ea 100644 --- a/codex-rs/core/src/tools/handlers/mcp_search_tests.rs +++ b/codex-rs/core/src/tools/handlers/mcp_search_tests.rs @@ -42,6 +42,32 @@ fn search_info_uses_connector_name_for_output_namespace_description() { ); } +#[test] +fn mcp_namespace_descriptions_are_bounded_without_mutating_metadata() { + let expected_description = "é".repeat(499); + let full_description = format!("{expected_description}🦀keep the complete app metadata"); + let mut info = tool_info(); + info.namespace_description = Some(full_description.clone()); + let handler = McpHandler::new(info).expect("MCP tool spec should build"); + let search_info = handler.search_info().expect("MCP search info"); + + assert_eq!( + search_info.source_info, + Some(ToolSearchSourceInfo { + name: "Calendar".to_string(), + description: Some(expected_description.clone()), + }) + ); + let LoadableToolSpec::Namespace(namespace) = search_info.entry.output else { + panic!("expected namespace search output"); + }; + assert_eq!(namespace.description, expected_description); + assert_eq!( + handler.tool_info.namespace_description, + Some(full_description) + ); +} + fn tool_info() -> ToolInfo { ToolInfo { server_name: "codex-apps".to_string(), diff --git a/codex-rs/core/src/tools/handlers/tool_search_spec.rs b/codex-rs/core/src/tools/handlers/tool_search_spec.rs index c8026bd305..cdcf9ff823 100644 --- a/codex-rs/core/src/tools/handlers/tool_search_spec.rs +++ b/codex-rs/core/src/tools/handlers/tool_search_spec.rs @@ -2,8 +2,11 @@ use codex_tools::JsonSchema; use codex_tools::TOOL_SEARCH_TOOL_NAME; use codex_tools::ToolSearchSourceInfo; use codex_tools::ToolSpec; +use codex_utils_string::take_bytes_at_char_boundary; use std::collections::BTreeMap; +const MAX_TOOL_SEARCH_SOURCE_DESCRIPTION_BYTES: usize = 4 * 1024; + #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub(crate) enum ToolSearchSourceListing { Include, @@ -45,14 +48,40 @@ pub(crate) fn create_tool_search_tool( let source_descriptions = if source_descriptions.is_empty() { "None currently enabled.".to_string() } else { - source_descriptions - .into_iter() - .map(|(name, description)| match description { - Some(description) => format!("- {name}: {description}"), - None => format!("- {name}"), - }) - .collect::>() - .join("\n") + let reserved_name_bytes = source_descriptions.keys().fold( + source_descriptions.len().saturating_sub(1), + |reserved, name| reserved.saturating_add(2).saturating_add(name.len()), + ); + let mut description_budget = + MAX_TOOL_SEARCH_SOURCE_DESCRIPTION_BYTES.saturating_sub(reserved_name_bytes); + let mut rendered = String::new(); + for (name, description) in source_descriptions { + let separator_bytes = usize::from(!rendered.is_empty()); + let required = separator_bytes.saturating_add(2).saturating_add(name.len()); + if required + > MAX_TOOL_SEARCH_SOURCE_DESCRIPTION_BYTES.saturating_sub(rendered.len()) + { + continue; + } + + if !rendered.is_empty() { + rendered.push('\n'); + } + rendered.push_str("- "); + rendered.push_str(&name); + + if let Some(description) = description + && description_budget >= 2 + { + rendered.push_str(": "); + description_budget -= 2; + let bounded_description = + take_bytes_at_char_boundary(&description, description_budget); + rendered.push_str(bounded_description); + description_budget -= bounded_description.len(); + } + } + rendered }; format!( "\n\nYou have access to tools from the following sources:\n{source_descriptions}\n" @@ -144,4 +173,47 @@ mod tests { assert!(!description.contains("Google Drive")); assert!(description.contains("use this tool (`tool_search`) to search")); } + + #[test] + fn create_tool_search_tool_bounds_aggregate_source_descriptions() { + let sources = (0..8) + .map(|index| ToolSearchSourceInfo { + name: format!("source-{index:02}"), + description: Some("🦀".repeat(300)), + }) + .collect::>(); + let ToolSpec::ToolSearch { description, .. } = create_tool_search_tool( + &sources, + /*default_limit*/ 8, + ToolSearchSourceListing::Include, + ) else { + panic!("expected tool search spec"); + }; + + let (_, source_section) = description + .split_once("You have access to tools from the following sources:\n") + .expect("tool search should retain its source introduction"); + let (source_descriptions, _) = source_section + .split_once("\nSome of the tools may not have been provided to you upfront") + .expect("tool search should retain its discovery instructions"); + assert!(source_descriptions.len() <= MAX_TOOL_SEARCH_SOURCE_DESCRIPTION_BYTES); + assert!(source_descriptions.starts_with("- source-00: 🦀")); + let advertised_names = source_descriptions + .lines() + .map(|line| { + let source = line + .strip_prefix("- ") + .expect("each source should be a complete list item"); + source + .split_once(": ") + .map_or(source, |(name, _)| name) + .to_string() + }) + .collect::>(); + let expected_names = (0..8) + .map(|index| format!("source-{index:02}")) + .collect::>(); + assert_eq!(advertised_names, expected_names); + assert!(description.contains("always use `tool_search`")); + } } diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 2dba184f7e..2220730c11 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -542,6 +542,81 @@ fn assert_cwd_tool_output(structured: &Value, expected_cwd: &Path) { ); } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn mcp_namespace_instructions_are_bounded_without_hiding_tools() -> anyhow::Result<()> { + skip_if_wine_exec!( + Ok(()), + "requires a Windows test_stdio_server in the Wine-exec environment" + ); + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let expected_description = "é".repeat(499); + let instructions = format!("{expected_description}🦀keep the valid MCP server"); + let response = mount_sse_once( + &server, + responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_assistant_message("msg-1", "done"), + responses::ev_completed("resp-1"), + ]), + ) + .await; + let command = remote_aware_stdio_server_bin()?; + let fixture = test_codex() + .with_model_info_override("gpt-5.4", |model| model.supports_search_tool = false) + .with_config(move |config| { + insert_mcp_server( + config, + "bounded", + stdio_transport( + command, + Some(HashMap::from([( + "MCP_TEST_SERVER_INSTRUCTIONS".to_string(), + instructions, + )])), + Vec::new(), + ), + TestMcpServerOptions { + environment_id: remote_aware_environment_id(), + ..Default::default() + }, + ); + }) + .build_with_auto_env(&server) + .await?; + wait_for_mcp_server(&fixture.codex, "bounded").await?; + + fixture + .codex + .submit(read_only_user_turn(&fixture, "show the bounded MCP tools")) + .await?; + wait_for_event(&fixture.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let body = response.single_request().body_json(); + let namespace = body + .get("tools") + .and_then(Value::as_array) + .and_then(|tools| { + tools + .iter() + .find(|tool| tool.get("name").and_then(Value::as_str) == Some("mcp__bounded")) + }) + .expect("valid MCP server should remain exposed"); + assert_eq!( + namespace.get("description").and_then(Value::as_str), + Some(expected_description.as_str()) + ); + assert!( + responses::namespace_child_tool(&body, "mcp__bounded", "echo").is_some(), + "bounding the namespace must not hide a valid MCP tool" + ); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] #[serial(mcp_test_value)] async fn stdio_server_round_trip() -> anyhow::Result<()> { @@ -559,7 +634,7 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> { let server_name = "rmcp"; let namespace = format!("mcp__{server_name}"); - mount_sse_once( + let search_mock = mount_sse_once( &server, responses::sse(vec![ responses::ev_response_created("resp-1"), @@ -595,6 +670,8 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> { .await; let expected_env_value = "propagated-env"; + let expected_description = "é".repeat(499); + let instructions = format!("{expected_description}🦀keep the complete MCP metadata"); let rmcp_test_server_bin = remote_aware_stdio_server_bin()?; let fixture = test_codex() @@ -604,10 +681,10 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> { server_name, stdio_transport( rmcp_test_server_bin, - Some(HashMap::from([( - "MCP_TEST_VALUE".to_string(), - expected_env_value.to_string(), - )])), + Some(HashMap::from([ + ("MCP_TEST_VALUE".to_string(), expected_env_value.to_string()), + ("MCP_TEST_SERVER_INSTRUCTIONS".to_string(), instructions), + ])), Vec::new(), ), TestMcpServerOptions { @@ -674,9 +751,42 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> { wait_for_event(&fixture.codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + let search_request = search_mock.single_request().body_json(); + let search_description = search_request + .get("tools") + .and_then(Value::as_array) + .and_then(|tools| { + tools + .iter() + .find(|tool| tool.get("type").and_then(Value::as_str) == Some("tool_search")) + }) + .and_then(|tool| tool.get("description").and_then(Value::as_str)) + .expect("the model should receive a tool search description"); + assert!( + search_description.len() < 5 * 1024, + "the complete tool search description must remain bounded" + ); + assert!(search_description.contains(&format!("- rmcp: {expected_description}"))); + assert!(!search_description.contains("🦀keep the complete MCP metadata")); + let search_output = call_mock .single_request() .tool_search_output(search_call_id); + let searched_namespace = search_output + .get("tools") + .and_then(Value::as_array) + .and_then(|tools| { + tools + .iter() + .find(|tool| tool.get("name").and_then(Value::as_str) == Some(namespace.as_str())) + }) + .expect("tool search should return the RMCP namespace"); + assert_eq!( + searched_namespace + .get("description") + .and_then(Value::as_str), + Some(expected_description.as_str()) + ); assert!( responses::namespace_child_tool(&search_output, &namespace, "echo").is_some(), "tool_search should surface the RMCP echo tool: {search_output:?}" diff --git a/codex-rs/rmcp-client/src/bin/test_stdio_server.rs b/codex-rs/rmcp-client/src/bin/test_stdio_server.rs index 19f95e1534..f1051dd9ea 100644 --- a/codex-rs/rmcp-client/src/bin/test_stdio_server.rs +++ b/codex-rs/rmcp-client/src/bin/test_stdio_server.rs @@ -52,6 +52,7 @@ const SMALL_PNG_BASE64: &str = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAA const APP_ONLY_CWD_MARKER_FILE_ENV: &str = "MCP_TEST_APP_ONLY_CWD_MARKER_FILE"; const DYNAMIC_SERVER_METADATA_ENV: &str = "MCP_TEST_DYNAMIC_SERVER_METADATA"; const INITIALIZE_BARRIER_FILE_ENV: &str = "MCP_TEST_INITIALIZE_BARRIER_FILE"; +const SERVER_INSTRUCTIONS_ENV: &str = "MCP_TEST_SERVER_INSTRUCTIONS"; fn dynamic_server_process_label() -> Option { std::env::var_os(DYNAMIC_SERVER_METADATA_ENV) @@ -493,7 +494,7 @@ impl ServerHandler for TestToolServer { )])); let server_info = ServerInfo::new(capabilities); - match dynamic_server_process_label() { + let server_info = match dynamic_server_process_label() { Some(process_label) => server_info .with_server_info( Implementation::new("codex-rmcp-test-server", env!("CARGO_PKG_VERSION")) @@ -503,6 +504,10 @@ impl ServerHandler for TestToolServer { None => { server_info.with_instructions("Use these tools to exercise the rmcp test server.") } + }; + match std::env::var(SERVER_INSTRUCTIONS_ENV) { + Ok(instructions) => server_info.with_instructions(instructions), + Err(_) => server_info, } }