Merge pull request #2534 from dgageot/board/refactoring-boilerplate-in-pkg-hooks-con-00fdb9e5

refactor(hooks): drop runtime shadow types and tighten the executor

Author: dgageot

pkg/config/latest/types.go

@@ -1727,6 +1727,15 @@ type HookDefinition struct {
 	Timeout int `json:"timeout,omitempty" yaml:"timeout,omitempty"`
 }
 
+// GetTimeout returns the per-hook execution timeout, defaulting to 60
+// seconds when [HookDefinition.Timeout] is zero or negative.
+func (h *HookDefinition) GetTimeout() time.Duration {
+	if h.Timeout <= 0 {
+		return 60 * time.Second
+	}
+	return time.Duration(h.Timeout) * time.Second
+}
+
 // validate validates the HooksConfig
 func (h *HooksConfig) validate() error {
 	// Validate PreToolUse matchers

pkg/hooks/config.go

@@ -4,50 +4,40 @@ import (
 	"github.com/docker/docker-agent/pkg/config/latest"
 )
 
-// FromConfig converts a [latest.HooksConfig] into the runtime [Config].
-func FromConfig(cfg *latest.HooksConfig) *Config {
-	if cfg == nil {
-		return nil
-	}
-	return &Config{
-		PreToolUse:      convertMatchers(cfg.PreToolUse),
-		PostToolUse:     convertMatchers(cfg.PostToolUse),
-		SessionStart:    convertHooks(cfg.SessionStart),
-		TurnStart:       convertHooks(cfg.TurnStart),
-		BeforeLLMCall:   convertHooks(cfg.BeforeLLMCall),
-		AfterLLMCall:    convertHooks(cfg.AfterLLMCall),
-		SessionEnd:      convertHooks(cfg.SessionEnd),
-		OnUserInput:     convertHooks(cfg.OnUserInput),
-		Stop:            convertHooks(cfg.Stop),
-		Notification:    convertHooks(cfg.Notification),
-		OnError:         convertHooks(cfg.OnError),
-		OnMaxIterations: convertHooks(cfg.OnMaxIterations),
-	}
-}
+// The hooks package and the persisted [latest.HooksConfig] used to
+// declare two parallel struct hierarchies with identical fields, just
+// to attach a single helper method (GetTimeout) and a typed Type
+// string. The cost was a 12-events-listed-in-four-places translation
+// layer that grew every time a new event was added.
+//
+// Today they're the same types: aliases below give the runtime the
+// 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 compileEvents.
+type (
+	// Config is the hooks configuration for an agent.
+	Config = latest.HooksConfig
+	// Hook is a single hook entry. The Type field is one of the
+	// HookType* constants below; unrecognised values are rejected by
+	// the executor at registry lookup.
+	Hook = latest.HookDefinition
+	// MatcherConfig pairs a tool-name regex with the hooks to run when
+	// it matches (used by EventPreToolUse and EventPostToolUse).
+	MatcherConfig = latest.HookMatcherConfig
+)
 
-func convertMatchers(in []latest.HookMatcherConfig) []MatcherConfig {
-	if len(in) == 0 {
-		return nil
-	}
-	out := make([]MatcherConfig, len(in))
-	for i, m := range in {
-		out[i] = MatcherConfig{Matcher: m.Matcher, Hooks: convertHooks(m.Hooks)}
-	}
-	return out
-}
+// 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
 
-func convertHooks(in []latest.HookDefinition) []Hook {
-	if len(in) == 0 {
-		return nil
-	}
-	out := make([]Hook, len(in))
-	for i, h := range in {
-		out[i] = Hook{
-			Type:    HookType(h.Type),
-			Command: h.Command,
-			Args:    h.Args,
-			Timeout: h.Timeout,
-		}
-	}
-	return out
-}
+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")))
 }