notifications (2/5): notification_history keyset listing

[illegalprime]

Stack 2/5 — base: eden/notifications-1-proto (#452)

1. Summary

Adds the read/list path for notification history. Background already on main: an Alertmanager webhook receiver persists each fired-alert notification into the notification_history table. This PR adds the ability to page through that history newest-first, so an operator can review what alerts fired for their organization. It delivers the DB/query/store layer only — a ListNotificationHistory SQL query, a supporting index, and a List store method using keyset pagination. The gRPC handler that calls this arrives in #455.

2. How it works

The flow is a single-page read keyed off the caller's organization and an optional cursor:

1. A caller (the HistoryService handler landing in notifications (4/5): Grafana-backed service, handlers, and server wiring #455) invokes store.List(ctx, organizationID, beforeID, limit). beforeID is nil for the first page; for subsequent pages the caller passes the id of the last row it already received.
2. List maps the Go arguments into sqlc query params (organizationID → NullInt64, beforeID → nullable cursor, limit → page size) and calls the generated ListNotificationHistory.
3. The SQL selects from notification_history filtered to the organization, with id < before_id applied only when a cursor is present, ordered id DESC, capped by LIMIT. It LEFT JOINs the device and discovered_device tables to resolve device_id into a human-readable device name and MAC at read time (falling back to '' when the device is unknown or soft-deleted).
4. Postgres serves the ordering and seek from the new (organization_id, id DESC) index rather than scanning.
5. List converts each generated row into a StoredNotification (the persisted Notification plus row identity ID, ReceivedAt, and the read-time DeviceName/DeviceMAC) and returns the slice.

Keyset (cursor) pagination: instead of OFFSET, each next page seeks past the last seen id (id < before_id) and reads the next LIMIT rows in id DESC order. Because notification_history is insert-only, ids are monotonic and stable, so pages never shift or duplicate as new rows arrive. The handler in #455 owns page-size policy (default 50, max 200); this layer just honors the limit it is given.

3. Diagrams

Component / data flow (boundaries this PR touches are bold in prose; the handler is in #455):

flowchart LR
    H["HistoryService handler (#455)"] -->|"List(orgID, beforeID, limit)"| S["SQLNotificationHistoryStore.List"]
    S -->|"ListNotificationHistoryParams"| Q["sqlc: ListNotificationHistory (generated)"]
    Q -->|"SELECT ... WHERE org_id=? AND id<? ORDER BY id DESC LIMIT ?"| PG[("Postgres: notification_history")]
    PG -. "index scan" .-> IDX["idx_notification_history_org_id (organization_id, id DESC)"]
    Q -.->|"LEFT JOIN"| DEV[("device / discovered_device")]
    PG -->|"rows"| S
    S -->|"[]StoredNotification"| H

Loading

Keyset pagination over an insert-only table (ordering by id DESC, seeking past the cursor):

sequenceDiagram
    participant C as Caller (#455)
    participant L as Store.List
    participant DB as Postgres
    C->>L: List(org, beforeID=nil, limit=50)
    L->>DB: WHERE org_id=org ORDER BY id DESC LIMIT 50
    DB-->>L: rows [id=900 .. 851]
    L-->>C: page 1 (last id = 851)
    C->>L: List(org, beforeID=851, limit=50)
    L->>DB: WHERE org_id=org AND id < 851 ORDER BY id DESC LIMIT 50
    DB-->>L: rows [id=850 .. 801]
    L-->>C: page 2 (last id = 801)
    Note over C,DB: New inserts get higher ids, already-paged rows never shift.

Loading

4. Areas of the code involved

Area / package / file	What changed	Why it matters for review
server/sqlc/queries/notification_history.sql	New query ListNotificationHistory: org filter, optional before_id cursor, id DESC, LIMIT, device-enrichment joins	Source of truth for the read path — review the SQL, cursor predicate, join correctness, and '' fallbacks here
server/internal/domain/notificationhistory/notificationhistory.go	New StoredNotification read-back type and Lister interface (List with beforeID *int64 cursor)	Defines the domain contract the handler (#455) codes against
server/internal/domain/stores/sqlstores/notification_history.go	New List method on the SQL store; asserts Lister is implemented; maps params and rows	Mapping layer between domain and sqlc; check cursor passthrough and row→StoredNotification conversion (helpers ptrToNullInt64/nullInt64ToPtr/nullTimeToPtr are pre-existing)
server/migrations/000087_notification_history_keyset_index.up.sql	New migration: CREATE INDEX CONCURRENTLY (organization_id, id DESC)	Backs the access pattern; review the CONCURRENTLY rationale and recovery note
server/migrations/000087_notification_history_keyset_index.down.sql	New DROP INDEX CONCURRENTLY IF EXISTS	Matching online rollback
server/generated/sqlc/notification_history.sql.go	Generated ListNotificationHistory query, params, row, scanner	Generated — skip
server/generated/sqlc/db.go	Generated prepared-statement registration/teardown for the new query	Generated — skip

5. Key technical decisions & trade-offs

• Keyset pagination (id < before_id, id DESC) over OFFSET/LIMIT — stable on an insert-only table (no page drift/skew as rows arrive) and index-friendly; cost is the caller must thread the last id as a cursor rather than a page number.
• CREATE INDEX CONCURRENTLY over a plain CREATE INDEX — a plain build takes ACCESS EXCLUSIVE on the live table and blocks webhook inserts for the build duration; CONCURRENTLY builds online (matches the convention in migration 000074). Trade-off: must be the sole statement and cannot run in a transaction, and a failed build can leave a dirty migration + an INVALID index (recovery noted inline in the migration).
• Migration numbered 000087 so it merges before the 000088 permission seed in notifications (3/5): notification:read and notification:manage permissions #454 — guarantees schema_migrations advances in numeric = merge order, avoiding golang-migrate skipping a lower-numbered migration on already-upgraded DBs.
• Device name/MAC resolved at read time via LEFT JOIN rather than denormalized onto notification_history at insert — names stay current and deleted/unknown devices degrade to ''; cost is a join per page (served by existing device keys).
• Lister as a separate interface from the existing Store (insert) interface — read and write capabilities are segregated so the notifications (4/5): Grafana-backed service, handlers, and server wiring #455 handler can depend only on the read surface.

6. Testing & validation

• just gen re-run so server/generated/sqlc matches the committed query (the generated-code CI check enforces a clean tree).
• The SQL compiles under sqlc; the store satisfies both notificationhistory.Store and notificationhistory.Lister via compile-time var _ = (*...) assertions.
• Not covered in this PR: no unit/integration test exercises List against a live DB, and there is no handler/end-to-end coverage — the gRPC HistoryService that calls List (and enforces the page-size clamp of default 50 / max 200) lands in notifications (4/5): Grafana-backed service, handlers, and server wiring #455, where request-level validation and tests belong. The CONCURRENTLY migration's online behavior is asserted by convention (matching 000074), not by an automated deploy test.

🤖 Generated with Claude Code

[github-actions]

🔐 Codex Security Review

Note: This is an automated security-focused code review generated by Codex.
It should be used as a supplementary check alongside human review.
False positives are possible - use your judgment.

Scope summary

• Reviewed pull request diff only (591590e960533ecb778b9d03032f5c9dd16f1fba...6f51c39065b22e590ba4fc5effa6f3b0efcea698, exact PR three-dot diff)
• Model: gpt-5.5

💡 Click "edited" above to see previous reviews for this PR.

---

Review Summary

Overall Risk: MEDIUM

Findings

[MEDIUM] Unbounded notification-history page limit reaches SQL

• Category: Reliability
• Location: server/internal/domain/stores/sqlstores/notification_history.go:64
• Description: The new List method accepts limit int32 and passes it directly into ListNotificationHistory as PageLimit; the SQL query then uses it directly as LIMIT page_limit. This new store boundary does not apply a default, reject negative values, or cap oversized values, unlike nearby pagination paths such as activity, diagnostics, fleetmanagement, collections, and curtailment.
• Impact: If this is wired directly to ListNotificationsRequest.page_size, an authenticated caller can request a very large page from a growing notification_history table, forcing a large index scan and causing sqlc to materialize all returned rows into memory before the store maps them. That can degrade DB/server availability or cause memory pressure.
• Recommendation: Normalize page size before calling SQL, preferably at the domain/store boundary as defense in depth: default <= 0 to a small value, cap to a fixed max such as 100 or 1000, and fetch pageSize + 1 only after applying the cap when computing has_more. Add tests for zero, negative, and oversized limits.

Notes

The authoritative diff only changed notification-history sqlc/store/query/index files. I did not see changed auth handlers, Connect-RPC wiring, frontend rendering, plugin logic, command execution, network discovery, pool configuration, or protobuf field definitions in .git/codex-review.diff.

No SQL injection, command injection, cryptostealing/pool hijack, or cross-org data exposure issue was evident in the changed hunks. This was a static review; I did not run tests.

---

_{Generated by Codex Security Review |
Triggered by: @illegalprime |
Review workflow run}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 923f5e3745

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[illegalprime]

Codex findings on this PR:

• [LOW] CREATE INDEX blocks writes — fixed here (commit d9857a9): now CREATE INDEX CONCURRENTLY (+ DROP ... CONCURRENTLY in the down migration), matching the 000074 convention. golang-migrate's postgres driver runs statements without an implicit transaction, so this is safe.
• [MEDIUM] unbounded page limit — resolved in notifications (4/5): Grafana-backed service, handlers, and server wiring #455: the HistoryService handler clamps pageSize (historyDefaultPageSize=50, historyMaxPageSize=200) before calling the store, which is the only caller. No change needed at the store layer.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d9857a968f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[illegalprime]

codex-connector triage (all fixed):

• [P1] Use the next sequential migration number — the "missing 000087" was a split artifact (it lived in the next PR), but the connector was right that the ordering was wrong: the index was 000088 while the seed was 000087 in a later-merging PR. Renumbered the index to 000087 here (merges first); seed becomes 000088 in notifications (3/5): notification:read and notification:manage permissions #454.
• [P2] Build the index concurrently — fixed: CREATE INDEX CONCURRENTLY (+ concurrent down).
• [P2] Regenerate sqlc — fixed: re-ran sqlc generate; the stale ListNotificationHistory comment is now in sync.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 607669536a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[ankitgoswami]

approving based on the PR description