diff --git a/codex-rs/config/src/strict_config.rs b/codex-rs/config/src/strict_config.rs index 7809b67931..c1523c706c 100644 --- a/codex-rs/config/src/strict_config.rs +++ b/codex-rs/config/src/strict_config.rs @@ -159,7 +159,7 @@ fn push_unknown_feature_paths( for feature_key in features .keys() .map(String::as_str) - .filter(|key| *key != "tool_registry" && !is_known_feature_key(key)) + .filter(|key| !is_known_feature_key(key)) { let mut path = prefix .iter() diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index b44f343fd5..6a91c3c20f 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -3102,7 +3102,7 @@ impl Session { prepared_recommendations, ) .or_cancel(cancellation_token) - .await?; + .await??; Ok(Arc::new(StepContext { turn: turn_context, environments, diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index e5c658401e..9d1cec66a4 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -172,6 +172,9 @@ pub(crate) async fn run_turn( run_hooks_and_record_inputs(&sess, &turn_context, &input).await; return Err(err); } + if matches!(err.details(), CodexErrorDetails::ToolCollision(_)) { + return Err(err); + } let error = err.to_codex_protocol_error(); sess.emit_turn_error_lifecycle(turn_context.as_ref(), error.clone()) .await; @@ -1474,7 +1477,7 @@ pub(crate) async fn built_tools( mcp: &codex_mcp::McpBinding, step_store: &ExtensionData, prepared_recommendations: PreparedToolRecommendations, -) -> Arc { +) -> CodexResult> { let all_mcp_tools = mcp.tools(); let connector_snapshot = mcp.config().connector_snapshot.clone(); @@ -1535,7 +1538,7 @@ pub(crate) async fn built_tools( .instrument(trace_span!("built_tools.load_discoverable_tools")) .await }; - Arc::new(build_tool_router( + Ok(Arc::new(build_tool_router( sess, turn_context, environments, @@ -1543,7 +1546,7 @@ pub(crate) async fn built_tools( apps_enabled, step_store, tool_suggest_candidates.as_ref(), - )) + )?)) } #[derive(Debug)] diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 417e622137..63c6ecc012 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -251,14 +251,13 @@ pub(crate) struct RegisteredTool { #[derive(Default)] pub struct ToolRegistry { tools: IndexMap, + first_collision: Option, } impl ToolRegistry { #[cfg(test)] pub(crate) fn from_tools(tools: impl IntoIterator>) -> Self { - let mut registry = Self { - tools: IndexMap::new(), - }; + let mut registry = Self::default(); for runtime in tools { registry.register_trusted(runtime); @@ -327,6 +326,9 @@ impl ToolRegistry { let tool_name = runtime.tool_name(); if tool_name.namespace.is_none() && tool_name.name == "shell_command" { tracing::warn!(tool_name = %tool_name, "skipping external tool with reserved name"); + if self.tools.contains_key(&tool_name) { + self.record_collision(tool_name); + } return false; } @@ -340,11 +342,21 @@ impl ToolRegistry { tool_name = %entry.key(), "skipping duplicate external tool that is already registered" ); + self.first_collision + .get_or_insert_with(|| entry.key().clone()); false } } } + pub(crate) fn record_collision(&mut self, tool_name: ToolName) { + self.first_collision.get_or_insert(tool_name); + } + + pub(crate) fn first_collision(&self) -> Option<&ToolName> { + self.first_collision.as_ref() + } + pub(crate) fn remove(&mut self, tool_name: &ToolName) -> Option> { self.tools.shift_remove(tool_name).map(|tool| tool.runtime) } diff --git a/codex-rs/core/src/tools/registry_tests.rs b/codex-rs/core/src/tools/registry_tests.rs index 4d8f1def8d..d479e81725 100644 --- a/codex-rs/core/src/tools/registry_tests.rs +++ b/codex-rs/core/src/tools/registry_tests.rs @@ -217,6 +217,7 @@ fn registry_preserves_external_winners_and_trusted_synthetic_order() { let mut registry = ToolRegistry::from_tools([Arc::clone(&first_handler)]); assert!(!registry.register_external(handler(first_name.clone()))); + assert_eq!(registry.first_collision(), Some(&first_name)); assert!(registry.register_external(handler(second_name.clone()))); registry.prepend_trusted(handler(synthetic_name.clone())); @@ -248,6 +249,7 @@ fn reserved_shell_command_rejects_external_runtimes_without_a_builtin() { ToolExposure::Direct, )); assert!(registry.tool(&shell_command_name).is_none()); + assert_eq!(registry.first_collision(), None); let namespaced_handler = handler(namespaced_shell_command_name.clone()); assert!(registry.register_external(Arc::clone(&namespaced_handler))); @@ -258,6 +260,46 @@ fn reserved_shell_command_rejects_external_runtimes_without_a_builtin() { ); } +#[test] +fn registry_preserves_explicit_functions_shell_command_without_a_collision() { + let tool_name = codex_tools::ToolName::namespaced("functions", "shell_command"); + let runtime = Arc::new(TestHandler { tool_name }); + let mut registry = ToolRegistry::default(); + + assert!(registry.register_external(runtime)); + assert_eq!(registry.first_collision(), None); +} + +#[test] +fn registry_records_reserved_shell_command_when_a_matching_tool_exists() { + let tool_name = codex_tools::ToolName::plain("shell_command"); + let trusted = Arc::new(TestHandler { + tool_name: tool_name.clone(), + }) as Arc; + let external = Arc::new(TestHandler { + tool_name: tool_name.clone(), + }); + let mut registry = ToolRegistry::from_tools([trusted]); + + assert!(!registry.register_external(external)); + assert_eq!(registry.first_collision(), Some(&tool_name)); +} + +#[test] +fn registry_allows_identical_names_in_different_namespaces() { + let handler = |tool_name| Arc::new(TestHandler { tool_name }) as Arc; + let mut registry = ToolRegistry::from_tools([handler(codex_tools::ToolName::namespaced( + "first", "lookup", + ))]); + + assert!( + registry.register_external(handler(codex_tools::ToolName::namespaced( + "second", "lookup", + ))) + ); + assert_eq!(registry.first_collision(), None); +} + #[tokio::test] async fn readiness_selects_exact_tool_with_registry_owned_exposure() { let (session, _turn) = crate::session::tests::make_session_and_context().await; diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index 3acc12f7ea..e54a96377a 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -96,6 +96,7 @@ impl ToolRouter { hosted_specs, tool_search_handler_cache, ) + .expect("test tool registry should not contain duplicate tools") } pub(crate) fn from_parts(registry: ToolRegistry, model_visible_specs: Vec) -> Self { diff --git a/codex-rs/core/src/tools/spec_plan.rs b/codex-rs/core/src/tools/spec_plan.rs index e7a33f40e4..729f5256e3 100644 --- a/codex-rs/core/src/tools/spec_plan.rs +++ b/codex-rs/core/src/tools/spec_plan.rs @@ -67,6 +67,8 @@ use codex_protocol::account::PlanType; use codex_protocol::config_types::WebSearchMode; use codex_protocol::dynamic_tools::DynamicToolNamespaceTool; use codex_protocol::dynamic_tools::DynamicToolSpec; +use codex_protocol::error::CodexErrorDetails; +use codex_protocol::error::Result as CodexResult; use codex_protocol::openai_models::ConfigShellToolType; use codex_protocol::openai_models::InputModality; use codex_protocol::openai_models::ToolMode; @@ -121,7 +123,7 @@ pub(crate) fn build_tool_router( apps_enabled: bool, step_store: &ExtensionData, tool_suggest_candidates: Option<&crate::tools::router::ToolSuggestCandidates>, -) -> ToolRouter { +) -> CodexResult { let default_agent_type_description = crate::agent::role::spawn_tool_spec::build(&std::collections::BTreeMap::new()); let wait_for_environment_tool_config = session @@ -313,15 +315,21 @@ pub(crate) fn finalize_tool_router( mut registry: ToolRegistry, hosted_specs: Vec, tool_search_handler_cache: &ToolSearchHandlerCache, -) -> ToolRouter { +) -> CodexResult { apply_direct_model_only_namespace_overrides(turn_context, &mut registry); let code_mode_enabled = matches!( effective_tool_mode(turn_context), ToolMode::CodeMode | ToolMode::CodeModeOnly ); if code_mode_enabled { - registry.remove(&ToolName::plain(codex_code_mode::PUBLIC_TOOL_NAME)); - registry.remove(&ToolName::plain(codex_code_mode::WAIT_TOOL_NAME)); + for tool_name in [ + ToolName::plain(codex_code_mode::PUBLIC_TOOL_NAME), + ToolName::plain(codex_code_mode::WAIT_TOOL_NAME), + ] { + if registry.remove(&tool_name).is_some() { + registry.record_collision(tool_name); + } + } } let tool_search_name = ToolName::plain(TOOL_SEARCH_TOOL_NAME); if search_tool_enabled(turn_context) @@ -331,15 +339,25 @@ pub(crate) fn finalize_tool_router( && tool.runtime.search_info().is_some() }) { - registry.remove(&tool_search_name); + if registry.remove(&tool_search_name).is_some() { + registry.record_collision(tool_search_name); + } append_tool_search_executor(turn_context, &mut registry, tool_search_handler_cache); } let code_mode_tool_names = register_code_mode_executors(turn_context, &mut registry); + if turn_context.config.tool_registry.error_on_tool_collisions + && let Some(tool_name) = registry.first_collision() + { + let namespace = tool_name.namespace.as_deref().unwrap_or("functions"); + let name = format!("{namespace}.{}", tool_name.name); + return Err(CodexErrorDetails::ToolCollision(name).into()); + } + let model_visible_specs = build_model_visible_specs(turn_context, ®istry, &code_mode_tool_names, hosted_specs); - ToolRouter::from_parts(registry, model_visible_specs) + Ok(ToolRouter::from_parts(registry, model_visible_specs)) } fn apply_direct_model_only_namespace_overrides( diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index ab691747aa..4ed42d46da 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -15,6 +15,7 @@ use codex_protocol::AgentPath; use codex_protocol::ThreadId; use codex_protocol::config_types::WebSearchMode; use codex_protocol::dynamic_tools::DynamicToolSpec; +use codex_protocol::error::CodexErrorDetails; use codex_protocol::openai_models::ApplyPatchToolType; use codex_protocol::openai_models::ConfigShellToolType; use codex_protocol::openai_models::InputModality; @@ -1229,6 +1230,144 @@ async fn unified_tool_runtimes_preserve_source_order_and_collision_priority() { assert_eq!(tool.description, "lookup test tool"); } +#[tokio::test] +async fn strict_tool_collisions_reject_external_and_synthetic_duplicates() { + let cases = [ + ( + "mcp__registry.lookup", + ToolPlanInputs { + tool_runtimes: vec![mcp_runtime( + "registry", + "mcp__registry", + "lookup", + ToolExposure::Direct, + )], + extension_tool_executors: vec![Arc::new(TestNamespaceExtensionTool { + namespace: "mcp__registry", + tool_name: "lookup", + })], + ..ToolPlanInputs::default() + }, + false, + false, + ), + ( + "functions.update_plan", + ToolPlanInputs { + dynamic_tools: vec![dynamic_tool( + /*namespace*/ None, + "update_plan", + /*defer_loading*/ false, + )], + ..ToolPlanInputs::default() + }, + false, + false, + ), + ( + "functions.exec", + ToolPlanInputs { + dynamic_tools: vec![dynamic_tool( + /*namespace*/ None, + codex_code_mode::PUBLIC_TOOL_NAME, + /*defer_loading*/ false, + )], + ..ToolPlanInputs::default() + }, + true, + false, + ), + ( + "functions.tool_search", + ToolPlanInputs { + tool_runtimes: vec![mcp_runtime( + "registry", + "mcp__registry", + "lookup", + ToolExposure::Deferred, + )], + dynamic_tools: vec![dynamic_tool( + /*namespace*/ None, + codex_tools::TOOL_SEARCH_TOOL_NAME, + /*defer_loading*/ false, + )], + ..ToolPlanInputs::default() + }, + false, + true, + ), + ]; + + for (expected_name, inputs, code_mode_enabled, search_enabled) in cases { + let (_session, mut turn) = make_session_and_context().await; + update_config(&mut turn, |config| { + config.tool_registry.error_on_tool_collisions = true; + }); + if code_mode_enabled { + set_feature(&mut turn, Feature::CodeMode, /*enabled*/ true); + } + turn.model_info.supports_search_tool = search_enabled; + let turn = Arc::new(turn); + let step_context = StepContext::for_test(Arc::clone(&turn)); + let mut registry = build_core_tool_registry( + step_context.turn.as_ref(), + &step_context.environments, + step_context.mcp.as_ref(), + inputs.tool_suggest_candidates.as_ref(), + inputs.wait_for_environment_tool_config.as_ref(), + ); + let hosted_specs = append_source_tools( + step_context.turn.as_ref(), + &mut registry, + inputs.tool_runtimes, + inputs.extension_tool_executors, + &inputs.dynamic_tools, + ); + + let error = super::finalize_tool_router( + step_context.turn.as_ref(), + registry, + hosted_specs, + &Default::default(), + ) + .err() + .expect("strict tool collision should fail tool planning"); + assert!(matches!( + error.details(), + CodexErrorDetails::ToolCollision(name) if name == expected_name + )); + assert_eq!( + error.to_string(), + format!("duplicate tool: {expected_name}") + ); + } +} + +#[tokio::test] +async fn strict_tool_collisions_allow_identical_names_in_different_namespaces() { + let plan = probe_with( + |turn| { + update_config(turn, |config| { + config.tool_registry.error_on_tool_collisions = true; + }); + set_features(turn, &[Feature::CodeMode, Feature::CodeModeOnly]); + }, + ToolPlanInputs { + dynamic_tools: vec![ + dynamic_tool(Some("first"), "lookup", /*defer_loading*/ false), + dynamic_tool(Some("second"), "lookup", /*defer_loading*/ false), + ], + ..ToolPlanInputs::default() + }, + ) + .await; + + plan.assert_registered_contains(&[ + &ToolName::namespaced("first", "lookup").to_string(), + &ToolName::namespaced("second", "lookup").to_string(), + ]); +} + #[tokio::test] async fn code_mode_uses_the_first_normalized_tool_identity() { for (code_mode_only, winner_exposure, shadow_is_deferred) in [ diff --git a/codex-rs/core/tests/suite/tools.rs b/codex-rs/core/tests/suite/tools.rs index aef85b743a..6f1172dd59 100644 --- a/codex-rs/core/tests/suite/tools.rs +++ b/codex-rs/core/tests/suite/tools.rs @@ -7,9 +7,12 @@ use std::time::Instant; use anyhow::Context; use anyhow::Result; +use codex_core::StartThreadOptions; use codex_core::config::Constrained; use codex_core::sandboxing::SandboxPermissions; use codex_features::Feature; +use codex_protocol::dynamic_tools::DynamicToolFunctionSpec; +use codex_protocol::dynamic_tools::DynamicToolSpec; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; @@ -17,7 +20,10 @@ use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::Op; use codex_protocol::protocol::ThreadSettingsOverrides; +use codex_protocol::user_input::UserInput; use core_test_support::assert_regex_match; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; @@ -25,6 +31,7 @@ use core_test_support::responses::ev_custom_tool_call; use core_test_support::responses::ev_custom_tool_call_with_namespace; use core_test_support::responses::ev_function_call; use core_test_support::responses::ev_response_created; +use core_test_support::responses::mount_response_once; use core_test_support::responses::mount_sse_once; use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; @@ -35,9 +42,12 @@ use core_test_support::skip_if_sandbox; use core_test_support::submit_thread_settings; use core_test_support::test_codex::local; use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_event; use regex_lite::Regex; use serde_json::Value; use serde_json::json; +use test_case::test_case; +use wiremock::ResponseTemplate; fn tool_names(body: &Value) -> Vec { body.get("tools") @@ -56,6 +66,129 @@ fn tool_names(body: &Value) -> Vec { .unwrap_or_default() } +#[test_case(false; "normal sampling")] +#[test_case(true; "pre sampling compaction")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn strict_tool_collisions_fail_the_turn_before_sampling(pre_compact: bool) -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let mut builder = test_codex().with_config(move |config| { + config.tool_registry.error_on_tool_collisions = true; + if pre_compact { + config.model_auto_compact_token_limit = Some(0); + } + }); + let test = builder.build_with_auto_env(&server).await?; + let thread = test + .thread_manager + .start_thread(StartThreadOptions { + dynamic_tools: vec![DynamicToolSpec::Function(DynamicToolFunctionSpec { + name: "update_plan".to_string(), + description: "Collides with the built-in planning tool.".to_string(), + input_schema: json!({ + "type": "object", + "properties": {}, + "additionalProperties": false, + }), + defer_loading: false, + })], + ..StartThreadOptions::new(test.config.clone()) + }) + .await? + .thread; + + thread + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "use the planning tool".to_string(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: Default::default(), + }) + .await?; + + let EventMsg::Error(error) = + wait_for_event(&thread, |event| matches!(event, EventMsg::Error(_))).await + else { + unreachable!("event predicate guarantees an error"); + }; + assert_eq!(error.message, "duplicate tool: functions.update_plan"); + + let EventMsg::TurnComplete(completed) = + wait_for_event(&thread, |event| matches!(event, EventMsg::TurnComplete(_))).await + else { + unreachable!("event predicate guarantees turn completion"); + }; + assert_eq!(completed.error, Some(error)); + assert!( + server + .received_requests() + .await + .context("mock server should expose received requests")? + .iter() + .all(|request| request.url.path() != "/v1/responses"), + "a colliding turn should fail before making a model request" + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn strict_tool_collisions_do_not_duplicate_unrelated_compaction_errors() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let error = json!({ + "error": { + "message": "compaction request is invalid", + "code": "invalid_request", + }, + }); + let compact_mock = + mount_response_once(&server, ResponseTemplate::new(400).set_body_json(&error)).await; + let mut builder = test_codex().with_config(|config| { + config.tool_registry.error_on_tool_collisions = true; + config.model_auto_compact_token_limit = Some(0); + }); + let test = builder.build_with_auto_env(&server).await?; + + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "trigger compaction".to_string(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: Default::default(), + }) + .await?; + + let mut errors = Vec::new(); + wait_for_event(&test.codex, |event| match event { + EventMsg::Error(error) => { + errors.push(error.message.clone()); + false + } + EventMsg::TurnComplete(_) => true, + _ => false, + }) + .await; + + assert_eq!( + errors, + vec![format!("Error running remote compact task: {error}")] + ); + assert_eq!(compact_mock.requests().len(), 1); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn empty_turn_environments_omits_environment_backed_tools() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index 1fb5dd6f6c..180eb87b21 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -654,9 +654,9 @@ pub fn canonical_feature_for_key(key: &str) -> Option { .map(|spec| spec.id) } -/// Returns `true` if the provided string matches a known feature toggle key. +/// Returns `true` if the provided string matches a known `[features]` key. pub fn is_known_feature_key(key: &str) -> bool { - feature_for_key(key).is_some() + key == "tool_registry" || feature_for_key(key).is_some() } /// Deserializable features table for TOML. diff --git a/codex-rs/features/src/tests.rs b/codex-rs/features/src/tests.rs index 3b7c7dc7ec..a7c003fbcb 100644 --- a/codex-rs/features/src/tests.rs +++ b/codex-rs/features/src/tests.rs @@ -40,7 +40,8 @@ fn tool_registry_config_is_not_a_feature_toggle() { }) ); assert!(features.entries().is_empty()); - assert!(!crate::is_known_feature_key("tool_registry")); + assert!(crate::is_known_feature_key("tool_registry")); + assert_eq!(feature_for_key("tool_registry"), None); } #[test] diff --git a/codex-rs/protocol/src/error.rs b/codex-rs/protocol/src/error.rs index 6eb195f7bd..33ef62273a 100644 --- a/codex-rs/protocol/src/error.rs +++ b/codex-rs/protocol/src/error.rs @@ -120,6 +120,9 @@ pub enum CodexErrorDetails { /// Invalid request. #[error("{0}")] InvalidRequest(String), + /// Multiple registered tools share the same effective name. + #[error("duplicate tool: {0}")] + ToolCollision(String), /// Invalid image. #[error("Image poisoning")] InvalidImageRequest(), @@ -367,6 +370,7 @@ impl CodexErr { | CodexErrorDetails::QuotaExceeded | CodexErrorDetails::InvalidImageRequest() | CodexErrorDetails::InvalidRequest(_) + | CodexErrorDetails::ToolCollision(_) | CodexErrorDetails::RefreshTokenFailed(_) | CodexErrorDetails::UnsupportedOperation(_) | CodexErrorDetails::Sandbox(_) diff --git a/codex-rs/protocol/src/error_tests.rs b/codex-rs/protocol/src/error_tests.rs index 631faf3d94..1c718017cf 100644 --- a/codex-rs/protocol/src/error_tests.rs +++ b/codex-rs/protocol/src/error_tests.rs @@ -56,6 +56,10 @@ fn retryability_preserves_error_details_distinctions() { }), true, ), + ( + CodexErrorDetails::ToolCollision("functions.update_plan".to_string()).into(), + false, + ), (CodexErr::InternalServerError, true), ];