feat: v1.0 UI parity batch — Identity Hub contacts, shielded receive, migration fix, nav pills, disclosure closure

[Claudius-Maginificent]

Why this PR exists

• Problem: A UI feature-parity audit against v0.10-dev found 31 items PR feat: rewrite Dash Evo Tool onto the new platform-wallet #860's platform-wallet rewrite needed to close before it can become v1.0-dev — 14 real gaps that had to land in v1.0 (Identity Hub contacts, shielded receive address, token tracking, settings/vote migration on upgrade, nav pills, single-key wallet), and 17 "accepted as gone" removals that still needed a real disclosure, a cleanup, or a formal sign-off rather than silence.
• What breaks without it: without this PR, upgrading from v0.10-dev silently resets a user's network selection and drops their scheduled DPNS masternode votes with no notice (a real vote-window deadline risk); Identity Hub's Contacts tab has dead Accept/Decline buttons and shows nobody's real contacts; a user cannot view or copy their own shielded receive address at all, so they cannot receive a private transfer; and several other v0.10-dev capabilities are gone with no CHANGELOG trail six weeks after being flagged.
• Blocking relationship: stacked on top of feat: rewrite Dash Evo Tool onto the new platform-wallet #860 (docs/platform-wallet-migration-design) — this PR targets that branch, not v1.0-dev directly, since feat: rewrite Dash Evo Tool onto the new platform-wallet #860 hasn't merged yet.

What was done

Part A — 14 v1.0 must-land items

• Identity Hub: Contacts Accept/Decline wired to the existing DashPay backend; Cancel implemented as an honest hide+notify (Platform's contactRequest documents are immutable/undeletable, so a true withdrawal isn't possible); real active-contacts list with search; local alias editing; pay-a-contact entry point.
• Shielded receive-address view restored (address, Copy, no unsafe diversified-address affordance — that specific control needs an upstream platform-wallet API that doesn't exist yet).
• Single-key wallet send/monitoring: investigated for real, confirmed blocked upstream (the only public watch-only-wallet registration path requires an HD seed); dead UI gates removed and the limitation clearly surfaced instead of shipping a broken or funds-unsafe workaround.
• Token "stop tracking" fix: a dismissed token balance no longer reappears after refresh.
• Settings/data migration: network, theme, onboarding, scheduled DPNS votes, and top-up history are imported from the legacy v0.10-dev database on first launch; a corrupt/unreadable vote row is surfaced via a warning banner instead of silently blocking wallet-fund access.
• Global nav: masternode object pill and Wallets pill made fully interactive and two-way bound.
• Expert-mode nav-refresh: investigated, confirmed already fixed by an earlier PR (stale audit finding).

Part B — 17 "not planned" closure items

• 3 undisclosed removals given a real paper trail (CHANGELOG entry, gap-audit table row, and the missing RootScreenType gravestone comment for the retired Masternode List Diff screen).
• 2 disclosed-but-incomplete removals strengthened with proper CHANGELOG coverage.
• 2 small dead-code deletions (a disk-read-on-every-network-switch dead settings field; a click sensor with no remaining consumer).
• 10 already-fully-disclosed removals formally signed off in a new closure record.

QA fix-wave (post-implementation, adversarially reviewed by 6 independent agents — 3 correctness, 1 security, 1 project-consistency, 1 live-desktop pass):

• Fixed a fund-blocking regression where a single corrupt legacy vote row could permanently block wallet-fund migration.
• Fixed a stuck-UI busy-flag bug and a double-send window on the (currently unreachable) single-key send screen.
• Ungated the two remaining legacy DashPay Pay/Send entry points to match the newly-ungated Identity Hub button, removing a stale gating inconsistency.
• Narrowed a TOCTOU race in contact cancellation and added an unhide affordance for hidden contacts.
• Closed a silent-transposition risk in the token-dismissal API by threading an existing typed identifier instead of a bare tuple.

DRY cleanup pass (post-QA, audited for duplication and consolidated):

• Unified two divergent identity-ID shorteners that rendered the same identity differently on the Contacts tab vs. an identity pill (user-visible fix, TDD RED→GREEN).
• Merged the Contacts-tab label-priority rule into the existing hub-wide display_label resolver so pills and the contacts list can no longer disagree on what to call an identity.
• Promoted a single model::dashpay::contact_request_recipient() extractor and a single contact_request_query() builder, replacing 5 and 11 duplicated call sites respectively.
• Promoted a single table_exists/column_exists SQLite schema-probe pair into database::mod, replacing 4 and 11 duplicated inline probes respectively (migration-error typing preserved at each call site).
• Extracted a shared per-row render helper for the Hub's incoming/sent contact-request lists.

Bot-review fix wave (10 findings from automated review, fixed across 4 independent agents, each merged separately):

• Token-dismissal race: switched contract_token_db from a single-blob read-modify-write to a presence-marker-per-pair scheme, closing a lost-update window between concurrent dismissals.
• Contact-request identity scoping: DashPayContactRequests/DashPayContactsWithInfo now carry their owning identity explicitly; a new sender_of_received_request()/recipient_of_sent_request() mirror pair stops a signed state transition from ever targeting the wrong identity. Withdrawal/decline resolution markers split into separate declined/withdrawn KV prefixes so the two states can no longer collide.
• Legacy-import durability: finish_unwire/legacy_import now decode scheduled-vote and top-up rows individually (decode_scheduled_vote_columns/decode_top_up_columns) instead of failing the whole batch on one bad row, and top-up history writes go through a proper merge seam (save_top_ups_in) instead of clobbering. A durable, per-network warning record surfaces any row that's still unreadable, with an explicit acknowledgement task instead of a silent drop.
• Contacts-tab UI correctness: combined the Contacts/ContactRequests load dispatch into a single AppAction::BackendTasks so one round-trip can't lose the other; added an in_flight guard so double-clicking Accept/Decline/Cancel can't double-submit; fixed an accepted_accounts: None handling bug (in both the Hub's unhide path and the legacy contact-details save path) that silently wiped a user's account allow-list instead of preserving it.

Testing

• cargo clippy --all-features --all-targets -- -D warnings: clean, 0 warnings, verified on a from-scratch build in an isolated target directory (not a shared/cached one) at the current HEAD.
• cargo test --all-features --workspace: 1591 lib + 216 kittest UI-integration tests, 0 failures, verified the same way — including reconciliation with a concurrently-merged v0.9.3→v1.0 upgrade-path lock test (tests/legacy_table_surface.rs's allow-list extended for its scratch-fixture read).
• cargo +nightly fmt --all: clean.
• A live interactive desktop smoke test confirmed no crashes and correct rendering across the changed screens in both light and dark themes; two screens (Identity Hub contacts, shielded tab) couldn't be exercised fully live in this sandbox for lack of outbound DAPI/network access — covered instead by the passing automated suites above.

Breaking changes

None. All changes are additive or fix regressions against the v0.10-dev baseline; no public API or on-disk schema changes beyond the new (additive, backward-compatible) legacy-import migration path.

Checklist

• Tests pass
• Clippy clean
• cargo +nightly fmt applied
• docs/user-stories.md updated for every story this PR implements or closes
• CHANGELOG updated for every disclosed removal

Attribution

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)

• master
• v1.0-dev

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 27ce5377-519b-47b5-beb6-e50d79cd251c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/v1.0-parity-batch

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

⛔ Blockers found — Sonnet deferred (commit f6f01db)
Canonical validated blockers: 6

[thepastaclaw]

Code Review

This parity batch has several well-tested improvements, and the wallet-drain/app-data separation is sound, but exact-head verification found five correctness blockers in the new contacts and migration paths. The Contacts tab currently discards its established-contact load, async results are not scoped to the identity that initiated them, top-up history can be permanently skipped or overwritten, cancellation can report success without persisting its only withdrawal marker, and malformed legacy vote columns can still abort the whole vote import. The remaining suggestions cover state-loss and concurrency gaps in the same new surfaces.

🔴 5 blocking | 🟡 5 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/ui/identity/contacts.rs`:
- [BLOCKING] src/ui/identity/contacts.rs:298-309: LoadContacts is overwritten before it can be dispatched
  `AppAction`'s `BitOrAssign` implementation replaces the current non-None action instead of merging it (`src/app.rs:397-406`). The second `action |= BackendTask(LoadContactRequests)` here therefore discards the immediately preceding `LoadContacts` action, even though the comment says both are dispatched together. `claim_load()` is already consumed, so the active/hidden contact lists, search, and Pay entry points stay empty until an unrelated invalidation happens. Return one `AppAction::BackendTasks` containing both tasks in `Concurrent` mode.
- [SUGGESTION] src/ui/identity/contacts.rs:347-353: Contact actions have no in-flight guard against duplicate submissions
  Accept, Decline, and the analogous Cancel path dispatch directly while leaving the row clickable until an async result arrives. Repeated clicks across frames can launch duplicate Platform writes for the same request, with no per-request busy state or immediate row removal. Track in-flight request IDs and disable their actions until completion, restoring the row on failure.
- [SUGGESTION] src/ui/identity/contacts.rs:151-161: Unhide replaces the contactInfo document with an empty accepted-account list
  `UpdateContactInfo` is a full replacement: `create_or_update_contact_info` constructs fresh private data and assigns every supplied field. This task preserves nickname and note but always supplies `accepted_accounts: Vec::new()`, erasing all accepted account metadata when a hidden contact is restored. `ContactData` exposes only one `account_reference`, so this UI cannot reconstruct the original collection; use a visibility-only backend operation that decrypts and preserves the existing private fields.

In `src/ui/identity/hub_screen.rs`:
- [BLOCKING] src/ui/identity/hub_screen.rs:357-369: Late contact results can be applied to a different active identity
  These result variants carry no queried identity, and the hub records every arriving result unconditionally. Switching identities resets the cache but cannot cancel an already-running load, so a result for identity A can arrive after identity B becomes active and populate B's Contacts tab. The resulting incoming rows are then rendered with B's `QualifiedIdentity`; Accept and Decline refetch only by document ID and do not verify that `toUserId` equals B, so the stale row can trigger a state transition for the wrong identity. Include the queried identity in both result variants and discard results that do not match the current selection.

In `src/backend_task/migration/finish_unwire.rs`:
- [BLOCKING] src/backend_task/migration/finish_unwire.rs:621-644: Top-up migration failures are made permanent by the shared sentinel
  Both a legacy top-up read failure and each `save_top_ups` failure are reduced to warnings, after which this function still returns success and `migrate_app_data` writes the per-network sentinel. Every later launch then short-circuits before retrying, permanently hiding history that never reached the new store. In addition, `save_top_ups` replaces the entire stored map, so a stale legacy snapshot can overwrite top-ups already recorded by a newer build. Preserve existing entries and track top-up completion separately, or leave a retryable state whenever readable history was not persisted.
- [SUGGESTION] src/backend_task/migration/finish_unwire.rs:483-489: Unreadable-vote warning is permanently lost after the first launch
  The same pass that reports `votes_unreadable` writes the app-data sentinel, but `MigrationStatus` is only in memory. On every later launch this short-circuit returns the default zero-count outcome, so the sticky warning can appear only during the discovery run; if the user closes or misses that banner, the unimported vote is never surfaced again even though its deadline may still matter. Persist the warning/count (or a durable acknowledgement) separately from the completion sentinel and restore it on later boots until acknowledged.

In `src/backend_task/dashpay/contact_requests.rs`:
- [BLOCKING] src/backend_task/dashpay/contact_requests.rs:1016-1025: Cancellation reports success when its withdrawal marker was not stored
  The local sidecar marker is the only state `load_contact_requests` consults to retire an immutable sent request. `mark_withdrawn` returns `()` and swallows both an unavailable wallet backend and a typed storage error, so `cancel_flow` still returns `Withdrawn`; the UI removes the row and announces success even though it reappears on refresh. Make `mark_withdrawn` return `Result<(), TaskError>` and propagate failure before returning `DashPayContactRequestCancelled`.
- [SUGGESTION] src/backend_task/dashpay/contact_requests.rs:186-196: One undirected marker conflates cancelled outgoing and declined incoming requests
  Both decline and cancel write the same `(owner, counterparty)` rejection marker, and this filter applies that marker symmetrically to incoming and outgoing rows. After A cancels a request to B, a later genuine request from B to A is silently filtered until A proactively sends to B again. Store resolution direction in the marker, or apply outgoing-cancellation markers only to outgoing documents.

In `src/database/legacy_import.rs`:
- [BLOCKING] src/database/legacy_import.rs:186-192: Malformed SQLite columns still abort the entire vote import
  The new row-level `unreadable` handling begins only after all five `row.get` conversions succeed. A negative timestamp decoded as `u64`, a NULL, or a SQLite type mismatch therefore returns from `read_scheduled_votes` immediately, discards every valid vote accumulated so far, and turns the app-data pass into a hard failure instead of the promised warning-and-skip behavior. Decode each row into a per-row result and count any conversion failure as unreadable so one corrupt row cannot wedge the batch.

In `src/context/contract_token_db.rs`:
- [SUGGESTION] src/context/contract_token_db.rs:407-430: Dismissed-token read-modify-write operations can lose concurrent updates
  Mark and clear each read the complete `TOKEN_UNTRACKED_KEY` set, mutate a local copy, and replace the complete payload without synchronization. Backend tasks run independently, so two quick dismissals or a mark racing a clear can both read the old set and then overwrite one another; the lost pair reappears on refresh. Serialize accesses to this blob with shared state, or store each identity-token pair under its own key.

[thepastaclaw]

Code Review

This cumulative review of PR #882 at head 108af44 confirms the latest push (875920f..108af44) is a behavior-preserving DRY consolidation (shared contactRequest query/recipient helpers, shared table_exists/column_exists probes, a unified display_label/shorten_id resolver, a shared request_row renderer) with no new correctness defects in the paths it touches. However, all 10 blocking/suggestion findings from the prior review at 875920f remain verified STILL_VALID against the current source: the Contacts tab's LoadContacts dispatch is still discarded by AppAction's overwrite-not-merge BitOrAssign, async contact results still aren't scoped to the identity that requested them, top-up migration failures are still made permanent by an unconditional sentinel write with a full-replace save, contact-request cancellation still reports success without confirming its withdrawal marker persisted, and malformed legacy vote columns still abort the entire scheduled-votes import via an early '?'. One additional finding (a dropped tracing::warn! diagnostic in the mutual-contact filter refactor) was confirmed as genuinely caused by this delta but is a nitpick that doesn't fit the comment budget alongside the 10 still-valid higher-severity findings, so it is tracked separately rather than posted. A second delta-lane candidate (a payments.rs typed-error-conversion gap) was verified false: payments.rs was never touched anywhere in this PR's full commit range, so the claimed PR responsibility for that inconsistency is inaccurate and the finding was dropped as out of scope.

Source: orchestrator openai/gpt-5.6-sol (high, orchestration-only); reviewers codex-general gpt-5.6-sol, sonnet5-general claude-sonnet-5, opus-general claude-opus-4-8, codex-rust-quality gpt-5.6-sol, sonnet5-rust-quality claude-sonnet-5, opus-rust-quality claude-opus-4-8; verifier sonnet5 claude-sonnet-5; experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_opus_sample, bucket 0.

🔴 5 blocking | 🟡 5 suggestion(s)

10 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/ui/identity/contacts.rs`:
- [BLOCKING] src/ui/identity/contacts.rs:317-332: LoadContacts is discarded before it can be dispatched
  `render_populated` fires two single-task actions back to back: `action |= AppAction::BackendTask(LoadContacts)` immediately followed by `action |= AppAction::BackendTask(LoadContactRequests)`. `AppAction::BitOrAssign` (src/app.rs:397-406) does `*self = rhs` whenever `rhs` is non-`None` — it replaces rather than merges — so the second assignment silently overwrites the first. Since `state.claim_load()` (line 321) is a one-shot guard already consumed by this call, `DashPayTask::LoadContacts` never actually reaches the backend on this dispatch: the established-contacts list, its search box, and the Pay entry points stay empty until some unrelated event re-arms the guard. `AppAction::BackendTasks(Vec<BackendTask>, BackendTasksExecutionMode::Concurrent)` already exists in the same enum and is unused here. Confirmed unchanged by the 875920fb..108af441 delta (only cosmetic doc-comment lines shifted).
- [SUGGESTION] src/ui/identity/contacts.rs:131-146: Contact actions have no in-flight guard against duplicate submissions
  The shared `request_row` helper (extracted by this delta from what was previously inlined in `received_section`/`sent_section`) dispatches Accept/Decline/Cancel directly off `response.action()` with no per-request busy state, and the row stays clickable on subsequent frames until the async result arrives via `hub_screen::display_task_result`. Repeated clicks before the first result lands can fire duplicate Platform state-transition writes for the same request. Track in-flight request IDs, disable the row's actions while a request is outstanding, and clear the guard on success or failure.
- [SUGGESTION] src/ui/identity/contacts.rs:176-185: Unhide replaces the contactInfo document with an empty accepted-account list
  `unhide_task` builds `DashPayTask::UpdateContactInfo` with `accepted_accounts: Vec::new()`. `create_or_update_contact_info` (src/backend_task/dashpay/contact_info.rs:342-347) constructs a fresh `ContactInfoPrivateData` and assigns `private_data.accepted_accounts = accepted_accounts` unconditionally, fully replacing the encrypted payload. Nickname and note ride along and are preserved, but every previously accepted account is erased the moment a hidden contact is restored. `ContactData` exposes only a single `account_reference`, so this UI has no way to reconstruct the original collection to pass through. Use a visibility-only backend operation that decrypts and preserves all existing private fields instead of reconstructing them from scratch.

In `src/ui/identity/hub_screen.rs`:
- [BLOCKING] src/ui/identity/hub_screen.rs:361-369: Late contact results can be applied to a different active identity
  `BackendTaskSuccessResult::DashPayContactRequests { incoming, outgoing }` and `DashPayContactsWithInfo(Vec<ContactData>)` (src/backend_task/mod.rs:225-230) carry no queried-identity field, and `display_task_result` records both unconditionally into `self.contacts_state` — unlike the sibling `DashPayProfileUpdated` arm two lines above, which guards on `saved_id` matching the currently selected identity. Switching identities resets the local cache but cannot cancel an in-flight load, so a result queried for identity A can arrive after the user has switched to identity B and populate B's Contacts tab with A's requests and contacts. Accept/Decline then act only on the request document ID, never checking that `toUserId` matches the now-active identity B, so a stale row can drive a state transition under the wrong identity. Thread the queried identity through both result variants and discard results whose identity doesn't match the current selection.

In `src/backend_task/migration/finish_unwire.rs`:
- [BLOCKING] src/backend_task/migration/finish_unwire.rs:611-646: Top-up migration failures are made permanent by the unconditional sentinel
  In `migrate_app_data_from_conn`, a `read_top_ups` failure and every per-identity `save_top_ups` failure are both reduced to `tracing::warn!` (lines 618-643), and the function still returns `Ok(outcome)`. `migrate_app_data` then unconditionally writes the per-network completion sentinel at line 549 regardless of whether any top-up history actually made it into the k/v store. Every later launch short-circuits on the `done` check (lines 483-489) and returns the default outcome without ever retrying — the missing audit trail becomes permanently unreachable through the new UI. Compounding this, `save_top_ups` (src/context/identity_db.rs:788-797) performs a full `kv.put` replacement rather than a merge, so a stale legacy snapshot read on a hypothetical retry could still clobber top-ups a newer build already recorded. Track top-up completion separately from the vote sentinel (withhold or mark retryable when any read/write failed) and merge rather than replace existing entries.
- [SUGGESTION] src/backend_task/migration/finish_unwire.rs:483-489: Unreadable-vote warning is permanently lost after the first launch
  The same pass that computes `outcome.votes_unreadable` also writes the app-data sentinel unconditionally (line 549), and the resulting `MigrationStatus::SucceededWithUnreadableVotes { count }` lives only in an in-memory `ArcSwap` (`context/migration_status.rs`). Every launch after the discovery run short-circuits at the `done` check (lines 483-489) and returns the zero-count default outcome, so the sticky warning banner can only ever appear once. If the user closes or misses it, an unimported legacy vote — whose voting-window deadline may still be live — is never surfaced again. Persist the unreadable count (or a durable acknowledgement) separately from the completion sentinel and restore the warning on later boots until acknowledged.

In `src/backend_task/dashpay/contact_requests.rs`:
- [BLOCKING] src/backend_task/dashpay/contact_requests.rs:891-962: Cancellation reports success when its withdrawal marker was not stored
  The local sidecar marker written by `dashpay_mark_rejected` is the only state `load_contact_requests` consults (via `dashpay_is_rejected`, lines 126-134) to retire an immutable, un-deletable sent `contactRequest`. `PlatformCancelOps::mark_withdrawn` (lines 952-962) returns `()` and swallows both an unavailable wallet backend and a typed `dashpay_mark_rejected` storage error, logging only at `debug`. `cancel_flow` (lines 891-905) calls it unconditionally and returns `Ok(CancelOutcome::Withdrawn)` regardless of whether the marker persisted, so `cancel_contact_request` reports `DashPayContactRequestCancelled`, the UI removes the row and shows success — but the request reappears as pending on the very next `load_contact_requests` refresh because nothing was actually recorded. Change `mark_withdrawn` to return `Result<(), TaskError>` and propagate the failure out of `cancel_flow` before it can report `Withdrawn`.
- [SUGGESTION] src/backend_task/dashpay/contact_requests.rs:126-159: One undirected marker conflates cancelled outgoing and declined incoming requests
  Decline (`reject_contact_request`) and cancel (`PlatformCancelOps::mark_withdrawn`, line 954) both write the identical `dashpay_mark_rejected(&owner_id, &counterparty_id)` sidecar marker, and `retain_unresolved` (lines 149-159) applies the same `(owner, counterparty)` predicate symmetrically: incoming rows are filtered by `doc.owner_id()`, outgoing rows by `contact_request_recipient(doc)`. After A cancels a request to B, the marker `(A, B)` also matches a later, genuine incoming request from B to A, silently hiding it until A proactively sends to B again. Persist the resolution direction in the marker, or scope cancellation markers to outgoing documents only.

In `src/database/legacy_import.rs`:
- [BLOCKING] src/database/legacy_import.rs:186-192: Malformed SQLite columns still abort the entire vote import
  `read_scheduled_votes`'s row-level `unreadable` handling (the `let Ok(voter_id) = ...` / `let Some(choice) = ...` guards) only begins after all five `row.get(...)?` conversions on lines 188-192 succeed. A NULL, a negative timestamp decoded as `u64` (line 191), or any other SQLite type mismatch propagates a `rusqlite::Error` straight out of the function via `?`, discarding every valid vote already accumulated in `out.votes` and turning the whole app-data pass into a hard `MigrationError::LegacyDbRead` failure instead of the intended warn-and-skip. This gap is real and untested: the delta's own new test (`scheduled_votes_count_unreadable_rows`) only exercises semantic decode failures (bad voter id / vote choice) that succeed at the `row.get` stage, not a `row.get` conversion failure itself. Decode each column through a per-row `Result` and count any conversion failure as `unreadable` so one corrupt row cannot wedge the batch.

In `src/context/contract_token_db.rs`:
- [SUGGESTION] src/context/contract_token_db.rs:407-431: Dismissed-token read-modify-write operations can lose concurrent updates
  `mark_token_balance_untracked` and `clear_untracked_token_balance` each independently `read_untracked` the complete `TOKEN_UNTRACKED_KEY` set, mutate a local `BTreeSet` copy, and `write_untracked` the whole payload back with no synchronization. Backend tasks run independently, so two quick dismissals — or a mark racing a clear — can both read the same stale set and then overwrite each other, silently losing one of the two updates (a dismissed pair reappearing, or a re-enabled pair staying hidden). Serialize this read-modify-write window behind shared state, or store each identity-token pair under its own key so concurrent writes can't clobber each other. File is untouched by the 875920fb..108af441 delta.

Prior finding reconciliation

• prior-1 — STILL_VALID: LoadContacts is overwritten before it can be dispatched
• prior-2 — STILL_VALID: Late contact results can be applied to a different active identity
• prior-3 — STILL_VALID: Top-up migration failures are made permanent by the shared sentinel
• prior-4 — STILL_VALID: Cancellation reports success when its withdrawal marker was not stored
• prior-5 — STILL_VALID: Malformed SQLite columns still abort the entire vote import
• prior-6 — STILL_VALID: One undirected marker conflates cancelled outgoing and declined incoming requests
• prior-7 — STILL_VALID: Dismissed-token read-modify-write operations can lose concurrent updates
• prior-8 — STILL_VALID: Unreadable-vote warning is permanently lost after the first launch
• prior-9 — STILL_VALID: Contact actions have no in-flight guard against duplicate submissions
• prior-10 — STILL_VALID: Unhide replaces the contactInfo document with an empty accepted-account list

Carried-forward prior findings

All ten prior findings remain valid at this head. Their existing inline threads were retained; no duplicate inline comments or false resolution replies were posted.

New findings in the latest delta

• NITPICK src/backend_task/dashpay/contact_requests.rs:103 — Iterator refactor silently dropped three malformed-toUserId diagnostics: Verified via git diff 875920fb..108af441 -- src/backend_task/dashpay/contact_requests.rs src/backend_task/dashpay/contacts.rs: the pre-refactor nested loops in both files logged tracing::warn! ("Invalid toUserId in contact request document, skipping", "Invalid toUserId in outgoing contact request, keeping in list", and "Failed to parse contact request toUserId (...), skipping") whenever a document's toUserId failed to parse during the mutual-contact/established-contacts computation. The new iterator-based contacts_established/contacts computations call contact_request_recipient(doc) (src/model/dashpay.rs:14-19), a pure function with no logging, and treat None as simply "not a match" via .filter()/.any() — all three warn! call sites were removed. Behavior is unchanged (both versions correctly exclude/keep the same documents), but a legitimate data-corruption signal that previously surfaced in logs is now silently invisible. Not included in the budgeted findings above (nitpick, and the 5 blocking + 5 suggestion prior findings already fill the 10-comment budget) but tracked here as caused by this delta.

[thepastaclaw]

Code Review

At exact head 0fcb6e7, PR #882's delta from 108af44 (git diff confirms exactly 2 files: migration/mod.rs +5 lines module registration, and a new 754-line #[cfg(test)]-only v093_upgrade.rs) is purely additive test code — zero production paths changed. All ten prior findings from the 108af44-verified review (five blocking, five suggestion) were independently re-read against the current head and remain STILL_VALID: none of their six underlying source files (contacts.rs, hub_screen.rs, finish_unwire.rs, contact_requests.rs, legacy_import.rs, contract_token_db.rs) appear in the diff. Spot-verified prior-1 (contacts.rs:317-332, two sequential AppAction |= assignments still discard LoadContacts) and prior-5 (legacy_import.rs:186-192, five propagating row.get(...)? calls still precede row-level unreadable handling) directly against source. review_action remains REQUEST_CHANGES on the strength of the five carried-forward blockers. The disputed delta claim was resolved by direct source inspection: second_launch_after_a_v093_upgrade_changes_nothing (v093_upgrade.rs:673-754) calls the boot() helper — which genuinely opens a fresh Database/AppKv/SecretStore/AppContext from disk (v093_upgrade.rs:367-405) — exactly once at line 678, then re-invokes import_legacy_settings and finish_unwire::run against that same live ctx/backend for the 'second launch,' never calling boot() or wire_backend() again. The is_wallet_registered assertion at 734-736 therefore checks the backend object already registered by the first migration pass, not a freshly reconstructed one. The two Sol lanes (codex-general, codex-rust-quality) are correct; the two Sonnet lanes (sonnet5-general, sonnet5-rust-quality) missed a real, if minor, test-coverage gap. This valid new finding could not be added to the canonical findings array because the ten mandatory carried-forward findings already fill comment_budget=10; it is preserved in new_findings_latest_delta and dropped_findings with an explicit budget-overflow reason rather than discarded. coderabbit_findings=None, so coderabbit_reactions is empty.

Source: Sol reviewer gpt-5.6-sol (lanes codex-general, codex-rust-quality) and Sonnet reviewer claude-sonnet-5 (lanes sonnet5-general, sonnet5-rust-quality); primary verifier claude-sonnet-5. The orchestration-only Sol role (gpt-5.6-sol, high, non-reviewing) is distinct from the codex-general/codex-rust-quality Sol review lanes above.

Experiment: sonnet-primary-opus-quarter-sample-20260710; cohort sonnet_primary; sample bucket 1 (not sampled for Opus).

🔴 5 blocking | 🟡 5 suggestion(s)

10 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/ui/identity/contacts.rs`:
- [BLOCKING] src/ui/identity/contacts.rs:317-332: LoadContacts is discarded before it can be dispatched
  `render_populated` fires two single-task actions back to back: `action |= AppAction::BackendTask(LoadContacts)` immediately followed by `action |= AppAction::BackendTask(LoadContactRequests)`. `AppAction::BitOrAssign` (src/app.rs:397-406) does `*self = rhs` whenever `rhs` is non-`None` — replace, not merge — so the second assignment silently overwrites the first. `state.claim_load()` (line 321) is a one-shot guard already consumed by this call, so `DashPayTask::LoadContacts` never reaches the backend on this dispatch: the established-contacts list, its search box, and the Pay entry points stay empty until an unrelated event re-arms the guard. `AppAction::BackendTasks(Vec<BackendTask>, BackendTasksExecutionMode::Concurrent)` already exists and is unused here. Verified byte-identical at current head — file absent from the 108af441..0fcb6e7e diff.
- [SUGGESTION] src/ui/identity/contacts.rs:131-146: Contact actions have no in-flight guard against duplicate submissions
  The shared `request_row` helper dispatches Accept/Decline/Cancel directly off `response.action()` with no per-request busy state, and the row stays clickable across frames until the async result arrives. Repeated clicks before the first result lands can fire duplicate Platform state-transition writes for the same request. Track in-flight request IDs and disable the row's actions until success or failure.
- [SUGGESTION] src/ui/identity/contacts.rs:176-185: Unhide replaces the contactInfo document with an empty accepted-account list
  `unhide_task` builds `DashPayTask::UpdateContactInfo` with `accepted_accounts: Vec::new()`. `create_or_update_contact_info` (src/backend_task/dashpay/contact_info.rs:342-347) constructs a fresh private payload and assigns this vector unconditionally, fully replacing the encrypted field. Nickname/note ride along and survive, but every previously accepted account is erased the moment a hidden contact is restored. Use a visibility-only backend operation that decrypts and preserves the existing private fields.

In `src/ui/identity/hub_screen.rs`:
- [BLOCKING] src/ui/identity/hub_screen.rs:361-369: Late contact results can be applied to a different active identity
  `BackendTaskSuccessResult::DashPayContactRequests { incoming, outgoing }` and `DashPayContactsWithInfo(Vec<ContactData>)` (src/backend_task/mod.rs:225-230) carry no queried-identity field, and `display_task_result` records both unconditionally — unlike the sibling `DashPayProfileUpdated` arm two lines above, which guards on identity match. Switching identities resets the local cache but cannot cancel an in-flight load, so a result queried for identity A can arrive after switching to B and populate B's Contacts tab with A's data. Accept/Decline then act only on request document ID, never verifying the acting identity, so a stale row can drive a state transition under the wrong identity. Thread the queried identity through both result variants and discard mismatches.

In `src/backend_task/migration/finish_unwire.rs`:
- [BLOCKING] src/backend_task/migration/finish_unwire.rs:611-646: Top-up migration failures are made permanent by the unconditional sentinel
  In `migrate_app_data_from_conn`, a `read_top_ups` failure and every per-identity `save_top_ups` failure are both reduced to `tracing::warn!`, and the function still returns `Ok(outcome)`. `migrate_app_data` then unconditionally writes the per-network completion sentinel at line 549 regardless of whether any top-up history was actually persisted. Every later launch short-circuits on the `done` check (lines 483-489) and never retries. `save_top_ups` (src/context/identity_db.rs:788-797) also performs a full `kv.put` replacement rather than a merge, so a stale legacy snapshot on a hypothetical retry could clobber newer top-ups. Track top-up completion separately from the vote sentinel and merge rather than replace on write.
- [SUGGESTION] src/backend_task/migration/finish_unwire.rs:483-489: Unreadable-vote warning is permanently lost after the first launch
  The pass that computes `outcome.votes_unreadable` also writes the app-data sentinel unconditionally, and the resulting count lives only in an in-memory `MigrationStatus`. Every launch after the discovery run short-circuits at the `done` check and returns a zero-count default outcome, so a warning missed on the first post-upgrade launch can never resurface even though the DPNS vote deadline may still be live. Persist the count or an acknowledgement separately from the completion sentinel.

In `src/backend_task/dashpay/contact_requests.rs`:
- [BLOCKING] src/backend_task/dashpay/contact_requests.rs:891-962: Cancellation reports success when its withdrawal marker was not stored
  The local sidecar marker written by `dashpay_mark_rejected` is the only state `load_contact_requests` consults (via `dashpay_is_rejected`) to retire an immutable, un-deletable sent `contactRequest`. `PlatformCancelOps::mark_withdrawn` (lines 952-962) returns `()` and swallows both an unavailable wallet backend and a typed storage error, logging only at `debug`. `cancel_flow` (891-905) calls it unconditionally and returns `Ok(CancelOutcome::Withdrawn)` regardless of whether the marker persisted, so the UI reports cancellation success while the row reappears on the next refresh. Change `mark_withdrawn` to return `Result<(), TaskError>` and propagate failure before reporting success.
- [SUGGESTION] src/backend_task/dashpay/contact_requests.rs:126-159: One undirected marker conflates cancelled outgoing and declined incoming requests
  Decline and cancel both write the identical `dashpay_mark_rejected(owner, counterparty)` sidecar marker, and `retain_unresolved` (149-159) applies the same predicate symmetrically to incoming and outgoing documents. After A cancels a request to B, the marker also matches a later genuine incoming request from B to A, silently hiding it until A proactively re-sends. Persist the resolution direction in the marker, or scope cancellation markers to outgoing documents only.

In `src/database/legacy_import.rs`:
- [BLOCKING] src/database/legacy_import.rs:186-192: Malformed SQLite columns still abort the entire vote import
  `read_scheduled_votes`'s row-level unreadable handling (the `let Ok(voter_id) = ...` / `let Some(choice) = ...` guards) only begins after all five `row.get(...)?` conversions on lines 188-192 succeed. A NULL, a negative timestamp decoded as `u64`, or any other SQLite type mismatch propagates a `rusqlite::Error` straight out of the function via `?`, discarding every valid row already accumulated instead of incrementing `unreadable` and continuing. This delta's new v093_upgrade.rs fixture only inserts one well-formed vote row, so it neither exercises nor closes this gap. Decode each column through a per-row `Result` and count conversion failures as unreadable.

In `src/context/contract_token_db.rs`:
- [SUGGESTION] src/context/contract_token_db.rs:407-431: Dismissed-token read-modify-write operations can lose concurrent updates
  `mark_token_balance_untracked` and `clear_untracked_token_balance` each independently read the complete untracked-token set, mutate a local copy, and write the whole payload back with no synchronization. Two concurrent backend tasks can read the same stale set and overwrite each other, silently losing a dismissal or re-enable. File untouched by any delta since 875920fb. Serialize this read-modify-write window or store each identity-token pair under its own key.

Prior finding reconciliation

• prior-1 — STILL_VALID: src/ui/identity/contacts.rs:317-332 read directly at 0fcb6e7: two sequential action |= single-task assignments still present verbatim; file is absent from the 108af44..0fcb6e7 diff stat (only migration/mod.rs and the new test file changed).
• prior-2 — STILL_VALID: src/ui/identity/hub_screen.rs untouched by the delta; DashPayContactRequests/DashPayContactsWithInfo still recorded unconditionally with no identity guard.
• prior-3 — STILL_VALID: src/backend_task/migration/finish_unwire.rs non-test code untouched by the delta (only the new #[cfg(test)] mod v093_upgrade registration was added to mod.rs, not to this file); top-up failure paths and unconditional sentinel write unchanged.
• prior-4 — STILL_VALID: src/backend_task/dashpay/contact_requests.rs untouched by the delta; mark_withdrawn still returns () and swallows persistence failures.
• prior-5 — STILL_VALID: src/database/legacy_import.rs untouched by the delta; row.get(...)? chain at 186-192 confirmed unchanged by direct read.
• prior-6 — STILL_VALID: src/backend_task/dashpay/contact_requests.rs untouched by the delta; undirected marker logic unchanged.
• prior-7 — STILL_VALID: src/context/contract_token_db.rs untouched by the delta; unsynchronized read-modify-write unchanged.
• prior-8 — STILL_VALID: src/backend_task/migration/finish_unwire.rs non-test code untouched; sentinel-gated default-outcome short-circuit unchanged.
• prior-9 — STILL_VALID: src/ui/identity/contacts.rs untouched by the delta; request_row still dispatches with no in-flight guard.
• prior-10 — STILL_VALID: src/ui/identity/contacts.rs untouched by the delta; unhide_task still passes accepted_accounts: Vec::new().

Carried-forward prior findings

All ten prior findings remain valid at this head. Their existing inline threads are retained; this review does not create duplicate inline comments or emit resolution replies.

New findings in the latest delta

• SUGGESTION src/backend_task/migration/v093_upgrade.rs:673-754 — Second-launch test never performs a second boot: Verified directly against source. boot() (v093_upgrade.rs:367-405) is a real cold-boot helper: it opens a fresh Database::new(&db_file), a fresh AppContext::open_app_kv(dir), a fresh AppContext::open_secret_store(dir), and constructs a new AppContext::new(...) from disk. second_launch_after_a_v093_upgrade_changes_nothing (673-754) calls boot() and wire_backend() exactly once at lines 678-679, runs the first migration, then for the claimed 'second launch' at line 702 only re-invokes import_legacy_settings(&app_kv, &ctx.db) and finish_unwire::run(&ctx) against that same live ctx/app_kv/backend — it never calls boot() or wire_backend() again to drop and reopen the Database, DetKv, SecretStore, AppContext, or WalletBackend. The is_wallet_registered assertion at 734-736 therefore observes the backend already registered by the first migration pass, not a freshly reconstructed one; it would still pass even if a real process restart could not rehydrate the persisted wallet from disk. This tests migration-function idempotency-within-a-process, not the cold-start hydration the test's name and doc comment ('the second launch after an upgrade') claim to lock. Reuse boot()/wire_backend() a second time against the same tmp.path() to construct a genuinely fresh context/backend for the second-launch assertions.

The latest-delta suggestion is verified and persisted, but the ten mandatory STILL_VALID carry-forwards fill the configured ten-finding budget.

[thepastaclaw]

Preliminary review — Codex only

At exact head 1d61b22, all ten prior implementation findings are fixed, while latest-1 remains valid because its second-launch test still reuses the first boot's live context and backend. Nine in-scope findings remain: seven blockers involving wrong-network startup, permanent migration data loss, destructive contact updates, and unreliable contact or shielded-address actions, plus two test-coverage suggestions. This is a preliminary Codex-only checkpoint; no Sonnet evidence was used.

Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — rust-quality (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 7 blocking | 🟡 2 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/app.rs`:
- [BLOCKING] src/app.rs:575-593: Settings-import errors can still start an upgrader on Mainnet
  The new import exists specifically to prevent a Testnet user from being relaunched on Mainnet, but any import error is logged and boot continues. On an upgrade with no app-settings blob, the following read selects `AppSettings::default()`, whose network is Mainnet, with no visible warning. Leaving the sentinel unwritten only protects a future launch; it does not make the current wrong-network session safe. Propagate the boot error or carry an actionable warning into the UI without starting on a guessed network.

In `src/database/legacy_import.rs`:
- [BLOCKING] src/database/legacy_import.rs:114-118: An invalid legacy network is permanently imported as Mainnet
  `read_app_settings` begins with Mainnet defaults and changes the network only when the present column is both text and recognized. A wrong SQLite type or unknown network string therefore returns a successful settings object containing Mainnet. `import_legacy_settings` persists that object and writes its one-shot sentinel, permanently converting a corrupt preference into an apparently successful network reset. A missing column may use a default, but an invalid present network must produce a typed failure or durable warning before the sentinel is written.
- [BLOCKING] src/database/legacy_import.rs:287-306: Unreadable top-up rows are finalized without a durable warning
  `read_top_ups` logs and skips column-conversion failures and non-32-byte identity IDs, but its return type carries no unreadable-row count. The caller consequently treats the import as complete and writes the app-data sentinel, so later launches never retry or warn that part of the user's top-up audit history was omitted. This also contradicts the PR description's claim that individually unreadable legacy rows receive durable warning handling. Return decoded histories together with an unreadable count and persist an acknowledgement-backed warning before recording completion.

In `src/backend_task/dashpay/contact_requests.rs`:
- [BLOCKING] src/backend_task/dashpay/contact_requests.rs:873-885: Decline reports success without storing its resolution marker
  The local declined marker is the only state `load_contact_requests` uses to retire the immutable incoming request, but an unavailable wallet backend or `dashpay_mark_declined` failure is discarded here. The task still returns `DashPayContactRequestRejected`, causing the UI to remove the row and announce success even though the request reappears after refresh and can be declined again. Require the backend and propagate the marker-write error before returning success, matching the corrected withdrawal path.

In `src/ui/identity/hub_screen.rs`:
- [BLOCKING] src/ui/identity/hub_screen.rs:424-428: Unrelated backend errors release every contact-action guard
  `AppState` sends every unhandled `TaskError` to the currently visible screen without identifying its originating task. An error from a profile load, contact refresh, or another background operation therefore clears all Accept, Decline, and Cancel guards while those paid transitions may still be running. The same request becomes clickable and can be submitted a second time before its original result arrives. Error results need enough task or request identity to release only the guard belonging to the failed contact action.

In `src/backend_task/dashpay/contact_info.rs`:
- [BLOCKING] src/backend_task/dashpay/contact_info.rs:175-192: Preserve becomes replacement with an empty account list on decode failure
  `AcceptedAccounts::Preserve` collapses a missing `privateData` field, decryption failure, and deserialization failure into an empty vector. `create_or_update_contact_info` then encrypts that vector and replaces the existing Platform document, so an alias edit or unhide can permanently erase the accepted-account allow-list when the stored payload is corrupt, keyed unexpectedly, or uses an unknown format. Only the absence of an existing document can safely resolve to an empty list; failure to read an existing payload must abort the update.

In `src/ui/wallets/shielded_tab.rs`:
- [BLOCKING] src/ui/wallets/shielded_tab.rs:258-287: Clipboard failure leaves no usable shielded receive address
  The rendered label contains only `truncate_address(&address, 20, 12)` and is configured as a clickable label, while the full address exists only in the clipboard payload and hover text. When clipboard access fails, the error tells the user to select and copy the address manually, but the visible text is ellipsized and the tooltip is not selectable. The restored receive flow is therefore unusable on systems where the clipboard helper fails. Render the complete address in a selectable, wrapping or horizontally scrollable control.

In `src/backend_task/migration/v093_upgrade.rs`:
- [SUGGESTION] src/backend_task/migration/v093_upgrade.rs:674-754: Second-launch test still never performs a second boot
  The test calls `boot()` and `wire_backend()` only once. Its supposed second launch invokes the migration functions again against the same live `AppContext`, database handle, app k/v handle, and already-registered `WalletBackend`; the wallet assertion therefore observes first-launch in-memory state. A cold-start hydration regression would still pass. Shut down and drop the first boot, reopen the same temporary directory into a fresh context and backend, and perform the second-launch assertions there.

In `src/ui/wallets/wallets_screen/mod.rs`:
- [SUGGESTION] src/ui/wallets/wallets_screen/mod.rs:3445-3475: Wallet-pill test manually performs the wiring it claims to protect
  The regression test directly calls both `apply_global_nav_effect` and `screen.apply_nav_effect`; it never exercises the production top-panel result handling at lines 2505-2511. Removing `self.apply_nav_effect(effect)` from the real `ui()` path would leave the page on the stale wallet while this test continued to pass. Drive a pill click through the UI harness, or extract the complete top-panel-effect-to-page-cache composition into one seam shared by production and the test.

[thepastaclaw]

🔴 Blocking: An invalid legacy network is permanently imported as Mainnet

read_app_settings begins with Mainnet defaults and changes the network only when the present column is both text and recognized. A wrong SQLite type or unknown network string therefore returns a successful settings object containing Mainnet. import_legacy_settings persists that object and writes its one-shot sentinel, permanently converting a corrupt preference into an apparently successful network reset. A missing column may use a default, but an invalid present network must produce a typed failure or durable warning before the sentinel is written.

_{source: ['codex']}

[thepastaclaw]

Resolved in c61da82 — An invalid legacy network is permanently imported as Mainnet no longer present.

_{Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.}

[thepastaclaw]

🔴 Blocking: Unreadable top-up rows are finalized without a durable warning

read_top_ups logs and skips column-conversion failures and non-32-byte identity IDs, but its return type carries no unreadable-row count. The caller consequently treats the import as complete and writes the app-data sentinel, so later launches never retry or warn that part of the user's top-up audit history was omitted. This also contradicts the PR description's claim that individually unreadable legacy rows receive durable warning handling. Return decoded histories together with an unreadable count and persist an acknowledgement-backed warning before recording completion.

_{source: ['codex']}

[thepastaclaw]

Resolved in c61da82 — Unreadable top-up rows are finalized without a durable warning no longer present.

_{Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.}

[thepastaclaw]

🔴 Blocking: Decline reports success without storing its resolution marker

The local declined marker is the only state load_contact_requests uses to retire the immutable incoming request, but an unavailable wallet backend or dashpay_mark_declined failure is discarded here. The task still returns DashPayContactRequestRejected, causing the UI to remove the row and announce success even though the request reappears after refresh and can be declined again. Require the backend and propagate the marker-write error before returning success, matching the corrected withdrawal path.

_{source: ['codex']}

[thepastaclaw]

Resolved in 27f5291 — Decline reports success without storing its resolution marker no longer present.

_{Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.}

[thepastaclaw]

🔴 Blocking: Unrelated backend errors release every contact-action guard

AppState sends every unhandled TaskError to the currently visible screen without identifying its originating task. An error from a profile load, contact refresh, or another background operation therefore clears all Accept, Decline, and Cancel guards while those paid transitions may still be running. The same request becomes clickable and can be submitted a second time before its original result arrives. Error results need enough task or request identity to release only the guard belonging to the failed contact action.

_{source: ['codex']}

[thepastaclaw]

Resolved in 27f5291 — Unrelated backend errors release every contact-action guard no longer present.

_{Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.}

[thepastaclaw]

🔴 Blocking: Preserve becomes replacement with an empty account list on decode failure

AcceptedAccounts::Preserve collapses a missing privateData field, decryption failure, and deserialization failure into an empty vector. create_or_update_contact_info then encrypts that vector and replaces the existing Platform document, so an alias edit or unhide can permanently erase the accepted-account allow-list when the stored payload is corrupt, keyed unexpectedly, or uses an unknown format. Only the absence of an existing document can safely resolve to an empty list; failure to read an existing payload must abort the update.

_{source: ['codex']}

[thepastaclaw]

Resolved in ff05c3a — Preserve becomes replacement with an empty account list on decode failure no longer present.

_{Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.}

[thepastaclaw]

🔴 Blocking: Clipboard failure leaves no usable shielded receive address

The rendered label contains only truncate_address(&address, 20, 12) and is configured as a clickable label, while the full address exists only in the clipboard payload and hover text. When clipboard access fails, the error tells the user to select and copy the address manually, but the visible text is ellipsized and the tooltip is not selectable. The restored receive flow is therefore unusable on systems where the clipboard helper fails. Render the complete address in a selectable, wrapping or horizontally scrollable control.

_{source: ['codex']}

[thepastaclaw]

Resolved in b94a868 — Clipboard failure leaves no usable shielded receive address no longer present.

_{Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.}

[thepastaclaw]

🟡 Suggestion: Second-launch test still never performs a second boot

The test calls boot() and wire_backend() only once. Its supposed second launch invokes the migration functions again against the same live AppContext, database handle, app k/v handle, and already-registered WalletBackend; the wallet assertion therefore observes first-launch in-memory state. A cold-start hydration regression would still pass. Shut down and drop the first boot, reopen the same temporary directory into a fresh context and backend, and perform the second-launch assertions there.

_{source: ['codex']}

[thepastaclaw]

Resolved in f6d89a2 — Second-launch test still never performs a second boot no longer present.

_{Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.}

[thepastaclaw]

🟡 Suggestion: Wallet-pill test manually performs the wiring it claims to protect

The regression test directly calls both apply_global_nav_effect and screen.apply_nav_effect; it never exercises the production top-panel result handling at lines 2505-2511. Removing self.apply_nav_effect(effect) from the real ui() path would leave the page on the stale wallet while this test continued to pass. Drive a pill click through the UI harness, or extract the complete top-panel-effect-to-page-cache composition into one seam shared by production and the test.

_{source: ['codex']}

[thepastaclaw]

Resolved in b94a868 — Wallet-pill test manually performs the wiring it claims to protect no longer present.

_{Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.}

[thepastaclaw]

Preliminary review — Codex only

At exact head f6f01db, all ten prior findings are fixed. Six in-scope blocking correctness or data-durability defects remain in the review fixes, along with the carried-forward second-launch test gap, so changes are still required. Focused tests were not rerun because the build required an unavailable Tenderdash dependency fetch.

Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (failed), gpt-5.6-sol — general (failed), gpt-5.6-sol — general (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 6 blocking

2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/ui/identity/hub_screen.rs`:
- [BLOCKING] src/ui/identity/hub_screen.rs:130-132: Fallback active identities discard every contact result
  The Contacts tab dispatches its loads for `resolve_selected_identity()`, which falls back to the first loaded user identity when the raw selection is absent or stale. The result guard instead compares with `selected_identity_id()`. `effective_view` routes a lone identity directly to `HubView::Home` without writing an explicit selection, and backend restoration preserves a stored `None`, so both loads run for the fallback identity but their results are rejected. The common one-identity case therefore leaves the Contacts tab empty. Compare results against the same resolved identity used to dispatch the tasks.

In `src/database/legacy_import.rs`:
- [BLOCKING] src/database/legacy_import.rs:521-539: Malformed top-up rows are permanently dropped without warning
  `read_top_ups` skips malformed columns and non-32-byte identity IDs but returns only successfully decoded histories. Unlike scheduled votes and identities, it neither counts unreadable rows nor creates a durable warning. `migrate_app_data` consequently writes its completion sentinel after the successful partial read, and later launches never examine the skipped legacy rows again. Preserve an unreadable-row count and surface it durably before marking the import complete so users know that part of their top-up audit history was not imported.

In `src/ui/state/contacts_view.rs`:
- [BLOCKING] src/ui/state/contacts_view.rs:75-86: Tab switches clear guards for requests still in flight
  `begin_request` prevents duplicate Accept, Decline, or Cancel transitions only while the request ID remains in `in_flight`, but `reset` clears that set. The Hub calls `reset` on ordinary tab switches without cancelling the backend operation. A user can switch away and back while the first paid transition is slow, reload the still-pending row, and submit it again before the original result arrives. Tab-entry cache resets must preserve live operation guards; release them when the corresponding operation resolves or through a lifecycle mechanism that can prove the task is no longer running.

In `src/backend_task/dashpay/contact_info.rs`:
- [BLOCKING] src/backend_task/dashpay/contact_info.rs:182-192: Preserve mode clears accounts when existing data is unreadable
  `AcceptedAccounts::Preserve` returns an empty list when an existing document lacks `privateData`, decryption fails, or the plaintext uses an unreadable format. `create_or_update_contact_info` then replaces the entire document with that empty list. An unhide or alias edit can therefore destroy the accepted-account allow-list it explicitly promised to preserve. Absence of an existing document can safely resolve to an empty list, but failure to read an existing document must abort with a typed error rather than overwrite unknown data.

In `src/context/identity_db.rs`:
- [BLOCKING] src/context/identity_db.rs:80-91: Top-up merge can lose concurrent history updates
  `save_top_ups_in` performs a whole-map get, merge, and put without synchronizing the sequence. Backend tasks and MCP requests can run concurrently, and the upstream top-up path does not serialize complete top-up operations. Two concurrent top-ups can therefore read the same stored map and later overwrite one another; concurrent MCP calls also derive the same next top-up index from the same identity snapshot, leaving only one amount recorded even if both paid transitions succeed. Serialize the per-identity mutation or store each top-up index under an independent key.

[thepastaclaw]

🔴 Blocking: Fallback active identities discard every contact result

The Contacts tab dispatches its loads for resolve_selected_identity(), which falls back to the first loaded user identity when the raw selection is absent or stale. The result guard instead compares with selected_identity_id(). effective_view routes a lone identity directly to HubView::Home without writing an explicit selection, and backend restoration preserves a stored None, so both loads run for the fallback identity but their results are rejected. The common one-identity case therefore leaves the Contacts tab empty. Compare results against the same resolved identity used to dispatch the tasks.

Suggested change

	fn result_is_for_selected_identity(&self, result_identity: &Identifier) -> bool {
	applies_to_selected_identity(self.app_context.selected_identity_id(), result_identity)
	}
	fn result_is_for_selected_identity(&self, result_identity: &Identifier) -> bool {
	self.app_context
	.resolve_selected_identity()
	.is_some_and(|identity| identity.identity.id() == *result_identity)
	}

_{source: ['codex']}

[thepastaclaw]

🔴 Blocking: Malformed top-up rows are permanently dropped without warning

read_top_ups skips malformed columns and non-32-byte identity IDs but returns only successfully decoded histories. Unlike scheduled votes and identities, it neither counts unreadable rows nor creates a durable warning. migrate_app_data consequently writes its completion sentinel after the successful partial read, and later launches never examine the skipped legacy rows again. Preserve an unreadable-row count and surface it durably before marking the import complete so users know that part of their top-up audit history was not imported.

_{source: ['codex']}

[thepastaclaw]

🔴 Blocking: Tab switches clear guards for requests still in flight

begin_request prevents duplicate Accept, Decline, or Cancel transitions only while the request ID remains in in_flight, but reset clears that set. The Hub calls reset on ordinary tab switches without cancelling the backend operation. A user can switch away and back while the first paid transition is slow, reload the still-pending row, and submit it again before the original result arrives. Tab-entry cache resets must preserve live operation guards; release them when the corresponding operation resolves or through a lifecycle mechanism that can prove the task is no longer running.

_{source: ['codex']}

[thepastaclaw]

🔴 Blocking: Preserve mode clears accounts when existing data is unreadable

AcceptedAccounts::Preserve returns an empty list when an existing document lacks privateData, decryption fails, or the plaintext uses an unreadable format. create_or_update_contact_info then replaces the entire document with that empty list. An unhide or alias edit can therefore destroy the accepted-account allow-list it explicitly promised to preserve. Absence of an existing document can safely resolve to an empty list, but failure to read an existing document must abort with a typed error rather than overwrite unknown data.

_{source: ['codex']}

[thepastaclaw]

🔴 Blocking: Top-up merge can lose concurrent history updates

save_top_ups_in performs a whole-map get, merge, and put without synchronizing the sequence. Backend tasks and MCP requests can run concurrently, and the upstream top-up path does not serialize complete top-up operations. Two concurrent top-ups can therefore read the same stored map and later overwrite one another; concurrent MCP calls also derive the same next top-up index from the same identity snapshot, leaving only one amount recorded even if both paid transitions succeed. Serialize the per-identity mutation or store each top-up index under an independent key.

_{source: ['codex']}