mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Remove smart_approvals alias migration (#15464)
Remove the legacy `smart_approvals` config migration from core config loading. This change: - stops rewriting `smart_approvals` into `guardian_approval` - stops backfilling `approvals_reviewer = "guardian_subagent"` - replaces the migration tests with regression coverage that asserts the deprecated key is ignored in root and profile scopes Verification: - `just fmt` - `cargo test -p codex-core smart_approvals_alias_is_ignored` - `cargo test -p codex-core approvals_reviewer_` - `just argument-comment-lint` Notes: - `cargo test -p codex-core` still hits an unrelated existing failure in `tools::js_repl::tests::js_repl_imported_local_files_can_access_repl_globals`; the JS REPL kernel exits after `mktemp` fails under the current environment. Enhancement request: requested cleanup to delete the `smart_approvals` alias migration; no public issue link is available. Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
Charley Cunningham
committed by
GitHub
Unverified
GitHub
parent
31728dd460
commit
e830000e41
@@ -5677,7 +5677,7 @@ approvals_reviewer = "guardian_subagent"
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn smart_approvals_alias_is_migrated_to_guardian_approval() -> std::io::Result<()> {
|
||||
async fn smart_approvals_alias_is_ignored() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
@@ -5692,23 +5692,19 @@ smart_approvals = true
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert!(config.features.enabled(Feature::GuardianApproval));
|
||||
assert_eq!(config.features.legacy_feature_usages().count(), 0);
|
||||
assert_eq!(
|
||||
config.approvals_reviewer,
|
||||
ApprovalsReviewer::GuardianSubagent
|
||||
);
|
||||
assert!(!config.features.enabled(Feature::GuardianApproval));
|
||||
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::User);
|
||||
|
||||
let serialized = tokio::fs::read_to_string(codex_home.path().join(CONFIG_TOML_FILE)).await?;
|
||||
assert!(serialized.contains("guardian_approval = true"));
|
||||
assert!(serialized.contains("approvals_reviewer = \"guardian_subagent\""));
|
||||
assert!(!serialized.contains("smart_approvals"));
|
||||
assert!(serialized.contains("smart_approvals = true"));
|
||||
assert!(!serialized.contains("guardian_approval"));
|
||||
assert!(!serialized.contains("approvals_reviewer"));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn smart_approvals_alias_is_migrated_in_profiles() -> std::io::Result<()> {
|
||||
async fn smart_approvals_alias_is_ignored_in_profiles() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
@@ -5719,106 +5715,6 @@ smart_approvals = true
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.fallback_cwd(Some(codex_home.path().to_path_buf()))
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert!(config.features.enabled(Feature::GuardianApproval));
|
||||
assert_eq!(config.features.legacy_feature_usages().count(), 0);
|
||||
assert_eq!(
|
||||
config.approvals_reviewer,
|
||||
ApprovalsReviewer::GuardianSubagent
|
||||
);
|
||||
|
||||
let serialized = tokio::fs::read_to_string(codex_home.path().join(CONFIG_TOML_FILE)).await?;
|
||||
assert!(serialized.contains("[profiles.guardian.features]"));
|
||||
assert!(serialized.contains("guardian_approval = true"));
|
||||
assert!(serialized.contains("approvals_reviewer = \"guardian_subagent\""));
|
||||
assert!(!serialized.contains("smart_approvals"));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn smart_approvals_alias_migration_preserves_disabled_profile_override() -> std::io::Result<()>
|
||||
{
|
||||
let codex_home = TempDir::new()?;
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
r#"[features]
|
||||
guardian_approval = true
|
||||
|
||||
[profiles.guardian.features]
|
||||
smart_approvals = false
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.fallback_cwd(Some(codex_home.path().to_path_buf()))
|
||||
.harness_overrides(ConfigOverrides {
|
||||
config_profile: Some("guardian".to_string()),
|
||||
..Default::default()
|
||||
})
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert!(!config.features.enabled(Feature::GuardianApproval));
|
||||
assert_eq!(config.features.legacy_feature_usages().count(), 0);
|
||||
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::User);
|
||||
|
||||
let serialized = tokio::fs::read_to_string(codex_home.path().join(CONFIG_TOML_FILE)).await?;
|
||||
assert!(serialized.contains("[profiles.guardian.features]"));
|
||||
assert!(serialized.contains("guardian_approval = false"));
|
||||
assert!(!serialized.contains("smart_approvals"));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn smart_approvals_alias_migration_preserves_existing_approvals_reviewer()
|
||||
-> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
r#"approvals_reviewer = "user"
|
||||
|
||||
[features]
|
||||
smart_approvals = true
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.fallback_cwd(Some(codex_home.path().to_path_buf()))
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert!(config.features.enabled(Feature::GuardianApproval));
|
||||
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::User);
|
||||
|
||||
let serialized = tokio::fs::read_to_string(codex_home.path().join(CONFIG_TOML_FILE)).await?;
|
||||
assert!(serialized.contains("guardian_approval = true"));
|
||||
assert!(serialized.contains("approvals_reviewer = \"user\""));
|
||||
assert!(!serialized.contains("smart_approvals"));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn smart_approvals_alias_migration_does_not_override_canonical_disabled_flag()
|
||||
-> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
r#"[features]
|
||||
guardian_approval = false
|
||||
smart_approvals = true
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.fallback_cwd(Some(codex_home.path().to_path_buf()))
|
||||
@@ -5829,9 +5725,10 @@ smart_approvals = true
|
||||
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::User);
|
||||
|
||||
let serialized = tokio::fs::read_to_string(codex_home.path().join(CONFIG_TOML_FILE)).await?;
|
||||
assert!(serialized.contains("guardian_approval = false"));
|
||||
assert!(!serialized.contains("approvals_reviewer = \"guardian_subagent\""));
|
||||
assert!(!serialized.contains("smart_approvals"));
|
||||
assert!(serialized.contains("[profiles.guardian.features]"));
|
||||
assert!(serialized.contains("smart_approvals = true"));
|
||||
assert!(!serialized.contains("guardian_approval"));
|
||||
assert!(!serialized.contains("approvals_reviewer"));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -104,7 +104,6 @@ use crate::config::profile::ConfigProfile;
|
||||
use codex_network_proxy::NetworkProxyConfig;
|
||||
use toml::Value as TomlValue;
|
||||
use toml_edit::DocumentMut;
|
||||
use toml_edit::value;
|
||||
|
||||
pub(crate) mod agent_roles;
|
||||
pub mod edit;
|
||||
@@ -652,9 +651,6 @@ impl ConfigBuilder {
|
||||
fallback_cwd,
|
||||
} = self;
|
||||
let codex_home = codex_home.map_or_else(find_codex_home, std::io::Result::Ok)?;
|
||||
if let Err(err) = maybe_migrate_smart_approvals_alias(&codex_home).await {
|
||||
tracing::warn!(error = %err, "failed to migrate smart_approvals feature alias");
|
||||
}
|
||||
let cli_overrides = cli_overrides.unwrap_or_default();
|
||||
let mut harness_overrides = harness_overrides.unwrap_or_default();
|
||||
let loader_overrides = loader_overrides.unwrap_or_default();
|
||||
@@ -702,111 +698,6 @@ impl ConfigBuilder {
|
||||
}
|
||||
}
|
||||
|
||||
fn config_scope_segments(scope: &[String], key: &str) -> Vec<String> {
|
||||
let mut segments = scope.to_vec();
|
||||
segments.push(key.to_string());
|
||||
segments
|
||||
}
|
||||
|
||||
fn feature_scope_segments(scope: &[String], feature_key: &str) -> Vec<String> {
|
||||
let mut segments = scope.to_vec();
|
||||
segments.push("features".to_string());
|
||||
segments.push(feature_key.to_string());
|
||||
segments
|
||||
}
|
||||
|
||||
fn push_smart_approvals_alias_migration_edits(
|
||||
edits: &mut Vec<ConfigEdit>,
|
||||
scope: &[String],
|
||||
features: &FeaturesToml,
|
||||
approvals_reviewer_missing: bool,
|
||||
) {
|
||||
let Some(alias_enabled) = features.entries.get("smart_approvals").copied() else {
|
||||
return;
|
||||
};
|
||||
let canonical_enabled = features
|
||||
.entries
|
||||
.get("guardian_approval")
|
||||
.copied()
|
||||
.unwrap_or(alias_enabled);
|
||||
|
||||
if !features.entries.contains_key("guardian_approval") {
|
||||
edits.push(ConfigEdit::SetPath {
|
||||
segments: feature_scope_segments(scope, "guardian_approval"),
|
||||
value: value(alias_enabled),
|
||||
});
|
||||
}
|
||||
if canonical_enabled && approvals_reviewer_missing {
|
||||
edits.push(ConfigEdit::SetPath {
|
||||
segments: config_scope_segments(scope, "approvals_reviewer"),
|
||||
value: value(ApprovalsReviewer::GuardianSubagent.to_string()),
|
||||
});
|
||||
}
|
||||
edits.push(ConfigEdit::ClearPath {
|
||||
segments: feature_scope_segments(scope, "smart_approvals"),
|
||||
});
|
||||
}
|
||||
|
||||
/// Rewrites the legacy `smart_approvals` feature flag to
|
||||
/// `guardian_approval` in `config.toml` before normal config loading.
|
||||
///
|
||||
/// If the old key is present, this preserves its value by setting
|
||||
/// `guardian_approval = <alias value>` when the new key is not already present.
|
||||
/// Because the deprecated flag historically meant "turn guardian review on",
|
||||
/// this migration also backfills `approvals_reviewer = "guardian_subagent"`
|
||||
/// in the same scope when that reviewer is not already configured there and the
|
||||
/// migrated feature value is `true`.
|
||||
/// In all cases it removes the deprecated `smart_approvals` entry so future
|
||||
/// loads only see the canonical feature flag name.
|
||||
async fn maybe_migrate_smart_approvals_alias(codex_home: &Path) -> std::io::Result<bool> {
|
||||
let config_path = codex_home.join(CONFIG_TOML_FILE);
|
||||
if !tokio::fs::try_exists(&config_path).await? {
|
||||
return Ok(false);
|
||||
}
|
||||
|
||||
let config_contents = tokio::fs::read_to_string(&config_path).await?;
|
||||
let Ok(config_toml) = toml::from_str::<ConfigToml>(&config_contents) else {
|
||||
return Ok(false);
|
||||
};
|
||||
|
||||
let mut edits = Vec::new();
|
||||
|
||||
let root_scope = Vec::new();
|
||||
if let Some(features) = config_toml.features.as_ref() {
|
||||
push_smart_approvals_alias_migration_edits(
|
||||
&mut edits,
|
||||
&root_scope,
|
||||
features,
|
||||
config_toml.approvals_reviewer.is_none(),
|
||||
);
|
||||
}
|
||||
|
||||
for (profile_name, profile) in &config_toml.profiles {
|
||||
if let Some(features) = profile.features.as_ref() {
|
||||
let scope = vec!["profiles".to_string(), profile_name.clone()];
|
||||
push_smart_approvals_alias_migration_edits(
|
||||
&mut edits,
|
||||
&scope,
|
||||
features,
|
||||
profile.approvals_reviewer.is_none(),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
if edits.is_empty() {
|
||||
return Ok(false);
|
||||
}
|
||||
|
||||
ConfigEditsBuilder::new(codex_home)
|
||||
.with_edits(edits)
|
||||
.apply()
|
||||
.await
|
||||
.map_err(|err| {
|
||||
std::io::Error::other(format!("failed to migrate guardian_approval alias: {err}"))
|
||||
})?;
|
||||
Ok(true)
|
||||
}
|
||||
|
||||
impl Config {
|
||||
/// This is the preferred way to create an instance of [Config].
|
||||
pub async fn load_with_cli_overrides(
|
||||
@@ -868,9 +759,6 @@ pub async fn load_config_as_toml_with_cli_overrides(
|
||||
cwd: &AbsolutePathBuf,
|
||||
cli_overrides: Vec<(String, TomlValue)>,
|
||||
) -> std::io::Result<ConfigToml> {
|
||||
if let Err(err) = maybe_migrate_smart_approvals_alias(codex_home).await {
|
||||
tracing::warn!(error = %err, "failed to migrate smart_approvals feature alias");
|
||||
}
|
||||
let config_layer_stack = load_config_layers_state(
|
||||
codex_home,
|
||||
Some(cwd.clone()),
|
||||
|
||||
Reference in New Issue
Block a user