diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index 5003dbd24f..0c5a28ef35 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -29,8 +29,8 @@ use super::HookListEntry; use super::HookListEntryHandler; use super::dispatcher::hook_event_name_label; use crate::config_rules::hook_states_from_stack; +use crate::engine::HookMatcher; use crate::events::common::matcher_pattern_for_event; -use crate::events::common::validate_matcher_pattern; use crate::events::session_end::SESSION_END_DEFAULT_TIMEOUT_SEC; use crate::events::session_end::SESSION_END_MAX_TIMEOUT_SEC; use crate::output_spill::AdditionalContextLimit; @@ -486,20 +486,22 @@ fn append_matcher_groups( ) { for (group_index, group) in groups.into_iter().enumerate() { let matcher = matcher_pattern_for_event(event_name, group.matcher.as_deref()); - if let Some(matcher) = matcher - && let Err(err) = validate_matcher_pattern(matcher) - { - let warning = format!( - "invalid matcher {matcher:?} in {}: {err}", - source.path.display() - ); - if group.hooks.is_empty() { - warnings.push(warning); - } else { - source.record_load_failure(warning, warnings); + let compiled_matcher = match matcher.map(HookMatcher::new).transpose() { + Ok(matcher) => matcher, + Err(err) => { + let matcher = matcher.unwrap_or_default(); + let warning = format!( + "invalid matcher {matcher:?} in {}: {err}", + source.path.display() + ); + if group.hooks.is_empty() { + warnings.push(warning); + } else { + source.record_load_failure(warning, warnings); + } + continue; } - continue; - } + }; for (handler_index, handler) in group.hooks.iter().cloned().enumerate() { let normalized = match handler { HookHandlerConfig::Command { @@ -720,7 +722,7 @@ fn append_matcher_groups( handlers.push(ConfiguredHandler { builtin, event_name, - matcher: matcher.map(ToOwned::to_owned), + matcher: compiled_matcher.clone(), timeout_sec, status_message, additional_context_limit: AdditionalContextLimit::from_config( @@ -862,6 +864,7 @@ fn hook_source_for_requirement_source(source: Option<&RequirementSource>) -> Hoo #[cfg(test)] mod tests { + use super::HookMatcher; use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerSource; use codex_config::HookEventsToml; @@ -1296,7 +1299,7 @@ mod tests { vec![ConfiguredHandler { builtin: false, event_name: HookEventName::PreToolUse, - matcher: Some("^Bash$".to_string()), + matcher: Some(HookMatcher::new("^Bash$").expect("valid matcher")), timeout_sec: 600, status_message: None, additional_context_limit: Default::default(), @@ -1366,7 +1369,7 @@ mod tests { assert_eq!( handlers .iter() - .map(|handler| handler.matcher.as_deref()) + .map(|handler| handler.matcher.as_ref().map(HookMatcher::as_str)) .collect::>(), vec![Some("other"), Some("other")] ); @@ -1446,7 +1449,7 @@ mod tests { .iter() .map(|handler| ( handler.timeout_sec, - handler.matcher.as_deref(), + handler.matcher.as_ref().map(HookMatcher::as_str), handler.execution_mode() )) .collect::>(), @@ -1558,7 +1561,10 @@ mod tests { assert_eq!(warnings, Vec::::new()); assert_eq!(handlers.len(), 1); - assert_eq!(handlers[0].matcher.as_deref(), Some("*")); + assert_eq!( + handlers[0].matcher.as_ref().map(HookMatcher::as_str), + Some("*") + ); } #[test] @@ -1582,7 +1588,10 @@ mod tests { assert_eq!(warnings, Vec::::new()); assert_eq!(handlers.len(), 1); assert_eq!(handlers[0].event_name, HookEventName::PostToolUse); - assert_eq!(handlers[0].matcher.as_deref(), Some("Edit|Write")); + assert_eq!( + handlers[0].matcher.as_ref().map(HookMatcher::as_str), + Some("Edit|Write") + ); } #[test] diff --git a/codex-rs/hooks/src/engine/dispatcher.rs b/codex-rs/hooks/src/engine/dispatcher.rs index c5ee0d2ce1..b34118d6f2 100644 --- a/codex-rs/hooks/src/engine/dispatcher.rs +++ b/codex-rs/hooks/src/engine/dispatcher.rs @@ -60,11 +60,11 @@ pub(crate) fn select_handlers_for_matcher_inputs( | HookEventName::PreCompact | HookEventName::PostCompact => { if matcher_inputs.is_empty() { - matches_matcher(handler.matcher.as_deref(), /*input*/ None) + matches_matcher(handler.matcher.as_ref(), /*input*/ None) } else { matcher_inputs .iter() - .any(|input| matches_matcher(handler.matcher.as_deref(), Some(input))) + .any(|input| matches_matcher(handler.matcher.as_ref(), Some(input))) } } HookEventName::UserPromptSubmit | HookEventName::Stop | HookEventName::Interrupt => { @@ -352,7 +352,8 @@ mod tests { ConfiguredHandler { builtin: false, event_name, - matcher: matcher.map(str::to_owned), + matcher: matcher + .map(|pattern| crate::engine::HookMatcher::new(pattern).expect("valid matcher")), timeout_sec: 5, status_message: None, additional_context_limit: Default::default(), @@ -609,7 +610,7 @@ mod tests { ), make_handler( HookEventName::UserPromptSubmit, - Some("["), + Some("^unmatched$"), "echo second", /*display_order*/ 1, ), diff --git a/codex-rs/hooks/src/engine/matcher.rs b/codex-rs/hooks/src/engine/matcher.rs new file mode 100644 index 0000000000..3cd95aacd1 --- /dev/null +++ b/codex-rs/hooks/src/engine/matcher.rs @@ -0,0 +1,49 @@ +//! Compiled hook matchers retain their original spelling for configuration identity. +//! Regex parsing happens during discovery; dispatch only evaluates admitted matchers. + +#[derive(Debug, Clone)] +pub(crate) enum HookMatcher { + All(String), + Exact(String), + Regex(regex::Regex), +} + +impl HookMatcher { + pub(crate) fn new(pattern: &str) -> Result { + if pattern.is_empty() || pattern == "*" { + Ok(Self::All(pattern.to_string())) + } else if pattern + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || ch == '_' || ch == '|') + { + Ok(Self::Exact(pattern.to_string())) + } else { + regex::Regex::new(pattern).map(Self::Regex) + } + } + + pub(crate) fn as_str(&self) -> &str { + match self { + Self::All(pattern) | Self::Exact(pattern) => pattern, + Self::Regex(regex) => regex.as_str(), + } + } + + pub(crate) fn matches(&self, input: Option<&str>) -> bool { + match self { + Self::All(_) => true, + Self::Exact(pattern) => { + input.is_some_and(|input| pattern.split('|').any(|candidate| candidate == input)) + } + Self::Regex(regex) => input.is_some_and(|input| regex.is_match(input)), + } + } +} + +impl PartialEq for HookMatcher { + fn eq(&self, other: &Self) -> bool { + self.as_str() == other.as_str() + } +} + +impl Eq for HookMatcher {} diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index deaf29679a..a007f3b1a4 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -1,10 +1,13 @@ pub(crate) mod command_runner; pub(crate) mod discovery; pub(crate) mod dispatcher; +mod matcher; pub(crate) mod mcp_runner; pub(crate) mod output_parser; pub(crate) mod schema_loader; +pub(crate) use matcher::HookMatcher; + use crate::events::compact::PostCompactRequest; use crate::events::compact::PreCompactOutcome; use crate::events::compact::PreCompactRequest; @@ -60,7 +63,7 @@ pub(crate) struct ConfiguredHandler { /// Internally admitted cleanup hook, enabled independently of per-hook state. pub builtin: bool, pub event_name: codex_protocol::protocol::HookEventName, - pub matcher: Option, + pub matcher: Option, pub timeout_sec: u64, pub status_message: Option, pub additional_context_limit: AdditionalContextLimit, diff --git a/codex-rs/hooks/src/events/common.rs b/codex-rs/hooks/src/events/common.rs index 7b4c4dcab3..c6e1a1d324 100644 --- a/codex-rs/hooks/src/events/common.rs +++ b/codex-rs/hooks/src/events/common.rs @@ -127,28 +127,11 @@ 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) { - return Ok(()); - } - regex::Regex::new(matcher).map(|_| ()) -} - -pub(crate) fn matches_matcher(matcher: Option<&str>, input: Option<&str>) -> bool { - match matcher { - None => true, - Some(matcher) if is_match_all_matcher(matcher) => true, - Some(matcher) if is_exact_matcher(matcher) => input - .map(|input| matcher.split('|').any(|candidate| candidate == input)) - .unwrap_or(false), - Some(matcher) => input - .and_then(|input| { - regex::Regex::new(matcher) - .ok() - .map(|regex| regex.is_match(input)) - }) - .unwrap_or(false), - } +pub(crate) fn matches_matcher( + matcher: Option<&crate::engine::HookMatcher>, + input: Option<&str>, +) -> bool { + matcher.is_none_or(|matcher| matcher.matches(input)) } pub(crate) fn matcher_inputs<'a>( @@ -162,16 +145,6 @@ pub(crate) fn matcher_inputs<'a>( .collect() } -fn is_match_all_matcher(matcher: &str) -> bool { - matcher.is_empty() || matcher == "*" -} - -fn is_exact_matcher(matcher: &str) -> bool { - matcher - .chars() - .all(|ch| ch.is_ascii_alphanumeric() || ch == '_' || ch == '|') -} - #[cfg(test)] mod tests { use codex_protocol::protocol::HookEventName; @@ -179,7 +152,7 @@ mod tests { use super::matcher_pattern_for_event; use super::matches_matcher; - use super::validate_matcher_pattern; + use crate::engine::HookMatcher; #[test] fn matcher_omitted_matches_all_occurrences() { @@ -189,75 +162,110 @@ mod tests { #[test] fn matcher_star_matches_all_occurrences() { - assert!(matches_matcher(Some("*"), Some("Bash"))); - assert!(matches_matcher(Some("*"), Some("Edit"))); - assert_eq!(validate_matcher_pattern("*"), Ok(())); + assert!(matches_matcher( + Some(&HookMatcher::new("*").expect("valid matcher")), + Some("Bash") + )); + assert!(matches_matcher( + Some(&HookMatcher::new("*").expect("valid matcher")), + Some("Edit") + )); + assert_eq!(HookMatcher::new("*").map(|_| ()), Ok(())); } #[test] fn matcher_empty_string_matches_all_occurrences() { - assert!(matches_matcher(Some(""), Some("Bash"))); - assert!(matches_matcher(Some(""), Some("SessionStart"))); - assert_eq!(validate_matcher_pattern(""), Ok(())); + assert!(matches_matcher( + Some(&HookMatcher::new("").expect("valid matcher")), + Some("Bash") + )); + assert!(matches_matcher( + Some(&HookMatcher::new("").expect("valid matcher")), + Some("SessionStart") + )); + assert_eq!(HookMatcher::new("").map(|_| ()), Ok(())); } #[test] fn exact_matcher_supports_pipe_alternatives() { - assert!(matches_matcher(Some("Edit|Write"), Some("Edit"))); - assert!(matches_matcher(Some("Edit|Write"), Some("Write"))); - assert!(!matches_matcher(Some("Edit|Write"), Some("Bash"))); - assert_eq!(validate_matcher_pattern("Edit|Write"), Ok(())); + assert!(matches_matcher( + Some(&HookMatcher::new("Edit|Write").expect("valid matcher")), + Some("Edit") + )); + assert!(matches_matcher( + Some(&HookMatcher::new("Edit|Write").expect("valid matcher")), + Some("Write") + )); + assert!(!matches_matcher( + Some(&HookMatcher::new("Edit|Write").expect("valid matcher")), + Some("Bash") + )); + assert_eq!(HookMatcher::new("Edit|Write").map(|_| ()), Ok(())); } #[test] fn literal_matcher_uses_exact_matching() { - assert!(matches_matcher(Some("Bash"), Some("Bash"))); - assert!(!matches_matcher(Some("Bash"), Some("BashOutput"))); assert!(matches_matcher( - Some("mcp__memory__create_entities"), + Some(&HookMatcher::new("Bash").expect("valid matcher")), + Some("Bash") + )); + assert!(!matches_matcher( + Some(&HookMatcher::new("Bash").expect("valid matcher")), + Some("BashOutput") + )); + assert!(matches_matcher( + Some(&HookMatcher::new("mcp__memory__create_entities").expect("valid matcher")), Some("mcp__memory__create_entities") )); assert!(!matches_matcher( - Some("mcp__memory"), + Some(&HookMatcher::new("mcp__memory").expect("valid matcher")), Some("mcp__memory__create_entities") )); - assert_eq!(validate_matcher_pattern("mcp__memory"), Ok(())); + assert_eq!(HookMatcher::new("mcp__memory").map(|_| ()), Ok(())); } #[test] fn matcher_uses_regex_when_it_contains_regex_characters() { - assert!(matches_matcher(Some("^Bash"), Some("BashOutput"))); - assert_eq!(validate_matcher_pattern("^Bash"), Ok(())); + assert!(matches_matcher( + Some(&HookMatcher::new("^Bash").expect("valid matcher")), + Some("BashOutput") + )); + assert_eq!(HookMatcher::new("^Bash").map(|_| ()), Ok(())); } #[test] fn mcp_matchers_support_regex_wildcards() { assert!(matches_matcher( - Some("mcp__memory__.*"), + Some(&HookMatcher::new("mcp__memory__.*").expect("valid matcher")), Some("mcp__memory__create_entities") )); assert!(matches_matcher( - Some("mcp__.*__write.*"), + Some(&HookMatcher::new("mcp__.*__write.*").expect("valid matcher")), Some("mcp__filesystem__write_file") )); assert!(!matches_matcher( - Some("mcp__.*__write.*"), + Some(&HookMatcher::new("mcp__.*__write.*").expect("valid matcher")), Some("mcp__filesystem__read_file") )); - assert_eq!(validate_matcher_pattern("mcp__memory__.*"), Ok(())); + assert_eq!(HookMatcher::new("mcp__memory__.*").map(|_| ()), Ok(())); } #[test] fn matcher_supports_anchored_regexes() { - assert!(matches_matcher(Some("^Bash$"), Some("Bash"))); - assert!(!matches_matcher(Some("^Bash$"), Some("BashOutput"))); - assert_eq!(validate_matcher_pattern("^Bash$"), Ok(())); + assert!(matches_matcher( + Some(&HookMatcher::new("^Bash$").expect("valid matcher")), + Some("Bash") + )); + assert!(!matches_matcher( + Some(&HookMatcher::new("^Bash$").expect("valid matcher")), + Some("BashOutput") + )); + assert_eq!(HookMatcher::new("^Bash$").map(|_| ()), Ok(())); } #[test] fn invalid_regex_is_rejected() { - assert!(validate_matcher_pattern("[").is_err()); - assert!(!matches_matcher(Some("["), Some("Bash"))); + assert!(HookMatcher::new("[").is_err()); } #[test] diff --git a/codex-rs/hooks/src/events/post_tool_use.rs b/codex-rs/hooks/src/events/post_tool_use.rs index aeb0ed20a7..0ca0f6c673 100644 --- a/codex-rs/hooks/src/events/post_tool_use.rs +++ b/codex-rs/hooks/src/events/post_tool_use.rs @@ -591,7 +591,7 @@ mod tests { ConfiguredHandler { builtin: false, event_name: HookEventName::PostToolUse, - matcher: Some("^Bash$".to_string()), + matcher: Some(crate::engine::HookMatcher::new("^Bash$").expect("valid matcher")), timeout_sec: 5, status_message: Some("running post tool use hook".to_string()), additional_context_limit: Default::default(), diff --git a/codex-rs/hooks/src/events/pre_tool_use.rs b/codex-rs/hooks/src/events/pre_tool_use.rs index c1baebd4f0..e57b823a1c 100644 --- a/codex-rs/hooks/src/events/pre_tool_use.rs +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -775,7 +775,7 @@ mod tests { ConfiguredHandler { builtin: false, event_name: HookEventName::PreToolUse, - matcher: Some("^Bash$".to_string()), + matcher: Some(crate::engine::HookMatcher::new("^Bash$").expect("valid matcher")), timeout_sec: 5, status_message: None, additional_context_limit: Default::default(), diff --git a/codex-rs/hooks/src/events/session_end_tests.rs b/codex-rs/hooks/src/events/session_end_tests.rs index 0c100ee4a4..05fa107d4f 100644 --- a/codex-rs/hooks/src/events/session_end_tests.rs +++ b/codex-rs/hooks/src/events/session_end_tests.rs @@ -62,7 +62,8 @@ fn handler(matcher: Option<&str>) -> ConfiguredHandler { ConfiguredHandler { builtin: false, event_name: HookEventName::SessionEnd, - matcher: matcher.map(str::to_string), + matcher: matcher + .map(|pattern| crate::engine::HookMatcher::new(pattern).expect("valid matcher")), timeout_sec: 2, status_message: None, additional_context_limit: Default::default(),