From 4f7d6b4ef75425e8ad6c7d50b7632be10e1acbcb Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 26 May 2026 10:34:43 +0200 Subject: [PATCH] chore: stop consuming legacy config profiles (#24076) ## Why The old config-profile mechanism should no longer influence runtime behavior now that profile selection has moved to file-based `--profile` config files. Core already rejects a selected legacy `profile = "..."` with a migration error in [`core/src/config/mod.rs`](https://github.com/openai/codex/blob/d6451fcb79edc4a71bc9e811bcda06fd3c36562e/codex-rs/core/src/config/mod.rs#L2521-L2529), but a few residual consumers still read legacy `[profiles.*]` data while performing managed-feature checks and personality migration. That kept dead legacy profile state relevant after selection had been removed, and could make personality migration depend on a stale or missing old profile. ## What changed - Stop scanning legacy `[profiles.*]` feature settings when validating managed feature requirements. - Make personality migration consider only top-level `personality` and `model_provider` settings. - Remove the now-unused `ConfigToml::get_config_profile` helper. - Update personality migration coverage to verify that legacy profile personality fields and missing legacy profile names no longer affect that migration path. This keeps the legacy `profile` / `profiles` config shape available for the remaining compatibility and migration diagnostics; it only removes these behavior consumers. ## Verification - Updated `core/tests/suite/personality_migration.rs` for the new legacy-profile behavior. - Focused test command: `cargo test -p codex-core personality_migration`. --- codex-rs/config/src/config_toml.rs | 21 ------ codex-rs/core/src/config/managed_features.rs | 74 +++---------------- codex-rs/core/src/personality_migration.rs | 9 +-- .../core/tests/suite/personality_migration.rs | 23 +++--- 4 files changed, 22 insertions(+), 105 deletions(-) diff --git a/codex-rs/config/src/config_toml.rs b/codex-rs/config/src/config_toml.rs index 34ae6a41b3..20a0769d83 100644 --- a/codex-rs/config/src/config_toml.rs +++ b/codex-rs/config/src/config_toml.rs @@ -821,27 +821,6 @@ impl ConfigToml { None } - - pub fn get_config_profile( - &self, - override_profile: Option, - ) -> Result { - let profile = override_profile.or_else(|| self.profile.clone()); - - match profile { - Some(key) => { - if let Some(profile) = self.profiles.get(key.as_str()) { - return Ok(profile.clone()); - } - - Err(std::io::Error::new( - std::io::ErrorKind::NotFound, - format!("config profile `{key}` not found"), - )) - } - None => Ok(ConfigProfile::default()), - } - } } /// Canonicalize the path and convert it to a string to be used as a key in the diff --git a/codex-rs/core/src/config/managed_features.rs b/codex-rs/core/src/config/managed_features.rs index 480225fb99..234a2835c6 100644 --- a/codex-rs/core/src/config/managed_features.rs +++ b/codex-rs/core/src/config/managed_features.rs @@ -9,7 +9,6 @@ use codex_config::RequirementSource; use codex_config::Sourced; use codex_config::config_toml::ConfigToml; -use codex_config::profile_toml::ConfigProfile; use codex_features::Feature; use codex_features::FeatureConfigSource; use codex_features::FeatureOverrides; @@ -269,27 +268,6 @@ fn explicit_feature_settings_in_config(cfg: &ConfigToml) -> Vec<(String, Feature enabled, )); } - for (profile_name, profile) in &cfg.profiles { - if let Some(features) = profile.features.as_ref() { - for (key, enabled) in features.entries() { - if let Some(feature) = feature_for_key(&key) { - explicit_settings.push(( - format!("profiles.{profile_name}.features.{key}"), - feature, - enabled, - )); - } - } - } - if let Some(enabled) = profile.experimental_use_unified_exec_tool { - explicit_settings.push(( - format!("profiles.{profile_name}.experimental_use_unified_exec_tool"), - Feature::UnifiedExec, - enabled, - )); - } - } - explicit_settings } @@ -339,47 +317,13 @@ pub(crate) fn validate_feature_requirements_in_config_toml( cfg: &ConfigToml, feature_requirements: Option<&Sourced>, ) -> std::io::Result<()> { - fn validate_profile( - cfg: &ConfigToml, - profile_name: Option<&str>, - profile: &ConfigProfile, - feature_requirements: Option<&Sourced>, - ) -> std::io::Result<()> { - let configured_features = Features::from_sources( - FeatureConfigSource { - features: cfg.features.as_ref(), - experimental_use_unified_exec_tool: cfg.experimental_use_unified_exec_tool, - }, - FeatureConfigSource { - features: profile.features.as_ref(), - experimental_use_unified_exec_tool: profile.experimental_use_unified_exec_tool, - }, - FeatureOverrides::default(), - ); - ManagedFeatures::from_configured(configured_features, feature_requirements.cloned()) - .map(|_| ()) - .map_err(|err| { - if let Some(profile_name) = profile_name { - std::io::Error::new( - err.kind(), - format!( - "invalid feature configuration for profile `{profile_name}`: {err}" - ), - ) - } else { - err - } - }) - } - - validate_profile( - cfg, - /*profile_name*/ None, - &ConfigProfile::default(), - feature_requirements, - )?; - for (profile_name, profile) in &cfg.profiles { - validate_profile(cfg, Some(profile_name), profile, feature_requirements)?; - } - Ok(()) + let configured_features = Features::from_sources( + FeatureConfigSource { + features: cfg.features.as_ref(), + experimental_use_unified_exec_tool: cfg.experimental_use_unified_exec_tool, + }, + FeatureConfigSource::default(), + FeatureOverrides::default(), + ); + ManagedFeatures::from_configured(configured_features, feature_requirements.cloned()).map(|_| ()) } diff --git a/codex-rs/core/src/personality_migration.rs b/codex-rs/core/src/personality_migration.rs index 975aecd4af..7109b58a4a 100644 --- a/codex-rs/core/src/personality_migration.rs +++ b/codex-rs/core/src/personality_migration.rs @@ -32,17 +32,14 @@ pub async fn maybe_migrate_personality( return Ok(PersonalityMigrationStatus::SkippedMarker); } - let config_profile = config_toml - .get_config_profile(/*override_profile*/ None) - .map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err))?; - if config_toml.personality.is_some() || config_profile.personality.is_some() { + if config_toml.personality.is_some() { create_marker(&marker_path).await?; return Ok(PersonalityMigrationStatus::SkippedExplicitPersonality); } - let model_provider_id = config_profile + let model_provider_id = config_toml .model_provider - .or_else(|| config_toml.model_provider.clone()) + .clone() .unwrap_or_else(|| "openai".to_string()); if !has_recorded_sessions(codex_home, model_provider_id.as_str(), state_db).await? { diff --git a/codex-rs/core/tests/suite/personality_migration.rs b/codex-rs/core/tests/suite/personality_migration.rs index 5151d4e55d..7c9183f8d9 100644 --- a/codex-rs/core/tests/suite/personality_migration.rs +++ b/codex-rs/core/tests/suite/personality_migration.rs @@ -253,7 +253,7 @@ async fn no_marker_explicit_global_personality_skips_migration() -> io::Result<( } #[tokio::test] -async fn no_marker_profile_personality_skips_migration() -> io::Result<()> { +async fn no_marker_profile_personality_does_not_skip_migration() -> io::Result<()> { let temp = TempDir::new()?; write_session_with_user_event(temp.path()).await?; let config_toml = parse_config_toml( @@ -267,23 +267,22 @@ personality = "friendly" let status = maybe_migrate_personality(temp.path(), &config_toml, /*state_db*/ None).await?; - assert_eq!( - status, - PersonalityMigrationStatus::SkippedExplicitPersonality - ); + assert_eq!(status, PersonalityMigrationStatus::Applied); assert_eq!( tokio::fs::try_exists(temp.path().join(PERSONALITY_MIGRATION_FILENAME)).await?, true ); assert_eq!( tokio::fs::try_exists(temp.path().join("config.toml")).await?, - false + true ); + let persisted = read_config_toml(temp.path()).await?; + assert_eq!(persisted.personality, Some(Personality::Pragmatic)); Ok(()) } #[tokio::test] -async fn marker_short_circuits_invalid_profile_resolution() -> io::Result<()> { +async fn marker_short_circuits_migration_with_legacy_profile() -> io::Result<()> { let temp = TempDir::new()?; tokio::fs::write(temp.path().join(PERSONALITY_MIGRATION_FILENAME), "v1\n").await?; let config_toml = parse_config_toml("profile = \"missing\"\n")?; @@ -295,18 +294,16 @@ async fn marker_short_circuits_invalid_profile_resolution() -> io::Result<()> { } #[tokio::test] -async fn invalid_selected_profile_returns_error_and_does_not_write_marker() -> io::Result<()> { +async fn missing_legacy_profile_does_not_block_migration() -> io::Result<()> { let temp = TempDir::new()?; let config_toml = parse_config_toml("profile = \"missing\"\n")?; - let err = maybe_migrate_personality(temp.path(), &config_toml, /*state_db*/ None) - .await - .expect_err("missing profile should fail"); + let status = maybe_migrate_personality(temp.path(), &config_toml, /*state_db*/ None).await?; - assert_eq!(err.kind(), io::ErrorKind::InvalidData); + assert_eq!(status, PersonalityMigrationStatus::SkippedNoSessions); assert_eq!( tokio::fs::try_exists(temp.path().join(PERSONALITY_MIGRATION_FILENAME)).await?, - false + true ); Ok(()) }