Skip to content

fix: use config-light notifier defaults#2059

Open
whoisasx wants to merge 6 commits into
AgentWrapper:mainfrom
whoisasx:feat/1958
Open

fix: use config-light notifier defaults#2059
whoisasx wants to merge 6 commits into
AgentWrapper:mainfrom
whoisasx:feat/1958

Conversation

@whoisasx
Copy link
Copy Markdown
Contributor

Closes #1958

Supersedes #1961 because the original upstream branch is no longer pushable from this session.

Summary

  • Keep fresh notifier config light: defaults.notifiers enables dashboard + desktop, with no generated notifier config blocks or notificationRouting entries.
  • Add core implicit routing policy: dashboard receives all priorities; desktop receives urgent + warning; explicit notificationRouting still overrides per priority.
  • Keep manual notifiers opt-in and update dashboard/desktop setup commands to write enablement/config overrides only.
  • Stop ao start from expanding notifier YAML defaults while retaining best-effort macOS AO Notifier.app setup.

Tests

  • pnpm build && pnpm typecheck && pnpm lint && pnpm test

Note: lint passes with existing warnings only.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 25, 2026

Greptile Summary

This PR replaces the old explicit-routing-on-setup model with a config-light design: fresh configs now carry only defaults.notifiers: [dashboard, desktop] with no generated notifiers config blocks or notificationRouting entries. A new implicit routing policy in resolveNotificationRoute handles the semantics — dashboard receives all priorities, desktop receives urgent+warning only, and any explicit notificationRouting entry still overrides per priority.

  • packages/core/src/notifier-resolution.ts — new resolveNotificationRoute implements the implicit policy; packages/core/src/plugin-registry.ts adds canImplicitlyRegisterNotifier so dashboard and desktop self-register when referenced in defaults.notifiers without requiring an explicit config block.
  • packages/cli/src/lib/dashboard-setup.ts / desktop-setup.ts — setup commands now only write defaults.notifiers enablement (and optional override config); ao start gains a best-effort macOS AO Notifier.app install via isMac() from platform.ts, with graceful error recovery.
  • packages/core/src/global-config.ts — default notifiers renamed from composiodashboard and notificationRouting default changed from hardcoded priority maps to {}; createDefaultGlobalConfig updated consistently.

Confidence Score: 4/5

The routing logic and plugin registration changes are correct and well-tested, but one stale display path was not updated alongside the routing model change.

The new implicit routing in resolveNotificationRoute and plugin-registry.ts work correctly, and the setup commands correctly write config-light YAML. However, getNotifierRoutingState in notifier-routing.ts was not updated to understand the implicit model — it only inspects explicit notificationRouting entries — so ao setup dashboard --status and ao setup desktop --status both print Routing: not routed for every config-light install, giving users incorrect information about their running configuration.

packages/cli/src/lib/notifier-routing.ts — getNotifierRoutingState and the status label logic need to account for the new implicit defaults-based routing.

Important Files Changed

Filename Overview
packages/core/src/notifier-resolution.ts New file implementing implicit routing: dashboard receives all priorities, desktop receives urgent+warning only, explicit notificationRouting overrides per priority. Logic is correct and well-tested.
packages/cli/src/lib/notifier-routing.ts getNotifierRoutingState is not updated for implicit routing — status display shows "not routed" for config-light setups where only defaults.notifiers is set. Also, applyNotifierRoutingPreset writes explicit routes in dashboard-first order, inconsistent with implicit desktop-first ordering for urgent/warning.
packages/cli/src/commands/start.ts ensureDefaultStartupNotifiers now handles macOS AO Notifier.app setup, correctly using isMac() from platform.ts. The function no longer expands YAML defaults. Tests added for both the happy path and error recovery.
packages/cli/src/lib/desktop-setup.ts wireDesktopConfig now only writes an explicit notifiers.desktop config block when needed. installAoNotifierAppForStartup correctly uses refresh=true to avoid reinstalling if already present.
packages/cli/src/lib/dashboard-setup.ts writeDashboardConfig now only writes an explicit notifiers.dashboard config block when needed, and always calls ensureNotifierDefault to add dashboard to defaults.notifiers.
packages/core/src/global-config.ts Default notifiers changed from ["composio", "desktop"] to ["dashboard", "desktop"] and default notificationRouting changed from explicit priority routes to empty {}. createDefaultGlobalConfig updated consistently.
packages/core/src/plugin-registry.ts canImplicitlyRegisterNotifier added so dashboard and desktop plugins can self-register when referenced by name in defaults.notifiers, without requiring an explicit config block.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Notification event fires] --> B{resolveNotificationRoute}
    B -->|explicit notificationRouting entry?| C[Return explicit route as-is]
    B -->|no explicit route| D{Check defaults.notifiers}
    D --> E{priority = urgent or warning?}
    E -->|yes| F[desktop first, then other non-desktop notifiers]
    E -->|no: action / info| G[non-desktop notifiers only]

    H[ao setup dashboard] --> I[ensureNotifierDefault: add dashboard to defaults.notifiers]
    I --> J{limit != default or existing config?}
    J -->|yes| K[Write notifiers.dashboard config block]
    J -->|no| L[No notifiers block written]
    I --> M{routing preset flag?}
    M -->|yes| N[applyNotifierRoutingPreset: write explicit notificationRouting entries]
    M -->|no| O[No notificationRouting written - implicit policy applies]

    P[ao start on macOS] --> Q{isMac?}
    Q -->|yes| R[installAoNotifierAppForStartup]
    R -->|ok| S[Continue startup]
    R -->|error| T[Warn and continue with dashboard only]
    Q -->|no| S
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
packages/cli/src/lib/notifier-routing.ts:87-122
**Status display shows "not routed" for all config-light setups**

`getNotifierRoutingState` only reads explicit `notificationRouting` entries to determine routing state. Under the new config-light model, `ao setup dashboard` and `ao setup desktop` write only `defaults.notifiers` with no `notificationRouting` entries. As a result, `ao setup dashboard --status` and `ao setup desktop --status` both print `Routing: not routed` for every new install, even though `resolveNotificationRoute` correctly routes those notifiers via implicit defaults. A user seeing that label would reasonably believe their notifications are not configured and might try to re-run setup with an explicit `--routing-preset` to "fix" it, defeating the whole config-light intent.

### Issue 2 of 2
packages/cli/src/lib/notifier-routing.ts:136-162
**`applyNotifierRoutingPreset` writes desktop after dashboard in urgent routes, inconsistent with implicit ordering**

When an explicit routing preset (e.g., `--routing-preset urgent-action`) is applied for `desktop` with `defaults.notifiers: ["dashboard", "desktop"]`, the function reconstructs the urgent route as `["dashboard", "desktop"]` (base order preserved, desktop re-appended after removal). However, the implicit policy in `resolveNotificationRoute` always puts `desktop` *first* for `urgent`/`warning`. This means toggling from implicit to explicit routing reverses which notifier is dispatched first for urgent events. If delivery order matters (e.g., ensuring the OS notification fires before the dashboard write), this silent reordering can be surprising.

Reviews (2): Last reviewed commit: "fix: add concise notification presentati..." | Re-trigger Greptile

Comment thread packages/cli/__tests__/commands/notify.test.ts
Comment thread packages/cli/src/commands/start.ts
Comment thread packages/cli/src/commands/start.ts Outdated
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.

Fix default notifier onboarding: remove Composio defaults, enable dashboard + desktop

1 participant