fix(notify): native desktop notifications on macOS/Windows + warn when inert (#231)#282
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
Walkthrough
ChangesDesktop notification delivery
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/test_gates.py (1)
109-124: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that the Windows title is excluded from
argvtoo.This test only protects the message. A regression that interpolates the notification title into the PowerShell command would still pass. Use a title containing shell/PowerShell-sensitive text and assert neither title nor message appears in
argv.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_gates.py` around lines 109 - 124, Update test_notify_windows_runs_powershell to use a title containing PowerShell-sensitive characters, then assert that neither the title nor the message appears in any argv element while preserving the existing environment-variable assertions.tests/test_engine.py (1)
119-130: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the “once per run” guarantee.
This test invokes
_warn_desktop_notifier_inert()only once, so it cannot detect duplicate journal entries or stderr warnings if startup reaches the helper twice. Invoke it twice and assert exactly one event and one warning line.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_engine.py` around lines 119 - 130, The test for _warn_desktop_notifier_inert should verify the once-per-run behavior by invoking the helper twice, then asserting exactly one "notify-desktop-unavailable" journal entry and exactly one matching stderr warning line. Update test_warn_desktop_notifier_inert_journals_and_prints while preserving the existing notifier setup and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 70-78: Update the changelog entry for notify.desktop to limit the
environment-variable claim to macOS and Windows, and explicitly state that Linux
passes the title and message through notify-send argv without shell
interpolation. Preserve the surrounding platform-dispatch and warning details.
In `@src/bmad_loop/cli.py`:
- Around line 265-274: The desktop-unavailable warning in src/bmad_loop/cli.py
lines 265-274 must conditionally mention the ATTENTION file only when
pol.notify.file is enabled; otherwise state that no alert channel is configured
and suggest enabling file notifications. Apply the same conditional guidance to
the stderr message in src/bmad_loop/engine.py lines 315-320 using
self.policy.notify.file.
In `@src/bmad_loop/engine.py`:
- Around line 329-330: Update the warning condition in the engine run flow
around _warn_desktop_notifier_inert() to suppress warnings only when _is_nested
is true. Ensure top-level runs, including those unable to install signal
handlers because they are off the main thread, still invoke the desktop notifier
warning.
In `@src/bmad_loop/gates.py`:
- Around line 24-38: Update the Windows PowerShell notifier around _WIN_TOAST_PS
and desktop_notifier_kind() so the AUMID used by
CreateToastNotifier('bmad-loop') is registered with a matching Start-menu
shortcut before exposing the "powershell" notifier. Perform this registration
during installation or validate it at notification time, and only select the
PowerShell path when the identity is available; otherwise use an
already-registered delivery path or disable the notifier.
---
Nitpick comments:
In `@tests/test_engine.py`:
- Around line 119-130: The test for _warn_desktop_notifier_inert should verify
the once-per-run behavior by invoking the helper twice, then asserting exactly
one "notify-desktop-unavailable" journal entry and exactly one matching stderr
warning line. Update test_warn_desktop_notifier_inert_journals_and_prints while
preserving the existing notifier setup and assertions.
In `@tests/test_gates.py`:
- Around line 109-124: Update test_notify_windows_runs_powershell to use a title
containing PowerShell-sensitive characters, then assert that neither the title
nor the message appears in any argv element while preserving the existing
environment-variable assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 090ffc30-8446-4c2b-bb5a-1f914ec80fc4
📒 Files selected for processing (10)
CHANGELOG.mdREADME.mdsrc/bmad_loop/checks.pysrc/bmad_loop/cli.pysrc/bmad_loop/engine.pysrc/bmad_loop/gates.pysrc/bmad_loop/policy.pytests/test_cli.pytests/test_engine.pytests/test_gates.py
…-main-thread gate (#231 review)
|
Thanks @coderabbitai — validated each finding against the code (and current WinRT docs for the Windows one). Addressed in
Docstring-coverage pre-merge warning is a CodeRabbit-wide metric (it counts test functions); this repo gates on |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline 00168bd
git diff 00168bd^ 00168bd -- \
src/bmad_loop/gates.py \
src/bmad_loop/engine.py \
src/bmad_loop/cli.py \
CHANGELOG.md \
tests/test_gates.py \
tests/test_engine.py
printf '\n--- gates outline ---\n'
ast-grep outline src/bmad_loop/gates.py --items all --match 'desktop_notifier_kind|notify|_WIN_TOAST_PS' --view expanded
printf '\n--- engine outline ---\n'
ast-grep outline src/bmad_loop/engine.py --items all --match 'run|_warn_desktop_notifier_inert' --view expanded
printf '\n--- relevant call sites and messages ---\n'
rg -n -C 5 '(_warn_desktop_notifier_inert|_is_nested|_owns_signals|CreateToastNotifier|BMAD_LOOP_NOTIFY|ATTENTION file|no alert channel|notify\.file)' \
src/bmad_loop/gates.py src/bmad_loop/engine.py src/bmad_loop/cli.py tests/test_gates.py tests/test_engine.py CHANGELOG.mdLength of output: 31267
Agreed on the helper test: 🐇 ✅ |
Comprehensive review findingsRecommendation: request changes before merge. I found four implementation defects and three verification gaps in the final PR diff ( Findings
Two additional candidates were dismissed: discarded nonzero notifier exits are intentional best-effort behavior, and the alleged PowerShell AUMID mismatch was not sufficiently supported as a defect. Remediation plan
Current validation is otherwise green: all reported PR checks pass; locally, |
…review) Second-model review of PR #282 raised 7 findings; all validated true and addressed on this branch. - gates.notify: catch ValueError at the best-effort subprocess boundary. An embedded NUL in the untrusted title/message reaches argv (notify-send) or an env value (osascript/PowerShell) and makes subprocess.run raise ValueError, which is not a SubprocessError subclass and was escaping the best-effort catch. - gates._notifier_argv: add a `--` terminator before the notify-send title/message so an option-shaped payload (e.g. `--help`) stays positional SUMMARY/BODY text instead of being parsed as a GLib option. - engine.run: emit the inert-notifier warning before the pre_run boundary, whose plugin `pause` veto raises RunPaused and would otherwise skip the warning. - engine: derive `_is_nested` from a module-level contextvars run-depth counter instead of signal ownership. An off-main-thread top-level run owns no signals yet is still depth-0 and must warn; a nested auto-sweep (run synchronously in-thread) is depth>0. Signal ownership now only governs handler install and session teardown. - Document that native macOS/Windows delivery is unit-tested at the command-construction level (headless CI cannot observe a toast); manual smoke test noted in CHANGELOG + gates.notify docstring. Tests: embedded-NUL/ValueError swallow, notify-send `--` terminator, pwsh-only dispatch, the desktop/file warning matrix, and off-main-thread + nested run() wiring. The three nested-reraise tests now simulate nesting via _run_depth. Also harden tests/test_tui_app.py render() with no_color=True so its column-alignment assertions are deterministic under a forced-color environment (FORCE_COLOR/CLICOLOR_FORCE) — unrelated to notify, but it was surfacing as a spurious failure when the suite runs under a color-forcing wrapper. Full suite: 2934 passed, 1 skipped (no FORCE_COLOR); trunk check --no-fix clean.
Addressed in
|
1 participant
Closes #231
Problem
notify.desktopdefaults totrue, but the only desktop channel wasnotify-send(Linux-only). On macOS/Windows theshutil.which("notify-send")guard made the call a silent no-op — the setting was enabled, valid, and did nothing, with no indication anywhere. Sincegates.notifyis the single choke every "a human is needed" path funnels through (escalations, deferrals, worktree-open failures, run-finished), an unattended macOS/Windows run that needed a human had no channel but the untailedATTENTIONfile in.bmad-loop/.Fix
A small notifier-resolution seam in
gates.py, consumed bynotify,validate, and the engine:gates.notify:osascript(macOS), a best-effort WinRTToastNotificationManagerPowerShell toast (Windows, dependency-free),notify-send(Linux). Resolution is gated onsys.platformfirst (notwhichalone) so PowerShell Core on Linux can't divert Linux offnotify-send. Windows stays strictly best-effort inside the existing try/except — a failure only ever no-ops.system attribute/$env:), never interpolated into the osascript/PowerShell command string, so quotes/newlines/metacharacters in story keys or error tails can't break out.notify-sendkeeps taking them as argv (already injection-safe). A test proves the raw text lands inenv, notargv.validatestill surface it:validateemits anotify.desktop-unavailablewarning finding whennotify.desktopis set but no notifier resolves on the platform.warning:line and journals anotify-desktop-unavailableevent (gated by_owns_signalsso nested auto-sweeps don't repeat it).No new policy field — reuses the existing
notify.desktop/notify.file.Tests
tests/test_gates.py: file sink;desktop_notifier_kindper platform +None; osascript/PowerShell/notify-send argv+env dispatch; the untrusted-text-via-env-not-argv assertion; no-op when no notifier; error swallow.tests/test_cli.py:validatewarns when no notifier / silent when one is available.tests/test_engine.py:_warn_desktop_notifier_inertjournals + prints when inert / no-ops when a notifier is present.Full suite green (2928 passed, 1 skipped);
trunk check --no-fixclean. The Windows toast path is unverifiable on this repo's Linux/macOS CI and is best-effort by design.Summary by CodeRabbit
[notify] desktop = truepolicy documentation to reflect platform-specific mechanisms.