diff --git a/docs/architecture/peer-device-mode.md b/docs/architecture/peer-device-mode.md index 5b45355f18..bfa46988db 100644 --- a/docs/architecture/peer-device-mode.md +++ b/docs/architecture/peer-device-mode.md @@ -161,6 +161,44 @@ applying or uploading settings, a host fans out `account://settings-applied` to attached controllers; the controller re-emits it locally so the frontend config cache and model selectors refresh without reconnecting. +The sync engine subscribes to successful local mutations at `ConfigService`, +in addition to legacy host notifications. This covers model, Skill, Agent +profile, and individual preference mutations through Desktop and CLI. Failed +writes, runtime-only credentials, reloads, and cloud imports do not emit this +local-change signal. Pending local edits take priority over the periodic pull; +a fetched blob is applied only if the local document still matches its +pre-fetch snapshot. The comparison and import share the config write lock. + +Older settings snapshots may omit fixed fields introduced by a newer build. +Imports preserve those local fields instead of replacing them with defaults. +Supplied arrays and dynamic maps remain authoritative, so deleted models, +profiles and list entries are not resurrected. Optional/default-elided fields +retain their existing reset semantics; an explicit raw backup restore also +honors omitted default memory and AI preferences. Legacy renamed fields still +pass through their migrations before values at the new names are preserved. + +Realtime voice credentials live in `app.voice_call` in the same persisted +configuration and export/backup format as model settings. Account settings +apply preserves the controller's existing voice fields when an older payload +omits them, and an empty voice API key from an unconfigured host does not erase +a configured local key. Non-empty synced keys still replace the local key. +Explicit file imports can restore or clear a supplied key; local voice saves +and resets can also clear it. A valid whole-config import creates a raw +`app_pre-import_*.json` backup before replacement, under the existing backup +retention policy. Config reload and model-reference reconciliation serialize +their reads and writes with local saves so stale snapshots cannot undo a +completed credential save. These rules do not change speech command routing: +capture, configuration and realtime connections remain on the controller. + +Config mutations publish in-memory values and change notifications only after +atomic persistence succeeds. Model CRUD and Agent/Skill map edits use a shared +read/modify/write operation; startup profile canonicalization updates only its +map. User backups have unique names even within the same second. Web UI reads +resolve legacy model metadata without writing it back, model edits read fresh +host data inside the client mutation queue, and AI-experience controls save +only edited fields. An explicit empty quick-action list stays empty across +reloads; defaults are supplied only when absent or when explicitly reset. + SSH `WorkspaceKind.Remote` remains a separate path (local session mirror + remote FS) and must not be mixed with Peer Device Mode. diff --git a/src/apps/desktop/src/api/config_api.rs b/src/apps/desktop/src/api/config_api.rs index b12c2f21cb..cd95bf1916 100644 --- a/src/apps/desktop/src/api/config_api.rs +++ b/src/apps/desktop/src/api/config_api.rs @@ -303,10 +303,11 @@ pub async fn import_config( match config_service.import_config_data(config_data).await { Ok(result) => { - state.ai_client_factory.invalidate_cache(); - info!("Config imported, AI client cache invalidated"); - // Notify auto-sync: config changed, upload to relay - crate::api::remote_connect_api::notify_settings_changed(); + if result.success { + state.ai_client_factory.invalidate_cache(); + info!("Config imported, AI client cache invalidated"); + crate::api::remote_connect_api::notify_settings_changed(); + } Ok(to_json_value(result, "import config result")?) } Err(e) => { diff --git a/src/apps/desktop/src/api/custom_agent_api.rs b/src/apps/desktop/src/api/custom_agent_api.rs index 413db1f7b5..45d6701220 100644 --- a/src/apps/desktop/src/api/custom_agent_api.rs +++ b/src/apps/desktop/src/api/custom_agent_api.rs @@ -394,36 +394,25 @@ pub async fn delete_custom_agent( let config_service = &state.config_service; - let mut agent_profiles: serde_json::Map = config_service - .get_config(Some("ai.agent_profiles")) - .await - .unwrap_or_default(); - if agent_profiles.remove(&agent_id).is_some() { - if let Err(error) = config_service - .set_config("ai.agent_profiles", &agent_profiles) - .await - { - warn!( - "Failed to clean up ai.agent_profiles after custom agent deletion: agent_id={}, error={}", - agent_id, error - ); - } - } - - let default_mode_id: Option = config_service - .get_config(Some("app.flow_chat.default_mode_id")) + if let Err(error) = config_service + .update_config( + "", + |config: &mut bitfun_core::service::config::GlobalConfig| { + config.ai.agent_profiles.remove(&agent_id); + if config.app.flow_chat.default_mode_id.as_deref() == Some(agent_id.as_str()) { + config.app.flow_chat.default_mode_id = None; + } + Ok(()) + }, + ) .await - .unwrap_or_default(); - if default_mode_id.as_deref() == Some(agent_id.as_str()) { - if let Err(error) = config_service - .set_config("app.flow_chat.default_mode_id", Option::::None) - .await - { - warn!( - "Failed to clear default chat input mode after custom agent deletion: agent_id={}, error={}", - agent_id, error - ); - } + { + warn!( + "Failed to clean up config after custom agent deletion: agent_id={}, error={}", + agent_id, error + ); + } else { + crate::api::remote_connect_api::notify_settings_changed(); } if let Err(error) = bitfun_core::service::config::reload_global_config().await { diff --git a/src/apps/desktop/src/tray.rs b/src/apps/desktop/src/tray.rs index b5ca83afcb..93a72c8f5c 100644 --- a/src/apps/desktop/src/tray.rs +++ b/src/apps/desktop/src/tray.rs @@ -138,26 +138,20 @@ async fn tray_toggle_desktop_pet(app: &AppHandle) -> Result<(), String> { .ok_or_else(|| "AppState not available".to_string())?; let config_service = &app_state.config_service; - let mut exp: AIExperienceConfig = config_service - .get_config(Some("app.ai_experience")) - .await - .map_err(|e| e.to_string())?; - - let desktop_on = desktop_pet_should_show(&exp); - - if desktop_on { - exp.enable_agent_companion = false; - } else { - exp.enable_agent_companion = true; - exp.agent_companion_display_mode = "desktop".to_string(); - } - - config_service - .set_config("app.ai_experience", &exp) + let show = config_service + .update_config("app.ai_experience", |exp: &mut AIExperienceConfig| { + if desktop_pet_should_show(exp) { + exp.enable_agent_companion = false; + } else { + exp.enable_agent_companion = true; + exp.agent_companion_display_mode = "desktop".to_string(); + } + Ok(desktop_pet_should_show(exp)) + }) .await .map_err(|e| e.to_string())?; + crate::api::remote_connect_api::notify_settings_changed(); - let show = desktop_pet_should_show(&exp); if show { crate::appearance::show_agent_companion_desktop_pet(app.clone()).await?; } else { diff --git a/src/crates/assembly/core/AGENTS.md b/src/crates/assembly/core/AGENTS.md index df45991f0b..00af6591e3 100644 --- a/src/crates/assembly/core/AGENTS.md +++ b/src/crates/assembly/core/AGENTS.md @@ -197,3 +197,19 @@ feature boundary changed, and the third for behavior. Run or test-target layout. Workspace checks and product-wide tests are CI-backed and are not the default Core precheck. For documentation-only changes, run `git diff --check`. + +Configuration persistence, account settings import, backup restore, legacy +field/deletion compatibility, local-change notifications, and save/reload/model +concurrency regressions have feature-free fixtures: + +```bash +cargo test -p bitfun-core --no-default-features --lib service::config:: +``` + +The account sync adapter requires `remote-connect`, which also covers +Agent-profile canonicalization in the focused configuration suite: + +```bash +cargo test -p bitfun-core --no-default-features --features remote-connect --lib service::config:: +cargo test -p bitfun-core --no-default-features --features remote-connect --lib service::remote_connect::settings_sync::tests +``` diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/skills/mode_overrides.rs b/src/crates/assembly/core/src/agentic/tools/implementations/skills/mode_overrides.rs index 3212d395ec..ae553dd515 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/skills/mode_overrides.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/skills/mode_overrides.rs @@ -108,27 +108,23 @@ pub async fn set_global_user_skill_disabled( } let config_service = GlobalConfigManager::get_service().await?; - let mut settings: SkillSettingsConfig = config_service - .get_config(Some("ai.skill_settings")) - .await - .unwrap_or_default(); - - if disabled { - settings - .globally_disabled_user_skills - .push(skill_key.to_string()); - } else { - settings - .globally_disabled_user_skills - .retain(|key| key != skill_key); - } - settings.globally_disabled_user_skills = - normalize_skill_keys(settings.globally_disabled_user_skills); - config_service - .set_config("ai.skill_settings", &settings) - .await?; - Ok(settings.globally_disabled_user_skills) + .update_config("ai.skill_settings", |settings: &mut SkillSettingsConfig| { + if disabled { + settings + .globally_disabled_user_skills + .push(skill_key.to_string()); + } else { + settings + .globally_disabled_user_skills + .retain(|key| key != skill_key); + } + settings.globally_disabled_user_skills = + normalize_skill_keys(std::mem::take(&mut settings.globally_disabled_user_skills)); + + Ok(settings.globally_disabled_user_skills.clone()) + }) + .await } pub fn project_mode_skills_path_for_remote(remote_root: &str) -> String { diff --git a/src/crates/assembly/core/src/service/config/manager.rs b/src/crates/assembly/core/src/service/config/manager.rs index 0e9878ddc0..e1d91c8048 100644 --- a/src/crates/assembly/core/src/service/config/manager.rs +++ b/src/crates/assembly/core/src/service/config/manager.rs @@ -225,6 +225,12 @@ pub struct ConfigManagerSettings { pub backup_count: usize, } +#[derive(Clone, Copy, PartialEq, Eq)] +pub(crate) enum ConfigImportSource { + Explicit, + AccountSync, +} + impl Default for ConfigManagerSettings { fn default() -> Self { Self { @@ -278,6 +284,18 @@ impl ConfigManager { &self.path_manager } + /// Reloads from the same storage root while the service holds its write + /// lock, so a concurrent save cannot be replaced by an older disk read. + pub(crate) async fn reload(&mut self) -> BitFunResult<()> { + let settings = ConfigManagerSettings { + path_manager: Some(self.path_manager.clone()), + auto_save: true, + backup_count: self.backup_count, + }; + *self = Self::new(settings).await?; + Ok(()) + } + /// Loads or creates the configuration file. async fn load_or_create_config(&mut self) -> BitFunResult<()> { if self.config_file.exists() { @@ -486,7 +504,11 @@ impl ConfigManager { /// Saves the configuration file. async fn save_config(&self) -> BitFunResult<()> { - let content = serde_json::to_string_pretty(&config_value_for_persistence(&self.config)?) + self.persist_config(&self.config).await + } + + async fn persist_config(&self, config: &GlobalConfig) -> BitFunResult<()> { + let content = serde_json::to_string_pretty(&config_value_for_persistence(config)?) .map_err(|e| BitFunError::config(format!("Config serialization failed: {}", e)))?; if let Some(parent) = self.config_file.parent() { @@ -518,13 +540,17 @@ impl ConfigManager { fs::create_dir_all(&backup_dir) .await .map_err(|e| BitFunError::config(format!("Failed to create backup directory: {e}")))?; - let backup_file = backup_dir.join(format!("app_{reason}_{timestamp}.json")); + let backup_file = backup_dir.join(format!( + "app_{reason}_{timestamp}_{}.json", + uuid::Uuid::new_v4().simple() + )); fs::write(&backup_file, content) .await .map_err(|e| BitFunError::config(format!("Failed to write config backup: {e}")))?; self.prune_backups(&backup_dir).await?; info!( - "Created pre-repair config backup: path={}", + "Created config backup: reason={}, path={}", + reason, backup_file.display() ); Ok(backup_file) @@ -592,88 +618,73 @@ impl ConfigManager { where T: serde::Serialize, { - let old_config = self.config.clone(); let json_value = serde_json::to_value(value) .map_err(|e| BitFunError::config(format!("Failed to serialize config value: {}", e)))?; let path = canonical_config_path(path); - self.set_value_by_path(path, json_value)?; + let mut config = self.config_with_value(path, json_value)?; // Apply capability-driven canonicalization before validation and persistence. // Speech/embedding/image-only models must never carry text-generation sentinels. - normalize_typed_config(&mut self.config); - self.config.last_modified = chrono::Utc::now(); + normalize_typed_config(&mut config); - let validation_result = match self.validate_config().await { - Ok(result) => result, - Err(error) => { - self.config = old_config; - return Err(error); - } - }; + let validation_result = self.providers.validate_config(&config).await?; if !validation_result.valid { - self.config = old_config; return Err(invalid_config_error( "Invalid configuration update", &validation_result, )); } - if path.is_empty() { - for provider_name in self.providers.get_provider_names() { - self.notify_config_changed(&provider_name, &old_config) - .await?; - } - } else { - self.notify_config_changed(path, &old_config).await?; - } - - self.save_config().await?; - - Ok(()) + self.commit_config(config, Some(path)).await } /// Resets configuration (supports dot-paths). pub async fn reset(&mut self, path: Option<&str>) -> BitFunResult<()> { - let old_config = self.config.clone(); - - if let Some(path) = path { + let config = if let Some(path) = path { let path = canonical_config_path(path); let default_config = self.providers.get_default_config(); let default_value = self.get_value_by_path_from_config(&default_config, path)?; - self.set_value_by_path(path, default_value)?; + self.config_with_value(path, default_value)? } else { - self.config = self.providers.get_default_config(); - } - - self.config.last_modified = chrono::Utc::now(); - - let validation_result = match self.validate_config().await { - Ok(result) => result, - Err(error) => { - self.config = old_config; - return Err(error); - } + self.providers.get_default_config() }; + + let validation_result = self.providers.validate_config(&config).await?; if !validation_result.valid { - self.config = old_config; return Err(invalid_config_error( "Invalid configuration reset", &validation_result, )); } - if let Some(path) = path { - let path = canonical_config_path(path); - self.notify_config_changed(path, &old_config).await?; - } else { - for provider_name in self.providers.get_provider_names() { - self.notify_config_changed(&provider_name, &old_config) - .await?; + self.commit_config(config, path.map(canonical_config_path)) + .await + } + + /// Publish only a configuration that has reached disk. Failed writes must + /// not change reads, runtime subscribers, or a subsequent unrelated save. + async fn commit_config( + &mut self, + mut config: GlobalConfig, + path: Option<&str>, + ) -> BitFunResult<()> { + config.last_modified = chrono::Utc::now(); + self.persist_config(&config).await?; + let old_config = std::mem::replace(&mut self.config, config); + let paths = match path.filter(|path| !path.is_empty()) { + Some(path) => vec![path.to_string()], + None => self.providers.get_provider_names(), + }; + for path in paths { + // A subscriber failure cannot undo an already committed file. Keep + // notifying the other providers and report the refresh failure. + if let Err(error) = self.notify_config_changed(&path, &old_config).await { + warn!( + "Configuration saved but change notification failed: path={}, error={}", + path, error + ); } } - - self.save_config().await?; - Ok(()) } @@ -699,12 +710,20 @@ impl ConfigManager { /// Imports configuration. pub async fn import_config(&mut self, config_data: serde_json::Value) -> BitFunResult<()> { - let old_config = self.config.clone(); + self.import_config_from_source(config_data, ConfigImportSource::Explicit) + .await + } + + pub(crate) async fn import_config_from_source( + &mut self, + mut config_data: Value, + source: ConfigImportSource, + ) -> BitFunResult<()> { + self.preserve_imported_settings(&mut config_data, source)?; let normalized = normalize_config_value(config_data); reject_unsupported_schema(&normalized.diagnostics)?; - let config_data = normalized.value; - let mut imported_config: GlobalConfig = serde_json::from_value(config_data) + let mut imported_config: GlobalConfig = serde_json::from_value(normalized.value) .map_err(|e| BitFunError::config(format!("Failed to parse imported config: {}", e)))?; let mut import_diagnostics = normalized.diagnostics; @@ -720,18 +739,100 @@ impl ConfigManager { )); } - self.config = imported_config; + // Imports replace the whole document. Keep the exact previous file + // recoverable before a cloud apply or an explicit backup restore. + let previous_content = fs::read_to_string(&self.config_file).await.map_err(|e| { + BitFunError::config(format!("Failed to read config before import backup: {e}")) + })?; + self.backup_raw_config(&previous_content, "pre-import") + .await?; + + self.commit_config(imported_config, None).await?; self.load_diagnostics = import_diagnostics; - self.config.last_modified = chrono::Utc::now(); - for provider_name in self.providers.get_provider_names() { - self.notify_config_changed(&provider_name, &old_config) - .await?; + info!("Successfully imported configuration"); + Ok(()) + } + + fn preserve_imported_settings( + &self, + config_data: &mut Value, + source: ConfigImportSource, + ) -> BitFunResult<()> { + // Exported account snapshots serialize fixed defaults in full. Raw + // on-disk backups intentionally elide memory/default AI preferences; + // their omissions must continue to mean reset when explicitly restored. + let mut shape = match source { + ConfigImportSource::AccountSync => serde_json::to_value(GlobalConfig::default())?, + ConfigImportSource::Explicit => config_value_for_persistence(&GlobalConfig::default())?, + }; + let root = shape + .as_object_mut() + .expect("GlobalConfig serializes as an object"); + for key in ["version", "schema_version", "last_modified"] { + root.remove(key); + } + // This optional record has a non-empty default, but None is omitted on + // export. Do not mistake that intentional omission for an unknown field. + shape["app"]["ai_experience"] + .as_object_mut() + .unwrap() + .remove("agent_companion_pet"); + + // Let actual legacy values reach their migrations before supplying a + // local value at the new name; otherwise preservation masks the rename. + if config_data.pointer("/ai/agent_models").is_some() { + shape["ai"] + .as_object_mut() + .unwrap() + .remove("agent_model_defaults"); } + if config_data.pointer("/ai/skip_tool_confirmation").is_some() { + shape.as_object_mut().unwrap().remove("tool_permissions"); + } + preserve_missing_config_fields( + config_data, + &serde_json::to_value(&self.config)?, + &shape, + "", + ); + self.preserve_imported_voice_config(config_data, source) + } - self.save_config().await?; + fn preserve_imported_voice_config( + &self, + config_data: &mut Value, + source: ConfigImportSource, + ) -> BitFunResult<()> { + let Some(root) = config_data.as_object_mut() else { + return Ok(()); + }; + let app = root.entry("app").or_insert_with(|| serde_json::json!({})); + let Some(app) = app.as_object_mut() else { + return Ok(()); + }; + let voice = app + .entry("voice_call") + .or_insert_with(|| serde_json::json!({})); + let Some(fields) = voice.as_object_mut() else { + return Ok(()); + }; - info!("Successfully imported configuration"); + // Older hosts omit this section; newer unconfigured hosts export an + // empty key. Neither is a request to erase this controller's saved + // credential. Explicit imports and local set/reset still honor clears. + if source == ConfigImportSource::AccountSync + && fields + .get("api_key") + .and_then(Value::as_str) + .is_some_and(|key| key.trim().is_empty()) + { + fields.remove("api_key"); + } + *voice = deep_merge( + serde_json::to_value(&self.config.app.voice_call)?, + voice.take(), + ); Ok(()) } @@ -746,12 +847,17 @@ impl ConfigManager { })?; } - let backup_file = backup_dir.join(format!("config_backup_{}.json", timestamp)); + let backup_file = backup_dir.join(format!( + "config_backup_{}_{}.json", + timestamp, + uuid::Uuid::new_v4().simple() + )); let content = serde_json::to_string_pretty(&config_value_for_persistence(&self.config)?) .map_err(|e| BitFunError::config(format!("Failed to serialize backup: {}", e)))?; - fs::write(&backup_file, content) + JsonFileStore + .write_text_atomic_create_new(&backup_file, &content) .await .map_err(|e| BitFunError::config(format!("Failed to write backup: {}", e)))?; @@ -789,6 +895,10 @@ impl ConfigManager { let config_value = serde_json::to_value(config) .map_err(|e| BitFunError::config(format!("Failed to serialize config: {}", e)))?; + if path.is_empty() { + return Ok(config_value); + } + let keys: Vec<&str> = path.split('.').collect(); let mut current = &config_value; @@ -801,12 +911,15 @@ impl ConfigManager { Ok(current.clone()) } - /// Sets a configuration value by dot-path. - fn set_value_by_path(&mut self, path: &str, value: serde_json::Value) -> BitFunResult<()> { + /// Builds a candidate configuration without changing the committed value. + fn config_with_value( + &self, + path: &str, + value: serde_json::Value, + ) -> BitFunResult { if path.is_empty() { - self.config = serde_json::from_value(value) - .map_err(|e| BitFunError::config(format!("Failed to deserialize config: {}", e)))?; - return Ok(()); + return serde_json::from_value(value) + .map_err(|e| BitFunError::config(format!("Failed to deserialize config: {}", e))); } let mut config_value = serde_json::to_value(&self.config) @@ -814,9 +927,8 @@ impl ConfigManager { let keys: Vec<&str> = path.split('.').filter(|k| !k.is_empty()).collect(); if keys.is_empty() { - self.config = serde_json::from_value(value) - .map_err(|e| BitFunError::config(format!("Failed to deserialize config: {}", e)))?; - return Ok(()); + return serde_json::from_value(value) + .map_err(|e| BitFunError::config(format!("Failed to deserialize config: {}", e))); } let last_key = keys.last().ok_or_else(|| { @@ -840,11 +952,9 @@ impl ConfigManager { ))); } - self.config = serde_json::from_value(config_value).map_err(|e| { + serde_json::from_value(config_value).map_err(|e| { BitFunError::config(format!("Failed to deserialize updated config: {}", e)) - })?; - - Ok(()) + }) } /// Notifies about a configuration change. @@ -915,6 +1025,47 @@ impl ConfigManager { } } +/// Fill absent fixed fields only. Arrays, dynamic maps, tagged selections, +/// credentials bound to an endpoint, and permission-policy documents remain +/// authoritative replacements when supplied. A blanket deep merge resurrects +/// deleted entries and changes the meaning of explicit reset payloads. +fn preserve_missing_config_fields(incoming: &mut Value, local: &Value, shape: &Value, path: &str) { + if matches!( + path, + "ai.review_teams" + | "ai.agent_model_defaults.subagents.builtin" + | "ai.proxy" + | "tool_permissions" + ) { + return; + } + let (Some(incoming), Some(local), Some(shape)) = ( + incoming.as_object_mut(), + local.as_object(), + shape.as_object(), + ) else { + return; + }; + if shape.contains_key("kind") || shape.contains_key("type") { + return; + } + for (key, child_shape) in shape { + let Some(local_value) = local.get(key) else { + continue; + }; + if let Some(value) = incoming.get_mut(key) { + let child_path = if path.is_empty() { + key.clone() + } else { + format!("{path}.{key}") + }; + preserve_missing_config_fields(value, local_value, child_shape, &child_path); + } else { + incoming.insert(key.clone(), local_value.clone()); + } + } +} + /// Configuration statistics. #[derive(Debug, Serialize, Deserialize)] pub struct ConfigStatistics { diff --git a/src/crates/assembly/core/src/service/config/mode_config_canonicalizer.rs b/src/crates/assembly/core/src/service/config/mode_config_canonicalizer.rs index 7035702961..331d1df805 100644 --- a/src/crates/assembly/core/src/service/config/mode_config_canonicalizer.rs +++ b/src/crates/assembly/core/src/service/config/mode_config_canonicalizer.rs @@ -379,153 +379,160 @@ pub async fn get_agent_profile_view(agent_id: &str) -> BitFunResult BitFunResult<()> { let config_service = GlobalConfigManager::get_service().await?; - let mut stored_configs = get_agent_profile_configs().await?; let agent_defaults = get_agent_defaults().await; let default_tools = agent_defaults .get(agent_id) .ok_or_else(|| BitFunError::config(format!("Agent does not exist: {}", agent_id)))?; let valid_tools = get_valid_tool_names().await; let profile_id = resolve_profile_id(agent_id); - let current = stored_configs.get(&profile_id); - - let enabled_tools = if let Some(tools) = config.get("enabled_tools") { - serde_json::from_value::>(tools.clone()).map_err(|error| { - BitFunError::config(format!( - "Invalid enabled_tools for mode '{}': {}", - agent_id, error - )) - })? - } else { - resolve_effective_tools(default_tools, current, &valid_tools) - }; - - let disabled_user_skills = if config - .as_object() - .map(|obj| obj.contains_key("disabled_user_skills")) - .unwrap_or(false) - { - match config.get("disabled_user_skills") { - Some(Value::Null) | None => Vec::new(), - Some(value) => { - serde_json::from_value::>(value.clone()).map_err(|error| { - BitFunError::config(format!( - "Invalid disabled_user_skills for mode '{}': {}", - agent_id, error - )) - })? - } - } - } else { - current - .map(|item| item.disabled_user_skills.clone()) - .unwrap_or_default() - }; - - let enabled_user_skills = if config - .as_object() - .map(|obj| obj.contains_key("enabled_user_skills")) - .unwrap_or(false) - { - match config.get("enabled_user_skills") { - Some(Value::Null) | None => Vec::new(), - Some(value) => { - serde_json::from_value::>(value.clone()).map_err(|error| { - BitFunError::config(format!( - "Invalid enabled_user_skills for mode '{}': {}", - agent_id, error - )) - })? - } - } - } else { - current - .map(|item| item.enabled_user_skills.clone()) - .unwrap_or_default() - }; - - let subagent_overrides = if config - .as_object() - .map(|obj| obj.contains_key("subagent_overrides")) - .unwrap_or(false) - { - match config.get("subagent_overrides") { - Some(Value::Null) | None => ParentSubagentOverrideConfig::new(), - Some(value) => serde_json::from_value::(value.clone()) - .map_err(|error| { - BitFunError::config(format!( - "Invalid subagent_overrides for mode '{}': {}", - agent_id, error - )) - })?, - } - } else { - current - .map(|item| item.subagent_overrides.clone()) - .unwrap_or_default() - }; - - let tool_permission_rules = if config - .as_object() - .map(|obj| obj.contains_key("tool_permission_rules")) - .unwrap_or(false) - { - match config.get("tool_permission_rules") { - Some(Value::Null) | None => Vec::new(), - Some(value) => { - serde_json::from_value::>(value.clone()).map_err(|error| { - BitFunError::config(format!( - "Invalid tool_permission_rules for mode '{}': {}", - agent_id, error - )) - })? - } - } - } else { - current - .map(|item| item.tool_permission_rules.clone()) - .unwrap_or_default() - }; - - if let Some(canonical) = stored_agent_profile_from_tool_selection( - agent_id, - enabled_tools, - disabled_user_skills, - enabled_user_skills, - subagent_overrides, - tool_permission_rules, - default_tools, - &valid_tools, - ) { - stored_configs.insert(profile_id, canonical); - } else { - stored_configs.remove(&profile_id); - } - config_service - .set_config("ai.agent_profiles", stored_configs) + .update_config( + "ai.agent_profiles", + |stored_configs: &mut HashMap| { + let current = stored_configs.get(&profile_id); + + let enabled_tools = if let Some(tools) = config.get("enabled_tools") { + serde_json::from_value::>(tools.clone()).map_err(|error| { + BitFunError::config(format!( + "Invalid enabled_tools for mode '{}': {}", + agent_id, error + )) + })? + } else { + resolve_effective_tools(default_tools, current, &valid_tools) + }; + + let disabled_user_skills = if config + .as_object() + .map(|obj| obj.contains_key("disabled_user_skills")) + .unwrap_or(false) + { + match config.get("disabled_user_skills") { + Some(Value::Null) | None => Vec::new(), + Some(value) => serde_json::from_value::>(value.clone()) + .map_err(|error| { + BitFunError::config(format!( + "Invalid disabled_user_skills for mode '{}': {}", + agent_id, error + )) + })?, + } + } else { + current + .map(|item| item.disabled_user_skills.clone()) + .unwrap_or_default() + }; + + let enabled_user_skills = if config + .as_object() + .map(|obj| obj.contains_key("enabled_user_skills")) + .unwrap_or(false) + { + match config.get("enabled_user_skills") { + Some(Value::Null) | None => Vec::new(), + Some(value) => serde_json::from_value::>(value.clone()) + .map_err(|error| { + BitFunError::config(format!( + "Invalid enabled_user_skills for mode '{}': {}", + agent_id, error + )) + })?, + } + } else { + current + .map(|item| item.enabled_user_skills.clone()) + .unwrap_or_default() + }; + + let subagent_overrides = if config + .as_object() + .map(|obj| obj.contains_key("subagent_overrides")) + .unwrap_or(false) + { + match config.get("subagent_overrides") { + Some(Value::Null) | None => ParentSubagentOverrideConfig::new(), + Some(value) => { + serde_json::from_value::(value.clone()) + .map_err(|error| { + BitFunError::config(format!( + "Invalid subagent_overrides for mode '{}': {}", + agent_id, error + )) + })? + } + } + } else { + current + .map(|item| item.subagent_overrides.clone()) + .unwrap_or_default() + }; + + let tool_permission_rules = if config + .as_object() + .map(|obj| obj.contains_key("tool_permission_rules")) + .unwrap_or(false) + { + match config.get("tool_permission_rules") { + Some(Value::Null) | None => Vec::new(), + Some(value) => serde_json::from_value::>(value.clone()) + .map_err(|error| { + BitFunError::config(format!( + "Invalid tool_permission_rules for mode '{}': {}", + agent_id, error + )) + })?, + } + } else { + current + .map(|item| item.tool_permission_rules.clone()) + .unwrap_or_default() + }; + + if let Some(canonical) = stored_agent_profile_from_tool_selection( + agent_id, + enabled_tools, + disabled_user_skills, + enabled_user_skills, + subagent_overrides, + tool_permission_rules, + default_tools, + &valid_tools, + ) { + stored_configs.insert(profile_id, canonical); + } else { + stored_configs.remove(&profile_id); + } + + Ok(()) + }, + ) .await } pub async fn reset_agent_profile_to_default(agent_id: &str) -> BitFunResult<()> { let config_service = GlobalConfigManager::get_service().await?; - let mut stored_configs = get_agent_profile_configs().await?; let profile_id = resolve_profile_id(agent_id); - if let Some(current) = stored_configs.get_mut(&profile_id) { - current.added_tools.clear(); - current.removed_tools.clear(); - - if current.disabled_user_skills.is_empty() - && current.enabled_user_skills.is_empty() - && current.subagent_overrides.is_empty() - && current.tool_permission_rules.is_empty() - { - stored_configs.remove(&profile_id); - } - } - config_service - .set_config("ai.agent_profiles", stored_configs) + .update_config( + "ai.agent_profiles", + |stored_configs: &mut HashMap| { + if let Some(current) = stored_configs.get_mut(&profile_id) { + current.added_tools.clear(); + current.removed_tools.clear(); + + if current.disabled_user_skills.is_empty() + && current.enabled_user_skills.is_empty() + && current.subagent_overrides.is_empty() + && current.tool_permission_rules.is_empty() + { + stored_configs.remove(&profile_id); + } + } + + Ok(()) + }, + ) .await } @@ -535,70 +542,64 @@ pub async fn canonicalize_agent_profile_configs( let config_service = GlobalConfigManager::get_service().await?; let valid_tools = get_valid_tool_names().await; let profile_defaults = get_profile_defaults().await; - let mut ai_value: Value = config_service.get_config(Some("ai")).await?; - let original_ai_value = ai_value.clone(); - let ai_object = ai_value - .as_object_mut() - .ok_or_else(|| BitFunError::config("AI config must be a JSON object".to_string()))?; - - let raw_agent_profiles = ai_object - .get("agent_profiles") - .and_then(Value::as_object) - .cloned() - .unwrap_or_default(); - - let mut rewritten_agent_profiles = Map::new(); - let mut updated_profiles = Vec::new(); - let mut removed_profile_configs = Vec::new(); - - for (profile_id, default_tools) in &profile_defaults { - let raw_profile = raw_agent_profiles.get(profile_id); - let canonical = - canonicalize_agent_profile(profile_id, raw_profile, default_tools, &valid_tools)?; - if let Some(config) = canonical { - if raw_profile.is_some() { - updated_profiles.push(AgentProfileConfigUpdateInfo { - profile_id: profile_id.clone(), - added_tools: config.added_tools.clone(), - removed_tools: config.removed_tools.clone(), - }); - } - rewritten_agent_profiles.insert(profile_id.clone(), serde_json::to_value(config)?); - } else if raw_profile.is_some() { - removed_profile_configs.push(profile_id.clone()); - } - } - - // Profiles we cannot resolve defaults for are kept, not dropped. Canonicalization - // runs at startup with no workspace, so project-scoped sub-agents are invisible - // here; pruning them would silently discard the user's stored selection every - // launch. Records that no longer deserialize are still removed, so one bad entry - // cannot take the whole map down when it is read back. - for (profile_id, raw_profile) in &raw_agent_profiles { - if profile_defaults.contains_key(profile_id) { - continue; - } - match serde_json::from_value::(raw_profile.clone()) { - Ok(config) => { - rewritten_agent_profiles.insert(profile_id.clone(), serde_json::to_value(config)?); - } - Err(_) => removed_profile_configs.push(profile_id.clone()), - } - } - - ai_object.insert( - "agent_profiles".to_string(), - Value::Object(rewritten_agent_profiles), - ); - - if ai_value != original_ai_value { - config_service.set_config("ai", ai_value).await?; - } - - Ok(AgentProfileConfigCanonicalizationReport { - removed_profile_configs, - updated_profiles, - }) + config_service + .update_config( + "ai.agent_profiles", + |raw_agent_profiles: &mut Map| { + let mut rewritten_agent_profiles = Map::new(); + let mut updated_profiles = Vec::new(); + let mut removed_profile_configs = Vec::new(); + + for (profile_id, default_tools) in &profile_defaults { + let raw_profile = raw_agent_profiles.get(profile_id); + let canonical = canonicalize_agent_profile( + profile_id, + raw_profile, + default_tools, + &valid_tools, + )?; + if let Some(config) = canonical { + if raw_profile.is_some() { + updated_profiles.push(AgentProfileConfigUpdateInfo { + profile_id: profile_id.clone(), + added_tools: config.added_tools.clone(), + removed_tools: config.removed_tools.clone(), + }); + } + rewritten_agent_profiles + .insert(profile_id.clone(), serde_json::to_value(config)?); + } else if raw_profile.is_some() { + removed_profile_configs.push(profile_id.clone()); + } + } + + // Profiles we cannot resolve defaults for are kept, not dropped. Canonicalization + // runs at startup with no workspace, so project-scoped sub-agents are invisible + // here; pruning them would silently discard the user's stored selection every + // launch. Records that no longer deserialize are still removed, so one bad entry + // cannot take the whole map down when it is read back. + for (profile_id, raw_profile) in raw_agent_profiles.iter() { + if profile_defaults.contains_key(profile_id) { + continue; + } + match serde_json::from_value::(raw_profile.clone()) { + Ok(config) => { + rewritten_agent_profiles + .insert(profile_id.clone(), serde_json::to_value(config)?); + } + Err(_) => removed_profile_configs.push(profile_id.clone()), + } + } + + *raw_agent_profiles = rewritten_agent_profiles; + + Ok(AgentProfileConfigCanonicalizationReport { + removed_profile_configs, + updated_profiles, + }) + }, + ) + .await } pub fn agent_profile_member_mode_ids_for(agent_id: &str) -> Vec { diff --git a/src/crates/assembly/core/src/service/config/service.rs b/src/crates/assembly/core/src/service/config/service.rs index dbbd85ea64..c2dde23dd3 100644 --- a/src/crates/assembly/core/src/service/config/service.rs +++ b/src/crates/assembly/core/src/service/config/service.rs @@ -2,19 +2,20 @@ //! //! Provides comprehensive configuration management functionality. -use super::manager::{ConfigManager, ConfigManagerSettings, ConfigStatistics}; +use super::manager::{ConfigImportSource, ConfigManager, ConfigManagerSettings, ConfigStatistics}; use super::types::*; use crate::util::errors::*; use log::{info, warn}; use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; use std::sync::Arc; -use tokio::sync::RwLock; +use tokio::sync::{watch, RwLock}; /// Configuration service. pub struct ConfigService { manager: Arc>, runtime_ai_models: Arc>>, + local_changes: watch::Sender<()>, } /// Configuration import/export format. @@ -62,6 +63,7 @@ impl ConfigService { let service = Self { manager: Arc::new(RwLock::new(manager)), runtime_ai_models: Arc::new(RwLock::new(BTreeMap::new())), + local_changes: watch::channel(()).0, }; let recovered_with_defaults = service @@ -131,6 +133,13 @@ impl ConfigService { self.runtime_ai_models.write().await.remove(model_id); } + /// Subscribe to successful persisted user mutations. Account sync consumes + /// this signal so individual UI/CLI mutation adapters cannot forget to mark + /// settings dirty. Cloud applies and reloads do not echo back as local edits. + pub fn subscribe_local_changes(&self) -> watch::Receiver<()> { + self.local_changes.subscribe() + } + /// Sets a configuration value (supports dot-paths). /// /// When the path touches AI models / default model slots / agent-model @@ -144,7 +153,41 @@ impl ConfigService { let mut manager = self.manager.write().await; manager.set(path, value).await?; } + self.after_config_change(path).await; + Ok(()) + } + /// Reads, modifies, validates, and persists a section under one write lock. + /// Use this for mutations of lists/maps or partial changes to a settings + /// object; taking a snapshot with get_config before set_config can discard + /// a concurrent save. The closure must not perform IO or await other work. + pub async fn update_config( + &self, + path: &str, + update: impl FnOnce(&mut T) -> BitFunResult, + ) -> BitFunResult + where + T: serde::Serialize + serde::de::DeserializeOwned, + { + let (result, changed) = { + let mut manager = self.manager.write().await; + let before: serde_json::Value = manager.get(path)?; + let mut value: T = serde_json::from_value(before.clone())?; + let result = update(&mut value)?; + let after = serde_json::to_value(value)?; + let changed = before != after; + if changed { + manager.set(path, after).await?; + } + (result, changed) + }; + if changed { + self.after_config_change(path).await; + } + Ok(result) + } + + async fn after_config_change(&self, path: &str) { let model_configuration_changed = Self::path_touches_models(path); if model_configuration_changed { if let Err(e) = self.reconcile_models("set_config").await { @@ -158,8 +201,7 @@ impl ConfigService { ) .await; } - - Ok(()) + self.local_changes.send_replace(()); } /// Atomically replaces one JSON configuration value when its current value @@ -182,11 +224,13 @@ impl ConfigService { return Ok(false); } manager.set(path, replacement).await?; + self.local_changes.send_replace(()); Ok(true) } fn path_touches_models(path: &str) -> bool { - path == "ai" + path.is_empty() + || path == "ai" || path.starts_with("ai.models") || path.starts_with("ai.default_models") || path.starts_with("ai.agent_model_defaults") @@ -221,6 +265,8 @@ impl ConfigService { .await; } + self.local_changes.send_replace(()); + Ok(()) } @@ -261,13 +307,62 @@ impl ConfigService { /// Imports raw configuration JSON. Keeping this boundary raw preserves /// legacy fields that are intentionally normalized before deserialization. + /// Missing realtime voice fields retain their local values; explicit empty + /// credentials in a user-imported backup are still authoritative. pub async fn import_config_data( &self, config_data: serde_json::Value, + ) -> BitFunResult { + self.import_config_data_from_source(config_data, ConfigImportSource::Explicit, None) + .await + } + + /// Applies account settings without treating an unconfigured host's empty + /// realtime voice key as a deletion of this controller's saved credential. + /// Non-empty synced keys still update normally; explicit imports and local + /// set/reset operations retain their credential-clearing semantics. + pub async fn import_account_settings( + &self, + config_data: serde_json::Value, + ) -> BitFunResult { + self.import_config_data_from_source(config_data, ConfigImportSource::AccountSync, None) + .await + } + + /// A periodic pull may spend seconds on the network. Apply its response + /// only if the local document still matches the pre-fetch snapshot, with + /// the comparison and import protected by the same manager write lock. + pub async fn import_account_settings_if_unchanged( + &self, + config_data: serde_json::Value, + expected_local_config: serde_json::Value, + ) -> BitFunResult { + self.import_config_data_from_source( + config_data, + ConfigImportSource::AccountSync, + Some(expected_local_config), + ) + .await + } + + async fn import_config_data_from_source( + &self, + config_data: serde_json::Value, + source: ConfigImportSource, + expected_local_config: Option, ) -> BitFunResult { let import_result = { let mut manager = self.manager.write().await; - manager.import_config(config_data).await + if let Some(expected) = expected_local_config { + if manager.export_config()? != expected { + return Ok(ConfigImportResult { + success: false, + errors: vec!["Local settings changed while cloud settings were being fetched; skipped the stale response".to_string()], + warnings: Vec::new(), + }); + } + } + manager.import_config_from_source(config_data, source).await }; match import_result { @@ -279,6 +374,9 @@ impl ConfigService { super::global::ConfigUpdateEvent::ModelConfigurationUpdated, ) .await; + if source == ConfigImportSource::Explicit { + self.local_changes.send_replace(()); + } Ok(ConfigImportResult { success: true, errors: Vec::new(), @@ -315,7 +413,7 @@ impl ConfigService { warnings.push("No AI models configured".to_string()); } - let config: GlobalConfig = self.get_config(None).await?; + let config = manager.get_config(); if config.ai.default_models.primary.is_none() { warnings.push("Primary model not configured".to_string()); } @@ -349,12 +447,9 @@ impl ConfigService { /// Reloads configuration. pub async fn reload(&self) -> BitFunResult<()> { - let settings = ConfigManagerSettings::default(); - let new_manager = ConfigManager::new(settings).await?; - { let mut manager = self.manager.write().await; - *manager = new_manager; + manager.reload().await?; } info!("Configuration reloaded"); @@ -389,45 +484,40 @@ impl ConfigService { /// Adds an AI model configuration. pub async fn add_ai_model(&self, model: AIModelConfig) -> BitFunResult<()> { - let mut config: GlobalConfig = self.get_config(None).await?; - config.ai.models.push(model); - self.set_config("ai.models", &config.ai.models).await + self.update_config("ai.models", |models: &mut Vec| { + models.push(model); + Ok(()) + }) + .await } /// Updates an AI model configuration. pub async fn update_ai_model(&self, model_id: &str, model: AIModelConfig) -> BitFunResult<()> { - let mut config: GlobalConfig = self.get_config(None).await?; - - if let Some(existing_model) = config.ai.models.iter_mut().find(|m| m.id == model_id) { - *existing_model = model; - self.set_config("ai.models", &config.ai.models).await - } else { - Err(BitFunError::config(format!( - "AI model '{}' not found", - model_id - ))) - } + self.update_config("ai.models", |models: &mut Vec| { + let existing = models + .iter_mut() + .find(|m| m.id == model_id) + .ok_or_else(|| BitFunError::config(format!("AI model '{}' not found", model_id)))?; + *existing = model; + Ok(()) + }) + .await } /// Deletes an AI model configuration. pub async fn delete_ai_model(&self, model_id: &str) -> BitFunResult<()> { - let mut config: GlobalConfig = self.get_config(None).await?; - - let original_len = config.ai.models.len(); - config.ai.models.retain(|m| m.id != model_id); - - if config.ai.models.len() == original_len { - return Err(BitFunError::config(format!( - "AI model '{}' not found", - model_id - ))); - } - - // Persist the list deletion. The follow-up reconcile pass triggered by - // `set_config` (and explicitly by `update_ai_model`) is responsible for - // cleaning every other place the deleted id might still be referenced - // (default slots, agent / func-agent mappings). - self.set_config("ai.models", &config.ai.models).await + self.update_config("ai.models", |models: &mut Vec| { + let original_len = models.len(); + models.retain(|m| m.id != model_id); + if models.len() == original_len { + return Err(BitFunError::config(format!( + "AI model '{}' not found", + model_id + ))); + } + Ok(()) + }) + .await } /// Atomically upserts a pure speech-recognition model, selects it as the @@ -535,6 +625,7 @@ impl ConfigService { ) .await; + self.local_changes.send_replace(()); Ok(SaveCloudSpeechConfigResult { model_id, created }) } @@ -555,12 +646,18 @@ impl ConfigService { /// /// `caller` is logged for diagnostics (e.g. `set_config`, `update_ai_model`). pub async fn reconcile_models(&self, caller: &str) -> BitFunResult { - let mut config: GlobalConfig = self.get_config(None).await?; - let reconciliation = super::normalization::reconcile_model_references(&mut config); - - if !reconciliation.is_noop() { - self.manager.write().await.set("", &config).await?; - } + let reconciliation = { + // Reconciliation writes a full config snapshot. Keep its read and + // write under one lock so unrelated saves (such as voice keys) + // cannot be overwritten by the snapshot we read earlier. + let mut manager = self.manager.write().await; + let mut config = manager.get_config().clone(); + let reconciliation = super::normalization::reconcile_model_references(&mut config); + if !reconciliation.is_noop() { + manager.set("", &config).await?; + } + reconciliation + }; let report = ReconcileModelsReport { invalidated_model_ids: reconciliation.invalidated_model_ids, @@ -691,6 +788,747 @@ mod tests { (service, dir) } + fn realtime_voice_fixture() -> VoiceCallConfig { + VoiceCallConfig { + api_key: "fixture-realtime-voice-key".to_string(), + voice: "fixture-voice".to_string(), + speed: 12, + loudness: -8, + microphone_device_id: "fixture-controller-microphone".to_string(), + ..Default::default() + } + } + + async fn restart_test_service(dir: &tempfile::TempDir, name: &str) -> ConfigService { + ConfigService::with_settings(ConfigManagerSettings { + path_manager: Some(Arc::new(PathManager::with_user_root_for_tests( + dir.path().join(name), + ))), + auto_save: true, + backup_count: 0, + }) + .await + .expect("restart config service") + } + + async fn race_config_operations( + service: &ConfigService, + first: A, + second: B, + ) -> (A::Output, B::Output) { + use std::future::poll_fn; + use std::task::Poll; + + let manager = service.manager.write().await; + let mut first = std::pin::pin!(first); + let mut second = std::pin::pin!(second); + poll_fn(|cx| { + assert!(first.as_mut().poll(cx).is_pending()); + assert!(second.as_mut().poll(cx).is_pending()); + Poll::Ready(()) + }) + .await; + drop(manager); + tokio::join!(first, second) + } + + #[tokio::test] + async fn concurrent_model_additions_keep_both_credentials_after_restart() { + let name = "concurrent-model-additions"; + let (service, dir) = test_service(name).await; + let (first, second) = race_config_operations( + &service, + service.add_ai_model(runtime_model("first", "first-fixture-key")), + service.add_ai_model(runtime_model("second", "second-fixture-key")), + ) + .await; + first.unwrap(); + second.unwrap(); + + let restarted = restart_test_service(&dir, name).await; + let models = restarted.get_ai_models().await.unwrap(); + assert_eq!(models.len(), 2); + assert!(models + .iter() + .any(|m| m.id == "first" && m.api_key == "first-fixture-key")); + assert!(models + .iter() + .any(|m| m.id == "second" && m.api_key == "second-fixture-key")); + } + + #[tokio::test] + async fn concurrent_model_delete_and_update_do_not_resurrect_deleted_credentials() { + let name = "concurrent-model-delete-update"; + let (service, dir) = test_service(name).await; + service + .add_ai_model(runtime_model("keep", "old-fixture-key")) + .await + .unwrap(); + service + .add_ai_model(runtime_model("remove", "deleted-fixture-key")) + .await + .unwrap(); + let (deleted, updated) = race_config_operations( + &service, + service.delete_ai_model("remove"), + service.update_ai_model("keep", runtime_model("keep", "new-fixture-key")), + ) + .await; + deleted.unwrap(); + updated.unwrap(); + + let restarted = restart_test_service(&dir, name).await; + let models = restarted.get_ai_models().await.unwrap(); + assert_eq!(models.len(), 1); + assert_eq!(models[0].id, "keep"); + assert_eq!(models[0].api_key, "new-fixture-key"); + } + + #[tokio::test] + async fn failed_config_writes_cannot_leak_into_a_later_successful_save() { + for reset in [false, true] { + let name = "failed-config-write"; + let (service, dir) = test_service(name).await; + service + .set_config("app.voice_call", realtime_voice_fixture()) + .await + .unwrap(); + service + .add_ai_model(runtime_model("keep", "fixture-model-key")) + .await + .unwrap(); + let before: serde_json::Value = service.get_config(None).await.unwrap(); + let changes = service.subscribe_local_changes(); + let config_file = dir.path().join(name).join("config/app.json"); + let preserved = config_file.with_extension("preserved.json"); + tokio::fs::rename(&config_file, &preserved).await.unwrap(); + // Replacing a directory with a file fails on every supported OS, + // without relying on permissions (which root can bypass in CI). + tokio::fs::create_dir(&config_file).await.unwrap(); + + let result = if reset { + service.reset_config(None).await + } else { + service + .set_config("app.voice_call.api_key", "unsaved-fixture-key") + .await + }; + assert!(result.is_err()); + assert!( + !changes.has_changed().unwrap(), + "Failed writes must not trigger account uploads" + ); + let after: serde_json::Value = service.get_config(None).await.unwrap(); + assert_eq!(after, before, "A failed write changed the in-memory config"); + + tokio::fs::remove_dir(&config_file).await.unwrap(); + tokio::fs::rename(&preserved, &config_file).await.unwrap(); + service.set_config("editor.font_size", 18).await.unwrap(); + let restarted = restart_test_service(&dir, name).await; + let saved: GlobalConfig = restarted.get_config(None).await.unwrap(); + assert_eq!( + saved.app.voice_call.api_key, + realtime_voice_fixture().api_key + ); + assert_eq!(saved.ai.models[0].api_key, "fixture-model-key"); + assert_eq!(saved.editor.font_size, 18); + } + } + + #[tokio::test] + async fn explicit_backups_are_distinct_even_when_requested_together() { + let (service, _dir) = test_service("concurrent-backups").await; + service + .add_ai_model(runtime_model("keep", "fixture-model-key")) + .await + .unwrap(); + let (first, second, third) = tokio::join!( + service.create_backup(), + service.create_backup(), + service.create_backup(), + ); + let backups = [first.unwrap(), second.unwrap(), third.unwrap()]; + assert_eq!( + backups + .iter() + .collect::>() + .len(), + 3 + ); + for backup in backups { + let saved: GlobalConfig = + serde_json::from_slice(&tokio::fs::read(backup).await.unwrap()).unwrap(); + assert_eq!(saved.ai.models[0].api_key, "fixture-model-key"); + } + } + + #[tokio::test] + async fn legacy_import_preserves_missing_settings_beyond_realtime_voice() { + for account_sync in [false, true] { + let name = "legacy-missing-settings"; + let (service, dir) = test_service(name).await; + let mut local = GlobalConfig::default(); + local.app.hooks.enabled = false; + local.app.notifications.permission_request_notify = false; + local.app.logging.include_sensitive_diagnostics = false; + local.app.ai_experience.voice_input.provider = "cloud".to_string(); + local.app.ai_experience.voice_input.microphone_device_id = + "fixture-microphone".to_string(); + local.ai.proxy.enabled = true; + local.ai.proxy.url = "http://127.0.0.1:12345".to_string(); + local.ai.proxy.password = Some("fixture-proxy-password".to_string()); + local.editor.minimap.enabled = false; + local.terminal.terminal_panel_position = "bottom".to_string(); + local.memories.use_memories = true; + service.set_config("", &local).await.unwrap(); + + let mut legacy = serde_json::to_value(GlobalConfig::default()).unwrap(); + legacy["version"] = serde_json::json!("0.2.18"); + for (parent, key) in [ + ("/app", "hooks"), + ("/app/notifications", "permission_request_notify"), + ("/app/logging", "include_sensitive_diagnostics"), + ("/app/ai_experience", "voice_input"), + ("/ai", "proxy"), + ("/editor", "minimap"), + ("/terminal", "terminal_panel_position"), + ] { + legacy + .pointer_mut(parent) + .unwrap() + .as_object_mut() + .unwrap() + .remove(key); + } + if account_sync { + legacy.as_object_mut().unwrap().remove("memories"); + } + let imported = if account_sync { + service.import_account_settings(legacy).await.unwrap() + } else { + service.import_config_data(legacy).await.unwrap() + }; + assert!(imported.success, "{:?}", imported.errors); + let restarted = restart_test_service(&dir, name).await; + let saved: GlobalConfig = restarted.get_config(None).await.unwrap(); + assert!(!saved.app.hooks.enabled); + assert!(!saved.app.notifications.permission_request_notify); + assert!(!saved.app.logging.include_sensitive_diagnostics); + assert_eq!(saved.app.ai_experience.voice_input.provider, "cloud"); + assert_eq!( + saved.app.ai_experience.voice_input.microphone_device_id, + "fixture-microphone" + ); + assert_eq!( + saved.ai.proxy.password.as_deref(), + Some("fixture-proxy-password") + ); + assert!(!saved.editor.minimap.enabled); + assert_eq!(saved.terminal.terminal_panel_position, "bottom"); + assert_eq!(saved.memories.use_memories, account_sync); + } + } + + #[tokio::test] + async fn imports_still_honor_explicit_deletions_and_default_elision_in_backups() { + for account_sync in [false, true] { + let (service, _dir) = test_service("import-explicit-deletions").await; + // A raw backup intentionally omits these default values. Restoring + // it must still reset them, even though legacy missing fixed fields + // now retain their local values. + let backup = service.create_backup().await.unwrap(); + let raw_backup: serde_json::Value = + serde_json::from_slice(&tokio::fs::read(backup).await.unwrap()).unwrap(); + let mut local = GlobalConfig::default(); + local.mcp_servers = + Some(serde_json::json!({"mcpServers": {"fixture": {"command": "fixture"}}})); + local.acp_clients = + Some(serde_json::json!({"acpClients": {"fixture": {"command": "fixture"}}})); + local.app.keybindings = Some(serde_json::json!({"version": 1, "overrides": {}})); + local.plugin = vec![PluginDeclarationConfig::Spec("fixture-plugin".to_string())]; + local + .ai + .agent_profiles + .insert("fixture-profile".to_string(), AgentProfileConfig::default()); + local.ai.agent_model_defaults.subagents.builtin.insert( + "fixture-override".to_string(), + SubagentModelSelection::Inherit, + ); + local.memories.use_memories = true; + local.ai.allow_tool_json_repair = false; + local.ai.max_rounds = 42; + local.app.notifications.enabled = false; + service.set_config("", &local).await.unwrap(); + + let mut incoming = if account_sync { + serde_json::to_value(GlobalConfig::default()).unwrap() + } else { + raw_backup + }; + incoming["ai"]["review_teams"] = serde_json::json!({}); + incoming["ai"]["agent_model_defaults"]["subagents"]["builtin"] = serde_json::json!({}); + incoming["workspace"]["exclude_patterns"] = serde_json::json!([]); + let result = if account_sync { + service.import_account_settings(incoming).await.unwrap() + } else { + service.import_config_data(incoming).await.unwrap() + }; + assert!(result.success, "{:?}", result.errors); + let saved: GlobalConfig = service.get_config(None).await.unwrap(); + assert!(saved.mcp_servers.is_none()); + assert!(saved.acp_clients.is_none()); + assert!(saved.app.keybindings.is_none()); + assert!(saved.plugin.is_empty()); + assert!(saved.ai.agent_profiles.is_empty()); + assert!(saved.ai.review_teams.is_empty()); + assert!(!saved + .ai + .agent_model_defaults + .subagents + .builtin + .contains_key("fixture-override")); + assert!(!saved + .ai + .agent_model_defaults + .subagents + .builtin + .contains_key("GeneralPurpose")); + assert!(saved.workspace.exclude_patterns.is_empty()); + assert!(!saved.memories.use_memories); + assert!(saved.ai.allow_tool_json_repair); + assert_eq!(saved.ai.max_rounds, GlobalConfig::default().ai.max_rounds); + assert!(saved.app.notifications.enabled); + } + } + + #[tokio::test] + async fn local_change_notifications_cover_mutations_without_echoing_cloud_restores() { + let (service, _dir) = test_service("config-local-notifications").await; + let mut changes = service.subscribe_local_changes(); + assert!(!changes.has_changed().unwrap()); + + service + .set_config("app.notifications.enabled", false) + .await + .unwrap(); + assert!(changes.has_changed().unwrap()); + changes.borrow_and_update(); + + service + .update_config("ai.skill_settings", |settings: &mut SkillSettingsConfig| { + settings + .globally_disabled_user_skills + .push("user::fixture".to_string()); + Ok(()) + }) + .await + .unwrap(); + assert!(changes.has_changed().unwrap()); + changes.borrow_and_update(); + + let snapshot: serde_json::Value = service.get_config(None).await.unwrap(); + assert!( + service + .import_account_settings(snapshot.clone()) + .await + .unwrap() + .success + ); + service.reload().await.unwrap(); + assert!( + !changes.has_changed().unwrap(), + "Cloud imports must not start an upload feedback loop" + ); + + assert!(service.import_config_data(snapshot).await.unwrap().success); + assert!(changes.has_changed().unwrap()); + changes.borrow_and_update(); + + service + .reset_config(Some("app.notifications")) + .await + .unwrap(); + assert!(changes.has_changed().unwrap()); + changes.borrow_and_update(); + + service + .install_runtime_ai_model(runtime_model("ephemeral", "runtime-fixture-key")) + .await + .unwrap(); + assert!( + !changes.has_changed().unwrap(), + "Runtime-only credentials must not be synced" + ); + + service.save_cloud_speech_config(serde_json::from_value(serde_json::json!({ + "preset": "custom", "name": "Speech fixture", "baseUrl": "https://example.com/v1", + "modelName": "speech-fixture", "apiKey": "speech-fixture-key" + })).unwrap()).await.unwrap(); + assert!(changes.has_changed().unwrap()); + } + + #[tokio::test] + async fn stale_cloud_pull_cannot_overwrite_a_save_made_during_the_fetch() { + let name = "config-stale-cloud-pull"; + let (service, dir) = test_service(name).await; + let before_fetch: serde_json::Value = service.get_config(None).await.unwrap(); + let mut cloud = before_fetch.clone(); + cloud["app"]["voice_call"]["api_key"] = serde_json::json!("old-cloud-fixture-key"); + + let (saved, imported) = race_config_operations( + &service, + service.set_config("app.voice_call.api_key", "new-local-fixture-key"), + service.import_account_settings_if_unchanged(cloud.clone(), before_fetch), + ) + .await; + saved.unwrap(); + let imported = imported.unwrap(); + assert!(!imported.success); + assert!(imported.errors[0].contains("Local settings changed")); + let restarted = restart_test_service(&dir, name).await; + assert_eq!( + restarted + .get_config::(Some("app.voice_call.api_key")) + .await + .unwrap(), + "new-local-fixture-key" + ); + + // A later pull with a current snapshot is still authoritative. + let current = service.get_config(None).await.unwrap(); + assert!( + service + .import_account_settings_if_unchanged(cloud, current) + .await + .unwrap() + .success + ); + assert_eq!( + service + .get_config::(Some("app.voice_call.api_key")) + .await + .unwrap(), + "old-cloud-fixture-key" + ); + } + + #[tokio::test] + async fn realtime_voice_survives_legacy_config_import_and_restart() { + let name = "realtime-voice-legacy-import"; + let (service, dir) = test_service(name).await; + let voice = realtime_voice_fixture(); + service.set_config("app.voice_call", &voice).await.unwrap(); + + // A pre-voice cloud payload still carries the user's model keys. + let mut legacy = serde_json::to_value(GlobalConfig::default()).unwrap(); + legacy["version"] = serde_json::json!("0.2.18"); + legacy["app"].as_object_mut().unwrap().remove("voice_call"); + legacy["ai"]["models"] = + serde_json::json!([runtime_model("synced-model", "fixture-synced-model-key")]); + let imported = service.import_config_data(legacy).await.unwrap(); + assert!(imported.success, "{:?}", imported.errors); + + drop(service); + let restarted = restart_test_service(&dir, name).await; + let config: GlobalConfig = restarted.get_config(None).await.unwrap(); + assert_eq!(config.ai.models[0].api_key, "fixture-synced-model-key"); + assert_eq!( + serde_json::to_value(config.app.voice_call).unwrap(), + serde_json::to_value(voice).unwrap() + ); + } + + #[tokio::test] + async fn realtime_voice_survives_model_reconciliation_racing_a_save() { + use std::future::{poll_fn, Future}; + use std::task::Poll; + + let name = "realtime-voice-reconcile-race"; + let (service, dir) = test_service(name).await; + let voice = realtime_voice_fixture(); + let mut manager = service.manager.write().await; + // Leave the default slots unreconciled, just as a model-list mutation + // does before its follow-up reconciliation acquires the manager. + manager + .set( + "ai.models", + vec![runtime_model("configured-model", "fixture-model-key")], + ) + .await + .unwrap(); + + let mut reconcile = std::pin::pin!(service.reconcile_models("realtime-voice-test")); + let mut save = std::pin::pin!(service.set_config("app.voice_call", &voice)); + // Queue reconciliation before the save. A read-then-write reconcile + // releases its snapshot lock and writes after the queued save, losing + // the key. A single write-locked transaction cannot do that. + poll_fn(|cx| { + assert!(reconcile.as_mut().poll(cx).is_pending()); + assert!(save.as_mut().poll(cx).is_pending()); + Poll::Ready(()) + }) + .await; + drop(manager); + let (reconciled, saved) = tokio::join!(reconcile, save); + assert!(!reconciled.unwrap().is_noop()); + saved.unwrap(); + + let restarted = restart_test_service(&dir, name).await; + let config: GlobalConfig = restarted.get_config(None).await.unwrap(); + assert_eq!(config.app.voice_call.api_key, voice.api_key); + assert_eq!( + config.ai.default_models.primary.as_deref(), + Some("configured-model") + ); + } + + #[tokio::test] + async fn realtime_voice_account_sync_preserves_keys_from_missing_or_empty_payloads() { + for incoming_voice in [ + None, + Some(serde_json::to_value(VoiceCallConfig::default()).unwrap()), + Some(serde_json::json!({ "api_key": " \n\t", "enabled": false })), + ] { + let name = "realtime-voice-account-compatibility"; + let (service, dir) = test_service(name).await; + let voice = realtime_voice_fixture(); + service + .add_ai_model(runtime_model( + "obsolete-model", + "fixture-obsolete-model-key", + )) + .await + .unwrap(); + service.set_config("app.voice_call", &voice).await.unwrap(); + let mut incoming = serde_json::to_value(GlobalConfig::default()).unwrap(); + let app = incoming["app"].as_object_mut().unwrap(); + match incoming_voice { + Some(voice) => { + app.insert("voice_call".to_string(), voice); + } + None => { + app.remove("voice_call"); + } + } + incoming["ai"]["models"] = + serde_json::json!([runtime_model("cloud-model", "fixture-cloud-model-key")]); + let expected_enabled = incoming["app"]["voice_call"]["enabled"] + .as_bool() + .unwrap_or(voice.enabled); + let imported = service.import_account_settings(incoming).await.unwrap(); + assert!(imported.success, "{:?}", imported.errors); + // This is the reload performed after an account settings apply. + service.reload().await.unwrap(); + + drop(service); + let restarted = restart_test_service(&dir, name).await; + let config: GlobalConfig = restarted.get_config(None).await.unwrap(); + assert_eq!(config.app.voice_call.api_key, voice.api_key); + assert_eq!(config.app.voice_call.enabled, expected_enabled); + assert_eq!(config.ai.models.len(), 1); + assert_eq!(config.ai.models[0].id, "cloud-model"); + assert_eq!(config.ai.models[0].api_key, "fixture-cloud-model-key"); + } + } + + #[tokio::test] + async fn realtime_voice_account_sync_updates_keys_and_backs_up_the_previous_file() { + let name = "realtime-voice-account-update"; + let (service, dir) = test_service(name).await; + service + .set_config("app.voice_call", realtime_voice_fixture()) + .await + .unwrap(); + let config_dir = service.get_statistics().await.config_directory; + let original = tokio::fs::read_to_string(config_dir.join("app.json")) + .await + .unwrap(); + + let mut incoming = serde_json::to_value(GlobalConfig::default()).unwrap(); + incoming["app"]["voice_call"] = serde_json::json!({ + "api_key": "fixture-updated-voice-key", + "voice": "fixture-updated-voice" + }); + let imported = service.import_account_settings(incoming).await.unwrap(); + assert!(imported.success, "{:?}", imported.errors); + + let backup = std::fs::read_dir(config_dir.join("backups")) + .unwrap() + .map(Result::unwrap) + .find(|entry| entry.file_name().to_string_lossy().contains("pre-import")) + .expect("pre-import backup"); + assert_eq!( + tokio::fs::read_to_string(backup.path()).await.unwrap(), + original + ); + + drop(service); + let restarted = restart_test_service(&dir, name).await; + let voice: VoiceCallConfig = restarted.get_config(Some("app.voice_call")).await.unwrap(); + assert_eq!(voice.api_key, "fixture-updated-voice-key"); + assert_eq!(voice.voice, "fixture-updated-voice"); + assert_eq!(voice.microphone_device_id, "fixture-controller-microphone"); + } + + #[tokio::test] + async fn realtime_voice_export_and_backup_restore_preserve_credentials() { + let name = "realtime-voice-backup-restore"; + let (service, dir) = test_service(name).await; + let voice = realtime_voice_fixture(); + service.set_config("app.voice_call", &voice).await.unwrap(); + service + .add_ai_model(runtime_model("configured-model", "fixture-model-key")) + .await + .unwrap(); + let export = service.export_config().await.unwrap(); + let backup = service.create_backup().await.unwrap(); + let backup: serde_json::Value = + serde_json::from_str(&tokio::fs::read_to_string(backup).await.unwrap()).unwrap(); + assert_eq!(export.config.app.voice_call.api_key, voice.api_key); + assert_eq!(backup["app"]["voice_call"]["api_key"], voice.api_key); + + service.reset_config(Some("app.voice_call")).await.unwrap(); + let restored = service.import_config(export).await.unwrap(); + assert!(restored.success, "{:?}", restored.errors); + let restored_voice: VoiceCallConfig = + service.get_config(Some("app.voice_call")).await.unwrap(); + assert_eq!(restored_voice.api_key, voice.api_key); + + service.reset_config(Some("app.voice_call")).await.unwrap(); + let restored = service.import_config_data(backup).await.unwrap(); + assert!(restored.success, "{:?}", restored.errors); + drop(service); + let restarted = restart_test_service(&dir, name).await; + let config: GlobalConfig = restarted.get_config(None).await.unwrap(); + assert_eq!(config.ai.models[0].api_key, "fixture-model-key"); + assert_eq!( + serde_json::to_value(config.app.voice_call).unwrap(), + serde_json::to_value(voice).unwrap() + ); + } + + #[tokio::test] + async fn realtime_voice_explicit_import_save_and_reset_can_clear_credentials() { + let name = "realtime-voice-explicit-clear"; + let (service, dir) = test_service(name).await; + for operation in ["import", "save", "reset"] { + service + .set_config("app.voice_call", realtime_voice_fixture()) + .await + .unwrap(); + match operation { + "import" => { + let incoming = serde_json::to_value(GlobalConfig::default()).unwrap(); + let imported = service.import_config_data(incoming).await.unwrap(); + assert!(imported.success, "{:?}", imported.errors); + } + "save" => { + service + .set_config( + "app.voice_call", + VoiceCallConfig { + enabled: false, + ..Default::default() + }, + ) + .await + .unwrap(); + } + _ => service.reset_config(Some("app.voice_call")).await.unwrap(), + } + let restarted = restart_test_service(&dir, name).await; + let voice: VoiceCallConfig = + restarted.get_config(Some("app.voice_call")).await.unwrap(); + assert!(voice.api_key.is_empty(), "operation={operation}"); + } + } + + #[tokio::test] + async fn realtime_voice_survives_model_mutations_and_application_upgrade() { + let name = "realtime-voice-upgrade"; + let (service, dir) = test_service(name).await; + let voice = realtime_voice_fixture(); + service.set_config("app.voice_call", &voice).await.unwrap(); + service + .add_ai_model(runtime_model("keep", "fixture-model-key")) + .await + .unwrap(); + service + .add_ai_model(runtime_model("remove", "fixture-removed-key")) + .await + .unwrap(); + service + .update_ai_model("keep", runtime_model("keep", "fixture-updated-model-key")) + .await + .unwrap(); + service.delete_ai_model("remove").await.unwrap(); + service.set_config("version", "0.2.18").await.unwrap(); + + drop(service); + let restarted = restart_test_service(&dir, name).await; + let config: GlobalConfig = restarted.get_config(None).await.unwrap(); + assert_eq!(config.version, env!("CARGO_PKG_VERSION")); + assert_eq!(config.app.voice_call.api_key, voice.api_key); + assert_eq!(config.ai.models.len(), 1); + assert_eq!(config.ai.models[0].id, "keep"); + assert_eq!(config.ai.models[0].api_key, "fixture-updated-model-key"); + } + + #[tokio::test] + async fn realtime_voice_survives_reload_racing_a_save() { + use std::future::{poll_fn, Future}; + use std::task::Poll; + + let name = "realtime-voice-reload-race"; + let (service, dir) = test_service(name).await; + let voice = realtime_voice_fixture(); + let manager = service.manager.write().await; + let mut reload = std::pin::pin!(service.reload()); + let mut save = std::pin::pin!(service.set_config("app.voice_call", &voice)); + poll_fn(|cx| { + assert!(reload.as_mut().poll(cx).is_pending()); + assert!(save.as_mut().poll(cx).is_pending()); + Poll::Ready(()) + }) + .await; + drop(manager); + let (reloaded, saved) = tokio::join!(reload, save); + reloaded.unwrap(); + saved.unwrap(); + + let restarted = restart_test_service(&dir, name).await; + let config: VoiceCallConfig = restarted.get_config(Some("app.voice_call")).await.unwrap(); + assert_eq!(config.api_key, voice.api_key); + let in_memory: VoiceCallConfig = service.get_config(Some("app.voice_call")).await.unwrap(); + assert_eq!(in_memory.api_key, voice.api_key); + } + + #[tokio::test] + async fn realtime_voice_invalid_import_keeps_memory_and_disk_unchanged() { + let (service, _dir) = test_service("realtime-voice-invalid-import").await; + service + .set_config("app.voice_call", realtime_voice_fixture()) + .await + .unwrap(); + let config_dir = service.get_statistics().await.config_directory; + let before = tokio::fs::read_to_string(config_dir.join("app.json")) + .await + .unwrap(); + let mut invalid = serde_json::to_value(GlobalConfig::default()).unwrap(); + invalid["app"]["voice_call"]["api_key"] = serde_json::Value::Null; + let imported = service.import_account_settings(invalid).await.unwrap(); + assert!(!imported.success); + assert_eq!( + tokio::fs::read_to_string(config_dir.join("app.json")) + .await + .unwrap(), + before + ); + let voice: VoiceCallConfig = service.get_config(Some("app.voice_call")).await.unwrap(); + assert_eq!(voice.api_key, realtime_voice_fixture().api_key); + assert!(!config_dir.join("backups").exists()); + } + #[tokio::test] async fn runtime_ai_model_is_effective_but_never_persisted() { let (service, _dir) = test_service("runtime-overlay-test").await; diff --git a/src/crates/assembly/core/src/service/config/types.rs b/src/crates/assembly/core/src/service/config/types.rs index ed4a7aad0b..c0a2d66cd8 100644 --- a/src/crates/assembly/core/src/service/config/types.rs +++ b/src/crates/assembly/core/src/service/config/types.rs @@ -463,10 +463,29 @@ pub struct AIExperienceConfig { /// Local speech-to-text settings for the chat composer. pub voice_input: VoiceInputConfig, /// User-defined quick actions (post-coding menu); persisted for the web UI. - #[serde(default)] + #[serde(default = "default_quick_actions")] pub quick_actions: Vec, } +fn default_quick_actions() -> Vec { + [ + ("commit", "Commit", "Commit all current code changes"), + ( + "create_pr", + "Create PR", + "Create a Pull Request for the current branch", + ), + ] + .into_iter() + .map(|(id, label, prompt)| AiExperienceQuickAction { + id: id.to_string(), + label: label.to_string(), + prompt: prompt.to_string(), + enabled: true, + }) + .collect() +} + /// User-selected Agent companion pet package. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] #[serde(rename_all = "camelCase")] @@ -1708,7 +1727,7 @@ impl Default for AIExperienceConfig { agent_companion_pet: default_agent_companion_pet(), enable_workspace_search: false, voice_input: VoiceInputConfig::default(), - quick_actions: Vec::new(), + quick_actions: default_quick_actions(), } } } @@ -2503,6 +2522,20 @@ mod tests { ); } + #[test] + fn quick_action_defaults_do_not_replace_an_explicit_legacy_empty_list() { + let absent: AIExperienceConfig = serde_json::from_value(serde_json::json!({})).unwrap(); + assert_eq!(absent.quick_actions.len(), 2); + let cleared: AIExperienceConfig = serde_json::from_value(serde_json::json!({ + "quick_actions": [] + })) + .unwrap(); + assert!(cleared.quick_actions.is_empty()); + let round_trip: AIExperienceConfig = + serde_json::from_value(serde_json::to_value(cleared).unwrap()).unwrap(); + assert!(round_trip.quick_actions.is_empty()); + } + #[test] fn ai_experience_quick_actions_round_trip_through_global_config() { let config: GlobalConfig = serde_json::from_value(serde_json::json!({ diff --git a/src/crates/assembly/core/src/service/remote_connect/settings_sync.rs b/src/crates/assembly/core/src/service/remote_connect/settings_sync.rs index 22450c9669..b279e07678 100644 --- a/src/crates/assembly/core/src/service/remote_connect/settings_sync.rs +++ b/src/crates/assembly/core/src/service/remote_connect/settings_sync.rs @@ -1,7 +1,8 @@ //! Account cloud settings sync engine, shared by Desktop and CLI. //! //! Owns the full settings sync lifecycle for one process: -//! - **Push**: `notify_changed()` marks local settings dirty; a 5s debounce +//! - **Push**: persisted ConfigService changes or `notify_settings_changed()` +//! mark local settings dirty; a 5s debounce //! later the engine exports the config and uploads it to the relay. Uploads //! are content-hash deduped so identical content is never re-uploaded. //! - **Pull**: an immediate pull on start, then every 30s, fetches the cloud @@ -286,7 +287,7 @@ pub async fn apply_settings_blob( blob: &SettingsBlob, force: bool, ) -> Result { - apply_settings_blob_for_generation(account, blob, force, None).await + apply_settings_blob_for_generation(account, blob, force, None, None).await } async fn apply_settings_blob_for_generation( @@ -294,6 +295,7 @@ async fn apply_settings_blob_for_generation( blob: &SettingsBlob, force: bool, generation: Option, + expected_local_config: Option, ) -> Result { if !force { let known = sync_state::load_settings_cursor(&account.user_id); @@ -313,10 +315,15 @@ async fn apply_settings_blob_for_generation( let config_service = crate::service::config::get_global_config_service() .await .map_err(|e| anyhow!("config service: {e}"))?; - let import_result = config_service - .import_config_data(inner_config) - .await - .map_err(|e| anyhow!("import cloud config: {e}"))?; + let import_result = match expected_local_config { + Some(expected) if !force => { + config_service + .import_account_settings_if_unchanged(inner_config, expected) + .await + } + _ => config_service.import_account_settings(inner_config).await, + } + .map_err(|e| anyhow!("import cloud config: {e}"))?; if !import_result.success { return Err(anyhow!( "import cloud config failed: {}", @@ -355,6 +362,17 @@ async fn pull_and_apply_settings_for_generation( relay_url: &str, generation: Option, ) -> Result { + let config_service = crate::service::config::get_global_config_service() + .await + .map_err(|e| anyhow!("config service: {e}"))?; + let expected_local_config = serde_json::to_value( + config_service + .export_config() + .await + .map_err(|e| anyhow!("export config: {e}"))? + .config, + ) + .map_err(|e| anyhow!("snapshot config: {e}"))?; let client = AccountClient::new(); let Some(blob) = client .fetch_settings_with_version(relay_url, account) @@ -365,7 +383,14 @@ async fn pull_and_apply_settings_for_generation( if generation.is_some_and(|value| !is_account_context_current(value)) { return Err(anyhow!("account context changed during settings pull")); } - apply_settings_blob_for_generation(account, &blob, false, generation).await + apply_settings_blob_for_generation( + account, + &blob, + false, + generation, + Some(expected_local_config), + ) + .await } async fn account_context() -> Result { @@ -427,28 +452,67 @@ async fn pull_from_loop() { /// converges right after start instead of one interval later. async fn settings_sync_loop(mut rx: mpsc::UnboundedReceiver<()>) { let mut next_pull = tokio::time::Instant::now(); + let mut local_changes = None; loop { + // Subscribe at the persistence owner as well as accepting legacy host + // notifications. Skills, Agent profiles, CLI mutations, and future + // settings must not depend on each adapter remembering an upload hook. + if local_changes.is_none() { + match crate::service::config::get_global_config_service().await { + Ok(service) => local_changes = Some(service.subscribe_local_changes()), + Err(error) => { + warn!("Settings sync: config subscription unavailable; will retry: {error}") + } + } + } let pull_deadline = tokio::time::sleep_until(next_pull); tokio::pin!(pull_deadline); - tokio::select! { - Some(()) = rx.recv() => { - // Drain further notifications during the debounce window. - let deadline = tokio::time::sleep(SETTINGS_PUSH_DEBOUNCE); - tokio::pin!(deadline); - loop { - tokio::select! { - _ = &mut deadline => break, - Some(()) = rx.recv() => {} - } + let push_requested = tokio::select! { + // A queued local save takes priority over a periodic pull. + biased; + Some(()) = rx.recv() => true, + available = wait_for_local_config_change(&mut local_changes) => { + if !available { + local_changes = None; + continue; } - push_from_loop().await; - } + true + }, _ = &mut pull_deadline => { next_pull = tokio::time::Instant::now() + SETTINGS_PULL_INTERVAL; pull_from_loop().await; + false } + }; + if !push_requested { + continue; } + + // Drain both notification sources during the same debounce window. + let deadline = tokio::time::sleep(SETTINGS_PUSH_DEBOUNCE); + tokio::pin!(deadline); + loop { + tokio::select! { + _ = &mut deadline => break, + Some(()) = rx.recv() => {}, + available = wait_for_local_config_change(&mut local_changes) => { + if !available { + local_changes = None; + } + } + } + } + push_from_loop().await; + } +} + +async fn wait_for_local_config_change( + receiver: &mut Option>, +) -> bool { + match receiver { + Some(receiver) => receiver.changed().await.is_ok(), + None => std::future::pending().await, } } diff --git a/src/web-ui/src/app/services/agentCompanionPetCommands.test.ts b/src/web-ui/src/app/services/agentCompanionPetCommands.test.ts index 7b5d65e62d..6fc631d9aa 100644 --- a/src/web-ui/src/app/services/agentCompanionPetCommands.test.ts +++ b/src/web-ui/src/app/services/agentCompanionPetCommands.test.ts @@ -72,7 +72,6 @@ describe('handleAgentCompanionPetCommand', () => { expect(saveSettingsMock).toHaveBeenCalledWith({ enable_agent_companion: false, - agent_companion_display_mode: 'desktop', }); }); diff --git a/src/web-ui/src/app/services/agentCompanionPetCommands.ts b/src/web-ui/src/app/services/agentCompanionPetCommands.ts index a3ce3fe1b3..6d5eb0c5da 100644 --- a/src/web-ui/src/app/services/agentCompanionPetCommands.ts +++ b/src/web-ui/src/app/services/agentCompanionPetCommands.ts @@ -40,7 +40,6 @@ async function closeAgentCompanionDesktopPet(): Promise { } await aiExperienceConfigService.saveSettings({ - ...settings, enable_agent_companion: false, }); log.info('Agent companion disabled from pet context menu'); diff --git a/src/web-ui/src/flow_chat/components/modern/SessionFilesBadge.test.tsx b/src/web-ui/src/flow_chat/components/modern/SessionFilesBadge.test.tsx index 4004e23cc6..91ea5e3e4e 100644 --- a/src/web-ui/src/flow_chat/components/modern/SessionFilesBadge.test.tsx +++ b/src/web-ui/src/flow_chat/components/modern/SessionFilesBadge.test.tsx @@ -122,7 +122,7 @@ vi.mock('../../hooks/useSessionStateMachine', () => ({ })); vi.mock('@/infrastructure/config/services/AIExperienceConfigService', () => ({ - DEFAULT_QUICK_ACTIONS: [], + DEFAULT_QUICK_ACTIONS: [{ id: 'fixture-default', label: 'Default fixture action', prompt: 'fixture', enabled: true }], aiExperienceConfigService: { getSettings: () => ({ quick_actions: [] }), addChangeListener: (listener: (settings: { quick_actions: unknown[] }) => void) => { @@ -254,6 +254,8 @@ describe('SessionFilesBadge', () => { expect(dom.window.document.body.textContent).toContain('Review'); expect(dom.window.document.body.textContent).not.toContain('Review: Strict'); expect(dom.window.document.body.textContent).not.toContain('Deep review'); + // The persisted [] means the user removed all quick actions. + expect(dom.window.document.body.textContent).not.toContain('Default fixture action'); }); it('shows a localized error when Review cannot be prepared', async () => { diff --git a/src/web-ui/src/flow_chat/components/modern/SessionFilesBadge.tsx b/src/web-ui/src/flow_chat/components/modern/SessionFilesBadge.tsx index e1a49eca16..1e710deccc 100644 --- a/src/web-ui/src/flow_chat/components/modern/SessionFilesBadge.tsx +++ b/src/web-ui/src/flow_chat/components/modern/SessionFilesBadge.tsx @@ -202,7 +202,7 @@ export const SessionFilesBadge: React.FC = ({ const [quickActions, setQuickActions] = useState(() => { const stored = aiExperienceConfigService.getSettings().quick_actions; - return (stored && stored.length > 0) ? stored : DEFAULT_QUICK_ACTIONS; + return stored ?? DEFAULT_QUICK_ACTIONS; }); const badgeRef = useRef(null); @@ -248,10 +248,10 @@ export const SessionFilesBadge: React.FC = ({ return; } const actions = settings.quick_actions; - setQuickActions((actions && actions.length > 0) ? actions : DEFAULT_QUICK_ACTIONS); + setQuickActions(actions ?? DEFAULT_QUICK_ACTIONS); unsubscribeSettings = aiExperienceConfigService.addChangeListener((nextSettings) => { const nextActions = nextSettings.quick_actions; - setQuickActions((nextActions && nextActions.length > 0) ? nextActions : DEFAULT_QUICK_ACTIONS); + setQuickActions(nextActions ?? DEFAULT_QUICK_ACTIONS); }); }, { signalName: 'bitfun:interactive-shell-ready', diff --git a/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.test.ts b/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.test.ts index 9278fb5ef4..25bba78194 100644 --- a/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.test.ts +++ b/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.test.ts @@ -122,4 +122,24 @@ describe('ConfigAPI batch config reads', () => { { timeout: 60_000 }, ); }); + + it('rejects unsuccessful imports without attaching credential-bearing documents to errors', async () => { + invokeMock.mockResolvedValueOnce({ success: false, errors: ['Invalid config'], warnings: [] }); + const document = { config: { app: { voice_call: { api_key: 'fixture-import-key' } } } }; + + const error = await configAPI.importConfig(document).catch(error => error); + + expect(error).toBeInstanceOf(Error); + expect(error.message).toContain('Invalid config'); + expect(error.context.request).toBeUndefined(); + expect(JSON.stringify(error)).not.toContain('fixture-import-key'); + }); + + it('accepts a confirmed import and rejects an ambiguous response', async () => { + invokeMock.mockResolvedValueOnce({ success: true, errors: [], warnings: [] }); + await expect(configAPI.importConfig({})).resolves.toBeUndefined(); + + invokeMock.mockResolvedValueOnce(undefined); + await expect(configAPI.importConfig({})).rejects.toThrow('Configuration import was not confirmed'); + }); }); diff --git a/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.ts b/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.ts index afbdecc301..ac9b09c336 100644 --- a/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.ts +++ b/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.ts @@ -191,11 +191,15 @@ export class ConfigAPI { async importConfig(configData: any): Promise { try { - await api.invoke('import_config', { + const result = await api.invoke<{ success: boolean; errors: string[] }>('import_config', { request: { configData } }); + if (!result?.success) { + throw new Error(result?.errors?.join('; ') || 'Configuration import was not confirmed'); + } } catch (error) { - throw createTauriCommandError('import_config', error, { configData }); + // Imported documents can contain credentials; never attach them to errors. + throw createTauriCommandError('import_config', error); } } diff --git a/src/web-ui/src/infrastructure/config/components/LocalVoiceModelsConfig.tsx b/src/web-ui/src/infrastructure/config/components/LocalVoiceModelsConfig.tsx index 274bb6bfc3..bad96d8751 100644 --- a/src/web-ui/src/infrastructure/config/components/LocalVoiceModelsConfig.tsx +++ b/src/web-ui/src/infrastructure/config/components/LocalVoiceModelsConfig.tsx @@ -22,10 +22,7 @@ import type { MenuItem } from '@/shared/context-menu-system/types/menu.types'; import { notificationService } from '@/shared/notification-system'; import { createLogger } from '@/shared/utils/logger'; import { useAIExperienceSettings } from '../hooks'; -import { - aiExperienceConfigService, - type AIExperienceSettings, -} from '../services/AIExperienceConfigService'; +import { aiExperienceConfigService } from '../services/AIExperienceConfigService'; import type { VoiceInputSettings } from '../types'; import { ConfigPageLoading, ConfigPageMessage } from './common'; import './VoiceInputConfig.scss'; @@ -133,15 +130,8 @@ const LocalVoiceModelsConfig: React.FC = ({ notificationService.error(t('messages.loadFailed')); return; } - const nextSettings: AIExperienceSettings = { - ...settings, - voice_input: { - ...settings.voice_input, - ...patch, - }, - }; try { - await aiExperienceConfigService.saveSettings(nextSettings); + await aiExperienceConfigService.saveSettings({ voice_input: patch }); } catch (error) { log.error('Failed to select local speech model', { error }); notificationService.error(t('messages.saveFailed')); diff --git a/src/web-ui/src/infrastructure/config/components/ModelSettingsPage.tsx b/src/web-ui/src/infrastructure/config/components/ModelSettingsPage.tsx index b6dc68e73e..5edb093eac 100644 --- a/src/web-ui/src/infrastructure/config/components/ModelSettingsPage.tsx +++ b/src/web-ui/src/infrastructure/config/components/ModelSettingsPage.tsx @@ -1635,42 +1635,23 @@ const ModelSettingsPage: React.FC = () => { isProviderGroupEdit ); - let updatedModels: AIModelConfigType[]; - if (editingConfig.id) { - updatedModels = aiModels.map(m => m.id === editingConfig.id ? configsToSave[0] : m); - } else if (isProviderGroupEdit) { - updatedModels = [ - ...aiModels.filter(model => !providerGroupModelIds.has(model.id || '')), - ...configsToSave, - ]; - } else { - updatedModels = [ - ...aiModels, - ...configsToSave, - ]; - } - - - await configManager.setConfig('ai.models', updatedModels); - setAiModels(updatedModels); - - // Auto-set as primary model if no primary model is configured and this is a new model - if (!editingConfig.id) { - try { - const currentDefaultModels = await configManager.getConfig>('ai.default_models') || {}; - const primaryModelExists = currentDefaultModels.primary && updatedModels.some(m => m.id === currentDefaultModels.primary); - if (!primaryModelExists) { - await configManager.setConfig('ai.default_models', { - ...currentDefaultModels, - primary: configsToSave[0]?.id, - }); - log.info('Auto-set primary model for first configured model', { modelId: configsToSave[0]?.id }); - notification.success(t('messages.autoSetPrimary')); + const updatedModels = await configManager.updateConfig('ai.models', current => { + if (editingConfig.id) { + if (!current.some(model => model.id === editingConfig.id)) { + throw new Error('The model was removed while it was being edited'); } - } catch (error) { - log.warn('Failed to auto-set primary model', { error }); + return current.map(model => model.id === editingConfig.id ? { ...model, ...configsToSave[0] } : model); } - } + if (isProviderGroupEdit) { + return [ + ...current.filter(model => !providerGroupModelIds.has(model.id || '')), + ...configsToSave, + ]; + } + return [...current, ...configsToSave]; + }); + setAiModels(updatedModels); + // The host reconciles default selectors using model capabilities. setIsEditing(false); @@ -1738,24 +1719,10 @@ const ModelSettingsPage: React.FC = () => { const handleDelete = async (id: string) => { try { - const updatedModels = aiModels.filter(m => m.id !== id); - await configManager.setConfig('ai.models', updatedModels); + const updatedModels = await configManager.updateConfig( + 'ai.models', current => current.filter(model => model.id !== id) + ); setAiModels(updatedModels); - - const currentDefaultModels = await configManager.getConfig>('ai.default_models') || {}; - const nextDefaultModels = { ...currentDefaultModels }; - let defaultModelsChanged = false; - - for (const key of ['primary', 'fast', 'image_understanding', 'speech_recognition']) { - if (nextDefaultModels[key] === id) { - nextDefaultModels[key] = null; - defaultModelsChanged = true; - } - } - - if (defaultModelsChanged) { - await configManager.setConfig('ai.default_models', nextDefaultModels); - } } catch (error) { log.error('Failed to delete config', { configId: id, error }); } @@ -1819,10 +1786,9 @@ const ModelSettingsPage: React.FC = () => { if (!config.id) return; try { - const updatedModels = aiModels.map(model => - model.id === config.id ? { ...model, enabled } : model + const updatedModels = await configManager.updateConfig( + 'ai.models', current => current.map(model => model.id === config.id ? { ...model, enabled } : model) ); - await configManager.setConfig('ai.models', updatedModels); setAiModels(updatedModels); } catch (error) { log.error('Failed to toggle model status', { configId: config.id, enabled, error }); diff --git a/src/web-ui/src/infrastructure/config/components/QuickActionsConfig.tsx b/src/web-ui/src/infrastructure/config/components/QuickActionsConfig.tsx index bef964a284..9d093d579c 100644 --- a/src/web-ui/src/infrastructure/config/components/QuickActionsConfig.tsx +++ b/src/web-ui/src/infrastructure/config/components/QuickActionsConfig.tsx @@ -212,7 +212,7 @@ const QuickActionsConfig: React.FC = () => { try { const settings = await aiExperienceConfigService.getSettingsAsync(); const stored = settings.quick_actions; - setActions((stored && stored.length > 0) ? stored : DEFAULT_QUICK_ACTIONS); + setActions(stored ?? DEFAULT_QUICK_ACTIONS); } catch (error) { log.error('Failed to load quick actions', error); } finally { @@ -224,8 +224,7 @@ const QuickActionsConfig: React.FC = () => { const persist = useCallback(async (next: QuickAction[]) => { try { - const settings = await aiExperienceConfigService.getSettingsAsync(); - await aiExperienceConfigService.saveSettings({ ...settings, quick_actions: next }); + await aiExperienceConfigService.saveSettings({ quick_actions: next }); setActions(next); notification.success(t('messages.saved')); } catch (error) { diff --git a/src/web-ui/src/infrastructure/config/components/RuntimeSettingsPages.tsx b/src/web-ui/src/infrastructure/config/components/RuntimeSettingsPages.tsx index 491043ee5b..565d35270c 100644 --- a/src/web-ui/src/infrastructure/config/components/RuntimeSettingsPages.tsx +++ b/src/web-ui/src/infrastructure/config/components/RuntimeSettingsPages.tsx @@ -426,7 +426,7 @@ const RuntimeSettingsPage: React.FC = ({ page }) => { const newSettings = { ...settings, [key]: value }; setSettings(newSettings); try { - await aiExperienceConfigService.saveSettings(newSettings); + await aiExperienceConfigService.saveSettings({ [key]: value }); notification.success(t('messages.saveSuccess')); } catch (error) { log.error('Failed to save AI features settings', error); @@ -493,7 +493,7 @@ const RuntimeSettingsPage: React.FC = ({ page }) => { if (settings.agent_companion_pet?.packagePath === pet.packagePath) { const next = { ...settings, agent_companion_pet: DEFAULT_AGENT_COMPANION_PET }; setSettings(next); - await aiExperienceConfigService.saveSettings(next); + await aiExperienceConfigService.saveSettings({ agent_companion_pet: DEFAULT_AGENT_COMPANION_PET }); } notification.success(t('features.pet.deleteSuccess')); } catch (error) { diff --git a/src/web-ui/src/infrastructure/config/components/SessionTitleConfig.tsx b/src/web-ui/src/infrastructure/config/components/SessionTitleConfig.tsx index 318dfcc678..37a1a52709 100644 --- a/src/web-ui/src/infrastructure/config/components/SessionTitleConfig.tsx +++ b/src/web-ui/src/infrastructure/config/components/SessionTitleConfig.tsx @@ -93,7 +93,7 @@ export const SessionTitleConfig: React.FC = () => { const next = { ...settings, enable_session_title_generation: checked }; setSettings(next); try { - await aiExperienceConfigService.saveSettings(next); + await aiExperienceConfigService.saveSettings({ enable_session_title_generation: checked }); notifySuccess(t('sessionTitle.messages.saveSuccess')); } catch (error) { log.error('Failed to save session title enable setting', error); diff --git a/src/web-ui/src/infrastructure/config/components/VoiceInputConfig.tsx b/src/web-ui/src/infrastructure/config/components/VoiceInputConfig.tsx index ce2d346e76..2d96327fda 100644 --- a/src/web-ui/src/infrastructure/config/components/VoiceInputConfig.tsx +++ b/src/web-ui/src/infrastructure/config/components/VoiceInputConfig.tsx @@ -12,10 +12,7 @@ import { isTauriRuntime } from '@/infrastructure/runtime'; import { notificationService } from '@/shared/notification-system'; import { createLogger } from '@/shared/utils/logger'; import { useAIExperienceSettings } from '../hooks'; -import { - aiExperienceConfigService, - type AIExperienceSettings, -} from '../services/AIExperienceConfigService'; +import { aiExperienceConfigService } from '../services/AIExperienceConfigService'; import type { VoiceInputSettings } from '../types'; import LocalVoiceModelsConfig from './LocalVoiceModelsConfig'; import { VoiceInputDiagnostics } from './VoiceInputDiagnostics'; @@ -158,15 +155,8 @@ const VoiceInputConfig: React.FC = () => { notificationService.error(t('messages.loadFailed')); return false; } - const nextSettings: AIExperienceSettings = { - ...settings, - voice_input: { - ...settings.voice_input, - ...patch, - }, - }; try { - await aiExperienceConfigService.saveSettings(nextSettings); + await aiExperienceConfigService.saveSettings({ voice_input: patch }); return true; } catch (error) { log.error('Failed to save voice input settings', { error }); diff --git a/src/web-ui/src/infrastructure/config/services/AIExperienceConfigService.test.ts b/src/web-ui/src/infrastructure/config/services/AIExperienceConfigService.test.ts index 0a81e8aea3..4fdca7243f 100644 --- a/src/web-ui/src/infrastructure/config/services/AIExperienceConfigService.test.ts +++ b/src/web-ui/src/infrastructure/config/services/AIExperienceConfigService.test.ts @@ -112,4 +112,41 @@ describe('AIExperienceConfigService startup behavior', () => { expect(configApiMock.getConfig).toHaveBeenCalledWith('app.ai_experience'); expect(configManagerMock.getConfig).not.toHaveBeenCalled(); }); + + it('does not reset cloud voice input when a stale settings view toggles another feature', async () => { + const persisted = { + enable_agent_companion: true, + voice_input: { provider: 'cloud', model_id: 'cloud-fixture', microphone_device_id: 'saved-mic' }, + quick_actions: [{ id: 'fixture', label: 'Fixture', prompt: 'fixture', enabled: true }], + }; + configManagerMock.getConfig.mockResolvedValue(persisted); + configManagerMock.setConfig.mockImplementation(async (path: string, value: unknown) => { + expect(path).toBe('app.ai_experience.enable_agent_companion'); + persisted.enable_agent_companion = value as boolean; + }); + const { aiExperienceConfigService } = await import('./AIExperienceConfigService'); + + // No preceding read is required; fallback defaults must never be saved. + await aiExperienceConfigService.saveSettings({ enable_agent_companion: false }); + await aiExperienceConfigService.reload(); + expect(aiExperienceConfigService.getSettings()).toMatchObject({ + enable_agent_companion: false, + voice_input: persisted.voice_input, + quick_actions: persisted.quick_actions, + }); + expect(configManagerMock.setConfig).toHaveBeenCalledTimes(1); + }); + + it('updates only an edited voice input field and preserves an explicitly empty quick action list', async () => { + configManagerMock.getConfig.mockResolvedValue({ quick_actions: [] }); + configManagerMock.setConfig.mockResolvedValue(undefined); + const { aiExperienceConfigService } = await import('./AIExperienceConfigService'); + + await aiExperienceConfigService.saveSettings({ voice_input: { microphone_device_id: 'new-mic' } }); + expect(configManagerMock.setConfig).toHaveBeenCalledWith( + 'app.ai_experience.voice_input.microphone_device_id', 'new-mic' + ); + await aiExperienceConfigService.reload(); + expect(aiExperienceConfigService.getSettings().quick_actions).toEqual([]); + }); }); diff --git a/src/web-ui/src/infrastructure/config/services/AIExperienceConfigService.ts b/src/web-ui/src/infrastructure/config/services/AIExperienceConfigService.ts index 7d5a108508..377491c6b8 100644 --- a/src/web-ui/src/infrastructure/config/services/AIExperienceConfigService.ts +++ b/src/web-ui/src/infrastructure/config/services/AIExperienceConfigService.ts @@ -33,6 +33,10 @@ export interface AIExperienceSettings { quick_actions?: QuickAction[]; } +export type AIExperienceSettingsPatch = Partial> & { + voice_input?: Partial; +}; + export type AgentCompanionDisplayMode = 'input' | 'desktop'; export interface AgentCompanionPetSelection { @@ -88,6 +92,7 @@ function normalizeSettings(settings: AIExperienceSettings | null | undefined): A ...defaultSettings.voice_input, ...settings?.voice_input, }, + quick_actions: settings?.quick_actions ?? DEFAULT_QUICK_ACTIONS, }; // Legacy configs used null to mean the built-in SVG panda. Resolve null to the current preset. if (!merged.agent_companion_pet) { @@ -128,10 +133,6 @@ export class AIExperienceConfigService { try { const settings = await configManager.getConfig(CONFIG_PATH); const merged = normalizeSettings(settings); - // Seed quick_actions with defaults when the stored value is absent. - if (!merged.quick_actions || merged.quick_actions.length === 0) { - merged.quick_actions = DEFAULT_QUICK_ACTIONS; - } this.cachedSettings = merged; } catch (error) { log.warn('Failed to load config, using defaults', error); @@ -164,12 +165,23 @@ export class AIExperienceConfigService { } - async saveSettings(settings: AIExperienceSettings): Promise { + /** Save only the fields the caller edited, never a cached settings snapshot. */ + async saveSettings(settings: AIExperienceSettingsPatch): Promise { this.ensureConfigWatcher(); try { - const normalized = normalizeSettings(settings); - await configManager.setConfig(CONFIG_PATH, normalized); - this.cachedSettings = normalized; + for (const [key, value] of Object.entries(settings)) { + if (value === undefined) continue; + if (key === 'voice_input' && value && typeof value === 'object') { + for (const [voiceKey, voiceValue] of Object.entries(value)) { + if (voiceValue !== undefined) { + await configManager.setConfig(`${CONFIG_PATH}.voice_input.${voiceKey}`, voiceValue); + } + } + } else { + await configManager.setConfig(`${CONFIG_PATH}.${key}`, value); + } + } + await this.loadSettings(); this.notifyListeners(); } catch (error) { log.error('Failed to save config', error); diff --git a/src/web-ui/src/infrastructure/config/services/ConfigManager.test.ts b/src/web-ui/src/infrastructure/config/services/ConfigManager.test.ts index 3b02d2162f..39eea647e7 100644 --- a/src/web-ui/src/infrastructure/config/services/ConfigManager.test.ts +++ b/src/web-ui/src/infrastructure/config/services/ConfigManager.test.ts @@ -41,6 +41,51 @@ describe('ConfigManager', () => { delete globalThis.__BITFUN_BOOTSTRAP_KEYBINDINGS__; }); + it('does not let a late legacy-model read overwrite an explicit save', async () => { + const read = createDeferred(); + configApiMocks.getConfig.mockReturnValueOnce(read.promise); + configApiMocks.setConfig.mockResolvedValueOnce(undefined); + const pendingRead = configManager.getConfig('ai.models'); + const saved = [{ id: 'saved', name: 'Saved', api_key: 'fixture-key', metadata: { provider_instance_id: 'saved-provider' } }]; + + await configManager.setConfig('ai.models', saved); + read.resolve([{ id: 'old', name: 'Old', base_url: 'https://example.com/v1' }]); + + await expect(pendingRead).resolves.toEqual(saved); + expect(configApiMocks.setConfig).toHaveBeenCalledTimes(1); + expect(configApiMocks.setConfig).toHaveBeenCalledWith('ai.models', saved); + }); + + it('calculates queued model edits from fresh host state without losing other credentials', async () => { + type Model = { id: string; name: string; api_key: string; metadata: { provider_instance_id: string } }; + const fixture = (id: string): Model => ({ id, name: id, api_key: `${id}-fixture-key`, metadata: { provider_instance_id: id } }); + let persisted = [fixture('existing')]; + const firstSave = createDeferred(); + configApiMocks.getConfig.mockImplementation(async () => structuredClone(persisted)); + configApiMocks.setConfig.mockImplementation(async (_path: string, value: Model[]) => { + await firstSave.promise; + persisted = structuredClone(value); + }); + + const first = configManager.updateConfig('ai.models', models => [...models, fixture('first')]); + const second = configManager.updateConfig('ai.models', models => [...models, fixture('second')]); + await vi.waitFor(() => expect(configApiMocks.setConfig).toHaveBeenCalledTimes(1)); + expect(configApiMocks.getConfig).toHaveBeenCalledTimes(1); + firstSave.resolve(); + await Promise.all([first, second]); + + expect(persisted).toEqual([fixture('existing'), fixture('first'), fixture('second')]); + expect(configApiMocks.getConfig).toHaveBeenCalledTimes(2); + }); + + it('refuses a model edit when the host read fails instead of saving an empty-list fallback', async () => { + configApiMocks.getConfig.mockRejectedValueOnce(new Error('Host disconnected')); + + await expect(configManager.updateConfig('ai.models', models => models.filter(Boolean))) + .rejects.toThrow('Host disconnected'); + expect(configApiMocks.setConfig).not.toHaveBeenCalled(); + }); + it('deduplicates concurrent reads for the same config path', async () => { const deferred = createDeferred(); configApiMocks.getConfig.mockReturnValueOnce(deferred.promise); @@ -431,7 +476,7 @@ describe('ConfigManager', () => { expect(configManager.get('ai.default_models')).toEqual({ chat: 'gpt-5' }); }); - it('migrates legacy models with the same base URL into one provider instance', async () => { + it('resolves legacy provider instances without writing during a read', async () => { const legacyModels = [ { id: 'model-a', @@ -460,7 +505,7 @@ describe('ConfigManager', () => { expect(firstProviderId).toMatch(/^provider_legacy_/); expect(migrated[1].metadata.provider_instance_id).toBe(firstProviderId); expect(migrated[2].metadata.provider_instance_id).not.toBe(firstProviderId); - expect(configApiMocks.setConfig).toHaveBeenCalledWith('ai.models', migrated); + expect(configApiMocks.setConfig).not.toHaveBeenCalled(); }); it('applyExternalReload notifies listeners only for paths whose value changed', async () => { diff --git a/src/web-ui/src/infrastructure/config/services/ConfigManager.ts b/src/web-ui/src/infrastructure/config/services/ConfigManager.ts index cb32f0e604..29b4550f86 100644 --- a/src/web-ui/src/infrastructure/config/services/ConfigManager.ts +++ b/src/web-ui/src/infrastructure/config/services/ConfigManager.ts @@ -91,7 +91,7 @@ class ConfigManagerImpl implements IConfigManager { log.info('Initializing config manager (proxy mode)'); } - private async migrateLegacyAiModelsIfNeeded(config: unknown): Promise { + private async resolveLegacyAiModels(config: unknown): Promise { if (!Array.isArray(config)) { return config; } @@ -145,8 +145,10 @@ class ConfigManagerImpl implements IConfigManager { return config; } - await configAPI.setConfig('ai.models', migratedModels); - log.info('Migrated legacy ai.models', { + // Reading is a compatibility projection, not permission to replace the + // model list. An asynchronous startup read may already be stale by now. + // These deterministic IDs are persisted by the next explicit model edit. + log.info('Resolved legacy model metadata for display', { migratedNameCount, migratedProviderInstanceCount, }); @@ -311,7 +313,7 @@ class ConfigManagerImpl implements IConfigManager { const readVersion = path ? this.getPathMutationVersion(path) : 0; const config = await configAPI.getConfig(path); const resolvedConfig = path === 'ai.models' - ? await this.migrateLegacyAiModelsIfNeeded(config) + ? await this.resolveLegacyAiModels(config) : config; if (path) { @@ -338,7 +340,7 @@ class ConfigManagerImpl implements IConfigManager { const readVersion = this.getPathMutationVersion(path); const config = await configAPI.getConfig(path, { skipRetryOnNotFound: true }); const resolvedConfig = path === 'ai.models' - ? await this.migrateLegacyAiModelsIfNeeded(config) + ? await this.resolveLegacyAiModels(config) : config; if (readVersion !== this.getPathMutationVersion(path)) { @@ -364,7 +366,7 @@ class ConfigManagerImpl implements IConfigManager { for (const path of paths) { const resolvedConfig = path === 'ai.models' - ? await this.migrateLegacyAiModelsIfNeeded(configs[path]) + ? await this.resolveLegacyAiModels(configs[path]) : configs[path]; resolvedConfigs[path] = await this.resolveReadValue( @@ -615,6 +617,34 @@ class ConfigManagerImpl implements IConfigManager { } } + /** Apply an edit to a fresh host value inside the client mutation queue. */ + async updateConfig(path: string, update: (current: T) => T): Promise { + // Model writes also reconcile default selectors on the host. Invalidate + // their cached siblings and serialize edits to the whole AI section. + const mutationPath = path === 'ai.models' ? 'ai' : path; + let previous!: T; + let next!: T; + await this.runMutation(mutationPath, async () => { + // Deliberately bypass cached/read-failure defaults: an unavailable host + // must not turn a partial edit into replacement of its config with []. + const value = await configAPI.getConfig(path); + previous = (path === 'ai.models' ? await this.resolveLegacyAiModels(value) : value) as T; + if (previous === undefined || previous === null) { + throw new Error(`Cannot update unavailable config: ${path}`); + } + next = update(previous); + await configAPI.setConfig(path, next); + }, () => { + this.configCache.set(path, next); + if (mutationPath === path) { + this.notifyConfigChange(path, previous, next); + } else { + this.notifyConfigChange(mutationPath, undefined, undefined); + } + }); + return next; + } + async saveCloudSpeechConfig( request: import('@/infrastructure/api/service-api/ConfigAPI').SaveCloudSpeechConfigRequest ): Promise { diff --git a/src/web-ui/src/infrastructure/config/types/index.ts b/src/web-ui/src/infrastructure/config/types/index.ts index e64d1c08f3..7651ba9fa7 100644 --- a/src/web-ui/src/infrastructure/config/types/index.ts +++ b/src/web-ui/src/infrastructure/config/types/index.ts @@ -542,6 +542,7 @@ export interface IConfigManager { validateConfig(): Promise; exportConfig(): Promise; importConfig(config: ConfigExport): Promise; + updateConfig(path: string, update: (current: T) => T): Promise; onConfigChange(callback: (path: string, oldValue: any, newValue: any) => void): () => void; refreshCache(): Promise; clearCache(): void;