Skip to content

refactor(hooks): simplify caching, dispatch flow, and notification helpers#2531

Merged
dgageot merged 2 commits intodocker:mainfrom
dgageot:board/simplifying-hooks-handling-7ba687cc
Apr 27, 2026
Merged

refactor(hooks): simplify caching, dispatch flow, and notification helpers#2531
dgageot merged 2 commits intodocker:mainfrom
dgageot:board/simplifying-hooks-handling-7ba687cc

Conversation

@dgageot
Copy link
Copy Markdown
Member

@dgageot dgageot commented Apr 27, 2026

Same features, less plumbing. Two focused commits split between pkg/hooks and pkg/runtime.

1. refactor(hooks): simplify executor caching and dispatch flow

  • Pre-build per-agent hooks.Executor once in NewLocalRuntime (new buildHooksExecutors). The map is read-only afterwards, so the hooksExecMu sync.RWMutex and the double-checked locking dance go away. hooksExec(a) is now a plain map lookup.
  • Drop matcher.rawpattern == nil now means "match all", collapsing matcher.matches() to a single boolean expression.
  • Move the "Executing hooks" debug log into Executor.Dispatch so it sits where the work actually happens. Drops the redundant exec.Has(event) early-check in dispatchHook (Dispatch already short-circuits on the empty-event case).
  • executeOnUserInputHooks uses r.CurrentAgent() instead of re-implementing the team lookup.
  • Remove unused builtins.AgentDefaults.IsZero() (and its test assertion). ApplyAgentDefaults already returns nil on the zero value, which is what callers actually rely on.

2. refactor(hooks): collapse paired notification helpers; extract hooksFor

  • executeNotificationHooks + executeOnErrorHooks + executeOnMaxIterationsHooksnotifyError + notifyMaxIterations — two composite helpers that fire the (notification, structured) event pair atomically. The two events are always emitted together in practice, so collapsing them removes 2 lines per callsite in loop.go and surfaces intent ("notify error", "notify max iterations") instead of leaving the reader to mentally join two calls. A private notify() helper shares the (level, message)-shaped Input building.
  • Drop the now-dead "invalid notification level" runtime check — the level is always a compile-time constant ("error" / "warning") inside the new helpers, which is stronger enforcement than the runtime branch.
  • Extract Executor.hooksFor(event, toolName) from Dispatch, so Dispatch reads top-to-bottom as "collect → serialize → run → aggregate". Drops the redundant len(matchers)==0 early-return; the len(hooks)==0 check now covers both the no-matchers and no-tool-match cases uniformly.

What stays the same

All hook events (12), all hook types (command, builtin), all builtins (add_date, add_environment_info, add_prompt_files), the agent-flag auto-injection, the Claude-compatible Output protocol (both Decision: "block" and HookSpecificOutput.PermissionDecision: "deny"), and the PreToolUse fail-closed security boundary are all preserved.

Validation

  • go test ./... — all green
  • mise lint — 0 issues
  • Reviewed by the PR-review sub-agent: no critical or non-critical issues found, fail-closed security boundary intact, no missed callsites, no race conditions in the new pre-build path.

Diff stat

 pkg/hooks/builtins/builtins.go      |   5 --
 pkg/hooks/builtins/builtins_test.go |   1 -
 pkg/hooks/executor.go               |  58 +++++++--------
 pkg/runtime/hooks.go                | 142 +++++++++++++++---------------------
 pkg/runtime/loop.go                 |   9 +--
 pkg/runtime/runtime.go              |  16 ++--
 6 files changed, 101 insertions(+), 130 deletions(-)

Assisted-By: docker-agent

dgageot added 2 commits April 27, 2026 15:02
Same features, less plumbing.

- Pre-build per-agent hooks.Executor once in NewLocalRuntime
  (buildHooksExecutors). hooksExec is now a plain map lookup;
  drop the double-checked locking and the hooksExecMu RWMutex.
- Drop the redundant exec.Has(event) early-check in dispatchHook;
  Dispatch already short-circuits on an empty event, and logs
  "Executing hooks" itself, so the runtime side stays focused on
  Warning emission and error handling.
- Drop matcher.raw and rely on pattern == nil to mean "match all",
  collapsing matcher.matches() into a single boolean expression.
- Use r.CurrentAgent() in executeOnUserInputHooks instead of
  re-implementing the team lookup.
- Remove the unused AgentDefaults.IsZero() helper (and its test
  assertion). ApplyAgentDefaults already returns nil on the zero
  value, which is what callers actually rely on.

Assisted-By: docker-agent
Express intent more directly at the runtime callsites and split the
matcher loop out of Executor.Dispatch.

- Replace the per-event executeNotificationHooks /
  executeOnErrorHooks / executeOnMaxIterationsHooks trio with two
  composite helpers, notifyError and notifyMaxIterations, that fire
  the (notification, structured) event pair atomically. The two
  events are always emitted together in practice, so collapsing them
  removes 2 lines per callsite in loop.go and surfaces intent
  ("notify error", "notify max iterations") instead of leaving the
  reader to mentally join two calls. A private notify() helper
  shares the (level, message)-shaped Input building.
- Drop the now-dead "invalid notification level" runtime check: the
  level is always a compile-time constant ("error" / "warning")
  inside the new helpers.
- Extract Executor.hooksFor(event, toolName) from Dispatch, so
  Dispatch reads top-to-bottom as "collect → serialize → run →
  aggregate". Drops the redundant len(matchers)==0 early-return
  (the len(hooks)==0 check now covers both cases uniformly).

Assisted-By: docker-agent
@dgageot dgageot requested a review from a team as a code owner April 27, 2026 13:38
@dgageot dgageot merged commit adcfafb into docker:main Apr 27, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants