mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Remove ToolName display helper (#21465)
## Why `ToolName::display()` made it too easy to flatten tool identity and accidentally compare rendered strings. Tool identity should stay structural until a legacy string boundary actually requires the flattened spelling. ## What - Removes `ToolName::display()` and relies on the existing `Display` impl for messages and errors. - Adds structural ordering for `ToolName` and uses it for sorting/deduping deferred tools. - Carries `ToolName` through tool/sandbox plumbing, flattening only at legacy boundaries such as hook payloads, telemetry tags, and Responses tool names. - Updates MCP normalization tests to assert `ToolName` structure instead of rendered strings. ## Testing - `cargo test -p codex-mcp test_normalize_tools` - `cargo test -p codex-core unavailable_tool` - `just fix -p codex-protocol` - `just fix -p codex-mcp` - `just fix -p codex-core`
This commit is contained in:
1 parent
bbb6bf0a37
commit
61142b6169
25 files changed
+123
-71
No files matched your search
@@ -93,6 +93,18 @@ fn model_tool_names(tools: &[ToolInfo]) -> HashSet<ToolName> {
|
||||
.collect::<HashSet<_>>()
|
||||
}
|
||||
|
||||
fn model_tool_name_len(name: &ToolName) -> usize {
|
||||
name.namespace.as_deref().map_or(0, str::len) + name.name.len()
|
||||
}
|
||||
|
||||
fn is_code_mode_compatible_tool_name(name: &ToolName) -> bool {
|
||||
name.namespace
|
||||
.as_deref()
|
||||
.into_iter()
|
||||
.chain(std::iter::once(name.name.as_str()))
|
||||
.flat_map(str::chars)
|
||||
.all(|c| c.is_ascii_alphanumeric() || c == '_')
|
||||
}
|
||||
#[test]
|
||||
fn declared_openai_file_fields_treat_names_literally() {
|
||||
let meta = serde_json::json!({
|
||||
@@ -334,17 +346,14 @@ fn test_normalize_tools_long_names_same_server() {
|
||||
|
||||
let names = model_tool_names(&model_tools);
|
||||
|
||||
assert!(names.iter().all(|name| name.display().len() == 64));
|
||||
assert!(names.iter().all(|name| model_tool_name_len(name) == 64));
|
||||
assert!(
|
||||
names
|
||||
.iter()
|
||||
.all(|name| name.namespace.as_deref() == Some("mcp__my_server__"))
|
||||
);
|
||||
assert!(
|
||||
names.iter().all(|name| name
|
||||
.display()
|
||||
.chars()
|
||||
.all(|c| c.is_ascii_alphanumeric() || c == '_')),
|
||||
names.iter().all(is_code_mode_compatible_tool_name),
|
||||
"model-visible names must be code-mode compatible: {names:?}"
|
||||
);
|
||||
}
|
||||
@@ -363,10 +372,9 @@ fn test_normalize_tools_sanitizes_invalid_characters() {
|
||||
ToolName::namespaced("mcp__server_one__", "tool_two_three")
|
||||
);
|
||||
assert_eq!(
|
||||
format!("{}{}", tool.callable_namespace, tool.callable_name),
|
||||
model_name.display()
|
||||
ToolName::namespaced(tool.callable_namespace.clone(), tool.callable_name.clone()),
|
||||
model_name
|
||||
);
|
||||
|
||||
// The callable parts are sanitized for model-visible tool calls, but the raw
|
||||
// MCP name is preserved for the actual MCP call.
|
||||
assert_eq!(tool.server_name, "server.one");
|
||||
@@ -375,10 +383,7 @@ fn test_normalize_tools_sanitizes_invalid_characters() {
|
||||
assert_eq!(tool.tool.name, "tool.two-three");
|
||||
|
||||
assert!(
|
||||
model_name
|
||||
.display()
|
||||
.chars()
|
||||
.all(|c| c.is_ascii_alphanumeric() || c == '_'),
|
||||
is_code_mode_compatible_tool_name(&model_name),
|
||||
"model-visible name must be code-mode compatible: {model_name:?}"
|
||||
);
|
||||
}
|
||||
@@ -425,10 +430,7 @@ fn test_normalize_tools_disambiguates_sanitized_namespace_collisions() {
|
||||
assert_eq!(raw_servers, HashSet::from(["basic-server", "basic_server"]));
|
||||
let model_names = model_tool_names(&model_tools);
|
||||
assert!(
|
||||
model_names.iter().all(|name| name
|
||||
.display()
|
||||
.chars()
|
||||
.all(|c| c.is_ascii_alphanumeric() || c == '_')),
|
||||
model_names.iter().all(is_code_mode_compatible_tool_name),
|
||||
"model-visible names must be code-mode compatible: {model_names:?}"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user