Skip to content

fix(tui): preserve exact background notification owners - #62201

Open
kevinmanase wants to merge 1 commit into
NousResearch:mainfrom
kevinmanase:codex/finish-notification-session-ownership
Open

fix(tui): preserve exact background notification owners#62201
kevinmanase wants to merge 1 commit into
NousResearch:mainfrom
kevinmanase:codex/finish-notification-session-ownership

Conversation

@kevinmanase

Copy link
Copy Markdown

Summary

Background completion and watch notifications now stay bound to the exact live UI session that started them, so output from one chat cannot surface in another chat that shares the same durable conversation identity. The ownership model distinguishes concurrent runtime sessions, preserves the originating stream owner through completion, and keeps terminal/watch attribution scoped to that owner.

Validation

  • scripts/run_tests.sh tests/test_tui_gateway_server.py tests/tools/test_notify_on_complete.py tests/tools/test_process_registry.py tests/tools/test_terminal_task_cwd.py tests/tools/test_watch_patterns.py tests/tools/test_zombie_process_cleanup.py tests/tui_gateway/test_delegation_session_lifecycle.py — 549 passed, 0 failed.
  • Codex CLI review against main — no findings.
  • Greptile CLI could not dispatch because NousResearch/hermes-agent is not connected to Greptile.

Related

Related: #61719


Compound Engineering
GPT-5.5

@alt-glitch alt-glitch added type/bug Something isn't working comp/tui Terminal UI (ui-tui/ + tui_gateway/) comp/gateway Gateway runner, session dispatch, delivery tool/delegate Subagent delegation tool/terminal Terminal execution and process management P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 10, 2026
@alt-glitch

alt-glitch commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #61719 and this branch share the core UI-session ownership patch; #62201 adds exact live-owner/background-stream coverage. The branch is now substantially behind current main, which also includes merged ownership routing in #65419, so it needs a rebase/split to show its remaining delta before a maintainer chooses consolidation. Not a duplicate.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for covering the exact live-UI-owner path, including delegated child execution and checkpoint/event propagation.

Current main still has the reported defect: tui_gateway/server.py:8667-8687 has multiple session pollers dequeueing a process-global queue, tools/process_registry.py:1189-1202 applies the positive ownership callback only to async-delegation events, and tui_gateway/server.py:8834-8842 routes terminal streams to the first matching durable session key. The proposed ownership propagation and fail-closed routing directly address those paths, with regression coverage for completion, watch, stream, checkpoint, and executor-boundary cases.

This overlaps open PR #61719, as noted in the existing maintainer comment, so the two implementations should be compared and one selected rather than merged together.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 11, 2026
@kevinmanase

Copy link
Copy Markdown
Author

This overlaps open PR #61719, as noted in the existing maintainer comment, so the two implementations should be compared and one selected rather than merged together.

Thanks. I compared the histories directly. e3f398f90 in #62201 is patch-equivalent to #61719's 54dbd8bd5, so these are not competing implementations. #62201 then adds 96129c9c7 and ecad36e27, which model a unique live owner and preserve exact background stream ownership. Those follow-ups address #61719's open review gap for plain background live output.

I think #62201 is the branch to keep and #61719 can be treated as superseded.

@alt-glitch alt-glitch added comp/tools Tool registry, model_tools, toolsets comp/desktop Electron desktop app (apps/desktop/*) needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have P2 Medium — degraded but workaround exists tool/terminal Terminal execution and process management and removed tool/terminal Terminal execution and process management sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages P3 Low — cosmetic, nice to have labels Jul 19, 2026
@kevinmanase
kevinmanase force-pushed the codex/finish-notification-session-ownership branch from ecad36e to 8531159 Compare August 15, 2026 05:53
@alt-glitch alt-glitch removed comp/gateway Gateway runner, session dispatch, delivery tool/delegate Subagent delegation labels Aug 15, 2026
@alt-glitch alt-glitch removed the comp/desktop Electron desktop app (apps/desktop/*) label Aug 15, 2026
Background process output and completion notifications were routed by
durable session_key alone. Several live UI tabs can share one key, so the
gateway delivered them to whichever tab came first in iteration order --
a user could see output from a command they never ran, and the tab that
actually started it saw nothing.

Track the commissioning UI tab on the process itself:

- ProcessSession gains origin_ui_session_id, set at spawn from
  HERMES_UI_SESSION_ID for every background process (not just
  notify/watch ones -- live streaming needs the same return address).
- Completion and watch_match notifications carry it, and it survives a
  gateway restart through the checkpoint.
- _owner_sid_for_process prefers the recorded origin when that tab is
  still live, and only then falls back to session_key matching. The
  fallback now also skips finalized tabs, which would drop the output.

Processes with no recorded origin (older checkpoints, CLI sessions) keep
the previous key-matching behavior.

The ownership-routing and delegation-context portions of the original
branch landed upstream in NousResearch#65419, so this is the remaining delta rebuilt
on current main.
@kevinmanase
kevinmanase force-pushed the codex/finish-notification-session-ownership branch from 8531159 to 2904730 Compare August 15, 2026 06:06
@alt-glitch alt-glitch added comp/gateway Gateway runner, session dispatch, delivery tool/delegate Subagent delegation and removed needs-decision Awaiting maintainer decision before any implementation labels Aug 15, 2026
@kevinmanase

Copy link
Copy Markdown
Author

@teknium1 rebased on current main and resolved. The branch was ~7,500 commits behind, so this needed a rework rather than a straight rebase.

On the #61719 overlap you raised. Checking against current main, #65419 already landed the ownership routing, and main also solved the delegated-child ContextVar problem a different (better) way, with contextvars.copy_context() instead of the bind_ui_session_id helper I had. So I dropped that whole commit rather than re-litigate it, and kept only what main is still missing. That also settles the #61719 comparison: the part the two branches shared is upstream now, so there is nothing left to choose between.

The defect is still live on main. Two concrete gaps:

  • ProcessSession has no origin_ui_session_id field at all, so a background process never records which UI tab commissioned it. tools/terminal_tool.py never sets one either.
  • _owner_sid_for_process in tui_gateway/server.py returns the first session whose session_key matches. session_key is the durable conversation key and several live tabs can share one, so live agent.terminal.output goes to whichever tab comes first in dict iteration order.

Net effect: a user can see output from a command they never ran, while the tab that actually ran it shows nothing.

The fix. Track the commissioning tab on the process itself, carry it on completion and watch_match notifications, persist it through the checkpoint so it survives a gateway restart, and prefer it when routing. Two deliberate fallbacks so nothing regresses:

  • A process with no recorded origin (older checkpoints, CLI sessions) keeps the previous key matching.
  • If the origin tab is gone, routing falls through to a live tab on the same conversation. The key-match loop now skips finalized tabs, which would otherwise swallow the output.

Verification.

  • tests/test_tui_gateway_server.py 573 passed, tests/tui_gateway/ 455 passed, and the process_registry / notify_on_complete / watch_patterns / zombie_cleanup suites pass.
  • The 9 new tests all fail without the fix, so they cover the bug rather than passing vacuously.
  • Ruff clean.

One thing to note if you run the suites in bulk: tests/tools/ tests/tui_gateway/ in a single pytest process has a large number of order-dependent failures on main already. I measured it both ways, 113 failed on clean main and 114 on this branch out of ~6,900. Run per-directory they are green. The residual tests/tools/ failures are confined to test_voice_mode.py, test_wake_word.py and test_web_tools_config.py, and I confirmed those fail identically with my changes reverted (7 failed / 96 passed either way). They look like missing optional deps in my environment, not anything from this PR.

Ready for review.

@alt-glitch alt-glitch added area/sessions Session lifecycle, resume, persistence, history needs-decision Awaiting maintainer decision before any implementation and removed comp/gateway Gateway runner, session dispatch, delivery labels Aug 15, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #61719 and #73351 address the same cross-session background-output family through overlapping but distinct ownership paths. This PR uses the exact originating live UI session; a maintainer should choose the preferred implementation.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(tui): preserve exact background notification owners

  1. Fallback destination is iteration-order dependenttui_gateway/server.py _owner_sid_for_process: when the origin tab is finalized/gone and several live tabs share the session_key, the fallback picks whichever live tab comes first in _sessions iteration order. The tests pin two-tab cases, but with 3+ live tabs on one conversation the destination is effectively arbitrary. Recording the most-recently-active tab (a last-activity timestamp on the session dict) would make the fallback deterministic.
  2. Broad except on the session-context readtools/terminal_tool.py: get_session_env("HERMES_UI_SESSION_ID", "") is wrapped in a bare except Exception: pass. A stale or recycled HERMES_UI_SESSION_ID (e.g. inherited through delegation from a tab that has since closed) silently routes output to a different live tab via the fallback. Consider validating the sid against known live sessions before pinning it, or at least logging when the inherited value does not resolve.
  3. Forward compatibility of the new event keytools/process_registry.py completion/watch events now carry origin_ui_session_id; verify all consumers of the completion queue (gateway watchers, CLI, dashboard) tolerate the extra key rather than strict-schema parsing it away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/tools Tool registry, model_tools, toolsets comp/tui Terminal UI (ui-tui/ + tui_gateway/) needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/delegate Subagent delegation 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.

4 participants