Skip to content

fix(desktop): isolate process notifications by session - #64043

Closed
xscale-team wants to merge 4 commits into
NousResearch:mainfrom
xscale-team:fix/session-scoped-process-notifications
Closed

fix(desktop): isolate process notifications by session#64043
xscale-team wants to merge 4 commits into
NousResearch:mainfrom
xscale-team:fix/session-scoped-process-notifications

Conversation

@xscale-team

@xscale-team xscale-team commented Jul 14, 2026

Copy link
Copy Markdown

Summary

  • require a non-empty durable owner for every agent-created terminal background process and detached subagent batch
  • prefer the gateway conversation key, then fall back to the dispatcher's explicit AIAgent.session_id when worker context is unavailable
  • persist the owner and notification mode in process checkpoints; quarantine legacy ownerless recovery records
  • register and arm process notifications before reader/poller threads start, closing the zero-duration completion race
  • exact-match all queued notification events at delivery, not only async-delegation completions
  • preserve compression-chain-aware routing for legitimate continuations

Root cause

ProcessRegistry.drain_notifications(session_key=..., owns_event=...) previously applied ownership checks only to async-delegation events. Ordinary terminal completions share the same process-global queue and also become synthetic user turns, so another desktop chat could consume a stale completion first.

A second creation-side gap made the issue possible: terminal_tool() received an explicit session_id from tool dispatch but ignored it when worker-thread context and task_id were empty. That produced ownerless processes. Notification flags were also armed after process start, allowing very short commands to finish before registration or notification setup.

This PR now enforces ownership at both boundaries:

  1. creation rejects ownerless terminal and delegation work;
  2. the durable owner and notification mode are stored before execution can complete;
  3. restart recovery restores owned records and quarantines ownerless legacy records;
  4. delivery requires positive ownership, including continuation-lineage resolution.

Losing an invalid legacy notification is safer than guessing an owner and contaminating another session. Legitimate terminal output remains available through the process registry, and delegation records remain queryable.

Incident evidence

At 2026-07-13 18:43, the desktop transport emitted terminal output for proc_af1041e85102 with an empty session ID. Its completion was persisted as a synthetic user turn in session 20260713_162533_199471, which had no matching process-launch record. That unrelated Scribe chat consequently began responding to a Fleet Model Router audit.

Verification

  • python -m pytest -q tests/tools/test_process_registry.py -> 108 passed
  • python -m pytest -q tests/tools/test_async_delegation.py tests/tools/test_delegate.py -> 186 passed
  • terminal ownership/notification/dispatch suites -> 82 passed
  • focused desktop notification routing -> 8 passed
  • full tests/test_tui_gateway_server.py -> 319 passed, 3 unrelated existing failures around stale tui versus desktop expectations
  • Ruff, py_compile, and git diff --check passed

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment

30 PRs reviewed (batch: 64044-63999). See aggregate summary at PR 64044.


Reviewed by Hermes Agent

@alt-glitch alt-glitch added type/bug Something isn't working comp/tui Terminal UI (ui-tui/ + tui_gateway/) comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 14, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: this is part of the ownership-filter-on-drain cluster with #63317, #61719, #54785 (and issue #62548). All isolate cross-session background-process completion notifications; #63317 in particular extends the ownership filter to every notification type (the same superset mechanism as this PR). Cross-linking for maintainer to pick the canonical landing.

@xscale-team

Copy link
Copy Markdown
Author

Live reproduction after the patch was committed but before restarting the desktop backend: a foreign terminal completion (proc_e9f0e5521efb, model-router security review, exit 1) was injected into this unrelated context-repair chat as a synthetic user turn, followed by that foreign turn's memory context. This confirms the affected runtime was still executing the old process-global notification routing and demonstrates the exact cross-session context contamination fixed by this PR. The foreign review output and its embedded question were ignored.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment

This PR isolates process notifications by session in desktop. Small, targeted fix.

Please verify:

  • Notifications are correctly scoped to their session
  • No cross-session notification leakage

Reviewed by Hermes Agent

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment

Changes

test_notification_poller regression: completion event now carries session_key for matching instead of relying on ownerless notification events being adopted by whichever poller wakes first.

Assessment

  • Correctness fix: ownerless notification events now fail closed instead of being silently adopted by the wrong poller. More robust test isolation.

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for addressing a real cross-session notification leak. Current main still limits drain_notifications() ownership checks to async_delegation events (tools/process_registry.py:1197-1214), and the TUI admits an ownerless non-delegation event because _notification_event_belongs_elsewhere() returns false for an empty key (tui_gateway/server.py:8584-8586) while the positive gate is async-only (tui_gateway/server.py:8763-8776).

Problems

  • In the new tools/process_registry.py spawn paths, the reader/poller is started before _write_checkpoint(). Registration and in-memory notification flags are ready first, but a fast child can finish before owner/notification metadata is durable. Move checkpointing before reader/poller startup in both helpers.

Suggested changes

  • Add regression coverage that proves a newly spawned, notification-enabled session is checkpointed before its reader/poller can finalize it.
  • Salvage carefully onto current main: 8ff3b67e6 already replaced the PR's older cli.py drain hunks with _drain_process_notifications, and 47d853fdf added restored-completion ownership handling in the overlapping registry code.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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 16, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing as superseded in substance. The two real leak paths this targeted are now fixed on main: ordinary addressed completions route by positive ownership at both the poller and the post-turn drain (#65419), and durable-restored/legacy events fail closed on unfiltered drains (#64593). Creation-side stamping also exists — terminal_tool persists session_key on background dispatch.

What we're NOT taking is the hard-reject of ownerless work at creation: internal events (watch_overflow_*, etc.) are deliberately ownerless, and delivery handles ownerless-legacy events by design (global delivery in single-session contexts). Rejecting them at the boundary is a policy change that fights that design and would break single-session CLI flows where ownerless is normal.

One idea here may still have legs: the zero-duration completion race (notification flags armed after process start, so very short commands can finish before registration). If you can demonstrate that on current main, a small focused PR for just that is welcome. Thanks @xscale-team — thorough work, most of it just landed via parallel routes.

@teknium1 teknium1 closed this Jul 16, 2026
@teknium1 teknium1 added the area/sessions Session lifecycle, resume, persistence, history label Jul 19, 2026
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/) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants