refactor(hooks): tighten executor types and dispatch test

Five small, related cleanups now that Config/Hook/MatcherConfig are
plain aliases for the persisted types:

- Drop matcher.raw. The "" and "*" matchers already short-circuit to a
  nil regexp, and so do flat events; matches() collapses to a single
  return statement and the struct loses an unused field.

- Merge hookResult and HandlerResult. They were near-duplicates with
  parallel field names; hookResult now embeds HandlerResult and only
  adds err. runHook stops copying field-by-field, the timeout/exec
  error paths return a fresh hookResult{err: ...} instead of zeroing
  three fields by hand, and aggregate reads the same Stdout/Stderr/
  ExitCode/Output names the rest of the package already uses.

- Pull the JSON-on-stdout fallback out of runHook into
  parseStdoutJSON. Three nested ifs for one straight-line decode is
  easier to read end-to-end.

- Replace the contextEvents map with EventType.consumesContext().
  Behaviour is identical (same four events) but the call site reads
  as a question about the event rather than a map lookup, and the
  knowledge moves next to the EventType definitions.

- Move HookType + HookTypeCommand/HookTypeBuiltin into config.go,
  next to the Hook alias they describe. Types.go is now strictly
  about events, decisions, and the Input/Output payloads.

- Compress dispatch_test.go's per-event fixtures into an onlyHooks
  map literal, iterated twice. The 12-event listing now lives in
  exactly one place in the test file (down from three), matching the
  one place it lives in compileEvents.

Assisted-By: docker-agent

Author: dgageot

pkg/hooks/config.go

@@ -14,8 +14,7 @@ import (
 // short names it always used, while the single source of truth (and
 // the YAML/JSON tags, the validate() method, and IsEmpty) lives next
 // to the schema in pkg/config/latest. Adding a new event is a one-line
-// change on [latest.HooksConfig] plus one line in the executor's
-// compile loop.
+// change on [latest.HooksConfig] plus one line in compileEvents.
 type (
 	// Config is the hooks configuration for an agent.
 	Config = latest.HooksConfig
@@ -27,3 +26,18 @@ type (
 	// it matches (used by EventPreToolUse and EventPostToolUse).
 	MatcherConfig = latest.HookMatcherConfig
 )
+
+// HookType values populate [Hook.Type]. It is an alias for string so
+// hooks authored in YAML round-trip through [latest.HookDefinition]
+// without any conversion; the executor validates the value at registry
+// lookup time.
+type HookType = string
+
+const (
+	// HookTypeCommand runs a shell command.
+	HookTypeCommand HookType = "command"
+	// HookTypeBuiltin dispatches to a named in-process Go function
+	// registered via [Registry.RegisterBuiltin]. The name is stored in
+	// [Hook.Command].
+	HookTypeBuiltin HookType = "builtin"
+)

pkg/hooks/dispatch_test.go

@@ -6,60 +6,61 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-// TestExecutorHasIsGeneric exercises the generic Has API across all
-// configured event kinds to ensure the eventTable is wired correctly. It
-// guards against regressions where adding a new event to the Config
-// struct silently fails to surface in Has/Dispatch.
+// trueHook is a minimal hook reused as the body of every per-event
+// fixture below — the hook's identity doesn't matter to Has, only its
+// presence does.
+var trueHook = []Hook{{Type: HookTypeCommand, Command: "true"}}
+
+// matcherWildcard wraps trueHook in the structure tool-scoped events
+// (PreToolUse, PostToolUse) require.
+var matcherWildcard = []MatcherConfig{{Matcher: "*", Hooks: trueHook}}
+
+// onlyHooks maps each known event to a Config that lights up exactly
+// that event. The cross-product test below iterates this map (once for
+// the empty check, once for the per-event check), so adding a new
+// event is a one-line update here — the same one-line update
+// compileEvents needs.
+var onlyHooks = map[EventType]*Config{
+	EventPreToolUse:      {PreToolUse: matcherWildcard},
+	EventPostToolUse:     {PostToolUse: matcherWildcard},
+	EventSessionStart:    {SessionStart: trueHook},
+	EventTurnStart:       {TurnStart: trueHook},
+	EventBeforeLLMCall:   {BeforeLLMCall: trueHook},
+	EventAfterLLMCall:    {AfterLLMCall: trueHook},
+	EventSessionEnd:      {SessionEnd: trueHook},
+	EventOnUserInput:     {OnUserInput: trueHook},
+	EventStop:            {Stop: trueHook},
+	EventNotification:    {Notification: trueHook},
+	EventOnError:         {OnError: trueHook},
+	EventOnMaxIterations: {OnMaxIterations: trueHook},
+}
+
+// TestExecutorHasIsGeneric exercises the generic Has API across every
+// event kind to ensure compileEvents is wired correctly. It guards
+// against regressions where adding a new event to the Config struct
+// silently fails to surface in Has/Dispatch.
 func TestExecutorHasIsGeneric(t *testing.T) {
 	t.Parallel()
 
 	// Empty config: no event has hooks.
 	empty := NewExecutor(nil, "/tmp", nil)
-	for _, ev := range []EventType{
-		EventPreToolUse, EventPostToolUse, EventSessionStart, EventTurnStart,
-		EventBeforeLLMCall, EventAfterLLMCall,
-		EventSessionEnd, EventOnUserInput, EventStop, EventNotification,
-		EventOnError, EventOnMaxIterations,
-	} {
+	for ev := range onlyHooks {
 		assert.Falsef(t, empty.Has(ev), "empty executor must not report Has(%s)", ev)
 	}
 
-	// Each event populated in turn — the cross-product test ensures that
-	// configuring event X never makes Has(Y) true for Y != X.
-	cases := []struct {
-		event  EventType
-		config *Config
-	}{
-		{EventPreToolUse, &Config{PreToolUse: []MatcherConfig{{Matcher: "*", Hooks: []Hook{{Type: HookTypeCommand, Command: "true"}}}}}},
-		{EventPostToolUse, &Config{PostToolUse: []MatcherConfig{{Matcher: "*", Hooks: []Hook{{Type: HookTypeCommand, Command: "true"}}}}}},
-		{EventSessionStart, &Config{SessionStart: []Hook{{Type: HookTypeCommand, Command: "true"}}}},
-		{EventTurnStart, &Config{TurnStart: []Hook{{Type: HookTypeCommand, Command: "true"}}}},
-		{EventBeforeLLMCall, &Config{BeforeLLMCall: []Hook{{Type: HookTypeCommand, Command: "true"}}}},
-		{EventAfterLLMCall, &Config{AfterLLMCall: []Hook{{Type: HookTypeCommand, Command: "true"}}}},
-		{EventSessionEnd, &Config{SessionEnd: []Hook{{Type: HookTypeCommand, Command: "true"}}}},
-		{EventOnUserInput, &Config{OnUserInput: []Hook{{Type: HookTypeCommand, Command: "true"}}}},
-		{EventStop, &Config{Stop: []Hook{{Type: HookTypeCommand, Command: "true"}}}},
-		{EventNotification, &Config{Notification: []Hook{{Type: HookTypeCommand, Command: "true"}}}},
-		{EventOnError, &Config{OnError: []Hook{{Type: HookTypeCommand, Command: "true"}}}},
-		{EventOnMaxIterations, &Config{OnMaxIterations: []Hook{{Type: HookTypeCommand, Command: "true"}}}},
-	}
-
-	for _, tc := range cases {
-		exec := NewExecutor(tc.config, "/tmp", nil)
-		for _, other := range []EventType{
-			EventPreToolUse, EventPostToolUse, EventSessionStart, EventTurnStart,
-			EventBeforeLLMCall, EventAfterLLMCall,
-			EventSessionEnd, EventOnUserInput, EventStop, EventNotification,
-			EventOnError, EventOnMaxIterations,
-		} {
-			if other == tc.event {
-				assert.Truef(t, exec.Has(other), "configuring %s must light up Has(%s)", tc.event, other)
+	// Cross-product: configuring event ev must light up Has(ev) and
+	// only Has(ev).
+	for ev, cfg := range onlyHooks {
+		exec := NewExecutor(cfg, "/tmp", nil)
+		for other := range onlyHooks {
+			if other == ev {
+				assert.Truef(t, exec.Has(other), "configuring %s must light up Has(%s)", ev, other)
 			} else {
-				assert.Falsef(t, exec.Has(other), "configuring %s must NOT light up Has(%s)", tc.event, other)
+				assert.Falsef(t, exec.Has(other), "configuring %s must NOT light up Has(%s)", ev, other)
 			}
 		}
 	}
 
 	// Unknown events fall through cleanly rather than panicking.
-	assert.False(t, NewExecutor(nil, "/tmp", nil).Has(EventType("does-not-exist")))
+	assert.False(t, empty.Has(EventType("does-not-exist")))
 }

pkg/hooks/executor.go

@@ -22,12 +22,14 @@ type Executor struct {
 	registry   *Registry
 	// events maps each event to its compiled matcher list. Flat events
 	// (everything except pre/post_tool_use) are stored as a single
-	// matcher with an empty pattern, unifying the dispatch path.
+	// matcher with a nil pattern, unifying the dispatch path.
 	events map[EventType][]matcher
 }
 
-// matcher is the compiled form of a [MatcherConfig]: an optional regex
-// pattern (nil means "match all") and the hooks to fire when it matches.
+// matcher is the compiled form of a [MatcherConfig]: a tool-name regex
+// plus the hooks to fire when it matches. A nil pattern matches every
+// tool — both "" and "*" matchers compile to nil, as do flat events
+// where the tool-name dimension doesn't apply.
 type matcher struct {
 	pattern *regexp.Regexp
 	hooks   []Hook
@@ -37,15 +39,6 @@ func (m *matcher) matches(toolName string) bool {
 	return m.pattern == nil || m.pattern.MatchString(toolName)
 }
 
-// hookResult is the outcome of a single hook invocation.
-type hookResult struct {
-	output   *Output
-	stdout   string
-	stderr   string
-	exitCode int
-	err      error
-}
-
 // NewExecutor creates a new hook executor backed by [DefaultRegistry].
 func NewExecutor(config *Config, workingDir string, env []string) *Executor {
 	return NewExecutorWithRegistry(config, workingDir, env, DefaultRegistry)
@@ -188,6 +181,17 @@ func dedupKey(h Hook) string {
 	return b.String()
 }
 
+// hookResult is the outcome of a single hook invocation: the raw
+// [HandlerResult] reported by the handler plus a post-execution err
+// (factory failure, timeout, exec error). When err is non-nil the
+// handler-reported fields are reset to a uniform "did not run"
+// representation so [aggregate] can rely on the err alone.
+type hookResult struct {
+	HandlerResult
+
+	err error
+}
+
 // runHook resolves the hook's [HookType] in the registry, applies its
 // timeout, and returns the structured outcome. JSON-on-stdout is parsed
 // into [Output] when the handler didn't already provide one.
@@ -205,7 +209,7 @@ func (e *Executor) runHook(ctx context.Context, hook Hook, inputJSON []byte) hoo
 	defer cancel()
 
 	res, err := handler.Run(timeoutCtx, inputJSON)
-	r := hookResult{stdout: res.Stdout, stderr: res.Stderr, exitCode: res.ExitCode, output: res.Output}
+	r := hookResult{HandlerResult: res}
 
 	// Normalize timeout/cancellation: handler error types vary, so we
 	// rewrite to a uniform error so PreToolUse fails closed cleanly.
@@ -214,40 +218,33 @@ func (e *Executor) runHook(ctx context.Context, hook Hook, inputJSON []byte) hoo
 		if errors.Is(ctxErr, context.DeadlineExceeded) {
 			reason = fmt.Sprintf("timed out after %s", hook.GetTimeout())
 		}
-		r.err = fmt.Errorf("hook %q %s: %w", hook.Command, reason, ctxErr)
-		r.exitCode = -1
-		r.output = nil
-		return r
+		return hookResult{err: fmt.Errorf("hook %q %s: %w", hook.Command, reason, ctxErr)}
 	}
 	if err != nil {
-		r.err = err
-		r.exitCode = -1
-		r.output = nil
-		return r
+		return hookResult{err: err}
 	}
 
 	// Fall back to the legacy "parse JSON from stdout" protocol.
-	if r.output == nil && r.exitCode == 0 && r.stdout != "" {
-		s := strings.TrimSpace(r.stdout)
-		if strings.HasPrefix(s, "{") {
-			var parsed Output
-			if jerr := json.Unmarshal([]byte(s), &parsed); jerr == nil {
-				r.output = &parsed
-			}
-		}
+	if r.Output == nil && r.ExitCode == 0 {
+		r.Output = parseStdoutJSON(r.Stdout)
 	}
 	return r
 }
 
-// contextEvents are the events whose runtime emit sites consume
-// Result.AdditionalContext. Plain stdout from a hook is routed there
-// for these events; for observational events it is silently dropped to
-// avoid the impression that it mattered.
-var contextEvents = map[EventType]bool{
-	EventSessionStart: true,
-	EventTurnStart:    true,
-	EventPostToolUse:  true,
-	EventStop:         true,
+// parseStdoutJSON returns a parsed [Output] when stdout begins with '{'
+// and decodes cleanly, or nil otherwise. Used for the legacy "JSON on
+// stdout" hook protocol where handlers don't pre-populate
+// [HandlerResult.Output].
+func parseStdoutJSON(stdout string) *Output {
+	s := strings.TrimSpace(stdout)
+	if !strings.HasPrefix(s, "{") {
+		return nil
+	}
+	var parsed Output
+	if err := json.Unmarshal([]byte(s), &parsed); err != nil {
+		return nil
+	}
+	return &parsed
 }
 
 // aggregate combines per-hook results into a single [Result].
@@ -263,36 +260,36 @@ func aggregate(results []hookResult, event EventType) *Result {
 				slog.Warn("PreToolUse hook failed to execute; denying tool call", "error", r.err)
 				final.Allowed = false
 				final.ExitCode = -1
-				final.Stderr = r.stderr
+				final.Stderr = r.Stderr
 				messages = append(messages, fmt.Sprintf("PreToolUse hook failed to execute: %v", r.err))
 			} else {
 				slog.Warn("Hook execution error", "error", r.err)
 			}
 			continue
 
-		case r.exitCode == 2:
+		case r.ExitCode == 2:
 			final.Allowed = false
 			final.ExitCode = 2
-			if r.stderr != "" {
-				final.Stderr = r.stderr
-				messages = append(messages, strings.TrimSpace(r.stderr))
+			if r.Stderr != "" {
+				final.Stderr = r.Stderr
+				messages = append(messages, strings.TrimSpace(r.Stderr))
 			}
 			continue
 
-		case r.exitCode != 0:
-			slog.Debug("Hook returned non-zero exit code", "exit_code", r.exitCode, "stderr", r.stderr)
+		case r.ExitCode != 0:
+			slog.Debug("Hook returned non-zero exit code", "exit_code", r.ExitCode, "stderr", r.Stderr)
 			continue
 
-		case r.output == nil:
+		case r.Output == nil:
 			// Plain stdout becomes AdditionalContext only for events
 			// whose runtime consumes it.
-			if r.stdout != "" && contextEvents[event] {
-				contexts = append(contexts, strings.TrimSpace(r.stdout))
+			if r.Stdout != "" && event.consumesContext() {
+				contexts = append(contexts, strings.TrimSpace(r.Stdout))
 			}
 			continue
 		}
 
-		out := r.output
+		out := r.Output
 		if !out.ShouldContinue() {
 			final.Allowed = false
 			if out.StopReason != "" {

pkg/hooks/types.go

@@ -41,20 +41,18 @@ const (
 	EventOnMaxIterations EventType = "on_max_iterations"
 )
 
-// HookType values populate [Hook.Type]. It is an alias for string so a
-// hook authored in YAML round-trips through [latest.HookDefinition]
-// without any conversion; the executor validates the value at registry
-// lookup time.
-type HookType = string
-
-const (
-	// HookTypeCommand runs a shell command.
-	HookTypeCommand HookType = "command"
-	// HookTypeBuiltin dispatches to a named in-process Go function
-	// registered via [Registry.RegisterBuiltin]. The name is stored in
-	// [Hook.Command].
-	HookTypeBuiltin HookType = "builtin"
-)
+// consumesContext reports whether the runtime emit site for e routes
+// [Result.AdditionalContext] somewhere meaningful (a system message, a
+// transient turn_start prompt, ...). For observational events it is
+// silently dropped, so plain stdout from a hook is also discarded for
+// those.
+func (e EventType) consumesContext() bool {
+	switch e {
+	case EventSessionStart, EventTurnStart, EventPostToolUse, EventStop:
+		return true
+	}
+	return false
+}
 
 // Input is the JSON-serializable payload passed to hooks via stdin.
 type Input struct {