mirror of
https://github.com/openai/codex.git
synced 2026-10-19 16:17:49 +00:00
Reuse MCP handler metadata for cache comparisons (#51755)
## What changed Remove the duplicate tool metadata stored in `CachedMcpHandler` and compare incoming metadata directly with `McpHandler::tool_info()`. Preserve complete agent plugin namespace descriptions in the handler, applying the existing 1,000-byte limit when building tool specs and search metadata. Truncation continues to respect UTF-8 character boundaries. ## Testing Extend the agent plugin namespace description test to verify that the handler retains the full description while search output remains bounded. GitOrigin-RevId: d0a9d5f6796143315235e658e77e059657e85f13
This commit is contained in:
@@ -28,7 +28,6 @@ pub(crate) struct McpHandlerCache {
|
||||
}
|
||||
|
||||
struct CachedMcpHandler {
|
||||
tool_info: McpToolInfo,
|
||||
agent_plugin: bool,
|
||||
schema_max_bytes: Option<NonZeroUsize>,
|
||||
handler: Arc<McpHandler>,
|
||||
@@ -98,7 +97,7 @@ fn append_mcp_tools(
|
||||
// Handlers contain immutable tool metadata, not a connection or authorization snapshot.
|
||||
// Preserve their identity across equivalent bindings so the search index can also be reused.
|
||||
let handler = if let Some(cached) = handlers.get(&tool_name).filter(|cached| {
|
||||
cached.tool_info == *tool
|
||||
cached.handler.tool_info() == tool
|
||||
&& cached.agent_plugin == agent_plugin
|
||||
&& cached.schema_max_bytes == tool_input_schema_max_bytes
|
||||
}) {
|
||||
@@ -123,7 +122,6 @@ fn append_mcp_tools(
|
||||
handlers.insert(
|
||||
tool_name.clone(),
|
||||
CachedMcpHandler {
|
||||
tool_info: tool.clone(),
|
||||
agent_plugin,
|
||||
schema_max_bytes: tool_input_schema_max_bytes,
|
||||
handler: Arc::clone(&handler),
|
||||
|
||||
@@ -50,6 +50,7 @@ const MAX_MCP_NAMESPACE_DESCRIPTION_BYTES: usize = 512 * 1024;
|
||||
|
||||
pub struct McpHandler {
|
||||
tool_info: ToolInfo,
|
||||
agent_plugin: bool,
|
||||
spec: Arc<ToolSpec>,
|
||||
code_mode_tool_definitions:
|
||||
OnceLock<(Option<usize>, bool, Vec<codex_code_mode::ToolDefinition>)>,
|
||||
@@ -80,23 +81,10 @@ impl McpHandler {
|
||||
}
|
||||
|
||||
fn with_agent_plugin(
|
||||
mut tool_info: ToolInfo,
|
||||
tool_info: ToolInfo,
|
||||
agent_plugin: bool,
|
||||
schema_max_bytes: Option<usize>,
|
||||
) -> Result<Self, serde_json::Error> {
|
||||
if agent_plugin {
|
||||
tool_info.namespace_description =
|
||||
tool_info
|
||||
.namespace_description
|
||||
.as_deref()
|
||||
.map(|description| {
|
||||
take_bytes_at_char_boundary(
|
||||
description,
|
||||
MAX_AGENT_PLUGIN_MCP_NAMESPACE_DESCRIPTION_BYTES,
|
||||
)
|
||||
.to_string()
|
||||
});
|
||||
}
|
||||
let spec = Arc::new(create_tool_spec(
|
||||
&tool_info,
|
||||
agent_plugin,
|
||||
@@ -104,11 +92,16 @@ impl McpHandler {
|
||||
)?);
|
||||
Ok(Self {
|
||||
tool_info,
|
||||
agent_plugin,
|
||||
spec,
|
||||
code_mode_tool_definitions: OnceLock::new(),
|
||||
})
|
||||
}
|
||||
|
||||
pub(crate) fn tool_info(&self) -> &ToolInfo {
|
||||
&self.tool_info
|
||||
}
|
||||
|
||||
pub(crate) fn model_spec_bytes(&self) -> Result<usize, serde_json::Error> {
|
||||
serde_json::to_vec(&self.spec).map(|spec| spec.len())
|
||||
}
|
||||
@@ -169,17 +162,14 @@ impl ToolExecutor<ToolInvocation> for McpHandler {
|
||||
.unwrap_or_else(|| self.tool_info.server_name.trim());
|
||||
let source_info = (!source_name.is_empty()).then(|| ToolSearchSourceInfo {
|
||||
name: source_name.to_string(),
|
||||
description: self
|
||||
.tool_info
|
||||
.namespace_description
|
||||
.as_deref()
|
||||
description: effective_namespace_description(&self.tool_info, self.agent_plugin)
|
||||
.map(str::trim)
|
||||
.filter(|description| !description.is_empty())
|
||||
.map(str::to_string),
|
||||
});
|
||||
|
||||
ToolSearchInfo::from_shared_spec(
|
||||
build_mcp_search_text(&self.tool_info),
|
||||
build_mcp_search_text(&self.tool_info, self.agent_plugin),
|
||||
Arc::clone(&self.spec),
|
||||
source_info,
|
||||
)
|
||||
@@ -520,9 +510,7 @@ fn create_tool_spec(
|
||||
} else {
|
||||
mcp_tool_to_responses_api_tool(&tool_name, &tool_info.tool, schema_max_bytes)?
|
||||
};
|
||||
let description = tool_info
|
||||
.namespace_description
|
||||
.as_deref()
|
||||
let description = effective_namespace_description(tool_info, agent_plugin)
|
||||
.map(str::trim)
|
||||
.filter(|description| !description.is_empty())
|
||||
.map(str::to_string)
|
||||
@@ -544,6 +532,18 @@ fn create_tool_spec(
|
||||
}))
|
||||
}
|
||||
|
||||
fn effective_namespace_description(tool_info: &ToolInfo, agent_plugin: bool) -> Option<&str> {
|
||||
let description = tool_info.namespace_description.as_deref()?;
|
||||
Some(if agent_plugin {
|
||||
take_bytes_at_char_boundary(
|
||||
description,
|
||||
MAX_AGENT_PLUGIN_MCP_NAMESPACE_DESCRIPTION_BYTES,
|
||||
)
|
||||
} else {
|
||||
description
|
||||
})
|
||||
}
|
||||
|
||||
fn mcp_hook_tool_input(raw_arguments: &str) -> Value {
|
||||
if raw_arguments.trim().is_empty() {
|
||||
return Value::Object(Map::new());
|
||||
@@ -552,7 +552,7 @@ fn mcp_hook_tool_input(raw_arguments: &str) -> Value {
|
||||
serde_json::from_str(raw_arguments).unwrap_or_else(|_| Value::String(raw_arguments.to_string()))
|
||||
}
|
||||
|
||||
fn build_mcp_search_text(info: &ToolInfo) -> String {
|
||||
fn build_mcp_search_text(info: &ToolInfo, agent_plugin: bool) -> String {
|
||||
let tool_name = info.canonical_tool_name();
|
||||
let mut schema_properties = info
|
||||
.tool
|
||||
@@ -583,7 +583,8 @@ fn build_mcp_search_text(info: &ToolInfo) -> String {
|
||||
{
|
||||
parts.push(connector_name.to_string());
|
||||
}
|
||||
if let Some(namespace_description) = info.namespace_description.as_deref().map(str::trim)
|
||||
if let Some(namespace_description) =
|
||||
effective_namespace_description(info, agent_plugin).map(str::trim)
|
||||
&& !namespace_description.is_empty()
|
||||
{
|
||||
parts.push(namespace_description.to_string());
|
||||
|
||||
@@ -92,8 +92,9 @@ fn mcp_namespace_descriptions_are_bounded_at_512_kib() {
|
||||
#[test]
|
||||
fn agent_plugin_namespace_descriptions_use_the_stricter_bound() {
|
||||
let expected_description = "é".repeat(MAX_AGENT_PLUGIN_MCP_NAMESPACE_DESCRIPTION_BYTES / 2);
|
||||
let full_description = format!("{expected_description}overflow");
|
||||
let mut info = tool_info();
|
||||
info.namespace_description = Some(format!("{expected_description}overflow"));
|
||||
info.namespace_description = Some(full_description.clone());
|
||||
let handler = McpHandler::new_agent_plugin(info).expect("MCP tool spec should build");
|
||||
let search_info = handler.search_info().expect("MCP search info");
|
||||
|
||||
@@ -108,6 +109,10 @@ fn agent_plugin_namespace_descriptions_use_the_stricter_bound() {
|
||||
panic!("expected namespace search output");
|
||||
};
|
||||
assert_eq!(namespace.description, expected_description);
|
||||
assert_eq!(
|
||||
handler.tool_info().namespace_description.as_deref(),
|
||||
Some(full_description.as_str())
|
||||
);
|
||||
}
|
||||
|
||||
fn tool_info() -> ToolInfo {
|
||||
|
||||
Reference in New Issue
Block a user