From f403c38a94f2ce928ae573693ebadc591283a31b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Wed, 22 Jul 2026 19:55:02 -0400 Subject: [PATCH] fix(tui): a default-model persist failure no longer reverts the live model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Persist-failure ≠ switch-failure. The default_model rollback arm re-ran set_default_model_inner(prev) AND issued a reverse SwitchModel whenever the config.toml write failed — the only mechanism in the codebase that deliberately re-shows the ORIGINAL model after a successful pick. On Windows, where AV/indexer file locks routinely failed that write (until 27d009c), every /model selection appeared to not take. The session switch succeeds independently and reports its own failures via handle_switch_model_complete; a disk-persist failure now keeps the live model, logs, and surfaces only the 'Could not save' toast — the same policy PersistPreferredModel already ships ('still active for this session'). Verified: kigi-tui 6874 tests green, clippy clean. --- .../kigi-tui/src/app/dispatch/settings/ui.rs | 81 +++++-------------- .../src/app/dispatch/tests/task_result.rs | 72 +++++++++++++++++ 2 files changed, 92 insertions(+), 61 deletions(-) diff --git a/crates/codegen/kigi-tui/src/app/dispatch/settings/ui.rs b/crates/codegen/kigi-tui/src/app/dispatch/settings/ui.rs index 1247619..315e16d 100644 --- a/crates/codegen/kigi-tui/src/app/dispatch/settings/ui.rs +++ b/crates/codegen/kigi-tui/src/app/dispatch/settings/ui.rs @@ -3,7 +3,7 @@ use super::setters::{ pr13_effective_default, set_ask_user_question_timeout_enabled_inner, set_auto_dark_theme_inner, set_auto_light_theme_inner, set_auto_update_inner, set_collapsed_edit_blocks_inner, - set_compact_mode, set_compact_mode_inner, set_contextual_hint_inner, set_default_model_inner, + set_compact_mode, set_compact_mode_inner, set_contextual_hint_inner, set_default_selected_permission_inner, set_display_refresh_auto_cadence_inner, set_fork_secondary_model_inner, set_group_tool_verbs_inner, set_hunk_tracker_mode_inner, set_invert_scroll_inner, set_keep_text_selection_inner, set_max_thoughts_width_inner, @@ -844,7 +844,7 @@ pub(in crate::app::dispatch) fn apply_setting_rollback( rollback_value: &crate::settings::SettingValue, ) -> Vec { use crate::settings::SettingValue; - let mut companion_effects: Vec = Vec::new(); + let companion_effects: Vec = Vec::new(); match (key, rollback_value) { ("compact_mode", SettingValue::Bool(b)) => set_compact_mode_inner(app, *b), ("show_timestamps", SettingValue::Bool(b)) => set_timestamps_inner(app, *b), @@ -934,65 +934,24 @@ pub(in crate::app::dispatch) fn apply_setting_rollback( // other rollback arms must not clobber it from the global canonical. sync_active_auto_flag(app); } - // default_model: best-effort rollback. If the prior model no - // longer resolves, leave optimistic value + log. - ("default_model", SettingValue::String(s)) => { - if s.is_empty() { - tracing::warn!( - target: "settings", - key = "default_model", - "rollback to empty string requested but no \ - 'clear current model' API exists — leaving live \ - state at optimistic value (next session reload \ - will resolve via shell default-resolution chain)", - ); - } else { - // Resolve the prior model ID back to a ModelId - // and call the typed inner. If resolution fails - // (catalog changed mid-flight), log + leave - // optimistic. - let (resolved, session_id) = if let ActiveView::Agent(aid) = app.active_view - && let Some(agent) = app.agents.get(&aid) - { - ( - agent.session.models.resolve_by_name_or_id(s), - agent.session.session_id.clone(), - ) - } else { - (None, None) - }; - match resolved { - Some(id) => { - let _ = set_default_model_inner(app, &id); - // Emit reverse SwitchModel so the ACP session - // matches the rolled-back pager mirror. - if let ActiveView::Agent(aid) = app.active_view - && let Some(sid) = session_id - { - if let Some(agent) = app.agents.get_mut(&aid) { - agent.session.model_switch_pending = true; - } - companion_effects.push(Effect::SwitchModel { - agent_id: aid, - session_id: sid, - model_id: id, - effort: None, - prev_model_id: None, - }); - } - } - None => { - tracing::warn!( - target: "settings", - key = "default_model", - value = %s, - "rollback model id no longer resolves in catalog — \ - in-memory state stays at optimistic value; ACP session \ - may diverge from pager mirror until next setter dispatch", - ); - } - } - } + // default_model: deliberately NO revert. The session switch already + // succeeded independently (its own failure path reports via + // `handle_switch_model_complete`); this arm fires when only the + // DISK write of the next-launch default failed — which must not + // undo a working switch. Same policy as `PersistPreferredModel` + // ("still active for this session"). Regression: reverting here + // (plus a reverse SwitchModel) made every picker selection appear + // to not take on Windows, where AV/indexer file locks routinely + // fail config.toml persists. + ("default_model", SettingValue::String(prior)) => { + tracing::warn!( + target: "settings", + key = "default_model", + prior = %prior, + "default-model persist failed; keeping the live session's \ + model (the switch succeeded) — only the next-launch \ + default is unsaved", + ); } // max_thoughts_width: direct inner call. ("max_thoughts_width", SettingValue::Int(i)) => set_max_thoughts_width_inner(app, *i), diff --git a/crates/codegen/kigi-tui/src/app/dispatch/tests/task_result.rs b/crates/codegen/kigi-tui/src/app/dispatch/tests/task_result.rs index f0d5434..188e6ae 100644 --- a/crates/codegen/kigi-tui/src/app/dispatch/tests/task_result.rs +++ b/crates/codegen/kigi-tui/src/app/dispatch/tests/task_result.rs @@ -1592,6 +1592,78 @@ fn rollback_reverts_thread_local_cache_too() { .unwrap(); } +/// A default_model persist failure must NOT revert the live session's +/// model. The switch already succeeded in the session (its own failure +/// path reports separately); a DISK write failure only means the default +/// won't stick for the next launch — same policy as PersistPreferredModel +/// ("still active for this session"). Regression: the rollback arm +/// re-showed the ORIGINAL model and issued a reverse SwitchModel, which +/// on Windows (persist failures from AV/indexer file locks) made every +/// picker selection appear to not take. +#[test] +fn default_model_persist_failure_keeps_live_model() { + use crate::settings::SettingValue; + let mut app = test_app_with_agent(); + let id = AgentId(0); + for (mid, name) in [("old-model", "Old Model"), ("new-model", "New Model")] { + let model_id = acp::ModelId::new(std::sync::Arc::from(mid)); + let info = acp::ModelInfo::new(model_id.clone(), name.to_string()); + app.agents + .get_mut(&id) + .unwrap() + .session + .models + .available + .insert(model_id.clone(), info.clone()); + app.models.available.insert(model_id, info); + } + // User picked the new model; optimistic update applied. + let _ = dispatch( + Action::SetDefaultModel(acp::ModelId::new(std::sync::Arc::from("new-model"))), + &mut app, + ); + assert_eq!( + app.agents[&id] + .session + .models + .current + .as_ref() + .map(|m| m.0.as_ref()), + Some("new-model"), + ); + + // Disk persist fails (the Windows sharing-violation shape). + let effects = dispatch( + Action::TaskComplete(TaskResult::SettingPersistFailed { + key: "default_model", + rollback_value: SettingValue::String("Old Model".into()), + error: "Access is denied. (os error 5)".into(), + }), + &mut app, + ); + + assert_eq!( + app.agents[&id] + .session + .models + .current + .as_ref() + .map(|m| m.0.as_ref()), + Some("new-model"), + "a persist failure must not revert the live session's model" + ); + assert!( + !effects + .iter() + .any(|e| matches!(e, Effect::SwitchModel { .. })), + "no reverse SwitchModel may be issued for a disk-persist failure" + ); + assert!( + read_toast(&app).contains("Could not save"), + "the save failure must still be surfaced" + ); +} + /// `set_yolo_mode_inner` is the backstop: even a (stale) rollback /// value of "always-approve" must not re-enable yolo under the pin. #[test]