Conversation
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Requesting changes at exact head 38c768109a29ff66a3c17b71ff46ff9e183ee847 (base 156b7ec31b84ac642d4cc52e5d35debb88ea59a4). Two authorization-lifecycle defects currently violate the PR's fail-closed/session-owned contract.
1. Reconnect leaves old evidence capabilities current
queryRelay captures _connectionGeneration and declines to observe a late HTTP response after generation drift (mobile/lib/shared/relay/relay_session.dart:201-244). However, _connect advances that generation without clearing/retiring evidenceClock (:531-555); the clock is cleared only during provider rebuild/disposal (:155-165, :1016-1041).
A selected-mention read begun on generation N can therefore return after reconnect to N+1. Its response is not observed, but snapshot(...) can still compare it against generation-N clock cells and report current. Publication then captures N+1 and may enqueue using generation-N roster/profile/runtime/policy evidence (:364-396). An automatic, resume, or explicit reconnect can consequently carry stale authorization onto the replacement socket.
Reproduction: replacing the PR test's provider invalidation with the production generation transition (debugSupersedeConnection) makes flutter test test/shared/mentions/selected_observation_test.dart fail: the pending reader returns authorization instead of the expected StateError. The shipped test passes because provider disposal clears the clock, so it does not cover ordinary reconnect.
Author action: retire all outstanding evidence capabilities whenever a socket generation is superseded, not only on provider disposal. Add a production-seam regression that blocks an HTTP query, replaces the ordinary connection generation without provider invalidation, releases the old response, and proves the read and queued publication fail closed. Preserve the delayed-old-response isolation assertion.
Verification owner: reviewer at the replacement immutable head; rerun the causal reconnect mutation and full mobile package gate.
2. A consented invitation can invalidate itself and cancel publication
The composer stores every successful read's isCurrent closure in one attempt-wide set (mobile/lib/features/channels/compose_bar/compose_bar_widget.dart:496-497; selected_mention_preparation.dart:41-43). All later read and post-write boundaries evaluate every old closure (compose_bar_widget.dart:510-523; helpers.dart:473-486).
A successful kind:9000 invitation legitimately changes kind:39002. If the active session observes that expected roster update before the write future settles, the pre-write roster snapshot becomes false and the immediate post-write ensureCurrent() throws before the required fresh read can supersede it. This contradicts the API's explicit “re-read after consent and accepted invitations” contract (mobile/lib/shared/mentions/selected_mention_authorization.dart:49-51). The user can consent, the membership can durably change, and the message can still be cancelled. Multiple invitees also expose partial-prefix behavior.
A production-composer-shaped widget regression using generation-bound SelectedMentionAuthorization.isCurrent reproduces this at the shipped head: after accepted invitation invalidates the old generation, the expected publication is absent. A candidate correction passes only when the newest read replaces prior capabilities and the stale post-write check occurs after the mandated re-read, while retaining the synchronous pre-enqueue guard.
Author action: model composer evidence as a replaceable latest-read generation rather than a monotonic union. After accepted membership writes, re-read before evaluating superseded roster capabilities, while preserving the guard immediately before each side effect. Add composer-level regressions for invite accepted → active session observes updated 39002 → post-invite read → message publishes, plus multiple-invitee/partial-prefix behavior.
The new queued-publication test at mobile/test/shared/mentions/selected_observation_test.dart:172-181 manually wires evidence.values.every(isCurrent) into withRelayPublicationGuard; it bypasses the changed _authorizeSelectedMentions → evidenceChecks → guardedDelivery path. Removing the production accumulation can leave that test green. Please bind policy/profile churn through the real composer send callback and mutation-prove the production invocation.
Verification owner: author for the production-seam regressions, then reviewer exact-head rerun and mutation check.
Validation and residual risk
- Live PR head and authenticated reviewer identity were rechecked before submission; reviewer
jedwards27is distinct from authorloganj. - Focused shipped tests pass, but the two production-shaped causal mutations above expose the defects.
git diff --checkwas clean. No rendering/accessibility delta was found; existing failure surfaces are reused.- Full mobile suite and native iOS journey were not independently rerun because these deterministic blockers already require a replacement head. CI was still in progress when reviewed.
156b7ec to
e5deefd
Compare
f80fabd to
668f1ff
Compare
e5deefd to
883a16b
Compare
|
Updated production-boundary/lifecycle evidence, including review 5158704956; this replaces the prior candidate evidence in place. Published SEND correction — 2026-09-10Head Cumulative coverage remains conditional on the existing stack: #7527 owns scope/transport and capacity/generation foundation, #7534 classification, #7536 complete observed-evidence binding, and #7539 production-boundary journeys. This is not standalone authorization completeness at an intermediate parent or a backport to #7387. The original reviewed parents' gaps are acknowledged rather than relabeled as already fixed there. R1/R2: empty latest required audience retires obsolete recipient proof only after successful operation-scope validation; retained recipients remain protected. Genuine community changes use a typed error and invitation StateError revalidates actual scope. Unavailable authority is not falsely described as a community switch. Accepted invitation prefixes remain irreversible and accounted for. A downstream onSend adapter can still encounter generic disconnected-session failure before the typed guard: no enqueue, but the specific community explanation is not exhaustive. The 41-row matrix includes nine added real-boundary journeys. Production rows use the actual default provider/HTTP reader and real SendMessage → signed-event relay → session. Removing reader currentness or composer aggregation independently leaks kind9; invitation counterparts leak accepted kind9000. Removing only composer guardedDelivery leaks kind9 after real draft editing while SendMessage's wrapper remains. Removing default-provider profile forwarding fails consent/post-ACK continuity (first failing assertion is unwanted kind9, not an independently logged kind9000 failure). Pure capacity and no-capacity profile-revocation rows isolate those causes; the waiting-revocation row does not independently separate coordinates from aged capacity. EMPTY/TYPED mutations fail their observable delivery/UI assertions. Source was restored. These are local mutation receipts on equivalent production, not new published-head mutation executions. Validation: exact reviewed candidate full-mobile suites passed 2137/2152/2162/2188 for #7527/#7534/#7536/#7539. All mobile production AND test tree objects are byte-identical after mechanical Desktop-only carry; no unnecessary mobile full rerun is claimed. Own-range source-size gates pass after carry; format/analyzer receipts apply to unchanged mobile bytes. Desktop JS package 6450/6450 passed; forum 19+63 and video 7 E2E / 4 buffering unit cases are scoped receipts on identical runtime inputs, not complete Playwright/CI passes. Private16 is now Independent delta review e6ec accepted the exact mobile candidates with the qualifications above; old GitHub approvals are not transferred. Normal push-triggered CI is running, not yet green; security skips are not a security verdict. No rerun wave, dispatch, merge or new PR. Evidence: |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE at exact head 668f1ffbf7fa192270593846dfa7dbeea5322278 (base 883a16baeebf07044bf96addba7f095a22dfaf0d).
The replacement fixes both prior lifecycle blockers:
- Connection replacement now retires outstanding evidence through the production
_connect→_supersedeConnectionpath, while delayed HTTP observation remains generation-gated (mobile/lib/shared/relay/relay_session.dart:196-244,421-436,547-570). - Complete authorization reads replace rather than union attempt evidence. After an accepted kind:9000 write, the unchanged operation scope permits the required fresh read, which becomes the sole evidence used for final publication (
mobile/lib/features/channels/compose_bar/selected_mention_preparation.dart:19-116;mobile/lib/features/channels/compose_bar/helpers.dart:475-529). This preserves valid self-observed roster progress and multi-invite/partial-prefix handling without stale revival. - Publication remains fenced across relay capacity and active socket generation, with no await between the final operation check and socket send (
mobile/lib/shared/relay/relay_session.dart:362-396,428-436). Accepted partial effects are counted and surfaced truthfully.
Exact-head verification:
- Full mobile package: 2,169 passed.
- Focused production reader + composer suites: 173 passed.
just mobile-check: passed; analyzer clean.just file-size-check: passed.- Causal mutation: removing reconnect clock retirement failed the production replacement-socket regression.
- Causal mutation: restoring attempt-wide evidence union failed the accepted-roster and multiple-accepted production journeys.
git diff --check: passed; worktree clean; reviewed local SHA matched the live head.
Author action: none.
Confidence gaps / verification owners: exact-head Mobile and Mobile Swift gates were green; Desktop Core/Smoke jobs were still pending at the final lane poll and own terminal completion. Native iOS/live-relay/VoiceOver acceptance was not run; this non-rendering authorization-lifecycle delta has full Flutter/widget production-path and mutation evidence, so that remains reviewer/tooling confidence rather than author rework.
Any new head invalidates this approval.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 883a16baeebf07044bf96addba7f095a22dfaf0d..668f1ffbf7fa192270593846dfa7dbeea5322278 (exact head 668f1ffbf7fa192270593846dfa7dbeea5322278)
Risk: high — selected-mention authorization crosses relay-session replacement, delayed HTTP reads, invitation side effects, and rate-limited publication.
Findings: No material defect found. The replacement resolves both prior lifecycle blockers:
- Connection replacement now retires outstanding evidence capabilities through
_connect → _supersedeConnection → evidenceClock.clear; delayed HTTP results are admitted only for their captured active generation (mobile/lib/shared/relay/relay_session.dart:196-244,421-436,547-570). - A complete authoritative composer read replaces rather than unions prior evidence. After an accepted kind:9000 invitation, the flow preserves operation scope, performs the required fresh read, and fences final publication with the replacement proof (
mobile/lib/features/channels/compose_bar/selected_mention_preparation.dart:19-116;mobile/lib/features/channels/compose_bar/helpers.dart:475-529). Single-invite, multiple-accepted, and partial-prefix paths retain truthful side-effect/recovery handling. - Selected reads retain session generation and rate-limit admission; after a transport wait, publication rechecks the active generation and operation fence synchronously before socket send (
mobile/lib/shared/relay/relay_session.dart:362-396,428-436;mobile/lib/shared/relay/relay_rate_limit_gate.dart:25-64). - The base-to-head diff adds no rendering or semantics code; no PR-caused accessibility defect was found.
Author action: none.
Verification owner: CI owns remaining exact-head Desktop Core/Smoke jobs; reviewer/tooling owns any optional native/live-relay observation. Neither is author rework.
Validation at exact clean head:
- PASS — full mobile package,
cd mobile && flutter test: 2,169 tests. - PASS — focused reader/composer suites: 173 tests.
- PASS —
just mobile-check: formatting unchanged; analyzer clean. - PASS —
just file-size-check: policy tests and all size gates; composer remains below its 1,200-line ceiling. - PASS, causal mutation — removing reconnect clock retirement makes the production replacement-socket regression fail.
- PASS, causal mutation — restoring attempt-wide evidence union makes accepted-roster production composer journeys fail.
- PASS —
git diff --check; clean worktrees; authenticated reviewerjedwards27differs from authorloganj. - PASS — exact-head Mobile, Mobile Swift, DCO, Semgrep, zizmor, and completed integration CI checks. At submission, Desktop Core and two Desktop Smoke shards remained pending; no failure was observed.
Manual/native evidence: not run. This is a non-rendering authorization-lifecycle change; deterministic production-path Flutter tests and causal mutations provide proportionate evidence.
Residual risk: the design guarantees session-observed currentness, not atomic authorization against a relay change the client has not observed. Relay enforcement remains final authority. Native iOS/live-relay acceptance was not independently witnessed, and remaining unrelated Desktop jobs are externally gated by CI.
883a16b to
2b2a5c6
Compare
668f1ff to
722d1aa
Compare
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 2b2a5c68111bacfd7de2486296fbc6a265ab0484..722d1aa4e3411b02894aac187a02d40140d8a482 (exact head 722d1aa4e3411b02894aac187a02d40140d8a482)
Risk: high — these regressions guard selected-mention authorization across signed relay evidence, invitation side effects, session replacement, capacity waits, media upload, and final message publication.
Findings: No author-actionable defect found. The changed-head own diff is test-only: it imports the production SendMessage seam and expands the composer classification matrix (mobile/test/features/channels/compose_bar_test.dart; mobile/test/features/channels/compose_bar_test/classification_tests.dart:4-46). The matrix exercises the default selected-authorization provider through real ComposeBar → SendMessage → SignedEventRelay → RelaySession, with signed profile/roster observations, accepted invitation ACKs, rate-limit waits, session/community changes, stale authority, media/reference-only delivery, partial accepted prefixes, and draft recovery (classification_tests.dart:82-100,141-179,228-394). It asserts exact kind-9 publication counts, recipient/reference tags, partial-effect reporting, community-switch copy, and preserved or cleared drafts (classification_tests.dart:395-473). No rendering or semantics production code changes in this base-to-head diff.
The production contracts protected by those tests remain coherent: complete selected reads replace prior evidence, an empty latest audience retires obsolete recipient proof, invitation progress is followed by a fresh read, and final delivery is fenced immediately before the real send (mobile/lib/features/channels/compose_bar/selected_mention_preparation.dart:19-68,89-119; mobile/lib/features/channels/compose_bar/compose_bar_widget.dart:490-576,615-676; mobile/lib/features/channels/send_message_provider.dart:54-119).
Author action: none.
Verification owner: CI/desktop maintainers own the unrelated red Desktop Smoke shard; reviewer/release tooling owns any optional native/live-relay observation.
Exact-head validation:
- PASS — complete touched composer package: 182 tests.
- PASS — focused new production classification matrix: 41/41.
- PASS —
just mobile-check(562 files unchanged; analyzer clean). - PASS —
just file-size-check(10 policy tests plus all surface checks). - PASS —
git diff --check; exact local/remote head and clean review worktrees rechecked. - PASS causal mutation — removing empty-audience evidence retirement breaks both obsolete-reference rows.
- PASS causal mutation — bypassing final guarded delivery breaks revoked/capacity/draft/media rows. Mutations were restored and clean exact-head state reverified.
- PASS CI — Mobile, Mobile Swift, Desktop Core, three Desktop Smoke shards, macOS/Windows builds, relay integration, DCO, Semgrep, and zizmor.
CI confidence gap, not author rework: Desktop Smoke shard 4 failed workflow-local-controls.spec.ts after three inconsistent attempts (missing trigger control versus a layout-coordinate assertion); a separate thread-head test was explicitly classified flaky. This PR's base-to-head diff touches only mobile tests, neither failing path, so no PR-caused required-gate defect is established. Full local mobile runs also encountered isolated failures in unrelated huddle/avatar/activity tests that immediately passed alone; the complete touched package remained green.
Manual/native evidence: not run. This changed head adds tests only; production-seam widget coverage plus causal mutations are proportionate.
Residual risk: session-observed currentness is not atomic against relay state the client has not observed; relay enforcement remains final authority. The unrelated red Desktop aggregate gate still requires CI-owner retry/waiver before merge under repository policy.
Any new head invalidates this approval.
2b2a5c6 to
4660aae
Compare
722d1aa to
11a20c4
Compare
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 4660aae48fc66f2547f835557032df641cad6f45..11a20c423fe5c458c961ed5dc48cd8703b089ca4 (exact live head 11a20c423fe5c458c961ed5dc48cd8703b089ca4)
Risk: high inherited authorization path, but this PR's own range is test-only.
Both assigned lanes completed and found no author-actionable defect. The stack is contiguous (#7527 → #7534 → #7536 → #7539), and this own range changes three test files only. Scoped review found no production file change under mobile/lib, desktop/src, or crates.
The composed production seam remains coherent: the composer captures session/config/draft/upload ownership, replaces complete evidence generations, rereads after invitation effects, and fences text and media delivery (mobile/lib/features/channels/compose_bar/compose_bar_widget.dart:478-576,615-718; selected_mention_preparation.dart:19-68,89-119). SendMessage rechecks before signing/optimistic insertion and before session submission, removing optimistic state on failure (send_message_provider.dart:63-119,152-157).
The expanded mobile matrix exercises the real ComposeBar → SendMessage → SignedEventRelay → RelaySession path across signed evidence, invitation ACKs and partial prefixes, session/scope replacement, rate-limit waits, media/reference-only delivery, recipient/reference tags, truthful errors, and draft recovery.
Author action: none.
Verification owner: CI/Desktop maintainers for any future workflow-test or snapshot flake; reviewer/release tooling for optional native iOS/live-relay observation.
Exact-head validation on clean trees:
- Full mobile package passed 2,188/2,188; touched composer suite passed 182/182.
- Full Desktop JS package passed 6,450/6,450.
just mobile-check,just file-size-check, andgit diff --checkpassed.- Focused changed Desktop workflow round trips passed in both lanes; exact-head CI's four Desktop smoke shards passed.
- Causal production mutation bypassing
guardedDeliveryfailed the intended revoked-publication row and was restored. - Mutating the new Desktop locator to the trigger input failed both affected round trips and was restored.
- Exact-head CI run
34436598677completed required gates without failure. Cancelled checks belong to a superseded run34436597387, not this exact-head run.
Confidence gaps, not author rework: neither lane reproduced failure by merely restoring the prior broad Desktop locator; the historical race is timing/order dependent. One Darwin-only unchanged keyboard-autocomplete snapshot differed by 438 pixels while the changed round trips and exact-head CI passed. No native iOS/VoiceOver/live-relay run was performed because this own range changes tests only.
Residual risk: authorization is current against state observed by the active session, not unseen relay state; relay enforcement remains final authority. Any new head invalidates this approval.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 4660aae48fc66f2547f835557032df641cad6f45..11a20c423fe5c458c961ed5dc48cd8703b089ca4 (exact head 11a20c423fe5c458c961ed5dc48cd8703b089ca4)
Risk: high inherited authorization boundary, but this own range is test-only: two commits, three files, +390/-60.
Composition/contracts traced: The restacked chain is contiguous: #7527 → #7534 → #7536 exact head/base links, then #7539 parented directly to base 4660aae4. The own range changes only mobile production-boundary regressions and the Desktop workflow E2E locator; scoped review found no mobile/lib, desktop/src, or crates production delta. On the composed head, selected-mention preparation still owns draft/session/config/upload scope, replaces complete evidence generations after invitation effects, and wraps text/media delivery through the composer and transport publication guards. Failure removes optimistic state. The new matrix exercises the real ComposeBar → SendMessage → SignedEventRelay → RelaySession path.
Findings: No material defect found.
- The expanded 41-row mobile matrix covers invitation ACK/partial-prefix behavior, session/scope replacement, revocation during waits and final enqueue, text/media/reference-only sends, exact tags, truthful errors, and byte-exact draft recovery.
- The Desktop workflow test now intersects the shared “Message text” label with
#wf-step-0-text, asserts the value, and settles animations before geometry sampling. Exact-head affected workflows and all four CI smoke shards pass. - No production rendering or accessibility bytes changed.
Author action: none.
Verification owner: reviewer/tooling owns the non-blocking Darwin snapshot reproducibility gap and optional native iOS/live-relay observation; CI’s exact-head required gates are terminal and passing.
Validation at exact clean head:
- PASS —
just mobile-test: 2,188/2,188. - PASS —
just desktop-test: 6,450/6,450. - PASS — focused mobile composer suite: 182/182.
- PASS —
just mobile-check,just file-size-check, andgit diff --check. - PASS — focused changed Desktop workflows: 2/2; exact-head CI’s four Desktop smoke shards passed.
- PASS, causal mutation — bypassing text composer
guardedDeliverypublishes forbidden kind 9 and fails the production final-revocation row. - PASS, causal mutation — targeting the trigger input instead of the step control fails both affected Desktop round trips.
- PASS — exact-head required CI is terminal with no failures; DCO trailers present; authenticated reviewer
jedwards27differs from authorloganj.
Confidence gaps: Reverting only to the old broad Desktop label locator did not reproduce the historical timing/order flake locally, though the new exact-ID binding is deterministic and green. A separate unchanged keyboard-autocomplete screenshot differed by 438 pixels on Darwin while the changed journeys and CI shards passed; reviewer/tooling owns that platform snapshot gap. Native iOS/VoiceOver/live-relay acceptance was not run because this head changes tests only.
Residual risk: client fencing covers authority observed by the active session, not unseen relay state; relay-side enforcement remains final authority. Any head movement expires this approval.
11a20c4 to
67b4022
Compare
4660aae to
40573cf
Compare
jedwards27
left a comment
There was a problem hiding this comment.
Both independent review lanes found no unresolved author-actionable defect at exact head 67b4022c12faa85df8f7f580d16d2942772d265b over base 40573cfac0391974400a9f65a1c9713471f35e9e.
The changed-head range is test-only and now exercises the production authorization/publication chain rather than a hand-built surrogate: selected-evidence acquisition, relay-session observations and invitation acknowledgements, ComposeBar preparation, media/text recovery, and final SendMessage → SignedEventRelay → RelaySession kind-9 publication. The matrix covers revoked/stale evidence, mixed audiences, partial accepted invitation prefixes, exact recipient tags, community switches, and draft/attachment recovery.
Regression falsifiability was independently demonstrated twice: bypassing only ComposeBar's production guarded-delivery fence caused the owning tests to fail on leaked kind-9 publication. Source was restored and each exact-head worktree was clean afterward.
Exact-head evidence:
just mobile-check: PASS (format + analyzer)just file-size-check: PASS- full Mobile suite: 2,188/2,188 PASS in one lane
- focused ComposeBar suite: 182/182 PASS in the second lane
git diff --check: PASS- relevant GitHub Mobile, DCO, Semgrep, zizmor, relay integration, and build checks: PASS at review time
Confidence gaps, not author defects: a second full-suite run hit two base-identical tests (voice_note_recording_test.dart temp-directory lifecycle and huddle_transport_test.dart timing/expectation) while the changed-area suite passed; native iOS/VoiceOver observation was not repeated because this head changes tests only. Verification ownership remains with integration/native acceptance owners. Several unrelated Desktop smoke/core jobs are still running; any later PR-caused required-gate failure must be resolved before merge.
Author action: none.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 40573cfac0391974400a9f65a1c9713471f35e9e..67b4022c12faa85df8f7f580d16d2942772d265b (exact head 67b4022c12faa85df8f7f580d16d2942772d265b)
Risk: high inherited authorization boundary; this own range is one test-only commit (+383/-58) in two mobile test files.
Composition/contracts traced: The head is directly parented to exact base 40573cfa; no production path changes in this range. Its mobile test subtree is byte-identical to previously reviewed heads (31b92bc3b7927c426430883f2e18af196ab3eb67). The expanded matrix reaches the real selected-authorization reader/session observation, invitation ACK, composer preparation/media upload, SendMessage → SignedEventRelay → RelaySession, and final socket-publication boundaries. Production capabilities still retire on session/socket replacement, eviction, and capacity change; async transport waits retain nested guards and recheck immediately before enqueue; stale-evidence failures preserve text/media drafts and truthful partial-effect recovery.
Findings: No material defect found.
- The 41-scenario matrix asserts no leaked kind 9/kind 9000, exact retained/demoted recipient and reference tags, mixed audiences, partial accepted-invitation prefixes, community/session replacement, revocation during waits/final enqueue, media/reference-only sends, and byte-exact draft recovery.
- No rendered widget, production behavior, persistence schema, or accessibility surface changes in this head.
Author action: none.
Verification owner: CI owns still-running exact-head Desktop Core/Smoke jobs; reviewer/test owners own two unrelated local full-suite failures and optional native iOS/live-relay observation. None is author rework.
Validation at exact clean head:
- PASS — full
just mobile-test: 2,188/2,188 in one independent lane. - PASS — focused compose-bar package: 182/182 in both independent lanes.
- PASS —
just mobile-check,just file-size-check,git diff --check, and DCO. - PASS, causal mutation — bypassing production text
guardedDeliverycauses six behavior failures, including forbidden kind-9 publication in final-revocation and media cases. - PASS — exact base/head, direct parent, clean worktrees, and mobile-test tree identity verified; authenticated reviewer
jedwards27differs from authorloganj. - PASS so far — exact-head Mobile, Mobile Swift, macOS/Windows builds, relay/integration, Semgrep, zizmor, and DCO CI jobs. Desktop Core and four Smoke shards remained in progress at final poll; no current-run failure was observed.
Confidence gaps: A second full mobile run completed 2,188 tests but reported two failures in byte-unchanged voice_note_recording_test.dart (deleted temp directory) and huddle_transport_test.dart (missing one expected packet); focused changed-area tests pass, and another full exact-head run is green. The final-fence mutation proves the production seam, not every individual condition in the 41-row matrix. Native iOS/VoiceOver/live-relay observation was not run because this exact range changes tests only.
Residual risk: active-session evidence cannot account for relay state the client has not observed; relay-side enforcement remains final authority. Any head movement expires this approval.
Signed-off-by: Logan Johnson <loganj@squareup.com>
40573cf to
06a016e
Compare
67b4022 to
130d06a
Compare
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE
Reviewed: 06a016e1cb69e9fbdcda9826a762c08ea800cba9..130d06ae9ab554215a2ddeb7a9a18a0d8812c886 (exact live head).
Risk: high boundary, test-only own delta. The two changed files add 41 production-bound mention-authorization journeys without changing production bytes. The covered path spans ComposeBar, session-observed authorization, invitation acknowledgement, SendMessage, SignedEventRelay, and RelaySession.
Findings: no blocking or non-blocking code defect found. The matrix covers revocation, replacement, capacity waits, socket/session changes, multiple recipients, reference-only mentions, media, partial irreversible prefixes, error truthfulness, and draft recovery. Both independent review lanes traced the production chain and found it fail-closed for observed evidence.
Author action: none.
Verification owner: CI owns the exact-head Desktop gate. Smoke shards 2 and 3 are currently red and Desktop Core is still running in run 34444699560; GitHub withholds logs until the run completes. This PR’s own range changes only Mobile tests, and exact-head Mobile is green, so those Desktop results are presently an external confidence/gate issue—not an established author-actionable defect. If completed logs establish a failure caused by this PR’s own delta, this approval is superseded.
Validation at matching exact head:
- full
just mobile-test: 2,188/2,188 pass; flutter test test/features/channels/compose_bar_test.dart: 182/182 pass;just mobile-check: pass, 562 files unchanged and analyzer clean;just file-size-check: pass, including 10/10 policy tests;git diff --check: pass;- causal mutation: replacing only the text path’s production
guardedDeliverywith rawonSendmakesproduction final revokedfail by publishing forbidden kind 9; restoration returns clean exact head; - exact-head Mobile, Mobile Swift, macOS build, integration, DCO, Semgrep, and zizmor checks are green.
Manual/native evidence: no new native iOS/live-relay/VoiceOver run. This head changes tests only; the Mobile tree is byte-identical to the prior reviewed head. Reviewer/tooling owns any additional native observation.
Residual risk: client fencing covers authority evidence observed by the active session; unseen relay state still relies on relay-side enforcement. Any new head expires this approval.
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed exact head 130d06ae9ab554215a2ddeb7a9a18a0d8812c886 over base 06a016e1cb69e9fbdcda9826a762c08ea800cba9. No author-actionable defect was found.
This head's own range is test-only: mobile/test/features/channels/compose_bar_test.dart and mobile/test/features/channels/classification_tests.dart (+383/-58). The expanded 41-scenario matrix exercises the production ComposeBar → selected-authorization/session evidence → invitation ACK → SendMessage → SignedEventRelay → RelaySession chain, including revocation, capacity waits, socket replacement, partial prefixes, multiple recipients, references, media, truthful partial-effect errors, and draft recovery. The protected text and media paths remain behind guardedDelivery, with SendMessage independently rechecking before signing/optimistic insertion and around relay submission.
Exact-head evidence
just mobile-test: 2,188/2,188 passed.- Focused composer suite: 182/182 passed.
just mobile-check: 562 files unchanged; analyzer clean.CHECK_FILE_SIZES_BASE=06a016e… just file-size-check: policy and surface gates passed.- Causal mutation replacing the text path's production
guardedDeliverywith rawonSendmade the revoked-evidence scenario publish a forbidden kind-9 event and fail; source was restored and the tree reverified clean. git diff --checkpassed.- Exact-head Mobile, Mobile Swift, macOS build, integration, DCO, Semgrep, and zizmor checks passed.
CI classification and residual risk
Desktop Core and smoke shards 1 and 4 passed. Smoke shards 2 and 3 failed in unrelated Desktop-only tests (huddle-transcription.spec.ts and profile-custom-emoji-status.spec.ts), while additional Desktop failures recovered on retry. The PR changes only mobile test files, so these failures are not evidence of a PR-caused defect. Author action: none. Verification owner: CI/maintainers should rerun or otherwise clear the Desktop gate before merge.
No native iOS/live-relay/VoiceOver observation was repeated for this tests-only changed head. That is a reviewer/release confidence gap, not author rework. Session-observed fencing also cannot cover relay state not yet seen by the active client; relay enforcement remains the final authority.
🤖
Summary (historical allocation; current owner split below)
Keep selected-mention authorization current against signed evidence actually observed by the active relay session, including events arriving while HTTP queries or rate-limited publication are waiting. A stale query must not revive an old policy or miss newly appearing profile evidence.
Related issue (historical publication context)
The following related-issue and testing text records earlier candidates, including their then-open findings, sizes and private compositions; it is not current-head coverage or status. See the existing final-candidate section below for superseding pins and scope.
Covering follow-up to #7387 and #7534; genuine own delta on #7536 (
156b7ec31b84ac642d4cc52e5d35debb88ea59a4), not a replacement/backport or duplicated ancestor graph. Existing #7534/#7536 and this branch were checked for duplicate publication; no existing observation PR found. Own range: 510 additions + 83 deletions = 593 lines including tests.Testing (historical candidate receipts)
Exact HEAD
38c768109a29ff66a3c17b71ff46ff9e183ee847:just mobile-checkPASS,just file-size-checkPASS; HEAD pinned before and after (B6_FINAL_OBSERVATION_{PIN,FULL,CHECK,SIZE}.txtinWORK_LOGS/MOBILE_FEEDBACK_CLASSIFICATION_20260909).B6_ISOLATION_MUTATION.txt). Earlier socket-observation mutation and old-source failures remain recorded, not claimed as new runs.a2437984d6fceeb60f90ca1cdce6036b63b8bcb6includes actual fix(mobile): show verified agent management provenance #7391 fixture correction and E9: full 2,203 pass, mobile-check/file-size PASS, preserving all 10 restart tests. Those restart tests are private composed evidence, not tests contained in this standalone range.Local observed currentness is not network-atomic authorization: unseen relay changes remain the relay's enforcement responsibility. No rendering change/native screenshot, simulator/VoiceOver/keyboard acceptance, full-repository CI pass, merge or review approval is claimed. Fresh independent review remains pending.
Published SEND correction — 2026-09-10
Head
722d1aa4e3411b02894aac187a02d40140d8a482; declared base2b2a5c68111bacfd7de2486296fbc6a265ab0484; actual own churn 441 including tests, strictly below600. Supersedes historical candidate pins and coverage below/above. Normal CI: https://github.com/block/buzz/actions/runs/34431162497 (completion pending).Cumulative coverage remains conditional on the existing stack: #7527 owns scope/transport and capacity/generation foundation, #7534 classification, #7536 complete observed-evidence binding, and #7539 production-boundary journeys. This is not standalone authorization completeness at an intermediate parent or a backport to #7387. The original reviewed parents' gaps are acknowledged rather than relabeled as already fixed there.
R1/R2: empty latest required audience retires obsolete recipient proof only after successful operation-scope validation; retained recipients remain protected. Genuine community changes use a typed error and invitation StateError revalidates actual scope. Unavailable authority is not falsely described as a community switch. Accepted invitation prefixes remain irreversible and accounted for. A downstream onSend adapter can still encounter generic disconnected-session failure before the typed guard: no enqueue, but the specific community explanation is not exhaustive.
The 41-row matrix includes nine added real-boundary journeys. Production rows use the actual default provider/HTTP reader and real SendMessage → signed-event relay → session. Removing reader currentness or composer aggregation independently leaks kind9; invitation counterparts leak accepted kind9000. Removing only composer guardedDelivery leaks kind9 after real draft editing while SendMessage's wrapper remains. Removing default-provider profile forwarding fails consent/post-ACK continuity (first failing assertion is unwanted kind9, not an independently logged kind9000 failure). Pure capacity and no-capacity profile-revocation rows isolate those causes; the waiting-revocation row does not independently separate coordinates from aged capacity. EMPTY/TYPED mutations fail their observable delivery/UI assertions. Source was restored. These are local mutation receipts on equivalent production, not new published-head mutation executions.
Validation: exact reviewed candidate full-mobile suites passed 2137/2152/2162/2188 for #7527/#7534/#7536/#7539. All mobile production AND test tree objects are byte-identical after mechanical Desktop-only carry; no unnecessary mobile full rerun is claimed. Own-range source-size gates pass after carry; format/analyzer receipts apply to unchanged mobile bytes. Desktop JS package 6450/6450 passed; forum 19+63 and video 7 E2E / 4 buffering unit cases are scoped receipts on identical runtime inputs, not complete Playwright/CI passes.
Private16 is now
79d3a431d08413225dda88f7fea7ca48d2a69e25: same mobile tree as reviewed0a28720e89b5319ebc682ea98ef73fae53f5e0d0(full2231); forum/video test repairs carried once each. Production correspondence to public SEND was independently byte-verified. Earlier same-production restart11 (ten classification plus original exact-draft) and mounted provenance1 receipts are reused, not new final-head executions. Public versus former668f1ffband private versuscc6c3210mobile growth is +19/-6 production and +343/-200 tests; allocation is not a net saving. This is 16 constituents, not all20, native/live-relay/VoiceOver or release acceptance. #7391's independent critique is not closed by composition.Independent delta review e6ec accepted the exact mobile candidates with the qualifications above; old GitHub approvals are not transferred. Normal push-triggered CI is running, not yet green; security skips are not a security verdict. No rerun wave, dispatch, merge or new PR. Evidence:
WORK_LOGS/MOBILE_FEEDBACK_CLASSIFICATION_20260909/GREEN_SEND_DELTA_REVIEW_D231.md,GREEN_SEND_IMPLEMENTATION.md, andGREEN_STACK_INTEGRATION.md/GREEN_6A5/.Docs carry + Desktop workflow test repair — 2026-09-10
Head carried to
11a20c423fe5c458c961ed5dc48cd8703b089ca4, one commit on the docs-carried parents (corrected #7536 base4660aae48fc66f2547f835557032df641cad6f45); the prior "Head722d1aa4" section above is a dated receipt for that pre-carry head, and approval 5162354965 was given at722d1aa4— old review bindings do not transfer to the carried head. Own churn vs the corrected parent is 450 including tests (441 mobile test lines + the 9-line Desktop spec delta: +390/−60), still strictly below 600; the inherited #7531 docs are not counted as own (comment-stripped equality;mobile/testsubtree31b92bc3b792…byte-identical to722d1aa4).The Desktop delta is test-only in
desktop/tests/e2e/workflow-local-controls.spec.ts(+7/−2): the message-step helper now intersects the shared "Message text" label with the existing first-step#wf-step-0-textcontrol and asserts the filled value — a controlled wrong-target run shows the old label-only locator filling the outgoing trigger INPUT (wf-trigger-filter-trigger-text-value, same label), dropping the send_message step text and timing out the reopened editor at helper line 55; the bound-target control round-tripstext: Workflow notificationthrough the production YAML.waitForAnimationsnow precedes all three operator geometry samples; a mixed-phase control fails the original x-order assertion (1243.11 vs 1013) and the settled control passes. Full final-candidate spec: 11 passed / 1 failed — the unchanged keyboard-template screenshot differs by the same 438 pixels with byte-identical actual PNGs on the unmodified HEAD, a pre-existing local Darwin snapshot limitation, not updated or claimed green; schedule+message roundtrips pass 2/2 in one worker after instrumentation removal. Desktoptsc --noEmit, exact-config Biome, both differential file-size gates,git diff --checkand hermitdart formatpass on the exact final tree. Normal CI on the new head: https://github.com/block/buzz/actions/runs/34436598677 (in progress). No product change, snapshot update, or whole-suite rerun is claimed; the incomplete-definition save-path question stays a separate product decision.Workflows helper test repair carried; own duplicate deduplicated — 2026-09-10
Head carried to
67b4022c12faa85df8f7f580d16d2942772d265bon corrected parent #753640573cfac0391974400a9f65a1c9713471f35e9e(the mobile "preserve SEND scope and authority at owner 7539" commit, author and message preserved). The previously published own repair commit11a20c423("test(desktop): bind workflow message step fill to step control", the 9-line local-controls delta) is now dropped as a verified duplicate:git cherry-pick --no-commitof that exact commit onto the carried parent produces an empty index — its entire content is the already-carried #7531 subpatch (SHA256b8c02397…), so nothing unrelated was dropped. Own range vs the corrected parent is now 383 additions + 58 deletions = 441 lines including tests (down from 450 at11a20c423; the −9 is exactly the deduplicated own repair), still strictly below 600.Desktop test files at this new head:
workflow-local-controls.spec.tsblob97f70051c995a634c7138417b79e7a00c93a19b6— byte-identical to the previously published leaf version (all prior local-controls receipts apply unchanged) — plus the newly carriedworkflows.spec.tsrepair, blob38e95034f0076c388c37bda6670f668e203cfc02(validated in #7531: full spec 29/29, causal red/green under identical animation slowdown; local provenance only, not public CI clearance). The Darwin keyboard-template screenshot golden remains a known unchanged local limitation (438 px; CI runs Linux where it passes). Mobile production, mobile test, and API-doc bytes are unchanged old-head→new-head. The two current-head approvals 5162865273/5162867480 were given at11a20c423and are invalidated by this head movement per their own terms; the earlier approval 5162354965 bound722d1aa4and never transferred. Normal CI on the new head is running; no green claim is made here. Evidence:WORK_LOGS/MOBILE_FEEDBACK_CLASSIFICATION_20260909/GREEN_WORKFLOWS_HELPER_REPAIR.mdandGREEN_DESKTOP_HELPER_PUBLICATION.md.Workflow regression test carry — 2026-09-10
Head rebased to
130d06ae9ab554215a2ddeb7a9a18a0d8812c886(single carry commit, author and message preserved, on parent #753606a016e1cb69e9fbdcda9826a762c08ea800cba9); no conflicts, no merge commits. Own range vs the new parent is unchanged at 383 additions + 58 deletions = 441 lines including tests; the only old-head→new-head content delta is the carried #7531 test-only +24/−0 regression coverage indesktop/tests/e2e/workflows.spec.ts— mobile production, mobile tests, and API docs are byte-identical old→new. The approvals 5163189540/5163217119 were given at67b4022c12faa85df8f7f580d16d2942772d265band do not transfer to this new head; delta review on the new head is expected per their own terms. Normal CI on the new head: https://github.com/block/buzz/actions/runs/34444699560 (no green claim is made here).