From 547453ff6cf2dee4856ec0fc92281a5daf1c62fb Mon Sep 17 00:00:00 2001 From: Franz Hemmer Date: Sat, 7 Nov 2026 03:11:48 -0500 Subject: [PATCH 1/3] fix: keep an account's saved key when its settings save is refused (#320) The account dialog saves a pasted key before the settings. When the settings save was then refused (a blank name, another CodexBar holding the lock, a conflict or an unreadable file), only a replaced OpenCode sign-in was put back: other accounts kept the new key, and a new account left its key under an id no account has. The dialog now reads what the key replaces before writing it. If the settings save fails, it puts the previous secret back, or removes the key when the id held none. A secret another writer saved meanwhile is left alone. A failure to undo the write is added to the dialog's message instead of being ignored. A key write failure still saves no account. Co-Authored-By: Claude Opus 5.5 --- crates/codexbar-app/src/captures.rs | 150 +++++++++++ crates/codexbar-app/src/settings_view.rs | 125 +++++++-- crates/codexbar-app/src/ui_tests.rs | 317 ++++++++++++++++++++++- 3 files changed, 560 insertions(+), 32 deletions(-) diff --git a/crates/codexbar-app/src/captures.rs b/crates/codexbar-app/src/captures.rs index bddb8206..fad30dd8 100644 --- a/crates/codexbar-app/src/captures.rs +++ b/crates/codexbar-app/src/captures.rs @@ -1042,3 +1042,153 @@ fn captures() { } let _ = std::fs::remove_dir_all(&settings); } + +/// A store that refuses to undo the account dialog's key write (#320): it won't save `sk-old` again and deletes +/// nothing. +struct RefusesUndo(MemoryCredentialStore); + +impl codexbar_store::credentials::CredentialStore for RefusesUndo { + fn read(&self, account_id: &str) -> Result, codexbar_store::credentials::CredentialError> { + self.0.read(account_id) + } + + fn write(&self, account_id: &str, secret: &str) -> Result<(), codexbar_store::credentials::CredentialError> { + if secret == "sk-old" { + return Err(codexbar_store::credentials::CredentialError { code: 5 }); + } + self.0.write(account_id, secret) + } + + fn delete(&self, _: &str) -> Result<(), codexbar_store::credentials::CredentialError> { + Err(codexbar_store::credentials::CredentialError { code: 5 }) + } +} + +/// #320: the account dialog when a refused settings save can't be undone in the credential store, light and dark: +/// an edited account whose previous key couldn't be put back, then a new account whose key couldn't be removed. +/// The settings are refused by a blank name; the dashboard's provider factory builds nothing, so no request is made. +#[test] +#[ignore = "writes screenshots; run with CODEXBAR_CAPTURE_DIR set"] +fn captures_refused_key_320() { + use codexbar_store::credentials::CredentialStore as _; + let Some(out) = std::env::var_os("CODEXBAR_CAPTURE_DIR").map(PathBuf::from) else { + return; + }; + std::fs::create_dir_all(&out).unwrap(); + let settings = std::env::temp_dir().join(format!("codexbar-captures-320-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&settings); + std::fs::create_dir_all(&settings).unwrap(); + std::fs::write(settings.join("settings.json"), SETTINGS).unwrap(); + + let text_system = gpui_kit::platform::current_platform(true).text_system(); + let mut cx = HeadlessAppContext::with_platform(text_system, Arc::new(crate::assets::Assets), || { + gpui_kit::platform::current_headless_renderer() + }); + let store = MemoryCredentialStore::default(); + store.write("or-1", "sk-old").unwrap(); + store.write("ms-1", "sk-ms-capture").unwrap(); + cx.update(|cx| { + gpui_kit::init(cx); + cx.set_reduce_motion(true); + theme::init(cx); + SettingsHub::init_with(cx, &settings, Arc::new(RefusesUndo(store))); + crate::prefs_hub::PrefsHub::init_in_memory(cx); + zoom::init(cx); + crate::notifications::Notifications::init(cx, Arc::new(RecordingNotifier::default()), false); + crate::package::Updates::init(cx, false); + crate::package::StartupSetting::init(cx); + }); + let mut dashboard = None; + let handle = cx + .open_window(size(px(1440.), px(960.)), |window, cx| { + let source = DataSource::Live { + history: Arc::new(std::sync::Mutex::new(codexbar_store::HistoryStore::in_memory( + chrono::Duration::days(30), + ))), + providers: Arc::new(|_| Vec::new()), + }; + let view = cx.new(|cx| Dashboard::new(source, window, cx)); + dashboard = Some(view.clone()); + cx.new(|cx| Root::new(view, window, cx)) + }) + .unwrap(); + let handle: AnyWindowHandle = handle.into(); + let dashboard = dashboard.unwrap(); + cx.run_until_parked(); + cx.update(|cx| dashboard.update(cx, |dashboard, cx| dashboard.show_view(DashboardView::Settings, cx))); + cx.run_until_parked(); + let openrouter = crate::catalog::PROVIDERS + .iter() + .position(|info| info.id == codexbar_store::settings::names::OPENROUTER) + .unwrap(); + for entry in ["0-1".to_owned(), format!("0-1-{openrouter}")] { + cx.update_window(handle, |_, window, cx| { + window.render_frame(cx); + window.within("settings-sidebar").click(entry, cx) + }) + .unwrap(); + cx.run_until_parked(); + } + + // Each case: open the dialog, clear the name, enter a key and press Save. + let cases = [ + ( + "edit-or-1", + "sk-new", + "refused-key-not-restored", + "couldn't be put back", + ), + ( + "add-OpenRouter", + "sk-abandoned", + "refused-new-key-not-removed", + "couldn't be removed", + ), + ]; + for (open, key, name, says) in cases { + cx.update_window(handle, |_, window, cx| { + window.render_frame(cx); + window.click(open, cx) + }) + .unwrap(); + cx.run_until_parked(); + for press in ["backspace", "tab", "tab"] { + cx.update_window(handle, |_, window, cx| window.press(press, cx)) + .unwrap(); + cx.run_until_parked(); + } + cx.update_window(handle, |_, window, cx| window.input(key, cx)).unwrap(); + cx.run_until_parked(); + cx.update_window(handle, |_, window, cx| { + window.render_frame(cx); + window.click("account-save", cx) + }) + .unwrap(); + cx.run_until_parked(); + let error = cx + .update_window(handle, |_, window, cx| { + window.render_frame(cx); + window + .try_find("account-error") + .and_then(|found| found.label().map(str::to_owned)) + }) + .unwrap() + .unwrap_or_default(); + assert!( + error.starts_with("Every account needs a name.") && error.contains(says), + "{name}: {error}" + ); + save(&mut cx, handle, &out, name); + cx.update(|cx| crate::prefs_hub::PrefsHub::set_appearance(cx, theme::Appearance::Dark)); + cx.run_until_parked(); + save(&mut cx, handle, &out, &format!("{name}-dark")); + cx.update(|cx| crate::prefs_hub::PrefsHub::set_appearance(cx, theme::Appearance::System)); + cx.update_window(handle, |_, window, cx| { + window.render_frame(cx); + window.press("escape", cx) + }) + .unwrap(); + cx.run_until_parked(); + } + let _ = std::fs::remove_dir_all(&settings); +} diff --git a/crates/codexbar-app/src/settings_view.rs b/crates/codexbar-app/src/settings_view.rs index 27b3fcbb..f32b9590 100644 --- a/crates/codexbar-app/src/settings_view.rs +++ b/crates/codexbar-app/src/settings_view.rs @@ -29,7 +29,7 @@ use crate::catalog::{self, PROVIDERS, SecretSpec}; use crate::notifications::{Notifications, NotifierStatus}; use crate::prefs_hub::PrefsHub; use crate::settings_hub::{SettingsHub, describe_source}; -use codexbar_store::credentials::SecretSource; +use codexbar_store::credentials::{CredentialError, CredentialStore, SecretSource}; /// Refresh choices required by #91, in minutes; 0 is Off. const REFRESH_CHOICES: [u64; 6] = [0, 1, 5, 15, 30, 60]; @@ -936,26 +936,19 @@ fn save_account(form: &AccountForm, existing: Option, window: &mu && !secret.is_empty() && previous.as_ref().is_some_and(crate::opencode_sign_in::is_managed); - // The secret is written first: if Credential Manager fails, the account isn't saved without its key. An OpenCode - // account on OAuth keeps its sign-in there instead, so no cookie is written over it. - // The sign-in a cookie replaces, put back if the settings can't be saved after it. - let mut replaced_sign_in = None; + // The secret is written first: if Credential Manager fails, the account isn't saved without its key. What it + // replaced is kept, to put back if the settings then can't be saved (#320). An OpenCode account on OAuth keeps + // its sign-in there instead, so no cookie is written over it. + let mut replaced = None; if info.secret.is_some() && !secret.is_empty() && !opencode_oauth { let store = SettingsHub::global(cx).credentials(); - let written = if replaces_opencode_sign_in { - // One write that leaves the old sign-in readable until the cookie is in place, with no renewal writing - // at the same time. - codexbar_providers::opencode_console::exclusive(|| { - replaced_sign_in = codexbar_store::credentials::read_long(store.as_ref(), &record.id)?; - codexbar_store::credentials::write_long(store.as_ref(), &record.id, &secret) - }) - } else { - store.write(&record.id, &secret) - }; - if let Err(err) = written { - *form.error.borrow_mut() = Some(err.to_string().into()); - window.refresh(); - return false; + match ReplacedSecret::write(store.as_ref(), &record.id, &secret, replaces_opencode_sign_in) { + Ok(written) => replaced = Some(written), + Err(err) => { + *form.error.borrow_mut() = Some(err.to_string().into()); + window.refresh(); + return false; + } } } use crate::managed::Managed; @@ -1019,20 +1012,98 @@ fn save_account(form: &AccountForm, existing: Option, window: &mu true } Err(err) => { - // The account is still on OAuth, so it gets its sign-in back. - if let Some(previous) = replaced_sign_in { - let store = SettingsHub::global(cx).credentials(); - let _ = codexbar_providers::opencode_console::exclusive(|| { - codexbar_store::credentials::write_long(store.as_ref(), &record_id, &previous) - }); - } - *form.error.borrow_mut() = Some(err.to_string().into()); + let store = SettingsHub::global(cx).credentials(); + let message = ReplacedSecret::undo(replaced, store.as_ref(), &record_id, &secret, &err); + *form.error.borrow_mut() = Some(message.into()); window.refresh(); false } } } +/// What the account dialog's key write replaced under the account's id, so a settings save that is then refused can +/// undo it (#320). +struct ReplacedSecret { + /// The secret saved before, or `None` when the id held none. + before: Option, + /// Written with `write_long`: an OpenCode sign-in that may span several credential parts, with no renewal + /// writing at the same time. + long: bool, +} + +impl ReplacedSecret { + /// Saves `secret` under `id` after reading what it replaces. Nothing is written if that can't be read, since it + /// couldn't be put back. + fn write(store: &dyn CredentialStore, id: &str, secret: &str, long: bool) -> Result { + use codexbar_store::credentials::{read_long, write_long}; + let write = || { + let before = if long { read_long(store, id)? } else { store.read(id)? }; + if long { + write_long(store, id, secret) + } else { + store.write(id, secret) + }?; + Ok(Self { before, long }) + }; + if long { + codexbar_providers::opencode_console::exclusive(write) + } else { + write() + } + } + + /// Undoes the write after the settings save failed with `refused`, and says why the save failed: the account + /// keeps the secret it had, and an id that held none is left empty. A secret another writer saved under the id + /// since then is left alone. A failure to undo it is added to the message, never hidden. + fn undo( + replaced: Option, + store: &dyn CredentialStore, + id: &str, + written: &str, + refused: &dyn std::fmt::Display, + ) -> String { + let Some(replaced) = replaced else { + return refused.to_string(); + }; + let had_secret = replaced.before.is_some(); + match replaced.restore(store, id, written) { + Ok(()) => refused.to_string(), + Err(err) if had_secret => { + format!("{refused} The new key is still saved, and the previous one couldn't be put back: {err}") + } + Err(err) => format!( + "{refused} The key is still saved in {} and couldn't be removed: {err}", + codexbar_store::credentials::STORE_SHORT_NAME + ), + } + } + + fn restore(self, store: &dyn CredentialStore, id: &str, written: &str) -> Result<(), CredentialError> { + use codexbar_store::credentials::{delete_long, read_long, write_long}; + let restore = || { + let now = if self.long { + read_long(store, id)? + } else { + store.read(id)? + }; + if now.as_deref() != Some(written) { + return Ok(()); + } + match (&self.before, self.long) { + (Some(before), true) => write_long(store, id, before), + (Some(before), false) => store.write(id, before), + (None, true) => delete_long(store, id), + (None, false) => store.delete(id), + } + }; + if self.long { + codexbar_providers::opencode_console::exclusive(restore) + } else { + restore() + } + } +} + fn confirm_remove(record: AccountRecord, window: &mut Window, cx: &mut App) { let label = record.label.clone(); // Only accounts that hold a pasted secret have anything in Credential Manager to delete. An account that is its own diff --git a/crates/codexbar-app/src/ui_tests.rs b/crates/codexbar-app/src/ui_tests.rs index 8a5df5b0..f1995b74 100644 --- a/crates/codexbar-app/src/ui_tests.rs +++ b/crates/codexbar-app/src/ui_tests.rs @@ -5,7 +5,7 @@ use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; -use codexbar_store::credentials::MemoryCredentialStore; +use codexbar_store::credentials::{CredentialError, CredentialStore, MemoryCredentialStore}; use gpui_kit::component::Root; use gpui_kit::test::TestWindowExt as _; use gpui_kit::{ @@ -1230,7 +1230,7 @@ fn open_live_with( cx: &mut TestAppContext, settings: &TempSettings, providers: Vec>, - store: MemoryCredentialStore, + store: impl CredentialStore + 'static, ) -> Entity { let providers = providers .into_iter() @@ -1252,7 +1252,7 @@ fn open_live_dyn( cx: &mut TestAppContext, settings: &TempSettings, providers: Vec>, - store: MemoryCredentialStore, + store: impl CredentialStore + 'static, ) -> Entity { let factory: crate::dashboard::ProviderFactory = Arc::new(move |_| providers.clone()); open_live_factory(cx, settings, factory, store) @@ -1263,7 +1263,7 @@ fn open_live_factory( cx: &mut TestAppContext, settings: &TempSettings, factory: crate::dashboard::ProviderFactory, - store: MemoryCredentialStore, + store: impl CredentialStore + 'static, ) -> Entity { use codexbar_store::HistoryStore; cx.update(|cx| { @@ -2767,7 +2767,7 @@ fn smart_order_re_ranks_when_held_alerts_change(cx: &mut TestAppContext) { fn open_settings_page( cx: &mut TestAppContext, settings: &TempSettings, - store: MemoryCredentialStore, + store: impl CredentialStore + 'static, page: usize, ) -> AnyWindowHandle { let dashboard = open_live_with(cx, settings, vec![], store); @@ -3466,6 +3466,313 @@ fn secure_store_failures_are_shown_and_nothing_is_saved_without_its_key(cx: &mut ); } +// A key saved before a settings save that is then refused is put back (#320). + +/// A store that saves the first `writes` secrets, then refuses every write and delete, so putting a key back fails. +struct FailsAfter { + inner: MemoryCredentialStore, + writes: std::sync::atomic::AtomicUsize, +} + +impl FailsAfter { + fn new(writes: usize) -> Self { + Self { + inner: MemoryCredentialStore::default(), + writes: std::sync::atomic::AtomicUsize::new(writes), + } + } +} + +impl CredentialStore for FailsAfter { + fn read(&self, account_id: &str) -> Result, CredentialError> { + self.inner.read(account_id) + } + + fn write(&self, account_id: &str, secret: &str) -> Result<(), CredentialError> { + use std::sync::atomic::Ordering; + self.writes + .try_update(Ordering::SeqCst, Ordering::SeqCst, |left| left.checked_sub(1)) + .map_err(|_| CredentialError { code: 5 })?; + self.inner.write(account_id, secret) + } + + fn delete(&self, _: &str) -> Result<(), CredentialError> { + Err(CredentialError { code: 5 }) + } +} + +/// A store that records every id written, so a test can see what a refused save left behind. Clones share it. +#[derive(Clone, Default)] +struct RecordingStore { + inner: Arc, + written: Arc>>, +} + +impl CredentialStore for RecordingStore { + fn read(&self, account_id: &str) -> Result, CredentialError> { + self.inner.read(account_id) + } + + fn write(&self, account_id: &str, secret: &str) -> Result<(), CredentialError> { + self.written.lock().unwrap().push(account_id.to_owned()); + self.inner.write(account_id, secret) + } + + fn delete(&self, account_id: &str) -> Result<(), CredentialError> { + self.inner.delete(account_id) + } +} + +/// Opens an account's dialog, clears its name or types a new one, enters `key` and presses Save. +fn edit_openrouter(cx: &mut TestAppContext, handle: AnyWindowHandle, id: &str, name: Option<&str>, key: &str) { + click(cx, handle, format!("edit-{id}")); + match name { + Some(name) => type_text(cx, handle, name), + None => press(cx, handle, "backspace"), + } + press(cx, handle, "tab"); + press(cx, handle, "tab"); + type_text(cx, handle, key); + click(cx, handle, "account-save"); +} + +/// Holds the shared settings write lock, as a second CodexBar saving settings does, until dropped. +fn hold_settings_lock(settings: &TempSettings) -> std::fs::File { + let mut options = std::fs::OpenOptions::new(); + options.read(true).write(true).create(true).truncate(false); + #[cfg(windows)] + { + use std::os::windows::fs::OpenOptionsExt as _; + options.share_mode(0); + } + let file = options.open(settings.0.join("settings.write.lock")).unwrap(); + #[cfg(not(windows))] + file.try_lock().unwrap(); + file +} + +#[gpui_kit::test] +fn a_refused_edit_keeps_the_accounts_saved_key(cx: &mut TestAppContext) { + let settings = TempSettings::new("settings-refused-key", "{}"); + let handle = open_settings_page(cx, &settings, MemoryCredentialStore::default(), 1); + add_openrouter(cx, handle, "Work", "sk-old"); + let id = saved_settings(&settings).accounts()[0].id.clone(); + let before = std::fs::read(settings.0.join("settings.json")).unwrap(); + + // Without a new key there is nothing to put back. + edit_openrouter(cx, handle, &id, None, ""); + assert_eq!( + label(cx, handle, "account-error").as_deref(), + Some("Every account needs a name.") + ); + assert_eq!(secret_of(cx, &id).as_deref(), Some("sk-old")); + press(cx, handle, "escape"); + + // A blank name is refused after the key was written: the old key is put back. + edit_openrouter(cx, handle, &id, None, "sk-new"); + assert_eq!( + label(cx, handle, "account-error").as_deref(), + Some("Every account needs a name.") + ); + assert!(exists(cx, handle, "account-save"), "the dialog stays open"); + assert_eq!(std::fs::read(settings.0.join("settings.json")).unwrap(), before); + assert_eq!(secret_of(cx, &id).as_deref(), Some("sk-old")); + press(cx, handle, "escape"); + assert_eq!(secret_of(cx, &id).as_deref(), Some("sk-old"), "cancelling keeps it"); + + // Another CodexBar holding the settings lock refuses the save too. + let held = hold_settings_lock(&settings); + edit_openrouter(cx, handle, &id, Some("Personal"), "sk-new"); + assert_eq!( + label(cx, handle, "account-error").as_deref(), + Some("Another CodexBar window is saving settings. Try again in a moment.") + ); + assert_eq!(saved_settings(&settings).accounts()[0].label, "Work"); + assert_eq!(secret_of(cx, &id).as_deref(), Some("sk-old")); + press(cx, handle, "escape"); + drop(held); + + // Once the save goes through, the new key is the one saved. + edit_openrouter(cx, handle, &id, Some("Personal"), "sk-new"); + assert_eq!(saved_settings(&settings).accounts()[0].label, "Personal"); + assert_eq!(secret_of(cx, &id).as_deref(), Some("sk-new")); +} + +#[gpui_kit::test] +fn a_refused_new_account_leaves_no_key_behind(cx: &mut TestAppContext) { + let settings = TempSettings::new("settings-refused-new", "{}"); + let store = RecordingStore::default(); + let handle = open_settings_page(cx, &settings, store.clone(), 1); + click(cx, handle, "add-OpenRouter"); + press(cx, handle, "backspace"); + press(cx, handle, "tab"); + press(cx, handle, "tab"); + type_text(cx, handle, "sk-abandoned"); + click(cx, handle, "account-save"); + assert_eq!( + label(cx, handle, "account-error").as_deref(), + Some("Every account needs a name.") + ); + press(cx, handle, "escape"); + assert!(saved_settings(&settings).accounts().is_empty()); + let written = store.written.lock().unwrap().clone(); + assert!(!written.is_empty(), "the key was written before the refusal"); + for id in written { + assert_eq!(store.inner.read(&id).unwrap(), None, "nothing is left under {id}"); + } +} + +#[gpui_kit::test] +fn a_key_that_cannot_be_put_back_is_reported(cx: &mut TestAppContext) { + let settings = TempSettings::new("settings-restore-fails", "{}"); + // The first key and the edit's key are saved; putting the first one back is refused. + let handle = open_settings_page(cx, &settings, FailsAfter::new(2), 1); + add_openrouter(cx, handle, "Work", "sk-old"); + let id = saved_settings(&settings).accounts()[0].id.clone(); + edit_openrouter(cx, handle, &id, None, "sk-new"); + let store_error = CredentialError { code: 5 }.to_string(); + assert_eq!( + label(cx, handle, "account-error"), + Some(format!( + "Every account needs a name. The new key is still saved, and the previous one couldn't be put back: \ + {store_error}" + )) + ); + assert_eq!( + secret_of(cx, &id).as_deref(), + Some("sk-new"), + "the message says what is saved" + ); +} + +#[gpui_kit::test] +fn a_new_accounts_key_that_cannot_be_removed_is_reported(cx: &mut TestAppContext) { + let settings = TempSettings::new("settings-remove-fails", "{}"); + let handle = open_settings_page(cx, &settings, FailsAfter::new(1), 1); + click(cx, handle, "add-OpenRouter"); + press(cx, handle, "backspace"); + press(cx, handle, "tab"); + press(cx, handle, "tab"); + type_text(cx, handle, "sk-abandoned"); + click(cx, handle, "account-save"); + let store_error = CredentialError { code: 5 }.to_string(); + assert_eq!( + label(cx, handle, "account-error"), + Some(format!( + "Every account needs a name. The key is still saved in {} and couldn't be removed: {store_error}", + codexbar_store::credentials::STORE_SHORT_NAME + )) + ); +} + +/// A store where another writer saves `theirs` for an account right after this one saves `mine` there. +struct ReplacedMeanwhile { + inner: MemoryCredentialStore, + mine: &'static str, + theirs: &'static str, +} + +impl CredentialStore for ReplacedMeanwhile { + fn read(&self, account_id: &str) -> Result, CredentialError> { + self.inner.read(account_id) + } + + fn write(&self, account_id: &str, secret: &str) -> Result<(), CredentialError> { + let secret = if secret == self.mine { self.theirs } else { secret }; + self.inner.write(account_id, secret) + } + + fn delete(&self, account_id: &str) -> Result<(), CredentialError> { + self.inner.delete(account_id) + } +} + +#[gpui_kit::test] +fn a_key_another_writer_saved_meanwhile_is_not_rolled_back(cx: &mut TestAppContext) { + let settings = TempSettings::new("settings-refused-newer", "{}"); + let store = ReplacedMeanwhile { + inner: MemoryCredentialStore::default(), + mine: "sk-mine", + theirs: "sk-theirs", + }; + let handle = open_settings_page(cx, &settings, store, 1); + add_openrouter(cx, handle, "Work", "sk-old"); + let id = saved_settings(&settings).accounts()[0].id.clone(); + edit_openrouter(cx, handle, &id, None, "sk-mine"); + assert_eq!( + label(cx, handle, "account-error").as_deref(), + Some("Every account needs a name.") + ); + assert_eq!( + secret_of(cx, &id).as_deref(), + Some("sk-theirs"), + "the newer key is left alone" + ); +} + +#[gpui_kit::test] +fn a_refused_cookie_leaves_a_signed_out_opencode_account_without_one(cx: &mut TestAppContext) { + let settings = TempSettings::new( + "opencode-refused-cookie-signed-out", + r#"{ "accountConfigurationVersion": 1, "accounts": [ + { "id": "oc-1", "providerId": "OpenCodeGo", "displayLabel": "Team", "enabled": true, + "authenticationMethod": "OAuth", "workspaceId": "wrk_01ABC" } ] }"#, + ); + let handle = open_opencode_accounts(cx, &settings, &FakeOpenCode::default()); + let held = hold_settings_lock(&settings); + click(cx, handle, "edit-oc-1"); + choose_browser_session(cx, handle); + press(cx, handle, "tab"); + type_text(cx, handle, "Fe26.2**new-cookie"); + click(cx, handle, "account-save"); + assert_eq!( + label_of(cx, "account-error").as_deref(), + Some("Another CodexBar window is saving settings. Try again in a moment.") + ); + drop(held); + let stored = + cx.update(|cx| codexbar_store::credentials::read_long(SettingsHub::global(cx).credentials().as_ref(), "oc-1")); + assert_eq!(stored.unwrap(), None, "the signed-out account keeps no cookie"); +} + +#[gpui_kit::test] +fn a_refused_cookie_gives_an_opencode_account_its_sign_in_back(cx: &mut TestAppContext) { + let settings = TempSettings::new( + "opencode-refused-cookie", + r#"{ "accountConfigurationVersion": 1, "accounts": [ + { "id": "oc-1", "providerId": "OpenCodeGo", "displayLabel": "Team", "enabled": true, + "authenticationMethod": "OAuth", "workspaceId": "wrk_01ABC", "externalAccountId": "usr_1" } ] }"#, + ); + let handle = open_opencode_accounts(cx, &settings, &FakeOpenCode::default()); + let sign_in = opencode_fixture::credential("wrk_01ABC", "usr_1").to_secret(); + cx.update(|cx| { + codexbar_store::credentials::write_long(SettingsHub::global(cx).credentials().as_ref(), "oc-1", &sign_in) + }) + .unwrap(); + let held = hold_settings_lock(&settings); + click(cx, handle, "edit-oc-1"); + choose_browser_session(cx, handle); + press(cx, handle, "tab"); + type_text(cx, handle, "Fe26.2**new-cookie"); + click(cx, handle, "account-save"); + assert_eq!( + label_of(cx, "account-error").as_deref(), + Some("Another CodexBar window is saving settings. Try again in a moment.") + ); + drop(held); + assert_eq!( + saved_settings(&settings).accounts()[0].method, + codexbar_store::settings::AuthMethod::OAuth + ); + let stored = + cx.update(|cx| codexbar_store::credentials::read_long(SettingsHub::global(cx).credentials().as_ref(), "oc-1")); + assert_eq!( + stored.unwrap().as_deref(), + Some(sign_in.as_str()), + "the OAuth sign-in is back" + ); +} + /// A provider that returns fixed accounts, such as a provider's real parser output. struct FixedProvider(Vec); From cd234559d9e558ebdac5ab0ba445e45236160ec3 Mon Sep 17 00:00:00 2001 From: Franz Hemmer Date: Sat, 7 Nov 2026 03:23:22 -0500 Subject: [PATCH 2/3] ci: render the account dialog captures for #320 on Linux The Linux rust job now runs captures_refused_key_320 after its tests, with Mesa's software Vulkan driver, and uploads its four PNGs as the account-dialog-captures artifact. The step fails if any frame is missing. The fixture now sets Light and Dark explicitly instead of relying on System for its light frame. A CI binding test pins both steps. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 27 +++++++++++++++++++++++++++ crates/codexbar-app/src/captures.rs | 15 ++++++++++----- scripts/tests/ci-workflow.test.cjs | 27 +++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b480f1f7..990197e5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -103,6 +103,33 @@ jobs: - name: Test run: cargo test --workspace --locked + # PR screenshots of the account dialog's refused-save messages (#320), from GPUI's Linux offscreen renderer: the + # only platform with one. The runner has no GPU, so Mesa's software Vulkan driver draws them. No window opens, + # and the fixture uses an in-memory key store, a temporary settings folder and no providers. + - name: Render account dialog captures + if: runner.os == 'Linux' + env: + CODEXBAR_CAPTURE_DIR: ${{ github.workspace }}/artifacts/captures + run: | + sudo apt-get install -y --no-install-recommends mesa-vulkan-drivers libvulkan1 + cargo test -p codexbar-app --bin codexbar --locked -- --ignored --exact captures::captures_refused_key_320 --nocapture + for name in refused-key-not-restored refused-key-not-restored-dark refused-new-key-not-removed refused-new-key-not-removed-dark; do + test -s "$CODEXBAR_CAPTURE_DIR/$name.png" || { echo "::error::$name.png was not rendered"; exit 1; } + done + + - name: Upload account dialog captures + if: runner.os == 'Linux' + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: account-dialog-captures + path: | + artifacts/captures/refused-key-not-restored.png + artifacts/captures/refused-key-not-restored-dark.png + artifacts/captures/refused-new-key-not-removed.png + artifacts/captures/refused-new-key-not-removed-dark.png + if-no-files-found: error + retention-days: 14 + rust-metrics: runs-on: ubuntu-latest timeout-minutes: 45 diff --git a/crates/codexbar-app/src/captures.rs b/crates/codexbar-app/src/captures.rs index fad30dd8..7d9203b5 100644 --- a/crates/codexbar-app/src/captures.rs +++ b/crates/codexbar-app/src/captures.rs @@ -1064,7 +1064,7 @@ impl codexbar_store::credentials::CredentialStore for RefusesUndo { } } -/// #320: the account dialog when a refused settings save can't be undone in the credential store, light and dark: +/// #320: the account dialog when a refused settings save can't be undone in the credential store, Light and Dark: /// an edited account whose previous key couldn't be put back, then a new account whose key couldn't be removed. /// The settings are refused by a blank name; the dashboard's provider factory builds nothing, so no request is made. #[test] @@ -1178,10 +1178,15 @@ fn captures_refused_key_320() { error.starts_with("Every account needs a name.") && error.contains(says), "{name}: {error}" ); - save(&mut cx, handle, &out, name); - cx.update(|cx| crate::prefs_hub::PrefsHub::set_appearance(cx, theme::Appearance::Dark)); - cx.run_until_parked(); - save(&mut cx, handle, &out, &format!("{name}-dark")); + // Light and Dark chosen explicitly, so neither frame depends on what System resolves to. + for (appearance, frame) in [ + (theme::Appearance::Light, name.to_owned()), + (theme::Appearance::Dark, format!("{name}-dark")), + ] { + cx.update(|cx| crate::prefs_hub::PrefsHub::set_appearance(cx, appearance)); + cx.run_until_parked(); + save(&mut cx, handle, &out, &frame); + } cx.update(|cx| crate::prefs_hub::PrefsHub::set_appearance(cx, theme::Appearance::System)); cx.update_window(handle, |_, window, cx| { window.render_frame(cx); diff --git a/scripts/tests/ci-workflow.test.cjs b/scripts/tests/ci-workflow.test.cjs index 99ee23c3..7f0df318 100644 --- a/scripts/tests/ci-workflow.test.cjs +++ b/scripts/tests/ci-workflow.test.cjs @@ -83,6 +83,33 @@ test('every platform checks the production binary without dev-dependency feature assert.ok(steps.indexOf(step) < names.indexOf('Test')); }); +// The account dialog's screenshots (#320) come from the Linux rust job, after its tests, and must exist: a capture that +// renders nothing fails the job instead of uploading an empty artifact. +test('the Linux rust job renders and uploads the account dialog captures', () => { + const steps = workflow.jobs.rust.steps; + const names = steps.map(step => step.name); + const render = steps.find(step => step.name === 'Render account dialog captures'); + const upload = steps.find(step => step.name === 'Upload account dialog captures'); + assert.ok(render && upload); + assert.ok(names.indexOf('Test') < steps.indexOf(render) && steps.indexOf(render) < steps.indexOf(upload)); + for (const step of [render, upload]) { + assert.equal(step.if, "runner.os == 'Linux'"); + assert.equal(step['continue-on-error'], undefined); + } + assert.equal(render.env.CODEXBAR_CAPTURE_DIR, '${{ github.workspace }}/artifacts/captures'); + assert.match(render.run, + /^cargo test -p codexbar-app --bin codexbar --locked -- --ignored --exact captures::captures_refused_key_320 --nocapture$/m); + const frames = ['refused-key-not-restored', 'refused-key-not-restored-dark', 'refused-new-key-not-removed', + 'refused-new-key-not-removed-dark']; + for (const frame of frames) { + assert.ok(render.run.includes(frame), frame); + } + assert.match(render.run, /test -s "\$CODEXBAR_CAPTURE_DIR\/\$name\.png" \|\| \{ .*exit 1; \}/); + assert.match(upload.uses, /^actions\/upload-artifact@/); + assert.equal(upload.with['if-no-files-found'], 'error'); + assert.deepEqual(upload.with.path.trim().split('\n'), frames.map(frame => `artifacts/captures/${frame}.png`)); +}); + // Every action is pinned to a reviewed commit, with the release it came from in a trailing comment (#301). // A movable tag such as `@v4` could change what runs after review, so a new reference must also be a SHA. test('every action reference in CI and Package is a full commit SHA with its version', () => { From 51072ef838fbacc5db9410bfe340e46b734b18cc Mon Sep 17 00:00:00 2001 From: Franz Hemmer Date: Sat, 7 Nov 2026 03:58:04 -0500 Subject: [PATCH 3/3] test: cover refused memory credential operations (#320) The account dialog now reads before writing, so its refusal test no longer exercises the failing memory-store write. Cover the store failure contract directly and verify refused writes and deletes preserve existing data. Co-Authored-By: Codex --- crates/codexbar-store/src/credentials.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/crates/codexbar-store/src/credentials.rs b/crates/codexbar-store/src/credentials.rs index cee2356a..e1da1324 100644 --- a/crates/codexbar-store/src/credentials.rs +++ b/crates/codexbar-store/src/credentials.rs @@ -448,6 +448,22 @@ mod tests { assert_eq!(read_long(&store, "acct").unwrap(), None); } + #[test] + fn refused_memory_store_operations_leave_saved_secrets_unchanged() { + let mut store = MemoryCredentialStore::default(); + store.write("existing", "before").unwrap(); + store.fail = true; + + assert_eq!(store.read("existing").unwrap_err().code, 5); + assert_eq!(store.write("existing", "after").unwrap_err().code, 5); + assert_eq!(store.write("new", "abandoned").unwrap_err().code, 5); + assert_eq!(store.delete("existing").unwrap_err().code, 5); + + store.fail = false; + assert_eq!(store.read("existing").unwrap().as_deref(), Some("before")); + assert_eq!(store.read("new").unwrap(), None); + } + #[test] fn memory_store_add_replace_read_delete() { let store = MemoryCredentialStore::default();