fix: keep an account's saved key when its settings save is refused (#320)

.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

crates/codexbar-app/src/captures.rs

@@ -1042,3 +1042,158 @@ 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<Option<String>, 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}"
+        );
+        // 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);
+            window.press("escape", cx)
+        })
+        .unwrap();
+        cx.run_until_parked();
+    }
+    let _ = std::fs::remove_dir_all(&settings);
+}

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<AccountRecord>, 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<AccountRecord>, 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<String>,
+    /// 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<Self, CredentialError> {
+        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<Self>,
+        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