Detailed changes
@@ -9,7 +9,6 @@ import (
"maps"
"net/http"
"net/url"
- "regexp"
"slices"
"strings"
"time"
@@ -371,7 +370,9 @@ func (t ToolGrep) GetTimeout() time.Duration {
}
// HookConfig defines a user-configured shell command that fires on a hook
-// event (e.g. PreToolUse).
+// event (e.g. PreToolUse). This is a pure-data struct: matcher compilation
+// is owned by hooks.Runner so a JSON round-trip, merge, or reload can't
+// silently drop compiled state.
type HookConfig struct {
// Regex pattern tested against the tool name. Empty means match all.
Matcher string `json:"matcher,omitempty" jsonschema:"description=Regex pattern tested against the tool name. Empty means match all tools."`
@@ -379,15 +380,6 @@ type HookConfig struct {
Command string `json:"command" jsonschema:"required,description=Shell command to execute when the hook fires"`
// Timeout in seconds. Default 30.
Timeout int `json:"timeout,omitempty" jsonschema:"description=Timeout in seconds for the hook command,default=30"`
-
- // Compiled matcher regex. Not serialized.
- matcherRegex *regexp.Regexp
-}
-
-// MatcherRegex returns the compiled matcher regex, or nil if no matcher is
-// set.
-func (h *HookConfig) MatcherRegex() *regexp.Regexp {
- return h.matcherRegex
}
// TimeoutDuration returns the hook timeout as a time.Duration, defaulting
@@ -905,8 +905,11 @@ func normalizeHookEvent(name string) string {
}
}
-// ValidateHooks normalizes event names and compiles matcher regexes for all
-// configured hooks. Returns an error if any regex is invalid.
+// ValidateHooks normalizes event names and checks that every configured
+// hook has a command and a syntactically valid matcher regex. Matcher
+// compilation used for matching is owned by hooks.Runner; this function
+// only validates up front so the user sees config errors at load time
+// rather than on the first tool call.
func (c *Config) ValidateHooks() error {
// Normalize event name keys.
for event, eventHooks := range c.Hooks {
@@ -918,17 +921,15 @@ func (c *Config) ValidateHooks() error {
}
for event, eventHooks := range c.Hooks {
- for i := range eventHooks {
- h := &c.Hooks[event][i]
+ for i, h := range eventHooks {
if h.Command == "" {
return fmt.Errorf("hook %s[%d]: command is required", event, i)
}
- if h.Matcher != "" {
- re, err := regexp.Compile(h.Matcher)
- if err != nil {
- return fmt.Errorf("hook %s[%d]: invalid matcher regex %q: %w", event, i, h.Matcher, err)
- }
- h.matcherRegex = re
+ if h.Matcher == "" {
+ continue
+ }
+ if _, err := regexp.Compile(h.Matcher); err != nil {
+ return fmt.Errorf("hook %s[%d]: invalid matcher regex %q: %w", event, i, h.Matcher, err)
}
}
}
@@ -373,6 +373,40 @@ func TestRunnerMatcherFiltering(t *testing.T) {
require.NoError(t, err)
require.Equal(t, DecisionNone, result.Decision)
})
+
+ // Runner must compile matchers itself; it cannot rely on
+ // ValidateHooks having run first. This is the guarantee that prevents
+ // the reload-drops-matcher class of bug.
+ t.Run("runner compiles matcher without ValidateHooks", func(t *testing.T) {
+ t.Parallel()
+ raw := []config.HookConfig{
+ {Command: `echo '{"decision":"deny","reason":"blocked"}'`, Matcher: "^bash$"},
+ }
+ r := NewRunner(raw, t.TempDir(), t.TempDir())
+
+ deny, err := r.Run(context.Background(), EventPreToolUse, "sess", "bash", `{}`)
+ require.NoError(t, err)
+ require.Equal(t, DecisionDeny, deny.Decision)
+
+ noop, err := r.Run(context.Background(), EventPreToolUse, "sess", "view", `{}`)
+ require.NoError(t, err)
+ require.Equal(t, DecisionNone, noop.Decision)
+ })
+
+ // A matcher that fails to compile at Runner construction must not
+ // degrade to match-everything; the hook is dropped instead.
+ t.Run("runner skips hooks with invalid matcher", func(t *testing.T) {
+ t.Parallel()
+ raw := []config.HookConfig{
+ {Command: `echo '{"decision":"deny","reason":"should not fire"}'`, Matcher: "[invalid"},
+ }
+ r := NewRunner(raw, t.TempDir(), t.TempDir())
+
+ result, err := r.Run(context.Background(), EventPreToolUse, "sess", "bash", `{}`)
+ require.NoError(t, err)
+ require.Equal(t, DecisionNone, result.Decision)
+ require.Empty(t, r.Hooks())
+ })
}
func TestValidateHooksInvalidRegex(t *testing.T) {
@@ -5,6 +5,7 @@ import (
"context"
"log/slog"
"os/exec"
+ "regexp"
"strings"
"sync"
"time"
@@ -12,25 +13,63 @@ import (
"github.com/charmbracelet/crush/internal/config"
)
+// compiledHook pairs a HookConfig with its compiled matcher regex. A nil
+// matcher means "match every tool".
+type compiledHook struct {
+ cfg config.HookConfig
+ matcher *regexp.Regexp
+}
+
// Runner executes hook commands and aggregates their results.
type Runner struct {
- hooks []config.HookConfig
+ hooks []compiledHook
cwd string
projectDir string
}
-// NewRunner creates a Runner from the given hook configs.
+// NewRunner creates a Runner from the given hook configs. Each hook's
+// Matcher is compiled here so the Runner is self-sufficient; callers do
+// not have to pre-compile matchers on the config, and reloads or merges
+// that rebuild HookConfig values can't silently strip compiled state.
+//
+// Hooks whose matcher fails to compile are skipped with a warning rather
+// than treated as match-everything. ValidateHooks is expected to have
+// caught syntax errors earlier, so this is defense in depth.
func NewRunner(hooks []config.HookConfig, cwd, projectDir string) *Runner {
+ compiled := make([]compiledHook, 0, len(hooks))
+ for _, h := range hooks {
+ ch := compiledHook{cfg: h}
+ if h.Matcher != "" {
+ re, err := regexp.Compile(h.Matcher)
+ if err != nil {
+ slog.Warn("Hook matcher failed to compile; skipping hook",
+ "matcher", h.Matcher,
+ "command", h.Command,
+ "error", err,
+ )
+ continue
+ }
+ ch.matcher = re
+ }
+ compiled = append(compiled, ch)
+ }
return &Runner{
- hooks: hooks,
+ hooks: compiled,
cwd: cwd,
projectDir: projectDir,
}
}
-// Hooks returns the hook configs the runner was created with.
+// Hooks returns the hook configs the runner was created with, in config
+// order. Hooks whose matcher failed to compile at construction are
+// omitted. Intended for diagnostics; callers should not rely on ordering
+// or identity beyond that.
func (r *Runner) Hooks() []config.HookConfig {
- return r.hooks
+ out := make([]config.HookConfig, len(r.hooks))
+ for i, h := range r.hooks {
+ out[i] = h.cfg
+ }
+ return out
}
// Run executes all matching hooks for the given event and tool, returning
@@ -93,9 +132,8 @@ func (r *Runner) Run(ctx context.Context, eventName, sessionID, toolName, toolIn
func (r *Runner) matchingHooks(toolName string) []config.HookConfig {
var matched []config.HookConfig
for _, h := range r.hooks {
- re := h.MatcherRegex()
- if re == nil || re.MatchString(toolName) {
- matched = append(matched, h)
+ if h.matcher == nil || h.matcher.MatchString(toolName) {
+ matched = append(matched, h.cfg)
}
}
return matched