feat(cli,tui): local /notify desktop notifications (supersedes #19302)#48593
Open
OutThisLife wants to merge 5 commits into
Open
feat(cli,tui): local /notify desktop notifications (supersedes #19302)#48593OutThisLife wants to merge 5 commits into
OutThisLife wants to merge 5 commits into
Conversation
Builds on PCinkusz's /notify command (previous commit) to fix one design flaw and tighten the implementation: - Per-session sentinel. The pending-notify flag was a single global file (~/.hermes/.notify_pending). The TUI gateway and dashboard serve many sessions from one process sharing one HERMES_HOME, so a /notify set in session A would fire on session B's next turn completion. Key the sentinel by HERMES_SESSION_KEY (resolved from the per-turn contextvar in the gateway, the slash worker's env, or os.environ in the classic CLI). Classic single-session CLI keeps the unsuffixed default file — no behavior change. - Single consume helper. The check->clear->fire block was copy-pasted at four sites (2 in cli.py, 2 in tui_gateway/server.py). Extract consume_pending_notification(session_key) and call it everywhere; the TUI sites pass session["session_key"] explicitly since that process has no per-session contextvar bound at the consume point. - Drop the unused config= param from fire_notification; add the missing trailing newline; reuse approval._get_session_platform() in the approval-notify guard. - tests/tools/test_notify_utils.py: per-session isolation, consume fire-once/scope, default-key, env-resolution. Co-authored-by: PCinkusz <pcinkusz123321@gmail.com>
11 tasks
alt-glitch
added
type/feature
Collaborator
|
Related: #19302 (original /notify PR by @PCinkusz, which this salvages and supersedes), #43225 (the originating feature request for a system notification when the agent response is ready). This PR rebases #19302 onto current |
Contributor
🔎 Lint report:
|
| Rule | Count |
|---|---|
PLW1514 |
1 |
First entries
tools/notify_utils.py:338: [PLW1514] `open` in text mode without explicit `encoding` argument
✅ Fixed issues: none
Unchanged: 0 pre-existing issues carried over.
ty (type checker)
Total: 11029 on HEAD, 11030 on base (✅ -1)
🆕 New issues (2):
| Rule | Count |
|---|---|
unresolved-import |
1 |
invalid-assignment |
1 |
First entries
tests/tools/test_notify_utils.py:8: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/run_agent/test_credits_notices_toggle.py:76: [invalid-assignment] invalid-assignment: Object of type `None` is not assignable to attribute `_credits_session_start_micros` of type `int`
✅ Fixed issues (2):
| Rule | Count |
|---|---|
unresolved-attribute |
2 |
First entries
run_agent.py:2941: [unresolved-attribute] unresolved-attribute: Object of type `Self@get_credits_spent_micros` has no attribute `_credits_session_start_micros`
tests/run_agent/test_credits_notices_toggle.py:76: [unresolved-attribute] unresolved-attribute: Unresolved attribute `_credits_session_start_micros` on type `AIAgent`
Unchanged: 5779 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
Plain `osascript display notification` attributes to the launching process; for an unsigned CLI that frequently can't register an app entry, so macOS delivers the notification silently to Notification Center with no banner and no toggle the user can enable. Prefer `terminal-notifier` when on PATH (it ships a real app bundle that shows banners and is grantable in System Settings), falling back to osascript otherwise. `brew install terminal-notifier` is the documented opt-in for reliable banners.
osascript is a dead end for an unsigned CLI on modern macOS (notifications permanently attributed to "Script Editor"; Apple removed sender override in Monterey) and terminal-notifier is broken on recent releases. The reliable approach used across CLI notifier projects is to let the terminal emulator raise the banner itself via OSC escape sequences — attributed to the terminal the user already trusts, click focuses it, zero dependencies. - Emit OSC 9 (iTerm2-style) or OSC 777 (urxvt-style, title+body) to /dev/tty — picked per terminal via TERM_PROGRAM/env so terminals that support both don't double-fire. Write to /dev/tty (not stdout, which the TUI/slash-worker capture); the sequences are non-rendering so they don't disturb a live TUI. - Works in iTerm2, Ghostty, kitty, WezTerm, Warp, VS Code, Cursor. Apple Terminal and unknown terminals return False and fall back to the existing OS-level path (notify-send / terminal-notifier / osascript / PowerShell). - tmux passthrough wrapping when $TMUX is set. - Add `import os` (the module didn't import it before; the new env reads need it). - Tests: terminal detection table, OSC 9/777 payloads, tmux wrap, unknown-terminal fallthrough, terminal-preferred-over-OS ordering.
Collaborator
Author
|
Update: reworked the macOS delivery after finding Primary path is now terminal-native OSC escape sequences (OSC 9 / OSC 777) written to |
…pt perms Research finding: terminfo.dev "support" for OSC 9/777 only means the parser consumes the sequence — VS Code/Cursor and Apple Terminal silently drop it without rendering anything (microsoft/vscode#294247, anthropics/claude-code#28338). Emitting OSC there made notifications no-op AND skipped the OS fallback. - _detect_terminal_osc now returns a flavor only for terminals that actually render: iTerm2, Ghostty, kitty, WezTerm. Everything else (VS Code/Cursor, Apple Terminal, unknown) falls through to the osascript path. VS Code/Cursor users wanting click-to-focus can install the "Terminal Notification" extension, which parses the OSC we already emit — documented, not assumed. - Add a one-time WARNING when the osascript fallback runs: on macOS Sequoia+, osascript notifications are attributed to "Script Editor" and silently dropped (exit 0, nothing shown) until the user grants Script Editor notification permission once. The hint spells out the fix so users aren't stuck staring at a no-op.
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.
Supersedes #19302 by @PCinkusz — rebased onto current
main(the original had merge conflicts intools/approval.pyandtui_gateway/server.py) and hardened. The author's commit is cherry-picked intact to preserve credit; a follow-up commit adds the fixes below.What it does
Adds a local
/notifyslash command to the classic CLI and the Ink TUI so the user gets a native OS desktop notification when the current/next turn finishes — the CLI/TUI analog of the desktop GUI's notifications./notify— notify when this turn (or the next) completes/notify <prompt>— set the flag and submit<prompt>/notify cancel— clear a pending notification/notifyis pending (suppressed on messaging-gateway sessions).Cross-platform delivery: Linux (
notify-send), macOS (osascript), Windows (PowerShell), WSL (WSLg→PowerShell fallback). Every path is fail-safe — a notification error can never crash the agent loop.Changes on top of #19302
~/.hermes/.notify_pendingflag. The TUI gateway/dashboard serve many sessions from one process sharing oneHERMES_HOME, so a/notifyin session A would fire on session B's turn completion. The sentinel is now keyed byHERMES_SESSION_KEY(resolved from the per-turn contextvar in the gateway, the slash worker's env, oros.environin the classic CLI). Classic single-session CLI keeps the unsuffixed default file — no behavior change.check → clear → fireblock was copy-pasted at 4 sites; replaced withconsume_pending_notification(session_key). TUI sites passsession["session_key"]explicitly (no per-session contextvar bound at the consume point).config=param fromfire_notification; added the missing trailing newline; reuseapproval._get_session_platform()in the approval-notify guard.tests/tools/test_notify_utils.pycovers per-session isolation, consume fire-once/scoping, default-key, and env resolution. Original approval tests retained.Validation
tests/tools/test_notify_utils.pytests/test_notify_approval.pytests/tools/test_approval.py+tests/tui_gateway/main; conflicts resolvedCloses #19302