From 68281f0d68a8452441911a2850aa47ad6ea9a5b4 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Thu, 9 Jul 2026 15:19:38 -0700 Subject: [PATCH] refactor(approvals): introduce neutral approval action --- codex-rs/core/src/session/tests.rs | 2 +- codex-rs/core/src/tools/approvals.rs | 88 ++++++++++++++++++- .../core/src/tools/runtimes/apply_patch.rs | 22 ++--- .../src/tools/runtimes/apply_patch_tests.rs | 26 ++---- codex-rs/core/src/tools/runtimes/shell.rs | 3 +- .../core/src/tools/runtimes/unified_exec.rs | 2 +- codex-rs/core/src/tools/sandboxing.rs | 2 +- 7 files changed, 106 insertions(+), 39 deletions(-) diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index ac8f9d049e..f159de740c 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -1053,7 +1053,7 @@ async fn danger_full_access_tool_attempts_do_not_enforce_managed_network() -> an id: ctx.call_id.to_string(), command: Vec::new(), #[allow(deprecated)] - cwd: ctx.turn.cwd.clone(), + cwd: PathUri::from_abs_path(&ctx.turn.cwd), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, justification: None, diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index 0d6727d854..60c389f946 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -8,6 +8,7 @@ use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; use crate::guardian::routes_approval_to_guardian_with_reviewer; use crate::hook_runtime::run_permission_request_hooks; +use crate::sandboxing::SandboxPermissions; use crate::session::session::Session; use crate::session::turn_context::TurnContext; use crate::tools::flat_tool_name; @@ -18,10 +19,90 @@ use crate::tools::sandboxing::ToolRuntime; use codex_config::types::ApprovalsReviewer; use codex_hooks::PermissionRequestDecision; use codex_otel::ToolDecisionSource; +use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::protocol::NetworkPolicyRuleAction; use codex_protocol::protocol::ReviewDecision; +use codex_utils_path_uri::PathUri; -pub(super) type ApprovalAction = crate::guardian::GuardianApprovalRequest; +#[derive(Clone, Debug, PartialEq)] +pub(crate) enum ApprovalAction { + Shell { + id: String, + command: Vec, + cwd: PathUri, + sandbox_permissions: SandboxPermissions, + additional_permissions: Option, + justification: Option, + }, + ExecCommand { + id: String, + command: Vec, + cwd: PathUri, + sandbox_permissions: SandboxPermissions, + additional_permissions: Option, + justification: Option, + tty: bool, + }, + ApplyPatch { + id: String, + cwd: PathUri, + files: Vec, + patch: String, + }, +} + +impl ApprovalAction { + fn into_guardian_request(self) -> std::io::Result { + Ok(match self { + Self::Shell { + id, + command, + cwd, + sandbox_permissions, + additional_permissions, + justification, + } => crate::guardian::GuardianApprovalRequest::Shell { + id, + command, + cwd: cwd.to_abs_path()?, + sandbox_permissions, + additional_permissions, + justification, + }, + Self::ExecCommand { + id, + command, + cwd, + sandbox_permissions, + additional_permissions, + justification, + tty, + } => crate::guardian::GuardianApprovalRequest::ExecCommand { + id, + command, + cwd: cwd.to_abs_path()?, + sandbox_permissions, + additional_permissions, + justification, + tty, + }, + Self::ApplyPatch { + id, + cwd, + files, + patch, + } => crate::guardian::GuardianApprovalRequest::ApplyPatch { + id, + cwd: cwd.to_abs_path()?, + files: files + .into_iter() + .map(|path| path.to_abs_path()) + .collect::>>()?, + patch, + }, + }) + } +} #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub(super) enum ApprovalReviewer { @@ -113,7 +194,10 @@ where let resolution = match reviewer { ApprovalReviewer::Guardian => { let review_id = new_guardian_review_id(); - let action = match tool.approval_action(req, &ctx) { + let action = match tool + .approval_action(req, &ctx) + .and_then(ApprovalAction::into_guardian_request) + { Ok(action) => action, Err(err) => { tracing::error!(%err, "failed to build automatic approval action"); diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index b839b05c44..af9a7f0a58 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -73,23 +73,13 @@ impl ApplyPatchRuntime { &self.committed_delta } - fn build_guardian_review_request( - req: &ApplyPatchRequest, - call_id: &str, - ) -> 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 - .file_paths - .iter() - .map(PathUri::to_abs_path) - .collect::>>()?; - Ok(ApprovalAction::ApplyPatch { + fn build_approval_action(req: &ApplyPatchRequest, call_id: &str) -> ApprovalAction { + ApprovalAction::ApplyPatch { id: call_id.to_string(), - cwd, - files, + cwd: req.action.cwd.clone(), + files: req.file_paths.clone(), patch: req.action.patch.clone(), - }) + } } fn file_system_sandbox_context_for_attempt( @@ -190,7 +180,7 @@ impl Approvable for ApplyPatchRuntime { req: &ApplyPatchRequest, ctx: &ApprovalCtx<'_>, ) -> std::io::Result { - ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id) + Ok(ApplyPatchRuntime::build_approval_action(req, ctx.call_id)) } fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool { 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 c70002e62f..69ba390d69 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs @@ -49,24 +49,17 @@ fn wants_no_sandbox_approval_granular_respects_sandbox_flag() { } #[tokio::test] -async fn guardian_review_request_includes_patch_context() { - let path = std::env::temp_dir() - .join("guardian-apply-patch-test.txt") - .abs(); - let action = - ApplyPatchAction::new_add_for_test(&PathUri::from_abs_path(&path), "hello".to_string()); - let expected_cwd = action.cwd.to_abs_path().expect("native patch cwd"); +async fn approval_action_preserves_patch_path_uris() { + let path = PathUri::parse("file:///C:/workspace/guardian-apply-patch-test.txt") + .expect("valid foreign path URI"); + let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string()); + let expected_cwd = action.cwd.clone(); let expected_patch = action.patch.clone(); let request = ApplyPatchRequest { turn_environment: test_turn_environment(codex_exec_server::LOCAL_ENVIRONMENT_ID), action, - file_paths: vec![PathUri::from_abs_path(&path)], - changes: HashMap::from([( - path.to_path_buf(), - FileChange::Add { - content: "hello".to_string(), - }, - )]), + file_paths: vec![path.clone()], + changes: HashMap::new(), exec_approval_requirement: ExecApprovalRequirement::NeedsApproval { reason: None, proposed_execpolicy_amendment: None, @@ -75,11 +68,10 @@ async fn guardian_review_request_includes_patch_context() { permissions_preapproved: false, }; - let guardian_request = ApplyPatchRuntime::build_guardian_review_request(&request, "call-1") - .expect("native guardian request cwd"); + let approval_action = ApplyPatchRuntime::build_approval_action(&request, "call-1"); assert_eq!( - guardian_request, + approval_action, ApprovalAction::ApplyPatch { id: "call-1".to_string(), cwd: expected_cwd, diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 3ff50f3f51..ae10d3a326 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -46,6 +46,7 @@ use codex_protocol::protocol::ReviewDecision; use codex_sandboxing::SandboxablePreference; use codex_shell_command::powershell::prefix_powershell_script_with_utf8; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; use futures::future::BoxFuture; use std::collections::HashMap; use tokio_util::sync::CancellationToken; @@ -184,7 +185,7 @@ impl Approvable for ShellRuntime { Ok(ApprovalAction::Shell { id: ctx.call_id.to_string(), command: req.command.clone(), - cwd: req.cwd.clone(), + cwd: PathUri::from_abs_path(&req.cwd), sandbox_permissions: req.sandbox_permissions, additional_permissions: req.additional_permissions.clone(), justification: req.justification.clone(), diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index a1b9b33c6d..d9947cec82 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -224,7 +224,7 @@ impl Approvable for UnifiedExecRuntime<'_> { Ok(ApprovalAction::ExecCommand { id: ctx.call_id.to_string(), command: req.command.clone(), - cwd: req.cwd.to_abs_path()?, + cwd: req.cwd.clone(), sandbox_permissions: req.sandbox_permissions, additional_permissions: req.additional_permissions.clone(), justification: req.justification.clone(), diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 700d08abc5..193b62f002 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -126,7 +126,7 @@ pub(crate) struct ApprovalCtx<'a> { pub network_approval_context: Option, } -pub(crate) type ApprovalAction = super::approvals::ApprovalAction; +pub(crate) use super::approvals::ApprovalAction; #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct PermissionRequestPayload {