From 05d99c83f0ddd1852a1aac4d86216cd9c8f497dd Mon Sep 17 00:00:00 2001 From: Mitsuhiro Kotake Date: Wed, 1 Jul 2026 17:48:13 +0900 Subject: [PATCH] fix flattened namespace tool collisions --- codex-rs/core/src/session/tests.rs | 28 +-- codex-rs/core/src/session/turn.rs | 6 +- .../core/src/stream_events_utils_tests.rs | 25 +-- codex-rs/core/src/tools/hook_names.rs | 9 + codex-rs/core/src/tools/registry.rs | 52 ++++-- codex-rs/core/src/tools/registry_tests.rs | 54 ++++++ codex-rs/core/src/tools/router.rs | 2 +- codex-rs/core/src/tools/router_tests.rs | 15 +- codex-rs/core/src/tools/spec_plan.rs | 124 +++++++++++-- codex-rs/core/src/tools/spec_plan_tests.rs | 165 ++++++++++++++++-- codex-rs/core/tests/suite/hooks.rs | 82 ++++++++- codex-rs/core/tests/suite/responses_lite.rs | 1 + 12 files changed, 489 insertions(+), 74 deletions(-) diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index ece9d446b5..52757712b1 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -623,17 +623,20 @@ async fn preview_session_start_hooks( fn test_tool_runtime(session: Arc, turn_context: Arc) -> ToolCallRuntime { let step_context = StepContext::for_test(Arc::clone(&turn_context)); - let router = Arc::new(ToolRouter::from_context( - step_context.as_ref(), - crate::tools::router::ToolRouterParams { - tool_suggest_candidates: None, - mcp_tools: None, - deferred_mcp_tools: None, - extension_tool_executors: Vec::new(), - dynamic_tools: turn_context.dynamic_tools.as_slice(), - }, - &Default::default(), - )); + let router = Arc::new( + ToolRouter::from_context( + step_context.as_ref(), + crate::tools::router::ToolRouterParams { + tool_suggest_candidates: None, + mcp_tools: None, + deferred_mcp_tools: None, + extension_tool_executors: Vec::new(), + dynamic_tools: turn_context.dynamic_tools.as_slice(), + }, + &Default::default(), + ) + .expect("test tool router should build"), + ); let tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())); ToolCallRuntime::new(router, session, step_context, tracker) } @@ -10476,7 +10479,8 @@ async fn fatal_tool_error_stops_turn_and_reports_error() { dynamic_tools: turn_context.dynamic_tools.as_slice(), }, &Default::default(), - ); + ) + .expect("test tool router should build"); let item = ResponseItem::CustomToolCall { id: None, status: None, diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index f16b525002..2caaa31ac4 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -1296,7 +1296,7 @@ pub(crate) async fn built_tools( ); let mcp_tools = has_mcp_servers.then_some(mcp_tool_exposure.direct_tools); let deferred_mcp_tools = mcp_tool_exposure.deferred_tools; - Ok(Arc::new(ToolRouter::from_context( + let router = ToolRouter::from_context( step_context, ToolRouterParams { mcp_tools, @@ -1306,7 +1306,9 @@ pub(crate) async fn built_tools( dynamic_tools: turn_context.dynamic_tools.as_slice(), }, &sess.services.tool_search_handler_cache, - ))) + ) + .map_err(CodexErr::InvalidRequest)?; + Ok(Arc::new(router)) } #[derive(Debug)] diff --git a/codex-rs/core/src/stream_events_utils_tests.rs b/codex-rs/core/src/stream_events_utils_tests.rs index b3312ad49d..327bafd884 100644 --- a/codex-rs/core/src/stream_events_utils_tests.rs +++ b/codex-rs/core/src/stream_events_utils_tests.rs @@ -282,17 +282,20 @@ async fn handle_output_item_done_returns_contributed_last_agent_message() { let session = Arc::new(session); let turn_context = Arc::new(turn_context); let step_context = StepContext::for_test(Arc::clone(&turn_context)); - let router = Arc::new(ToolRouter::from_context( - step_context.as_ref(), - crate::tools::router::ToolRouterParams { - tool_suggest_candidates: None, - mcp_tools: None, - deferred_mcp_tools: None, - extension_tool_executors: Vec::new(), - dynamic_tools: turn_context.dynamic_tools.as_slice(), - }, - &Default::default(), - )); + let router = Arc::new( + ToolRouter::from_context( + step_context.as_ref(), + crate::tools::router::ToolRouterParams { + tool_suggest_candidates: None, + mcp_tools: None, + deferred_mcp_tools: None, + extension_tool_executors: Vec::new(), + dynamic_tools: turn_context.dynamic_tools.as_slice(), + }, + &Default::default(), + ) + .expect("test tool router should build"), + ); let tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())); let tool_runtime = ToolCallRuntime::new(router, Arc::clone(&session), step_context, tracker); let item = assistant_output_text("original assistant text"); diff --git a/codex-rs/core/src/tools/hook_names.rs b/codex-rs/core/src/tools/hook_names.rs index 92ebe8aa56..dc930071c3 100644 --- a/codex-rs/core/src/tools/hook_names.rs +++ b/codex-rs/core/src/tools/hook_names.rs @@ -55,6 +55,15 @@ impl HookToolName { Self::new("Bash") } + /// Adds a matcher-only compatibility name without changing the canonical + /// name serialized into hook stdin. + pub(crate) fn add_matcher_alias(&mut self, alias: impl Into) { + let alias = alias.into(); + if alias != self.name && !self.matcher_aliases.contains(&alias) { + self.matcher_aliases.push(alias); + } + } + /// Returns the canonical hook name serialized into hook stdin. pub(crate) fn name(&self) -> &str { &self.name diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index bc2056da10..a3aa253623 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -1,3 +1,4 @@ +use std::collections::BTreeSet; use std::collections::HashMap; use std::sync::Arc; use std::sync::atomic::AtomicBool; @@ -336,13 +337,18 @@ impl ToolRegistry { #[cfg(test)] #[instrument(level = "trace", skip_all)] pub(crate) fn from_tools(tools: impl IntoIterator>) -> Self { - Self::from_tools_with_namespace_tool_spec_mode(tools, NamespaceToolSpecMode::Preserve) + Self::from_tools_with_namespace_tool_spec_mode( + tools, + NamespaceToolSpecMode::Preserve, + &BTreeSet::new(), + ) } #[instrument(level = "trace", skip_all)] pub(crate) fn from_tools_with_namespace_tool_spec_mode( tools: impl IntoIterator>, namespace_tool_spec_mode: NamespaceToolSpecMode, + suppressed_flat_aliases: &BTreeSet, ) -> Self { let mut tools_by_name = HashMap::new(); for tool in tools { @@ -356,7 +362,7 @@ impl ToolRegistry { let mut registry = Self::new(tools_by_name); if namespace_tool_spec_mode == NamespaceToolSpecMode::Flatten { - registry.add_flat_tool_aliases(); + registry.add_flat_tool_aliases(suppressed_flat_aliases); } registry } @@ -390,12 +396,15 @@ impl ToolRegistry { }) } - fn add_flat_tool_aliases(&mut self) { + fn add_flat_tool_aliases(&mut self, suppressed_flat_aliases: &BTreeSet) { for (name, tool) in &self.tools { if name.namespace.is_none() { continue; } let flat_alias = ToolName::plain(name.canonical_flat_name().into_owned()); + if suppressed_flat_aliases.contains(flat_alias.name.as_str()) { + continue; + } if self.tools.contains_key(&flat_alias) || self.flat_tool_aliases.contains_key(&flat_alias) { @@ -485,11 +494,8 @@ impl ToolRegistry { } let dispatch_trace = ToolDispatchTrace::start(&invocation); - let tool = match self.resolved_tool(&tool_name) { - Some((resolved_tool_name, tool)) => { - invocation.tool_name = resolved_tool_name; - tool - } + let (resolved_tool_name, tool) = match self.resolved_tool(&tool_name) { + Some(resolved_tool) => resolved_tool, None => { let message = unsupported_tool_call_message(&invocation.payload, &tool_name); let log_payload = invocation.payload.log_payload(); @@ -508,6 +514,8 @@ impl ToolRegistry { return Err(err); } }; + let resolved_tool_name_for_hooks = resolved_tool_name.clone(); + invocation.tool_name = resolved_tool_name; let telemetry_tags = tool.telemetry_tags(&invocation).await; let mut tool_result_tags = @@ -541,7 +549,12 @@ impl ToolRegistry { notify_tool_start(&invocation).await; - if let Some(pre_tool_use_payload) = tool.pre_tool_use_payload(&invocation) { + if let Some(mut pre_tool_use_payload) = tool.pre_tool_use_payload(&invocation) { + add_model_visible_flat_alias( + &mut pre_tool_use_payload.tool_name, + &tool_name, + &resolved_tool_name_for_hooks, + ); match run_pre_tool_use_hooks( &invocation.session, &invocation.turn, @@ -621,7 +634,7 @@ impl ToolRegistry { Err(_) => false, }; emit_metric_for_tool_read(&invocation, success); - let post_tool_use_payload = if success { + let mut post_tool_use_payload = if success { let guard = response_cell.lock().await; guard .as_ref() @@ -629,6 +642,13 @@ impl ToolRegistry { } else { None }; + if let Some(post_tool_use_payload) = post_tool_use_payload.as_mut() { + add_model_visible_flat_alias( + &mut post_tool_use_payload.tool_name, + &tool_name, + &resolved_tool_name_for_hooks, + ); + } let post_tool_use_outcome = if let Some(post_tool_use_payload) = post_tool_use_payload { Some( run_post_tool_use_hooks( @@ -772,6 +792,18 @@ fn function_hook_tool_name(invocation: &ToolInvocation) -> HookToolName { HookToolName::new(flat_tool_name(&invocation.tool_name).into_owned()) } +fn add_model_visible_flat_alias( + hook_tool_name: &mut HookToolName, + requested_tool_name: &ToolName, + resolved_tool_name: &ToolName, +) { + // A differing plain request can only come from `flat_tool_aliases`, whose + // keys are the function names exposed when namespace wrappers are flattened. + if requested_tool_name.namespace.is_none() && requested_tool_name != resolved_tool_name { + hook_tool_name.add_matcher_alias(requested_tool_name.name.clone()); + } +} + fn function_hook_tool_input(arguments: &str) -> Value { if arguments.trim().is_empty() { return Value::Object(serde_json::Map::new()); diff --git a/codex-rs/core/src/tools/registry_tests.rs b/codex-rs/core/src/tools/registry_tests.rs index 35bb1a02eb..fcf5a2f780 100644 --- a/codex-rs/core/src/tools/registry_tests.rs +++ b/codex-rs/core/src/tools/registry_tests.rs @@ -188,6 +188,7 @@ fn handler_resolves_flat_aliases_when_namespace_specs_are_flattened() { let registry = ToolRegistry::from_tools_with_namespace_tool_spec_mode( [handler.clone()], codex_tools::NamespaceToolSpecMode::Flatten, + &std::collections::BTreeSet::new(), ); let flat_name = codex_tools::ToolName::plain("mcp__calendar__create_event"); @@ -204,6 +205,59 @@ fn handler_resolves_flat_aliases_when_namespace_specs_are_flattened() { ); } +#[test] +fn suppressed_flat_aliases_keep_logical_runtimes_registered() { + let first_name = codex_tools::ToolName::namespaced("agents", "spawn_agent"); + let second_name = codex_tools::ToolName::namespaced("agents_", "spawn_agent"); + let first_handler = Arc::new(TestHandler { + tool_name: first_name.clone(), + }) as Arc; + let second_handler = Arc::new(TestHandler { + tool_name: second_name.clone(), + }) as Arc; + let registry = ToolRegistry::from_tools_with_namespace_tool_spec_mode( + [first_handler, second_handler], + codex_tools::NamespaceToolSpecMode::Flatten, + &std::collections::BTreeSet::from(["agents__spawn_agent".to_string()]), + ); + + assert_eq!( + registry.tool_names_for_test(), + vec![first_name.clone(), second_name.clone()] + ); + assert!(registry.tool(&first_name).is_some()); + assert!(registry.tool(&second_name).is_some()); + assert!( + registry + .tool(&codex_tools::ToolName::plain("agents__spawn_agent")) + .is_none() + ); +} + +#[test] +fn flattened_model_visible_name_is_only_a_hook_matcher_alias() { + let requested_tool_name = codex_tools::ToolName::plain("web__run"); + let resolved_tool_name = codex_tools::ToolName::namespaced("web", "run"); + let mut hook_tool_name = HookToolName::new("webrun"); + + add_model_visible_flat_alias( + &mut hook_tool_name, + &requested_tool_name, + &resolved_tool_name, + ); + + assert_eq!(hook_tool_name.name(), "webrun"); + assert_eq!(hook_tool_name.matcher_aliases(), ["web__run".to_string()]); + + let mut exact_hook_tool_name = HookToolName::new("webrun"); + add_model_visible_flat_alias( + &mut exact_hook_tool_name, + &resolved_tool_name, + &resolved_tool_name, + ); + assert!(exact_hook_tool_name.matcher_aliases().is_empty()); +} + #[tokio::test] async fn function_tools_expose_default_hook_payloads_and_rewrites() -> anyhow::Result<()> { 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 aca86b124b..4d13d5a9db 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -62,7 +62,7 @@ impl ToolRouter { step_context: &StepContext, params: ToolRouterParams<'_>, tool_search_handler_cache: &ToolSearchHandlerCache, - ) -> Self { + ) -> Result { build_tool_router(step_context, params, tool_search_handler_cache) } diff --git a/codex-rs/core/src/tools/router_tests.rs b/codex-rs/core/src/tools/router_tests.rs index 62391395f8..717bdfb979 100644 --- a/codex-rs/core/src/tools/router_tests.rs +++ b/codex-rs/core/src/tools/router_tests.rs @@ -124,7 +124,8 @@ async fn parallel_support_does_not_match_namespaced_local_tool_names() -> anyhow dynamic_tools: turn.dynamic_tools.as_slice(), }, &Default::default(), - ); + ) + .expect("test tool router should build"); let parallel_tool_name = ["exec_command", "shell_command"] .into_iter() @@ -236,7 +237,8 @@ async fn mcp_parallel_support_uses_handler_data() -> anyhow::Result<()> { dynamic_tools: turn.dynamic_tools.as_slice(), }, &Default::default(), - ); + ) + .expect("test tool router should build"); let call = ToolCall { tool_name: ToolName::namespaced("mcp__echo__", "query_with_delay"), @@ -274,7 +276,8 @@ async fn tools_without_handlers_do_not_support_parallel() -> anyhow::Result<()> dynamic_tools: turn.dynamic_tools.as_slice(), }, &Default::default(), - ); + ) + .expect("test tool router should build"); assert!(!router.tool_supports_parallel(&ToolCall { tool_name: ToolName::plain("web_search"), @@ -331,7 +334,8 @@ async fn specs_filter_deferred_dynamic_tools() -> anyhow::Result<()> { dynamic_tools: &dynamic_tools, }, &Default::default(), - ); + ) + .expect("test tool router should build"); assert_eq!( namespace_function_names(&router.model_visible_specs(), "codex_app"), @@ -398,7 +402,8 @@ async fn extension_tool_executors_are_model_visible_and_dispatchable() -> anyhow dynamic_tools: turn.dynamic_tools.as_slice(), }, &Default::default(), - ); + ) + .expect("test tool router should build"); assert!( router.model_visible_specs().iter().any( diff --git a/codex-rs/core/src/tools/spec_plan.rs b/codex-rs/core/src/tools/spec_plan.rs index b930bd2d51..77dd7dab34 100644 --- a/codex-rs/core/src/tools/spec_plan.rs +++ b/codex-rs/core/src/tools/spec_plan.rs @@ -93,6 +93,7 @@ use codex_tools::serialize_tool_specs; use codex_tools::shell_command_backend_for_features; use codex_tools::shell_type_for_model_and_features; use std::collections::BTreeMap; +use std::collections::BTreeSet; use std::collections::HashSet; use std::sync::Arc; use tracing::instrument; @@ -164,10 +165,10 @@ pub(crate) fn build_tool_router( step_context: &StepContext, params: ToolRouterParams<'_>, tool_search_handler_cache: &ToolSearchHandlerCache, -) -> ToolRouter { +) -> Result { let (model_visible_specs, registry) = - build_tool_specs_and_registry(step_context, params, tool_search_handler_cache); - ToolRouter::from_parts(registry, model_visible_specs) + build_tool_specs_and_registry(step_context, params, tool_search_handler_cache)?; + Ok(ToolRouter::from_parts(registry, model_visible_specs)) } #[instrument(level = "trace", skip_all)] @@ -175,7 +176,7 @@ fn build_tool_specs_and_registry( step_context: &StepContext, params: ToolRouterParams<'_>, tool_search_handler_cache: &ToolSearchHandlerCache, -) -> (Vec, ToolRegistry) { +) -> Result<(Vec, ToolRegistry), String> { let turn_context = step_context.turn.as_ref(); let ToolRouterParams { mcp_tools, @@ -200,9 +201,83 @@ fn build_tool_specs_and_registry( let mut planned_tools = PlannedTools::default(); add_tool_sources(&context, &mut planned_tools); apply_direct_model_only_namespace_overrides(turn_context, &mut planned_tools); + // Reject aliases that could be exposed before deriving tool-search or + // code-mode tools. Keep unreachable logical runtimes registered, but + // suppress their ambiguous flat aliases before building the registry. + let suppressed_flat_aliases = + if namespace_tool_spec_mode(turn_context) == NamespaceToolSpecMode::Flatten { + validate_flattened_tool_name_collisions(turn_context, planned_tools.runtimes())? + } else { + BTreeSet::new() + }; append_tool_search_executor(&context, &mut planned_tools); prepend_code_mode_executors(&context, &mut planned_tools); - build_model_visible_specs_and_registry(turn_context, planned_tools) + build_model_visible_specs_and_registry(turn_context, planned_tools, &suppressed_flat_aliases) +} + +fn validate_flattened_tool_name_collisions( + turn_context: &TurnContext, + runtimes: &[PlannedRuntime], +) -> Result, String> { + let mut logical_names_by_flat_name = BTreeMap::>::new(); + for runtime in runtimes { + let tool_name = runtime.tool_name(); + let exposure = runtime.exposure(); + let reachable = (exposure.is_direct() + && !is_hidden_by_code_mode_only(turn_context, &tool_name, exposure)) + || (exposure == ToolExposure::Deferred + && search_tool_enabled(turn_context) + && runtime.search_info().is_some()); + logical_names_by_flat_name + .entry(tool_name.canonical_flat_name().into_owned()) + .or_default() + .entry(tool_name) + .and_modify(|already_reachable| *already_reachable |= reachable) + .or_insert(reachable); + } + + let mut collisions = Vec::new(); + let mut suppressed_flat_aliases = BTreeSet::new(); + for (flat_name, logical_names) in logical_names_by_flat_name { + if logical_names.len() <= 1 { + continue; + } + let has_reachable_namespaced_tool = logical_names + .iter() + .any(|(tool_name, reachable)| *reachable && tool_name.namespace.is_some()); + let reachable_tool_count = logical_names + .values() + .filter(|reachable| **reachable) + .count(); + let logical_names = logical_names + .keys() + .map(|tool_name| match tool_name.namespace.as_deref() { + Some(namespace) => { + format!("namespace `{namespace}` tool `{}`", tool_name.name) + } + None => format!("plain tool `{}`", tool_name.name), + }) + .collect::>() + .join(", "); + let collision = format!("`{flat_name}`: {logical_names}"); + if has_reachable_namespaced_tool || reachable_tool_count > 1 { + collisions.push(collision); + } else { + warn!( + "ignoring unreachable flattened tool name collision and suppressing its flat alias: {collision}" + ); + suppressed_flat_aliases.insert(flat_name); + } + } + + if collisions.is_empty() { + Ok(suppressed_flat_aliases) + } else { + Err(format!( + "cannot flatten namespace tools because canonical flat tool names collide: {}", + collisions.join("; ") + )) + } } fn apply_direct_model_only_namespace_overrides( @@ -238,7 +313,8 @@ fn apply_direct_model_only_namespace_overrides( fn build_model_visible_specs_and_registry( turn_context: &TurnContext, planned_tools: PlannedTools, -) -> (Vec, ToolRegistry) { + suppressed_flat_aliases: &BTreeSet, +) -> Result<(Vec, ToolRegistry), String> { let PlannedTools { runtimes, hosted_specs, @@ -265,14 +341,38 @@ fn build_model_visible_specs_and_registry( specs.extend(hosted_specs); let namespace_tool_spec_mode = namespace_tool_spec_mode(turn_context); - let registry = - ToolRegistry::from_tools_with_namespace_tool_spec_mode(runtimes, namespace_tool_spec_mode); - let model_visible_specs = dedupe_model_visible_function_names(serialize_tool_specs( - merge_into_namespaces(specs), + let registry = ToolRegistry::from_tools_with_namespace_tool_spec_mode( + runtimes, namespace_tool_spec_mode, - )); + suppressed_flat_aliases, + ); + let model_visible_specs = + serialize_tool_specs(merge_into_namespaces(specs), namespace_tool_spec_mode); + let model_visible_specs = match namespace_tool_spec_mode { + NamespaceToolSpecMode::Preserve => dedupe_model_visible_function_names(model_visible_specs), + NamespaceToolSpecMode::Flatten => { + let mut function_name_counts = BTreeMap::<&str, usize>::new(); + for spec in &model_visible_specs { + if let ToolSpec::Function(tool) = spec { + *function_name_counts.entry(tool.name.as_str()).or_default() += 1; + } + } + let duplicate_function_names = function_name_counts + .into_iter() + .filter(|(_, count)| *count > 1) + .map(|(name, _)| format!("`{name}`")) + .collect::>(); + if !duplicate_function_names.is_empty() { + return Err(format!( + "cannot expose duplicate model-visible function names: {}", + duplicate_function_names.join(", ") + )); + } + model_visible_specs + } + }; - (model_visible_specs, registry) + Ok((model_visible_specs, registry)) } fn spec_for_model_request( diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index 8f13d17a01..d9f6c88257 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -177,10 +177,10 @@ impl ToolPlanProbe { } } -async fn probe_with( +async fn probe_result_with( configure_turn: impl FnOnce(&mut TurnContext), inputs: ToolPlanInputs, -) -> ToolPlanProbe { +) -> Result { let (_session, mut turn) = make_session_and_context().await; configure_turn(&mut turn); let turn = Arc::new(turn); @@ -195,8 +195,17 @@ async fn probe_with( dynamic_tools: inputs.dynamic_tools.as_slice(), }, &Default::default(), - ); - ToolPlanProbe::from_router(router) + )?; + Ok(ToolPlanProbe::from_router(router)) +} + +async fn probe_with( + configure_turn: impl FnOnce(&mut TurnContext), + inputs: ToolPlanInputs, +) -> ToolPlanProbe { + probe_result_with(configure_turn, inputs) + .await + .expect("tool plan should build") } async fn probe(configure_turn: impl FnOnce(&mut TurnContext)) -> ToolPlanProbe { @@ -701,17 +710,20 @@ async fn environment_tools_follow_the_step_context() { /*loaded_agents_md*/ None, )); - let plan = ToolPlanProbe::from_router(ToolRouter::from_context( - step_context.as_ref(), - ToolRouterParams { - mcp_tools: None, - deferred_mcp_tools: None, - tool_suggest_candidates: None, - extension_tool_executors: Vec::new(), - dynamic_tools: &[], - }, - &Default::default(), - )); + let plan = ToolPlanProbe::from_router( + ToolRouter::from_context( + step_context.as_ref(), + ToolRouterParams { + mcp_tools: None, + deferred_mcp_tools: None, + tool_suggest_candidates: None, + extension_tool_executors: Vec::new(), + dynamic_tools: &[], + }, + &Default::default(), + ) + .expect("tool plan should build"), + ); plan.assert_visible_contains(&["exec_command", "apply_patch", "view_image"]); } @@ -858,6 +870,123 @@ async fn mcp_and_tool_search_follow_direct_and_deferred_tool_exposure() { flattened_searchable_mcp.assert_visible_contains(&["tool_search"]); } +#[tokio::test] +async fn namespace_tools_disabled_rejects_flat_alias_collision_with_plain_tool() { + let err = match probe_result_with( + |turn| { + turn.model_info.supports_search_tool = false; + disable_namespace_tools(turn); + set_feature(turn, Feature::MultiAgentV2, /*enabled*/ true); + update_config(turn, |config| { + config.multi_agent_v2.tool_namespace = Some("agents".to_string()); + }); + }, + ToolPlanInputs { + dynamic_tools: vec![dynamic_tool( + None, + "agents__spawn_agent", + // This plain runtime is not discoverable with search disabled, + // but its exact registry name would still steal the alias. + /*defer_loading*/ + true, + )], + ..ToolPlanInputs::default() + }, + ) + .await + { + Ok(_) => panic!("flat alias collision should reject the tool plan"), + Err(err) => err, + }; + + assert_eq!( + err, + "cannot flatten namespace tools because canonical flat tool names collide: `agents__spawn_agent`: namespace `agents` tool `spawn_agent`, plain tool `agents__spawn_agent`" + ); +} + +#[tokio::test] +async fn namespace_tools_disabled_rejects_cross_source_namespaced_flat_collisions() { + let err = match probe_result_with( + |turn| { + disable_namespace_tools(turn); + set_feature(turn, Feature::MultiAgentV2, /*enabled*/ true); + update_config(turn, |config| { + config.multi_agent_v2.tool_namespace = Some("agents".to_string()); + }); + }, + ToolPlanInputs { + dynamic_tools: vec![dynamic_tool( + Some("agents_"), + "spawn_agent", + /*defer_loading*/ false, + )], + ..ToolPlanInputs::default() + }, + ) + .await + { + Ok(_) => panic!("cross-source flat collision should reject the tool plan"), + Err(err) => err, + }; + + assert_eq!( + err, + "cannot flatten namespace tools because canonical flat tool names collide: `agents__spawn_agent`: namespace `agents` tool `spawn_agent`, namespace `agents_` tool `spawn_agent`" + ); +} + +#[tokio::test] +async fn namespace_tools_disabled_rejects_discoverable_flat_collisions() { + let err = match probe_result_with( + |turn| { + turn.model_info.supports_search_tool = true; + disable_namespace_tools(turn); + }, + ToolPlanInputs { + dynamic_tools: vec![ + dynamic_tool(Some("agents"), "spawn_agent", /*defer_loading*/ true), + dynamic_tool(Some("agents_"), "spawn_agent", /*defer_loading*/ true), + ], + ..ToolPlanInputs::default() + }, + ) + .await + { + Ok(_) => panic!("discoverable flat collision should reject the tool plan"), + Err(err) => err, + }; + + assert_eq!( + err, + "cannot flatten namespace tools because canonical flat tool names collide: `agents__spawn_agent`: namespace `agents` tool `spawn_agent`, namespace `agents_` tool `spawn_agent`" + ); +} + +#[tokio::test] +async fn namespace_tools_disabled_ignores_unreachable_flat_collisions() { + let plan = probe_with( + |turn| { + turn.model_info.supports_search_tool = false; + disable_namespace_tools(turn); + }, + ToolPlanInputs { + dynamic_tools: vec![ + dynamic_tool(Some("agents"), "spawn_agent", /*defer_loading*/ true), + dynamic_tool(Some("agents_"), "spawn_agent", /*defer_loading*/ true), + ], + ..ToolPlanInputs::default() + }, + ) + .await; + + plan.assert_visible_lacks(&["agents__spawn_agent", "tool_search"]); + plan.assert_registered_contains(&[ + &ToolName::namespaced("agents", "spawn_agent").to_string(), + &ToolName::namespaced("agents_", "spawn_agent").to_string(), + ]); +} + #[tokio::test] async fn deferred_extension_tools_are_discoverable_with_tool_search() { let plan = probe_with( @@ -895,7 +1024,8 @@ async fn tool_search_cache_rebuilds_when_deferred_sources_change() { dynamic_tools: &[], }, &cache, - ); + ) + .expect("first tool plan should build"); let first_plan = ToolPlanProbe::from_router(first_router); let (_session, mut second_turn) = make_session_and_context().await; @@ -912,7 +1042,8 @@ async fn tool_search_cache_rebuilds_when_deferred_sources_change() { dynamic_tools: &[], }, &cache, - ); + ) + .expect("second tool plan should build"); let second_plan = ToolPlanProbe::from_router(second_router); let ToolSpec::ToolSearch { diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index a5c82d5767..cabbbf607a 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -668,7 +668,11 @@ elif mode == "exit_2": Ok(()) } -fn write_logging_pre_and_blocking_post_tool_use_hooks(home: &Path, feedback: &str) -> Result<()> { +fn write_logging_pre_and_blocking_post_tool_use_hooks( + home: &Path, + matcher: &str, + feedback: &str, +) -> Result<()> { let pre_script_path = home.join("pre_tool_use_hook.py"); let pre_log_path = home.join("pre_tool_use_hook_log.jsonl"); let post_script_path = home.join("post_tool_use_hook.py"); @@ -702,7 +706,7 @@ raise SystemExit(2) let hooks = serde_json::json!({ "hooks": { "PreToolUse": [{ - "matcher": "Bash", + "matcher": matcher, "hooks": [{ "type": "command", "command": format!("python3 {}", pre_script_path.display()), @@ -710,7 +714,7 @@ raise SystemExit(2) }] }], "PostToolUse": [{ - "matcher": "Bash", + "matcher": matcher, "hooks": [{ "type": "command", "command": format!("python3 {}", post_script_path.display()), @@ -4009,6 +4013,76 @@ async fn post_tool_use_spills_large_feedback_message() -> Result<()> { Ok(()) } +#[tokio::test] +async fn flattened_alias_matches_pre_and_post_tool_use_hooks() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "flattened-hook-alias"; + let feedback = "flattened alias post hook ran"; + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "agents__list_agents", "{}"), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "hook alias observed"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let mut builder = test_codex() + .with_pre_build_hook(move |home| { + write_logging_pre_and_blocking_post_tool_use_hooks( + home, + "agents__list_agents", + feedback, + ) + .expect("failed to write flattened alias hook fixture"); + }) + .with_config(|config| { + config + .features + .enable(Feature::MultiAgentV2) + .expect("test config should allow feature update"); + config.multi_agent_v2.tool_namespace = Some("agents".to_string()); + config.model_provider.namespace_tools = Some(false); + trust_discovered_hooks(config); + }); + let test = builder.build(&server).await?; + + test.submit_turn("list agents").await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + let output_item = requests[1].function_call_output(call_id); + let output = output_item + .get("output") + .and_then(Value::as_str) + .expect("list_agents output string"); + assert_eq!(output, feedback); + + let pre_hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?; + assert_eq!(pre_hook_inputs.len(), 1); + assert_eq!(pre_hook_inputs[0]["hook_event_name"], "PreToolUse"); + assert_eq!(pre_hook_inputs[0]["tool_name"], "agentslist_agents"); + assert_eq!(pre_hook_inputs[0]["tool_use_id"], call_id); + + let post_hook_inputs = read_post_tool_use_hook_inputs(test.codex_home_path())?; + assert_eq!(post_hook_inputs.len(), 1); + assert_eq!(post_hook_inputs[0]["hook_event_name"], "PostToolUse"); + assert_eq!(post_hook_inputs[0]["tool_name"], "agentslist_agents"); + assert_eq!(post_hook_inputs[0]["tool_use_id"], call_id); + + Ok(()) +} + #[tokio::test] async fn post_tool_use_blocks_when_exec_session_completes_via_write_stdin() -> Result<()> { skip_if_no_network!(Ok(())); @@ -4063,7 +4137,7 @@ async fn post_tool_use_blocks_when_exec_session_completes_via_write_stdin() -> R let mut builder = test_codex() .with_pre_build_hook(|home| { - write_logging_pre_and_blocking_post_tool_use_hooks(home, feedback) + write_logging_pre_and_blocking_post_tool_use_hooks(home, "Bash", feedback) .expect("failed to write tool use hook test fixture"); }) .with_config(|config| { diff --git a/codex-rs/core/tests/suite/responses_lite.rs b/codex-rs/core/tests/suite/responses_lite.rs index 21ea32a8da..e129c79399 100644 --- a/codex-rs/core/tests/suite/responses_lite.rs +++ b/codex-rs/core/tests/suite/responses_lite.rs @@ -282,6 +282,7 @@ async fn responses_lite_exposes_standalone_tools_for_actor_authorized_provider() configure_responses_tools(config); config.model_provider.name = "local".to_string(); config.model_provider.requires_openai_auth = false; + config.model_provider.namespace_tools = Some(true); config.model_provider.http_headers = Some(HashMap::from([( "x-openai-actor-authorization".to_string(), "test-actor-authorization".to_string(),