notifications (2/5): notification_history keyset listing by illegalprime · Pull Request #453 · block/proto-fleet

illegalprime · 2026-06-15T15:53:33Z

Stack 2/5 — base: `eden/notifications-1-proto`

Adds the read/list side of notification history on top of the webhook-driven persistence already on `main`.

`ListNotifications` sqlc query + keyset index on `notification_history(id)` for stable id-based pagination
`Lister` interface and `StoredNotification` read-back type

Stack

proto + generated code
this PR — notification_history keyset listing
notification:read / notification:manage permissions
Grafana-backed service, handlers, server wiring
notifications UI

🤖 Generated with Claude Code

github-actions · 2026-06-15T15:56:17Z

🔐 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 (cd175c127b14bc0386b38e0f36e910d5d3111753...b89405f73516fc9f89396a4197ef438f272e1855, 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] Notification History Page Size Is Not Capped

Category: Reliability
Location: server/internal/domain/stores/sqlstores/notification_history.go:64
Description: The new List method forwards caller-controlled limit directly into LIMIT sqlc.arg('page_limit') with no defaulting or upper bound.
Impact: Once this lister is wired to ListNotifications, a caller can request a very large page and force the DB/sqlc layer to materialize a large result set, creating avoidable CPU and memory pressure.
Recommendation: Normalize at the service boundary or in this store: default <=0, cap to a fixed max, and consider fetching limit+1 to compute has_more.

[LOW] Migration Recovery Instructions Use The Wrong Version

Category: SQLi/Database
Location: server/migrations/000087_notification_history_keyset_index.up.sql:8
Description: The migration comment says a failed build leaves dirty version 88 and recommends migrate force 87. For migration 000087, the dirty version should be 87; after dropping an invalid partial index, the retry path should force back to 86.
Impact: An operator following this runbook could mark the failed migration as applied without rebuilding the index, leaving the schema in an unexpected state.
Recommendation: Update the comment to match the established pattern: dirty at version 87, drop invalid residue, then migrate force 86 before redeploying.

Notes

No changed hunk introduced raw SQL interpolation, shell execution, auth/session handling, plugin trust-boundary changes, frontend rendering changes, pool/wallet configuration changes, or protobuf wire-format changes. The new query is sqlc-parameterized and scoped by organization_id.

_{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".

chatgpt-codex-connector · 2026-06-15T16:09:01Z

@@ -0,0 +1,4 @@
+-- Index matching the ListNotifications keyset pattern (org_id, id DESC); existing indexes are on received_at and can't serve it.
+
+CREATE INDEX idx_notification_history_org_id


Use the next sequential migration number

This tree jumps from 000086 directly to 000088 (I checked server/migrations, and no 000087 exists), even though the repo's migration workflow is sequential. Once a database runs this file, golang-migrate records version 88, so any follow-up stack that later adds the missing 000087 migration will be skipped on already-deployed environments. Rename this migration to the next unused version, or include the missing 87 before this one.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-06-15T16:09:01Z

+CREATE INDEX idx_notification_history_org_id
+    ON notification_history (organization_id, id DESC);


Build the history index concurrently

In deployed environments with existing notification_history rows and incoming alertmanager webhooks, this plain CREATE INDEX holds a write-blocking lock on the table for the duration of the build, so InsertNotificationHistory calls can stall during deploy. This table is already populated by the webhook path, and the repo uses CREATE INDEX CONCURRENTLY for similar high-row-count list indexes under the same migration runner; use the concurrent form here as well.

Useful? React with 👍 / 👎.

CREATE INDEX took ACCESS EXCLUSIVE on the live notification_history table, blocking webhook-driven inserts for the duration of the build. Switch to CREATE INDEX CONCURRENTLY (and a matching DROP INDEX CONCURRENTLY in the down migration) so the build runs online, matching the convention established in 000074. golang-migrate's postgres driver runs statements without an implicit transaction, so CONCURRENTLY is safe. Addresses Codex security review finding (LOW) on #453. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

illegalprime · 2026-06-15T17:39:04Z

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".

chatgpt-codex-connector · 2026-06-15T17:42:08Z

+	EndsAt         sql.NullTime
+}
+
+// One page of an organization's notifications, newest first. Keyset


Regenerate sqlc output from the committed query

This generated comment doesn't match the committed query source (server/sqlc/queries/notification_history.sql now has only the one-line comment on ListNotificationHistory), which means this sqlc file was produced from an earlier version. The generated-code workflow runs just gen and then git diff --quiet (.github/workflows/generated-code-check.yml), so CI will leave the tree dirty until the sqlc artifacts are regenerated from the current SQL.

Useful? React with 👍 / 👎.

CREATE INDEX took ACCESS EXCLUSIVE on the live notification_history table, blocking webhook-driven inserts for the duration of the build. Switch to CREATE INDEX CONCURRENTLY (and a matching DROP INDEX CONCURRENTLY in the down migration) so the build runs online, matching the convention established in 000074. golang-migrate's postgres driver runs statements without an implicit transaction, so CONCURRENTLY is safe. Addresses Codex security review finding (LOW) on #453. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Two review fixes: - The keyset-index migration was numbered 000088 while the permission-seed migration in the next stack PR is 000087. Since this PR merges first, deploying it would advance schema_migrations to 88 and golang-migrate would then skip the lower-numbered 000087 seed on already-upgraded DBs. Renumber the index to 000087 (this PR merges first); the seed becomes 000088 in the authz PR so the numeric order matches the merge order. - Regenerate sqlc: the committed ListNotificationHistory query comment had been shortened but generated/sqlc was stale, so 'just gen' + the generated-code CI check would have failed on a dirty tree. Addresses Codex connector findings (P1/P2) on #453. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…to the mount PR Two review fixes: - Renumber the permission-seed migration 000087 -> 000088 so it sits above the keyset-index migration (now 000087) that merges first in the stack, keeping numeric order aligned with merge order (see #453). - Move the notification entries out of registeredServices (the RBAC contract fixture): TestRPCContract_RegisteredServicesMatchMainMux compares it against the handlers mounted in main.go, and the mounts land in the next stack PR, so listing them here failed the test standalone. The ProcedurePermissions policy map (and its catalog keys) stay here; only the mount-coupled registration moves to the server PR. Addresses Codex connector findings (P1/P2) on #454. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

illegalprime · 2026-06-15T18:16:22Z

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".

chatgpt-codex-connector · 2026-06-15T18:19:40Z

+-- leaves schema_migrations.dirty=true at version 88 and may leave an INVALID
+-- index in pg_indexes; recovery is to DROP the INVALID index and
+-- `migrate force 87` before re-deploying.


Correct the failed-migration recovery version

If this 000087 concurrent index build fails, golang-migrate leaves the database dirty at version 87, not 88; the documented migrate force 87 is unsafe because Force sets the stored migration version and resets dirty to false (pkg.go.dev/github.com/golang-migrate/migrate/v4#Migrate.Force), so the next up treats 000087 as already applied and skips rebuilding the missing index. The recovery text should force the previous version (86) before redeploying.

Useful? React with 👍 / 👎.

CREATE INDEX took ACCESS EXCLUSIVE on the live notification_history table, blocking webhook-driven inserts for the duration of the build. Switch to CREATE INDEX CONCURRENTLY (and a matching DROP INDEX CONCURRENTLY in the down migration) so the build runs online, matching the convention established in 000074. golang-migrate's postgres driver runs statements without an implicit transaction, so CONCURRENTLY is safe. Addresses Codex security review finding (LOW) on #453. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Two review fixes: - The keyset-index migration was numbered 000088 while the permission-seed migration in the next stack PR is 000087. Since this PR merges first, deploying it would advance schema_migrations to 88 and golang-migrate would then skip the lower-numbered 000087 seed on already-upgraded DBs. Renumber the index to 000087 (this PR merges first); the seed becomes 000088 in the authz PR so the numeric order matches the merge order. - Regenerate sqlc: the committed ListNotificationHistory query comment had been shortened but generated/sqlc was stale, so 'just gen' + the generated-code CI check would have failed on a dirty tree. Addresses Codex connector findings (P1/P2) on #453. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…to the mount PR Two review fixes: - Renumber the permission-seed migration 000087 -> 000088 so it sits above the keyset-index migration (now 000087) that merges first in the stack, keeping numeric order aligned with merge order (see #453). - Move the notification entries out of registeredServices (the RBAC contract fixture): TestRPCContract_RegisteredServicesMatchMainMux compares it against the handlers mounted in main.go, and the mounts land in the next stack PR, so listing them here failed the test standalone. The ProcedurePermissions policy map (and its catalog keys) stay here; only the mount-coupled registration moves to the server PR. Addresses Codex connector findings (P1/P2) on #454. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Adds the ListNotifications sqlc query and a keyset index on notification_history(id) for stable id-based pagination, plus the Lister interface and StoredNotification read-back type. Builds on the existing webhook-driven history persistence already on main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

CREATE INDEX took ACCESS EXCLUSIVE on the live notification_history table, blocking webhook-driven inserts for the duration of the build. Switch to CREATE INDEX CONCURRENTLY (and a matching DROP INDEX CONCURRENTLY in the down migration) so the build runs online, matching the convention established in 000074. golang-migrate's postgres driver runs statements without an implicit transaction, so CONCURRENTLY is safe. Addresses Codex security review finding (LOW) on #453. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Two review fixes: - The keyset-index migration was numbered 000088 while the permission-seed migration in the next stack PR is 000087. Since this PR merges first, deploying it would advance schema_migrations to 88 and golang-migrate would then skip the lower-numbered 000087 seed on already-upgraded DBs. Renumber the index to 000087 (this PR merges first); the seed becomes 000088 in the authz PR so the numeric order matches the merge order. - Regenerate sqlc: the committed ListNotificationHistory query comment had been shortened but generated/sqlc was stale, so 'just gen' + the generated-code CI check would have failed on a dirty tree. Addresses Codex connector findings (P1/P2) on #453. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…to the mount PR Two review fixes: - Renumber the permission-seed migration 000087 -> 000088 so it sits above the keyset-index migration (now 000087) that merges first in the stack, keeping numeric order aligned with merge order (see #453). - Move the notification entries out of registeredServices (the RBAC contract fixture): TestRPCContract_RegisteredServicesMatchMainMux compares it against the handlers mounted in main.go, and the mounts land in the next stack PR, so listing them here failed the test standalone. The ProcedurePermissions policy map (and its catalog keys) stay here; only the mount-coupled registration moves to the server PR. Addresses Codex connector findings (P1/P2) on #454. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

illegalprime requested a review from a team as a code owner June 15, 2026 15:53

github-actions Bot assigned illegalprime Jun 15, 2026

github-actions Bot added the server label Jun 15, 2026

illegalprime force-pushed the eden/notifications-1-proto branch from c64a5c2 to d032a2d Compare June 15, 2026 16:02

illegalprime mentioned this pull request Jun 15, 2026

notifications: preliminary UI work #306

Closed

chatgpt-codex-connector Bot reviewed Jun 15, 2026

View reviewed changes

github-actions Bot added javascript Pull requests that update javascript code client shared labels Jun 15, 2026

chatgpt-codex-connector Bot reviewed Jun 15, 2026

View reviewed changes

illegalprime force-pushed the eden/notifications-2-history-store branch from d9857a9 to c49d919 Compare June 15, 2026 17:44

github-actions Bot removed javascript Pull requests that update javascript code client shared labels Jun 15, 2026

illegalprime mentioned this pull request Jun 15, 2026

notifications (3/5): notification:read and notification:manage permissions #454

Open

chatgpt-codex-connector Bot reviewed Jun 15, 2026

View reviewed changes

illegalprime force-pushed the eden/notifications-1-proto branch from d032a2d to 7a2ceb2 Compare June 15, 2026 18:35

illegalprime force-pushed the eden/notifications-2-history-store branch from 6076695 to 03bb87c Compare June 15, 2026 18:35

illegalprime and others added 2 commits June 15, 2026 15:09

illegalprime force-pushed the eden/notifications-1-proto branch from 7a2ceb2 to cd175c1 Compare June 15, 2026 19:24

illegalprime force-pushed the eden/notifications-2-history-store branch from 03bb87c to b89405f Compare June 15, 2026 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

notifications (2/5): notification_history keyset listing#453

notifications (2/5): notification_history keyset listing#453
illegalprime wants to merge 3 commits into
eden/notifications-1-protofrom
eden/notifications-2-history-store

illegalprime commented Jun 15, 2026

Uh oh!

github-actions Bot commented Jun 15, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jun 15, 2026

Uh oh!

chatgpt-codex-connector Bot Jun 15, 2026

Uh oh!

illegalprime commented Jun 15, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jun 15, 2026

Uh oh!

illegalprime commented Jun 15, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jun 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		@@ -0,0 +1,4 @@
		-- Index matching the ListNotifications keyset pattern (org_id, id DESC); existing indexes are on received_at and can't serve it.

		CREATE INDEX idx_notification_history_org_id

		CREATE INDEX idx_notification_history_org_id
		ON notification_history (organization_id, id DESC);

Conversation

illegalprime commented Jun 15, 2026

Stack

Uh oh!

github-actions Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔐 Codex Security Review

Review Summary

Findings

[MEDIUM] Notification History Page Size Is Not Capped

[LOW] Migration Recovery Instructions Use The Wrong Version

Notes

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

illegalprime commented Jun 15, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

illegalprime commented Jun 15, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

github-actions Bot commented Jun 15, 2026 •

edited

Loading