feat: add macOS desktop notification Stop hook#846
Conversation
|
❌ Analysis Failed
Troubleshooting
Retry: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Stop-phase macOS desktop notification: documentation entry, a new Stop hook registration in Changes
Sequence DiagramsequenceDiagram
participant HookSystem as Hook System
participant Script as desktop-notify.js
participant macOS as osascript / macOS
HookSystem->>Script: invoke run(raw) on Stop
Script->>Script: parse raw (JSON or {})
Script->>Script: extract first non-empty line\nsanitize and truncate to MAX_BODY_LENGTH
alt platform is macOS
Script->>macOS: run osascript (display notification)
macOS->>macOS: show desktop notification
else
Script->>Script: skip notification (no-op)
end
Script->>HookSystem: return raw unchanged
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR adds a All three concerns raised in the previous review round have been resolved in this revision:
Remaining minor note:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CC as Claude Code
participant RWF as run-with-flags.js
participant DN as desktop-notify.js
participant OS as osascript
CC->>RWF: Stop hook fired (stdin JSON)
RWF->>DN: run(raw)
DN->>DN: isMacOS check
alt not macOS
DN-->>RWF: return raw (no-op)
else macOS
DN->>DN: JSON.parse(raw)
DN->>DN: extractSummary(last_assistant_message)
DN->>OS: spawnSync('osascript', ['-e', script], {timeout:5000})
alt success
OS-->>DN: exit 0
else failure
OS-->>DN: non-zero / error / signal
DN->>DN: log error to stderr
end
DN-->>RWF: return raw
end
RWF-->>CC: stdout passthrough
Reviews (4): Last reviewed commit: "fix: add spawnSync error logging and res..." | Re-trigger Greptile |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/hooks/desktop-notify.js (1)
50-52: Consider tracking cross-platform support.The TODO comments indicate Windows and Linux support is planned. Based on learnings about ensuring cross-platform compatibility, consider opening an issue to track this work so it doesn't get lost.
Would you like me to help draft an issue to track Windows (PowerShell
BurntToastor native toast) and Linux (notify-send) support?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/desktop-notify.js` around lines 50 - 52, Add a tracked issue for cross-platform desktop notifications referenced by the TODOs in scripts/hooks/desktop-notify.js (functions notifyWindows and notifyLinux); the issue should describe implementing notifyWindows (PowerShell BurntToast or native toast API), notifyLinux (notify-send or libnotify), a clear API/abstraction in the existing notify function, fallback behavior for unsupported platforms, testing requirements, and acceptance criteria so this planned work isn't lost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/hooks/desktop-notify.js`:
- Around line 50-52: Add a tracked issue for cross-platform desktop
notifications referenced by the TODOs in scripts/hooks/desktop-notify.js
(functions notifyWindows and notifyLinux); the issue should describe
implementing notifyWindows (PowerShell BurntToast or native toast API),
notifyLinux (notify-send or libnotify), a clear API/abstraction in the existing
notify function, fallback behavior for unsupported platforms, testing
requirements, and acceptance criteria so this planned work isn't lost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1007207f-cc8e-4be9-931d-84b42be8d921
📒 Files selected for processing (3)
hooks/README.mdhooks/hooks.jsonscripts/hooks/desktop-notify.js
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="hooks/hooks.json">
<violation number="1" location="hooks/hooks.json:298">
P2: New Stop desktop notification hook exposes assistant summary text through macOS notifications by default, creating avoidable privacy/data-leak risk.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "node \"${CLAUDE_PLUGIN_ROOT}/scripts/hooks/run-with-flags.js\" \"stop:desktop-notify\" \"scripts/hooks/desktop-notify.js\" \"standard,strict\"", |
There was a problem hiding this comment.
P2: New Stop desktop notification hook exposes assistant summary text through macOS notifications by default, creating avoidable privacy/data-leak risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At hooks/hooks.json, line 298:
<comment>New Stop desktop notification hook exposes assistant summary text through macOS notifications by default, creating avoidable privacy/data-leak risk.</comment>
<file context>
@@ -289,6 +289,18 @@
+ "hooks": [
+ {
+ "type": "command",
+ "command": "node \"${CLAUDE_PLUGIN_ROOT}/scripts/hooks/run-with-flags.js\" \"stop:desktop-notify\" \"scripts/hooks/desktop-notify.js\" \"standard,strict\"",
+ "async": true,
+ "timeout": 5
</file context>
There was a problem hiding this comment.
This is intentional — the hook is opt-in by design (only active on standard and strict profiles, not minimal). Users who install the plugin explicitly want task completion notifications. They can disable it anytime via ECC_DISABLED_HOOKS=stop:desktop-notify. The notification only shows the first line of the response (truncated to 100 chars), not the full conversation.
|
❌ Analysis Failed
Troubleshooting
Retry: |
There was a problem hiding this comment.
3 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/hooks/desktop-notify.js">
<violation number="1" location="scripts/hooks/desktop-notify.js:47">
P2: Notification text is made lossy by stripping backslashes and rewriting quotes, causing inaccurate user-visible summaries.</violation>
<violation number="2" location="scripts/hooks/desktop-notify.js:50">
P2: Internal osascript timeout (3s) is shorter than the hook’s configured 5s timeout, causing avoidable early notification aborts.</violation>
<violation number="3" location="scripts/hooks/desktop-notify.js:50">
P2: Check the `spawnSync` result and emit a warning on `error` or non-zero exit; with `stdio: 'ignore'` this currently fails silently when `osascript` is denied permissions or times out.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
❌ Analysis Failed
Troubleshooting
Retry: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/hooks/desktop-notify.js (1)
63-75: Consider defensive null check forrawparameter.Line 67's
raw.trim()would throwTypeErrorifrawis unexpectedlynullorundefined. While the catch block handles this gracefully andrun-with-flags.jstypically provides a string, a defensive check would make the function more robust for direct callers.🛡️ Optional defensive check
function run(raw) { try { if (!isMacOS) return raw; - const input = raw.trim() ? JSON.parse(raw) : {}; + const input = raw && raw.trim() ? JSON.parse(raw) : {}; const summary = extractSummary(input.last_assistant_message); notifyMacOS(TITLE, summary); } catch (err) { log(`[DesktopNotify] Error: ${err.message}`); } - return raw; + return raw ?? ''; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/desktop-notify.js` around lines 63 - 75, The run function calls raw.trim() which will throw if raw is null/undefined; add a defensive null/undefined check at the top of run (or normalize raw to an empty string) so you only call .trim() on a string; update the early-return behavior to still return raw (or the normalized string) for direct callers, and keep the existing try/catch and notifyMacOS(TITLE, summary) flow—refer to the run function and extractSummary/notifyMacOS usage when locating the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/hooks/desktop-notify.js`:
- Around line 63-75: The run function calls raw.trim() which will throw if raw
is null/undefined; add a defensive null/undefined check at the top of run (or
normalize raw to an empty string) so you only call .trim() on a string; update
the early-return behavior to still return raw (or the normalized string) for
direct callers, and keep the existing try/catch and notifyMacOS(TITLE, summary)
flow—refer to the run function and extractSummary/notifyMacOS usage when
locating the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e54bd14a-535b-44e3-b1d2-3610ca5cdc9c
📒 Files selected for processing (2)
hooks/hooks.jsonscripts/hooks/desktop-notify.js
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/hooks.json
|
thanks for the pr. quick triage pass done, a maintainer will take a closer look soon. |
Add a new Stop hook that sends a native macOS notification with the task summary (first line of last_assistant_message) when Claude finishes responding. Uses osascript via spawnSync for shell injection safety. Supports run-with-flags fast require() path. Only active on standard and strict profiles; silently skips on non-macOS platforms.
- Replace JSON.stringify with curly quote substitution for AppleScript compatibility (AppleScript does not support \" backslash escapes) - Reduce spawnSync timeout from 5000ms to 3000ms to leave headroom within the 5s hook deadline
- Check spawnSync result and log warning on failure via stderr - Restore osascript timeout to 5000ms, increase hook deadline to 10s for sufficient headroom
c21118e to
f6b1048
Compare
|
Analysis Failed
Troubleshooting
Retry: |
|
ECC Tools had a ref-resolution bug on PR analysis retries and fork/base lookups. The app-side fixes are merged upstream in ECC-Tools#13 and ECC-Tools#15, so this failure path is now fixed in the app code. Once that deployed version is live, rerunning |
…hook feat: add macOS desktop notification Stop hook
What Changed
Add a new
Stophook (desktop-notify.js) that sends a native macOS desktop notification with the task summary when Claude finishes responding.scripts/hooks/desktop-notify.js— Extracts the first line oflast_assistant_message(truncated to 100 chars) and sends it viaosascripthooks/hooks.json— Registered the hook in theStoparray withasync: trueandtimeout: 5hooks/README.md— Added entry to the Lifecycle Hooks tableWhy This Change
There is no built-in way to know when Claude Code finishes a task if you switch away from the terminal. This hook provides Codex-style desktop notifications so users can multitask without constantly checking the terminal.
Testing Done
last_assistant_messageto the script — macOS notification appeared with correct summary{}— notification appeared with fallback text "Done"node tests/run-all.js)last_assistant_message→ fallback to "Done"Type of Change
feat:New featureSecurity & Quality Checklist
spawnSyncwith argument array (not shell string) to prevent command injectionJSON.stringifyused for osascript arguments to safely escape user contentDocumentation
hooks/README.md)Summary by cubic
Add a Stop hook that sends a native macOS notification with a short task summary when Claude finishes responding. Uses safe AppleScript escaping, logs failures, and fits within the hook timeout for reliable delivery.
New Features
scripts/hooks/desktop-notify.js: takes the first non-empty line oflast_assistant_message(≤100 chars), sends viaosascript, supportsrun-with-flagsfast path, and passes stdin through unchanged; no-op on non-macOS.hooks/hooks.jsonforstandardandstrictwithasync: trueandtimeout: 10;hooks/README.mddocuments the hook.Bug Fixes
osascriptfailures and use a 5000msspawnSynctimeout to stay within the 10s hook window.Written for commit f6b1048. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation