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!(