fix(mobile): preserve exact mention recipients in saved drafts - #7387
fix(mobile): preserve exact mention recipients in saved drafts#7387loganj wants to merge 1 commit into
Conversation
5696495 to
8a78110
Compare
Independent durable-mention readiness evidence — 2026-09-07Verified current stacked range:
2026-09-07 update: the stale composed reference described above has been retired — the |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — request changes
Reviewed base 39afd73b0adfde14164f4b10dbd089cb498312b6 through exact head 8a781108507dc4501192b0101185decc1ed6f118.
Major — saved display classification can authorize a later membership mutation
DraftMention.isAgent is persisted as device-local JSON (mobile/lib/shared/mentions/draft_mention.dart:3-24) and restored directly into MentionCandidate.isAgent (mobile/lib/features/channels/compose_bar/draft_lifecycle.dart:97-109). On send, a currently non-member candidate with that restored bit is classified into agentPubkeys (mobile/lib/features/channels/compose_bar/helpers.dart:541-551) and silently added with role=bot (helpers.dart:606-617, via _addMentionedNonMembers at lines 436-474).
That makes stale presentation metadata an authorization/provenance input. A draft can be saved while a pubkey is an eligible directory agent, then reopened after that directory or verified classification disappears. Sending still emits a durable kind 9000 bot-membership mutation instead of using the human invite / reference-only path. The relay does not repair this trust boundary: its PUT_USER policy permits an authorized channel member to assign the bot role (crates/buzz-relay/src/handlers/channel_authz.rs:119-166). This also contradicts DraftMention's own contract that display classification is not authorization.
Two independent production-widget probes reproduced the stale-provenance path at this head: persisted is_agent: true, no current member/directory-agent evidence, then reopen/send emitted kind 9000 with the target p tag and role=bot, without a human confirmation dialog. The checked-in restart/rejected-send regression does not exercise classification changing between save and send.
Required change: Preserve the exact label-to-pubkey binding, but re-derive agent status and mention eligibility at send from current authoritative evidence (current membership role and current verified directory/NIP-OA evidence). If current evidence cannot establish agent status, fail closed into the human invite/reference-only path or another explicit safe choice. Add a production-seam regression covering: eligible agent selected → draft saved → current agent provenance removed/changed → restart/reopen/send → no silent bot-role PUT_USER, with the intended fallback asserted.
Validation
- Focused mobile production-seam suites: 129 passed on a clean tree with head checked before and after as
8a781108507dc4501192b0101185decc1ed6f118. - The new checked-in widget regression
exact draft recipients survive restart and failed-send recovery: passed independently. git diff --check 39afd73b0adfde14164f4b10dbd089cb498312b6..8a781108507dc4501192b0101185decc1ed6f118: passed.- GitHub Mobile CI and required checks are green on this head.
- Supplemental full mobile suite: 2,084 passed, but its post-run SHA could not be re-attested after the disposable worktree metadata was removed, so it is not relied upon for this verdict.
Confidence gap: no native iOS simulator/device journey was observed, so keyboard, VoiceOver, and native lifecycle behavior remain unverified. This is verification outstanding, not separate author rework.
8a78110 to
e544b24
Compare
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — request changes
Reviewed base acd841354a692243f1cb4c04059baad42f1d2abb through exact live head e544b247c32bdccf1b316d3329be247faf412f5f.
[P2] Restored display classification still authorizes a silent bot-membership mutation
DraftMention persists device-local is_agent while explicitly stating that display classification is not authorization (mobile/lib/shared/mentions/draft_mention.dart:1-7,22-25). Restore copies that bit directly into MentionCandidate.isAgent (mobile/lib/features/channels/compose_bar/draft_lifecycle.dart:97-109), and mention resolution preserves it (mobile/lib/features/channels/compose_bar/agent_mention_labels.dart:10-35). At send, _scanNonMemberMentions re-reads membership but uses only candidate.isAgent to classify a now-non-member identity as an agent (mobile/lib/features/channels/compose_bar/helpers.dart:504-559). That classification is then silently added with role=bot (helpers.dart:436-474,605-618). No current verified directory/NIP-OA provenance is re-derived. Relay authorization does not repair this boundary: an authorized channel member can request the bot role, subject only to optional target policy (crates/buzz-relay/src/handlers/channel_authz.rs:119-166).
The resulting journey remains unsafe: select an eligible agent → save draft → target leaves and current agent provenance disappears → restart/reopen → send. Stale local presentation metadata still causes a durable bot-membership mutation, bypassing the human Invite / send-without-inviting reference fallback. The new durable-draft regression covers rename, restart, thread switching, and failed-send recovery, but does not change current authority before send (mobile/test/features/channels/compose_bar_test/durable_mention_tests.dart:3-91).
Required change: preserve the exact label-to-pubkey identity, but at send re-derive agent status and mention eligibility from current membership role plus current verified directory/NIP-OA evidence. If current evidence is absent or unavailable, fail closed through the explicit human Invite / send-without-inviting path (or an equivalently safe reference-only choice), never a silent bot add.
Add a production-widget regression for: saved is_agent:true binding → current agent provenance withdrawn while target remains a non-member → restart/reopen/send. Assert no kind 9000 bot add occurs without explicit invitation and that send-without-inviting emits a mention reference rather than a p recipient. Mutation-prove the test by restoring trust in the saved bit and confirming it fails.
Validation
git diff --check acd841354...e544b247c: passed.- Focused full files
compose_drafts_provider_test.dartandcompose_bar_test.dart: 129 passed on a clean tree pinned toe544b247cbefore and after. just mobile-checkand fulljust mobile-test: 2,085 tests passed on a clean tree pinned toe544b247cbefore and after.- GitHub Mobile and security checks are green on this head; green coverage does not exercise the stale-provenance transition above.
- Repo-policy audit found no new production
unwrap/expectorunsafe; the publicDraftMentionis documented and both commits carry DCO sign-off.
Confidence gap: no real iOS simulator/device, keyboard, or VoiceOver journey was observed. No new visual control is introduced, but restored-chip and prompt/fallback behavior still need native observation after the code defect is fixed. This is reviewer/tooling-owned verification, not additional author rework.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES
Reviewed: acd841354a692243f1cb4c04059baad42f1d2abb..e544b247c32bdccf1b316d3329be247faf412f5f.
[P2] Restored display classification still authorizes silent bot membership
DraftMention.isAgent persists as local display metadata (draft_mention.dart:3-7,22-25) and restores directly into MentionCandidate.isAgent (draft_lifecycle.dart:97-109). Send preserves that candidate and _scanNonMemberMentions uses the restored bit to select the silent agent branch (helpers.dart:504-559), which emits a bot membership add (helpers.dart:436-474,605-618) without re-deriving current verified directory/NIP-OA provenance.
An agent selected while eligible can lose provenance before restart, yet sending the restored draft still silently adds the now-unverified non-member as a bot instead of using explicit human invite/reference-only behavior.
Author action: persist exact identity binding, but re-derive current agent classification/eligibility at send from current membership plus verified directory/owner evidence. If unavailable, fail safe through explicit invite/reference-only behavior. Add a production-widget provenance-removal-across-restart regression asserting no silent bot kind:9000; mutation-prove the send-time authority seam.
Verification owner: author for patch/regression; reviewer for exact-head relay and production-seam rerun.
Validation: focused persistence/composer files passed 129 tests; independent full Mobile run passed 2,085; just mobile-check and git diff --check passed at clean exact head. Residual risk: no native iOS restart/keyboard/VoiceOver journey; reviewer/tooling gap, not separate author work.
|
Updated covering-fix evidence for reviews 5157599403 / 5157603652 (saved display classification authorizing membership). This replaces the prior historical-head summary; it is not a backport or change to #7387's head. Published covering graph: #7527 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: The two prior composed failures were corrected naturally in #7391: signed |
|
a6dc76b to
b3b1256
Compare
e544b24 to
3bb12de
Compare
|
Addressed reviews 5157599403 / 5157603652 in The parent now persists only exact The remaining unique production change is the original five-line listener-owner fence. Its original exact-recipient/thread-switch/restart/rejected-send test fails on the parent without that fence (old listener erases the draft during thread hydration), and passes with it. Parent malformed-record denial and complete signed-tag rendering authority are preserved. Final diff: 229 additions, 0 deletions, 3 files. Exact-tree validation: full Mobile 2,124 pass; draft/provider/channel-thread/renderer focus 366 pass; analyzer clean; format 3 files unchanged; diff check clean. No native iOS restart/keyboard/VoiceOver or live relay journey is claimed. Requested re-review from jedwards27. |
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed exact head 3bb12de1377dc600822bcc54e63001a4947d26bc against base b3b1256470ba032097642239131a8a08a8c85ecc. The prior stale-provenance blocker is resolved.
Persisted drafts now retain only the literal label→pubkey binding, and hydration marks that binding as requiring current revalidation. At send time the implementation substitutes current candidate evidence or refuses visibly; only a currently validated agent can enter the signed kind-9000 role=bot membership path. If the target is now an ordinary human, the existing explicit prompt remains in control and “Do nothing” produces a reference-only mention tag. If current provenance is unavailable, nothing is published and the draft remains recoverable. The owner-identity fence also prevents an outgoing thread's stale listener from overwriting the newly hydrated draft during thread switches.
Evidence
- Full clean exact-head
compose_bar_test.dart: 130/130 passed, including restart, rejected-send recovery, withdrawn-provenance fallbacks, and thread-switch ownership. - Causal stale-authority mutation (
isAgent: true, revalidation disabled): both provenance-loss probes failed on an observed signed kind 9000 carryingrole=bot, demonstrating that the tests enforce the authorization boundary rather than cosmetic state. - Causal owner-fence deletion: exact-recipient/thread-switch/restart coverage failed, demonstrating that the lifecycle fence is required.
- Production-widget fallback rows: 2/2 passed; assertions observed no kind 9000. The current-human “Do nothing” path emitted no
p, exactly one reference-onlymention, and a valid signature. just mobile-check: passed (556 formatted files, zero changes; analyzer clean).git diff --check: passed. Exact ancestry, DCO, mergeability, and CI were rechecked immediately before review; GitHub reportsMERGEABLE/CLEANand Mobile, DCO, Semgrep, and zizmor green.
A separate full local just mobile-test run reported 2,122 passed / 2 failed in untouched tests (activity_provider_test.dart:344 and settings_profile_header_test.dart:134). The changed package's GitHub Mobile check is green, and the failures do not implicate this three-file diff, so this is a confidence gap rather than author action.
Author action: none.
Residual verification: native iOS process restart, keyboard/VoiceOver behavior, and live-relay persistence were not observed. Reviewer/tooling owns those checks; the absence of that native evidence is not author rework.
) <!-- Draft PR body for #7385 · fix(mobile): bind same-name mentions to exact selected identities · #7385 --> 🤖 ## Summary If two people in a channel share a display name, mobile couldn't tell them apart in mentions: picking the second could overwrite the first's selection, a rendered mention linked whichever same-name person matched first, and a later rename or a shorter name could re-bind the text to the wrong recipient. This PR binds every mention to the exact selected identity: - Each same-name selection keeps its own recipient instead of overwriting by name; conflicting picks get a qualified label like `Name (key…)`. - The longest matching label wins, so a shorter or interior name can never claim part of a longer one and steal its identity. - Rendering resolves recipients by the signed identity key from the event's tags — never from message text alone — so qualified labels stay correct regardless of tag order and survive later renames; an untagged ambiguous label blocks shorter mentions instead of silently re-binding. Ports the landed Desktop exact-recipient behavior (see `docs/mention-editor.md`). ### Related issue - Fixes: N/A. No mobile issue; Desktop's landed exact-recipient fixes are the reference this ports. - Independent base (`main`). #7387 (child) persists these exact selections in saved drafts. - Landing note: branches in this series overlap in the composer — when rebasing, keep exact/durable mention bindings, the explicit invite/reference-only choice, the account/visit/revision fences, and authorization before membership preparation and publication; don't resolve conflicts by taking either side wholesale. - Draft — not requesting merge yet; the security advisory run for this range timed out without results (no verdict). ### Testing - Regressions cover same-name collisions, prefix/overlap, removal, tag order, and renames. - At `acd841354a692243f1cb4c04059baad42f1d2abb`: `just mobile-check` and full `just mobile-test` pass (2,082 tests). The earlier full `just ci` receipt linked below is reused only for unchanged non-mobile code/tooling, not claimed as a rerun at this head. - Previous-head evidence: `just mobile-check`, the full mobile test suite, and full local `just ci` all pass — receipts in the [exact-head evidence comment](#7385 (comment)). - Verification is widget-test level; no native device or simulator run is claimed. To see it: mention two teammates with the same display name — both stay distinct, the second shows a qualified label, and the rendered message keeps both correct even after either renames. ### Screenshots Flutter production-widget test renders — not native-device screenshots or acceptance captures. | Scenario | Before | After | |---|---|---| | Qualified mention chip for same-name recipients, at 200% text scale (deliberate stress fixture) |  |  | <details> <summary>Capture provenance</summary> Rendered by the Flutter widget engine in a `flutter test` run (production widgets, production theme; no device or simulator). Before: this PR's declared base `3c7f288c60d67df78577b237e27c3dfc8831aaa1`. After: its head `39afd73b0adfde14164f4b10dbd089cb498312b6`. </details> Signed-off-by: Logan Johnson <loganj@squareup.com>
Recompose the durable-mention child on the approved exact-mention parent. Reuse its atomic key-only draft persistence and current-candidate revalidation instead of restoring the obsolete display-classification authorization path. Retain the previously reviewed owner-token fence so an outgoing draft listener cannot erase bindings while another thread hydrates. Keep the original restart, thread-switch and rejected-send regression. Add production signed-send probes for eligible agent selection followed by provenance withdrawal and restart: refuse unavailable identities or explicitly send a human reference without a bot-membership mutation. A rollback to trusted restored agent classification fails both probes on kind 9000 role=bot. Signed-off-by: Logan Johnson <loganj@squareup.com>
3bb12de to
8a68995
Compare
🔐 Codex Security Review
|
|
Recomposed onto #7385 was squash-merged onto This push is a mechanical recomposition of the exact same change onto actual current
jedwards27's approval |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: d074576871a66c72e4a9eb5ad59ec0b22a7f0081..8a689956085aecd65465a7f46b1634e40c672c50 (exact live head 8a689956085aecd65465a7f46b1634e40c672c50)
Risk: high — persisted exact recipient identity feeds signed message tags and can trigger channel-membership mutation.
Behavior/contracts traced: account/community/channel/thread-scoped draft persistence; atomic text + label→pubkey restoration; malformed/legacy compatibility; current-candidate revalidation; human invite/reference-only fallback; agent membership preparation; signed relay events; listener replacement; failed-send and restart recovery.
Findings: no blocking or non-blocking code defect found. The rebase preserved the previously reviewed patch byte-for-byte: git range-diff maps 3bb12de13 to 8a6899560 with =, and all three PR-owned paths are identical. Persisted state remains presentation binding only—no trusted agent bit—and hydration marks identities requiresRevalidation (mobile/lib/features/channels/compose_bar/draft_lifecycle.dart:95-107). Send resolves those identities against current community candidates or refuses visibly (mobile/lib/features/channels/compose_bar/agent_mention_labels.dart:40-50; compose_bar_widget.dart:479-517). Current-human fallback requires an explicit choice and produces a reference-only tag; unavailable identities send nothing and keep the draft. The owner token retires the outgoing scope listener before replacement hydration (draft_lifecycle.dart:84-87,132-170).
Author action: none.
Verification owner: reviewer/tooling for native iOS restart, keyboard/VoiceOver, and live-relay persistence; CI/release gate for remaining repository checks.
Validation at matching clean HEAD:
- PASS — full
just mobile-test: 2,124/2,124. - PASS —
just mobile-check: 556 files, 0 changed; analyzer clean. - PASS — full
flutter test test/features/channels/compose_bar_test.dart: 130/130. - PASS — causal deletion of the production owner guard fails thread-switch restoration; restored tree clean.
- PASS — causal stale-classification mutation emits forbidden signed kind
9000bot-membership events in both provenance-withdrawal cases; restored tree clean. - PASS —
git diff --check, exact ancestry/merge-base, DCO, clean tree, and exact live-head freshness. - CI snapshot: Mobile, Mobile Swift, DCO, Semgrep, zizmor, and macOS/Windows builds pass. One Desktop Smoke E2E shard failed while other desktop/integration jobs remain in progress; this PR changes only mobile files, so that is an external CI gate/confidence gap, not an observed author-actionable defect.
Manual/native evidence: none claimed. Widget-level production UI and signed socket-event evidence cover the dangerous authorization effect, but no simulator/device run was performed.
Residual risk: native iOS process-restart/keyboard/VoiceOver behavior and a live-relay persistence journey remain unwitnessed. No control or handler changed, existing IME/semantics coverage passes, and the missing observation does not establish a code defect. Any new head invalidates this approval.
🤖
Summary
Saved drafts remembered their text but not exactly who each mention referred to: reopening a draft (or restarting the app) could re-bind a mention to the wrong same-name teammate, and a draft restored in one thread could capture text arriving for another. This PR saves the exact selected identities alongside the draft text and restores them together, so a reopened draft names the same recipients — with their agent markings — that you picked, including after a failed send.
Related issue
Testing
e544b247c32bdccf1b316d3329be247faf412f5f:just mobile-checkand fulljust mobile-testpass (2,085 tests). The earlier fulljust cireceipt linked below is reused only for unchanged non-mobile code/tooling, not claimed as a rerun at this head.just mobile-check, the full mobile test suite, and full localjust ciall pass — receipts in the exact-head evidence comment.To see it: mention an agent or a namesake, leave the channel, restart the app, and return — the draft still names the exact recipients you picked.
Screenshots
Flutter production-widget test renders — not native-device screenshots or acceptance captures.
Capture provenance
Rendered by the Flutter widget engine in a
flutter testrun (production widgets, production theme; no device or simulator). Before: this PR's declared base39afd73b0adfde14164f4b10dbd089cb498312b6. After: its head8a781108507dc4501192b0101185decc1ed6f118.