From cffae615943d84dc215c045d2402259e324fa41b Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 12 Jan 2026 11:57:39 -0800 Subject: [PATCH] Avoid crash on invalid enum values in config.toml MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes bug #9104 which results in a panic when config.toml is parsed and one or more enum values are incorrect. Rather than relying on serde to enforce values (and panic if they're incorrect), this change adds a soft failure that falls back to default values. Ideally, we should report errors like this to the user (for all code paths that parse the config — including the TUI, exec, app server, MCP server, etc.), but that's a much larger change. For this PR, I'm sticking with the current strategy of silently falling back to defaults when there's a bad config value. --- codex-rs/core/src/config/mod.rs | 143 +++++++++++++++++++++++++++++++- 1 file changed, 141 insertions(+), 2 deletions(-) diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 7ce0c13d1f..e19d0db73f 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -386,6 +386,11 @@ pub struct ConfigBuilder { loader_overrides: Option, } +#[derive(Debug, Clone, PartialEq, Eq)] +struct ConfigWarning { + pub message: String, +} + impl ConfigBuilder { pub fn codex_home(mut self, codex_home: PathBuf) -> Self { self.codex_home = Some(codex_home); @@ -408,6 +413,11 @@ impl ConfigBuilder { } pub async fn build(self) -> std::io::Result { + let (config, _) = self.build_with_warnings().await?; + Ok(config) + } + + async fn build_with_warnings(self) -> std::io::Result<(Config, Vec)> { let Self { codex_home, cli_overrides, @@ -426,6 +436,7 @@ impl ConfigBuilder { load_config_layers_state(&codex_home, Some(cwd), &cli_overrides, loader_overrides) .await?; let merged_toml = config_layer_stack.effective_config(); + let (merged_toml, warnings) = sanitize_config_toml(merged_toml); // Note that each layer in ConfigLayerStack should have resolved // relative paths to absolute paths based on the parent folder of the @@ -434,15 +445,116 @@ impl ConfigBuilder { let config_toml: ConfigToml = merged_toml .try_into() .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; - Config::load_config_with_layer_stack( + let config = Config::load_config_with_layer_stack( config_toml, harness_overrides, codex_home, config_layer_stack, - ) + )?; + Ok((config, warnings)) } } +fn sanitize_config_toml(mut merged_toml: TomlValue) -> (TomlValue, Vec) { + let mut warnings = Vec::new(); + let allowed = [ + ReasoningEffort::None, + ReasoningEffort::Minimal, + ReasoningEffort::Low, + ReasoningEffort::Medium, + ReasoningEffort::High, + ReasoningEffort::XHigh, + ] + .iter() + .map(|effort| effort.to_string()) + .collect::>(); + + if let TomlValue::Table(table) = &mut merged_toml { + sanitize_reasoning_effort_table(table, "model_reasoning_effort", &allowed, &mut warnings); + + if let Some(TomlValue::Table(profiles)) = table.get_mut("profiles") { + for (profile_name, profile_value) in profiles.iter_mut() { + if let TomlValue::Table(profile_table) = profile_value { + let path = format!("profiles.{profile_name}.model_reasoning_effort"); + sanitize_reasoning_effort_table(profile_table, &path, &allowed, &mut warnings); + } + } + } + } + + (merged_toml, warnings) +} + +fn sanitize_reasoning_effort_table( + table: &mut toml::map::Map, + path: &str, + allowed: &[String], + warnings: &mut Vec, +) { + let Some(value) = table.get("model_reasoning_effort") else { + return; + }; + + let raw = match value.as_str() { + Some(raw) => raw.to_string(), + None => value.to_string(), + }; + let normalized = raw.trim().to_ascii_lowercase(); + + if allowed.iter().any(|effort| effort == &normalized) { + if normalized != raw { + table.insert( + "model_reasoning_effort".to_string(), + TomlValue::String(normalized), + ); + } + return; + } + + table.remove("model_reasoning_effort"); + + let suggestion = closest_value(&normalized, allowed); + let suggestion_text = suggestion + .as_deref() + .map(|value| format!(" Did you mean \"{value}\"?")) + .unwrap_or_default(); + let allowed_text = allowed.join(", "); + warnings.push(ConfigWarning { + message: format!( + "Invalid config value for {path}: \"{raw}\". Allowed values: {allowed_text}.{suggestion_text} Falling back to default." + ), + }); +} + +fn closest_value(input: &str, options: &[String]) -> Option { + let mut best: Option<(&String, usize)> = None; + for option in options { + let distance = edit_distance(input, option); + match best { + Some((_, best_distance)) if distance >= best_distance => {} + _ => best = Some((option, distance)), + } + } + best.map(|(option, _)| option.clone()) +} + +fn edit_distance(a: &str, b: &str) -> usize { + let mut costs = (0..=b.len()).collect::>(); + for (i, ca) in a.chars().enumerate() { + let mut last = i; + costs[0] = i + 1; + for (j, cb) in b.chars().enumerate() { + let next = costs[j + 1]; + let replace_cost = if ca == cb { last } else { last + 1 }; + let insert_cost = costs[j] + 1; + let delete_cost = costs[j + 1] + 1; + costs[j + 1] = replace_cost.min(insert_cost).min(delete_cost); + last = next; + } + } + costs[b.len()] +} + impl Config { /// This is the preferred way to create an instance of [Config]. pub async fn load_with_cli_overrides( @@ -490,6 +602,7 @@ pub async fn load_config_as_toml_with_cli_overrides( .await?; let merged_toml = config_layer_stack.effective_config(); + let (merged_toml, _) = sanitize_config_toml(merged_toml); let cfg = deserialize_config_toml_with_base(merged_toml, codex_home).map_err(|e| { tracing::error!("Failed to deserialize overridden config: {e}"); e @@ -1640,6 +1753,32 @@ persistence = "none" ); } + #[tokio::test] + async fn invalid_reasoning_effort_emits_warning_and_uses_default() -> anyhow::Result<()> { + let codex_home = TempDir::new()?; + let config_path = codex_home.path().join(CONFIG_TOML_FILE); + + tokio::fs::write( + &config_path, + r#" +model_reasoning_effort = "super-high" +"#, + ) + .await?; + + let (config, warnings) = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .build_with_warnings() + .await?; + + assert_eq!(config.model_reasoning_effort, None); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].message.contains("model_reasoning_effort")); + assert!(warnings[0].message.contains("Allowed values")); + + Ok(()) + } + #[test] fn tui_config_missing_notifications_field_defaults_to_enabled() { let cfg = r#"