From 0df7e9a8207296e6b7c967e22a4fee48af35aa09 Mon Sep 17 00:00:00 2001 From: sayan-oai Date: Wed, 15 Apr 2026 21:02:59 +0800 Subject: [PATCH] register all mcp tools with namespace (#17404) stacked on #17402. MCP tools returned by `tool_search` (deferred tools) get registered in our `ToolRegistry` with a different format than directly available tools. this leads to two different ways of accessing MCP tools from our tool catalog, only one of which works for each. fix this by registering all MCP tools with the namespace format, since this info is already available. also, direct MCP tools are registered to responsesapi without a namespace, while deferred MCP tools have a namespace. this means we can receive MCP `FunctionCall`s in both formats from namespaces. fix this by always registering MCP tools with namespace, regardless of deferral status. make code mode track `ToolName` provenance of tools so it can map the literal JS function name string to the correct `ToolName` for invocation, rather than supporting both in core. this lets us unify to a single canonical `ToolName` representation for each MCP tool and force everywhere to use that one, without supporting fallbacks. --- codex-rs/Cargo.lock | 1 + .../tests/suite/v2/mcp_server_elicitation.rs | 8 +- codex-rs/code-mode/Cargo.toml | 1 + codex-rs/code-mode/src/description.rs | 52 ++-- codex-rs/code-mode/src/runtime/callbacks.rs | 24 +- codex-rs/code-mode/src/runtime/globals.rs | 8 +- codex-rs/code-mode/src/runtime/mod.rs | 5 +- codex-rs/code-mode/src/service.rs | 3 +- .../codex-mcp/src/mcp_connection_manager.rs | 20 +- .../src/mcp_connection_manager_tests.rs | 37 +++ codex-rs/core/src/codex.rs | 9 +- codex-rs/core/src/codex_tests.rs | 12 +- codex-rs/core/src/mcp_tool_exposure.rs | 6 +- codex-rs/core/src/tools/code_mode/mod.rs | 57 +++-- codex-rs/core/src/tools/js_repl/mod.rs | 78 ++++-- codex-rs/core/src/tools/parallel.rs | 2 +- codex-rs/core/src/tools/router.rs | 63 +++-- codex-rs/core/src/tools/spec.rs | 30 ++- codex-rs/core/src/tools/spec_tests.rs | 242 ++++++++++++------ codex-rs/core/tests/common/responses.rs | 47 ++++ codex-rs/core/tests/suite/code_mode.rs | 45 +++- codex-rs/core/tests/suite/js_repl.rs | 81 ++++++ codex-rs/core/tests/suite/openai_file_mcp.rs | 30 +-- codex-rs/core/tests/suite/plugins.rs | 37 +-- codex-rs/core/tests/suite/rmcp_client.rs | 102 ++++++-- codex-rs/core/tests/suite/search_tool.rs | 62 ++++- codex-rs/core/tests/suite/sqlite_state.rs | 9 +- codex-rs/core/tests/suite/truncation.rs | 22 +- codex-rs/protocol/src/lib.rs | 2 + codex-rs/{tools => protocol}/src/tool_name.rs | 22 +- codex-rs/tools/src/code_mode.rs | 111 ++++++-- codex-rs/tools/src/code_mode_tests.rs | 3 + codex-rs/tools/src/lib.rs | 4 +- codex-rs/tools/src/responses_api.rs | 11 +- codex-rs/tools/src/responses_api_tests.rs | 13 +- codex-rs/tools/src/tool_discovery.rs | 4 +- codex-rs/tools/src/tool_registry_plan.rs | 118 +++++---- .../tools/src/tool_registry_plan_tests.rs | 144 +++++++---- .../tools/src/tool_registry_plan_types.rs | 17 +- codex-rs/tools/src/tool_spec.rs | 4 + codex-rs/tools/src/tool_spec_tests.rs | 56 ++++ 41 files changed, 1170 insertions(+), 432 deletions(-) rename codex-rs/{tools => protocol}/src/tool_name.rs (62%) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 381ea3a11..3a99c2aa6 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1831,6 +1831,7 @@ name = "codex-code-mode" version = "0.0.0" dependencies = [ "async-trait", + "codex-protocol", "deno_core_icudata", "pretty_assertions", "serde", diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs index 862bdf848..13ebe0b99 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs @@ -66,8 +66,9 @@ use tokio::time::timeout; const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); const CONNECTOR_ID: &str = "calendar"; const CONNECTOR_NAME: &str = "Calendar"; +const TOOL_NAMESPACE: &str = "mcp__codex_apps__calendar"; +const CALLABLE_TOOL_NAME: &str = "_confirm_action"; const TOOL_NAME: &str = "calendar_confirm_action"; -const QUALIFIED_TOOL_NAME: &str = "mcp__codex_apps__calendar_confirm_action"; const TOOL_CALL_ID: &str = "call-calendar-confirm"; const ELICITATION_MESSAGE: &str = "Allow this request?"; @@ -85,9 +86,10 @@ async fn mcp_server_elicitation_round_trip() -> Result<()> { ]), responses::sse(vec![ responses::ev_response_created("resp-1"), - responses::ev_function_call( + responses::ev_function_call_with_namespace( TOOL_CALL_ID, - QUALIFIED_TOOL_NAME, + TOOL_NAMESPACE, + CALLABLE_TOOL_NAME, &tool_call_arguments, ), responses::ev_completed("resp-1"), diff --git a/codex-rs/code-mode/Cargo.toml b/codex-rs/code-mode/Cargo.toml index fa7ce63a4..d2f42359d 100644 --- a/codex-rs/code-mode/Cargo.toml +++ b/codex-rs/code-mode/Cargo.toml @@ -14,6 +14,7 @@ workspace = true [dependencies] async-trait = { workspace = true } +codex-protocol = { workspace = true } deno_core_icudata = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } diff --git a/codex-rs/code-mode/src/description.rs b/codex-rs/code-mode/src/description.rs index ac18fafd8..1aee1f7a3 100644 --- a/codex-rs/code-mode/src/description.rs +++ b/codex-rs/code-mode/src/description.rs @@ -1,3 +1,4 @@ +use codex_protocol::ToolName; use serde::Deserialize; use serde::Serialize; use serde_json::Value as JsonValue; @@ -129,6 +130,7 @@ pub enum CodeModeToolKind { #[derive(Clone, Debug, PartialEq)] pub struct ToolDefinition { pub name: String, + pub tool_name: ToolName, pub description: String, pub kind: CodeModeToolKind, pub input_schema: Option, @@ -269,11 +271,15 @@ pub fn build_exec_tool_description( for tool in enabled_tools { let name = tool.name.as_str(); let nested_description = render_code_mode_sample_for_definition(tool); - let next_namespace = namespace_descriptions - .get(name) + let namespace_description = tool + .tool_name + .namespace + .as_ref() + .and_then(|namespace| namespace_descriptions.get(namespace)); + let next_namespace = namespace_description .map(|namespace_description| namespace_description.name.as_str()); if next_namespace != current_namespace { - if let Some(namespace_description) = namespace_descriptions.get(name) { + if let Some(namespace_description) = namespace_description { let namespace_description_text = namespace_description.description.trim(); if !namespace_description_text.is_empty() { nested_tool_sections.push(format!( @@ -346,7 +352,7 @@ pub fn augment_tool_definition(mut definition: ToolDefinition) -> ToolDefinition pub fn enabled_tool_metadata(definition: &ToolDefinition) -> EnabledToolMetadata { EnabledToolMetadata { - tool_name: definition.name.clone(), + tool_name: definition.tool_name.clone(), global_name: normalize_code_mode_identifier(&definition.name), description: definition.description.clone(), kind: definition.kind, @@ -355,7 +361,7 @@ pub fn enabled_tool_metadata(definition: &ToolDefinition) -> EnabledToolMetadata #[derive(Clone, Debug, Eq, PartialEq, Serialize)] pub struct EnabledToolMetadata { - pub tool_name: String, + pub tool_name: ToolName, pub global_name: String, pub description: String, pub kind: CodeModeToolKind, @@ -706,6 +712,7 @@ mod tests { use super::build_exec_tool_description; use super::normalize_code_mode_identifier; use super::parse_exec_source; + use codex_protocol::ToolName; use pretty_assertions::assert_eq; use serde_json::Value as JsonValue; use serde_json::json; @@ -770,6 +777,7 @@ mod tests { fn augment_tool_definition_appends_typed_declaration() { let definition = ToolDefinition { name: "hidden_dynamic_tool".to_string(), + tool_name: ToolName::plain("hidden_dynamic_tool"), description: "Test tool".to_string(), kind: CodeModeToolKind::Function, input_schema: Some(json!({ @@ -798,6 +806,7 @@ mod tests { fn augment_tool_definition_includes_property_descriptions_as_comments() { let definition = ToolDefinition { name: "weather_tool".to_string(), + tool_name: ToolName::plain("weather_tool"), description: "Weather tool".to_string(), kind: CodeModeToolKind::Function, input_schema: Some(json!({ @@ -846,6 +855,7 @@ mod tests { let description = build_exec_tool_description( &[ToolDefinition { name: "foo".to_string(), + tool_name: ToolName::plain("foo"), description: "bar".to_string(), kind: CodeModeToolKind::Function, input_schema: None, @@ -870,26 +880,18 @@ bar" #[test] fn code_mode_only_description_groups_namespace_instructions_once() { - let namespace_descriptions = BTreeMap::from([ - ( - "mcp__sample__alpha".to_string(), - ToolNamespaceDescription { - name: "mcp__sample".to_string(), - description: "Shared namespace guidance.".to_string(), - }, - ), - ( - "mcp__sample__beta".to_string(), - ToolNamespaceDescription { - name: "mcp__sample".to_string(), - description: "Shared namespace guidance.".to_string(), - }, - ), - ]); + let namespace_descriptions = BTreeMap::from([( + "mcp__sample__".to_string(), + ToolNamespaceDescription { + name: "mcp__sample".to_string(), + description: "Shared namespace guidance.".to_string(), + }, + )]); let description = build_exec_tool_description( &[ ToolDefinition { name: "mcp__sample__alpha".to_string(), + tool_name: ToolName::namespaced("mcp__sample__", "alpha"), description: "First tool".to_string(), kind: CodeModeToolKind::Function, input_schema: Some(json!({ @@ -905,6 +907,7 @@ bar" }, ToolDefinition { name: "mcp__sample__beta".to_string(), + tool_name: ToolName::namespaced("mcp__sample__", "beta"), description: "Second tool".to_string(), kind: CodeModeToolKind::Function, input_schema: Some(json!({ @@ -935,7 +938,7 @@ bar" #[test] fn code_mode_only_description_omits_empty_namespace_sections() { let namespace_descriptions = BTreeMap::from([( - "mcp__sample__alpha".to_string(), + "mcp__sample__".to_string(), ToolNamespaceDescription { name: "mcp__sample".to_string(), description: String::new(), @@ -944,6 +947,7 @@ bar" let description = build_exec_tool_description( &[ToolDefinition { name: "mcp__sample__alpha".to_string(), + tool_name: ToolName::namespaced("mcp__sample__", "alpha"), description: "First tool".to_string(), kind: CodeModeToolKind::Function, input_schema: Some(json!({ @@ -969,6 +973,7 @@ bar" fn code_mode_only_description_renders_shared_mcp_types_once() { let first_tool = augment_tool_definition(ToolDefinition { name: "mcp__sample__alpha".to_string(), + tool_name: ToolName::namespaced("mcp__sample__", "alpha"), description: "First tool".to_string(), kind: CodeModeToolKind::Function, input_schema: Some(json!({ @@ -1002,6 +1007,7 @@ bar" }); let second_tool = augment_tool_definition(ToolDefinition { name: "mcp__sample__beta".to_string(), + tool_name: ToolName::namespaced("mcp__sample__", "beta"), description: "Second tool".to_string(), kind: CodeModeToolKind::Function, input_schema: Some(json!({ @@ -1038,6 +1044,7 @@ bar" &[ ToolDefinition { name: first_tool.name, + tool_name: first_tool.tool_name, description: "First tool".to_string(), kind: first_tool.kind, input_schema: first_tool.input_schema, @@ -1045,6 +1052,7 @@ bar" }, ToolDefinition { name: second_tool.name, + tool_name: second_tool.tool_name, description: "Second tool".to_string(), kind: second_tool.kind, input_schema: second_tool.input_schema, diff --git a/codex-rs/code-mode/src/runtime/callbacks.rs b/codex-rs/code-mode/src/runtime/callbacks.rs index 5511baca2..ace6f6114 100644 --- a/codex-rs/code-mode/src/runtime/callbacks.rs +++ b/codex-rs/code-mode/src/runtime/callbacks.rs @@ -15,7 +15,13 @@ pub(super) fn tool_callback( args: v8::FunctionCallbackArguments, mut retval: v8::ReturnValue, ) { - let tool_name = args.data().to_rust_string_lossy(scope); + let tool_index = match args.data().to_rust_string_lossy(scope).parse::() { + Ok(tool_index) => tool_index, + Err(_) => { + throw_type_error(scope, "invalid tool callback data"); + return; + } + }; let input = if args.length() == 0 { Ok(None) } else { @@ -36,6 +42,22 @@ pub(super) fn tool_callback( let promise = resolver.get_promise(scope); let resolver = v8::Global::new(scope, resolver); + let tool_name = { + let Some(state) = scope.get_slot::() else { + throw_type_error(scope, "runtime state unavailable"); + return; + }; + let Some(tool_name) = state + .enabled_tools + .get(tool_index) + .map(|tool| tool.tool_name.clone()) + else { + throw_type_error(scope, "tool callback data is out of range"); + return; + }; + tool_name + }; + let Some(state) = scope.get_slot_mut::() else { throw_type_error(scope, "runtime state unavailable"); return; diff --git a/codex-rs/code-mode/src/runtime/globals.rs b/codex-rs/code-mode/src/runtime/globals.rs index 2d419db90..b40136c44 100644 --- a/codex-rs/code-mode/src/runtime/globals.rs +++ b/codex-rs/code-mode/src/runtime/globals.rs @@ -53,10 +53,10 @@ fn build_tools_object<'s>( .map(|state| state.enabled_tools.clone()) .unwrap_or_default(); - for tool in enabled_tools { + for (tool_index, tool) in enabled_tools.iter().enumerate() { let name = v8::String::new(scope, &tool.global_name) .ok_or_else(|| "failed to allocate tool name".to_string())?; - let function = tool_function(scope, &tool.tool_name)?; + let function = tool_function(scope, tool_index)?; tools.set(scope, name.into(), function.into()); } Ok(tools) @@ -116,9 +116,9 @@ where fn tool_function<'s>( scope: &mut v8::PinScope<'s, '_>, - tool_name: &str, + tool_index: usize, ) -> Result, String> { - let data = v8::String::new(scope, tool_name) + let data = v8::String::new(scope, &tool_index.to_string()) .ok_or_else(|| "failed to allocate tool callback data".to_string())?; let template = v8::FunctionTemplate::builder(tool_callback) .data(data.into()) diff --git a/codex-rs/code-mode/src/runtime/mod.rs b/codex-rs/code-mode/src/runtime/mod.rs index febe54216..0f50edd32 100644 --- a/codex-rs/code-mode/src/runtime/mod.rs +++ b/codex-rs/code-mode/src/runtime/mod.rs @@ -9,6 +9,7 @@ use std::sync::OnceLock; use std::sync::mpsc as std_mpsc; use std::thread; +use codex_protocol::ToolName; use serde_json::Value as JsonValue; use tokio::sync::mpsc; @@ -62,7 +63,7 @@ pub(crate) enum TurnMessage { ToolCall { cell_id: String, id: String, - name: String, + name: ToolName, input: Option, }, Notify { @@ -87,7 +88,7 @@ pub(crate) enum RuntimeEvent { YieldRequested, ToolCall { id: String, - name: String, + name: ToolName, input: Option, }, Notify { diff --git a/codex-rs/code-mode/src/service.rs b/codex-rs/code-mode/src/service.rs index c14fee195..8a3788013 100644 --- a/codex-rs/code-mode/src/service.rs +++ b/codex-rs/code-mode/src/service.rs @@ -5,6 +5,7 @@ use std::sync::atomic::Ordering; use std::time::Duration; use async_trait::async_trait; +use codex_protocol::ToolName; use serde_json::Value as JsonValue; use tokio::sync::Mutex; use tokio::sync::mpsc; @@ -26,7 +27,7 @@ use crate::runtime::spawn_runtime; pub trait CodeModeTurnHost: Send + Sync { async fn invoke_tool( &self, - tool_name: String, + tool_name: ToolName, input: Option, cancellation_token: CancellationToken, ) -> Result; diff --git a/codex-rs/codex-mcp/src/mcp_connection_manager.rs b/codex-rs/codex-mcp/src/mcp_connection_manager.rs index 9f9483053..fadb07d58 100644 --- a/codex-rs/codex-mcp/src/mcp_connection_manager.rs +++ b/codex-rs/codex-mcp/src/mcp_connection_manager.rs @@ -36,6 +36,7 @@ use codex_async_utils::CancelErr; use codex_async_utils::OrCancelExt; use codex_config::Constrained; use codex_config::types::OAuthCredentialsStoreMode; +use codex_protocol::ToolName; use codex_protocol::approvals::ElicitationRequest; use codex_protocol::approvals::ElicitationRequestEvent; use codex_protocol::mcp::CallToolResult; @@ -155,6 +156,12 @@ pub struct ToolInfo { pub connector_description: Option, } +impl ToolInfo { + pub fn canonical_tool_name(&self) -> ToolName { + ToolName::namespaced(self.callable_namespace.clone(), self.callable_name.clone()) + } +} + const META_OPENAI_FILE_PARAMS: &str = "openai/fileParams"; pub fn declared_openai_file_input_param_names( @@ -1206,14 +1213,11 @@ impl McpConnectionManager { .with_context(|| format!("resources/read failed for `{server}` ({uri})")) } - pub async fn resolve_tool_info(&self, name: &str, namespace: Option<&str>) -> Option { - let qualified_name = match namespace { - Some(namespace) if name.starts_with(namespace) => name.to_string(), - Some(namespace) => format!("{namespace}{name}"), - None => name.to_string(), - }; - - self.list_all_tools().await.get(&qualified_name).cloned() + pub async fn resolve_tool_info(&self, tool_name: &ToolName) -> Option { + let all_tools = self.list_all_tools().await; + all_tools + .into_values() + .find(|tool| tool.canonical_tool_name() == *tool_name) } pub async fn notify_sandbox_state_change(&self, sandbox_state: &SandboxState) -> Result<()> { diff --git a/codex-rs/codex-mcp/src/mcp_connection_manager_tests.rs b/codex-rs/codex-mcp/src/mcp_connection_manager_tests.rs index bfa50c418..663f76d5d 100644 --- a/codex-rs/codex-mcp/src/mcp_connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/mcp_connection_manager_tests.rs @@ -1,4 +1,5 @@ use super::*; +use codex_protocol::ToolName; use codex_protocol::protocol::GranularApprovalConfig; use codex_protocol::protocol::McpAuthStatus; use pretty_assertions::assert_eq; @@ -646,6 +647,42 @@ async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() { assert_eq!(tool.callable_name, "calendar_create_event"); } +#[tokio::test] +async fn resolve_tool_info_accepts_canonical_namespaced_tool_names() { + let startup_tools = vec![create_test_tool("rmcp", "echo")]; + let pending_client = futures::future::pending::>() + .boxed() + .shared(); + let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); + let sandbox_policy = Constrained::allow_any(SandboxPolicy::new_read_only_policy()); + let mut manager = McpConnectionManager::new_uninitialized(&approval_policy, &sandbox_policy); + 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()), + }, + ); + + let tool = manager + .resolve_tool_info(&ToolName::namespaced("mcp__rmcp__", "echo")) + .await + .expect("split MCP tool namespace and 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::>() diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 95e932b1d..c063a5c97 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -91,6 +91,7 @@ use codex_otel::current_span_trace_id; use codex_otel::current_span_w3c_trace_context; use codex_otel::set_parent_from_w3c_trace_context; use codex_protocol::ThreadId; +use codex_protocol::ToolName; use codex_protocol::approvals::ElicitationRequestEvent; use codex_protocol::approvals::ExecPolicyAmendment; use codex_protocol::approvals::NetworkPolicyAmendment; @@ -4453,16 +4454,12 @@ impl Session { .await } - pub(crate) async fn resolve_mcp_tool_info( - &self, - name: &str, - namespace: Option<&str>, - ) -> Option { + pub(crate) async fn resolve_mcp_tool_info(&self, tool_name: &ToolName) -> Option { self.services .mcp_connection_manager .read() .await - .resolve_tool_info(name, namespace) + .resolve_tool_info(tool_name) .await } diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 6de8909e6..0ff4ea5a2 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -904,7 +904,7 @@ fn mcp_tool_exposure_searches_large_effective_tool_sets() { } #[test] -fn mcp_tool_exposure_directly_exposes_explicit_apps_in_large_search_sets() { +fn mcp_tool_exposure_directly_exposes_explicit_apps_without_deferred_overlap() { let config = test_config(); let tools_config = tools_config_for_mcp_tool_exposure(/*search_tool*/ true); let mut mcp_tools = numbered_mcp_tools(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD - 1); @@ -935,13 +935,19 @@ fn mcp_tool_exposure_directly_exposes_explicit_apps_in_large_search_sets() { ); assert_eq!( exposure.deferred_tools.as_ref().map(HashMap::len), - Some(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD) + Some(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD - 1) ); let deferred_tools = exposure .deferred_tools .as_ref() .expect("large tool sets should be discoverable through tool_search"); - assert!(deferred_tools.contains_key("mcp__codex_apps__calendar_create_event")); + assert!( + tool_names + .iter() + .all(|direct_tool_name| !deferred_tools.contains_key(direct_tool_name)), + "direct tools should not also be deferred: {tool_names:?}" + ); + assert!(!deferred_tools.contains_key("mcp__codex_apps__calendar_create_event")); assert!(deferred_tools.contains_key("mcp__rmcp__tool_0")); } diff --git a/codex-rs/core/src/mcp_tool_exposure.rs b/codex-rs/core/src/mcp_tool_exposure.rs index d7858c17f..16aa50cfd 100644 --- a/codex-rs/core/src/mcp_tool_exposure.rs +++ b/codex-rs/core/src/mcp_tool_exposure.rs @@ -41,9 +41,13 @@ pub(crate) fn build_mcp_tool_exposure( let direct_tools = filter_codex_apps_mcp_tools(all_mcp_tools, explicitly_enabled_connectors, config); + for direct_tool_name in direct_tools.keys() { + deferred_tools.remove(direct_tool_name); + } + McpToolExposure { direct_tools, - deferred_tools: Some(deferred_tools), + deferred_tools: (!deferred_tools.is_empty()).then_some(deferred_tools), } } diff --git a/codex-rs/core/src/tools/code_mode/mod.rs b/codex-rs/core/src/tools/code_mode/mod.rs index de3a1fa33..0004ec6f5 100644 --- a/codex-rs/core/src/tools/code_mode/mod.rs +++ b/codex-rs/core/src/tools/code_mode/mod.rs @@ -116,7 +116,7 @@ struct CoreTurnHost { impl CodeModeTurnHost for CoreTurnHost { async fn invoke_tool( &self, - tool_name: String, + tool_name: ToolName, input: Option, cancellation_token: CancellationToken, ) -> Result { @@ -288,38 +288,39 @@ async fn build_nested_router(exec: &ExecContext) -> ToolRouter { async fn call_nested_tool( exec: ExecContext, tool_runtime: ToolCallRuntime, - tool_name: String, + tool_name: ToolName, input: Option, cancellation_token: CancellationToken, ) -> Result { - if tool_name == PUBLIC_TOOL_NAME { + if tool_name.namespace.is_none() && tool_name.name == PUBLIC_TOOL_NAME { return Err(FunctionCallError::RespondToModel(format!( "{PUBLIC_TOOL_NAME} cannot invoke itself" ))); } - let payload = if let Some(tool_info) = exec - .session - .resolve_mcp_tool_info(&tool_name, /*namespace*/ None) - .await - { - match serialize_function_tool_arguments(&tool_name, input) { - Ok(raw_arguments) => ToolPayload::Mcp { - server: tool_info.server_name, - tool: tool_info.tool.name.to_string(), - raw_arguments, - }, - Err(error) => return Err(FunctionCallError::RespondToModel(error)), - } - } else { - match build_nested_tool_payload(tool_runtime.find_spec(&tool_name), &tool_name, input) { - Ok(payload) => payload, - Err(error) => return Err(FunctionCallError::RespondToModel(error)), - } - }; + let (tool_call_name, payload) = + if let Some(tool_info) = exec.session.resolve_mcp_tool_info(&tool_name).await { + let raw_arguments = match serialize_function_tool_arguments(&tool_name, input) { + Ok(raw_arguments) => raw_arguments, + Err(error) => return Err(FunctionCallError::RespondToModel(error)), + }; + ( + tool_info.canonical_tool_name(), + ToolPayload::Mcp { + server: tool_info.server_name, + tool: tool_info.tool.name.to_string(), + raw_arguments, + }, + ) + } else { + match build_nested_tool_payload(tool_runtime.find_spec(&tool_name), &tool_name, input) { + Ok(payload) => (tool_name, payload), + Err(error) => return Err(FunctionCallError::RespondToModel(error)), + } + }; let call = ToolCall { - tool_name: ToolName::plain(tool_name.clone()), + tool_name: tool_call_name, call_id: format!("{PUBLIC_TOOL_NAME}-{}", uuid::Uuid::new_v4()), payload, }; @@ -339,7 +340,7 @@ fn tool_kind_for_spec(spec: &ToolSpec) -> codex_code_mode::CodeModeToolKind { fn tool_kind_for_name( spec: Option, - tool_name: &str, + tool_name: &ToolName, ) -> Result { spec.as_ref() .map(tool_kind_for_spec) @@ -348,7 +349,7 @@ fn tool_kind_for_name( fn build_nested_tool_payload( spec: Option, - tool_name: &str, + tool_name: &ToolName, input: Option, ) -> Result { let actual_kind = tool_kind_for_name(spec, tool_name)?; @@ -363,7 +364,7 @@ fn build_nested_tool_payload( } fn build_function_tool_payload( - tool_name: &str, + tool_name: &ToolName, input: Option, ) -> Result { let arguments = serialize_function_tool_arguments(tool_name, input)?; @@ -371,7 +372,7 @@ fn build_function_tool_payload( } fn serialize_function_tool_arguments( - tool_name: &str, + tool_name: &ToolName, input: Option, ) -> Result { match input { @@ -385,7 +386,7 @@ fn serialize_function_tool_arguments( } fn build_freeform_tool_payload( - tool_name: &str, + tool_name: &ToolName, input: Option, ) -> Result { match input { diff --git a/codex-rs/core/src/tools/js_repl/mod.rs b/codex-rs/core/src/tools/js_repl/mod.rs index b72800949..92bd4c610 100644 --- a/codex-rs/core/src/tools/js_repl/mod.rs +++ b/codex-rs/core/src/tools/js_repl/mod.rs @@ -45,6 +45,8 @@ use codex_sandboxing::SandboxCommand; use codex_sandboxing::SandboxManager; use codex_sandboxing::SandboxTransformRequest; use codex_sandboxing::SandboxablePreference; +use codex_tools::ResponsesApiNamespaceTool; +use codex_tools::ToolName; use codex_tools::ToolSpec; use codex_utils_output_truncation::TruncationPolicy; use codex_utils_output_truncation::truncate_text; @@ -1574,29 +1576,67 @@ impl JsReplManager { }, ); - let payload = if let Some(tool_info) = exec + let specs = router.specs(); + let requested_tool_name = specs + .iter() + .find_map(|spec| match spec { + ToolSpec::Function(tool) if tool.name == req.tool_name => { + Some(ToolName::plain(req.tool_name.clone())) + } + ToolSpec::Freeform(tool) if tool.name == req.tool_name => { + Some(ToolName::plain(req.tool_name.clone())) + } + ToolSpec::Namespace(namespace) => { + namespace.tools.iter().find_map(|tool| match tool { + ResponsesApiNamespaceTool::Function(tool) => { + let tool_name = + ToolName::namespaced(namespace.name.clone(), tool.name.clone()); + (tool_name.display() == req.tool_name).then_some(tool_name) + } + }) + } + ToolSpec::LocalShell {} + | ToolSpec::ImageGeneration { .. } + | ToolSpec::ToolSearch { .. } + | ToolSpec::WebSearch { .. } + | ToolSpec::Function(_) + | ToolSpec::Freeform(_) => None, + }) + .unwrap_or_else(|| ToolName::plain(req.tool_name.clone())); + let (tool_call_name, payload) = if let Some(tool_info) = exec .session - .resolve_mcp_tool_info(&req.tool_name, /*namespace*/ None) + .resolve_mcp_tool_info(&requested_tool_name) .await { - crate::tools::context::ToolPayload::Mcp { - server: tool_info.server_name, - tool: tool_info.tool.name.to_string(), - raw_arguments: req.arguments.clone(), - } - } else if is_freeform_tool(&router.specs(), &req.tool_name) { - crate::tools::context::ToolPayload::Custom { - input: req.arguments.clone(), - } + ( + tool_info.canonical_tool_name(), + crate::tools::context::ToolPayload::Mcp { + server: tool_info.server_name, + tool: tool_info.tool.name.to_string(), + raw_arguments: req.arguments.clone(), + }, + ) + } else if matches!( + router.find_spec(&requested_tool_name), + Some(ToolSpec::Freeform(_)) + ) { + ( + requested_tool_name, + crate::tools::context::ToolPayload::Custom { + input: req.arguments.clone(), + }, + ) } else { - crate::tools::context::ToolPayload::Function { - arguments: req.arguments.clone(), - } + ( + requested_tool_name, + crate::tools::context::ToolPayload::Function { + arguments: req.arguments.clone(), + }, + ) }; - let tool_name = req.tool_name.clone(); let call = crate::tools::router::ToolCall { - tool_name: codex_tools::ToolName::plain(tool_name.clone()), + tool_name: tool_call_name, call_id: req.id.clone(), payload, }; @@ -1755,12 +1795,6 @@ fn split_exec_result_content_items( } } -fn is_freeform_tool(specs: &[ToolSpec], name: &str) -> bool { - specs - .iter() - .any(|spec| spec.name() == name && matches!(spec, ToolSpec::Freeform(_))) -} - fn is_js_repl_internal_tool(name: &str) -> bool { matches!(name, "js_repl" | "js_repl_reset") } diff --git a/codex-rs/core/src/tools/parallel.rs b/codex-rs/core/src/tools/parallel.rs index a1965db5c..b4dbfaa6c 100644 --- a/codex-rs/core/src/tools/parallel.rs +++ b/codex-rs/core/src/tools/parallel.rs @@ -48,7 +48,7 @@ impl ToolCallRuntime { } } - pub(crate) fn find_spec(&self, tool_name: &str) -> Option { + pub(crate) fn find_spec(&self, tool_name: &codex_tools::ToolName) -> Option { self.router.find_spec(tool_name) } diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index 645650525..183b9fb41 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -16,6 +16,7 @@ use codex_protocol::models::SearchToolCallParams; use codex_protocol::models::ShellToolCallParams; use codex_tools::ConfiguredToolSpec; use codex_tools::DiscoverableTool; +use codex_tools::ResponsesApiNamespaceTool; use codex_tools::ToolName; use codex_tools::ToolSpec; use codex_tools::ToolsConfig; @@ -102,20 +103,48 @@ impl ToolRouter { self.model_visible_specs.clone() } - pub fn find_spec(&self, tool_name: &str) -> Option { - self.specs - .iter() - .find(|config| config.name() == tool_name) - .map(|config| config.spec.clone()) + pub fn find_spec(&self, tool_name: &ToolName) -> Option { + self.specs.iter().find_map(|config| match &config.spec { + ToolSpec::Function(tool) + if tool_name.namespace.is_none() && tool.name == tool_name.name => + { + Some(config.spec.clone()) + } + ToolSpec::Freeform(tool) + if tool_name.namespace.is_none() && tool.name == tool_name.name => + { + Some(config.spec.clone()) + } + ToolSpec::Namespace(namespace) => namespace.tools.iter().find_map(|tool| match tool { + ResponsesApiNamespaceTool::Function(tool) + if tool_name.namespace.as_deref() == Some(namespace.name.as_str()) + && tool.name == tool_name.name => + { + Some(ToolSpec::Function(tool.clone())) + } + _ => None, + }), + _ => None, + }) } fn configured_tool_supports_parallel(&self, tool_name: &ToolName) -> bool { - tool_name.namespace.is_none() - && self - .specs - .iter() - .filter(|config| config.supports_parallel_tool_calls) - .any(|config| config.name() == tool_name.name.as_str()) + if tool_name.namespace.is_some() { + return false; + } + + self.specs + .iter() + .filter(|config| config.supports_parallel_tool_calls) + .any(|config| match &config.spec { + ToolSpec::Function(tool) => tool.name == tool_name.name.as_str(), + ToolSpec::Freeform(tool) => tool.name == tool_name.name.as_str(), + ToolSpec::Namespace(_) + | ToolSpec::ToolSearch { .. } + | ToolSpec::LocalShell {} + | ToolSpec::ImageGeneration { .. } + | ToolSpec::WebSearch { .. } => false, + }) } pub fn tool_supports_parallel(&self, call: &ToolCall) -> bool { @@ -141,16 +170,10 @@ impl ToolRouter { call_id, .. } => { - let mcp_tool = session - .resolve_mcp_tool_info(&name, namespace.as_deref()) - .await; - let tool_name = match namespace { - Some(namespace) => ToolName::namespaced(namespace, name), - None => ToolName::plain(name), - }; - if let Some(tool_info) = mcp_tool { + let tool_name = ToolName::new(namespace, name); + if let Some(tool_info) = session.resolve_mcp_tool_info(&tool_name).await { Ok(Some(ToolCall { - tool_name, + tool_name: tool_info.canonical_tool_name(), call_id, payload: ToolPayload::Mcp { server: tool_info.server_name, diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 188be42ba..9c05e6cc3 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -11,6 +11,7 @@ use codex_tools::DiscoverableTool; use codex_tools::ToolHandlerKind; use codex_tools::ToolNamespace; use codex_tools::ToolRegistryPlanDeferredTool; +use codex_tools::ToolRegistryPlanMcpTool; use codex_tools::ToolRegistryPlanParams; use codex_tools::ToolUserShellType; use codex_tools::ToolsConfig; @@ -29,25 +30,31 @@ pub(crate) fn tool_user_shell_type(user_shell: &Shell) -> ToolUserShellType { } } -struct McpToolPlanInputs { - mcp_tools: HashMap, +struct McpToolPlanInputs<'a> { + mcp_tools: Vec>, tool_namespaces: HashMap, } -fn map_mcp_tools_for_plan(mcp_tools: &HashMap) -> McpToolPlanInputs { +fn map_mcp_tools_for_plan(mcp_tools: &HashMap) -> McpToolPlanInputs<'_> { McpToolPlanInputs { mcp_tools: mcp_tools - .iter() - .map(|(name, tool)| (name.clone(), tool.tool.clone())) + .values() + .map(|tool| ToolRegistryPlanMcpTool { + name: tool.canonical_tool_name(), + tool: &tool.tool, + }) .collect(), tool_namespaces: mcp_tools - .iter() - .map(|(name, tool)| { + .values() + .map(|tool| { ( - name.clone(), + tool.callable_namespace.clone(), ToolNamespace { name: tool.callable_namespace.clone(), - description: tool.server_instructions.clone(), + description: tool + .connector_description + .clone() + .or_else(|| tool.server_instructions.clone()), }, ) }) @@ -99,8 +106,7 @@ pub(crate) fn build_specs_with_discoverable_tools( tools .values() .map(|tool| ToolRegistryPlanDeferredTool { - tool_name: tool.callable_name.as_str(), - tool_namespace: tool.callable_namespace.as_str(), + name: tool.canonical_tool_name(), server_name: tool.server_name.as_str(), connector_name: tool.connector_name.as_deref(), connector_description: tool.connector_description.as_deref(), @@ -114,7 +120,7 @@ pub(crate) fn build_specs_with_discoverable_tools( ToolRegistryPlanParams { mcp_tools: mcp_tool_plan_inputs .as_ref() - .map(|inputs| &inputs.mcp_tools), + .map(|inputs| inputs.mcp_tools.as_slice()), deferred_mcp_tools: deferred_mcp_tool_sources.as_deref(), tool_namespaces: mcp_tool_plan_inputs .as_ref() diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index b0fe3f843..ee993b0d5 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -19,6 +19,7 @@ use codex_protocol::protocol::SessionSource; use codex_tools::ConfiguredToolSpec; use codex_tools::DiscoverableTool; use codex_tools::JsonSchema; +use codex_tools::ResponsesApiNamespaceTool; use codex_tools::ResponsesApiTool; use codex_tools::ShellCommandBackendConfig; use codex_tools::TOOL_SEARCH_TOOL_NAME; @@ -67,6 +68,25 @@ fn mcp_tool_info(tool: rmcp::model::Tool) -> ToolInfo { } } +fn mcp_tool_info_with_display_name(display_name: &str, tool: rmcp::model::Tool) -> ToolInfo { + let (callable_namespace, callable_name) = display_name + .rsplit_once('/') + .map(|(namespace, callable_name)| (format!("{namespace}/"), callable_name.to_string())) + .unwrap_or_else(|| ("".to_string(), display_name.to_string())); + + ToolInfo { + server_name: "test_server".to_string(), + callable_name, + callable_namespace, + server_instructions: None, + tool, + connector_id: None, + connector_name: None, + plugin_display_names: Vec::new(), + connector_description: None, + } +} + fn discoverable_connector(id: &str, name: &str, description: &str) -> DiscoverableTool { let slug = name.replace(' ', "-").to_lowercase(); DiscoverableTool::Connector(Box::new(AppInfo { @@ -109,8 +129,11 @@ fn deferred_responses_api_tool_serializes_with_defer_loading() { ); let serialized = serde_json::to_value(ToolSpec::Function( - mcp_tool_to_deferred_responses_api_tool("mcp__codex_apps__lookup_order".to_string(), &tool) - .expect("convert deferred tool"), + mcp_tool_to_deferred_responses_api_tool( + &ToolName::namespaced("mcp__codex_apps__", "lookup_order"), + &tool, + ) + .expect("convert deferred tool"), )) .expect("serialize deferred tool"); @@ -118,7 +141,7 @@ fn deferred_responses_api_tool_serializes_with_defer_loading() { serialized, serde_json::json!({ "type": "function", - "name": "mcp__codex_apps__lookup_order", + "name": "lookup_order", "description": "Look up an order", "strict": false, "defer_loading": true, @@ -173,6 +196,25 @@ fn find_tool<'a>(tools: &'a [ConfiguredToolSpec], expected_name: &str) -> &'a Co .unwrap_or_else(|| panic!("expected tool {expected_name}")) } +fn find_namespace_function_tool<'a>( + tools: &'a [ConfiguredToolSpec], + expected_namespace: &str, + expected_name: &str, +) -> &'a ResponsesApiTool { + let namespace_tool = find_tool(tools, expected_namespace); + let ToolSpec::Namespace(namespace) = &namespace_tool.spec else { + panic!("expected namespace tool {expected_namespace}"); + }; + namespace + .tools + .iter() + .find_map(|tool| match tool { + ResponsesApiNamespaceTool::Function(tool) if tool.name == expected_name => Some(tool), + _ => None, + }) + .unwrap_or_else(|| panic!("expected tool {expected_namespace}{expected_name} in namespace")) +} + fn multi_agent_v2_tools_config() -> ToolsConfig { let config = test_config(); let model_info = construct_model_info_offline("gpt-5-codex", &config); @@ -910,8 +952,43 @@ fn search_tool_registers_namespaced_mcp_tool_aliases() { assert!(registry.has_handler(&ToolName::plain(TOOL_SEARCH_TOOL_NAME))); assert!(registry.has_handler(&app_alias)); assert!(registry.has_handler(&mcp_alias)); - assert!(registry.has_handler(&ToolName::plain("mcp__codex_apps__calendar_create_event"))); - assert!(registry.has_handler(&ToolName::plain("mcp__rmcp__echo"))); +} + +#[test] +fn direct_mcp_tools_register_namespaced_handlers() { + let config = test_config(); + let model_info = construct_model_info_offline("gpt-5-codex", &config); + let mut features = Features::with_defaults(); + features.enable(Feature::UnifiedExec); + let available_models = Vec::new(); + let tools_config = ToolsConfig::new(&ToolsConfigParams { + model_info: &model_info, + available_models: &available_models, + features: &features, + image_generation_tool_auth_allowed: true, + web_search_mode: Some(WebSearchMode::Cached), + session_source: SessionSource::Cli, + sandbox_policy: &SandboxPolicy::DangerFullAccess, + windows_sandbox_level: WindowsSandboxLevel::Disabled, + }); + + let (_, registry) = build_specs( + &tools_config, + Some(HashMap::from([( + "mcp__test_server__echo".to_string(), + mcp_tool_info(mcp_tool( + "echo", + "Echo", + serde_json::json!({"type": "object"}), + )), + )])), + /*deferred_mcp_tools*/ None, + &[], + ) + .build(); + + assert!(registry.has_handler(&ToolName::namespaced("mcp__test_server__", "echo"))); + assert!(!registry.has_handler(&ToolName::plain("mcp__test_server__echo"))); } #[test] @@ -936,27 +1013,30 @@ fn test_mcp_tool_property_missing_type_defaults_to_string() { &tools_config, Some(HashMap::from([( "dash/search".to_string(), - mcp_tool_info(mcp_tool( - "search", - "Search docs", - serde_json::json!({ - "type": "object", - "properties": { - "query": {"description": "search query"} - } - }), - )), + mcp_tool_info_with_display_name( + "dash/search", + mcp_tool( + "search", + "Search docs", + serde_json::json!({ + "type": "object", + "properties": { + "query": {"description": "search query"} + } + }), + ), + ), )])), /*deferred_mcp_tools*/ None, &[], ) .build(); - let tool = find_tool(&tools, "dash/search"); + let tool = find_namespace_function_tool(&tools, "dash/", "search"); assert_eq!( - tool.spec, - ToolSpec::Function(ResponsesApiTool { - name: "dash/search".to_string(), + *tool, + ResponsesApiTool { + name: "search".to_string(), parameters: JsonSchema::object( /*properties*/ BTreeMap::from([( @@ -970,7 +1050,7 @@ fn test_mcp_tool_property_missing_type_defaults_to_string() { strict: false, output_schema: Some(mcp_call_tool_result_output_schema(serde_json::json!({}))), defer_loading: None, - }) + } ); } @@ -996,25 +1076,28 @@ fn test_mcp_tool_preserves_integer_schema() { &tools_config, Some(HashMap::from([( "dash/paginate".to_string(), - mcp_tool_info(mcp_tool( - "paginate", - "Pagination", - serde_json::json!({ - "type": "object", - "properties": {"page": {"type": "integer"}} - }), - )), + mcp_tool_info_with_display_name( + "dash/paginate", + mcp_tool( + "paginate", + "Pagination", + serde_json::json!({ + "type": "object", + "properties": {"page": {"type": "integer"}} + }), + ), + ), )])), /*deferred_mcp_tools*/ None, &[], ) .build(); - let tool = find_tool(&tools, "dash/paginate"); + let tool = find_namespace_function_tool(&tools, "dash/", "paginate"); assert_eq!( - tool.spec, - ToolSpec::Function(ResponsesApiTool { - name: "dash/paginate".to_string(), + *tool, + ResponsesApiTool { + name: "paginate".to_string(), parameters: JsonSchema::object( /*properties*/ BTreeMap::from([( @@ -1028,7 +1111,7 @@ fn test_mcp_tool_preserves_integer_schema() { strict: false, output_schema: Some(mcp_call_tool_result_output_schema(serde_json::json!({}))), defer_loading: None, - }) + } ); } @@ -1055,25 +1138,28 @@ fn test_mcp_tool_array_without_items_gets_default_string_items() { &tools_config, Some(HashMap::from([( "dash/tags".to_string(), - mcp_tool_info(mcp_tool( - "tags", - "Tags", - serde_json::json!({ - "type": "object", - "properties": {"tags": {"type": "array"}} - }), - )), + mcp_tool_info_with_display_name( + "dash/tags", + mcp_tool( + "tags", + "Tags", + serde_json::json!({ + "type": "object", + "properties": {"tags": {"type": "array"}} + }), + ), + ), )])), /*deferred_mcp_tools*/ None, &[], ) .build(); - let tool = find_tool(&tools, "dash/tags"); + let tool = find_namespace_function_tool(&tools, "dash/", "tags"); assert_eq!( - tool.spec, - ToolSpec::Function(ResponsesApiTool { - name: "dash/tags".to_string(), + *tool, + ResponsesApiTool { + name: "tags".to_string(), parameters: JsonSchema::object( /*properties*/ BTreeMap::from([( @@ -1090,7 +1176,7 @@ fn test_mcp_tool_array_without_items_gets_default_string_items() { strict: false, output_schema: Some(mcp_call_tool_result_output_schema(serde_json::json!({}))), defer_loading: None, - }) + } ); } @@ -1116,27 +1202,30 @@ fn test_mcp_tool_anyof_defaults_to_string() { &tools_config, Some(HashMap::from([( "dash/value".to_string(), - mcp_tool_info(mcp_tool( - "value", - "AnyOf Value", - serde_json::json!({ - "type": "object", - "properties": { - "value": {"anyOf": [{"type": "string"}, {"type": "number"}]} - } - }), - )), + mcp_tool_info_with_display_name( + "dash/value", + mcp_tool( + "value", + "AnyOf Value", + serde_json::json!({ + "type": "object", + "properties": { + "value": {"anyOf": [{"type": "string"}, {"type": "number"}]} + } + }), + ), + ), )])), /*deferred_mcp_tools*/ None, &[], ) .build(); - let tool = find_tool(&tools, "dash/value"); + let tool = find_namespace_function_tool(&tools, "dash/", "value"); assert_eq!( - tool.spec, - ToolSpec::Function(ResponsesApiTool { - name: "dash/value".to_string(), + *tool, + ResponsesApiTool { + name: "value".to_string(), parameters: JsonSchema::object( /*properties*/ BTreeMap::from([( @@ -1156,7 +1245,7 @@ fn test_mcp_tool_anyof_defaults_to_string() { strict: false, output_schema: Some(mcp_call_tool_result_output_schema(serde_json::json!({}))), defer_loading: None, - }) + } ); } @@ -1181,12 +1270,14 @@ fn test_get_openai_tools_mcp_tools_with_additional_properties_schema() { &tools_config, Some(HashMap::from([( "test_server/do_something_cool".to_string(), - mcp_tool_info(mcp_tool( - "do_something_cool", - "Do something cool", - serde_json::json!({ - "type": "object", - "properties": { + mcp_tool_info_with_display_name( + "test_server/do_something_cool", + mcp_tool( + "do_something_cool", + "Do something cool", + serde_json::json!({ + "type": "object", + "properties": { "string_argument": {"type": "string"}, "number_argument": {"type": "number"}, "object_argument": { @@ -1203,22 +1294,23 @@ fn test_get_openai_tools_mcp_tools_with_additional_properties_schema() { }, "required": ["addtl_prop"], "additionalProperties": false + } } } - } - }), - )), + }), + ), + ), )])), /*deferred_mcp_tools*/ None, &[], ) .build(); - let tool = find_tool(&tools, "test_server/do_something_cool"); + let tool = find_namespace_function_tool(&tools, "test_server/", "do_something_cool"); assert_eq!( - tool.spec, - ToolSpec::Function(ResponsesApiTool { - name: "test_server/do_something_cool".to_string(), + *tool, + ResponsesApiTool { + name: "do_something_cool".to_string(), parameters: JsonSchema::object( /*properties*/ BTreeMap::from([ @@ -1268,7 +1360,7 @@ fn test_get_openai_tools_mcp_tools_with_additional_properties_schema() { strict: false, output_schema: Some(mcp_call_tool_result_output_schema(serde_json::json!({}))), defer_loading: None, - }) + } ); } diff --git a/codex-rs/core/tests/common/responses.rs b/codex-rs/core/tests/common/responses.rs index 2e2155ebd..8def3ac6e 100644 --- a/codex-rs/core/tests/common/responses.rs +++ b/codex-rs/core/tests/common/responses.rs @@ -125,6 +125,10 @@ impl ResponsesRequest { self.body_json().to_string().contains(&json_fragment) } + pub fn tool_by_name(&self, namespace: &str, tool_name: &str) -> Option { + namespace_child_tool(&self.body_json(), namespace, tool_name).cloned() + } + pub fn instructions_text(&self) -> String { self.body_json()["instructions"] .as_str() @@ -315,6 +319,31 @@ pub(crate) fn output_value_to_text(value: &Value) -> Option { } } +pub fn namespace_child_tool<'a>( + body: &'a Value, + namespace: &str, + tool_name: &str, +) -> Option<&'a Value> { + let tools = body.get("tools")?.as_array()?; + for tool in tools { + if tool.get("name").and_then(Value::as_str) != Some(namespace) + || tool.get("type").and_then(Value::as_str) != Some("namespace") + { + continue; + } + + let child_tools = tool.get("tools")?.as_array()?; + if let Some(child_tool) = child_tools + .iter() + .find(|tool| tool.get("name").and_then(Value::as_str) == Some(tool_name)) + { + return Some(child_tool); + } + } + + None +} + #[cfg(test)] mod tests { use super::*; @@ -780,6 +809,24 @@ pub fn ev_function_call(call_id: &str, name: &str, arguments: &str) -> Value { }) } +pub fn ev_function_call_with_namespace( + call_id: &str, + namespace: &str, + name: &str, + arguments: &str, +) -> Value { + serde_json::json!({ + "type": "response.output_item.done", + "item": { + "type": "function_call", + "call_id": call_id, + "namespace": namespace, + "name": name, + "arguments": arguments + } + }) +} + pub fn ev_tool_search_call(call_id: &str, arguments: &serde_json::Value) -> Value { serde_json::json!({ "type": "response.output_item.done", diff --git a/codex-rs/core/tests/suite/code_mode.rs b/codex-rs/core/tests/suite/code_mode.rs index 76a70d507..f010c9e58 100644 --- a/codex-rs/core/tests/suite/code_mode.rs +++ b/codex-rs/core/tests/suite/code_mode.rs @@ -177,12 +177,25 @@ async fn run_code_mode_turn_with_rmcp( server: &MockServer, prompt: &str, code: &str, +) -> Result<(TestCodex, ResponseMock)> { + run_code_mode_turn_with_rmcp_mode(server, prompt, code, /*code_mode_only*/ false).await +} + +async fn run_code_mode_turn_with_rmcp_mode( + server: &MockServer, + prompt: &str, + code: &str, + code_mode_only: bool, ) -> Result<(TestCodex, ResponseMock)> { let rmcp_test_server_bin = stdio_server_bin()?; let mut builder = test_codex() .with_model("test-gpt-5.1-codex") .with_config(move |config| { - let _ = config.features.enable(Feature::CodeMode); + let _ = if code_mode_only { + config.features.enable(Feature::CodeModeOnly) + } else { + config.features.enable(Feature::CodeMode) + }; let mut servers = config.mcp_servers.get().clone(); servers.insert( @@ -1989,6 +2002,36 @@ contentLength=0" Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn code_mode_only_can_call_mcp_tool() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let code = r#" +const result = await tools.mcp__rmcp__echo({ message: "ping" }); +text(`echo=${result.structuredContent?.echo ?? "missing"}`); +"#; + + let (_test, second_mock) = run_code_mode_turn_with_rmcp_mode( + &server, + "use exec to run the rmcp echo tool in code mode only", + code, + /*code_mode_only*/ true, + ) + .await?; + + let req = second_mock.single_request(); + let (output, success) = custom_tool_output_body_and_success(&req, "call-1"); + assert_ne!( + success, + Some(false), + "code_mode_only rmcp tool call failed unexpectedly: {output}" + ); + assert_eq!(output, "echo=ECHOING: ping"); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn code_mode_exposes_mcp_tools_on_global_tools_object() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/core/tests/suite/js_repl.rs b/codex-rs/core/tests/suite/js_repl.rs index 5f52c24e1..2016897b3 100644 --- a/codex-rs/core/tests/suite/js_repl.rs +++ b/codex-rs/core/tests/suite/js_repl.rs @@ -1,6 +1,8 @@ #![allow(clippy::expect_used, clippy::unwrap_used)] use anyhow::Result; +use codex_config::types::McpServerConfig; +use codex_config::types::McpServerTransportConfig; use codex_features::Feature; use codex_protocol::protocol::EventMsg; use core_test_support::responses; @@ -12,12 +14,15 @@ use core_test_support::responses::ev_custom_tool_call; use core_test_support::responses::ev_response_created; use core_test_support::responses::sse; use core_test_support::skip_if_no_network; +use core_test_support::stdio_server_bin; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event_match; +use std::collections::HashMap; use std::fs; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; use std::path::Path; +use std::time::Duration; use tempfile::tempdir; use wiremock::MockServer; @@ -594,6 +599,82 @@ async fn js_repl_can_invoke_builtin_tools() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn js_repl_can_invoke_mcp_tools_by_display_name() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let rmcp_test_server_bin = stdio_server_bin()?; + let mut builder = test_codex().with_config(move |config| { + config + .features + .enable(Feature::JsRepl) + .expect("test config should allow feature update"); + + let mut servers = config.mcp_servers.get().clone(); + servers.insert( + "rmcp".to_string(), + McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: rmcp_test_server_bin, + args: Vec::new(), + env: None, + env_vars: Vec::new(), + cwd: None, + }, + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: Some(Duration::from_secs(10)), + tool_timeout_sec: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth_resource: None, + tools: HashMap::new(), + }, + ); + config + .mcp_servers + .set(servers) + .expect("test mcp servers should accept any configuration"); + }); + let test = builder.build(&server).await?; + + responses::mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-1"), + ev_custom_tool_call( + "call-1", + "js_repl", + r#" +const result = await codex.tool("mcp__rmcp__echo", { message: "ping" }); +console.log(result.output); +"#, + ), + ev_completed("resp-1"), + ]), + ) + .await; + let final_mock = responses::mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + ) + .await; + + test.submit_turn("use js_repl to call an MCP tool").await?; + + let req = final_mock.single_request(); + assert_js_repl_ok(&req, "call-1", "ECHOING: ping"); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn js_repl_tool_call_rejects_recursive_js_repl_invocation() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/core/tests/suite/openai_file_mcp.rs b/codex-rs/core/tests/suite/openai_file_mcp.rs index 134ad6dfd..211aa6738 100644 --- a/codex-rs/core/tests/suite/openai_file_mcp.rs +++ b/codex-rs/core/tests/suite/openai_file_mcp.rs @@ -10,7 +10,7 @@ use core_test_support::apps_test_server::AppsTestServer; use core_test_support::apps_test_server::DOCUMENT_EXTRACT_TEXT_RESOURCE_URI; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; -use core_test_support::responses::ev_function_call; +use core_test_support::responses::ev_function_call_with_namespace; use core_test_support::responses::ev_response_created; use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; @@ -26,7 +26,8 @@ use wiremock::matchers::header; use wiremock::matchers::method; use wiremock::matchers::path; -const DOCUMENT_EXTRACT_TOOL: &str = "mcp__codex_apps__calendar_extract_text"; +const DOCUMENT_EXTRACT_NAMESPACE: &str = "mcp__codex_apps__calendar"; +const DOCUMENT_EXTRACT_TOOL: &str = "_extract_text"; fn configure_apps(config: &mut Config, chatgpt_base_url: &str) { if let Err(err) = config.features.enable(Feature::Apps) { @@ -35,18 +36,6 @@ fn configure_apps(config: &mut Config, chatgpt_base_url: &str) { config.chatgpt_base_url = chatgpt_base_url.to_string(); } -fn tool_by_name<'a>(body: &'a Value, name: &str) -> &'a Value { - body.get("tools") - .and_then(Value::as_array) - .and_then(|tools| { - tools.iter().find(|tool| { - tool.get("name").and_then(Value::as_str) == Some(name) - || tool.get("type").and_then(Value::as_str) == Some(name) - }) - }) - .unwrap_or_else(|| panic!("missing tool {name} in /v1/responses request: {body:?}")) -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn codex_apps_file_params_upload_local_paths_before_mcp_tool_call() -> Result<()> { let server = start_mock_server().await; @@ -93,8 +82,9 @@ async fn codex_apps_file_params_upload_local_paths_before_mcp_tool_call() -> Res vec![ sse(vec![ ev_response_created("resp-1"), - ev_function_call( + ev_function_call_with_namespace( call_id, + DOCUMENT_EXTRACT_NAMESPACE, DOCUMENT_EXTRACT_TOOL, &json!({"file": "report.txt"}).to_string(), ), @@ -123,8 +113,14 @@ async fn codex_apps_file_params_upload_local_paths_before_mcp_tool_call() -> Res .await?; let requests = mock.requests(); - let body = requests[0].body_json(); - let extract_tool = tool_by_name(&body, DOCUMENT_EXTRACT_TOOL); + let Some(extract_tool) = + requests[0].tool_by_name(DOCUMENT_EXTRACT_NAMESPACE, DOCUMENT_EXTRACT_TOOL) + else { + let body = requests[0].body_json(); + panic!( + "missing tool {DOCUMENT_EXTRACT_NAMESPACE}{DOCUMENT_EXTRACT_TOOL} in /v1/responses request: {body:?}" + ) + }; assert_eq!( extract_tool.pointer("/parameters/properties/file"), Some(&json!({ diff --git a/codex-rs/core/tests/suite/plugins.rs b/codex-rs/core/tests/suite/plugins.rs index ab2c85ac0..b19385cd5 100644 --- a/codex-rs/core/tests/suite/plugins.rs +++ b/codex-rs/core/tests/suite/plugins.rs @@ -168,22 +168,6 @@ fn tool_names(body: &serde_json::Value) -> Vec { .unwrap_or_default() } -fn tool_description(body: &serde_json::Value, tool_name: &str) -> Option { - body.get("tools") - .and_then(serde_json::Value::as_array) - .and_then(|tools| { - tools.iter().find_map(|tool| { - if tool.get("name").and_then(serde_json::Value::as_str) == Some(tool_name) { - tool.get("description") - .and_then(serde_json::Value::as_str) - .map(str::to_string) - } else { - None - } - }) - }) -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn capability_sections_render_in_developer_message_in_order() -> Result<()> { skip_if_no_network!(Ok(())); @@ -319,20 +303,27 @@ async fn explicit_plugin_mentions_inject_plugin_guidance() -> Result<()> { assert!( request_tools .iter() - .any(|name| name == "mcp__codex_apps__google_calendar_create_event"), + .any(|name| name == "mcp__codex_apps__google_calendar"), "expected plugin app tools to become visible for this turn: {request_tools:?}" ); - let echo_description = tool_description(&request_body, "mcp__sample__echo") + let echo_tool = request + .tool_by_name("mcp__sample__", "echo") + .expect("plugin MCP tool should be present"); + let echo_description = echo_tool + .get("description") + .and_then(serde_json::Value::as_str) .expect("plugin MCP tool description should be present"); assert!( echo_description.contains("This tool is part of plugin `sample`."), "expected plugin MCP provenance in tool description: {echo_description:?}" ); - let calendar_description = tool_description( - &request_body, - "mcp__codex_apps__google_calendar_create_event", - ) - .expect("plugin app tool description should be present"); + let calendar_tool = request + .tool_by_name("mcp__codex_apps__google_calendar", "_create_event") + .expect("plugin app tool should be present"); + let calendar_description = calendar_tool + .get("description") + .and_then(serde_json::Value::as_str) + .expect("plugin app tool description should be present"); assert!( calendar_description.contains("This tool is part of plugin `sample`."), "expected plugin app provenance in tool description: {calendar_description:?}" diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 1dff2348b..b86094f91 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -91,13 +91,18 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> { let call_id = "call-123"; let server_name = "rmcp"; - let tool_name = format!("mcp__{server_name}__echo"); + let namespace = format!("mcp__{server_name}__"); - mount_sse_once( + let call_mock = mount_sse_once( &server, responses::sse(vec![ responses::ev_response_created("resp-1"), - responses::ev_function_call(call_id, &tool_name, "{\"message\":\"ping\"}"), + responses::ev_function_call_with_namespace( + call_id, + &namespace, + "echo", + "{\"message\":\"ping\"}", + ), responses::ev_completed("resp-1"), ]), ) @@ -223,6 +228,13 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> { wait_for_event(&fixture.codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; let output_item = final_mock.single_request().function_call_output(call_id); + let request = call_mock.single_request(); + assert!( + request.tool_by_name(&namespace, "echo").is_some(), + "direct MCP tool should be sent as a namespace child tool: {:?}", + request.body_json() + ); + let output_text = output_item .get("output") .and_then(Value::as_str) @@ -246,13 +258,14 @@ async fn stdio_mcp_tool_call_includes_sandbox_state_meta() -> anyhow::Result<()> let call_id = "sandbox-meta-call"; let server_name = "rmcp"; - let tool_name = format!("mcp__{server_name}__sandbox_meta"); + let namespace = format!("mcp__{server_name}__"); + let tool_name = format!("{namespace}sandbox_meta"); - mount_sse_once( + let call_mock = mount_sse_once( &server, responses::sse(vec![ responses::ev_response_created("resp-1"), - responses::ev_function_call(call_id, &tool_name, "{}"), + responses::ev_function_call_with_namespace(call_id, &namespace, "sandbox_meta", "{}"), responses::ev_completed("resp-1"), ]), ) @@ -301,11 +314,43 @@ async fn stdio_mcp_tool_call_includes_sandbox_state_meta() -> anyhow::Result<()> .build(&server) .await?; + let tools_ready_deadline = Instant::now() + Duration::from_secs(30); + loop { + fixture.codex.submit(Op::ListMcpTools).await?; + let list_event = wait_for_event_with_timeout( + &fixture.codex, + |ev| matches!(ev, EventMsg::McpListToolsResponse(_)), + Duration::from_secs(10), + ) + .await; + let EventMsg::McpListToolsResponse(tool_list) = list_event else { + unreachable!("event guard guarantees McpListToolsResponse"); + }; + if tool_list.tools.contains_key(&tool_name) { + break; + } + + let available_tools: Vec<&str> = tool_list.tools.keys().map(String::as_str).collect(); + if Instant::now() >= tools_ready_deadline { + panic!( + "timed out waiting for MCP tool {tool_name} to become available; discovered tools: {available_tools:?}" + ); + } + sleep(Duration::from_millis(200)).await; + } + let sandbox_policy = SandboxPolicy::new_read_only_policy(); fixture .submit_turn_with_policy("call the rmcp sandbox_meta tool", sandbox_policy.clone()) .await?; + let request = call_mock.single_request(); + assert!( + request.tool_by_name(&namespace, "sandbox_meta").is_some(), + "direct MCP tool should be sent as a namespace child tool: {:?}", + request.body_json() + ); + let output_item = final_mock.single_request().function_call_output(call_id); let output_text = output_item .get("output") @@ -346,15 +391,15 @@ async fn stdio_mcp_parallel_tool_calls_default_false_runs_serially() -> anyhow:: let first_call_id = "sync-serial-1"; let second_call_id = "sync-serial-2"; let server_name = "rmcp"; - let tool_name = format!("mcp__{server_name}__sync"); + let namespace = format!("mcp__{server_name}__"); let args = json!({ "sleep_after_ms": 100 }).to_string(); mount_sse_once( &server, responses::sse(vec![ responses::ev_response_created("resp-1"), - responses::ev_function_call(first_call_id, &tool_name, &args), - responses::ev_function_call(second_call_id, &tool_name, &args), + responses::ev_function_call_with_namespace(first_call_id, &namespace, "sync", &args), + responses::ev_function_call_with_namespace(second_call_id, &namespace, "sync", &args), responses::ev_completed("resp-1"), ]), ) @@ -488,7 +533,7 @@ async fn stdio_mcp_parallel_tool_calls_opt_in_runs_concurrently() -> anyhow::Res let first_call_id = "sync-1"; let second_call_id = "sync-2"; let server_name = "rmcp"; - let tool_name = format!("mcp__{server_name}__sync"); + let namespace = format!("mcp__{server_name}__"); let args = json!({ "sleep_after_ms": 100, "barrier": { @@ -503,8 +548,8 @@ async fn stdio_mcp_parallel_tool_calls_opt_in_runs_concurrently() -> anyhow::Res &server, responses::sse(vec![ responses::ev_response_created("resp-1"), - responses::ev_function_call(first_call_id, &tool_name, &args), - responses::ev_function_call(second_call_id, &tool_name, &args), + responses::ev_function_call_with_namespace(first_call_id, &namespace, "sync", &args), + responses::ev_function_call_with_namespace(second_call_id, &namespace, "sync", &args), responses::ev_completed("resp-1"), ]), ) @@ -604,13 +649,14 @@ async fn stdio_image_responses_round_trip() -> anyhow::Result<()> { let call_id = "img-1"; let server_name = "rmcp"; let tool_name = format!("mcp__{server_name}__image"); + let namespace = format!("mcp__{server_name}__"); // First stream: model decides to call the image tool. mount_sse_once( &server, responses::sse(vec![ responses::ev_response_created("resp-1"), - responses::ev_function_call(call_id, &tool_name, "{}"), + responses::ev_function_call_with_namespace(call_id, &namespace, "image", "{}"), responses::ev_completed("resp-1"), ]), ) @@ -793,7 +839,7 @@ async fn stdio_image_responses_are_sanitized_for_text_only_model() -> anyhow::Re let call_id = "img-text-only-1"; let server_name = "rmcp"; - let tool_name = format!("mcp__{server_name}__image"); + let namespace = format!("mcp__{server_name}__"); let text_only_model_slug = "rmcp-text-only-model"; let models_mock = mount_models_once( @@ -843,7 +889,7 @@ async fn stdio_image_responses_are_sanitized_for_text_only_model() -> anyhow::Re &server, responses::sse(vec![ responses::ev_response_created("resp-1"), - responses::ev_function_call(call_id, &tool_name, "{}"), + responses::ev_function_call_with_namespace(call_id, &namespace, "image", "{}"), responses::ev_completed("resp-1"), ]), ) @@ -964,13 +1010,18 @@ async fn stdio_server_propagates_whitelisted_env_vars() -> anyhow::Result<()> { let call_id = "call-1234"; let server_name = "rmcp_whitelist"; - let tool_name = format!("mcp__{server_name}__echo"); + let namespace = format!("mcp__{server_name}__"); mount_sse_once( &server, responses::sse(vec![ responses::ev_response_created("resp-1"), - responses::ev_function_call(call_id, &tool_name, "{\"message\":\"ping\"}"), + responses::ev_function_call_with_namespace( + call_id, + &namespace, + "echo", + "{\"message\":\"ping\"}", + ), responses::ev_completed("resp-1"), ]), ) @@ -1106,13 +1157,18 @@ async fn streamable_http_tool_call_round_trip() -> anyhow::Result<()> { let call_id = "call-456"; let server_name = "rmcp_http"; - let tool_name = format!("mcp__{server_name}__echo"); + let namespace = format!("mcp__{server_name}__"); mount_sse_once( &server, responses::sse(vec![ responses::ev_response_created("resp-1"), - responses::ev_function_call(call_id, &tool_name, "{\"message\":\"ping\"}"), + responses::ev_function_call_with_namespace( + call_id, + &namespace, + "echo", + "{\"message\":\"ping\"}", + ), responses::ev_completed("resp-1"), ]), ) @@ -1311,12 +1367,18 @@ async fn streamable_http_with_oauth_round_trip_impl() -> anyhow::Result<()> { let call_id = "call-789"; let server_name = "rmcp_http_oauth"; let tool_name = format!("mcp__{server_name}__echo"); + let namespace = format!("mcp__{server_name}__"); mount_sse_once( &server, responses::sse(vec![ responses::ev_response_created("resp-1"), - responses::ev_function_call(call_id, &tool_name, "{\"message\":\"ping\"}"), + responses::ev_function_call_with_namespace( + call_id, + &namespace, + "echo", + "{\"message\":\"ping\"}", + ), responses::ev_completed("resp-1"), ]), ) diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs index c52431665..74de499d7 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -23,6 +23,7 @@ use core_test_support::responses::ev_response_created; use core_test_support::responses::ev_tool_search_call; use core_test_support::responses::mount_sse_once; use core_test_support::responses::mount_sse_sequence; +use core_test_support::responses::namespace_child_tool; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; @@ -45,6 +46,7 @@ const CALENDAR_CREATE_TOOL: &str = "mcp__codex_apps__calendar_create_event"; const CALENDAR_LIST_TOOL: &str = "mcp__codex_apps__calendar_list_events"; const SEARCH_CALENDAR_NAMESPACE: &str = "mcp__codex_apps__calendar"; const SEARCH_CALENDAR_CREATE_TOOL: &str = "_create_event"; +const SEARCH_CALENDAR_LIST_TOOL: &str = "_list_events"; fn tool_names(body: &Value) -> Vec { body.get("tools") @@ -215,8 +217,17 @@ async fn tool_search_disabled_by_default_exposes_apps_tools_directly() -> Result let body = mock.single_request().body_json(); let tools = tool_names(&body); assert!(!tools.iter().any(|name| name == TOOL_SEARCH_TOOL_NAME)); - assert!(tools.iter().any(|name| name == CALENDAR_CREATE_TOOL)); - assert!(tools.iter().any(|name| name == CALENDAR_LIST_TOOL)); + assert!( + namespace_child_tool( + &body, + SEARCH_CALENDAR_NAMESPACE, + SEARCH_CALENDAR_CREATE_TOOL + ) + .is_some() + ); + assert!( + namespace_child_tool(&body, SEARCH_CALENDAR_NAMESPACE, SEARCH_CALENDAR_LIST_TOOL).is_some() + ); Ok(()) } @@ -332,6 +343,7 @@ async fn search_tool_hides_apps_tools_without_search() -> Result<()> { assert!(tools.iter().any(|name| name == TOOL_SEARCH_TOOL_NAME)); assert!(!tools.iter().any(|name| name == CALENDAR_CREATE_TOOL)); assert!(!tools.iter().any(|name| name == CALENDAR_LIST_TOOL)); + assert!(!tools.iter().any(|name| name == SEARCH_CALENDAR_NAMESPACE)); Ok(()) } @@ -365,11 +377,16 @@ async fn explicit_app_mentions_expose_apps_tools_without_search() -> Result<()> let body = mock.single_request().body_json(); let tools = tool_names(&body); assert!( - tools.iter().any(|name| name == CALENDAR_CREATE_TOOL), + namespace_child_tool( + &body, + SEARCH_CALENDAR_NAMESPACE, + SEARCH_CALENDAR_CREATE_TOOL + ) + .is_some(), "expected explicit app mention to expose create tool, got tools: {tools:?}" ); assert!( - tools.iter().any(|name| name == CALENDAR_LIST_TOOL), + namespace_child_tool(&body, SEARCH_CALENDAR_NAMESPACE, SEARCH_CALENDAR_LIST_TOOL).is_some(), "expected explicit app mention to expose list tool, got tools: {tools:?}" ); @@ -523,6 +540,12 @@ async fn tool_search_returns_deferred_tools_without_follow_up_tool_injection() - .any(|name| name == CALENDAR_CREATE_TOOL), "app tools should still be hidden before search: {first_request_tools:?}" ); + assert!( + !first_request_tools + .iter() + .any(|name| name == SEARCH_CALENDAR_NAMESPACE), + "app namespace should still be hidden before search: {first_request_tools:?}" + ); let output_item = tool_search_output_item(&requests[1], call_id); assert_eq!( @@ -570,6 +593,12 @@ async fn tool_search_returns_deferred_tools_without_follow_up_tool_injection() - .any(|name| name == CALENDAR_CREATE_TOOL), "follow-up request should rely on tool_search_output history, not tool injection: {second_request_tools:?}" ); + assert!( + !second_request_tools + .iter() + .any(|name| name == SEARCH_CALENDAR_NAMESPACE), + "follow-up request should rely on tool_search_output history, not namespace injection: {second_request_tools:?}" + ); let output_item = requests[2].function_call_output("calendar-call-1"); assert_eq!( @@ -584,6 +613,12 @@ async fn tool_search_returns_deferred_tools_without_follow_up_tool_injection() - .any(|name| name == CALENDAR_CREATE_TOOL), "post-tool follow-up should still rely on tool_search_output history, not tool injection: {third_request_tools:?}" ); + assert!( + !third_request_tools + .iter() + .any(|name| name == SEARCH_CALENDAR_NAMESPACE), + "post-tool follow-up should still rely on tool_search_output history, not namespace injection: {third_request_tools:?}" + ); Ok(()) } @@ -683,16 +718,19 @@ async fn tool_search_indexes_only_enabled_non_app_mcp_tools() -> Result<()> { .any(|name| name == "mcp__rmcp__echo"), "non-app MCP tools should be hidden before search in large-search mode: {first_request_tools:?}" ); + assert!( + !first_request_tools.iter().any(|name| name == "mcp__rmcp__"), + "non-app MCP namespace should be hidden before search in large-search mode: {first_request_tools:?}" + ); let echo_tools = tool_search_output_tools(&requests[1], echo_call_id); - let rmcp_echo_tools = echo_tools - .iter() - .filter(|tool| tool.get("name").and_then(Value::as_str) == Some("mcp__rmcp__")) - .flat_map(|namespace| namespace.get("tools").and_then(Value::as_array)) - .flatten() - .filter_map(|tool| tool.get("name").and_then(Value::as_str).map(str::to_string)) - .collect::>(); - assert_eq!(rmcp_echo_tools, vec!["echo".to_string()]); + let echo_output = json!({ "tools": echo_tools }); + let rmcp_echo_tool = namespace_child_tool(&echo_output, "mcp__rmcp__", "echo") + .expect("tool_search should return rmcp echo as a namespace child tool"); + assert_eq!( + rmcp_echo_tool.get("type").and_then(Value::as_str), + Some("function") + ); let image_tools = tool_search_output_tools(&requests[1], image_call_id); let found_rmcp_image_tool = image_tools diff --git a/codex-rs/core/tests/suite/sqlite_state.rs b/codex-rs/core/tests/suite/sqlite_state.rs index 1c9d6a87e..ef517d74f 100644 --- a/codex-rs/core/tests/suite/sqlite_state.rs +++ b/codex-rs/core/tests/suite/sqlite_state.rs @@ -325,12 +325,17 @@ async fn mcp_call_marks_thread_memory_mode_polluted_when_configured() -> Result< let server = start_mock_server().await; let call_id = "call-123"; let server_name = "rmcp"; - let tool_name = format!("mcp__{server_name}__echo"); + let namespace = format!("mcp__{server_name}__"); mount_sse_once( &server, responses::sse(vec![ ev_response_created("resp-1"), - ev_function_call(call_id, &tool_name, "{\"message\":\"ping\"}"), + responses::ev_function_call_with_namespace( + call_id, + &namespace, + "echo", + "{\"message\":\"ping\"}", + ), ev_completed("resp-1"), ]), ) diff --git a/codex-rs/core/tests/suite/truncation.rs b/codex-rs/core/tests/suite/truncation.rs index 288e8f4bd..5bca13de5 100644 --- a/codex-rs/core/tests/suite/truncation.rs +++ b/codex-rs/core/tests/suite/truncation.rs @@ -331,7 +331,7 @@ async fn mcp_tool_call_output_exceeds_limit_truncated_for_model() -> Result<()> let call_id = "rmcp-truncated"; let server_name = "rmcp"; - let tool_name = format!("mcp__{server_name}__echo"); + let namespace = format!("mcp__{server_name}__"); // Build a very large message to exceed 10KiB once serialized. let large_msg = "long-message-with-newlines-".repeat(6000); @@ -341,7 +341,12 @@ async fn mcp_tool_call_output_exceeds_limit_truncated_for_model() -> Result<()> &server, sse(vec![ responses::ev_response_created("resp-1"), - responses::ev_function_call(call_id, &tool_name, &args_json.to_string()), + responses::ev_function_call_with_namespace( + call_id, + &namespace, + "echo", + &args_json.to_string(), + ), responses::ev_completed("resp-1"), ]), ) @@ -426,13 +431,13 @@ async fn mcp_image_output_preserves_image_and_no_text_summary() -> Result<()> { let call_id = "rmcp-image-no-trunc"; let server_name = "rmcp"; - let tool_name = format!("mcp__{server_name}__image"); + let namespace = format!("mcp__{server_name}__"); mount_sse_once( &server, sse(vec![ ev_response_created("resp-1"), - ev_function_call(call_id, &tool_name, "{}"), + responses::ev_function_call_with_namespace(call_id, &namespace, "image", "{}"), ev_completed("resp-1"), ]), ) @@ -705,7 +710,7 @@ async fn mcp_tool_call_output_not_truncated_with_custom_limit() -> Result<()> { let call_id = "rmcp-untruncated"; let server_name = "rmcp"; - let tool_name = format!("mcp__{server_name}__echo"); + let namespace = format!("mcp__{server_name}__"); let large_msg = "a".repeat(80_000); let args_json = serde_json::json!({ "message": large_msg }); @@ -713,7 +718,12 @@ async fn mcp_tool_call_output_not_truncated_with_custom_limit() -> Result<()> { &server, sse(vec![ responses::ev_response_created("resp-1"), - responses::ev_function_call(call_id, &tool_name, &args_json.to_string()), + responses::ev_function_call_with_namespace( + call_id, + &namespace, + "echo", + &args_json.to_string(), + ), responses::ev_completed("resp-1"), ]), ) diff --git a/codex-rs/protocol/src/lib.rs b/codex-rs/protocol/src/lib.rs index de580de6f..2506dae74 100644 --- a/codex-rs/protocol/src/lib.rs +++ b/codex-rs/protocol/src/lib.rs @@ -2,8 +2,10 @@ pub mod account; mod agent_path; pub mod auth; mod thread_id; +mod tool_name; pub use agent_path::AgentPath; pub use thread_id::ThreadId; +pub use tool_name::ToolName; pub mod approvals; pub mod config_types; pub mod dynamic_tools; diff --git a/codex-rs/tools/src/tool_name.rs b/codex-rs/protocol/src/tool_name.rs similarity index 62% rename from codex-rs/tools/src/tool_name.rs rename to codex-rs/protocol/src/tool_name.rs index da18233ab..d09bc1ce7 100644 --- a/codex-rs/tools/src/tool_name.rs +++ b/codex-rs/protocol/src/tool_name.rs @@ -1,12 +1,23 @@ +use serde::Deserialize; +use serde::Serialize; +use std::fmt; + /// Identifies a callable tool, preserving the namespace split when the model /// provides one. -#[derive(Clone, Debug, Eq, Hash, PartialEq)] +#[derive(Clone, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)] pub struct ToolName { pub name: String, pub namespace: Option, } impl ToolName { + pub fn new(namespace: Option, name: impl Into) -> Self { + Self { + name: name.into(), + namespace, + } + } + pub fn plain(name: impl Into) -> Self { Self { name: name.into(), @@ -29,6 +40,15 @@ impl ToolName { } } +impl fmt::Display for ToolName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match &self.namespace { + Some(namespace) => write!(f, "{namespace}{}", self.name), + None => f.write_str(&self.name), + } + } +} + impl From for ToolName { fn from(name: String) -> Self { Self::plain(name) diff --git a/codex-rs/tools/src/code_mode.rs b/codex-rs/tools/src/code_mode.rs index 35d95b271..e45c7be1d 100644 --- a/codex-rs/tools/src/code_mode.rs +++ b/codex-rs/tools/src/code_mode.rs @@ -1,7 +1,9 @@ use crate::FreeformTool; use crate::FreeformToolFormat; use crate::JsonSchema; +use crate::ResponsesApiNamespaceTool; use crate::ResponsesApiTool; +use crate::ToolName; use crate::ToolSpec; use codex_code_mode::CodeModeToolKind; use codex_code_mode::ToolDefinition as CodeModeToolDefinition; @@ -9,22 +11,46 @@ use std::collections::BTreeMap; /// Augment tool descriptions with code-mode-specific exec samples. pub fn augment_tool_spec_for_code_mode(spec: ToolSpec) -> ToolSpec { - let Some(description) = code_mode_tool_definition_for_spec(&spec) - .map(codex_code_mode::augment_tool_definition) - .map(|definition| definition.description) - else { - return spec; - }; - match spec { ToolSpec::Function(mut tool) => { + let Some(description) = + augmented_description_for_spec(&ToolSpec::Function(tool.clone())) + else { + return ToolSpec::Function(tool); + }; tool.description = description; ToolSpec::Function(tool) } ToolSpec::Freeform(mut tool) => { + let Some(description) = + augmented_description_for_spec(&ToolSpec::Freeform(tool.clone())) + else { + return ToolSpec::Freeform(tool); + }; tool.description = description; ToolSpec::Freeform(tool) } + ToolSpec::Namespace(mut namespace) => { + for tool in &mut namespace.tools { + match tool { + ResponsesApiNamespaceTool::Function(tool) => { + let tool_name = + ToolName::namespaced(namespace.name.clone(), tool.name.clone()); + let definition = CodeModeToolDefinition { + name: tool_name.display(), + tool_name, + description: tool.description.clone(), + kind: CodeModeToolKind::Function, + input_schema: serde_json::to_value(&tool.parameters).ok(), + output_schema: tool.output_schema.clone(), + }; + tool.description = + codex_code_mode::augment_tool_definition(definition).description; + } + } + } + ToolSpec::Namespace(namespace) + } other => other, } } @@ -42,7 +68,9 @@ pub fn collect_code_mode_tool_definitions<'a>( ) -> Vec { let mut tool_definitions = specs .into_iter() - .filter_map(tool_spec_to_code_mode_tool_definition) + .flat_map(code_mode_tool_definitions_for_spec) + .filter(|definition| codex_code_mode::is_code_mode_nested_tool(&definition.name)) + .map(codex_code_mode::augment_tool_definition) .collect::>(); tool_definitions.sort_by(|left, right| left.name.cmp(&right.name)); tool_definitions.dedup_by(|left, right| left.name == right.name); @@ -54,7 +82,7 @@ pub fn collect_code_mode_exec_prompt_tool_definitions<'a>( ) -> Vec { let mut tool_definitions = specs .into_iter() - .filter_map(code_mode_tool_definition_for_spec) + .flat_map(code_mode_tool_definitions_for_spec) .filter(|definition| codex_code_mode::is_code_mode_nested_tool(&definition.name)) .collect::>(); tool_definitions.sort_by(|left, right| left.name.cmp(&right.name)); @@ -137,26 +165,61 @@ SOURCE: /[\s\S]+/ }) } +fn augmented_description_for_spec(spec: &ToolSpec) -> Option { + code_mode_tool_definition_for_spec(spec) + .map(codex_code_mode::augment_tool_definition) + .map(|definition| definition.description) +} + fn code_mode_tool_definition_for_spec(spec: &ToolSpec) -> Option { + code_mode_tool_definitions_for_spec(spec).into_iter().next() +} + +fn code_mode_tool_definitions_for_spec(spec: &ToolSpec) -> Vec { match spec { - ToolSpec::Function(tool) => Some(CodeModeToolDefinition { - name: tool.name.clone(), - description: tool.description.clone(), - kind: CodeModeToolKind::Function, - input_schema: serde_json::to_value(&tool.parameters).ok(), - output_schema: tool.output_schema.clone(), - }), - ToolSpec::Freeform(tool) => Some(CodeModeToolDefinition { - name: tool.name.clone(), - description: tool.description.clone(), - kind: CodeModeToolKind::Freeform, - input_schema: None, - output_schema: None, - }), + ToolSpec::Function(tool) => { + let name = tool.name.clone(); + vec![CodeModeToolDefinition { + tool_name: ToolName::plain(name.clone()), + name, + description: tool.description.clone(), + kind: CodeModeToolKind::Function, + input_schema: serde_json::to_value(&tool.parameters).ok(), + output_schema: tool.output_schema.clone(), + }] + } + ToolSpec::Freeform(tool) => { + let name = tool.name.clone(); + vec![CodeModeToolDefinition { + tool_name: ToolName::plain(name.clone()), + name, + description: tool.description.clone(), + kind: CodeModeToolKind::Freeform, + input_schema: None, + output_schema: None, + }] + } + ToolSpec::Namespace(namespace) => namespace + .tools + .iter() + .map(|tool| match tool { + ResponsesApiNamespaceTool::Function(tool) => { + let tool_name = ToolName::namespaced(namespace.name.clone(), tool.name.clone()); + CodeModeToolDefinition { + name: tool_name.display(), + tool_name, + description: tool.description.clone(), + kind: CodeModeToolKind::Function, + input_schema: serde_json::to_value(&tool.parameters).ok(), + output_schema: tool.output_schema.clone(), + } + } + }) + .collect(), ToolSpec::LocalShell {} | ToolSpec::ImageGeneration { .. } | ToolSpec::ToolSearch { .. } - | ToolSpec::WebSearch { .. } => None, + | ToolSpec::WebSearch { .. } => Vec::new(), } } diff --git a/codex-rs/tools/src/code_mode_tests.rs b/codex-rs/tools/src/code_mode_tests.rs index ec30c6e8b..15cca164e 100644 --- a/codex-rs/tools/src/code_mode_tests.rs +++ b/codex-rs/tools/src/code_mode_tests.rs @@ -7,6 +7,7 @@ use crate::FreeformTool; use crate::FreeformToolFormat; use crate::JsonSchema; use crate::ResponsesApiTool; +use crate::ToolName; use crate::ToolSpec; use pretty_assertions::assert_eq; use serde_json::json; @@ -106,6 +107,7 @@ fn tool_spec_to_code_mode_tool_definition_returns_augmented_nested_tools() { tool_spec_to_code_mode_tool_definition(&spec), Some(codex_code_mode::ToolDefinition { name: "apply_patch".to_string(), + tool_name: ToolName::plain("apply_patch"), description: r#"Apply a patch exec tool declaration: @@ -184,6 +186,7 @@ fn create_wait_tool_matches_expected_spec() { fn create_code_mode_tool_matches_expected_spec() { let enabled_tools = vec![codex_code_mode::ToolDefinition { name: "update_plan".to_string(), + tool_name: ToolName::plain("update_plan"), description: "Update the plan".to_string(), kind: codex_code_mode::CodeModeToolKind::Function, input_schema: None, diff --git a/codex-rs/tools/src/lib.rs b/codex-rs/tools/src/lib.rs index c310222a0..b30e0bffc 100644 --- a/codex-rs/tools/src/lib.rs +++ b/codex-rs/tools/src/lib.rs @@ -18,7 +18,6 @@ mod responses_api; mod tool_config; mod tool_definition; mod tool_discovery; -mod tool_name; mod tool_registry_plan; mod tool_registry_plan_types; mod tool_spec; @@ -50,6 +49,7 @@ pub use code_mode::collect_code_mode_tool_definitions; pub use code_mode::create_code_mode_tool; pub use code_mode::create_wait_tool; pub use code_mode::tool_spec_to_code_mode_tool_definition; +pub use codex_protocol::ToolName; pub use dynamic_tool::parse_dynamic_tool; pub use image_detail::can_request_original_image_detail; pub use image_detail::normalize_output_image_detail; @@ -113,13 +113,13 @@ pub use tool_discovery::collect_tool_suggest_entries; pub use tool_discovery::create_tool_search_tool; pub use tool_discovery::create_tool_suggest_tool; pub use tool_discovery::filter_tool_suggest_discoverable_tools_for_client; -pub use tool_name::ToolName; pub use tool_registry_plan::build_tool_registry_plan; pub use tool_registry_plan_types::ToolHandlerKind; pub use tool_registry_plan_types::ToolHandlerSpec; pub use tool_registry_plan_types::ToolNamespace; pub use tool_registry_plan_types::ToolRegistryPlan; pub use tool_registry_plan_types::ToolRegistryPlanDeferredTool; +pub use tool_registry_plan_types::ToolRegistryPlanMcpTool; pub use tool_registry_plan_types::ToolRegistryPlanParams; pub use tool_spec::ConfiguredToolSpec; pub use tool_spec::ResponsesApiWebSearchFilters; diff --git a/codex-rs/tools/src/responses_api.rs b/codex-rs/tools/src/responses_api.rs index 50bb4f223..205c07510 100644 --- a/codex-rs/tools/src/responses_api.rs +++ b/codex-rs/tools/src/responses_api.rs @@ -1,5 +1,6 @@ use crate::JsonSchema; use crate::ToolDefinition; +use crate::ToolName; use crate::parse_dynamic_tool; use crate::parse_mcp_tool; use codex_protocol::dynamic_tools::DynamicToolSpec; @@ -70,20 +71,22 @@ pub fn dynamic_tool_to_responses_api_tool( } pub fn mcp_tool_to_responses_api_tool( - name: String, + tool_name: &ToolName, tool: &rmcp::model::Tool, ) -> Result { Ok(tool_definition_to_responses_api_tool( - parse_mcp_tool(tool)?.renamed(name), + parse_mcp_tool(tool)?.renamed(tool_name.name.clone()), )) } pub fn mcp_tool_to_deferred_responses_api_tool( - name: String, + tool_name: &ToolName, tool: &rmcp::model::Tool, ) -> Result { Ok(tool_definition_to_responses_api_tool( - parse_mcp_tool(tool)?.renamed(name).into_deferred(), + parse_mcp_tool(tool)? + .renamed(tool_name.name.clone()) + .into_deferred(), )) } diff --git a/codex-rs/tools/src/responses_api_tests.rs b/codex-rs/tools/src/responses_api_tests.rs index c3ce4cec2..799a2385d 100644 --- a/codex-rs/tools/src/responses_api_tests.rs +++ b/codex-rs/tools/src/responses_api_tests.rs @@ -7,6 +7,7 @@ use super::mcp_tool_to_deferred_responses_api_tool; use super::tool_definition_to_responses_api_tool; use crate::JsonSchema; use crate::ToolDefinition; +use crate::ToolName; use codex_protocol::dynamic_tools::DynamicToolSpec; use pretty_assertions::assert_eq; use serde_json::json; @@ -106,19 +107,23 @@ fn mcp_tool_to_deferred_responses_api_tool_sets_defer_loading() { assert_eq!( mcp_tool_to_deferred_responses_api_tool( - "mcp__codex_apps__lookup_order".to_string(), + &ToolName::namespaced("mcp__codex_apps__", "lookup_order"), &tool, ) .expect("convert deferred tool"), ResponsesApiTool { - name: "mcp__codex_apps__lookup_order".to_string(), + name: "lookup_order".to_string(), description: "Look up an order".to_string(), strict: false, defer_loading: Some(true), - parameters: JsonSchema::object(BTreeMap::from([( + parameters: JsonSchema::object( + BTreeMap::from([( "order_id".to_string(), JsonSchema::string(/*description*/ None), - )]), Some(vec!["order_id".to_string()]), Some(false.into())), + )]), + Some(vec!["order_id".to_string()]), + Some(false.into()) + ), output_schema: None, } ); diff --git a/codex-rs/tools/src/tool_discovery.rs b/codex-rs/tools/src/tool_discovery.rs index f5d84a952..e35da5735 100644 --- a/codex-rs/tools/src/tool_discovery.rs +++ b/codex-rs/tools/src/tool_discovery.rs @@ -2,6 +2,7 @@ use crate::JsonSchema; use crate::ResponsesApiNamespace; use crate::ResponsesApiNamespaceTool; use crate::ResponsesApiTool; +use crate::ToolName; use crate::ToolSearchOutputTool; use crate::ToolSpec; use crate::mcp_tool_to_deferred_responses_api_tool; @@ -242,7 +243,8 @@ pub fn collect_tool_search_output_tools<'a>( let tools = tools .iter() .map(|tool| { - mcp_tool_to_deferred_responses_api_tool(tool.tool_name.to_string(), tool.tool) + let tool_name = ToolName::namespaced(tool.tool_namespace, tool.tool_name); + mcp_tool_to_deferred_responses_api_tool(&tool_name, tool.tool) .map(ResponsesApiNamespaceTool::Function) }) .collect::, _>>()?; diff --git a/codex-rs/tools/src/tool_registry_plan.rs b/codex-rs/tools/src/tool_registry_plan.rs index 82b4c3a14..cec938962 100644 --- a/codex-rs/tools/src/tool_registry_plan.rs +++ b/codex-rs/tools/src/tool_registry_plan.rs @@ -1,12 +1,13 @@ use crate::CommandToolOptions; use crate::REQUEST_USER_INPUT_TOOL_NAME; +use crate::ResponsesApiNamespace; +use crate::ResponsesApiNamespaceTool; use crate::ShellToolOptions; use crate::SpawnAgentToolOptions; use crate::TOOL_SEARCH_DEFAULT_LIMIT; use crate::TOOL_SEARCH_TOOL_NAME; use crate::TOOL_SUGGEST_TOOL_NAME; use crate::ToolHandlerKind; -use crate::ToolName; use crate::ToolRegistryPlan; use crate::ToolRegistryPlanParams; use crate::ToolSearchSource; @@ -61,7 +62,6 @@ use crate::request_user_input_tool_description; use crate::tool_registry_plan_types::agent_type_description; use codex_protocol::openai_models::ApplyPatchToolType; use codex_protocol::openai_models::ConfigShellToolType; -use rmcp::model::Tool as McpTool; use std::collections::BTreeMap; pub fn build_tool_registry_plan( @@ -76,9 +76,9 @@ pub fn build_tool_registry_plan( .tool_namespaces .into_iter() .flatten() - .map(|(name, detail)| { + .map(|(namespace, detail)| { ( - name.clone(), + namespace.clone(), codex_code_mode::ToolNamespaceDescription { name: detail.name.clone(), description: detail.description.clone().unwrap_or_default(), @@ -100,9 +100,8 @@ pub fn build_tool_registry_plan( .iter() .map(|configured_tool| &configured_tool.spec), ); - enabled_tools.sort_by(|left, right| { - compare_code_mode_tool_names(&left.name, &right.name, &namespace_descriptions) - }); + enabled_tools + .sort_by(|left, right| compare_code_mode_tools(left, right, &namespace_descriptions)); plan.push_spec( create_code_mode_tool( &enabled_tools, @@ -266,10 +265,7 @@ pub fn build_tool_registry_plan( plan.register_handler(TOOL_SEARCH_TOOL_NAME, ToolHandlerKind::ToolSearch); for tool in deferred_mcp_tools { - plan.register_handler( - ToolName::namespaced(tool.tool_namespace, tool.tool_name), - ToolHandlerKind::Mcp, - ); + plan.register_handler(tool.name.clone(), ToolHandlerKind::Mcp); } } @@ -471,28 +467,56 @@ pub fn build_tool_registry_plan( } if let Some(mcp_tools) = params.mcp_tools { - let mut entries: Vec<(String, &McpTool)> = mcp_tools - .iter() - .map(|(name, tool)| (name.clone(), tool)) - .collect(); - entries.sort_by(|left, right| left.0.cmp(&right.0)); + let mut entries = mcp_tools.to_vec(); + entries.sort_by_key(|tool| tool.name.display()); + let mut namespace_entries = BTreeMap::new(); - for (name, tool) in entries { - match mcp_tool_to_responses_api_tool(name.clone(), tool) { - Ok(converted_tool) => { - plan.push_spec( - ToolSpec::Function(converted_tool), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - plan.register_handler(name, ToolHandlerKind::Mcp); - } - Err(error) => { - tracing::error!( - "Failed to convert {name:?} MCP tool to OpenAI tool: {error:?}" - ); + for tool in entries { + let Some(namespace) = tool.name.namespace.as_ref() else { + let tool_name = &tool.name; + tracing::error!("Skipping MCP tool `{tool_name}`: MCP tools must be namespaced"); + continue; + }; + namespace_entries + .entry(namespace.clone()) + .or_insert_with(Vec::new) + .push(tool); + } + + for (namespace, mut entries) in namespace_entries { + entries.sort_by_key(|tool| tool.name.name.clone()); + let description = params + .tool_namespaces + .and_then(|namespaces| namespaces.get(&namespace)) + .and_then(|namespace| namespace.description.clone()) + .unwrap_or_default(); + let mut tools = Vec::new(); + for tool in entries { + match mcp_tool_to_responses_api_tool(&tool.name, tool.tool) { + Ok(converted_tool) => { + tools.push(ResponsesApiNamespaceTool::Function(converted_tool)); + plan.register_handler(tool.name, ToolHandlerKind::Mcp); + } + Err(error) => { + let tool_name = &tool.name; + tracing::error!( + "Failed to convert `{tool_name}` MCP tool to OpenAI tool: {error:?}" + ); + } } } + + if !tools.is_empty() { + plan.push_spec( + ToolSpec::Namespace(ResponsesApiNamespace { + name: namespace, + description, + tools, + }), + /*supports_parallel_tool_calls*/ false, + config.code_mode_enabled, + ); + } } } @@ -518,41 +542,31 @@ pub fn build_tool_registry_plan( plan } -fn compare_code_mode_tool_names( - left_name: &str, - right_name: &str, +fn compare_code_mode_tools( + left: &codex_code_mode::ToolDefinition, + right: &codex_code_mode::ToolDefinition, namespace_descriptions: &BTreeMap, ) -> std::cmp::Ordering { - let left_namespace = code_mode_namespace_name(left_name, namespace_descriptions); - let right_namespace = code_mode_namespace_name(right_name, namespace_descriptions); + let left_namespace = code_mode_namespace_name(left, namespace_descriptions); + let right_namespace = code_mode_namespace_name(right, namespace_descriptions); left_namespace .cmp(&right_namespace) - .then_with(|| { - code_mode_function_name(left_name, left_namespace) - .cmp(code_mode_function_name(right_name, right_namespace)) - }) - .then_with(|| left_name.cmp(right_name)) + .then_with(|| left.tool_name.name.cmp(&right.tool_name.name)) + .then_with(|| left.name.cmp(&right.name)) } fn code_mode_namespace_name<'a>( - name: &str, + tool: &codex_code_mode::ToolDefinition, namespace_descriptions: &'a BTreeMap, ) -> Option<&'a str> { - namespace_descriptions - .get(name) + tool.tool_name + .namespace + .as_ref() + .and_then(|namespace| namespace_descriptions.get(namespace)) .map(|namespace_description| namespace_description.name.as_str()) } -fn code_mode_function_name<'a>(name: &'a str, namespace: Option<&str>) -> &'a str { - namespace - .and_then(|namespace| { - name.strip_prefix(namespace) - .and_then(|suffix| suffix.strip_prefix("__")) - }) - .unwrap_or(name) -} - #[cfg(test)] #[path = "tool_registry_plan_tests.rs"] mod tests; diff --git a/codex-rs/tools/src/tool_registry_plan_tests.rs b/codex-rs/tools/src/tool_registry_plan_tests.rs index 69b8b55b5..7b9e8ed9a 100644 --- a/codex-rs/tools/src/tool_registry_plan_tests.rs +++ b/codex-rs/tools/src/tool_registry_plan_tests.rs @@ -7,12 +7,15 @@ use crate::FreeformTool; use crate::JsonSchema; use crate::JsonSchemaPrimitiveType; use crate::JsonSchemaType; +use crate::ResponsesApiNamespaceTool; use crate::ResponsesApiTool; use crate::ResponsesApiWebSearchFilters; use crate::ResponsesApiWebSearchUserLocation; use crate::ToolHandlerSpec; +use crate::ToolName; use crate::ToolNamespace; use crate::ToolRegistryPlanDeferredTool; +use crate::ToolRegistryPlanMcpTool; use crate::ToolsConfigParams; use crate::WaitAgentTimeoutOptions; use crate::mcp_call_tool_result_output_schema; @@ -1075,7 +1078,7 @@ fn test_build_specs_mcp_tools_converted() { let (tools, _) = build_specs( &tools_config, Some(HashMap::from([( - "test_server/do_something_cool".to_string(), + ToolName::namespaced("test_server/", "do_something_cool"), mcp_tool( "do_something_cool", "Do something cool", @@ -1101,11 +1104,11 @@ fn test_build_specs_mcp_tools_converted() { &[], ); - let tool = find_tool(&tools, "test_server/do_something_cool"); + let tool = find_namespace_function_tool(&tools, "test_server/", "do_something_cool"); assert_eq!( - &tool.spec, - &ToolSpec::Function(ResponsesApiTool { - name: "test_server/do_something_cool".to_string(), + tool, + &ResponsesApiTool { + name: "do_something_cool".to_string(), parameters: JsonSchema::object( BTreeMap::from([ ( @@ -1144,7 +1147,7 @@ fn test_build_specs_mcp_tools_converted() { strict: false, output_schema: Some(mcp_call_tool_result_output_schema(serde_json::json!({}))), defer_loading: None, - }) + } ); } @@ -1167,16 +1170,16 @@ fn test_build_specs_mcp_tools_sorted_by_name() { let tools_map = HashMap::from([ ( - "test_server/do".to_string(), - mcp_tool("a", "a", serde_json::json!({"type": "object"})), + ToolName::namespaced("test_server/", "do"), + mcp_tool("do", "a", serde_json::json!({"type": "object"})), ), ( - "test_server/something".to_string(), - mcp_tool("b", "b", serde_json::json!({"type": "object"})), + ToolName::namespaced("test_server/", "something"), + mcp_tool("something", "b", serde_json::json!({"type": "object"})), ), ( - "test_server/cool".to_string(), - mcp_tool("c", "c", serde_json::json!({"type": "object"})), + ToolName::namespaced("test_server/", "cool"), + mcp_tool("cool", "c", serde_json::json!({"type": "object"})), ), ]); @@ -1187,17 +1190,14 @@ fn test_build_specs_mcp_tools_sorted_by_name() { &[], ); - let mcp_names: Vec<_> = tools - .iter() - .map(|tool| tool.name().to_string()) - .filter(|name| name.starts_with("test_server/")) - .collect(); - let expected = vec![ - "test_server/cool".to_string(), - "test_server/do".to_string(), - "test_server/something".to_string(), - ]; - assert_eq!(mcp_names, expected); + assert_eq!( + namespace_function_names(&tools, "test_server/"), + vec![ + "cool".to_string(), + "do".to_string(), + "something".to_string(), + ] + ); } #[test] @@ -1222,7 +1222,7 @@ fn search_tool_description_lists_each_mcp_source_once() { &tools_config, Some(HashMap::from([ ( - "mcp__codex_apps__calendar_create_event".to_string(), + ToolName::namespaced("mcp__codex_apps__calendar", "_create_event"), mcp_tool( "calendar_create_event", "Create calendar event", @@ -1230,7 +1230,7 @@ fn search_tool_description_lists_each_mcp_source_once() { ), ), ( - "mcp__rmcp__echo".to_string(), + ToolName::namespaced("mcp__rmcp__", "echo"), mcp_tool("echo", "Echo", serde_json::json!({"type": "object"})), ), ])), @@ -1577,7 +1577,7 @@ fn code_mode_augments_mcp_tool_descriptions_with_namespaced_sample() { let (tools, _) = build_specs( &tools_config, Some(HashMap::from([( - "mcp__sample__echo".to_string(), + ToolName::namespaced("mcp__sample__", "echo"), mcp_tool( "echo", "Echo text", @@ -1595,11 +1595,8 @@ fn code_mode_augments_mcp_tool_descriptions_with_namespaced_sample() { &[], ); - let ToolSpec::Function(ResponsesApiTool { description, .. }) = - &find_tool(&tools, "mcp__sample__echo").spec - else { - panic!("expected function tool"); - }; + let ResponsesApiTool { description, .. } = + find_namespace_function_tool(&tools, "mcp__sample__", "echo"); assert_eq!( description, @@ -1633,7 +1630,7 @@ fn code_mode_preserves_nullable_and_literal_mcp_input_shapes() { let (tools, _) = build_specs( &tools_config, Some(HashMap::from([( - "mcp__sample__fn".to_string(), + ToolName::namespaced("mcp__sample__", "fn"), mcp_tool( "fn", "Sample fn", @@ -1684,11 +1681,8 @@ fn code_mode_preserves_nullable_and_literal_mcp_input_shapes() { &[], ); - let ToolSpec::Function(ResponsesApiTool { description, .. }) = - &find_tool(&tools, "mcp__sample__fn").spec - else { - panic!("expected function tool"); - }; + let ResponsesApiTool { description, .. } = + find_namespace_function_tool(&tools, "mcp__sample__", "fn"); assert!(description.contains( r#"exec tool declaration: @@ -1851,7 +1845,7 @@ fn search_capable_model_info() -> ModelInfo { fn build_specs<'a>( config: &ToolsConfig, - mcp_tools: Option>, + mcp_tools: Option>, deferred_mcp_tools: Option>>, dynamic_tools: &[DynamicToolSpec], ) -> (Vec, Vec) { @@ -1866,7 +1860,7 @@ fn build_specs<'a>( fn build_specs_with_discoverable_tools<'a>( config: &ToolsConfig, - mcp_tools: Option>, + mcp_tools: Option>, deferred_mcp_tools: Option>>, discoverable_tools: Option>, dynamic_tools: &[DynamicToolSpec], @@ -1883,16 +1877,25 @@ fn build_specs_with_discoverable_tools<'a>( fn build_specs_with_optional_tool_namespaces<'a>( config: &ToolsConfig, - mcp_tools: Option>, + mcp_tools: Option>, deferred_mcp_tools: Option>>, tool_namespaces: Option>, discoverable_tools: Option>, dynamic_tools: &[DynamicToolSpec], ) -> (Vec, Vec) { + let mcp_tool_inputs = mcp_tools.as_ref().map(|mcp_tools| { + mcp_tools + .iter() + .map(|(name, tool)| ToolRegistryPlanMcpTool { + name: name.clone(), + tool, + }) + .collect::>() + }); let plan = build_tool_registry_plan( config, ToolRegistryPlanParams { - mcp_tools: mcp_tools.as_ref(), + mcp_tools: mcp_tool_inputs.as_deref(), deferred_mcp_tools: deferred_mcp_tools.as_deref(), tool_namespaces: tool_namespaces.as_ref(), discoverable_tools: discoverable_tools.as_deref(), @@ -1968,16 +1971,16 @@ fn code_mode_augments_mcp_tool_descriptions_with_structured_output_sample() { let (tools, _) = build_specs( &tools_config, - Some(HashMap::from([("mcp__sample__echo".to_string(), tool)])), + Some(HashMap::from([( + ToolName::namespaced("mcp__sample__", "echo"), + tool, + )])), /*deferred_mcp_tools*/ None, &[], ); - let ToolSpec::Function(ResponsesApiTool { description, .. }) = - &find_tool(&tools, "mcp__sample__echo").spec - else { - panic!("expected function tool"); - }; + let ResponsesApiTool { description, .. } = + find_namespace_function_tool(&tools, "mcp__sample__", "echo"); assert_eq!( description, @@ -2017,8 +2020,7 @@ fn deferred_mcp_tool<'a>( connector_description: Option<&'a str>, ) -> ToolRegistryPlanDeferredTool<'a> { ToolRegistryPlanDeferredTool { - tool_name, - tool_namespace, + name: ToolName::namespaced(tool_namespace, tool_name), server_name, connector_name, connector_description, @@ -2089,6 +2091,39 @@ fn find_tool<'a>(tools: &'a [ConfiguredToolSpec], expected_name: &str) -> &'a Co .unwrap_or_else(|| panic!("expected tool {expected_name}")) } +fn find_namespace_function_tool<'a>( + tools: &'a [ConfiguredToolSpec], + expected_namespace: &str, + expected_name: &str, +) -> &'a ResponsesApiTool { + let namespace_tool = find_tool(tools, expected_namespace); + let ToolSpec::Namespace(namespace) = &namespace_tool.spec else { + panic!("expected namespace tool {expected_namespace}"); + }; + namespace + .tools + .iter() + .find_map(|tool| match tool { + ResponsesApiNamespaceTool::Function(tool) if tool.name == expected_name => Some(tool), + _ => None, + }) + .unwrap_or_else(|| panic!("expected tool {expected_namespace}{expected_name} in namespace")) +} + +fn namespace_function_names(tools: &[ConfiguredToolSpec], expected_namespace: &str) -> Vec { + let namespace_tool = find_tool(tools, expected_namespace); + let ToolSpec::Namespace(namespace) = &namespace_tool.spec else { + panic!("expected namespace tool {expected_namespace}"); + }; + namespace + .tools + .iter() + .map(|tool| match tool { + ResponsesApiNamespaceTool::Function(tool) => tool.name.clone(), + }) + .collect() +} + fn expect_object_schema( schema: &JsonSchema, ) -> (&BTreeMap, Option<&Vec>) { @@ -2137,6 +2172,17 @@ fn strip_descriptions_tool(spec: &mut ToolSpec) { ToolSpec::Function(ResponsesApiTool { parameters, .. }) => { strip_descriptions_schema(parameters); } + ToolSpec::Namespace(namespace) => { + for tool in &mut namespace.tools { + match tool { + ResponsesApiNamespaceTool::Function(ResponsesApiTool { + parameters, .. + }) => { + strip_descriptions_schema(parameters); + } + } + } + } ToolSpec::Freeform(FreeformTool { .. }) | ToolSpec::LocalShell {} | ToolSpec::ImageGeneration { .. } diff --git a/codex-rs/tools/src/tool_registry_plan_types.rs b/codex-rs/tools/src/tool_registry_plan_types.rs index 7459954dc..b9d66a0c2 100644 --- a/codex-rs/tools/src/tool_registry_plan_types.rs +++ b/codex-rs/tools/src/tool_registry_plan_types.rs @@ -6,7 +6,6 @@ use crate::ToolsConfig; use crate::WaitAgentTimeoutOptions; use crate::augment_tool_spec_for_code_mode; use codex_protocol::dynamic_tools::DynamicToolSpec; -use rmcp::model::Tool as McpTool; use std::collections::HashMap; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -58,7 +57,7 @@ pub struct ToolRegistryPlan { #[derive(Debug, Clone, Copy)] pub struct ToolRegistryPlanParams<'a> { - pub mcp_tools: Option<&'a HashMap>, + pub mcp_tools: Option<&'a [ToolRegistryPlanMcpTool<'a>]>, pub deferred_mcp_tools: Option<&'a [ToolRegistryPlanDeferredTool<'a>]>, pub tool_namespaces: Option<&'a HashMap>, pub discoverable_tools: Option<&'a [DiscoverableTool]>, @@ -73,10 +72,18 @@ pub struct ToolNamespace { pub description: Option, } -#[derive(Debug, Clone, Copy)] +/// Direct MCP tool metadata needed to expose the Responses API namespace tool +/// while registering its runtime handler with the canonical namespace/name +/// identity. +#[derive(Debug, Clone)] +pub struct ToolRegistryPlanMcpTool<'a> { + pub name: ToolName, + pub tool: &'a rmcp::model::Tool, +} + +#[derive(Debug, Clone)] pub struct ToolRegistryPlanDeferredTool<'a> { - pub tool_name: &'a str, - pub tool_namespace: &'a str, + pub name: ToolName, pub server_name: &'a str, pub connector_name: Option<&'a str>, pub connector_description: Option<&'a str>, diff --git a/codex-rs/tools/src/tool_spec.rs b/codex-rs/tools/src/tool_spec.rs index 24644e226..81db2f558 100644 --- a/codex-rs/tools/src/tool_spec.rs +++ b/codex-rs/tools/src/tool_spec.rs @@ -1,5 +1,6 @@ use crate::FreeformTool; use crate::JsonSchema; +use crate::ResponsesApiNamespace; use crate::ResponsesApiTool; use codex_protocol::config_types::WebSearchConfig; use codex_protocol::config_types::WebSearchContextSize; @@ -20,6 +21,8 @@ const WEB_SEARCH_TEXT_AND_IMAGE_CONTENT_TYPES: [&str; 2] = ["text", "image"]; pub enum ToolSpec { #[serde(rename = "function")] Function(ResponsesApiTool), + #[serde(rename = "namespace")] + Namespace(ResponsesApiNamespace), #[serde(rename = "tool_search")] ToolSearch { execution: String, @@ -57,6 +60,7 @@ impl ToolSpec { pub fn name(&self) -> &str { match self { ToolSpec::Function(tool) => tool.name.as_str(), + ToolSpec::Namespace(namespace) => namespace.name.as_str(), ToolSpec::ToolSearch { .. } => "tool_search", ToolSpec::LocalShell {} => "local_shell", ToolSpec::ImageGeneration { .. } => "image_generation", diff --git a/codex-rs/tools/src/tool_spec_tests.rs b/codex-rs/tools/src/tool_spec_tests.rs index c94f54e6b..0b9862562 100644 --- a/codex-rs/tools/src/tool_spec_tests.rs +++ b/codex-rs/tools/src/tool_spec_tests.rs @@ -1,4 +1,5 @@ use super::ConfiguredToolSpec; +use super::ResponsesApiNamespace; use super::ResponsesApiWebSearchFilters; use super::ResponsesApiWebSearchUserLocation; use super::ToolSpec; @@ -6,6 +7,7 @@ use crate::AdditionalProperties; use crate::FreeformTool; use crate::FreeformToolFormat; use crate::JsonSchema; +use crate::ResponsesApiNamespaceTool; use crate::ResponsesApiTool; use crate::create_tools_json_for_responses_api; use codex_protocol::config_types::WebSearchContextSize; @@ -34,6 +36,15 @@ fn tool_spec_name_covers_all_variants() { .name(), "lookup_order" ); + assert_eq!( + ToolSpec::Namespace(ResponsesApiNamespace { + name: "mcp__demo__".to_string(), + description: "Demo tools".to_string(), + tools: Vec::new(), + }) + .name(), + "mcp__demo__" + ); assert_eq!( ToolSpec::ToolSearch { execution: "sync".to_string(), @@ -163,6 +174,51 @@ fn create_tools_json_for_responses_api_includes_top_level_name() { ); } +#[test] +fn namespace_tool_spec_serializes_expected_wire_shape() { + assert_eq!( + serde_json::to_value(ToolSpec::Namespace(ResponsesApiNamespace { + name: "mcp__demo__".to_string(), + description: "Demo tools".to_string(), + tools: vec![ResponsesApiNamespaceTool::Function(ResponsesApiTool { + name: "lookup_order".to_string(), + description: "Look up an order".to_string(), + strict: false, + defer_loading: None, + parameters: JsonSchema::object( + BTreeMap::from([( + "order_id".to_string(), + JsonSchema::string(/*description*/ None), + )]), + /*required*/ None, + /*additional_properties*/ None, + ), + output_schema: None, + })], + })) + .expect("serialize namespace tool"), + json!({ + "type": "namespace", + "name": "mcp__demo__", + "description": "Demo tools", + "tools": [ + { + "type": "function", + "name": "lookup_order", + "description": "Look up an order", + "strict": false, + "parameters": { + "type": "object", + "properties": { + "order_id": { "type": "string" }, + }, + }, + }, + ], + }) + ); +} + #[test] fn web_search_tool_spec_serializes_expected_wire_shape() { assert_eq!(