fix(runtime): avoid false on_user_input notifications - #4010
Conversation
aheritier
left a comment
There was a problem hiding this comment.
Verified locally on 6d5ad03: go build ./..., go test ./pkg/runtime/..., and the new tests under -race all pass. CI is green on this SHA.
The core reasoning holds up. on_user_input means "control is being handed back to a human", and the three gates match that: loop.go:221 skips sub-session and non-interactive teardowns, loop_steps.go:91-105 moves the hook after the non-interactive auto-stop and behind a ctx.Err() guard so a cancelled run can't announce a wait it will never perform, and the elicitation callsite now uses genai.ConversationIDFromContext(ctx) — consistent with elicitation.go:524, which already derived its session id the same way, so the previously-empty session_id is now correct.
Test quality is the strongest part: I applied pkg/runtime/on_user_input_test.go unchanged to origin/main, and 7 of the 8 tests fail there (only RootInteractiveStreamEnd_FiresOnce, which pins pre-existing semantics, passes). That is real regression coverage rather than restated implementation.
The exported signature change is contained: git grep NotifyUserInput finds only hookDispatcher (tool_dispatch.go:129) and the test stub (toolexec/helpers_test.go:62), matching what the description claims.
[should-fix] One-shot --exec runs still emit the false notification
Not a regression from this PR, and not a blocker — but the description says non-interactive runs are now skipped, and that is only true for A2A, MCP serve, chatserver, and evals, which set session.NonInteractive explicitly. Nothing under cmd/ ever sets it (git grep NonInteractive -- cmd returns nothing), so a CLI run keeps NonInteractive == false:
pkg/cli/runner.go:291-298—len(userMessages) > 0: runsoneLoopper message, then returns and the process exitspkg/cli/runner.go:279-289/:299-312— the-and non-tty-stdin branches: singleoneLoop, then exit
Each of those streams tears down through loop.go:221 as a root, "interactive" session, so the hook fires — the exact shape asserted by TestOnUserInputHooks_RootInteractiveStreamEnd_FiresOnce. Concretely, docker agent run --exec agent.yaml "do X" in CI still triggers the desktop notification documented at docs/guides/tips/index.md:389, with no user to notify. Only pkg/cli/runner.go:314-336 (the bare interactive prompt loop) genuinely waits. Worth a follow-up: mark the session non-interactive in the one-shot branches, or have cli.Run signal which mode it is in.
[optional] Third copy of the recording-builtin harness
runtimeWithRecordedUserInput (on_user_input_test.go:31-58) is now the third near-identical copy alongside runtimeWithRecordedAgentSwitch (on_agent_switch_test.go:55-83) and runtimeWithRecordedSessionResume (on_session_resume_test.go:19-41). Following the established pattern is defensible; at three copies a shared helper parameterized by event + builtin name would pay for itself.
[optional] Docs could state the narrowed contract
docs/configuration/hooks/index.md:53 ("When the agent is waiting for user input") is not stale — the fix makes behavior match it more closely. Since consumers deliberately cannot distinguish teardown reasons, a clause noting that sub-sessions, background agents, and non-interactive runs never fire it would save readers a source dive.
[optional] Label
Labelled kind/feat while the commit is fix(runtime):; that will place this under "What's New" instead of "Bug Fixes" in the generated changelog.
Nice touches: every non-obvious gate carries a short WHY comment with a repository-qualified #4004 reference rather than a bare assertion, and the cancelled-context test also asserts MaxIterationsReached is still emitted, pinning that the guard narrows only the input wait.
Summary
on_user_inputat stream teardown only when an interactive root session is waiting for the next promptmax_iterationsFixes #4004
Issue expectations
on_user_inputmax_iterationsremain coveredmax_iterationsbehavior must be explicitValidation
task buildtask lintgo vet ./...task testResidual consideration
toolexec.HookDispatcher.NotifyUserInputnow receives the resolved agent explicitly. This is technically an exported interface signature change, but repository-wide search found only the runtime implementation and its test stub.