diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 696a8adbd839..b7cc02fadcdf 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -1060,9 +1060,10 @@ async fn danger_full_access_tool_attempts_do_not_enforce_managed_network() -> an ) -> std::io::Result { Ok(crate::tools::sandboxing::ApprovalAction::Shell { id: ctx.call_id.to_string(), + environment_id: codex_exec_server::LOCAL_ENVIRONMENT_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 0d6727d8546e..1c6fd1037d81 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,119 @@ 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_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; -pub(super) type ApprovalAction = crate::guardian::GuardianApprovalRequest; +#[derive(Clone, Debug, PartialEq)] +pub(crate) enum ApprovalAction { + Shell { + id: String, + environment_id: String, + command: Vec, + cwd: PathUri, + sandbox_permissions: SandboxPermissions, + additional_permissions: Option, + justification: Option, + }, + ExecCommand { + id: String, + environment_id: String, + command: Vec, + cwd: PathUri, + sandbox_permissions: SandboxPermissions, + additional_permissions: Option, + justification: Option, + tty: bool, + }, + ApplyPatch { + id: String, + environment_id: String, + cwd: PathUri, + files: Vec, + patch: String, + }, +} + +impl ApprovalAction { + fn into_guardian_request(self) -> std::io::Result { + Ok(match self { + Self::Shell { + id, + environment_id, + command, + cwd, + sandbox_permissions, + additional_permissions, + justification, + } => crate::guardian::GuardianApprovalRequest::Shell { + id, + command, + cwd: guardian_cwd(&environment_id, cwd)?, + sandbox_permissions, + additional_permissions, + justification, + }, + Self::ExecCommand { + id, + environment_id, + command, + cwd, + sandbox_permissions, + additional_permissions, + justification, + tty, + } => crate::guardian::GuardianApprovalRequest::ExecCommand { + id, + command, + cwd: guardian_cwd(&environment_id, cwd)?, + sandbox_permissions, + additional_permissions, + justification, + tty, + }, + Self::ApplyPatch { + id, + environment_id, + cwd, + files, + patch, + } => crate::guardian::GuardianApprovalRequest::ApplyPatch { + id, + cwd: guardian_cwd(&environment_id, cwd)?, + files: files + .into_iter() + .map(|path| path.to_abs_path()) + .collect::>>()?, + patch, + }, + }) + } +} + +fn guardian_cwd(environment_id: &str, cwd: PathUri) -> std::io::Result { + match cwd.to_abs_path() { + Ok(cwd) => Ok(cwd), + Err(err) if environment_id != codex_exec_server::LOCAL_ENVIRONMENT_ID => Err(err), + Err(_) => { + let cwd_display = cwd.to_string(); + let path = cwd.to_url().to_file_path().map_err(|()| { + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("local cwd URI `{cwd_display}` is not a host-native path"), + ) + })?; + AbsolutePathBuf::from_absolute_path_checked(path).map_err(|err| { + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("local cwd URI `{cwd_display}` is not absolute: {err}"), + ) + }) + } + } +} #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub(super) enum ApprovalReviewer { @@ -113,7 +223,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"); @@ -211,3 +324,7 @@ fn record_resolution( source, ); } + +#[cfg(all(test, unix))] +#[path = "approvals_tests.rs"] +mod tests; diff --git a/codex-rs/core/src/tools/approvals_tests.rs b/codex-rs/core/src/tools/approvals_tests.rs new file mode 100644 index 000000000000..9cf066e09372 --- /dev/null +++ b/codex-rs/core/src/tools/approvals_tests.rs @@ -0,0 +1,22 @@ +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn guardian_cwd_preserves_drive_shaped_local_posix_path() { + let native_cwd = AbsolutePathBuf::try_from(std::path::PathBuf::from("/C:/workspace")) + .expect("drive-shaped POSIX path should be absolute"); + let cwd = PathUri::from_abs_path(&native_cwd); + + assert_eq!( + guardian_cwd(codex_exec_server::LOCAL_ENVIRONMENT_ID, cwd) + .expect("local cwd should retain the host path convention"), + native_cwd + ); +} + +#[test] +fn guardian_cwd_rejects_foreign_remote_path() { + let cwd = PathUri::parse("file:///C:/workspace").expect("valid Windows path URI"); + + assert!(guardian_cwd(codex_exec_server::REMOTE_ENVIRONMENT_ID, cwd).is_err()); +} diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index e5617e0a508f..97af94f893aa 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -73,23 +73,14 @@ 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, + environment_id: req.turn_environment.environment_id.clone(), + cwd: req.action.cwd.clone(), + files: req.file_paths.clone(), patch: req.action.patch.clone(), - }) + } } fn file_system_sandbox_context_for_attempt( @@ -188,7 +179,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 c70002e62f07..767a57b73704 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,13 +68,13 @@ 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(), + environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), cwd: expected_cwd, files: vec![path], patch: expected_patch, diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 3ff50f3f51ad..145820c76802 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; @@ -183,8 +184,9 @@ impl Approvable for ShellRuntime { ) -> std::io::Result { Ok(ApprovalAction::Shell { id: ctx.call_id.to_string(), + environment_id: req.turn_environment.environment_id.clone(), 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 a1b9b33c6d5c..6a67987c2579 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -223,8 +223,9 @@ impl Approvable for UnifiedExecRuntime<'_> { ) -> std::io::Result { Ok(ApprovalAction::ExecCommand { id: ctx.call_id.to_string(), + environment_id: req.turn_environment.environment_id.clone(), 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 4412e38fe46a..25078a9524bb 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 {