fix(platform-wallet-storage): reject duplicate (wallet_id, identity_index) on write

[Claudius-Maginificent]

TL;DR

Adds a write-path guard in rs-platform-wallet-storage that rejects a second identity claiming
an already-occupied (wallet_id, identity_index) slot — at store() time and again as a
flush-time backstop — with no schema change and no migration.

User story

As an operator of a wallet built on rs-platform-wallet-storage, I need (wallet_id, identity_index) to always identify exactly one identity, because that pair drives HD
derivation-path lookups: a silent duplicate would let two different identities resolve to the
same derived key material.

Scenario

1. Wallet W already has identity A stored at identity_index = 3.
2. Something (buggy caller, race, replayed write) attempts to store() identity B also at
   identity_index = 3 for wallet W.
3. Before this PR: the write silently succeeds; W now has two identities claiming index 3,
   and downstream derivation-path logic has no way to know which one is authoritative.
4. After this PR: store() returns WalletStorageError::IdentityIndexConflict immediately.
   The same probe also runs as a backstop inside the flush transaction, closing the
   FlushMode::Manual window where two individually-valid store() calls could still merge into
   an on-disk duplicate before either lands.

Detailed discussion

Design. Implements the locked design from memcan TODO 4fbe235c-1bcd-4a32-baea-5b4f9d67e526
(R1–R15, domain invariants I1–I7). Deliberately no defence-in-depth — the probe is the only
enforcement, no schema constraint, no migration — so every writer of identities rows had to be
re-derived and checked by hand, not just the obvious store() path.

Core pieces:

• check_index_conflicts(&Connection/&Transaction, &WalletId, &IdentityChangeSet) — callable
  against either a bare connection or a transaction, so the same probe covers both the store-time
  check and the flush-time backstop.
• Two new WalletStorageError variants, IdentityIndexConflict and WalletlessIdentityIndex
  (both non-transient, Constraint-kind), with explicit arms in every wildcard-free match table
  in the crate.
• Buffer::store_checked(wallet_id, cs, check) — runs the probe and the merge as one critical
  section under the buffer lock.
• SqlitePersister::store() builds a merged view (buffered ∪ incoming, via the real Merge
  impl) and checks that before accepting a write.
• flush_inner() now holds the connection across take-for-flush and the write itself, closing
  the drain-to-commit window.

A race the fix itself introduced, and how it was closed. The first version of the store-time
probe ran under the connection lock while the buffer merge ran under a separate lock — two
threads racing store() on the same wallet with conflicting indices could both pass the
disk-state probe, both merge into one contradictory buffered changeset, and have the flush-time
backstop's fatal-drop path silently discard whichever caller's entry didn't "win," even though
that caller had already received Ok(()). Caught by adversarial QA
(tests/sqlite_identity_index_concurrency.rs, ~1-in-several-hundred repro window) and closed by
making probe+merge share one critical section per wallet (above), verified both by construction
(lock order re-derived independently twice: conn → buffer, one direction, Buffer never
reaches for conn) and by stress testing (500+ iterations, three-way races, store-vs-delete
races).

delete_wallet's error-tolerance carve-out. The flush-time backstop's fatal-drop path meant
a wallet with a contradictory buffered changeset could become permanently undeletable. The fix
added a carve-out — originally matching the whole Constraint error-kind bucket, which QA showed
would silently swallow unrelated corruption (any FK/CHECK/UNIQUE/NOT-NULL violation) at delete
time too. Narrowed to match exactly the two new variants.

QA. Two full adversarial passes plus a focused re-verification pass on the new locking
machinery specifically, all independently ledger-verified (test logs, clippy, git diff-tree
scope checks) rather than trusted from agent reports. Zero outstanding findings.

Out of scope, logged separately:IdentityChangeSet::merge's and_modify branch (in the
sibling rs-platform-wallet crate) doesn't copy identity_index on a reindex-while-buffered
merge — confirmed real, confirmed not to create a false negative in this guard, filed as memcan
TODO e97aa67c-7ee0-4d41-a7bb-f4e39c695164 (low priority) rather than fixed inline, to keep this
PR's diff scoped to rs-platform-wallet-storage.

Testing:cargo test -p platform-wallet-storage (new tests in
sqlite_identity_index_uniqueness.rs, sqlite_identity_index_concurrency.rs,
sqlite_delete_wallet_constraint_carveout.rs, plus updates to the two wildcard-free
error-classification test files), cargo clippy -p platform-wallet-storage --all-targets,
cargo fmt.

[coderabbitai]

Important

Review skipped

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

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 Plus

Run ID: c0013e9c-d2a3-4dfa-8c84-f8fb6c430bce

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

---

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 — Opus deferred (commit c0264e4)
Canonical validated blockers: 2

[thepastaclaw]

Preliminary review — Codex only

The write guard correctly rejects straightforward duplicate identity slots and closes the original in-process check-to-merge race. However, the delete carve-out can lose buffered writes after a failed deletion, and Immediate mode still allows another flush to discard an accepted identity changeset without reporting the failure to its originating store call; atomic reindexing is also rejected despite having a unique final state. Source: Codex general, Rust-quality, and security reviewer evidence (exact reviewer backend model IDs were not present in the supplied projection); final verifier backend: cliproxy/gpt-5.6-sol; openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus 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), gpt-5.6-sol — security-auditor (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 2 blocking | 🟡 2 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 `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:743-754: Retain the drained changeset until deletion commits
The identity-conflict carve-out consumes `cs` and leaves `drained_slot` empty before the wallet has been deleted. If a later operation fails—such as automatic backup creation, opening or executing the delete transaction, or committing it—the outer `restore_buffer` call has nothing to restore. The wallet remains, but the previously accepted Manual-mode changeset, including any unrelated valid sub-changesets bundled with the conflicting identity write, is permanently lost. Put `cs` back into `drained_slot` after rolling back the pre-flush transaction; the existing `drop(drained_slot.take())` after the delete commit will still discard it on the successful path.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:1169-1185: Concurrent flush can hide rejection from the identity writer
Immediate-mode `store()` releases the connection after `store_checked()` and reacquires it only in `flush_inner()`. During that gap, another thread can drain the accepted changeset. For example, process A checks a free slot and buffers identity A; process B independently commits identity B to that slot; then another thread in process A calls `flush(wallet_id)`, drains A, detects B at the transactional backstop, and fatally drops A. When A's original store call resumes, its own `flush_inner()` sees an empty buffer and returns `Ok(())`. The caller is therefore told that A is durable even though its mapping was discarded, contradicting the documented Immediate-mode guarantee and allowing it to continue using ambiguous HD-derived key material. Serialize the checked merge and its Immediate-mode flush as one operation—by retaining exclusive ownership of the connection/write sequence or using an equivalent per-wallet operation lock—so no other flush can consume the changeset before its originating `store()` receives the result.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:178-196: Treat identities reindexed by the same changeset as vacating their old slots
The disk occupant is treated as vacating a slot only when it appears in `cs.removed`, even though `apply()` also updates `identity_index` for entries in `cs.identities`. A changeset such as `{A -> 2, B -> 1}` is therefore rejected when A currently occupies index 1, despite its final state being unique; a two-way swap is rejected for the same reason. Evaluate every on-disk occupant and ignore one when the changeset removes it or assigns it a different final index. Iterating all matching rows also preserves the intended behavior for legacy databases that may already contain more than one occupant.
In `packages/rs-platform-wallet-storage/tests/sqlite_identity_index_concurrency.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/tests/sqlite_identity_index_concurrency.rs:95-138: Replace the scheduling stress loop with a deterministic interleaving test
The barrier aligns the two store calls only at their start; it does not force either thread into the former probe-to-merge window. A regression can therefore pass all 500 attempts depending on scheduling, while each attempt creates a database and six threads and four threads continuously contend on the connection mutex. Add a test-only synchronization point that pauses the first caller while it holds the relevant critical section, starts the second caller, and then releases the first. That would exercise the ordering guarantee deterministically with substantially less timing-dependent CI load.