Skip to content

feat(proactive-notify): add queue, voice, macos_notif action modules - #1129

Closed
bassilkhilo-ag2 wants to merge 1 commit into
sonichi:mainfrom
bassilkhilo-ag2:feat/proactive-notify-missing-actions
Closed

bassilkhilo-ag2 wants to merge 1 commit into
sonichi:mainfrom
bassilkhilo-ag2:feat/proactive-notify-missing-actions

Conversation

@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator

Problem

channel-policy.yaml.example references three action channel names —
queue, voice, and macos_notif — but none of those modules existed
under skills/proactive-notify/scripts/actions/. The runner does
importlib.import_module(f"actions.{channel}"), so any ping routed to
these channels failed with ModuleNotFoundError at runtime:

  • fyi-tier pings: default channel is queue → silent failure every run
  • Quiet-hours downgrades: important → queue, critical → stays as sms
  • Voice-client-connected override: routes all tiers to voice → failure
  • macOS notification: no path to send desktop banner

Solution

Add three new action modules:

queue.py

Appends to $WORKSPACE/skills/proactive-notify/state/queue.jsonl as
newline-delimited JSON entries (id, ts, ping name, urgency, body).
Idempotent directory creation; OSError returns {"ok": False, "error": ...}.

voice.py

Writes $WORKSPACE/results/proactive-{ts}.txt — the standard path
polled by task-bridge.ts for narration to the active voice session.

macos_notif.py

Fires osascript for a macOS desktop notification banner. Properly
escapes backslashes and double-quotes in the body/title before
interpolating into the AppleScript string literal. Custom title via
SUTANDO_NOTIF_TITLE env var (default: "Sutando"). Catches
FileNotFoundError, TimeoutExpired, and CalledProcessError.

Tests

tests/proactive-notify-actions-queue-voice-notif.test.py — 34 tests:

  • TestQueueModuleExists (2) + TestQueueSend (9): append logic, JSONL
    format, directory creation, error propagation
  • TestVoiceModuleExists (2) + TestVoiceSend (7): file naming, content,
    dir creation, error path
  • TestMacosNotifModuleExists (3) + TestMacosNotifSend (11): osascript
    call shape, title env override, all failure modes, injection escaping

All 34 pass on Python 3.9.

Checklist

  • queue.py — fyi + quiet-hours sink
  • voice.py — voice session narration
  • macos_notif.py — desktop notification banner
  • 34 tests, all green
  • No hardcoded owner literals (follows SKILL.md "No personal literals in code")

🤖 Generated with Claude Code

@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator Author

Note: Stacks on #1130 (base proactive-notify skill). Merge #1130 first.

@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator Author

Updating dependency: stacks on #968 (not #1130 which I just closed as duplicate). Merge #968 first.

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

BLOCKING

Same build_log.md leak as #1124 / #1131. Please drop it.

  1. build_log.md +114/-0 — identical Pass 207-211 content (branch names, commit SHAs, pending pushes for port/* + merge/stando-sync). Per-instance operational state; in .gitignore. Would overwrite/diverge the build_log.md from the prior author.

  2. Scope creep — title is feat(proactive-notify): add queue, voice, macos_notif action modules. The 3 action modules + their test (actions/queue.py, actions/voice.py, actions/macos_notif.py, tests/proactive-notify-actions-queue-voice-notif.test.py) are scoped. Out of scope: src/health-check.py +14/-2 and tests/health-check-bridge-multi-pid.test.py +88/-0 — same edits as in #1131. Move them to one shared PR rather than duplicating into every branch.

NON-BLOCKING

The 3 action modules themselves look right:

  • macos_notif.py — explicit \\ and " escaping before AppleScript interpolation is correct; subprocess.run(..., timeout=5) with FileNotFoundError/TimeoutExpired/CalledProcessError covers the failure modes.
  • voice.py writes results/proactive-{ts}.txt — matches task-bridge.ts poll pattern.
  • queue.py JSONL append + mkdir(parents=True, exist_ok=True) is idempotent.

Question: in macos_notif.py, do you want a length cap on body/title before passing to osascript? macOS notification banners truncate around ~250 chars anyway, but very long bodies could blow the command-line length budget. Probably fine in practice — flag for awareness, not required.

@VasiliyRad VasiliyRad 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.

Agree with Susan's review — the three action modules are solid, but the PR needs cleanup before merge.

Blocking (same as Susan's):

  • Drop build_log.md — per-instance operational state, gitignored, would conflict with other instances.
  • Move src/health-check.py and tests/health-check-bridge-multi-pid.test.py out of this branch. They duplicate #1131 and are unrelated to proactive-notify actions. One PR per concern.

Action modules look good (non-blocking notes):

  • queue.py: JSONL append with idempotent mkdir and OSError fallback — clean.
  • voice.py: Writes the standard results/proactive-{ts}.txt path that task-bridge polls — correct integration.
  • macos_notif.py: AppleScript escaping for \ and " is correct. subprocess.run with timeout=5 and all three exception handlers (FileNotFoundError, TimeoutExpired, CalledProcessError) cover the failure surface. Susan's note about a length cap is fair but not blocking.
  • Test file: 34 tests, thorough coverage including error paths and escaping edge cases.

Once build_log.md and the health-check changes are dropped, this is ready to approve.

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

Three action channels were referenced in channel-policy.yaml.example
but had no implementation — any ping routed to them failed with
ModuleNotFoundError at runtime:

- queue: appends to state/queue.jsonl (fyi tier + quiet-hours fallback)
- voice: writes results/proactive-{ts}.txt for voice-agent narration
- macos_notif: fires osascript desktop notification, escapes AppleScript
  string literals to prevent injection via event body

34 tests across all three modules — all passing.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bassilkhilo-ag2
bassilkhilo-ag2 force-pushed the feat/proactive-notify-missing-actions branch from 2172ef0 to be59874 Compare May 27, 2026 04:02
@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator Author

Rebased to clean diff — removed 3 bundled commits that were carrying build_log.md, src/health-check.py, and tests/health-check-bridge-multi-pid.test.py from unrelated passes. Only the 4 skill files remain:

  • skills/proactive-notify/scripts/actions/macos_notif.py
  • skills/proactive-notify/scripts/actions/queue.py
  • skills/proactive-notify/scripts/actions/voice.py
  • tests/proactive-notify-actions-queue-voice-notif.test.py

All 34 tests still pass. Re-requesting review.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants