Conversation
Contributor
Test Coverage Report
Per-file breakdown
Uncovered lines
|
Contributor
Greptile SummaryThis PR overhauls the notifier onboarding flow: it removes the old implicit Composio/manual-notifier defaults and replaces them with an explicit startup routine that wires dashboard to all priorities and desktop (AO Notifier.app) to urgent-only, with best-effort macOS install on
Confidence Score: 3/5Needs cross-platform fix before merging: on Linux and Windows every The core onboarding logic is well-structured and idempotent, and the Composio cleanup is correct. However, the
|
| Filename | Overview |
|---|---|
| packages/cli/src/lib/startup-notifier-defaults.ts | New module implementing idempotent YAML-preserving notifier defaults. Correctly guards custom desktop configs. Exports createStartupNotifierConfig which is unused elsewhere in the PR. |
| packages/cli/src/commands/start.ts | Adds ensureDefaultStartupNotifiers which gates AO Notifier.app install inside if (isMac()) but leaves desktopMode = "enable" on Linux/Windows, causing ensureDesktopNotifier to write backend: "ao-app" into the config on every platform. |
| packages/core/src/global-config.ts | Replaces old Composio defaults with createDefaultNotifierConfigs() (desktop + dashboard). The factory hardcodes backend: "ao-app" which is macOS-only; applied on all platforms when notifiers is absent from a user's YAML. |
| packages/core/src/plugin-registry.ts | Adds canImplicitlyRegisterNotifier guard that prevents manual opt-in notifiers (composio, slack, etc.) from being auto-instantiated when referenced only by routing; dashboard and desktop remain implicitly registerable. |
| packages/plugins/notifier-desktop/src/index.ts | Extracts usesAoNotifierNativeActions helper and adds isMac() guard, fixing action-label hiding on Linux/Windows when backend is ao-app. Also retains the ENOENT-aware error handling in detectTerminalNotifier. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ao start invoked] --> B[runStartup]
B --> C[ensureDefaultStartupNotifiers]
C --> D{config.configPath exists?}
D -- No --> E[return config unchanged]
D -- Yes --> F[resolveStartupNotifierConfigPath]
F --> G{isMac?}
G -- Yes --> H[installAoNotifierAppForStartup]
H -- success --> I[desktopMode = enable]
H -- throws --> J[desktopMode = disable-default]
G -- No --> I
I --> K[ensureStartupNotifierDefaults]
J --> K
K --> L{changed?}
L -- Yes --> M[writeFileSync YAML]
M --> N[loadConfig targetConfigPath]
L -- No --> O[return original config]
N --> O
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/cli/src/lib/startup-notifier-defaults.ts:168-189
`createStartupNotifierConfig` is exported but never imported anywhere in the codebase — only `ensureStartupNotifierDefaults` is used. If this is intended as a future public API, a brief comment would clarify; otherwise it can be removed to avoid confusion with the similarly-structured internal logic in `ensureStartupNotifierDefaults`.
```suggestion
/** Returns the canonical startup notifier configuration for a given port.
* Used when programmatically building a fresh config; for in-place YAML
* updates prefer `ensureStartupNotifierDefaults`.
*/
export function createStartupNotifierConfig(port = 3000): StartupNotifierConfig {
const dashboardUrl = `http://localhost:${port}`;
return {
notifiers: {
desktop: {
plugin: "desktop",
backend: "ao-app",
dashboardUrl,
},
dashboard: {
plugin: "dashboard",
limit: DEFAULT_DASHBOARD_NOTIFICATION_LIMIT,
},
},
notificationRouting: {
urgent: ["desktop", "dashboard"],
action: ["dashboard"],
warning: ["dashboard"],
info: ["dashboard"],
},
};
}
```
Reviews (5): Last reviewed commit: "fix: preserve notifier routing fallbacks..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
whoisasx
requested review from
AgentWrapper,
Copilot,
i-trytoohard and
illegalcall
Contributor
There was a problem hiding this comment.
Pull request overview
Updates AO’s first-run notification onboarding to remove implicit Composio defaults and ensure startup config enables dashboard notifications for all priorities and desktop notifications for urgent only, with best-effort macOS AO Notifier.app setup.
Changes:
- Remove implicit Composio/manual notifier instantiation from routing-only references; allow implicit registration only for dashboard/desktop (and for external plugin loads).
- Change global config + auto-generated config defaults to dashboard(all) + desktop(urgent) and add startup-time config patching for existing installs.
- Update desktop/dashboard setup defaults and expand test coverage for fresh config + startup routing behavior.
Architecture & Design
- New
ensureStartupNotifierDefaults()introduces config-rewrite behavior duringao start. This is a high-impact path; correctness around preserving existing routing semantics is important to avoid surprising notifier changes.
Backward Compatibility
- Blocking:
ensureStartupNotifierDefaults()currently builds per-priority routes without consideringdefaults.notifiersfallback semantics, which can disable previously-configured notifiers that were only specified viadefaults.notifiers(see stored comment IDs: 001). This can break existing configs that intentionally omitnotificationRouting.
Testing
- Added/updated tests cover the new “fresh config” and startup rewrite paths; however, the missing
defaults.notifiersfallback scenario should be covered once fixed.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/plugins/notifier-desktop/src/index.ts | Minor log formatting in Windows toast failure path. |
| packages/integration-tests/src/notifier-desktop.integration.test.ts | Fix ENOENT mocking for desktop setup command detection. |
| packages/core/src/plugin-registry.ts | Restrict implicit notifier registration from routing-only references (dashboard/desktop only). |
| packages/core/src/global-config.ts | Remove Composio defaults; add startup notifier configs + routing defaults to global config schema/defaults. |
| packages/core/src/tests/plugin-registry.test.ts | Add regression test for “manual opt-in” notifiers not being implicitly registered. |
| packages/core/src/tests/global-config.test.ts | Assert fresh global config contains dashboard+desktop defaults and no Composio. |
| packages/cli/src/lib/startup-notifier-defaults.ts | New startup config mutator: ensures dashboard(all) + desktop(urgent) and removes implicit manual defaults. |
| packages/cli/src/lib/desktop-setup.ts | Change non-interactive default routing to urgent-only; add startup installer helper for AO Notifier.app. |
| packages/cli/src/lib/dashboard-setup.ts | Change non-interactive default routing preset to all priorities. |
| packages/cli/src/commands/start.ts | Apply startup notifier defaults during ao start, best-effort macOS notifier app setup. |
| packages/cli/tests/lib/startup-notifier-defaults.test.ts | New unit tests for startup notifier defaults rewrite behavior. |
| packages/cli/tests/lib/path-equality.test.ts | Adjust assertion to match canonicalCompareKey behavior. |
| packages/cli/tests/commands/start.test.ts | Update autoCreateConfig expectations for new notifier defaults. |
| packages/cli/tests/commands/setup.test.ts | Update setup command expectations for new routing presets (dashboard=all, desktop=urgent-only). |
Contributor
Author
|
Superseded by #2059. The original PR head branch is in ComposioHQ/agent-orchestrator and this session no longer has push permission there; the validated update was pushed to the fork branch instead. |
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
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.
Summary
Fixes #1958
Tests