From bf4737b520d9b320b79520a2afe7e9ee81e0ec4b Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Thu, 7 May 2026 02:30:57 +0300 Subject: [PATCH] Use spec loop for config enum warnings --- codex-rs/config/src/lenient.rs | 486 ++++++++++++++++++--------------- 1 file changed, 266 insertions(+), 220 deletions(-) diff --git a/codex-rs/config/src/lenient.rs b/codex-rs/config/src/lenient.rs index 5cc096f709..c115148fb4 100644 --- a/codex-rs/config/src/lenient.rs +++ b/codex-rs/config/src/lenient.rs @@ -31,19 +31,226 @@ use codex_protocol::protocol::AskForApproval; use serde::de::DeserializeOwned; use toml::Value as TomlValue; -const SHELL_ENVIRONMENT_POLICY: &str = "shell_environment_policy"; -const CONTEXT_SIZE: &str = "context_size"; -const DEFAULT_TOOLS_APPROVAL_MODE: &str = "default_tools_approval_mode"; -const APPROVAL_MODE: &str = "approval_mode"; +use self::PathSegment::AnyTable; +use self::PathSegment::Key; -macro_rules! check_fields { - ($value:expr, $path:expr, $warnings:expr, $( $key:expr => $ty:ty ),+ $(,)?) => { - $( - check_value::<$ty>($value, &child_path($path, $key), $warnings); - )+ - }; +type FieldParser = fn(&TomlValue) -> bool; +type EnumFieldSpec = (&'static [PathSegment], FieldParser); + +/// One segment in a known enum-valued config path. +enum PathSegment { + /// Match this literal table key. + Key(&'static str), + /// Expand across every table child at this point, such as `profiles.*`. + AnyTable, } +static ENUM_FIELD_SPECS: &[EnumFieldSpec] = &[ + (&[Key("approval_policy")], parses_as::), + (&[Key("approvals_reviewer")], parses_as::), + (&[Key("sandbox_mode")], parses_as::), + ( + &[Key("forced_login_method")], + parses_as::, + ), + ( + &[Key("cli_auth_credentials_store")], + parses_as::, + ), + ( + &[Key("mcp_oauth_credentials_store")], + parses_as::, + ), + (&[Key("file_opener")], parses_as::), + ( + &[Key("model_reasoning_effort")], + parses_as::, + ), + ( + &[Key("plan_mode_reasoning_effort")], + parses_as::, + ), + ( + &[Key("model_reasoning_summary")], + parses_as::, + ), + (&[Key("model_verbosity")], parses_as::), + (&[Key("personality")], parses_as::), + (&[Key("service_tier")], parses_as::), + ( + &[Key("experimental_thread_store")], + parses_as::, + ), + (&[Key("web_search")], parses_as::), + ( + &[Key("shell_environment_policy"), Key("inherit")], + parses_as::, + ), + ( + &[Key("history"), Key("persistence")], + parses_as::, + ), + ( + &[Key("tui"), Key("notifications")], + parses_as::, + ), + ( + &[Key("tui"), Key("notification_method")], + parses_as::, + ), + ( + &[Key("tui"), Key("notification_condition")], + parses_as::, + ), + ( + &[Key("tui"), Key("alternate_screen")], + parses_as::, + ), + ( + &[Key("tui"), Key("session_picker_view")], + parses_as::, + ), + ( + &[Key("realtime"), Key("version")], + parses_as::, + ), + (&[Key("realtime"), Key("type")], parses_as::), + ( + &[Key("realtime"), Key("transport")], + parses_as::, + ), + (&[Key("realtime"), Key("voice")], parses_as::), + ( + &[Key("tools"), Key("web_search"), Key("context_size")], + parses_as::, + ), + ( + &[Key("windows"), Key("sandbox")], + parses_as::, + ), + ( + &[Key("profiles"), AnyTable, Key("service_tier")], + parses_as::, + ), + ( + &[Key("profiles"), AnyTable, Key("approval_policy")], + parses_as::, + ), + ( + &[Key("profiles"), AnyTable, Key("approvals_reviewer")], + parses_as::, + ), + ( + &[Key("profiles"), AnyTable, Key("sandbox_mode")], + parses_as::, + ), + ( + &[Key("profiles"), AnyTable, Key("model_reasoning_effort")], + parses_as::, + ), + ( + &[Key("profiles"), AnyTable, Key("plan_mode_reasoning_effort")], + parses_as::, + ), + ( + &[Key("profiles"), AnyTable, Key("model_reasoning_summary")], + parses_as::, + ), + ( + &[Key("profiles"), AnyTable, Key("model_verbosity")], + parses_as::, + ), + ( + &[Key("profiles"), AnyTable, Key("personality")], + parses_as::, + ), + ( + &[Key("profiles"), AnyTable, Key("web_search")], + parses_as::, + ), + ( + &[ + Key("profiles"), + AnyTable, + Key("tools"), + Key("web_search"), + Key("context_size"), + ], + parses_as::, + ), + ( + &[Key("profiles"), AnyTable, Key("windows"), Key("sandbox")], + parses_as::, + ), + ( + &[ + Key("profiles"), + AnyTable, + Key("tui"), + Key("session_picker_view"), + ], + parses_as::, + ), + ( + &[ + Key("mcp_servers"), + AnyTable, + Key("default_tools_approval_mode"), + ], + parses_as::, + ), + ( + &[ + Key("mcp_servers"), + AnyTable, + Key("tools"), + AnyTable, + Key("approval_mode"), + ], + parses_as::, + ), + ( + &[Key("apps"), AnyTable, Key("default_tools_approval_mode")], + parses_as::, + ), + ( + &[ + Key("apps"), + AnyTable, + Key("tools"), + AnyTable, + Key("approval_mode"), + ], + parses_as::, + ), + ( + &[ + Key("plugins"), + AnyTable, + Key("mcp_servers"), + AnyTable, + Key("default_tools_approval_mode"), + ], + parses_as::, + ), + ( + &[ + Key("plugins"), + AnyTable, + Key("mcp_servers"), + AnyTable, + Key("tools"), + AnyTable, + Key("approval_mode"), + ], + parses_as::, + ), + ( + &[Key("marketplaces"), AnyTable, Key("source_type")], + parses_as::, + ), +]; + /// Deserializes config while turning known invalid enum-valued settings into warnings. /// /// The config structs use `serde_with::DefaultOnError` on their enum fields so @@ -55,211 +262,34 @@ pub(crate) fn deserialize_with_enum_warnings( value: TomlValue, ) -> Result<(TomlValue, ConfigToml, Vec), toml::de::Error> { let mut sanitized = value; - let warnings = remove_invalid_enum_values(&mut sanitized); + let mut warnings = Vec::new(); + for (spec_path, parser) in ENUM_FIELD_SPECS { + for path in matching_paths(&sanitized, spec_path) { + remove_if_invalid(&mut sanitized, &path, *parser, &mut warnings); + } + } let parsed = sanitized.clone().try_into::()?; Ok((sanitized, parsed, warnings)) } -fn remove_invalid_enum_values(value: &mut TomlValue) -> Vec { - let mut warnings = Vec::new(); - let path = Vec::new(); - scan_config_toml(value, &path, &mut warnings); - warnings -} - -fn scan_config_toml(value: &mut TomlValue, path: &[String], warnings: &mut Vec) { - check_fields!( - value, - path, - warnings, - "approval_policy" => AskForApproval, - "approvals_reviewer" => ApprovalsReviewer, - "sandbox_mode" => SandboxMode, - "forced_login_method" => ForcedLoginMethod, - "cli_auth_credentials_store" => AuthCredentialsStoreMode, - "mcp_oauth_credentials_store" => OAuthCredentialsStoreMode, - "file_opener" => UriBasedFileOpener, - "model_reasoning_effort" => ReasoningEffort, - "plan_mode_reasoning_effort" => ReasoningEffort, - "model_reasoning_summary" => ReasoningSummary, - "model_verbosity" => Verbosity, - "personality" => Personality, - "service_tier" => ServiceTier, - "experimental_thread_store" => ThreadStoreToml, - "web_search" => WebSearchMode, - ); - - scan_shell_environment_policy(value, &child_path(path, SHELL_ENVIRONMENT_POLICY), warnings); - scan_history(value, &child_path(path, "history"), warnings); - scan_tui(value, &child_path(path, "tui"), warnings); - scan_realtime(value, &child_path(path, "realtime"), warnings); - scan_tools(value, &child_path(path, "tools"), warnings); - scan_windows(value, &child_path(path, "windows"), warnings); - scan_profiles(value, &child_path(path, "profiles"), warnings); - scan_mcp_servers(value, &child_path(path, "mcp_servers"), warnings); - scan_apps(value, &child_path(path, "apps"), warnings); - scan_plugins(value, &child_path(path, "plugins"), warnings); - scan_marketplaces(value, &child_path(path, "marketplaces"), warnings); -} - -fn scan_shell_environment_policy( - value: &mut TomlValue, - path: &[String], - warnings: &mut Vec, -) { - check_fields!( - value, - path, - warnings, - "inherit" => ShellEnvironmentPolicyInherit, - ); -} - -fn scan_history(value: &mut TomlValue, path: &[String], warnings: &mut Vec) { - check_fields!( - value, - path, - warnings, - "persistence" => HistoryPersistence, - ); -} - -fn scan_tui(value: &mut TomlValue, path: &[String], warnings: &mut Vec) { - check_fields!( - value, - path, - warnings, - "notifications" => Notifications, - "notification_method" => NotificationMethod, - "notification_condition" => NotificationCondition, - "alternate_screen" => AltScreenMode, - "session_picker_view" => SessionPickerViewMode, - ); -} - -fn scan_realtime(value: &mut TomlValue, path: &[String], warnings: &mut Vec) { - check_fields!( - value, - path, - warnings, - "version" => RealtimeWsVersion, - "type" => RealtimeWsMode, - "transport" => RealtimeTransport, - "voice" => RealtimeVoice, - ); -} - -fn scan_tools(value: &mut TomlValue, path: &[String], warnings: &mut Vec) { - check_fields!( - value, - &child_path(path, "web_search"), - warnings, - CONTEXT_SIZE => WebSearchContextSize, - ); -} - -fn scan_windows(value: &mut TomlValue, path: &[String], warnings: &mut Vec) { - check_fields!( - value, - path, - warnings, - "sandbox" => WindowsSandboxModeToml, - ); -} - -fn scan_profiles(value: &mut TomlValue, path: &[String], warnings: &mut Vec) { - for profile_path in child_table_paths(value, path) { - scan_profile(value, &profile_path, warnings); - } -} - -fn scan_profile(value: &mut TomlValue, path: &[String], warnings: &mut Vec) { - check_fields!( - value, - path, - warnings, - "service_tier" => ServiceTier, - "approval_policy" => AskForApproval, - "approvals_reviewer" => ApprovalsReviewer, - "sandbox_mode" => SandboxMode, - "model_reasoning_effort" => ReasoningEffort, - "plan_mode_reasoning_effort" => ReasoningEffort, - "model_reasoning_summary" => ReasoningSummary, - "model_verbosity" => Verbosity, - "personality" => Personality, - "web_search" => WebSearchMode, - ); - scan_tools(value, &child_path(path, "tools"), warnings); - scan_windows(value, &child_path(path, "windows"), warnings); - check_fields!( - value, - &child_path(path, "tui"), - warnings, - "session_picker_view" => SessionPickerViewMode, - ); -} - -fn scan_mcp_servers(value: &mut TomlValue, path: &[String], warnings: &mut Vec) { - for server_path in child_table_paths(value, path) { - scan_tool_approval_overrides(value, &server_path, warnings); - } -} - -fn scan_apps(value: &mut TomlValue, path: &[String], warnings: &mut Vec) { - for app_path in child_table_paths(value, path) { - scan_tool_approval_overrides(value, &app_path, warnings); - } -} - -fn scan_plugins(value: &mut TomlValue, path: &[String], warnings: &mut Vec) { - for plugin_path in child_table_paths(value, path) { - for server_path in child_table_paths(value, &child_path(&plugin_path, "mcp_servers")) { - scan_tool_approval_overrides(value, &server_path, warnings); - } - } -} - -fn scan_tool_approval_overrides( - value: &mut TomlValue, - path: &[String], - warnings: &mut Vec, -) { - check_fields!( - value, - path, - warnings, - DEFAULT_TOOLS_APPROVAL_MODE => AppToolApproval, - ); - for tool_path in child_table_paths(value, &child_path(path, "tools")) { - check_fields!( - value, - &tool_path, - warnings, - APPROVAL_MODE => AppToolApproval, - ); - } -} - -fn scan_marketplaces(value: &mut TomlValue, path: &[String], warnings: &mut Vec) { - for marketplace_path in child_table_paths(value, path) { - check_fields!( - value, - &marketplace_path, - warnings, - "source_type" => MarketplaceSourceType, - ); - } -} - -/// Checks one known enum leaf by parsing the raw TOML value as the real field type. -fn check_value(value: &mut TomlValue, path: &[String], warnings: &mut Vec) +fn parses_as(value: &TomlValue) -> bool where T: DeserializeOwned, { + value.clone().try_into::().is_ok() +} + +/// Removes one known enum leaf when its raw TOML value does not parse as its real type. +fn remove_if_invalid( + value: &mut TomlValue, + path: &[String], + parser: FieldParser, + warnings: &mut Vec, +) { let Some(raw_value) = value_at_path(value, path) else { return; }; - if raw_value.clone().try_into::().is_ok() { + if parser(raw_value) { return; } let Some(invalid_value) = remove_value_at_path(value, path) else { @@ -271,15 +301,37 @@ where )); } -/// Returns table child paths in TOML map order without holding a borrow. -fn child_table_paths(value: &TomlValue, path: &[String]) -> Vec> { - let Some(table) = value_at_path(value, path).and_then(TomlValue::as_table) else { - return Vec::new(); - }; - table - .iter() - .filter_map(|(key, value)| value.is_table().then(|| child_path(path, key))) - .collect() +/// 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 { + Key(key) => { + for path in &mut paths { + path.push((*key).to_string()); + } + } + 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; + } + } + } + paths } fn value_at_path<'a>(value: &'a TomlValue, path: &[String]) -> Option<&'a TomlValue> { @@ -299,12 +351,6 @@ fn remove_value_at_path(value: &mut TomlValue, path: &[String]) -> Option Vec { - let mut child = path.to_vec(); - child.push(key.to_string()); - child -} - fn display_path(path: &[String]) -> String { path.iter() .map(|key| display_key(key))