Enforce strict tool name collision errors (#37020)

## What changed

- Track the first duplicate effective tool name while assembling the tool registry, including collisions introduced by external tools, code mode, and tool search.
- When `[features.tool_registry].error_on_tool_collisions` is enabled, fail the turn with a `duplicate tool: <namespace>.<name>` error before sending a model request.
- Continue allowing identical tool names in different namespaces, and preserve the existing collision behavior when strict checking is disabled.

## Testing

- Cover registry collision tracking, strict tool planning across tool sources, namespace isolation, failure before sampling, and pre-sampling compaction.

GitOrigin-RevId: 4cf83538fb513cec0b9c8b567780caaaadd3243d
This commit is contained in:
rka-oai
2026-08-05 04:00:50 +00:00
committed by copyberry
parent 5d89ab65dc
commit 1e489adad0
13 changed files with 374 additions and 17 deletions
+1 -1
View File
@@ -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()
+1 -1
View File
@@ -3102,7 +3102,7 @@ impl Session {
prepared_recommendations,
)
.or_cancel(cancellation_token)
.await?;
.await??;
Ok(Arc::new(StepContext {
turn: turn_context,
environments,
+6 -3
View File
@@ -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<ToolRouter> {
) -> CodexResult<Arc<ToolRouter>> {
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)]
+15 -3
View File
@@ -251,14 +251,13 @@ pub(crate) struct RegisteredTool {
#[derive(Default)]
pub struct ToolRegistry {
tools: IndexMap<ToolName, RegisteredTool>,
first_collision: Option<ToolName>,
}
impl ToolRegistry {
#[cfg(test)]
pub(crate) fn from_tools(tools: impl IntoIterator<Item = Arc<dyn CoreToolRuntime>>) -> 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<Arc<dyn CoreToolRuntime>> {
self.tools.shift_remove(tool_name).map(|tool| tool.runtime)
}
+42
View File
@@ -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<dyn CoreToolRuntime>;
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<dyn CoreToolRuntime>;
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;
+1
View File
@@ -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<ToolSpec>) -> Self {
+24 -6
View File
@@ -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<ToolRouter> {
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<ToolSpec>,
tool_search_handler_cache: &ToolSearchHandlerCache,
) -> ToolRouter {
) -> CodexResult<ToolRouter> {
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, &registry, &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(
+139
View File
@@ -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 [
+133
View File
@@ -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<String> {
body.get("tools")
@@ -56,6 +66,129 @@ fn tool_names(body: &Value) -> Vec<String> {
.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(()));
+2 -2
View File
@@ -654,9 +654,9 @@ pub fn canonical_feature_for_key(key: &str) -> Option<Feature> {
.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.
+2 -1
View File
@@ -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]
+4
View File
@@ -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(_)
+4
View File
@@ -56,6 +56,10 @@ fn retryability_preserves_error_details_distinctions() {
}),
true,
),
(
CodexErrorDetails::ToolCollision("functions.update_plan".to_string()).into(),
false,
),
(CodexErr::InternalServerError, true),
];