diff --git a/codex-rs/core/src/tools/handlers/dynamic.rs b/codex-rs/core/src/tools/handlers/dynamic.rs index 40e132958b..87609b8cb1 100644 --- a/codex-rs/core/src/tools/handlers/dynamic.rs +++ b/codex-rs/core/src/tools/handlers/dynamic.rs @@ -33,7 +33,7 @@ impl ToolHandler for DynamicToolHandler { fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { Some(PreToolUsePayload { - tool_name: HookToolName::for_function_tool(&invocation.tool_name), + tool_name: HookToolName::for_dynamic_tool(&invocation.tool_name), tool_input: dynamic_tool_input(invocation).ok()?, }) } @@ -44,7 +44,7 @@ impl ToolHandler for DynamicToolHandler { result: &Self::Output, ) -> Option { Some(PostToolUsePayload { - tool_name: HookToolName::for_function_tool(&invocation.tool_name), + tool_name: HookToolName::for_dynamic_tool(&invocation.tool_name), tool_use_id: invocation.call_id.clone(), tool_input: dynamic_tool_input(invocation).ok()?, tool_response: result diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index ccd0254684..3cd01c9a87 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -29,7 +29,7 @@ impl ToolHandler for McpHandler { }; Some(PreToolUsePayload { - tool_name: HookToolName::for_function_tool(&invocation.tool_name), + tool_name: HookToolName::for_mcp_tool(&invocation.tool_name), tool_input: mcp_hook_tool_input(raw_arguments), }) } @@ -46,7 +46,7 @@ impl ToolHandler for McpHandler { let tool_response = result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; Some(PostToolUsePayload { - tool_name: HookToolName::for_function_tool(&invocation.tool_name), + tool_name: HookToolName::for_mcp_tool(&invocation.tool_name), tool_use_id: invocation.call_id.clone(), tool_input: result.tool_input.clone(), tool_response, @@ -78,7 +78,7 @@ impl ToolHandler for McpHandler { let (server, tool, raw_arguments) = payload; let arguments_str = raw_arguments; - let hook_tool_name = HookToolName::for_function_tool(&model_tool_name); + let hook_tool_name = HookToolName::for_mcp_tool(&model_tool_name); let started = Instant::now(); let result = handle_mcp_tool_call( @@ -148,7 +148,7 @@ mod tests { payload, }), Some(PreToolUsePayload { - tool_name: HookToolName::for_function_tool(&codex_tools::ToolName::namespaced( + tool_name: HookToolName::for_mcp_tool(&codex_tools::ToolName::namespaced( "mcp__memory__", "create_entities", )), @@ -202,7 +202,7 @@ mod tests { assert_eq!( McpHandler.post_tool_use_payload(&invocation, &output), Some(PostToolUsePayload { - tool_name: HookToolName::for_function_tool(&codex_tools::ToolName::namespaced( + tool_name: HookToolName::for_mcp_tool(&codex_tools::ToolName::namespaced( "mcp__filesystem__", "read_file", )), diff --git a/codex-rs/core/src/tools/hook_names.rs b/codex-rs/core/src/tools/hook_names.rs index 2eaa09a8d8..d57224c7a1 100644 --- a/codex-rs/core/src/tools/hook_names.rs +++ b/codex-rs/core/src/tools/hook_names.rs @@ -27,21 +27,34 @@ impl HookToolName { } } - /// Builds the canonical hook-facing identity for function-style tools that - /// do not need Claude-compatibility aliases. + /// Builds the canonical hook-facing identity for dynamic tools. /// - /// MCP tool names already use a stable fully qualified `mcp__...__tool` - /// form, so we preserve them verbatim. Other namespaced tools use the - /// `dynamic__namespace__tool` form so hooks can wildcard an entire dynamic - /// namespace without colliding with plain tool names. - pub(crate) fn for_function_tool(tool_name: &ToolName) -> Self { + /// Plain dynamic tools keep their original name. Namespaced dynamic tools + /// use `dynamic__namespace__tool` so hooks can target the dynamic-tool + /// surface without colliding with plain Codex tools or MCP names. + /// + /// Each segment is escaped independently to keep the flattened form + /// unambiguous even when namespaces or tool names contain separator-like + /// substrings or punctuation. + pub(crate) fn for_dynamic_tool(tool_name: &ToolName) -> Self { match tool_name.namespace.as_deref() { - Some(namespace) if namespace.starts_with("mcp__") => Self::new(tool_name.display()), - Some(namespace) => Self::new(format!("dynamic__{namespace}__{}", tool_name.name)), + Some(namespace) => Self::new(format!( + "dynamic__{}__{}", + encode_dynamic_segment(namespace), + encode_dynamic_segment(&tool_name.name), + )), None => Self::new(tool_name.name.clone()), } } + /// Builds the canonical hook-facing identity for MCP tools. + /// + /// MCP tool names already use the stable fully qualified + /// `mcp__server__tool` form, so we preserve them verbatim. + pub(crate) fn for_mcp_tool(tool_name: &ToolName) -> Self { + Self::new(tool_name.display()) + } + /// Returns the hook identity for file edits performed through `apply_patch`. /// /// The serialized name remains `apply_patch` so logs and policies can key @@ -71,6 +84,38 @@ impl HookToolName { } } +fn encode_dynamic_segment(segment: &str) -> String { + let bytes = segment.as_bytes(); + let mut encoded = String::with_capacity(segment.len()); + + for (index, byte) in bytes.iter().copied().enumerate() { + if should_preserve_dynamic_byte(bytes, index, byte) { + encoded.push(char::from(byte)); + } else { + encoded.push('Z'); + encoded.push(char::from(HEX_DIGITS[usize::from(byte >> 4)])); + encoded.push(char::from(HEX_DIGITS[usize::from(byte & 0x0F)])); + } + } + + encoded +} + +fn should_preserve_dynamic_byte(bytes: &[u8], index: usize, byte: u8) -> bool { + match byte { + b'a'..=b'z' | b'A'..=b'Y' | b'0'..=b'9' => true, + b'_' => { + index > 0 + && index + 1 < bytes.len() + && bytes[index - 1].is_ascii_alphanumeric() + && bytes[index + 1].is_ascii_alphanumeric() + } + _ => false, + } +} + +const HEX_DIGITS: &[u8; 16] = b"0123456789ABCDEF"; + #[cfg(test)] mod tests { use super::HookToolName; @@ -78,32 +123,56 @@ mod tests { use pretty_assertions::assert_eq; #[test] - fn for_function_tool_keeps_plain_tool_names() { + fn for_dynamic_tool_keeps_plain_tool_names() { assert_eq!( - HookToolName::for_function_tool(&ToolName::plain("tool_search")), + HookToolName::for_dynamic_tool(&ToolName::plain("tool_search")), HookToolName::new("tool_search"), ); } #[test] - fn for_function_tool_keeps_mcp_names_stable() { + fn for_mcp_tool_keeps_mcp_names_stable() { assert_eq!( - HookToolName::for_function_tool(&ToolName::namespaced( - "mcp__memory__", - "create_entities", - )), + HookToolName::for_mcp_tool(&ToolName::namespaced("mcp__memory__", "create_entities",)), HookToolName::new("mcp__memory__create_entities"), ); } #[test] - fn for_function_tool_prefixes_dynamic_namespaces() { + fn for_dynamic_tool_prefixes_dynamic_namespaces() { assert_eq!( - HookToolName::for_function_tool(&ToolName::namespaced( - "codex_app", - "automation_update", - )), + HookToolName::for_dynamic_tool( + &ToolName::namespaced("codex_app", "automation_update",) + ), HookToolName::new("dynamic__codex_app__automation_update"), ); } + + #[test] + fn for_dynamic_tool_does_not_spoof_mcp_namespaces() { + assert_eq!( + HookToolName::for_dynamic_tool( + &ToolName::namespaced("mcp__filesystem__", "read_file",) + ), + HookToolName::new("dynamic__mcpZ5FZ5FfilesystemZ5FZ5F__read_file"), + ); + } + + #[test] + fn for_dynamic_tool_escapes_ambiguous_delimiters() { + let first = HookToolName::for_dynamic_tool(&ToolName::namespaced("foo__bar", "baz")); + let second = HookToolName::for_dynamic_tool(&ToolName::namespaced("foo", "bar__baz")); + + assert_eq!(first, HookToolName::new("dynamic__fooZ5FZ5Fbar__baz")); + assert_eq!(second, HookToolName::new("dynamic__foo__barZ5FZ5Fbaz")); + assert_ne!(first, second); + } + + #[test] + fn for_dynamic_tool_escapes_punctuation_and_edge_underscores() { + assert_eq!( + HookToolName::for_dynamic_tool(&ToolName::namespaced("_google-drive", "update.file_",)), + HookToolName::new("dynamic__Z5FgoogleZ2Ddrive__updateZ2EfileZ5F"), + ); + } } diff --git a/codex-rs/hooks/src/events/common.rs b/codex-rs/hooks/src/events/common.rs index b41775e5f1..c334d1ae1d 100644 --- a/codex-rs/hooks/src/events/common.rs +++ b/codex-rs/hooks/src/events/common.rs @@ -109,7 +109,7 @@ pub(crate) fn matcher_pattern_for_event( } pub(crate) fn validate_matcher_pattern(matcher: &str) -> Result<(), regex::Error> { - if is_match_all_matcher(matcher) || is_exact_matcher(matcher) { + if is_match_all_matcher(matcher) || is_literal_tool_name_matcher(matcher) { return Ok(()); } regex::Regex::new(matcher).map(|_| ()) @@ -119,7 +119,7 @@ pub(crate) fn matches_matcher(matcher: Option<&str>, input: Option<&str>) -> boo match matcher { None => true, Some(matcher) if is_match_all_matcher(matcher) => true, - Some(matcher) if is_exact_matcher(matcher) => input + Some(matcher) if is_literal_tool_name_matcher(matcher) => input .map(|input| matcher.split('|').any(|candidate| candidate == input)) .unwrap_or(false), Some(matcher) => input @@ -147,10 +147,19 @@ fn is_match_all_matcher(matcher: &str) -> bool { matcher.is_empty() || matcher == "*" } -fn is_exact_matcher(matcher: &str) -> bool { +fn is_literal_tool_name_matcher(matcher: &str) -> bool { matcher - .chars() - .all(|ch| ch.is_ascii_alphanumeric() || ch == '_' || ch == '|') + .split('|') + .all(|candidate| !candidate.is_empty() && !contains_regex_syntax(candidate)) +} + +fn contains_regex_syntax(candidate: &str) -> bool { + candidate.chars().any(|ch| { + matches!( + ch, + '\\' | '^' | '$' | '*' | '(' | ')' | '[' | ']' | '{' | '}' + ) + }) } #[cfg(test)] @@ -194,6 +203,18 @@ mod tests { fn literal_matcher_uses_exact_matching() { assert!(matches_matcher(Some("Bash"), Some("Bash"))); assert!(!matches_matcher(Some("Bash"), Some("BashOutput"))); + assert!(matches_matcher( + Some("tool-search.v2"), + Some("tool-search.v2") + )); + assert!(!matches_matcher( + Some("tool-search.v2"), + Some("tool-searchXv2") + )); + assert!(matches_matcher( + Some("lookup+ticket"), + Some("lookup+ticket") + )); assert!(matches_matcher( Some("dynamic__codex_app__automation_update"), Some("dynamic__codex_app__automation_update") @@ -210,6 +231,8 @@ mod tests { Some("mcp__memory"), Some("mcp__memory__create_entities") )); + assert_eq!(validate_matcher_pattern("tool-search.v2"), Ok(())); + assert_eq!(validate_matcher_pattern("lookup+ticket"), Ok(())); assert_eq!(validate_matcher_pattern("mcp__memory"), Ok(())); }