diff --git a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst index 4f7853dcc4..aaed74c052 100644 Binary files a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst and b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst differ diff --git a/codex-rs/app-server/src/request_processors/persisted_resume_settings.rs b/codex-rs/app-server/src/request_processors/persisted_resume_settings.rs index a9747d58ab..0b66ce67d2 100644 --- a/codex-rs/app-server/src/request_processors/persisted_resume_settings.rs +++ b/codex-rs/app-server/src/request_processors/persisted_resume_settings.rs @@ -43,7 +43,11 @@ impl From<&ThreadSettingsSnapshot> for PersistedResumeSettings { fn from(settings: &ThreadSettingsSnapshot) -> Self { Self { approval_policy: settings.approval_policy, - approvals_reviewer: Some(settings.approvals_reviewer), + approvals_reviewer: Some( + settings + .requested_approvals_reviewer + .unwrap_or(settings.approvals_reviewer), + ), active_permission_profile: settings.active_permission_profile.clone(), unnamed_permission_profile: settings .active_permission_profile @@ -78,9 +82,12 @@ fn latest_persisted_resume_settings(history: &[RolloutItem]) -> Option turn_context.approvals_reviewer, - RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)) => { - Some(event.thread_settings.approvals_reviewer) - } + RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)) => Some( + event + .thread_settings + .requested_approvals_reviewer + .unwrap_or(event.thread_settings.approvals_reviewer), + ), _ => None, }) }), @@ -93,7 +100,11 @@ fn latest_persisted_resume_settings(history: &[RolloutItem]) -> Option Result< Ok(()) } +#[test_case(ApprovalsReviewer::AutoReview, "", ApprovalsReviewer::AutoReview; "explicit auto review")] +#[test_case(ApprovalsReviewer::User, "", ApprovalsReviewer::User; "requirement removed")] +#[test_case(ApprovalsReviewer::User, "allowed_approvals_reviewers = [\"auto_review\"]", ApprovalsReviewer::AutoReview; "allowlist tightened")] #[tokio::test] -async fn thread_resume_preserves_persisted_approvals_reviewer() -> Result<()> { +async fn thread_resume_preserves_persisted_approvals_reviewer( + requested_reviewer: ApprovalsReviewer, + resumed_requirements: &str, + expected_reviewer: ApprovalsReviewer, +) -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; mock_responses_config(&server.uri()).write(codex_home.path())?; + let requirements_path = codex_home.path().join("requirements.toml"); + std::fs::write( + &requirements_path, + "[auto_review]\nrequired_on_models = [\"gpt-5.4\"]\n", + )?; + let thread_id = { let mut mcp = TestAppServer::builder() .with_codex_home(codex_home.path()) @@ -1214,12 +1227,16 @@ async fn thread_resume_preserves_persisted_approvals_reviewer() -> Result<()> { let start_id = mcp .send_thread_start_request(ThreadStartParams { model: Some("gpt-5.4".to_string()), - approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + approvals_reviewer: Some(requested_reviewer), ..Default::default() }) .await?; - let ThreadStartResponse { thread, .. } = - timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(start_id)).await??; + let ThreadStartResponse { + thread, + approvals_reviewer, + .. + } = timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(start_id)).await??; + assert_eq!(approvals_reviewer, ApprovalsReviewer::AutoReview); let turn_id = mcp .send_turn_start_request(TurnStartParams { @@ -1246,6 +1263,7 @@ async fn thread_resume_preserves_persisted_approvals_reviewer() -> Result<()> { thread.id }; + std::fs::write(requirements_path, resumed_requirements)?; let config_path = codex_home.path().join("config.toml"); let config = std::fs::read_to_string(&config_path)?; std::fs::write( @@ -1271,7 +1289,7 @@ async fn thread_resume_preserves_persisted_approvals_reviewer() -> Result<()> { approvals_reviewer, .. } = timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(resume_id)).await??; - assert_eq!(approvals_reviewer, ApprovalsReviewer::AutoReview); + assert_eq!(approvals_reviewer, expected_reviewer); Ok(()) } diff --git a/codex-rs/core/src/agent/control/spawn.rs b/codex-rs/core/src/agent/control/spawn.rs index a4ac17f71a..e557f814c9 100644 --- a/codex-rs/core/src/agent/control/spawn.rs +++ b/codex-rs/core/src/agent/control/spawn.rs @@ -281,6 +281,7 @@ impl LocalAgentControl { // Preserve the worker's permissions while keeping the current wake configuration. config.permissions = evicted.config.permissions.clone(); config.approvals_reviewer = evicted.config.approvals_reviewer; + config.requested_approvals_reviewer = evicted.config.requested_approvals_reviewer; } let mut environment_selections = evicted_environments .as_ref() diff --git a/codex-rs/core/src/agent/control_tests.rs b/codex-rs/core/src/agent/control_tests.rs index 91fcfe365f..a0f3ea2e2d 100644 --- a/codex-rs/core/src/agent/control_tests.rs +++ b/codex-rs/core/src/agent/control_tests.rs @@ -1172,10 +1172,13 @@ async fn cancelled_v2_send_finishes_reload_registration() { .is_some() ); + let mut sender_config = harness.config.clone(); + sender_config.approvals_reviewer = ApprovalsReviewer::AutoReview; + sender_config.requested_approvals_reviewer = Some(ApprovalsReviewer::AutoReview); let followup_request = |message: &str| SendRequest { caller: parent_id, target: AgentTarget::Id(child_id), - resume_config: harness.config.clone(), + resume_config: sender_config.clone(), input: AgentInput::Message { message: AgentMessage::Plaintext(message.to_string()), mode: MessageDeliveryMode::TriggerTurn, @@ -1223,6 +1226,11 @@ async fn cancelled_v2_send_finishes_reload_registration() { .evicted_environments(child_id) .is_none() ); + assert_eq!( + reloaded.config_snapshot().await.approvals_reviewer, + ApprovalsReviewer::User, + "reload must preserve the worker's reviewer instead of inheriting its sender's request", + ); // Evict and reload again with the single child slot. This also catches a leaked // pin or pending residency reservation left behind by the cancelled sender. assert_eq!( @@ -2459,6 +2467,7 @@ async fn spawn_agent_fork_from_paginated_parent_uses_model_context_prefix( service_tier: None, approval_policy: AskForApproval::Never, approvals_reviewer: ApprovalsReviewer::User, + requested_approvals_reviewer: None, permission_profile: PermissionProfile::workspace_write(), active_permission_profile: None, cwd: harness.config.cwd.clone(), diff --git a/codex-rs/core/src/codex_thread.rs b/codex-rs/core/src/codex_thread.rs index 1dd31677bb..5b47ffe4c2 100644 --- a/codex-rs/core/src/codex_thread.rs +++ b/codex-rs/core/src/codex_thread.rs @@ -161,6 +161,8 @@ pub struct CodexThreadSettingsOverrides { pub profile_workspace_roots: Option>, pub approval_policy: Option, pub approvals_reviewer: Option, + /// Retained selection when restoring an effective reviewer forced by policy. + pub requested_approvals_reviewer: Option, pub sandbox_policy: Option, pub permission_profile: Option, pub active_permission_profile: Option, @@ -877,6 +879,7 @@ impl CodexThread { profile_workspace_roots, approval_policy, approvals_reviewer, + requested_approvals_reviewer, sandbox_policy, permission_profile, active_permission_profile, @@ -890,6 +893,7 @@ impl CodexThread { disabled_plugin_ids, } = overrides; SessionSettingsUpdate { + requested_approvals_reviewer, turn_extension_init, step_settings: StepSettingsUpdate { model, diff --git a/codex-rs/core/src/config/guardian_policy.rs b/codex-rs/core/src/config/guardian_policy.rs new file mode 100644 index 0000000000..e1f21cde38 --- /dev/null +++ b/codex-rs/core/src/config/guardian_policy.rs @@ -0,0 +1,23 @@ +//! Resolves the effective Guardian reviewer while retaining the requested choice. + +use codex_config::ConfigRequirements; +use codex_protocol::config_types::ApprovalsReviewer; + +/// Resolves the effective reviewer without replacing the retained request. +/// Trusted private reviewers omit the model to bypass model-required review. +pub(crate) fn resolve_guardian_reviewer( + requested: ApprovalsReviewer, + model: Option<&str>, + requirements: &ConfigRequirements, +) -> super::ConstraintResult { + let reviewer = if model.is_some_and(|model| requirements.auto_review_required_for_model(model)) + { + ApprovalsReviewer::AutoReview + } else if requirements.approvals_reviewer.can_set(&requested).is_ok() { + requested + } else { + requirements.approvals_reviewer.value() + }; + requirements.approvals_reviewer.can_set(&reviewer)?; + Ok(reviewer) +} diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 6990af063c..d32db75af7 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -168,6 +168,7 @@ use toml_edit::DocumentMut; mod auth_keyring; pub mod edit; +mod guardian_policy; mod managed_features; mod metrics; mod network_config; @@ -179,6 +180,7 @@ mod permission_profile_selection; mod permissions; mod requirements; mod resolved_permission_profile; +pub(crate) use guardian_policy::resolve_guardian_reviewer; mod runtime_refresh; pub(crate) use runtime_refresh::RuntimeConfigRefresh; #[cfg(test)] @@ -693,6 +695,9 @@ pub struct Config { /// been escalated. This does not disable separate safety checks such as /// ARC. pub approvals_reviewer: ApprovalsReviewer, + /// Requested reviewer when managed requirements replace the effective choice. + /// `None` uses `approvals_reviewer`, including programmatically constructed configs. + pub requested_approvals_reviewer: Option, /// enforce_residency means web traffic cannot be routed outside of a /// particular geography. HTTP clients should direct their requests @@ -3993,6 +3998,7 @@ impl Config { let mut approvals_reviewer = approvals_reviewer_override .or(cfg.approvals_reviewer) .unwrap_or(ApprovalsReviewer::User); + let requested_approvals_reviewer = approvals_reviewer; if !approvals_reviewer_was_explicit && let Err(err) = constrained_approvals_reviewer.can_set(&approvals_reviewer) { @@ -4593,6 +4599,9 @@ impl Config { explicit_permission_profile_mode, custom_permission_profiles, approvals_reviewer: constrained_approvals_reviewer.value(), + requested_approvals_reviewer: (requested_approvals_reviewer + != constrained_approvals_reviewer.value()) + .then_some(requested_approvals_reviewer), enforce_residency: enforce_residency.value, notify: cfg.notify, base_instructions, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index b620d57db5..64941d620f 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -798,6 +798,9 @@ impl Session { "No models are available. Set `model` explicitly or check your model catalog configuration.".to_string(), )); } + let requested_approvals_reviewer = config + .requested_approvals_reviewer + .unwrap_or(config.approvals_reviewer); let trusted_guardian_reviewer = crate::guardian::is_basic_session_source(&session_source) && !matches!(conversation_history, InitialHistory::Resumed(_)); if config @@ -824,13 +827,17 @@ impl Session { ); } } - config - .config_layer_stack - .requirements() - .approvals_reviewer - .can_set(&ApprovalsReviewer::AutoReview) - .map_err(|err| CodexErr::InvalidRequest(err.to_string()))?; - config.approvals_reviewer = ApprovalsReviewer::AutoReview; + } + let reviewer = crate::config::resolve_guardian_reviewer( + requested_approvals_reviewer, + (!trusted_guardian_reviewer).then_some(model.as_str()), + config.config_layer_stack.requirements(), + ) + .map_err(|err| CodexErr::InvalidRequest(err.to_string()))?; + { + let config = Arc::make_mut(&mut config); + config.requested_approvals_reviewer = Some(requested_approvals_reviewer); + config.approvals_reviewer = reviewer; } if allow_provider_model_fallback && let Some(requested_model) = config.model.as_ref() diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index dc4ae7ba02..5cc388cd40 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -336,6 +336,9 @@ impl SessionConfiguration { service_tier: self.step_settings.service_tier.clone(), approval_policy: self.step_settings.approval_policy.value(), approvals_reviewer: self.step_settings.approvals_reviewer, + requested_approvals_reviewer: self + .original_config_do_not_use + .requested_approvals_reviewer, permission_profile: self.materialized_permission_profile(environment_selections), active_permission_profile: self.active_permission_profile(), cwd: self.legacy_fallback_cwd.clone(), @@ -369,6 +372,9 @@ impl SessionConfiguration { ), approval_policy: Some(self.step_settings.approval_policy.value()), approvals_reviewer: Some(self.step_settings.approvals_reviewer), + requested_approvals_reviewer: self + .original_config_do_not_use + .requested_approvals_reviewer, permission_profile: Some(self.permission_profile()), active_permission_profile: self.active_permission_profile(), windows_sandbox_level: Some(self.windows_sandbox_level), @@ -575,6 +581,17 @@ impl SessionConfiguration { &updates.step_settings, &next_configuration.step_settings_constraints(next_environments), )?); + if let Some(reviewer) = updates + .requested_approvals_reviewer + .or(updates.step_settings.approvals_reviewer) + && next_configuration + .original_config_do_not_use + .requested_approvals_reviewer + != Some(reviewer) + { + Arc::make_mut(&mut next_configuration.original_config_do_not_use) + .requested_approvals_reviewer = Some(reviewer); + } Ok(next_configuration) } @@ -624,6 +641,7 @@ pub(crate) struct SessionSettingsCommit { #[derive(Default, Clone)] pub(crate) struct SessionSettingsUpdate { pub(crate) step_settings: StepSettingsUpdate, + pub(crate) requested_approvals_reviewer: Option, /// Omission preserves the current data; an empty initializer clears it. pub(crate) turn_extension_init: Option, pub(crate) environments: Option, diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 6a6bef52a5..e6d7448dfe 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -2483,6 +2483,11 @@ pub struct ThreadSettingsSnapshot { pub service_tier: Option, pub approval_policy: AskForApproval, pub approvals_reviewer: ApprovalsReviewer, + /// Original selection before managed policy resolved the effective reviewer. + /// Older histories contain only `approvals_reviewer`. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional)] + pub requested_approvals_reviewer: Option, pub permission_profile: PermissionProfile, #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional)] diff --git a/codex-rs/state/src/extract.rs b/codex-rs/state/src/extract.rs index 2b22fd5d78..35358a62b1 100644 --- a/codex-rs/state/src/extract.rs +++ b/codex-rs/state/src/extract.rs @@ -745,6 +745,7 @@ mod tests { service_tier: None, approval_policy: AskForApproval::Never, approvals_reviewer: ApprovalsReviewer::User, + requested_approvals_reviewer: None, permission_profile: permission_profile.clone(), active_permission_profile: None, cwd: cwd.clone().try_into().expect("absolute settings cwd"), diff --git a/codex-rs/thread-manager-sample/src/main.rs b/codex-rs/thread-manager-sample/src/main.rs index ecee9645d4..1aab982144 100644 --- a/codex-rs/thread-manager-sample/src/main.rs +++ b/codex-rs/thread-manager-sample/src/main.rs @@ -265,6 +265,7 @@ async fn new_config( explicit_permission_profile_mode: false, custom_permission_profiles: Vec::new(), approvals_reviewer: ApprovalsReviewer::User, + requested_approvals_reviewer: None, enforce_residency: Constrained::allow_any(/*initial_value*/ None), hide_agent_reasoning: false, show_raw_agent_reasoning: false, diff --git a/codex-rs/thread-store/src/thread_metadata_sync.rs b/codex-rs/thread-store/src/thread_metadata_sync.rs index c62bfe1bea..6f1a7d1791 100644 --- a/codex-rs/thread-store/src/thread_metadata_sync.rs +++ b/codex-rs/thread-store/src/thread_metadata_sync.rs @@ -750,6 +750,7 @@ mod tests { service_tier: None, approval_policy: AskForApproval::Never, approvals_reviewer: ApprovalsReviewer::User, + requested_approvals_reviewer: None, permission_profile: permission_profile.clone(), active_permission_profile: None, cwd: cwd.clone().try_into().expect("absolute settings cwd"), diff --git a/codex-rs/tui/src/app/misalignment_policy.rs b/codex-rs/tui/src/app/misalignment_policy.rs index eece358c3b..7eb6f5b58a 100644 --- a/codex-rs/tui/src/app/misalignment_policy.rs +++ b/codex-rs/tui/src/app/misalignment_policy.rs @@ -171,7 +171,6 @@ impl App { params.runtime_workspace_roots = Some(config.permissions.user_visible_workspace_roots().to_vec()); params.approval_policy = Some(config.permissions.approval_policy.value().into()); - params.approvals_reviewer = Some(config.approvals_reviewer.into()); params.sandbox_policy = sandbox_policy; params.permissions = permissions; } diff --git a/codex-rs/tui/src/app/safety_buffering.rs b/codex-rs/tui/src/app/safety_buffering.rs index 72da56bfcd..659bcbacaf 100644 --- a/codex-rs/tui/src/app/safety_buffering.rs +++ b/codex-rs/tui/src/app/safety_buffering.rs @@ -62,7 +62,6 @@ impl App { cwd, active_permission_profile, approval_policy, - approvals_reviewer, model: turn_model, effort, collaboration_mode, @@ -217,7 +216,6 @@ impl App { }; let retry_thread_id = started.session.thread_id; *approval_policy = started.session.approval_policy; - *approvals_reviewer = Some(started.session.approvals_reviewer); *active_permission_profile = started.session.active_permission_profile.clone(); self.detach_current_thread_for_navigation(app_server, Some(retry_thread_id)) diff --git a/codex-rs/tui/src/app/tests/misalignment_policy_tests.rs b/codex-rs/tui/src/app/tests/misalignment_policy_tests.rs index 04e554de99..29ced74fcc 100644 --- a/codex-rs/tui/src/app/tests/misalignment_policy_tests.rs +++ b/codex-rs/tui/src/app/tests/misalignment_policy_tests.rs @@ -252,7 +252,7 @@ async fn misalignment_continuation_requires_current_review_and_submits_once() -> runtime_workspace_roots: (!server_owned) .then(|| config.permissions.user_visible_workspace_roots().to_vec()), approval_policy: (!server_owned).then_some(AskForApproval::OnRequest), - approvals_reviewer: (!server_owned).then(|| config.approvals_reviewer.into()), + approvals_reviewer: None, sandbox_policy: (!server_owned && !preserve) .then(|| config.legacy_sandbox_policy().into()), cyber_access_program: rollout_enabled.then_some(if daybreak { diff --git a/codex-rs/tui/src/app/tests/startup_defaults_tests.rs b/codex-rs/tui/src/app/tests/startup_defaults_tests.rs index bed5ce30b0..ea537b9f80 100644 --- a/codex-rs/tui/src/app/tests/startup_defaults_tests.rs +++ b/codex-rs/tui/src/app/tests/startup_defaults_tests.rs @@ -517,8 +517,20 @@ requires_openai_auth = false serde_json::json!({"cwd": server_config.cwd.display().to_string(), "includeLayers": true}) ], ); - if choice == "saved" { - assert_eq!(started.session.approvals_reviewer, ApprovalsReviewer::User); + if matches!(choice, "saved" | "embedded_saved") { + let expected_turn_policy = if choice == "saved" { + AskForApproval::OnRequest + } else { + AskForApproval::Never + }; + assert_eq!( + started.session.approvals_reviewer, + if choice == "saved" { + ApprovalsReviewer::User + } else { + ApprovalsReviewer::AutoReview + } + ); let (mut app, _events, mut ops) = make_test_app_with_channels().await; app.app_server_target = target; app.config.permissions.approval_policy = @@ -530,7 +542,7 @@ requires_openai_auth = false .await?; assert_eq!( app.config.permissions.approval_policy.value(), - AskForApproval::OnRequest.to_core() + expected_turn_policy.to_core() ); app.chat_widget.insert_str("first turn"); app.chat_widget @@ -538,8 +550,11 @@ requires_openai_auth = false app.submit_thread_op(&mut server, thread_id, next_user_turn_op(&mut ops)) .await?; let turn = &recorded_params(&requests, "turn/start")[0]; - assert_eq!(turn["approvalsReviewer"], "user"); - assert_eq!(turn["approvalPolicy"], "on-request"); + assert!(turn["approvalsReviewer"].is_null()); + assert_eq!( + turn["approvalPolicy"], + serde_json::json!(expected_turn_policy) + ); } server.shutdown().await?; proxy.await??; diff --git a/codex-rs/tui/src/app/user_turn_submission.rs b/codex-rs/tui/src/app/user_turn_submission.rs index 229d21e22b..cdbda63973 100644 --- a/codex-rs/tui/src/app/user_turn_submission.rs +++ b/codex-rs/tui/src/app/user_turn_submission.rs @@ -120,6 +120,7 @@ pub(super) async fn try_submit_user_turn( }); let selected_active = selected_profile.map(|profile| ActivePermissionProfile::new(profile.profile_id.clone())); + // Ordinary turns inherit the server reviewer; echoing its effective value loses the request. let (turn_approval_policy, turn_approvals_reviewer) = if let Some(profile) = selected_profile { ( profile.approval_policy, @@ -130,17 +131,10 @@ pub(super) async fn try_submit_user_turn( { ( Some(config.permissions.approval_policy.value().into()), - Some(config.approvals_reviewer.into()), + approvals_reviewer.map(Into::into), ) } else { - ( - Some(*approval_policy), - Some( - approvals_reviewer - .unwrap_or(config.approvals_reviewer) - .into(), - ), - ) + (Some(*approval_policy), approvals_reviewer.map(Into::into)) }; // Only explicit choices may override the saved server profile. let explicit_profile = app diff --git a/codex-rs/tui/src/app_server_session.rs b/codex-rs/tui/src/app_server_session.rs index 78f5ab59f1..38ada25c33 100644 --- a/codex-rs/tui/src/app_server_session.rs +++ b/codex-rs/tui/src/app_server_session.rs @@ -1274,6 +1274,12 @@ impl AppServerSession { // Active-session config has already adopted the server's roots. Fork does // not restore saved roots when omitted, so preserve this explicit selection. params.runtime_workspace_roots = Some(config.workspace_roots.clone()); + if selected_profile.is_none() { + params.approvals_reviewer = None; + if let Some(overrides) = params.config.as_mut() { + overrides.remove("approvals_reviewer"); + } + } } if self.thread_params_mode() == ThreadParamsMode::Remote && permission_mode == ForkPermissionMode::InheritSaved