github/codex
4
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-20 00:18:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

Compile hook matchers during discovery (#49379)

Browse Source 
## Why

Hook dispatch recompiles regex matchers for each input even though discovery already validates them.

## What changed

Store compiled `HookMatcher` values on configured handlers and reuse them during dispatch. Preserve match-all, exact-name, pipe-alternative, and regex matching semantics, along with the original pattern spelling for configuration equality. Invalid regexes continue to be rejected during discovery.

GitOrigin-RevId: 117a8a19c7bc0e55ccbedcd5940b806a17ebba10
This commit is contained in:
jif
2026-09-29 22:04:56 +00:00
committed by copyberry
parent 2a34aef794
commit bd4204efc2
8 changed files with 158 additions and 87 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/hooks/src/engine/discovery.rs
+29 -20
View File
@@ -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<_>>(),
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::<Vec<_>>(),
@@ -1558,7 +1561,10 @@ mod tests {
assert_eq!(warnings, Vec::<String>::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::<String>::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]
codex-rs/hooks/src/engine/dispatcher.rs
+5 -4
View File
@@ -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,
),
codex-rs/hooks/src/engine/matcher.rs
+49
View File
@@ -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<Self, regex::Error> {
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 {}
codex-rs/hooks/src/engine/mod.rs
+4 -1
View File
@@ -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<String>,
pub matcher: Option<HookMatcher>,
pub timeout_sec: u64,
pub status_message: Option<String>,
pub additional_context_limit: AdditionalContextLimit,
codex-rs/hooks/src/events/common.rs
+67 -59
View File
@@ -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]
codex-rs/hooks/src/events/post_tool_use.rs
+1 -1
View File
@@ -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(),
codex-rs/hooks/src/events/pre_tool_use.rs
+1 -1
View File
@@ -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(),
codex-rs/hooks/src/events/session_end_tests.rs
+2 -1
View File
@@ -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(),