Skip to content

fix(gateway): honor background_process_notifications=off for watch reinjection - #9341

Closed
Lidang-Jiang wants to merge 1 commit into
NousResearch:mainfrom
Lidang-Jiang:fix/9290-bg-notify-off
Closed

fix(gateway): honor background_process_notifications=off for watch reinjection#9341
Lidang-Jiang wants to merge 1 commit into
NousResearch:mainfrom
Lidang-Jiang:fix/9290-bg-notify-off

Conversation

@Lidang-Jiang

@Lidang-Jiang Lidang-Jiang commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Makes display.background_process_notifications: off disable watch-pattern synthetic reinjection while still draining queued watch events. Non-off modes continue to inject notifications using the queued event's originating thread/user context, and async_delegation events remain available to their dedicated watcher.

Related Issue

Fixes #9290

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • drain gateway watch events at the end of every agent turn, regardless of notification mode
  • discard drained watch/process events without synthetic reinjection when the mode is off
  • pass the queued evt to _inject_watch_notification() in other modes so delivery preserves the originating process context
  • requeue async_delegation events for the dedicated async-delegation watcher
  • add regressions for off, all, origin routing, queue draining, and delegation retention

How to Test

python -m pytest -q \
  tests/gateway/test_background_process_notifications.py \
  tests/tools/test_watch_patterns.py \
  tests/scripts/test_release_acp_registry.py
ruff check .
python scripts/check-windows-footguns.py --all

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Ubuntu 24.04

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Before/After

Before

On the pre-fix implementation, the complete queue-drain harness output was:

manual_inject_handle_message_calls= 1
queue_drain_handle_message_calls= 2
queue_empty= True

Even with notifications set to off, queued watch events were synthetically reinjected.

After

With this commit, the same complete harness output is:

queue_drain_handle_message_calls= 0
queue_empty= True

Complete focused regression-suite output:

.................................................................        [100%]
65 passed in 1.33s

Additional Verification

  • scripts/run_tests.sh -j 16: 40,267 tests passed. Fifteen unrelated failures and three process-exit timeouts remained; the recurring functional failures were reproduced on the unmodified upstream/main baseline, and the one branch-only file-state failure passed on an isolated rerun.
  • ruff check .: passed (apart from the repository's pre-existing invalid-noqa warnings).
  • git diff --check: passed.
  • python scripts/check-windows-footguns.py --all: passed across 757 Python files.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery tool/terminal Terminal execution and process management labels Apr 27, 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 fix. The notification-off behavior is still missing on current main: gateway/run.py:11753-11758 drains watch events and reinjects every formatted event without checking the mode.

Problems

  • The submitted helper passes original_event to _inject_watch_notification (PR diff gateway/run.py:7179). Current main changed that method to consume the queued event dictionary (gateway/run.py:15309) so routing comes from the process origin. The existing regression at tests/gateway/test_background_process_notifications.py:450-481 verifies that foreground-event routing must not be used.

Suggested changes

  • Port the off gate into the current drain at gateway/run.py:11753, while retaining _drain_gateway_watch_events() for all modes and passing evt to _inject_watch_notification for non-off modes.
  • Update the test fixture to use the current queued-event routing contract and cover both queue draining under off and retained injection under all.

Automated hermes-sweeper review.

Comment thread gateway/run.py Outdated
if notify_mode == "off":
continue
synth_text = _format_gateway_process_notification(evt)
if synth_text:

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's _inject_watch_notification now expects the queued event dictionary so it can route to the process origin (gateway/run.py:15309); preserve evt here rather than passing the foreground event. The current regression at tests/gateway/test_background_process_notifications.py:450 protects against cross-thread foreground routing.

@Lidang-Jiang Lidang-Jiang Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in a220f04. I rebuilt the branch on current main, retained _drain_gateway_watch_events() for every notification mode, skipped synthetic reinjection only when the mode is off, and now pass the queued evt to _inject_watch_notification() so routing continues to use the originating process context. The updated tests cover queue draining under off, retained injection under all, origin routing, and async-delegation retention.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state 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 12, 2026
@Lidang-Jiang
Lidang-Jiang force-pushed the fix/9290-bg-notify-off branch from a678bd6 to 8e41a6a Compare July 13, 2026 03:34
Signed-off-by: Lidang-Jiang <lidangjiang@gmail.com>
@teknium1

Copy link
Copy Markdown
Contributor

Thanks @Lidang-Jiang — your fix landed in PR #85923 (rebase-merged, your commit and authorship preserved via cherry-pick). The off-mode gate now covers the post-turn watch drain exactly as you wrote it, rebased onto the current drain site. Closing this original; #9290 is fixed by your work.

@teknium1 teknium1 closed this Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery 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-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.

Bug: background_process_notifications=off still injects synthetic watch-pattern SYSTEM messages into conversations

3 participants