mirror of
https://github.com/openai/codex.git
synced 2026-10-21 07:42:01 +00:00
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
This commit is contained in:
@@ -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<ToolInvocation> 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<ToolSpec, serde_json::Error>
|
||||
|
||||
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());
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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::<Vec<_>>()
|
||||
.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::<Vec<_>>();
|
||||
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::<Vec<_>>();
|
||||
let expected_names = (0..8)
|
||||
.map(|index| format!("source-{index:02}"))
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(advertised_names, expected_names);
|
||||
assert!(description.contains("always use `tool_search`"));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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:?}"
|
||||
|
||||
@@ -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<String> {
|
||||
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,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user