ZacharyZhang-NY/Kigi-CLI
1
0
Fork 0
Code Issues Pull Requests Actions 13 Packages Projects Releases Wiki Activity

fix(tui): a default-model persist failure no longer reverts the live model

Browse Source 
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.
This commit is contained in:
ZacharyZhang-NY
2026-07-22 19:55:02 -04:00
parent 27d009cb6e
commit f403c38a94
2 changed files with 92 additions and 61 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/codegen/kigi-tui/src/app/dispatch/settings/ui.rs
+20 -61
View File
@@ -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<Effect> {
use crate::settings::SettingValue;
let mut companion_effects: Vec<Effect> = Vec::new();
let companion_effects: Vec<Effect> = 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),
crates/codegen/kigi-tui/src/app/dispatch/tests/task_result.rs
+72
View File
@@ -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]