Simplify MCP tool handler plumbing#21595
Conversation
3360003 to
7d1a603
Compare
7d1a603 to
c22ab62
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 775d4cbd40
ℹ️ 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".
sayan-oai
left a comment
There was a problem hiding this comment.
very satisfying to review
jif-oai
left a comment
There was a problem hiding this comment.
I like the direction. I feel like it erases the semantic distinction, then recover it later through a generic path (the scan on the generic tags is the good example). Ideally we don't need this last step anymore in the future as it makes the flow not very smooth
(I still think we should solve the metrics cardinality issue)
| execution: "client".to_string(), | ||
| tools: Vec::new(), | ||
| }, | ||
| ToolPayload::Mcp { .. } => ResponseInputItem::McpToolCallOutput { |
There was a problem hiding this comment.
This means a cancelled nested MCP call (through code-mode) will change it's output type. Good for me but to be checked with research
There was a problem hiding this comment.
I'm actually not sure that user-cancellation works for code mode at all. Will followup.
…andler-plumbing # Conflicts: # codex-rs/codex-mcp/src/connection_manager_tests.rs # codex-rs/core/src/tools/router_tests.rs
Agreed but unfortunately we already publish those metrics and I don't want to break them. Fixed cardinality. |
Why
The MCP tool path had accumulated a few core-owned special cases: a dedicated payload variant, resolver plumbing, a legacy
AfterToolUsetranslation path, and a side channel for parallel-call metadata. That madeToolRegistryand the spec builder know more about MCP than they needed to.This change moves MCP-specific execution details back onto
ToolInfoandMcpHandlersocodex-corecan treat MCP calls like normal function calls while still preserving MCP-specific dispatch and telemetry behavior where it belongs.What changed
resolve_mcp_tool_info,ToolPayload::Mcp,ToolKind, and the remaining registry-side MCP resolver pathMcpHandlerandToolInfo, includingsupports_parallel_tool_callsAfterToolUseconsumer incore, which removes the need for handler-specificafter_tool_use_payloadimplementationsToolInfodirectly and dropping the direct/deferred MCP wrapper structs and the parallel-server side tableTesting
cargo check -p codex-core -p codex-mcp -p codex-otelcargo test -p codex-core mcp_parallel_support_uses_exact_payload_servercargo test -p codex-core direct_mcp_tools_register_namespaced_handlerscargo test -p codex-core search_tool_description_lists_each_mcp_source_oncecargo test -p codex-mcp list_all_tools_uses_startup_snapshot_while_client_is_pendingjust fix -p codex-core -p codex-mcp -p codex-otel