From 6297f1947a563591e6e0cbf14c022297bf00eedd Mon Sep 17 00:00:00 2001 From: zk <> Date: Wed, 17 Jun 2026 14:51:45 +0800 Subject: [PATCH] fix(acp): persist runtime model and mode into assistant preferences MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When users switched the model or mode inside an ACP conversation, the change reached the running agent through `set_config_option` but never flowed back into `assistant_preferences.last_*`. The next conversation seeded from the same assistant in `auto` mode therefore re-used the previously persisted value, ignoring the user's most recent pick. Mirror runtime model/mode switches into the persisted assistant snapshot and preference, but only when the agent confirms the change as observed — `command_ack` means the request was accepted, not applied, and unrelated option ids (e.g. `thought_level`) have no preference mapping. Persistence failures are logged and do not roll back the user-facing config switch. --- crates/aionui-conversation/src/service_ops.rs | 58 ++++- .../aionui-conversation/src/service_test.rs | 223 ++++++++++++++++++ 2 files changed, 276 insertions(+), 5 deletions(-) diff --git a/crates/aionui-conversation/src/service_ops.rs b/crates/aionui-conversation/src/service_ops.rs index 2c869c4e..339ad36c 100644 --- a/crates/aionui-conversation/src/service_ops.rs +++ b/crates/aionui-conversation/src/service_ops.rs @@ -10,12 +10,14 @@ use std::path::Component; use aionui_api_types::{ - GetConfigOptionsResponse, SetConfigOptionRequest, SetConfigOptionResponse, SideQuestionRequest, - SideQuestionResponse, SlashCommandItem, WorkspaceBrowseQuery, WorkspaceEntry, + ConfigOptionConfirmation, GetConfigOptionsResponse, SetConfigOptionRequest, SetConfigOptionResponse, + SideQuestionRequest, SideQuestionResponse, SlashCommandItem, WorkspaceBrowseQuery, WorkspaceEntry, }; +use aionui_common::ErrorChain; +use tracing::warn; use crate::ConversationError; -use crate::service::ConversationService; +use crate::service::{AssistantRuntimePreferenceUpdate, ConversationService}; const MAX_DIR_DEPTH: usize = 10; @@ -48,10 +50,56 @@ impl ConversationService { reason: "value must not be empty".into(), }); } - self.task(conversation_id)? + let response = self + .task(conversation_id)? .set_config_option(option_id, &req.value) .await - .map_err(ConversationError::from) + .map_err(ConversationError::from)?; + + // Mirror runtime model/mode switches into the persisted assistant + // snapshot + preference so the next conversation seeded from this + // assistant in `auto` mode reflects the latest pick. We only act on + // observed confirmations — `command_ack` means the agent merely + // accepted the request, not that the value is in effect, and + // unrelated option ids (e.g. `thought_level`) have no preference + // mapping. Persistence failures are logged but do not roll back the + // user-facing config switch. + if response.confirmation == ConfigOptionConfirmation::Observed { + let updates = match option_id { + "model" => Some(AssistantRuntimePreferenceUpdate { + model: Some(req.value.as_str()), + permission: None, + }), + "mode" => Some(AssistantRuntimePreferenceUpdate { + model: None, + permission: Some(req.value.as_str()), + }), + _ => None, + }; + if let Some(updates) = updates { + if let Err(err) = self.persist_runtime_assistant_snapshot(conversation_id, updates).await { + warn!( + conversation_id, + option_id, + error = %ErrorChain(&err), + "Failed to persist runtime assistant snapshot after set_config_option", + ); + } + if let Err(err) = self + .persist_runtime_assistant_preferences(conversation_id, updates) + .await + { + warn!( + conversation_id, + option_id, + error = %ErrorChain(&err), + "Failed to persist runtime assistant preferences after set_config_option", + ); + } + } + } + + Ok(response) } // ── Usage / Slash commands ────────────────────────────────────── diff --git a/crates/aionui-conversation/src/service_test.rs b/crates/aionui-conversation/src/service_test.rs index 38edf233..1d5b2645 100644 --- a/crates/aionui-conversation/src/service_test.rs +++ b/crates/aionui-conversation/src/service_test.rs @@ -2505,6 +2505,229 @@ async fn command_ack_does_not_persist_assistant_preference_in_core_service() { assert!(refreshed.extra.get("current_model_id").is_none()); } +#[tokio::test] +async fn set_config_option_persists_runtime_model_into_assistant_preference_when_observed() { + let task_mgr = Arc::new(MockTaskManager::new()); + let (svc, _broadcaster, repo, definition_repo, overlay_repo, preference_repo) = + make_service_with_mock_task_manager_and_assistant_support(task_mgr.clone()).await; + + upsert_test_assistant_definition( + &definition_repo, + "asstdef_acp_auto", + "assistant-acp-auto", + "codex", + "auto", + "auto", + ) + .await; + overlay_repo + .upsert(&UpsertAssistantOverlayParams { + definition_id: "asstdef_acp_auto", + enabled: true, + sort_order: 0, + agent_backend_override: None, + last_used_at: None, + }) + .await + .unwrap(); + preference_repo + .upsert(&UpsertAssistantPreferenceParams { + definition_id: "asstdef_acp_auto", + last_model_id: Some("legacy-acp-model"), + last_permission_value: Some("legacy-mode"), + last_skill_ids: "[]", + last_disabled_builtin_skill_ids: "[]", + last_mcp_ids: "[]", + }) + .await + .unwrap(); + + let conv = create_assistant_backed_conversation(&svc, "user_1", "acp", "codex", "assistant-acp-auto").await; + + let agent = Arc::new(MockAgent::new(&conv.id)); + task_mgr.insert_agent(&conv.id, AgentInstance::Mock(agent)); + + let result = svc + .set_config_option( + &conv.id, + "model", + SetConfigOptionRequest { + value: "gpt-5.5".to_owned(), + }, + ) + .await + .unwrap(); + assert_eq!(result.confirmation, ConfigOptionConfirmation::Observed); + + let pref_after_model = preference_repo.get("asstdef_acp_auto").await.unwrap().unwrap(); + assert_eq!(pref_after_model.last_model_id.as_deref(), Some("gpt-5.5")); + assert_eq!(pref_after_model.last_permission_value.as_deref(), Some("legacy-mode")); + let snapshot_after_model = repo.get_assistant_snapshot(&conv.id).await.unwrap().unwrap(); + assert_eq!(snapshot_after_model.resolved_model_id.as_deref(), Some("gpt-5.5")); + + svc.set_config_option( + &conv.id, + "mode", + SetConfigOptionRequest { + value: "plan".to_owned(), + }, + ) + .await + .unwrap(); + let pref_after_mode = preference_repo.get("asstdef_acp_auto").await.unwrap().unwrap(); + assert_eq!(pref_after_mode.last_model_id.as_deref(), Some("gpt-5.5")); + assert_eq!(pref_after_mode.last_permission_value.as_deref(), Some("plan")); + let snapshot_after_mode = repo.get_assistant_snapshot(&conv.id).await.unwrap().unwrap(); + assert_eq!(snapshot_after_mode.resolved_permission_value.as_deref(), Some("plan")); + + // Unrelated option ids must not touch preferences. + svc.set_config_option( + &conv.id, + "thought_level", + SetConfigOptionRequest { + value: "high".to_owned(), + }, + ) + .await + .unwrap(); + let pref_after_thought = preference_repo.get("asstdef_acp_auto").await.unwrap().unwrap(); + assert_eq!(pref_after_thought.last_model_id.as_deref(), Some("gpt-5.5")); + assert_eq!(pref_after_thought.last_permission_value.as_deref(), Some("plan")); +} + +#[tokio::test] +async fn set_config_option_skips_preference_write_back_when_default_mode_is_fixed() { + let task_mgr = Arc::new(MockTaskManager::new()); + let (svc, _broadcaster, repo, definition_repo, overlay_repo, preference_repo) = + make_service_with_mock_task_manager_and_assistant_support(task_mgr.clone()).await; + + upsert_test_assistant_definition( + &definition_repo, + "asstdef_acp_fixed", + "assistant-acp-fixed", + "codex", + "fixed", + "fixed", + ) + .await; + overlay_repo + .upsert(&UpsertAssistantOverlayParams { + definition_id: "asstdef_acp_fixed", + enabled: true, + sort_order: 0, + agent_backend_override: None, + last_used_at: None, + }) + .await + .unwrap(); + preference_repo + .upsert(&UpsertAssistantPreferenceParams { + definition_id: "asstdef_acp_fixed", + last_model_id: Some("legacy-fixed-model"), + last_permission_value: Some("legacy-fixed-mode"), + last_skill_ids: "[]", + last_disabled_builtin_skill_ids: "[]", + last_mcp_ids: "[]", + }) + .await + .unwrap(); + + let conv = create_assistant_backed_conversation(&svc, "user_1", "acp", "codex", "assistant-acp-fixed").await; + let agent = Arc::new(MockAgent::new(&conv.id)); + task_mgr.insert_agent(&conv.id, AgentInstance::Mock(agent)); + + svc.set_config_option( + &conv.id, + "model", + SetConfigOptionRequest { + value: "transient-model".to_owned(), + }, + ) + .await + .unwrap(); + svc.set_config_option( + &conv.id, + "mode", + SetConfigOptionRequest { + value: "transient-mode".to_owned(), + }, + ) + .await + .unwrap(); + + let pref = preference_repo.get("asstdef_acp_fixed").await.unwrap().unwrap(); + assert_eq!(pref.last_model_id.as_deref(), Some("legacy-fixed-model")); + assert_eq!(pref.last_permission_value.as_deref(), Some("legacy-fixed-mode")); + // The snapshot still tracks the runtime override so the active session reflects it, + // even though the persisted assistant preference must not change for fixed defaults. + let snapshot = repo.get_assistant_snapshot(&conv.id).await.unwrap().unwrap(); + assert_eq!(snapshot.resolved_model_id.as_deref(), Some("transient-model")); + assert_eq!(snapshot.resolved_permission_value.as_deref(), Some("transient-mode")); +} + +#[tokio::test] +async fn set_config_option_command_ack_does_not_persist_assistant_preference() { + let task_mgr = Arc::new(MockTaskManager::new()); + let (svc, _broadcaster, repo, definition_repo, overlay_repo, preference_repo) = + make_service_with_mock_task_manager_and_assistant_support(task_mgr.clone()).await; + + upsert_test_assistant_definition( + &definition_repo, + "asstdef_acp_ack", + "assistant-acp-ack", + "codex", + "auto", + "auto", + ) + .await; + overlay_repo + .upsert(&UpsertAssistantOverlayParams { + definition_id: "asstdef_acp_ack", + enabled: true, + sort_order: 0, + agent_backend_override: None, + last_used_at: None, + }) + .await + .unwrap(); + preference_repo + .upsert(&UpsertAssistantPreferenceParams { + definition_id: "asstdef_acp_ack", + last_model_id: Some("legacy-ack-model"), + last_permission_value: Some("legacy-ack-mode"), + last_skill_ids: "[]", + last_disabled_builtin_skill_ids: "[]", + last_mcp_ids: "[]", + }) + .await + .unwrap(); + + let conv = create_assistant_backed_conversation(&svc, "user_1", "acp", "codex", "assistant-acp-ack").await; + let agent = Arc::new( + MockAgent::new(&conv.id).with_set_config_option_response(SetConfigOptionResponse { + confirmation: ConfigOptionConfirmation::CommandAck, + config_options: None, + }), + ); + task_mgr.insert_agent(&conv.id, AgentInstance::Mock(agent)); + + svc.set_config_option( + &conv.id, + "model", + SetConfigOptionRequest { + value: "ack-only-model".to_owned(), + }, + ) + .await + .unwrap(); + + let pref = preference_repo.get("asstdef_acp_ack").await.unwrap().unwrap(); + assert_eq!(pref.last_model_id.as_deref(), Some("legacy-ack-model")); + assert_eq!(pref.last_permission_value.as_deref(), Some("legacy-ack-mode")); + let snapshot = repo.get_assistant_snapshot(&conv.id).await.unwrap().unwrap(); + assert_eq!(snapshot.resolved_model_id.as_deref(), Some("legacy-ack-model")); +} + #[tokio::test] async fn update_aionrs_model_updates_assistant_preference_only_when_snapshot_model_mode_is_auto() { let task_mgr = Arc::new(MockTaskManager::new());