From e53c444964b02aef5d72dfde0a31470f43456f3e Mon Sep 17 00:00:00 2001 From: andrewgu-oai Date: Fri, 11 Sep 2026 00:43:02 +0000 Subject: [PATCH] Warn about ignored configuration settings (#44691) ## Why Unrecognized settings can be silently ignored, leaving typos or deprecated configuration unnoticed. ## What changed - Collect warnings for unrecognized fields in effective configuration and requirements layers, including unknown feature keys in configuration. - Report sources and field names without configuration values, show up to three entries, and provide migration hints for `network_proxy`, `allowed_permissions`, and `include_view_image_tool`. - Emit project-specific warnings when starting a thread and suppress repeated startup warnings in the app server and TUI. ## Testing Add coverage for merged configuration, disabled layers, startup and project warning delivery, omission of configuration values, and warning deduplication. GitOrigin-RevId: 464cef87882502b19865ae10b50b6f82defb7296 --- .../request_processors/thread_processor.rs | 15 ++- .../app-server/tests/suite/strict_config.rs | 71 +++++++++- codex-rs/config/src/loader/mod.rs | 24 ++-- .../config/src/requirements_layers/layer.rs | 14 +- .../config/src/requirements_layers/mod.rs | 1 + codex-rs/config/src/strict_config.rs | 122 +++++++++++++++++- codex-rs/config/src/strict_config_tests.rs | 51 ++++++++ .../core/src/config/config_loader_tests.rs | 14 +- codex-rs/tui/src/chatwidget/protocol.rs | 3 + .../tui/src/chatwidget/tests/app_server.rs | 27 ++-- codex-rs/tui/src/chatwidget/warnings.rs | 7 +- 11 files changed, 305 insertions(+), 44 deletions(-) diff --git a/codex-rs/app-server/src/request_processors/thread_processor.rs b/codex-rs/app-server/src/request_processors/thread_processor.rs index 172177cdac..4a5f226a87 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor.rs @@ -1424,10 +1424,23 @@ impl ThreadRequestProcessor { .map_err(|err| config_load_error(&err))?; } + // Thread config can include project-local warnings absent at initialization. + let mut config_warnings = config + .startup_warnings + .iter() + .map(|summary| ConfigWarningNotification { + summary: summary.clone(), + details: None, + path: None, + range: None, + }) + .collect::>(); if let Ok(Some(err)) = codex_core::check_execpolicy_for_warnings(&config.config_layer_stack).await { - let notification = crate::exec_policy_config_warning(&err); + config_warnings.push(crate::exec_policy_config_warning(&err)); + } + for notification in config_warnings { if !initial_config_warnings.contains(¬ification) { listener_task_context .outgoing diff --git a/codex-rs/app-server/tests/suite/strict_config.rs b/codex-rs/app-server/tests/suite/strict_config.rs index d9fbdfd1d4..6adee1d6cb 100644 --- a/codex-rs/app-server/tests/suite/strict_config.rs +++ b/codex-rs/app-server/tests/suite/strict_config.rs @@ -1,7 +1,76 @@ -use std::process::Command; +//! Config errors and startup/project warnings through the public API. use anyhow::Result; +use app_test_support::TestAppServer; +use codex_app_server_protocol::ConfigWarningNotification; +use codex_app_server_protocol::ThreadStartParams; +use codex_core::config::set_project_trust_level; +use codex_protocol::config_types::TrustLevel; +use pretty_assertions::assert_ne; +use std::process::Command; +use std::time::Duration; use tempfile::TempDir; +use tokio::time::timeout; + +#[tokio::test] +async fn ignored_config_fields_emit_startup_and_project_warnings() -> Result<()> { + let home = TempDir::new()?; + std::fs::write( + home.path().join("config.toml"), + "network_proxy = { nested = 'private_value' }", + )?; + std::fs::write( + home.path().join("requirements.toml"), + "allowed_permissions = [':read-only']", + )?; + let read_timeout = Duration::from_secs(30); + let mut server = TestAppServer::builder() + .with_codex_home(home.path()) + .build_initialized_with_timeout(read_timeout) + .await?; + let startup_warning: ConfigWarningNotification = + timeout(read_timeout, server.read_notification("configWarning")).await??; + let summary = &startup_warning.summary; + assert!(summary.contains("`network_proxy` is ignored.")); + assert!(summary.contains("`allowed_permissions` is ignored.")); + assert!(!summary.contains("private_value")); + + server.start_thread(ThreadStartParams::default()).await?; + // Config warnings are sent before the thread/start response. Other startup + // warnings (e.g. missing bubblewrap on Linux) may still be buffered here. + while server + .pending_notification_methods() + .iter() + .any(|method| method == "configWarning") + { + let warning: ConfigWarningNotification = + timeout(read_timeout, server.read_notification("configWarning")).await??; + assert_ne!( + warning, startup_warning, + "thread/start repeated the startup warning" + ); + } + + let project = TempDir::new()?; + std::fs::create_dir(project.path().join(".git"))?; + std::fs::create_dir(project.path().join(".codex"))?; + std::fs::write( + project.path().join(".codex/config.toml"), + "project_setting = 'private_value'", + )?; + set_project_trust_level(home.path(), project.path(), TrustLevel::Trusted)?; + server + .start_thread(ThreadStartParams { + cwd: Some(project.path().display().to_string()), + ..Default::default() + }) + .await?; + let warning: ConfigWarningNotification = + timeout(read_timeout, server.read_notification("configWarning")).await??; + assert!(warning.summary.contains("`project_setting` is ignored.")); + assert!(!warning.summary.contains("private_value")); + Ok(()) +} #[test] fn strict_config_rejects_unknown_config_fields_for_standalone_app_server() -> Result<()> { diff --git a/codex-rs/config/src/loader/mod.rs b/codex-rs/config/src/loader/mod.rs index 82dbcc3d59..cf2ea1e603 100644 --- a/codex-rs/config/src/loader/mod.rs +++ b/codex-rs/config/src/loader/mod.rs @@ -34,7 +34,8 @@ use crate::state::ConfigLoadOptions; use crate::state::LoaderOverrides; use crate::state::validate_enabled_config_layers; use crate::strict_config::config_error_from_ignored_toml_value_fields; -use crate::strict_config::ignored_toml_value_field; +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::thread_config::ThreadConfigContext; use crate::thread_config::ThreadConfigLoader; @@ -242,8 +243,7 @@ pub async fn load_config_layers_state( let loaded_config_layers = layer_io::load_config_layers_internal(fs, codex_home, overrides.clone(), strict_config) .await?; - let mut startup_warnings = (!loaded_config_layers.startup_warnings.is_empty()) - .then(|| loaded_config_layers.startup_warnings.clone()); + let mut startup_warnings = loaded_config_layers.startup_warnings.clone(); if !ignore_managed_requirements { requirements_layers.extend(system_requirements_layer); requirements_layers.extend(bundle_requirements_layers); @@ -257,7 +257,7 @@ pub async fn load_config_layers_state( } let mut config_requirements_toml = - compose_requirements(requirements_layers)?.unwrap_or_default(); + compose_requirements(requirements_layers.clone())?.unwrap_or_default(); // Remote app servers enforce auth policy for their workspaces; do not let local // requirements reintroduce authentication restrictions for those workspaces. if overrides.ignore_login_requirements { @@ -436,9 +436,7 @@ pub async fn load_config_layers_state( ) .await?; layers.extend(project_layers.layers); - startup_warnings - .get_or_insert_with(Vec::new) - .extend(project_layers.startup_warnings); + startup_warnings.extend(project_layers.startup_warnings); } // Add a layer for runtime overrides from the CLI or UI, if any exist. @@ -521,10 +519,11 @@ pub async fn load_config_layers_state( config_requirements_toml.into_toml(), )? .with_user_and_project_exec_policy_rules_ignored(ignore_user_and_project_exec_policy_rules); - Ok(match startup_warnings { - Some(startup_warnings) => config_layer_stack.with_startup_warnings(startup_warnings), - None => config_layer_stack, - }) + startup_warnings.extend(ignored_config_warning( + &config_layer_stack, + &requirements_layers, + )); + Ok(config_layer_stack.with_startup_warnings(startup_warnings)) } async fn load_user_config_layer( @@ -676,8 +675,9 @@ fn validate_cli_overrides_strictly( base_dir: &Path, ) -> io::Result<()> { let _guard = AbsolutePathBufGuard::new(base_dir); - if let Some(ignored_path) = ignored_toml_value_field::(cli_overrides_layer.clone()) + 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"), diff --git a/codex-rs/config/src/requirements_layers/layer.rs b/codex-rs/config/src/requirements_layers/layer.rs index 57baaf50a8..6a4e1bed57 100644 --- a/codex-rs/config/src/requirements_layers/layer.rs +++ b/codex-rs/config/src/requirements_layers/layer.rs @@ -102,11 +102,7 @@ impl ComposableRequirementsLayer { let mut regular_toml = parse_layer_toml(&toml, &source)?; // These fields can only be set locally; ignore them before validating cloud policy. - if matches!(source, RequirementSource::EnterpriseManaged { .. }) { - for field in LOCAL_ONLY_AUTH_REQUIREMENTS { - remove_top_level_field(&mut regular_toml, field); - } - } + strip_cloud_auth_requirements(&source, &mut regular_toml); // Provider fragments can be incomplete until all requirements layers // are merged. Resolve explicit paths while their source base is still @@ -278,6 +274,14 @@ fn strip_special_fields(layer_toml: &mut TomlValue) { remove_nested_field_and_prune_empty(layer_toml, &["auto_review", "required_on_models"]); } +pub(crate) fn strip_cloud_auth_requirements(source: &RequirementSource, value: &mut TomlValue) { + if matches!(source, RequirementSource::EnterpriseManaged { .. }) { + for field in LOCAL_ONLY_AUTH_REQUIREMENTS { + remove_top_level_field(value, field); + } + } +} + fn remove_top_level_field(value: &mut TomlValue, key: &str) -> Option { value.as_table_mut()?.remove(key) } diff --git a/codex-rs/config/src/requirements_layers/mod.rs b/codex-rs/config/src/requirements_layers/mod.rs index 8138594682..569b1d2c12 100644 --- a/codex-rs/config/src/requirements_layers/mod.rs +++ b/codex-rs/config/src/requirements_layers/mod.rs @@ -6,5 +6,6 @@ mod rules; mod stack; pub use layer::RequirementsLayerEntry; +pub(crate) use layer::strip_cloud_auth_requirements; pub use stack::compose_requirements; pub use stack::compose_requirements_for_hostname; diff --git a/codex-rs/config/src/strict_config.rs b/codex-rs/config/src/strict_config.rs index c1523c706c..93d86973b3 100644 --- a/codex-rs/config/src/strict_config.rs +++ b/codex-rs/config/src/strict_config.rs @@ -1,5 +1,10 @@ //! Strict config validation built on top of serde's ignored-field tracking. +use crate::CONFIG_TOML_FILE; +use crate::ConfigLayerStack; +use crate::ConfigRequirementsToml; +use crate::RequirementsLayerEntry; +use crate::config_toml::ConfigToml; use crate::diagnostics::ConfigDiagnosticSource; use crate::diagnostics::ConfigError; use crate::diagnostics::config_error_from_toml_for_source; @@ -7,8 +12,13 @@ use crate::diagnostics::default_range; use crate::diagnostics::span_for_config_path; 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 codex_features::is_known_feature_key; +use codex_utils_absolute_path::AbsolutePathBufGuard; use serde::de::DeserializeOwned; +use std::collections::BTreeSet; +use std::fmt::Write; use std::path::Path; use toml::Value as TomlValue; @@ -84,7 +94,7 @@ fn config_error_from_ignored_toml_value_fields_for_source( } } -pub(crate) fn ignored_toml_value_field(value: TomlValue) -> Option { +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| { let path_segments = ignored_path_segments(&ignored_path); @@ -93,13 +103,10 @@ pub(crate) fn ignored_toml_value_field(value: TomlValue) -> } }); if result.is_err() { - return None; + return Vec::new(); } ignored_paths - .into_iter() - .next() - .map(|path_segments| path_segments.join(".")) } pub(crate) fn unknown_feature_toml_value_field(value: &TomlValue) -> Option { @@ -195,6 +202,111 @@ fn push_ignored_path_segments(path: &serde_ignored::Path<'_>, segments: &mut Vec } } +pub(crate) fn ignored_config_warning( + config: &ConfigLayerStack, + requirements: &[RequirementsLayerEntry], +) -> Option { + let mut fields = BTreeSet::new(); + // Validate the merged config so incomplete layer fragments (e.g. a provider + // override) do not hide diagnostics. + let effective = config.effective_config(); + let unknown_features = unknown_feature_toml_value_path(&effective); + for path in ignored_toml_value_fields::(effective) + .into_iter() + .chain(unknown_features) + { + // Origins only track leaf values; an ignored field can be a whole table. + for layer in config.layers_high_to_low().filter(|layer| { + path.iter() + .try_fold(&layer.config, |value, key| match value { + toml::Value::Array(items) => { + key.parse::().ok().and_then(|i| items.get(i)) + } + _ => value.get(key), + }) + .is_some() + }) { + fields.insert(( + format_config_layer_source(&layer.name, CONFIG_TOML_FILE), + path.clone(), + )); + } + } + + // Requirements use a different schema and lose unknown fields during + // composition. Inspect their original layers, with the same path bases. + // Normal loading remains responsible for reporting parse/type errors. + for layer in requirements { + let Ok((source, mut value, base_dir)) = layer.clone().into_raw_parts() else { + continue; + }; + let _guard = base_dir + .as_ref() + .map(|base_dir| AbsolutePathBufGuard::new(base_dir.as_path())); + strip_cloud_auth_requirements(&source, &mut value); + fields.extend( + ignored_toml_value_fields::(value) + .into_iter() + .map(|path| (format!("requirements ({source})"), path)), + ); + } + + if fields.is_empty() { + return None; + } + let count = fields.len(); + let setting = if count == 1 { "setting" } else { "settings" }; + let mut warning = format!( + "Codex is ignoring {count} unrecognized configuration {setting}. Check for typos or deprecated settings." + ); + for (source, path) in fields.iter().take(3) { + let source = bounded_label(source); + let key = bounded_label(&path.join(".")); + let hint = match path + .iter() + .map(String::as_str) + .collect::>() + .as_slice() + { + ["network_proxy"] => { + " Use [permissions..network] for network settings, or [experimental_network] in requirements.toml for enforced network policy." + } + ["allowed_permissions"] => { + " Use [allowed_permission_profiles] and default_permissions; the old allowlist is ignored even when both forms are present." + } + ["include_view_image_tool"] + | ["features", "include_view_image_tool"] + | ["profiles", _, "include_view_image_tool"] + | ["profiles", _, "features", "include_view_image_tool"] => { + " Use [features].view_image to configure the image tool." + } + _ => "", + }; + let _ = write!(warning, "\n {source}: `{key}` is ignored.{hint}"); + } + if fields.len() > 3 { + let remaining = fields.len() - 3; + let _ = write!(warning, "\n ... and {remaining} more ignored settings."); + } + Some(warning) +} + +fn bounded_label(value: &str) -> String { + let mut label = String::new(); + for character in value.chars() { + if label.len() >= 200 { + label.push('…'); + break; + } + if character.is_control() { + label.extend(character.escape_default()); + } else { + label.push(character); + } + } + label +} + #[cfg(test)] #[path = "strict_config_tests.rs"] mod tests; diff --git a/codex-rs/config/src/strict_config_tests.rs b/codex-rs/config/src/strict_config_tests.rs index e16270c4af..0463848db2 100644 --- a/codex-rs/config/src/strict_config_tests.rs +++ b/codex-rs/config/src/strict_config_tests.rs @@ -1,4 +1,7 @@ use super::*; +use crate::ConfigLayerEntry; +use crate::ConfigLayerSource; +use crate::ConfigRequirements; use crate::config_toml::ConfigToml; use crate::diagnostics::TextPosition; use crate::diagnostics::TextRange; @@ -150,3 +153,51 @@ collapsed = true"#; assert_eq!(error, None); } + +fn layer(source: ConfigLayerSource, contents: &str) -> ConfigLayerEntry { + ConfigLayerEntry::new(source, toml::from_str(contents).unwrap()) +} + +#[test] +fn checks_merged_config_and_ignores_disabled_layers() { + let layers = vec![ + layer( + ConfigLayerSource::EnterpriseManaged { + id: "cfg".into(), + name: "Defaults".into(), + }, + r#" +[model_providers.custom] +name = "Custom" +wire_api = "responses" +unknown_timeout = 1 +[model_providers.custom.http_headers] +custom_header = "accepted" +"#, + ), + ConfigLayerEntry::new_disabled( + ConfigLayerSource::SessionFlags, + toml::from_str("disabled_unknown = true").unwrap(), + "not trusted", + ), + layer( + ConfigLayerSource::SessionFlags, + r#" +[model_providers.custom] +unknown_timeout = 42 +[profiles.work.features] +include_view_image_tool = false +"#, + ), + ]; + let config = ConfigLayerStack::new( + layers, + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .unwrap(); + assert_eq!( + ignored_config_warning(&config, &[]).unwrap(), + "Codex is ignoring 3 unrecognized configuration settings. Check for typos or deprecated settings.\n enterprise-managed (Defaults, cfg): `model_providers.custom.unknown_timeout` is ignored.\n session-flags: `model_providers.custom.unknown_timeout` is ignored.\n session-flags: `profiles.work.features.include_view_image_tool` is ignored. Use [features].view_image to configure the image tool." + ); +} diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index f09c2f6cc2..49d4b27105 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -3727,7 +3727,7 @@ async fn project_layers_disabled_when_untrusted_or_unknown() -> std::io::Result< tokio::fs::create_dir_all(nested.join(".codex")).await?; tokio::fs::write( nested.join(".codex").join(CONFIG_TOML_FILE), - r#"foo = "child" + r#"model = "child" profile = "ignored" "#, ) @@ -3749,7 +3749,7 @@ profile = "ignored" tokio::fs::write( &untrusted_config_path, format!( - r#"foo = "user" + r#"model = "user" {untrusted_config_contents}"# ), ) @@ -3774,7 +3774,7 @@ profile = "ignored" "expected untrusted project layer to be disabled" ); assert_eq!( - project_layers_untrusted[0].config.get("foo"), + project_layers_untrusted[0].config.get("model"), Some(&TomlValue::String("child".to_string())) ); assert!( @@ -3782,7 +3782,7 @@ profile = "ignored" "expected unsupported project config keys to be ignored even when the layer is disabled" ); assert_eq!( - layers_untrusted.effective_config().get("foo"), + layers_untrusted.effective_config().get("model"), Some(&TomlValue::String("user".to_string())) ); let empty_warnings: &[String] = &[]; @@ -3792,7 +3792,7 @@ profile = "ignored" tokio::fs::create_dir_all(&codex_home_unknown).await?; tokio::fs::write( codex_home_unknown.join(CONFIG_TOML_FILE), - r#"foo = "user" + r#"model = "user" "#, ) .await?; @@ -3816,7 +3816,7 @@ profile = "ignored" "expected unknown-trust project layer to be disabled" ); assert_eq!( - project_layers_unknown[0].config.get("foo"), + project_layers_unknown[0].config.get("model"), Some(&TomlValue::String("child".to_string())) ); assert!( @@ -3824,7 +3824,7 @@ profile = "ignored" "expected unsupported project config keys to be ignored even when the layer is disabled" ); assert_eq!( - layers_unknown.effective_config().get("foo"), + layers_unknown.effective_config().get("model"), Some(&TomlValue::String("user".to_string())) ); assert_eq!(layers_unknown.startup_warnings(), Some(empty_warnings)); diff --git a/codex-rs/tui/src/chatwidget/protocol.rs b/codex-rs/tui/src/chatwidget/protocol.rs index 9ec441ad2e..10dd1bd4e0 100644 --- a/codex-rs/tui/src/chatwidget/protocol.rs +++ b/codex-rs/tui/src/chatwidget/protocol.rs @@ -241,6 +241,9 @@ impl ChatWidget { if self.warning_display_state.startup_complete { self.on_warning(message); } else if self.warning_display_state.should_display(&message) { + self.warning_display_state + .startup_config_warnings + .insert(message.clone()); self.add_to_history(history_cell::StartupWarningsCell::new(vec![message])); self.request_redraw(); } diff --git a/codex-rs/tui/src/chatwidget/tests/app_server.rs b/codex-rs/tui/src/chatwidget/tests/app_server.rs index f6d9e6d96a..c6ad08d768 100644 --- a/codex-rs/tui/src/chatwidget/tests/app_server.rs +++ b/codex-rs/tui/src/chatwidget/tests/app_server.rs @@ -1008,26 +1008,31 @@ async fn config_warning_during_turn_remains_inline() { } #[tokio::test] -async fn live_app_server_config_warning_prefixes_summary() { +async fn startup_config_warning_is_not_repeated_by_thread() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + let message = "Codex is ignoring 1 unrecognized configuration setting. Check for typos or deprecated settings."; - chat.handle_server_notification( + for notification in [ ServerNotification::ConfigWarning(ConfigWarningNotification { - summary: "Invalid configuration; using defaults.".to_string(), + summary: message.to_string(), details: None, path: None, range: None, }), - /*replay_kind*/ None, - ); + ServerNotification::Warning(WarningNotification { + thread_id: Some("thread-1".to_string()), + message: message.to_string(), + }), + ] { + chat.handle_server_notification(notification, /*replay_kind*/ None); + } let cells = drain_insert_history_transcript(&mut rx); - assert_eq!(cells.len(), 1, "expected one warning history cell"); - let rendered = lines_to_single_string(&cells[0]); - assert!( - rendered.contains("Invalid configuration; using defaults."), - "expected config warning summary, got {rendered}" - ); + assert_eq!(cells.len(), 1); + insta::assert_snapshot!(lines_to_single_string(&cells[0]), @" + ⚠ Codex is ignoring 1 unrecognized configuration setting. Check for typos or + deprecated settings. + "); } #[tokio::test] diff --git a/codex-rs/tui/src/chatwidget/warnings.rs b/codex-rs/tui/src/chatwidget/warnings.rs index ea82a51eae..22a32946c1 100644 --- a/codex-rs/tui/src/chatwidget/warnings.rs +++ b/codex-rs/tui/src/chatwidget/warnings.rs @@ -8,13 +8,16 @@ const FALLBACK_MODEL_METADATA_WARNING_SUFFIX: &str = pub(super) struct WarningDisplayState { /// Completed resume history does not end startup; active work does. pub(super) startup_complete: bool, + /// Initialization config warnings may also arrive as ordinary thread warnings. + pub(super) startup_config_warnings: HashSet, fallback_model_metadata_slugs: HashSet, } impl WarningDisplayState { pub(super) fn should_display(&mut self, message: &str) -> bool { - fallback_model_metadata_warning_slug(message) - .is_none_or(|slug| self.fallback_model_metadata_slugs.insert(slug.to_string())) + !self.startup_config_warnings.contains(message) + && fallback_model_metadata_warning_slug(message) + .is_none_or(|slug| self.fallback_model_metadata_slugs.insert(slug.to_string())) } }