openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-23 18:08:45 +00:00
Code Issues Packages Projects Releases Wiki Activity

Honor guardian reviews for cached approvals

Browse Source
This commit is contained in:
Abhinav Vedmala
2026-04-15 18:59:14 -07:00
parent 66e17f9881
commit b7d467e29e
4 changed files with 179 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/tools/approval.rs
+68 -1
View File
@@ -51,6 +51,7 @@ pub(crate) fn guardian_review_id_for_turn(turn: &TurnContext) -> Option<String>
pub(crate) async fn with_cached_approval<K, F, Fut>(
services: &SessionServices,
tool_name: &str,
guardian_review_id: Option<&str>,
keys: Vec<K>,
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);
}
}
codex-rs/core/src/tools/runtimes/apply_patch.rs
+26 -20
View File
@@ -155,26 +155,32 @@ impl Approvable<ApplyPatchRequest> 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
})
}
codex-rs/core/src/tools/runtimes/shell.rs
+42 -36
View File
@@ -152,42 +152,48 @@ impl Approvable<ShellRequest> 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
})
}
codex-rs/core/src/tools/runtimes/unified_exec.rs
+43 -37
View File
@@ -130,43 +130,49 @@ impl Approvable<UnifiedExecRequest> 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
})
}