Skip to content

feat: add durable notification foundation#57

Merged
whoisasx merged 1 commit into
mainfrom
feat/54
May 31, 2026
Merged

feat: add durable notification foundation#57
whoisasx merged 1 commit into
mainfrom
feat/54

Conversation

@whoisasx

Copy link
Copy Markdown
Collaborator

Closes #54\n\n## Summary\n- Add provider-neutral notification domain, renderer, dedupe, and store-backed enqueuer\n- Add SQLite notifications table, idempotent enqueue/read/archive APIs, CDC triggers, and sqlc-generated queries\n- Wire lifecycle notifications to durable persistence and add lifecycle/storage/renderer/dedupe tests\n\n## Validation\n- cd backend && go test ./...\n- cd backend && go vet ./...

@whoisasx whoisasx requested a review from Copilot May 31, 2026 18:40
@whoisasx whoisasx requested review from illegalcall and removed request for Copilot May 31, 2026 18:40
@greptile-apps

greptile-apps Bot commented May 31, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR wires a complete durable notification foundation into the lifecycle loop: a SQLite-backed notifications table with goose migration, sqlc-generated queries, CDC triggers, a provider-neutral renderer, a dedupe engine, and a store-backed enqueuer that replaces the noopNotifier stub.

  • Storage layer: adds notifications table with JSON-validity constraints, a partial index for unread rows, and CDC triggers that emit notification_created / notification_updated events only on meaningful state transitions (insert or read_at/archived_at changes).
  • Notification domain: adds Renderer (rehydrates session + PR facts to build a rich payload), Enqueuer (implements ports.Notifier), and ComputeDedupeKey (restart-safe hash over lifecycle condition vectors); lifecycle.reactions is updated to pass Reaction/Escalation context through ports.Event.
  • Test coverage: unit tests for renderer, dedupe, enqueuer, and store; integration tests verify end-to-end enqueue and CDC emission for NeedsInput, ApprovedAndGreen, and PRMerged lifecycle paths.

Confidence Score: 4/5

The new notification pipeline is well-structured and the idempotency/dedupe design is sound, but the filter switch in ListNotifications silently discards the session/project dimension whenever UnreadOnly is set, which will produce wrong results the first time an API handler uses both together.

The overall design is solid — migration, triggers, renderer, dedupe, and enqueuer all hold together, and concurrent enqueue is protected by the write mutex with correct ErrNoRows handling. The one behaviour gap is ListNotifications: the switch gives UnreadOnly unconditional priority over SessionID / ProjectID, so a caller expecting unread notifications for session X silently gets all unread notifications. No current external endpoint exercises the combined filter, but the NotificationFilter struct invites it and there is no guard or documentation warning against it.

backend/internal/storage/sqlite/notification_store.go (ListNotifications filter precedence); backend/internal/notification/renderer.go (IsBehind field never populated)

Important Files Changed

Filename Overview
backend/internal/storage/sqlite/notification_store.go Core notification persistence layer — idempotent enqueue via ON CONFLICT DO NOTHING + dedupe-key fallback read, read/unread/archive lifecycle with CDC, and a filter switch that silently ignores session/project when UnreadOnly is also set
backend/internal/notification/renderer.go Converts lifecycle events to durable domain.Notification rows; MergePayload.IsBehind is defined in the schema but never populated here
backend/internal/notification/dedupe.go Deterministic dedupe key derivation using SHA-256 (truncated to 16 bytes); restart-safe design avoids PR updated_at timestamp to prevent duplicates on re-poll
backend/internal/storage/sqlite/migrations/0002_notifications.sql Adds notifications table with JSON-validity constraints, partial index for unread, and CDC triggers for insert and selective update (read_at/archived_at changes only)
backend/internal/notification/enqueuer.go Thin store-backed ports.Notifier that delegates to Renderer then EnqueueNotification; correctly compiles against the interface
backend/internal/lifecycle/reactions.go Adds Reaction/Escalation context to fireNotify and escalate calls; TickEscalations now captures attempt count and duration for escalation events
backend/internal/ports/outbound.go Extends Event with Reaction, Escalation, DedupeKey, CauseKey, OccurredAt; adds PriorityWarning constant
backend/internal/storage/sqlite/pr_facts.go New PRFactsForSession helper picks the active PR (first open/draft) and folds in unresolved review comments; extracted from lifecycle to support renderer rehydration
backend/internal/integration/lifecycle_sqlite_test.go Adds three end-to-end durable notification tests (NeedsInput, ApprovedAndGreen, PRMerged) plus CDC assertion helper; good coverage of the full enqueue path
backend/lifecycle_wiring.go Replaces noopNotifier with live Renderer+Enqueuer; removes the TODO stub comment appropriately
backend/internal/storage/sqlite/gen/notifications.sql.go sqlc-generated query implementations; InsertNotification uses ON CONFLICT DO NOTHING which returns sql.ErrNoRows on duplicate, correctly handled in notification_store.go

Sequence Diagram

sequenceDiagram
    participant LCM as lifecycle.Manager
    participant Enqueuer as notification.Enqueuer
    participant Renderer as notification.Renderer
    participant Store as sqlite.Store
    participant CDC as change_log (trigger)

    LCM->>Enqueuer: "Notify(ctx, ports.Event{Reaction, Escalation, CauseKey})"
    Enqueuer->>Renderer: Render(ctx, event)
    Renderer->>Store: GetSession(ctx, sessionID)
    Store-->>Renderer: SessionRecord
    Renderer->>Store: PRFactsForSession(ctx, sessionID)
    Store-->>Renderer: PRFacts
    Renderer-->>Enqueuer: domain.Notification (with DedupeKey, Payload JSON)
    Enqueuer->>Store: EnqueueNotification(ctx, row)
    Store->>Store: INSERT … ON CONFLICT (dedupe_key) DO NOTHING
    alt new row inserted
        Store-->>Enqueuer: "(row, created=true, nil)"
        Store->>CDC: AFTER INSERT trigger → notification_created
    else duplicate dedupe_key
        Store->>Store: GetNotificationByDedupeKey(dedupe_key)
        Store-->>Enqueuer: "(existing row, created=false, nil)"
    end
    Enqueuer-->>LCM: nil
Loading

Comments Outside Diff (2)

  1. backend/internal/storage/sqlite/notification_store.go, line 1807-1816 (link)

    P1 UnreadOnly silently ignores SessionID / ProjectID

    The switch evaluates filter.UnreadOnly first, so a caller that passes {UnreadOnly: true, SessionID: "x"} gets all unread notifications across every session instead of only the ones for session "x". The missing combination has no test coverage and the doc-comment on NotificationFilter doesn't flag the restriction. Future API handlers are likely to pass both fields expecting intersection semantics.

  2. backend/internal/notification/renderer.go, line 824-831 (link)

    P2 IsBehind field is defined but never set

    MergePayload declares an isBehind JSON field, and the migration schema exposes it to downstream consumers, but mergePayload() only populates Ready and Conflicts. Any client reading isBehind will always receive null even when the PR is genuinely behind its base branch. If the data isn't available yet, removing the field (or adding a comment marking it as a future placeholder) would prevent silent misuse.

Reviews (1): Last reviewed commit: "feat: add durable notification foundatio..." | Re-trigger Greptile

@whoisasx

Copy link
Copy Markdown
Collaborator Author

@copilot

@whoisasx whoisasx requested review from AgentWrapper and illegalcall and removed request for AgentWrapper and illegalcall May 31, 2026 18:46
@whoisasx whoisasx merged commit 83d1ea1 into main May 31, 2026
4 checks passed
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.

Notifier foundation: durable notification domain, storage, CDC, and lifecycle enqueue

1 participant