mirror of
https://github.com/openai/codex.git
synced 2026-09-14 11:57:03 +00:00
Tighten managed feature requirement coverage
This commit is contained in:
@@ -817,6 +817,9 @@ where
|
||||
fn apply_requirement_feature_constraints(
|
||||
features: &mut Features,
|
||||
requirement_features: Option<&Sourced<RequirementsFeaturesToml>>,
|
||||
cfg: &ConfigToml,
|
||||
config_profile: &ConfigProfile,
|
||||
feature_overrides: &FeatureOverrides,
|
||||
startup_warnings: &mut Vec<String>,
|
||||
) -> 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");
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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<Feature> {
|
||||
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<String>) {
|
||||
|
||||
Reference in New Issue
Block a user