fix(tui): isolate background notifications by UI session - #61719
fix(tui): isolate background notifications by UI session#61719soria-clawd-bot wants to merge 1 commit into
Conversation
d4c33b6 to
54dbd8b
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the child-executor and process-queue paths. The core premise is confirmed on current origin/main: tools/process_registry.py:1085-1093 emits non-delegation completion events with only session_key, while tui_gateway/server.py:8677-8729 permits orphaned completion/watch events through after applying its fail-closed gate only to async_delegation.
Problems
tools/terminal_tool.py:2365capturesorigin_ui_session_idonly whennotify_on_completeorwatch_patternsis enabled. Live process output is emitted for every background process (tools/process_registry.py:217-225,951-959on current main), while this PR's terminal owner routing requires positive ownership. A delegated plainbackground=trueprocess will therefore have no exact UI owner and itsagent.terminal.outputchunks will be dropped.
Suggested changes
- Capture the UI origin for every background spawn and add a delegated-child test for plain background live-output routing.
Automated hermes-sweeper review.
| # Delegated children replace session_key with their temporary child | ||
| # session, but inherit HERMES_UI_SESSION_ID from the parent chat. | ||
| origin_ui_session_id = "" | ||
| if notify_on_complete or watch_patterns: |
There was a problem hiding this comment.
This condition leaves origin_ui_session_id empty for plain background=true processes. ProcessRegistry streams every output chunk, and the new positive-ownership terminal router will drop a delegated child's stream without an exact origin. Capture the UI origin for all background spawns and add the corresponding regression test.
|
Status map for maintainers: the ownership-routing core of this PR landed on main via merged |
… 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).
What does this PR do?
Prevents Desktop/WebUI background-process completion and watch notifications from contaminating unrelated conversations.
A delegated child can start
terminal(background=true, notify_on_complete=true)and exit before the process finishes. The process keeps the child executionsession_key, but that child is no longer a live UI session. The TUI/WebUI server historically adopted the orphaned event into whichever session poller dequeued the process-global queue first.This patch keeps execution isolation and return routing separate:
session_keyfor process/tool isolation;origin_ui_session_id;This supersedes the direct-TUI portions of #35672 and #54785: both route by
session_keybut do not preserve the exact UI origin across delegated executors, persist that return address, or gate every post-turn event type. It is complementary to #16455, which guards stale physical sessions in the messaging gateway path.Related Issue
Fixes #42674
Related: #35652, #16455, #35672, #54785
Type of Change
Changes Made
tools/process_registry.py: store/checkpointorigin_ui_session_id, include it on completion/watch events, apply the ownership callback to every post-turn event type, and normalize malformed/future event types without crashing.tools/terminal_tool.py: capture the live UI origin when starting notified background processes.gateway/session_context.py+tools/delegate_tool.py: bind that origin across both delegated-child executor boundaries without replacing the child's isolationsession_key.tui_gateway/server.py: route by exact origin when present; fail closed for ownerless completion/watch events, including global overflow summaries.How to Test
Local verification on Ubuntu/Linux:
doctortimeout reproduce on cleanorigin/main; the web-server timing exception passed 3/3 when rerun alone after the saturated 32-way run.compileall, a diff secret/injection heuristic scan, andgit diff --checkpassed.Checklist
Code
pytest tests/ -qand all tests passDocumentation & Housekeeping
cli-config.yaml.example: N/A; no config keys changedCONTRIBUTING.md/AGENTS.md: N/A; no workflow changeContextVarand dataclass fields onlyScreenshots / Logs
The outer/inner executor regression fails without the binding (
worker_origin == "") and passes with the patch ("parent-tab"), with the resulting process completion event carryingorigin_ui_session_id="parent-tab".The two-session overflow regression fails without the expanded fail-closed set (the first poller starts an unrelated agent turn) and passes with the patch (no emit, no turn, queue drained).