diff --git a/internal/config/config.go b/internal/config/config.go index 1b8156c68f30b7eddf75d78c414f03662d94f5f2..33251615d44252aaeb2b5db58577759b5dfdff51 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -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 diff --git a/internal/config/load.go b/internal/config/load.go index d9f6807e290d97bfb7418b1def2ed471514f8335..967af2de8cd6bffd6a8bd08e77b562998a8d1913 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -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) } } } diff --git a/internal/hooks/hooks_test.go b/internal/hooks/hooks_test.go index 4c7353aed49fbf98ca6d9bf00b372d8a63e0a2ab..c0fbec7edf97a1ca9645a4d1efc1403508354ca3 100644 --- a/internal/hooks/hooks_test.go +++ b/internal/hooks/hooks_test.go @@ -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) { diff --git a/internal/hooks/runner.go b/internal/hooks/runner.go index e3b462b39f721620522d4f2a6d61a114a07d0fa6..0e6ba6abba8e564353579b2b024c11527ab965e9 100644 --- a/internal/hooks/runner.go +++ b/internal/hooks/runner.go @@ -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