From ad4f591021d58b7039ba339bc40c0d1fca24f57a Mon Sep 17 00:00:00 2001 From: Tosin Amuda Date: Tue, 25 Aug 2026 20:16:34 +0200 Subject: [PATCH 1/7] fix(settings): stop discarding the reason a key could not be saved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/app/store/internals.test.ts | 26 ++++++++++++++++++- src/features/settings/agentConfigControls.tsx | 3 ++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/app/store/internals.test.ts b/src/app/store/internals.test.ts index 2b753ca..44b1a3e 100644 --- a/src/app/store/internals.test.ts +++ b/src/app/store/internals.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import type { Workspace } from "../workspaceModel"; -import { nextUntitledPath } from "./internals"; +import { errorMessage, nextUntitledPath } from "./internals"; // nextUntitledPath only reads `files` (their relative paths) + `openFilePaths`, // so a minimal stand-in exercises the real collision logic without a full @@ -40,3 +40,27 @@ describe("nextUntitledPath", () => { expect(nextUntitledPath(ws([]), "Projects/")).toBe("Projects/untitled-1.md"); }); }); + +describe("errorMessage", () => { + it("surfaces a plain string, which is how Tauri rejects", () => { + // The case that matters. `invoke` rejects with a String, not an Error, so + // the common `err instanceof Error ? err.message : fallback` throws away + // whatever the backend said. That is how "Could not unlock the credential + // store. Reset it in Settings." reached a user as "Could not save the + // OpenRouter API key" — a sentence with no next step in it. + expect(errorMessage("Could not unlock the credential store.", "fallback")).toBe( + "Could not unlock the credential store.", + ); + }); + + it("uses an Error's message", () => { + expect(errorMessage(new Error("disk is full"), "fallback")).toBe("disk is full"); + }); + + it("falls back when there is nothing to say", () => { + expect(errorMessage(new Error(" "), "fallback")).toBe("fallback"); + expect(errorMessage(" ", "fallback")).toBe("fallback"); + expect(errorMessage(undefined, "fallback")).toBe("fallback"); + expect(errorMessage({ code: 500 }, "fallback")).toBe("fallback"); + }); +}); diff --git a/src/features/settings/agentConfigControls.tsx b/src/features/settings/agentConfigControls.tsx index 1dcb5b0..94c6cdd 100644 --- a/src/features/settings/agentConfigControls.tsx +++ b/src/features/settings/agentConfigControls.tsx @@ -10,6 +10,7 @@ import { type HarnessRuntimeVerification, } from "../../lib/ipc/harnessClient"; import { ModelPicker } from "./ModelPicker"; +import { errorMessage } from "../../app/store/internals"; /** * The per-agent configuration controls shared by the Settings detail screen. @@ -74,7 +75,7 @@ export function HarnessCredentialForm({ harnessId, name }: { harnessId: string; setSaved(true); window.setTimeout(() => setSaved(false), 4000); } catch (err) { - setError(err instanceof Error ? err.message : `Could not save the ${name} API key`); + setError(errorMessage(err, `Could not save the ${name} API key`)); } finally { setSaving(false); } From b22ca9575b51e887f865113ca66a29be91d3cb7c Mon Sep 17 00:00:00 2001 From: Tosin Amuda Date: Tue, 25 Aug 2026 20:22:13 +0200 Subject: [PATCH 2/7] fix(credentials): a lost master key is recoverable, not a dead end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src-tauri/src/harness/credentials.rs | 9 ++-- src-tauri/src/harness/secret_store.rs | 67 +++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 3 deletions(-) diff --git a/src-tauri/src/harness/credentials.rs b/src-tauri/src/harness/credentials.rs index beb3025..20ca11b 100644 --- a/src-tauri/src/harness/credentials.rs +++ b/src-tauri/src/harness/credentials.rs @@ -177,9 +177,12 @@ impl Credential { let dir = CONFIG_DIR.get().ok_or("credential store is not initialised")?; match SecretStore::load(dir)? { Opened::Ready(fresh) => *guard = Some(fresh), - Opened::KeyLost => { - return Err("Could not unlock the credential store. Reset it in Settings.".to_owned()) - } + // Do what the comment above promises. The file cannot be + // opened by any key that exists, and the user is in the middle + // of saying "save this key" — refusing left them permanently + // unable to, with "Reset it in Settings" (which erases every + // workspace and conversation) as the only way out. + Opened::KeyLost => *guard = Some(SecretStore::recover(dir)?), } } let store = guard.as_mut().ok_or("credential store is not initialised")?; diff --git a/src-tauri/src/harness/secret_store.rs b/src-tauri/src/harness/secret_store.rs index 4687708..c3362b8 100644 --- a/src-tauri/src/harness/secret_store.rs +++ b/src-tauri/src/harness/secret_store.rs @@ -25,6 +25,7 @@ use std::collections::BTreeMap; use std::path::{Path, PathBuf}; +use std::time::{SystemTime, UNIX_EPOCH}; use base64::Engine as _; use chacha20poly1305::aead::{Aead, KeyInit, OsRng}; @@ -106,6 +107,26 @@ impl SecretStore { }) } + /// Start over after {@link Opened::KeyLost}: the file cannot be opened by + /// any key that exists, so a fresh one is minted and the store begins empty. + /// + /// The unreadable file is RENAMED, never overwritten. `decide` refuses to + /// mint over an existing file on the grounds that it would destroy the only + /// key that could open it — the concern is right, and moving the file aside + /// answers it: if the key ever reappears (a keychain restored from backup, + /// iCloud sync catching up) the ciphertext is still on disk to recover by + /// hand. What it must not do is leave the user unable to save anything, + /// which is what refusing did. + pub fn recover(config_dir: &Path) -> Result { + let path = config_dir.join(STORE_FILE); + if let Some(kept) = set_aside(config_dir)? { + eprintln!("credential store could not be unlocked; kept at {}", kept.display()); + } + let key = new_master_key(); + write_master_key(&key)?; + Ok(Self { path, key, secrets: BTreeMap::new() }) + } + pub fn get(&self, id: &str) -> Option<&str> { self.secrets.get(id).map(String::as_str) } @@ -227,6 +248,25 @@ fn decrypt(key: &[u8; KEY_LEN], envelope: &Envelope) -> Result Result, String> { + let path = config_dir.join(STORE_FILE); + if !path.exists() { + return Ok(None); + } + let stamp = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_secs()) + .unwrap_or(0); + let kept = config_dir.join(format!("{STORE_FILE}.unreadable-{stamp}")); + std::fs::rename(&path, &kept) + .map_err(|e| format!("could not set the unreadable store aside: {e}"))?; + Ok(Some(kept)) +} + fn new_master_key() -> [u8; KEY_LEN] { let mut key = [0u8; KEY_LEN]; OsRng.fill_bytes(&mut key); @@ -290,6 +330,33 @@ mod tests { SecretStore { path: dir.join(STORE_FILE), key, secrets: BTreeMap::new() } } + /// The safety property of recovery, testable without the OS vault: the + /// unreadable file is preserved, never destroyed. `recover` itself mints a + /// master key and so cannot run in a test — but this is the half that could + /// lose someone's data, and the half `decide`'s "minting would overwrite the + /// only key that could ever open it" objection is really about. + #[test] + fn set_aside_keeps_the_unreadable_bytes() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join(STORE_FILE); + std::fs::write(&path, b"unreadable ciphertext").expect("write"); + + let kept = set_aside(dir.path()).expect("no error").expect("a file was moved"); + + assert!(!path.exists(), "the store must be out of the way so a fresh one can mint"); + assert_eq!( + std::fs::read(&kept).expect("kept file"), + b"unreadable ciphertext", + "the bytes must survive — a key that reappears can still open them", + ); + } + + #[test] + fn set_aside_is_a_no_op_with_nothing_to_keep() { + let dir = tempfile::tempdir().expect("tempdir"); + assert!(set_aside(dir.path()).expect("no error").is_none()); + } + fn envelope_for(key: [u8; KEY_LEN], pairs: &[(&str, &str)]) -> Envelope { let secrets: BTreeMap = pairs.iter().map(|(k, v)| ((*k).to_owned(), (*v).to_owned())).collect(); From c8f709df03e9e64a52e2d79a30ea540ffdc3db06 Mon Sep 17 00:00:00 2001 From: Tosin Amuda Date: Tue, 25 Aug 2026 20:35:54 +0200 Subject: [PATCH 3/7] test(credentials): prove the recovery, against a real keychain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src-tauri/src/harness/secret_store.rs | 93 ++++++++++++++++++++++++++- 1 file changed, 92 insertions(+), 1 deletion(-) diff --git a/src-tauri/src/harness/secret_store.rs b/src-tauri/src/harness/secret_store.rs index c3362b8..c08c1fb 100644 --- a/src-tauri/src/harness/secret_store.rs +++ b/src-tauri/src/harness/secret_store.rs @@ -273,8 +273,53 @@ fn new_master_key() -> [u8; KEY_LEN] { key } +/// The keychain service the master key lives under. +/// +/// A constant in the app. Under `cfg(test)` it can be pointed at a throwaway +/// name, because the alternative is that `recover` — the whole recovery path — +/// stays untestable: exercising it for real means minting a master key, and a +/// test must never write to the developer's own `ai.latentic.compose` entry. +#[cfg(not(test))] +fn keyring_service() -> String { + KEYRING_SERVICE.to_owned() +} + +#[cfg(test)] +thread_local! { + static TEST_SERVICE: std::cell::RefCell> = + const { std::cell::RefCell::new(None) }; +} + +#[cfg(test)] +fn keyring_service() -> String { + TEST_SERVICE.with(|s| s.borrow().clone()).unwrap_or_else(|| KEYRING_SERVICE.to_owned()) +} + +/// Point the master key at a throwaway service for the current test, and delete +/// whatever it left behind when the guard drops. +#[cfg(test)] +struct TestKeychain(String); + +#[cfg(test)] +impl TestKeychain { + fn new(name: &str) -> Self { + TEST_SERVICE.with(|s| *s.borrow_mut() = Some(name.to_owned())); + Self(name.to_owned()) + } +} + +#[cfg(test)] +impl Drop for TestKeychain { + fn drop(&mut self) { + if let Ok(entry) = keyring::Entry::new(&self.0, KEYRING_ACCOUNT) { + let _ = entry.delete_credential(); + } + TEST_SERVICE.with(|s| *s.borrow_mut() = None); + } +} + fn entry() -> Result { - keyring::Entry::new(KEYRING_SERVICE, KEYRING_ACCOUNT) + keyring::Entry::new(&keyring_service(), KEYRING_ACCOUNT) .map_err(|e| format!("no OS credential store available: {e}")) } @@ -357,6 +402,52 @@ mod tests { assert!(set_aside(dir.path()).expect("no error").is_none()); } + /// The bug, end to end, against a real OS keychain. + /// + /// A store file whose master key does not exist is exactly the state a user + /// reached: `load` reported `KeyLost`, nothing could be saved, and no + /// amount of retrying changed it. This asserts the whole way out — the + /// unreadable file is kept, a new key is minted, and the secret the user + /// was trying to save is there afterwards and readable on a reopen. + #[test] + fn a_store_with_no_key_recovers_and_accepts_the_save_that_was_blocked() { + let _keychain = TestKeychain::new("ai.latentic.compose.test.recover"); + let dir = tempfile::tempdir().expect("tempdir"); + + // The wedged state: a file encrypted under a key the keychain has never + // heard of. + let stranded = envelope_for([9u8; KEY_LEN], &[("openrouter", "old-secret")]); + std::fs::write( + dir.path().join(STORE_FILE), + serde_json::to_vec(&stranded).expect("serialize"), + ) + .expect("write"); + + assert!( + matches!(SecretStore::load(dir.path()).expect("load"), Opened::KeyLost), + "premise: this is the state that could not be escaped", + ); + + let mut recovered = SecretStore::recover(dir.path()).expect("recovery"); + recovered.set("openrouter", "the-key-the-user-typed").expect("the save that used to fail"); + + // Readable on a fresh open — the master key really did persist. + match SecretStore::load(dir.path()).expect("reopen") { + Opened::Ready(reopened) => { + assert_eq!(reopened.get("openrouter"), Some("the-key-the-user-typed")); + } + Opened::KeyLost => panic!("still wedged after recovery"), + } + + // And the old ciphertext was kept, not destroyed. + let kept: Vec<_> = std::fs::read_dir(dir.path()) + .expect("read dir") + .filter_map(Result::ok) + .filter(|e| e.file_name().to_string_lossy().contains(".unreadable-")) + .collect(); + assert_eq!(kept.len(), 1, "the unreadable store must be preserved"); + } + fn envelope_for(key: [u8; KEY_LEN], pairs: &[(&str, &str)]) -> Envelope { let secrets: BTreeMap = pairs.iter().map(|(k, v)| ((*k).to_owned(), (*v).to_owned())).collect(); From 4a8d568c10277b287c0233e7cab14217e1b095ea Mon Sep 17 00:00:00 2001 From: Tosin Amuda Date: Tue, 25 Aug 2026 21:16:17 +0200 Subject: [PATCH 4/7] perf(credentials): open the keychain when a key is wanted, not at boot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- src-tauri/src/harness/credentials.rs | 89 +++++++++++++++++----------- src-tauri/src/lib.rs | 18 +++--- 2 files changed, 61 insertions(+), 46 deletions(-) diff --git a/src-tauri/src/harness/credentials.rs b/src-tauri/src/harness/credentials.rs index 20ca11b..f591a1e 100644 --- a/src-tauri/src/harness/credentials.rs +++ b/src-tauri/src/harness/credentials.rs @@ -18,9 +18,10 @@ use serde::Serialize; use super::secret_store::{Opened, SecretStore}; -/// The process-wide store. `None` until [`init_from_dir`] runs at boot, and -/// again if the master key was lost — in which case reads return nothing and -/// writes mint a fresh store, which is exactly "re-enter your keys". +/// The process-wide store. `None` until something first needs a credential — +/// boot only records the directory, because opening this reads the keychain. +/// It stays `None` when the master key was lost, in which case reads return +/// nothing and writes mint a fresh store, which is exactly "re-enter your keys". static STORE: OnceLock>> = OnceLock::new(); static CONFIG_DIR: OnceLock = OnceLock::new(); @@ -28,19 +29,52 @@ fn store() -> &'static Mutex> { STORE.get_or_init(|| Mutex::new(None)) } -/// Load the store and fold in any keys written by the per-entry scheme. -/// Returns whether the previous secrets were unrecoverable, so the host can -/// say so rather than leaving the user wondering where their key went. -pub fn init_from_dir(config_dir: &std::path::Path) -> Result { +/// Record where the store lives. Deliberately no I/O. +/// +/// Opening it reads the OS keychain, and this runs inside Tauri's `setup`, +/// before `app.run()`. A keychain read there is not merely slow: on a machine +/// whose login keychain locks, it raises a modal unlock prompt with no window +/// behind it yet. The launches that need a credential at all are the minority — +/// an agent with its own auth (Claude, Codex) or none (Ollama) never asks — so +/// the store opens on first use instead. See `loaded_for_read` / `loaded_for_write`. +pub fn set_config_dir(config_dir: &std::path::Path) { let _ = CONFIG_DIR.set(config_dir.to_path_buf()); - match SecretStore::load(config_dir)? { - Opened::Ready(mut loaded) => { - migrate_legacy_entries(&mut loaded); - *store().lock().map_err(|_| "credential store lock poisoned")? = Some(loaded); - Ok(false) +} + +/// Open the store for a READ, migrating anything the old per-harness scheme +/// left behind. +/// +/// A lost key yields `None` rather than recovering: recovery mints a master key +/// and moves a file aside, and a read must not do either. Saving is where that +/// belongs, because there the user has said what to put in the new store. +fn loaded_for_read(guard: &mut Option) -> Option<&mut SecretStore> { + if guard.is_none() { + let dir = CONFIG_DIR.get()?; + match SecretStore::load(dir).ok()? { + Opened::Ready(mut fresh) => { + migrate_legacy_entries(&mut fresh); + *guard = Some(fresh); + } + Opened::KeyLost => return None, } - Opened::KeyLost => Ok(true), } + guard.as_mut() +} + +/// Open the store for a WRITE, recovering from a lost master key rather than +/// refusing — the caller is in the middle of saving a key. +fn loaded_for_write(guard: &mut Option) -> Result<&mut SecretStore, String> { + if guard.is_none() { + let dir = CONFIG_DIR.get().ok_or("credential store is not initialised")?; + *guard = Some(match SecretStore::load(dir)? { + Opened::Ready(mut fresh) => { + migrate_legacy_entries(&mut fresh); + fresh + } + Opened::KeyLost => SecretStore::recover(dir)?, + }); + } + guard.as_mut().ok_or_else(|| "credential store is not initialised".to_owned()) } /// Move keys written by the old one-entry-per-harness scheme into the store. @@ -124,9 +158,9 @@ fn every_spec() -> Vec { /// without passing through the environment, where the agent's own shell tool /// would inherit it. pub fn secret_for(provider_id: &str) -> Option { - let guard = store().lock().ok()?; - let value = guard.as_ref()?.get(provider_id)?; - (!value.trim().is_empty()).then(|| value.to_owned()) + let mut guard = store().lock().ok()?; + let value = loaded_for_read(&mut guard)?.get(provider_id)?.to_owned(); + (!value.trim().is_empty()).then_some(value) } pub struct Credential { @@ -153,9 +187,9 @@ impl Credential { if !self.host_managed() { return None; } - let guard = store().lock().ok()?; - let value = guard.as_ref()?.get(&self.spec.keychain_service)?; - (!value.trim().is_empty()).then(|| value.to_owned()) + let mut guard = store().lock().ok()?; + let value = loaded_for_read(&mut guard)?.get(&self.spec.keychain_service)?.to_owned(); + (!value.trim().is_empty()).then_some(value) } pub fn status(&self) -> CredentialStatus { @@ -171,22 +205,7 @@ impl Credential { } let value = value.trim(); let mut guard = store().lock().map_err(|_| "credential store lock poisoned")?; - // A lost master key leaves no store. Saving a key is the recovery, so - // build a fresh one rather than refusing the write. - if guard.is_none() { - let dir = CONFIG_DIR.get().ok_or("credential store is not initialised")?; - match SecretStore::load(dir)? { - Opened::Ready(fresh) => *guard = Some(fresh), - // Do what the comment above promises. The file cannot be - // opened by any key that exists, and the user is in the middle - // of saying "save this key" — refusing left them permanently - // unable to, with "Reset it in Settings" (which erases every - // workspace and conversation) as the only way out. - Opened::KeyLost => *guard = Some(SecretStore::recover(dir)?), - } - } - let store = guard.as_mut().ok_or("credential store is not initialised")?; - store.set(&self.spec.keychain_service, value)?; + loaded_for_write(&mut guard)?.set(&self.spec.keychain_service, value)?; // Deliberately not exported. The registry rebuilds each harness per // call and reads the value straight from the store, so a variable would // add nothing but reach — every child the agent spawns inherits it. diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 48a1c5b..3ec3862 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -206,17 +206,13 @@ pub fn run() { { eprintln!("custom-agent store init failed: {error}"); } - // Then the credential store, which must come after the - // custom agents so their keys migrate too. One decrypt - // covers every provider, and it carries over anything the - // old one-entry-per-harness scheme left behind. - match harness::credentials::init_from_dir(&config_dir) { - Ok(true) => eprintln!( - "credential store could not be unlocked — saved API keys must be re-entered" - ), - Ok(false) => {} - Err(error) => eprintln!("credential store init failed: {error}"), - } + // The credential store only learns WHERE it lives here. + // Opening it reads the keychain, and this is inside `setup`, + // before `app.run()` — where a locked login keychain would + // raise a modal unlock prompt with no window behind it. It + // opens on first use instead, which for an agent with its + // own auth or none is never. + harness::credentials::set_config_dir(&config_dir); } Err(error) => eprintln!("app config dir unavailable for custom agents: {error}"), } From 0c60af8eb017897b98c9a9a92925bc3ec99043ba Mon Sep 17 00:00:00 2001 From: Tosin Amuda Date: Tue, 25 Aug 2026 21:50:18 +0200 Subject: [PATCH 5/7] fix(settings): a saved key should look saved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../settings/agentConfigControls.test.tsx | 69 +++++++++++++++++++ src/features/settings/agentConfigControls.tsx | 41 ++++++++++- 2 files changed, 107 insertions(+), 3 deletions(-) create mode 100644 src/features/settings/agentConfigControls.test.tsx diff --git a/src/features/settings/agentConfigControls.test.tsx b/src/features/settings/agentConfigControls.test.tsx new file mode 100644 index 0000000..7e3006f --- /dev/null +++ b/src/features/settings/agentConfigControls.test.tsx @@ -0,0 +1,69 @@ +// @vitest-environment jsdom +import { cleanup, render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const status = vi.fn(async () => ({ configured: false })); +const setCredential = vi.fn(async () => {}); +vi.mock("../../lib/ipc/harnessClient", () => ({ + harnessCredentialStatus: (...args: unknown[]) => status(...(args as [])), + harnessSetCredential: (...args: unknown[]) => setCredential(...(args as [])), +})); + +import { HarnessCredentialForm } from "./agentConfigControls"; + +/** + * The API key form's RESTING states — what someone sees when they are not + * mid-action. A saved key used to look identical to an empty one: the success + * banner cleared itself after four seconds and left helper text phrased as an + * instruction ("Paste a new one to replace it"), so the screen never said the + * key was there. + */ +describe("HarnessCredentialForm", () => { + beforeEach(() => { + status.mockReset().mockResolvedValue({ configured: false }); + setCredential.mockReset().mockResolvedValue(undefined); + }); + afterEach(cleanup); + + it("says a key is saved, rather than only implying it", async () => { + status.mockResolvedValue({ configured: true }); + render(); + expect(await screen.findByText(/a key is saved/i)).toBeTruthy(); + }); + + it("offers no save until something is typed", async () => { + render(); + const save = await screen.findByRole("button", { name: /save key/i }); + expect((save as HTMLButtonElement).disabled).toBe(true); + + await userEvent.type(screen.getByLabelText(/openrouter api key/i), "k"); + expect((save as HTMLButtonElement).disabled).toBe(false); + }); + + it("never submits an empty value, which would clear the stored key", async () => { + status.mockResolvedValue({ configured: true }); + render(); + const replace = await screen.findByRole("button", { name: /replace key/i }); + + // The footgun: this button was enabled with an empty field, and submitting + // it stored "" — which deletes the key, reporting success either way. + expect((replace as HTMLButtonElement).disabled).toBe(true); + expect(setCredential).not.toHaveBeenCalled(); + }); + + it("makes removing a key a separate, deliberate action", async () => { + status.mockResolvedValue({ configured: true }); + render(); + const remove = await screen.findByRole("button", { name: /remove key/i }); + + await userEvent.click(remove); + await waitFor(() => expect(setCredential).toHaveBeenCalledWith("openrouter", "")); + }); + + it("shows no remove action when there is nothing to remove", async () => { + render(); + await screen.findByRole("button", { name: /save key/i }); + expect(screen.queryByRole("button", { name: /remove key/i })).toBeNull(); + }); +}); diff --git a/src/features/settings/agentConfigControls.tsx b/src/features/settings/agentConfigControls.tsx index 94c6cdd..e6111f1 100644 --- a/src/features/settings/agentConfigControls.tsx +++ b/src/features/settings/agentConfigControls.tsx @@ -1,5 +1,6 @@ import { FormEvent, useEffect, useState } from "react"; import { Button, InlineNotification, PasswordInput } from "@carbon/react"; +import { CheckmarkFilled } from "@carbon/react/icons"; import { harnessCapabilitiesOf } from "../../app/workspaceStore"; import { useHarnessStore } from "../../app/store/harnessStore"; @@ -62,6 +63,24 @@ export function HarnessCredentialForm({ harnessId, name }: { harnessId: string; }; }, [harnessId]); + // Clearing has to be its own action. It used to be reachable by submitting an + // empty field — a primary button, one careless click, and the key was gone + // with the same wording as storing one. + async function handleRemove() { + setSaving(true); + setError(null); + setSaved(false); + try { + await harnessSetCredential(harnessId, ""); + setApiKey(""); + setConfigured((await harnessCredentialStatus(harnessId)).configured); + } catch (err) { + setError(errorMessage(err, `Could not remove the ${name} API key`)); + } finally { + setSaving(false); + } + } + async function handleSave(event: FormEvent) { event.preventDefault(); setSaving(true); @@ -89,13 +108,22 @@ export function HarnessCredentialForm({ harnessId, name }: { harnessId: string; labelText={`${name} API key`} helperText={ configured - ? "A key is saved. Paste a new one to replace it." + ? "Paste a new key to replace the saved one." : `Paste your ${name} API key. Stored locally in your OS keychain.` } value={apiKey} onChange={(event) => setApiKey(event.currentTarget.value)} placeholder={configured ? "Replace saved key" : `Paste ${name} API key`} /> + {/* The resting state has to SAY it is configured. The success banner + clears itself after four seconds, and what remained was helper text + phrased as an instruction — so a saved key looked like an empty field + nobody had filled in yet. */} + {configured && !error && !saved ? ( +

+ A key is saved — {name} is ready to use. +

+ ) : null} {error ? ( ) : null}
- + {configured ? ( + + ) : null}
); From c72b1b6fe9b802948313346e3c202d061a17afa4 Mon Sep 17 00:00:00 2001 From: Tosin Amuda Date: Tue, 25 Aug 2026 22:08:57 +0200 Subject: [PATCH 6/7] feat(settings): show WHICH key is saved, not just that one is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "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". --- src-tauri/src/harness/credentials.rs | 78 ++++++++++++++++++- .../settings/agentConfigControls.test.tsx | 18 ++++- src/features/settings/agentConfigControls.tsx | 20 ++++- src/lib/ipc/harnessClient.ts | 4 + 4 files changed, 114 insertions(+), 6 deletions(-) diff --git a/src-tauri/src/harness/credentials.rs b/src-tauri/src/harness/credentials.rs index f591a1e..748de0d 100644 --- a/src-tauri/src/harness/credentials.rs +++ b/src-tauri/src/harness/credentials.rs @@ -171,6 +171,36 @@ pub struct Credential { #[serde(rename_all = "camelCase")] pub struct CredentialStatus { pub configured: bool, + /// Enough of the stored key to recognise WHICH one it is, and no more. + /// `None` when nothing is stored, or when the value is too short to show + /// any of without giving most of it away. + pub hint: Option, +} + +/// `sk-or…9f2c` — the shape every provider uses to list keys it will not show +/// again. The point is telling a stale key from the current one; it is not a +/// redaction of something the user may later reveal, because nothing here ever +/// reveals it. +/// +/// Short values get no characters at all. A hint is only safe while it is a +/// small fraction of the secret, and "small fraction" stops being true fast. +fn hint_for(secret: &str) -> Option { + const HEAD: usize = 5; + const TAIL: usize = 4; + /// Below this, HEAD + TAIL would be most of the value. + const MIN_LEN: usize = 16; + + let secret = secret.trim(); + if secret.is_empty() { + return None; + } + let chars: Vec = secret.chars().collect(); + if chars.len() < MIN_LEN { + return Some("•".repeat(8)); + } + let head: String = chars[..HEAD].iter().collect(); + let tail: String = chars[chars.len() - TAIL..].iter().collect(); + Some(format!("{head}…{tail}")) } impl Credential { @@ -193,8 +223,10 @@ impl Credential { } pub fn status(&self) -> CredentialStatus { + let stored = self.read(); CredentialStatus { - configured: !self.host_managed() || self.read().is_some(), + configured: !self.host_managed() || stored.is_some(), + hint: stored.as_deref().and_then(hint_for), } } @@ -223,3 +255,47 @@ pub fn forget_all() { *guard = None; } } + +#[cfg(test)] +mod hint_tests { + use super::hint_for; + + #[test] + fn shows_enough_to_tell_two_keys_apart() { + assert_eq!( + hint_for("sk-or-v1-0123456789abcdef9f2c").as_deref(), + Some("sk-or…9f2c"), + ); + } + + #[test] + fn a_short_value_gives_up_no_characters() { + // The guard that matters. Head + tail on a short secret is most of it, + // and a hint is only safe while it stays a small fraction. + for short in ["abc", "sk-1234", "123456789012345"] { + let hint = hint_for(short).expect("something"); + assert!( + !hint.contains(|c: char| c.is_ascii_alphanumeric()), + "{short} leaked characters through its hint: {hint}", + ); + } + } + + #[test] + fn nothing_stored_is_nothing_to_hint_at() { + assert_eq!(hint_for(""), None); + assert_eq!(hint_for(" "), None); + } + + #[test] + fn never_reveals_more_than_a_fraction() { + let secret = "sk-or-v1-".to_owned() + &"a".repeat(48); + let hint = hint_for(&secret).expect("a hint"); + let revealed = hint.chars().filter(|c| *c != '…').count(); + assert!( + revealed * 4 < secret.chars().count(), + "hint revealed {revealed} of {} characters", + secret.chars().count(), + ); + } +} diff --git a/src/features/settings/agentConfigControls.test.tsx b/src/features/settings/agentConfigControls.test.tsx index 7e3006f..2f16fff 100644 --- a/src/features/settings/agentConfigControls.test.tsx +++ b/src/features/settings/agentConfigControls.test.tsx @@ -3,7 +3,8 @@ import { cleanup, render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -const status = vi.fn(async () => ({ configured: false })); +type Status = { configured: boolean; hint?: string | null }; +const status = vi.fn(async (): Promise => ({ configured: false })); const setCredential = vi.fn(async () => {}); vi.mock("../../lib/ipc/harnessClient", () => ({ harnessCredentialStatus: (...args: unknown[]) => status(...(args as [])), @@ -21,7 +22,7 @@ import { HarnessCredentialForm } from "./agentConfigControls"; */ describe("HarnessCredentialForm", () => { beforeEach(() => { - status.mockReset().mockResolvedValue({ configured: false }); + status.mockReset().mockResolvedValue({ configured: false } as Status); setCredential.mockReset().mockResolvedValue(undefined); }); afterEach(cleanup); @@ -32,6 +33,19 @@ describe("HarnessCredentialForm", () => { expect(await screen.findByText(/a key is saved/i)).toBeTruthy(); }); + it("names WHICH key is saved, so a stale one is recognisable", async () => { + status.mockResolvedValue({ configured: true, hint: "sk-or…9f2c" }); + render(); + expect(await screen.findByText("sk-or…9f2c")).toBeTruthy(); + }); + + it("falls back to the plain wording when the backend sends no hint", async () => { + // A short secret gets no hint at all, and an older backend sends no field. + status.mockResolvedValue({ configured: true }); + render(); + expect(await screen.findByText(/a key is saved/i)).toBeTruthy(); + }); + it("offers no save until something is typed", async () => { render(); const save = await screen.findByRole("button", { name: /save key/i }); diff --git a/src/features/settings/agentConfigControls.tsx b/src/features/settings/agentConfigControls.tsx index e6111f1..67781d9 100644 --- a/src/features/settings/agentConfigControls.tsx +++ b/src/features/settings/agentConfigControls.tsx @@ -45,6 +45,7 @@ export function ModelSection({ harnessId }: { harnessId: string }) { export function HarnessCredentialForm({ harnessId, name }: { harnessId: string; name: string }) { const [apiKey, setApiKey] = useState(""); const [configured, setConfigured] = useState(false); + const [hint, setHint] = useState(null); const [saving, setSaving] = useState(false); const [saved, setSaved] = useState(false); const [error, setError] = useState(null); @@ -53,7 +54,10 @@ export function HarnessCredentialForm({ harnessId, name }: { harnessId: string; let active = true; void harnessCredentialStatus(harnessId) .then((status) => { - if (active) setConfigured(status.configured); + if (active) { + setConfigured(status.configured); + setHint(status.hint ?? null); + } }) .catch(() => { if (active) setConfigured(false); @@ -73,7 +77,9 @@ export function HarnessCredentialForm({ harnessId, name }: { harnessId: string; try { await harnessSetCredential(harnessId, ""); setApiKey(""); - setConfigured((await harnessCredentialStatus(harnessId)).configured); + const cleared = await harnessCredentialStatus(harnessId); + setConfigured(cleared.configured); + setHint(cleared.hint ?? null); } catch (err) { setError(errorMessage(err, `Could not remove the ${name} API key`)); } finally { @@ -91,6 +97,7 @@ export function HarnessCredentialForm({ harnessId, name }: { harnessId: string; setApiKey(""); const status = await harnessCredentialStatus(harnessId); setConfigured(status.configured); + setHint(status.hint ?? null); setSaved(true); window.setTimeout(() => setSaved(false), 4000); } catch (err) { @@ -121,7 +128,14 @@ export function HarnessCredentialForm({ harnessId, name }: { harnessId: string; nobody had filled in yet. */} {configured && !error && !saved ? (

- A key is saved — {name} is ready to use. + {" "} + {hint ? ( + <> + Saved key {hint} — {name} is ready to use. + + ) : ( + <>A key is saved — {name} is ready to use. + )}

) : null} {error ? ( diff --git a/src/lib/ipc/harnessClient.ts b/src/lib/ipc/harnessClient.ts index 2d2075a..c9a41d7 100644 --- a/src/lib/ipc/harnessClient.ts +++ b/src/lib/ipc/harnessClient.ts @@ -492,6 +492,10 @@ export async function harnessSetCredential(harnessId: string, value: string): Pr /** Whether a harness's API key is stored (or none is needed). */ export interface HarnessCredentialStatus { configured: boolean; + /** `sk-or…9f2c` — enough to recognise WHICH key is stored, never enough to + * use it. Absent when nothing is stored, or the value was too short to + * show any of safely. */ + hint?: string | null; } /** Read whether a harness's API key is stored (or none is needed). */ From 2f50f0b80c780e41dd875c36ce36415b1e9ebbbf Mon Sep 17 00:00:00 2001 From: Tosin Amuda Date: Tue, 25 Aug 2026 23:49:57 +0200 Subject: [PATCH 7/7] fix(credentials): opening the store must not read from it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src-tauri/src/harness/credentials.rs | 74 ++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/src-tauri/src/harness/credentials.rs b/src-tauri/src/harness/credentials.rs index 748de0d..804f4bc 100644 --- a/src-tauri/src/harness/credentials.rs +++ b/src-tauri/src/harness/credentials.rs @@ -25,6 +25,39 @@ use super::secret_store::{Opened, SecretStore}; static STORE: OnceLock>> = OnceLock::new(); static CONFIG_DIR: OnceLock = OnceLock::new(); +thread_local! { + /// True while THIS thread is opening the store. + /// + /// Opening migrates the old per-harness entries, and to know which entries + /// exist it asks the registry for every credential spec — which builds each + /// harness, and building one reads its key. That read cannot be served: the + /// store is mid-open and this thread already holds its lock, so attempting + /// it deadlocks rather than returning anything. It reports "no key", which + /// is what it meant anyway — the specs are wanted for their NAMES. + static OPENING: std::cell::Cell = const { std::cell::Cell::new(false) }; +} + +/// Whether a credential read must decline because the store is being built. +fn opening() -> bool { + OPENING.with(std::cell::Cell::get) +} + +/// Sets the flag for as long as the returned guard lives. +struct OpeningGuard; + +impl OpeningGuard { + fn new() -> Self { + OPENING.with(|f| f.set(true)); + Self + } +} + +impl Drop for OpeningGuard { + fn drop(&mut self) { + OPENING.with(|f| f.set(false)); + } +} + fn store() -> &'static Mutex> { STORE.get_or_init(|| Mutex::new(None)) } @@ -52,6 +85,7 @@ fn loaded_for_read(guard: &mut Option) -> Option<&mut SecretStore> let dir = CONFIG_DIR.get()?; match SecretStore::load(dir).ok()? { Opened::Ready(mut fresh) => { + let _opening = OpeningGuard::new(); migrate_legacy_entries(&mut fresh); *guard = Some(fresh); } @@ -68,6 +102,7 @@ fn loaded_for_write(guard: &mut Option) -> Result<&mut SecretStore, let dir = CONFIG_DIR.get().ok_or("credential store is not initialised")?; *guard = Some(match SecretStore::load(dir)? { Opened::Ready(mut fresh) => { + let _opening = OpeningGuard::new(); migrate_legacy_entries(&mut fresh); fresh } @@ -158,6 +193,9 @@ fn every_spec() -> Vec { /// without passing through the environment, where the agent's own shell tool /// would inherit it. pub fn secret_for(provider_id: &str) -> Option { + if opening() { + return None; + } let mut guard = store().lock().ok()?; let value = loaded_for_read(&mut guard)?.get(provider_id)?.to_owned(); (!value.trim().is_empty()).then_some(value) @@ -217,6 +255,9 @@ impl Credential { if !self.host_managed() { return None; } + if opening() { + return None; + } let mut guard = store().lock().ok()?; let value = loaded_for_read(&mut guard)?.get(&self.spec.keychain_service)?.to_owned(); (!value.trim().is_empty()).then_some(value) @@ -299,3 +340,36 @@ mod hint_tests { ); } } + +#[cfg(test)] +mod reentrancy_tests { + use super::{secret_for, store, OpeningGuard}; + use std::sync::mpsc; + use std::time::Duration; + + /// Opening the store must never ask the store for anything. + /// + /// Migration needs the list of credential specs, and getting it builds every + /// harness — which reads that harness's key. Do that while the loader holds + /// the store lock and `std::sync::Mutex` deadlocks, because it is not + /// reentrant. The symptom is not an error: Settings sits on its loading + /// skeletons for ever. + /// + /// The failure is a hang, so the work runs on another thread and the + /// assertion is a deadline. Remove the `opening()` guard and this fails with + /// its message rather than wedging the suite. + #[test] + fn opening_the_store_never_reads_from_it() { + let (tx, rx) = mpsc::channel(); + std::thread::spawn(move || { + let _held = store().lock().expect("store lock"); + let _opening = OpeningGuard::new(); + let _ = tx.send(secret_for("openrouter")); + }); + + let seen = rx + .recv_timeout(Duration::from_secs(5)) + .expect("a credential read while opening must return, not deadlock"); + assert_eq!(seen, None, "the store is mid-build; it has nothing to report"); + } +}