diff --git a/codex-rs/core/src/config_edit.rs b/codex-rs/core/src/config_edit.rs index ae41ebcb..4b3a5cbd 100644 --- a/codex-rs/core/src/config_edit.rs +++ b/codex-rs/core/src/config_edit.rs @@ -7,6 +7,12 @@ use toml_edit::DocumentMut; pub const CONFIG_KEY_MODEL: &str = "model"; pub const CONFIG_KEY_EFFORT: &str = "model_reasoning_effort"; +#[derive(Copy, Clone)] +enum NoneBehavior { + Skip, + Remove, +} + /// Persist overrides into `config.toml` using explicit key segments per /// override. This avoids ambiguity with keys that contain dots or spaces. pub async fn persist_overrides( @@ -14,47 +20,12 @@ pub async fn persist_overrides( profile: Option<&str>, overrides: &[(&[&str], &str)], ) -> Result<()> { - let config_path = codex_home.join(CONFIG_TOML_FILE); + let with_options: Vec<(&[&str], Option<&str>)> = overrides + .iter() + .map(|(segments, value)| (*segments, Some(*value))) + .collect(); - let mut doc = match tokio::fs::read_to_string(&config_path).await { - Ok(s) => s.parse::()?, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - tokio::fs::create_dir_all(codex_home).await?; - DocumentMut::new() - } - Err(e) => return Err(e.into()), - }; - - let effective_profile = if let Some(p) = profile { - Some(p.to_owned()) - } else { - doc.get("profile") - .and_then(|i| i.as_str()) - .map(|s| s.to_string()) - }; - - for (segments, val) in overrides.iter().copied() { - let value = toml_edit::value(val); - if let Some(ref name) = effective_profile { - if segments.first().copied() == Some("profiles") { - apply_toml_edit_override_segments(&mut doc, segments, value); - } else { - let mut seg_buf: Vec<&str> = Vec::with_capacity(2 + segments.len()); - seg_buf.push("profiles"); - seg_buf.push(name.as_str()); - seg_buf.extend_from_slice(segments); - apply_toml_edit_override_segments(&mut doc, &seg_buf, value); - } - } else { - apply_toml_edit_override_segments(&mut doc, segments, value); - } - } - - let tmp_file = NamedTempFile::new_in(codex_home)?; - tokio::fs::write(tmp_file.path(), doc.to_string()).await?; - tmp_file.persist(config_path)?; - - Ok(()) + persist_overrides_with_behavior(codex_home, profile, &with_options, NoneBehavior::Skip).await } /// Persist overrides where values may be optional. Any entries with `None` @@ -65,16 +36,17 @@ pub async fn persist_non_null_overrides( profile: Option<&str>, overrides: &[(&[&str], Option<&str>)], ) -> Result<()> { - let filtered: Vec<(&[&str], &str)> = overrides - .iter() - .filter_map(|(k, v)| v.map(|vv| (*k, vv))) - .collect(); + persist_overrides_with_behavior(codex_home, profile, overrides, NoneBehavior::Skip).await +} - if filtered.is_empty() { - return Ok(()); - } - - persist_overrides(codex_home, profile, &filtered).await +/// Persist overrides where `None` values clear any existing values from the +/// configuration file. +pub async fn persist_overrides_and_clear_if_none( + codex_home: &Path, + profile: Option<&str>, + overrides: &[(&[&str], Option<&str>)], +) -> Result<()> { + persist_overrides_with_behavior(codex_home, profile, overrides, NoneBehavior::Remove).await } /// Apply a single override onto a `toml_edit` document while preserving @@ -121,6 +93,125 @@ fn apply_toml_edit_override_segments( current[last] = value; } +async fn persist_overrides_with_behavior( + codex_home: &Path, + profile: Option<&str>, + overrides: &[(&[&str], Option<&str>)], + none_behavior: NoneBehavior, +) -> Result<()> { + if overrides.is_empty() { + return Ok(()); + } + + let should_skip = match none_behavior { + NoneBehavior::Skip => overrides.iter().all(|(_, value)| value.is_none()), + NoneBehavior::Remove => false, + }; + + if should_skip { + return Ok(()); + } + + let config_path = codex_home.join(CONFIG_TOML_FILE); + + let read_result = tokio::fs::read_to_string(&config_path).await; + let mut doc = match read_result { + Ok(contents) => contents.parse::()?, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + if overrides + .iter() + .all(|(_, value)| value.is_none() && matches!(none_behavior, NoneBehavior::Remove)) + { + return Ok(()); + } + + tokio::fs::create_dir_all(codex_home).await?; + DocumentMut::new() + } + Err(e) => return Err(e.into()), + }; + + let effective_profile = if let Some(p) = profile { + Some(p.to_owned()) + } else { + doc.get("profile") + .and_then(|i| i.as_str()) + .map(|s| s.to_string()) + }; + + let mut mutated = false; + + for (segments, value) in overrides.iter().copied() { + let mut seg_buf: Vec<&str> = Vec::new(); + let segments_to_apply: &[&str]; + + if let Some(ref name) = effective_profile { + if segments.first().copied() == Some("profiles") { + segments_to_apply = segments; + } else { + seg_buf.reserve(2 + segments.len()); + seg_buf.push("profiles"); + seg_buf.push(name.as_str()); + seg_buf.extend_from_slice(segments); + segments_to_apply = seg_buf.as_slice(); + } + } else { + segments_to_apply = segments; + } + + match value { + Some(v) => { + let item_value = toml_edit::value(v); + apply_toml_edit_override_segments(&mut doc, segments_to_apply, item_value); + mutated = true; + } + None => { + if matches!(none_behavior, NoneBehavior::Remove) + && remove_toml_edit_segments(&mut doc, segments_to_apply) + { + mutated = true; + } + } + } + } + + if !mutated { + return Ok(()); + } + + let tmp_file = NamedTempFile::new_in(codex_home)?; + tokio::fs::write(tmp_file.path(), doc.to_string()).await?; + tmp_file.persist(config_path)?; + + Ok(()) +} + +fn remove_toml_edit_segments(doc: &mut DocumentMut, segments: &[&str]) -> bool { + use toml_edit::Item; + + if segments.is_empty() { + return false; + } + + let mut current = doc.as_table_mut(); + for seg in &segments[..segments.len() - 1] { + let Some(item) = current.get_mut(seg) else { + return false; + }; + + match item { + Item::Table(table) => { + current = table; + } + _ => { + return false; + } + } + } + + current.remove(segments[segments.len() - 1]).is_some() +} + #[cfg(test)] mod tests { use super::*; @@ -574,6 +665,81 @@ model = "o3" assert_eq!(contents, expected); } + #[tokio::test] + async fn persist_clear_none_removes_top_level_value() { + let tmpdir = tempdir().expect("tmp"); + let codex_home = tmpdir.path(); + + let seed = r#"model = "gpt-5" +model_reasoning_effort = "medium" +"#; + tokio::fs::write(codex_home.join(CONFIG_TOML_FILE), seed) + .await + .expect("seed write"); + + persist_overrides_and_clear_if_none( + codex_home, + None, + &[ + (&[CONFIG_KEY_MODEL], None), + (&[CONFIG_KEY_EFFORT], Some("high")), + ], + ) + .await + .expect("persist"); + + let contents = read_config(codex_home).await; + let expected = "model_reasoning_effort = \"high\"\n"; + assert_eq!(contents, expected); + } + + #[tokio::test] + async fn persist_clear_none_respects_active_profile() { + let tmpdir = tempdir().expect("tmp"); + let codex_home = tmpdir.path(); + + let seed = r#"profile = "team" + +[profiles.team] +model = "gpt-4" +model_reasoning_effort = "minimal" +"#; + tokio::fs::write(codex_home.join(CONFIG_TOML_FILE), seed) + .await + .expect("seed write"); + + persist_overrides_and_clear_if_none( + codex_home, + None, + &[ + (&[CONFIG_KEY_MODEL], None), + (&[CONFIG_KEY_EFFORT], Some("high")), + ], + ) + .await + .expect("persist"); + + let contents = read_config(codex_home).await; + let expected = r#"profile = "team" + +[profiles.team] +model_reasoning_effort = "high" +"#; + assert_eq!(contents, expected); + } + + #[tokio::test] + async fn persist_clear_none_noop_when_file_missing() { + let tmpdir = tempdir().expect("tmp"); + let codex_home = tmpdir.path(); + + persist_overrides_and_clear_if_none(codex_home, None, &[(&[CONFIG_KEY_MODEL], None)]) + .await + .expect("persist"); + + assert!(!codex_home.join(CONFIG_TOML_FILE).exists()); + } + // Test helper moved to bottom per review guidance. async fn read_config(codex_home: &Path) -> String { let p = codex_home.join(CONFIG_TOML_FILE); diff --git a/codex-rs/mcp-server/src/codex_message_processor.rs b/codex-rs/mcp-server/src/codex_message_processor.rs index 6cd6fdec..2850c6b8 100644 --- a/codex-rs/mcp-server/src/codex_message_processor.rs +++ b/codex-rs/mcp-server/src/codex_message_processor.rs @@ -20,7 +20,7 @@ use codex_core::config::ConfigToml; use codex_core::config::load_config_as_toml; use codex_core::config_edit::CONFIG_KEY_EFFORT; use codex_core::config_edit::CONFIG_KEY_MODEL; -use codex_core::config_edit::persist_non_null_overrides; +use codex_core::config_edit::persist_overrides_and_clear_if_none; use codex_core::default_client::get_codex_user_agent; use codex_core::exec::ExecParams; use codex_core::exec_env::create_env; @@ -519,7 +519,7 @@ impl CodexMessageProcessor { (&[CONFIG_KEY_EFFORT], effort_str.as_deref()), ]; - match persist_non_null_overrides( + match persist_overrides_and_clear_if_none( &self.config.codex_home, self.config.active_profile.as_deref(), &overrides, diff --git a/codex-rs/mcp-server/tests/suite/set_default_model.rs b/codex-rs/mcp-server/tests/suite/set_default_model.rs index e11b69ed..7ffee343 100644 --- a/codex-rs/mcp-server/tests/suite/set_default_model.rs +++ b/codex-rs/mcp-server/tests/suite/set_default_model.rs @@ -1,5 +1,6 @@ +use std::path::Path; + use codex_core::config::ConfigToml; -use codex_protocol::config_types::ReasoningEffort; use codex_protocol::mcp_protocol::SetDefaultModelParams; use codex_protocol::mcp_protocol::SetDefaultModelResponse; use mcp_test_support::McpProcess; @@ -14,7 +15,8 @@ const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn set_default_model_persists_overrides() { - let codex_home = TempDir::new().unwrap_or_else(|e| panic!("create tempdir: {e}")); + let codex_home = TempDir::new().expect("create tempdir"); + create_config_toml(codex_home.path()).expect("write config.toml"); let mut mcp = McpProcess::new(codex_home.path()) .await @@ -25,8 +27,8 @@ async fn set_default_model_persists_overrides() { .expect("init failed"); let params = SetDefaultModelParams { - model: Some("o4-mini".to_string()), - reasoning_effort: Some(ReasoningEffort::High), + model: Some("gpt-4.1".to_string()), + reasoning_effort: None, }; let request_id = mcp @@ -53,10 +55,22 @@ async fn set_default_model_persists_overrides() { assert_eq!( ConfigToml { - model: Some("o4-mini".to_string()), - model_reasoning_effort: Some(ReasoningEffort::High), + model: Some("gpt-4.1".to_string()), + model_reasoning_effort: None, ..Default::default() }, config_toml, ); } + +// Helper to create a config.toml; mirrors create_conversation.rs +fn create_config_toml(codex_home: &Path) -> std::io::Result<()> { + let config_toml = codex_home.join("config.toml"); + std::fs::write( + config_toml, + r#" +model = "gpt-5" +model_reasoning_effort = "medium" +"#, + ) +} diff --git a/codex-rs/protocol/src/mcp_protocol.rs b/codex-rs/protocol/src/mcp_protocol.rs index 42f38d46..24357ce1 100644 --- a/codex-rs/protocol/src/mcp_protocol.rs +++ b/codex-rs/protocol/src/mcp_protocol.rs @@ -424,8 +424,11 @@ pub struct GetUserSavedConfigResponse { #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)] #[serde(rename_all = "camelCase")] pub struct SetDefaultModelParams { + /// If set to None, this means `model` should be cleared in config.toml. #[serde(skip_serializing_if = "Option::is_none")] pub model: Option, + /// If set to None, this means `model_reasoning_effort` should be cleared + /// in config.toml. #[serde(skip_serializing_if = "Option::is_none")] pub reasoning_effort: Option, }