Credentials: recover, explain, defer, and say what is stored - #188
Merged
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.
`init_from_dir` read the OS keychain from inside Tauri's `setup`, before
`app.run()` and before the first `std::thread::spawn` sixty lines below —
so on the main thread, ahead of any window.
A warm read is ~10ms and unremarkable. A login keychain that LOCKS is
not: the read raises a modal unlock prompt with no window behind it yet.
And the launches that need a credential at all are the minority — an
agent with its own auth (Claude, Codex) or none (Ollama) never asks.
The file already knew. One line above the call:
// A plain JSON read, so safe inline (unlike the keychain).
and then it read the keychain inline.
Boot now only records the directory. Two things had to move with it,
because deleting the eager call alone would have broken them quietly:
* `migrate_legacy_entries` ran ONLY on the eager path. Deferring without
moving it would have stranded every key the old one-entry-per-harness
scheme wrote, with nothing to show it had happened.
* `secret_for` and `Credential::read` never loaded at all — they took the
guard and read whatever eager init had left. Lazily they returned
nothing for ever.
Both now go through `loaded_for_read`, which loads and migrates, and
returns `None` on a lost key rather than recovering: recovery mints a key
and moves a file, and a read must do neither. `loaded_for_write` is the
one that recovers, because there the user has said what the new store is
for.
Measured, not assumed. With a `write_master_key` backtrace and no master
key present, the app boots and sits for thirty seconds with its window up
and mints nothing:
mints: 0
master key: still absent
An earlier reading of mine said otherwise. That was a key minted moments
after boot by the UI asking for credential status on a restored
OpenRouter chat — need-driven, after the window exists, which is the
behaviour wanted. Not `setup`.
Also drops a comment referring to `export_all`, which no longer exists.
Reported from the screen: the key was stored and the panel still read like an empty form waiting to be filled in. Three things made a configured key indistinguishable from an unconfigured one at rest: * The only success signal was an InlineNotification that clears itself after four seconds. Look away and there is nothing. * What remained was helper text phrased as an INSTRUCTION — "A key is saved. Paste a new one to replace it." — under an empty field. The first clause is the status and it is buried in the middle of a sentence telling you to type. * The primary button still said "Save key", which is what a form says when it has not been used. The resting state now says it plainly, with the same green check the default-Markdown-app section already uses, and the button says "Replace key" once there is something to replace. It also closes a real footgun found while testing this. "Save key" was enabled with an empty field, and submitting empty does not fail — it stores "", which CLEARS the stored key, and reports success exactly as saving one does. That is one careless click on a primary button from losing a key silently. It cost me a confusing test run earlier, where a click that missed the field "saved" nothing and looked like it had worked. The button is now disabled until something is typed, and clearing is its own explicit "Remove key" action. The tests are the resting states rather than the mechanics, since the resting state was the defect. Confirmed to bite: restoring the always-enabled button and dropping the status line fails three of the five.
"A key is saved" answers the wrong question. It tells you the slot is full, which you can also infer from the button; it does not tell you whether what is in there is the key you meant. A rotated key and a stale one look identical. The status line now reads `Saved key sk-or…9f2c`, the shape every provider uses to list keys it will not show again. Deliberately NOT a reveal. Every issuer of these keys — OpenAI, Stripe, GitHub, AWS — shows the value once and never again, and Compose has a sharper reason than convention: the encrypted store exists so a model with a `bash` tool cannot reach the key, and painting it on screen gives back what the store was protecting. Editing is worse still, since a secret you cannot see is not one you can edit; replace-in-full is the only coherent gesture, and that already works. `hint_for` is a pure function with the safety rule as a test rather than a comment: below sixteen characters it emits bullets and no characters at all, because head-plus-tail of a short secret is most of the secret. Another test pins that a hint never exposes more than a quarter of what it describes. The frontend treats the field as optional, so a status without one — a short secret, or a backend that predates this — falls back to the plain wording rather than rendering an empty code span. Also fixes an order-of-operations mistake of my own: I ran `pnpm typecheck` before extending the tests, so the mock's inferred type never saw `hint`, and `pnpm build` failed on the very thing I had just "verified".
Regression from deferring the load, reported from the screen: Settings →
AI agents sat on its loading skeletons for ever.
Opening the store migrates the old per-harness entries, and to know which
entries to look for it asks the registry for every credential spec. That
builds each harness — and building the OpenRouter one reads its key
(`registry.rs`), which reaches for the store lock the loader is already
holding. `std::sync::Mutex` is not reentrant, so the thread waits on
itself.
Nothing errors. The list simply never arrives.
The previous code called `migrate_legacy_entries` BEFORE taking the lock,
so the same read found `None` and moved on. Deferring the load put the
migration inside the lock and turned that into a deadlock.
A thread-local flag restores the old outcome explicitly: while a thread
is building the store, a credential read declines before touching the
mutex. That is also what it means — the specs are wanted for their NAMES,
and mid-build there is nothing to report.
The failure is a hang, so the test cannot assert its way to it: the work
runs on another thread against a deadline. Removing the guard fails it
cleanly instead of wedging the suite —
a credential read while opening must return, not deadlock: Timeout
Worth recording how this got through. I checked that boot no longer
touched the keychain, which was true and proved nothing about what
happens when something finally opens it. I verified the property I
intended to change and never exercised the path I had rewritten.
This was referenced Aug 25, 2026
tosinamuda
added a commit
that referenced
this pull request
Aug 26, 2026
Reported as a macOS password prompt on every launch. Deferring the read in `setup` was not enough, because two things reached for a key moments later — which from the user's seat is the same thing. **Listing agents read every agent's key.** `harness_list` builds the whole registry to enumerate it, and building OpenRouter resolves its stored key. Nothing about listing needs one: `credentialRequired` is a CAPABILITY — "does this agent take a key" — and the picker asks for stored state separately. `Secrets::Skip` builds identity and capability only. **The boot readiness probe asked for auth state.** It wants `installed`, which is what gates the composer (`workspaceModel.rs:421`); auth state is carried in the same struct and, for a hosted provider, costs a keychain read to fill in. Measured before deciding: `installed` is true either way, and nothing in the front end reads `authConfigured` at all. So `harness_readiness` now takes `with_auth`, default false. The intent sits at the call site: Settings, which displays agent status, opts in; boot and the default-agent probe do not. Migration takes `Skip` too. It wants each spec's NAMES, and resolving keys there was both a reason to open the keychain and the source of the deadlock fixed in #188 — the guard for which is now defence in depth rather than load-bearing. Verified with a CONTROLLED measurement, because an earlier claim of mine rested on a probe I had not shown could fire: CONTROL: 1 (boot reaches credential setup; stderr works) KEYCHAIN OPEN: 0 (the store is never opened during launch) `compose_registry()` went with it: every caller now states which mode it wants, so the convenient default that hid the question is gone.
This was referenced Aug 26, 2026
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.
Started as "saving an OpenRouter key fails" and ended up covering the whole credential path. Seven commits, each with its own reason to exist.
The original failure
credentials.encexisted while its master key did not, so the store openedKeyLost— a branch that builds no store, so there was nothing to save into.Decision::Mintonly fires when there is no file, so the state could not resolve itself and every retry failed identically. The only escape was Reset all data: erasing every workspace, conversation and setting to remove one 216-byte unreadable file.The save path already said what it should do —
— and then refused the write. It now 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 answered rather than ignored: the file is renamed, never replaced, so a key that reappears can still open it.Checked against the reference rather than assumed. VS Code stores one Electron
safeStoragekey in the keychain ("Code Safe Storage") with the ciphertext beside it — the same shape as this — and when its key goes missing it reports the failure and you sign in again. It does not wedge. This matches known-good behaviour.Why nobody could see what was wrong
Tauri's
invokerejects with a plainString, not anError, soerr instanceof Error ? err.message : fallbackalways took the fallback. A locked keychain, a lost master key, a full disk — all arrived as "Could not save the OpenRouter API key".errorMessage()already existed for exactly this, with a comment explaining it; 41 sites still hand-roll the check. This fixes the one that reached a user.The keychain no longer opens at boot
It was read inside
setup, beforeapp.run()and sixty lines before the firstthread::spawn— on the main thread, ahead of any window. ~10ms warm, but a login keychain that locks turns it into a modal unlock prompt with nothing behind it. The launches needing a credential are the minority; agents with their own auth or none never ask.Two things had to move with it, because deleting the eager call alone breaks them silently:
migrate_legacy_entriesran only on the eager path, andsecret_for/Credential::readnever loaded at all.A regression I introduced, and the user caught
Moving migration inside the lock deadlocked. Migration asks the registry for every credential spec; that builds each harness; building OpenRouter reads its key; that reaches for the lock the loader already holds.
std::sync::Mutexis not reentrant. Nothing errors — Settings just sits on its skeletons for ever.A thread-local flag restores what the old code did implicitly: while a thread is building the store, a credential read declines before touching the mutex. The specs are wanted for their names.
A saved key now looks saved
The only success signal was a notification that cleared after four seconds; what remained was helper text phrased as an instruction under an empty field, and a primary button still saying "Save key". It now says
Saved key sk-or…9f2cwith a check, and "Replace key" once there is something to replace.That also closes a real footgun found while testing: "Save key" was enabled with an empty field, and submitting empty stores
"", which CLEARS the key — reporting success exactly as saving one does. Removing is now its own deliberate action.The hint is deliberately not a reveal. Every issuer of these keys shows the value once and never again, and Compose has a sharper reason: the encrypted store exists so a model with a
bashtool cannot reach the key.Testing
253 Rust, 558 frontend, clippy clean. Every new test was confirmed to fail against the behaviour it describes before being kept — including the deadlock, whose failure mode is a hang, so it runs on another thread against a deadline:
Verified through the UI on a real build, twice: the wedged state reproduced from the user's own orphaned file, recovered, saved, and the unreadable ciphertext preserved byte-for-byte.