docs: specify desktop-driven mobile push suppression - #7809
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. |
|
@buzz-security-review 11554f3 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11554f352f
ℹ️ 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".
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: c213d90d5c450e2e579505da4a4145f528ecbcf2..11554f352fcce9bae044c1ad37888da545b8e075 (exact head 11554f352fcce9bae044c1ad37888da545b8e075)
Risk: high — this docs-only draft defines notification-loss and privacy contracts that later implementations will treat as normative.
Behavior/contracts traced: Desktop activity eligibility; per-community/per-installation preference semantics; relay suppression leases and final queued-push revalidation; multi-session expiry; gateway ownership; VISION_MOBILE.md offline/trust requirements; user outcomes when Desktop cannot alert.
Findings:
-
Blocking — rule 29 lets a broken or uncooperative relay prevent the user from turning notifications off.
docs/mobile-push-suppression.md:81reverts an unconfirmed preference change after about ten seconds and permits later relay sync to restore the relay's value. That conflicts with the controlling contract inVISION_MOBILE.md:3-14: critical intent, explicitly including “stopping notifications from a relay,” must never depend on a relay response. It also collapses desired local state and relay-confirmed delivery state, so an ambiguous acknowledgement can silently turn notifications back on.Author action: Make disable locally authoritative and durable across restart, retrying relay/gateway cleanup independently; enabling may remain relay-confirmed. Alternatively, change
VISION_MOBILE.mdthrough its owner-reviewed product path and explicitly accept/document the offline regression. Specify timeout-before-commit, timeout-after-commit, restart, reconnect, stale-sync, and cross-install behavior. -
Blocking — the claimed “redundant” mobile alert may be the only deliverable alert. The Purpose says suppression avoids redundant pushes (
docs/mobile-push-suppression.md:7-9), but rule 7 suppresses mobile when Desktop alerts are disabled, OS permission is denied, or Focus silences them (:29); rule 27 supplies no override (:77), in-app activity may keep suppression active after focus is lost (:33), and suppressed pushes are never replayed (:57-61). A user working in another app can therefore receive no alert anywhere for a DM or mention, with no recourse. That is a materially different product contract from avoiding redundancy.Author action: Choose and document a safe contract before labeling these as agreed requirements: either require an eligible Desktop notification path, or provide an explicit accessible suppression control/disclosure with its default and migration behavior. If zero-alert delivery is intentional, state that plainly in Purpose and examples rather than describing the mobile alert as redundant.
Verification owner: spec/product owner and Mobile vision owner for both contract decisions; implementation owner later for the offline preference and notification-delivery matrices.
Validation: exact-head source/diff review; git diff --check passed independently in the review lane; repository just check passed in the product lane; GitHub exact-head checks completed with no failures (docs path selection skipped runtime suites). Authenticated reviewer jedwards27 differs from PR author brow.
Manual/native evidence: none required for this explicitly “draft, not ready for implementation” documentation change.
Residual risk / non-blocking open work: The draft honestly leaves session authentication/isolation, logout, reconnect ordering, stale replay/clock handling, known-transition propagation, and privacy-preserving installation aggregation unresolved. Before implementation-ready status, bind renewals to account + community + unique Desktop session, define relay-time admission and monotonic ordering, fail open to push under uncertainty, and place suppression in the relay's final pre-transport queued-push revalidation. These are confidence gaps/open design work, not additional defects in this draft.
— :bot: Jude’s code review agent
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: c213d90d5c450e2e579505da4a4145f528ecbcf2..11554f352fcce9bae044c1ad37888da545b8e075 (exact head 11554f352fcce9bae044c1ad37888da545b8e075)
Risk: high product-contract risk despite a documentation-only diff. These agreed requirements govern whether message alerts are silently withheld and whether a user's opt-out survives relay failure.
Blocking findings
1. Major — rule 29 makes “stop notifications” depend on the relay
docs/mobile-push-suppression.md:81 requires a preference change to revert after roughly ten seconds without relay confirmation and allows a later sync to restore the relay's value. Applied to disabling push, that directly conflicts with the controlling mobile contract: critical intent including “stopping notifications from a relay” must never be blocked on a relay response (VISION_MOBILE.md:3-14). It also regresses the current fail-closed behavior: mobile durably stores pushNotificationsEnabled = false, journals a higher-generation tombstone, and retries cleanup after reconnect (mobile/lib/shared/community/community_provider.dart:461-504); the UI immediately renders that local state as off (mobile/lib/features/settings/settings_page/notifications_section.dart:34-36,64-79). The PR body acknowledges the tension, but rule 29 is still labeled an agreed requirement.
Consequence: a broken or uncooperative relay can prevent opt-out or silently turn notifications back on after an ambiguous acknowledgement.
Author action: make disabling locally authoritative and durable across restart, with relay/gateway cleanup retried independently; distinguish desired local state from relay-confirmed delivery state. Enabling may remain relay-confirmed. The alternative is an owner-reviewed change to VISION_MOBILE.md that explicitly accepts and explains the regression.
Verification owner: mobile vision owner + spec author. The revised contract should cover offline disable, timeout before/after relay commit, lost acknowledgement, restart, reconnect, stale sync, and sibling-installation isolation.
2. Major — the “redundant” alert rationale permits zero alert paths with no user recourse
The Purpose says suppression avoids redundant mobile pushes (docs/mobile-push-suppression.md:7-9), but rule 7 suppresses mobile even when Desktop alerts are disabled in Buzz, denied by the OS, or silenced by Focus (:29). Rule 9 can continue suppression for ten minutes after Buzz loses focus based only on earlier in-app activity (:33), rule 19 never replays the discarded alert (:57), and rule 27 offers no suppression override (:77).
Consequence: a user can receive neither a Desktop alert nor a mobile alert for a time-sensitive DM, mention, or thread reply. The unread message remains, but the alert is permanently discarded. Calling that mobile alert “redundant” obscures the actual product choice.
Author action: choose and state a coherent user-trust contract before these become agreed requirements: either require an eligible Desktop notification path for suppression, or provide an explicit accessible suppression control/disclosure with its default and migration behavior. If intentionally accepting zero alert paths, say so plainly in Purpose and behavioral examples rather than describing the mobile alert as redundant.
Verification owner: product/spec owner. Later implementation proof should cover Desktop permission off, Buzz alerts off, OS Focus, background/unfocused fallback, lock/sleep, and mobile delivery.
Architecture and residual design work
The relay-as-suppression-authority / gateway-agnostic split is coherent with the current seam: relay matching creates installation-specific wakes and performs a final generation revalidation before transport (crates/buzz-relay/src/push_runtime.rs:248-313,384-469), while gateway delivery requests carry only an opaque grant, request ID, and expiry (crates/buzz-push-gateway/src/model.rs:32-43). Implementation should put suppression into that final relay revalidation, not only initial matching.
The draft honestly leaves authentication, unique desktop-session identity, logout, stale-session arbitration, reconnect ordering, clock/skew handling, signal contents, mobile-permission semantics, and cross-identity/community/relay/install isolation unresolved (docs/mobile-push-suppression.md:108-116,124-132,159). Those are confidence gaps for this explicitly non-implementation-ready draft, not additional blockers. Before implementation-ready status, specify monotonic per-session ordering, delayed/replayed-signal rejection, known-transition propagation bounds, fail-open behavior, and accessible pending/failure/repeated-toggle UX.
Validation:
- Exact-head clean checkout confirmed before validation.
git diff --check c213d90d5c450e2e579505da4a4145f528ecbcf2..HEAD— pass.- Local Markdown relative-target probe across both added documents — pass; no tabs or trailing whitespace.
- Independent full repository
just checkat the exact head — pass; final mobile analysis reported no issues. - GitHub exact-head checks observed with no failures: path detection, DCO, Desktop Release Candidate, Semgrep, and zizmor passed; runtime lanes were skipped by docs-only path selection.
Manual/native evidence: none required for a document explicitly marked “draft, not ready for implementation.”
Residual risk: no runtime protocol or device behavior is proven. That is appropriate at this stage, but implementation must not begin until the deferred identity, timing, ordering, privacy, and failure contracts are closed.
Signed-off-by: Tom Brow <tomb@block.xyz>
Signed-off-by: Tom Brow <tomb@block.xyz>
|
🤖 Review dispositions for the new head:
Local links, anchors, whitespace checks, and commit/push hooks passed. No runtime files changed. Broad local builds were not repeated because of the documented disk-space constraint. Please review these revised contracts. |
|
@codex review |
|
@builderbot review |
|
@buzz-security-review 71241eb |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE
Reviewed exact head 71241eb6272bb9d7e11e0f01e60bca1ce12d9b5e against base c213d90d5c450e2e579505da4a4145f528ecbcf2 from systems/integration and product/UI perspectives. The two prior blockers are resolved as explicit, internally consistent product contracts:
VISION_MOBILE.mdnow explicitly permits relay-confirmed push preference changes and requires bounded pending, visible failure/uncertainty, warning that notifications may continue, no automatic retry, and separate recourse. Rules 28–31 and the recovery examples carry those requirements through with per-installation ownership, restoration of the last confirmed value, explicit retry only, no queued intent across restart, and authoritative relay resync.- The spec now plainly states that suppression may leave both devices silent—including for DMs and mentions—and that suppressed pushes are not replayed. The behavioral rules and matrix consistently preserve unread state while making Desktop alert availability irrelevant. The former “avoid redundant notifications” framing is gone.
No unresolved author-actionable defect remains in this explicitly non-implementation-ready design draft.
Residual implementation risks remain, with no author action required for this draft:
- Product/Mobile UX must design discoverable user disclosure and the separate recourse required by the vision before implementation-ready status.
- Mobile UI/accessibility must validate pending/failure announcement, focus behavior, repeated toggles, text scaling, reduced motion, and operable retry.
- Mobile implementation must migrate the current durable local desired-state/retry behavior without replaying stale intent or affecting sibling installations; cover offline/reject, commit-with-lost-ack, no-commit timeout, restart, resync, upgrade, and multiple-installation cases.
- Product plus Desktop/Mobile/relay owners must close the explicitly deferred identity, logout, ordering/timing, propagation, OS-permission, reconnect, and privacy-preserving aggregation contracts before implementation.
Verification at the reviewed head:
- Complete
+242/-1three-file Markdown diff reviewed againstAGENTS.md,VISION.md,VISION_MOBILE.md,TESTING.md, NIP-PL, and the current Mobile preference flow. - Independent clean exact-head checks reported
git diff --checkand Markdown file/anchor validation passing. - Live remote head remained
71241eb6272bb9d7e11e0f01e60bca1ce12d9b5eimmediately before submission. - All required GitHub checks were passing or intentionally skipped; none were pending or failing.
- No runtime/native behavior is claimed for this documentation-only draft.
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: c213d90d5c450e2e579505da4a4145f528ecbcf2..71241eb6272bb9d7e11e0f01e60bca1ce12d9b5e (exact head 71241eb6272bb9d7e11e0f01e60bca1ce12d9b5e)
Risk: high product-contract impact, low runtime risk — documentation only, but it defines notification-loss, relay-trust, and preference semantics for later implementation.
Behavior/contracts traced: Desktop suppression eligibility; zero-alert outcomes; per-installation preference ownership; bounded pending/failure UX; relay acknowledgement and authoritative resync; restart/reconnect behavior; relay/gateway ownership; controlling VISION_MOBILE.md contract.
Findings: no unresolved author-actionable defect.
The two prior blockers are resolved:
- The controlling vision now explicitly distinguishes locally enforceable actions from relay-controlled push preferences (
VISION_MOBILE.md:13). Rule 29 carries that decision through with bounded pending, immediate known failure, restoration to the last confirmed value, explicit “notifications may continue” copy, no queued/restart retry, explicit retry only, and authoritative resync (docs/mobile-push-suppression.md:81-85). Rules 28/31 retain per-installation ownership and prevent cached state from becoming a new choice; examples cover reject/offline, lost acknowledgement, restart, and reconnect (:109-111). - The Purpose and behavioral matrix now state plainly that suppression can leave both devices silent, including for DMs and mentions, and that suppressed pushes are not replayed (
docs/mobile-push-suppression.md:7-9,94). The former “redundant notification” characterization is gone. The tradeoff is severe, but it is no longer hidden or internally contradictory.
Author action: none.
Verification owner: required CI gates for the four still-running Desktop jobs; Product/Mobile UX before implementation-ready status for user disclosure, relay-abuse recourse, pending/failure accessibility, defaults/migration, and repeated-toggle behavior; Desktop/Mobile/relay protocol owners for the explicitly deferred identity, ordering, clock, reconnect, and privacy mechanics.
Validation: clean detached checkout at exact head; follow-up delta reviewed against AGENTS.md, VISION_MOBILE.md, current Mobile preference ownership, and the relay/push seam; git diff --check 11554f352fcce9bae044c1ad37888da545b8e075..71241eb6272bb9d7e11e0f01e60bca1ce12d9b5e passed. Independent lanes also report complete-head git diff --check and Markdown target/anchor validation passed. GitHub exact-head checks had no observed failures; four Desktop jobs remained in progress at submission.
Manual/native evidence: not run and not claimed; the document remains explicitly “draft, not ready for implementation.”
Residual risk: implementation must not replay today's durable local desired state over newer relay-confirmed per-installation state. Separate relay-abuse recourse and automatic-suppression disclosure remain product work. These are named confidence gaps for the implementation-ready phase, not defects requiring this draft's author to change the current head.
— :bot: Jude’s code review agent
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear for this documentation draft
Reviewed HEAD 71241eb6272bb9d7e11e0f01e60bca1ce12d9b5e against BASE c213d90d5c450e2e579505da4a4145f528ecbcf2. No author-actionable defect found in the three-file change. This is not implementation approval or product-owner sign-off on the Mobile vision change.
- Product contract:
docs/mobile-push-suppression.md:9,29,57,79-87now explicitly permits neither device to alert, including for DMs/mentions, retains unread state without notification replay, and defines relay-confirmed preferences per installation/identity/community. Immediate known failure, bounded pending, lost-acknowledgment ambiguity, explicit retry, restart, and authoritative resync are consistent with the proposed exception inVISION_MOBILE.md:13; cached state cannot become a new settings choice. The vision change remains subject to its document-owner review requirement (VISION_MOBILE.md:7). - Systems contract: native-desktop activity, multiple eligible sessions, community-scoped connectivity, known lock/sleep/quit, 30-second stale expiry, ten-minute inactivity and five-second tolerance, private signaling, and queued-versus-already-sent pushes form a coherent draft contract. Relay-side enforcement preserves the NIP-PL relay/gateway boundary and independent installation leases. I verified the existing matching/final-revalidation seam in
crates/buzz-relay/src/push_runtime.rsand the opaque gateway delivery request. Browser-originated, mobile-to-mobile, calls/non-message alerts, and sender overrides are explicitly outside the initial scope. - Readiness limits: the draft correctly says it is not ready for implementation. Authentication/session binding, delayed-signal rejection, ordering/clock bounds, privacy-preserving installation aggregation, accessible failure recovery, independent relay-abuse recourse, and migration from the current durable local disable/tombstone flow still need design and implementation evidence. None is represented here as already implemented. The current mobile flow (
mobile/lib/shared/community/community_provider.dart:461-504) differs deliberately from this proposed contract; a future implementation must address that transition.
Validation: source-only on the pinned Blox host, including exact-base repository guidance, vision, testing/architecture documents, NIP-PL, the complete changed documents, and complementary product/UX and systems reviews. No checkout, build, tests, repository-code execution, or CI reruns. Existing exact-head CI metadata includes a failed Desktop Smoke E2E (4) and failed Desktop aggregates; logs were not investigated and causality is not attributed to this docs-only PR. This review does not claim green CI or verified device behavior.
A documentation-only update to #7809 selected desktop builds and E2E tests because path detection compared an old PR base SHA with GitHub's newer synthetic merge commit. The [failing run's path-detection log](https://github.com/block/buzz/actions/runs/35789957773/job/106955810630) includes four unrelated desktop files from `main`; `VISION_MOBILE.md` did not match a runtime filter. Use GitHub's PR file list for pull requests so unrelated changes in the synthetic merge commit cannot select runtime suites. The existing directory filters are unchanged: root and docs/ Markdown do not select runtime suites, while Markdown under runtime directories still selects its affected suites. Mixed code/documentation changes retain normal coverage. Always-on security, policy, and source-contract checks and full push-to-main coverage are unchanged. Added regression coverage runs the pinned paths-filter action against real fixture repositories, including a synthetic merge containing unrelated upstream desktop code. All 35 selection scenarios and 26 required-check scenarios pass; the new regressions failed before the fix. Existing required-context isolation, file-size policy, and security-review contract checks pass, as do script lint and workflow syntax validation. Full workflow lint reports the same two pre-existing shell-quoting findings in the untouched dead-token guard. A desktop E2E build was run to diagnose the unrelated required smoke failure. Related: #7809 (incident, unchanged) and #5756 (shared-input path coverage, separate scope). No matching issue found. PRs with at least 3,000 changed files now select every runtime suite, avoiding GitHub's PR-file-list ceiling. Boundary tests cover 2,999, 3,000, and 3,001 files, including a runtime file omitted beyond the API cap; disabling the safeguard makes the latter two regressions fail. Required checks now run and fail when path selection fails, is cancelled, or is skipped. Regression coverage exercises the workflow conditions and shell checks for all 13 required wrappers, plus an API-denial case against the pinned action. Mutating either the scheduling guard or the result check makes all 13 failure regressions fail. The required mention-settings smoke test expected a pin after explicitly disabling automatic mentions. Waiting for its old avatar to exit reproduced the CI failure 3/3; the test now checks that subsequent mentions remain manual and retains outgoing recipient-tag assertions. Corrected browser regression: 20/20 repeated runs; related picker unit tests: 18/18. No desktop production behavior changed. The always() requirement is now pinned in required-wrapper regression tests; deleting it fails all 26 gate cases. --------- Signed-off-by: Tom Brow <tomb@block.xyz>
|
Test failure due to a desktop voice-note test in Smoke E2E shard 4. this has been fixed in #7821. Bringing the fix above into this branch to resolve the test failure. |
…in-ui * origin/main: feat(buzz-relay): NIP-FI stateless enforcement (S3) — upgrade gate, NIP-42 pairing, session lifetime, JWKS warm (#7224) feat(web): add Browse releases link next to invite download (#2255) docs(nips): fix stray angle brackets in created_at clauses (#4486) docs: specify desktop-driven mobile push suppression (#7809) feat(mobile): add the contextual identity-name resolver (#7894) Automate owner deletion preparation (#7830) feat(relay): implement NIP-AR channel artifacts (#7919) fix(desktop): resolve unlisted project channel requests (#7619) Schedule the deletion drain safely (#7827) fix(mobile): stale community selection during mobile invite setup (#7951) Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz>
Summary
Define when desktop activity suppresses otherwise eligible mobile message notifications: a ten-minute inactivity window, short-lived relay-owned suppression, and per-installation push preferences. Include a shared glossary and renewal timing diagram. The gateway remains unaware of suppression.
This is a design draft ready for review, not an implementation-ready protocol. Wire format, synchronization, and several failure-handling details remain explicitly open. Please also review the tension between the agreed relay-confirmed preference changes and VISION_MOBILE.md's requirement that stopping notifications must not depend on a relay response.
Related issue
Related: #3234 covers per-channel notification settings and push filtering; this spec focuses on desktop-activity suppression. No duplicate spec PR found.
Testing
Documentation-only change. Local Markdown links and anchors resolve; whitespace validation and commit hooks passed. The full local CI gate could not complete: dependency compilation exhausted local disk space (OS error 28). GitHub checks currently report no failures.