feat(mcp): headless masternode/evonode credit withdrawals (det-cli)

[lklimek]

Why this PR exists

• Problem: There is no headless (det-cli / MCP) path to operate a masternode/evonode identity's Platform credits. Operators must use the desktop GUI to load a node identity and withdraw its credits.
• What breaks without it: A node operator running headless — a server, CI, or scripted environment with no GUI — cannot (1) load a masternode/evonode identity from its ProTxHash + owner/voting/payout keys, nor (2) withdraw the node's Platform credits. The only existing withdrawal tooling assumes a User identity and the GUI.
• Blocking relationship: Based on PR feat: platform-wallet backend rewrite (spec + implementation) #860 (docs/platform-wallet-migration-design); merged up to its tip 0d301d01 (incl. overlay feat(overlay): blocking progress overlay + SPV-sync hard-block #863), so the diff here is masternode-only.

What was done

Two new MCP tools — no backend change; both dispatch existing IdentityTasks:

• identity_masternode_load — load a masternode/evonode identity by ProTxHash + owner/voting/payout private keys (WIF or hex), persisted locally. Non-destructive. Reports keys loaded, available withdrawal modes, and the registered payout address.
• identity_masternode_credits_withdraw — withdraw the node identity's Platform credits. key_mode=owner forces the destination to the registered payout address (dispatches to_address = None so Platform consensus routes the payout — matches the GUI/locked design); key_mode=transfer sends to any Core address (Platform/bech32m rejected). Destructive; SPV-gated.

Thin tool layer over IdentityTask::LoadIdentity / WithdrawFromIdentity; stateless parsing in model/masternode_input.rs. Private keys wrapped in Secret with a hand-written redacting Debug — never reach logs or the MCP error payload.

Headless lifecycle fixes (found + validated during live testing)

Live headless runs surfaced several standalone-det-cli lifecycle bugs (general to the headless path, fixed here so the feature is usable end-to-end):

• Backend-wiring ordering (acb06b14): withdraw resolved the identity (get_identity_by_id → wallet_backend()) before ensure_spv_synced built the backend → cold-process WalletBackendNotYetWired ("wallet is still starting up", -32603). Moved the gate ahead of the lookup. Live-validated: owner-mode withdrawal completes to the payout address.
• Cold-start migration race (7b5353f4): ensure_spv_synced waits for the storage migration (ensure_storage_ready, new StorageNotReady = -32005) before dispatch.
• Graceful backend stop on exit (ec983601): standalone/headless exit awaits WalletBackend::shutdown() (stops SPV, drains the persister) before terminating.
• Exit-time Tokio panic — deterministic hard-exit (98e96380 → superseded by 74cf43af): the sync coordinators (identity/platform-address/shielded) run on detached OS threads whose tokio::select!{ sleep | cancel } loop can poll the timer wheel against a shutting-down runtime and panic. A 100 ms grace (98e96380) was tried and failed live (it races the teardown). The deterministic fix (74cf43af): the one-shot CLI flushes stdout/stderr and std::process::exit() after the result — the runtime is never gracefully dropped while coordinator threads are alive, so there's no teardown for them to race. Safe because the withdrawal's state transition is already on-chain, SQLite is transaction-atomic, and the storage-registry lock is process-global (reclaimed on exit).

Testing

• Unit + tool registration/schema tests; masternode validators, key-redaction, owner/transfer pre-flight.
• Backend-e2e (#[ignore], E2E_MN_*) compiles; network-gated cases run manually against testnet.
• QA at the tip — PASS, zero regressions: cargo test --lib 971/971, --test kittest 146/146, doctests, e2e/backend-e2e compile, clippy -D warnings clean, fmt clean.
• Live: owner-mode withdraw succeeds end-to-end (det-cli, testnet). The hard-exit's confirmation (clean exit, no coordinator panic) is a manual headless retest — can't be unit-reproduced.

Breaking changes

None.

Checklist

• No backend / TaskError change for the tools (lifecycle fixes touch the MCP/CLI seam + a new StorageNotReady MCP error)
• No real secrets in the diff (verified)
• QA fix-pass landed + re-verified post-merge
• Merged up to date with feat: platform-wallet backend rewrite (spec + implementation) #860 tip (incl. overlay feat(overlay): blocking progress overlay + SPV-sync hard-block #863)
• Headless lifecycle fixes (ordering, migration race, graceful stop, deterministic hard-exit) — QA-green; ordering + withdraw live-validated
• Manual headless retest confirming clean exit (code 0, no coordinator panic)

Follow-ups (tracked, not in this PR)

• Security (codebase-wide audit): encrypt directly-loaded identity keys at rest (PrivateKeyData::Clear → SecretStore/AES-GCM at the DetKv seam, HIGH); SecretStore vault opened with empty passphrase (MED); ClosedSingleKey derives Debug over raw key bytes (MED); det-cli keys via argv — implement stdin/env input (MED); .env plaintext creds (LOW); non-Zeroizing no-password key material (LOW).
• Upstream: quiesce() should join the coordinator JoinHandle (then DET could keep the graceful teardown instead of hard-exit).
• ensure_spv_synced swallows backend-wiring errors → misleading SpvSyncFailed timeout.
• Credit-withdrawal fee estimate over-shoots actual (~2×).

_{🤖 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: 8ba3e6bb-c15e-4365-afed-113dd030265f

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/masternode-cli-withdraw

---

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 and usage tips.}

[thepastaclaw]

✅ Review complete (commit 74cf43a)

[thepastaclaw]

Code Review

Owner-mode withdraw dispatches Some(payout_address) instead of None, diverging from the locked design and the e2e test's assumption — confirmed blocking. The new transfer-mode path also newly exposes a pre-existing non-ASCII panic in is_platform_address_string through the MCP surface. Remaining items are test-coverage and Rust-quality refinements.

🔴 2 blocking | 🟡 4 suggestion(s) | 💬 1 nitpick(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/mcp/tools/identity.rs`:
- [BLOCKING] src/mcp/tools/identity.rs:978-1013: Owner-mode withdraw dispatches Some(payout_address); spec and e2e expect None
  The owner-mode arm at lines 979–1000 unconditionally wraps the resolved payout address in `Some(...)`, so `IdentityTask::WithdrawFromIdentity` is dispatched with `Some(payout_address)`. The locked design (`docs/ai-design/2026-06-18-masternode-cli-withdraw/03-dev-plan.md:21`) and the e2e test (`tests/backend-e2e/identity_masternode_withdraw.rs:296-302`, TC-MN-050) both require owner mode to dispatch `to_address = None` so Platform consensus authoritatively routes the payout to the registered address — the resolved payout address should only be echoed in the output. Because the test hand-constructs the task with `None`, the divergence is silently uncaught. The owner arm should resolve the payout address only for the output echo and pass `None` to the dispatched task; the transfer arm continues to pass `Some(checked)`. After this fix, also reconsider the trailing `unwrap_or_default()` on `dispatched_address` (line 1034) which becomes a real `None` branch in owner mode — replace it with a branch-specific value so the caller always learns where the funds went.
- [SUGGESTION] src/mcp/tools/identity.rs:739-749: Wrap private keys in Secret before the SPV await
  Cheap validation runs at lines 732–737, then the tool awaits `ensure_spv_synced` before moving `owner_private_key`, `voting_private_key`, and `payout_private_key` into `Secret`. If the SPV wait is long or times out, those key strings sit in the async state machine as ordinary `String`s — not mlocked, not zeroized on drop. The rest of the crate moves sensitive values into `Secret`/`Zeroizing` at the boundary, before any await. Wrap the three key params into `Secret::new` immediately after key-presence validation; the validation ordering is preserved and the plaintext lifetime is bounded.
- [SUGGESTION] src/mcp/tools/identity.rs:732-741: Validate pro_tx_hash before the SPV wait
  The load tool already validates network, node type, and key-presence before `ensure_spv_synced`, but skips `pro_tx_hash`. A malformed ProTxHash therefore waits on SPV and, if SPV is unavailable, returns an SPV error instead of an input error. `masternode_input::decode_identity_id` is already available and cheap. Calling it before `ensure_spv_synced` keeps the cheap-validation-before-SPV pattern consistent with the other parameters and lets the load tool reuse the decoded `Identifier` in `IdentityInputToLoad` (the backend currently re-parses the string).
- [SUGGESTION] src/mcp/tools/identity.rs:766-781: KeyMode vocabulary duplicated between Tool A output and Tool B input
  The `available_withdrawal_keys` output writes literal `"owner"` / `"transfer"` strings, and `parse_key_mode` (`src/model/masternode_input.rs:55`) parses the same vocabulary — but each side hand-writes its own literals. A typo on either side becomes silent contract drift between Tool A's output and Tool B's input. Both pieces live in this PR, so adding a small `KeyMode::as_str()` (or `Display`) in `model/masternode_input.rs` and using it on both sides removes the duplication without adding any abstraction overhead.

In `src/model/address.rs`:
- [BLOCKING] src/model/address.rs:14-24: is_platform_address_string panics on non-ASCII input — now reachable from the new MCP tool
  `is_platform_address_string` slices `s[..hrp.len()]` by byte offset after only a length check. A non-ASCII string whose first code point straddles the HRP boundary (4 for `dash`, 5 for `tdash`) panics on the string slice. The function pre-dates this PR, but the new `parse_transfer_core_address` in `src/mcp/tools/identity.rs:876` feeds it user-controlled `to_address` from a JSON MCP request after only `trim()`, which does not normalize non-ASCII. A single bad request to `identity_masternode_credits_withdraw` can panic the in-process MCP server. Comparing on `s.as_bytes()` (e.g. `s.as_bytes().get(..hrp.len()).is_some_and(|h| h.eq_ignore_ascii_case(hrp.as_bytes())) && s.as_bytes().get(hrp.len()) == Some(&b'1')`) makes the check non-panicking. Add a regression test for non-ASCII input at the same time.

In `tests/backend-e2e/identity_masternode_withdraw.rs`:
- [SUGGESTION] tests/backend-e2e/identity_masternode_withdraw.rs:264-314: TC-MN-050/051 bypass the tool — owner-mode dispatch is never exercised
  `test_mn050_owner_withdraw_to_payout` and the companion TC-MN-051 build `IdentityTask::WithdrawFromIdentity` directly rather than calling `IdentityMasternodeCreditsWithdraw::invoke`. The tool's owner/transfer destination resolution, purpose→KeyID mapping, `payout_address` echo, owner+address contradiction guard, and SPV gate are therefore never exercised end-to-end on testnet — exactly how the blocking owner-mode mismatch went undetected. Drive these tests through the tool's `invoke()` so the production code path is what the suite verifies. The test comment on line 296 ("OWNER mode dispatches to_address = None") then becomes an actual assertion on the tool's behavior rather than a description of the hand-built task.

Note: this review is pinned to dispatcher-claimed commit 6d823786; the PR branch had already advanced to b9cb11bb before posting, so findings are posted body-only rather than inline-mapped against the newer live diff.

[thepastaclaw]

Code Review

Verifier pass against b9cb11b. The prior blocking owner-mode dispatch bug (QA-001) is RESOLVED via the new pure resolve_withdrawal_plan (owner -> dispatch_address: None) wired in at line 1066, with new unit-test coverage. One prior blocking finding still applies: is_platform_address_string byte-slices a &str and panics on non-ASCII input, and this path is now reachable from the new identity_masternode_credits_withdraw tool BEFORE the SPV gate. Five lower-severity carried-forward quality items (Secret-wrap timing, pre-SPV ProTxHash validation, duplicate withdrawal-mode labels, KeyMode vocabulary duplication, and e2e tests bypassing invoke()) are unchanged at HEAD. No new latest-delta defects beyond these carried-forward items.

🔴 1 blocking | 🟡 4 suggestion(s) | 💬 1 nitpick(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/model/address.rs`:
- [BLOCKING] src/model/address.rs:14-24: is_platform_address_string panics on non-ASCII input; reachable from the new withdraw tool
  STILL VALID at b9cb11bb. The helper byte-slices `s[..hrp.len()]` after only a `s.len() > hrp.len()` length check — if `hrp.len()` (4 for `dash`, 5 for `tdash`) falls inside a multi-byte UTF-8 code point, the slice panics. The new `identity_masternode_credits_withdraw` tool feeds user-controlled `to_address` straight through `parse_transfer_core_address` (src/mcp/tools/identity.rs:888), which is called by `resolve_withdrawal_plan` at line 1057 — BEFORE the SPV gate at line 1062. A single MCP request with `key_mode=transfer` and a non-ASCII `to_address` whose first character is multi-byte (e.g. `日本人ABC`, 9 bytes with char boundaries at 0/3/6/9) panics the in-process MCP server instead of returning `InvalidParam`. The fix compares on bytes (the function is ASCII-only by design). Add a regression test for non-ASCII input — the new withdraw tests cover only ASCII.

In `src/mcp/tools/identity.rs`:
- [SUGGESTION] src/mcp/tools/identity.rs:743-768: Wrap masternode private keys in Secret before the SPV await
  STILL VALID. Cheap validation runs at lines 746-752, then `ensure_spv_synced(&ctx).await` runs at line 756 with `param.{owner,voting,payout}_private_key` still held as ordinary `String`s. Only at lines 762-764 are they moved into `Secret::new(...)`. If the SPV gate waits a long time or times out (up to the 10-minute cap), the plaintext key material sits in the async state machine without zeroize/mlock protection. The rest of the crate moves sensitive values into `Secret`/`Zeroizing` at the input boundary. Wrap the three keys into `Secret::new` immediately after `require_at_least_one_signing_key` so plaintext lifetime is bounded; validation ordering is preserved.
- [SUGGESTION] src/mcp/tools/identity.rs:743-759: Validate pro_tx_hash before the SPV wait
  STILL VALID. The load tool validates network presence/match, node type, and the at-least-one-signing-key rule before `ensure_spv_synced`, but does not validate `pro_tx_hash` — line 759 forwards the raw `String` into `IdentityInputToLoad.identity_id_input` for the backend task to re-parse after SPV. `masternode_input::decode_identity_id` is a stateless cheap check; calling it before the SPV gate keeps the cheap-validation-before-network pattern consistent and lets a malformed ProTxHash fail as `InvalidParam` rather than as an SPV error when the chain is unavailable. The decoded `Identifier` could then be reused instead of having the backend re-parse the string.
- [SUGGESTION] src/mcp/tools/identity.rs:781-805: KeyMode vocabulary duplicated between Tool A output and Tool B input/output
  STILL VALID. The load tool's `available_withdrawal_keys` output hand-writes literal `"owner"` / `"transfer"` strings at lines 788 and 792; the withdraw tool's `key_mode` echo writes the same literals again at 1083-1084; `parse_key_mode` in src/model/masternode_input.rs:55 parses the same vocabulary. A typo on either side becomes silent contract drift between Tool A's advertised modes and Tool B's accepted input. All three call sites live in this PR — adding a small `KeyMode::as_str()` (or `Display`) in `model/masternode_input.rs` and using it on both sides removes the duplication without adding abstraction overhead.

In `tests/backend-e2e/identity_masternode_withdraw.rs`:
- [SUGGESTION] tests/backend-e2e/identity_masternode_withdraw.rs:264-382: E2E withdrawal tests still hand-build IdentityTask::WithdrawFromIdentity instead of driving invoke()
  PARTIALLY MITIGATED, STILL VALID. The latest delta adds unit tests against the new pure `resolve_withdrawal_plan` resolver, which is the regression guard for the QA-001 owner-mode dispatch bug. But `git grep -n 'IdentityMasternodeCreditsWithdraw::invoke\|IdentityMasternodeLoad::invoke' tests/` returns nothing: TC-MN-050 (lines 297-302) and TC-MN-051 (lines 365-370) still construct `IdentityTask::WithdrawFromIdentity` directly rather than calling `IdentityMasternodeCreditsWithdraw::invoke`. As a result the SPV gate ordering, network echo, owner+address contradiction check, identity DB lookup, and `IdentityMasternodeWithdrawOutput::to_address` echo are not exercised against a real chain — only the pure resolver is. Drive at least TC-MN-050/051 through `IdentityMasternodeCreditsWithdraw::invoke` so the production code path is what the e2e suite verifies on testnet.

[thepastaclaw]

🟡 Suggestion: Wrap masternode private keys in Secret before the SPV await

STILL VALID. Cheap validation runs at lines 746-752, then ensure_spv_synced(&ctx).await runs at line 756 with param.{owner,voting,payout}_private_key still held as ordinary Strings. Only at lines 762-764 are they moved into Secret::new(...). If the SPV gate waits a long time or times out (up to the 10-minute cap), the plaintext key material sits in the async state machine without zeroize/mlock protection. The rest of the crate moves sensitive values into Secret/Zeroizing at the input boundary. Wrap the three keys into Secret::new immediately after require_at_least_one_signing_key so plaintext lifetime is bounded; validation ordering is preserved.

Suggested change

	// Cheap validation first, before the SPV wait: network presence/match,
	// node type, and the at-least-one-signing-key rule all reject without
	// touching the network.
	require_nonblank_network(&param.network)?;
	resolve::require_network(&ctx, Some(&param.network))?;
	let identity_type = masternode_input::parse_node_type(&param.node_type)?;
	masternode_input::require_at_least_one_signing_key(
	&param.owner_private_key,
	&param.payout_private_key,
	)?;
	// Load fetches the identity (and, with a voting key, the voter identity)
	// over DAPI; proof verification needs a synced chain.
	resolve::ensure_spv_synced(&ctx).await?;
	let input = IdentityInputToLoad {
	identity_id_input: param.pro_tx_hash,
	identity_type,
	alias_input: param.alias.trim().to_owned(),
	voting_private_key_input: Secret::new(param.voting_private_key),
	owner_private_key_input: Secret::new(param.owner_private_key),
	payout_address_private_key_input: Secret::new(param.payout_private_key),
	keys_input: vec![],
	derive_keys_from_wallets: false,
	selected_wallet_seed_hash: None,
	};
	let voting_private_key = Secret::new(param.voting_private_key);
	let owner_private_key = Secret::new(param.owner_private_key);
	let payout_private_key = Secret::new(param.payout_private_key);
	// Load fetches the identity (and, with a voting key, the voter identity)
	// over DAPI; proof verification needs a synced chain.
	resolve::ensure_spv_synced(&ctx).await?;
	let input = IdentityInputToLoad {
	identity_id_input: param.pro_tx_hash,
	identity_type,
	alias_input: param.alias.trim().to_owned(),
	voting_private_key_input: voting_private_key,
	owner_private_key_input: owner_private_key,
	payout_address_private_key_input: payout_private_key,

_{source: ['claude-general', 'claude-rust-quality', 'codex-general', 'codex-rust-quality']}

[thepastaclaw]

🟡 Suggestion: E2E withdrawal tests still hand-build IdentityTask::WithdrawFromIdentity instead of driving invoke()

PARTIALLY MITIGATED, STILL VALID. The latest delta adds unit tests against the new pure resolve_withdrawal_plan resolver, which is the regression guard for the QA-001 owner-mode dispatch bug. But git grep -n 'IdentityMasternodeCreditsWithdraw::invoke\|IdentityMasternodeLoad::invoke' tests/ returns nothing: TC-MN-050 (lines 297-302) and TC-MN-051 (lines 365-370) still construct IdentityTask::WithdrawFromIdentity directly rather than calling IdentityMasternodeCreditsWithdraw::invoke. As a result the SPV gate ordering, network echo, owner+address contradiction check, identity DB lookup, and IdentityMasternodeWithdrawOutput::to_address echo are not exercised against a real chain — only the pure resolver is. Drive at least TC-MN-050/051 through IdentityMasternodeCreditsWithdraw::invoke so the production code path is what the e2e suite verifies on testnet.

_{source: ['claude-general', 'claude-rust-quality', 'codex-general', 'codex-rust-quality']}

[thepastaclaw]

🟡 Suggestion: Validate pro_tx_hash before the SPV wait

STILL VALID. The load tool validates network presence/match, node type, and the at-least-one-signing-key rule before ensure_spv_synced, but does not validate pro_tx_hash — line 759 forwards the raw String into IdentityInputToLoad.identity_id_input for the backend task to re-parse after SPV. masternode_input::decode_identity_id is a stateless cheap check; calling it before the SPV gate keeps the cheap-validation-before-network pattern consistent and lets a malformed ProTxHash fail as InvalidParam rather than as an SPV error when the chain is unavailable. The decoded Identifier could then be reused instead of having the backend re-parse the string.

_{source: ['claude-general', 'claude-rust-quality', 'codex-general', 'codex-rust-quality']}

[thepastaclaw]

Resolved in acb06b1 — Validate pro_tx_hash before the SPV wait 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: KeyMode vocabulary duplicated between Tool A output and Tool B input/output

STILL VALID. The load tool's available_withdrawal_keys output hand-writes literal "owner" / "transfer" strings at lines 788 and 792; the withdraw tool's key_mode echo writes the same literals again at 1083-1084; parse_key_mode in src/model/masternode_input.rs:55 parses the same vocabulary. A typo on either side becomes silent contract drift between Tool A's advertised modes and Tool B's accepted input. All three call sites live in this PR — adding a small KeyMode::as_str() (or Display) in model/masternode_input.rs and using it on both sides removes the duplication without adding abstraction overhead.

_{source: ['claude-general', 'claude-rust-quality', 'codex-general', 'codex-rust-quality']}

[thepastaclaw]

Resolved in acb06b1 — KeyMode vocabulary duplicated between Tool A output and Tool B input/output 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]

💬 Nitpick: available_withdrawal_keys may contain duplicates

STILL VALID. The loop at lines 783-796 unconditionally pushes "owner" / "transfer" once per matching key. QualifiedIdentity::available_withdrawal_keys() returns one entry per loaded key per purpose, so an identity carrying multiple OWNER or TRANSFER keys would emit ["owner","owner","transfer"]. Cosmetic for typical masternode identities (one of each), but agents that read this as the set of supported modes would be surprised. Use a BTreeSet<&str> or guard with contains before pushing to make the set semantics explicit.

Suggested change

	let mut owner_key_loaded = false;
	let mut payout_key_loaded = false;
	let mut available_withdrawal_keys = Vec::new();
	for key in qi.available_withdrawal_keys() {
	match key.identity_public_key.purpose() {
	Purpose::OWNER => {
	owner_key_loaded = true;
	available_withdrawal_keys.push("owner".to_owned());
	}
	Purpose::TRANSFER => {
	payout_key_loaded = true;
	available_withdrawal_keys.push("transfer".to_owned());
	}
	_ => {}
	}
	}
	let mut owner_key_loaded = false;
	let mut payout_key_loaded = false;
	let mut available_withdrawal_keys = std::collections::BTreeSet::new();
	for key in qi.available_withdrawal_keys() {
	match key.identity_public_key.purpose() {
	Purpose::OWNER => {
	owner_key_loaded = true;
	available_withdrawal_keys.insert("owner");
	}
	Purpose::TRANSFER => {
	payout_key_loaded = true;
	available_withdrawal_keys.insert("transfer");
	}
	_ => {}
	}
	}
	Ok(IdentityMasternodeLoadOutput {
	identity_id: qi.identity.id().to_string(Encoding::Base58),
	node_type: identity_type.to_string().to_ascii_lowercase(),
	alias: qi.alias.clone(),
	owner_key_loaded,
	voting_key_loaded: qi.associated_voter_identity.is_some(),
	payout_key_loaded,
	available_withdrawal_keys: available_withdrawal_keys.into_iter().map(str::to_owned).collect(),

_{source: ['claude-general', 'claude-rust-quality', 'codex-general', 'codex-rust-quality']}

[thepastaclaw]

Resolved in acb06b1 — available_withdrawal_keys may contain duplicates 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]

Code Review

Carried-forward + incremental review at acb06b1. The non-ASCII panic in is_platform_address_string remains the sole blocking issue — reproduced locally: slicing s[..hrp.len()] panics on UTF-8 boundary inputs and is still reachable through the new MCP withdraw tool's transfer-mode path. The new lifecycle delta (graceful wallet-backend shutdown, storage-migration wait, withdraw-tool reordering) is mostly sound, but the shutdown is incomplete on two paths (stdio ? error short-circuits, and headless after network_switch) and the storage-migration wait observes a migration state that is never set in standalone MCP. Five prior quality/test findings still apply unchanged; one (pre-SPV ID validation) was partially fixed for the withdraw tool but not for the load tool.

🔴 1 blocking | 🟡 4 suggestion(s)

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

4 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/model/address.rs`:
- [BLOCKING] src/model/address.rs:14-24: is_platform_address_string panics on non-ASCII input; reachable from the new MCP withdraw tool
  Line 17 still slices `s[..hrp.len()]` against a `&str`. Rust panics if the byte offset falls inside a multi-byte UTF-8 code point. With `hrp.len() == 4` (`dash`) or `5` (`tdash`), any input whose first character is a 3-byte sequence (e.g. `日本ABC`, char boundaries 0/3/6/9) trips the slice at byte 4 — verified locally: `&"日本ABC"[..4]` panics with `byte index 4 is not a char boundary`. The new `identity_masternode_credits_withdraw` tool feeds user-controlled `to_address` through `parse_transfer_core_address` (src/mcp/tools/identity.rs:1066, inside `resolve_withdrawal_plan`) — on a synced standalone process, one MCP request with `key_mode=transfer` and a non-ASCII `to_address` panics the in-process MCP server instead of returning `InvalidParam`. Switch to byte comparison (HRPs are ASCII by design) and add a non-ASCII regression test.

In `src/bin/det_cli/headless.rs`:
- [SUGGESTION] src/bin/det_cli/headless.rs:48-56: Headless shutdown only stops the current backend after a network_switch
  The headless exit path shuts down only `swappable.load_full()`'s currently active context. `BackendTask::SwitchNetwork` (src/backend_task/mod.rs:572-628) builds a fresh `AppContext` for the new network and wires its backend, but never calls `stop_spv()` on the old context. After a session that switches networks, the previous network's `WalletBackend` (run loop + periodic coordinators) is still alive and is then dropped during runtime teardown — exactly the timer-registration panic class this delta is meant to prevent. The stdio path has the same shape via `DashMcpService::shutdown_wallet_backend`. Track every context wired during the session (or shut down the outgoing backend at the `swap_context` callsite in src/mcp/tools/network.rs:250) so all backends are quiesced before runtime drop.

In `src/mcp/mod.rs`:
- [SUGGESTION] src/mcp/mod.rs:29-44: start_stdio skips shutdown_wallet_backend() on the `?` error paths
  Both `service.serve(...).await?` at :36 and `server.waiting().await?` at :37 short-circuit with `?` before the shutdown at :42. If `waiting()` errors after a tool has already wired the wallet backend (e.g. mid-session transport hiccup), the runtime drops the backend uncleanly — the same panic class the commit was meant to eliminate. The 5-second `runtime.shutdown_timeout(...)` backstop in `src/bin/det_cli/connect.rs:36` only bounds the wait, it doesn't quiesce the coordinators. Capture `let result = server.waiting().await;` (no `?`), call `service_for_shutdown.shutdown_wallet_backend().await` unconditionally, then return `result` — mirroring the layout already used in `src/bin/det_cli/headless.rs:30-58` where shutdown runs regardless of the server result.

In `src/mcp/resolve.rs`:
- [SUGGESTION] src/mcp/resolve.rs:130-161: ensure_storage_ready waits for a migration that standalone MCP never starts
  `ensure_storage_ready` only blocks when `ctx.migration_status().state().is_running()`. In standalone/headless MCP, `init_app_context` (src/mcp/server.rs:232) does not dispatch any migration task, and `MigrationTask::FinishUnwire` is only ever dispatched from the GUI cold-start path (`AppState::dispatch_cold_start_migration` at src/app.rs:1121,1881) or the shielded-tab retry button. That means det-cli/headless always sees `MigrationState::Idle`, the new wait short-circuits, and the underlying `run_backend_task` gate (`is_wallet_touching` short-circuits only when migration is *Running*) also passes — so wallet-touching tools may run against not-yet-migrated sidecars on a cold standalone process. Either dispatch the migration from the headless/CLI context path before serving tools, or kick it off inside `ensure_spv_synced` before observing its state.

In `src/mcp/tools/identity.rs`:
- [SUGGESTION] src/mcp/tools/identity.rs:781-1088: KeyMode vocabulary duplicated across load output, withdraw echo, and parser
  `available_withdrawal_keys` writes literal `"owner"` / `"transfer"` at :788 and :792; the withdraw tool repeats the same literals at :1087-1088; `parse_key_mode` (src/model/masternode_input.rs:55) parses the same vocabulary from raw strings. All three sites live in this PR, so a typo on any side becomes silent contract drift between Tool A's advertised modes and Tool B's accepted input. A small `KeyMode::as_str()` (or `Display`) in `model/masternode_input.rs` used at every site removes the duplication with no abstraction overhead.

[thepastaclaw]

🟡 Suggestion: Headless shutdown only stops the current backend after a network_switch

The headless exit path shuts down only swappable.load_full()'s currently active context. BackendTask::SwitchNetwork (src/backend_task/mod.rs:572-628) builds a fresh AppContext for the new network and wires its backend, but never calls stop_spv() on the old context. After a session that switches networks, the previous network's WalletBackend (run loop + periodic coordinators) is still alive and is then dropped during runtime teardown — exactly the timer-registration panic class this delta is meant to prevent. The stdio path has the same shape via DashMcpService::shutdown_wallet_backend. Track every context wired during the session (or shut down the outgoing backend at the swap_context callsite in src/mcp/tools/network.rs:250) so all backends are quiesced before runtime drop.

_{source: ['codex']}

[thepastaclaw]

🟡 Suggestion: start_stdio skips shutdown_wallet_backend() on the ? error paths

Both service.serve(...).await? at :36 and server.waiting().await? at :37 short-circuit with ? before the shutdown at :42. If waiting() errors after a tool has already wired the wallet backend (e.g. mid-session transport hiccup), the runtime drops the backend uncleanly — the same panic class the commit was meant to eliminate. The 5-second runtime.shutdown_timeout(...) backstop in src/bin/det_cli/connect.rs:36 only bounds the wait, it doesn't quiesce the coordinators. Capture let result = server.waiting().await; (no ?), call service_for_shutdown.shutdown_wallet_backend().await unconditionally, then return result — mirroring the layout already used in src/bin/det_cli/headless.rs:30-58 where shutdown runs regardless of the server result.

_{source: ['claude']}

[thepastaclaw]

🟡 Suggestion: ensure_storage_ready waits for a migration that standalone MCP never starts

ensure_storage_ready only blocks when ctx.migration_status().state().is_running(). In standalone/headless MCP, init_app_context (src/mcp/server.rs:232) does not dispatch any migration task, and MigrationTask::FinishUnwire is only ever dispatched from the GUI cold-start path (AppState::dispatch_cold_start_migration at src/app.rs:1121,1881) or the shielded-tab retry button. That means det-cli/headless always sees MigrationState::Idle, the new wait short-circuits, and the underlying run_backend_task gate (is_wallet_touching short-circuits only when migration is Running) also passes — so wallet-touching tools may run against not-yet-migrated sidecars on a cold standalone process. Either dispatch the migration from the headless/CLI context path before serving tools, or kick it off inside ensure_spv_synced before observing its state.

_{source: ['codex']}

[thepastaclaw]

🟡 Suggestion: KeyMode vocabulary duplicated across load output, withdraw echo, and parser

available_withdrawal_keys writes literal "owner" / "transfer" at :788 and :792; the withdraw tool repeats the same literals at :1087-1088; parse_key_mode (src/model/masternode_input.rs:55) parses the same vocabulary from raw strings. All three sites live in this PR, so a typo on any side becomes silent contract drift between Tool A's advertised modes and Tool B's accepted input. A small KeyMode::as_str() (or Display) in model/masternode_input.rs used at every site removes the duplication with no abstraction overhead.

_{source: ['claude', 'codex']}

[thepastaclaw]

Code Review

Incremental review at 98e9638. The latest delta (acb06b1..98e9638) adds a documented 100 ms post-shutdown grace sleep in DashMcpService::shutdown_wallet_backend (stdio path) plus block-comment documentation. All 9 prior findings from the acb06b1 review remain valid at HEAD: the non-ASCII slice panic in is_platform_address_string is reachable from the new MCP withdraw tool's parse_transfer_core_address before the SPV gate; Secret-wrap timing, pre-SPV ProTxHash validation in the load tool, KeyMode vocabulary duplication, available_withdrawal_keys duplicates, ensure_storage_ready/migration mismatch, start_stdio error-path skip, network-switch lifecycle leak, and the e2e bypass of invoke() are all unchanged. The new delta also exposes a related gap: the 100 ms coordinator-thread grace lives only in the stdio shutdown helper — the headless HTTP path in src/bin/det_cli/headless.rs:54-56 calls backend.shutdown().await directly without the same grace.

🔴 1 blocking | 🟡 1 suggestion(s)

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

8 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/model/address.rs`:
- [BLOCKING] src/model/address.rs:14-24: is_platform_address_string panics on non-ASCII input; reachable from the new MCP withdraw tool
  CARRIED FORWARD from prior reviews. Line 17 slices `s[..hrp.len()]` against a `&str` after only a `s.len() > hrp.len()` length check. Rust panics if the byte offset falls inside a multi-byte UTF-8 code point. With `PLATFORM_HRP_MAINNET="dash"` (4 bytes) or `PLATFORM_HRP_TESTNET="tdash"` (5 bytes), any input whose first character is a 3-byte UTF-8 sequence (e.g. `日本ABC` — char boundaries 0/3/6/9) trips the slice at byte 4 with `byte index 4 is not a char boundary`. The new `identity_masternode_credits_withdraw` tool feeds user-controlled `to_address` through `parse_transfer_core_address` (src/mcp/tools/identity.rs:888), called from `resolve_withdrawal_plan` at line 974 AFTER the SPV gate but before any address parsing — so a single MCP request with `key_mode="transfer"` and a non-ASCII `to_address` panics the in-process MCP server instead of returning `InvalidParam`. The HRPs are ASCII by design; switch to byte comparison and add a non-ASCII regression test.

In `src/bin/det_cli/headless.rs`:
- [SUGGESTION] src/bin/det_cli/headless.rs:53-56: Headless HTTP shutdown skips the new coordinator-grace window
  NEW IN LATEST DELTA. Commit 98e96380 adds a 100 ms post-`backend.shutdown()` sleep in `DashMcpService::shutdown_wallet_backend` (src/mcp/server.rs:179-190), explicitly documenting that `backend.shutdown()` cancels coordinators but does NOT join their dedicated OS threads — they may still reach the outer `tokio::select!` and call `tokio::time::sleep(interval)` as the runtime begins shutdown, panicking with `A Tokio 1.x context was found, but it is being shutdown.` The headless HTTP exit path here does NOT use that helper: it calls `backend.shutdown().await` directly at line 55, then exits the `block_on` block and enters `runtime.shutdown_timeout(Duration::from_secs(5))` at line 63. This is the primary `det-cli --headless` path the PR adds for operators, so the exit-panic mitigation is incomplete in the path that needs it most. Either share the grace via a helper, or call the existing `service.shutdown_wallet_backend()` from here.

[thepastaclaw]

🟡 Suggestion: Headless HTTP shutdown skips the new coordinator-grace window

NEW IN LATEST DELTA. Commit 98e9638 adds a 100 ms post-backend.shutdown() sleep in DashMcpService::shutdown_wallet_backend (src/mcp/server.rs:179-190), explicitly documenting that backend.shutdown() cancels coordinators but does NOT join their dedicated OS threads — they may still reach the outer tokio::select! and call tokio::time::sleep(interval) as the runtime begins shutdown, panicking with A Tokio 1.x context was found, but it is being shutdown. The headless HTTP exit path here does NOT use that helper: it calls backend.shutdown().await directly at line 55, then exits the block_on block and enters runtime.shutdown_timeout(Duration::from_secs(5)) at line 63. This is the primary det-cli --headless path the PR adds for operators, so the exit-panic mitigation is incomplete in the path that needs it most. Either share the grace via a helper, or call the existing service.shutdown_wallet_backend() from here.

Suggested change

	let current_ctx = swappable.load_full();
	if let Ok(backend) = current_ctx.wallet_backend() {
	backend.shutdown().await;
	}
	if let Ok(backend) = current_ctx.wallet_backend() {
	backend.shutdown().await;
	tokio::time::sleep(Duration::from_millis(100)).await;
	}

_{source: ['claude', 'codex']}

[thepastaclaw]

Resolved in 74cf43a — Headless HTTP shutdown skips the new coordinator-grace window 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]

Code Review

Cumulative verification at 74cf43a. The latest delta replaces the 100ms post-shutdown grace with std::process::exit in run_stdio_server, run_headless, and the one-shot CLI path — this deterministically eliminates the coordinator-thread/timer-wheel panic class, fully resolving prior finding #2. Nine prior findings remain STILL VALID and are carried forward; one new low-confidence finding (one-shot CLI hard-exits without draining the in-process backend, codex-rust-quality only) is added. The lone blocking issue is the non-ASCII slice panic in is_platform_address_string, reachable from the new MCP withdraw tool's transfer-mode path with user-controlled input.

🔴 1 blocking | 🟡 1 suggestion(s)

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

8 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/model/address.rs`:
- [BLOCKING] src/model/address.rs:14-24: is_platform_address_string panics on non-ASCII input; reachable from the new MCP withdraw tool
  CARRIED FORWARD — STILL VALID at 74cf43af. Line 17 still does `s[..hrp.len()].eq_ignore_ascii_case(hrp)` against a `&str` after only a byte-length check `s.len() > hrp.len()`. Rust panics when the byte offset falls inside a multi-byte UTF-8 code point. With `PLATFORM_HRP_MAINNET = "dash"` (4 bytes) or `PLATFORM_HRP_TESTNET = "tdash"` (5 bytes), any input whose first character is a 3-byte UTF-8 sequence (e.g. `日本ABC`) trips the slice at byte 4 with `byte index 4 is not a char boundary`. The new `identity_masternode_credits_withdraw` tool feeds user-controlled `to_address` through `parse_transfer_core_address` (src/mcp/tools/identity.rs:888), invoked from `resolve_withdrawal_plan` at line 974 — so a single MCP request with `key_mode="transfer"` and a non-ASCII `to_address` panics the in-process MCP service instead of returning `InvalidParam`. The unit tests at lines 410-425 only cover ASCII inputs. HRPs are ASCII by design — compare on bytes and add a non-ASCII regression test.

In `src/bin/det_cli/main.rs`:
- [SUGGESTION] src/bin/det_cli/main.rs:129-146: One-shot standalone CLI hard-exits without draining the in-process backend
  NEW IN LATEST DELTA. The one-shot CLI now calls `std::process::exit` immediately after `runtime.block_on(run(cli))`. In standalone mode `run()` uses `connect_in_process()`, which spawns a local `DashMcpService`; wallet-facing tools such as `identity_masternode_credits_withdraw` wire the wallet backend and start coordinator persisters via `ensure_spv_synced`. Unlike `start_stdio` and `run_headless`, this path has no handle to the service after the call and never awaits `DashMcpService::shutdown_wallet_backend()`, so process exit kills in-flight coordinator change-set writes. The inline comment correctly notes that SQLite transaction atomicity prevents corruption and that the tool's own writes are committed, but it does not address pending coordinator persister writes that were started in the background during the tool invocation. Capture the service from `connect_in_process` for one-shot calls and run `shutdown_wallet_backend().await` before computing `exit_code`.

[thepastaclaw]

🟡 Suggestion: One-shot standalone CLI hard-exits without draining the in-process backend

NEW IN LATEST DELTA. The one-shot CLI now calls std::process::exit immediately after runtime.block_on(run(cli)). In standalone mode run() uses connect_in_process(), which spawns a local DashMcpService; wallet-facing tools such as identity_masternode_credits_withdraw wire the wallet backend and start coordinator persisters via ensure_spv_synced. Unlike start_stdio and run_headless, this path has no handle to the service after the call and never awaits DashMcpService::shutdown_wallet_backend(), so process exit kills in-flight coordinator change-set writes. The inline comment correctly notes that SQLite transaction atomicity prevents corruption and that the tool's own writes are committed, but it does not address pending coordinator persister writes that were started in the background during the tool invocation. Capture the service from connect_in_process for one-shot calls and run shutdown_wallet_backend().await before computing exit_code.

_{source: ['codex']}