From b65ab465cefa7b89c68317366b67e44431b90af4 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Sat, 3 Oct 2026 05:07:10 +0000 Subject: [PATCH] Reject unknown TUI keys in strict config validation (#50525) ## Why Flattened notification settings prevent `serde_ignored` from detecting unknown keys in `[tui]`, allowing them to pass `--strict-config` validation. ## What changed Detect unknown TUI keys by comparing configured keys with a serialized `Tui`, keeping accepted keys derived from the configuration type. Apply this check to config files and `-c`/`--config` overrides so keys such as `tui.bogus_key_xyz` produce an unknown-field error while valid notification settings remain accepted. ## Testing Add regression coverage for unknown TUI keys, diagnostic source ranges, valid notification settings, and generic validation behavior. Extend CLI tests to cover TUI overrides and config validation before terminal checks. GitOrigin-RevId: b04467523442183e7a54415d82c3f632f97ef319 --- codex-rs/cli/tests/features.rs | 26 ++++++-- codex-rs/config/src/loader/mod.rs | 18 +++-- codex-rs/config/src/strict_config.rs | 41 ++++++++++-- codex-rs/config/src/strict_config_tests.rs | 77 ++++++++++++++++++++++ 4 files changed, 141 insertions(+), 21 deletions(-) diff --git a/codex-rs/cli/tests/features.rs b/codex-rs/cli/tests/features.rs index 9ce0314e32..6da9fd9325 100644 --- a/codex-rs/cli/tests/features.rs +++ b/codex-rs/cli/tests/features.rs @@ -27,13 +27,19 @@ fn codex_command(codex_home: &Path) -> Result { #[test] fn strict_config_rejects_unknown_config_override() -> Result<()> { - let codex_home = TempDir::new()?; - - let mut cmd = codex_command(codex_home.path())?; - cmd.args(["--strict-config", "-c", "foo=bar", "exec", "hello"]) - .assert() - .failure() - .stderr(contains("unknown configuration field")); + for (config_override, ignored_path) in [ + ("foo=bar", "foo"), + ("tui.bogus_key_xyz=1", "tui.bogus_key_xyz"), + ] { + let codex_home = TempDir::new()?; + let mut cmd = codex_command(codex_home.path())?; + cmd.args(["--strict-config", "-c", config_override, "exec", "hello"]) + .assert() + .failure() + .stderr(contains(format!( + "unknown configuration field `{ignored_path}`" + ))); + } Ok(()) } @@ -73,6 +79,12 @@ fn interactive_validates_config_before_requiring_terminal() -> Result<()> { "unknown_key = true", "unknown configuration field", ), + ( + &["--strict-config"], + "config.toml", + "[tui]\nbogus_key_xyz = 1", + "unknown configuration field `tui.bogus_key_xyz`", + ), ( &["--strict-config", "-c", "foo=bar"], "config.toml", diff --git a/codex-rs/config/src/loader/mod.rs b/codex-rs/config/src/loader/mod.rs index 33854b0270..e524076d7b 100644 --- a/codex-rs/config/src/loader/mod.rs +++ b/codex-rs/config/src/loader/mod.rs @@ -45,6 +45,7 @@ use crate::strict_config::config_error_from_ignored_toml_value_fields; use crate::strict_config::ignored_config_warning; use crate::strict_config::ignored_toml_value_fields; use crate::strict_config::unknown_feature_toml_value_field; +use crate::strict_config::unknown_tui_toml_value_path; use crate::thread_config::ThreadConfigContext; use crate::thread_config::ThreadConfigLoader; use codex_file_system::ExecutorFileSystem; @@ -653,16 +654,13 @@ fn validate_cli_overrides_strictly( base_dir: &Path, ) -> io::Result<()> { let _guard = AbsolutePathBufGuard::new(base_dir); - if let Some(path) = ignored_toml_value_fields::(cli_overrides_layer.clone()).first() - { - let ignored_path = path.join("."); - return Err(io::Error::new( - io::ErrorKind::InvalidData, - format!("unknown configuration field `{ignored_path}` in -c/--config override"), - )); - } - - if let Some(ignored_path) = unknown_feature_toml_value_field(cli_overrides_layer) { + let ignored_path = ignored_toml_value_fields::(cli_overrides_layer.clone()) + .into_iter() + .chain(unknown_tui_toml_value_path(cli_overrides_layer)) + .next() + .map(|path| path.join(".")) + .or_else(|| unknown_feature_toml_value_field(cli_overrides_layer)); + if let Some(ignored_path) = ignored_path { return Err(io::Error::new( io::ErrorKind::InvalidData, format!("unknown configuration field `{ignored_path}` in -c/--config override"), diff --git a/codex-rs/config/src/strict_config.rs b/codex-rs/config/src/strict_config.rs index 038adbf988..b8e8f8d88d 100644 --- a/codex-rs/config/src/strict_config.rs +++ b/codex-rs/config/src/strict_config.rs @@ -14,15 +14,17 @@ use crate::diagnostics::span_for_toml_key_path; use crate::diagnostics::text_range_from_span; use crate::format_config_layer_source; use crate::requirements_layers::strip_cloud_auth_requirements; +use crate::types::Tui; use codex_features::is_known_feature_key; use codex_utils_absolute_path::AbsolutePathBufGuard; use serde::de::DeserializeOwned; +use std::any::TypeId; use std::collections::BTreeSet; use std::fmt::Write; use std::path::Path; use toml::Value as TomlValue; -pub fn config_error_from_ignored_toml_fields( +pub fn config_error_from_ignored_toml_fields( path: impl AsRef, contents: &str, ) -> Option { @@ -35,7 +37,7 @@ pub fn config_error_from_ignored_toml_fields( } } -pub(crate) fn config_error_from_ignored_toml_value_fields( +pub(crate) fn config_error_from_ignored_toml_value_fields( path: impl AsRef, contents: &str, value: TomlValue, @@ -47,7 +49,9 @@ pub(crate) fn config_error_from_ignored_toml_value_fields( ) } -pub(crate) fn config_error_from_ignored_toml_value_fields_for_source_name( +pub(crate) fn config_error_from_ignored_toml_value_fields_for_source_name< + T: DeserializeOwned + 'static, +>( source_name: &str, contents: &str, value: TomlValue, @@ -59,12 +63,17 @@ pub(crate) fn config_error_from_ignored_toml_value_fields_for_source_name( +fn config_error_from_ignored_toml_value_fields_for_source( source: ConfigDiagnosticSource<'_>, contents: &str, value: TomlValue, ) -> Option { let unknown_feature_paths = unknown_feature_toml_value_path(&value); + let unknown_tui_paths = if TypeId::of::() == TypeId::of::() { + unknown_tui_toml_value_path(&value) + } else { + Vec::new() + }; let mut ignored_paths = Vec::new(); let mut ignored_callback = |ignored_path: serde_ignored::Path<'_>| { let path_segments = ignored_path_segments(&ignored_path); @@ -77,6 +86,7 @@ fn config_error_from_ignored_toml_value_fields_for_source( match result { Ok(_) => unknown_field_error_from_paths(source, contents, ignored_paths) + .or_else(|| unknown_field_error_from_paths(source, contents, unknown_tui_paths)) .or_else(|| unknown_field_error_from_paths(source, contents, unknown_feature_paths)), Err(err) => { let path_hint = err.path().clone(); @@ -94,6 +104,29 @@ fn config_error_from_ignored_toml_value_fields_for_source( } } +pub(crate) fn unknown_tui_toml_value_path(value: &TomlValue) -> Vec> { + // `serde_ignored` cannot observe keys consumed by the flattened notification settings. + // Round-trip the TUI config so accepted input keys stay derived from `Tui` itself. + let Some(tui_value) = value.as_table().and_then(|root| root.get("tui")) else { + return Vec::new(); + }; + let Some(configured_tui) = tui_value.as_table() else { + return Vec::new(); + }; + let Ok(tui) = tui_value.clone().try_into::() else { + return Vec::new(); + }; + let Ok(TomlValue::Table(serialized_tui)) = TomlValue::try_from(tui) else { + return Vec::new(); + }; + + configured_tui + .keys() + .filter(|key| !serialized_tui.contains_key(*key)) + .map(|key| vec!["tui".to_string(), key.clone()]) + .collect() +} + pub(crate) fn ignored_toml_value_fields(value: TomlValue) -> Vec> { let mut ignored_paths = Vec::new(); let result: Result = serde_ignored::deserialize(value, |ignored_path| { diff --git a/codex-rs/config/src/strict_config_tests.rs b/codex-rs/config/src/strict_config_tests.rs index 4587ac0217..22a10b48ef 100644 --- a/codex-rs/config/src/strict_config_tests.rs +++ b/codex-rs/config/src/strict_config_tests.rs @@ -6,6 +6,7 @@ use crate::config_toml::ConfigToml; use crate::diagnostics::TextPosition; use crate::diagnostics::TextRange; use pretty_assertions::assert_eq; +use std::collections::BTreeMap; use std::path::PathBuf; #[test] @@ -91,6 +92,82 @@ foo = true"#; ); } +#[test] +fn strict_config_rejects_unknown_tui_key() { + let path = Path::new("/tmp/config.toml"); + let contents = r#" +[tui] +bogus_key_xyz = 1"#; + + let error = config_error_from_ignored_toml_fields::(path, contents) + .expect("unknown TUI key error"); + + assert_eq!( + error, + ConfigError::new( + path.to_path_buf(), + TextRange { + start: TextPosition { line: 3, column: 1 }, + end: TextPosition { + line: 3, + column: 13, + }, + }, + "unknown configuration field `tui.bogus_key_xyz`", + ) + ); +} + +#[test] +fn strict_config_accepts_tui_notification_settings() { + let path = Path::new("/tmp/config.toml"); + + for contents in [ + "[tui]\nnotifications = false\n", + "[tui]\nnotification_method = \"bel\"\n", + "[tui]\nnotification_condition = \"always\"\n", + ] { + assert_eq!( + config_error_from_ignored_toml_fields::(path, contents), + None + ); + } +} + +#[test] +fn generic_validation_does_not_apply_config_toml_tui_fields() { + let path = Path::new("/tmp/config.toml"); + let contents = "[tui]\nplugin_key = 1\n"; + + assert_eq!( + config_error_from_ignored_toml_fields::>>( + path, contents, + ), + None + ); +} + +#[test] +fn wrapped_config_toml_still_rejects_unknown_feature_key() { + let path = Path::new("/tmp/config.toml"); + let contents = "[features]\nfoo = true\n"; + + let error = config_error_from_ignored_toml_fields::>(path, contents) + .expect("unknown feature error"); + + assert_eq!( + error, + ConfigError::new( + path.to_path_buf(), + TextRange { + start: TextPosition { line: 2, column: 1 }, + end: TextPosition { line: 2, column: 3 }, + }, + "unknown configuration field `features.foo`", + ) + ); +} + #[test] fn strict_config_accepts_tool_registry_config() { let path = Path::new("/tmp/config.toml");