From 44cafe4a8c5f4c3f7ea946e05990a74e41108848 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Mon, 29 Jun 2026 17:07:06 -0400 Subject: [PATCH] chore(approvals) consolidate guardian calls for shell tools --- codex-rs/core/src/session/tests.rs | 16 +++ codex-rs/core/src/tools/orchestrator.rs | 22 +++- .../core/src/tools/runtimes/apply_patch.rs | 30 ++--- .../src/tools/runtimes/apply_patch_tests.rs | 2 +- codex-rs/core/src/tools/runtimes/shell.rs | 42 +++---- .../core/src/tools/runtimes/unified_exec.rs | 44 ++++--- codex-rs/core/src/tools/sandboxing.rs | 4 + codex-rs/core/tests/suite/guardian_review.rs | 116 ++++++++++++++++++ 8 files changed, 210 insertions(+), 66 deletions(-) diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 9ae29a09176e..6816f0a4b37b 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -1039,6 +1039,22 @@ async fn danger_full_access_tool_attempts_do_not_enforce_managed_network() -> an ) -> futures::future::BoxFuture<'a, ReviewDecision> { Box::pin(async { ReviewDecision::Approved }) } + + fn approval_action( + &self, + _req: &(), + ctx: &crate::tools::sandboxing::ApprovalCtx<'_>, + ) -> std::io::Result { + Ok(crate::tools::sandboxing::ApprovalAction::Shell { + id: ctx.call_id.to_string(), + command: Vec::new(), + #[allow(deprecated)] + cwd: ctx.turn.cwd.clone(), + sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, + additional_permissions: None, + justification: None, + }) + } } impl crate::tools::sandboxing::Sandboxable for ProbeToolRuntime { diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index aaa0dfe9d14a..da3e4acddd1c 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -9,6 +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::review_approval_request; use crate::guardian::routes_approval_to_guardian; use crate::hook_runtime::run_permission_request_hooks; use crate::network_policy_decision::network_approval_context_from_payload; @@ -563,7 +564,26 @@ impl ToolOrchestrator { } else { ToolDecisionSource::User }; - let decision = tool.start_approval_async(req, approval_ctx).await; + let decision = if let Some(review_id) = approval_ctx.guardian_review_id.clone() { + match tool.approval_action(req, &approval_ctx) { + Ok(action) => { + review_approval_request( + approval_ctx.session, + approval_ctx.turn, + review_id, + action, + approval_ctx.retry_reason.clone(), + ) + .await + } + Err(err) => { + tracing::error!(%err, "failed to build guardian approval action"); + ReviewDecision::Abort + } + } + } else { + tool.start_approval_async(req, approval_ctx).await + }; let tool_name = flat_tool_name(&tool_ctx.tool_name); otel.tool_decision( tool_name.as_ref(), diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 840a0dd01e73..b839b05c4489 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -4,11 +4,10 @@ //! selected turn environment filesystem for both local and remote turns, with //! sandboxing enforced by the explicit filesystem sandbox context. use crate::exec::is_likely_sandbox_denied; -use crate::guardian::GuardianApprovalRequest; -use crate::guardian::review_approval_request; use crate::session::turn_context::TurnEnvironment; use crate::tools::hook_names::HookToolName; use crate::tools::sandboxing::Approvable; +use crate::tools::sandboxing::ApprovalAction; use crate::tools::sandboxing::ApprovalCtx; use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::PermissionRequestPayload; @@ -77,7 +76,7 @@ impl ApplyPatchRuntime { fn build_guardian_review_request( req: &ApplyPatchRequest, call_id: &str, - ) -> std::io::Result { + ) -> std::io::Result { // TODO(anp): Remove this conversion once the guardian API supports PathUri. let cwd = req.action.cwd.to_abs_path()?; let files = req @@ -85,7 +84,7 @@ impl ApplyPatchRuntime { .iter() .map(PathUri::to_abs_path) .collect::>>()?; - Ok(GuardianApprovalRequest::ApplyPatch { + Ok(ApprovalAction::ApplyPatch { id: call_id.to_string(), cwd, files, @@ -152,22 +151,7 @@ impl Approvable for ApplyPatchRuntime { let retry_reason = ctx.retry_reason.clone(); let approval_keys = self.approval_keys(req); let changes = req.changes.clone(); - let guardian_review_id = ctx.guardian_review_id.clone(); Box::pin(async move { - if let Some(review_id) = guardian_review_id { - let action = match ApplyPatchRuntime::build_guardian_review_request( - req, - ctx.call_id, - ) { - Ok(action) => action, - Err(err) => { - tracing::error!(cwd = %req.action.cwd, %err, "guardian apply_patch cwd is not host-native"); - return ReviewDecision::Abort; - } - }; - return review_approval_request(session, turn, review_id, action, retry_reason) - .await; - } if req.permissions_preapproved && retry_reason.is_none() { return ReviewDecision::Approved; } @@ -201,6 +185,14 @@ impl Approvable for ApplyPatchRuntime { }) } + fn approval_action( + &self, + req: &ApplyPatchRequest, + ctx: &ApprovalCtx<'_>, + ) -> std::io::Result { + ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id) + } + fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool { match policy { AskForApproval::Never => false, diff --git a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs index f1c6f43aa342..a37cd45c1386 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs @@ -80,7 +80,7 @@ async fn guardian_review_request_includes_patch_context() { assert_eq!( guardian_request, - GuardianApprovalRequest::ApplyPatch { + ApprovalAction::ApplyPatch { id: "call-1".to_string(), cwd: expected_cwd, files: vec![path], diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 0d386103db77..3ff50f3f51ad 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -10,9 +10,7 @@ pub(crate) mod zsh_fork_backend; use crate::command_canonicalization::canonicalize_command_for_approval; use crate::exec::ExecCapturePolicy; -use crate::guardian::GuardianApprovalRequest; use crate::guardian::GuardianNetworkAccessTrigger; -use crate::guardian::review_approval_request; use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; use crate::sandboxing::execute_env; @@ -29,6 +27,7 @@ use crate::tools::runtimes::disable_powershell_profile_for_elevated_windows_sand use crate::tools::runtimes::exec_env_for_sandbox_permissions; use crate::tools::runtimes::maybe_wrap_shell_lc_with_snapshot; use crate::tools::sandboxing::Approvable; +use crate::tools::sandboxing::ApprovalAction; use crate::tools::sandboxing::ApprovalCtx; use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::PermissionRequestPayload; @@ -145,30 +144,14 @@ impl Approvable for ShellRuntime { let command = req.command.clone(); let cwd = req.cwd.clone(); let environment_id = Some(req.turn_environment.environment_id.clone()); - let retry_reason = ctx.retry_reason.clone(); - let reason = retry_reason.clone().or_else(|| req.justification.clone()); + let reason = ctx + .retry_reason + .clone() + .or_else(|| req.justification.clone()); let session = ctx.session; let turn = ctx.turn; let call_id = ctx.call_id.to_string(); - let guardian_review_id = ctx.guardian_review_id.clone(); Box::pin(async move { - if let Some(review_id) = guardian_review_id { - return review_approval_request( - session, - turn, - review_id, - GuardianApprovalRequest::Shell { - id: call_id, - command, - cwd: cwd.clone(), - sandbox_permissions: req.sandbox_permissions, - additional_permissions: req.additional_permissions.clone(), - justification: req.justification.clone(), - }, - retry_reason, - ) - .await; - } with_cached_approval(&session.services, "shell", keys, move || async move { let available_decisions = None; session @@ -193,6 +176,21 @@ impl Approvable for ShellRuntime { }) } + fn approval_action( + &self, + req: &ShellRequest, + ctx: &ApprovalCtx<'_>, + ) -> std::io::Result { + Ok(ApprovalAction::Shell { + id: ctx.call_id.to_string(), + command: req.command.clone(), + cwd: req.cwd.clone(), + sandbox_permissions: req.sandbox_permissions, + additional_permissions: req.additional_permissions.clone(), + justification: req.justification.clone(), + }) + } + fn exec_approval_requirement(&self, req: &ShellRequest) -> Option { Some(req.exec_approval_requirement.clone()) } diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 5df42cc979a5..5bb30645e3da 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -7,9 +7,7 @@ the process manager to spawn PTYs once an ExecRequest is prepared. use crate::command_canonicalization::canonicalize_command_for_approval; use crate::exec::ExecCapturePolicy; use crate::exec::ExecExpiration; -use crate::guardian::GuardianApprovalRequest; use crate::guardian::GuardianNetworkAccessTrigger; -use crate::guardian::review_approval_request; use crate::sandboxing::ExecOptions; use crate::sandboxing::ExecServerEnvConfig; use crate::sandboxing::SandboxPermissions; @@ -26,6 +24,7 @@ use crate::tools::runtimes::exec_env_for_sandbox_permissions; use crate::tools::runtimes::maybe_wrap_shell_lc_with_snapshot; use crate::tools::runtimes::shell::zsh_fork_backend; use crate::tools::sandboxing::Approvable; +use crate::tools::sandboxing::ApprovalAction; use crate::tools::sandboxing::ApprovalCtx; use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::PermissionRequestPayload; @@ -179,9 +178,10 @@ impl Approvable for UnifiedExecRuntime<'_> { let call_id = ctx.call_id.to_string(); let command = req.command.clone(); let environment_id = Some(req.turn_environment.environment_id.clone()); - let retry_reason = ctx.retry_reason.clone(); - let reason = retry_reason.clone().or_else(|| req.justification.clone()); - let guardian_review_id = ctx.guardian_review_id.clone(); + let reason = ctx + .retry_reason + .clone() + .or_else(|| req.justification.clone()); Box::pin(async move { let native_cwd = match req.cwd.to_abs_path() { Ok(c) => c, @@ -192,24 +192,6 @@ impl Approvable for UnifiedExecRuntime<'_> { return ReviewDecision::Abort; } }; - if let Some(review_id) = guardian_review_id { - return review_approval_request( - session, - turn, - review_id, - GuardianApprovalRequest::ExecCommand { - id: call_id, - command, - cwd: native_cwd.clone(), - sandbox_permissions: req.sandbox_permissions, - additional_permissions: req.additional_permissions.clone(), - justification: req.justification.clone(), - tty: req.tty, - }, - retry_reason, - ) - .await; - } with_cached_approval(&session.services, "unified_exec", keys, || async move { let available_decisions = None; session @@ -234,6 +216,22 @@ impl Approvable for UnifiedExecRuntime<'_> { }) } + fn approval_action( + &self, + req: &UnifiedExecRequest, + ctx: &ApprovalCtx<'_>, + ) -> std::io::Result { + Ok(ApprovalAction::ExecCommand { + id: ctx.call_id.to_string(), + command: req.command.clone(), + cwd: req.cwd.to_abs_path()?, + sandbox_permissions: req.sandbox_permissions, + additional_permissions: req.additional_permissions.clone(), + justification: req.justification.clone(), + tty: req.tty, + }) + } + fn exec_approval_requirement( &self, req: &UnifiedExecRequest, diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 8e9e274f24af..4c351eeb949c 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -133,6 +133,8 @@ pub(crate) struct ApprovalCtx<'a> { pub network_approval_context: Option, } +pub(crate) type ApprovalAction = crate::guardian::GuardianApprovalRequest; + #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct PermissionRequestPayload { pub tool_name: HookToolName, @@ -368,6 +370,8 @@ pub(crate) trait Approvable { req: &'a Req, ctx: ApprovalCtx<'a>, ) -> BoxFuture<'a, ReviewDecision>; + + fn approval_action(&self, req: &Req, ctx: &ApprovalCtx<'_>) -> std::io::Result; } pub(crate) trait Sandboxable { diff --git a/codex-rs/core/tests/suite/guardian_review.rs b/codex-rs/core/tests/suite/guardian_review.rs index c1da2620332a..383954b81658 100644 --- a/codex-rs/core/tests/suite/guardian_review.rs +++ b/codex-rs/core/tests/suite/guardian_review.rs @@ -20,6 +20,7 @@ use core_test_support::responses::start_mock_server; use core_test_support::responses::start_websocket_server; use core_test_support::skip_if_no_network; use core_test_support::skip_if_sandbox; +use core_test_support::skip_if_wine_exec; use core_test_support::test_codex::local_selections; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; @@ -110,6 +111,121 @@ async fn guardian_session_prewarms_and_is_reused_for_first_review() -> Result<() Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn guardian_denial_rejects_tool_call_with_rationale() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + skip_if_wine_exec!( + Ok(()), + "Guardian approval actions require host-native paths" + ); + + let server = start_mock_server().await; + let approval_policy = AskForApproval::OnRequest; + let sandbox_policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + let sandbox_policy_for_config = sandbox_policy.clone(); + + let mut builder = test_codex().with_config(move |config| { + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config + .set_legacy_sandbox_policy(sandbox_policy_for_config) + .expect("set sandbox policy"); + }); + let test = builder.build_with_auto_env(&server).await?; + + let output_file = test.cwd.path().join("guardian-denied.txt"); + let command = format!("printf should-not-run > {}", output_file.display()); + let tool_args = json!({ + "cmd": command, + "yield_time_ms": 1_000_u64, + "sandbox_permissions": SandboxPermissions::RequireEscalated, + "justification": "Exercise Guardian denial routing.", + }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-parent-tool-denied"), + ev_function_call( + "exec-call-denied", + "exec_command", + &serde_json::to_string(&tool_args)?, + ), + ev_completed("resp-parent-tool-denied"), + ]), + sse(vec![ + ev_response_created("resp-guardian-denied"), + ev_assistant_message( + "msg-guardian-denied", + &json!({ + "risk_level": "high", + "user_authorization": "low", + "outcome": "deny", + "rationale": "The requested write has unacceptable test risk.", + }) + .to_string(), + ), + ev_completed("resp-guardian-denied"), + ]), + sse(vec![ + ev_response_created("resp-parent-after-denial"), + ev_assistant_message("msg-parent-after-denial", "denied"), + ev_completed("resp-parent-after-denial"), + ]), + ], + ) + .await; + + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "run a command that Guardian should deny".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: codex_protocol::protocol::ThreadSettingsOverrides { + approval_policy: Some(approval_policy), + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + sandbox_policy: Some(sandbox_policy), + ..Default::default() + }, + }) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let requests = responses.requests(); + let guardian_request = requests + .iter() + .find(|request| request.body_contains_text("Exercise Guardian denial routing.")) + .expect("expected Guardian review request"); + assert!(guardian_request.body_contains_text(&command)); + + let tool_output = requests + .iter() + .find_map(|request| request.function_call_output_text("exec-call-denied")) + .expect("expected rejected tool output to be returned to the parent model"); + assert!( + tool_output.contains("The requested write has unacceptable test risk."), + "Guardian rationale missing from rejected tool output: {tool_output}" + ); + assert!( + !output_file.exists(), + "Guardian-denied command unexpectedly executed" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn guardian_review_session_does_not_inherit_legacy_notify() -> Result<()> { skip_if_no_network!(Ok(()));