From 237e916ca6b54413aef7a6491deb38ff8b23078f Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 12 Jan 2026 14:53:16 -0800 Subject: [PATCH] =?UTF-8?q?Tried=20another=20approach=20=E2=80=94=C2=A0a?= =?UTF-8?q?=20lenient=20decoder?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- codex-rs/Cargo.lock | 2 +- codex-rs/Cargo.toml | 1 + codex-rs/core/Cargo.toml | 2 +- codex-rs/core/src/config/mod.rs | 112 +++++++++++++++++++++++----- codex-rs/core/src/config/profile.rs | 8 -- codex-rs/core/src/config/service.rs | 13 ++-- codex-rs/core/src/config/types.rs | 13 ---- 7 files changed, 105 insertions(+), 46 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 1d48617826..d355da65bd 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1323,7 +1323,7 @@ dependencies = [ "seccompiler", "serde", "serde_json", - "serde_with", + "serde_path_to_error", "serde_yaml", "serial_test", "sha1", diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index a2d8b09b60..3befc31744 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -187,6 +187,7 @@ seccompiler = "0.5.0" sentry = "0.46.0" serde = "1" serde_json = "1" +serde_path_to_error = "0.1" serde_with = "3.16" serde_yaml = "0.9" serial_test = "3.2.0" diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 598ba8ed51..a9e06bc6c3 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -57,7 +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_path_to_error = { 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 7a19d2b8fb..100f8970d9 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -45,8 +45,6 @@ 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; @@ -433,9 +431,7 @@ impl ConfigBuilder { // relative paths to absolute paths based on the parent folder of the // respective config file, so we should be safe to deserialize without // AbsolutePathBufGuard here. - let config_toml: ConfigToml = merged_toml - .try_into() - .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; + let config_toml = deserialize_config_toml_lenient(merged_toml, None)?; Config::load_config_with_layer_stack( config_toml, harness_overrides, @@ -492,7 +488,7 @@ pub async fn load_config_as_toml_with_cli_overrides( .await?; let merged_toml = config_layer_stack.effective_config(); - let cfg = deserialize_config_toml_with_base(merged_toml, codex_home).map_err(|e| { + let cfg = deserialize_config_toml_lenient(merged_toml, Some(codex_home)).map_err(|e| { tracing::error!("Failed to deserialize overridden config: {e}"); e })?; @@ -500,6 +496,7 @@ pub async fn load_config_as_toml_with_cli_overrides( Ok(cfg) } +#[cfg(test)] fn deserialize_config_toml_with_base( root_value: TomlValue, config_base_dir: &Path, @@ -512,6 +509,97 @@ fn deserialize_config_toml_with_base( .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e)) } +fn deserialize_config_toml_lenient( + mut root_value: TomlValue, + config_base_dir: Option<&Path>, +) -> std::io::Result { + loop { + let attempt = match config_base_dir { + Some(base_dir) => { + let _guard = AbsolutePathBufGuard::new(base_dir); + deserialize_config_toml_with_path(&root_value) + } + None => deserialize_config_toml_with_path(&root_value), + }; + + match attempt { + Ok(config) => return Ok(config), + Err(err) => { + let path = err.path().clone(); + let inner = err.into_inner(); + if !is_unknown_enum_error(&inner) { + return Err(std::io::Error::new(std::io::ErrorKind::InvalidData, inner)); + } + if path.to_string().is_empty() || !remove_toml_path(&mut root_value, &path) { + return Err(std::io::Error::new(std::io::ErrorKind::InvalidData, inner)); + } + } + } + } +} + +fn deserialize_config_toml_with_path( + value: &TomlValue, +) -> Result> { + serde_path_to_error::deserialize(value.clone()) +} + +fn is_unknown_enum_error(error: &toml::de::Error) -> bool { + error.to_string().contains("unknown variant") +} + +#[derive(Debug)] +enum TomlPathSegment { + Key(String), + Index(usize), +} + +fn remove_toml_path(root: &mut TomlValue, path: &serde_path_to_error::Path) -> bool { + let segments: Vec = path + .iter() + .filter_map(|segment| match segment { + serde_path_to_error::Segment::Seq { index } => Some(TomlPathSegment::Index(*index)), + serde_path_to_error::Segment::Map { key } => Some(TomlPathSegment::Key(key.clone())), + serde_path_to_error::Segment::Enum { .. } => None, + serde_path_to_error::Segment::Unknown => None, + }) + .collect(); + + if segments.is_empty() { + return false; + } + + let mut current = root; + for segment in &segments[..segments.len() - 1] { + current = match (current, segment) { + (TomlValue::Table(table), TomlPathSegment::Key(key)) => match table.get_mut(key) { + Some(value) => value, + None => return false, + }, + (TomlValue::Array(items), TomlPathSegment::Index(index)) => { + match items.get_mut(*index) { + Some(value) => value, + None => return false, + } + } + _ => return false, + }; + } + + match (current, segments.last()) { + (TomlValue::Table(table), Some(TomlPathSegment::Key(key))) => table.remove(key).is_some(), + (TomlValue::Array(items), Some(TomlPathSegment::Index(index))) => { + if *index < items.len() { + items.remove(*index); + true + } else { + false + } + } + _ => false, + } +} + pub async fn load_global_mcp_servers( codex_home: &Path, ) -> std::io::Result> { @@ -689,7 +777,6 @@ 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. @@ -708,7 +795,6 @@ pub struct ConfigToml { /// Default approval policy for executing commands. #[serde(default)] - #[serde_as(as = "DefaultOnError")] pub approval_policy: Option, #[serde(default)] @@ -716,7 +802,6 @@ pub struct ConfigToml { /// Sandbox mode to use. #[serde(default)] - #[serde_as(as = "DefaultOnError")] pub sandbox_mode: Option, /// Sandbox configuration to apply if `sandbox` is `WorkspaceWrite`. @@ -742,7 +827,6 @@ 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. @@ -750,7 +834,6 @@ 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. @@ -763,7 +846,6 @@ 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. @@ -797,7 +879,6 @@ 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. @@ -812,14 +893,11 @@ pub struct ConfigToml { 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. @@ -908,11 +986,9 @@ 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 571eeae24f..ade8f98e7b 100644 --- a/codex-rs/core/src/config/profile.rs +++ b/codex-rs/core/src/config/profile.rs @@ -1,8 +1,6 @@ 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; @@ -12,7 +10,6 @@ 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, @@ -20,19 +17,14 @@ pub struct ConfigProfile { /// [`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 8cf24c5fca..913c02df1d 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_overridden_by_managed_is_accepted() { + async fn invalid_user_value_rejected_even_if_overridden_by_managed() { 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 response = service + let error = service .write_value(ConfigValueWriteParams { file_path: Some(tmp.path().join(CONFIG_TOML_FILE).display().to_string()), key_path: "approval_policy".to_string(), @@ -968,13 +968,16 @@ remote_compaction = true expected_version: None, }) .await - .expect("write succeeds"); + .expect_err("should fail validation"); - assert_eq!(response.status, WriteStatus::OkOverridden); + assert_eq!( + error.write_error_code(), + Some(ConfigWriteErrorCode::ConfigValidationError) + ); let contents = std::fs::read_to_string(tmp.path().join(CONFIG_TOML_FILE)).expect("read config"); - assert!(contents.contains("approval_policy = \"bogus\"")); + assert_eq!(contents.trim(), "model = \"user\""); } #[tokio::test] diff --git a/codex-rs/core/src/config/types.rs b/codex-rs/core/src/config/types.rs index 1f1fb4253d..bf6e42271d 100644 --- a/codex-rs/core/src/config/types.rs +++ b/codex-rs/core/src/config/types.rs @@ -15,8 +15,6 @@ 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"; @@ -256,11 +254,9 @@ 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 @@ -336,7 +332,6 @@ 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 @@ -347,12 +342,10 @@ pub struct OtelConfigToml { /// 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, } @@ -414,13 +407,11 @@ 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). @@ -510,7 +501,6 @@ 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. @@ -546,7 +536,6 @@ 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, } @@ -620,11 +609,9 @@ 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,