Repository navigation
Conversation
|
@codex 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a2076af70
ℹ️ 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>
Signed-off-by: Tom Brow <tomb@block.xyz>
Signed-off-by: Tom Brow <tomb@block.xyz>
48f2a73 to
a660019
Compare
Push banners arrive silently because the gateway's fixed APNs body never requests a sound, even though the iOS app requests sound permission. Add `aps.sound: "default"` to that constant so ordinary alerts request the system sound even if the notification extension is not invoked. Preserve the existing alert text, mutable-content flag, and class-blind, content-free delivery contract; synchronize its normative specification and executable models. Companion client workaround: [#8429](#8429). This gateway change also covers the original-payload fallback and requires no mobile source change. Sound remains subject to iOS settings and authorization. It does not bypass Focus or Silent Mode, select critical alerts, or guarantee vibration. Every public-profile wake requests the same sound, including generic/restricted fallback banners. ### Validation - Extended the real HTTP transport test: reproduced missing sound (`null` versus `default`) before the fix, then passed with the fix. All 79 runnable gateway library tests pass; 17 infrastructure/manual tests remain ignored. - Added a guard against drift between the emitted bytes, NIP-PL, and both formal models. Fixed-payload and stateful-delivery models and mutation checks pass. - Inspected the 0.24.0 (7) iOS source: sound permission is requested; ordinary enrichment and restricted fallback copy incoming content. A macOS UserNotifications probe confirms copy retention, but does not test iOS Intents or audible device delivery. - Focused existing native resolver/handoff tests pass (35 tests on macOS). These cover resolution and fallback behavior, not iOS sound playback. - Deployed exact head [b8a0eb6](b8a0eb6) to the personal dev push gateway (Railway deployment `b3e52fc8-d1d3-48d7-ae21-1a83c5db2e99`). Verified the dev relay already routes to that gateway. On an iPhone 12 mini running the existing Buzz Dev 0.22 (10140), without the client workaround, automation captured the test banner and Tom confirmed audible sound on October 18. - Broad local validation reached an unrelated ACP startup-deadline failure in `signals_cancel_startup_turn_and_pending_stdin_without_hanging`. No ACP source or dependency changed, and ACP does not depend on the gateway. The full local gate is not reported as passing. - Local desktop pre-push tests passed 3,420 cases but failed two unchanged agent-discovery checks (a 300 ms subprocess deadline and a login-shell PATH comparison). Desktop does not depend on the gateway. Retried publication with only the Rust-suite and desktop-Tauri lanes excluded under the standing unrelated-failure exception; other hooks remain enabled. ### Before merge - [x] Verify a real push banner and audible sound through the dev relay and candidate dev gateway on an iOS device without the client workaround. Signed-off-by: Tom Brow <tomb@block.xyz>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a660019997
ℹ️ 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>
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Review state: COMMENTED with one author-actionable P2. Repository policy requires agent reviews to be comments; recommendation is to fix this before merge.
Reviewed: bbb5183f33df3c6a5f7c587cc41233bc555cd15c...93a435284b29277c4516daa33e7fe47c081106e6 (exact clean head 93a435284b29277c4516daa33e7fe47c081106e6)
Risk: medium — this is a small change, but it alters the user-visible iOS notification-service handoff across ordinary, timeout, revoked-scope, and age-restricted delivery paths.
[P2] Bind regression coverage to the production notification-extension seam
mobile/ios/NotificationService/NotificationService.swift:109-110 contains the complete fix: default a missing sound, then construct the restricted fallback from the sound-bearing mutable copy. The surviving handoff suite at mobile/ios/BuzzPushKit/Tests/BuzzPushKitTests/BuzzNotificationHandoffTests.swift:6-61 exercises string handoff behavior only. There are no sound assertions in the current mobile tests, and NotificationService.swift is compiled only into the extension target, not the Swift package test target.
That means deleting line 109 makes old-gateway notifications silent again, and moving line 110 back above it makes the restricted/age-fallback path silent, without failing any current regression test. This is exactly the production-seam/falsifiability gap prohibited by AGENTS.md:234-238 and TESTING.md:25-32. Earlier commits on this PR added sound tests and a testable production constructor, then removed both; the final implementation is simpler, but its claimed contract is now unguarded.
Author action: add the smallest production preparation/handoff seam used by NotificationService.didReceive (or test the extension target directly), with assertions that:
- missing sound becomes
.default; - an existing custom/non-default sound is preserved; and
- the restricted fallback inherits the selected sound.
Mutation-bypass the production calls so removing either behavior causes a test failure. No broad refactor or physical-device test is required for this finding.
Verification owner: author for the regression test; exact-head Mobile Swift (Native) and Mobile Swift (Release) CI for compile/package verification.
Implementation assessment
No separate code defect found. The nil-only assignment preserves APNs-provided default, custom, and critical sound objects. Building restrictedFallbackContent from that copy carries the sound through ordinary resolver completion, revoked-scope fallback, extension expiry, and age restriction while preserving existing redaction. iOS remains authoritative for notification authorization, Focus, Silent Mode, and per-app sound settings. The merged gateway fix in #8431 is compatible, but its commit is release-coupled through main; it is not in this branch's ancestry.
Validation
- PASS — exact diff and lifecycle/caller trace;
git diff --check; clean worktree at matching head. - PASS — scoped search of
mobile/**tests found no sound assertion and confirmed the extension source is absent from package test targets. - PASS — exact-head Mobile Checks, Android, Flutter shards 2/3 and 3/3, Mobile Swift Release, DCO, Semgrep, zizmor, and security review.
- FAIL, apparently unrelated — Flutter shard 1/3:
departed channel composer and route are collectible(1178passed /1failed /1skipped). The PR diff is one native Swift file; the failure is in Dart route/composer retention. Author action: none unless rerun/investigation ties it to this PR. Verification owner: CI/mobile gate. - NOT RUN locally —
swift test --package-path mobile/ios/BuzzPushKitexits69before execution because this host has not accepted the Xcode license. Author action: none. Verification owner: reviewer tooling / native CI. - NOT YET TERMINAL at submission — exact-head Mobile Swift Native.
Manual/native evidence: the PR records audible real-device delivery on final build 10151 against the unchanged gateway, but that observation predates head 93a435284, which moved restricted-fallback construction. The ordinary path has device evidence; the final restricted path does not.
Residual risk: exact-head restricted-fallback audibility remains device-unwitnessed, native CI is still running, and the unrelated Flutter aggregate gate is red. Those are confidence/gate items, separate from the production-bound regression-test defect above.
|
🤖 Regarding “Bind regression coverage to the production notification-extension seam”: Tom explicitly directed that this temporary workaround have no added tests or helper seam. That task-specific instruction overrides the general testing guidance for this PR. The requested coverage was intentionally removed; we will retain the minimal implementation rather than reintroduce it. Your review found no separate implementation defect and confirmed nil-only sound preservation and restricted-fallback propagation. One factual correction: build 10151 was built and installed from the current head 93a4352, including the fallback-order adjustment. At approximately 23:20 UTC, automation verified its exact notification banner and Tom explicitly confirmed hearing the sound. This verifies ordinary delivery on the final head; it does not claim a forced restricted-fallback device test. The PR description reflects that evidence. Please reassess the recommendation under the explicit no-added-tests exception. The unrelated Flutter retention failure remains a required CI blocker; it is not waived by the device validation. |
Temporarily default missing notification sound until the [gateway fix](#8431) is deployed. Preserve any sound already provided by APNs. ### Validation Final build 10151: automation verified the push banner on a real iPhone 12 mini against the unchanged staging gateway, and Tom confirmed hearing the sound. iOS build, two existing handoff tests, and normal pre-push passed. ### Before merge - [x] Verify notification delivery and sound on the final iPhone build. Replaces [#8429](#8429) with the identical commit to obtain a fresh owner review. The prior review requested tests; this temporary workaround intentionally adds no tests or helper at the author's request. [Prior review response](#8429 (comment)). --------- Signed-off-by: Tom Brow <tomb@block.xyz>
Temporarily default missing notification sound until the gateway fix is deployed. Preserve any sound already provided by APNs.
Validation
Final build 10151: automation verified the push banner on a real iPhone 12 mini against the unchanged staging gateway, and Tom confirmed hearing the sound. iOS build, two existing handoff tests, and normal pre-push passed.
Before merge