From 5e5d0dfa0c3ecc055fa53e4b9bb46781b7c5bc1b Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Mon, 15 Jun 2026 22:52:27 +0000 Subject: [PATCH 01/12] Add keyed shell environment rules to config --- codex-rs/config/src/lib.rs | 1 + codex-rs/config/src/merge.rs | 71 +++++++++++++ codex-rs/config/src/merge_tests.rs | 140 ++++++++++++++++++++++++++ codex-rs/config/src/types.rs | 30 ++++-- codex-rs/config/src/types_tests.rs | 69 +++++++++++++ codex-rs/core/config.schema.json | 22 +++- codex-rs/protocol/src/config_types.rs | 10 ++ 7 files changed, 332 insertions(+), 11 deletions(-) diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index ed26757715f8..c2ef5ad785b3 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -74,6 +74,7 @@ pub use config_requirements::RemoteSandboxConfigToml; pub use config_requirements::RequirementSource; pub use config_requirements::ResidencyRequirement; pub use config_requirements::SandboxModeRequirement; +pub use config_requirements::ShellEnvironmentPolicyRequirementsToml; pub use config_requirements::Sourced; pub use config_requirements::WebSearchModeRequirement; pub use config_requirements::WindowsRequirementsToml; diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index 94d670154663..f2cb51034503 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -9,6 +9,8 @@ pub fn merge_toml_values(base: &mut TomlValue, overlay: &TomlValue) { } fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &mut Vec) { + prepare_shell_environment_policy_merge(base, overlay, path); + if let TomlValue::Table(overlay_table) = overlay && let TomlValue::Table(base_table) = base { @@ -34,6 +36,75 @@ fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &m } } +fn prepare_shell_environment_policy_merge( + base: &mut TomlValue, + overlay: &TomlValue, + path: &[String], +) { + if !matches!(path, [policy] if policy == "shell_environment_policy") { + return; + } + let TomlValue::Table(base) = base else { + return; + }; + let TomlValue::Table(overlay) = overlay else { + return; + }; + + // Ordinary config keeps accepting legacy arrays while `rules` is the + // canonical keyed form. Reconcile the two shapes only at this boundary so + // layer precedence remains correct and array compatibility can be removed + // cleanly after the migration. + for (legacy_field, opposite_field, action) in [ + ("exclude", "include_only", "exclude"), + ("include_only", "exclude", "include"), + ] { + let Some(patterns) = overlay + .get(legacy_field) + .and_then(TomlValue::as_array) + .and_then(|items| { + items + .iter() + .map(TomlValue::as_str) + .collect::>>() + }) + else { + continue; + }; + + if let Some(base_rules) = base.get_mut("rules").and_then(TomlValue::as_table_mut) { + base_rules.retain(|pattern, value| { + value.as_str() != Some(action) && !patterns.contains(&pattern) + }); + } + remove_patterns_from_legacy_array(base, opposite_field, &patterns); + } + + let Some(overlay_rules) = overlay.get("rules").and_then(TomlValue::as_table) else { + return; + }; + let patterns = overlay_rules.keys().map(String::as_str).collect::>(); + remove_patterns_from_legacy_array(base, "exclude", &patterns); + remove_patterns_from_legacy_array(base, "include_only", &patterns); +} + +fn remove_patterns_from_legacy_array( + table: &mut toml::map::Map, + field: &str, + patterns: &[&str], +) { + let Some(items) = table.get_mut(field).and_then(TomlValue::as_array_mut) else { + return; + }; + if !items.iter().all(|item| item.as_str().is_some()) { + return; + } + items.retain(|item| { + item.as_str() + .is_none_or(|pattern| !patterns.contains(&pattern)) + }); +} + fn is_permission_network_domains_path(path: &[String]) -> bool { matches!( path, diff --git a/codex-rs/config/src/merge_tests.rs b/codex-rs/config/src/merge_tests.rs index f9da6e7ebc14..271f6c75dcdc 100644 --- a/codex-rs/config/src/merge_tests.rs +++ b/codex-rs/config/src/merge_tests.rs @@ -1,6 +1,7 @@ use super::*; use crate::config_toml::ConfigToml; use crate::types::MemoriesToml; +use crate::types::ShellEnvironmentPolicyToml; use pretty_assertions::assert_eq; fn parse_toml(value: &str) -> TomlValue { @@ -124,3 +125,142 @@ fn merge_toml_values_normalizes_permission_network_domains_before_overlaying() { ); assert_eq!(base, expected); } + +#[test] +fn shell_environment_policy_legacy_array_overlay_replaces_legacy_array() { + let mut base = parse_toml( + r#" +[shell_environment_policy] +exclude = ["LOW_*", "SHARED_*"] +"#, + ); + let overlay = parse_toml( + r#" +[shell_environment_policy] +exclude = ["HIGH_*"] +"#, + ); + + merge_toml_values(&mut base, &overlay); + + assert_eq!(base, overlay); +} + +#[test] +fn shell_environment_policy_rules_overlay_merges_by_key() { + let mut base = parse_toml( + r#" +[shell_environment_policy.rules] +"FLIP_*" = "exclude" +"KEEP_*" = "include" +"#, + ); + let overlay = parse_toml( + r#" +[shell_environment_policy.rules] +"ADD_*" = "exclude" +"FLIP_*" = "include" +"#, + ); + + merge_toml_values(&mut base, &overlay); + + assert_eq!( + base, + parse_toml( + r#" +[shell_environment_policy.rules] +"ADD_*" = "exclude" +"FLIP_*" = "include" +"KEEP_*" = "include" +"#, + ) + ); +} + +#[test] +fn shell_environment_policy_rules_override_lower_legacy_arrays_by_pattern() { + let mut base = parse_toml( + r#" +[shell_environment_policy] +exclude = ["FLIP_TO_INCLUDE", "KEEP_EXCLUDED"] +include_only = ["FLIP_TO_EXCLUDE", "KEEP_INCLUDED"] +"#, + ); + let overlay = parse_toml( + r#" +[shell_environment_policy.rules] +"ADD_INCLUDED" = "include" +"FLIP_TO_EXCLUDE" = "exclude" +"FLIP_TO_INCLUDE" = "include" +"#, + ); + + merge_toml_values(&mut base, &overlay); + + assert_eq!( + base, + parse_toml( + r#" +[shell_environment_policy] +exclude = ["KEEP_EXCLUDED"] +include_only = ["KEEP_INCLUDED"] + +[shell_environment_policy.rules] +"ADD_INCLUDED" = "include" +"FLIP_TO_EXCLUDE" = "exclude" +"FLIP_TO_INCLUDE" = "include" +"#, + ) + ); + + let config: ConfigToml = base.try_into().expect("merged config should deserialize"); + assert_eq!( + codex_protocol::config_types::ShellEnvironmentPolicy::from(config.shell_environment_policy), + codex_protocol::config_types::ShellEnvironmentPolicy::from(ShellEnvironmentPolicyToml { + exclude: Some(vec![ + "KEEP_EXCLUDED".to_string(), + "FLIP_TO_EXCLUDE".to_string(), + ]), + include_only: Some(vec![ + "KEEP_INCLUDED".to_string(), + "ADD_INCLUDED".to_string(), + "FLIP_TO_INCLUDE".to_string(), + ]), + ..Default::default() + }) + ); +} + +#[test] +fn shell_environment_policy_legacy_array_replaces_lower_rules_for_its_action() { + let mut base = parse_toml( + r#" +[shell_environment_policy.rules] +"FLIP_TO_EXCLUDE" = "include" +"LOW_EXCLUDED" = "exclude" +"KEEP_INCLUDED" = "include" +"#, + ); + let overlay = parse_toml( + r#" +[shell_environment_policy] +exclude = ["FLIP_TO_EXCLUDE", "HIGH_EXCLUDED"] +"#, + ); + + merge_toml_values(&mut base, &overlay); + + assert_eq!( + base, + parse_toml( + r#" +[shell_environment_policy] +exclude = ["FLIP_TO_EXCLUDE", "HIGH_EXCLUDED"] + +[shell_environment_policy.rules] +"KEEP_INCLUDED" = "include" +"#, + ) + ); +} diff --git a/codex-rs/config/src/types.rs b/codex-rs/config/src/types.rs index 044d09ca85b4..319bb4e4505b 100644 --- a/codex-rs/config/src/types.rs +++ b/codex-rs/config/src/types.rs @@ -19,6 +19,7 @@ pub use codex_protocol::config_types::Personality; pub use codex_protocol::config_types::ServiceTier; use codex_protocol::config_types::ShellEnvironmentPolicy; use codex_protocol::config_types::ShellEnvironmentPolicyInherit; +use codex_protocol::config_types::ShellEnvironmentPolicyRule; pub use codex_protocol::config_types::WebSearchMode; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::BTreeMap; @@ -935,14 +936,21 @@ pub struct ShellEnvironmentPolicyToml { pub ignore_default_excludes: Option, - /// List of regular expressions. + /// Legacy list of regular expressions to exclude. pub exclude: Option>, pub r#set: Option>, - /// List of regular expressions. + /// Legacy list of regular expressions to include. pub include_only: Option>, + /// Pattern actions used by the canonical table representation. + /// + /// Ordinary config keeps accepting the legacy arrays above during the + /// migration. Requirements accept only this keyed form, keeping array + /// compatibility isolated so the legacy fields can be deprecated later. + pub rules: Option>, + pub experimental_use_profile: Option, } @@ -951,16 +959,22 @@ impl From for ShellEnvironmentPolicy { // Default to inheriting the full environment when not specified. let inherit = toml.inherit.unwrap_or(ShellEnvironmentPolicyInherit::All); let ignore_default_excludes = toml.ignore_default_excludes.unwrap_or(true); - let exclude = toml - .exclude - .unwrap_or_default() + let mut exclude = toml.exclude.unwrap_or_default(); + let mut include_only = toml.include_only.unwrap_or_default(); + for (pattern, rule) in toml.rules.unwrap_or_default() { + exclude.retain(|candidate| candidate != &pattern); + include_only.retain(|candidate| candidate != &pattern); + match rule { + ShellEnvironmentPolicyRule::Include => include_only.push(pattern), + ShellEnvironmentPolicyRule::Exclude => exclude.push(pattern), + } + } + let exclude = exclude .into_iter() .map(|s| EnvironmentVariablePattern::new_case_insensitive(&s)) .collect(); let r#set = toml.r#set.unwrap_or_default(); - let include_only = toml - .include_only - .unwrap_or_default() + let include_only = include_only .into_iter() .map(|s| EnvironmentVariablePattern::new_case_insensitive(&s)) .collect(); diff --git a/codex-rs/config/src/types_tests.rs b/codex-rs/config/src/types_tests.rs index 2c3f69d9867e..ac1b1bcb5a43 100644 --- a/codex-rs/config/src/types_tests.rs +++ b/codex-rs/config/src/types_tests.rs @@ -1,6 +1,75 @@ use super::*; use pretty_assertions::assert_eq; +#[test] +fn shell_environment_policy_accepts_legacy_lists_and_rules() { + let legacy: ShellEnvironmentPolicyToml = toml::from_str( + r#" +exclude = ["LEGACY_*", "SHARED_*"] +include_only = ["PATH", "HOME"] +"#, + ) + .expect("legacy arrays should remain valid in config.toml"); + assert_eq!( + legacy, + ShellEnvironmentPolicyToml { + exclude: Some(vec!["LEGACY_*".to_string(), "SHARED_*".to_string()]), + include_only: Some(vec!["PATH".to_string(), "HOME".to_string()]), + ..Default::default() + } + ); + + let ruled: ShellEnvironmentPolicyToml = toml::from_str( + r#" +exclude = ["KEEP_EXCLUDED", "FLIP_TO_INCLUDE"] +include_only = ["KEEP_INCLUDED", "FLIP_TO_EXCLUDE"] + +[rules] +"FLIP_TO_EXCLUDE" = "exclude" +"FLIP_TO_INCLUDE" = "include" +"#, + ) + .expect("rules should be valid in config.toml"); + assert_eq!( + ruled, + ShellEnvironmentPolicyToml { + exclude: Some(vec![ + "KEEP_EXCLUDED".to_string(), + "FLIP_TO_INCLUDE".to_string(), + ]), + include_only: Some(vec![ + "KEEP_INCLUDED".to_string(), + "FLIP_TO_EXCLUDE".to_string(), + ]), + rules: Some(BTreeMap::from([ + ( + "FLIP_TO_EXCLUDE".to_string(), + ShellEnvironmentPolicyRule::Exclude, + ), + ( + "FLIP_TO_INCLUDE".to_string(), + ShellEnvironmentPolicyRule::Include, + ), + ])), + ..Default::default() + } + ); + assert_eq!( + ShellEnvironmentPolicy::from(ruled), + ShellEnvironmentPolicy::from(ShellEnvironmentPolicyToml { + exclude: Some(vec![ + "KEEP_EXCLUDED".to_string(), + "FLIP_TO_EXCLUDE".to_string(), + ]), + include_only: Some(vec![ + "KEEP_INCLUDED".to_string(), + "FLIP_TO_INCLUDE".to_string(), + ]), + ..Default::default() + }) + ); +} + #[test] fn deserialize_skill_config_with_name_selector() { let cfg: SkillConfig = toml::from_str( diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 3d7cffdcdd49..761f2949aa89 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2620,12 +2620,20 @@ } ] }, + "ShellEnvironmentPolicyRule": { + "description": "Assigns a shell environment variable pattern to the include-only or exclude set. Includes do not re-add variables removed by another exclude pattern.", + "enum": [ + "include", + "exclude" + ], + "type": "string" + }, "ShellEnvironmentPolicyToml": { "additionalProperties": false, "description": "Policy for building the `env` when spawning a process via shell-like tools.", "properties": { "exclude": { - "description": "List of regular expressions.", + "description": "Legacy list of regular expressions to exclude.", "items": { "type": "string" }, @@ -2638,7 +2646,7 @@ "type": "boolean" }, "include_only": { - "description": "List of regular expressions.", + "description": "Legacy list of regular expressions to include.", "items": { "type": "string" }, @@ -2647,6 +2655,13 @@ "inherit": { "$ref": "#/definitions/ShellEnvironmentPolicyInherit" }, + "rules": { + "additionalProperties": { + "$ref": "#/definitions/ShellEnvironmentPolicyRule" + }, + "description": "Pattern actions used by the canonical table representation.\n\nOrdinary config keeps accepting the legacy arrays above during the migration. Requirements accept only this keyed form, keeping array compatibility isolated so the legacy fields can be deprecated later.", + "type": "object" + }, "set": { "additionalProperties": { "type": "string" @@ -5203,6 +5218,7 @@ "ignore_default_excludes": null, "include_only": null, "inherit": null, + "rules": null, "set": null } }, @@ -5280,4 +5296,4 @@ }, "title": "ConfigToml", "type": "object" -} +} \ No newline at end of file diff --git a/codex-rs/protocol/src/config_types.rs b/codex-rs/protocol/src/config_types.rs index 713945dbde74..fe1545ee0929 100644 --- a/codex-rs/protocol/src/config_types.rs +++ b/codex-rs/protocol/src/config_types.rs @@ -199,6 +199,16 @@ pub enum ShellEnvironmentPolicyInherit { None, } +/// Assigns a shell environment variable pattern to the include-only or exclude +/// set. Includes do not re-add variables removed by another exclude pattern. +#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "lowercase")] +#[ts(export_to = "v2/")] +pub enum ShellEnvironmentPolicyRule { + Include, + Exclude, +} + pub type EnvironmentVariablePattern = WildMatchPattern<'*', '?'>; /// Deriving the `env` based on this policy works as follows: From 0e7e76038bbc4a19bc5ddc1878cb00a46332f29d Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Mon, 15 Jun 2026 22:54:32 +0000 Subject: [PATCH 02/12] Keep managed shell types out of config rules layer --- codex-rs/config/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index c2ef5ad785b3..ed26757715f8 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -74,7 +74,6 @@ pub use config_requirements::RemoteSandboxConfigToml; pub use config_requirements::RequirementSource; pub use config_requirements::ResidencyRequirement; pub use config_requirements::SandboxModeRequirement; -pub use config_requirements::ShellEnvironmentPolicyRequirementsToml; pub use config_requirements::Sourced; pub use config_requirements::WebSearchModeRequirement; pub use config_requirements::WindowsRequirementsToml; From 822b6dc1b56bc7f3b0d514a77a56809d752bddc8 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 16 Jun 2026 06:07:02 +0000 Subject: [PATCH 03/12] Align shell environment filters with the managed config spec --- codex-rs/config/src/merge.rs | 110 ++++++++++++++++++++++---- codex-rs/config/src/merge_tests.rs | 52 ++++++------ codex-rs/config/src/types.rs | 49 ++++++++++-- codex-rs/config/src/types_tests.rs | 58 +++++++------- codex-rs/core/config.schema.json | 32 ++++---- codex-rs/protocol/src/config_types.rs | 2 +- 6 files changed, 207 insertions(+), 96 deletions(-) diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index f2cb51034503..71cbc310a2ff 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -21,6 +21,10 @@ fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &m normalize_network_domain_keys(base_table); normalize_network_domain_keys(&mut overlay_table); } + if is_shell_environment_filters_path(path) { + normalize_case_insensitive_keys(base_table); + normalize_case_insensitive_keys(&mut overlay_table); + } for (key, value) in overlay_table { path.push(key.clone()); @@ -51,14 +55,21 @@ fn prepare_shell_environment_policy_merge( return; }; - // Ordinary config keeps accepting legacy arrays while `rules` is the + // Ordinary config keeps accepting legacy arrays while `filters` is the // canonical keyed form. Reconcile the two shapes only at this boundary so // layer precedence remains correct and array compatibility can be removed // cleanly after the migration. - for (legacy_field, opposite_field, action) in [ - ("exclude", "include_only", "exclude"), - ("include_only", "exclude", "include"), - ] { + let overlay_has_filters = overlay.contains_key("filters"); + let overlay_has_legacy = + overlay.contains_key("exclude") || overlay.contains_key("include_only"); + if overlay_has_filters && !overlay_has_legacy { + convert_legacy_to_filters(base); + } else if overlay_has_legacy && !overlay_has_filters { + convert_filters_to_legacy(base); + } + + for (legacy_field, opposite_field) in [("exclude", "include_only"), ("include_only", "exclude")] + { let Some(patterns) = overlay .get(legacy_field) .and_then(TomlValue::as_array) @@ -71,21 +82,70 @@ fn prepare_shell_environment_policy_merge( else { continue; }; + remove_patterns_from_legacy_array(base, opposite_field, &patterns); + } +} - if let Some(base_rules) = base.get_mut("rules").and_then(TomlValue::as_table_mut) { - base_rules.retain(|pattern, value| { - value.as_str() != Some(action) && !patterns.contains(&pattern) - }); +fn convert_legacy_to_filters(table: &mut toml::map::Map) { + let mut filters = match table.get("filters") { + Some(TomlValue::Table(filters)) => filters.clone(), + Some(_) => return, + None => toml::map::Map::new(), + }; + table.remove("filters"); + normalize_case_insensitive_keys(&mut filters); + for (field, action) in [("exclude", "exclude"), ("include_only", "include")] { + let Some(TomlValue::Array(patterns)) = table.get(field).cloned() else { + continue; + }; + table.remove(field); + for pattern in patterns { + if let TomlValue::String(pattern) = pattern { + filters + .entry(pattern.to_ascii_lowercase()) + .or_insert_with(|| TomlValue::String(action.to_string())); + } } - remove_patterns_from_legacy_array(base, opposite_field, &patterns); } + if !filters.is_empty() { + table.insert("filters".to_string(), TomlValue::Table(filters)); + } +} - let Some(overlay_rules) = overlay.get("rules").and_then(TomlValue::as_table) else { +fn convert_filters_to_legacy(table: &mut toml::map::Map) { + let Some(TomlValue::Table(mut filters)) = table.get("filters").cloned() else { return; }; - let patterns = overlay_rules.keys().map(String::as_str).collect::>(); - remove_patterns_from_legacy_array(base, "exclude", &patterns); - remove_patterns_from_legacy_array(base, "include_only", &patterns); + table.remove("filters"); + normalize_case_insensitive_keys(&mut filters); + for (pattern, action) in filters { + match action.as_str() { + Some("exclude") => push_legacy_pattern(table, "exclude", "include_only", pattern), + Some("include") => push_legacy_pattern(table, "include_only", "exclude", pattern), + _ => {} + } + } +} + +fn push_legacy_pattern( + table: &mut toml::map::Map, + field: &str, + opposite_field: &str, + pattern: String, +) { + remove_patterns_from_legacy_array(table, opposite_field, &[pattern.as_str()]); + let items = table + .entry(field.to_string()) + .or_insert_with(|| TomlValue::Array(Vec::new())); + let Some(items) = items.as_array_mut() else { + return; + }; + if !items.iter().any(|item| { + item.as_str() + .is_some_and(|candidate| candidate.eq_ignore_ascii_case(&pattern)) + }) { + items.push(TomlValue::String(pattern)); + } } fn remove_patterns_from_legacy_array( @@ -100,11 +160,22 @@ fn remove_patterns_from_legacy_array( return; } items.retain(|item| { - item.as_str() - .is_none_or(|pattern| !patterns.contains(&pattern)) + item.as_str().is_none_or(|candidate| { + !patterns + .iter() + .any(|pattern| candidate.eq_ignore_ascii_case(pattern)) + }) }); } +fn is_shell_environment_filters_path(path: &[String]) -> bool { + matches!( + path, + [policy, filters] + if policy == "shell_environment_policy" && filters == "filters" + ) +} + fn is_permission_network_domains_path(path: &[String]) -> bool { matches!( path, @@ -120,6 +191,13 @@ fn normalize_network_domain_keys(table: &mut toml::map::Map) } } +fn normalize_case_insensitive_keys(table: &mut toml::map::Map) { + let entries = std::mem::take(table); + for (key, value) in entries { + table.insert(key.to_ascii_lowercase(), value); + } +} + #[cfg(test)] #[path = "merge_tests.rs"] mod tests; diff --git a/codex-rs/config/src/merge_tests.rs b/codex-rs/config/src/merge_tests.rs index 271f6c75dcdc..81636b15338c 100644 --- a/codex-rs/config/src/merge_tests.rs +++ b/codex-rs/config/src/merge_tests.rs @@ -147,19 +147,19 @@ exclude = ["HIGH_*"] } #[test] -fn shell_environment_policy_rules_overlay_merges_by_key() { +fn shell_environment_policy_filters_overlay_merges_by_key_case_insensitively() { let mut base = parse_toml( r#" -[shell_environment_policy.rules] +[shell_environment_policy.filters] "FLIP_*" = "exclude" "KEEP_*" = "include" "#, ); let overlay = parse_toml( r#" -[shell_environment_policy.rules] +[shell_environment_policy.filters] "ADD_*" = "exclude" -"FLIP_*" = "include" +"flip_*" = "include" "#, ); @@ -169,17 +169,17 @@ fn shell_environment_policy_rules_overlay_merges_by_key() { base, parse_toml( r#" -[shell_environment_policy.rules] -"ADD_*" = "exclude" -"FLIP_*" = "include" -"KEEP_*" = "include" +[shell_environment_policy.filters] +"add_*" = "exclude" +"flip_*" = "include" +"keep_*" = "include" "#, ) ); } #[test] -fn shell_environment_policy_rules_override_lower_legacy_arrays_by_pattern() { +fn shell_environment_policy_filters_override_lower_legacy_arrays_by_pattern() { let mut base = parse_toml( r#" [shell_environment_policy] @@ -189,7 +189,7 @@ include_only = ["FLIP_TO_EXCLUDE", "KEEP_INCLUDED"] ); let overlay = parse_toml( r#" -[shell_environment_policy.rules] +[shell_environment_policy.filters] "ADD_INCLUDED" = "include" "FLIP_TO_EXCLUDE" = "exclude" "FLIP_TO_INCLUDE" = "include" @@ -202,14 +202,12 @@ include_only = ["FLIP_TO_EXCLUDE", "KEEP_INCLUDED"] base, parse_toml( r#" -[shell_environment_policy] -exclude = ["KEEP_EXCLUDED"] -include_only = ["KEEP_INCLUDED"] - -[shell_environment_policy.rules] -"ADD_INCLUDED" = "include" -"FLIP_TO_EXCLUDE" = "exclude" -"FLIP_TO_INCLUDE" = "include" +[shell_environment_policy.filters] +"add_included" = "include" +"flip_to_exclude" = "exclude" +"flip_to_include" = "include" +"keep_excluded" = "exclude" +"keep_included" = "include" "#, ) ); @@ -219,13 +217,13 @@ include_only = ["KEEP_INCLUDED"] codex_protocol::config_types::ShellEnvironmentPolicy::from(config.shell_environment_policy), codex_protocol::config_types::ShellEnvironmentPolicy::from(ShellEnvironmentPolicyToml { exclude: Some(vec![ - "KEEP_EXCLUDED".to_string(), - "FLIP_TO_EXCLUDE".to_string(), + "flip_to_exclude".to_string(), + "keep_excluded".to_string(), ]), include_only: Some(vec![ - "KEEP_INCLUDED".to_string(), - "ADD_INCLUDED".to_string(), - "FLIP_TO_INCLUDE".to_string(), + "add_included".to_string(), + "flip_to_include".to_string(), + "keep_included".to_string(), ]), ..Default::default() }) @@ -233,10 +231,10 @@ include_only = ["KEEP_INCLUDED"] } #[test] -fn shell_environment_policy_legacy_array_replaces_lower_rules_for_its_action() { +fn shell_environment_policy_legacy_array_replaces_lower_filters_for_its_action() { let mut base = parse_toml( r#" -[shell_environment_policy.rules] +[shell_environment_policy.filters] "FLIP_TO_EXCLUDE" = "include" "LOW_EXCLUDED" = "exclude" "KEEP_INCLUDED" = "include" @@ -257,9 +255,7 @@ exclude = ["FLIP_TO_EXCLUDE", "HIGH_EXCLUDED"] r#" [shell_environment_policy] exclude = ["FLIP_TO_EXCLUDE", "HIGH_EXCLUDED"] - -[shell_environment_policy.rules] -"KEEP_INCLUDED" = "include" +include_only = ["keep_included"] "#, ) ); diff --git a/codex-rs/config/src/types.rs b/codex-rs/config/src/types.rs index 319bb4e4505b..dc68290e27d1 100644 --- a/codex-rs/config/src/types.rs +++ b/codex-rs/config/src/types.rs @@ -18,8 +18,8 @@ pub use codex_protocol::config_types::ModeKind; pub use codex_protocol::config_types::Personality; pub use codex_protocol::config_types::ServiceTier; use codex_protocol::config_types::ShellEnvironmentPolicy; +use codex_protocol::config_types::ShellEnvironmentPolicyFilter; use codex_protocol::config_types::ShellEnvironmentPolicyInherit; -use codex_protocol::config_types::ShellEnvironmentPolicyRule; pub use codex_protocol::config_types::WebSearchMode; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::BTreeMap; @@ -929,7 +929,7 @@ impl From for codex_app_server_protocol::SandboxSettings } /// Policy for building the `env` when spawning a process via shell-like tools. -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, JsonSchema)] +#[derive(Serialize, Debug, Clone, PartialEq, Default, JsonSchema)] #[schemars(deny_unknown_fields)] pub struct ShellEnvironmentPolicyToml { pub inherit: Option, @@ -949,11 +949,46 @@ pub struct ShellEnvironmentPolicyToml { /// Ordinary config keeps accepting the legacy arrays above during the /// migration. Requirements accept only this keyed form, keeping array /// compatibility isolated so the legacy fields can be deprecated later. - pub rules: Option>, + pub filters: Option>, pub experimental_use_profile: Option, } +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct ShellEnvironmentPolicyTomlRaw { + inherit: Option, + ignore_default_excludes: Option, + exclude: Option>, + r#set: Option>, + include_only: Option>, + filters: Option>, + experimental_use_profile: Option, +} + +impl<'de> Deserialize<'de> for ShellEnvironmentPolicyToml { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let raw = ShellEnvironmentPolicyTomlRaw::deserialize(deserializer)?; + if raw.filters.is_some() && (raw.exclude.is_some() || raw.include_only.is_some()) { + return Err(serde::de::Error::custom( + "cannot mix `filters` with legacy `exclude` or `include_only`", + )); + } + Ok(Self { + inherit: raw.inherit, + ignore_default_excludes: raw.ignore_default_excludes, + exclude: raw.exclude, + r#set: raw.r#set, + include_only: raw.include_only, + filters: raw.filters, + experimental_use_profile: raw.experimental_use_profile, + }) + } +} + impl From for ShellEnvironmentPolicy { fn from(toml: ShellEnvironmentPolicyToml) -> Self { // Default to inheriting the full environment when not specified. @@ -961,12 +996,12 @@ impl From for ShellEnvironmentPolicy { let ignore_default_excludes = toml.ignore_default_excludes.unwrap_or(true); let mut exclude = toml.exclude.unwrap_or_default(); let mut include_only = toml.include_only.unwrap_or_default(); - for (pattern, rule) in toml.rules.unwrap_or_default() { + for (pattern, filter) in toml.filters.unwrap_or_default() { exclude.retain(|candidate| candidate != &pattern); include_only.retain(|candidate| candidate != &pattern); - match rule { - ShellEnvironmentPolicyRule::Include => include_only.push(pattern), - ShellEnvironmentPolicyRule::Exclude => exclude.push(pattern), + match filter { + ShellEnvironmentPolicyFilter::Include => include_only.push(pattern), + ShellEnvironmentPolicyFilter::Exclude => exclude.push(pattern), } } let exclude = exclude diff --git a/codex-rs/config/src/types_tests.rs b/codex-rs/config/src/types_tests.rs index ac1b1bcb5a43..2bffcdc4427c 100644 --- a/codex-rs/config/src/types_tests.rs +++ b/codex-rs/config/src/types_tests.rs @@ -2,7 +2,7 @@ use super::*; use pretty_assertions::assert_eq; #[test] -fn shell_environment_policy_accepts_legacy_lists_and_rules() { +fn shell_environment_policy_accepts_legacy_lists_or_filters() { let legacy: ShellEnvironmentPolicyToml = toml::from_str( r#" exclude = ["LEGACY_*", "SHARED_*"] @@ -19,57 +19,59 @@ include_only = ["PATH", "HOME"] } ); - let ruled: ShellEnvironmentPolicyToml = toml::from_str( + let filtered: ShellEnvironmentPolicyToml = toml::from_str( r#" -exclude = ["KEEP_EXCLUDED", "FLIP_TO_INCLUDE"] -include_only = ["KEEP_INCLUDED", "FLIP_TO_EXCLUDE"] - -[rules] +[filters] "FLIP_TO_EXCLUDE" = "exclude" "FLIP_TO_INCLUDE" = "include" "#, ) - .expect("rules should be valid in config.toml"); + .expect("filters should be valid in config.toml"); assert_eq!( - ruled, + filtered, ShellEnvironmentPolicyToml { - exclude: Some(vec![ - "KEEP_EXCLUDED".to_string(), - "FLIP_TO_INCLUDE".to_string(), - ]), - include_only: Some(vec![ - "KEEP_INCLUDED".to_string(), - "FLIP_TO_EXCLUDE".to_string(), - ]), - rules: Some(BTreeMap::from([ + filters: Some(BTreeMap::from([ ( "FLIP_TO_EXCLUDE".to_string(), - ShellEnvironmentPolicyRule::Exclude, + ShellEnvironmentPolicyFilter::Exclude, ), ( "FLIP_TO_INCLUDE".to_string(), - ShellEnvironmentPolicyRule::Include, + ShellEnvironmentPolicyFilter::Include, ), ])), ..Default::default() } ); assert_eq!( - ShellEnvironmentPolicy::from(ruled), + ShellEnvironmentPolicy::from(filtered), ShellEnvironmentPolicy::from(ShellEnvironmentPolicyToml { - exclude: Some(vec![ - "KEEP_EXCLUDED".to_string(), - "FLIP_TO_EXCLUDE".to_string(), - ]), - include_only: Some(vec![ - "KEEP_INCLUDED".to_string(), - "FLIP_TO_INCLUDE".to_string(), - ]), + exclude: Some(vec!["FLIP_TO_EXCLUDE".to_string()]), + include_only: Some(vec!["FLIP_TO_INCLUDE".to_string()]), ..Default::default() }) ); } +#[test] +fn shell_environment_policy_rejects_mixed_legacy_lists_and_filters() { + let error = toml::from_str::( + r#" +exclude = ["LEGACY_*"] + +[filters] +"CANONICAL_*" = "include" +"#, + ) + .expect_err("one config layer must not mix legacy lists and filters"); + + assert!( + error + .to_string() + .contains("cannot mix `filters` with legacy `exclude` or `include_only`") + ); +} + #[test] fn deserialize_skill_config_with_name_selector() { let cfg: SkillConfig = toml::from_str( diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 761f2949aa89..22034964a05c 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2595,6 +2595,14 @@ ], "type": "string" }, + "ShellEnvironmentPolicyFilter": { + "description": "Assigns a shell environment variable pattern to the include-only or exclude set. Includes do not re-add variables removed by another exclude pattern.", + "enum": [ + "include", + "exclude" + ], + "type": "string" + }, "ShellEnvironmentPolicyInherit": { "oneOf": [ { @@ -2620,14 +2628,6 @@ } ] }, - "ShellEnvironmentPolicyRule": { - "description": "Assigns a shell environment variable pattern to the include-only or exclude set. Includes do not re-add variables removed by another exclude pattern.", - "enum": [ - "include", - "exclude" - ], - "type": "string" - }, "ShellEnvironmentPolicyToml": { "additionalProperties": false, "description": "Policy for building the `env` when spawning a process via shell-like tools.", @@ -2642,6 +2642,13 @@ "experimental_use_profile": { "type": "boolean" }, + "filters": { + "additionalProperties": { + "$ref": "#/definitions/ShellEnvironmentPolicyFilter" + }, + "description": "Pattern actions used by the canonical table representation.\n\nOrdinary config keeps accepting the legacy arrays above during the migration. Requirements accept only this keyed form, keeping array compatibility isolated so the legacy fields can be deprecated later.", + "type": "object" + }, "ignore_default_excludes": { "type": "boolean" }, @@ -2655,13 +2662,6 @@ "inherit": { "$ref": "#/definitions/ShellEnvironmentPolicyInherit" }, - "rules": { - "additionalProperties": { - "$ref": "#/definitions/ShellEnvironmentPolicyRule" - }, - "description": "Pattern actions used by the canonical table representation.\n\nOrdinary config keeps accepting the legacy arrays above during the migration. Requirements accept only this keyed form, keeping array compatibility isolated so the legacy fields can be deprecated later.", - "type": "object" - }, "set": { "additionalProperties": { "type": "string" @@ -5215,10 +5215,10 @@ "default": { "exclude": null, "experimental_use_profile": null, + "filters": null, "ignore_default_excludes": null, "include_only": null, "inherit": null, - "rules": null, "set": null } }, diff --git a/codex-rs/protocol/src/config_types.rs b/codex-rs/protocol/src/config_types.rs index fe1545ee0929..4e54301d91ca 100644 --- a/codex-rs/protocol/src/config_types.rs +++ b/codex-rs/protocol/src/config_types.rs @@ -204,7 +204,7 @@ pub enum ShellEnvironmentPolicyInherit { #[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "lowercase")] #[ts(export_to = "v2/")] -pub enum ShellEnvironmentPolicyRule { +pub enum ShellEnvironmentPolicyFilter { Include, Exclude, } From 543af266c480fa99aa15c9c3878ab873c21f06ab Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 16 Jun 2026 17:33:07 +0000 Subject: [PATCH 04/12] Address shell environment filter review feedback --- codex-rs/config/src/merge.rs | 65 ++++++++----------- codex-rs/config/src/state.rs | 20 ++++++ codex-rs/config/src/types.rs | 2 +- codex-rs/core/config.schema.json | 2 +- .../core/src/config/config_loader_tests.rs | 31 +++++++++ 5 files changed, 81 insertions(+), 39 deletions(-) diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index 71cbc310a2ff..4d4909363a93 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -9,7 +9,7 @@ pub fn merge_toml_values(base: &mut TomlValue, overlay: &TomlValue) { } fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &mut Vec) { - prepare_shell_environment_policy_merge(base, overlay, path); + reconcile_shell_environment_policy_representations(base, overlay, path); if let TomlValue::Table(overlay_table) = overlay && let TomlValue::Table(base_table) = base @@ -17,14 +17,8 @@ fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &m normalize_key_aliases(path, base_table); let mut overlay_table = overlay_table.clone(); normalize_key_aliases(path, &mut overlay_table); - if is_permission_network_domains_path(path) { - normalize_network_domain_keys(base_table); - normalize_network_domain_keys(&mut overlay_table); - } - if is_shell_environment_filters_path(path) { - normalize_case_insensitive_keys(base_table); - normalize_case_insensitive_keys(&mut overlay_table); - } + normalize_merge_table_keys(path, base_table); + normalize_merge_table_keys(path, &mut overlay_table); for (key, value) in overlay_table { path.push(key.clone()); @@ -40,7 +34,11 @@ fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &m } } -fn prepare_shell_environment_policy_merge( +/// Ordinary config keeps accepting legacy arrays while `filters` is the +/// canonical keyed form. Reconcile the two shapes only at this boundary so +/// layer precedence remains correct and array compatibility can be removed +/// cleanly after the migration. +fn reconcile_shell_environment_policy_representations( base: &mut TomlValue, overlay: &TomlValue, path: &[String], @@ -55,10 +53,6 @@ fn prepare_shell_environment_policy_merge( return; }; - // Ordinary config keeps accepting legacy arrays while `filters` is the - // canonical keyed form. Reconcile the two shapes only at this boundary so - // layer precedence remains correct and array compatibility can be removed - // cleanly after the migration. let overlay_has_filters = overlay.contains_key("filters"); let overlay_has_legacy = overlay.contains_key("exclude") || overlay.contains_key("include_only"); @@ -93,7 +87,7 @@ fn convert_legacy_to_filters(table: &mut toml::map::Map) { None => toml::map::Map::new(), }; table.remove("filters"); - normalize_case_insensitive_keys(&mut filters); + normalize_table_keys(&mut filters, |key| key.to_ascii_lowercase()); for (field, action) in [("exclude", "exclude"), ("include_only", "include")] { let Some(TomlValue::Array(patterns)) = table.get(field).cloned() else { continue; @@ -117,7 +111,7 @@ fn convert_filters_to_legacy(table: &mut toml::map::Map) { return; }; table.remove("filters"); - normalize_case_insensitive_keys(&mut filters); + normalize_table_keys(&mut filters, |key| key.to_ascii_lowercase()); for (pattern, action) in filters { match action.as_str() { Some("exclude") => push_legacy_pattern(table, "exclude", "include_only", pattern), @@ -168,33 +162,30 @@ fn remove_patterns_from_legacy_array( }); } -fn is_shell_environment_filters_path(path: &[String]) -> bool { - matches!( - path, - [policy, filters] - if policy == "shell_environment_policy" && filters == "filters" - ) -} - -fn is_permission_network_domains_path(path: &[String]) -> bool { - matches!( - path, +/// Canonicalizes keys for maps whose keys compare independently of TOML's +/// case-sensitive key semantics, so equivalent entries collide before merging. +fn normalize_merge_table_keys(path: &[String], table: &mut toml::map::Map) { + match path { [permissions, _, network, domains] - if permissions == "permissions" && network == "network" && domains == "domains" - ) -} - -fn normalize_network_domain_keys(table: &mut toml::map::Map) { - let entries = std::mem::take(table); - for (pattern, value) in entries { - table.insert(normalize_host(&pattern), value); + if permissions == "permissions" && network == "network" && domains == "domains" => + { + normalize_table_keys(table, normalize_host); + } + [policy, filters] if policy == "shell_environment_policy" && filters == "filters" => { + // Environment-variable patterns compare case-insensitively. + normalize_table_keys(table, |key| key.to_ascii_lowercase()); + } + _ => {} } } -fn normalize_case_insensitive_keys(table: &mut toml::map::Map) { +fn normalize_table_keys( + table: &mut toml::map::Map, + normalize_key: impl Fn(&str) -> String, +) { let entries = std::mem::take(table); for (key, value) in entries { - table.insert(key.to_ascii_lowercase(), value); + table.insert(normalize_key(&key), value); } } diff --git a/codex-rs/config/src/state.rs b/codex-rs/config/src/state.rs index 9d8de1c8852c..463ab23e601a 100644 --- a/codex-rs/config/src/state.rs +++ b/codex-rs/config/src/state.rs @@ -1,5 +1,7 @@ +use crate::CONFIG_TOML_FILE; use crate::config_requirements::ConfigRequirements; use crate::config_requirements::ConfigRequirementsToml; +use crate::format_config_layer_source; use super::fingerprint::record_origins; use super::fingerprint::version_for_toml; @@ -275,6 +277,24 @@ impl ConfigLayerStack { requirements: ConfigRequirements, requirements_toml: ConfigRequirementsToml, ) -> std::io::Result { + if let Some(layer) = layers.iter().find(|layer| { + layer + .config + .get("shell_environment_policy") + .and_then(TomlValue::as_table) + .is_some_and(|policy| { + policy.contains_key("filters") + && (policy.contains_key("exclude") || policy.contains_key("include_only")) + }) + }) { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!( + "cannot mix `filters` with legacy `exclude` or `include_only` in {}", + format_config_layer_source(&layer.name, CONFIG_TOML_FILE) + ), + )); + } let user_layer_index = verify_layer_ordering(&layers)?; Ok(Self { layers, diff --git a/codex-rs/config/src/types.rs b/codex-rs/config/src/types.rs index dc68290e27d1..69fd80816e3d 100644 --- a/codex-rs/config/src/types.rs +++ b/codex-rs/config/src/types.rs @@ -947,7 +947,7 @@ pub struct ShellEnvironmentPolicyToml { /// Pattern actions used by the canonical table representation. /// /// Ordinary config keeps accepting the legacy arrays above during the - /// migration. Requirements accept only this keyed form, keeping array + /// migration. Requirements will accept only this keyed form, keeping array /// compatibility isolated so the legacy fields can be deprecated later. pub filters: Option>, diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 22034964a05c..0bbcbea13160 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2646,7 +2646,7 @@ "additionalProperties": { "$ref": "#/definitions/ShellEnvironmentPolicyFilter" }, - "description": "Pattern actions used by the canonical table representation.\n\nOrdinary config keeps accepting the legacy arrays above during the migration. Requirements accept only this keyed form, keeping array compatibility isolated so the legacy fields can be deprecated later.", + "description": "Pattern actions used by the canonical table representation.\n\nOrdinary config keeps accepting the legacy arrays above during the migration. Requirements will accept only this keyed form, keeping array compatibility isolated so the legacy fields can be deprecated later.", "type": "object" }, "ignore_default_excludes": { diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index 87a46fd14a52..b02a811d5986 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -379,6 +379,37 @@ unknown_key = true"#; assert_eq!(config_error, &expected_config_error); } +#[tokio::test] +async fn non_strict_config_rejects_mixed_shell_environment_policy_forms_per_layer() { + let tmp = tempdir().expect("tempdir"); + let contents = r#" +[shell_environment_policy] +exclude = ["LEGACY_*"] + +[shell_environment_policy.filters] +"CANONICAL_*" = "include" +"#; + std::fs::write(tmp.path().join(CONFIG_TOML_FILE), contents).expect("write config"); + + let err = ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .cli_overrides(vec![( + "shell_environment_policy.filters.HIGHER_*".to_string(), + TomlValue::String("exclude".to_string()), + )]) + .strict_config(/*strict_config*/ false) + .build() + .await + .expect_err("one config layer must not mix legacy lists and filters"); + + assert!( + err.to_string() + .contains("cannot mix `filters` with legacy `exclude` or `include_only`") + ); +} + #[tokio::test] async fn strict_config_rejects_unknown_cli_override_key() { let tmp = tempdir().expect("tempdir"); From a0b18b2598dd4475dde200a77db6030fe0e7815f Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 16 Jun 2026 17:50:48 +0000 Subject: [PATCH 05/12] Fix shell filter Clippy lints --- codex-rs/config/src/merge.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index 4d4909363a93..71208d06de80 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -87,7 +87,7 @@ fn convert_legacy_to_filters(table: &mut toml::map::Map) { None => toml::map::Map::new(), }; table.remove("filters"); - normalize_table_keys(&mut filters, |key| key.to_ascii_lowercase()); + normalize_table_keys(&mut filters, str::to_ascii_lowercase); for (field, action) in [("exclude", "exclude"), ("include_only", "include")] { let Some(TomlValue::Array(patterns)) = table.get(field).cloned() else { continue; @@ -111,7 +111,7 @@ fn convert_filters_to_legacy(table: &mut toml::map::Map) { return; }; table.remove("filters"); - normalize_table_keys(&mut filters, |key| key.to_ascii_lowercase()); + normalize_table_keys(&mut filters, str::to_ascii_lowercase); for (pattern, action) in filters { match action.as_str() { Some("exclude") => push_legacy_pattern(table, "exclude", "include_only", pattern), @@ -173,7 +173,7 @@ fn normalize_merge_table_keys(path: &[String], table: &mut toml::map::Map { // Environment-variable patterns compare case-insensitively. - normalize_table_keys(table, |key| key.to_ascii_lowercase()); + normalize_table_keys(table, str::to_ascii_lowercase); } _ => {} } From d4e440eb320f5e72665c0f149b3b5199e0184291 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 16 Jun 2026 18:01:38 +0000 Subject: [PATCH 06/12] Simplify shell policy tests --- codex-rs/config/src/merge_tests.rs | 18 ------------------ .../core/src/config/config_loader_tests.rs | 2 +- 2 files changed, 1 insertion(+), 19 deletions(-) diff --git a/codex-rs/config/src/merge_tests.rs b/codex-rs/config/src/merge_tests.rs index 81636b15338c..1e41512d16c9 100644 --- a/codex-rs/config/src/merge_tests.rs +++ b/codex-rs/config/src/merge_tests.rs @@ -1,7 +1,6 @@ use super::*; use crate::config_toml::ConfigToml; use crate::types::MemoriesToml; -use crate::types::ShellEnvironmentPolicyToml; use pretty_assertions::assert_eq; fn parse_toml(value: &str) -> TomlValue { @@ -211,23 +210,6 @@ include_only = ["FLIP_TO_EXCLUDE", "KEEP_INCLUDED"] "#, ) ); - - let config: ConfigToml = base.try_into().expect("merged config should deserialize"); - assert_eq!( - codex_protocol::config_types::ShellEnvironmentPolicy::from(config.shell_environment_policy), - codex_protocol::config_types::ShellEnvironmentPolicy::from(ShellEnvironmentPolicyToml { - exclude: Some(vec![ - "flip_to_exclude".to_string(), - "keep_excluded".to_string(), - ]), - include_only: Some(vec![ - "add_included".to_string(), - "flip_to_include".to_string(), - "keep_included".to_string(), - ]), - ..Default::default() - }) - ); } #[test] diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index b02a811d5986..0096919eaff2 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -380,7 +380,7 @@ unknown_key = true"#; } #[tokio::test] -async fn non_strict_config_rejects_mixed_shell_environment_policy_forms_per_layer() { +async fn non_strict_config_rejects_mixed_shell_environment_policy_before_higher_layer_merge() { let tmp = tempdir().expect("tempdir"); let contents = r#" [shell_environment_policy] From 81c349d8c4fe80f9554f648c0c341923c09eb9db Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 16 Jun 2026 20:27:50 +0000 Subject: [PATCH 07/12] Harden shell environment policy loading --- codex-rs/config/src/schema.rs | 35 +++++- codex-rs/config/src/state.rs | 32 +++--- codex-rs/config/src/state_tests.rs | 28 +++++ codex-rs/config/src/types.rs | 11 +- codex-rs/core/config.schema.json | 18 +++ .../core/src/config/config_loader_tests.rs | 104 ++++++++++++++++++ codex-rs/core/src/config/schema_tests.rs | 29 +++++ 7 files changed, 237 insertions(+), 20 deletions(-) diff --git a/codex-rs/config/src/schema.rs b/codex-rs/config/src/schema.rs index c641f1703da5..fec33cbeb3c6 100644 --- a/codex-rs/config/src/schema.rs +++ b/codex-rs/config/src/schema.rs @@ -117,12 +117,43 @@ pub fn mcp_servers_schema(schema_gen: &mut SchemaGenerator) -> Schema { /// Build the config schema for `config.toml`. pub fn config_schema() -> RootSchema { - SchemaSettings::draft07() + let mut schema = SchemaSettings::draft07() .with(|settings| { settings.option_add_null_type = false; }) .into_generator() - .into_root_schema_for::() + .into_root_schema_for::(); + add_shell_environment_policy_constraints(&mut schema); + schema +} + +// Mirror the custom deserializer's mixed-form rejection in the generated schema. +fn add_shell_environment_policy_constraints(schema: &mut RootSchema) { + let Some(Schema::Object(policy)) = schema.definitions.get_mut("ShellEnvironmentPolicyToml") + else { + return; + }; + let all_of = policy + .subschemas + .get_or_insert_default() + .all_of + .get_or_insert_default(); + for fields in [["exclude", "filters"], ["filters", "include_only"]] { + let forbidden_fields = Schema::Object(SchemaObject { + object: Some(Box::new(ObjectValidation { + required: fields.into_iter().map(str::to_string).collect(), + ..Default::default() + })), + ..Default::default() + }); + all_of.push(Schema::Object(SchemaObject { + subschemas: Some(Box::new(SubschemaValidation { + not: Some(Box::new(forbidden_fields)), + ..Default::default() + })), + ..Default::default() + })); + } } /// Canonicalize a JSON value by sorting its keys. diff --git a/codex-rs/config/src/state.rs b/codex-rs/config/src/state.rs index 463ab23e601a..f79b0f00726e 100644 --- a/codex-rs/config/src/state.rs +++ b/codex-rs/config/src/state.rs @@ -9,6 +9,7 @@ use super::key_aliases::normalized_with_key_aliases; use super::merge::merge_toml_values; use crate::CloudConfigBundleLoader; use crate::ProfileV2Name; +use crate::types::ShellEnvironmentPolicyToml; use codex_app_server_protocol::ConfigLayer; use codex_app_server_protocol::ConfigLayerMetadata; use codex_app_server_protocol::ConfigLayerSource; @@ -277,23 +278,20 @@ impl ConfigLayerStack { requirements: ConfigRequirements, requirements_toml: ConfigRequirementsToml, ) -> std::io::Result { - if let Some(layer) = layers.iter().find(|layer| { - layer - .config - .get("shell_environment_policy") - .and_then(TomlValue::as_table) - .is_some_and(|policy| { - policy.contains_key("filters") - && (policy.contains_key("exclude") || policy.contains_key("include_only")) - }) - }) { - return Err(std::io::Error::new( - std::io::ErrorKind::InvalidData, - format!( - "cannot mix `filters` with legacy `exclude` or `include_only` in {}", - format_config_layer_source(&layer.name, CONFIG_TOML_FILE) - ), - )); + // Validate enabled layers before merging to reject mixed forms and malformed entries + for layer in layers.iter().filter(|layer| !layer.is_disabled()) { + let Some(policy) = layer.config.get("shell_environment_policy") else { + continue; + }; + let _: ShellEnvironmentPolicyToml = policy.clone().try_into().map_err(|error| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!( + "invalid shell environment policy in {}: {error}", + format_config_layer_source(&layer.name, CONFIG_TOML_FILE) + ), + ) + })?; } let user_layer_index = verify_layer_ordering(&layers)?; Ok(Self { diff --git a/codex-rs/config/src/state_tests.rs b/codex-rs/config/src/state_tests.rs index fb6e26968b1a..bebe68e34fdd 100644 --- a/codex-rs/config/src/state_tests.rs +++ b/codex-rs/config/src/state_tests.rs @@ -39,6 +39,34 @@ no_memories_if_mcp_or_web_search = true ); } +#[test] +fn disabled_layers_do_not_validate_shell_environment_policy() { + let layer = ConfigLayerEntry::new_disabled( + ConfigLayerSource::Project { + dot_codex_folder: AbsolutePathBuf::from_absolute_path("/untrusted/.codex") + .expect("project path should be absolute"), + }, + toml::from_str( + r#" +[shell_environment_policy] +exclude = ["LEGACY_*"] + +[shell_environment_policy.filters] +"CANONICAL_*" = "include" +"#, + ) + .expect("project config"), + "project is untrusted", + ); + + ConfigLayerStack::new( + vec![layer], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("disabled layers should not be validated"); +} + #[test] fn active_user_layer_is_highest_precedence_user_layer() { let temp_dir = TempDir::new().expect("tempdir"); diff --git a/codex-rs/config/src/types.rs b/codex-rs/config/src/types.rs index 69fd80816e3d..5f32670ab83c 100644 --- a/codex-rs/config/src/types.rs +++ b/codex-rs/config/src/types.rs @@ -955,7 +955,6 @@ pub struct ShellEnvironmentPolicyToml { } #[derive(Deserialize)] -#[serde(deny_unknown_fields)] struct ShellEnvironmentPolicyTomlRaw { inherit: Option, ignore_default_excludes: Option, @@ -977,6 +976,16 @@ impl<'de> Deserialize<'de> for ShellEnvironmentPolicyToml { "cannot mix `filters` with legacy `exclude` or `include_only`", )); } + if let Some(filters) = raw.filters.as_ref() { + let mut patterns = std::collections::HashSet::new(); + for pattern in filters.keys() { + if !patterns.insert(pattern.to_ascii_lowercase()) { + return Err(serde::de::Error::custom(format!( + "duplicate shell environment filter `{pattern}` ignoring ASCII case" + ))); + } + } + } Ok(Self { inherit: raw.inherit, ignore_default_excludes: raw.ignore_default_excludes, diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 0bbcbea13160..cdd6eb227169 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2630,6 +2630,24 @@ }, "ShellEnvironmentPolicyToml": { "additionalProperties": false, + "allOf": [ + { + "not": { + "required": [ + "exclude", + "filters" + ] + } + }, + { + "not": { + "required": [ + "filters", + "include_only" + ] + } + } + ], "description": "Policy for building the `env` when spawning a process via shell-like tools.", "properties": { "exclude": { diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index 0096919eaff2..cdef5c42eea7 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -410,6 +410,110 @@ exclude = ["LEGACY_*"] ); } +#[tokio::test] +async fn shell_environment_policy_unknown_fields_follow_strict_config() { + let tmp = tempdir().expect("tempdir"); + let contents = r#" +[shell_environment_policy] +future_field = true +"#; + let config_path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&config_path, contents).expect("write config"); + + ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .strict_config(/*strict_config*/ false) + .build() + .await + .expect("non-strict config should ignore unknown fields"); + + let err = ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .strict_config(/*strict_config*/ true) + .build() + .await + .expect_err("strict config should reject unknown fields"); + + assert_eq!( + config_error_from_io(&err).message, + "unknown configuration field `shell_environment_policy.future_field`" + ); +} + +#[tokio::test] +async fn non_strict_config_rejects_duplicate_shell_environment_policy_filters_before_merging() { + let tmp = tempdir().expect("tempdir"); + let contents = r#" +[shell_environment_policy.filters] +"SECRET_TOKEN" = "exclude" +"secret_token" = "include" +"#; + std::fs::write(tmp.path().join(CONFIG_TOML_FILE), contents).expect("write config"); + + let err = ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .cli_overrides(vec![( + "shell_environment_policy.filters.PATH".to_string(), + TomlValue::String("include".to_string()), + )]) + .strict_config(/*strict_config*/ false) + .build() + .await + .expect_err("case-insensitive duplicate filters should be rejected per layer"); + + assert!( + err.to_string() + .contains("duplicate shell environment filter") + ); +} + +#[tokio::test] +async fn non_strict_config_rejects_malformed_shell_policy_before_representation_conversion() { + let cases = [ + ( + r#" +[shell_environment_policy] +exclude = ["SECRET_*", 17] +"#, + vec![( + "shell_environment_policy.filters.PATH".to_string(), + TomlValue::String("include".to_string()), + )], + ), + ( + r#" +[shell_environment_policy.filters] +"SECRET_*" = "exlude" +"#, + vec![( + "shell_environment_policy.exclude".to_string(), + TomlValue::Array(vec![TomlValue::String("PATH".to_string())]), + )], + ), + ]; + + for (contents, cli_overrides) in cases { + let tmp = tempdir().expect("tempdir"); + std::fs::write(tmp.path().join(CONFIG_TOML_FILE), contents).expect("write config"); + + ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .cli_overrides(cli_overrides) + .strict_config(/*strict_config*/ false) + .build() + .await + .expect_err("malformed shell policy should be rejected before conversion"); + } +} + #[tokio::test] async fn strict_config_rejects_unknown_cli_override_key() { let tmp = tempdir().expect("tempdir"); diff --git a/codex-rs/core/src/config/schema_tests.rs b/codex-rs/core/src/config/schema_tests.rs index dd67ead89867..479f7f318c1c 100644 --- a/codex-rs/core/src/config/schema_tests.rs +++ b/codex-rs/core/src/config/schema_tests.rs @@ -73,3 +73,32 @@ fn config_schema_hides_unsupported_inline_mcp_bearer_token() { (false, true), ); } + +#[test] +fn shell_environment_policy_schema_rejects_mixed_filter_representations() { + let schema_json = config_schema_json().expect("serialize config schema"); + let schema_value: serde_json::Value = + serde_json::from_slice(&schema_json).expect("decode schema json"); + let constraints = schema_value + .pointer("/definitions/ShellEnvironmentPolicyToml/allOf") + .expect("shell environment policy constraints should exist") + .as_array() + .expect("shell environment policy constraints should be an array"); + let required_pairs = constraints + .iter() + .map(|constraint| { + constraint + .pointer("/not/required") + .expect("constraint should prohibit a required-field pair") + .clone() + }) + .collect::>(); + + assert_eq!( + required_pairs, + vec![ + serde_json::json!(["exclude", "filters"]), + serde_json::json!(["filters", "include_only"]), + ] + ); +} From 84f259737e5cbf34c0bfe6f63a33962367f4e22a Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 16 Jun 2026 20:58:00 +0000 Subject: [PATCH 08/12] Simplify shell policy schema validation --- codex-rs/config/src/schema.rs | 35 ++----------------- codex-rs/core/config.schema.json | 18 ---------- .../core/src/config/config_loader_tests.rs | 2 +- codex-rs/core/src/config/schema_tests.rs | 29 --------------- 4 files changed, 3 insertions(+), 81 deletions(-) diff --git a/codex-rs/config/src/schema.rs b/codex-rs/config/src/schema.rs index fec33cbeb3c6..c641f1703da5 100644 --- a/codex-rs/config/src/schema.rs +++ b/codex-rs/config/src/schema.rs @@ -117,43 +117,12 @@ pub fn mcp_servers_schema(schema_gen: &mut SchemaGenerator) -> Schema { /// Build the config schema for `config.toml`. pub fn config_schema() -> RootSchema { - let mut schema = SchemaSettings::draft07() + SchemaSettings::draft07() .with(|settings| { settings.option_add_null_type = false; }) .into_generator() - .into_root_schema_for::(); - add_shell_environment_policy_constraints(&mut schema); - schema -} - -// Mirror the custom deserializer's mixed-form rejection in the generated schema. -fn add_shell_environment_policy_constraints(schema: &mut RootSchema) { - let Some(Schema::Object(policy)) = schema.definitions.get_mut("ShellEnvironmentPolicyToml") - else { - return; - }; - let all_of = policy - .subschemas - .get_or_insert_default() - .all_of - .get_or_insert_default(); - for fields in [["exclude", "filters"], ["filters", "include_only"]] { - let forbidden_fields = Schema::Object(SchemaObject { - object: Some(Box::new(ObjectValidation { - required: fields.into_iter().map(str::to_string).collect(), - ..Default::default() - })), - ..Default::default() - }); - all_of.push(Schema::Object(SchemaObject { - subschemas: Some(Box::new(SubschemaValidation { - not: Some(Box::new(forbidden_fields)), - ..Default::default() - })), - ..Default::default() - })); - } + .into_root_schema_for::() } /// Canonicalize a JSON value by sorting its keys. diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index cdd6eb227169..0bbcbea13160 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2630,24 +2630,6 @@ }, "ShellEnvironmentPolicyToml": { "additionalProperties": false, - "allOf": [ - { - "not": { - "required": [ - "exclude", - "filters" - ] - } - }, - { - "not": { - "required": [ - "filters", - "include_only" - ] - } - } - ], "description": "Policy for building the `env` when spawning a process via shell-like tools.", "properties": { "exclude": { diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index cdef5c42eea7..9b514f8d9ee7 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -489,7 +489,7 @@ exclude = ["SECRET_*", 17] ( r#" [shell_environment_policy.filters] -"SECRET_*" = "exlude" +"SECRET_*" = "keep" "#, vec![( "shell_environment_policy.exclude".to_string(), diff --git a/codex-rs/core/src/config/schema_tests.rs b/codex-rs/core/src/config/schema_tests.rs index 479f7f318c1c..dd67ead89867 100644 --- a/codex-rs/core/src/config/schema_tests.rs +++ b/codex-rs/core/src/config/schema_tests.rs @@ -73,32 +73,3 @@ fn config_schema_hides_unsupported_inline_mcp_bearer_token() { (false, true), ); } - -#[test] -fn shell_environment_policy_schema_rejects_mixed_filter_representations() { - let schema_json = config_schema_json().expect("serialize config schema"); - let schema_value: serde_json::Value = - serde_json::from_slice(&schema_json).expect("decode schema json"); - let constraints = schema_value - .pointer("/definitions/ShellEnvironmentPolicyToml/allOf") - .expect("shell environment policy constraints should exist") - .as_array() - .expect("shell environment policy constraints should be an array"); - let required_pairs = constraints - .iter() - .map(|constraint| { - constraint - .pointer("/not/required") - .expect("constraint should prohibit a required-field pair") - .clone() - }) - .collect::>(); - - assert_eq!( - required_pairs, - vec![ - serde_json::json!(["exclude", "filters"]), - serde_json::json!(["filters", "include_only"]), - ] - ); -} From 8cec5f1a634aa053565d51971fa42fb478fdaed9 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 16 Jun 2026 23:54:34 +0000 Subject: [PATCH 09/12] Fix shell filter override reporting --- .../app-server/src/config_manager_service.rs | 28 ++++++++- .../src/config_manager_service_tests.rs | 62 +++++++++++++++++++ 2 files changed, 87 insertions(+), 3 deletions(-) diff --git a/codex-rs/app-server/src/config_manager_service.rs b/codex-rs/app-server/src/config_manager_service.rs index 4b42c28aa904..809314388f51 100644 --- a/codex-rs/app-server/src/config_manager_service.rs +++ b/codex-rs/app-server/src/config_manager_service.rs @@ -616,6 +616,26 @@ fn value_at_path<'a>(root: &'a TomlValue, segments: &[String]) -> Option<&'a Tom Some(current) } +/// Looks up a path according to its config semantics. +/// +/// Shell filter patterns compare case-insensitively and are normalized in the +/// merged effective config, while raw layers preserve their original spelling. +/// Match that path segment case-insensitively so equivalent raw and effective +/// paths compare without changing ordinary TOML path semantics. +fn value_at_semantic_path<'a>(root: &'a TomlValue, segments: &[String]) -> Option<&'a TomlValue> { + let [policy, filters, pattern] = segments else { + return value_at_path(root, segments); + }; + if policy != "shell_environment_policy" || filters != "filters" { + return value_at_path(root, segments); + } + + let filters = value_at_path(root, &segments[..2])?.as_table()?; + filters + .iter() + .find_map(|(candidate, value)| candidate.eq_ignore_ascii_case(pattern).then_some(value)) +} + fn override_message(layer: &ConfigLayerSource) -> String { match layer { ConfigLayerSource::Mdm { domain, key: _ } => { @@ -653,10 +673,10 @@ fn compute_override_metadata( segments: &[String], ) -> Option { let user_value = match layers.get_active_user_layer() { - Some(user_layer) => value_at_path(&user_layer.config, segments), + Some(user_layer) => value_at_semantic_path(&user_layer.config, segments), None => return None, }; - let effective_value = value_at_path(effective, segments); + let effective_value = value_at_semantic_path(effective, segments); if user_value.is_some() && user_value == effective_value { return None; @@ -696,7 +716,9 @@ fn find_effective_layer( segments: &[String], ) -> Option { for layer in layers.layers_high_to_low() { - if let Some(meta) = value_at_path(&layer.config, segments).map(|_| layer.metadata()) { + if let Some(meta) = + value_at_semantic_path(&layer.config, segments).map(|_| layer.metadata()) + { return Some(meta); } } diff --git a/codex-rs/app-server/src/config_manager_service_tests.rs b/codex-rs/app-server/src/config_manager_service_tests.rs index 8deae8b0658c..f09da45832ac 100644 --- a/codex-rs/app-server/src/config_manager_service_tests.rs +++ b/codex-rs/app-server/src/config_manager_service_tests.rs @@ -841,6 +841,68 @@ async fn write_value_reports_managed_override() { assert_eq!(overridden.effective_value, serde_json::json!("never")); } +#[tokio::test] +async fn write_value_matches_shell_filter_paths_case_insensitively() -> Result<()> { + let tmp = tempdir()?; + let config_path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&config_path, "")?; + + let service = ConfigManager::without_managed_config_for_tests(tmp.path().to_path_buf()); + let result = service + .write_value(ConfigValueWriteParams { + file_path: Some(config_path.display().to_string()), + key_path: "shell_environment_policy.filters.PATH".to_string(), + value: serde_json::json!("include"), + merge_strategy: MergeStrategy::Replace, + expected_version: None, + }) + .await?; + + assert_eq!(result.status, WriteStatus::Ok); + assert_eq!(result.overridden_metadata, None); + assert!(std::fs::read_to_string(config_path)?.contains("PATH = \"include\"")); + Ok(()) +} + +#[tokio::test] +async fn write_value_reports_case_insensitive_shell_filter_override() -> Result<()> { + let tmp = tempdir()?; + let config_path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&config_path, "")?; + + let managed_path = tmp.path().join("managed_config.toml"); + std::fs::write( + &managed_path, + "[shell_environment_policy.filters]\npath = \"exclude\"\n", + )?; + let managed_file = AbsolutePathBuf::try_from(managed_path.clone())?; + let service = ConfigManager::new_for_tests( + tmp.path().to_path_buf(), + vec![], + LoaderOverrides::with_managed_config_path_for_tests(managed_path), + CloudConfigBundleLoader::default(), + ); + + let result = service + .write_value(ConfigValueWriteParams { + file_path: Some(config_path.display().to_string()), + key_path: "shell_environment_policy.filters.PATH".to_string(), + value: serde_json::json!("include"), + merge_strategy: MergeStrategy::Replace, + expected_version: None, + }) + .await?; + + assert_eq!(result.status, WriteStatus::OkOverridden); + let overridden = result.overridden_metadata.expect("overridden metadata"); + assert_eq!( + overridden.overriding_layer.name, + ConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file } + ); + assert_eq!(overridden.effective_value, serde_json::json!("exclude")); + Ok(()) +} + #[tokio::test] async fn upsert_merges_tables_replace_overwrites() -> Result<()> { let tmp = tempdir().expect("tempdir"); From ce0ebcff28480e4bb7fcbec50b9e901504c2f757 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 17 Jun 2026 00:17:25 +0000 Subject: [PATCH 10/12] Make shell filter merges case-sensitive --- .../app-server/src/config_manager_service.rs | 28 +-------- .../src/config_manager_service_tests.rs | 62 ------------------- codex-rs/config/src/merge.rs | 55 ++++++---------- codex-rs/config/src/merge_tests.rs | 19 +++--- codex-rs/config/src/types.rs | 2 + .../core/src/config/config_loader_tests.rs | 7 +-- 6 files changed, 38 insertions(+), 135 deletions(-) diff --git a/codex-rs/app-server/src/config_manager_service.rs b/codex-rs/app-server/src/config_manager_service.rs index 809314388f51..4b42c28aa904 100644 --- a/codex-rs/app-server/src/config_manager_service.rs +++ b/codex-rs/app-server/src/config_manager_service.rs @@ -616,26 +616,6 @@ fn value_at_path<'a>(root: &'a TomlValue, segments: &[String]) -> Option<&'a Tom Some(current) } -/// Looks up a path according to its config semantics. -/// -/// Shell filter patterns compare case-insensitively and are normalized in the -/// merged effective config, while raw layers preserve their original spelling. -/// Match that path segment case-insensitively so equivalent raw and effective -/// paths compare without changing ordinary TOML path semantics. -fn value_at_semantic_path<'a>(root: &'a TomlValue, segments: &[String]) -> Option<&'a TomlValue> { - let [policy, filters, pattern] = segments else { - return value_at_path(root, segments); - }; - if policy != "shell_environment_policy" || filters != "filters" { - return value_at_path(root, segments); - } - - let filters = value_at_path(root, &segments[..2])?.as_table()?; - filters - .iter() - .find_map(|(candidate, value)| candidate.eq_ignore_ascii_case(pattern).then_some(value)) -} - fn override_message(layer: &ConfigLayerSource) -> String { match layer { ConfigLayerSource::Mdm { domain, key: _ } => { @@ -673,10 +653,10 @@ fn compute_override_metadata( segments: &[String], ) -> Option { let user_value = match layers.get_active_user_layer() { - Some(user_layer) => value_at_semantic_path(&user_layer.config, segments), + Some(user_layer) => value_at_path(&user_layer.config, segments), None => return None, }; - let effective_value = value_at_semantic_path(effective, segments); + let effective_value = value_at_path(effective, segments); if user_value.is_some() && user_value == effective_value { return None; @@ -716,9 +696,7 @@ fn find_effective_layer( segments: &[String], ) -> Option { for layer in layers.layers_high_to_low() { - if let Some(meta) = - value_at_semantic_path(&layer.config, segments).map(|_| layer.metadata()) - { + if let Some(meta) = value_at_path(&layer.config, segments).map(|_| layer.metadata()) { return Some(meta); } } diff --git a/codex-rs/app-server/src/config_manager_service_tests.rs b/codex-rs/app-server/src/config_manager_service_tests.rs index f09da45832ac..8deae8b0658c 100644 --- a/codex-rs/app-server/src/config_manager_service_tests.rs +++ b/codex-rs/app-server/src/config_manager_service_tests.rs @@ -841,68 +841,6 @@ async fn write_value_reports_managed_override() { assert_eq!(overridden.effective_value, serde_json::json!("never")); } -#[tokio::test] -async fn write_value_matches_shell_filter_paths_case_insensitively() -> Result<()> { - let tmp = tempdir()?; - let config_path = tmp.path().join(CONFIG_TOML_FILE); - std::fs::write(&config_path, "")?; - - let service = ConfigManager::without_managed_config_for_tests(tmp.path().to_path_buf()); - let result = service - .write_value(ConfigValueWriteParams { - file_path: Some(config_path.display().to_string()), - key_path: "shell_environment_policy.filters.PATH".to_string(), - value: serde_json::json!("include"), - merge_strategy: MergeStrategy::Replace, - expected_version: None, - }) - .await?; - - assert_eq!(result.status, WriteStatus::Ok); - assert_eq!(result.overridden_metadata, None); - assert!(std::fs::read_to_string(config_path)?.contains("PATH = \"include\"")); - Ok(()) -} - -#[tokio::test] -async fn write_value_reports_case_insensitive_shell_filter_override() -> Result<()> { - let tmp = tempdir()?; - let config_path = tmp.path().join(CONFIG_TOML_FILE); - std::fs::write(&config_path, "")?; - - let managed_path = tmp.path().join("managed_config.toml"); - std::fs::write( - &managed_path, - "[shell_environment_policy.filters]\npath = \"exclude\"\n", - )?; - let managed_file = AbsolutePathBuf::try_from(managed_path.clone())?; - let service = ConfigManager::new_for_tests( - tmp.path().to_path_buf(), - vec![], - LoaderOverrides::with_managed_config_path_for_tests(managed_path), - CloudConfigBundleLoader::default(), - ); - - let result = service - .write_value(ConfigValueWriteParams { - file_path: Some(config_path.display().to_string()), - key_path: "shell_environment_policy.filters.PATH".to_string(), - value: serde_json::json!("include"), - merge_strategy: MergeStrategy::Replace, - expected_version: None, - }) - .await?; - - assert_eq!(result.status, WriteStatus::OkOverridden); - let overridden = result.overridden_metadata.expect("overridden metadata"); - assert_eq!( - overridden.overriding_layer.name, - ConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file } - ); - assert_eq!(overridden.effective_value, serde_json::json!("exclude")); - Ok(()) -} - #[tokio::test] async fn upsert_merges_tables_replace_overwrites() -> Result<()> { let tmp = tempdir().expect("tempdir"); diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index 71208d06de80..4fdb81c2f17e 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -17,8 +17,10 @@ fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &m normalize_key_aliases(path, base_table); let mut overlay_table = overlay_table.clone(); normalize_key_aliases(path, &mut overlay_table); - normalize_merge_table_keys(path, base_table); - normalize_merge_table_keys(path, &mut overlay_table); + if is_permission_network_domains_path(path) { + normalize_network_domain_keys(base_table); + normalize_network_domain_keys(&mut overlay_table); + } for (key, value) in overlay_table { path.push(key.clone()); @@ -87,7 +89,6 @@ fn convert_legacy_to_filters(table: &mut toml::map::Map) { None => toml::map::Map::new(), }; table.remove("filters"); - normalize_table_keys(&mut filters, str::to_ascii_lowercase); for (field, action) in [("exclude", "exclude"), ("include_only", "include")] { let Some(TomlValue::Array(patterns)) = table.get(field).cloned() else { continue; @@ -96,7 +97,7 @@ fn convert_legacy_to_filters(table: &mut toml::map::Map) { for pattern in patterns { if let TomlValue::String(pattern) = pattern { filters - .entry(pattern.to_ascii_lowercase()) + .entry(pattern) .or_insert_with(|| TomlValue::String(action.to_string())); } } @@ -107,11 +108,10 @@ fn convert_legacy_to_filters(table: &mut toml::map::Map) { } fn convert_filters_to_legacy(table: &mut toml::map::Map) { - let Some(TomlValue::Table(mut filters)) = table.get("filters").cloned() else { + let Some(TomlValue::Table(filters)) = table.get("filters").cloned() else { return; }; table.remove("filters"); - normalize_table_keys(&mut filters, str::to_ascii_lowercase); for (pattern, action) in filters { match action.as_str() { Some("exclude") => push_legacy_pattern(table, "exclude", "include_only", pattern), @@ -134,10 +134,10 @@ fn push_legacy_pattern( let Some(items) = items.as_array_mut() else { return; }; - if !items.iter().any(|item| { - item.as_str() - .is_some_and(|candidate| candidate.eq_ignore_ascii_case(&pattern)) - }) { + if !items + .iter() + .any(|item| item.as_str().is_some_and(|candidate| candidate == pattern)) + { items.push(TomlValue::String(pattern)); } } @@ -154,38 +154,23 @@ fn remove_patterns_from_legacy_array( return; } items.retain(|item| { - item.as_str().is_none_or(|candidate| { - !patterns - .iter() - .any(|pattern| candidate.eq_ignore_ascii_case(pattern)) - }) + item.as_str() + .is_none_or(|candidate| !patterns.iter().any(|pattern| candidate == *pattern)) }); } -/// Canonicalizes keys for maps whose keys compare independently of TOML's -/// case-sensitive key semantics, so equivalent entries collide before merging. -fn normalize_merge_table_keys(path: &[String], table: &mut toml::map::Map) { - match path { +fn is_permission_network_domains_path(path: &[String]) -> bool { + matches!( + path, [permissions, _, network, domains] - if permissions == "permissions" && network == "network" && domains == "domains" => - { - normalize_table_keys(table, normalize_host); - } - [policy, filters] if policy == "shell_environment_policy" && filters == "filters" => { - // Environment-variable patterns compare case-insensitively. - normalize_table_keys(table, str::to_ascii_lowercase); - } - _ => {} - } + if permissions == "permissions" && network == "network" && domains == "domains" + ) } -fn normalize_table_keys( - table: &mut toml::map::Map, - normalize_key: impl Fn(&str) -> String, -) { +fn normalize_network_domain_keys(table: &mut toml::map::Map) { let entries = std::mem::take(table); - for (key, value) in entries { - table.insert(normalize_key(&key), value); + for (pattern, value) in entries { + table.insert(normalize_host(&pattern), value); } } diff --git a/codex-rs/config/src/merge_tests.rs b/codex-rs/config/src/merge_tests.rs index 1e41512d16c9..7ccb40c1bca0 100644 --- a/codex-rs/config/src/merge_tests.rs +++ b/codex-rs/config/src/merge_tests.rs @@ -146,7 +146,7 @@ exclude = ["HIGH_*"] } #[test] -fn shell_environment_policy_filters_overlay_merges_by_key_case_insensitively() { +fn shell_environment_policy_filters_overlay_merges_by_exact_key() { let mut base = parse_toml( r#" [shell_environment_policy.filters] @@ -169,9 +169,10 @@ fn shell_environment_policy_filters_overlay_merges_by_key_case_insensitively() { parse_toml( r#" [shell_environment_policy.filters] -"add_*" = "exclude" +"ADD_*" = "exclude" +"FLIP_*" = "exclude" +"KEEP_*" = "include" "flip_*" = "include" -"keep_*" = "include" "#, ) ); @@ -202,11 +203,11 @@ include_only = ["FLIP_TO_EXCLUDE", "KEEP_INCLUDED"] parse_toml( r#" [shell_environment_policy.filters] -"add_included" = "include" -"flip_to_exclude" = "exclude" -"flip_to_include" = "include" -"keep_excluded" = "exclude" -"keep_included" = "include" +"ADD_INCLUDED" = "include" +"FLIP_TO_EXCLUDE" = "exclude" +"FLIP_TO_INCLUDE" = "include" +"KEEP_EXCLUDED" = "exclude" +"KEEP_INCLUDED" = "include" "#, ) ); @@ -237,7 +238,7 @@ exclude = ["FLIP_TO_EXCLUDE", "HIGH_EXCLUDED"] r#" [shell_environment_policy] exclude = ["FLIP_TO_EXCLUDE", "HIGH_EXCLUDED"] -include_only = ["keep_included"] +include_only = ["KEEP_INCLUDED"] "#, ) ); diff --git a/codex-rs/config/src/types.rs b/codex-rs/config/src/types.rs index 5f32670ab83c..81908de6f274 100644 --- a/codex-rs/config/src/types.rs +++ b/codex-rs/config/src/types.rs @@ -949,6 +949,8 @@ pub struct ShellEnvironmentPolicyToml { /// Ordinary config keeps accepting the legacy arrays above during the /// migration. Requirements will accept only this keyed form, keeping array /// compatibility isolated so the legacy fields can be deprecated later. + /// Pattern keys merge case-sensitively across config layers even though the + /// resulting patterns match environment variable names case-insensitively. pub filters: Option>, pub experimental_use_profile: Option, diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index 9b514f8d9ee7..8fe1275221dc 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -445,12 +445,11 @@ future_field = true } #[tokio::test] -async fn non_strict_config_rejects_duplicate_shell_environment_policy_filters_before_merging() { +async fn non_strict_config_rejects_shell_filter_case_variants_across_layers() { let tmp = tempdir().expect("tempdir"); let contents = r#" [shell_environment_policy.filters] "SECRET_TOKEN" = "exclude" -"secret_token" = "include" "#; std::fs::write(tmp.path().join(CONFIG_TOML_FILE), contents).expect("write config"); @@ -459,13 +458,13 @@ async fn non_strict_config_rejects_duplicate_shell_environment_policy_filters_be .fallback_cwd(Some(tmp.path().to_path_buf())) .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) .cli_overrides(vec![( - "shell_environment_policy.filters.PATH".to_string(), + "shell_environment_policy.filters.secret_token".to_string(), TomlValue::String("include".to_string()), )]) .strict_config(/*strict_config*/ false) .build() .await - .expect_err("case-insensitive duplicate filters should be rejected per layer"); + .expect_err("case variants across layers should be rejected after merging"); assert!( err.to_string() From 97d10706a087506939d8244cb73dc22fa6e0dcbc Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 17 Jun 2026 00:41:04 +0000 Subject: [PATCH 11/12] Fix shell filter CI checks --- codex-rs/config/src/merge.rs | 2 +- codex-rs/core/config.schema.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index 4fdb81c2f17e..fad8a81c2d68 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -155,7 +155,7 @@ fn remove_patterns_from_legacy_array( } items.retain(|item| { item.as_str() - .is_none_or(|candidate| !patterns.iter().any(|pattern| candidate == *pattern)) + .is_none_or(|candidate| !patterns.contains(&candidate)) }); } diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 0bbcbea13160..9735c4879ad5 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2646,7 +2646,7 @@ "additionalProperties": { "$ref": "#/definitions/ShellEnvironmentPolicyFilter" }, - "description": "Pattern actions used by the canonical table representation.\n\nOrdinary config keeps accepting the legacy arrays above during the migration. Requirements will accept only this keyed form, keeping array compatibility isolated so the legacy fields can be deprecated later.", + "description": "Pattern actions used by the canonical table representation.\n\nOrdinary config keeps accepting the legacy arrays above during the migration. Requirements will accept only this keyed form, keeping array compatibility isolated so the legacy fields can be deprecated later. Pattern keys merge case-sensitively across config layers even though the resulting patterns match environment variable names case-insensitively.", "type": "object" }, "ignore_default_excludes": { From 1a2dbaac61864a515d724b1b3022e626889602c4 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 17 Jun 2026 20:38:23 +0000 Subject: [PATCH 12/12] Simplify shell filter representation merges --- codex-rs/config/src/merge.rs | 116 +++-------------------------- codex-rs/config/src/merge_tests.rs | 15 ++-- 2 files changed, 19 insertions(+), 112 deletions(-) diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index fad8a81c2d68..a3152f1f8880 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -9,7 +9,7 @@ pub fn merge_toml_values(base: &mut TomlValue, overlay: &TomlValue) { } fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &mut Vec) { - reconcile_shell_environment_policy_representations(base, overlay, path); + replace_shell_environment_policy_filter_representation(base, overlay, path); if let TomlValue::Table(overlay_table) = overlay && let TomlValue::Table(base_table) = base @@ -36,11 +36,9 @@ fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &m } } -/// Ordinary config keeps accepting legacy arrays while `filters` is the -/// canonical keyed form. Reconcile the two shapes only at this boundary so -/// layer precedence remains correct and array compatibility can be removed -/// cleanly after the migration. -fn reconcile_shell_environment_policy_representations( +/// Switching between legacy arrays and keyed filters replaces lower filter +/// fields instead of attempting to reconcile the two representations. +fn replace_shell_environment_policy_filter_representation( base: &mut TomlValue, overlay: &TomlValue, path: &[String], @@ -55,108 +53,12 @@ fn reconcile_shell_environment_policy_representations( return; }; - let overlay_has_filters = overlay.contains_key("filters"); - let overlay_has_legacy = - overlay.contains_key("exclude") || overlay.contains_key("include_only"); - if overlay_has_filters && !overlay_has_legacy { - convert_legacy_to_filters(base); - } else if overlay_has_legacy && !overlay_has_filters { - convert_filters_to_legacy(base); + if overlay.contains_key("filters") { + base.remove("exclude"); + base.remove("include_only"); + } else if overlay.contains_key("exclude") || overlay.contains_key("include_only") { + base.remove("filters"); } - - for (legacy_field, opposite_field) in [("exclude", "include_only"), ("include_only", "exclude")] - { - let Some(patterns) = overlay - .get(legacy_field) - .and_then(TomlValue::as_array) - .and_then(|items| { - items - .iter() - .map(TomlValue::as_str) - .collect::>>() - }) - else { - continue; - }; - remove_patterns_from_legacy_array(base, opposite_field, &patterns); - } -} - -fn convert_legacy_to_filters(table: &mut toml::map::Map) { - let mut filters = match table.get("filters") { - Some(TomlValue::Table(filters)) => filters.clone(), - Some(_) => return, - None => toml::map::Map::new(), - }; - table.remove("filters"); - for (field, action) in [("exclude", "exclude"), ("include_only", "include")] { - let Some(TomlValue::Array(patterns)) = table.get(field).cloned() else { - continue; - }; - table.remove(field); - for pattern in patterns { - if let TomlValue::String(pattern) = pattern { - filters - .entry(pattern) - .or_insert_with(|| TomlValue::String(action.to_string())); - } - } - } - if !filters.is_empty() { - table.insert("filters".to_string(), TomlValue::Table(filters)); - } -} - -fn convert_filters_to_legacy(table: &mut toml::map::Map) { - let Some(TomlValue::Table(filters)) = table.get("filters").cloned() else { - return; - }; - table.remove("filters"); - for (pattern, action) in filters { - match action.as_str() { - Some("exclude") => push_legacy_pattern(table, "exclude", "include_only", pattern), - Some("include") => push_legacy_pattern(table, "include_only", "exclude", pattern), - _ => {} - } - } -} - -fn push_legacy_pattern( - table: &mut toml::map::Map, - field: &str, - opposite_field: &str, - pattern: String, -) { - remove_patterns_from_legacy_array(table, opposite_field, &[pattern.as_str()]); - let items = table - .entry(field.to_string()) - .or_insert_with(|| TomlValue::Array(Vec::new())); - let Some(items) = items.as_array_mut() else { - return; - }; - if !items - .iter() - .any(|item| item.as_str().is_some_and(|candidate| candidate == pattern)) - { - items.push(TomlValue::String(pattern)); - } -} - -fn remove_patterns_from_legacy_array( - table: &mut toml::map::Map, - field: &str, - patterns: &[&str], -) { - let Some(items) = table.get_mut(field).and_then(TomlValue::as_array_mut) else { - return; - }; - if !items.iter().all(|item| item.as_str().is_some()) { - return; - } - items.retain(|item| { - item.as_str() - .is_none_or(|candidate| !patterns.contains(&candidate)) - }); } fn is_permission_network_domains_path(path: &[String]) -> bool { diff --git a/codex-rs/config/src/merge_tests.rs b/codex-rs/config/src/merge_tests.rs index 7ccb40c1bca0..500222d879a9 100644 --- a/codex-rs/config/src/merge_tests.rs +++ b/codex-rs/config/src/merge_tests.rs @@ -179,10 +179,11 @@ fn shell_environment_policy_filters_overlay_merges_by_exact_key() { } #[test] -fn shell_environment_policy_filters_override_lower_legacy_arrays_by_pattern() { +fn shell_environment_policy_filters_replace_lower_legacy_filter_fields() { let mut base = parse_toml( r#" [shell_environment_policy] +inherit = "core" exclude = ["FLIP_TO_INCLUDE", "KEEP_EXCLUDED"] include_only = ["FLIP_TO_EXCLUDE", "KEEP_INCLUDED"] "#, @@ -202,21 +203,25 @@ include_only = ["FLIP_TO_EXCLUDE", "KEEP_INCLUDED"] base, parse_toml( r#" +[shell_environment_policy] +inherit = "core" + [shell_environment_policy.filters] "ADD_INCLUDED" = "include" "FLIP_TO_EXCLUDE" = "exclude" "FLIP_TO_INCLUDE" = "include" -"KEEP_EXCLUDED" = "exclude" -"KEEP_INCLUDED" = "include" "#, ) ); } #[test] -fn shell_environment_policy_legacy_array_replaces_lower_filters_for_its_action() { +fn shell_environment_policy_legacy_arrays_replace_lower_filters() { let mut base = parse_toml( r#" +[shell_environment_policy] +inherit = "core" + [shell_environment_policy.filters] "FLIP_TO_EXCLUDE" = "include" "LOW_EXCLUDED" = "exclude" @@ -237,8 +242,8 @@ exclude = ["FLIP_TO_EXCLUDE", "HIGH_EXCLUDED"] parse_toml( r#" [shell_environment_policy] +inherit = "core" exclude = ["FLIP_TO_EXCLUDE", "HIGH_EXCLUDED"] -include_only = ["KEEP_INCLUDED"] "#, ) );