From 6afcc5bebb784957bac74728113e249b7102e554 Mon Sep 17 00:00:00 2001 From: daniel-oai Date: Tue, 3 Mar 2026 13:41:49 -0800 Subject: [PATCH] Tighten managed feature requirement coverage --- codex-rs/core/src/config/mod.rs | 135 ++++++++++++++++++++++- codex-rs/core/src/config_loader/tests.rs | 35 +++++- codex-rs/core/src/features.rs | 12 +- 3 files changed, 173 insertions(+), 9 deletions(-) diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 6a1ef1b2da..53095dc03b 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -817,6 +817,9 @@ where fn apply_requirement_feature_constraints( features: &mut Features, requirement_features: Option<&Sourced>, + cfg: &ConfigToml, + config_profile: &ConfigProfile, + feature_overrides: &FeatureOverrides, startup_warnings: &mut Vec, ) -> std::io::Result<()> { let Some(requirement_features) = requirement_features else { @@ -865,7 +868,9 @@ fn apply_requirement_feature_constraints( )); } - if features.enabled(spec.id) { + if features.enabled(spec.id) + && feature_is_explicitly_enabled(spec.id, cfg, config_profile, feature_overrides) + { startup_warnings.push(format!( "Configured value for `features.{key}` is disallowed by requirements; forcing false." )); @@ -873,9 +878,63 @@ fn apply_requirement_feature_constraints( features.disable(spec.id); } + features.normalize_dependencies(); + Ok(()) } +fn feature_is_explicitly_enabled( + feature: Feature, + cfg: &ConfigToml, + config_profile: &ConfigProfile, + feature_overrides: &FeatureOverrides, +) -> bool { + feature_map_explicitly_enables(cfg.features.as_ref(), feature) + || feature_map_explicitly_enables(config_profile.features.as_ref(), feature) + || legacy_feature_setting_explicitly_enables( + feature, + cfg, + config_profile, + feature_overrides, + ) +} + +fn feature_map_explicitly_enables(features: Option<&FeaturesToml>, feature: Feature) -> bool { + features.is_some_and(|features| { + features.entries.iter().any(|(key, enabled)| { + *enabled + && (canonical_feature_spec(key).map(|spec| spec.id) == Some(feature) + || canonical_feature_for_alias(key) == Some(feature)) + }) + }) +} + +fn legacy_feature_setting_explicitly_enables( + feature: Feature, + cfg: &ConfigToml, + config_profile: &ConfigProfile, + feature_overrides: &FeatureOverrides, +) -> bool { + match feature { + Feature::ApplyPatchFreeform => { + config_profile.include_apply_patch_tool == Some(true) + || cfg.experimental_use_freeform_apply_patch == Some(true) + || config_profile.experimental_use_freeform_apply_patch == Some(true) + || feature_overrides.include_apply_patch_tool == Some(true) + } + Feature::UnifiedExec => { + cfg.experimental_use_unified_exec_tool == Some(true) + || config_profile.experimental_use_unified_exec_tool == Some(true) + } + Feature::WebSearchRequest => { + cfg.tools.as_ref().and_then(|tools| tools.web_search) == Some(true) + || config_profile.tools_web_search == Some(true) + || feature_overrides.web_search_request == Some(true) + } + _ => false, + } +} + fn mcp_server_matches_requirement( requirement: &McpServerRequirement, server: &McpServerConfig, @@ -1805,10 +1864,13 @@ impl Config { web_search_request: override_tools_web_search_request, }; - let mut features = Features::from_config(&cfg, &config_profile, feature_overrides); + let mut features = Features::from_config(&cfg, &config_profile, feature_overrides.clone()); apply_requirement_feature_constraints( &mut features, requirements.features.as_ref(), + &cfg, + &config_profile, + &feature_overrides, &mut startup_warnings, )?; let windows_sandbox_mode = resolve_windows_sandbox_mode(&cfg, &config_profile); @@ -6309,8 +6371,40 @@ speaker = "Desk Speakers" } #[tokio::test] - async fn managed_feature_requirements_disable_unified_exec_with_warning() -> std::io::Result<()> - { + async fn managed_feature_requirements_disable_default_feature_without_warning() + -> std::io::Result<()> { + let codex_home = TempDir::new()?; + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .cloud_requirements(CloudRequirementsLoader::new(async { + Ok(Some(crate::config_loader::ConfigRequirementsToml { + features: Some(crate::config_loader::RequirementsFeaturesToml { + entries: [("unified_exec".to_string(), false)].into_iter().collect(), + }), + ..Default::default() + })) + })) + .build() + .await?; + + assert!(!config.features.enabled(Feature::UnifiedExec)); + assert!(!config.use_experimental_unified_exec_tool); + assert!( + !config + .startup_warnings + .iter() + .any(|warning| warning.contains("features.unified_exec")), + "{:?}", + config.startup_warnings + ); + + Ok(()) + } + + #[tokio::test] + async fn managed_feature_requirements_warn_on_explicit_local_override() -> std::io::Result<()> { let codex_home = TempDir::new()?; std::fs::write( codex_home.path().join(CONFIG_TOML_FILE), @@ -6346,6 +6440,39 @@ unified_exec = true Ok(()) } + #[tokio::test] + async fn managed_feature_requirements_re_normalize_feature_dependencies() -> std::io::Result<()> + { + let codex_home = TempDir::new()?; + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#" +[features] +js_repl = true +js_repl_tools_only = true +"#, + )?; + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .cloud_requirements(CloudRequirementsLoader::new(async { + Ok(Some(crate::config_loader::ConfigRequirementsToml { + features: Some(crate::config_loader::RequirementsFeaturesToml { + entries: [("js_repl".to_string(), false)].into_iter().collect(), + }), + ..Default::default() + })) + })) + .build() + .await?; + + assert!(!config.features.enabled(Feature::JsRepl)); + assert!(!config.features.enabled(Feature::JsReplToolsOnly)); + + Ok(()) + } + #[tokio::test] async fn managed_feature_requirements_reject_true_values() { let codex_home = TempDir::new().expect("tempdir"); diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index 1d2657ea04..a1eacb77fb 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -494,6 +494,9 @@ async fn load_requirements_toml_produces_expected_constraints() -> anyhow::Resul allowed_approval_policies = ["never", "on-request"] allowed_web_search_modes = ["cached"] enforce_residency = "us" + +[features] +unified_exec = false "#, ) .await?; @@ -515,6 +518,24 @@ enforce_residency = "us" .cloned(), Some(vec![crate::config_loader::WebSearchModeRequirement::Cached]) ); + assert_eq!( + config_requirements_toml + .features + .as_ref() + .map(|requirements| requirements.value.clone()), + Some(crate::config_loader::RequirementsFeaturesToml { + entries: [("unified_exec".to_string(), false)].into_iter().collect(), + }) + ); + assert_eq!( + config_requirements_toml + .features + .as_ref() + .map(|requirements| requirements.source.clone()), + Some(RequirementSource::SystemRequirementsToml { + file: AbsolutePathBuf::from_absolute_path(&requirements_file)?, + }) + ); let config_requirements: ConfigRequirements = config_requirements_toml.try_into()?; assert_eq!( config_requirements.approval_policy.value(), @@ -552,6 +573,15 @@ enforce_residency = "us" config_requirements.enforce_residency.value(), Some(crate::config_loader::ResidencyRequirement::Us) ); + assert_eq!( + config_requirements + .features + .as_ref() + .map(|requirements| requirements.value.clone()), + Some(crate::config_loader::RequirementsFeaturesToml { + entries: [("unified_exec".to_string(), false)].into_iter().collect(), + }) + ); Ok(()) } @@ -668,7 +698,9 @@ async fn load_config_layers_includes_cloud_requirements() -> anyhow::Result<()> allowed_approval_policies: Some(vec![AskForApproval::Never]), allowed_sandbox_modes: None, allowed_web_search_modes: None, - features: None, + features: Some(crate::config_loader::RequirementsFeaturesToml { + entries: [("unified_exec".to_string(), false)].into_iter().collect(), + }), mcp_servers: None, rules: None, enforce_residency: None, @@ -690,6 +722,7 @@ async fn load_config_layers_includes_cloud_requirements() -> anyhow::Result<()> layers.requirements_toml().allowed_approval_policies, expected.allowed_approval_policies ); + assert_eq!(layers.requirements_toml().features, expected.features); assert_eq!( layers .requirements() diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index 65d3c10ce2..2cef350c15 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -351,10 +351,7 @@ impl Features { } overrides.apply(&mut features); - if features.enabled(Feature::JsReplToolsOnly) && !features.enabled(Feature::JsRepl) { - tracing::warn!("js_repl_tools_only requires js_repl; disabling js_repl_tools_only"); - features.disable(Feature::JsReplToolsOnly); - } + features.normalize_dependencies(); features } @@ -362,6 +359,13 @@ impl Features { pub fn enabled_features(&self) -> Vec { self.enabled.iter().copied().collect() } + + pub(crate) fn normalize_dependencies(&mut self) { + if self.enabled(Feature::JsReplToolsOnly) && !self.enabled(Feature::JsRepl) { + tracing::warn!("js_repl_tools_only requires js_repl; disabling js_repl_tools_only"); + self.disable(Feature::JsReplToolsOnly); + } + } } fn legacy_usage_notice(alias: &str, feature: Feature) -> (String, Option) {