fix(mobile): read fresh exact-key agent authorization - #7393
Conversation
…rship Signed-off-by: Logan Johnson <loganj@squareup.com>
Signed-off-by: Logan Johnson <loganj@squareup.com>
Independent authorization readiness evidence — 2026-09-07Verified current stacked range:
|
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 804c07f016b7f73504b614d1bf81ef6f0120d83d..882f69cbaf0f851717f071fba366b3b3a4a877d8 (exact head 882f69cbaf0f851717f071fba366b3b3a4a877d8)
Risk: high — this changes mobile's relay-derived agent authorization, membership provenance, freshness, and user-visible mention eligibility.
Blocking finding
[P1] The global authorization query excludes the revocation snapshot it must observe.
agentDirectoryProvider invokes readAgentAuthorization without an exact channelId (mobile/lib/shared/mentions/agent_identity_provider.dart:68-86). That path adds #p: [viewer] to its kind 39002 query (mobile/lib/shared/mentions/agent_authorization.dart:101-124). When the relay publishes a newer replacement roster removing the viewer, that authoritative head no longer contains the viewer's p tag and therefore cannot match the query. The fold at agent_authorization.dart:47-85 sees only the older viewer-present roster and retains its channel as current authorization.
That stale channel is consumed by mention eligibility at mobile/lib/features/channels/mentions/mention_candidates_provider.dart:80-104 and mention_candidates.dart:91-118. A non-owned respond_to:anyone agent can consequently remain visible/selectable based on a shared membership that the relay has revoked. The implementation's exact-destination path correctly avoids this trap, but the production directory uses the global path.
A filter-faithful temporary regression with an older viewer+agent roster and a newer viewer-removed replacement reproduced the defect at this head:
Expected: empty
Actual: ['room']
probe_rc=1
The committed removal test does not detect it because its fake query returns supplied events without applying the emitted #p filter.
Author action: make the global read removal-aware. Do not use #p=viewer as the sole source of roster heads when absence from the latest replacement is the revocation signal. A bounded option is to derive current candidate destinations independently and query their exact #d heads in batches, then select the latest relay-signed snapshot per destination before requiring both viewer and agent membership. Add a filter-faithful regression bound through agentDirectoryProvider into autocomplete eligibility, not only the helper.
Verification owner: author for the patch, production-seam regression, and full mobile gates; reviewer for exact-new-head delta and adversarial rerun.
Validation
At matching clean local HEAD 882f69cbaf0f851717f071fba366b3b3a4a877d8:
git diff --check 804c07f016b7f73504b614d1bf81ef6f0120d83d..HEAD— pass.flutter test test/shared/mentions/agent_authorization_test.dart test/shared/mentions/agent_policy_test.dart— 17 passed.just mobile-check— pass; 551 files formatted/0 changed, Flutter analyze reported no issues.- Filter-faithful viewer-removal causal probe — failed as above, then temporary changes were removed.
- GitHub CI's Mobile job and aggregate checks are green at this exact head. Those gates do not cover the missing filter-faithful regression.
A separate full just mobile-test run reported 2,094 passing tests, but its checkout metadata was removed externally before a post-run SHA/status recheck; I therefore do not treat that run as independently exact-head-pinned evidence. This does not affect the source-backed defect or the clean exact-head checks above.
Manual/native evidence: no native iOS device/simulator journey was run. The PR changes no layout, but live relay/autocomplete behavior remains unobserved.
Residual risk: async scope fencing, NIP-11 self authority selection, bounded pagination, and failure propagation appear covered by focused tests, but the authorization revocation defect is unresolved. A new head invalidates this verdict until its delta and regression are reviewed.
Signed-off-by: Logan Johnson <loganj@squareup.com>
|
Addressed review #7393 (review) in c73ff57: #p is now destination discovery only; bounded exact relay-author #d head reads (without #p) replace all seed snapshots before eligibility. The filter-faithful regression exercises viewer-removal through autocomplete, and exact-destination reader API is unchanged. Full mobile suite on this head: 2,095 passed; flutter analyze clean. First full run had one unrelated voice-note temporary-directory cleanup failure; unchanged-head full repeat passed. No native-device validation or repository-wide just ci claim. PR remains ready, 570 additions + 11 deletions against 804c07f. |
Signed-off-by: Logan Johnson <loganj@squareup.com>
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 804c07f016b7f73504b614d1bf81ef6f0120d83d..df61d54cb2e4f534a19ce8782a7fd6ccd165b467 (exact live head df61d54cb2e4f534a19ce8782a7fd6ccd165b467)
Risk: critical — this is authorization state used by a user-visible path that can silently add and notify an agent.
Blocking finding
[P1] A relay-signed roster removal does not invalidate the cached directory, and send trusts the stale selection.
The new exact-key reread fixes freshness when a directory read occurs: mobile/lib/shared/mentions/agent_authorization.dart:25-55 now uses the #p result only for destination discovery and reads each current exact #d head before authorizing it. The production lifecycle does not cause that read after a roster replacement, however.
agentDirectoryProvider performs the authorization read only when its non-auto-disposed FutureProvider is constructed and watches only relay session state (mobile/lib/shared/mentions/agent_identity_provider.dart:68-90). Autocomplete keeps consuming that cached success (mobile/lib/features/channels/mentions/mention_candidates_provider.dart:80-104). The kind 39002 live path refreshes current-channel membership and bot-role state, but no membership-triggered invalidation of agentDirectoryProvider was found in the mobile source paths reviewed; the explicit production invalidation found is the special kind 10100 DM identity path at mobile/lib/features/channels/channel_detail_page.dart:150-163.
That stale value has a concrete consequence: after a replacement roster removes an agent while the viewer remains, the picker can reclassify the removed agent as an eligible non-member (mobile/lib/features/channels/mentions/mention_candidates.dart:91-118). Selection persists the stale MentionCandidate, and _scanNonMemberMentions refreshes ordinary members but trusts candidate.isAgent without reauthorizing the exact agent/destination key (mobile/lib/features/channels/compose_bar/helpers.dart:521-558). Send can therefore silently add the revoked agent back and notify it (mobile/lib/features/channels/compose_bar/compose_bar_widget.dart:472-508). This defeats revocation in an already-open compose journey.
The committed regression does not protect this lifecycle: it calls readAgentAuthorization directly and then the pure candidate builder (mobile/test/shared/mentions/agent_authorization_test.dart:153-201), bypassing the cached provider and send seam. An independent temporary provider-lifecycle probe at this exact head read an included agent, changed the fake relay to a newer exact #d head removing the viewer, and reproduced the stale result on the second provider read (expected empty; received one entry). Removing the new exact-#d reread separately made the committed removal test fail causally, confirming that narrower fix works but is not reached by production refresh.
Author action: connect authoritative kind 39002 replacement updates to a scope-fenced directory refresh so autocomplete drops revoked entries. Also reauthorize selected non-member agents at the exact destination/key before add/send, fail closed while preserving the draft on query failure or scope change, and add production-seam regressions proving (1) one mounted provider/autocomplete instance observes a later signed removal and (2) a candidate selected before revocation is neither added nor p-tagged afterward.
Verification owner: author for the refresh/send fence, regressions, and full mobile gates; :bot: Jude’s code review agent for exact-new-head adversarial rerun.
Validation at the reviewed head
- PASS — focused authorization/policy suites: 18 tests.
- PASS —
just mobile-check: 551 files checked, no formatting changes; analyzer reported no issues. - PASS — full
just mobile-test: 2,095 tests. - PASS —
git diff --check 804c07f016b7f73504b614d1bf81ef6f0120d83d...HEAD. - PASS — causal exact-head mutation of the new exact-
#dreread made its removal regression fail, then pass after restoration. - PASS — GitHub
Clients / Mobileand aggregateMobilechecks at this head. - PASS — final local/live head match and clean reviewer trees reported by both independent lanes.
Manual/native evidence: no native iOS/live-relay journey was run. Loading and errors currently fail closed to no relay-agent suggestions, but explicit loading/error/retry UX was not observed; that is a reviewer/tooling confidence gap, not additional author action beyond the established stale-success defect.
Residual risk: native relay/autocomplete timing remains unobserved. The source trace, full mobile suite, and causal provider probe establish the blocking cache/send defect despite that gap.
— :bot: Jude’s code review agent
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES
Reviewed: 804c07f016b7f73504b614d1bf81ef6f0120d83d..df61d54cb2e4f534a19ce8782a7fd6ccd165b467.
The changed head fixes refresh-time #p removal blindness by using #p only for destination discovery and then reading exact relay-signed #d heads. The new helper regression is mutation-causal.
[P1] Live roster revocation never refreshes the cached production directory
agentDirectoryProvider performs its fresh authorization read only at construction (agent_identity_provider.dart:68-90); autocomplete consumes cached data (mention_candidates_provider.dart:80-104). Live kind 39002 updates refresh channel member/bot providers, but do not invalidate the directory. A replacement roster removing an agent can therefore leave it visible as an eligible non-member; a previously selected stale candidate can be silently re-added because send trusts candidate.isAgent rather than reauthorizing.
The committed test calls readAgentAuthorization directly with fresh sessions and then a pure candidate helper, bypassing provider cache/lifecycle. A production-provider probe reproduced stale output after a newer exact-#d removal.
Author action: invalidate/recompute directory authorization on authoritative roster-head updates with generation fencing, and preferably reauthorize selected destination/key at send. Add production-seam tests proving one mounted provider/autocomplete drops a later removal and a pre-revocation selection cannot auto-add or recipient-tag afterward.
Verification owner: author for lifecycle/send fixes and full Mobile gates; reviewer for causal exact-head rerun.
Validation: focused authorization/policy suites passed 18; full Mobile passed 2,095; just mobile-check, mutation of exact-#d reread, policy audit, and git diff --check passed at clean exact head. Loading/error currently fails closed to no suggestions but lacks explicit affordance; native iOS timing was not observed. Those are confidence gaps, not extra author work.
|
Coverage for new reviews 5157594911 / 5157603700 and the reconsideration 5158416346: this PR remains frozen at The cached-directory/stale-send finding is covered across sibling descendants:
#7394 and #7395 are still unmerged siblings. Neither this frozen parent nor either sibling alone supplies both protections. Integrate both (and the existing #7390 invitation rollout prerequisite) before enabling widened discovery. #7530 carries the corrected discovery parent without changing its own authority/cache patch. Native integrated select/add/send validation and repository-wide CI remain separate integration evidence, not asserted by this provider/widget result. Update at the unchanged head: the send-time body overclaim quoted in review 5158416346 is now corrected in the PR body — two hunks (summary paragraph and the directory-entries bullet), no source or head change, head still One review-scope question, so the next pass can be decisive: at this unchanged reader-only head, is there a remaining defect in the reader itself (fresh signed exact-key resolution, bounded pagination, failure propagation), or is the CHANGES_REQUESTED verdict a requirement that this intermediate PR be standalone-safe at its own head — that it independently carry the send-time reauthorization and mounted directory refresh its children add? The answer determines the remedy: a reader fix here versus the inert-parent/immutable-integration-head option the review describes. |
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES remains after author-response reconsideration
Reviewed: 804c07f016b7f73504b614d1bf81ef6f0120d83d..df61d54cb2e4f534a19ce8782a7fd6ccd165b467 (unchanged exact head). Reconsidered against author response 5606140554.
Risk: critical — relay-derived authorization controls a user-visible path that can add and notify an agent.
[P1] The production directory/send path is still not standalone-safe
The response usefully makes the stack dependency explicit, but it also confirms that neither this parent nor either unmerged sibling alone supplies both required protections. That does not resolve this head's production contract:
agentDirectoryProviderperforms one non-auto-disposed authorization future and watches only session state (mobile/lib/shared/mentions/agent_identity_provider.dart:68-90); autocomplete consumes that cached success (mobile/lib/features/channels/mentions/mention_candidates_provider.dart:80-104). A later signed roster replacement does not refresh the mounted directory.- The committed removal regression creates a fresh reader/session rather than exercising one mounted provider (
mobile/test/shared/mentions/agent_authorization_test.dart:153-200). - Send still trusts persisted
MentionCandidate.isAgent(mobile/lib/features/channels/compose_bar/helpers.dart:521-558) and proceeds to add/send (mobile/lib/features/channels/compose_bar/compose_bar_widget.dart:472-508) without exact destination/key reauthorization.
The PR body still says resolution uses fresh signed data “at send time” and that only this fresh read authorizes agent mentions. This head has no send-time authorization call site and no live directory refresh. #7394 and #7395 are open parallel descendants based on this head; future code and a deployment-order instruction do not make this independently mergeable production wiring safe. The response's “no standalone-safety claim” narrows the author's assertion, but does not narrow the code or existing PR contract.
Author action: either (1) make this head standalone-safe with scope-fenced authoritative kind-39002 directory refresh and exact destination/key reauthorization before add/send, plus mounted-autocomplete and selected-before-revocation publication regressions; or (2) make this parent genuinely inert/primitive-only and move production adoption and its contract to one immutable integration head containing both protections. Naming separate open siblings is insufficient.
Verification owner: author for the standalone patch or immutable integrated adoption head, causal production-seam regressions, and full Mobile gates; reviewer for exact-new-head delta, mutation/adversarial rerun, and lifecycle/send verification.
Validation: exact local/live head equality and clean reviewer tree; git diff --check passed. Prior same-head evidence remains applicable because source bytes are unchanged: focused 18 tests, just mobile-check, full 2,095-test Mobile suite, and exact-#d mutation passed; the mounted-provider lifecycle probe reproduced stale output. Authenticated reviewer jedwards27 differs from PR author loganj. At reconsideration, #7394/#7395 Mobile lanes passed, while some sibling CI jobs remained in progress; sibling CI does not certify this parent's standalone contract.
Confidence gap / residual risk: no native integrated select → revoke → add/send journey was run. That is not an additional defect and is not the basis for this verdict.
— :bot: Jude’s code review agent
2 participants
🤖
Summary
Whether mobile let you mention an agent depended on stale, self-declared runtime claims: an advertised running instance could authorize a mention without real channel membership, membership was read from a contact field anyone can set, and an owned agent with no running instance couldn't be mentioned at all. This PR supplies the read-only fresh signed authority reader that resolves each requested agent key — the latest signed profile plus the owner's current policy (no runtime advertisement required), and channel membership signed by the relay itself rather than self-declared. It does not enforce this at send time: at this head the send path still trusts the persisted picker classification. Actual send-time enforcement lives in the unmerged stack — #7394 @2fd0d0bc (send-time fresh authorization on this reader), #7534 @85ddfafe (currentness fences at the compose boundary), #7536 @156b7ec3 (publication guard across the relay rate-limit wait), #7539 @38c76810 (observed-evidence checks into enqueue) — which must merge on top of this PR, in order and with the #7390 invitation rollout prerequisite, before widened discovery is enabled. No standalone-safety, whole-integration, or network-atomic claim is made here.
Desktop reference:
commands/agent_discovery/relay_directory.rs,commands/identity_archive.rs.Related issue
Testing
just mobile-check, the full mobile test suite, and full localjust ciall pass — receipts in the exact-head evidence comment.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 base804c07f016b7f73504b614d1bf81ef6f0120d83d. After: its head882f69cbaf0f851717f071fba366b3b3a4a877d8.