git.iko.soy/crush
on main help sign in
refactor(hooks): move matcher compilation into the runner
c84ca2dChristian Rocha
internal/config/config.go+3−11
compare with main
unifiedsplit
@@ -9,7 +9,6 @@ import (
9 "maps"
9 "maps"
10 "net/http"
10 "net/http"
11 "net/url"
11 "net/url"
12− "regexp"
13 "slices"
12 "slices"
14 "strings"
13 "strings"
15 "time"
14 "time"
@@ -371,7 +370,9 @@ func (t ToolGrep) GetTimeout() time.Duration {
371}
370}
372
371
373// HookConfig defines a user-configured shell command that fires on a hook
372// HookConfig defines a user-configured shell command that fires on a hook
374−// event (e.g. PreToolUse).
373+// event (e.g. PreToolUse). This is a pure-data struct: matcher compilation
374+// is owned by hooks.Runner so a JSON round-trip, merge, or reload can't
375+// silently drop compiled state.
375type HookConfig struct {
376type HookConfig struct {
376 // Regex pattern tested against the tool name. Empty means match all.
377 // Regex pattern tested against the tool name. Empty means match all.
377 Matcher string `json:"matcher,omitempty" jsonschema:"description=Regex pattern tested against the tool name. Empty means match all tools."`
378 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 {
379 Command string `json:"command" jsonschema:"required,description=Shell command to execute when the hook fires"`
380 Command string `json:"command" jsonschema:"required,description=Shell command to execute when the hook fires"`
380 // Timeout in seconds. Default 30.
381 // Timeout in seconds. Default 30.
381 Timeout int `json:"timeout,omitempty" jsonschema:"description=Timeout in seconds for the hook command,default=30"`
382 Timeout int `json:"timeout,omitempty" jsonschema:"description=Timeout in seconds for the hook command,default=30"`
382−
383− // Compiled matcher regex. Not serialized.
384− matcherRegex *regexp.Regexp
385−}
386−
387−// MatcherRegex returns the compiled matcher regex, or nil if no matcher is
388−// set.
389−func (h *HookConfig) MatcherRegex() *regexp.Regexp {
390− return h.matcherRegex
391}
383}
392
384
393// TimeoutDuration returns the hook timeout as a time.Duration, defaulting
385// TimeoutDuration returns the hook timeout as a time.Duration, defaulting
internal/config/load.go+11−10
@@ -905,8 +905,11 @@ func normalizeHookEvent(name string) string {
905 }
905 }
906}
906}
907
907
908−// ValidateHooks normalizes event names and compiles matcher regexes for all
908+// ValidateHooks normalizes event names and checks that every configured
909−// configured hooks. Returns an error if any regex is invalid.
909+// hook has a command and a syntactically valid matcher regex. Matcher
910+// compilation used for matching is owned by hooks.Runner; this function
911+// only validates up front so the user sees config errors at load time
912+// rather than on the first tool call.
910func (c *Config) ValidateHooks() error {
913func (c *Config) ValidateHooks() error {
911 // Normalize event name keys.
914 // Normalize event name keys.
912 for event, eventHooks := range c.Hooks {
915 for event, eventHooks := range c.Hooks {
@@ -918,17 +921,15 @@ func (c *Config) ValidateHooks() error {
918 }
921 }
919
922
920 for event, eventHooks := range c.Hooks {
923 for event, eventHooks := range c.Hooks {
921− for i := range eventHooks {
924+ for i, h := range eventHooks {
922− h := &c.Hooks[event][i]
923 if h.Command == "" {
925 if h.Command == "" {
924 return fmt.Errorf("hook %s[%d]: command is required", event, i)
926 return fmt.Errorf("hook %s[%d]: command is required", event, i)
925 }
927 }
926− if h.Matcher != "" {
928+ if h.Matcher == "" {
929+ continue
930+ }
927− re, err := regexp.Compile(h.Matcher)
931+ if _, err := regexp.Compile(h.Matcher); err != nil {
928− if err != nil {
929− return fmt.Errorf("hook %s[%d]: invalid matcher regex %q: %w", event, i, h.Matcher, err)
932+ return fmt.Errorf("hook %s[%d]: invalid matcher regex %q: %w", event, i, h.Matcher, err)
930− }
931− h.matcherRegex = re
932 }
933 }
933 }
934 }
934 }
935 }
internal/hooks/hooks_test.go+34
@@ -373,6 +373,40 @@ func TestRunnerMatcherFiltering(t *testing.T) {
373 require.NoError(t, err)
373 require.NoError(t, err)
374 require.Equal(t, DecisionNone, result.Decision)
374 require.Equal(t, DecisionNone, result.Decision)
375 })
375 })
376+
377+ // Runner must compile matchers itself; it cannot rely on
378+ // ValidateHooks having run first. This is the guarantee that prevents
379+ // the reload-drops-matcher class of bug.
380+ t.Run("runner compiles matcher without ValidateHooks", func(t *testing.T) {
381+ t.Parallel()
382+ raw := []config.HookConfig{
383+ {Command: `echo '{"decision":"deny","reason":"blocked"}'`, Matcher: "^bash$"},
384+ }
385+ r := NewRunner(raw, t.TempDir(), t.TempDir())
386+
387+ deny, err := r.Run(context.Background(), EventPreToolUse, "sess", "bash", `{}`)
388+ require.NoError(t, err)
389+ require.Equal(t, DecisionDeny, deny.Decision)
390+
391+ noop, err := r.Run(context.Background(), EventPreToolUse, "sess", "view", `{}`)
392+ require.NoError(t, err)
393+ require.Equal(t, DecisionNone, noop.Decision)
394+ })
395+
396+ // A matcher that fails to compile at Runner construction must not
397+ // degrade to match-everything; the hook is dropped instead.
398+ t.Run("runner skips hooks with invalid matcher", func(t *testing.T) {
399+ t.Parallel()
400+ raw := []config.HookConfig{
401+ {Command: `echo '{"decision":"deny","reason":"should not fire"}'`, Matcher: "[invalid"},
402+ }
403+ r := NewRunner(raw, t.TempDir(), t.TempDir())
404+
405+ result, err := r.Run(context.Background(), EventPreToolUse, "sess", "bash", `{}`)
406+ require.NoError(t, err)
407+ require.Equal(t, DecisionNone, result.Decision)
408+ require.Empty(t, r.Hooks())
409+ })
376}
410}
377
411
378func TestValidateHooksInvalidRegex(t *testing.T) {
412func TestValidateHooksInvalidRegex(t *testing.T) {
internal/hooks/runner.go+46−8
@@ -5,6 +5,7 @@ import (
5 "context"
5 "context"
6 "log/slog"
6 "log/slog"
7 "os/exec"
7 "os/exec"
8+ "regexp"
8 "strings"
9 "strings"
9 "sync"
10 "sync"
10 "time"
11 "time"
@@ -12,25 +13,63 @@ import (
12 "github.com/charmbracelet/crush/internal/config"
13 "github.com/charmbracelet/crush/internal/config"
13)
14)
14
15
16+// compiledHook pairs a HookConfig with its compiled matcher regex. A nil
17+// matcher means "match every tool".
18+type compiledHook struct {
19+ cfg config.HookConfig
20+ matcher *regexp.Regexp
21+}
22+
15// Runner executes hook commands and aggregates their results.
23// Runner executes hook commands and aggregates their results.
16type Runner struct {
24type Runner struct {
17− hooks []config.HookConfig
25+ hooks []compiledHook
18 cwd string
26 cwd string
19 projectDir string
27 projectDir string
20}
28}
21
29
22−// NewRunner creates a Runner from the given hook configs.
30+// NewRunner creates a Runner from the given hook configs. Each hook's
31+// Matcher is compiled here so the Runner is self-sufficient; callers do
32+// not have to pre-compile matchers on the config, and reloads or merges
33+// that rebuild HookConfig values can't silently strip compiled state.
34+//
35+// Hooks whose matcher fails to compile are skipped with a warning rather
36+// than treated as match-everything. ValidateHooks is expected to have
37+// caught syntax errors earlier, so this is defense in depth.
23func NewRunner(hooks []config.HookConfig, cwd, projectDir string) *Runner {
38func NewRunner(hooks []config.HookConfig, cwd, projectDir string) *Runner {
39+ compiled := make([]compiledHook, 0, len(hooks))
40+ for _, h := range hooks {
41+ ch := compiledHook{cfg: h}
42+ if h.Matcher != "" {
43+ re, err := regexp.Compile(h.Matcher)
44+ if err != nil {
45+ slog.Warn("Hook matcher failed to compile; skipping hook",
46+ "matcher", h.Matcher,
47+ "command", h.Command,
48+ "error", err,
49+ )
50+ continue
51+ }
52+ ch.matcher = re
53+ }
54+ compiled = append(compiled, ch)
55+ }
24 return &Runner{
56 return &Runner{
25− hooks: hooks,
57+ hooks: compiled,
26 cwd: cwd,
58 cwd: cwd,
27 projectDir: projectDir,
59 projectDir: projectDir,
28 }
60 }
29}
61}
30
62
31−// Hooks returns the hook configs the runner was created with.
63+// Hooks returns the hook configs the runner was created with, in config
64+// order. Hooks whose matcher failed to compile at construction are
65+// omitted. Intended for diagnostics; callers should not rely on ordering
66+// or identity beyond that.
32func (r *Runner) Hooks() []config.HookConfig {
67func (r *Runner) Hooks() []config.HookConfig {
68+ out := make([]config.HookConfig, len(r.hooks))
69+ for i, h := range r.hooks {
70+ out[i] = h.cfg
71+ }
33− return r.hooks
72+ return out
34}
73}
35
74
36// Run executes all matching hooks for the given event and tool, returning
75// 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
93func (r *Runner) matchingHooks(toolName string) []config.HookConfig {
132func (r *Runner) matchingHooks(toolName string) []config.HookConfig {
94 var matched []config.HookConfig
133 var matched []config.HookConfig
95 for _, h := range r.hooks {
134 for _, h := range r.hooks {
96− re := h.MatcherRegex()
97− if re == nil || re.MatchString(toolName) {
135+ if h.matcher == nil || h.matcher.MatchString(toolName) {
98− matched = append(matched, h)
136+ matched = append(matched, h.cfg)
99 }
137 }
100 }
138 }
101 return matched
139 return matched