From b7d467e29e98041cf996083fb551510b505df32b Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Wed, 15 Apr 2026 18:59:14 -0700 Subject: [PATCH] Honor guardian reviews for cached approvals --- codex-rs/core/src/tools/approval.rs | 69 +++++++++++++++- .../core/src/tools/runtimes/apply_patch.rs | 46 ++++++----- codex-rs/core/src/tools/runtimes/shell.rs | 78 +++++++++--------- .../core/src/tools/runtimes/unified_exec.rs | 80 ++++++++++--------- 4 files changed, 179 insertions(+), 94 deletions(-) diff --git a/codex-rs/core/src/tools/approval.rs b/codex-rs/core/src/tools/approval.rs index d9bdc74823..dea9e58705 100644 --- a/codex-rs/core/src/tools/approval.rs +++ b/codex-rs/core/src/tools/approval.rs @@ -51,6 +51,7 @@ pub(crate) fn guardian_review_id_for_turn(turn: &TurnContext) -> Option pub(crate) async fn with_cached_approval( services: &SessionServices, tool_name: &str, + guardian_review_id: Option<&str>, keys: Vec, fetch: F, ) -> ReviewDecision @@ -69,7 +70,9 @@ where .all(|key| matches!(store.get(key), Some(ReviewDecision::ApprovedForSession))) }; - if already_approved { + // Guardian-backed approval flows still need a fresh review for each action, + // even when the same request was previously approved for the session. + if already_approved && guardian_review_id.is_none() { return ReviewDecision::ApprovedForSession; } @@ -147,3 +150,67 @@ where ) .await } + +#[cfg(test)] +mod tests { + use super::with_cached_approval; + use crate::codex::make_session_and_context; + use codex_protocol::protocol::ReviewDecision; + use pretty_assertions::assert_eq; + use std::sync::Arc; + use std::sync::atomic::AtomicBool; + use std::sync::atomic::Ordering; + + #[tokio::test] + async fn cached_approval_short_circuits_when_reuse_is_allowed() { + let (session, _turn) = make_session_and_context().await; + let key = "shell"; + session + .services + .tool_approvals + .lock() + .await + .put(key, ReviewDecision::ApprovedForSession); + let fetched = Arc::new(AtomicBool::new(false)); + let fetched_for_closure = Arc::clone(&fetched); + + let decision = + with_cached_approval(&session.services, "shell", None, vec![key], || async move { + fetched_for_closure.store(true, Ordering::SeqCst); + ReviewDecision::Approved + }) + .await; + + assert_eq!(decision, ReviewDecision::ApprovedForSession); + assert_eq!(fetched.load(Ordering::SeqCst), false); + } + + #[tokio::test] + async fn cached_approval_still_fetches_when_fresh_approval_is_required() { + let (session, _turn) = make_session_and_context().await; + let key = "shell"; + session + .services + .tool_approvals + .lock() + .await + .put(key, ReviewDecision::ApprovedForSession); + let fetched = Arc::new(AtomicBool::new(false)); + let fetched_for_closure = Arc::clone(&fetched); + + let decision = with_cached_approval( + &session.services, + "shell", + Some("guardian-review-id"), + vec![key], + || async move { + fetched_for_closure.store(true, Ordering::SeqCst); + ReviewDecision::Approved + }, + ) + .await; + + assert_eq!(decision, ReviewDecision::Approved); + assert_eq!(fetched.load(Ordering::SeqCst), true); + } +} diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index f346f6b7dd..f60fc5d78b 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -155,26 +155,32 @@ impl Approvable for ApplyPatchRuntime { .decision; } - with_cached_approval(&session.services, "apply_patch", approval_keys, || async { - request_approval( - session, - turn, - ctx.guardian_review_id.clone(), - ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id), - retry_reason, - || async move { - let rx_approve = session - .request_patch_approval( - turn, call_id, changes, /*reason*/ None, - /*grant_root*/ None, - ) - .await; - rx_approve.await.unwrap_or_default() - }, - ) - .await - .decision - }) + with_cached_approval( + &session.services, + "apply_patch", + ctx.guardian_review_id.as_deref(), + approval_keys, + || async { + request_approval( + session, + turn, + ctx.guardian_review_id.clone(), + ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id), + retry_reason, + || async move { + let rx_approve = session + .request_patch_approval( + turn, call_id, changes, /*reason*/ None, + /*grant_root*/ None, + ) + .await; + rx_approve.await.unwrap_or_default() + }, + ) + .await + .decision + }, + ) .await }) } diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 34350d3b3d..50181376ef 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -152,42 +152,48 @@ impl Approvable for ShellRuntime { let turn = ctx.turn; let call_id = ctx.call_id.to_string(); Box::pin(async move { - with_cached_approval(&session.services, "shell", keys, || async { - request_approval( - session, - turn, - ctx.guardian_review_id.clone(), - GuardianApprovalRequest::Shell { - id: call_id.clone(), - command: command.clone(), - cwd: cwd.clone(), - sandbox_permissions: req.sandbox_permissions, - additional_permissions: req.additional_permissions.clone(), - justification: req.justification.clone(), - }, - retry_reason, - || async move { - session - .request_command_approval( - turn, - call_id, - /*approval_id*/ None, - command, - cwd, - reason, - ctx.network_approval_context.clone(), - req.exec_approval_requirement - .proposed_execpolicy_amendment() - .cloned(), - req.additional_permissions.clone(), - None, - ) - .await - }, - ) - .await - .decision - }) + with_cached_approval( + &session.services, + "shell", + ctx.guardian_review_id.as_deref(), + keys, + || async { + request_approval( + session, + turn, + ctx.guardian_review_id.clone(), + GuardianApprovalRequest::Shell { + id: call_id.clone(), + command: command.clone(), + cwd: cwd.clone(), + sandbox_permissions: req.sandbox_permissions, + additional_permissions: req.additional_permissions.clone(), + justification: req.justification.clone(), + }, + retry_reason, + || async move { + session + .request_command_approval( + turn, + call_id, + /*approval_id*/ None, + command, + cwd, + reason, + ctx.network_approval_context.clone(), + req.exec_approval_requirement + .proposed_execpolicy_amendment() + .cloned(), + req.additional_permissions.clone(), + None, + ) + .await + }, + ) + .await + .decision + }, + ) .await }) } diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 7604bffbc9..022c779e3b 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -130,43 +130,49 @@ impl Approvable for UnifiedExecRuntime<'_> { let retry_reason = ctx.retry_reason.clone(); let reason = retry_reason.clone().or_else(|| req.justification.clone()); Box::pin(async move { - with_cached_approval(&session.services, "unified_exec", keys, || async { - request_approval( - session, - turn, - ctx.guardian_review_id.clone(), - GuardianApprovalRequest::ExecCommand { - id: call_id.clone(), - command: command.clone(), - cwd: cwd.clone(), - sandbox_permissions: req.sandbox_permissions, - additional_permissions: req.additional_permissions.clone(), - justification: req.justification.clone(), - tty: req.tty, - }, - retry_reason, - || async move { - session - .request_command_approval( - turn, - call_id, - /*approval_id*/ None, - command, - cwd.clone(), - reason, - ctx.network_approval_context.clone(), - req.exec_approval_requirement - .proposed_execpolicy_amendment() - .cloned(), - req.additional_permissions.clone(), - None, - ) - .await - }, - ) - .await - .decision - }) + with_cached_approval( + &session.services, + "unified_exec", + ctx.guardian_review_id.as_deref(), + keys, + || async { + request_approval( + session, + turn, + ctx.guardian_review_id.clone(), + GuardianApprovalRequest::ExecCommand { + id: call_id.clone(), + command: command.clone(), + cwd: cwd.clone(), + sandbox_permissions: req.sandbox_permissions, + additional_permissions: req.additional_permissions.clone(), + justification: req.justification.clone(), + tty: req.tty, + }, + retry_reason, + || async move { + session + .request_command_approval( + turn, + call_id, + /*approval_id*/ None, + command, + cwd.clone(), + reason, + ctx.network_approval_context.clone(), + req.exec_approval_requirement + .proposed_execpolicy_amendment() + .cloned(), + req.additional_permissions.clone(), + None, + ) + .await + }, + ) + .await + .decision + }, + ) .await }) }