fix(cli): honor display.background_process_notifications=off in the process drain - #123123
Closed
liuhao1024 wants to merge 1 commit into
Closed
liuhao1024 wants to merge 1 commit into
liuhao1024 wants to merge 1 commit into
Conversation
…rocess drain The documented `off` mode gated only the gateway's completion injection (NousResearch#9290); the CLI drain never consulted the key, so setting it silently did nothing on the CLI — leaving no escape hatch against the post-Ctrl+C notification turn restarts (NousResearch#123114). Mirror the gateway semantics: the drain still claims and acknowledges completion events (durable rows converge instead of replaying on restart), but the turn-starting `_pending_input` injection is suppressed. Fixes NousResearch#123114
alt-glitch
added
type/bug
arkheioncorp
left a comment
arkheioncorp
left a comment
There was a problem hiding this comment.
Hermes Agent Review — COMMENT
Verdict: No blocking issues.
✅ Looks Good
- hermes_cli/cli_process_notifications.py — reads and returns only for . The gate in skips the turn-starting injection but still drains, claims, and acknowledges events — matching the documented semantics (#9290).
- tests/ — Parametrized over / / : → no injection; others → injection. All modes assert is non-empty, confirming the claim/ack chain always runs.
💡 Suggestions
- None. The in is appropriate for defensive config reading.
No security concerns, no logic errors.
arkheioncorp
left a comment
arkheioncorp
left a comment
There was a problem hiding this comment.
Hermes Agent Review — COMMENT
Verdict: No blocking issues.
Looks Good
- hermes_cli/cli_process_notifications.py — _background_notifications_suppressed reads CLI_CONFIG.display.background_process_notifications and returns True only for off. The gate in _drain_process_notifications skips the turn-starting injection but still drains, claims, and acknowledges events — matching the documented semantics (#9290).
- tests/ — Parametrized over off / concise / all: off → no injection; others → injection. All modes assert acked is non-empty, confirming the claim/ack chain always runs.
Suggestions
- None. The try/except Exception in _background_notifications_suppressed is appropriate for defensive config reading.
No security concerns, no logic errors.
Author
|
Thanks for the review — good to have the drain/claim/ack chain and the |
OutThisLife
pushed a commit
that referenced
this pull request
Sep 26, 2026
… as a user bubble
A `terminal(background=true, heartbeat=N)` tick queued a notification every N seconds
whether or not the process had printed anything, and every queued event costs the owning
session a full model turn. On Desktop and the TUI that turn painted the wake as a user
bubble ("[Background process ... heartbeat #9 ... (no new output since the last
heartbeat)]") followed by the model's "Still running normally." — over and over, for a
process whose row on the status stack already said it was running — and while the wake
held the session's turn, the user's own prompt sat queued behind it.
- `ProcessRegistry._emit_heartbeat` skips a tick with no new output. The sequence counts
delivered beats only; the "(no new output)" placeholder in the formatter is gone.
- TUI/Desktop type heartbeat rows `display_kind: hidden` (the kind both clients and the
transcript preview already honour); the CLI paints a one-line receipt and persists the
row hidden, so reopening the session in Desktop shows only the agent's reply.
- Desktop hydration drops heartbeat rows persisted by older backends the same way.
- `display.background_process_notifications: off` is honored by the TUI/Desktop poller and
the CLI drain, not just the messaging gateway. `off` mutes process-driven wakes only:
a finished `delegate_task(background=true)` still lands.
Supersedes #123123 (cherry-picked; scoped so `off` keeps subagent results) and #119202
(cherry-picked; `heartbeat: 0` is schema-valid so models that materialize every field
stop tripping the foreground guard).
OutThisLife
added a commit
that referenced
this pull request
Sep 27, 2026
… as a user bubble
A `terminal(background=true, heartbeat=N)` tick queued a notification every N seconds
whether or not the process had printed anything, and every queued event costs the owning
session a full model turn. On Desktop and the TUI that turn painted the wake as a user
bubble ("[Background process ... heartbeat #9 ... (no new output since the last
heartbeat)]") followed by the model's "Still running normally." — over and over, for a
process whose row on the status stack already said it was running — and while the wake
held the session's turn, the user's own prompt sat queued behind it.
- `ProcessRegistry._emit_heartbeat` skips a tick with no new output. The sequence counts
delivered beats only; the "(no new output)" placeholder in the formatter is gone.
- TUI/Desktop type heartbeat rows `display_kind: hidden` (the kind both clients and the
transcript preview already honour); the CLI paints a one-line receipt and persists the
row hidden, so reopening the session in Desktop shows only the agent's reply.
- Desktop hydration drops heartbeat rows persisted by older backends the same way.
- `display.background_process_notifications: off` is honored by the TUI/Desktop poller and
the CLI drain, not just the messaging gateway. `off` mutes process-driven wakes only:
a finished `delegate_task(background=true)` still lands.
Supersedes #123123 (cherry-picked; scoped so `off` keeps subagent results) and #119202
(cherry-picked; `heartbeat: 0` is schema-valid so models that materialize every field
stop tripping the foreground guard).
OutThisLife
added a commit
that referenced
this pull request
Sep 27, 2026
… as a user bubble
A `terminal(background=true, heartbeat=N)` tick queued a notification every N seconds
whether or not the process had printed anything, and every queued event costs the owning
session a full model turn. On Desktop and the TUI that turn painted the wake as a user
bubble ("[Background process ... heartbeat #9 ... (no new output since the last
heartbeat)]") followed by the model's "Still running normally." — over and over, for a
process whose row on the status stack already said it was running — and while the wake
held the session's turn, the user's own prompt sat queued behind it.
- `ProcessRegistry._emit_heartbeat` skips a tick with no new output. The sequence counts
delivered beats only; the "(no new output)" placeholder in the formatter is gone.
- TUI/Desktop type heartbeat rows `display_kind: hidden` (the kind both clients and the
transcript preview already honour); the CLI paints a one-line receipt and persists the
row hidden, so reopening the session in Desktop shows only the agent's reply.
- Desktop hydration drops heartbeat rows persisted by older backends the same way.
- `display.background_process_notifications: off` is honored by the TUI/Desktop poller and
the CLI drain, not just the messaging gateway. `off` mutes process-driven wakes only:
a finished `delegate_task(background=true)` still lands.
Supersedes #123123 (cherry-picked; scoped so `off` keeps subagent results) and #119202
(cherry-picked; `heartbeat: 0` is schema-valid so models that materialize every field
stop tripping the foreground guard).
Author
|
Closing — the fix landed on main as ede966b ("fix(cli): honor display.background_process_notifications=off in the process drain", carried with this PR's authorship), which carries the exact source and test changes from this branch. Nothing further to carry here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
display.background_process_notifications: offwas only ever consulted by the gateway's completion injection (gateway/run_config_loaders.py); the CLI's_drain_process_notificationsnever read the key, so the documentedoffmode silently did nothing on the CLI. That left users hitting #123114 (a finished-but-unread background process restarts a new turn right after everyCtrl+C) with no configuration escape hatch at all — turning the key "off" changed nothing.This PR makes the CLI drain honor
off. It mirrors the gateway's semantics (gateway/run_notifications.py::_drain_watch_notifications): completion events are still drained, claimed and acknowledged — so durableasync_delegationsrows converge todeliveredinstead of replaying on the next restart — but the turn-starting_pending_input.put(...)injection is suppressed. Every other documented mode (concise/all/result/error) keeps today's CLI behavior unchanged.With this fix, a user who sets
offgets theCtrl+Cbehavior the issue asks for as a stopgap: the interrupted turn ends and nothing new starts on its own, whilewait/log/pollremain the way to read finished background output. The fuller purge UX proposed in the issue (ProcessRegistry.purge_notifications,/purge,ctrl_c_purge_notifications) is intentionally left open as a follow-up.Related Issue
Relates to #123114 — restores the documented
display.background_process_notifications: offescape hatch the issue reports as silently broken on the CLI; the proposed purge UX remains open for a follow-up.Type of Change
Changes Made
hermes_cli/cli_process_notifications.py— addCLIProcessNotificationsMixin._background_notifications_suppressed()(readsCLI_CONFIG'sdisplay.background_process_notifications, fails open, only the exactoffmode suppresses) and gate the injection loop in_drain_process_notificationswith it; the drain/claim/acknowledge chain runs in every mode.tests/hermes_cli/test_process_notification_display.py— parametrized regression:offconsumes and acknowledges events while leaving_pending_inputempty;conciseandallkeep injecting; the claim/ack chain is asserted to run under every mode.How to Test
python -m pytest tests/hermes_cli/test_process_notification_display.py tests/hermes_cli/test_cli_async_delegation_delivery.py tests/hermes_cli/test_subagent_notification_display.py -q— Observed result: 8 passed.python -m pytest tests/tools/test_async_delegation.py -q— Observed result: 36 passed (durable delivery chain unaffected).test_drain_honors_background_process_notifications_off[off-False]fail (_pending_inputstill receives the event); reapplying it turns the suite green.display.background_process_notifications: offin the CLI config, start aterminal(background=True)process, let it finish unread, interrupt the next turn withCtrl+C— the turn should end and no new notification turn should start.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/A (no new key; the existing documented key now takes effect)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A