feedback

codex-rs/core/src/tools/approvals.rs

@@ -6,6 +6,7 @@ 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_with_reviewer;
 use crate::hook_runtime::run_permission_request_hooks;
 use crate::session::session::Session;
 use crate::session::turn_context::TurnContext;
@@ -17,7 +18,6 @@ use crate::tools::sandboxing::ToolRuntime;
 use codex_config::types::ApprovalsReviewer;
 use codex_hooks::PermissionRequestDecision;
 use codex_otel::ToolDecisionSource;
-use codex_protocol::protocol::AskForApproval;
 use codex_protocol::protocol::NetworkPolicyRuleAction;
 use codex_protocol::protocol::ReviewDecision;
 
@@ -35,19 +35,12 @@ impl ApprovalReviewer {
     }
 
     fn for_reviewer(turn: &TurnContext, reviewer: ApprovalsReviewer) -> Self {
-        if Self::routes_to_guardian(turn, reviewer) {
+        if routes_approval_to_guardian_with_reviewer(turn, reviewer) {
             Self::Guardian
         } else {
             Self::User
         }
     }
-
-    fn routes_to_guardian(turn: &TurnContext, reviewer: ApprovalsReviewer) -> bool {
-        matches!(
-            turn.approval_policy.value(),
-            AskForApproval::OnRequest | AskForApproval::Granular(_)
-        ) && reviewer == ApprovalsReviewer::AutoReview
-    }
 }
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]

codex-rs/core/tests/suite/hooks.rs

@@ -26,6 +26,7 @@ use codex_protocol::request_permissions::RequestPermissionProfile;
 use codex_protocol::request_permissions::RequestPermissionsResponse;
 use codex_protocol::user_input::UserInput;
 use codex_utils_absolute_path::AbsolutePathBuf;
+use core_test_support::TestTargetOs;
 use core_test_support::hooks::trust_discovered_hooks;
 use core_test_support::hooks::trust_hooks;
 use core_test_support::managed_network_requirements_loader;
@@ -48,6 +49,7 @@ use core_test_support::streaming_sse::StreamingSseChunk;
 use core_test_support::streaming_sse::start_streaming_sse_server;
 use core_test_support::test_codex::test_codex;
 use core_test_support::test_codex::turn_permission_fields;
+use core_test_support::test_target_os;
 use core_test_support::wait_for_event;
 use pretty_assertions::assert_eq;
 use serde_json::Value;
@@ -2024,8 +2026,13 @@ async fn permission_request_hook_allow_bypasses_strict_auto_review() -> Result<(
     let server = start_mock_server().await;
     let permission_call_id = "strict-hook-permissions";
     let command_call_id = "strict-hook-shell-command";
-    let marker = std::env::temp_dir().join("strict-hook-shell-command-marker");
-    let command = format!("rm -f {}", marker.display());
+    let marker_name = "strict-hook-shell-command-marker";
+    let command = match test_target_os() {
+        TestTargetOs::Linux | TestTargetOs::MacOs => format!("rm -f {marker_name}"),
+        TestTargetOs::Windows => {
+            format!("Remove-Item -Force -ErrorAction SilentlyContinue {marker_name}")
+        }
+    };
     let requested_permissions = RequestPermissionProfile {
         network: Some(NetworkPermissions {
             enabled: Some(true),
@@ -2079,9 +2086,17 @@ async fn permission_request_hook_allow_bypasses_strict_auto_review() -> Result<(
                 .enable(Feature::RequestPermissionsTool)
                 .expect("test config should allow feature update");
         });
-    let test = builder.build(&server).await?;
+    let test = builder.build_with_auto_env(&server).await?;
 
-    fs::write(&marker, "seed").context("create strict auto-review marker")?;
+    let marker = test
+        .executor_environment()
+        .selection()
+        .cwd
+        .join(marker_name)?;
+    test.fs()
+        .write_file(&marker, b"seed".to_vec(), /*sandbox*/ None)
+        .await
+        .context("create strict auto-review marker")?;
     let (sandbox_policy, permission_profile) =
         turn_permission_fields(PermissionProfile::Disabled, test.config.cwd.as_path());
     test.codex
@@ -2130,7 +2145,10 @@ async fn permission_request_hook_allow_bypasses_strict_auto_review() -> Result<(
     assert_eq!(requests.len(), 3);
     requests[2].function_call_output(command_call_id);
     assert!(
-        !marker.exists(),
+        test.fs()
+            .read_file(&marker, /*sandbox*/ None)
+            .await
+            .is_err(),
         "hook-approved command should remove marker without Guardian review"
     );
     assert_single_permission_request_hook_input(