mirror of
https://github.com/openai/codex.git
synced 2026-10-29 19:08:43 +00:00
codex: fail open on managed feature write conflicts
This commit is contained in:
@@ -699,7 +699,7 @@ async fn reserved_builtin_provider_override_rejected() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn write_value_rejects_feature_requirement_conflict() {
|
||||
async fn write_value_allows_feature_requirement_conflict() {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
std::fs::write(tmp.path().join(CONFIG_TOML_FILE), "").unwrap();
|
||||
|
||||
@@ -717,7 +717,7 @@ async fn write_value_rejects_feature_requirement_conflict() {
|
||||
}),
|
||||
);
|
||||
|
||||
let error = service
|
||||
let response = service
|
||||
.write_value(ConfigValueWriteParams {
|
||||
file_path: Some(tmp.path().join(CONFIG_TOML_FILE).display().to_string()),
|
||||
key_path: "features.personality".to_string(),
|
||||
@@ -726,21 +726,53 @@ async fn write_value_rejects_feature_requirement_conflict() {
|
||||
expected_version: None,
|
||||
})
|
||||
.await
|
||||
.expect_err("conflicting feature write should fail");
|
||||
.expect("conflicting feature write should be persisted");
|
||||
|
||||
assert_eq!(
|
||||
error.write_error_code(),
|
||||
Some(ConfigWriteErrorCode::ConfigValidationError)
|
||||
);
|
||||
assert!(
|
||||
error
|
||||
.to_string()
|
||||
.contains("invalid value for `features`: `features.personality=false`"),
|
||||
"{error}"
|
||||
);
|
||||
assert_eq!(response.status, WriteStatus::Ok);
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(tmp.path().join(CONFIG_TOML_FILE)).unwrap(),
|
||||
""
|
||||
"[features]\npersonality = false\n"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn write_value_allows_unrelated_write_with_feature_requirement_conflict() {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
std::fs::write(
|
||||
tmp.path().join(CONFIG_TOML_FILE),
|
||||
"[features]\npersonality = false\n",
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let service = ConfigManager::new_for_tests(
|
||||
tmp.path().to_path_buf(),
|
||||
vec![],
|
||||
LoaderOverrides::without_managed_config_for_tests(),
|
||||
CloudRequirementsLoader::new(async {
|
||||
Ok(Some(ConfigRequirementsToml {
|
||||
feature_requirements: Some(FeatureRequirementsToml {
|
||||
entries: BTreeMap::from([("personality".to_string(), true)]),
|
||||
}),
|
||||
..Default::default()
|
||||
}))
|
||||
}),
|
||||
);
|
||||
|
||||
let response = service
|
||||
.write_value(ConfigValueWriteParams {
|
||||
file_path: Some(tmp.path().join(CONFIG_TOML_FILE).display().to_string()),
|
||||
key_path: "model".to_string(),
|
||||
value: serde_json::json!("gpt-5.4"),
|
||||
merge_strategy: MergeStrategy::Replace,
|
||||
expected_version: None,
|
||||
})
|
||||
.await
|
||||
.expect("unrelated write should not be blocked by feature conflict");
|
||||
|
||||
assert_eq!(response.status, WriteStatus::Ok);
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(tmp.path().join(CONFIG_TOML_FILE)).unwrap(),
|
||||
"model = \"gpt-5.4\"\n[features]\npersonality = false\n"
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -251,68 +251,6 @@ fn push_feature_requirement_warning(
|
||||
}
|
||||
}
|
||||
|
||||
fn explicit_feature_settings_in_config(cfg: &ConfigToml) -> Vec<(String, Feature, bool)> {
|
||||
let mut explicit_settings = Vec::new();
|
||||
|
||||
if let Some(features) = cfg.features.as_ref() {
|
||||
for (key, enabled) in features.entries() {
|
||||
if let Some(feature) = feature_for_key(&key) {
|
||||
explicit_settings.push((format!("features.{key}"), feature, enabled));
|
||||
}
|
||||
}
|
||||
}
|
||||
if let Some(enabled) = cfg.experimental_use_unified_exec_tool {
|
||||
explicit_settings.push((
|
||||
"experimental_use_unified_exec_tool".to_string(),
|
||||
Feature::UnifiedExec,
|
||||
enabled,
|
||||
));
|
||||
}
|
||||
explicit_settings
|
||||
}
|
||||
|
||||
pub(crate) fn validate_explicit_feature_settings_in_config_toml(
|
||||
cfg: &ConfigToml,
|
||||
feature_requirements: Option<&Sourced<FeatureRequirementsToml>>,
|
||||
) -> std::io::Result<()> {
|
||||
let Some(Sourced {
|
||||
value: feature_requirements,
|
||||
source,
|
||||
}) = feature_requirements
|
||||
else {
|
||||
return Ok(());
|
||||
};
|
||||
|
||||
let pinned_features = parse_feature_requirements(
|
||||
feature_requirements.clone(),
|
||||
source,
|
||||
/*startup_warnings*/ None,
|
||||
);
|
||||
if pinned_features.is_empty() {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let allowed = feature_requirements_display(&pinned_features);
|
||||
for (path, feature, enabled) in explicit_feature_settings_in_config(cfg) {
|
||||
if pinned_features
|
||||
.get(&feature)
|
||||
.is_some_and(|required| *required != enabled)
|
||||
{
|
||||
return Err(std::io::Error::new(
|
||||
std::io::ErrorKind::InvalidData,
|
||||
ConstraintError::InvalidValue {
|
||||
field_name: "features",
|
||||
candidate: format!("{path}={enabled}"),
|
||||
allowed,
|
||||
requirement_source: source.clone(),
|
||||
},
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) fn validate_feature_requirements_in_config_toml(
|
||||
cfg: &ConfigToml,
|
||||
feature_requirements: Option<&Sourced<FeatureRequirementsToml>>,
|
||||
|
||||
@@ -1614,12 +1614,11 @@ pub fn deserialize_config_toml_with_base(
|
||||
.map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))
|
||||
}
|
||||
|
||||
/// Validate user-visible feature settings against managed feature requirements.
|
||||
/// Validate managed feature requirements that cannot be normalized at runtime.
|
||||
pub fn validate_feature_requirements_for_config_toml(
|
||||
cfg: &ConfigToml,
|
||||
feature_requirements: Option<&Sourced<FeatureRequirementsToml>>,
|
||||
) -> std::io::Result<()> {
|
||||
managed_features::validate_explicit_feature_settings_in_config_toml(cfg, feature_requirements)?;
|
||||
managed_features::validate_feature_requirements_in_config_toml(cfg, feature_requirements)
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user