feat: add notifier delivery runtime by whoisasx · Pull Request #58 · aoagents/ReverbCode

whoisasx · 2026-05-31T19:47:56Z

Summary

add notification settings, route resolution, retry policy, and dispatcher-backed manager for the central notifier runtime
add durable notification_deliveries storage, CDC events, routed_at tracking, and sqlc regeneration
wire notifier runtime into daemon startup/shutdown without native desktop shell fallbacks

Tests

cd backend && go test ./...
cd backend && go vet ./...

Closes #55

greptile-apps · 2026-05-31T19:51:46Z

Greptile Summary

This PR adds the central notifier delivery runtime: a durable notification_deliveries table with CDC triggers, an exponential-backoff RetryPolicy, a Manager that tick-routes unrouted notifications into delivery rows, and daemon startup/shutdown wiring. The migration also introduces notification_delivery_attempts as scaffolding for future audit tracking.

Routing layer: Manager.RunOnce releases expired leases, then routes unrouted notifications into per-sink DeliveryRow records using ResolveRoutes; idempotent ON CONFLICT DO NOTHING inserts make the tick safe to replay.
Retry policy: MarkDeliveryError now reads the current attempt count before scheduling the next retry via NextAttemptAt(now, row.Attempts+1), so exponential backoff is attempt-aware.
Shutdown: notifierStack.Stop() blocks on the dispatcher's done channel, which exits after the context is cancelled, so graceful drain is integrated into the daemon's existing stop sequence.

Confidence Score: 5/5

Safe to merge; the routing, backoff, and shutdown logic are all correct and the previously-identified bugs have been fixed.

The core routing loop is idempotent (ON CONFLICT DO NOTHING), attempt-aware backoff is correctly implemented using the fetched delivery row, and all daemon error paths call notifier.Stop(). No data-loss or correctness bugs were found in the changed code.

backend/internal/storage/sqlite/notification_delivery_store.go — the lease-expiry re-queue path uses next_attempt_at = now rather than a backoff-derived timestamp, which is asymmetric with the explicit-error path.

Important Files Changed

Filename	Overview
backend/internal/notification/manager.go	Core routing manager; tick-based routing, idempotent delivery enqueue, and attempt-aware retry scheduling are all correct.
backend/internal/notification/retry.go	Exponential backoff with crypto jitter; BackoffDelay, NextAttemptAt, and ClassifyError are correct and well-guarded.
backend/internal/storage/sqlite/notification_delivery_store.go	Delivery store is thorough; ReleaseExpiredDeliveryLeases re-queues expired rows with next_attempt_at = now, bypassing the backoff policy for lease-expired retries.
backend/internal/storage/sqlite/migrations/0003_notification_deliveries.sql	Well-structured migration; adds routed_at, partial index, notification_deliveries table with idempotency constraints, CDC triggers, and notification_delivery_attempts scaffold table.
backend/internal/notification/routing.go	Route resolution is clean; dashboard is correctly excluded from delivery row creation, unknown routes create skipped rows for operator visibility.
backend/internal/daemon/daemon.go	Notifier startup and multi-path shutdown are correctly integrated; notifier.Stop() is called in all error paths after startNotifier.
backend/internal/notification/settings.go	NormalizeSettings fills zero sub-fields with defaults while preserving explicit values, including Enabled: false; cloneSettings prevents aliasing on slice fields.
backend/internal/notification/delivery.go	NormalizeDelivery correctly applies manager-supplied MaxAttempts before falling back to the store default; ID generation uses crypto/rand.
backend/internal/daemon/notifier_wiring.go	Thin wiring; Stop() guards against nil stack and blocks on done channel until the dispatcher goroutine exits.

Sequence Diagram

sequenceDiagram
    participant D as Daemon
    participant M as Manager (tick)
    participant S as SQLite Store
    participant E as Electron (AO-app)

    D->>M: Start(ctx) → startDispatcher goroutine
    loop every 1s
        M->>S: ReleaseExpiredDeliveryLeases(now)
        S-->>M: n released
        M->>S: ListUnroutedNotifications(limit)
        S-->>M: []Notification
        loop per notification
            M->>M: ResolveRoutes(settings, priority)
            M->>S: EnqueueDelivery(DeliveryRow) [ON CONFLICT DO NOTHING]
            M->>S: MarkNotificationRouted(id, now)
        end
    end
    E->>M: ClaimDesktopDeliveries(owner, limit)
    M->>S: "ClaimDueDeliveries(sink=ao-app, owner, now, limit, leaseTTL)"
    S-->>E: "[]DeliveryRow (status=leased)"
    alt delivery success
        E->>M: MarkDeliverySent(id, owner, externalID)
        M->>S: "UPDATE status=sent, attempts+1"
    else transient error
        E->>M: MarkDeliveryError(id, owner, code, msg)
        M->>S: GetDelivery(id) → row.Attempts
        M->>M: NextAttemptAt(now, row.Attempts+1)
        M->>S: "MarkDeliveryRetry → status=retry_wait or failed"
    else permanent error
        E->>M: MarkDeliveryError(id, owner, code, msg)
        M->>S: "MarkDeliveryFailed → status=failed"
    end
    note over M,S: Lease expiry path (no explicit error)
    M->>S: "ReleaseExpiredDeliveryLeases → attempts+1, next_attempt_at=now"

_{Reviews (6): Last reviewed commit: "fix: preserve notification surface disab..." | Re-trigger Greptile}

whoisasx

Reviewed locally with focused storage/runtime/config passes. Backend checks are green (go test ./..., gofmt -l ., go vet ./..., go test -race ./...), but I found a few correctness issues that should be fixed before merge.

Copilot

Pull request overview

This PR introduces the backend “notifier runtime” layer that routes durable notifications into durable notification_deliveries (desktop/AO-app handoff), including routing/settings defaults, retry/lease mechanics, and CDC events, and wires it into daemon startup/shutdown.

Changes:

Add notification runtime package (settings, routing, retry policy, dispatcher loop, manager API) and integrate it into daemon lifecycle.
Add notification_deliveries (+ attempt audit table) schema, CDC triggers/events, and a routed_at marker for notifications; regenerate sqlc code.
Implement SQLite storage for delivery enqueue/claim/lease/retry/sent state + unit/integration tests.

Reviewed changes

Copilot reviewed 25 out of 29 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
backend/notifier_wiring.go	Starts notifier manager/dispatcher and exposes a stop hook for shutdown sequencing.
backend/main.go	Wires notifier runtime into daemon boot and shutdown ordering.
backend/internal/storage/sqlite/queries/notifications.sql	Extends notification queries to include `routed_at` in returned columns.
backend/internal/storage/sqlite/queries/notification_deliveries.sql	Adds sqlc queries for unrouted notifications, routing marker, and delivery insert/get APIs.
backend/internal/storage/sqlite/notification_store.go	Maps new `routed_at` column into the domain notification row.
backend/internal/storage/sqlite/notification_delivery_store.go	Implements durable delivery enqueue/claim/lease/release/update operations on SQLite.
backend/internal/storage/sqlite/notification_delivery_store_test.go	Unit tests for enqueue idempotency, leasing, expiry, terminal states, and CDC behavior.
backend/internal/storage/sqlite/migrations/0003_notification_deliveries.sql	Adds `routed_at`, delivery tables, indexes, and CDC triggers.
backend/internal/storage/sqlite/gen/querier.go	Extends sqlc querier interface with delivery and routing methods.
backend/internal/storage/sqlite/gen/notifications.sql.go	Regenerated sqlc code including `routed_at` scans/returns.
backend/internal/storage/sqlite/gen/notification_deliveries.sql.go	New sqlc-generated code for delivery + routing queries.
backend/internal/storage/sqlite/gen/models.go	Adds sqlc models for `NotificationDelivery` (+ attempt model).
backend/internal/notification/store.go	Defines notifier runtime durable store interface (routing + deliveries).
backend/internal/notification/settings.go	Adds settings provider + normalization/defaulting behavior.
backend/internal/notification/settings_test.go	Tests defaulting and clone/immutability behavior for settings provider.
backend/internal/notification/routing.go	Implements priority→route resolution for dashboard/desktop + unknown route skipping.
backend/internal/notification/routing_test.go	Tests routing decisions for defaults, disables, and unknown routes.
backend/internal/notification/retry.go	Implements retry policy (backoff + jitter) and error classification.
backend/internal/notification/retry_test.go	Tests backoff caps, jitter bounds, and retry classification logic.
backend/internal/notification/manager.go	Adds manager orchestration for routing + AO-app delivery claiming and completion APIs.
backend/internal/notification/enqueuer.go	Renames enqueuer store interface to avoid collision with runtime store.
backend/internal/notification/dispatcher.go	Adds background dispatcher loop that ticks `Manager.RunOnce`.
backend/internal/notification/dispatcher_test.go	Tests dispatcher loop behavior and routing/error handling interactions.
backend/internal/notification/delivery.go	Defines delivery row/status model, ID generation, and normalization helpers.
backend/internal/integration/notification_runtime_test.go	Integration test verifying desktop deliveries are created only for eligible priorities.
backend/internal/domain/notification.go	Adds `RoutedAt` to the domain notification model.
backend/internal/config/config.go	Adds notification config types and safe default notification configuration.
backend/internal/config/config_test.go	Verifies notification defaults are populated by config load.
backend/internal/cdc/event.go	Adds CDC event types for delivery created/updated.

Files not reviewed (4)

backend/internal/storage/sqlite/gen/models.go: Language not supported
backend/internal/storage/sqlite/gen/notification_deliveries.sql.go: Language not supported
backend/internal/storage/sqlite/gen/notifications.sql.go: Language not supported
backend/internal/storage/sqlite/gen/querier.go: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 25 out of 29 changed files in this pull request and generated 1 comment.

Files not reviewed (4)

backend/internal/storage/sqlite/gen/models.go: Language not supported
backend/internal/storage/sqlite/gen/notification_deliveries.sql.go: Language not supported
backend/internal/storage/sqlite/gen/notifications.sql.go: Language not supported
backend/internal/storage/sqlite/gen/querier.go: Language not supported

Copilot

Pull request overview

Copilot reviewed 25 out of 29 changed files in this pull request and generated 1 comment.

Files not reviewed (4)

backend/internal/storage/sqlite/gen/models.go: Language not supported
backend/internal/storage/sqlite/gen/notification_deliveries.sql.go: Language not supported
backend/internal/storage/sqlite/gen/notifications.sql.go: Language not supported
backend/internal/storage/sqlite/gen/querier.go: Language not supported

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

AgentWrapper · 2026-05-31T23:03:50Z

-	EventPRCheckRecorded     EventType = "pr_check_recorded"
-	EventNotificationCreated EventType = "notification_created"
-	EventNotificationUpdated EventType = "notification_updated"
+	EventSessionCreated              EventType = "session_created"


these sound like notification events. Can we decouple them from CDC?
For notifications we should simply create new entries in the notifications table.

AgentWrapper · 2026-05-31T23:07:28Z

+// NotificationConfig contains the global notification settings used by the
+// central notifier runtime. It intentionally starts global (not per-project) so
+// the routing model can grow without changing lifecycle reactions.
+type NotificationConfig struct {


all our configs need to be stored in sqlite now.

Can we add a new settings table in sqlite instead and have a single boolean called enable_notifications

AgentWrapper · 2026-05-31T23:08:19Z

+	Dashboard DashboardNotificationConfig
+	Desktop   DesktopNotificationConfig
+	Routing   NotificationRoutingConfig
+	Retry     NotificationRetryConfig


let's not add any retries before notifications are completely operational

AgentWrapper · 2026-05-31T23:08:41Z

+
+type DesktopNotificationConfig struct {
+	Enabled         bool
+	Priorities      []ports.Priority


let's remove priorities altogether for now

AgentWrapper · 2026-05-31T23:11:50Z

+	Priorities map[ports.Priority][]string
+}
+
+type NotificationRetryConfig struct {


remove all logic related to retries

AgentWrapper · 2026-05-31T23:12:50Z

+type NotificationRoutingConfig struct {
+	// Priorities maps notification priority to built-in route names. The
+	// notifier currently implements dashboard and desktop only.
+	Priorities map[ports.Priority][]string


why don't we have a priority level instead of the config having a list of priorities

Is there any case when a user will not want high priority notifs but want low priority notifs

greptile-apps Bot reviewed May 31, 2026

View reviewed changes

Comment thread backend/internal/notification/manager.go Outdated

Comment thread backend/internal/storage/sqlite/notification_delivery_store.go Outdated

Comment thread backend/internal/storage/sqlite/migrations/0003_notification_deliveries.sql

whoisasx commented May 31, 2026

View reviewed changes

Comment thread backend/internal/storage/sqlite/migrations/0003_notification_deliveries.sql

Comment thread backend/internal/storage/sqlite/notification_delivery_store.go Outdated

Comment thread backend/internal/storage/sqlite/notification_delivery_store.go Outdated

whoisasx requested a review from Copilot May 31, 2026 21:32

Copilot started reviewing on behalf of whoisasx May 31, 2026 21:32 View session

Copilot AI reviewed May 31, 2026

View reviewed changes

Comment thread backend/internal/notification/retry.go

Comment thread backend/internal/notification/settings.go

Comment thread backend/internal/storage/sqlite/notification_delivery_store.go Outdated

Comment thread backend/notifier_wiring.go Outdated

whoisasx requested a review from Copilot May 31, 2026 21:47

Copilot started reviewing on behalf of whoisasx May 31, 2026 21:47 View session

Copilot AI reviewed May 31, 2026

View reviewed changes

Comment thread backend/internal/storage/sqlite/migrations/0003_notification_deliveries.sql

whoisasx requested a review from Copilot May 31, 2026 21:58

Copilot started reviewing on behalf of whoisasx May 31, 2026 21:59 View session

Copilot AI reviewed May 31, 2026

View reviewed changes

Comment thread backend/internal/notification/settings.go Outdated

whoisasx added 6 commits June 1, 2026 03:47

feat: add notifier delivery runtime

217f6b1

fix: address notifier delivery review feedback

d4622fe

fix: harden notification delivery leases

041c8c8

fix: address notifier review cleanup

d39e8e0

docs: clarify notification routing migration

f0c57ac

fix: preserve notification surface disables

5c07e81

whoisasx force-pushed the feat/55 branch from c33ffb0 to 5c07e81 Compare May 31, 2026 22:19

whoisasx requested a review from Copilot May 31, 2026 22:28

Copilot started reviewing on behalf of whoisasx May 31, 2026 22:28 View session

Copilot AI reviewed May 31, 2026

View reviewed changes

whoisasx requested a review from Copilot May 31, 2026 22:53

Copilot started reviewing on behalf of whoisasx May 31, 2026 22:54 View session

Copilot AI reviewed May 31, 2026

View reviewed changes

whoisasx merged commit d06c0ce into main May 31, 2026
6 of 8 checks passed

AgentWrapper reviewed May 31, 2026

View reviewed changes

AgentWrapper mentioned this pull request May 31, 2026

Revert "feat: add notifier delivery runtime" #63

Merged

Conversation

whoisasx commented May 31, 2026

Summary

Tests

Uh oh!

greptile-apps Bot commented May 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

Uh oh!

Uh oh!

whoisasx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AgentWrapper May 31, 2026

Choose a reason for hiding this comment

Uh oh!

AgentWrapper May 31, 2026

Choose a reason for hiding this comment

Uh oh!

AgentWrapper May 31, 2026

Choose a reason for hiding this comment

Uh oh!

AgentWrapper May 31, 2026

Choose a reason for hiding this comment

Uh oh!

AgentWrapper May 31, 2026

Choose a reason for hiding this comment

Uh oh!

AgentWrapper May 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greptile-apps Bot commented May 31, 2026 •

edited

Loading