fix(gateway): SSE delivery for background process notifications + subagent protection - #35553
fix(gateway): SSE delivery for background process notifications + subagent protection#35553someaka wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Review Findings
This is a substantial gateway improvement PR with two major features. The diff is large (489 additions, 202 deletions) but limited to gateway/run.py, and the code quality is high throughout.
✅ Looks Good
SSE Delivery:
- Background process notifications pushed directly to api_server SSE streams — bypasses
adapter.send()/handle_message()which cannot handle HTTP/SSE delivery. This is architecturally correct. - The
_push_fnpattern is clean — the api_server adapter exposes apush_process_eventmethod, and the watcher calls it whenplatform_name == "api_server".
Subagent Protection:
_agent_has_active_subagents()correctly checksAIAgent._active_childrenwith proper guards forNone, sentinel values, and non-collection types (MagicMock protection).- Demotes
interrupttoqueuewhen subagents are in flight — preventsAIAgent.interrupt()from cascading through children and aborting delegate_task work (#30170). - Cleaner code organization compared to the removed scattered checks — the logic is now consolidated in
_handle_active_session_busy_message. /stopand/newslash commands bypass the demotion, preserving an explicit escape hatch.- Busy ack messages updated to clearly explain the subagent protection state.
Other:
- Compacting context regex updated for cleaner summaries.
- Loop command handling restored in gateway mode with proper
/loopdispatch. - Mattermost adapter registered in
_create_adapter(). /starthandling cleaned up — moved to the main command dispatch flow rather than being checked in multiple places.- Various small refactors (busy_ack_detail display config, MCP reload agent cache logic).
No Issues Found
Reviewed by Hermes Agent
53ab2ca to
7d1e7c3
Compare
83cb260 to
7e1c58a
Compare
…agent protection - Deliver background process notifications to WebUI via api_server SSE stream - Add _agent_has_active_subagents() guard to prevent premature session cleanup - Compacting context regex updated for cleaner summaries - Loop command handling restored in gateway mode Fixes NousResearch#28547 (subagent protection), NousResearch#15248 (WebUI notification delivery)
7e1c58a to
a50d54a
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tackling both notification delivery and the subagent interruption case.
Problems
- The subagent-protection portion is already on current main in
99d62f6ba(gateway/run.py:5145,:5436,:9575) with regression coverage intests/gateway/test_subagent_protection_30170.py:209. - The proposed SSE call at
gateway/run.py:11892targetspush_process_event, but currentgateway/platforms/api_server.py:836-844explicitly has no persistent async-delivery channel, and the API run queues are keyed byrun_id(:881-886) and removed when their SSE stream ends (:4613-4616). The PR supplies only a session key, so it cannot identify a surviving stream. - The MCP reload hunk refreshes
agent.toolsfor cached agents (gateway/run.py:10580in this diff), which changes tool schemas on live conversations and conflicts with the prompt-cache invariant inAGENTS.md.
Suggested changes
- Salvage only the still-needed WebUI delivery work after designing a persistent, session-addressable transport and adding an end-to-end completion test.
- Leave the already-shipped #30170 protection and avoid live cached-agent toolset mutation.
Automated hermes-sweeper review.
| # do not work for HTTP/SSE delivery. Push directly. | ||
| if platform_name == "api_server": | ||
| _api_adapter = self.adapters.get(Platform.API_SERVER) | ||
| _push_fn = getattr(_api_adapter, "push_process_event", None) |
There was a problem hiding this comment.
push_process_event is not implemented by the current api_server adapter, and this PR changes no adapter file. More fundamentally, its SSE queues are keyed by run_id and removed when the originating stream ends, so this completion event needs a persistent session-addressable transport rather than a best-effort dynamic method lookup.
| @@ -10457,6 +10580,25 @@ async def _execute_mcp_reload(self, event: MessageEvent) -> str: | |||
| except Exception: | |||
There was a problem hiding this comment.
Refreshing agent.tools on cached live agents swaps the tool schema mid-conversation. That invalidates the cached prompt prefix; keep MCP discovery deferred to a new session or use the cache-aware invalidation path instead.
|
Thanks @someaka — reviewed against current origin/main. Both headline halves have since landed in fuller forms: subagent protection is #30170 (_agent_has_active_subagents + busy/PRIORITY-path demotion), and api_server delivery for process/watch notifications flows through the deliver_wake self-post fallback in _inject_watch_notification. Because the branch is diffed against May-era main it would also revert several later fixes (#32790 auth logging, display-setting gating, compaction regex narrowing). Closing as superseded; if you still want the /loop gateway command from this branch, please open it as a fresh focused PR. |
What
Gateway improvements for notification delivery and session safety.
SSE delivery:
Subagent protection:
_agent_has_active_subagents()checksAIAgent._active_childrenAIAgent.interrupt()from cascading and killing every child ([Bug]: Sending a message while delegate_task is running kills the subagent — interrupt propagates unconditionally to children #30170)Other:
Why
Fixes #28547 (subagent protection) and #15248 (WebUI notification delivery).
Without SSE delivery, WebUI users never see background process completions. Without subagent protection, a conversational follow-up can destroy minutes of in-flight subagent work.