From 55540dde14e7b9d2a1bbf88b30ddf6385cbf07e4 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 12 Jan 2026 14:02:34 -0800 Subject: [PATCH] Switched to serde to validate (and fall back to default) if enum value is invalid --- codex-rs/Cargo.lock | 1 + codex-rs/core/Cargo.toml | 1 + codex-rs/core/src/config/mod.rs | 54 +++++++++++------------------ codex-rs/core/src/config/profile.rs | 13 +++++++ codex-rs/core/src/config/service.rs | 13 +++---- codex-rs/core/src/config/types.rs | 16 +++++++++ 6 files changed, 57 insertions(+), 41 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index ab7df33fa4..1d48617826 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1323,6 +1323,7 @@ dependencies = [ "seccompiler", "serde", "serde_json", + "serde_with", "serde_yaml", "serial_test", "sha1", diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index d3ee08c03b..598ba8ed51 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -57,6 +57,7 @@ regex-lite = { workspace = true } reqwest = { workspace = true, features = ["json", "stream"] } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } +serde_with = { workspace = true } serde_yaml = { workspace = true } sha1 = { workspace = true } sha2 = { workspace = true } diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 2d9d305cf9..7a19d2b8fb 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -45,6 +45,8 @@ use codex_utils_absolute_path::AbsolutePathBufGuard; use dirs::home_dir; use serde::Deserialize; use serde::Serialize; +use serde_with::DefaultOnError; +use serde_with::serde_as; use similar::DiffableStr; use std::collections::BTreeMap; use std::collections::HashMap; @@ -426,7 +428,6 @@ 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 = 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 @@ -444,37 +445,6 @@ impl ConfigBuilder { } } -fn sanitize_config_toml(mut merged_toml: TomlValue) -> TomlValue { - if let TomlValue::Table(table) = &mut merged_toml { - sanitize_reasoning_effort_table(table); - - 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 { - sanitize_reasoning_effort_table(profile_table); - } - } - } - } - - merged_toml -} - -fn sanitize_reasoning_effort_table(table: &mut toml::map::Map) { - let Some(value) = table.get("model_reasoning_effort") else { - return; - }; - - let Some(raw) = value.as_str() else { - table.remove("model_reasoning_effort"); - return; - }; - - if serde_json::from_str::(&format!("\"{raw}\"")).is_err() { - table.remove("model_reasoning_effort"); - } -} - impl Config { /// This is the preferred way to create an instance of [Config]. pub async fn load_with_cli_overrides( @@ -522,7 +492,6 @@ 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 @@ -720,6 +689,7 @@ pub fn set_default_oss_provider(codex_home: &Path, provider: &str) -> std::io::R } /// Base config deserialized from ~/.codex/config.toml. +#[serde_as] #[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq)] pub struct ConfigToml { /// Optional override of model selection. @@ -737,12 +707,16 @@ pub struct ConfigToml { pub model_auto_compact_token_limit: Option, /// Default approval policy for executing commands. + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub approval_policy: Option, #[serde(default)] pub shell_environment_policy: ShellEnvironmentPolicyToml, /// Sandbox mode to use. + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub sandbox_mode: Option, /// Sandbox configuration to apply if `sandbox` is `WorkspaceWrite`. @@ -768,6 +742,7 @@ pub struct ConfigToml { /// When set, restricts the login mechanism users may use. #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub forced_login_method: Option, /// Preferred backend for storing CLI auth credentials. @@ -775,6 +750,7 @@ pub struct ConfigToml { /// keyring: Use an OS-specific keyring service. /// auto: Use the keyring if available, otherwise use a file. #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub cli_auth_credentials_store: Option, /// Definition for MCP servers that Codex can reach out to for tool calls. @@ -787,6 +763,7 @@ pub struct ConfigToml { /// file: Use a file in the Codex home directory. /// auto (default): Use the OS-specific keyring service if available, otherwise use a file. #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub mcp_oauth_credentials_store: Option, /// Optional fixed port for the local HTTP callback server used during MCP OAuth login. @@ -819,6 +796,8 @@ pub struct ConfigToml { /// Optional URI-based file opener. If set, citations to files in the model /// output will be hyperlinked using the specified URI scheme. + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub file_opener: Option, /// Collection of settings that are specific to the TUI. @@ -832,9 +811,15 @@ pub struct ConfigToml { /// Defaults to `false`. pub show_raw_agent_reasoning: Option, + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub model_reasoning_effort: Option, + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub model_reasoning_summary: Option, /// Optional verbosity control for GPT-5 models (Responses API `text.verbosity`). + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub model_verbosity: Option, /// Override to force-enable reasoning summaries for the configured model. @@ -923,8 +908,11 @@ impl From for UserSavedConfig { } } +#[serde_as] #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] pub struct ProjectConfig { + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub trust_level: Option, } diff --git a/codex-rs/core/src/config/profile.rs b/codex-rs/core/src/config/profile.rs index e1c45c1f16..571eeae24f 100644 --- a/codex-rs/core/src/config/profile.rs +++ b/codex-rs/core/src/config/profile.rs @@ -1,6 +1,8 @@ use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; use serde::Serialize; +use serde_with::DefaultOnError; +use serde_with::serde_as; use crate::protocol::AskForApproval; use codex_protocol::config_types::ReasoningSummary; @@ -10,16 +12,27 @@ use codex_protocol::openai_models::ReasoningEffort; /// Collection of common configuration options that a user can define as a unit /// in `config.toml`. +#[serde_as] #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] pub struct ConfigProfile { pub model: Option, /// The key in the `model_providers` map identifying the /// [`ModelProviderInfo`] to use. pub model_provider: Option, + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub approval_policy: Option, + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub sandbox_mode: Option, + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub model_reasoning_effort: Option, + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub model_reasoning_summary: Option, + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub model_verbosity: Option, pub chatgpt_base_url: Option, pub experimental_instructions_file: Option, diff --git a/codex-rs/core/src/config/service.rs b/codex-rs/core/src/config/service.rs index 913c02df1d..8cf24c5fca 100644 --- a/codex-rs/core/src/config/service.rs +++ b/codex-rs/core/src/config/service.rs @@ -941,7 +941,7 @@ remote_compaction = true } #[tokio::test] - async fn invalid_user_value_rejected_even_if_overridden_by_managed() { + async fn invalid_user_value_overridden_by_managed_is_accepted() { let tmp = tempdir().expect("tempdir"); std::fs::write(tmp.path().join(CONFIG_TOML_FILE), "model = \"user\"").unwrap(); @@ -959,7 +959,7 @@ remote_compaction = true }, ); - let error = service + let response = service .write_value(ConfigValueWriteParams { file_path: Some(tmp.path().join(CONFIG_TOML_FILE).display().to_string()), key_path: "approval_policy".to_string(), @@ -968,16 +968,13 @@ remote_compaction = true expected_version: None, }) .await - .expect_err("should fail validation"); + .expect("write succeeds"); - assert_eq!( - error.write_error_code(), - Some(ConfigWriteErrorCode::ConfigValidationError) - ); + assert_eq!(response.status, WriteStatus::OkOverridden); let contents = std::fs::read_to_string(tmp.path().join(CONFIG_TOML_FILE)).expect("read config"); - assert_eq!(contents.trim(), "model = \"user\""); + assert!(contents.contains("approval_policy = \"bogus\"")); } #[tokio::test] diff --git a/codex-rs/core/src/config/types.rs b/codex-rs/core/src/config/types.rs index 2b41c3c52f..1f1fb4253d 100644 --- a/codex-rs/core/src/config/types.rs +++ b/codex-rs/core/src/config/types.rs @@ -15,6 +15,8 @@ use serde::Deserialize; use serde::Deserializer; use serde::Serialize; use serde::de::Error as SerdeError; +use serde_with::DefaultOnError; +use serde_with::serde_as; pub const DEFAULT_OTEL_ENVIRONMENT: &str = "dev"; @@ -254,9 +256,11 @@ impl UriBasedFileOpener { } /// Settings that govern if and what will be written to `~/.codex/history.jsonl`. +#[serde_as] #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default)] pub struct History { /// If true, history entries will not be written to disk. + #[serde_as(as = "DefaultOnError")] pub persistence: HistoryPersistence, /// If set, the maximum size of the history file in bytes. The oldest entries @@ -332,6 +336,7 @@ pub enum OtelExporterKind { } /// OTEL settings loaded from config.toml. Fields are optional so we can apply defaults. +#[serde_as] #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default)] pub struct OtelConfigToml { /// Log user prompt in traces @@ -341,9 +346,13 @@ pub struct OtelConfigToml { pub environment: Option, /// Optional log exporter + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub exporter: Option, /// Optional trace exporter + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub trace_exporter: Option, } @@ -405,11 +414,13 @@ impl Default for ScrollInputMode { } /// Collection of settings that are specific to the TUI. +#[serde_as] #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default)] pub struct Tui { /// Enable desktop notifications from the TUI when the terminal is unfocused. /// Defaults to `true`. #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub notifications: Notifications, /// Enable animations (welcome screen, shimmer effects, spinners). @@ -499,6 +510,7 @@ pub struct Tui { /// - `wheel`: always use wheel behavior (fixed lines per wheel notch). /// - `trackpad`: always use trackpad behavior (fractional accumulation; wheel may feel slow). #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub scroll_mode: ScrollInputMode, /// Auto-mode threshold: maximum time (ms) for the first tick-worth of events to arrive. @@ -534,6 +546,7 @@ pub struct Tui { /// Using alternate screen provides a cleaner fullscreen experience but prevents /// scrollback in terminal multiplexers like Zellij that follow the xterm spec. #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub alternate_screen: AltScreenMode, } @@ -607,8 +620,11 @@ pub enum ShellEnvironmentPolicyInherit { /// Policy for building the `env` when spawning a process via either the /// `shell` or `local_shell` tool. +#[serde_as] #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default)] pub struct ShellEnvironmentPolicyToml { + #[serde(default)] + #[serde_as(as = "DefaultOnError")] pub inherit: Option, pub ignore_default_excludes: Option,