feat(push): make gateway origin configurable - #7261

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: af33072eb0

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6616b83b92

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f2c4ce76e2

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2fcf5648e9

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 843170db4c

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 838d0dc34f

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b59a1d019

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: deab86b83e

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 18d4a07a49

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ea811c27be

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 14d38ce88a

: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

1. [HIGH] Stale acceptance can survive APNs rotation and retire the only recoverable authority. markBuzzPushGatewayMigrationAcceptedIfCurrent checks token/attempt currentness before returning an asynchronous persistence operation (mobile/lib/shared/push/push_bootstrap.dart:168-174, called at :897-906). markPushLeaseAccepted then 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.

2. [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.

3. [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 of mobile/lib found 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.

4. [MEDIUM] Added public Swift API violates the repository documentation gate. AGENTS.md:147-150 requires documentation for new public API. Added public properties/initializers are undocumented, including BuzzPushReplacementQueueState (mobile/ios/BuzzPushKit/Sources/BuzzPushKit/BuzzDevPushEnrollmentDriver.swift:93-102), public protocol/default methods (:107-163), and BuzzPushLegacyRecoveryInventory plus 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.

5. [MEDIUM] Required PostgreSQL integration fixture still encodes pre-migration uniqueness and fails the replacement-tombstone regression. Exact-head CI fails PostgreSQL Domain / PostgreSQL Tests and aggregate PostgreSQL Tests. Local cargo test -p buzz-push-gateway postgres::postgres_tests -- --ignored --test-threads=1 reproduced 9 passed, 1 failed: replacement_installation_preserves_unexpired_revocation_tombstone returns Conflict at crates/buzz-push-gateway/src/postgres.rs:1224-1226. The test helper full_schema() retains unconditional app_attest_key_id UNIQUE and (app_profile, token_fingerprint) UNIQUE constraints at postgres.rs:909-922, so it does not model migration 0005’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.

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.