feat(wallet): unify wallet secret storage on one vault seam with per-secret at-rest encryption

[lklimek]

Why this PR exists

• Problem: Wallet secrets were exposed at rest. Identity private keys were written plaintext into det-app.sqlite; ClosedSingleKey's derived Debug could print raw key bytes; no-password key material sat in non-zeroized memory. The storage layer freely serialized plaintext secrets through serde.
• What breaks without it: A host-local attacker (another account, malware with file-read, a leaked backup, swap/heap forensics) reading det-app.sqlite recovers identity private keys verbatim — full control of the on-chain identity — with no user mitigation available (PrivateKeyData::Encrypted had no constructor).
• Blocking relationship: Stacked on feat: rewrite Dash Evo Tool onto the new platform-wallet #860 (docs/platform-wallet-migration-design). It depends on that branch's SecretStore / SecretAccess / wallet_backend engine, so the base is set accordingly. Merge after (or onto) feat: rewrite Dash Evo Tool onto the new platform-wallet #860.

What was done

• One secret seam (src/wallet_backend/secret_seam.rs): all three secret classes — HD seed, imported single key, identity private key — store raw bytes through a single put/get/delete_secret chokepoint into the upstream SecretStore vault. No DET-side serialization of plaintext.
• No-serialization invariant, compiler-enforced: SecretBytes has no Serialize, so no persisted struct can embed a plaintext secret. Guarded by compile_fail doctests + a source-audit test.
• Per-secret at-rest encryption (Tier-2): password-protected secrets are sealed with the upstream per-secret envelope — Argon2id KDF + XChaCha20-Poly1305 AEAD, a fresh per-object salt, and AAD bound to wallet_id‖label. Same password on two secrets derives different keys; a copied envelope fails the tag under a different scope. DET's own per-wallet AES-GCM envelopes are dropped (retained decode-only as legacy migration readers).
• Per-secret passwords: each encrypted secret is prompted and (optionally) remembered independently. The just-in-time SecretAccess cache and prompt are keyed by SecretScope (HD seed_hash / single-key address / identity label), so one secret's remembered password can never satisfy another. Remember is opt-in / off by default.
• Identity keys never resident: PrivateKeyData::InVault placeholder; fetched per-signature through the async Signer → resolve_private_key_bytes → SecretAccess::with_secret(IdentityKey) chokepoint and zeroized on scope exit.
• Optional per-identity identity-key encryption (SEC-001): identity keys default to the keyless vault (prompt-free, so headless/MCP signing keeps working), but a user may opt in per identity to seal that identity's keys Tier-2 under a separate per-identity password. The opt-in unit is the at-rest scheme — scope_has_passphrase(IdentityKey) probes the seam scheme() rather than a parallel flag, so the prompt gate cannot drift from the stored state. A fail-closed guard makes a keyless write of a new key onto a protected identity impossible (it is sealed Tier-2 or refused, never plaintext), and adding a key to a protected identity verifies the password before the on-chain broadcast — so a headless add fails closed with no orphaned on-chain key. Opt-in/out lives in a collapsible "Key Protection" section on the key screen (default closed, off the everyday path).
• Crash-safe keep-protection migration: no-password seeds/keys migrate eagerly to raw on load; password-protected wallets migrate lazily at the existing unlock dialog (one prompt, no second dialog) by re-sealing under the same password as Tier-2 — protection is kept, never downgraded to raw. Order is always vault-write before legacy delete, with a best-effort GC of any orphaned legacy envelope; a secret in neither form raises a loud typed error — never silent key loss.
• Folded fixes: ClosedSingleKey redacting Debug; no-password material now lives in zeroizing SecretBytes.

Residual (accepted, by design)

At-rest confidentiality applies to password-protected secrets (the Tier-2 envelope). The vault file is opened keyless (file_unprotected), which upstream documents as obfuscation, not confidentiality. So no-password wallet secrets and migrated raw single keys rest on filesystem permissions (0600) plus obfuscation. Identity private keys now have optional per-identity Tier-2 encryption (SEC-001) for users who want real at-rest confidentiality; they remain keyless by default so headless/MCP signing stays prompt-free. The remaining keyless tier (no-password wallet / single-key material) still awaits a host-held vault passphrase or OS-keyring backend. Recorded in the threat model under docs/ai-design/2026-06-19-secret-storage-seam/.

Testing

• cargo +nightly fmt --all clean; cargo clippy --all-features --all-targets -- -D warnings clean.
• cargo test --all-features --workspace: full offline suite (lib + kittest + e2e) green, 0 failed; compile-fail invariant doctests pass.
• det-cli standalone smoke green.
• Seam coverage: no-serialization invariant guard; raw round-trip per class; Tier-2 round-trip (get(None)→NeedsPassword, get_secret(Some(pw))→bytes); per-secret isolation; lazy keep-protection migration (legacy AES-GCM → Tier-2 re-seal, legacy entry GC'd); wrong-password re-ask; eager unprotected migration; write-fault no-loss ordering; legacy-format read; headless split; identity residency; on-disk no-leak; identity-key delete.
• SEC-001 coverage: opt-in seals Tier-2 (scheme→Protected, password-free read fails); sign-time prompt; headless opted-in identity → SecretPromptUnavailable; downgrade guard refuses a Tier-1 write of a new key onto a protected identity; add-key to a protected identity fails closed before the on-chain broadcast; opt-out reverts to keyless; per-identity password isolation; non-opted-in identities byte-for-byte unchanged.
• Independent security audits: SHIP — the seam/Tier-2 feature, and a second adversarial pass on the SEC-001 new-key / pre-broadcast hardening, both cleared with 0 CRITICAL/HIGH/MEDIUM blockers and all funds-safety invariants verified.

Breaking changes

• Wallet DB must be dropped and re-migrated. The feat: rewrite Dash Evo Tool onto the new platform-wallet #860 base bump rewrites migration V001 in place (drops core_derived_addresses + account_address_pools); existing spv//platform-wallet.sqlite databases are not forward-compatible.
• On-disk wallets and identities migrate transparently via dual-format readers. Password-protected wallets keep prompting for their password (just-in-time, optional remember) — protection is preserved, not removed.
• Identity-key encryption (SEC-001) is opt-in and per-identity — no forced migration; existing identities stay keyless until a user opts in.

Checklist

• fmt + clippy clean
• tests green (offline suite)
• design + test spec committed (docs/ai-design/2026-06-19-secret-storage-seam/)
• independent security audits (SHIP — seam/Tier-2 + SEC-001 hardening)
• live testnet e2e — the migrated-InVault sign path and the SEC-001 interactive on-chain add-key-to-protected-identity leg are unit-covered up to the seam; the on-chain round-trips are ready but blocked on the e2e funding-wallet rehydration gap (not the secret-handling path)

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: e0d022da-1fa2-4c8d-87a8-245941184c8a

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 security/secret-handling-hardening

---

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.}

[lklimek]

TO DO: re-introduce password-protected secrets, this time based on platform-wallet-storage secret store. Waiting for upstream (platform-wallet-storage secret store) to deliver per-secret encryption.

[Copilot]

Pull request overview

This PR strengthens wallet secret hygiene by consolidating secret storage behind a vault-backed seam (raw bytes, no serde-serialization of plaintext) and adds regression coverage for the interim at-rest disclosure as well as a live (ignored) end-to-end test proving InVault identity signing/broadcast.

Changes:

• Add UI kittests asserting the interim at-rest disclosure banner renders correctly and remains jargon-free.
• Add a live-network (ignored) backend E2E that migrates an identity key to InVault and proves JIT vault fetch signing works end-to-end.
• Update wallet backend glue (hydration/gating) to support the new storage approach and lifecycle behavior.

Reviewed changes

Copilot reviewed 37 out of 38 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/kittest/main.rs	Registers the new disclosure banner kittests module.
tests/kittest/disclosure_banner.rs	Adds kittest coverage for disclosure banner copy/type/icon and jargon-free constraints.
tests/backend-e2e/main.rs	Registers the new identity_in_vault_sign backend E2E module.
tests/backend-e2e/identity_in_vault_sign.rs	Adds a live-network ignored E2E proving InVault identity signing/broadcast path.
src/wallet_backend/hydration.rs	Prefers raw-seam seeds and performs eager migration for legacy no-password envelopes.
src/wallet_backend/coordinator_gate.rs	Adjusts coordinator gating logic for restart-in-place behavior.
src/backend_task/wallet/derive_identity_key_for_display.rs	Implements vault-backed identity-key display derivation (WIF) for the UI path.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[lklimek]

Out of scope for this PR (the wallet secret-storage seam), and the thread is marked outdated — the referenced hunk has since changed. Leaving it open for separate assessment rather than resolving it here.

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

[lklimek]

Acknowledged but deferred. The open WalletSeed / OpenWalletSeed model intentionally holds the plaintext seed and is shared with the create/import path, so switching this site to a JIT-only fetch is a model-wide change beyond this PR's risk budget. Note the related eager-migration seed copy was wrapped in Zeroizing (commit 564fe7dc). Leaving this thread open to track the larger refactor rather than resolving it.

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

[thepastaclaw]

✅ Review complete (commit 2f40b30)

[thepastaclaw]

Code Review

Two convergent blocking findings hold up: identity write paths serialize PrivateKeyData::Clear/AlwaysClear straight into the meta_identity blob (the new migration only runs on bulk load, so every newly added identity key lands plaintext at rest until reload), and any imported single key that has been Tier-2 re-wrapped vanishes from the wallet picker on cold boot because SingleKeyView::rebuild_wallet does a password-free SecretStore::get and turns NeedsPassword into a fatal per-entry error. The rest of the PR (HD seed Tier-2 keep-protection, secret seam, tests) is well-structured; remaining items are small consistency/hygiene/doc nits.

🔴 2 blocking | 🟡 5 suggestion(s) | 💬 3 nitpick(s)

4 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/context/identity_db.rs`:
- [BLOCKING] src/context/identity_db.rs:398-458: Identity write paths still serialize plaintext private keys into the blob
  `insert_local_qualified_identity` (line 415) and `update_local_qualified_identity` (line 446) both feed `qualified_identity.to_bytes()` directly into the k/v store without first routing the keystore through `migrate_keystore_to_vault` (or any equivalent `take_plaintext_for_vault` step). `PrivateKeyData` derives bincode `Encode`, and `KeyStorage::insert_non_encrypted` (encrypted_key_storage.rs:516-528) writes `PrivateKeyData::Clear` / `PrivateKeyData::AlwaysClear` with the raw `[u8; 32]` payload — both variants get serialized verbatim. Every new-key path that calls these persist methods therefore lands plaintext private keys in `det-app.sqlite` until the next bulk load happens to run `migrate_identity_keys_to_vault` (only invoked from `load_identities_filtered`, line 571). Concrete callers in this PR: `backend_task/identity/add_key_to_identity.rs:45,124` (`insert_non_encrypted` → `update_local_qualified_identity`) and `ui/identities/keys/key_info_screen.rs:691-698`. This breaks the headline invariant the PR is set up to deliver — *identity private keys never plaintext at rest* — for exactly the key class the PR set out to protect. The fix is to run a vault-first migration (or take_plaintext_for_vault + IdentityKeyView::store_all) inside the write paths so the blob persisted to disk never carries `Clear`/`AlwaysClear` payloads.
- [SUGGESTION] src/context/identity_db.rs:617-639: `get_identity_by_id` skips the vault migration that `load_identities_filtered` runs
  `load_identities_filtered` calls `self.migrate_identity_keys_to_vault(...)` (line 571) for every identity it decodes, so cold-boot bulk loads migrate any residual `Clear`/`AlwaysClear` to the vault. `get_identity_by_id` (line 617-639) goes through the same decode pipeline but does not invoke the migration — it only calls `hydrate_top_ups`. Several production paths use this single-get lookup before signing or updating (token, identity, masternode, send, and MCP/CLI resolve), so for a legacy blob those callers receive resident `PrivateKeyData::Clear` bytes and can re-persist them through `update_local_qualified_identity`, prolonging the residency window even after a bulk load would have cleaned them up. Once the write-path fix above is in place this becomes the remaining residency gap on the read side — mirror the migration call here so the typed guarantee holds uniformly across all identity read APIs.
- [SUGGESTION] src/context/identity_db.rs:256-295: `migrate_keystore_to_vault` clones the entire `KeyStorage` on every identity load
  `let before = qi.private_keys.clone();` runs unconditionally at the top of the migration helper (line 262), even though the steady-state case post-first-boot is `KeystoreMigration::Nothing` and the clone is only used on the failure-restore path. `take_plaintext_for_vault` already iterates and short-circuits cleanly, so the clone could be deferred (call `take_plaintext_for_vault` first, only snapshot when `taken` is non-empty, and restore from `taken` itself on vault-write failure). `load_identities_filtered` calls this once per identity at cold start, so the cost scales linearly with identity count for no benefit.

In `src/wallet_backend/single_key.rs`:
- [BLOCKING] src/wallet_backend/single_key.rs:578-608: Tier-2-migrated imported single keys disappear from the wallet picker on cold boot
  `rebuild_wallet` reads the vault row with `self.secret_store.get(&single_key_namespace_id(), &label)` (line 580-585) and then decodes the bytes as a legacy `SingleKeyEntry`. After a passphrase-protected imported key is first used through the chokepoint, `verify_passphrase` (single_key.rs:383-395) and `SecretAccess::migrate_single_key_to_tier2` (secret_access.rs:716-735) overwrite the same label with a Tier-2 object-password envelope via `set_secret(..., Some(&pw))` / `put_secret_protected`. From that point on the upstream backend reports the label as `SecretStoreError::NeedsPassword` for any password-less `get` — exactly what `secret_seam.rs::scheme()` relies on (line 164-169). On the next cold boot `rebuild_wallet` propagates that error through `?` as `TaskError::SecretStore`, and `hydrate_wallets` (line 521-533) catches it as `Err`, logs and skips. The entry therefore never makes it into `ctx.single_key_wallets`, and `WalletsScreen` no longer lists the passphrase-protected imported key even though its sidecar metadata (`ImportedKey.public_key_bytes`, alias, address) is intact and the key is still recoverable. The rebuild path needs to branch on `SecretSeam::scheme()` (or treat `NeedsPassword` as a non-fatal `Protected` outcome) and, for the protected case, construct a closed display wallet from the non-secret sidecar — the same shape `rebuild_closed_passphrase_wallet` already produces — instead of decoding a `SingleKeyEntry` that is no longer there.
- [SUGGESTION] src/wallet_backend/single_key.rs:231-257: Fresh protected-WIF imports bypass the Tier-2 chokepoint and write the legacy AES-GCM envelope
  The protected import path builds a `SingleKeyEntry::protected`, bincode-encodes it, and writes it through password-less `SecretStore::set` (line 231-243). That is the retained legacy envelope, not the new per-secret Tier-2 seam (`put_secret_protected` / `set_secret`). Migration to Tier-2 then happens lazily on the first unlock (`verify_passphrase` line 383-395, `migrate_single_key_to_tier2` in secret_access.rs:716-735). The user supplies the password at import time, so the legacy framing serves no purpose — writing via `put_secret_protected` straight away makes the storage chokepoint a single shape from import onward, and the existing test `sec_002_import_with_passphrase_encrypts_payload` would tighten to asserting `SecretScheme::Protected` instead of just non-plaintext bytes.

In `src/wallet_backend/secret_access.rs`:
- [SUGGESTION] src/wallet_backend/secret_access.rs:624-634: Legacy-envelope `delete?` in the HD `Absent` branch can fail a successful unlock
  `decrypt_jit`'s HD `Absent` branch propagates the legacy-envelope cleanup via `view.delete(seed_hash)?;` (line 633). The peer Tier-2 branch a few lines above (line 606-612) is intentionally log-and-swallow with the rationale 'idempotent + best-effort, and a stale envelope will be GC'd on the next read.' The migration block itself even says *'Crash-safe: the re-store (upsert) precedes the delete… a crash between leaves both forms and the next read takes the new one.'* Propagating the delete error breaks that contract: a transient sqlite hiccup turns a successful seed unlock + re-wrap into a user-visible failure, even though the re-store already succeeded and the scheme probe will prefer the new label on the next read. Match the Protected-branch pattern so the GC is best-effort here too.

In `src/wallet_backend/hydration.rs`:
- [SUGGESTION] src/wallet_backend/hydration.rs:128-147: Eager seed migration extracts the 64-byte seed onto a non-zeroizing stack array
  The eager no-password seed migration converts the envelope payload into a bare `[u8; 64]` on the stack (line 130) and passes it into `set_raw`. The array is dropped at end-of-block with no zeroization. Every other secret-handling site introduced by this PR is careful to keep raw seed material wrapped in `Zeroizing` so it wipes on drop (e.g. `decrypt_hd_seed` returns `Zeroizing<[u8; 64]>`, `Plaintext::HdSeed`). Funds-safety is fine — the seed is also held by the `Vec<u8>` envelope, which is itself non-zeroizing (pre-existing) — but this widens the secret-residency window the rest of the rework is consciously consolidating. Wrap the local in `Zeroizing` for consistency.

[lklimek]

🟠 [MEDIUM] SEC-001: Identity private keys are stored keyless (obfuscation-only) with no Tier-2 path

Every identity private key is written as raw 32 bytes through the seam into the keyless file_unprotected vault, and the chokepoint hard-codes identity scopes as never-protected: scope_has_passphrase returns Ok(false) for SecretScope::IdentityKey { .. } unconditionally (src/wallet_backend/secret_access.rs:566-567), and there is no *_protected write path for identity keys at all. The keyless vault is, by upstream's own words quoted in open_secret_store (src/wallet_backend/single_key.rs:813-831), "obfuscation, not confidentiality" — its key derives from an empty passphrase under a public salt, so anyone who can READ the vault file re-derives it. This is done deliberately so headless/MCP identity signing works without a prompt, and it is honestly documented (the ADR under docs/ai-design/2026-06-19-secret-storage-seam/). The finding is the residual itself: the PR's stated win — moving identity keys out of plaintext SQLite so a host-local attacker can no longer recover keys — is not actually achieved for identity keys. After the change a host-local attacker still recovers every identity private key; the only at-rest defence that improved is owner-only file permissions (0600 file / 0700 parent dir) plus non-literal encoding.

secret_access.rs:566-567 — identity keys are never password-gated

// Identity keys are stored raw, unprotected — always prompt-free.
SecretScope::IdentityKey { .. } => Ok(false),

Recommendation. Treat identity-key confidentiality as a first-class option rather than an unconditional keyless store. Concretely: (1) offer an opt-in to seal identity keys under the wallet's existing object password via the same Tier-2 path (set_secret/get_secret), accepting that headless signing of those identities then requires the password; or (2) when a real key holder is available, route identity keys through SecretStore::os (OS keyring) instead of file_unprotected. At minimum, surface the obfuscation-only status to the user in the UI (the open_secret_store doc is developer-facing only) so an Everyday User understands that identity keys are protected by file permissions alone.

Impact. An attacker with read access to the user's data directory (stolen laptop, backup, malware running as the user, a misconfigured sync/cloud-backup folder) can recover every identity private key from the keyless vault and sign Platform state transitions — transfer credits, withdraw, mutate documents — for those identities. The blast radius is the set of identities whose keys are stored locally; the likelihood is gated on local file access (the same threat model the PR was written to address).

_{Tags: A02 Cryptographic Failures, A04 Insecure Design, CWE-312, CWE-522 · full location: src/wallet_backend/identity_key_store.rs:1-13,52-63}

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

[lklimek]

Accepted residual, tracked as follow-up — not addressed in this PR. Identity keys stay keyless (obfuscation-only) by design so headless/MCP identity signing remains prompt-free; an opt-in Tier-2 seal or OS-keyring path for identity keys is a separate change. Leaving open as a known residual.

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

[lklimek]

Substantially addressed by this PR's SEC-001 work. Identity private keys are now optionally encryptable at rest through the same Tier-2 envelope used elsewhere (Argon2id + XChaCha20-Poly1305, per-object salt, AAD-bound). Protection is gated per-identity by scope_has_passphrase(IdentityKey) probing the stored vault scheme — Protected → prompt, Unprotected → prompt-free, Absent → IdentityKeyMissing — so the at-rest scheme is the flag and can't drift from a parallel boolean. The keyless (obfuscation-only) shape remains the default residual tier for identities you haven't opted to protect.

Leaving this thread for you to close — flagging that the optional-protection path requested here now exists.

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

[thepastaclaw]

Code Review

The latest delta (904dc83..83414f6) is a single-file, test-only race fix in src/context/wallet_lifecycle.rs — no production code changed. All ten prior findings from 904dc83 are re-verified STILL VALID at head 83414f6, including the two blocking residency/usability defects (identity write paths still serialize PrivateKeyData::Clear/AlwaysClear plaintext into the meta_identity blob, and Tier-2-rewrapped imported single keys still disappear from the wallet picker on cold boot). Both Claude and Codex agreed unanimously across general + rust-quality lanes; no new findings emerged from the delta. REQUEST_CHANGES remains the recommendation until the two blockers are resolved.

🔴 2 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)

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

6 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/context/identity_db.rs`:
- [BLOCKING] src/context/identity_db.rs:398-458: Identity write paths still serialize plaintext private keys into the blob
  Carried forward from prior review on 904dc830 — re-verified STILL VALID at 83414f65 (latest delta is test-only). `insert_local_qualified_identity` (line 415) and `update_local_qualified_identity` (line 446) both feed `qualified_identity.to_bytes()` directly into `kv.put(...)` without first routing the keystore through `migrate_keystore_to_vault` (or an equivalent `take_plaintext_for_vault` step). `PrivateKeyData` derives bincode `Encode` and `KeyStorage::insert_non_encrypted` (encrypted_key_storage.rs:516-528) writes `PrivateKeyData::Clear` / `PrivateKeyData::AlwaysClear` with the raw `[u8; 32]` payload verbatim. Every new-key path that calls these persist methods therefore lands plaintext private keys in `det-app.sqlite` until the next bulk load happens to run `migrate_identity_keys_to_vault` (only invoked from `load_identities_filtered`, line 571). Concrete callers in this PR: `backend_task/identity/add_key_to_identity.rs:45,124` and `ui/identities/keys/key_info_screen.rs:691-698`. This breaks the headline PR invariant — *identity private keys never plaintext at rest* — for exactly the key class the PR set out to protect. Fix by running a vault-first migration (or `take_plaintext_for_vault` + `IdentityKeyView::store_all`) inside the write paths so the blob persisted to disk never carries `Clear`/`AlwaysClear` payloads.
- [SUGGESTION] src/context/identity_db.rs:617-639: `get_identity_by_id` skips the vault migration that `load_identities_filtered` runs
  Carried forward from prior review on 904dc830 — re-verified STILL VALID at 83414f65. `load_identities_filtered` calls `self.migrate_identity_keys_to_vault(...)` (line 571) for every decoded identity, but `get_identity_by_id` (line 617-639) only calls `hydrate_top_ups` and never invokes the migration. Single-get lookups feed token, identity, masternode, send, and MCP/CLI resolve paths before signing or updating, so a legacy blob gives those callers resident `PrivateKeyData::Clear` bytes that they can re-persist through `update_local_qualified_identity`, prolonging the residency window even after a bulk load would have cleaned them up. Once the write-path fix in finding #1 lands, this is the remaining read-side residency gap — mirror the migration call here so the typed guarantee holds uniformly across all identity read APIs.

In `src/wallet_backend/single_key.rs`:
- [BLOCKING] src/wallet_backend/single_key.rs:578-608: Tier-2-migrated imported single keys disappear from the wallet picker on cold boot
  Carried forward from prior review on 904dc830 — re-verified STILL VALID at 83414f65. `rebuild_wallet` reads the vault row with `self.secret_store.get(&single_key_namespace_id(), &label)` (line 580-585) and only after a successful legacy `SingleKeyEntry::decode` does it branch on `entry.has_passphrase`. After a passphrase-protected imported key is first used through the chokepoint, `verify_passphrase` (line 383-395) and `SecretAccess::migrate_single_key_to_tier2` overwrite the same label with a Tier-2 object-password envelope via `set_secret(..., Some(&pw))` / `put_secret_protected`. From that point the upstream backend returns `SecretStoreError::NeedsPassword` for any password-less `get` — exactly what `secret_seam.rs::scheme()` relies on. The `?` at line 583-585 converts that into `TaskError::SecretStore`; `hydrate_wallets` catches the `Err`, logs and skips. The entry never makes it into `ctx.single_key_wallets`, so `WalletsScreen` no longer lists the passphrase-protected imported key even though its sidecar metadata (`ImportedKey.public_key_bytes`, alias, address) is intact and the key is still recoverable. Branch on `SecretSeam::scheme()` (or treat `NeedsPassword` as a non-fatal `Protected` outcome) and, for the protected case, construct a closed display wallet from the non-secret sidecar — the same shape `rebuild_closed_passphrase_wallet` already produces — instead of decoding a `SingleKeyEntry` that is no longer there.

[lklimek]

@thepastaclaw — both carried-forward blockers are addressed in the current HEAD (0eaa422c). The CHANGES_REQUESTED review (submitted 12:24Z) examined the delta 904dc830..83414f65 (the test-only CI fix) and so did not include the fix commits 564fe7dc / bf435c4d that resolve them:

Blocker 1 — identity write paths serializing plaintext keys. All three write entry points now route the keystore through the vault before producing bytes: insert_local_qualified_identity, update_local_qualified_identity, and set_identity_alias call encode_identity_blob_vault_first (src/context/identity_db.rs:312), which store_alls plaintext keys to the vault and emits InVault placeholders only (fund-safe ordering: vault write before bytes). The one remaining direct to_bytes() in persist_identity_blob is reached only via migrate_keystore_to_vault's persist closure, which passes an already-InVault qi. Covered by the test write_path_encodes_invault_only_and_vaults_plaintext.

Blocker 2 — Tier-2 single keys vanishing on cold boot. rebuild_wallet (src/wallet_backend/single_key.rs:606) now branches on SecretSeam::scheme() before reading; a Protected entry is rebuilt closed via rebuild_closed_tier2_wallet from meta.public_key_bytes, never reading the secret (no fatal NeedsPassword). Covered by the test tier2_protected_single_key_rebuilds_closed_and_is_listed.

Both tests pass in the green Test Suite on 0eaa422c. A re-review of the current HEAD should clear these. Separately, optional at-rest encryption for identity private keys (SEC-001) is now being added within this PR's scope.

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

[thepastaclaw]

Code Review

The delta from 83414f6 to 0eaa422 is a substantive fix push that resolves both prior blockers — identity write paths now route through encode_identity_blob_vault_first (insert/update/alias-edit vault plaintext keys before encoding the persisted blob), and Tier-2-protected single keys now rebuild as closed wallets via SecretSeam::scheme() probing instead of erroring out of the wallet picker. Five further prior suggestions/nitpicks are FIXED (HD Absent best-effort delete, eager-seed Zeroizing wrap, fresh-protected-WIF Tier-2 import, migrate_keystore_to_vault clone short-circuit, WalletMeta::uses_password doc). Three lower-severity prior findings remain STILL VALID and are carried forward: get_identity_by_id still skips load-side identity-key migration, the ImportedKey::has_passphrase doc still misdescribes fresh-import shape (fresh protected imports now write Tier-2 directly, not legacy AES-GCM), and the prompt-meta setters still silently no-op on a poisoned lock. No new defects introduced by the delta.

🟡 1 suggestion(s) | 💬 2 nitpick(s)

2 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/context/identity_db.rs`:
- [SUGGESTION] src/context/identity_db.rs:660-682: `get_identity_by_id` still skips identity-key vault migration
  Carried forward and re-verified STILL VALID at 0eaa422c. `load_identities_filtered` calls `self.migrate_identity_keys_to_vault(...)` for every decoded identity, but `get_identity_by_id` (lines 660-682) only calls `hydrate_top_ups`. With the write-path fix in place (`encode_identity_blob_vault_first`), the residual exposure is narrower than before: any subsequent persistence will strip plaintext before writing, so the on-disk residency window now closes on the next write rather than the next bulk load. However, single-get callers still receive resident `PrivateKeyData::Clear`/`AlwaysClear` bytes in memory for legacy blobs and `KeyStorage` shape differs from the bulk-load path. Mirror the migration call here so the typed guarantee holds uniformly across all identity read APIs.

[thepastaclaw]

Code Review

The fcf6da1 delta lands the full SEC-001 feature (per-identity Tier-2 key encryption, encode-path downgrade guard, verify-then-seal split in add_key_to_identity, opt-in/opt-out tasks, Key Protection UI). All ten previously-resolved findings remain fixed; three prior STILL VALID findings carry forward (get_identity_by_id migration gap; ImportedKey::has_passphrase doc; prompt-meta setters silently no-op on poisoned lock — now extended to a third setter, set_identity_prompt_index). One new in-scope blocker emerged in the delta: protect_identity_keys does not verify the supplied password against already-Protected keys during a Mixed-state "Finish protecting" recovery, allowing a split-password identity that violates SEC-001's one-password-per-identity invariant. A second concern (post-broadcast seal failure in add_key_to_identity could orphan a key) is treated as a suggestion because it inherits the codebase's existing broadcast-then-persist pattern.

🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)

1 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/backend_task/identity/protect_identity_keys.rs`:
- [BLOCKING] src/backend_task/identity/protect_identity_keys.rs:151-170: `Finish protecting` recovery can split one identity across two passwords
  `seal_identity_keys` skips already-`Protected` keys without verifying that the supplied `password` can open them. In the documented Mixed-state recovery path (key A already Protected, key B still Unprotected after a crash mid-opt-in), the user can re-run with a different password than A's original: A stays sealed under the old password, B gets newly sealed under the new password, and the task reports success and records only the new hint. The result violates SEC-001's one-per-identity-password invariant — `find_protected_identity_key_scope` picks one Protected key arbitrarily as the verify anchor for the next `add_key_to_identity` precondition, so sign/opt-out behavior depends on which key the iterator hits first. The UI status copy ("Finish protecting them with the same password you set") is the only guard. Verify every existing `Protected` label opens under `password` before mutating any `Unprotected` label, so a mismatch returns `IdentityKeyPassphraseIncorrect` upfront with zero state changes.

In `src/backend_task/identity/add_key_to_identity.rs`:
- [SUGGESTION] src/backend_task/identity/add_key_to_identity.rs:147-175: Protected add-key: a post-broadcast vault-write failure orphans the on-chain key
  By the time `seal_new_identity_key_with_password` runs at line 151-160, `broadcast_and_wait` (line 88) has already accepted the new public key on Platform. If the vault write fails (I/O error, corrupt keystore, etc.) the function returns `Err` before `update_local_qualified_identity` at line 175, and `private_key` is dropped — the on-chain identity gains a key with no locally-saved private bytes. The codebase already follows broadcast-then-persist throughout, and the most catastrophic failure modes (wrong password, headless host) were correctly moved pre-broadcast via the new precondition, so this is consistent with the existing pattern rather than a regression introduced by the protected branch. Still, the protected path adds one new fallible disk write between broadcast and the existing persist step. Consider staging the protected secret in a recoverable form (e.g., write the seal under a pending label tied to the upcoming key_id, then promote on success) so a transient vault failure does not silently orphan the key.

In `src/context/identity_db.rs`:
- [SUGGESTION] src/context/identity_db.rs:723-745: `get_identity_by_id` still skips identity-key vault migration (now also reached by SEC-001 protect/unprotect tasks)
  Carried forward from prior reviews and re-verified STILL VALID at fcf6da15. `load_identities_filtered` calls `self.migrate_identity_keys_to_vault(...)` for every decoded identity (line 670), but `get_identity_by_id` (723-744) only calls `hydrate_top_ups`. The delta widens the surface: the new SEC-001 backend tasks `protect_identity_keys` and `unprotect_identity_keys` both fetch via `get_identity_by_id` (protect_identity_keys.rs:47-49, 94-96). `seal_identity_keys` skips keys whose scheme is `Absent` (protect_identity_keys.rs:166) — so any `PrivateKeyData::Clear`/`AlwaysClear` key that has not yet been bulk-load-migrated is silently passed over by an opt-in run, leaving a resident plaintext key the user thinks is now sealed. Mirror the migration call here (or have `protect_identity_keys` route through `load_identities_filtered`-equivalent first) so the typed guarantee holds across all read APIs.

[thepastaclaw]

Code Review

Carried-forward prior findings: four of the five prior findings (mismatched-password Mixed-state guard, get_identity_by_id migration, ImportedKey docstring, and prompt-meta poison-safety) are FIXED with targeted patches and dedicated regression tests; the post-broadcast orphan finding is INTENTIONALLY_DEFERRED — the structural non-atomicity remains, but the user-visible silent-orphan concern is addressed by the new typed IdentityKeyAddedButNotSaved variant that preserves the upstream cause and gives a concrete retry action. New finding in the latest delta: one suggestion-level concern raised only by codex-rust-quality — the silent on-read migration in get_identity_by_id discards KeystoreMigration::VaultWriteFailed, so an opt-in run that hits a vault-write failure during that migration ends with the QI still holding resident Clear/AlwaysClear keys whose vault labels are Absent; seal_identity_keys then skips every Absent label and returns Ok(0), so IdentityKeysProtected { count: 0 } is emitted while plaintext keys remain on disk. Trigger is narrow (the vault must fail to write during the silent migration) and count: 0 is honestly reported, but on the SEC-001 opt-in task it is a silent false-success path that warrants an explicit guard.

🟡 1 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/backend_task/identity/protect_identity_keys.rs`:
- [SUGGESTION] src/backend_task/identity/protect_identity_keys.rs:47-57: Opt-in can report success after a silently swallowed legacy-key migration failure
  The fix for prior finding 3 made `get_identity_by_id` call `migrate_identity_keys_to_vault`, but that migration's outcome is discarded via `let _ = ...` in `identity_db.rs:845`. In the failure path, `migrate_keystore_to_vault` (`identity_db.rs:314-323`) restores `qi.private_keys = before` and returns `VaultWriteFailed`, so the returned `QualifiedIdentity` still carries `Clear`/`AlwaysClear` keys while the vault labels for those `(target, key_id)` pairs remain `Absent`. `protect_identity_keys` then takes `qi.private_keys.keys_set()` (which returns every key regardless of storage scheme) and calls `seal_identity_keys`. Inside that loop the new `verify_existing_protection_password` only inspects `Protected` keys, and the match arm `SecretScheme::Protected | SecretScheme::Absent => {}` silently skips every Absent label — so the loop returns `Ok(0)`, the hint sidecar is recorded, and `BackendTaskSuccessResult::IdentityKeysProtected { identity_id, count: 0 }` is emitted on an identity whose plaintext keys are still resident on disk. The trigger is narrow (the vault write must fail during the silent migration, or the Mixed-state `ProtectedSkipped` branch is taken), but on the SEC-001 opt-in task itself a false-success report is the failure mode we are most trying to avoid. The cleanest fix is an explicit guard at the protect-layer boundary: before reporting success, reject `qi.private_keys.has_plaintext_for_vault()` with a typed actionable error so the user retries instead of believing the identity is sealed.