From 0ee7d7541f64a21b6f7e6e9eeb82cc09a21db66a Mon Sep 17 00:00:00 2001 From: xli-oai Date: Tue, 16 Jun 2026 19:53:16 -0700 Subject: [PATCH] codex: fail open on managed feature write conflicts --- .../src/config_manager_service_tests.rs | 60 +++++++++++++----- codex-rs/core/src/config/managed_features.rs | 62 ------------------- codex-rs/core/src/config/mod.rs | 3 +- 3 files changed, 47 insertions(+), 78 deletions(-) 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 5c6e3de5a1..382c37a629 100644 --- a/codex-rs/app-server/src/config_manager_service_tests.rs +++ b/codex-rs/app-server/src/config_manager_service_tests.rs @@ -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" ); } diff --git a/codex-rs/core/src/config/managed_features.rs b/codex-rs/core/src/config/managed_features.rs index 234a2835c6..19f817504a 100644 --- a/codex-rs/core/src/config/managed_features.rs +++ b/codex-rs/core/src/config/managed_features.rs @@ -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>, -) -> 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>, diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index ff666b51e9..f049c578f1 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -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>, ) -> 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) }