From 5a7e9a75c943a712b0deb5ed87437eed90f476fa Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 11 Jun 2026 11:50:06 -0400 Subject: [PATCH] permissions: apply reviewer overrides in core --- codex-rs/core/src/codex_delegate.rs | 136 +++++++++--------- codex-rs/core/src/codex_delegate_tests.rs | 1 + codex-rs/core/src/guardian/mod.rs | 1 - codex-rs/core/src/guardian/review.rs | 7 - codex-rs/core/src/guardian/tests.rs | 15 +- codex-rs/core/src/session/mod.rs | 77 +++++++--- codex-rs/core/src/session/tests.rs | 65 ++++++++- .../core/src/session/tests/guardian_tests.rs | 2 + codex-rs/core/src/state/turn.rs | 15 +- codex-rs/core/src/tools/network_approval.rs | 8 +- codex-rs/core/src/tools/orchestrator.rs | 9 +- .../tools/runtimes/shell/unix_escalation.rs | 9 +- codex-rs/core/tests/suite/auto_review.rs | 1 + codex-rs/core/tests/suite/remote_env.rs | 1 + .../core/tests/suite/request_permissions.rs | 8 ++ 15 files changed, 243 insertions(+), 112 deletions(-) diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 5b68ecf917..54ce4ceff7 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -33,7 +33,6 @@ use tokio_util::sync::CancellationToken; use crate::config::Config; use crate::guardian::GuardianApprovalRequest; use crate::guardian::new_guardian_review_id; -use crate::guardian::routes_approval_to_guardian; use crate::guardian::routes_approval_to_guardian_with_reviewer; use crate::guardian::spawn_approval_request_review; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT; @@ -460,7 +459,10 @@ async fn handle_exec_approval( available_decisions, .. } = event; - let decision = if routes_approval_to_guardian(parent_ctx) { + let approvals_reviewer = parent_session + .approvals_reviewer_for_turn(parent_ctx.config.approvals_reviewer) + .await; + let decision = if routes_approval_to_guardian_with_reviewer(parent_ctx, approvals_reviewer) { let review_cancel = cancel_token.child_token(); let review_rx = spawn_approval_request_review( Arc::clone(parent_session), @@ -538,70 +540,74 @@ async fn handle_patch_approval( .. } = event; let approval_id = call_id.clone(); - let guardian_decision = if routes_approval_to_guardian(parent_ctx) { - let files = changes - .keys() - .map(|path| { - #[allow(deprecated)] - parent_ctx.cwd.join(path) - }) - .collect::>(); - let review_cancel = cancel_token.child_token(); - let patch = changes - .iter() - .map(|(path, change)| match change { - codex_protocol::protocol::FileChange::Add { content } => { - format!("*** Add File: {}\n{}", path.display(), content) - } - codex_protocol::protocol::FileChange::Delete { content } => { - format!("*** Delete File: {}\n{}", path.display(), content) - } - codex_protocol::protocol::FileChange::Update { - unified_diff, - move_path, - } => { - if let Some(move_path) = move_path { - format!( - "*** Update File: {}\n*** Move to: {}\n{}", - path.display(), - move_path.display(), - unified_diff - ) - } else { - format!("*** Update File: {}\n{}", path.display(), unified_diff) + let approvals_reviewer = parent_session + .approvals_reviewer_for_turn(parent_ctx.config.approvals_reviewer) + .await; + let guardian_decision = + if routes_approval_to_guardian_with_reviewer(parent_ctx, approvals_reviewer) { + let files = changes + .keys() + .map(|path| { + #[allow(deprecated)] + parent_ctx.cwd.join(path) + }) + .collect::>(); + let review_cancel = cancel_token.child_token(); + let patch = changes + .iter() + .map(|(path, change)| match change { + codex_protocol::protocol::FileChange::Add { content } => { + format!("*** Add File: {}\n{}", path.display(), content) } - } - }) - .collect::>() - .join("\n"); - let review_rx = spawn_approval_request_review( - Arc::clone(parent_session), - Arc::clone(parent_ctx), - new_guardian_review_id(), - GuardianApprovalRequest::ApplyPatch { - id: approval_id.clone(), - #[allow(deprecated)] - cwd: parent_ctx.cwd.clone(), - files, - patch, - }, - reason.clone(), - GuardianApprovalRequestSource::DelegatedSubagent, - review_cancel.clone(), - ); - Some( - await_approval_with_cancel( - async move { review_rx.await.unwrap_or_default() }, - parent_session, - &approval_id, - cancel_token, - Some(&review_cancel), + codex_protocol::protocol::FileChange::Delete { content } => { + format!("*** Delete File: {}\n{}", path.display(), content) + } + codex_protocol::protocol::FileChange::Update { + unified_diff, + move_path, + } => { + if let Some(move_path) = move_path { + format!( + "*** Update File: {}\n*** Move to: {}\n{}", + path.display(), + move_path.display(), + unified_diff + ) + } else { + format!("*** Update File: {}\n{}", path.display(), unified_diff) + } + } + }) + .collect::>() + .join("\n"); + let review_rx = spawn_approval_request_review( + Arc::clone(parent_session), + Arc::clone(parent_ctx), + new_guardian_review_id(), + GuardianApprovalRequest::ApplyPatch { + id: approval_id.clone(), + #[allow(deprecated)] + cwd: parent_ctx.cwd.clone(), + files, + patch, + }, + reason.clone(), + GuardianApprovalRequestSource::DelegatedSubagent, + review_cancel.clone(), + ); + Some( + await_approval_with_cancel( + async move { review_rx.await.unwrap_or_default() }, + parent_session, + &approval_id, + cancel_token, + Some(&review_cancel), + ) + .await, ) - .await, - ) - } else { - None - }; + } else { + None + }; let decision = if let Some(decision) = guardian_decision { decision } else { @@ -823,6 +829,7 @@ where permissions: Default::default(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }; parent_session .notify_request_permissions_response(call_id, empty.clone()) @@ -833,6 +840,7 @@ where permissions: Default::default(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }), } } diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index cb7767a16c..2cb9e8b7d1 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -210,6 +210,7 @@ async fn handle_request_permissions_uses_tool_call_id_for_round_trip() { }, scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }; #[allow(deprecated)] let delegated_cwd = parent_ctx.cwd.join("delegated-cwd"); diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index b06a2daad3..0984e5b022 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -38,7 +38,6 @@ pub(crate) use review::record_guardian_denial_for_test; pub(crate) use review::review_approval_request; #[cfg(test)] pub(crate) use review::review_approval_request_with_cancel; -pub(crate) use review::routes_approval_to_guardian; pub(crate) use review::routes_approval_to_guardian_with_reviewer; pub(crate) use review::spawn_approval_request_review; pub(crate) use review_session::GuardianReviewSessionManager; diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 99f9c5d999..9c4550f66a 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -162,13 +162,6 @@ fn guardian_risk_level_str(level: GuardianRiskLevel) -> &'static str { } } -/// Whether this turn should route allowed approval prompts through the guardian -/// reviewer instead of surfacing them to the user. ARC may still block actions -/// earlier in the flow. -pub(crate) fn routes_approval_to_guardian(turn: &TurnContext) -> bool { - routes_approval_to_guardian_with_reviewer(turn, turn.config.approvals_reviewer) -} - /// Whether an approval with its own reviewer selection should be routed through guardian. pub(crate) fn routes_approval_to_guardian_with_reviewer( turn: &TurnContext, diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 8f6e01298c..bab1038358 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -1137,12 +1137,18 @@ async fn routes_approval_to_guardian_requires_guardian_reviewer() { config.approvals_reviewer = ApprovalsReviewer::User; turn.config = Arc::new(config.clone()); - assert!(!routes_approval_to_guardian(&turn)); + assert!(!routes_approval_to_guardian_with_reviewer( + &turn, + turn.config.approvals_reviewer + )); config.approvals_reviewer = ApprovalsReviewer::AutoReview; turn.config = Arc::new(config); - assert!(routes_approval_to_guardian(&turn)); + assert!(routes_approval_to_guardian_with_reviewer( + &turn, + turn.config.approvals_reviewer + )); } #[tokio::test] @@ -1175,7 +1181,10 @@ async fn routes_approval_to_guardian_allows_granular_review_policy() { })) .expect("test setup should allow updating approval policy"); - assert!(routes_approval_to_guardian(&turn)); + assert!(routes_approval_to_guardian_with_reviewer( + &turn, + turn.config.approvals_reviewer + )); } #[test] diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index ddb31a9a52..982b3d37a1 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2151,6 +2151,7 @@ impl Session { permissions: RequestPermissionProfile::default(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }); } AskForApproval::Granular(granular_config) @@ -2160,6 +2161,7 @@ impl Session { permissions: RequestPermissionProfile::default(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }); } AskForApproval::OnFailure @@ -2170,7 +2172,13 @@ impl Session { let requested_permissions = args.permissions; - if crate::guardian::routes_approval_to_guardian(turn_context.as_ref()) { + let approvals_reviewer = self + .approvals_reviewer_for_turn(turn_context.config.approvals_reviewer) + .await; + if crate::guardian::routes_approval_to_guardian_with_reviewer( + turn_context.as_ref(), + approvals_reviewer, + ) { let originating_turn_state = { let active = self.active_turn.lock().await; active.as_ref().map(|active| Arc::clone(&active.turn_state)) @@ -2204,12 +2212,14 @@ impl Session { permissions: requested_permissions.clone(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, } } ReviewDecision::ApprovedForSession => RequestPermissionsResponse { permissions: requested_permissions.clone(), scope: PermissionGrantScope::Session, strict_auto_review: false, + approvals_reviewer: None, }, ReviewDecision::NetworkPolicyAmendment { network_policy_amendment, @@ -2218,11 +2228,13 @@ impl Session { permissions: requested_permissions.clone(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }, NetworkPolicyRuleAction::Deny => RequestPermissionsResponse { permissions: RequestPermissionProfile::default(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }, }, ReviewDecision::Abort | ReviewDecision::Denied | ReviewDecision::TimedOut => { @@ -2230,6 +2242,7 @@ impl Session { permissions: RequestPermissionProfile::default(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, } } }; @@ -2314,6 +2327,7 @@ impl Session { permissions: RequestPermissionProfile::default(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }); }; let mut environment = turn_environment.selection(); @@ -2443,14 +2457,6 @@ impl Session { response: RequestPermissionsResponse, cwd: &Path, ) -> RequestPermissionsResponse { - if response.strict_auto_review && matches!(response.scope, PermissionGrantScope::Session) { - return RequestPermissionsResponse { - permissions: RequestPermissionProfile::default(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - }; - } - if response.permissions.is_empty() { return response; } @@ -2464,6 +2470,7 @@ impl Session { .into(), scope: response.scope, strict_auto_review: response.strict_auto_review, + approvals_reviewer: response.approvals_reviewer, } } @@ -2473,27 +2480,37 @@ impl Session { environment_id: &str, originating_turn_state: Option<&Arc>>, ) { - if response.permissions.is_empty() { - return; - } + let approvals_reviewer = response.approvals_reviewer_override(); match response.scope { PermissionGrantScope::Turn => { if let Some(turn_state) = originating_turn_state { let mut ts = turn_state.lock().await; - let permissions: AdditionalPermissionProfile = - response.permissions.clone().into(); - ts.record_granted_permissions(environment_id, permissions); - if response.strict_auto_review { - ts.enable_strict_auto_review(); + if !response.permissions.is_empty() { + let permissions: AdditionalPermissionProfile = + response.permissions.clone().into(); + ts.record_granted_permissions(environment_id, permissions); + } + if let Some(approvals_reviewer) = approvals_reviewer { + ts.set_approvals_reviewer_override(approvals_reviewer); } } } PermissionGrantScope::Session => { - let mut state = self.state.lock().await; - state.record_granted_permissions( - environment_id, - response.permissions.clone().into(), - ); + if !response.permissions.is_empty() { + let mut state = self.state.lock().await; + state.record_granted_permissions( + environment_id, + response.permissions.clone().into(), + ); + } + if let (Some(turn_state), Some(approvals_reviewer)) = + (originating_turn_state, approvals_reviewer) + { + turn_state + .lock() + .await + .set_approvals_reviewer_override(approvals_reviewer); + } } } } @@ -2525,6 +2542,22 @@ impl Session { ts.strict_auto_review_enabled() } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn reads must stay consistent with the matching turn state" + )] + pub(crate) async fn approvals_reviewer_for_turn( + &self, + fallback: ApprovalsReviewer, + ) -> ApprovalsReviewer { + let active = self.active_turn.lock().await; + let Some(active) = active.as_ref() else { + return fallback; + }; + let ts = active.turn_state.lock().await; + ts.approvals_reviewer_override().unwrap_or(fallback) + } + pub(crate) async fn granted_session_permissions( &self, environment_id: &str, diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 29de7ca662..e4a97f55c8 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -5258,6 +5258,7 @@ async fn notify_request_permissions_response_ignores_unmatched_call_id() { }, scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }, ) .await; @@ -5293,6 +5294,7 @@ async fn record_granted_request_permissions_for_turn_uses_originating_turn() { permissions: requested_permissions.clone(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }, codex_exec_server::LOCAL_ENVIRONMENT_ID, Some(&originating_turn_state), @@ -5340,6 +5342,7 @@ async fn request_permission_grants_are_environment_keyed() { permissions: requested_permissions.clone(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }, "remote", Some(&originating_turn_state), @@ -5361,6 +5364,7 @@ async fn request_permission_grants_are_environment_keyed() { permissions: requested_permissions.clone(), scope: PermissionGrantScope::Session, strict_auto_review: false, + approvals_reviewer: None, }, "remote", /*originating_turn_state*/ None, @@ -5393,6 +5397,7 @@ async fn enable_strict_auto_review_for_turn_uses_originating_turn() { permissions: requested_permissions.clone(), scope: PermissionGrantScope::Turn, strict_auto_review: true, + approvals_reviewer: None, }, codex_exec_server::LOCAL_ENVIRONMENT_ID, Some(&originating_turn_state), @@ -5408,7 +5413,7 @@ async fn enable_strict_auto_review_for_turn_uses_originating_turn() { } #[test] -fn strict_auto_review_session_scope_grants_no_permissions() { +fn strict_auto_review_session_scope_preserves_session_grant() { let requested_permissions = RequestPermissionProfile { network: Some(codex_protocol::models::NetworkPermissions { enabled: Some(true), @@ -5422,6 +5427,7 @@ fn strict_auto_review_session_scope_grants_no_permissions() { permissions: requested_permissions, scope: PermissionGrantScope::Session, strict_auto_review: true, + approvals_reviewer: None, }, std::path::Path::new("/tmp"), ); @@ -5429,13 +5435,59 @@ fn strict_auto_review_session_scope_grants_no_permissions() { assert_eq!( response, codex_protocol::request_permissions::RequestPermissionsResponse { - permissions: RequestPermissionProfile::default(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, + permissions: RequestPermissionProfile { + network: Some(codex_protocol::models::NetworkPermissions { + enabled: Some(true), + }), + ..RequestPermissionProfile::default() + }, + scope: PermissionGrantScope::Session, + strict_auto_review: true, + approvals_reviewer: None, } ); } +#[tokio::test] +async fn session_scope_auto_review_records_session_grant_and_turn_reviewer_override() { + let (session, _turn_context) = make_session_and_context().await; + let originating_active_turn = ActiveTurn::default(); + let originating_turn_state = Arc::clone(&originating_active_turn.turn_state); + *session.active_turn.lock().await = Some(originating_active_turn); + + let requested_permissions = RequestPermissionProfile { + network: Some(codex_protocol::models::NetworkPermissions { + enabled: Some(true), + }), + ..RequestPermissionProfile::default() + }; + session + .record_granted_request_permissions_for_turn( + &codex_protocol::request_permissions::RequestPermissionsResponse { + permissions: requested_permissions.clone(), + scope: PermissionGrantScope::Session, + strict_auto_review: true, + approvals_reviewer: None, + }, + codex_exec_server::LOCAL_ENVIRONMENT_ID, + Some(&originating_turn_state), + ) + .await; + + assert_eq!( + session + .granted_session_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID) + .await, + Some(requested_permissions.into()) + ); + assert!( + originating_turn_state + .lock() + .await + .strict_auto_review_enabled() + ); +} + #[tokio::test] async fn request_permissions_emits_event_when_granular_policy_allows_requests() { let (session, mut turn_context, rx) = make_session_and_context_with_rx().await; @@ -5464,6 +5516,7 @@ async fn request_permissions_emits_event_when_granular_policy_allows_requests() }, scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }; let handle = tokio::spawn({ @@ -5619,6 +5672,7 @@ async fn request_permissions_tool_resolves_relative_paths_against_selected_envir permissions: request.permissions, scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }, ) .await; @@ -5740,6 +5794,7 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec permissions: request.permissions, scope: PermissionGrantScope::Session, strict_auto_review: false, + approvals_reviewer: None, }, ) .await; @@ -5755,6 +5810,7 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec permissions: expected_permissions.clone(), scope: PermissionGrantScope::Session, strict_auto_review: false, + approvals_reviewer: None, }; let response = tokio::time::timeout(StdDuration::from_secs(1), handle) @@ -5821,6 +5877,7 @@ async fn request_permissions_is_auto_denied_when_granular_policy_blocks_tool_req permissions: RequestPermissionProfile::default(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, } ) ); diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index c73add2920..8a031f8f4a 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -152,6 +152,7 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() { permissions: requested_permissions.clone(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }) ); assert_eq!( @@ -400,6 +401,7 @@ async fn strict_auto_review_turn_grant_forces_guardian_for_shell_command_policy_ }, scope: PermissionGrantScope::Turn, strict_auto_review: true, + approvals_reviewer: None, }, codex_exec_server::LOCAL_ENVIRONMENT_ID, Some(&originating_turn_state), diff --git a/codex-rs/core/src/state/turn.rs b/codex-rs/core/src/state/turn.rs index d1e31b7994..407b966b6a 100644 --- a/codex-rs/core/src/state/turn.rs +++ b/codex-rs/core/src/state/turn.rs @@ -8,6 +8,7 @@ use tokio_util::sync::CancellationToken; use tokio_util::task::AbortOnDropHandle; use codex_extension_api::ExtensionData; +use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::dynamic_tools::DynamicToolResponse; use codex_protocol::protocol::TurnEnvironmentSelection; use codex_protocol::request_permissions::RequestPermissionProfile; @@ -93,7 +94,7 @@ pub(crate) struct TurnState { pub(crate) pending_input: TurnInputQueue, mailbox_delivery_phase: MailboxDeliveryPhase, granted_permissions_by_environment_id: HashMap, - strict_auto_review_enabled: bool, + approvals_reviewer_override: Option, pub(crate) tool_calls: u64, pub(crate) has_memory_citation: bool, pub(crate) token_usage_at_turn_start: TokenUsage, @@ -231,11 +232,15 @@ impl TurnState { .cloned() } - pub(crate) fn enable_strict_auto_review(&mut self) { - self.strict_auto_review_enabled = true; + pub(crate) fn strict_auto_review_enabled(&self) -> bool { + self.approvals_reviewer_override == Some(ApprovalsReviewer::AutoReview) } - pub(crate) fn strict_auto_review_enabled(&self) -> bool { - self.strict_auto_review_enabled + pub(crate) fn set_approvals_reviewer_override(&mut self, reviewer: ApprovalsReviewer) { + self.approvals_reviewer_override = Some(reviewer); + } + + pub(crate) fn approvals_reviewer_override(&self) -> Option { + self.approvals_reviewer_override } } diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 9cc808a5b6..b9b6f81a90 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -4,7 +4,7 @@ use crate::guardian::guardian_rejection_message; use crate::guardian::guardian_timeout_message; use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; -use crate::guardian::routes_approval_to_guardian; +use crate::guardian::routes_approval_to_guardian_with_reviewer; use crate::hook_runtime::run_permission_request_hooks; use crate::network_policy_decision::denied_network_policy_message; use crate::session::session::Session; @@ -496,7 +496,11 @@ impl NetworkApprovalService { } } } - let use_guardian = routes_approval_to_guardian(&turn_context); + let approvals_reviewer = session + .approvals_reviewer_for_turn(turn_context.config.approvals_reviewer) + .await; + let use_guardian = + routes_approval_to_guardian_with_reviewer(&turn_context, approvals_reviewer); let guardian_review_id = use_guardian.then(new_guardian_review_id); let approval_decision = if let Some(review_id) = guardian_review_id.clone() { review_approval_request( diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 77cbffde32..dc78e0c517 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -9,7 +9,7 @@ caching). use crate::guardian::guardian_rejection_message; use crate::guardian::guardian_timeout_message; use crate::guardian::new_guardian_review_id; -use crate::guardian::routes_approval_to_guardian; +use crate::guardian::routes_approval_to_guardian_with_reviewer; use crate::hook_runtime::run_permission_request_hooks; use crate::network_policy_decision::network_approval_context_from_payload; use crate::tools::flat_tool_name; @@ -142,8 +142,13 @@ impl ToolOrchestrator { let otel = turn_ctx.session_telemetry.clone(); let otel_tn = flat_tool_name(&tool_ctx.tool_name).into_owned(); let otel_ci = &tool_ctx.call_id; + let approvals_reviewer = tool_ctx + .session + .approvals_reviewer_for_turn(turn_ctx.config.approvals_reviewer) + .await; let strict_auto_review = tool_ctx.session.strict_auto_review_enabled_for_turn().await; - let use_guardian = routes_approval_to_guardian(turn_ctx) || strict_auto_review; + let use_guardian = routes_approval_to_guardian_with_reviewer(turn_ctx, approvals_reviewer) + || strict_auto_review; // 1) Approval let mut already_approved = false; diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 71fa89f980..d1bfa1c8ef 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -8,7 +8,7 @@ use crate::guardian::guardian_rejection_message; use crate::guardian::guardian_timeout_message; use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; -use crate::guardian::routes_approval_to_guardian; +use crate::guardian::routes_approval_to_guardian_with_reviewer; use crate::hook_runtime::run_permission_request_hooks; use crate::sandboxing::ExecOptions; use crate::sandboxing::ExecRequest; @@ -422,7 +422,6 @@ impl CoreShellActionProvider { let call_id = self.call_id.clone(); let approval_id = Some(Uuid::new_v4().to_string()); let source = self.tool_name; - let guardian_review_id = routes_approval_to_guardian(&turn).then(new_guardian_review_id); Ok(stopwatch .pause_for(async move { // 1) Run PermissionRequest hooks @@ -457,6 +456,12 @@ impl CoreShellActionProvider { } // 2) Route to Guardian if configured + let approvals_reviewer = session + .approvals_reviewer_for_turn(turn.config.approvals_reviewer) + .await; + let guardian_review_id = + routes_approval_to_guardian_with_reviewer(&turn, approvals_reviewer) + .then(new_guardian_review_id); if let Some(review_id) = guardian_review_id.clone() { let decision = review_approval_request( &session, diff --git a/codex-rs/core/tests/suite/auto_review.rs b/codex-rs/core/tests/suite/auto_review.rs index 5fe1cbc2f2..aa6f6fb08d 100644 --- a/codex-rs/core/tests/suite/auto_review.rs +++ b/codex-rs/core/tests/suite/auto_review.rs @@ -192,6 +192,7 @@ async fn remote_model_override_uses_catalog_model_for_strict_auto_review() -> Re permissions: permissions_request.permissions, scope: PermissionGrantScope::Turn, strict_auto_review: true, + approvals_reviewer: None, }, }) .await?; diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index 00f5a0c514..aac5e4a744 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -409,6 +409,7 @@ async fn remote_request_permissions_grant_unblocks_later_remote_exec() -> Result permissions: expected_permissions.clone(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }; let command = format!( "printf 'remote-request-permissions-ok' > {relative_target_path} && cat {relative_target_path}" diff --git a/codex-rs/core/tests/suite/request_permissions.rs b/codex-rs/core/tests/suite/request_permissions.rs index f78ccf1ac7..e730a13184 100644 --- a/codex-rs/core/tests/suite/request_permissions.rs +++ b/codex-rs/core/tests/suite/request_permissions.rs @@ -501,6 +501,7 @@ async fn request_permissions_tool_is_auto_denied_when_granular_request_permissio permissions: RequestPermissionProfile::default(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, } ); @@ -1126,6 +1127,7 @@ async fn request_permissions_grants_apply_to_later_exec_command_calls() -> Resul permissions: normalized_requested_permissions.clone(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }, }) .await?; @@ -1244,6 +1246,7 @@ async fn request_permissions_preapprove_explicit_exec_permissions_outside_on_req permissions: normalized_requested_permissions, scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }, }) .await?; @@ -1361,6 +1364,7 @@ async fn request_permissions_grants_apply_to_later_shell_command_calls() -> Resu permissions: normalized_requested_permissions.clone(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }, }) .await?; @@ -1474,6 +1478,7 @@ async fn request_permissions_grants_apply_to_later_shell_command_calls_without_i permissions: normalized_requested_permissions.clone(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }, }) .await?; @@ -1624,6 +1629,7 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions() permissions: granted_permissions.clone(), scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }, }) .await?; @@ -1749,6 +1755,7 @@ async fn request_permissions_grants_do_not_carry_across_turns() -> Result<()> { permissions: normalized_requested_permissions, scope: PermissionGrantScope::Turn, strict_auto_review: false, + approvals_reviewer: None, }, }) .await?; @@ -1870,6 +1877,7 @@ async fn request_permissions_session_grants_carry_across_turns() -> Result<()> { permissions: normalized_requested_permissions, scope: PermissionGrantScope::Session, strict_auto_review: false, + approvals_reviewer: None, }, }) .await?;