Move MCP tool naming mode into manager#21576
Conversation
pakrym-oai
force-pushed
the
pakrym/mcp-tool-names-flag
branch
6 times, most recently
from
c23f0b7 to
9869ea8
Compare
pakrym-oai
force-pushed
the
pakrym/mcp-tool-names-flag
branch
from
9869ea8 to
a0a59cf
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0a59cffb4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fn should_collect_unavailable_tool(name: &str, namespace: Option<&str>) -> bool { | ||
| namespace.is_some_and(|namespace| namespace.starts_with("mcp__")) || name.starts_with("mcp__") | ||
| } |
There was a problem hiding this comment.
wouldnt this break with the new flag on? we wouldnt catch them as unavailable if they dont have the prefix
| callable_namespace: callable_namespace_with_prefix( | ||
| &sanitize_responses_api_tool_name(&tool.callable_namespace), | ||
| prefix_mcp_tool_names, | ||
| ), |
There was a problem hiding this comment.
the mcp__ prefix was preventing collisions with reserved top-level namespaces like web and tools, and dynamic namespaces. what if user now has mcp server named web, or a conflicting dynamic namespace? i dont think we currently have all the info needed in McpConnectionManager to do collision detection, but maybe we can at least reserve some known names
sayan-oai
left a comment
sayan-oai
left a comment
There was a problem hiding this comment.
approach makes sense, and centralizing into McpConnectionManager is attractive. left some questions but overall makes sense to me
…-flag # Conflicts: # codex-rs/codex-mcp/src/connection_manager_tests.rs # codex-rs/core/src/tools/handlers/mcp.rs # codex-rs/core/src/tools/spec.rs # codex-rs/core/src/unavailable_tool.rs
…-flag # Conflicts: # codex-rs/core/src/config/config_tests.rs # codex-rs/core/tests/suite/sqlite_state.rs
…-flag # Conflicts: # codex-rs/codex-mcp/src/connection_manager.rs # codex-rs/codex-mcp/src/mcp/mod.rs # codex-rs/codex-mcp/src/mcp/mod_tests.rs # codex-rs/core/src/config/config_tests.rs # codex-rs/core/src/config/mod.rs # codex-rs/core/src/connectors.rs # codex-rs/core/src/mcp_tool_call_tests.rs # codex-rs/core/src/session/mcp.rs # codex-rs/core/src/session/session.rs
…-flag # Conflicts: # codex-rs/core/config.schema.json
2 participants
Why
The
non_prefixed_mcp_tool_namesfeature should be applied where MCP tools become model-visible, not by remapping names later in core. Keeping the decision inMcpConnectionManagerconstruction makesToolInfothe 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
McpToolNameModetocodex-mcpand flow it throughMcpConfigintoMcpConnectionManager::new.ToolInfonames in the manager using either legacy-prefixed namespaces or non-prefixed namespaces; the legacy path addsmcp__without restoring the old trailing namespace suffix.ToolNamevalues directly.__namespace separator.mcp__...matcher aliases.Testing
cargo test -p codex-mcp --libcargo test -p codex-core --lib tools::spec::tests -- --nocapturecargo test -p codex-core --lib mcp_tools -- --nocapturecargo test -p codex-core --lib mcp_tool_exposure -- --nocapturecargo test -p codex-core --test all mcp_tool -- --nocapturecargo test -p codex-core --test all search_tool -- --nocapturecargo test -p codex-core --test all hooks_mcp -- --nocapturecargo test -p codex-core --test all code_mode_uses_non_prefixed_mcp_tool_names_when_feature_enabled -- --nocapturecargo test -p codex-toolscargo test -p codex-features