Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion codex-rs/app-server/tests/suite/v2/mcp_tool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ url = "{mcp_server_url}/mcp"
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn mcp_tool_call_completion_notification_contains_truncated_large_result() -> Result<()> {
let call_id = "call-large-mcp";
let namespace = format!("mcp__{TEST_SERVER_NAME}__");
let namespace = format!("mcp__{TEST_SERVER_NAME}");
let responses = vec![
responses::sse(vec![
responses::ev_response_created("resp-1"),
Expand Down
6 changes: 3 additions & 3 deletions codex-rs/codex-mcp/src/codex_apps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use serde::Serialize;
use sha1::Digest;
use sha1::Sha1;

pub(crate) const CODEX_APPS_TOOLS_CACHE_SCHEMA_VERSION: u8 = 2;
pub(crate) const CODEX_APPS_TOOLS_CACHE_SCHEMA_VERSION: u8 = 3;

#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct CodexAppsToolsCacheKey {
Expand Down Expand Up @@ -127,9 +127,9 @@ pub(crate) fn normalize_codex_apps_callable_namespace(
if server_name == CODEX_APPS_MCP_SERVER_NAME
&& let Some(connector_name) = connector_name
{
format!("mcp__{}__{}", server_name, sanitize_name(connector_name))
format!("{}__{}", server_name, sanitize_name(connector_name))
} else {
format!("mcp__{server_name}__")
server_name.to_string()
Comment thread
pakrym-oai marked this conversation as resolved.
}
}

Expand Down
14 changes: 11 additions & 3 deletions codex-rs/codex-mcp/src/connection_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use crate::server::EffectiveMcpServer;
use crate::server::McpServerMetadata;
use crate::tools::ToolInfo;
use crate::tools::filter_tools;
use crate::tools::normalize_tools_for_model;
use crate::tools::normalize_tools_for_model_with_prefix;
use crate::tools::tool_with_model_visible_input_schema;
use anyhow::Context;
use anyhow::Result;
Expand Down Expand Up @@ -74,6 +74,7 @@ pub struct McpConnectionManager {
clients: HashMap<String, AsyncManagedClient>,
server_metadata: HashMap<String, McpServerMetadata>,
host_owned_codex_apps_enabled: bool,
prefix_mcp_tool_names: bool,
elicitation_requests: ElicitationRequestManager,
startup_cancellation_token: CancellationToken,
}
Expand All @@ -82,11 +83,13 @@ impl McpConnectionManager {
pub fn new_uninitialized(
approval_policy: &Constrained<AskForApproval>,
permission_profile: &Constrained<PermissionProfile>,
prefix_mcp_tool_names: bool,
) -> Self {
Self {
clients: HashMap::new(),
server_metadata: HashMap::new(),
host_owned_codex_apps_enabled: false,
prefix_mcp_tool_names,
elicitation_requests: ElicitationRequestManager::new(
approval_policy.value(),
permission_profile.get().clone(),
Expand Down Expand Up @@ -179,6 +182,7 @@ impl McpConnectionManager {
codex_home: PathBuf,
codex_apps_tools_cache_key: CodexAppsToolsCacheKey,
host_owned_codex_apps_enabled: bool,
prefix_mcp_tool_names: bool,
client_elicitation_capability: ElicitationCapability,
tool_plugin_provenance: ToolPluginProvenance,
auth: Option<&CodexAuth>,
Expand Down Expand Up @@ -290,6 +294,7 @@ impl McpConnectionManager {
clients,
server_metadata,
host_owned_codex_apps_enabled,
prefix_mcp_tool_names,
elicitation_requests: elicitation_requests.clone(),
startup_cancellation_token: cancel_token.clone(),
};
Expand Down Expand Up @@ -375,7 +380,7 @@ impl McpConnectionManager {
};
tools.extend(server_tools);
}
normalize_tools_for_model(tools)
normalize_tools_for_model_with_prefix(tools, self.prefix_mcp_tool_names)
}

/// Force-refresh codex apps tools by bypassing the in-process cache.
Expand Down Expand Up @@ -426,7 +431,10 @@ impl McpConnectionManager {
tool.tool = tool_with_model_visible_input_schema(&tool.tool);
tool
});
Ok(normalize_tools_for_model(tools))
Ok(normalize_tools_for_model_with_prefix(
tools,
self.prefix_mcp_tool_names,
))
}

/// Returns a single map that contains all resources. Each key is the
Expand Down
131 changes: 98 additions & 33 deletions codex-rs/codex-mcp/src/connection_manager_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::rmcp_client::StartupOutcomeError;
use crate::tools::ToolFilter;
use crate::tools::ToolInfo;
use crate::tools::filter_tools;
use crate::tools::normalize_tools_for_model;
use crate::tools::normalize_tools_for_model_with_prefix;
use crate::tools::tool_with_model_visible_input_schema;
use codex_config::Constrained;
use codex_config::McpServerConfig;
Expand All @@ -35,11 +35,10 @@ use std::sync::Arc;
use tempfile::tempdir;

fn create_test_tool(server_name: &str, tool_name: &str) -> ToolInfo {
let tool_namespace = format!("mcp__{server_name}__");
ToolInfo {
server_name: server_name.to_string(),
callable_name: tool_name.to_string(),
callable_namespace: tool_namespace,
callable_namespace: server_name.to_string(),
namespace_description: None,
tool: Tool {
name: tool_name.to_string().into(),
Expand Down Expand Up @@ -93,7 +92,10 @@ fn model_tool_names(tools: &[ToolInfo]) -> HashSet<ToolName> {
}

fn model_tool_name_len(name: &ToolName) -> usize {
name.namespace.as_deref().map_or(0, str::len) + name.name.len()
name.namespace
.as_deref()
.map_or(0, |namespace| namespace.len() + "__".len())
+ name.name.len()
}

fn is_code_mode_compatible_tool_name(name: &ToolName) -> bool {
Expand Down Expand Up @@ -297,13 +299,14 @@ fn test_normalize_tools_short_non_duplicated_names() {
create_test_tool("server1", "tool2"),
];

let model_tools = normalize_tools_for_model(tools);
let model_tools =
normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true);

assert_eq!(
model_tool_names(&model_tools),
HashSet::from([
ToolName::namespaced("mcp__server1__", "tool1"),
ToolName::namespaced("mcp__server1__", "tool2")
ToolName::namespaced("mcp__server1", "tool1"),
ToolName::namespaced("mcp__server1", "tool2")
])
);
}
Expand All @@ -315,12 +318,13 @@ fn test_normalize_tools_duplicated_names_skipped() {
create_test_tool("server1", "duplicate_tool"),
];

let model_tools = normalize_tools_for_model(tools);
let model_tools =
normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true);

// Only the first tool should remain, the second is skipped
assert_eq!(
model_tool_names(&model_tools),
HashSet::from([ToolName::namespaced("mcp__server1__", "duplicate_tool")])
HashSet::from([ToolName::namespaced("mcp__server1", "duplicate_tool")])
);
}

Expand All @@ -339,7 +343,8 @@ fn test_normalize_tools_long_names_same_server() {
),
];

let model_tools = normalize_tools_for_model(tools);
let model_tools =
normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true);

assert_eq!(model_tools.len(), 2);

Expand All @@ -349,7 +354,7 @@ fn test_normalize_tools_long_names_same_server() {
assert!(
names
.iter()
.all(|name| name.namespace.as_deref() == Some("mcp__my_server__"))
.all(|name| name.namespace.as_deref() == Some("mcp__my_server"))
);
assert!(
names.iter().all(is_code_mode_compatible_tool_name),
Expand All @@ -361,14 +366,15 @@ fn test_normalize_tools_long_names_same_server() {
fn test_normalize_tools_sanitizes_invalid_characters() {
let tools = vec![create_test_tool("server.one", "tool.two-three")];

let model_tools = normalize_tools_for_model(tools);
let model_tools =
normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true);

assert_eq!(model_tools.len(), 1);
let tool = model_tools.into_iter().next().expect("one tool");
let model_name = tool.canonical_tool_name();
assert_eq!(
model_name,
ToolName::namespaced("mcp__server_one__", "tool_two_three")
ToolName::namespaced("mcp__server_one", "tool_two_three")
);
assert_eq!(
ToolName::namespaced(tool.callable_namespace.clone(), tool.callable_name.clone()),
Expand All @@ -377,7 +383,7 @@ fn test_normalize_tools_sanitizes_invalid_characters() {
// The callable parts are sanitized for model-visible tool calls, but the raw
// MCP name is preserved for the actual MCP call.
assert_eq!(tool.server_name, "server.one");
assert_eq!(tool.callable_namespace, "mcp__server_one__");
assert_eq!(tool.callable_namespace, "mcp__server_one");
assert_eq!(tool.callable_name, "tool_two_three");
assert_eq!(tool.tool.name, "tool.two-three");

Expand All @@ -391,15 +397,16 @@ fn test_normalize_tools_sanitizes_invalid_characters() {
fn test_normalize_tools_keeps_hyphenated_mcp_tools_callable() {
let tools = vec![create_test_tool("music-studio", "get-strudel-guide")];

let model_tools = normalize_tools_for_model(tools);
let model_tools =
normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true);

assert_eq!(model_tools.len(), 1);
let tool = model_tools.into_iter().next().expect("one tool");
assert_eq!(
tool.canonical_tool_name(),
ToolName::namespaced("mcp__music_studio__", "get_strudel_guide")
ToolName::namespaced("mcp__music_studio", "get_strudel_guide")
);
assert_eq!(tool.callable_namespace, "mcp__music_studio__");
assert_eq!(tool.callable_namespace, "mcp__music_studio");
assert_eq!(tool.callable_name, "get_strudel_guide");
assert_eq!(tool.tool.name, "get-strudel-guide");
}
Expand All @@ -411,7 +418,8 @@ fn test_normalize_tools_disambiguates_sanitized_namespace_collisions() {
create_test_tool("basic_server", "query"),
];

let model_tools = normalize_tools_for_model(tools);
let model_tools =
normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true);

assert_eq!(model_tools.len(), 2);
let mut namespaces = model_tools
Expand Down Expand Up @@ -441,7 +449,8 @@ fn test_normalize_tools_disambiguates_sanitized_tool_name_collisions() {
create_test_tool("server", "tool_name"),
];

let model_tools = normalize_tools_for_model(tools);
let model_tools =
normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true);

assert_eq!(model_tools.len(), 2);
let raw_tool_names = model_tools
Expand Down Expand Up @@ -687,8 +696,11 @@ async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() {
.shared();
let approval_policy = Constrained::allow_any(AskForApproval::OnFailure);
let permission_profile = Constrained::allow_any(PermissionProfile::default());
let mut manager =
McpConnectionManager::new_uninitialized(&approval_policy, &permission_profile);
let mut manager = McpConnectionManager::new_uninitialized(
&approval_policy,
&permission_profile,
/*prefix_mcp_tool_names*/ true,
);
manager.clients.insert(
CODEX_APPS_MCP_SERVER_NAME.to_string(),
AsyncManagedClient {
Expand All @@ -705,7 +717,7 @@ async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() {
.iter()
.find(|tool| {
tool.canonical_tool_name()
== ToolName::namespaced("mcp__codex_apps__", "calendar_create_event")
== ToolName::namespaced("mcp__codex_apps", "calendar_create_event")
})
.expect("tool from startup cache");
assert_eq!(tool.server_name, CODEX_APPS_MCP_SERVER_NAME);
Expand All @@ -720,8 +732,11 @@ async fn resolve_tool_info_accepts_canonical_namespaced_tool_names() {
.shared();
let approval_policy = Constrained::allow_any(AskForApproval::OnFailure);
let permission_profile = Constrained::allow_any(PermissionProfile::default());
let mut manager =
McpConnectionManager::new_uninitialized(&approval_policy, &permission_profile);
let mut manager = McpConnectionManager::new_uninitialized(
&approval_policy,
&permission_profile,
/*prefix_mcp_tool_names*/ false,
);
manager.clients.insert(
"rmcp".to_string(),
AsyncManagedClient {
Expand All @@ -734,11 +749,52 @@ async fn resolve_tool_info_accepts_canonical_namespaced_tool_names() {
);

let tool = manager
.resolve_tool_info(&ToolName::namespaced("mcp__rmcp__", "echo"))
.resolve_tool_info(&ToolName::namespaced("rmcp", "echo"))
.await
.expect("split MCP tool namespace and name should resolve");

let expected = ("rmcp", "mcp__rmcp__", "echo", "echo");
let expected = ("rmcp", "rmcp", "echo", "echo");
assert_eq!(
(
tool.server_name.as_str(),
tool.callable_namespace.as_str(),
tool.callable_name.as_str(),
tool.tool.name.as_ref(),
),
expected
);
}

#[tokio::test]
async fn list_all_tools_applies_legacy_mcp_prefix_by_default() {
let startup_tools = vec![create_test_tool("rmcp", "echo")];
let pending_client = futures::future::pending::<Result<ManagedClient, StartupOutcomeError>>()
.boxed()
.shared();
let approval_policy = Constrained::allow_any(AskForApproval::OnFailure);
let permission_profile = Constrained::allow_any(PermissionProfile::default());
let mut manager = McpConnectionManager::new_uninitialized(
&approval_policy,
&permission_profile,
/*prefix_mcp_tool_names*/ true,
);
manager.clients.insert(
"rmcp".to_string(),
AsyncManagedClient {
client: pending_client,
startup_snapshot: Some(startup_tools),
startup_complete: Arc::new(std::sync::atomic::AtomicBool::new(false)),
tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()),
cancel_token: CancellationToken::new(),
},
);

let tool = manager
.resolve_tool_info(&ToolName::namespaced("mcp__rmcp", "echo"))
.await
.expect("legacy-prefixed MCP tool name should resolve");

let expected = ("rmcp", "mcp__rmcp", "echo", "echo");
assert_eq!(
(
tool.server_name.as_str(),
Expand All @@ -757,8 +813,11 @@ async fn list_all_tools_blocks_while_client_is_pending_without_startup_snapshot(
.shared();
let approval_policy = Constrained::allow_any(AskForApproval::OnFailure);
let permission_profile = Constrained::allow_any(PermissionProfile::default());
let mut manager =
McpConnectionManager::new_uninitialized(&approval_policy, &permission_profile);
let mut manager = McpConnectionManager::new_uninitialized(
&approval_policy,
&permission_profile,
/*prefix_mcp_tool_names*/ true,
);
manager.clients.insert(
CODEX_APPS_MCP_SERVER_NAME.to_string(),
AsyncManagedClient {
Expand All @@ -782,8 +841,11 @@ async fn list_all_tools_does_not_block_when_startup_snapshot_cache_hit_is_empty(
.shared();
let approval_policy = Constrained::allow_any(AskForApproval::OnFailure);
let permission_profile = Constrained::allow_any(PermissionProfile::default());
let mut manager =
McpConnectionManager::new_uninitialized(&approval_policy, &permission_profile);
let mut manager = McpConnectionManager::new_uninitialized(
&approval_policy,
&permission_profile,
/*prefix_mcp_tool_names*/ true,
);
manager.clients.insert(
CODEX_APPS_MCP_SERVER_NAME.to_string(),
AsyncManagedClient {
Expand Down Expand Up @@ -816,8 +878,11 @@ async fn list_all_tools_uses_startup_snapshot_when_client_startup_fails() {
.shared();
let approval_policy = Constrained::allow_any(AskForApproval::OnFailure);
let permission_profile = Constrained::allow_any(PermissionProfile::default());
let mut manager =
McpConnectionManager::new_uninitialized(&approval_policy, &permission_profile);
let mut manager = McpConnectionManager::new_uninitialized(
&approval_policy,
&permission_profile,
/*prefix_mcp_tool_names*/ true,
);
let startup_complete = Arc::new(std::sync::atomic::AtomicBool::new(true));
manager.clients.insert(
CODEX_APPS_MCP_SERVER_NAME.to_string(),
Expand All @@ -835,7 +900,7 @@ async fn list_all_tools_uses_startup_snapshot_when_client_startup_fails() {
.iter()
.find(|tool| {
tool.canonical_tool_name()
== ToolName::namespaced("mcp__codex_apps__", "calendar_create_event")
== ToolName::namespaced("mcp__codex_apps", "calendar_create_event")
})
.expect("tool from startup cache");
assert_eq!(tool.server_name, CODEX_APPS_MCP_SERVER_NAME);
Expand Down
Loading
Loading