mirror of
https://github.com/openai/codex.git
synced 2026-11-12 22:42:36 +00:00
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`.
This commit is contained in:
1 parent
e8651516f4
commit
4f7d6b4ef7
4 files changed
+22
-105
No files matched your search
@@ -821,27 +821,6 @@ impl ConfigToml {
|
|||||||
|
|
||||||
None
|
None
|
||||||
}
|
}
|
||||||
|
|
||||||
pub fn get_config_profile(
|
|
||||||
&self,
|
|
||||||
override_profile: Option<String>,
|
|
||||||
) -> Result<ConfigProfile, std::io::Error> {
|
|
||||||
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
|
/// Canonicalize the path and convert it to a string to be used as a key in the
|
||||||
|
|||||||
@@ -9,7 +9,6 @@ use codex_config::RequirementSource;
|
|||||||
use codex_config::Sourced;
|
use codex_config::Sourced;
|
||||||
|
|
||||||
use codex_config::config_toml::ConfigToml;
|
use codex_config::config_toml::ConfigToml;
|
||||||
use codex_config::profile_toml::ConfigProfile;
|
|
||||||
use codex_features::Feature;
|
use codex_features::Feature;
|
||||||
use codex_features::FeatureConfigSource;
|
use codex_features::FeatureConfigSource;
|
||||||
use codex_features::FeatureOverrides;
|
use codex_features::FeatureOverrides;
|
||||||
@@ -269,27 +268,6 @@ fn explicit_feature_settings_in_config(cfg: &ConfigToml) -> Vec<(String, Feature
|
|||||||
enabled,
|
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
|
explicit_settings
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -339,47 +317,13 @@ pub(crate) fn validate_feature_requirements_in_config_toml(
|
|||||||
cfg: &ConfigToml,
|
cfg: &ConfigToml,
|
||||||
feature_requirements: Option<&Sourced<FeatureRequirementsToml>>,
|
feature_requirements: Option<&Sourced<FeatureRequirementsToml>>,
|
||||||
) -> std::io::Result<()> {
|
) -> std::io::Result<()> {
|
||||||
fn validate_profile(
|
let configured_features = Features::from_sources(
|
||||||
cfg: &ConfigToml,
|
FeatureConfigSource {
|
||||||
profile_name: Option<&str>,
|
features: cfg.features.as_ref(),
|
||||||
profile: &ConfigProfile,
|
experimental_use_unified_exec_tool: cfg.experimental_use_unified_exec_tool,
|
||||||
feature_requirements: Option<&Sourced<FeatureRequirementsToml>>,
|
},
|
||||||
) -> std::io::Result<()> {
|
FeatureConfigSource::default(),
|
||||||
let configured_features = Features::from_sources(
|
FeatureOverrides::default(),
|
||||||
FeatureConfigSource {
|
);
|
||||||
features: cfg.features.as_ref(),
|
ManagedFeatures::from_configured(configured_features, feature_requirements.cloned()).map(|_| ())
|
||||||
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(())
|
|
||||||
}
|
}
|
||||||
@@ -32,17 +32,14 @@ pub async fn maybe_migrate_personality(
|
|||||||
return Ok(PersonalityMigrationStatus::SkippedMarker);
|
return Ok(PersonalityMigrationStatus::SkippedMarker);
|
||||||
}
|
}
|
||||||
|
|
||||||
let config_profile = config_toml
|
if config_toml.personality.is_some() {
|
||||||
.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() {
|
|
||||||
create_marker(&marker_path).await?;
|
create_marker(&marker_path).await?;
|
||||||
return Ok(PersonalityMigrationStatus::SkippedExplicitPersonality);
|
return Ok(PersonalityMigrationStatus::SkippedExplicitPersonality);
|
||||||
}
|
}
|
||||||
|
|
||||||
let model_provider_id = config_profile
|
let model_provider_id = config_toml
|
||||||
.model_provider
|
.model_provider
|
||||||
.or_else(|| config_toml.model_provider.clone())
|
.clone()
|
||||||
.unwrap_or_else(|| "openai".to_string());
|
.unwrap_or_else(|| "openai".to_string());
|
||||||
|
|
||||||
if !has_recorded_sessions(codex_home, model_provider_id.as_str(), state_db).await? {
|
if !has_recorded_sessions(codex_home, model_provider_id.as_str(), state_db).await? {
|
||||||
|
|||||||
@@ -253,7 +253,7 @@ async fn no_marker_explicit_global_personality_skips_migration() -> io::Result<(
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[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()?;
|
let temp = TempDir::new()?;
|
||||||
write_session_with_user_event(temp.path()).await?;
|
write_session_with_user_event(temp.path()).await?;
|
||||||
let config_toml = parse_config_toml(
|
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?;
|
let status = maybe_migrate_personality(temp.path(), &config_toml, /*state_db*/ None).await?;
|
||||||
|
|
||||||
assert_eq!(
|
assert_eq!(status, PersonalityMigrationStatus::Applied);
|
||||||
status,
|
|
||||||
PersonalityMigrationStatus::SkippedExplicitPersonality
|
|
||||||
);
|
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
tokio::fs::try_exists(temp.path().join(PERSONALITY_MIGRATION_FILENAME)).await?,
|
tokio::fs::try_exists(temp.path().join(PERSONALITY_MIGRATION_FILENAME)).await?,
|
||||||
true
|
true
|
||||||
);
|
);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
tokio::fs::try_exists(temp.path().join("config.toml")).await?,
|
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(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[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()?;
|
let temp = TempDir::new()?;
|
||||||
tokio::fs::write(temp.path().join(PERSONALITY_MIGRATION_FILENAME), "v1\n").await?;
|
tokio::fs::write(temp.path().join(PERSONALITY_MIGRATION_FILENAME), "v1\n").await?;
|
||||||
let config_toml = parse_config_toml("profile = \"missing\"\n")?;
|
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]
|
#[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 temp = TempDir::new()?;
|
||||||
let config_toml = parse_config_toml("profile = \"missing\"\n")?;
|
let config_toml = parse_config_toml("profile = \"missing\"\n")?;
|
||||||
|
|
||||||
let err = maybe_migrate_personality(temp.path(), &config_toml, /*state_db*/ None)
|
let status = maybe_migrate_personality(temp.path(), &config_toml, /*state_db*/ None).await?;
|
||||||
.await
|
|
||||||
.expect_err("missing profile should fail");
|
|
||||||
|
|
||||||
assert_eq!(err.kind(), io::ErrorKind::InvalidData);
|
assert_eq!(status, PersonalityMigrationStatus::SkippedNoSessions);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
tokio::fs::try_exists(temp.path().join(PERSONALITY_MIGRATION_FILENAME)).await?,
|
tokio::fs::try_exists(temp.path().join(PERSONALITY_MIGRATION_FILENAME)).await?,
|
||||||
false
|
true
|
||||||
);
|
);
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in new issue
Block a user