diff --git a/src-tauri/src/harness/credentials.rs b/src-tauri/src/harness/credentials.rs index beb3025..804f4bc 100644 --- a/src-tauri/src/harness/credentials.rs +++ b/src-tauri/src/harness/credentials.rs @@ -18,29 +18,98 @@ 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(); +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)) } -/// 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) => { + let _opening = OpeningGuard::new(); + 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) => { + let _opening = OpeningGuard::new(); + 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 +193,12 @@ 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()) + 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) } pub struct Credential { @@ -137,6 +209,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 { @@ -153,14 +255,19 @@ 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()) + 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) } 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), } } @@ -171,19 +278,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), - Opened::KeyLost => { - return Err("Could not unlock the credential store. Reset it in Settings.".to_owned()) - } - } - } - 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. @@ -201,3 +296,80 @@ 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(), + ); + } +} + +#[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"); + } +} diff --git a/src-tauri/src/harness/secret_store.rs b/src-tauri/src/harness/secret_store.rs index 4687708..c08c1fb 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,14 +248,78 @@ 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); 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}")) } @@ -290,6 +375,79 @@ 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()); + } + + /// 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(); 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}"), } 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.test.tsx b/src/features/settings/agentConfigControls.test.tsx new file mode 100644 index 0000000..2f16fff --- /dev/null +++ b/src/features/settings/agentConfigControls.test.tsx @@ -0,0 +1,83 @@ +// @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"; + +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 [])), + 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 } as Status); + 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("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 }); + 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 1dcb5b0..67781d9 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"; @@ -10,6 +11,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. @@ -43,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); @@ -51,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); @@ -61,6 +67,26 @@ 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(""); + 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 { + setSaving(false); + } + } + async function handleSave(event: FormEvent) { event.preventDefault(); setSaving(true); @@ -71,10 +97,11 @@ 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) { - 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); } @@ -88,13 +115,29 @@ 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 ? ( +

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

+ ) : null} {error ? ( ) : null}
- + {configured ? ( + + ) : null}
); 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). */