fix(desktop): refresh channels after access-revoked closure - #7784
wesbillman wants to merge 4 commits into
Conversation
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
🔐 Codex Security Review
|
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Verdict: no blocking code findings
Reviewed head 806c1e4e2969fd60ab167fe0b6522c5ba952556a against base a61239f0d8036aff58176f5c0ce7f080c66e21b7. This is a comment review, not approval or merge certification.
The 27-line production change meets the stated recovery contract: only the exact access-revoked CLOSED reason on a known channel-scoped live subscription emits a refresh hint; existing terminal retirement happens first. It does not infer membership/archive state or restart the retired subscription. The hook reuses the existing debounce/idle-aware refresh and cleans up on unmount/session reset.
I traced the authoritative path through get_channels, native archive conversion and the stable list hash (which includes archive/membership), IPC conversion, query settlement, and sidebar/active-channel consumers. Successful updated state reaches existing sidebar filtering and stream composer read-only controls; a failed fetch retains the previous list rather than inventing an empty result. Reconnect replay checks the live map and subscription identity, so retired entries are not restored.
Validation and remaining gates
- Source review only, including the new six-case production-path fixture and complementary independent review lanes. No reviewer test execution or live archive mutation. The PR reports exact-head hooks and 6,514/6,514 desktop tests; I did not reproduce that run.
- The fixture binds real subscription creation, CLOSED dispatch, mounted hook, query refresh and IPC conversion, with native IPC stubbed. Native discovery, rendered sidebar and live incident ordering are not exercised. The existing busy→idle re-arm behavior is source/helper-test coverage, not a new integrated regression. These are disclosed evidence limits, not demonstrated code defects.
- The PR remains draft. Hosted desktop checks were still running at the review snapshot, full local
just ciis explicitly incomplete, and the security bot still requests review of this range. This review does not waive those gates. No code changes requested; complete outstanding validation before treating this as merge-ready. The broader symptom report #7026 and archived-channel delete policy remain outside this PR’s demonstrated result.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: a61239f0d8036aff58176f5c0ce7f080c66e21b7..806c1e4e2969fd60ab167fe0b6522c5ba952556a (exact head 806c1e4e2969fd60ab167fe0b6522c5ba952556a)
Risk: high — this changes relay subscription lifecycle behavior and the renderer's authoritative channel-refresh path after an authorization-related terminal frame.
Finding
P2 — the regression suite does not protect the PR's claimed exact CLOSED-reason boundary. Production correctly gates the hint on exact equality with restricted: channel access revoked (desktop/src/shared/api/relayClientSession.ts:801-806), but the negative case only sends an unrelated invalid: bad filter reason (desktop/src/features/channels/channelAccessRevocation.test.mjs:257-272). It never sends a near-miss restricted: reason on a known channel-scoped live subscription.
I broadened the production predicate to accept every string beginning with restricted:. All six new tests still passed (6/6, exit 0). That mutant would refresh the authoritative channel list for ordinary restricted closures, contrary to the PR's explicit exact-match contract. The repository requires regression tests to bind and falsify their production guard (TESTING.md:25-32; AGENTS.md:192-196).
Author action: add a known, channel-scoped live-subscription negative case using a different restricted reason, such as restricted: not a channel member, and assert that no channel refresh occurs. Verify that broadening the predicate to startsWith("restricted:") makes the test fail, then restore the exact predicate and prove it passes.
Verification owner: author for the regression addition; reviewer to mutation-check the final exact head.
Integrated contract trace
The production path itself appears sound:
- The hint is accepted only for the current connection generation, a known live subscription with a non-empty
#h, and the exact access-revoked reason. Unknown, retired, global, stale-generation, and unrelated frames are ignored. - Existing CLOSED handling remains terminal: the subscription is removed before listeners run and cannot be replayed on reconnect.
- CLOSED remains only a hint. Archive, removal, and membership state still come from the authoritative
get_channelsquery; sidebar/composer behavior derives from that result. - Burst hints use the existing trailing, idle-aware refresh. Failed refreshes retain cached data and do not create a new retry loop. Unmount and community disconnect remove listeners and queued work.
- Archived, removed, unchanged, and failed authoritative outcomes are exercised through real subscription creation/CLOSED dispatch and the query/cache path. No new visual or accessibility control is introduced.
Validation
At exact clean head 806c1e4e2969fd60ab167fe0b6522c5ba952556a:
- Full Desktop package suite: 6,514/6,514 passed, 0 failed.
- Focused production-path fixture: 6/6 passed.
- Causal removal of hook registration and suppression of hint emission each failed the positive outcome rows as expected.
- Exact-boundary broadening mutant (
startsWith("restricted:")): 6/6 still passed — reproduces the finding. pnpm typecheck,pnpm check, andgit diff --check: passed/clean.- Hosted Desktop core, four smoke shards, Windows/macOS builds, integration shards, DCO, Semgrep, and zizmor: green at the reviewed head.
Manual/native evidence: none. The fixture stubs native IPC while exercising the renderer subscription/query seam; no live-relay archive/removal journey or rendered-pixel capture was performed.
Residual risk: the exact incident ordering and busy-to-idle behavior were source/helper traced rather than reproduced through a live native journey. These are confidence gaps, not additional author defects.
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
|
Pinky is commenting on Wes’s behalf. @jedwards27 Addressed the exact-reason regression finding in
|
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE at exact head 51ec7482ad1e600bd02ea6b9ed265db8a70e7266 against base c213d90d5c450e2e579505da4a4145f528ecbcf2.
No author-actionable defects found. The prior blocker is fixed: the new production-path negative control uses the near-miss reason restricted: not a channel member, remains terminal, advances the real debounce, and proves zero authoritative reads. Broadening the production predicate from byte-exact equality to startsWith("restricted:") makes exactly that row fail (6/7), while restoring the predicate returns 7/7.
The merged/refactored lifecycle also preserves the intended contract:
relayClientSession.ts:865-938generation-fences inbound frames, captures the owned subscription, performs terminal cleanup, and only then classifies the CLOSED frame through the extracted helper.relayChannelAccessRevocations.ts:18-26emits a hint only for a live channel-scoped#hsubscription with the exactrestricted: channel access revokedreason.useLiveChannelUpdates.ts:163-178,365-381,486-495owns one trailing, idle-aware authoritative invalidation listener and cancels the timer/listener on unmount. The CLOSED reason remains only a hint; authoritative channel state determines archived, removed, unchanged, and failed outcomes.- Terminal cleanup deletes the subscription before reconnect replay (
relayClosedRecovery.ts:135-143;relayClientSession.ts:1042-1056), so reconnect cannot resurrect the revoked subscription. Disconnect/community switch clears listeners and subscriptions before the keyed subtree remounts. - The refactor satisfies package policy:
relayClientSession.tsis 1,197 lines under the 1,200-line frontend ceiling; the isolated helper is 28 lines and the differential size gate passes.
Exact-head validation from clean detached checkouts:
- Full Desktop package suite: 6,589/6,589 passed.
- Focused production fixture: 7/7 passed.
- Exact-reason broad-predicate mutant: expected failure, 6/7.
- Removing the production notification seam: expected failure in all four authoritative outcome rows; restoration returned 7/7.
pnpm --dir desktop typecheck: passed.pnpm --dir desktop build: passed (existing chunk/dynamic-import warnings only).pnpm check, differential file-size gate, andgit diff --check: passed; policy audit found no added productionunwrap/expect, exported APIs are documented, and commits are signed off.- Live preflight: head/base remain pinned, exactly four Desktop files (
+379/-0), PR mergeable. Desktop integration, Windows/macOS builds, DCO, Semgrep, zizmor, and release-candidate checks are green. Desktop Core and four Smoke shards remain pending; CI owns those gates.
Author action: none.
Residual risk / verification owner: CI must complete the pending exact-head Desktop Core and Smoke jobs. No live-relay/native rendered journey was run; this change is transport/query-state recovery with strong production-seam coverage, so that is a reviewer/tooling confidence gap rather than author rework.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: c213d90d5c450e2e579505da4a4145f528ecbcf2..51ec7482ad1e600bd02ea6b9ed265db8a70e7266 (exact head 51ec7482ad1e600bd02ea6b9ed265db8a70e7266)
Risk: high — subscription lifecycle and authoritative channel-state recovery affect access-revocation correctness and sidebar state.
Behavior/contracts traced: current-generation CLOSED dispatch; terminal subscription removal and drain cancellation; exact reason/channel-scope classification; reconnect and community-switch cleanup; idle-aware authoritative query invalidation; archive/remove/unchanged/failure outcomes; unmount cleanup; file-size policy.
Findings: no unresolved author-actionable defect. The prior exact-reason test blocker is closed. channelAccessRevocation.test.mjs:291-305 now drives a known channel-scoped live subscription through restricted: not a channel member and asserts no authoritative read. relayChannelAccessRevocations.ts:18-26 preserves exact equality with restricted: channel access revoked; the hint is emitted only after existing terminal handling and never becomes authoritative state. Authoritative archive/removal/membership still comes from the channels query.
Author action: none.
Verification owner: exact-head CI/merge protection for remaining aggregate gates.
Validation at matching head:
- Full Desktop package suite: 6,589/6,589 PASS in both lanes.
- Focused production-path fixture: 7/7 PASS.
- Broadening equality to
startsWith("restricted:"): expected FAIL 6/7, solely on the new near-miss row (actual 1 !== expected 0). - Removing hint emission: expected failures in all four authoritative outcome rows; restoring exact head returned 7/7 PASS.
pnpm --dir desktop typecheck: PASS.pnpm --dir desktop build: PASS (existing warnings only).pnpm check, differential file-size gate, andgit diff --check: PASS.relayClientSession.tsis 1,197 lines under the 1,200-line ceiling; the extracted helper is 28 lines.- Live head refreshed immediately before submission and remained
51ec7482ad1e600bd02ea6b9ed265db8a70e7266; GitHub checks were 20 passing, 0 failing, 5 pending.
Manual/native evidence: no native app or live-relay archive/removal journey. The production-seam fixture binds actual subscription creation, raw CLOSED dispatch, listener/debounce, query settlement, and conversion; it does not witness native IPC or rendered sidebar pixels.
Residual risk: exact incident ordering and a native/live-relay journey remain unwitnessed. Those are reviewer/tooling or follow-up verification gaps, not author rework for this bounded recovery mechanism.
Pinky is updating this PR on Wes’s behalf.
Summary
CLOSED restricted: channel access revokedon a known channel-scoped live subscription as a hint to refresh authoritative channel state.useLiveChannelUpdates’ existing debounced, idle-aware query refresh. Keep terminal retirement, explicit membership denial, and polling unchanged.The hint does not establish archive or membership state. This closes a missed-update recovery gap without changing the relay or blindly retrying subscriptions.
Related symptom report: #7026 (not claimed fully resolved; archived-channel delete policy is separate).
Related new-client fix: block/buzz-app#106
Originating conversation: buzz://message?channel=cb38cd5a-c920-40a1-b770-e9b89c289476&id=d22c1eed473016cf1f73c889a4cc88f1491ee092cfe3794575f8fea8003aa19e
Review follow-up and main integration
At
51ec7482ad1e600bd02ea6b9ed265db8a70e7266, based on mainc213d90d5c450e2e579505da4a4145f528ecbcf2:restricted: not a channel memberstays terminal but must not refresh channels.startsWith("restricted:")produced 6,588 passes / 1 expected failure, specifically the new no-refresh assertion (actual: 1,expected: 0). Restored the exact predicate; the final full suite passed 6,589/6,589.relayChannelAccessRevocations.ts, with unchanged session ownership and cleanup. No limit change, new timer, retry policy, or unrelated refactor. Final feature diff: 46 production additions across three files, plus the 333-line regression fixture.Validation
At exact pushed head
51ec7482ad1e600bd02ea6b9ed265db8a70e7266:jedwards27; the changes-requested review and required approval remain for the reviewer to update.Evidence boundaries
just ciattempt hit a 300-second timeout compilingbuzz-voice; full localjust ciremains incomplete. Current delivery uses mandatory path-scoped hooks and fresh hosted CI; no unrelated native/mobile lane was bypassed or claimed rerun locally.