github/codex
4
0 Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-13 14:46:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

Move MCP tool naming mode into manager (#21576)

Browse Source 
## Why

The `non_prefixed_mcp_tool_names` feature should be applied where MCP
tools become model-visible, not by remapping names later in core.
Keeping the decision in `McpConnectionManager` construction makes
`ToolInfo` the single shaped view that spec building, deferred tool
search, routing, and unavailable-tool placeholders can consume directly.

This also preserves the existing external behavior while the feature is
off, and keeps the feature-on behavior for code mode and hooks explicit
at the manager boundary.

## What Changed

- Add `McpToolNameMode` to `codex-mcp` and flow it through `McpConfig`
into `McpConnectionManager::new`.
- Normalize MCP `ToolInfo` names in the manager using either
legacy-prefixed namespaces or non-prefixed namespaces; the legacy path
adds `mcp__` without restoring the old trailing namespace suffix.
- Remove the core-side MCP name remapping path so specs, tool search,
session resolution, and unavailable-tool placeholder construction use
the manager-provided `ToolName` values directly.
- Keep code mode flattening on the `__` namespace separator.
- Preserve hook compatibility by giving non-prefixed MCP hook names
legacy `mcp__...` matcher aliases.
- Add/adjust integration and unit coverage for non-prefixed code-mode
behavior, hook matching with the feature on and off, and manager-level
legacy prefixing.

## Testing

- `cargo test -p codex-mcp --lib`
- `cargo test -p codex-core --lib tools::spec::tests -- --nocapture`
- `cargo test -p codex-core --lib mcp_tools -- --nocapture`
- `cargo test -p codex-core --lib mcp_tool_exposure -- --nocapture`
- `cargo test -p codex-core --test all mcp_tool -- --nocapture`
- `cargo test -p codex-core --test all search_tool -- --nocapture`
- `cargo test -p codex-core --test all hooks_mcp -- --nocapture`
- `cargo test -p codex-core --test all
code_mode_uses_non_prefixed_mcp_tool_names_when_feature_enabled --
--nocapture`
- `cargo test -p codex-tools`
- `cargo test -p codex-features`
This commit is contained in:
pakrym-oai authored and GitHub committed 2026-05-26 08:21:15 -07:00
1 parent b637fd26aa
commit ff7513cd83
30 files changed
+611 -146
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

codex-rs/codex-mcp/src/connection_manager_tests.rs
+179 -31
View File
@@ -14,7 +14,7 @@ use crate::server::McpServerOrigin;
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;
@@ -37,13 +37,12 @@ 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(),
supports_parallel_tool_calls: false,
server_origin: None,
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(),
@@ -97,7 +96,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 {
@@ -301,13 +303,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")
])
);
}
@@ -319,12 +322,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")])
);
}
@@ -343,7 +347,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);
@@ -353,7 +358,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),
@@ -365,14 +370,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()),
@@ -381,7 +387,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");
@@ -395,19 +401,59 @@ 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");
}
#[test]
fn test_normalize_tools_disambiguates_reserved_unprefixed_namespaces() {
let tools = vec![
create_test_tool("tools", "list"),
create_test_tool("web", "search"),
];
let model_tools =
normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ false);
assert_eq!(model_tools.len(), 2);
let namespaces = model_tools
.iter()
.map(|tool| tool.callable_namespace.as_str())
.collect::<HashSet<_>>();
assert_eq!(namespaces.len(), 2);
assert!(
namespaces
.iter()
.all(|namespace| !matches!(*namespace, "tools" | "web")),
"reserved namespaces should be disambiguated: {namespaces:?}"
);
assert!(
namespaces
.iter()
.any(|namespace| namespace.starts_with("tools_"))
);
assert!(
namespaces
.iter()
.any(|namespace| namespace.starts_with("web_"))
);
let model_names = model_tool_names(&model_tools);
assert!(
model_names.iter().all(is_code_mode_compatible_tool_name),
"model-visible names must be code-mode compatible: {model_names:?}"
);
}
#[test]
fn test_normalize_tools_disambiguates_sanitized_namespace_collisions() {
let tools = vec![
@@ -415,7 +461,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
@@ -445,7 +492,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
@@ -691,8 +739,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 {
@@ -709,13 +760,97 @@ 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);
assert_eq!(tool.callable_name, "calendar_create_event");
}
#[tokio::test]
async fn list_all_tools_accepts_canonical_namespaced_tool_names() {
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*/ false,
);
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 tools = manager.list_all_tools().await;
let tool = tools
.iter()
.find(|tool| tool.canonical_tool_name() == ToolName::namespaced("rmcp", "echo"))
.expect("split MCP tool namespace and name should resolve");
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 tools = manager.list_all_tools().await;
let tool = tools
.iter()
.find(|tool| tool.canonical_tool_name() == ToolName::namespaced("mcp__rmcp", "echo"))
.expect("legacy-prefixed MCP tool name should resolve");
let expected = ("rmcp", "mcp__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_blocks_while_client_is_pending_without_startup_snapshot() {
let pending_client = futures::future::pending::<Result<ManagedClient, StartupOutcomeError>>()
@@ -723,8 +858,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 {
@@ -748,8 +886,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 {
@@ -782,8 +923,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(),
@@ -801,7 +945,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);
@@ -817,8 +961,11 @@ async fn list_all_tools_adds_server_metadata_to_cached_tools() {
.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.server_metadata.insert(
server_name.to_string(),
McpServerMetadata {
@@ -927,6 +1074,7 @@ async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() {
is_workspace_account: false,
},
/*host_owned_codex_apps_enabled*/ false,
/*prefix_mcp_tool_names*/ true,
ElicitationCapability::default(),
ToolPluginProvenance::default(),
/*auth*/ None,