From 8a71f8b6348a4ff48a403615674b17fb890b320e Mon Sep 17 00:00:00 2001 From: Celia Chen Date: Wed, 10 Dec 2025 11:14:27 -0800 Subject: [PATCH] [app-server] Make sure that config writes preserve comments & order or configs (#7789) Make sure that config writes preserve comments and order of configs by utilizing the ConfigEditsBuilder in core. Tested by running a real example and made sure that nothing in the config file changes other than the configs to edit. --- codex-rs/Cargo.lock | 1 + codex-rs/app-server/Cargo.toml | 1 + codex-rs/app-server/src/config_api.rs | 190 ++++++++++++++++++++++---- codex-rs/core/src/config/edit.rs | 26 ++++ 4 files changed, 189 insertions(+), 29 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index df26fcfc9..304343bf2 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -851,6 +851,7 @@ dependencies = [ "tempfile", "tokio", "toml", + "toml_edit", "tracing", "tracing-subscriber", "uuid", diff --git a/codex-rs/app-server/Cargo.toml b/codex-rs/app-server/Cargo.toml index e4a326a2c..948facdea 100644 --- a/codex-rs/app-server/Cargo.toml +++ b/codex-rs/app-server/Cargo.toml @@ -35,6 +35,7 @@ sha2 = { workspace = true } mcp-types = { workspace = true } tempfile = { workspace = true } toml = { workspace = true } +toml_edit = { workspace = true } tokio = { workspace = true, features = [ "io-std", "macros", diff --git a/codex-rs/app-server/src/config_api.rs b/codex-rs/app-server/src/config_api.rs index ae02927f7..98fe93fb2 100644 --- a/codex-rs/app-server/src/config_api.rs +++ b/codex-rs/app-server/src/config_api.rs @@ -1,6 +1,5 @@ use crate::error_code::INTERNAL_ERROR_CODE; use crate::error_code::INVALID_REQUEST_ERROR_CODE; -use anyhow::anyhow; use codex_app_server_protocol::ConfigBatchWriteParams; use codex_app_server_protocol::ConfigLayer; use codex_app_server_protocol::ConfigLayerMetadata; @@ -15,6 +14,8 @@ use codex_app_server_protocol::MergeStrategy; use codex_app_server_protocol::OverriddenMetadata; use codex_app_server_protocol::WriteStatus; use codex_core::config::ConfigToml; +use codex_core::config::edit::ConfigEdit; +use codex_core::config::edit::ConfigEditsBuilder; use codex_core::config_loader::LoadedConfigLayers; use codex_core::config_loader::LoaderOverrides; use codex_core::config_loader::load_config_layers_with_overrides; @@ -26,9 +27,8 @@ use sha2::Sha256; use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; -use tempfile::NamedTempFile; -use tokio::task; use toml::Value as TomlValue; +use toml_edit::Item as TomlItem; const SESSION_FLAGS_SOURCE: &str = "--config"; const MDM_SOURCE: &str = "com.openai.codex/config_toml_base64"; @@ -141,19 +141,20 @@ impl ConfigApi { } let mut user_config = layers.user.config.clone(); - let mut mutated = false; let mut parsed_segments = Vec::new(); + let mut config_edits = Vec::new(); for (key_path, value, strategy) in edits.into_iter() { let segments = parse_key_path(&key_path).map_err(|message| { config_write_error(ConfigWriteErrorCode::ConfigValidationError, message) })?; + let original_value = value_at_path(&user_config, &segments).cloned(); let parsed_value = parse_value(value).map_err(|message| { config_write_error(ConfigWriteErrorCode::ConfigValidationError, message) })?; - let changed = apply_merge(&mut user_config, &segments, parsed_value.as_ref(), strategy) - .map_err(|err| match err { + apply_merge(&mut user_config, &segments, parsed_value.as_ref(), strategy).map_err( + |err| match err { MergeError::PathNotFound => config_write_error( ConfigWriteErrorCode::ConfigPathNotFound, "Path not found", @@ -161,9 +162,24 @@ impl ConfigApi { MergeError::Validation(message) => { config_write_error(ConfigWriteErrorCode::ConfigValidationError, message) } - })?; + }, + )?; + + let updated_value = value_at_path(&user_config, &segments).cloned(); + if original_value != updated_value { + let edit = match updated_value { + Some(value) => ConfigEdit::SetPath { + segments: segments.clone(), + value: toml_value_to_item(&value) + .map_err(|err| internal_error("failed to build config edits", err))?, + }, + None => ConfigEdit::ClearPath { + segments: segments.clone(), + }, + }; + config_edits.push(edit); + } - mutated |= changed; parsed_segments.push(segments); } @@ -183,8 +199,10 @@ impl ConfigApi { ) })?; - if mutated { - self.persist_user_config(&user_config) + if !config_edits.is_empty() { + ConfigEditsBuilder::new(&self.codex_home) + .with_edits(config_edits) + .apply() .await .map_err(|err| internal_error("failed to persist config.toml", err))?; } @@ -253,25 +271,6 @@ impl ConfigApi { mdm, }) } - - async fn persist_user_config(&self, user_config: &TomlValue) -> anyhow::Result<()> { - let codex_home = self.codex_home.clone(); - let serialized = toml::to_string_pretty(user_config)?; - - task::spawn_blocking(move || -> anyhow::Result<()> { - std::fs::create_dir_all(&codex_home)?; - - let target = codex_home.join(CONFIG_FILE_NAME); - let tmp = NamedTempFile::new_in(&codex_home)?; - std::fs::write(tmp.path(), serialized.as_bytes())?; - tmp.persist(&target)?; - Ok(()) - }) - .await - .map_err(|err| anyhow!("config persistence task panicked: {err}"))??; - - Ok(()) - } } fn parse_value(value: JsonValue) -> Result, String> { @@ -422,6 +421,44 @@ fn clear_path(root: &mut TomlValue, segments: &[String]) -> Result anyhow::Result { + match value { + TomlValue::Table(table) => { + let mut table_item = toml_edit::Table::new(); + table_item.set_implicit(false); + for (key, val) in table { + table_item.insert(key, toml_value_to_item(val)?); + } + Ok(TomlItem::Table(table_item)) + } + other => Ok(TomlItem::Value(toml_value_to_value(other)?)), + } +} + +fn toml_value_to_value(value: &TomlValue) -> anyhow::Result { + match value { + TomlValue::String(val) => Ok(toml_edit::Value::from(val.clone())), + TomlValue::Integer(val) => Ok(toml_edit::Value::from(*val)), + TomlValue::Float(val) => Ok(toml_edit::Value::from(*val)), + TomlValue::Boolean(val) => Ok(toml_edit::Value::from(*val)), + TomlValue::Datetime(val) => Ok(toml_edit::Value::from(*val)), + TomlValue::Array(items) => { + let mut array = toml_edit::Array::new(); + for item in items { + array.push(toml_value_to_value(item)?); + } + Ok(toml_edit::Value::Array(array)) + } + TomlValue::Table(table) => { + let mut inline = toml_edit::InlineTable::new(); + for (key, val) in table { + inline.insert(key, toml_value_to_value(val)?); + } + Ok(toml_edit::Value::InlineTable(inline)) + } + } +} + #[derive(Clone)] struct LayerState { name: ConfigLayerName, @@ -735,9 +772,104 @@ fn config_write_error(code: ConfigWriteErrorCode, message: impl Into) -> #[cfg(test)] mod tests { use super::*; + use anyhow::Result; use pretty_assertions::assert_eq; use tempfile::tempdir; + #[test] + fn toml_value_to_item_handles_nested_config_tables() { + let config = r#" +[mcp_servers.docs] +command = "docs-server" + +[mcp_servers.docs.http_headers] +X-Doc = "42" +"#; + + let value: TomlValue = toml::from_str(config).expect("parse config example"); + let item = toml_value_to_item(&value).expect("convert to toml_edit item"); + + let root = item.as_table().expect("root table"); + assert!(!root.is_implicit(), "root table should be explicit"); + + let mcp_servers = root + .get("mcp_servers") + .and_then(TomlItem::as_table) + .expect("mcp_servers table"); + assert!( + !mcp_servers.is_implicit(), + "mcp_servers table should be explicit" + ); + + let docs = mcp_servers + .get("docs") + .and_then(TomlItem::as_table) + .expect("docs table"); + assert_eq!( + docs.get("command") + .and_then(TomlItem::as_value) + .and_then(toml_edit::Value::as_str), + Some("docs-server") + ); + + let http_headers = docs + .get("http_headers") + .and_then(TomlItem::as_table) + .expect("http_headers table"); + assert_eq!( + http_headers + .get("X-Doc") + .and_then(TomlItem::as_value) + .and_then(toml_edit::Value::as_str), + Some("42") + ); + } + + #[tokio::test] + async fn write_value_preserves_comments_and_order() -> Result<()> { + let tmp = tempdir().expect("tempdir"); + let original = r#"# Codex user configuration +model = "gpt-5" +approval_policy = "on-request" + +[notice] +# Preserve this comment +hide_full_access_warning = true + +[features] +unified_exec = true +"#; + std::fs::write(tmp.path().join(CONFIG_FILE_NAME), original)?; + + let api = ConfigApi::new(tmp.path().to_path_buf(), vec![]); + api.write_value(ConfigValueWriteParams { + file_path: Some(tmp.path().join(CONFIG_FILE_NAME).display().to_string()), + key_path: "features.remote_compaction".to_string(), + value: json!(true), + merge_strategy: MergeStrategy::Replace, + expected_version: None, + }) + .await + .expect("write succeeds"); + + let updated = + std::fs::read_to_string(tmp.path().join(CONFIG_FILE_NAME)).expect("read config"); + let expected = r#"# Codex user configuration +model = "gpt-5" +approval_policy = "on-request" + +[notice] +# Preserve this comment +hide_full_access_warning = true + +[features] +unified_exec = true +remote_compaction = true +"#; + assert_eq!(updated, expected); + Ok(()) + } + #[tokio::test] async fn read_includes_origins_and_layers() { let tmp = tempdir().expect("tempdir"); diff --git a/codex-rs/core/src/config/edit.rs b/codex-rs/core/src/config/edit.rs index 68e2d206f..37c2aba6e 100644 --- a/codex-rs/core/src/config/edit.rs +++ b/codex-rs/core/src/config/edit.rs @@ -555,6 +555,14 @@ impl ConfigEditsBuilder { self } + pub fn with_edits(mut self, edits: I) -> Self + where + I: IntoIterator, + { + self.edits.extend(edits); + self + } + /// Apply edits on a blocking thread. pub fn apply_blocking(self) -> anyhow::Result<()> { apply_blocking(&self.codex_home, self.profile.as_deref(), &self.edits) @@ -603,6 +611,24 @@ model_reasoning_effort = "high" assert_eq!(contents, expected); } + #[test] + fn builder_with_edits_applies_custom_paths() { + let tmp = tempdir().expect("tmpdir"); + let codex_home = tmp.path(); + + ConfigEditsBuilder::new(codex_home) + .with_edits(vec![ConfigEdit::SetPath { + segments: vec!["enabled".to_string()], + value: value(true), + }]) + .apply_blocking() + .expect("persist"); + + let contents = + std::fs::read_to_string(codex_home.join(CONFIG_TOML_FILE)).expect("read config"); + assert_eq!(contents, "enabled = true\n"); + } + #[test] fn blocking_set_model_preserves_inline_table_contents() { let tmp = tempdir().expect("tmpdir");