Skip to content

feat: add notification service foundation#141

Open
whoisasx wants to merge 5 commits into
mainfrom
feat/140
Open

feat: add notification service foundation#141
whoisasx wants to merge 5 commits into
mainfrom
feat/140

Conversation

@whoisasx
Copy link
Copy Markdown
Collaborator

@whoisasx whoisasx commented Jun 6, 2026

Closes #140

Summary

  • Add notification domain vocabulary, central notification service, maker, enrichment, semantic actions, and durable fingerprint dedupe.
  • Wire lifecycle PR/activity/runtime decisions to emit notification intents while preserving agent nudge behavior.
  • Add SQLite notification persistence, sqlc queries/store methods, trigger-owned notification CDC, and daemon wiring.

Tests

  • npm run sqlc
  • cd backend && go test ./internal/domain ./internal/lifecycle ./internal/service/notification ./internal/storage/sqlite/store ./internal/integration ./internal/daemon
  • npm run lint

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 6, 2026

Greptile Summary

This PR introduces the full notification service foundation: domain vocabulary, central Service (enrich → make → fingerprint → upsert), DefaultMaker, semantic Actions, SHA-256 fingerprint dedup, and SQLite persistence with CDC triggers. Lifecycle PR/activity/runtime decisions are wired to emit NotificationIntent values while preserving the existing agent-nudge path.

  • Domain layer (domain/notification.go): type-safe NotificationIntent, Notification, NotificationResolveFilter, and Validate/Normalize helpers. Validated intent pipeline prevents malformed rows from reaching the store.
  • Notification service (service/notification/): validates intent, enriches from local durable facts (session, project, PR, checks, comments, threads), builds semantic action descriptors, computes a stable SHA-256 fingerprint, and calls UpsertNotification; MergeCompleted automatically resolves superseded PR notifications.
  • Storage (store/notification_store.go, migrations/0008_notifications.sql): fingerprint-based no-op dedup, LIKE-safe escapeLikePrefix, and DB-trigger-owned CDC for notification_created / notification_updated events.

Confidence Score: 4/5

The 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

Filename Overview
backend/internal/domain/notification.go New file defining the full notification domain model, intent/context types, constants, and domain-level Validate/Normalize helpers. Well-structured with type-safe IDs and JSON-safe validation.
backend/internal/lifecycle/reactions.go Wires PR/activity/runtime lifecycle decisions into NotificationIntent emissions. Preserves agent-nudge suppression for waiting-input sessions. boundedString truncates at byte level; CI loop still exits after the first failing check (pre-existing shape, flagged separately).
backend/internal/lifecycle/manager.go Adds optional notificationSink dependency via NewWithDeps; splits mutate into mutateRecord to expose (record, changed) for post-mutation intent building. Best-effort pattern correctly guards nil notifications sink.
backend/internal/service/notification/service.go Central notification service: validates intent, enriches from local store, builds actions and canonical copy, fingerprints, and delegates to UpsertNotification. MergeCompleted path resolves superseded PR notifications.
backend/internal/storage/sqlite/store/notification_store.go Implements UpsertNotification, ResolveNotifications, list/get accessors. Fingerprint-based no-op dedup and LIKE wildcard escaping are correct; however UpdateNotificationContent overwrites status/read_at/dismissed_at from the incoming (always-unread) notification, losing user interaction state on content refresh.
backend/internal/service/notification/enrich.go choosePR correctly returns nil when a specific PR URL is requested but not found (regression from prior review fixed). bounded helper has the same byte-level truncation issue as boundedString in reactions.go.
backend/internal/storage/sqlite/migrations/0008_notifications.sql Adds notifications table with appropriate indexes, recreates change_log with new event types, and adds CDC triggers for insert/update. Down migration cleanly removes the new event types and table.
backend/internal/service/notification/dedupe.go SHA-256 fingerprint over sorted, stable fields. Action and failed-check slices are sorted before hashing, ensuring order-independence across repeated enrichments.
backend/internal/service/notification/maker.go DefaultMaker produces concise channel-neutral copy for all V1 types. Body intentionally mirrors Summary per the clarifying comment added in a prior review iteration. truncateRunes correctly handles UTF-8.

Sequence Diagram

sequenceDiagram
    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
Loading

Reviews (7): Last reviewed commit: "fix: make lifecycle notifications best e..." | Re-trigger Greptile

Comment thread backend/internal/storage/sqlite/store/notification_store.go
Comment thread backend/internal/service/notification/maker.go
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.NotificationIntentservice/notification pipeline (enrich → actions → maker copy → fingerprint → persist/upsert).
  • Add notifications SQLite 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.

Comment thread backend/internal/lifecycle/reactions.go
Comment thread backend/internal/lifecycle/reactions.go Outdated
Comment thread backend/internal/service/notification/enrich.go
@Vaibhaav-Tiwari
Copy link
Copy Markdown
Collaborator

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.
The intent model should match the canonical notification model around session scope. The canonical Notification allows SessionID to be optional, but NotificationIntent currently requires a SessionID. That is fine for the first session-based notifications, but it will not work cleanly for future project-level or global notifications. Either V1 should clearly state that all intents are session-scoped, or NotificationIntent should also allow an optional session ID

@whoisasx
Copy link
Copy Markdown
Collaborator Author

whoisasx commented Jun 7, 2026

@Vaibhaav-Tiwari addressed both points in 686f3fd.

  • Notification emission from LCM is now best-effort: Notify errors are logged with the notification type/source/dedupe key, but lifecycle facts and existing agent nudge behavior continue. In particular, merged PRs still terminate sessions even if the merge.completed notification fails.
  • Agreed on session scope: notifications are now session-scoped end-to-end. domain.Notification.SessionID is required, notifications.session_id is NOT NULL, and sqlc/store/service mappings use a non-pointer session id. I left NotificationResolveFilter.SessionID optional only as a query filter.

Validation run: npm run sqlc, targeted backend tests, and npm run lint.

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.

Implement LCM intent-driven notification service foundation

4 participants