fix(hooks): address review findings on the new hook events

Three issues surfaced by code review:

* subagent_stop now fires on the error path of both sub-session helpers.
  Previously the hook was placed *after* the for-range childEvents loop,
  which an ErrorEvent skipped via early return \u2014 so handlers configured
  to observe sub-agent completions silently missed every failed run.
  Move the dispatch into a defer at the top of each helper so it fires
  for both success and failure (handlers can detect failure via empty
  stop_response or by correlating with the parent's error event).

* Clarify the EventPermissionRequest doc comment. The old text said the
  hook "mirrors pre_tool_use" \u2014 misleading, because pre_tool_use treats
  allow as the implicit default while permission_request treats it as
  an explicit auto-approve verdict (and that asymmetry is the whole
  reason Result.PermissionAllowed exists separately from Result.Allowed).
  New comment spells out the contract.

* examples/hooks.yaml now documents that the command-style hooks need
  jq, with install hints and a note on the graceful-degradation behaviour
  when jq is missing.

Plus a regression test pinning the user_prompt_submit gating contract:
fires exactly once on a top-level submission, never on a sub-session
(SendUserMessage=false). The test caches a counter across the two
cases via a tiny shared scaffold.

Assisted-By: docker-agent

Author: dgageot

examples/hooks.yaml

@@ -36,6 +36,12 @@
 #             for parameters. Three are shipped: add_date,
 #             add_environment_info, add_prompt_files.
 #
+# Requirements: the command-style hooks below pipe stdin through `jq` for
+# convenient JSON access. If you don't have jq installed (`brew install jq`
+# on macOS, `apt-get install jq` on Debian/Ubuntu), a hook will exit non-zero
+# and the runtime will log a 'Hook execution error' warning but otherwise
+# continue — use plain `awk`/`sed`/`grep` or any other parser if you prefer.
+#
 # Try these prompts:
 #   "Run: echo hello"           → allowed
 #   "Run: rm -rf /tmp/test"     → denied by pre_tool_use

pkg/hooks/types.go

@@ -22,9 +22,20 @@ const (
 	// patterns like a tool-call loop detector.
 	EventPostToolUse EventType = "post_tool_use"
 	// EventPermissionRequest fires just before the runtime would prompt
-	// the user to confirm a tool call. Hook may auto-allow or auto-deny
-	// via [HookSpecificOutput.PermissionDecision]; otherwise control
-	// falls through to the interactive confirmation.
+	// the user to confirm a tool call (i.e. when neither --yolo nor a
+	// permissions rule short-circuited the decision and the tool is not
+	// read-only). The hook can short-circuit the prompt by returning
+	// [HookSpecificOutput.PermissionDecision] = "allow" (sets
+	// [Result.PermissionAllowed] true — the runtime invokes the tool
+	// without asking) or "deny" (sets [Result.Allowed] false — the
+	// runtime rejects the tool with the hook's reason). Returning
+	// nothing falls through to the interactive confirmation.
+	//
+	// Unlike pre_tool_use — where allow is the implicit default and only
+	// deny carries new information — here allow is the explicit
+	// auto-approve verdict; that asymmetry is why permission_request
+	// has its own [Result.PermissionAllowed] flag separate from
+	// [Result.Allowed].
 	EventPermissionRequest EventType = "permission_request"
 	// EventSessionStart fires when a session begins or resumes.
 	EventSessionStart EventType = "session_start"

pkg/runtime/agent_delegation.go

@@ -180,6 +180,17 @@ func mergeExcludedTools(parent, child []string) []string {
 // This is the "interactive" path used by transfer_task where the parent agent
 // loop is blocked while the child executes.
 func (r *LocalRuntime) runSubSessionForwarding(ctx context.Context, parent, child *session.Session, span trace.Span, evts chan Event, parentAgent *agent.Agent, subAgentName string) (*tools.ToolCallResult, error) {
+	// subagent_stop fires after the child's stream has fully drained,
+	// using the *parent* agent's executor so handlers configured on the
+	// orchestrator see every child completion in one place — success or
+	// failure. The deferred call ensures we don't lose the event when an
+	// ErrorEvent triggers an early return below; handlers can detect a
+	// failed run by an empty stop_response (or by correlating with the
+	// session-level error event the parent already received).
+	defer func() {
+		r.executeSubagentStopHooks(ctx, parent, child, parentAgent, subAgentName, child.GetLastAssistantMessageContent())
+	}()
+
 	childEvents := r.RunStream(ctx, child)
 	for event := range childEvents {
 		evts <- event
@@ -200,14 +211,8 @@ func (r *LocalRuntime) runSubSessionForwarding(ctx context.Context, parent, chil
 	parent.AddSubSession(child)
 	evts <- SubSessionCompleted(parent.ID, child, parentAgent.Name())
 
-	// subagent_stop fires after the child's stream has fully drained,
-	// using the *parent* agent's executor so handlers configured on the
-	// orchestrator see every child completion in one place.
-	response := child.GetLastAssistantMessageContent()
-	r.executeSubagentStopHooks(ctx, parent, child, parentAgent, subAgentName, response)
-
 	span.SetStatus(codes.Ok, "sub-session completed")
-	return tools.ResultSuccess(response), nil
+	return tools.ResultSuccess(child.GetLastAssistantMessageContent()), nil
 }
 
 // runSubSessionCollecting runs a child session, collecting output via an
@@ -217,6 +222,17 @@ func (r *LocalRuntime) runSubSessionForwarding(ctx context.Context, parent, chil
 // It returns a RunResult containing either the final assistant message or
 // an error message.
 func (r *LocalRuntime) runSubSessionCollecting(ctx context.Context, parent, child *session.Session, subAgentName string, onContent func(string)) *agenttool.RunResult {
+	// subagent_stop fires after the background sub-session has fully
+	// drained — success or failure. The parent agent at the time of
+	// dispatch (whoever called run_background_agent) owns the executor;
+	// we resolve it via CurrentAgent because the background path doesn't
+	// carry the parent agent name. dispatchHook silently no-ops when
+	// CurrentAgent is nil. The deferred call ensures the hook fires even
+	// when an ErrorEvent or ctx cancellation breaks us out of the loop.
+	defer func() {
+		r.executeSubagentStopHooks(ctx, parent, child, r.CurrentAgent(), subAgentName, child.GetLastAssistantMessageContent())
+	}()
+
 	var errMsg string
 	events := r.RunStream(ctx, child)
 	for event := range events {
@@ -245,13 +261,6 @@ func (r *LocalRuntime) runSubSessionCollecting(ctx context.Context, parent, chil
 	result := child.GetLastAssistantMessageContent()
 	parent.AddSubSession(child)
 
-	// subagent_stop fires after the background sub-session has fully
-	// drained. The parent agent at the time of dispatch (whoever called
-	// run_background_agent) owns the executor; we resolve it via
-	// CurrentAgent because the background path doesn't carry the parent
-	// agent name. dispatchHook silently no-ops when CurrentAgent is nil.
-	r.executeSubagentStopHooks(ctx, parent, child, r.CurrentAgent(), subAgentName, result)
-
 	return &agenttool.RunResult{Result: result}
 }
 

pkg/runtime/user_prompt_submit_test.go

@@ -0,0 +1,106 @@
+package runtime
+
+import (
+	"context"
+	"sync/atomic"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/docker/docker-agent/pkg/agent"
+	"github.com/docker/docker-agent/pkg/config/latest"
+	"github.com/docker/docker-agent/pkg/hooks"
+	"github.com/docker/docker-agent/pkg/session"
+	"github.com/docker/docker-agent/pkg/team"
+)
+
+// TestUserPromptSubmitFiresOncePerTopLevelTurn pins the contract that
+// user_prompt_submit fires exactly once per real user message in a
+// top-level session: not once per LLM call, not once per turn, but
+// once per submission. The runtime gates the dispatch in
+// [LocalRuntime.RunStream].
+func TestUserPromptSubmitFiresOncePerTopLevelTurn(t *testing.T) {
+	t.Parallel()
+
+	calls, rt, sess := setupUserPromptSubmitCounter(t,
+		session.WithUserMessage("hi"),
+	)
+
+	for range rt.RunStream(t.Context(), sess) {
+	}
+
+	assert.Equal(t, int32(1), calls.Load(),
+		"user_prompt_submit must fire exactly once for a top-level user submission")
+}
+
+// TestUserPromptSubmitSkippedForSubSessions pins the design choice
+// that user_prompt_submit fires for *human* prompts only. Sub-sessions
+// (transferred tasks, background agents, skill sub-sessions) carry a
+// runtime-synthesised "Please proceed." kick-off message that no human
+// authored, so firing the hook there would be noise. The runtime gates
+// the dispatch on [session.Session.SendUserMessage], which is exactly
+// the same flag the runtime uses to decide whether to emit a
+// [UserMessageEvent] \u2014 a sub-session sets it to false.
+func TestUserPromptSubmitSkippedForSubSessions(t *testing.T) {
+	t.Parallel()
+
+	calls, rt, sess := setupUserPromptSubmitCounter(t,
+		session.WithUserMessage("synthesised kick-off"),
+		session.WithSendUserMessage(false),
+	)
+
+	for range rt.RunStream(t.Context(), sess) {
+	}
+
+	assert.Equal(t, int32(0), calls.Load(),
+		"user_prompt_submit must NOT fire for sub-sessions (SendUserMessage=false): "+
+			"their kick-off message is synthesised by the runtime, not authored by a human")
+}
+
+// setupUserPromptSubmitCounter wires up a single-turn mock runtime with
+// a builtin user_prompt_submit hook that atomically increments the
+// returned counter on every dispatch. Both tests above share this
+// scaffolding so the only thing that varies between them is the
+// session's [session.WithSendUserMessage] flag.
+func setupUserPromptSubmitCounter(t *testing.T, opts ...session.Opt) (*atomic.Int32, *LocalRuntime, *session.Session) {
+	t.Helper()
+
+	const counterName = "test-user-prompt-submit-counter"
+	var calls atomic.Int32
+
+	stream := newStreamBuilder().
+		AddContent("ok").
+		AddStopWithUsage(3, 2).
+		Build()
+	prov := &mockProvider{id: "test/mock-model", stream: stream}
+
+	root := agent.New("root", "test agent",
+		agent.WithModel(prov),
+		agent.WithHooks(&latest.HooksConfig{
+			UserPromptSubmit: []latest.HookDefinition{
+				{Type: "builtin", Command: counterName},
+			},
+		}),
+	)
+	tm := team.New(team.WithAgents(root))
+
+	rt, err := NewLocalRuntime(tm,
+		WithSessionCompaction(false),
+		WithModelStore(mockModelStore{}),
+	)
+	require.NoError(t, err)
+
+	require.NoError(t, rt.hooksRegistry.RegisterBuiltin(
+		counterName,
+		func(_ context.Context, _ *hooks.Input, _ []string) (*hooks.Output, error) {
+			calls.Add(1)
+			return nil, nil
+		},
+	))
+
+	sess := session.New(opts...)
+	sess.Title = "Unit Test"
+
+	return &calls, rt, sess
+}