merge upstream notification support into guardrails fork#27
Conversation
- Add Notification service with zero dependencies (darwin/linux/win32) - macOS notifications attributed to Terminal.app (fixes click target) - Add terminal focus detection to avoid notifications when focused - Add 'notifications' config option to tui.json (default: true) - Listen to session.idle for completion notifications - Update session.error to also show system notifications - Fix documentation plugin example (osascript attribution) - Beautify TUI toast: full borders + variant icons (✓✗ℹ⚠)
- Add .catch() to session.idle notification to prevent crashes - Use proper escapeForOsascript function for macOS - Add more terminals to detection list (vscode, code) - Restore escapeXml for Windows branch
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
There was a problem hiding this comment.
Pull request overview
This PR merges upstream TUI system-notification support into the fork, adds follow-up hardening (macOS focus detection + Windows toast payload handling), and updates docs/CI to match the new notification behavior and reporting output paths.
Changes:
- Add a cross-platform notification utility and wire it into the TUI for
session.idle/session.error. - Introduce a
notificationstoggle intui.jsonschema/config and update UI copy + docs to reference built-in notifications. - Add tests for terminal-name normalization + Windows toast XML escaping, and ensure CI creates/upload artifact directories correctly.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/web/src/content/docs/zh-tw/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/zh-cn/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/tr/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/th/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/ru/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/pt-br/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/plugins.mdx | Updates notification plugin example and mentions built-in TUI notifications toggle. |
| packages/web/src/content/docs/pl/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/nb/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/ko/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/ja/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/it/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/fr/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/es/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/de/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/da/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/bs/plugins.mdx | Updates macOS osascript notification example. |
| packages/web/src/content/docs/ar/plugins.mdx | Updates macOS osascript notification example. |
| packages/opencode/test/notification.test.ts | Adds tests for terminal-name normalization and XML escaping. |
| packages/opencode/src/notification/index.ts | Introduces notification implementation for macOS/Linux/Windows (PowerShell stdin XML). |
| packages/opencode/src/config/tui-schema.ts | Adds notifications option to tui.json schema. |
| packages/opencode/src/cli/cmd/tui/ui/toast.tsx | Toast polish: border/icon treatment + safer unknown error typing. |
| packages/opencode/src/cli/cmd/tui/feature-plugins/home/tips-view.tsx | Updates tip text to promote built-in notifications toggle. |
| packages/opencode/src/cli/cmd/tui/app.tsx | Hooks session events to show system notifications when appropriate. |
| packages/opencode/package.json | Ensures .artifacts/unit exists before writing JUnit output. |
| packages/app/package.json | Ensures .artifacts/unit exists before writing JUnit output. |
| .github/workflows/test.yml | Uploads hidden files so .artifacts/... is included in artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .optional() | ||
| .describe("Show system notifications when sessions complete or error. Requires terminal focus-loss detection."), |
There was a problem hiding this comment.
notifications is declared as .boolean().default(true).optional(). In Zod, calling .optional() after .default() prevents the default from being applied when the value is undefined, so notifications will parse to undefined instead of true. Reorder to .optional().default(true) (or drop .optional() entirely) so the runtime config and generated schema correctly reflect the default.
Also, the description implies notifications require focus-loss detection, but terminalIsFocused() currently returns false on non-macOS, meaning notifications will fire even when the terminal is focused on Linux/Windows; consider clarifying the description accordingly.
| .optional() | |
| .describe("Show system notifications when sessions complete or error. Requires terminal focus-loss detection."), | |
| .describe( | |
| "Show system notifications when sessions complete or error. Terminal focus detection may vary by platform.", | |
| ), |
| void Notification.terminalIsFocused().then((focused) => { | ||
| if (focused) return | ||
| if (tuiConfig.notifications === false) return |
There was a problem hiding this comment.
In the session.error handler, Notification.terminalIsFocused() runs unconditionally and only checks tuiConfig.notifications === false inside the .then(...). If notifications are disabled, this still executes osascript on macOS (potentially prompting for permissions / adding overhead) even though the result is discarded. Consider guarding on tuiConfig.notifications before calling terminalIsFocused() (similar to the session.idle handler).
| void Notification.terminalIsFocused().then((focused) => { | |
| if (focused) return | |
| if (tuiConfig.notifications === false) return | |
| if (tuiConfig.notifications === false) return | |
| void Notification.terminalIsFocused().then((focused) => { | |
| if (focused) return |
| // Send notification on session completion | ||
| if (event.type === "session.idle") { | ||
| await $`osascript -e 'display notification "Session completed!" with title "opencode"'` | ||
| await $`osascript -e 'tell application "Terminal" to display notification "Session completed!" with title "opencode"'` |
There was a problem hiding this comment.
The AppleScript example wraps display notification in tell application "Terminal" .... On macOS this can trigger an Automation permission prompt (“osascript wants to control Terminal”) and may fail if the user denies it. If the goal is just to post a notification, consider using display notification ... without targeting Terminal (or add a short note explaining the permission prompt / why Terminal is targeted).
| await $`osascript -e 'tell application "Terminal" to display notification "Session completed!" with title "opencode"'` | |
| await $`osascript -e 'display notification "Session completed!" with title "opencode"'` |
6393d95
into
fix/guardrails-bootstrap-and-openrouter
Summary
tui.jsonnotification toggleVerification