feat(push): make gateway origin configurable - #7261
Conversation
Signed-off-by: Tom Brow <tomb@block.xyz>
🔐 Codex Security Review
|
|
@codex review |
|
@builderbot review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af33072eb0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
|
@codex review |
|
@builderbot review |
|
@buzz-security-review 7d7cc99 |
Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
|
@codex review |
|
@builderbot review |
|
@buzz-security-review 6616b83b965261d6efc72dce9d4714212d545649 |
|
@buzz-security-review 6616b83 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6616b83b92
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
|
@codex review |
|
@builderbot review |
|
@buzz-security-review 4a60901 |
This reverts commit 4a60901. Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
|
@codex review |
|
@builderbot review |
|
@buzz-security-review f2c4ce76e5e769c25702a11c6d243b19e04f7319 |
|
@buzz-security-review f2c4ce7 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2c4ce76e2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
|
@codex review |
|
@codex review |
Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fcf5648e9
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 843170db4c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 838d0dc34f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b59a1d019
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: deab86b83e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18d4a07a49
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea811c27be
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14d38ce88a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| token, | ||
| 'retired:${(retiredRelayOrigins.toList()..sort()).join(',')}', | ||
| 'replacement:${(replacementRelayOrigins.toList()..sort()).join(',')}', | ||
| 'replacement-generation:$replacementGeneration', | ||
| ].join('|'); |
There was a problem hiding this comment.
When push is enabled for a community while an existing migration is awaiting descriptor resolution or enrollment, the hook rebuilds but this attempt key remains unchanged, so tryBegin leaves the old task current even though its captured candidate list omitted that community. The old task can then complete cleanup and remove the relay origin; if the user switches away before normal active publication runs, the newly enabled community has no lease and no durable replacement work until it is visited again. Include the enabled migration candidates in the attempt generation/currentness check.
AGENTS.md reference: AGENTS.md:L176-L182
Useful? React with 👍 / 👎.
| if (!attemptIsCurrent()) return Future.value(false); | ||
| return markAccepted(); |
There was a problem hiding this comment.
When APNs rotates the token after this pre-check but while the asynchronous markAccepted persistence is running, the obsolete lease can still durably record the target gateway origin. The later currentness check prevents checkpointing, but the next token-keyed attempt filters out a retired-only inactive community because its accepted origin already equals the target, then completes cleanup while its published lease still uses the old token. Fresh evidence beyond the resolved token-acceptance thread is that this helper returns the unfenced future directly, leaving the persistence window unprotected; make acceptance itself conditional on the live token/attempt or durably requeue the origin if it becomes stale.
AGENTS.md reference: AGENTS.md:L176-L182
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 7a9a5233d9d755e715be0c585cf7850e935d28cf..14d38ce88a590bf467dc608b4e2c2dba1d38c1e7 (exact head 14d38ce88a590bf467dc608b4e2c2dba1d38c1e7)
Risk: critical — this changes push authority migration, durable recovery, revocation, APNs-token fencing, relay/gateway deployment configuration, database migrations, and mobile build inputs.
Behavior/contracts traced: relay delivery configuration; gateway origin/audience validation; PostgreSQL authority, grant, and tombstone persistence; mobile legacy inventory, enrollment journals, replacement queues, migration attempts, lease publication, revocation cleanup, APNs rotation, Settings health; Helm/Compose and iOS/Android build configuration.
Blocking findings
-
[HIGH] Stale acceptance can survive APNs rotation and retire the only recoverable authority.
markBuzzPushGatewayMigrationAcceptedIfCurrentchecks token/attempt currentness before returning an asynchronous persistence operation (mobile/lib/shared/push/push_bootstrap.dart:168-174, called at:897-906).markPushLeaseAcceptedthen serializes, reads, saves, updates provider state, and snapshots without carrying that fence into the authoritative mutation (mobile/lib/shared/community/community_provider.dart:415-449). If APNs rotates after the pre-check but while persistence awaits, the stale lease can record the target origin; candidate recomputation then filters the community as already accepted (push_bootstrap.dart:136-145,587-592) and cleanup may retire the old authority even though the accepted lease contains the old token.Author action: move token/attempt currentness into the serialized authoritative acceptance mutation, or atomically persist and compare an attempt/token generation immediately before save/state update. A stale completion must preserve or requeue replacement inventory. Add a production-seam regression that pauses acceptance, rotates APNs, resumes, and proves stale acceptance cannot remove new-token work.
-
[MEDIUM] Enabling a community during migration does not invalidate the captured candidate set. Attempt identity/currentness includes token, origins, and replacement generation, but not the enabled-community migration set (
mobile/lib/shared/push/push_bootstrap.dart:567-585). A rebuild caused by community changes (:690-698) therefore leaves the old task current. If community B is enabled while target resolution/enrollment is awaiting, the old task can finish its captured groups and complete cleanup (:656-659) without replacing B; an inactive/unvisited B is not guaranteed repair by active publication.Author action: include a stable fingerprint of enabled communities and relevant relay/accepted-origin state in attempt identity/currentness, or durably merge newly eligible candidates before cleanup. Add a production-seam regression that enables B while migration is paused and proves cleanup cannot complete until B is replaced.
-
[HIGH] Exhausted migration can disable renewal while Settings still claims notifications are healthy. Initialization/migration stops after six failures and defers to a future app launch (
mobile/lib/shared/push/push_bootstrap.dart:452-477,660-685), while remaining migration work suppresses active publication (:703-710). The Settings state is derived only from persisted opt-in and OS authorization and can continue saying “Receive message notifications from this community” (mobile/lib/features/settings/settings_page/notifications_section.dart:13-34). Scoped review ofmobile/libfound no production consumer surfacing the relevant migration/transport errors. After an upgrade or origin cutover, users can lose renewal and eventually delivery while the UI affirmatively reports health.Author action: expose durable migration/transport health separately from permission, show degraded/failed delivery, and provide a bounded retry that re-arms migration without requiring restart. Add a widget/integration regression for opted-in + authorized + repeated migration failure → degraded/retry, then retry success → healthy and publication resumed.
-
[MEDIUM] Added public Swift API violates the repository documentation gate.
AGENTS.md:147-150requires documentation for new public API. Added public properties/initializers are undocumented, includingBuzzPushReplacementQueueState(mobile/ios/BuzzPushKit/Sources/BuzzPushKit/BuzzDevPushEnrollmentDriver.swift:93-102), public protocol/default methods (:107-163), andBuzzPushLegacyRecoveryInventoryplus nested recovery state (mobile/ios/BuzzPushKit/Sources/BuzzPushKit/BuzzPushLegacyRecovery.swift:3-55).Author action: document every public declaration added by this diff—properties, initializers, protocol requirements, and default implementations—or reduce visibility where external API is unnecessary.
Verification owner: author for repairs and focused regressions; reviewer for adversarial re-review and mutation proof; CI/release owners for required gates and native cutover evidence.
Validation at exact head: full Flutter suite passed 2,097 tests; BuzzPushKit passed 91 XCTest + 22 Swift Testing cases; push-gateway Rust passed 50 with 11 PostgreSQL/live tests ignored; formatting, Flutter analysis, origin-validator probes, security-review checks, git diff --check, and relevant Rust/mobile/Swift/security/image/Helm lanes passed. One Desktop smoke shard failed without established PR attribution; several live integration/Desktop jobs were still running at review time. Those gate states are confidence gaps, not additional author defects.
Manual/native evidence: no installed old→new mobile cutover with real APNs rotation and gateway failure/recovery was run. Before release, the native/release owner should verify existing opt-in/grant migration, unchanged permission, token rotation, failure/retry, and delivered notification on an isolated identity/device.
Residual risk: PostgreSQL/live tests ignored locally and native cutover not independently observed. These gaps do not create the four defects above; exact-head re-review is required after fixes.
PostgreSQL gate correction: after submission, the assigned systems lane completed the previously outstanding live suite and established a fifth blocking defect at this same exact head.
-
[MEDIUM] Required PostgreSQL integration fixture still encodes pre-migration uniqueness and fails the replacement-tombstone regression. Exact-head CI fails
PostgreSQL Domain / PostgreSQL Testsand aggregatePostgreSQL Tests. Localcargo test -p buzz-push-gateway postgres::postgres_tests -- --ignored --test-threads=1reproduced 9 passed, 1 failed:replacement_installation_preserves_unexpired_revocation_tombstonereturnsConflictatcrates/buzz-push-gateway/src/postgres.rs:1224-1226. The test helperfull_schema()retains unconditionalapp_attest_key_id UNIQUEand(app_profile, token_fingerprint) UNIQUEconstraints atpostgres.rs:909-922, so it does not model migration0005’s active-only partial indexes.Author action: update
full_schema()to the migrated active-only uniqueness DDL—preferably by applying the actual gateway migrations instead of duplicating schema—and rerun the complete PostgreSQL package lane. Verification owner: reviewer for the ignored PostgreSQL suite; CI for the required exact-head gate.
This corrects the earlier validation snapshot: the PostgreSQL/live suite is now a reproduced PR-caused required-gate failure, not merely an outstanding confidence gap. The REQUEST CHANGES verdict is unchanged.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
CHANGES REQUESTED
Reviewed exact head 14d38ce88a590bf467dc608b4e2c2dba1d38c1e7 against exact base 7a9a5233d9d755e715be0c585cf7850e935d28cf. This is a source-only review: no checkout, build, tests, or PR-code execution.
The intended contract is explicit, sovereign gateway configuration while preserving opt-in and recovering/replacing every affected enabled community before retiring old push authority. Fixed NIP-PL v1 transcript audiences are compatible with configurable transport URLs and are not a defect.
1. [P1] Preserve replacement work when APNs rotates during retired-gateway cleanup
Primary anchor: AppDelegate.swift:697–708.
Migrate two opted-in communities from gateway A to an independent gateway B using token T1. Once both replacement leases and acceptedGatewayOrigin=B are durable, pause the old-gateway revocation response, deliver APNs token T2, then let revocation succeed. The driver removes the retired grants/journals and cleanup state before the token fence runs (driver:1001–1007, 1136–1143). Ordinary retired-only migration has no replacement queue, so skipping clearReplacementRelayOrigins preserves nothing. Completion returns empty inventory, while the inactive community’s replacement lease still targets T1. A concurrent T2 attempt also skips that community because eligibility compares only the accepted gateway origin (push_bootstrap.dart:136–145). Restart cannot recover the deleted work.
The same underlying gap exists when T2 arrives during the asynchronous acceptance save: the pre-check at push_bootstrap.dart:168–174 is not carried into CommunityListNotifier.markPushLeaseAccepted. Retain affected origins independently of retired cleanup records and bind acceptance/checkpoint to the token generation. Throwing after the current cleanup fence fails is too late. Add production-seam coverage for two communities, an initially empty replacement queue, and rotation during suspended cleanup/acceptance.
2. [P2] Invalidate migration attempts when the eligible community set changes
Primary anchor: push_bootstrap.dart:567–585.
The attempt key/currentness includes token and native inventories, but not community opt-in or the captured candidate set. Start migration with A enabled and B disabled, both represented in retired native grants. While A’s descriptor/enrollment is awaiting, enable B, then leave A active. The communities dependency rebuilds the hook, but tryBegin rejects the unchanged key; the existing task still processes only its captured A candidate. B’s ordinary publisher is suppressed while it awaits migration. The A-only task can therefore reach cleanup at lines 656–659, retire B’s old installation and remove its migration inventory without publishing B’s replacement. B loses delivery until separately visited/repaired. No token rotation is necessary.
Include relevant community state in attempt currentness or durably merge newly eligible work before cleanup. Cover enabling B during suspended A migration through the actual bootstrap/provider/native boundary.
3. [P2] Apply active-only uniqueness in the PostgreSQL tombstone test fixture
Primary anchor: postgres.rs:1198–1226.
The new replacement_installation_preserves_unexpired_revocation_tombstone test creates and revokes an installation, then expects enrollment with the same key/token to succeed while retaining the original row. Its private full_schema() still declares unconditional uniqueness for both fields (lines 909–922) and returns without applying migration 0005. PostgresAuthorityStore::new() does not migrate either. Consequently, the retained row makes the replacement INSERT ... ON CONFLICT DO NOTHING insert zero rows and return AuthorityError::Conflict; the new test fails at its replacement expect instead of testing migrated production behavior. Align the isolated fixture with the active-only indexes from migration 0005, then validate the PostgreSQL package suite. This is a source-derived test defect, not a claim that tests were executed or that the migrated production schema has the same constraints.
Compatibility note (non-blocking): this deliberately removes implicit production gateway configuration. Deployers must set the relay delivery URL, server gateway origin, and Helm gatewayOrigin before upgrading. The documented fail-closed behavior matches the stated contract; a release-note reminder would help.
Coverage and limits: reviewed Flutter/native inventory, enrollment, replacement, acceptance, cleanup, token changes, opt-in/out, gateway recovery/revocation and persistence, relay configuration, Helm/Compose, and mobile build inputs. Android configuration is covered; APNs migration is iOS-only. No new desktop/web push runtime surface or event-kind/class registry is introduced by the changed files. Native device cutover and live PostgreSQL behavior were not exercised in this read-only automation.
3 participants
Make the push gateway origin configurable.
Push delivery currently embeds a deployment-specific gateway origin across the relay, gateway, mobile client, and chart. This change makes the origin an explicit deployment and build input, so public, internal, and self-hosted deployments can use their own gateway without source changes.
The gateway derives its delivery endpoint, App Attest audiences, and chart-managed route hostname from one configured origin. Relay push now requires an explicit delivery endpoint when enabled and fails clearly when it is absent. Mobile builds likewise require an explicit gateway origin and fail when it is not supplied.
Existing iOS enrollment, grant, and pending state is inventoried and migrated before retired gateway authority is revoked. Gateway-neutral legacy recovery handles both retained opaque capabilities and response-loss enrollment journals, while durable replacement queues and generation fences preserve recovery across partial failures, APNs token rotation, and concurrent migration attempts. Notification permission is unaffected.
Self-hosting remains straightforward: the canonical Compose environment template and instructions explicitly configure the public gateway origin or delivery endpoint instead of relying on an in-code fallback.
Validation