diff --git a/codex-rs/core/src/tools/handlers/dynamic.rs b/codex-rs/core/src/tools/handlers/dynamic.rs index 87609b8cb1..1e02c62ecb 100644 --- a/codex-rs/core/src/tools/handlers/dynamic.rs +++ b/codex-rs/core/src/tools/handlers/dynamic.rs @@ -241,7 +241,7 @@ mod tests { assert_eq!( DynamicToolHandler.pre_tool_use_payload(&invocation), Some(PreToolUsePayload { - tool_name: HookToolName::new("automation_update"), + tool_name: HookToolName::new("dynamic__default__automation_update"), tool_input: json!({ "id": 1 }), }) ); diff --git a/codex-rs/core/src/tools/hook_names.rs b/codex-rs/core/src/tools/hook_names.rs index d57224c7a1..7adc38410a 100644 --- a/codex-rs/core/src/tools/hook_names.rs +++ b/codex-rs/core/src/tools/hook_names.rs @@ -29,22 +29,24 @@ impl HookToolName { /// Builds the canonical hook-facing identity for dynamic tools. /// - /// 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. + /// Dynamic tools always use `dynamic__namespace__tool` so hooks can target + /// the dynamic-tool surface without colliding with plain Codex tools or + /// MCP names. Unnamespaced tools are assigned to the synthetic `default` + /// namespace for hook identity purposes. /// /// 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) => Self::new(format!( - "dynamic__{}__{}", - encode_dynamic_segment(namespace), - encode_dynamic_segment(&tool_name.name), - )), - None => Self::new(tool_name.name.clone()), - } + let namespace = tool_name + .namespace + .as_deref() + .unwrap_or(DEFAULT_DYNAMIC_HOOK_NAMESPACE); + Self::new(format!( + "{DYNAMIC_HOOK_PREFIX}{}__{}", + encode_dynamic_segment(namespace), + encode_dynamic_segment(&tool_name.name), + )) } /// Builds the canonical hook-facing identity for MCP tools. @@ -92,7 +94,7 @@ fn encode_dynamic_segment(segment: &str) -> String { if should_preserve_dynamic_byte(bytes, index, byte) { encoded.push(char::from(byte)); } else { - encoded.push('Z'); + encoded.push('%'); encoded.push(char::from(HEX_DIGITS[usize::from(byte >> 4)])); encoded.push(char::from(HEX_DIGITS[usize::from(byte & 0x0F)])); } @@ -103,17 +105,21 @@ fn encode_dynamic_segment(segment: &str) -> String { 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'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' => true, b'_' => { index > 0 && index + 1 < bytes.len() && bytes[index - 1].is_ascii_alphanumeric() && bytes[index + 1].is_ascii_alphanumeric() + && bytes[index - 1] != b'_' + && bytes[index + 1] != b'_' } _ => false, } } +const DYNAMIC_HOOK_PREFIX: &str = "dynamic__"; +const DEFAULT_DYNAMIC_HOOK_NAMESPACE: &str = "default"; const HEX_DIGITS: &[u8; 16] = b"0123456789ABCDEF"; #[cfg(test)] @@ -123,10 +129,10 @@ mod tests { use pretty_assertions::assert_eq; #[test] - fn for_dynamic_tool_keeps_plain_tool_names() { + fn for_dynamic_tool_assigns_default_namespace_to_plain_tool_names() { assert_eq!( HookToolName::for_dynamic_tool(&ToolName::plain("tool_search")), - HookToolName::new("tool_search"), + HookToolName::new("dynamic__default__tool_search"), ); } @@ -154,7 +160,7 @@ mod tests { HookToolName::for_dynamic_tool( &ToolName::namespaced("mcp__filesystem__", "read_file",) ), - HookToolName::new("dynamic__mcpZ5FZ5FfilesystemZ5FZ5F__read_file"), + HookToolName::new("dynamic__mcp%5F%5Ffilesystem%5F%5F__read_file"), ); } @@ -163,8 +169,8 @@ mod tests { 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_eq!(first, HookToolName::new("dynamic__foo%5F%5Fbar__baz")); + assert_eq!(second, HookToolName::new("dynamic__foo__bar%5F%5Fbaz")); assert_ne!(first, second); } @@ -172,7 +178,23 @@ mod tests { 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"), + HookToolName::new("dynamic__%5Fgoogle%2Ddrive__update%2Efile%5F"), + ); + } + + #[test] + fn for_dynamic_tool_keeps_single_internal_underscores() { + assert_eq!( + HookToolName::for_dynamic_tool(&ToolName::namespaced("codex_app", "automation_update")), + HookToolName::new("dynamic__codex_app__automation_update"), + ); + } + + #[test] + fn for_dynamic_tool_percent_encodes_utf8_bytes() { + assert_eq!( + HookToolName::for_dynamic_tool(&ToolName::namespaced("検", "索")), + HookToolName::new("dynamic__%E6%A4%9C__%E7%B4%A2"), ); } } diff --git a/codex-rs/core/tests/suite/hooks_dynamic.rs b/codex-rs/core/tests/suite/hooks_dynamic.rs index 6ccbf6cef6..70c625f12f 100644 --- a/codex-rs/core/tests/suite/hooks_dynamic.rs +++ b/codex-rs/core/tests/suite/hooks_dynamic.rs @@ -36,6 +36,7 @@ use wiremock::MockServer; const DYNAMIC_TOOL_NAME: &str = "automation_update"; const DYNAMIC_NAMESPACE: &str = "codex_app"; +const PLAIN_DYNAMIC_HOOK_NAME: &str = "dynamic__default__automation_update"; const DYNAMIC_HOOK_NAME: &str = "dynamic__codex_app__automation_update"; fn dynamic_tool(namespace: Option<&str>, name: &str) -> DynamicToolSpec { @@ -332,7 +333,7 @@ async fn pre_tool_use_blocks_plain_dynamic_tool_before_execution() -> Result<()> &server, vec![dynamic_tool(/*namespace*/ None, DYNAMIC_TOOL_NAME)], move |home| { - if let Err(err) = write_pre_tool_use_hook(home, DYNAMIC_TOOL_NAME, block_reason) { + if let Err(err) = write_pre_tool_use_hook(home, PLAIN_DYNAMIC_HOOK_NAME, block_reason) { panic!("failed to write plain dynamic pre hook: {err}"); } }, @@ -356,7 +357,7 @@ async fn pre_tool_use_blocks_plain_dynamic_tool_before_execution() -> Result<()> .expect("blocked plain dynamic tool output"); assert!( output.contains(&format!( - "Tool call blocked by PreToolUse hook: {block_reason}. Tool: {DYNAMIC_TOOL_NAME}" + "Tool call blocked by PreToolUse hook: {block_reason}. Tool: {PLAIN_DYNAMIC_HOOK_NAME}" )), "blocked plain dynamic tool output should mention the reason and tool name", ); @@ -372,7 +373,7 @@ async fn pre_tool_use_blocks_plain_dynamic_tool_before_execution() -> Result<()> }), json!({ "hook_event_name": "PreToolUse", - "tool_name": DYNAMIC_TOOL_NAME, + "tool_name": PLAIN_DYNAMIC_HOOK_NAME, "tool_use_id": call_id, "tool_input": { "job": "plain" }, }), diff --git a/codex-rs/hooks/src/events/common.rs b/codex-rs/hooks/src/events/common.rs index c334d1ae1d..93c437c1aa 100644 --- a/codex-rs/hooks/src/events/common.rs +++ b/codex-rs/hooks/src/events/common.rs @@ -276,6 +276,26 @@ mod tests { assert_eq!(validate_matcher_pattern("dynamic__codex_app__.*"), Ok(())); } + #[test] + fn dynamic_matchers_support_percent_encoded_names() { + assert!(matches_matcher( + Some("dynamic__default__%E6%A4%9C%E7%B4%A2"), + Some("dynamic__default__%E6%A4%9C%E7%B4%A2") + )); + assert!(!matches_matcher( + Some("dynamic__default__%E6%A4%9C%E7%B4%A2"), + Some("dynamic__default__%E6%A4%9C%E7%B4%A2_extra") + )); + assert!(matches_matcher( + Some("dynamic__default__%E6%A4%9C.*"), + Some("dynamic__default__%E6%A4%9C%E7%B4%A2") + )); + assert_eq!( + validate_matcher_pattern("dynamic__default__%E6%A4%9C.*"), + Ok(()) + ); + } + #[test] fn matcher_supports_anchored_regexes() { assert!(matches_matcher(Some("^Bash$"), Some("Bash")));