diff --git a/codex-rs/app-server/src/config_manager_service.rs b/codex-rs/app-server/src/config_manager_service.rs index 64c5bee684..4255b83e62 100644 --- a/codex-rs/app-server/src/config_manager_service.rs +++ b/codex-rs/app-server/src/config_manager_service.rs @@ -403,10 +403,47 @@ fn parse_key_path(path: &str) -> Result, String> { if path.trim().is_empty() { return Err("keyPath must not be empty".to_string()); } - Ok(path - .split('.') - .map(std::string::ToString::to_string) - .collect()) + + let mut segments = Vec::new(); + let mut segment = String::new(); + let mut chars = path.chars(); + let mut quoted = false; + + // Split on dots unless they appear inside a quoted segment. Bare segments + // intentionally stay permissive so existing paths like `sample@catalog` + // remain valid. + while let Some(ch) = chars.next() { + match ch { + '"' if segment.is_empty() && !quoted => quoted = true, + '"' if quoted => quoted = false, + '\\' if quoted => { + // Quoted segments may escape punctuation that would otherwise + // participate in parsing, such as `.` or `"`. + let Some(escaped) = chars.next() else { + return Err("unterminated escape in keyPath".to_string()); + }; + segment.push(escaped); + } + '.' if !quoted => { + if segment.is_empty() { + return Err("keyPath segments must not be empty".to_string()); + } + segments.push(std::mem::take(&mut segment)); + } + '"' => return Err("invalid quoted keyPath segment".to_string()), + _ => segment.push(ch), + } + } + + if quoted { + return Err("unterminated quoted keyPath segment".to_string()); + } + if segment.is_empty() { + return Err("keyPath segments must not be empty".to_string()); + } + + segments.push(segment); + Ok(segments) } #[derive(Debug)] diff --git a/codex-rs/app-server/tests/suite/v2/config_rpc.rs b/codex-rs/app-server/tests/suite/v2/config_rpc.rs index 38ba12f95f..56c4d0b7d1 100644 --- a/codex-rs/app-server/tests/suite/v2/config_rpc.rs +++ b/codex-rs/app-server/tests/suite/v2/config_rpc.rs @@ -893,6 +893,71 @@ async fn config_batch_write_applies_multiple_edits() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn config_batch_write_preserves_dotted_profile_names() -> Result<()> { + let tmp_dir = TempDir::new()?; + let codex_home = tmp_dir.path().canonicalize()?; + write_config( + &tmp_dir, + r#" +profile = "team.prod" + +[profiles."team.prod"] +model = "gpt-5.3-spark" + +[profiles.team.prod] +model = "should-stay-put" +"#, + )?; + + let mut mcp = McpProcess::new(&codex_home).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let batch_id = mcp + .send_config_batch_write_request(ConfigBatchWriteParams { + file_path: Some(codex_home.join("config.toml").display().to_string()), + edits: vec![ + ConfigEdit { + key_path: "profiles.\"team.prod\".model".to_string(), + value: json!("gpt-5.5"), + merge_strategy: MergeStrategy::Replace, + }, + ConfigEdit { + key_path: "items.sample@catalog.enabled".to_string(), + value: json!(true), + merge_strategy: MergeStrategy::Replace, + }, + ], + expected_version: None, + reload_user_config: false, + }) + .await?; + let batch_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(batch_id)), + ) + .await??; + let batch_write: ConfigWriteResponse = to_response(batch_resp)?; + assert_eq!(batch_write.status, WriteStatus::Ok); + + let config: toml::Value = + toml::from_str(&std::fs::read_to_string(codex_home.join("config.toml"))?)?; + assert_eq!( + config["profiles"]["team.prod"]["model"].as_str(), + Some("gpt-5.5") + ); + assert_eq!( + config["profiles"]["team"]["prod"]["model"].as_str(), + Some("should-stay-put") + ); + assert_eq!( + config["items"]["sample@catalog"]["enabled"].as_bool(), + Some(true) + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn config_batch_write_updates_multiple_desktop_settings() -> Result<()> { let tmp_dir = TempDir::new()?; diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index 15e4f9b6cd..9271089c43 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -1183,11 +1183,15 @@ impl App { } AppEvent::PersistModelSelection { model, effort } => { let profile = self.active_profile.as_deref(); - match ConfigEditsBuilder::for_config(&self.config) - .with_profile(profile) - .set_model(Some(model.as_str()), effort) - .apply() - .await + match crate::config_update::write_config_batch( + app_server.request_handle(), + crate::config_update::build_model_selection_edits( + profile, + model.as_str(), + effort, + ), + ) + .await { Ok(()) => { let effort_label = effort @@ -1261,11 +1265,14 @@ impl App { } AppEvent::PersistPersonalitySelection { personality } => { let profile = self.active_profile.as_deref(); - match ConfigEditsBuilder::for_config(&self.config) - .with_profile(profile) - .set_personality(Some(personality)) - .apply() - .await + match crate::config_update::write_config_batch( + app_server.request_handle(), + vec![crate::config_update::replace_config_value( + crate::config_update::profile_scoped_key_path(profile, "personality"), + serde_json::json!(personality.to_string()), + )], + ) + .await { Ok(()) => { let label = Self::personality_label(personality); @@ -1298,14 +1305,16 @@ impl App { self.refresh_status_line(); let profile = self.active_profile.as_deref(); self.config.service_tier = service_tier.clone(); - let mut edits = ConfigEditsBuilder::for_config(&self.config) - .with_profile(profile) - .set_service_tier(service_tier.clone()); + let edits = crate::config_update::build_service_tier_selection_edits( + profile, + service_tier.as_deref(), + ); if service_tier.is_none() { self.config.notices.fast_default_opt_out = Some(true); - edits = edits.set_fast_default_opt_out(/*opted_out*/ true); } - match edits.apply().await { + match crate::config_update::write_config_batch(app_server.request_handle(), edits) + .await + { Ok(()) => { let mut message = if let Some(service_tier) = service_tier { format!("Service tier set to {service_tier}") @@ -1476,23 +1485,17 @@ impl App { self.sync_active_thread_permission_settings_to_cached_session() .await; let profile = self.active_profile.as_deref(); - let segments = if let Some(profile) = profile { - vec![ - "profiles".to_string(), - profile.to_string(), - "approvals_reviewer".to_string(), - ] - } else { - vec!["approvals_reviewer".to_string()] - }; - if let Err(err) = ConfigEditsBuilder::for_config(&self.config) - .with_profile(profile) - .with_edits([ConfigEdit::SetPath { - segments, - value: policy.to_string().into(), - }]) - .apply() - .await + if let Err(err) = crate::config_update::write_config_batch( + app_server.request_handle(), + vec![crate::config_update::replace_config_value( + crate::config_update::profile_scoped_key_path( + profile, + "approvals_reviewer", + ), + serde_json::json!(policy.to_string()), + )], + ) + .await { tracing::error!( error = %err, @@ -1583,27 +1586,23 @@ impl App { } AppEvent::PersistPlanModeReasoningEffort(effort) => { let profile = self.active_profile.as_deref(); - let segments = if let Some(profile) = profile { - vec![ - "profiles".to_string(), - profile.to_string(), - "plan_mode_reasoning_effort".to_string(), - ] - } else { - vec!["plan_mode_reasoning_effort".to_string()] - }; + let key_path = crate::config_update::profile_scoped_key_path( + profile, + "plan_mode_reasoning_effort", + ); let edit = if let Some(effort) = effort { - ConfigEdit::SetPath { - segments, - value: effort.to_string().into(), - } + crate::config_update::replace_config_value( + key_path, + serde_json::json!(effort.to_string()), + ) } else { - ConfigEdit::ClearPath { segments } + crate::config_update::clear_config_value(key_path) }; - if let Err(err) = ConfigEditsBuilder::for_config(&self.config) - .with_edits([edit]) - .apply() - .await + if let Err(err) = crate::config_update::write_config_batch( + app_server.request_handle(), + vec![edit], + ) + .await { tracing::error!( error = %err, diff --git a/codex-rs/tui/src/config_update.rs b/codex-rs/tui/src/config_update.rs new file mode 100644 index 0000000000..bcdbe29a0d --- /dev/null +++ b/codex-rs/tui/src/config_update.rs @@ -0,0 +1,124 @@ +//! App-server-backed config update helpers for the TUI. +//! +//! This module centralizes the small typed update helpers the TUI uses +//! when a config mutation must be owned by the app server rather than written +//! to the local `config.toml` directly. + +use codex_app_server_client::AppServerRequestHandle; +use codex_app_server_protocol::ClientRequest; +use codex_app_server_protocol::ConfigBatchWriteParams; +use codex_app_server_protocol::ConfigEdit; +use codex_app_server_protocol::ConfigWriteResponse; +use codex_app_server_protocol::MergeStrategy; +use codex_app_server_protocol::RequestId; +use color_eyre::eyre::Result; +use color_eyre::eyre::WrapErr; +use serde_json::Value as JsonValue; +use uuid::Uuid; + +pub(crate) fn replace_config_value(key_path: impl Into, value: JsonValue) -> ConfigEdit { + ConfigEdit { + key_path: key_path.into(), + value, + merge_strategy: MergeStrategy::Replace, + } +} + +pub(crate) fn clear_config_value(key_path: impl Into) -> ConfigEdit { + replace_config_value(key_path, JsonValue::Null) +} + +pub(crate) fn profile_scoped_key_path(profile: Option<&str>, key_path: &str) -> String { + if let Some(profile) = profile { + let profile = serde_json::Value::String(profile.to_string()).to_string(); + format!("profiles.{profile}.{key_path}") + } else { + key_path.to_string() + } +} + +pub(crate) fn build_model_selection_edits( + profile: Option<&str>, + model: &str, + effort: Option, +) -> Vec { + let effort_edit = effort.map_or_else( + || clear_config_value(profile_scoped_key_path(profile, "model_reasoning_effort")), + |effort| { + replace_config_value( + profile_scoped_key_path(profile, "model_reasoning_effort"), + serde_json::json!(effort.to_string()), + ) + }, + ); + vec![ + replace_config_value( + profile_scoped_key_path(profile, "model"), + serde_json::json!(model), + ), + effort_edit, + ] +} + +pub(crate) fn build_service_tier_selection_edits( + profile: Option<&str>, + service_tier: Option<&str>, +) -> Vec { + let service_tier_edit = service_tier.map_or_else( + || clear_config_value(profile_scoped_key_path(profile, "service_tier")), + |service_tier| { + let config_value = + match codex_protocol::config_types::ServiceTier::from_request_value(service_tier) { + Some(codex_protocol::config_types::ServiceTier::Fast) => "fast", + Some(codex_protocol::config_types::ServiceTier::Flex) => "flex", + None => service_tier, + }; + replace_config_value( + profile_scoped_key_path(profile, "service_tier"), + serde_json::json!(config_value), + ) + }, + ); + let mut edits = vec![service_tier_edit]; + if service_tier.is_none() { + edits.push(replace_config_value( + "notice.fast_default_opt_out", + serde_json::json!(true), + )); + } + edits +} + +pub(crate) async fn write_config_batch( + request_handle: AppServerRequestHandle, + edits: Vec, +) -> Result<()> { + let request_id = RequestId::String(format!("tui-config-write-{}", Uuid::new_v4())); + let _: ConfigWriteResponse = request_handle + .request_typed(ClientRequest::ConfigBatchWrite { + request_id, + params: ConfigBatchWriteParams { + edits, + file_path: None, + expected_version: None, + reload_user_config: true, + }, + }) + .await + .wrap_err("config/batchWrite failed in TUI")?; + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn profile_scoped_key_path_quotes_dotted_profile_names() { + assert_eq!( + profile_scoped_key_path(Some("team.prod"), "model"), + "profiles.\"team.prod\".model" + ); + } +} diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 69b6bec0f3..871cdcc0dd 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -112,6 +112,7 @@ mod clipboard_copy; mod clipboard_paste; mod collaboration_modes; mod color; +mod config_update; pub(crate) mod custom_terminal; mod pets; pub use custom_terminal::Terminal;