mirror of
https://github.com/openai/codex.git
synced 2026-11-12 21:20:34 +00:00
Resolve normalized tool name collisions in code mode (#36129)
## What changed - Track the first registered tool for each normalized code-mode identifier and use that tool consistently for dispatch metadata and model-facing code-mode declarations. - Skip duplicate normalized names when building the code-mode executor while preserving shadowed tools in the registry and direct tool exposure where applicable. - Cover collisions across dynamic and namespaced tools, direct and deferred exposure, code-mode-only operation, and Responses Lite metadata. GitOrigin-RevId: a4acb57ce44e7de1398981b22af4b87e2a35870f
This commit is contained in:
@@ -90,6 +90,7 @@ use codex_tools::shell_command_backend_for_features;
|
||||
use codex_tools::shell_type_for_model_and_features;
|
||||
use futures::future::BoxFuture;
|
||||
use std::collections::BTreeMap;
|
||||
use std::collections::btree_map::Entry;
|
||||
use std::sync::Arc;
|
||||
use tracing::instrument;
|
||||
|
||||
@@ -261,9 +262,10 @@ pub(crate) fn finalize_tool_router(
|
||||
append_tool_search_executor(turn_context, &mut registry, tool_search_handler_cache);
|
||||
}
|
||||
|
||||
register_code_mode_executors(turn_context, &mut registry);
|
||||
let code_mode_tool_names = register_code_mode_executors(turn_context, &mut registry);
|
||||
|
||||
let model_visible_specs = build_model_visible_specs(turn_context, ®istry, hosted_specs);
|
||||
let model_visible_specs =
|
||||
build_model_visible_specs(turn_context, ®istry, &code_mode_tool_names, hosted_specs);
|
||||
ToolRouter::from_parts(registry, model_visible_specs)
|
||||
}
|
||||
|
||||
@@ -293,6 +295,7 @@ fn apply_direct_model_only_namespace_overrides(
|
||||
fn build_model_visible_specs(
|
||||
turn_context: &TurnContext,
|
||||
registry: &ToolRegistry,
|
||||
code_mode_tool_names: &BTreeMap<String, ToolName>,
|
||||
hosted_specs: Vec<ToolSpec>,
|
||||
) -> Vec<ToolSpec> {
|
||||
let mut specs = Vec::new();
|
||||
@@ -306,6 +309,7 @@ fn build_model_visible_specs(
|
||||
turn_context,
|
||||
exposure,
|
||||
&tool_name,
|
||||
code_mode_tool_names,
|
||||
spec,
|
||||
));
|
||||
}
|
||||
@@ -324,6 +328,7 @@ fn spec_for_model_request(
|
||||
turn_context: &TurnContext,
|
||||
exposure: ToolExposure,
|
||||
tool_name: &ToolName,
|
||||
code_mode_tool_names: &BTreeMap<String, ToolName>,
|
||||
spec: ToolSpec,
|
||||
) -> ToolSpec {
|
||||
let tool_mode = effective_tool_mode(turn_context);
|
||||
@@ -331,6 +336,11 @@ fn spec_for_model_request(
|
||||
&& exposure.is_available_in_code_mode()
|
||||
&& !is_excluded_from_code_mode(turn_context, tool_name)
|
||||
&& codex_code_mode::is_code_mode_nested_tool(spec.name())
|
||||
&& code_mode_tool_names
|
||||
.get(&codex_code_mode::normalize_code_mode_identifier(
|
||||
&codex_tools::code_mode_name_for_tool_name(tool_name),
|
||||
))
|
||||
.is_some_and(|winner| winner == tool_name)
|
||||
{
|
||||
codex_tools::augment_tool_spec_for_code_mode(spec)
|
||||
} else {
|
||||
@@ -495,12 +505,16 @@ fn is_excluded_from_code_mode(turn_context: &TurnContext, tool_name: &ToolName)
|
||||
})
|
||||
}
|
||||
|
||||
fn register_code_mode_executors(turn_context: &TurnContext, registry: &mut ToolRegistry) {
|
||||
fn register_code_mode_executors(
|
||||
turn_context: &TurnContext,
|
||||
registry: &mut ToolRegistry,
|
||||
) -> BTreeMap<String, ToolName> {
|
||||
let tool_mode = effective_tool_mode(turn_context);
|
||||
if !matches!(tool_mode, ToolMode::CodeMode | ToolMode::CodeModeOnly) {
|
||||
return;
|
||||
return BTreeMap::new();
|
||||
}
|
||||
|
||||
let mut code_mode_tool_names = BTreeMap::new();
|
||||
let mut code_mode_nested_tool_specs = Vec::new();
|
||||
let mut exec_prompt_tool_specs = Vec::new();
|
||||
let mut deferred_exec_prompt_tool_specs = Vec::new();
|
||||
@@ -511,11 +525,33 @@ fn register_code_mode_executors(turn_context: &TurnContext, registry: &mut ToolR
|
||||
continue;
|
||||
}
|
||||
|
||||
if is_excluded_from_code_mode(turn_context, &executor.tool_name()) {
|
||||
let tool_name = executor.tool_name();
|
||||
if is_excluded_from_code_mode(turn_context, &tool_name) {
|
||||
continue;
|
||||
}
|
||||
|
||||
let spec = executor.spec();
|
||||
let Some(code_mode_tool) = codex_tools::tool_spec_to_code_mode_tool_definition(&spec)
|
||||
else {
|
||||
continue;
|
||||
};
|
||||
|
||||
let code_mode_name = code_mode_tool.name;
|
||||
match code_mode_tool_names.entry(codex_code_mode::normalize_code_mode_identifier(
|
||||
&code_mode_name,
|
||||
)) {
|
||||
Entry::Vacant(entry) => {
|
||||
entry.insert(tool_name);
|
||||
}
|
||||
Entry::Occupied(_) => {
|
||||
tracing::warn!(
|
||||
tool_name = %tool_name,
|
||||
code_mode_name = %code_mode_name,
|
||||
"skipping tool with a duplicate normalized code-mode name"
|
||||
);
|
||||
continue;
|
||||
}
|
||||
}
|
||||
|
||||
if exposure == ToolExposure::Deferred {
|
||||
if deferred_tools_guidance_enabled {
|
||||
@@ -528,19 +564,9 @@ fn register_code_mode_executors(turn_context: &TurnContext, registry: &mut ToolR
|
||||
}
|
||||
|
||||
if turn_context.model_info.use_responses_lite {
|
||||
let code_mode_tool_names =
|
||||
collect_code_mode_exec_prompt_tool_definitions(code_mode_nested_tool_specs.iter())
|
||||
.into_iter()
|
||||
.map(|tool| {
|
||||
(
|
||||
codex_code_mode::normalize_code_mode_identifier(&tool.name),
|
||||
tool.tool_name,
|
||||
)
|
||||
})
|
||||
.collect();
|
||||
turn_context
|
||||
.turn_metadata_state
|
||||
.set_code_mode_tool_names(code_mode_tool_names);
|
||||
.set_code_mode_tool_names(code_mode_tool_names.clone());
|
||||
}
|
||||
|
||||
let namespace_descriptions = code_mode_namespace_descriptions(&exec_prompt_tool_specs);
|
||||
@@ -567,6 +593,8 @@ fn register_code_mode_executors(turn_context: &TurnContext, registry: &mut ToolR
|
||||
|
||||
registry.prepend_trusted(Arc::new(CodeModeWaitHandler));
|
||||
registry.prepend_trusted(Arc::new(execute_handler));
|
||||
|
||||
code_mode_tool_names
|
||||
}
|
||||
|
||||
#[instrument(level = "trace", skip_all, fields(tool_spec_count = specs.len()))]
|
||||
|
||||
@@ -31,6 +31,7 @@ use serde_json::json;
|
||||
use crate::WaitForEnvironmentToolConfig;
|
||||
use crate::config::CurrentTimeReminderConfig;
|
||||
use crate::environment_selection::TurnEnvironmentState;
|
||||
use crate::responses_metadata::CodexResponsesRequestKind;
|
||||
use crate::session::step_context::StepContext;
|
||||
use crate::session::tests::make_session_and_context;
|
||||
use crate::session::tests::mcp_config_for_test;
|
||||
@@ -1079,6 +1080,93 @@ async fn unified_tool_runtimes_preserve_source_order_and_collision_priority() {
|
||||
assert_eq!(tool.description, "lookup test tool");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn code_mode_uses_the_first_normalized_tool_identity() {
|
||||
for (code_mode_only, winner_exposure, shadow_is_deferred) in [
|
||||
(false, ToolExposure::Direct, true),
|
||||
(false, ToolExposure::Deferred, false),
|
||||
(true, ToolExposure::Direct, true),
|
||||
(true, ToolExposure::Deferred, false),
|
||||
] {
|
||||
let mut metadata_state = None;
|
||||
let plan = probe_with(
|
||||
|turn| {
|
||||
set_feature(turn, Feature::CodeMode, /*enabled*/ true);
|
||||
if code_mode_only {
|
||||
set_feature(turn, Feature::CodeModeOnly, /*enabled*/ true);
|
||||
}
|
||||
turn.model_info.supports_search_tool = true;
|
||||
turn.model_info.use_responses_lite = true;
|
||||
metadata_state = Some(Arc::clone(&turn.turn_metadata_state));
|
||||
},
|
||||
ToolPlanInputs {
|
||||
tool_runtimes: vec![mcp_runtime(
|
||||
"winner",
|
||||
"normalized-alias",
|
||||
"lookup",
|
||||
winner_exposure,
|
||||
)],
|
||||
dynamic_tools: vec![dynamic_tool(
|
||||
Some("normalized_alias"),
|
||||
"lookup",
|
||||
shadow_is_deferred,
|
||||
)],
|
||||
..ToolPlanInputs::default()
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
||||
let winner_name = ToolName::namespaced("normalized-alias", "lookup");
|
||||
let shadow_name = ToolName::namespaced("normalized_alias", "lookup");
|
||||
plan.assert_registered_contains(&[&winner_name.to_string(), &shadow_name.to_string()]);
|
||||
assert_eq!(plan.exposure(&winner_name.to_string()), winner_exposure);
|
||||
assert_eq!(
|
||||
plan.exposure(&shadow_name.to_string()),
|
||||
if shadow_is_deferred {
|
||||
ToolExposure::Deferred
|
||||
} else {
|
||||
ToolExposure::Direct
|
||||
},
|
||||
);
|
||||
|
||||
let metadata = metadata_state
|
||||
.expect("tool planning should capture the turn metadata")
|
||||
.to_responses_metadata(
|
||||
"installation".to_string(),
|
||||
"window".to_string(),
|
||||
CodexResponsesRequestKind::Turn,
|
||||
);
|
||||
let code_mode_tool_names = metadata
|
||||
.code_mode_tool_names
|
||||
.as_ref()
|
||||
.expect("Responses Lite should receive the supported code-mode tools");
|
||||
assert_eq!(
|
||||
code_mode_tool_names.get("normalized_alias__lookup"),
|
||||
Some(&winner_name),
|
||||
);
|
||||
assert!(!code_mode_tool_names.contains_key(codex_tools::TOOL_SEARCH_TOOL_NAME));
|
||||
|
||||
if !code_mode_only && !shadow_is_deferred {
|
||||
let ToolSpec::Namespace(namespace) = plan.visible_spec("normalized_alias") else {
|
||||
panic!("expected the shadowed dynamic tool to remain directly visible");
|
||||
};
|
||||
let [ResponsesApiNamespaceTool::Function(shadow)] = namespace.tools.as_slice() else {
|
||||
panic!("expected exactly one shadowed dynamic tool");
|
||||
};
|
||||
assert_eq!(shadow.description, "lookup dynamic tool");
|
||||
}
|
||||
|
||||
let ToolSpec::Freeform(exec) = plan.visible_spec(codex_code_mode::PUBLIC_TOOL_NAME) else {
|
||||
panic!("expected code mode exec tool");
|
||||
};
|
||||
assert!(!exec.description.contains("lookup dynamic tool"));
|
||||
assert_eq!(
|
||||
exec.description.contains("lookup test tool"),
|
||||
code_mode_only && winner_exposure == ToolExposure::Direct,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn deferred_extension_tools_are_discoverable_with_tool_search() {
|
||||
let plan = probe_with(
|
||||
|
||||
@@ -21,6 +21,7 @@ use codex_protocol::dynamic_tools::DynamicToolNamespaceTool;
|
||||
use codex_protocol::dynamic_tools::DynamicToolResponse;
|
||||
use codex_protocol::dynamic_tools::DynamicToolSpec;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::openai_models::ToolMode;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::Op;
|
||||
@@ -3704,6 +3705,238 @@ text(JSON.stringify(tool));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn code_mode_uses_the_first_dynamic_tool_for_a_normalized_name() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
for use_responses_lite in [false, true] {
|
||||
let server = responses::start_mock_server().await;
|
||||
let mut builder = test_codex()
|
||||
.with_model_info_override("gpt-5.5", move |model_info| {
|
||||
model_info.use_responses_lite = use_responses_lite;
|
||||
model_info.tool_mode = Some(ToolMode::CodeMode);
|
||||
})
|
||||
.with_config(|config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::CodeMode)
|
||||
.expect("code mode should be enabled");
|
||||
});
|
||||
let base_test = builder.build_with_auto_env(&server).await?;
|
||||
let new_thread = base_test
|
||||
.thread_manager
|
||||
.start_thread(StartThreadOptions {
|
||||
dynamic_tools: [
|
||||
("foo-bar", "First normalized dynamic tool."),
|
||||
("foo_bar", "Shadowed normalized dynamic tool."),
|
||||
]
|
||||
.into_iter()
|
||||
.map(|(name, description)| {
|
||||
DynamicToolSpec::Function(DynamicToolFunctionSpec {
|
||||
name: name.to_string(),
|
||||
description: description.to_string(),
|
||||
input_schema: serde_json::json!({
|
||||
"type": "object",
|
||||
"properties": {},
|
||||
"additionalProperties": false,
|
||||
}),
|
||||
defer_loading: false,
|
||||
})
|
||||
})
|
||||
.collect(),
|
||||
..StartThreadOptions::new(base_test.config.clone())
|
||||
})
|
||||
.await?;
|
||||
let mut test = base_test;
|
||||
test.codex = new_thread.thread;
|
||||
test.session_configured = new_thread.session_configured;
|
||||
|
||||
let first_mock = responses::mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_custom_tool_call(
|
||||
"call-1",
|
||||
"exec",
|
||||
r#"
|
||||
const matches = ALL_TOOLS.filter(({ name }) => name === "foo_bar");
|
||||
const output = await tools.foo_bar({});
|
||||
text(JSON.stringify({
|
||||
count: matches.length,
|
||||
name: matches[0]?.name ?? null,
|
||||
description: matches[0]?.description ?? null,
|
||||
output,
|
||||
}));
|
||||
"#,
|
||||
),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let second_mock = responses::mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_assistant_message("msg-1", "done"),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let cwd = test.config.cwd.clone();
|
||||
let (sandbox_policy, permission_profile) =
|
||||
turn_permission_fields(PermissionProfile::Disabled, cwd.as_path());
|
||||
test.codex
|
||||
.submit(Op::UserInput {
|
||||
items: vec![UserInput::Text {
|
||||
text: "inspect and call normalized dynamic tools".to_string(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
final_output_json_schema: None,
|
||||
responsesapi_client_metadata: None,
|
||||
additional_context: Default::default(),
|
||||
thread_settings: codex_protocol::protocol::ThreadSettingsOverrides {
|
||||
environments: Some(codex_protocol::protocol::TurnEnvironmentSelections::new(
|
||||
cwd,
|
||||
Vec::new(),
|
||||
)),
|
||||
approval_policy: Some(AskForApproval::Never),
|
||||
sandbox_policy: Some(sandbox_policy),
|
||||
permission_profile,
|
||||
collaboration_mode: Some(codex_protocol::config_types::CollaborationMode {
|
||||
mode: codex_protocol::config_types::ModeKind::Default,
|
||||
settings: codex_protocol::config_types::Settings {
|
||||
model: test.session_configured.model.clone(),
|
||||
reasoning_effort: None,
|
||||
developer_instructions: None,
|
||||
},
|
||||
}),
|
||||
..Default::default()
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
||||
let turn_id = wait_for_event_match(&test.codex, |event| match event {
|
||||
EventMsg::TurnStarted(event) => Some(event.turn_id.clone()),
|
||||
_ => None,
|
||||
})
|
||||
.await;
|
||||
let request = wait_for_event_match(&test.codex, |event| match event {
|
||||
EventMsg::DynamicToolCallRequest(request) => Some(request.clone()),
|
||||
_ => None,
|
||||
})
|
||||
.await;
|
||||
assert_eq!(request.namespace, None);
|
||||
assert_eq!(request.tool, "foo-bar");
|
||||
assert_eq!(request.arguments, serde_json::json!({}));
|
||||
test.codex
|
||||
.submit(Op::DynamicToolResponse {
|
||||
id: request.call_id,
|
||||
response: DynamicToolResponse {
|
||||
content_items: vec![DynamicToolCallOutputContentItem::InputText {
|
||||
text: "first-winner".to_string(),
|
||||
}],
|
||||
success: true,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
wait_for_event(&test.codex, |event| match event {
|
||||
EventMsg::TurnComplete(event) => event.turn_id == turn_id,
|
||||
_ => false,
|
||||
})
|
||||
.await;
|
||||
|
||||
let first_body = first_mock.single_request().body_json();
|
||||
let model_tools = if use_responses_lite {
|
||||
first_body["input"]
|
||||
.as_array()
|
||||
.and_then(|input| {
|
||||
input.iter().find(|item| {
|
||||
item.get("type").and_then(Value::as_str) == Some("additional_tools")
|
||||
})
|
||||
})
|
||||
.and_then(|item| item["tools"].as_array())
|
||||
.expect("the Responses Lite request should contain its additional tools")
|
||||
} else {
|
||||
first_body["tools"]
|
||||
.as_array()
|
||||
.expect("the Responses request should contain its visible tools")
|
||||
};
|
||||
let visible_dynamic_tools = model_tools
|
||||
.iter()
|
||||
.filter(|tool| matches!(tool["name"].as_str(), Some("foo-bar" | "foo_bar")))
|
||||
.map(|tool| {
|
||||
(
|
||||
tool["name"]
|
||||
.as_str()
|
||||
.expect("dynamic tools should have a name"),
|
||||
tool["description"]
|
||||
.as_str()
|
||||
.expect("dynamic tools should have a description"),
|
||||
)
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(
|
||||
visible_dynamic_tools,
|
||||
[
|
||||
(
|
||||
"foo-bar",
|
||||
concat!(
|
||||
"First normalized dynamic tool.\n\n",
|
||||
"exec tool declaration:\n",
|
||||
"```ts\n",
|
||||
"declare const tools: { foo_bar(args: {}): Promise<unknown>; };\n",
|
||||
"```",
|
||||
),
|
||||
),
|
||||
("foo_bar", "Shadowed normalized dynamic tool."),
|
||||
]
|
||||
);
|
||||
|
||||
if use_responses_lite {
|
||||
let metadata: Value = serde_json::from_str(
|
||||
first_body["client_metadata"]["x-codex-turn-metadata"]
|
||||
.as_str()
|
||||
.expect("Responses Lite should contain serialized turn metadata"),
|
||||
)?;
|
||||
assert_eq!(
|
||||
metadata["code_mode_tool_names"]["foo_bar"],
|
||||
serde_json::json!({
|
||||
"name": "foo-bar",
|
||||
"namespace": null,
|
||||
}),
|
||||
);
|
||||
assert!(
|
||||
metadata["code_mode_tool_names"]
|
||||
.get("tool_search")
|
||||
.is_none()
|
||||
);
|
||||
}
|
||||
|
||||
let exec_description = model_tools
|
||||
.iter()
|
||||
.find(|tool| tool["name"] == "exec")
|
||||
.and_then(|tool| tool["description"].as_str())
|
||||
.expect("the model request should contain the code-mode exec tool");
|
||||
assert!(!exec_description.contains("First normalized dynamic tool."));
|
||||
assert!(!exec_description.contains("Shadowed normalized dynamic tool."));
|
||||
|
||||
let request = second_mock.single_request();
|
||||
let output = custom_tool_output_last_non_empty_text(&request, "call-1")
|
||||
.expect("code mode should return normalized tool metadata");
|
||||
let result: Value = serde_json::from_str(&output)?;
|
||||
assert_eq!(result["count"], serde_json::json!(1));
|
||||
assert_eq!(result["name"], serde_json::json!("foo_bar"));
|
||||
assert_eq!(result["output"], serde_json::json!("first-winner"));
|
||||
let description = result["description"]
|
||||
.as_str()
|
||||
.expect("the winning tool should have a description");
|
||||
assert!(description.contains("First normalized dynamic tool."));
|
||||
assert!(!description.contains("Shadowed normalized dynamic tool."));
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn code_mode_can_call_hidden_dynamic_tools() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
Reference in New Issue
Block a user