diff --git a/codex-rs/config/src/lenient.rs b/codex-rs/config/src/lenient.rs index 3ca8e25852..d0e5ded574 100644 --- a/codex-rs/config/src/lenient.rs +++ b/codex-rs/config/src/lenient.rs @@ -1,161 +1,141 @@ -use crate::config_toml::ConfigToml; +use crate::schema::config_schema; use serde_json::Map as JsonMap; use serde_json::Value as JsonValue; -use std::collections::BTreeMap; use std::collections::BTreeSet; use std::panic::AssertUnwindSafe; use std::panic::catch_unwind; -use std::sync::OnceLock; use toml::Value as TomlValue; -static ENUM_FIELD_SPECS: OnceLock> = OnceLock::new(); - -#[derive(Clone, Debug)] -struct EnumFieldSpec { - path: Vec, - allowed_values: BTreeSet, - allows_non_string: bool, -} - -/// One segment in a known enum-valued config path discovered from JSON Schema. -#[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd)] -enum PathSegment { - /// Match this literal table key. - Key(String), - /// Expand across every table child at this point, such as `profiles.*`. - AnyTable, -} - #[derive(Default)] -struct ScalarChoices { +struct EnumChoices { allowed_strings: BTreeSet, allows_non_string: bool, } -/// Deserializes config while turning known invalid enum-valued settings into warnings. +enum PathElement { + Key(String), + Index(usize), +} + +/// Return best-effort startup warnings for raw TOML values that look like invalid enums. /// -/// The config structs use `serde_with::DefaultOnError` on their enum fields so -/// an invalid field can default instead of rejecting the whole file. This helper -/// uses the same generated JSON Schema that backs `config.schema.json` to find -/// string enum-valued leaves. It then does a best-effort pass over the raw TOML: -/// invalid enum leaves are deleted from the sanitized TOML and reported as -/// startup warnings, while full shape/type validation stays with Serde. If the -/// warning pass itself cannot run, config loading continues with the original -/// TOML and no warnings. -pub(crate) fn deserialize_with_enum_warnings( - value: TomlValue, -) -> Result<(TomlValue, ConfigToml, Vec), toml::de::Error> { - let (sanitized, warnings) = sanitize_for_enum_warnings(value); - let parsed = sanitized.clone().try_into::()?; - Ok((sanitized, parsed, warnings)) -} - -fn sanitize_for_enum_warnings(value: TomlValue) -> (TomlValue, Vec) { - let original = value.clone(); +/// `DefaultOnError` is responsible for keeping config deserialization lenient. +/// This pass is intentionally advisory: it walks the final merged TOML value +/// against the generated config schema and reports enum-looking mismatches +/// without changing the TOML that will be deserialized. +pub(crate) fn enum_value_warnings(value: &TomlValue) -> Vec { catch_unwind(AssertUnwindSafe(|| { - let mut sanitized = value; + let Ok(schema) = serde_json::to_value(config_schema()) else { + return Vec::new(); + }; + let definitions = schema.get("definitions").and_then(JsonValue::as_object); + let mut path = Vec::new(); + let mut ref_stack = Vec::new(); let mut warnings = Vec::new(); - for spec in enum_field_specs() { - for path in matching_paths(&sanitized, &spec.path) { - remove_if_invalid(&mut sanitized, &path, spec, &mut warnings); - } - } - (sanitized, warnings) + + collect_warnings( + value, + &schema, + definitions, + &mut path, + &mut ref_stack, + &mut warnings, + ); + + warnings })) - .unwrap_or((original, Vec::new())) + .unwrap_or_default() } -fn enum_field_specs() -> &'static [EnumFieldSpec] { - ENUM_FIELD_SPECS.get_or_init(|| catch_unwind(build_enum_field_specs).unwrap_or_default()) -} - -fn build_enum_field_specs() -> Vec { - let Ok(schema) = serde_json::to_value(crate::schema::config_schema()) else { - return Vec::new(); - }; - let definitions = schema.get("definitions").and_then(JsonValue::as_object); - let mut choices_by_path = BTreeMap::, ScalarChoices>::new(); - let mut path = Vec::new(); - let mut ref_stack = Vec::new(); - - collect_enum_fields( - &schema, - definitions, - &mut path, - &mut choices_by_path, - &mut ref_stack, - ); - - choices_by_path - .into_iter() - .filter_map(|(path, choices)| { - if choices.allowed_strings.is_empty() { - return None; - } - Some(EnumFieldSpec { - path, - allowed_values: choices.allowed_strings, - allows_non_string: choices.allows_non_string, - }) - }) - .collect() -} - -fn collect_enum_fields( +fn collect_warnings( + value: &TomlValue, schema: &JsonValue, definitions: Option<&JsonMap>, - path: &mut Vec, - choices_by_path: &mut BTreeMap, ScalarChoices>, + path: &mut Vec, ref_stack: &mut Vec, + warnings: &mut Vec, ) { - let mut choices = ScalarChoices::default(); - collect_scalar_choices(schema, definitions, &mut choices, ref_stack); - if !choices.allowed_strings.is_empty() { - choices_by_path - .entry(path.clone()) - .or_default() - .merge(choices); + let mut choices = EnumChoices::default(); + collect_enum_choices(schema, definitions, ref_stack, &mut choices); + let invalid_enum_value = if let Some(value) = value.as_str() { + !choices.allowed_strings.contains(value) + } else { + !choices.allows_non_string + }; + if !choices.allowed_strings.is_empty() && invalid_enum_value { + let warning = format!( + "Ignoring invalid config value at {}: {value}", + display_path(path) + ); + if !warnings.contains(&warning) { + warnings.push(warning); + } } if let Some(definition) = resolve_definition(schema, definitions, ref_stack) { - collect_enum_fields(definition, definitions, path, choices_by_path, ref_stack); + collect_warnings(value, definition, definitions, path, ref_stack, warnings); ref_stack.pop(); } - for child in schema_composition_children(schema) { - collect_enum_fields(child, definitions, path, choices_by_path, ref_stack); + for child_schema in schema_composition_children(schema) { + collect_warnings(value, child_schema, definitions, path, ref_stack, warnings); } - if let Some(properties) = schema.get("properties").and_then(JsonValue::as_object) { - for (key, child) in properties { - path.push(PathSegment::Key(key.clone())); - collect_enum_fields(child, definitions, path, choices_by_path, ref_stack); + let properties = schema.get("properties").and_then(JsonValue::as_object); + if let (Some(table), Some(properties)) = (value.as_table(), properties) { + for (key, child_schema) in properties { + let Some(child_value) = table.get(key) else { + continue; + }; + path.push(PathElement::Key(key.clone())); + collect_warnings( + child_value, + child_schema, + definitions, + path, + ref_stack, + warnings, + ); path.pop(); } } - let Some(additional_properties) = schema.get("additionalProperties") else { - return; - }; - if !additional_properties.is_object() { - return; + let additional_properties = schema + .get("additionalProperties") + .filter(|additional_properties| additional_properties.is_object()); + if let (Some(table), Some(additional_properties)) = (value.as_table(), additional_properties) { + for (key, child_value) in table { + if properties.is_some_and(|properties| properties.contains_key(key)) { + continue; + } + path.push(PathElement::Key(key.clone())); + collect_warnings( + child_value, + additional_properties, + definitions, + path, + ref_stack, + warnings, + ); + path.pop(); + } + } + + let items = schema.get("items"); + if let (Some(array), Some(items)) = (value.as_array(), items) { + for (index, child_value) in array.iter().enumerate() { + path.push(PathElement::Index(index)); + collect_warnings(child_value, items, definitions, path, ref_stack, warnings); + path.pop(); + } } - path.push(PathSegment::AnyTable); - collect_enum_fields( - additional_properties, - definitions, - path, - choices_by_path, - ref_stack, - ); - path.pop(); } -fn collect_scalar_choices( +fn collect_enum_choices( schema: &JsonValue, definitions: Option<&JsonMap>, - choices: &mut ScalarChoices, ref_stack: &mut Vec, + choices: &mut EnumChoices, ) { choices .allowed_strings @@ -163,12 +143,12 @@ fn collect_scalar_choices( choices.allows_non_string |= schema_allows_non_string(schema); if let Some(definition) = resolve_definition(schema, definitions, ref_stack) { - collect_scalar_choices(definition, definitions, choices, ref_stack); + collect_enum_choices(definition, definitions, ref_stack, choices); ref_stack.pop(); } - for child in schema_composition_children(schema) { - collect_scalar_choices(child, definitions, choices, ref_stack); + for child_schema in schema_composition_children(schema) { + collect_enum_choices(child_schema, definitions, ref_stack, choices); } } @@ -220,87 +200,26 @@ fn schema_composition_children(schema: &JsonValue) -> impl Iterator, -) { - let Some(raw_value) = value_at_path(value, path) else { - return; - }; - if let Some(raw_value) = raw_value.as_str() { - if spec.allowed_values.contains(raw_value) { - return; - } - } else if spec.allows_non_string { - return; +fn display_path(path: &[PathElement]) -> String { + if path.is_empty() { + return "".to_string(); } - let Some(invalid_value) = remove_value_at_path(value, path) else { - return; - }; - warnings.push(format!( - "Ignoring invalid config value at {}: {invalid_value}", - display_path(path) - )); -} -/// Expands a spec path into concrete TOML paths without holding borrows across mutation. -fn matching_paths(value: &TomlValue, spec_path: &[PathSegment]) -> Vec> { - let mut paths = vec![Vec::new()]; - for segment in spec_path { - match segment { - PathSegment::Key(key) => { - for path in &mut paths { - path.push(key.clone()); + let mut output = String::new(); + for element in path { + match element { + PathElement::Key(key) => { + if !output.is_empty() { + output.push('.'); } + output.push_str(&display_key(key)); } - PathSegment::AnyTable => { - let mut expanded = Vec::new(); - for path in paths { - let Some(table) = value_at_path(value, &path).and_then(TomlValue::as_table) - else { - continue; - }; - expanded.extend(table.iter().filter_map(|(key, value)| { - if !value.is_table() { - return None; - } - let mut child = path.clone(); - child.push(key.clone()); - Some(child) - })); - } - paths = expanded; + PathElement::Index(index) => { + output.push_str(&format!("[{index}]")); } } } - paths -} - -fn value_at_path<'a>(value: &'a TomlValue, path: &[String]) -> Option<&'a TomlValue> { - let mut current = value; - for key in path { - current = current.as_table()?.get(key)?; - } - Some(current) -} - -fn remove_value_at_path(value: &mut TomlValue, path: &[String]) -> Option { - let (last, parents) = path.split_last()?; - let mut current = value; - for key in parents { - current = current.as_table_mut()?.get_mut(key)?; - } - current.as_table_mut()?.remove(last) -} - -fn display_path(path: &[String]) -> String { - path.iter() - .map(|key| display_key(key)) - .collect::>() - .join(".") + output } fn display_key(key: &str) -> String { @@ -314,10 +233,3 @@ fn display_key(key: &str) -> String { let escaped = key.replace('\\', "\\\\").replace('"', "\\\""); format!("\"{escaped}\"") } - -impl ScalarChoices { - fn merge(&mut self, other: ScalarChoices) { - self.allowed_strings.extend(other.allowed_strings); - self.allows_non_string |= other.allows_non_string; - } -} diff --git a/codex-rs/config/src/loader/mod.rs b/codex-rs/config/src/loader/mod.rs index 55779b075e..0154d38bc0 100644 --- a/codex-rs/config/src/loader/mod.rs +++ b/codex-rs/config/src/loader/mod.rs @@ -15,7 +15,6 @@ use crate::diagnostics::ConfigError; use crate::diagnostics::config_error_from_toml; use crate::diagnostics::first_layer_config_error_from_entries as typed_first_layer_config_error_from_entries; use crate::diagnostics::io_error_from_config_error; -use crate::lenient::deserialize_with_enum_warnings; use crate::merge::merge_toml_values; use crate::overrides::build_cli_overrides_layer; use crate::project_root_markers::default_project_root_markers; @@ -870,15 +869,12 @@ pub fn resolve_relative_paths_in_config_toml( value_from_config_toml: TomlValue, base_dir: &Path, ) -> io::Result { - // Use the serialize/deserialize round-trip to convert the - // `toml::Value` into a `ConfigToml` with `AbsolutePathBuf` fields resolved - // by the guard. Invalid enum values are ignored only for this typed - // projection; copy_shape_from_original preserves them in the raw layer so - // the final config load can still surface the startup warning. + // Use the serialize/deserialize round-trip to convert the `toml::Value` + // into a `ConfigToml` with `AbsolutePathBuf` fields resolved by the guard. + // copy_shape_from_original preserves raw non-path leaves so invalid enum + // values can still be reported from the final merged TOML. let _guard = AbsolutePathBufGuard::new(base_dir); - let Ok((_sanitized, resolved, _enum_warnings)) = - deserialize_with_enum_warnings(value_from_config_toml.clone()) - else { + let Ok(resolved) = value_from_config_toml.clone().try_into::() else { return Ok(value_from_config_toml); }; drop(_guard); @@ -897,9 +893,9 @@ pub fn resolve_relative_paths_in_config_toml( } /// Ensure that every field in `original` is present in the returned -/// `toml::Value`, taking the value from `resolved` where possible. This ensures -/// the fields that we "removed" during the serialize/deserialize round-trip in -/// `resolve_config_paths` are preserved, out of an abundance of caution. +/// `toml::Value`, copying only the absolute path strings produced by the typed +/// round-trip. Other leaves stay raw so lenient enum defaults do not overwrite +/// the value that should produce a startup warning. fn copy_shape_from_original(original: &TomlValue, resolved: &TomlValue) -> TomlValue { match (original, resolved) { (TomlValue::Table(original_table), TomlValue::Table(resolved_table)) => { @@ -921,7 +917,14 @@ fn copy_shape_from_original(original: &TomlValue, resolved: &TomlValue) -> TomlV } TomlValue::Array(items) } - (_, resolved_value) => resolved_value.clone(), + (TomlValue::String(original), TomlValue::String(resolved)) => { + if original != resolved && Path::new(resolved).is_absolute() { + TomlValue::String(resolved.clone()) + } else { + TomlValue::String(original.clone()) + } + } + (original_value, _) => original_value.clone(), } } diff --git a/codex-rs/config/src/state.rs b/codex-rs/config/src/state.rs index 6961c7a253..f8561956a7 100644 --- a/codex-rs/config/src/state.rs +++ b/codex-rs/config/src/state.rs @@ -4,7 +4,7 @@ use crate::config_requirements::ConfigRequirementsToml; use super::fingerprint::record_origins; use super::fingerprint::version_for_toml; use super::key_aliases::normalized_with_key_aliases; -use super::lenient::deserialize_with_enum_warnings; +use super::lenient::enum_value_warnings; use super::merge::merge_toml_values; use crate::config_toml::ConfigToml; use codex_app_server_protocol::ConfigLayer; @@ -320,32 +320,16 @@ impl ConfigLayerStack { /// Deserializes the merged config-layer view and returns any soft warnings. /// - /// This is the boundary where user-editable config stays lenient for - /// invalid enum values while consumers still receive a fully typed config. - /// Each layer is sanitized before it is merged so a higher-precedence - /// invalid enum value falls back to a valid lower-precedence value. + /// Invalid enum-valued settings are handled by `DefaultOnError` during + /// deserialization. Warnings are collected from the final raw TOML view so + /// warning discovery remains advisory and cannot change config semantics. pub fn deserialize_effective_config_with_warnings( &self, ) -> Result<(TomlValue, ConfigToml, Vec), toml::de::Error> { - let mut merged = TomlValue::Table(toml::map::Map::new()); - let mut warnings = Vec::new(); - - for layer in self.get_layers( - ConfigLayerStackOrdering::LowestPrecedenceFirst, - /*include_disabled*/ false, - ) { - let mut normalized_layer = TomlValue::Table(toml::map::Map::new()); - merge_toml_values(&mut normalized_layer, &layer.config); - let (sanitized_layer, _typed_layer, layer_warnings) = - deserialize_with_enum_warnings(normalized_layer)?; - warnings.extend(layer_warnings); - merge_toml_values(&mut merged, &sanitized_layer); - } - - let (sanitized_effective, typed_effective, effective_warnings) = - deserialize_with_enum_warnings(merged)?; - warnings.extend(effective_warnings); - Ok((sanitized_effective, typed_effective, warnings)) + let merged = self.effective_config(); + let warnings = enum_value_warnings(&merged); + let typed = merged.clone().try_into::()?; + Ok((merged, typed, warnings)) } /// Returns field origins for the merged config-layer view. diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index d82cabc81a..a36e651e30 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -25,7 +25,6 @@ use codex_config::loader::load_config_layers_state; use codex_config::loader::load_requirements_toml; use codex_config::version_for_toml; use codex_exec_server::LOCAL_FS; -use codex_protocol::config_types::SandboxMode; use codex_protocol::config_types::TrustLevel; use codex_protocol::config_types::WebSearchMode; use codex_protocol::models::PermissionProfile; @@ -148,18 +147,21 @@ context_size = "galactic" ) .await?; - // The invalid enum leaves are removed before ConfigToml is produced, but - // unrelated valid settings still survive in the effective config. + // The warning pass reads the final TOML value without mutating it, while + // DefaultOnError lets unrelated valid settings still deserialize. let (effective_config, _config_toml, enum_warnings) = layers.deserialize_effective_config_with_warnings()?; let expected_config = toml::from_str::( r#" model = "gpt-5-codex" +sandbox_mode = "make-it-so" [tui] +notification_method = "loudly" theme = "loudly" [tools.web_search] +context_size = "galactic" "#, ) .expect("expected config should parse"); @@ -177,7 +179,7 @@ theme = "loudly" } #[tokio::test] -async fn invalid_higher_precedence_enum_falls_back_to_lower_layer() -> anyhow::Result<()> { +async fn invalid_higher_precedence_enum_warns_without_mutating_merge() -> anyhow::Result<()> { let tmp = tempdir().expect("tempdir"); let system_config_path = tmp.path().join("system_config.toml"); std::fs::write(&system_config_path, r#"sandbox_mode = "workspace-write""#) @@ -213,8 +215,8 @@ async fn invalid_higher_precedence_enum_falls_back_to_lower_layer() -> anyhow::R enum_warnings, ), ( - Some(&TomlValue::String("workspace-write".to_string())), - Some(SandboxMode::WorkspaceWrite), + Some(&TomlValue::String("make-it-so".to_string())), + None, vec!["Ignoring invalid config value at sandbox_mode: \"make-it-so\"".to_string()], ) ); @@ -325,6 +327,7 @@ sandbox_mode = "hold-my-coffee" r#" [profiles."alpha.beta"] model = "gpt-5-codex" +sandbox_mode = "hold-my-coffee" "#, ) .expect("expected config should parse"), diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 4720ec575b..606822e6e9 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -976,9 +976,8 @@ impl ConfigBuilder { // respective config file, so we should be safe to deserialize without // AbsolutePathBufGuard here. // - // Invalid enum-valued settings default at the typed field boundary and - // are removed from the merged TOML view when the best-effort scan can - // identify the offending leaf. + // Invalid enum-valued settings default at the typed field boundary. + // The warning scan reads the final merged TOML view without mutating it. let (_merged_toml, config_toml, enum_warnings) = match config_layer_stack .deserialize_effective_config_with_warnings() {