diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 6c0b48b1bd5..dcd1edf80c8 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -79,26 +79,60 @@ pub const DEFAULT_STARTUP_TIMEOUT: Duration = Duration::from_secs(10); /// Default timeout for individual tool calls. const DEFAULT_TOOL_TIMEOUT: Duration = Duration::from_secs(60); +/// The Responses API requires tool names to match `^[a-zA-Z0-9_-]+$`. +/// MCP server/tool names are user-controlled, so sanitize the fully-qualified +/// name we expose to the model by replacing any disallowed character with `_`. +fn sanitize_responses_api_tool_name(name: &str) -> String { + let mut sanitized = String::with_capacity(name.len()); + for c in name.chars() { + if c.is_ascii_alphanumeric() || c == '_' || c == '-' { + sanitized.push(c); + } else { + sanitized.push('_'); + } + } + + if sanitized.is_empty() { + "_".to_string() + } else { + sanitized + } +} + +fn sha1_hex(s: &str) -> String { + let mut hasher = Sha1::new(); + hasher.update(s.as_bytes()); + let sha1 = hasher.finalize(); + format!("{sha1:x}") +} + fn qualify_tools(tools: I) -> HashMap where I: IntoIterator, { let mut used_names = HashSet::new(); + let mut seen_raw_names = HashSet::new(); let mut qualified_tools = HashMap::new(); for tool in tools { - let mut qualified_name = format!( + let qualified_name_raw = format!( "mcp{}{}{}{}", MCP_TOOL_NAME_DELIMITER, tool.server_name, MCP_TOOL_NAME_DELIMITER, tool.tool_name ); - if qualified_name.len() > MAX_TOOL_NAME_LENGTH { - let mut hasher = Sha1::new(); - hasher.update(qualified_name.as_bytes()); - let sha1 = hasher.finalize(); - let sha1_str = format!("{sha1:x}"); + if !seen_raw_names.insert(qualified_name_raw.clone()) { + warn!("skipping duplicated tool {}", qualified_name_raw); + continue; + } - // Truncate to make room for the hash suffix - let prefix_len = MAX_TOOL_NAME_LENGTH - sha1_str.len(); + // Start from a "pretty" name (sanitized), then deterministically disambiguate on + // collisions by appending a hash of the *raw* (unsanitized) qualified name. This + // ensures tools like `foo.bar` and `foo_bar` don't collapse to the same key. + let mut qualified_name = sanitize_responses_api_tool_name(&qualified_name_raw); + // Enforce length constraints early; use the raw name for the hash input so the + // output remains stable even when sanitization changes. + if qualified_name.len() > MAX_TOOL_NAME_LENGTH { + let sha1_str = sha1_hex(&qualified_name_raw); + let prefix_len = MAX_TOOL_NAME_LENGTH - sha1_str.len(); qualified_name = format!("{}{}", &qualified_name[..prefix_len], sha1_str); } @@ -1035,6 +1069,28 @@ mod tests { ); } + #[test] + fn test_qualify_tools_sanitizes_invalid_characters() { + let tools = vec![create_test_tool("server.one", "tool.two")]; + + let qualified_tools = qualify_tools(tools); + + assert_eq!(qualified_tools.len(), 1); + let (qualified_name, tool) = qualified_tools.into_iter().next().expect("one tool"); + assert_eq!(qualified_name, "mcp__server_one__tool_two"); + + // The key is sanitized for OpenAI, but we keep original parts for the actual MCP call. + assert_eq!(tool.server_name, "server.one"); + assert_eq!(tool.tool_name, "tool.two"); + + assert!( + qualified_name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-'), + "qualified name must be Responses API compatible: {qualified_name:?}" + ); + } + #[test] fn tool_filter_allows_by_default() { let filter = ToolFilter::default();