mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
feat: Warn and continue on unknown feature requirements (#19038)
Requirements feature flags now fail open like config feature flags, but with a startup warning. <img width="443" height="68" alt="image" src="https://github.com/user-attachments/assets/76767fa7-8ce8-4fc7-8a09-902fcdda6298" />
This commit is contained in:
committed by
GitHub
Unverified
parent
fb6308cf64
commit
951be1a8a1
@@ -7070,10 +7070,10 @@ async fn feature_requirements_normalize_runtime_feature_mutations() -> std::io::
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn feature_requirements_reject_collab_legacy_alias() {
|
||||
let codex_home = TempDir::new().expect("tempdir");
|
||||
async fn feature_requirements_warn_on_collab_legacy_alias() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
let err = ConfigBuilder::default()
|
||||
let config = ConfigBuilder::without_managed_config_for_tests()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.cloud_requirements(CloudRequirementsLoader::new(async {
|
||||
Ok(Some(crate::config_loader::ConfigRequirementsToml {
|
||||
@@ -7084,15 +7084,49 @@ async fn feature_requirements_reject_collab_legacy_alias() {
|
||||
}))
|
||||
}))
|
||||
.build()
|
||||
.await
|
||||
.expect_err("legacy aliases should be rejected");
|
||||
.await?;
|
||||
|
||||
assert_eq!(err.kind(), std::io::ErrorKind::InvalidData);
|
||||
assert!(config.features.enabled(Feature::Collab));
|
||||
assert!(
|
||||
err.to_string()
|
||||
.contains("use canonical feature key `multi_agent`"),
|
||||
"{err}"
|
||||
config.startup_warnings.iter().any(|warning| {
|
||||
warning.contains("Using legacy `features` requirement `collab`")
|
||||
&& warning.contains("prefer canonical feature key `multi_agent`")
|
||||
}),
|
||||
"{:?}",
|
||||
config.startup_warnings
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn feature_requirements_warn_and_ignore_unknown_feature() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
let config = ConfigBuilder::without_managed_config_for_tests()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.cloud_requirements(CloudRequirementsLoader::new(async {
|
||||
Ok(Some(crate::config_loader::ConfigRequirementsToml {
|
||||
feature_requirements: Some(crate::config_loader::FeatureRequirementsToml {
|
||||
entries: BTreeMap::from([("made_up_feature".to_string(), true)]),
|
||||
}),
|
||||
..Default::default()
|
||||
}))
|
||||
}))
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert!(
|
||||
config
|
||||
.startup_warnings
|
||||
.iter()
|
||||
.any(|warning| warning
|
||||
.contains("Ignoring unknown `features` requirement `made_up_feature`")),
|
||||
"{:?}",
|
||||
config.startup_warnings
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -31,13 +31,37 @@ impl ManagedFeatures {
|
||||
pub(crate) fn from_configured(
|
||||
configured_features: Features,
|
||||
feature_requirements: Option<Sourced<FeatureRequirementsToml>>,
|
||||
) -> std::io::Result<Self> {
|
||||
Self::from_configured_with_optional_warnings(
|
||||
configured_features,
|
||||
feature_requirements,
|
||||
/*startup_warnings*/ None,
|
||||
)
|
||||
}
|
||||
|
||||
pub(crate) fn from_configured_with_warnings(
|
||||
configured_features: Features,
|
||||
feature_requirements: Option<Sourced<FeatureRequirementsToml>>,
|
||||
startup_warnings: &mut Vec<String>,
|
||||
) -> std::io::Result<Self> {
|
||||
Self::from_configured_with_optional_warnings(
|
||||
configured_features,
|
||||
feature_requirements,
|
||||
Some(startup_warnings),
|
||||
)
|
||||
}
|
||||
|
||||
fn from_configured_with_optional_warnings(
|
||||
configured_features: Features,
|
||||
feature_requirements: Option<Sourced<FeatureRequirementsToml>>,
|
||||
startup_warnings: Option<&mut Vec<String>>,
|
||||
) -> std::io::Result<Self> {
|
||||
let (pinned_features, source) = match feature_requirements {
|
||||
Some(Sourced {
|
||||
value: feature_requirements,
|
||||
source,
|
||||
}) => (
|
||||
parse_feature_requirements(feature_requirements, &source)?,
|
||||
parse_feature_requirements(feature_requirements, &source, startup_warnings),
|
||||
Some(source),
|
||||
),
|
||||
None => (BTreeMap::new(), None),
|
||||
@@ -171,7 +195,8 @@ fn feature_requirements_display(feature_requirements: &BTreeMap<Feature, bool>)
|
||||
fn parse_feature_requirements(
|
||||
feature_requirements: FeatureRequirementsToml,
|
||||
source: &RequirementSource,
|
||||
) -> std::io::Result<BTreeMap<Feature, bool>> {
|
||||
mut startup_warnings: Option<&mut Vec<String>>,
|
||||
) -> BTreeMap<Feature, bool> {
|
||||
let mut pinned_features = BTreeMap::new();
|
||||
for (key, enabled) in feature_requirements.entries {
|
||||
if let Some(feature) = canonical_feature_for_key(&key) {
|
||||
@@ -180,22 +205,34 @@ fn parse_feature_requirements(
|
||||
}
|
||||
|
||||
if let Some(feature) = feature_for_key(&key) {
|
||||
return Err(std::io::Error::new(
|
||||
std::io::ErrorKind::InvalidData,
|
||||
push_feature_requirement_warning(
|
||||
&mut startup_warnings,
|
||||
format!(
|
||||
"invalid `features` requirement `{key}` from {source}: use canonical feature key `{}`",
|
||||
"Using legacy `features` requirement `{key}` from {source}; prefer canonical feature key `{}`",
|
||||
feature.key()
|
||||
),
|
||||
));
|
||||
);
|
||||
pinned_features.insert(feature, enabled);
|
||||
continue;
|
||||
}
|
||||
|
||||
return Err(std::io::Error::new(
|
||||
std::io::ErrorKind::InvalidData,
|
||||
format!("invalid `features` requirement `{key}` from {source}"),
|
||||
));
|
||||
push_feature_requirement_warning(
|
||||
&mut startup_warnings,
|
||||
format!("Ignoring unknown `features` requirement `{key}` from {source}"),
|
||||
);
|
||||
}
|
||||
|
||||
Ok(pinned_features)
|
||||
pinned_features
|
||||
}
|
||||
|
||||
fn push_feature_requirement_warning(
|
||||
startup_warnings: &mut Option<&mut Vec<String>>,
|
||||
message: String,
|
||||
) {
|
||||
tracing::warn!("{message}");
|
||||
if let Some(startup_warnings) = startup_warnings.as_deref_mut() {
|
||||
startup_warnings.push(message);
|
||||
}
|
||||
}
|
||||
|
||||
fn explicit_feature_settings_in_config(cfg: &ConfigToml) -> Vec<(String, Feature, bool)> {
|
||||
@@ -272,7 +309,11 @@ pub(crate) fn validate_explicit_feature_settings_in_config_toml(
|
||||
return Ok(());
|
||||
};
|
||||
|
||||
let pinned_features = parse_feature_requirements(feature_requirements.clone(), source)?;
|
||||
let pinned_features = parse_feature_requirements(
|
||||
feature_requirements.clone(),
|
||||
source,
|
||||
/*startup_warnings*/ None,
|
||||
);
|
||||
if pinned_features.is_empty() {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
@@ -1666,7 +1666,11 @@ impl Config {
|
||||
},
|
||||
feature_overrides,
|
||||
);
|
||||
let features = ManagedFeatures::from_configured(configured_features, feature_requirements)?;
|
||||
let features = ManagedFeatures::from_configured_with_warnings(
|
||||
configured_features,
|
||||
feature_requirements,
|
||||
&mut startup_warnings,
|
||||
)?;
|
||||
let windows_sandbox_mode = resolve_windows_sandbox_mode(&cfg, &config_profile);
|
||||
let windows_sandbox_private_desktop =
|
||||
resolve_windows_sandbox_private_desktop(&cfg, &config_profile);
|
||||
|
||||
Reference in New Issue
Block a user