feat(account): save multiple Vintage Story accounts and switch between them - #253
Conversation
…n them
RiftLauncher supported one active account. On a shared device, a
second player had to log out and enter their credentials again every
time they wanted to play. This lets several accounts be saved and
switched with no new login.
Data model: config.account (a single AccountPublicType | null)
becomes config.accounts (a list) and config.activeAccountId (the
playerUid of the one the next launch writes into clientsettings.json,
or null). playerUid is the key, not a new id field, since the launch
path already trusts it as the account's identity. Switching needs no
IPC channel or file write: clientsettings.json is written from
exactly one place, EXECUTE_GAME, right before launch, so a config
field change is the whole feature.
The secret store (src/ipc/accountStore.ts) moves from one encrypted
AccountSecrets blob to one encrypted map keyed by playerUid
(saveAccountSecrets, getAccountSecrets, removeAccountSecrets,
listStoredAccountIds), still one safeStorage-encrypted file, still
never exposing a session key to the renderer. LOGOUT is replaced by
REMOVE_ACCOUNT, since "log out" has no coherent meaning once there is
a list; logging in twice with the same account refreshes its entry in
place instead of duplicating it.
Multi-account introduces one real new hazard: if the active account's
secrets can't be read (a locked keyring), the settings file might
still hold a HOUSEMATE's session, written there directly by the game
on their own login. With one account that was harmless; with several
it means launching already signed in as someone else. EXECUTE_GAME
now clears a foreign session (a playeruid that isn't ours) before
launching with none of our own, narrow enough to never touch a file
that already shows nobody or shows us
(domain/account/clientSettings.ts: removeSessionFromClientSettings,
clearForeignClientSettingsSession).
Migration (schema 3 to 4, domain/config/migrations.ts) moves the old
single account into a one-entry list; a sibling impure step in
configManager.ts (migrateAccountStore, next to the existing
migrateLegacyAccount) re-keys the v1 secret store under that account's
playerUid so the session survives the upgrade, not just the account
record. Both config.json and the secret store back up their
pre-migration bytes once, to a rolling .bak file, before being
overwritten, on the industry-standard "never destroy the source until
the new path is proven" principle: a bug in either migration then
costs nothing, since the original is still sitting right next to it.
UI: SessionButton becomes a Listbox (matching LanguagesMenu's existing
pattern) when at least one account is saved, showing the active
player's name, every saved account, "Add another account", and
"Remove {name}" for the active one. The logged-out state (no accounts)
is unchanged.
Deferred, all additive later: importing an account from an existing
installation's clientsettings.json, a stored-session-missing
indicator, per-installation account binding, and the other locales
(en-US is the only one the repo's own i18n-parity check requires
staying in sync).
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
This is a careful piece of work and the reasoning in the description matches what the code actually does, which is not always true of PRs this size. The secret store, the switching semantics and the migration all hold up under reading, and I confirmed the migration end to end on a real build rather than only against fixtures. Two things stop me approving it as it stands, and the first is the one that matters.
1. Nothing pins the launch-time account choice, so the whole feature can silently regress
EXECUTE_GAME picks the account to launch as with
const account = config.accounts.find((candidate) => candidate.playerUid === config.activeAccountId) ?? nullI replaced that line with config.accounts[0] ?? null and ran the full suite. 129 files, 1570 tests, all green. The reason is that every fixture in tests/ipc/gameHandlers.test.ts writes a config with exactly one account, so a handler that ignores activeAccountId entirely is indistinguishable from a correct one. The promise this PR makes to a household, that you switch to Bob and the next launch signs Bob in, currently has no regression net at the handler level at all.
The same gap covers cross-account adoption. The domain guard itself is sound and it is pinned: sessionToAdopt still refuses to adopt when stringSettings.playeruid !== session.playerUid, and when I deleted that line the pre-existing #209 test "writes ours over a session belonging to another player, uid and all" went red immediately. adoptRefreshedSession is then keyed on account.playerUid, which is by construction the same uid the file just matched, so account A's refreshed session structurally cannot land in account B's store entry. I am satisfied there is no live bug here. What is missing is the test that keeps it that way once someone edits this handler in six months, because with a single-account fixture the adoption call site could be keyed on almost anything and stay green.
Two tests would close both holes, and they are cheap because the fixture helper already takes an accounts array:
- Two saved accounts,
activeAccountIdset to the second, noclientsettings.jsonyet. Assert theplayeruidwritten into the file is the second account's, not the first's. - Two saved accounts, active is B, and the settings file already holds A's
playeruidwith asessionkeythe launcher has never seen. Assert the outcome is a write and not an adoption, that A's key is gone from the file, and thatsaveAccountSecretswas either not called or called with B's uid.
2. The secret-store re-key gets exactly one chance, and a locked keyring burns it
migrateAccountStore is gated on the raw document still carrying config.account. The schema 3 to 4 step deletes that field, and the config is then written at schema 4 whether or not the re-key succeeded. adoptLegacySingleAccountSecrets calls assertSecureStorage(), which throws when the keyring is not available, and migrateAccountStore catches that, logs a warning and returns false.
Put together: an upgrading user whose keyring happens to be locked on that one launch gets a schema 4 config with their account listed and active, and a v1 account-secrets.json sitting on disk that nothing will ever read again. Next launch there is no config.account left, so the re-key never retries. getAccountSecrets sees version 1, treats it as unreadable and answers null, so they launch with no session, the game prompts them, and because the no-secrets branch only ever clears a foreign session the launcher never picks up what the game wrote either. Nothing is destroyed and the account record survives, but the session is gone for good and the only way out is logging in again through "Add another account", with nothing on screen explaining why. The description already lists a stored-session-missing indicator as deferred, which is fair, but this turns a transient keyring problem into a permanent one.
The narrow fix is to not let the config commit past schema 3 until the store is v2. Leaving config.account in place when the re-key throws would make the whole thing retry on the next launch for free, which is the same idempotent-by-construction argument the rest of the migration already rests on. Whatever shape you pick, please leave a test on it: I broke both migrated.activeAccountId = uid and migrated.accounts = uid ? [account] : [] and both went red, so the pure step is well covered, it is only this impure sibling's failure path that is not.
3. Smaller things, none of them blocking
An undecryptable store now costs every saved account its session rather than the one the old store held. readAccounts caches an empty map on any failure and the next saveAccountSecrets writes that map back over the file. Your comment in writeAccounts says exactly this and I think the call is right given the backup, but it is worth a follow-up issue rather than only a comment, because the blast radius genuinely grew.
backupConfigBeforeMigration only runs when migration.applied.length > 0, yet the save at the end also fires for hadLegacyAccountSecrets and reKeyedAccountStore on their own. Those two paths overwrite config.json with no backup, which is a small hole in a policy the PR otherwise states plainly.
listStoredAccountIds has no production caller, only its own tests. useAccount and AccountContext are now in the same position: SessionButton moved to useAccountList, and grepping src/ finds no other consumer, so an entire provider layer plus the activeAccount and account memos are being rendered for nobody but configContextSlices.test.tsx. Both can go, and the diff gets shorter for it.
What I checked and what came back clean
Secrets. AccountPublicType is the only account shape that crosses the preload bridge, and grepping src/preload and src/renderer for sessionKey, sessionSignature, mptoken, sessionkey and sessionsignature returns nothing at all. No log line in accountStore.ts or accountHandlers.ts carries a secret or an identity. The store is still one safeStorage blob, still chmod 600, still written temp-then-move. On the live run I grepped the app log and both files under Logs/ for the seeded uids, emails and session field names and got zero hits.
Switching. It is a config field and nothing else. There is no logout endpoint anywhere in src/ipc or src/domain, so switching or removing an account cannot sign anybody out on the service side, which was the point. A failed login raises a notification and returns without dispatching, so the currently active account is never touched by a bad add-account attempt. I confirmed the persistence live: seeded Alice and Bob with Alice active, opened the switcher, clicked Bob, and activeAccountId on disk went to uid-b with both accounts still listed. Breaking SET_ACTIVE_ACCOUNT into a no-op turned three tests red across the reducer and the DOM suite, so that half is properly pinned.
Migration, verified on a real build rather than only in fixtures. I built build:unpack, pointed the binary at an isolated config home holding a schema 3 config with one account, and launched it. It came out at schema 4 with accounts holding that account, activeAccountId set to its playerUid, config.pre-migration.bak.json written alongside, and every installation preserved. The switcher rendered the migrated player on the trigger, and opening it showed the account with its email, "Add another account" and "Remove LegacyPlayer". The remove dialog interpolated the name correctly and the add option opened the login form. Breaking the normalizeConfig dangling-id fallback to null and breaking the store re-key both went red.
The rest. Typed tagged verdicts on both new flows with exhaustive switches at the call site. Every new string has an en-US key and nothing in src/ still references the removed logout keys. The domain files import nothing from electron or the filesystem. REMOVE_ACCOUNT is the only channel change, it is gated by assertTrustedIpcSender and validates its argument through assertString. package.json is untouched, and Listbox, motion and clsx were all already in use.
Gates, run fresh after npm ci: typecheck passes on all three projects, lint:ci gives 0 errors and the same 15 warnings as dev, format:check passes, test:coverage is 129 files with 1570 passed and 2 skipped at 92.5 statements, 89.66 branches, 91.49 functions and 94 lines, all above the floor. Six mutations run in total, five caught, one survived, and that one is finding 1.
Happy to look again as soon as the two multi-account handler tests exist and the re-key gets a retry. The design underneath is right, it just needs the net drawn tight enough that the next person cannot quietly cut it.
Responds to Pixnop's review on PR #253. Nothing pinned that EXECUTE_GAME picks the account to launch as from config.activeAccountId, not the first saved account: every fixture in gameHandlers.test.ts saved exactly one account, so a handler that ignored activeAccountId entirely would have stayed green. Two new tests close that: one asserts a launch with two saved accounts signs in as the active one and writes that uid into clientsettings.json, not the first account in the list; the other asserts a session the game wrote for a different saved account is overwritten rather than adopted into the active account's store entry, pinning the call site finding 1 was actually about (the domain guard itself was already covered by clientSettings.test.ts). The secret-store re-key (migrateAccountStore) got exactly one chance: it read the uid to re-key by from the raw document's legacy `account` field, which the schema 3 to 4 step deletes, so a locked keyring on that one launch stranded the account at schema 4 with an unreadable v1 store and no way to retry. The literal fix the review sketched, holding the config back at schema 3 until the re-key succeeds, does not work here: normalizeConfig drops any field it does not recognize, account included, and the renderer saves the whole config back within seconds of launch, so a config held at schema 3 loses the account record outright on that very first save, which is worse than the strand. Instead, migrateAccountStore now falls back to the already-migrated config's activeAccountId when the raw document has no legacy account field, so it retries on every later launch until the v1 store is gone or re-keyed. adoptLegacySingleAccountSecrets is already a no-op once that is true, so the retry costs one failed read per launch and nothing else. A new configManager.test.ts case pins the retry across two simulated launches, including that the on-disk config actually reaches schema 4 with no account field left after the first, burned attempt. Also, from the same review: - backupConfigBeforeMigration's gate now covers hadLegacyAccountSecrets and reKeyedAccountStore too, not only a schema-version migration: both paths save a real document and neither took a backup before. - Removed listStoredAccountIds (zero production callers) and AccountContext/useAccount/the activeAccount and account memos in ConfigContext.tsx: SessionButton already reads useAccountList, and nothing else in src/ still calls useAccount. Verified both fixes with a mutation check: reverting the account pick to config.accounts[0] turns the two new gameHandlers tests red, and reverting migrateAccountStore's activeAccountId fallback turns the new retry test red. Both reverted after confirming.
|
Both blocking findings are addressed in Two new tests in On the re-key retry, I didn't take the literal fix you sketched. Holding the config back at schema 3 until the re-key succeeds doesn't work here: Also bundled in: The one item left open is the Local gates on the new head: typecheck, lint (0 errors, same 15 pre-existing warnings), format, and build:unpack all pass. |
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
Re-reviewed at 4b9f41d. Gates on that head: typecheck clean across all three projects, lint 0 errors with the same 15 warnings dev already carries, format check clean, and test:coverage green at 129 files, 1,571 passed, 2 skipped, with statements 92.5, branches 89.71, functions 91.46 and lines 94, all above the vitest.config.ts floors.
Finding 1, launch-time account choice: mostly resolved, one branch still open
The mutation that decided the last round now goes red. Replacing the account pick in EXECUTE_GAME with config.accounts[0] ?? null turns both new tests red ("signs in as the active account, not the first one saved" and "overwrites another player's refreshed session instead of adopting it into the active account"), 2 failed out of 1,573. That is the net the feature was missing and it is there now.
What is still unpinned is the second place the same mistake can be made. The adoption call site is keyed independently:
await adoptRefreshedSession(account.playerUid, written.secrets)Change that one argument to config.accounts[0]!.playerUid and the whole suite stays green: 129 files, 1,571 passed, 2 skipped, nothing red.
The consequence is not cosmetic. sessionToAdopt only returns a session when the settings file's playeruid matches the account being launched, so an adoption always carries the active account's freshly refreshed key. Storing it under the first saved account's id writes account B's live session into account A's store entry, and the next launch as A signs the player in as B. That is exactly the cross-account identity confusion the foreign-session guard further down this handler exists to prevent, arriving through the secret store instead of through clientsettings.json.
The two tests you added cover the pick and the foreign-overwrite path. Neither reaches the adopt branch with more than one account saved: the foreign-session fixture takes the write path rather than the adopt path, and the existing adoption test (the #204 one) saves exactly one account. One more case closes it. Two accounts, the second active, the settings file already holding a refreshed session under the active account's own uid, then assert saveAccountSecrets was called with the active uid and with nothing else. It should go red under the mutation above.
Finding 2, the burned migration: resolved
The activeAccountId fallback in migrateAccountStore is the right shape, and your argument against holding the document at schema 3 is correct: normalizeConfig would drop account on the renderer's first save and the record would be gone outright rather than merely stranded. That is worse than what it fixes, so the sketch in my last review was wrong and this is better.
I checked the retry does what it claims. Reverting the fallback to null turns "asks again on the next launch when a locked keyring burned the first attempt" red, 1 failed out of 1,573. Tracing it by hand agrees: adoptLegacySingleAccountSecrets calls assertSecureStorage after it has confirmed a v1 file but before it copies or writes anything, so a locked keyring throws with the v1 bytes untouched, and every later cold read of the config retries against the same uid. A v2 file short-circuits on the version check before it ever touches the keyring, so the standing retry costs one readJSON per launch and nothing else.
The non-blocking notes from last time
Undecryptable store costing every account. Moved. Filed as #259 and actually fixed in #261, reviewed separately.
Backup skipped on two paths. Resolved for config.json. mustSave now gates the backup on all three reasons and configManager.test.ts covers the two that previously slipped. The store-side equivalent is still open on this branch: adoptLegacySingleAccountSecrets copies the v1 file aside only after a successful decrypt, so a v1 store that decrypts wrong (a rebuilt keyring, a different backend) is overwritten by the next login with no snapshot at all. #261 closes exactly that case, which is another reason the two want to land together.
Dead exports. Resolved. listStoredAccountIds, AccountContext, useAccount and the two ConfigContext memos are gone, and nothing under src/ still refers to any of them.
One new thing, non-blocking
removeAccountSecrets reports success when it removed nothing. With the keyring locked, readAccounts yields an empty map, accounts.delete(accountId) returns false, and the function returns true on the "nothing to remove is not a failure" rule. The renderer then drops the account from config and shows "Removed Alice", while Alice's session key is still sitting in account-secrets.json under a uid that no longer appears anywhere in the config, so nothing can ever remove it again.
The old clearAccountSecrets removed the file outright and did not care whether the keyring was open, so this is a small regression in a credential-deletion path rather than a pre-existing quirk. Encrypted at rest and 0600, so the exposure is limited, but the launcher tells the player it did something it did not do. Telling "this account had no stored session" apart from "the store could not be read" would fix it, and #261 introduces the very flag that makes that distinction available.
Merge order
#261 is based on this branch and closes a hole this branch still has, so this one goes first. The sequencing note is on that PR.
Requesting changes for the adoption-key test. Everything else on the blocking list is closed, and the rest of the diff reads well: the schema 3 to 4 step, the normalizeConfig invariant that activeAccountId always names an entry or is null, and the Listbox rewrite of SessionButton all hold up.
Two conflicts, both where this branch and dev's atomic-write work touched the
same lines. accountStore.ts's writeAccounts drops its manual temp-then-move
block for dev's writeJsonAtomic(storePath, contents, { mode: 0o600 }), keeping
the multi-account payload and the cachedAccounts assignment. gameHandlers.test.ts
keeps both the CURRENT_CONFIG_SCHEMA and writeJsonAtomic imports.
Two tests needed updating as a consequence, not as a resolution. write-file-atomic
renames a sibling temp file over the destination, so a read-only destination file
no longer blocks a write: the foreign-session-clear test now chmods the
installation folder to 0o500, matching the sibling test above it. And
accountStore.test.ts's leftover-temp check filtered for a .tmp suffix that
write-file-atomic never writes, so it now asserts the store file is the only
entry in the folder.
Every other fixture in this file saves one account, which makes 'the active account' and 'the first saved account' the same uid, so a call site keyed on either stays green. Two accounts with the second active, and clientsettings.json holding a game-refreshed session under the active account's own uid, separates them: adoptRefreshedSession(config.accounts[0]!.playerUid, ...) turns this test red and nothing else in the file. PR #253 review, finding 1.
|
The adoption call site is now covered by a test that can tell the two keys apart. While confirming that I hit a second thing. This branch no longer merged into That merge broke one of my own tests, and the reason is not a merge accident. "resolves session-write-failed when a foreign session cannot be cleared" forced the clear to fail by On Gates on the merged head, local, Node 24.15.0, WSL: |
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
Approving at 35c4304.
The mutation that held this up now bites. Keying the adoption call site on config.accounts[0]!.playerUid instead of the launched account's uid turns "adopts the refreshed session under the active account's uid, not the first saved account's" red and nothing else, 1 failed out of 1,617. Last round that same edit went through the entire suite untouched. The fixture is the right shape for it: two accounts with the second active, and clientsettings.json holding a game-refreshed session under the active account's own uid, which is the only arrangement that pulls "the active account" apart from "the first saved account" on the adopt branch specifically. The assertion pins the whole saveAccountSecrets call list rather than just the uid, so a stray second call cannot hide inside it either.
The two mutations from the earlier rounds still hold. Replacing the account pick in EXECUTE_GAME with config.accounts[0] ?? null now fails three tests instead of two, since the new one catches that mistake as well. Reverting the migrateAccountStore fallback to null fails "asks again on the next launch when a locked keyring burned the first attempt", 1 out of 1,617.
The removeAccountSecrets note from last time is unchanged on this head, which is what I expected. It still returns true after deleting nothing from a map that is empty only because the store could not be read. It was never blocking here, and #261 fixes it in b4accd9 using the unreadable flag that PR introduces, so it disappears the moment the two land together, which is the plan anyway.
Gates on this head after a fresh npm ci: typecheck clean on all three projects, lint:ci at 0 errors with the same 15 warnings dev already carries, format:check clean, and test:coverage green at 136 files, 1,615 passed and 2 skipped, with statements 92.61, branches 89.88, functions 91.99 and lines 94.03, all above the vitest.config.ts floors of 87, 85, 85 and 89.
#261 is still based on this branch and closes a hole this one still has, so this goes in first and #261 follows onto dev right behind it.
2 participants
Summary
RiftLauncher supported one active Vintage Story account. On a shared device, a second player had to log out and enter their credentials again every time they wanted to play. This lets several accounts be saved and switched between with no new login, matching the ask in #238.
What changed
config.account(a singleAccountPublicType | null) becomesconfig.accounts(a list) plusconfig.activeAccountId(theplayerUidof the one the next launch writes intoclientsettings.json, ornull).playerUidis the key rather than a new id field, since the launch path already trusts it as the account's identity (sessionToAdoptinclientSettings.ts). Switching needed no new IPC channel or file write once I traced the launch path:clientsettings.jsonis written from exactly one place,EXECUTE_GAME, right before launch, so switching is a config field change that the next launch simply reads.The secret store (
src/ipc/accountStore.ts) moves from onesafeStorage-encryptedAccountSecretsblob to one encrypted map keyed byplayerUid. It's still a single file, still never exposes a session key to the renderer.LOGOUTis replaced byREMOVE_ACCOUNT, since "log out" has no coherent meaning once there's a list; logging into an already-saved account refreshes its entry in place rather than duplicating it.The one new hazard, and the guard for it
With one account, a settings file the launcher couldn't update was harmless: whatever stale session it held was that account's own. With more than one, it can be a housemate's, since the game writes their session into
clientsettings.jsondirectly on their own successful login. If the active account's secrets can't be read (a locked keyring), launching without checking could start the game already signed in as somebody else.EXECUTE_GAMEnow clears a foreign session (aplayeruidthat isn't ours) before launching with none of our own. It's narrow by construction: it never touches a file that already shows nobody, or already shows us. Two new functions indomain/account/clientSettings.ts:removeSessionFromClientSettings(strips exactly the eight session keys, leaves everything else) andclearForeignClientSettingsSession(reads first, only clears when theplayeruiddemonstrably belongs to someone else).Migration
Schema 3 to 4 (
domain/config/migrations.ts) moves the old single account into a one-entry list. A sibling impure step inconfigManager.ts(migrateAccountStore, next to the existingmigrateLegacyAccount) re-keys the v1 secret store under that account'splayerUid, so the session survives the upgrade, not just the account record.Both
config.jsonand the secret store back up their pre-migration bytes once, to a rolling.bakfile, before being overwritten. I looked at industry practice for this before implementing it: the consistent recommendation across schema-migration and rollback-strategy sources is a verified backup taken before any migration runs, and a non-destructive default (never delete the old shape until the new one is proven). A bug in either migration then costs nothing, since the original bytes are sitting right next to the result. I considered per-account keychain items (a real security improvement other multi-account tools use) instead of onesafeStorage-encrypted blob, and didn't pursue it:safeStorageuses one process-bound key regardless of how the ciphertext is split, so splitting the blob buys nothing here without also bringing back a native per-item keychain dependency (keytar, which Electron's own docs moved away from). Noted as a real but separate architectural question, not something to fold into this PR.UI
SessionButtonbecomes aListbox(matchingLanguagesMenu's existing dropdown pattern) once at least one account is saved: it shows the active player's name, every saved account, "Add another account", and "Remove {name}" for the active one. The logged-out state (no accounts saved) is unchanged from today.Scope
I looked for a smaller slice to split this into and didn't find one that leaves the app in a consistent state at every step: the migration is only safe with its re-key and backup, and "add and switch" isn't complete without "remove" once there's a list (today's single logout button has no coherent replacement otherwise). Deferred, all additive later: importing an account from an existing installation's
clientsettings.json, a stored-session-missing indicator, per-installation account binding, and the other locale files (the repo's own i18n-parity check only requires en-US to stay in sync; other locales lagging is documented as expected).Update: response to review
Pixnop confirmed the design, the secret store, the switching semantics, and the migration on a real end-to-end build, then found two blocking gaps and three non-blocking notes.
Nothing pinned the launch-time account choice, so the feature had no regression net. Every fixture in
tests/ipc/gameHandlers.test.tssaved exactly one account, soEXECUTE_GAMEpickingconfig.accounts.find(candidate => candidate.playerUid === config.activeAccountId)versus justconfig.accounts[0]was indistinguishable at the test level: swapping in the latter left all 1,570 tests green. Two new tests close this: one saves two accounts with the second active and asserts the launch writes the second account'splayeruidintoclientsettings.jsonand reads its secrets, not the first account's; the other saves two accounts, seeds the settings file with a session belonging to the non-active one, and asserts the launch overwrites it (a write, not an adoption) rather than carrying a stranger's refreshed session into the active account's store entry. I verified both are load-bearing: reverting the account-picking line toconfig.accounts[0] ?? nullturns both red.The secret-store re-key got exactly one chance.
migrateAccountStoreread theplayerUidto re-key by from the raw document's legacyaccountfield, and the schema 3 to 4 step deletes that field the same pass. A locked keyring on the one launch that first sees a legacy account meantadoptLegacySingleAccountSecretsthrew, the config still committed to schema 4 regardless, and there was noaccountfield left on any later launch to retry from: the session was gone for good, recoverable only by logging in again with nothing on screen explaining why.The review's own sketch was to hold the config back at schema 3 until the re-key succeeds. That does not work here:
normalizeConfigdrops any field it does not recognize,accountincluded, and the renderer saves the whole config back within seconds of launch (ConfigContext.tsx's save effect), so a config held at schema 3 loses the account record outright on that very first save, which is strictly worse than the strand it would be fixing. Instead,migrateAccountStorenow falls back to the already-migrated config'sactiveAccountIdwhen the raw document carries no legacyaccountfield, so it retries on every later launch until the v1 store is gone or successfully re-keyed.adoptLegacySingleAccountSecretsis already a no-op once that is true, so the retry costs one failed read per launch, nothing else. A newconfigManager.test.tscase pins the retry across two simulated launches, including that the on-disk config actually reaches schema 4 with noaccountfield left after the first, burned attempt, and that the second launch retries with the same uid.Two smaller things from the same review, both bundled in:
backupConfigBeforeMigrationonly ran when a schema migration itself applied, sohadLegacyAccountSecretsandreKeyedAccountStorefiring on their own overwroteconfig.jsonwith no backup. Its gate now covers all three, for free from the same refactor that fixed the retry.listStoredAccountIdshad zero production callers, andAccountContext/useAccount/theactiveAccountandaccountmemos inConfigContext.tsxwere rendered for nobody onceSessionButtonmoved touseAccountList. All removed;configContextSlices.test.tsxnow subscribes its whole-config probe throughuseAccountListinstead.One item left as a follow-up rather than fixed here: an undecryptable secret store now costs every saved account its session in one shot, not just the one account the old single-account store held, since
readAccountscaches an empty map on any read failure and the next save writes it back over the file. The comment inwriteAccountsalready explains why refusing to write is worse (a permanently broken save path), so there is no cheap change to bundle in; a real fix needs a decision about that refusal policy, tracked as #259.Regression proof
tests/ipc/accountStore.test.ts: two accounts round-trip independently, saving or removing one leaves the other untouched, the last account's removal deletes the file,__proto__as an id is stored as ordinary data (the store is aMap, not an object),adoptLegacySingleAccountSecretsre-keys a v1 file and backs it up once.tests/domain/account/credentials.test.ts:parseStoredSecretsByIdreads a well-formed payload, drops individual unreadable entries without losing the rest, first-wins on a duplicate id.tests/domain/account/clientSettings.test.ts:removeSessionFromClientSettingsstrips exactly the eight session keys;clearForeignClientSettingsSessionclears a genuinely foreign session and leaves our own, an empty, or an unreadable file alone.tests/domain/config/migrations.test.tsandtests/ipc/configManager.test.ts: the schema 3→4 step, the account-store re-key including its retry-on-failure path, and the config.json/secret-store backups (now including the two previously ungated paths), including that a second migration does not overwrite the first backup.tests/ipc/gameHandlers.test.ts: the wrong-player guard clears a housemate's session before launching, leaves our own or an empty file alone, resolvessession-write-failedwhen a foreign session can't be cleared, and (new) signs in as the active account rather than the first saved one, and overwrites rather than adopts a foreign session when a second account is active.tests/renderer/configReducer.test.tsandtests/renderer-dom/sessionButtonSwitchAccount.test.tsx:ADD_ACCOUNT/REMOVE_ACCOUNT/SET_ACTIVE_ACCOUNT, and the switcher UI end to end (switching persists the new active id, add opens the login form, remove confirms and calls the IPC channel, a failed remove leaves the account in place with an error toast).Testing
npm run typecheck: passes (all three projects).npm run lint:ci: 0 errors, 15 pre-existing warnings (unchanged fromdev).npm run format:check: passes.npm run test:coverage: 129 files, 1,571 passed, 2 skipped. Coverage 92.48% statements, 89.67% branches, 91.46% functions, 94% lines, all above thevitest.config.tsfloor.npm run build:unpack: passes on Linux x64.git diff --check: passes.Verified both fixes with a mutation check before pushing: reverting the account pick to
config.accounts[0] ?? nullturns the two newgameHandlers.test.tstests red; revertingmigrateAccountStore'sactiveAccountIdfallback turns the newconfigManager.test.tsretry test red. Both reverted after confirming.Limitations
Not verified:
safeStoragebehaviour against a real OS keychain (DPAPI, macOS Keychain), and an actual upgrade-in-place migration from an installed prior build. The migration was exercised against fixture files and a fakedsafeStorage, never a real keyring. This machine is Linux only, so Windows/macOS builds were not produced or run here. The account-store blast-radius item above is tracked as #259, not fixed here.Related issues
Fixes #238. Follow-up for the account-store blast-radius item filed as #259.