From c7e207973b89cb9f6e30e2675dd109e01fda2e61 Mon Sep 17 00:00:00 2001 From: celia-oai Date: Tue, 9 Dec 2025 20:15:09 -0800 Subject: [PATCH 1/3] changes --- codex-rs/Cargo.lock | 1 + codex-rs/app-server/Cargo.toml | 1 + codex-rs/app-server/src/config_api.rs | 126 +++++++++++++++++++++++--- 3 files changed, 116 insertions(+), 12 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index cfb46697475..f4d8c70d250 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -901,6 +901,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 99d5a7a1410..0c0bd479731 100644 --- a/codex-rs/app-server/Cargo.toml +++ b/codex-rs/app-server/Cargo.toml @@ -34,6 +34,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 ae02927f7af..a6a054a13c0 100644 --- a/codex-rs/app-server/src/config_api.rs +++ b/codex-rs/app-server/src/config_api.rs @@ -15,6 +15,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::apply_blocking; use codex_core::config_loader::LoadedConfigLayers; use codex_core::config_loader::LoaderOverrides; use codex_core::config_loader::load_config_layers_with_overrides; @@ -26,9 +28,9 @@ 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"; @@ -143,11 +145,13 @@ 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) })?; @@ -163,6 +167,18 @@ impl ConfigApi { } })?; + if changed { + let updated_value = value_at_path(&user_config, &segments).cloned(); + let mut path = segments.clone(); + collect_config_edits( + &mut path, + original_value.as_ref(), + updated_value.as_ref(), + &mut config_edits, + ) + .map_err(|err| internal_error("failed to build config edits", err))?; + } + mutated |= changed; parsed_segments.push(segments); } @@ -174,7 +190,7 @@ impl ConfigApi { ) })?; - let updated_layers = layers.with_user_config(user_config.clone()); + let updated_layers = layers.clone().with_user_config(user_config.clone()); let effective = updated_layers.effective_config(); validate_config(&effective).map_err(|err| { config_write_error( @@ -184,7 +200,7 @@ impl ConfigApi { })?; if mutated { - self.persist_user_config(&user_config) + self.persist_user_config(config_edits) .await .map_err(|err| internal_error("failed to persist config.toml", err))?; } @@ -254,18 +270,15 @@ impl ConfigApi { }) } - async fn persist_user_config(&self, user_config: &TomlValue) -> anyhow::Result<()> { + async fn persist_user_config(&self, edits: Vec) -> anyhow::Result<()> { + if edits.is_empty() { + return Ok(()); + } + 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(()) + apply_blocking(&codex_home, None, &edits) }) .await .map_err(|err| anyhow!("config persistence task panicked: {err}"))??; @@ -422,6 +435,95 @@ fn clear_path(root: &mut TomlValue, segments: &[String]) -> Result, + original: Option<&TomlValue>, + updated: Option<&TomlValue>, + edits: &mut Vec, +) -> anyhow::Result<()> { + match (original, updated) { + (Some(old_value), Some(new_value)) if old_value == new_value => {} + (Some(TomlValue::Table(old_table)), Some(TomlValue::Table(new_table))) => { + for (key, old_value) in old_table.iter() { + if new_table.contains_key(key) { + let mut nested = path.clone(); + nested.push(key.clone()); + collect_config_edits(&mut nested, Some(old_value), new_table.get(key), edits)?; + } else { + let mut nested = path.clone(); + nested.push(key.clone()); + edits.push(ConfigEdit::ClearPath { segments: nested }); + } + } + + for (key, new_value) in new_table.iter() { + if old_table.contains_key(key) { + continue; + } + + let mut nested = path.clone(); + nested.push(key.clone()); + edits.push(ConfigEdit::SetPath { + segments: nested, + value: toml_value_to_item(new_value)?, + }); + } + } + (_, Some(new_value)) => { + edits.push(ConfigEdit::SetPath { + segments: path.clone(), + value: toml_value_to_item(new_value)?, + }); + } + (Some(_), None) => { + edits.push(ConfigEdit::ClearPath { + segments: path.clone(), + }); + } + (None, None) => {} + } + + Ok(()) +} + +fn toml_value_to_item(value: &TomlValue) -> 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.clone())), + 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, From 02aafa2e84765764331eb1ab75b70bc1444c86a2 Mon Sep 17 00:00:00 2001 From: celia-oai Date: Tue, 9 Dec 2025 21:06:16 -0800 Subject: [PATCH 2/3] changes --- codex-rs/app-server/src/config_api.rs | 206 +++++++++++++++----------- codex-rs/core/src/config/edit.rs | 23 +++ 2 files changed, 140 insertions(+), 89 deletions(-) diff --git a/codex-rs/app-server/src/config_api.rs b/codex-rs/app-server/src/config_api.rs index a6a054a13c0..8d711fe18c3 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; @@ -16,7 +15,7 @@ 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::apply_blocking; +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; @@ -28,7 +27,6 @@ use sha2::Sha256; use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; -use tokio::task; use toml::Value as TomlValue; use toml_edit::Item as TomlItem; @@ -143,7 +141,6 @@ 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(); @@ -156,8 +153,8 @@ impl ConfigApi { 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", @@ -165,21 +162,24 @@ impl ConfigApi { MergeError::Validation(message) => { config_write_error(ConfigWriteErrorCode::ConfigValidationError, message) } - })?; - - if changed { - let updated_value = value_at_path(&user_config, &segments).cloned(); - let mut path = segments.clone(); - collect_config_edits( - &mut path, - original_value.as_ref(), - updated_value.as_ref(), - &mut config_edits, - ) - .map_err(|err| internal_error("failed to build config edits", err))?; + }, + )?; + + 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); } @@ -199,8 +199,10 @@ impl ConfigApi { ) })?; - if mutated { - self.persist_user_config(config_edits) + 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))?; } @@ -269,22 +271,6 @@ impl ConfigApi { mdm, }) } - - async fn persist_user_config(&self, edits: Vec) -> anyhow::Result<()> { - if edits.is_empty() { - return Ok(()); - } - - let codex_home = self.codex_home.clone(); - - task::spawn_blocking(move || -> anyhow::Result<()> { - apply_blocking(&codex_home, None, &edits) - }) - .await - .map_err(|err| anyhow!("config persistence task panicked: {err}"))??; - - Ok(()) - } } fn parse_value(value: JsonValue) -> Result, String> { @@ -435,57 +421,6 @@ fn clear_path(root: &mut TomlValue, segments: &[String]) -> Result, - original: Option<&TomlValue>, - updated: Option<&TomlValue>, - edits: &mut Vec, -) -> anyhow::Result<()> { - match (original, updated) { - (Some(old_value), Some(new_value)) if old_value == new_value => {} - (Some(TomlValue::Table(old_table)), Some(TomlValue::Table(new_table))) => { - for (key, old_value) in old_table.iter() { - if new_table.contains_key(key) { - let mut nested = path.clone(); - nested.push(key.clone()); - collect_config_edits(&mut nested, Some(old_value), new_table.get(key), edits)?; - } else { - let mut nested = path.clone(); - nested.push(key.clone()); - edits.push(ConfigEdit::ClearPath { segments: nested }); - } - } - - for (key, new_value) in new_table.iter() { - if old_table.contains_key(key) { - continue; - } - - let mut nested = path.clone(); - nested.push(key.clone()); - edits.push(ConfigEdit::SetPath { - segments: nested, - value: toml_value_to_item(new_value)?, - }); - } - } - (_, Some(new_value)) => { - edits.push(ConfigEdit::SetPath { - segments: path.clone(), - value: toml_value_to_item(new_value)?, - }); - } - (Some(_), None) => { - edits.push(ConfigEdit::ClearPath { - segments: path.clone(), - }); - } - (None, None) => {} - } - - Ok(()) -} - fn toml_value_to_item(value: &TomlValue) -> anyhow::Result { match value { TomlValue::Table(table) => { @@ -506,7 +441,7 @@ fn toml_value_to_value(value: &TomlValue) -> anyhow::Result { 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.clone())), + TomlValue::Datetime(val) => Ok(toml_edit::Value::from(*val)), TomlValue::Array(items) => { let mut array = toml_edit::Array::new(); for item in items { @@ -840,6 +775,99 @@ mod tests { 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() { + 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).unwrap(); + + 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); + } + #[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 68e2d206f0d..71d0cb2c747 100644 --- a/codex-rs/core/src/config/edit.rs +++ b/codex-rs/core/src/config/edit.rs @@ -555,6 +555,11 @@ impl ConfigEditsBuilder { self } + pub fn with_edits(mut self, edits: Vec) -> Self { + 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 +608,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"); From b27a294c753cde98fdb08fce563c4c04ca641e73 Mon Sep 17 00:00:00 2001 From: celia-oai Date: Wed, 10 Dec 2025 10:46:09 -0800 Subject: [PATCH 3/3] comment --- codex-rs/app-server/src/config_api.rs | 8 +++++--- codex-rs/core/src/config/edit.rs | 5 ++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/codex-rs/app-server/src/config_api.rs b/codex-rs/app-server/src/config_api.rs index 8d711fe18c3..98fe93fb259 100644 --- a/codex-rs/app-server/src/config_api.rs +++ b/codex-rs/app-server/src/config_api.rs @@ -190,7 +190,7 @@ impl ConfigApi { ) })?; - let updated_layers = layers.clone().with_user_config(user_config.clone()); + let updated_layers = layers.with_user_config(user_config.clone()); let effective = updated_layers.effective_config(); validate_config(&effective).map_err(|err| { config_write_error( @@ -772,6 +772,7 @@ 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; @@ -825,7 +826,7 @@ X-Doc = "42" } #[tokio::test] - async fn write_value_preserves_comments_and_order() { + async fn write_value_preserves_comments_and_order() -> Result<()> { let tmp = tempdir().expect("tempdir"); let original = r#"# Codex user configuration model = "gpt-5" @@ -838,7 +839,7 @@ hide_full_access_warning = true [features] unified_exec = true "#; - std::fs::write(tmp.path().join(CONFIG_FILE_NAME), original).unwrap(); + std::fs::write(tmp.path().join(CONFIG_FILE_NAME), original)?; let api = ConfigApi::new(tmp.path().to_path_buf(), vec![]); api.write_value(ConfigValueWriteParams { @@ -866,6 +867,7 @@ unified_exec = true remote_compaction = true "#; assert_eq!(updated, expected); + Ok(()) } #[tokio::test] diff --git a/codex-rs/core/src/config/edit.rs b/codex-rs/core/src/config/edit.rs index 71d0cb2c747..37c2aba6efd 100644 --- a/codex-rs/core/src/config/edit.rs +++ b/codex-rs/core/src/config/edit.rs @@ -555,7 +555,10 @@ impl ConfigEditsBuilder { self } - pub fn with_edits(mut self, edits: Vec) -> Self { + pub fn with_edits(mut self, edits: I) -> Self + where + I: IntoIterator, + { self.edits.extend(edits); self }