mirror of
https://github.com/openai/codex.git
synced 2026-04-29 08:56:38 +00:00
Reuse guardian session across approvals (#14668)
## Summary - reuse a guardian subagent session across approvals so reviews keep a stable prompt cache key and avoid one-shot startup overhead - clear the guardian child history before each review so prior guardian decisions do not leak into later approvals - include the `smart_approvals` -> `guardian_approval` feature flag rename in the same PR to minimize release latency on a very tight timeline - add regression coverage for prompt-cache-key reuse without prior-review prompt bleed ## Request - Bug/enhancement request: internal guardian prompt-cache and latency improvement request --------- Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
Charley Cunningham
committed by
GitHub
GitHub
parent
ba463a9dc7
commit
6fdeb1d602
@@ -614,8 +614,8 @@ 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_guardian_approval_alias(&codex_home).await {
|
||||
tracing::warn!(error = %err, "failed to migrate guardian_approval feature alias");
|
||||
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();
|
||||
@@ -664,17 +664,63 @@ impl ConfigBuilder {
|
||||
}
|
||||
}
|
||||
|
||||
/// Rewrites the legacy `guardian_approval` feature flag to
|
||||
/// `smart_approvals` in `config.toml` before normal config loading.
|
||||
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 and enabled, this preserves the enabled state by
|
||||
/// setting `smart_approvals = true` when the new key is not already present.
|
||||
/// 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.
|
||||
/// In all cases it removes the deprecated `guardian_approval` entry so future
|
||||
/// 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_guardian_approval_alias(codex_home: &Path) -> std::io::Result<bool> {
|
||||
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);
|
||||
@@ -687,59 +733,25 @@ async fn maybe_migrate_guardian_approval_alias(codex_home: &Path) -> std::io::Re
|
||||
|
||||
let mut edits = Vec::new();
|
||||
|
||||
if let Some(features) = config_toml.features.as_ref()
|
||||
&& let Some(enabled) = features.entries.get("guardian_approval").copied()
|
||||
{
|
||||
if enabled && !features.entries.contains_key("smart_approvals") {
|
||||
edits.push(ConfigEdit::SetPath {
|
||||
segments: vec!["features".to_string(), "smart_approvals".to_string()],
|
||||
value: value(true),
|
||||
});
|
||||
}
|
||||
if enabled && config_toml.approvals_reviewer.is_none() {
|
||||
edits.push(ConfigEdit::SetPath {
|
||||
segments: vec!["approvals_reviewer".to_string()],
|
||||
value: value(ApprovalsReviewer::GuardianSubagent.to_string()),
|
||||
});
|
||||
}
|
||||
edits.push(ConfigEdit::ClearPath {
|
||||
segments: vec!["features".to_string(), "guardian_approval".to_string()],
|
||||
});
|
||||
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 Some(enabled) = features.entries.get("guardian_approval").copied()
|
||||
{
|
||||
if enabled && !features.entries.contains_key("smart_approvals") {
|
||||
edits.push(ConfigEdit::SetPath {
|
||||
segments: vec![
|
||||
"profiles".to_string(),
|
||||
profile_name.clone(),
|
||||
"features".to_string(),
|
||||
"smart_approvals".to_string(),
|
||||
],
|
||||
value: value(true),
|
||||
});
|
||||
}
|
||||
if enabled && profile.approvals_reviewer.is_none() {
|
||||
edits.push(ConfigEdit::SetPath {
|
||||
segments: vec![
|
||||
"profiles".to_string(),
|
||||
profile_name.clone(),
|
||||
"approvals_reviewer".to_string(),
|
||||
],
|
||||
value: value(ApprovalsReviewer::GuardianSubagent.to_string()),
|
||||
});
|
||||
}
|
||||
edits.push(ConfigEdit::ClearPath {
|
||||
segments: vec![
|
||||
"profiles".to_string(),
|
||||
profile_name.clone(),
|
||||
"features".to_string(),
|
||||
"guardian_approval".to_string(),
|
||||
],
|
||||
});
|
||||
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(),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -752,7 +764,7 @@ async fn maybe_migrate_guardian_approval_alias(codex_home: &Path) -> std::io::Re
|
||||
.apply()
|
||||
.await
|
||||
.map_err(|err| {
|
||||
std::io::Error::other(format!("failed to migrate smart_approvals alias: {err}"))
|
||||
std::io::Error::other(format!("failed to migrate guardian_approval alias: {err}"))
|
||||
})?;
|
||||
Ok(true)
|
||||
}
|
||||
@@ -818,8 +830,8 @@ 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_guardian_approval_alias(codex_home).await {
|
||||
tracing::warn!(error = %err, "failed to migrate guardian_approval feature alias");
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user