Skip to content

fix: suppress stale process watch notifications - #32242

Open
Redcircle031 wants to merge 1 commit into
NousResearch:mainfrom
Redcircle031:fix/suppress-stale-watch-notifications
Open

fix: suppress stale process watch notifications#32242
Redcircle031 wants to merge 1 commit into
NousResearch:mainfrom
Redcircle031:fix/suppress-stale-watch-notifications

Conversation

@Redcircle031

Copy link
Copy Markdown

Summary

  • Skip all queued notification types for consumed process sessions, not just completion events
  • Mark killed process sessions as consumed before moving them to finished
  • Add regression coverage for stale watch events and kill suppression

Test Plan

  • uv run --extra dev --extra pty pytest -q tests/tools/test_process_registry.py -q

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have tool/terminal Terminal execution and process management labels May 25, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the focused regression coverage. The stale-watch-event premise is still present on current main, but this patch needs updating for current delivery semantics.

Problems

  • tools/process_registry.py:890 must preserve main's completion-specific _drain_should_skip() path at tools/process_registry.py:1184; it carries the _poll_observed CLI dedup contract documented at tools/process_registry.py:1138-1149 and covered by tests/tools/test_notify_on_complete.py:381-405.
  • The TUI poller bypasses drain_notifications() and only suppresses consumed completion events at tui_gateway/server.py:8745 and :8810. Gateway watch delivery also takes all watch events at gateway/run.py:2569-2570 before injecting them at gateway/run.py:11873-11879. Those paths need the same consumed-session guard.
  • The new kill test assigns _is_host_pid_alive at tests/tools/test_process_registry.py:1040, but current main uses _host_pid_is_ours() at tools/process_registry.py:1485, so it does not control the current detached-host branch.

Suggested changes

  • Keep _drain_should_skip() for completions, add a separate watch-event consumed guard, update the TUI/gateway consumers, and test each path.
  • Rework the kill test around a mocked local session.process and _terminate_host_pid.

This is an automated hermes-sweeper review.

Comment thread tools/process_registry.py
break
_evt_sid = evt.get("session_id", "")
if evt.get("type") == "completion" and self.is_completion_consumed(_evt_sid):
if _evt_sid and self.is_completion_consumed(_evt_sid):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Current main uses _drain_should_skip() for completion events so _poll_observed still suppresses the CLI's duplicate post-poll completion. Preserve that completion branch and add a separate consumed-session check for watch events instead of replacing it with is_completion_consumed() for every event.

s.pid_scope = "host"
s.pid = 12345
registry._running[s.id] = s
registry._is_host_pid_alive = MagicMock(return_value=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This does not control current main's detached-host kill path: kill_process() calls _host_pid_is_ours() before _terminate_host_pid. Use a mocked local session.process plus _terminate_host_pid, or mock the current identity-check seam, so the test reliably exercises the killed path.

@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 13, 2026
@cirwel

cirwel commented Jul 31, 2026

Copy link
Copy Markdown

Independent confirmation from a current CLI maintenance run: stale completions from superseded watchers arrived after later authoritative results. This queue-consumption fix complements #41903, which removes process completion notices from synthetic user input. Suggested integration regression: consume or kill an older watched session, complete a newer session, drain notifications, and assert the old session emits neither a user-input message nor terminal output while the current completion is displayed once.

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

Labels

P3 Low — cosmetic, nice to have 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 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