From 202ccccea14c337fe703d3f88db2b5a53049ee0e Mon Sep 17 00:00:00 2001 From: bflatastic Date: Mon, 2 Nov 2026 14:07:04 +0700 Subject: [PATCH] Changing AI settings never saves over ones that couldn't be read Fixes #261 --- crates/ai/src/lib.rs | 21 +++++++++++++++------ crates/ai/src/tests.rs | 30 ++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/crates/ai/src/lib.rs b/crates/ai/src/lib.rs index 3b238ffa..3b584822 100644 --- a/crates/ai/src/lib.rs +++ b/crates/ai/src/lib.rs @@ -288,6 +288,15 @@ impl AiHub { } } + /// The configuration to change and save back. An [ai] section that can't be read is an error + /// here, never the empty default, so a change can't save over it (#261). + fn config_to_change(&self) -> Result { + match self.engine.try_session() { + Some(s) => AiConfig::from_table(&s.settings.ai), + None => AiConfig::from_table(&Settings::load_from(&self.settings_path()).ai), + } + } + /// Save the AI configuration into the settings file, then the session: a save that fails /// changes nothing, so the app never shows a backend or role that isn't saved. pub fn save_config(&self, cfg: &AiConfig) -> Result<(), ToolError> { @@ -326,7 +335,7 @@ impl AiHub { /// Choose the backend for `role`; `None` goes back to its default (the builder: an agent app, /// else any AI backend; the extractor: none). pub fn set_role(&self, role: Role, name: Option<&str>) -> Result<(), ToolError> { - let mut cfg = self.config(); + let mut cfg = self.config_to_change()?; let role = config::role_key(role).to_string(); match name { Some(n) if cfg.get(n).is_none() => { @@ -392,10 +401,10 @@ impl AiHub { if entry.config.name.trim().is_empty() { return Err(ToolError::invalid("A backend needs a name.")); } + let mut cfg = self.config_to_change()?; if let Some(k) = key.filter(|k| !k.trim().is_empty()) { self.keys.set(&entry.config.name, k.trim())?; } - let mut cfg = self.config(); cfg.upsert(entry); self.save_config(&cfg) } @@ -403,7 +412,7 @@ impl AiHub { /// Turn a backend on or off; it keeps its settings and key. Returns false when there was no /// such backend. pub fn set_enabled(&self, name: &str, on: bool) -> Result { - let mut cfg = self.config(); + let mut cfg = self.config_to_change()?; let Some(b) = cfg.backends.iter_mut().find(|b| b.config.name == name) else { return Ok(false) }; b.config.enabled = on; self.save_config(&cfg)?; @@ -413,7 +422,7 @@ impl AiHub { /// The program an agent app runs (`None` or empty: found by its usual name, on PATH and in the /// usual install folders). Returns false when there was no such backend. pub fn set_command(&self, name: &str, command: Option<&str>) -> Result { - let mut cfg = self.config(); + let mut cfg = self.config_to_change()?; let Some(b) = cfg.backends.iter_mut().find(|b| b.config.name == name) else { return Ok(false) }; b.options.command = command.map(str::trim).filter(|c| !c.is_empty()).map(str::to_string); self.save_config(&cfg)?; @@ -432,7 +441,7 @@ impl AiHub { if model.is_empty() { return Err(ToolError::invalid("Name a model.")); } - let mut cfg = self.config(); + let mut cfg = self.config_to_change()?; let Some(b) = cfg.backends.iter_mut().find(|b| b.config.name == name) else { return Ok(false) }; match b.config.kind { BackendKind::AgentCli => b.options.cli_model = Some(model.to_string()), @@ -455,7 +464,7 @@ impl AiHub { /// Remove a backend and its stored key. Returns false when there was no such backend. pub fn remove_backend(&self, name: &str) -> Result { - let mut cfg = self.config(); + let mut cfg = self.config_to_change()?; let removed = cfg.remove(name); if removed { self.save_config(&cfg)?; diff --git a/crates/ai/src/tests.rs b/crates/ai/src/tests.rs index c586e915..7c55c84f 100644 --- a/crates/ai/src/tests.rs +++ b/crates/ai/src/tests.rs @@ -138,6 +138,36 @@ fn a_failed_save_changes_nothing() { assert!(h.backends().is_empty(), "the app shows only what was saved"); } +/// #261: one bad price made the whole [ai] section read as empty, and adding a backend saved +/// that over the backends, roles and prices. +#[test] +fn ai_settings_that_cant_be_read_are_never_saved_over() { + let (h, d) = hub(); + add(&h, "Claude Code", BackendKind::AgentCli, "http://127.0.0.1:9", None); + add(&h, "Kimi K3 (OpenRouter)", BackendKind::Openrouter, "http://127.0.0.1:9", None); + h.set_role(Role::Builder, Some("Kimi K3 (OpenRouter)")).unwrap(); + let file = d.path().join("config.toml"); + let mut bad = h.engine.session().settings.clone(); + let price: toml::Table = toml::from_str( + "[m] +input_per_mtok = 1.0 +", + ) + .unwrap(); + bad.ai.insert("prices".into(), toml::Value::Table(price)); + bad.save_to(&file).unwrap(); + h.engine.session().settings = bad; + let before = std::fs::read(&file).unwrap(); + let cfg = BackendConfig { name: "Other".into(), kind: BackendKind::Openrouter, model: "m".into(), base_url: None, ssh_host: None, enabled: true }; + let e = h.add_backend(cfg, Some("sk-test".into())).unwrap_err(); + assert_eq!(e.code, ErrorCode::BadSettings, "{e:?}"); + assert!(h.set_role(Role::Builder, None).is_err()); + assert!(h.set_enabled("Claude Code", false).is_err()); + assert!(h.remove_backend("Claude Code").is_err()); + assert_eq!(std::fs::read(&file).unwrap(), before, "the settings file was changed"); + assert!(h.keys.get("Other").is_none(), "a refused add stored its key"); +} + #[test] fn the_person_chooses_who_builds() { let (h, _d) = hub();