Skip to content

fix(gateway): route plain background live output to its spawn-time UI owner (#61719 residual) - #73351

Open
ayushnangia wants to merge 2 commits into
NousResearch:mainfrom
ayushnangia:fix/bg-output-ui-owner
Open

fix(gateway): route plain background live output to its spawn-time UI owner (#61719 residual)#73351
ayushnangia wants to merge 2 commits into
NousResearch:mainfrom
ayushnangia:fix/bg-output-ui-owner

Conversation

@ayushnangia

Copy link
Copy Markdown
Contributor

Summary

Live agent.terminal.output chunks from a plain background=true process (no notify_on_complete, no watch_patterns) — a delegated child's, in particular — are dropped by the desktop router: the process has no positive UI owner. This completes the remaining scope of #61719 after its core landed.

Root cause

Owner-routing for post-turn completions landed in merged 54d0948d3 ("route post-turn completions by owner"), and drain_notifications now requires positive ownership for routed events. But the sweeper's flagged problem on #61719 survived the merge:

  • tools/terminal_tool.py captured routing metadata only inside the background and (notify_on_complete or watch_patterns) gate — while live output is emitted for every background process (process_registry.on_output).
  • _owner_sid_for_process() (tui_gateway/server.py, _wire_agent_terminal_output) matched only by session_key equality against live TUI sessions. A delegated child's ProcessSession.session_key is the subagent's internal key, which never matches a window — so its chunks were emitted with sid="", and a session-less event from a registry reader thread is dropped by write_json (documented at the _live_transports registry).

Changes

  • tools/process_registry.py: ProcessSession.origin_ui_session_id — spawn-time UI owner, persisted/restored in the crash checkpoint alongside the existing watcher metadata.
  • tools/terminal_tool.py: capture HERMES_UI_SESSION_ID for every background spawn, outside the notify/watch gate — same session-env pattern the codebase already uses for UI routing (tools/desktop_ui.py:39, tools/delegate_tool.py:3191). Empty outside TUI/desktop contexts; best-effort with fallback.
  • tui_gateway/server.py: _owner_sid_for_process() prefers the recorded origin while that window is still live, then falls back to the legacy session_key match — so existing routing behavior is byte-identical for processes without a recorded origin or after the owning window closes.

Validation

case before after
delegated child, plain background=true live output sid="" → dropped routed to spawn-time owner window
plain background, no recorded origin (non-desktop spawn) session_key match unchanged
recorded origin but window closed, unknown key falls back to "" (no leak to other windows)
  • 2 new tests red on unfixed main, green with fix: test_background_spawn_captures_ui_origin_without_notify (capture without notify/watch) and test_agent_terminal_output_routes_by_spawn_time_ui_owner (child-shape routing + fallback + stale-origin cases)
  • scripts/run_tests.sh tests/tools/test_notify_on_complete.py tests/test_tui_gateway_server.py tests/tools/test_process_registry.py — 636 passed, 0 failed

Scope notes

  • Completion/watch notifications are untouched — they already carry watcher metadata and flow through the owner-routing from 54d0948d3.
  • A checkpoint-recovered process keeps its recorded origin; if that window is gone after restart the sid simply no longer matches a live session and routing falls back — no stale delivery.

Salvages the remaining scope of #61719 by @soria-clawd-bot (sweeper review problem: UI origin captured only when notify/watch enabled). Refs #54785, 54d0948d3.

@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets comp/tui Terminal UI (ui-tui/ + tui_gateway/) comp/desktop Electron desktop app (apps/desktop/*) tool/terminal Terminal execution and process management sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state P2 Medium — degraded but workaround exists labels Jul 28, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #62201 has the broader owner-routing implementation across notifications and delegation. This PR isolates the remaining plain-background live-output path.

@ayushnangia

Copy link
Copy Markdown
Contributor Author

Correct framing, thanks — mapping the relationship precisely for whoever picks between them:

  • fix(tui): preserve exact background notification owners #62201 (last commit Jul 10) is the broad implementation, but it predates merged 54d0948d3, which already landed the completion/notification owner-routing core on main — a large share of its 12-file diff now overlaps what's merged.
  • This PR is the residual only, rebuilt against current main: spawn-time UI-owner capture for every background spawn + _owner_sid_for_process preference, 144 added lines, no overlap with the merged routing.

If maintainers prefer salvaging #62201's broader scope instead, the two red-on-main tests here (plain-background capture without notify/watch; delegated-child-shape routing with live-window guard) transfer directly as its regression matrix.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the remaining live-output routing path. The underlying routing gap is present on current main: tui_gateway/server.py:8850-8858 can only map a process by session_key.

Problems

  • The proposed assignment is too late for a reliable fix. tools/process_registry.py:798-807 starts the local reader before spawn_local() returns, and that reader calls _emit_output() at tools/process_registry.py:964-974. The PR assigns origin_ui_session_id only after the spawn call returns, so immediate process output can still be emitted without a UI owner.
  • The new capture test stubs spawn_local() with a SimpleNamespace, so it cannot cover that startup ordering.

Suggested changes

  • Capture the UI sid before spawning and pass it into spawn_local() / spawn_via_env() so ProcessSession has the owner before any reader or poller starts.
  • Add an immediate-output regression case using the real registry lifecycle.

Automated hermes-sweeper review.

@ayushnangia
ayushnangia force-pushed the fix/bg-output-ui-owner branch from c98b378 to 574911b Compare July 30, 2026 16:04
@ayushnangia

Copy link
Copy Markdown
Contributor Author

Both points addressed in 574911b61 (rebased onto current main):

  • Startup race: spawn_local() / spawn_via_env() now accept origin_ui_session_id and set it on ProcessSession at construction — before the reader thread (started inside spawn_local()) or the sandbox log poller exists. terminal_tool captures HERMES_UI_SESSION_ID before the spawn call and passes it in; the post-spawn assignment is gone.
  • Real-lifecycle coverage: test_immediate_output_carries_ui_owner_through_real_registry spawns echo through the real registry with an on_output recorder that captures the session's owner at emission time — the first chunk must already carry it. The stubbed capture test now asserts the kwarg is handed to the spawn (fails on the post-spawn variant).

scripts/run_tests.sh across notify_on_complete, tui_gateway_server, process_registry, terminal_task_cwd, watch_patterns: 587 passed, 0 failed. One pre-existing exact-kwargs assertion in test_terminal_task_cwd was extended with the new parameter.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
… owner (NousResearch#61719 residual)

Merged 54d0948 landed owner-routing for post-turn completions, but the
sweeper's flagged gap on NousResearch#61719 survived: terminal_tool captured routing
metadata only when notify_on_complete/watch_patterns were set, while live
agent.terminal.output chunks are emitted for EVERY background process.
_owner_sid_for_process matched only by session_key, so a delegated child's
process (subagent-internal key, never a live TUI session) emitted its live
output with sid "" — dropped by write_json.

- ProcessSession.origin_ui_session_id: spawn-time UI owner, persisted in
  the crash checkpoint like the watcher metadata
- terminal_tool: capture HERMES_UI_SESSION_ID for every background spawn
  (same session-env pattern as desktop_ui.py:39 / delegate_tool.py:3191)
- _owner_sid_for_process: prefer the recorded origin while that window is
  live; legacy session_key equality remains the fallback

Salvages the remaining scope of NousResearch#61719 (bot-authored, sweeper problem
unaddressed).
… reader starts

Sweeper review: the post-spawn assignment races the local reader thread —
spawn_local() starts it before returning, and _emit_output() can forward
the first chunks while origin_ui_session_id is still empty.

- spawn_local()/spawn_via_env() accept origin_ui_session_id and set it on
  ProcessSession at construction, before any reader/poller exists
- terminal_tool captures HERMES_UI_SESSION_ID before the spawn call and
  passes it in; the post-spawn assignment is removed
- capture test now asserts the kwarg is handed to the spawn (red on the
  post-spawn variant); new real-registry-lifecycle regression proves the
  first emitted chunk of a fast process already carries the owner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants