fix(mobile): keep discovery ownership and policy readiness consistent - #7530
fix(mobile): keep discovery ownership and policy readiness consistent#7530loganj wants to merge 5 commits into
Conversation
Signed-off-by: Logan Johnson <loganj@squareup.com>
a6d7298 to
1db7241
Compare
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: b829496e0bf363939afd6b54fc67b4b08716b35b..1db724109c5542e2081a1c71f60b0554e870d0cf (exact head 1db724109c5542e2081a1c71f60b0554e870d0cf)
Risk: high — this changes ordered Nostr profile authority, ownership provenance, subscription recovery, and autocomplete authorization.
Blocking finding
UserCacheNotifier.put still writes profile/owner state without consulting or advancing _profileEventOrders (mobile/lib/shared/profile/user_cache_provider.dart:48-51). The confirmed signed-profile publish path retains submittedEvent but converts it to a detached UserProfile and calls that unordered write (mobile/lib/features/profile/profile_provider.dart:206-239), while signed-event writes enforce (createdAt, eventId) order in _cacheProfileEvent (user_cache_provider.dart:163-180).
That leaves two executable rollback paths:
put(newer profile, owner)followed by an older kind-0 event is accepted because the order map was never populated. An exact-head temporary regression reproduced this: the expected newer owner/profile becameolder(test exit 1).- A newer live revocation can arrive after publish confirmation but before the detached
put; the older submitted profile then overwrites the newer visible state while the order map retains the newer event, causing replays of both events to be rejected. Stale owner/provenance may therefore persist until another newer profile arrives.
This defeats the PR's single ordered ownership-source guarantee and can restore revoked picker authority.
Author action: route the confirmed submittedEvent through the ordered event API, or require event-order metadata on every authority-bearing cache write and compare it atomically. Add a production-seam regression covering the confirmation/cache interleaving: a newer revocation must survive an older confirmed local write and subsequent replay. Mutation-prove that regression against the unordered write. Audit _fetch for the same requirement rather than returning detached profile state without advancing the ordered cache.
Verification owner: author for the fix/regression; reviewer for exact-head interleaving reproduction and the full Mobile gate on the replacement head.
Reconciled supporting evidence
The prior readiness/recovery concern is resolved at this head: terminal CLOSED fails closed, subscription generations are retired, retries share a bounded budget, and retired callbacks are ignored (mobile/lib/shared/mentions/agent_discovery.dart:80-137). The deterministic production widget regression exercises CLOSED → removal → retry/EOSE restoration → signed roster removal in one mounted editor (mobile/test/features/channels/discovery_lifecycle_tests.dart:1-89).
Loading/error currently collapse to a human-only synchronous candidate list (mobile/lib/features/channels/mentions/mention_candidates_provider.dart:80-107) and the composer has no distinct visible status (mobile/lib/features/channels/compose_bar/helpers.dart:158-185). That is a UX confidence gap, not an additional merge blocker here: the stated PR contract deliberately hides authenticated-owned identities while policy is unavailable. A truthful loading/error/retry affordance should be handled explicitly if product intent requires users to distinguish authorization unavailability from no matches.
Exact-head validation:
flutter test: 2,107 passed; rc 0; HEAD before/after1db724109c; clean worktree.dart format --output=none --set-exit-if-changed .: 555 files, 0 changed.flutter analyze: no issues.- Focused discovery/cache suite: 15 passed.
- Independent targeted production/widget suite: 130 passed;
just mobile-checkpassed;git diff --checkpassed. - Temporary rollback reproducer failed as described, was removed, and the checkout was returned clean.
CI: at review time, Mobile remained in progress. Desktop/Linux failures occurred in mobile-only CI paths during apt/browser dependency setup; Windows and macOS Desktop builds passed. These failures do not erase the reproduced mobile defect.
Manual/native evidence: none for this exact head. This branch does not compose sibling #7391's provenance renderer, so it cannot independently clear that native provenance journey.
Residual risk: native iOS/VoiceOver behavior and the composed #7391 provenance journey remain unverified. Neither gap substitutes for the concrete ordered-authority defect above.
— :bot: Jude's code review agent
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES on exact head 1db724109c5542e2081a1c71f60b0554e870d0cf over b829496e0bf363939afd6b54fc67b4b08716b35b.
The claimed single ordered profile authority still has an unordered production write. UserCacheNotifier.put writes profile state without advancing _profileEventOrders (mobile/lib/shared/profile/user_cache_provider.dart:48-51), while current-user profile publication retains the confirmed submittedEvent but constructs a detached profile and calls put (mobile/lib/features/profile/profile_provider.dart:206-239). A newer live event can land between confirmation and that write; the older detached write then overwrites it while the order map still records the newer event, allowing stale owner/provenance to persist.
A temporary exact-head production-level cache regression bound the defect: put(newer owned profile) followed by cacheProfileEvent(older revoked kind:0) expected the newer profile/owner to survive; actual state rolled back to the older profile. The test was removed and the checkout returned clean.
The replacement head does improve terminal-CLOSED recovery and deterministic readiness tests: full local Flutter suite passed 2,107 tests; format/analyze and focused discovery/cache tests passed. Those gains do not close the unordered authority bypass. The picker also collapses loading/error to an empty result with no visible retry state; treat that UX as follow-up unless product intent requires explicit disclosure, but it is not needed for the authority blocker.
Author action: route signed-event-derived profile writes through the ordered event API, or require atomic event order on every authority-bearing write. Add a deterministic interleaving regression: newer revocation arrives between confirmation and local commit; the older publish cannot overwrite it, and replay cannot preserve stale provenance.
Verification owner: author for fix/regression and green exact-head Mobile; reviewer for failing-before/passing-after mutation, full suite, provenance/picker consequence, and ancestor composition.
Desktop apt/hash failures were ambient on this mobile-only diff. Native iOS/VoiceOver and sibling #7391 composition remain confidence gaps, not the request-changes basis.
Signed-off-by: Logan Johnson <loganj@squareup.com>
|
Addressed both new reviews (5157940400 / 5157949930) at verified published head The production write family now has one ordered signed-event authority:
Regression binds the actual ProfileNotifier + signed publication seam: inject a newer signed owner revocation while the older confirmation response is returning, then assert the newer profile, absent owner and exact governing event survive detached put, event replay and stale hydration. A subsequent edit refuses stale history, then preserves the confirmed newer custom metadata, revoked tags and event timestamp. Separate first-seed coverage preserves legitimate local display use. Reintroducing detached publication fails the immediate current-profile assertion; removing the put restriction fails the owner assertion. Restored source passed the final gates. At exact final head: full |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE at exact head e9c8be29e2b59080f8e9529a67853d950d8c6a0c against base b829496e0bf363939afd6b54fc67b4b08716b35b.
No unresolved author-actionable defect remains in the changed-head delta or full affected boundary.
The previous split-authority concern is closed. Profile hydration, refresh, live kind-0 delivery, and confirmed local writes now converge on the ordered UserCacheNotifier event authority. The write path refuses relay state that conflicts with a newer governing cached event before publishing, late confirmations cannot lower newer ownership/profile state, account/community generation fences stale async cache writes, and discovery updates the shared authority before invalidating its directory. The mention picker also fails closed for authenticated-owned agents while policy discovery is unresolved, without dropping ordinary human candidates.
Exact-head evidence:
just mobile-check: passed (555 files unchanged; analyzer clean).just file-size-check: passed.- Six affected test files: 50/50 passed; an overlapping affected-file run independently passed 48 tests.
git diff --check: passed on a clean tree.- GitHub exact-head
Clients / Mobileand Mobile aggregate checks: passed. - Mutation check independently demonstrated that removing the current-relay-head fold breaks the profile merge regression test; the exact-head implementation was restored and the tree rechecked clean.
Confidence gaps, not author defects:
- Full mobile-suite runs encountered unrelated
voice_note_recording_test.darttemp-directory teardownPathNotFoundExceptions; each implicated test passed when rerun in isolation (one lane: 2,108 passed / 1 failed before isolated pass; another: 2,107 passed / 2 failed before the 20-test file passed). - The required Desktop aggregate is red from a persistent forum draft E2E failure plus a retry-green huddle test in Desktop-only files. This PR changes only
mobile/**; the mobile checks are green. CI/maintainers own that unrelated required-gate failure. - No native iOS simulator/relay journey was run, so end-to-end lifecycle/picker behavior retains native-observation risk. Verification owner: reviewer/tooling or release validation; author action: none.
Any new head invalidates this approval.
Signed-off-by: Logan Johnson <loganj@squareup.com>
The seeded two-second status expired while the popover and editor setup ran, so the smoke test opened a new-status editor instead of the saved draft it intended to exercise. Freeze the browser clock before navigation, assert the saved editor state before editing, and advance the clock past the seeded deadline only after the draft is open, retaining every existing error/draft/save assertion. A sibling control test crosses the deadline before opening to prove the saved-editor precondition fails under that wrong ordering. Signed-off-by: Logan Johnson <loganj@squareup.com>
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: b829496e0bf363939afd6b54fc67b4b08716b35b..7d5409f37e5573c19567ca80cde78a3c4a1dd908 (exact head 7d5409f37e5573c19567ca80cde78a3c4a1dd908)
Risk: high — this changes live profile authority, discovery cardinality, ownership provenance, and fail-closed mention authorization.
Blocking finding
The new shared-authority path turns an unrestricted live profile stream into an unbounded discovery registry, then feeds that registry into a hard 1,000-key limit.
- The discovery subscription has no author/tag restriction and receives all live kind:0 events (
mobile/lib/shared/mentions/agent_discovery.dart:104-110). This PR newly writes every such event intoUserCacheNotifier(:111-118). - The cache's profile/event maps have no capacity or eviction for the account/community lifetime (
mobile/lib/shared/profile/user_cache_provider.dart:15-20,110-118). Unrelated profile traffic therefore grows retained state. - Every cached owner-bearing profile is promoted into the next directory authorization request (
mobile/lib/shared/mentions/agent_identity_provider.dart:94-102), butreadAgentAuthorizationrejects requests above 1,000 keys (mobile/lib/shared/mentions/agent_authorization.dart:20-25). After 1,001 distinct valid owner-attested profiles have been observed, a subsequent directory refresh persistently fails closed and authenticated agents disappear from autocomplete. This also conflicts with the repository's 50K-agent scale target (VISION.md:196-203).
The added revocation/replay test covers one identity, not unrelated kind:0 retention or the 1,000→1,001 boundary (mobile/test/shared/mentions/agent_discovery_test.dart:282-337). Two independent exact-head source passes reproduced this contract chain.
Author action: do not retain an unrestricted global kind:0 stream as discovery authority. Bound discovery-owned identities with explicit eviction/generation semantics, or subscribe/refresh only exact relevant keys, while preserving live owner-revocation ordering. Add production-seam regressions proving unrelated kind:0 events do not grow discovery authority and the 1,000→1,001 transition cannot wedge the directory.
Verification owner: author for the design and regressions; reviewer for causal mutation, full-capacity distinct-key probe, stale callback/account-community rebuild, and the full Mobile gate on the replacement head.
Fresh Desktop delta
The 68b042a..7d5409f delta is test-only (desktop/tests/e2e/profile-custom-emoji-status.spec.ts:196-276). Freezing the browser clock before navigation, asserting the saved-editor precondition, advancing past expiry only after editing, and adding the already-expired-before-open control make the regression causal. No defect was found in that delta.
Validation and confidence gaps
- Exact local/live head match and clean dedicated checkout confirmed immediately before submission.
git diff --check b829496e0..7d5409f: passed.- DCO, Semgrep, zizmor, changed-path detection, and Desktop E2E relay were green when polled; exact-head Mobile and Desktop suites were still running.
- The focused Desktop test could not start in two lanes because another local server occupied port 4173. That is reviewer-tooling contamination, not author rework.
- No native iOS/VoiceOver or composed #7391 provenance journey was run. Those remain reviewer/release-owned confidence gaps, separate from the concrete blocker.
— :bot: Jude's code review agent
clock.install({ time }) only sets the fake clock's epoch: fake time keeps
flowing with real time, so the seeded two-second status could still
expire during popover/editor setup on a slow machine — a 3s real delay
before opening the editor reproduces the original fresh-editor failure.
clock.setFixedTime pins Date.now() while timers, network, and rendering
keep running (the same idiom as the channels.spec.ts ephemeral-countdown
test), so the saved editor preconditions hold regardless of setup
duration. Expiry is crossed deliberately by advancing the pinned date
past the deadline and firing the pending timer with fastForward; the
expired-before-opening control test crosses the same deadline before
opening.
Signed-off-by: Logan Johnson <loganj@squareup.com>
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: b829496e0bf363939afd6b54fc67b4b08716b35b..4d83496d61f5c5945030ac3b5b1351dcb1ff8cf5 (exact head 4d83496d61f5c5945030ac3b5b1351dcb1ff8cf5)
Risk: high — this changes live Nostr profile authority, authorization cardinality, revocation ordering, and fail-closed agent autocomplete.
Blocking finding
The PR still turns an unrestricted live profile stream into an unbounded authorization input, then rejects that input above 1,000 identities.
- Discovery subscribes without an author/tag restriction to kinds including kind:0, and this PR writes every received kind:0 event into the shared cache (
mobile/lib/shared/mentions/agent_discovery.dart:104-115). - The cache retains profile and governing-event maps without a capacity or eviction policy (
mobile/lib/shared/profile/user_cache_provider.dart:15-20,114-118,167-185). - The next directory refresh promotes every cached owner-bearing profile into
requestedKeys(mobile/lib/shared/mentions/agent_identity_provider.dart:94-102). readAgentAuthorizationrejects the entire request when that set exceeds 1,000 keys (mobile/lib/shared/mentions/agent_authorization.dart:20-25).
Unrelated but valid owner-attested profile traffic can therefore grow PR-owned retained state until the 1,000→1,001 transition makes authenticated agents disappear from autocomplete after refresh. The picker intentionally falls back to humans while directory policy errors, so this presents as silent omission rather than a recoverable visible state (mobile/lib/features/channels/mentions/mention_candidates_provider.dart:80-105). It also conflicts with the repository's 50K-agent scale target (VISION.md:196-203).
The searched PR tests cover one identity's revocation/replay ordering, not unrelated kind:0 intake or the exact 1,000→1,001 boundary (mobile/test/shared/mentions/agent_discovery_test.dart:282-337). Two independent exact-head review lanes re-traced this same production chain.
Author action: prevent unrelated kind:0 traffic from entering discovery authorization cardinality. Constrain subscription/refresh to exact discovery-owned identities, or introduce bounded retention with explicit eviction/generation semantics that preserve live revocation ordering. Add production-seam regressions for unrelated profiles and the 1,000→1,001 distinct-key transition, including post-refresh autocomplete and account/community rebuild behavior.
Verification owner: author for design, fix, and regressions; reviewer for mutation proof, exact boundary behavior, revocation ordering, stale callback/reconnect/account-community rebuild, stacked composition, and the full Mobile gate on the replacement head.
Fresh Desktop delta
The sole 7d5409f..4d83496 change replaces a flowing fake clock with page.clock.setFixedTime, explicitly advances Date.now() beyond expiry, then fires the pending timer (desktop/tests/e2e/profile-custom-emoji-status.spec.ts:196-276). This removes machine-speed dependence while retaining saved-editor preconditions and an expired-before-open negative control. No defect was found in this delta; an exact-head focused run of both status-expiry cases passed 2/2.
Validation and confidence gaps
- Live PR head, local HEAD, supplied base, and clean dedicated checkout were rechecked immediately before submission.
git diff --check b829496e0..4d83496: passed.- Exact-head lane evidence:
just mobile-checkpassed; fulljust mobile-testpassed 2,109/2,109; Desktop check/typecheck/unit suite passed 6,450/6,450; focused status-expiry E2E passed 2/2. - At the final CI poll, DCO, Semgrep, zizmor, changed-path detection, relay artifact production, Desktop Windows build, and Desktop E2E relay were green; Mobile, Desktop, macOS, Mobile Swift, and integration jobs remained in progress. No exact-head red gate was observed. Pending CI is a confidence gap, not this request-changes basis.
- No native iOS/VoiceOver or composed provenance journey was run. That remains reviewer/release-owned residual risk.
— :bot: Jude’s code review agent
Summary
An owner revocation could refresh the mobile agent directory while leaving the loaded profile—and therefore lifecycle ownership, provenance, or ordinary-channel mention fallback—unchanged. Owned agents could also reappear in autocomplete while policy was loading or failed.
Related issue
N/A; requested-change followup for #7389, #7391 and #7392. Closest existing PRs are #7395 (discovery lifecycle) and those three review targets; exact-branch search found no existing followup.
Dependency graph: based on published #7395 (
b829496e0bf363939afd6b54fc67b4b08716b35b), which depends on #7393 → #7392 → #7389. #7391 is a separate child of #7389: its provenance renderers need this shared-cache fix integrated alongside them, not backported into the earlier slices. #7390/#7394 remain rollout prerequisites for discovery's invitation/publication workflow, not compile dependencies of this PR. No changes to the promised #7393 or SEND bases.putonly seeds absent local display state and cannot overwrite newer ownership evidence.Testing
At exact
e9c8be29e2b59080f8e9529a67853d950d8c6a0c, based on unchanged corrected #7395b829496e0bf363939afd6b54fc67b4b08716b35b(+451/−86 = 537 lines):just mobile-check: passed (format/analyzer).just mobile-test: full mobile suite, 2,109 passed.just file-size-check: passed.just ci: the bounded 90-second local attempt at corrected parent headb829496e0was terminated during workspace clippy, and that limitation carries forward — repository-wide CI completion is not claimed at this head. Full mobile suite above is the executed evidence here.No layout change or native device/simulator validation; tests cover authority delivery and picker eligibility, not an integrated native #7391 provenance journey. Existing #7391 widget evidence remains renderer-only. No screenshots added to Git.
Desktop status-expiry smoke correction (2026-09-10)
Smoke3 at
68b042a73a714edd0fca04283300bcb09974b497(run 34431095101, job 102726723382) failedprofile-custom-emoji-status.spec.ts"keeps an open status draft when the saved status expires": the seeded two-second status expired during popover/editor setup (retry1 opened the editor ~14 ms before the deadline; typing began after expiry), so the dialog opened as a new-status editor and the expired-duration alert never appeared. Source inspection found no product defect — the dialog's open-capture effect and the user-status expiration timer behave as designed.4d83496d61f5c5945030ac3b5b1351dcb1ff8cf5(own churn 595→598 vs base) repairs the test with the repository's existing Playwright clock idiom:clock.setFixedTimepins the browser Date before navigation (timers, network, and rendering keep running), so the saved editor state asserted before editing (input/emoji, Duration "Custom", Clear present, no presets) holds however slow the machine is; the draft is edited, the pinned Date is advanced past the same seeded two-second deadline, and the pending expiration timer is fired withclock.fastForward, keeping every existing alert/draft/save assertion unchanged. An earlier attempt on this lane (7d5409f37, +51/−1) usedclock.install({ time }), but that only sets the fake clock's epoch — installed Playwright 1.60.0 keeps fake time flowing with real time, and a diagnostic 3s real delay before opening the editor reproduced the fresh-editor failure — so it was corrected rather than left racing the deadline on slow CI. A sibling control test crosses the deadline before opening and asserts the opposite new-status editor; reordering the main test locally fails the saved-editor precondition (empty input), so the precondition is falsifiable. Local validation: full spec 9/9 passing on an isolated runner (retries 0),tsc --noEmitand Biome clean. Published CI on the new head supplies package-level evidence; no full-package claim is made from this spec alone, and no mobile or production files changed.