Skip to content

fix(mobile): revalidate exact agent intent before publication - #7394

Open
loganj wants to merge 5 commits into
fix/mobile-authorization-20260905from
fix/mobile-publication-20260905
Open

fix(mobile): revalidate exact agent intent before publication#7394
loganj wants to merge 5 commits into
fix/mobile-authorization-20260905from
fix/mobile-publication-20260905

Conversation

@loganj

@loganj loganj commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

🤖

Summary

Mentions of agents were authorized once, at suggestion time — so a permission revoked between selection and send, a member removed mid-compose, or a policy change during a long media upload still published the message, silently shrinking or misdirecting its audience. This PR revalidates every selected agent recipient against fresh owner policy and relay membership right before the message is prepared and again immediately before it is delivered, including on the media path. Any denial, missing evidence, failed read or revocation sends nothing and keeps your draft — recipients still selected — instead of publishing a partial audience.

  • Suggestions are never publication authority; only the fresh authorization read is.
  • Sends are fenced by account/community, channel/thread visit, edits and overlapping attempts: a send that loses its original intent (you edited, navigated, or a newer attempt started) doesn't publish.
  • Drafts containing agents or pending media stay recoverable while authorization is pending; existing background upload behavior for member-only sends is unchanged.
  • Client-side revalidation is not an atomic authorization protocol — the relay remains authoritative.

Desktop reference: agentMentionRevalidation.ts, ui/useMentionSendFlow.ts.

Related issue

Testing

Screenshots

Flutter production-widget test renders — not native-device screenshots or acceptance captures.

Scenario Before After
Sending a DM after the mentioned agent's policy was revoked mid-compose Before: the send goes through with the silently reduced audience and the composer clears After: a visible authorization error blocks the send and the draft text is retained
Capture provenance

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 882f69cbaf0f851717f071fba366b3b3a4a877d8. After: its head 95b2465b6eeb26547c90296d012fafef56af1508.

@loganj

loganj commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Independent publication readiness evidence — 2026-09-07

Verified current stacked range: fix/mobile-authorization-20260905 at 882f69cbaf0f851717f071fba366b3b3a4a877d895b2465b6eeb26547c90296d012fafef56af1508; +421/−46 = 467 across 5 files. Still OPEN/draft. Reconciliation only — no source changes, pushes, or test reruns.

  • Local, exact unchanged head (identical HEAD before and after the run): just mobile-check exit 0; full just mobile-test exit 0 (2,102 passed); full just ci exit 0. Receipts: WORK_LOGS/OWNERSHIP_0324B6FF/7394-results.txt (mobile-check:0, mobile-test:0, ci:0) + 7394-envelope.log.
  • Remote: CI run 33994067502 completed success at this exact head. Rollup: 30 success / 25 intentional skips; nothing pending or failing.
  • Fresh review: prepare + final fresh authorization with the exact intended agent keys; read failure, policy denial, missing key, membership removal, or invitation failure preserve the draft and suppress publication; media authorization runs after upload, before delivery. A proposed extra post-authorization await-boundary check was rejected after an ordering probe proved the competing edit happened after publication; all experiment changes were reverted and no defect is claimed from it (preserved only as rejected-publication-experiment.diff, not proposed code).
  • Advisory: intentionally ineligible on this stacked (non-main) base — skipped jobs are not required gates. Root fix(mobile): verify latest signed owner profiles consistently #7389's advisory is complete for its exact range (Overall Risk: NONE, zero findings).
  • Widget evidence only; no native device/simulator. Draft retained; no merge requested.

@loganj
loganj marked this pull request as ready for review September 8, 2026 13:54
@loganj
loganj requested a review from a team as a code owner September 8, 2026 13:54

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES

Reviewed: 882f69cbaf0f851717f071fba366b3b3a4a877d8..95b2465b6eeb26547c90296d012fafef56af1508 (exact head 95b2465b6eeb26547c90296d012fafef56af1508)

Risk: high — this changes mobile authorization, relay-derived membership/policy, async composer state, draft recovery, and the final publication boundary.

Behavior/contracts traced: exact selected agent keys; viewer/community/channel/visit/draft-generation fences; owner policy and relay-self-signed membership reads; pre-add and pre-delivery authorization; text/media draft retention; final signing/submission; denial, missing evidence, read failure, revocation, removal, edits during reads, and post-upload revocation.

Blocking findings

  1. [P2] Editing while authorization is pending reports a false access/membership failure. isAuthorizationCurrent intentionally becomes false when the draft revision changes (mobile/lib/features/channels/compose_bar/compose_bar_widget.dart:480-489), but authorizeAgentMentions maps that stale-intent result—and every caught exception—to “Could not authorize a mentioned agent. Check its access and channel membership…” (mobile/lib/shared/mentions/agent_publication.dart:35-40,61-62). _sendTextOnlyDraft then displays it (mobile/lib/features/channels/compose_bar/draft_lifecycle.dart:55-61). I reproduced this through the production widget seam by adding a negative snackbar assertion to the shipped pending-authorization/edit test: publication remained canceled and the newer draft survived, but the misleading authorization snackbar appeared. The same catch-all can erase community-switch provenance while the fresh read is pending.

    Author action: distinguish stale visit/account/draft cancellation from authorization/read denial. A newer edit should cancel silently; a community change should preserve the existing specific community-changed message. Add production-widget assertions for both outcomes.

  2. [P3] New public provider lacks required API documentation. agentAuthorizationReaderProvider is a new public top-level declaration without a doc comment (mobile/lib/shared/mentions/agent_publication.dart:12-23), contrary to AGENTS.md:147-150.

    Author action: document the provider or make it private if it is not intended as public API.

Validation at matching head:

  • just mobile-check: PASS (independent lane; 553 files formatted, 0 changed; analyzer 0 issues).
  • just mobile-test: one clean full run PASS, 2,102 tests. A separate run reached 2,101 pass / 1 unrelated channel_sort_manager_test.dart:279 ordering failure, then the exact isolated test passed; exact-head Mobile CI is green.
  • flutter test test/features/channels/compose_bar_test.dart: PASS, 121 tests.
  • Focused authorization/policy packages: PASS, 17 tests.
  • Mutation checks: removing either production authorization phase caused the relevant composer regressions to fail.
  • Strengthened pending-authorization/edit UI assertion: FAIL causally because the false access snackbar is present; log retained as .scratch/pr7394-carl-edit-cancel-repro.log.
  • git diff --check 882f69c..95b2465: PASS.
  • Live GitHub state immediately before review: exact head unchanged; Clients / Mobile and Mobile SUCCESS; MERGEABLE/CLEAN. Authenticated reviewer jedwards27; author loganj.

Manual/native evidence: author-provided Flutter production-widget light-theme captures were inspected; no simulator/device journey was run. The capture confirms retained draft/agent chip and visible authorization error but does not establish native lifecycle, VoiceOver, or dark-theme behavior.

Residual risk: client revalidation is not atomic with relay publication; the relay remains authoritative. Native observation remains reviewer/tooling-owned after the defects are fixed.

Verification owner: author for both fixes and regression assertions; reviewer for mutation-check and exact-head revalidation.

Signed-off-by: Logan Johnson <loganj@squareup.com>
Signed-off-by: Logan Johnson <loganj@squareup.com>
Signed-off-by: Logan Johnson <loganj@squareup.com>
@loganj
loganj force-pushed the fix/mobile-publication-20260905 branch from 95b2465 to b1e30bd Compare September 9, 2026 17:03
@loganj

loganj commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

The cancellation/denial distinction and reader documentation are now at 2fd0d0b over unchanged #7393 df61d54. The review correction applies the same classification to earlier member-scan success/failure and invitation-prompt continuation: stale edits/visits stay silent, actual community changes show “Message not sent: the community changed”, and genuine failures remain visible. Tests cover reader, pending scan, failed scan and mixed human-agent prompt in both edit/scope directions; the scan scope cases fail against the previous production file. Full mobile suite 2,110 passed; format/analyze clean at this exact head. Existing screenshots remain historical, not native acceptance. #7387 exhaustive fresh classification/restart integration is still unresolved, and independent #7527 covers invitation commitment recovery. No human re-review requested while integration remains open.

Signed-off-by: Logan Johnson <loganj@squareup.com>
Signed-off-by: Logan Johnson <loganj@squareup.com>

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES

Reviewed: df61d54cb2e4f534a19ce8782a7fd6ccd165b467..2fd0d0bcc45d987b62668985f0eae0134ccde2e2 (exact head 2fd0d0bcc45d987b62668985f0eae0134ccde2e2)

The prior cancellation-provenance and public-provider documentation findings are fixed, and the authorization/add/send ordering is otherwise sound. One user-visible scope-classification defect remains.

Blocking finding

  1. [P2] An equivalent relay-config refresh is falsely treated as a community switch. The pending-publication fence captures the RelayConfig object and later requires object identity with identical(config, ref.read(relayConfigProvider)) (mobile/lib/features/channels/compose_bar/compose_bar_widget.dart:484-494). RelayConfig has identity equality, and RelayConfigNotifier.update always constructs a fresh instance (mobile/lib/shared/relay/relay_provider.dart:12-18,80-98). Therefore a refresh/rebuild that preserves the same canonical relay destination and signing identity still fails the fence: the valid publication is suppressed and the user can receive “Message not sent: the community changed” although no community changed. An exact-head production-widget mutation probe reproduced zero sends where one was expected after replacing the config with equivalent values.

    Author action: fence on stable publication-scope identity rather than RelayConfig object identity—for example, canonical relay URL plus signing pubkey, or the active community's stable ID—and add a production-widget regression that replaces/rebuilds the config with equivalent destination and identity while authorization is pending. Assert exactly one send and no community-change snackbar; retain the genuine community-switch and edit-cancellation matrix.

Exact-head validation

  • Full flutter test: PASS, 2,110 tests, clean tree at exact head.
  • flutter analyze: PASS, no issues, clean tree at exact head.
  • Composer-focused package: PASS, 128 tests, including cancellation, authorization, upload, and recovery paths.
  • Authorization-order mutation check: removing the second fresh authorization caused the revocation regression to fail, confirming that gate is effective.
  • Live PR head was re-fetched and matched the reviewed SHA before submission.
  • Clients / Mobile, Mobile Swift, macOS build, and Windows build checks are green. The Desktop and relay-backed Desktop failures stop in Chrome/apt setup with a hash-sum mismatch before product tests execute; the six-path PR diff is mobile-only, so those failures are non-causal infrastructure noise rather than evidence against this change.

Residual risk: client-side revalidation is not atomic with relay publication; the relay remains authoritative. No simulator/device journey is claimed for this head.

Verification owner: author for the stable-scope fix and regression; reviewer for exact-head rerun of the equivalent-refresh regression, cancellation matrix, analyzer, and full mobile suite.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent — REQUEST CHANGES on exact head 2fd0d0bcc45d987b62668985f0eae0134ccde2e2 over df61d54cb2e4f534a19ce8782a7fd6ccd165b467.

The cancellation-message fix is incomplete because publication scope is fenced by Dart object identity: identical(config, ref.read(relayConfigProvider)) in mobile/lib/features/channels/compose_bar/compose_bar_widget.dart. A semantically equivalent RelayConfig replacement while authorization is pending is treated as a community change. The production-widget probe expected one send for unchanged canonical relay/destination/signing identity; actual send count was zero and the flow can emit the false “Message not sent: the community changed” state.

The rest of the publication state machine earned trust: prepare authorization → acknowledged member add → exact-key final authorization → send, with revision/source fences and draft recovery. Full Flutter suite passed 2,110 tests, analyzer passed, composer package passed 128 tests, and removing the final authorization call causally failed the revoke regression. Exact-head Mobile is green. Broad Desktop/relay reds died during Chrome apt setup with hash mismatches before product execution and are not PR-causal.

Author action: compare a stable publication-scope identity—active community ID and/or canonical relay URL plus signing pubkey—instead of provider object identity. Add a production-seam widget regression replacing RelayConfig with a semantically equivalent instance during pending authorization and assert one send with no community-change snackbar. Retain genuine switch/edit cancellation coverage.

Verification owner: author for fix/regression; reviewer for causal mutation, complete mobile suite, and exact-head rerun. Native device/AX remains a confidence gap, not the blocker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants