mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
fix: preserve auto review across config and delegation (#26230)
## Why Auto Review should remain the effective approval reviewer when settings cross runtime boundaries. A config or app-server round trip must not change the reviewer identity, and delegated work must not silently fall back to user review. This requires both a stable canonical serialized value and propagation of the effective setting. `auto_review` is the canonical value across protocol and app-server output, while `guardian_subagent` remains accepted as backward-compatible input. ## What changed - serialize `ApprovalsReviewer::AutoReview` consistently as `auto_review` across core protocol and app-server v2 - continue accepting `guardian_subagent` when reading existing config or client requests - carry the active turn's approval reviewer into spawned agents - update config/debug expectations and add delegated-task regression coverage ## Scope This does not change Guardian policy or remove compatibility with existing `guardian_subagent` inputs. It preserves the selected reviewer across serialization, config reloads, app-server settings, and delegated task setup. Related Guardian changes are split independently: - #26231 adds denials and soft denials - #26334 retries transient reviewer failures - #26333 reuses narrowly scoped low-risk approvals - #26232 adds TUI denial recovery ## Validation - `just test -p codex-app-server-protocol` (224 passed) - regression coverage for delegated task reviewer propagation - serialization coverage for canonical `auto_review` output and legacy `guardian_subagent` input --------- Co-authored-by: saud-oai <saud@openai.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
2375cb6449
commit
9e0d7f02c9
@@ -233,7 +233,7 @@ impl From<CoreAskForApproval> for AskForApproval {
|
||||
pub enum ApprovalsReviewer {
|
||||
#[serde(rename = "user")]
|
||||
User,
|
||||
#[serde(rename = "guardian_subagent", alias = "auto_review")]
|
||||
#[serde(rename = "auto_review", alias = "guardian_subagent")]
|
||||
AutoReview,
|
||||
}
|
||||
|
||||
|
||||
@@ -65,7 +65,7 @@ fn approvals_reviewer_serializes_auto_review_and_accepts_legacy_guardian_subagen
|
||||
);
|
||||
assert_eq!(
|
||||
serde_json::to_string(&ApprovalsReviewer::AutoReview).expect("serialize reviewer"),
|
||||
"\"guardian_subagent\""
|
||||
"\"auto_review\""
|
||||
);
|
||||
|
||||
for value in ["user", "auto_review", "guardian_subagent"] {
|
||||
|
||||
@@ -1467,7 +1467,7 @@ foo = "xyzzy"
|
||||
TomlValue::Table(toml::map::Map::from_iter([(
|
||||
"allowed_approvals_reviewers".to_string(),
|
||||
TomlValue::Array(vec![
|
||||
TomlValue::String("guardian_subagent".to_string()),
|
||||
TomlValue::String("auto_review".to_string()),
|
||||
TomlValue::String("user".to_string()),
|
||||
]),
|
||||
)]))
|
||||
|
||||
@@ -262,6 +262,7 @@ pub(crate) fn apply_spawn_agent_runtime_overrides(
|
||||
.map_err(|err| {
|
||||
FunctionCallError::RespondToModel(format!("approval_policy is invalid: {err}"))
|
||||
})?;
|
||||
config.approvals_reviewer = turn.config.approvals_reviewer;
|
||||
config.permissions.shell_environment_policy = turn.shell_environment_policy.clone();
|
||||
config.codex_linux_sandbox_exe = turn.codex_linux_sandbox_exe.clone();
|
||||
#[allow(deprecated)]
|
||||
|
||||
@@ -24,6 +24,7 @@ use codex_model_provider::create_model_provider;
|
||||
use codex_model_provider_info::built_in_model_providers;
|
||||
use codex_protocol::AgentPath;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::config_types::ServiceTier;
|
||||
use codex_protocol::config_types::ShellEnvironmentPolicy;
|
||||
use codex_protocol::models::BaseInstructions;
|
||||
@@ -2331,6 +2332,9 @@ async fn spawn_agent_reapplies_runtime_sandbox_after_role_config() {
|
||||
turn.approval_policy
|
||||
.set(AskForApproval::OnRequest)
|
||||
.expect("approval policy should be set");
|
||||
let mut config = (*turn.config).clone();
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
set_turn_config(&mut turn, config);
|
||||
turn.permission_profile = expected_permission_profile.clone();
|
||||
assert_ne!(
|
||||
expected_permission_profile,
|
||||
@@ -2370,6 +2374,7 @@ async fn spawn_agent_reapplies_runtime_sandbox_after_role_config() {
|
||||
.await;
|
||||
assert_eq!(snapshot.sandbox_policy(), expected_sandbox);
|
||||
assert_eq!(snapshot.approval_policy, AskForApproval::OnRequest);
|
||||
assert_eq!(snapshot.approvals_reviewer, ApprovalsReviewer::AutoReview);
|
||||
assert_eq!(snapshot.permission_profile, expected_permission_profile);
|
||||
let child_thread = manager
|
||||
.get_thread(agent_id)
|
||||
|
||||
@@ -166,8 +166,8 @@ pub enum ApprovalsReviewer {
|
||||
#[default]
|
||||
#[serde(rename = "user")]
|
||||
User,
|
||||
#[serde(rename = "guardian_subagent", alias = "auto_review")]
|
||||
#[strum(serialize = "guardian_subagent")]
|
||||
#[serde(rename = "auto_review", alias = "guardian_subagent")]
|
||||
#[strum(serialize = "auto_review")]
|
||||
AutoReview,
|
||||
}
|
||||
|
||||
@@ -760,7 +760,7 @@ mod tests {
|
||||
);
|
||||
assert_eq!(
|
||||
serde_json::to_string(&ApprovalsReviewer::AutoReview).expect("serialize reviewer"),
|
||||
"\"guardian_subagent\""
|
||||
"\"auto_review\""
|
||||
);
|
||||
|
||||
for value in ["user", "auto_review", "guardian_subagent"] {
|
||||
|
||||
@@ -1824,7 +1824,7 @@ async fn update_feature_flags_enabling_guardian_selects_auto_review() -> Result<
|
||||
|
||||
let config = std::fs::read_to_string(codex_home.path().join("config.toml"))?;
|
||||
assert!(config.contains("guardian_approval = true"));
|
||||
assert!(config.contains("approvals_reviewer = \"guardian_subagent\""));
|
||||
assert!(config.contains("approvals_reviewer = \"auto_review\""));
|
||||
assert!(config.contains("approval_policy = \"on-request\""));
|
||||
assert!(config.contains("sandbox_mode = \"workspace-write\""));
|
||||
app_server.shutdown().await?;
|
||||
@@ -1988,7 +1988,7 @@ async fn update_feature_flags_enabling_guardian_overrides_explicit_manual_review
|
||||
);
|
||||
|
||||
let config = std::fs::read_to_string(codex_home.path().join("config.toml"))?;
|
||||
assert!(config.contains("approvals_reviewer = \"guardian_subagent\""));
|
||||
assert!(config.contains("approvals_reviewer = \"auto_review\""));
|
||||
assert!(config.contains("guardian_approval = true"));
|
||||
assert!(config.contains("approval_policy = \"on-request\""));
|
||||
assert!(config.contains("sandbox_mode = \"workspace-write\""));
|
||||
|
||||
@@ -755,7 +755,7 @@ mod tests {
|
||||
"allowed_approval_policies: on-request (source: {requirements_source})"
|
||||
)));
|
||||
assert!(rendered.contains(
|
||||
"allowed_approvals_reviewers: guardian_subagent (source: MDM managed_config.toml (legacy))"
|
||||
"allowed_approvals_reviewers: auto_review (source: MDM managed_config.toml (legacy))"
|
||||
));
|
||||
assert!(
|
||||
rendered.contains(
|
||||
@@ -818,7 +818,7 @@ mod tests {
|
||||
|
||||
let rendered = render_to_text(&render_debug_config_lines(&stack));
|
||||
assert!(rendered.contains(
|
||||
"allowed_approvals_reviewers: guardian_subagent (source: MDM managed_config.toml (legacy))"
|
||||
"allowed_approvals_reviewers: auto_review (source: MDM managed_config.toml (legacy))"
|
||||
));
|
||||
assert!(!rendered.contains("Requirements:\n <none>"));
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user