Stop discarding the reason a key could not be saved - #187
Closed
tosinamuda wants to merge 3 commits into
Closed
Conversation
Saving an OpenRouter key failed with:
Setup error Could not save the OpenRouter API key
The backend had said something far better:
Could not unlock the credential store. Reset it in Settings.
That message was thrown away one line from the screen. Tauri's `invoke`
rejects with a plain String, not an Error, so
err instanceof Error ? err.message : `Could not save the ${name} API key`
always takes the fallback for an IPC failure. Every possible cause — a
locked keychain, a lost master key, a full disk — arrived as the same
sentence, and that sentence contains no next step.
`errorMessage()` already exists for exactly this, and its comment already
says why:
Tauri `invoke` rejects with a plain String, not an Error — surface it
instead of masking the real backend reason behind the generic fallback.
So this is not a new lesson, it is an unapplied one. 41 sites still hand-
roll the `instanceof Error` test; this fixes the one that demonstrably
bit a user and leaves the sweep to its own change.
The helper had no tests at all, which is how the lesson stayed learnable
in a comment and lost in practice. It has some now, including the string
case that regressed.
Saving an API key could fail permanently. If `credentials.enc` existed
while the master key that encrypts it did not, `load` returned `KeyLost`,
which builds no store — so there was nothing to save into, and every
retry failed the same way. `Decision::Mint` only fires when there is no
file at all, so the state could not resolve itself.
The only escape was "Reset all data": erasing every workspace,
conversation and setting to remove one 216-byte file that nothing can
read. A user hit this today.
The save path already said what it should do:
// A lost master key leaves no store. Saving a key is the recovery, so
// build a fresh one rather than refusing the write.
and then refused the write. This does what the comment promises.
`decide`'s objection to minting over an existing file — "it would
overwrite the only key that could ever open it" — is right, and
`set_aside` answers it rather than ignoring it: the file is RENAMED, not
replaced. If the key ever reappears, from a restored keychain or a
keychain sync that was only late, the ciphertext is still on disk.
Refusing to write protected bytes that were already unreadable, at the
cost of a permanently unusable feature.
Checked against the reference implementation rather than assumed. VS Code
stores one Electron safeStorage key in the OS keychain ("Code Safe
Storage") and the ciphertext beside it — the same shape as this. When its
key goes missing it reports the decrypt failure and the user signs in
again; it does not wedge. So this is matching known-good behaviour, not
inventing semantics.
`set_aside` is split out because `recover` mints a master key and so
cannot run in a test without writing to the developer's real keychain.
The extracted half is the one that could destroy data, and it is the half
the objection above is really about.
The previous commit claimed a lost master key was now recoverable and
tested only the half that does not touch the OS vault. That is the wrong
half to stop at: `recover` mints a key, and minting was the entire
question.
`keyring_service()` is a `cfg(test)` seam so a test can point the master
key at a throwaway service. Without it the choice was between never
exercising recovery and writing to the developer's own
`ai.latentic.compose` entry, and a test that can corrupt the machine it
runs on is not a test anyone will keep running. `TestKeychain` deletes
what it created on drop. Production has no seam: `keyring_service()`
under `cfg(not(test))` returns the constant.
The new case walks the user's actual path — a store encrypted under a key
the keychain has never heard of, asserted to be `KeyLost` first so the
premise cannot rot, then recovered, saved into, and REOPENED to prove the
minted key persisted, then checked that the unreadable ciphertext is
still on disk.
Confirmed to bite. Mutating `set_aside` to delete instead of rename fails
both cases:
assertion `left == right` failed: the unreadable store must be preserved
Still untested: the one-line wiring in `credentials.rs::store()`, which
reaches recovery through a process-global store and config dir, and the
path through the UI. Named rather than implied.
Contributor
Author
|
Superseded by #188, which landed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Saving an OpenRouter key failed with:
The backend had said something far more useful:
That message was thrown away one line from the screen.
Why
Tauri's
invokerejects with a plainString, not anError. So this:always takes the fallback for an IPC failure. A locked keychain, a lost master key, a full disk — every cause arrives as the same sentence, and that sentence contains no next step. I spent a while diagnosing a keychain problem that the app already knew the answer to.
This is an unapplied lesson, not a new one
errorMessage()already exists insrc/app/store/internals.ts, and its comment already says why:Someone hit this before and wrote the fix. 41 sites still hand-roll the
instanceof Errortest. This changes the one that demonstrably reached a user and leaves the sweep to its own change — some of those sites never cross IPC, and a blind find-and-replace would be worse than the bug.The helper had no tests
Zero. Which is how the lesson stayed learnable in a comment and lost in practice. It has some now, including the string case that regressed, plus the blank-message and non-Error fallbacks.
551 frontend tests pass.
The underlying state, for the record
The user's
credentials.encexisted while its master key did not, soSecretStore::loadreturnedKeyLost— correctly, since the file is undecryptable without the key. ButKeyLostbuilds no store, andDecision::Mintonly fires when there is no file, so saving a new key could never succeed. The advice "re-enter them" cannot work until the orphaned file is gone.The only in-app recovery is Reset all data, which erases every workspace, conversation and setting to delete one 216-byte unreadable file. That deserves a targeted "reset credentials" instead — filed separately rather than smuggled in here.