Skip hook loading and lifecycle events for subagents - #10596
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7b723b9de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
🤖 Addressed - added an |
SessionStart hooks can now return {"banner":"..."} on stdout to
contribute lines to the CLI startup banner. Multiple plugins can each
provide banner content, printed between 'goose is ready' and the
context usage bar.
This enables plugins like session-continuity tools, project dashboards,
and reminder systems to surface information at the moment it's most
useful - before the user types their first prompt.
Backwards compatible: hooks that don't output JSON or lack a banner
field behave exactly as before.
Subagents were creating a full HookManager on every delegate spawn, discovering all enabled plugins, parsing hooks.json files, compiling regex matchers, and then firing SessionStart (which executed user scripts like banner generators mid-session). This adds is_subagent to AgentConfig (defaults false) and assigns an empty HookManager when true, eliminating plugin discovery overhead and preventing spurious lifecycle events during delegation.
When the CLI calls emit_hook_with_banners(SessionStart) for banner collection, mark the event as already emitted so reply() does not fire it again on the first agent turn. Non-CLI paths (gateway, ACP server, scheduler) that go directly through reply() still emit it once via the atomic swap guard.
6e60aee to
7781486
Compare
|
🤖 Rebased onto current main. Conflict resolved (unused imports from earlier iteration removed). CI was green before, should pass again on the new base. Ready for review whenever someone has bandwidth. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7781486fab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if event == crate::hooks::HookEvent::SessionStart { | ||
| self.session_start_emitted.store(true, Ordering::Release); | ||
| } |
There was a problem hiding this comment.
Guard SessionStart in the state-machine path
When GOOSE_STATE_MACHINE=1 is enabled for a fresh interactive session, this flag only suppresses the legacy reply emission; StateMachine::emit_entry_hooks still emits SessionStart unconditionally at crates/goose/src/agents/state_machine/machine.rs:64-70. Consequently, the hook executed for the banner in session/mod.rs runs again when the first prompt enters the state machine, repeating setup, analytics, or other side effects. Fresh evidence beyond the earlier comment is that the legacy branch now checks this atomic at agent.rs:1936, while the state-machine branch does not, so the suppression needs to be shared with and tested in both paths.
AGENTS.md reference: AGENTS.md:L21-L23
Useful? React with 👍 / 👎.
|
Thanks for the submission! It makes sense to me. Merging. |
* origin/main: fix(mcp): prune dead notification subscribers (#11032) chore: remove the extension and tool count suggestion (#10869) feat: compaction in the GDK (#11042) fix(provider): retry transient errors on first stream item before ending turn (#10968) feat(cli): add /new to start a fresh session without restarting (#10767) feat(acp): title new sessions from _meta.sessionTitle (#10712) fix: adjust rmcp::model::Meta ref (#11107) Skip hook loading and lifecycle events for subagents (#10596) Sanitize Unicode tags in Responses output (#10745) fix(conversation): sanitize nested tool responses (#10609) fix(hints): bound recursive file expansion (#10546) fix(providers): drop stale signed thinking blocks after a mid-conversation model switch (#10007) fix(desktop): clarify compact cost display (#11093) Index messages by (session_id, created_timestamp, id) to stop on-disk sort storms (#10874) docs: add tool shim guide covering when to enable, backends, and troubleshooting (#10858) fix(deep-link): route extension/session deep links to regular windows not standalone app windows (#10908) fix(ui): raise chat input z-index so slash menu appears above loading indicator (#11015) fix(ui): support remote working directory for external backend (#10827)
* main: fix(mcp): prune dead notification subscribers (#11032) chore: remove the extension and tool count suggestion (#10869) feat: compaction in the GDK (#11042) fix(provider): retry transient errors on first stream item before ending turn (#10968) feat(cli): add /new to start a fresh session without restarting (#10767) feat(acp): title new sessions from _meta.sessionTitle (#10712) fix: adjust rmcp::model::Meta ref (#11107) Skip hook loading and lifecycle events for subagents (#10596) Sanitize Unicode tags in Responses output (#10745) fix(conversation): sanitize nested tool responses (#10609) fix(hints): bound recursive file expansion (#10546) fix(providers): drop stale signed thinking blocks after a mid-conversation model switch (#10007) fix(desktop): clarify compact cost display (#11093) Index messages by (session_id, created_timestamp, id) to stop on-disk sort storms (#10874) docs: add tool shim guide covering when to enable, backends, and troubleshooting (#10858) fix(deep-link): route extension/session deep links to regular windows not standalone app windows (#10908) fix(ui): raise chat input z-index so slash menu appears above loading indicator (#11015) fix(ui): support remote working directory for external backend (#10827)
Problem
When goose delegates a task to a subagent, the subagent creates a fresh
Agentinstance that loads the full hook lifecycle - discovering plugins, parsinghooks.json, compiling matchers, and firingSessionStart. This means every delegate triggers events meant for user-facing sessions: banner scripts execute mid-conversation, session-tracking plugins log phantom sessions, and any plugin subscribing to lifecycle events receives signals that violate the hook contract.How this was discovered
While building goose-banner - a plugin that replaces the default "goose is ready" message with useful session context (active threads, token spend, project state) - the banner scripts were firing every time a delegate spun up. This surfaced the broader issue: the hook system's
SessionStartevent doesn't actually mean "a user started a session" - it fires for subagents too, which breaks the contract plugins rely on.Benefit
Hook contract correctness.
SessionStartfires exactly once per user-facing session. Subagents no longer trigger lifecycle events. Plugin authors can trust the signals they receive without building their own "am I a subagent?" guards.Ecosystem scalability. As the plugin ecosystem grows (permission gates, audit logging, compliance checks, session tracking), every new plugin multiplies the per-delegate overhead today. After this fix, the cost of adding plugins stays zero for subagents permanently.
No more mid-session noise. Banner output, session counters, and state initialization no longer fire during delegates. The terminal stays clean. Plugins that track session state don't log phantom sessions.
Foundation for plugin trust. Plugins like goose-banner and spore (retrieval tracking, interoception) rely on
SessionStartto initialize per-session state. Without this fix, subagent-triggeredSessionStartoverwrites the parent session's state mid-conversation.Fix (two mechanisms)
1. Skip hooks for subagents
Adds
is_subagent: booltoAgentConfig(defaultsfalse). Whentrue,Agent::with_configassigns an emptyHookManager::default()instead of callingHookManager::load(). Both delegate paths insummon.rs(sync and async) setis_subagent = true.2. Prevent double-emission in the parent
Adds an
AtomicBoolguard (session_start_emitted) toAgent. The CLI banner path sets it viaemit_hook_with_banners, andreply()uses an atomicswapto skip if already fired. Non-CLI paths (gateway, ACP server, scheduler) that only go throughreply()still emit exactly once via the same guard.What this does NOT affect
SessionEndhooks - were never fired for subagents even before this change.Performance (real but secondary)
Per-delegate overhead eliminated:
Savings are marginal today (~50s/week for a heavy user with 3 plugins), but compound as the plugin ecosystem grows.
Commits
emit_hook_with_bannersto the CLI path so plugins can surface context at session startup (the feature that exposed the bug)is_subagentflag onAgentConfig, emptyHookManagerfor delegatesAtomicBoolguard ensuresSessionStartfires at most once perAgentinstance regardless of code path