Conversation
Greptile SummaryThis PR introduces the full notification service foundation: domain vocabulary, central
Confidence Score: 4/5The notification pipeline is well-structured and well-tested, but UpsertNotification's content-update path overwrites status/read_at/dismissed_at with always-unread values, silently discarding user interaction state whenever enriched facts evolve. The user read/dismissed interaction state is reset on every content update in UpsertNotification: only ID and CreatedAt are carried forward from the existing row, while Status, ReadAt, and DismissedAt are overwritten from the always-unread incoming notification. A user who dismisses a CI-failing notification will see it resurface unread the next time enriched facts change. backend/internal/storage/sqlite/store/notification_store.go — the update branch in UpsertNotification needs to copy Status, ReadAt, and DismissedAt from the existing row before calling notificationUpdateParams. Important Files Changed
Sequence DiagramsequenceDiagram
participant LCM as lifecycle.Manager
participant Svc as notification.Service
participant Enrich as enrich (local store)
participant Maker as DefaultMaker
participant Store as sqlite.Store
participant DB as SQLite (triggers)
LCM->>LCM: mutateRecord (state change)
LCM->>Svc: notifyBestEffort(NotificationIntent)
Svc->>Svc: intent.Validate()
Svc->>Enrich: enrich(intent) to EnrichedFacts
Enrich->>Store: GetSession / GetProject / ListPRsBySession
Enrich->>Store: ListChecks / ListPRComments / ListPRReviewThreads
Svc->>Svc: buildActions(intent, facts)
Svc->>Maker: Make(MakeInput) to NotificationContent
Svc->>Svc: fingerprint(intent, facts, actions, content)
Svc->>Store: UpsertNotification(Notification)
Store->>DB: GetNotificationByDedupeKey
alt fingerprint unchanged
DB-->>Store: "no-op changed=false"
else new or changed fingerprint
DB->>DB: INSERT or UPDATE notifications
DB->>DB: CDC trigger insert change_log
DB-->>Store: "changed=true"
end
Store-->>Svc: stored, changed, err
alt MergeCompleted
Svc->>Store: ResolveNotifications
Store->>DB: UPDATE notifications SET status resolved
DB->>DB: CDC trigger notification_updated
end
Svc-->>LCM: err best-effort logged on failure
Reviews (7): Last reviewed commit: "fix: make lifecycle notifications best e..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Adds the backend “notification foundation” described in #140 by introducing domain-level notification vocabulary, a central notification service that enriches intents into canonical persisted notifications (with durable fingerprint dedupe), and a SQLite-backed persistence + trigger-owned CDC stream. This integrates notifications into lifecycle decision points (CI failing, review feedback, merge conflicts/ready/completed, session waiting input/exited) while preserving existing agent nudge behavior.
Changes:
- Add
domain.NotificationIntent→service/notificationpipeline (enrich → actions → maker copy → fingerprint → persist/upsert). - Add
notificationsSQLite table, sqlc queries, store methods, and CDC triggers/events (notification_created,notification_updated). - Wire lifecycle + daemon startup to emit notification intents and persist them via the notification service.
Reviewed changes
Copilot reviewed 27 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/sqlc.yaml | Adds sqlc type overrides for notifications.* columns to use domain types. |
| backend/internal/storage/sqlite/store/notification_store.go | Implements notification upsert/dedupe, list/get, and resolve-by-filter APIs. |
| backend/internal/storage/sqlite/store/notification_store_test.go | Adds store-level tests for notification CRUD, dedupe update, and resolve filtering. |
| backend/internal/storage/sqlite/store/notification_cdc_test.go | Verifies trigger-owned CDC behavior and payload JSON validity for notifications. |
| backend/internal/storage/sqlite/queries/pr.sql | Extends PR selects to include last_nudge_signature. |
| backend/internal/storage/sqlite/queries/notifications.sql | Introduces sqlc queries for inserting/updating/reading/listing/resolving notifications. |
| backend/internal/storage/sqlite/migrations/0007_notifications.sql | Adds notifications table and extends change_log + trigger set for notification CDC. |
| backend/internal/storage/sqlite/gen/pr.sql.go | Regenerates PR queries, adds GetPRLastNudgeSignature/UpdatePRLastNudgeSignature, includes last_nudge_signature in scans. |
| backend/internal/storage/sqlite/gen/notifications.sql.go | Generated sqlc layer for notification queries/models. |
| backend/internal/storage/sqlite/gen/models.go | Adds generated Notification model and extends PR with LastNudgeSignature. |
| backend/internal/service/pr/manager.go | Extends PR write mapping to include more PR metadata from observations. |
| backend/internal/service/notification/store.go | Defines notification service’s storage boundary (durable-facts, no network calls, trigger CDC). |
| backend/internal/service/notification/service.go | Adds notification service orchestrating validate→enrich→actions→maker→fingerprint→persist (+ resolve superseded on merge). |
| backend/internal/service/notification/service_test.go | Tests notify pipeline, dedupe behavior, fallback behavior, error paths, and merge-completed resolution. |
| backend/internal/service/notification/maker.go | Adds Maker interface and DefaultMaker for concise fallback copy and truncation. |
| backend/internal/service/notification/maker_test.go | Validates default copy for all V1 types and ensures evidence doesn’t leak into summary. |
| backend/internal/service/notification/enrich.go | Implements enrichment from local durable facts and builds structured Data/Subject. |
| backend/internal/service/notification/dedupe.go | Implements fingerprint hashing for durable dedupe decisions. |
| backend/internal/service/notification/actions.go | Builds semantic action descriptors (route/link/callback) from intent + facts. |
| backend/internal/ports/pr_observations.go | Extends PR observation DTO to include timestamps/SHAs/URLs and review thread IDs. |
| backend/internal/lifecycle/reactions.go | Emits notification intents at lifecycle PR decision points and projects SCM observations into PR observations. |
| backend/internal/lifecycle/manager.go | Adds optional notification sink wiring via Deps / NewWithDeps, and emits session input/exited notifications. |
| backend/internal/lifecycle/manager_test.go | Adds tests asserting lifecycle emits notification intents and handles sink errors/no-op behavior. |
| backend/internal/integration/lifecycle_sqlite_test.go | Wires notification service into integration stack and verifies persistence + CDC broadcast. |
| backend/internal/daemon/wiring_test.go | Verifies daemon wiring injects notification sink into lifecycle and persists notifications. |
| backend/internal/daemon/lifecycle_wiring.go | Extends lifecycle stack wiring to accept a notification sink and pass it to LCM. |
| backend/internal/daemon/daemon.go | Instantiates notification service and wires it into lifecycle at daemon start. |
| backend/internal/cdc/event.go | Adds notification CDC event types. |
| backend/internal/domain/notification.go | Adds notification domain types, validation, normalization, and resolve filter. |
| backend/internal/domain/notification_test.go | Adds validation and constant mapping tests for the notification domain. |
Files not reviewed (3)
- backend/internal/storage/sqlite/gen/models.go: Language not supported
- backend/internal/storage/sqlite/gen/notifications.sql.go: Language not supported
- backend/internal/storage/sqlite/gen/pr.sql.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
the notification flow should not be allowed to break lifecycle updates right? LCM is responsible for recording important session facts like activity termination, and PR state, so those updates must still succeed even if creating a notification fails. if Notify(ctx, intent) returns an error, LCM should log or track it, but it should not block the main lifecycle change. Notifications can be retried or deduplicated later. |
|
@Vaibhaav-Tiwari addressed both points in 686f3fd.
Validation run: |
Closes #140
Summary
Tests