From 4242d2e71fec666cd25082d5c3af10868ee94adc Mon Sep 17 00:00:00 2001
From: Scott Werner <stwerner@vt.edu>
Date: Tue, 7 Jul 2026 16:53:16 -0400
Subject: [PATCH] hooks: warn when a matcher pattern fails to compile
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A hook matcher regex that failed to compile was dropped silently in compile_matchers, while matches() carried a comment claiming the failure had "already warned" — which never happened. A typo'd matcher made a hook silently never fire, with no signal.

Emit a warning at construction (naming the pattern and error), flatten the nested conditionals, and correct the stale comment. Add a test asserting an invalid pattern never matches.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 lib/crates/fabro-hooks/src/runner.rs | 44 ++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/lib/crates/fabro-hooks/src/runner.rs b/lib/crates/fabro-hooks/src/runner.rs
index 006570730..791191c96 100644
--- a/lib/crates/fabro-hooks/src/runner.rs
+++ b/lib/crates/fabro-hooks/src/runner.rs
@@ -80,11 +80,22 @@ impl HookRunner {
     fn compile_matchers(config: &HookSettings) -> HashMap<String, regex::Regex> {
         let mut map = HashMap::new();
         for hook in &config.hooks {
-            if let Some(ref pattern) = hook.matcher {
-                if !map.contains_key(pattern) {
-                    if let Ok(re) = regex::Regex::new(pattern) {
-                        map.insert(pattern.clone(), re);
-                    }
+            let Some(pattern) = hook.matcher.as_ref() else {
+                continue;
+            };
+            if map.contains_key(pattern) {
+                continue;
+            }
+            match regex::Regex::new(pattern) {
+                Ok(re) => {
+                    map.insert(pattern.clone(), re);
+                }
+                Err(error) => {
+                    tracing::warn!(
+                        pattern = %pattern,
+                        error = %error,
+                        "hook matcher pattern failed to compile; hooks using it will never match"
+                    );
                 }
             }
         }
@@ -148,7 +159,8 @@ impl HookRunner {
             return true;
         };
         let Some(re) = self.compiled_matchers.get(pattern) else {
-            // Pattern failed to compile during construction — already warned
+            // Pattern failed to compile at construction (warned there); treat as
+            // no match so a broken matcher never silently matches everything.
             return false;
         };
         [
@@ -431,6 +443,26 @@ mod tests {
         assert!(runner.filter_hooks(&ctx).is_empty());
     }
 
+    #[tokio::test]
+    async fn invalid_matcher_pattern_never_matches() {
+        // A matcher that fails to compile is dropped at construction (with a
+        // warning); the hook must then never match rather than silently match
+        // everything.
+        let mut hook = make_hook(HookEvent::StageStart, "bad-matcher");
+        hook.matcher = Some("[unterminated(".into());
+        let config = HookSettings { hooks: vec![hook] };
+        let runner = HookRunner::with_executor(
+            config,
+            Arc::new(MockExecutor {
+                decision: HookDecision::Proceed,
+            }),
+        );
+
+        let mut ctx = make_context(HookEvent::StageStart);
+        ctx.node_id = Some("agent".into());
+        assert!(runner.filter_hooks(&ctx).is_empty());
+    }
+
     #[tokio::test]
     async fn blocking_hook_block_decision() {
         let config = HookSettings {