fix(accounts): account removal must stick — incoming set authoritative + lock-held read-modify-write

[iceteaSA]

Problem

Removing a fallback account (/openai-account remove <id>, the TUI account modal, or the openai-auth CLI remove) did not stick. mergeStorageForSave unioned the on-disk (latest, re-read under the lock) and incoming account arrays via a Map and never deleted, so a removed account was resurrected on the next load. A second, quieter facet: orphaned per-account state could linger.

Proven with a repro test before fixing: seed {a, b} → reload → splice b → save → reload returns {a, b} (b resurrected).

Fix

1. mergeStorageForSave — incoming account set is authoritative. Returns {...latest, ...incoming, accounts: incoming.accounts}, so a removal (splice) sticks. Non-account top-level fields still fall back to the latest on-disk snapshot for fields incoming omits.

2. mutateAccounts — a lock-held read-modify-write helper. Because the incoming set is now authoritative, a caller that did loadAccounts() (lock-free) → mutate → saveAccounts() had an inverse TOCTOU: a concurrent (cross-process) add landing between the load and the lock-held save would be wiped by the stale snapshot. mutateAccounts re-reads config+state under the lock (same config→state lock order as saveAccounts) and applies the mutation to fresh data, so a concurrent add is preserved. All mutation paths route through it: commands switch/remove/order/add, CLI login/remove, and the loader's mainAccountId + main-refresh-state writes.

3. saveAccountState full-save no longer prunes the accounts map. Removal cleans per-account state via the saveAccounts → stateFromStorage rebuild (which rebuilds the state file from the authoritative account set). The prune was reachable only from the lock-free background-refresh path, where a concurrent add could make it wrongly drop the new account's state (token loss). Reverted; the scoped-save prune is unchanged.

Review

Cross-family race council (MiniMax-M3 + gemini-3.1-pro + claude-sonnet-4-6). Round 1 caught two real issues in an initial version (the full-save prune was a new regression; the inverse-wipe window) — both addressed by this revision (revert + mutateAccounts). Round 2: APPROVE ×3.

Verification

• Repro RED→GREEN for the resurrection bug; a concurrency test proves mutateAccounts preserves [a, b, c] where a naive stale-snapshot saveAccounts wipes it to [a, c]; a production-removal-path test proves the stateFromStorage rebuild drops removed state without the reverted prune.
• bun run build clean · bun run types clean · bun test 400 pass / 0 fail · bunx biome check src 0 warnings.

Discovered via a cross-tree review against the Anthropic sibling plugin; the sibling's tree keeps the config account list authoritative from the start and does not have the resurrection facet.

---

^{Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.}

---

Summary by cubic

Fixes account removal not persisting by making the incoming account list authoritative and adding a lock-held mutation path to avoid lost concurrent writes. Removed accounts no longer resurrect on reload, and concurrent additions are preserved.

• Bug Fixes

  • mergeStorageForSave now treats incoming.accounts as the source of truth, so removals stick; other top-level fields fall back to on-disk values if missing from incoming.
  • saveAccountState full saves no longer prune the state map; state is rebuilt from the authoritative account list. Scoped prunes remain limited to the requested ids.

• New Features

  • Added mutateAccounts for lock-held read-modify-write. It re-reads config and state under the lock, applies the mutation, and writes both files. All mutation paths (openai-account switch/remove/order/add, CLI login/remove, and main account/refresh-state writes) now use it to prevent TOCTOU wipes.

^{Written for commit 280f367. Summary will update on new commits.}

[cubic-dev-ai]

1 issue found across 7 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[iceteaSA]

Closing as superseded by upstream.

38df1ff (in v0.2.0) independently landed the same fix this PR implements — same diagnosis (the saveAccounts union-merge structurally can't express a removal/reorder, so /openai-account remove and order were silent no-ops) and the same solution: a mutateAccounts(mutate, path) lock-held read-modify-write that writes the mutator's result authoritatively while reading the freshest on-disk state under the lock (so a concurrent add is still preserved), with add/remove/order routed through it and scalar-field callers staying on saveAccounts. Tests match too (removal-not-resurrected, reorder-persists, concurrent-add-preserved).

Rebasing this branch onto current main would only re-assert work already there, and main's accounts.ts is now ahead of this branch (the token-rollback-on-timestamp-ties helpers from 8365cd8), so I'd rather not risk a conflict resolution that reverts those.

One piece here isn't yet covered upstream: a test asserting the state file is scrubbed of a dropped account's access/refresh secrets after the authoritative rewrite (the removal test currently checks the config file's id list, not state-file secrets-at-rest). I'll open that as a small focused test-only PR against current main.

Thanks for the audit train — converged independently on the same design.