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 |
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.
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
* origin/main: (29 commits) fix(acp): pace targeted overflow recovery on consumer capacity (#7325) fix(link-preview): keep composer fetches user-paced (#7211) feat(mesh): upgrade to mesh-llm 0.76.0-rc8 and recommend Qwen3.8 27B (#6189) fix(agent): route GPT-5+ model-service FQNs to Responses (#7358) fix(buzz-acp): wake held ACP threads and fence forked sessions (#7340) fix(mobile): style inline code with the app mono face (#6631) chore(release): release Buzz Desktop version 0.5.23 (#7381) fix(desktop): keep packaged frontendDist relative so Windows embeds assets (#7177) fix(sidebar): simplify unread indicators and emphasize priority activity (#7134) Add generic information-flow control core (#7293) feat(buzz-acp): update base prompt; add buzz context and skills to Pi agents (#7335) fix(desktop): restore mention chip identity icons (#7338) Persist video playback speed preference (#7336) Verify ACP relay events before prompt routing (#7010) fix(buzz-acp): bound busy-owner hold to prevent cross-channel starvation (#7337) feat(desktop): invite owned agents from standalone forums (#7125) fix(desktop): authorize remote mentions at publication (#7124) fix(acp): rename system tag to agent-instructions (#7332) fix(desktop): bind duplicate mention selections to exact recipients (#7133) refactor(relay): extract NIP-29 membership authorization (#7285) ... Signed-off-by: Tom Brow <tomb@block.xyz>
Signed-off-by: Tom Brow <tomb@block.xyz>
|
🤖 @jedwards27 Addressed your review in fe1fe6040 and 8c3b63caf. Tom confirmed push has no existing enabled users. I removed legacy gateway migration, its replacement queues/cleanup, acceptance markers, and the legacy-only recovery endpoint. That eliminates the migration races in findings 1 and 2 and the exhausted-migration publication block in finding 3; there is no migration left to expose or retry in Settings. Normal enrollment retains response-loss recovery and ordinary publication retries. The token-rotation regression fails when journal replay is removed. The remaining public Swift APIs are documented (finding 4), and the PostgreSQL fixture now applies the actual gateway migrations (finding 5). All nine live PostgreSQL tests pass, including tombstone replacement. The full local gate, Swift tests/release build, and iOS simulator build pass. First-time notification opt-in is preserved and regression-tested in c9f679564. No physical-device APNs delivery test is claimed. |
|
🤖 @wesbillman Addressed your review in fe1fe6040 and 8c3b63caf. Tom confirmed push has no existing enabled users, so I removed the legacy gateway-migration coordinator, replacement queues, retired-gateway cleanup, and migration acceptance markers entirely. This removes the cleanup/token and changing-community-set paths in findings 1 and 2. Normal enrollment still retains its exact response-loss journal across token changes, with a mutation-checked regression; grants remain scoped to the configured gateway. For finding 3, the fixture now applies the actual gateway migrations. All nine live PostgreSQL tests pass. The full local gate, Swift tests/release build, and iOS simulator build also pass. Explicit deployment configuration and fixed NIP-PL audiences remain intentional. First-time notification opt-in is preserved and regression-tested in c9f679564. No physical-device APNs delivery test is claimed. |
blockers resolved
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 c9f6795649cab552019ed4d652f21868fd402517 against base c045321a7fb3ca8939f28519ce7a555a6f597728. Source/metadata only: no checkout, build, tests, or PR-code execution.
The revised contract is explicit gateway configuration, gateway-scoped fresh enrollment, preserved opt-in, and ordinary response-loss recovery. The author’s confirmation that push has no existing enabled users makes removal of legacy migration a valid simplification. Fixed registered NIP-PL v1 App Attest audiences remain intentional.
[P1] Wire the required origin into the signed release builders before enabling the build gates
Primary anchor: mobile/android/app/build.gradle.kts:46–60. The matching iOS gate is installed at project.pbxproj:515.
The supported signed builders live in squareup/buzz-releases (identified by this repository’s AGENTS.md). At its inspected main commit 65f360a3f467adc846e59a9f30e9495980e0b7a3, scripts/build-android-play.sh:92–95 invokes flutter build appbundle --release with only version arguments. Its pipeline supplies signing/toolchain configuration, but neither the builder nor the pipeline supplies or forwards BUZZ_PUSH_GATEWAY_URL as a Dart define. Point that unchanged release path at a candidate containing this head: pushGatewayOrigins is empty and the new compileFlutterBuildRelease pre-action throws before the AAB is produced. Disabling relay push does not bypass this unconditional build gate.
The checked-in iOS dogfood and App Store builders also write only BUNDLE_IDENTIFIER into AppOverrides.xcconfig, then call scripts/lib.sh:403–404 without a gateway define. The new iOS script can accept an ambient build setting, but no durable injection is declared in those inspected pipeline/build paths. I have not inspected external worker environments and do not claim they are empty. This is a signed-release integration finding, not a claim that release PR checks compile/sign artifacts.
Smallest safe resolution: land and link the companion release-repository change supplying the reviewed HTTPS origin to Android, dogfood iOS, and App Store iOS before a candidate with these gates is consumed. Add release-contract coverage for forwarding that input. Keep explicit configuration and fail-closed validation; restoring an implicit production default is not requested. Public-repo fixture defines alone do not exercise these actual release producers.
Previous review disposition: the two mobile migration-inventory/candidate races in review 5122073657 are eliminated by deleting the migration path under the revised contract. The PostgreSQL fixture now applies the actual scoped gateway migrations rather than stale unconditional uniqueness DDL. The first-opt-in trigger and superseded-acceptance retry changes have regression coverage bound to their production functions. No other actionable defect was established in the reviewed changed paths.
Coverage and limits: integrated Flutter bootstrap/lease acceptance and opt-out/removal, native journal/token/key recovery and scoped Keychain/bridge storage, gateway authorization/revocation/tombstones and schema paths, relay configuration, Helm/Compose, and mobile build producers. No new desktop/web push runtime or event-kind/class registry is introduced. Live PostgreSQL, signed artifact builds, and physical-device APNs delivery were not exercised; author-reported passes are not independent validation.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Reviewed exact head c9f6795649cab552019ed4d652f21868fd402517 against base c045321a7fb3ca8939f28519ce7a555a6f597728.
The previously reported blockers are resolved at this head:
- APNs lease acceptance now rejects stale generations and stale desired-subscription fingerprints, and bootstrap converts rejection into retry (
mobile/lib/shared/community/community_provider.dart:415-447,mobile/lib/shared/push/push_bootstrap.dart:136-147). - Push bootstrap now invalidates on the enabled-community opt-in state and has first-opt-in regression coverage (
mobile/lib/shared/push/push_bootstrap.dart:318-340,mobile/test/shared/push/push_bootstrap_test.dart). - Durable generation reservation, response-loss/final-save recovery, restart-safe tombstone retries, serialized outbox triggers, and gateway-origin isolation are covered by the exact-head Dart/Swift/Rust suites.
- PostgreSQL CI applies the production gateway migrations, including migration 0045's active-only uniqueness constraint, and the PostgreSQL required checks pass.
- The added Swift public API has doc comments, satisfying
AGENTS.md:147-150;git diff --checkalso passes. - The configured gateway origin remains authoritative for transport and persisted-grant scoping. The fixed
push.buzz.xyzaudience is the registered NIP-PL v1 transcript value rather than a cross-origin authority leak (crates/buzz-push-gateway/src/config.rs:46-90,mobile/ios/BuzzPushKit/Sources/BuzzPushKit/BuzzDevPushEnrollmentDriver.swift:399-427).
Exact-head validation:
cargo test -p buzz-push-gateway: 48 passed, 10 environment-gated ignored, 0 failed.cd mobile/ios/BuzzPushKit && swift test: 91 passed, 0 failed.- Focused Flutter push tests: 67 passed.
- Independent full Flutter run: 2,082 passed. A separate full run had one unrelated temporary-directory teardown failure in
voice_note_recording_test.dart; its isolated rerun passed. - GitHub required checks are terminal green at review submission, including Mobile, Rust lint/unit, Windows Rust, PostgreSQL/integration paths, Security, macOS build, Desktop Release Candidate, and DCO.
One non-required raw x86_64 musl job failed before compilation while downloading Hermit (curl: (35) Recv failure: Connection reset by peer), while the required Rust Cross-Compile / Results aggregate is green. This is an infrastructure confidence note, not an author-actionable source defect. A non-required Desktop Domain / Desktop Core child job was still running at final refresh; all configured required checks were terminal green.
Residual rollout risk: no credentialed physical-device APNs delivery, permission-denied/re-enable, token-rotation, or notification-tap journey was observed in this review. Preserve the rollout-required dogfood evidence before release. A new head invalidates this approval.
Verdict: APPROVE.
Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Codex <noreply@openai.com> Ai-assisted: true
|
🤖 @wesbillman You were right that the unconditional build gate broke unconfigured signed builders. Tom chose to make gateway configuration optional instead of requiring a release-repository rollout. Addressed your review in 883b8e52d. With no gateway URL, Android and iOS builds succeed, push discovery/registration/enrollment/lease publication stay inactive, and Settings shows “Unavailable in this build.” Any supplied URL is still validated strictly, including HTTPS-only release/profile configuration. There is no implicit production gateway. Existing signed builders can therefore keep building without configuration; supplying the reviewed origin remains necessary when those artifacts should support push. Validation: full local gate and pre-push checks passed; Android debug APK, unsigned release AAB, iOS simulator app, and unsigned iOS release all built without a gateway define. A malformed supplied origin failed the real Android build gate. Added production-bound disabled-runtime/Settings tests, iOS build-phase omission/validation tests, and no-configuration CI coverage. Configured-push regression tests also pass. Signed release pipelines and physical-device delivery were not exercised. |
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 883b8e52df361e382f6ea16b9d9c10861785c004 against base c045321a7fb3ca8939f28519ce7a555a6f597728, focusing on the 12-file delta from previously reviewed c9f6795649cab552019ed4d652f21868fd402517.
The revised contract is optional mobile gateway configuration: omission produces a build with push unavailable; supplied origins remain validated, with portless HTTPS required for release/profile. No production gateway is selected implicitly. The existing clean-cutover decision and fixed registered NIP-PL audiences remain unchanged.
[P2] Make the optional Flutter arguments safe under macOS Bash 3.2
Anchor: Justfile:867–873. The same issue appears in mobile-build-android:842–851.
Both recipes use #!/usr/bin/env bash and set -euo pipefail, leave gateway_args=() empty when configuration is omitted, then expand "${gateway_args[@]}". On macOS’s Bash 3.2, expanding an array with no assigned elements under nounset raises an unbound-variable error. Consequently, the documented just mobile-dev path with no environment/Xcode gateway setting exits before flutter run; just mobile-build-android fails the same way on that shell. This breaks the newly intended unconfigured-development path even though direct Flutter builds now accept omission.
Evidence is source-based, not an executed PR reproduction: the Studio’s installed bash resolves to /bin/bash, version 3.2.57(1)-release, and the exact-head tree does not supply a Bash shim/package. GNU explicitly records the change in Bash 4.4’s release history: using ${a[@]} or ${a[*]} without assigned elements under nounset “no longer throws an unbound variable error.” The new workflow’s Android recipe runs on Ubuntu; its macOS job invokes Flutter directly, so neither tests this recipe on the affected shell.
Smallest fix: construct/forward optional arguments compatibly with Bash 3.2, or branch explicitly between the configured and unconfigured Flutter invocations. Preserve argument quoting and optional configuration; no new Bash dependency or production default is needed. Add a production-recipe regression for the omitted and supplied cases using the supported macOS shell and a stub Flutter command.
Previous P1 resolved: Android now accepts zero gateway definitions, and the iOS build phase returns successfully when unconfigured. The previously identified signed-builder omission is no longer a deterministic build failure. Those artifacts remain intentionally push-unavailable until their producer supplies an origin; a companion release-repository rollout is not required for that contract.
Coverage and limits: reviewed the new Dart configuration gate through descriptor discovery, registration, active enrollment/lease publication, and noninteractive unavailable Settings state; native startup/callback/method-channel entry points; iOS/Android validation, argument forwarding, and regression/CI source. Preserved independent opt-out/removal cleanup and the previous review’s coverage of unchanged gateway/relay, scoped persistence, and recovery behavior. No new event-kind/class registry or desktop/web push surface is introduced. All repository processing ran on the Studio. No checkout, build, tests, snippet reproduction, or PR-code execution; no independent CI verdict, signed-artifact build, or physical-device APNs result is claimed.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: c045321a7fb3ca8939f28519ce7a555a6f597728..883b8e52df361e382f6ea16b9d9c10861785c004 (exact head 883b8e52df361e382f6ea16b9d9c10861785c004)
Risk: critical — the changed head alters the upgrade contract for persisted push authority, relay leases, native notification state, and the user's only opt-out affordance.
Behavior/contracts traced: configured and unconfigured iOS/Android builds; authenticated startup; persisted community opt-in and accepted lease state; relay capability discovery and lease renewal; APNs enrollment/grants; iOS NSE community/signing-key snapshots; Settings truthfulness; expiry/revocation; CI and release-build inputs.
Blocking finding
[HIGH] A configured → unconfigured upgrade can keep delivering notifications while Settings says push is unavailable and removes the opt-out.
The new unconfigured guards stop fresh descriptor discovery, APNs registration, and endpoint-grant reads (mobile/lib/shared/push/push_relay_capability_provider.dart:24, mobile/lib/shared/push/push_bridge.dart:142-200). Settings then replaces the switch with “Unavailable in this build” (mobile/lib/features/settings/settings_page/notifications_section.dart:13-25). That is sound for a fresh install, but not for an upgrade carrying persisted authority.
Authenticated startup still loads the existing opted-in communities and synchronizes their native snapshot (mobile/lib/shared/auth/auth_provider.dart:20-41, mobile/lib/shared/community/community_provider.dart:209-220). registerBuzzPushCommunitySnapshot exports every persisted push-enabled community and its nsec-derived signing key to the iOS notification service extension without checking Env.pushGatewayConfigured (mobile/lib/shared/push/push_bridge.dart:262-298). Scoped searches of mobile/lib, mobile/ios/BuzzPushKit/Sources, and mobile/ios/Runner found no configured→unconfigured path that publishes a higher-generation lease tombstone, clears the NSE snapshot/signing keys, or revokes the prior gateway delegation/installation.
Consequently, a user who opted in on a configured build can upgrade to an artifact without the define, lose the in-app off switch, and continue receiving server-driven APNs from retained authority. The relay advertises leases up to 2,592,000 seconds (crates/buzz-relay/src/nip11.rs:256-260), matching the gateway chart's 30-day grant lifetime (deploy/charts/buzz-push-gateway/values.yaml:24). The added unconfigured test seeds only fresh state, not an upgrade with persisted opt-in, accepted lease, grant, and native snapshot (mobile/test/shared/push/push_unconfigured_build_test.dart:20-115). This is a false privacy state and strands recovery.
Author action: Prefer retaining the required-build-input behavior until a durable shutdown transition exists. If optional unconfigured artifacts are required, add a configured→unconfigured state transition that preserves enough prior origin/generation authority to publish higher-generation tombstones and revoke old delegation/installation state, retries durably across offline/restart and outcome-ambiguous failures, clears NSE community/signing-key state, and keeps the UI honest until retirement is proven. Add an upgrade-shaped regression seeded with persisted accepted authority and native snapshot state; prove the first failed retirement attempt survives restart and eventually disables delivery.
Verification owner: author for the state transition and regression; reviewer for adversarial re-review and mutation proof; CI/release owner for exact-head signed/native upgrade evidence.
Exact-head validation
At clean matching head 883b8e52df361e382f6ea16b9d9c10861785c004:
- Independent
just mobile-test: PASS — configured suite 2,083 passed / 3 skipped; unconfigured suite 3 passed. - Independent BuzzPushKit suite: PASS — 73 XCTest + 18 Swift Testing cases.
- Independent
cargo test -p buzz-push-gateway: PASS — 48 passed / 10 environment-gated ignored. - Independent
flutter analyze, changed-Dart formatting, gateway-origin validator/tests, focused relay config tests, andgit diff --check: PASS. - Mutation checks removing each new descriptor-discovery, native-registration, and Settings guard made the new unconfigured regression fail, then the clean source was restored.
- GitHub checks observed green include Mobile, Mobile Swift, Rust lint/unit/Windows/cross-compile, PostgreSQL Tests, relay E2E/backend integration, Security, image builds, Helm validation, macOS build, Desktop Release Candidate, and desktop smoke shards. Desktop Core and two desktop integration shards remained in progress at final review refresh; they are confidence debt, not the reason for this verdict.
Confidence gaps / residual risk
No physical-device APNs delivery, denied/re-enabled permission, signed-artifact upgrade, or notification-tap journey was observed. Docker-backed local PostgreSQL/relay reproduction was unavailable due enforced organization sign-in; the corresponding GitHub PostgreSQL and relay gates passed. These are release/device verification obligations with author action: none beyond the blocker above. Any new head invalidates this review.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: c045321a7fb3ca8939f28519ce7a555a6f597728..883b8e52df361e382f6ea16b9d9c10861785c004 (exact head 883b8e52df361e382f6ea16b9d9c10861785c004)
Risk: critical — changed build configuration crosses durable push authority, privacy controls, native notification state, persistence/recovery, and upgrade behavior.
Behavior/contracts traced: configured/unconfigured build inputs; persisted opt-in, lease/grant, and community state; authenticated startup; Flutter→native snapshot export; relay lease lifetime; Settings truthfulness; revocation/tombstone and retry paths; first-run versus upgrade behavior.
Findings:
- Blocking MAJOR — configured → unconfigured upgrades can retain live push delivery while Settings says “Unavailable in this build” and removes the opt-out. The new guards stop fresh descriptor discovery/registration/grant reads (
mobile/lib/shared/push/push_relay_capability_provider.dart:24,mobile/lib/shared/push/push_bridge.dart:142-200) and Settings renders only the unavailable state (mobile/lib/features/settings/settings_page/notifications_section.dart:13-25). But authenticated startup still loads persisted communities and synchronizes them (mobile/lib/shared/auth/auth_provider.dart:20-41,mobile/lib/shared/community/community_provider.dart:209-220), andmobile/lib/shared/push/push_bridge.dart:262-298still exports every persisted push-enabled community and signing key to the iOS NSE without checking gateway configuration. No changed-head transition publishes higher-generation tombstones, revokes prior delegation/installations, clears native snapshot/key material, or retries that retirement across offline/restart. Existing relay authority can remain live for 30 days (crates/buzz-relay/src/nip11.rs:256-260;deploy/charts/buzz-push-gateway/values.yaml:24). APNs delivery is server-driven, so omitting a compile-time origin does not neutralize previously granted authority.
Consequence: a user who opted in on a configured artifact can upgrade to an unconfigured artifact, continue receiving notifications, and lose the only visible control while the UI falsely claims push is unavailable. This is a PR-caused privacy and recovery regression, not a native-observation confidence gap.
Author action: safest: retain the prior build failure when gateway configuration is omitted until a durable shutdown transition exists. If optional artifacts are required now, implement configured → unconfigured retirement that preserves old origin/generation state long enough to publish higher-generation lease tombstones and revoke gateway delegation/installations; retries outcome-ambiguous/offline failures across restart; clears NSE community/signing-key snapshots; and keeps the UI truthful until retirement is established. Add an upgrade-shaped production-seam regression seeded with persisted opted-in authority and native snapshot state, and prove failed-first-attempt/restart recovery.
Verification owner: author for the transition and regression; reviewers for exact-head mutation/recovery proof; CI/release owner for signed/native upgrade and APNs dogfood.
Validation: at clean matching exact head: full Flutter suite passed (2,083 passed, 3 skipped); unconfigured-artifact suite passed (3/3); BuzzPushKit passed (73 XCTest + 18 Swift Testing); flutter analyze, formatting, git diff --check, gateway Rust tests (48 passed, 10 environment-gated ignored), focused relay config tests, and gateway-origin validation passed. Independent mutations proved the new fresh-unconfigured guards are exercised, but the checked-in regression does not seed the configured-upgrade authority state. At final refresh, no CI failure was reported; several Desktop/integration jobs remained nonterminal and are not the basis for this verdict.
Manual/native evidence: no physical-device APNs delivery or signed configured→unconfigured installation was observed.
Residual risk: native/device evidence remains release-owned, but both assigned lanes independently established the retained-authority path from exact-head production source. Docker-backed local relay/Postgres reproduction was unavailable due organization sign-in; this does not weaken the author-actionable upgrade defect.
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