Switched to serde to validate (and fall back to default) if enum value is invalid

This commit is contained in:
Eric Traut
2026-01-12 14:02:34 -08:00
parent d6b38d80dd
commit 55540dde14
6 changed files with 57 additions and 41 deletions

1
codex-rs/Cargo.lock generated
View File

@@ -1323,6 +1323,7 @@ dependencies = [
"seccompiler",
"serde",
"serde_json",
"serde_with",
"serde_yaml",
"serial_test",
"sha1",

View File

@@ -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 }

View File

@@ -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<String, TomlValue>) {
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::<ReasoningEffort>(&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<i64>,
/// Default approval policy for executing commands.
#[serde(default)]
#[serde_as(as = "DefaultOnError")]
pub approval_policy: Option<AskForApproval>,
#[serde(default)]
pub shell_environment_policy: ShellEnvironmentPolicyToml,
/// Sandbox mode to use.
#[serde(default)]
#[serde_as(as = "DefaultOnError")]
pub sandbox_mode: Option<SandboxMode>,
/// 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<ForcedLoginMethod>,
/// 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<AuthCredentialsStoreMode>,
/// 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<OAuthCredentialsStoreMode>,
/// 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<UriBasedFileOpener>,
/// 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<bool>,
#[serde(default)]
#[serde_as(as = "DefaultOnError")]
pub model_reasoning_effort: Option<ReasoningEffort>,
#[serde(default)]
#[serde_as(as = "DefaultOnError")]
pub model_reasoning_summary: Option<ReasoningSummary>,
/// Optional verbosity control for GPT-5 models (Responses API `text.verbosity`).
#[serde(default)]
#[serde_as(as = "DefaultOnError")]
pub model_verbosity: Option<Verbosity>,
/// Override to force-enable reasoning summaries for the configured model.
@@ -923,8 +908,11 @@ impl From<ConfigToml> for UserSavedConfig {
}
}
#[serde_as]
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)]
pub struct ProjectConfig {
#[serde(default)]
#[serde_as(as = "DefaultOnError")]
pub trust_level: Option<TrustLevel>,
}

View File

@@ -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<String>,
/// The key in the `model_providers` map identifying the
/// [`ModelProviderInfo`] to use.
pub model_provider: Option<String>,
#[serde(default)]
#[serde_as(as = "DefaultOnError")]
pub approval_policy: Option<AskForApproval>,
#[serde(default)]
#[serde_as(as = "DefaultOnError")]
pub sandbox_mode: Option<SandboxMode>,
#[serde(default)]
#[serde_as(as = "DefaultOnError")]
pub model_reasoning_effort: Option<ReasoningEffort>,
#[serde(default)]
#[serde_as(as = "DefaultOnError")]
pub model_reasoning_summary: Option<ReasoningSummary>,
#[serde(default)]
#[serde_as(as = "DefaultOnError")]
pub model_verbosity: Option<Verbosity>,
pub chatgpt_base_url: Option<String>,
pub experimental_instructions_file: Option<AbsolutePathBuf>,

View File

@@ -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]

View File

@@ -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<String>,
/// Optional log exporter
#[serde(default)]
#[serde_as(as = "DefaultOnError")]
pub exporter: Option<OtelExporterKind>,
/// Optional trace exporter
#[serde(default)]
#[serde_as(as = "DefaultOnError")]
pub trace_exporter: Option<OtelExporterKind>,
}
@@ -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<ShellEnvironmentPolicyInherit>,
pub ignore_default_excludes: Option<bool>,