Refactor permission request approval flow

Co-authored-by: Codex <noreply@openai.com>
2026-09-09 15:58:47 +00:00 · 2026-04-12 12:10:58 -07:00
parent 312987d9ef
commit 6f56c2b579
4 changed files with 154 additions and 97 deletions
--- a/codex-rs/core/src/hook_runtime.rs
+++ b/codex-rs/core/src/hook_runtime.rs
@@ -2,7 +2,6 @@ use std::future::Future;
 use std::sync::Arc;

 use codex_hooks::PermissionRequestDecision;
-use codex_hooks::PermissionRequestGuardianReview;
 use codex_hooks::PermissionRequestOutcome;
 use codex_hooks::PermissionRequestRequest;
 use codex_hooks::PostToolUseOutcome;
@@ -14,10 +13,8 @@ use codex_hooks::UserPromptSubmitOutcome;
 use codex_hooks::UserPromptSubmitRequest;
 use codex_protocol::items::TurnItem;
 use codex_protocol::models::DeveloperInstructions;
-use codex_protocol::models::PermissionProfile;
 use codex_protocol::models::ResponseInputItem;
 use codex_protocol::models::ResponseItem;
-use codex_protocol::models::SandboxPermissions;
 use codex_protocol::protocol::AskForApproval;
 use codex_protocol::protocol::EventMsg;
 use codex_protocol::protocol::HookCompletedEvent;
@@ -29,6 +26,8 @@ use serde_json::Value;
 use crate::codex::Session;
 use crate::codex::TurnContext;
 use crate::event_mapping::parse_turn_item;
+use crate::tools::sandboxing::PermissionRequestHookRequest;
+use crate::tools::sandboxing::PermissionRequestPayload;

 pub(crate) struct HookRuntimeOutcome {
    pub should_stop: bool,
@@ -154,14 +153,20 @@ pub(crate) async fn run_pre_tool_use_hooks(
 pub(crate) async fn run_permission_request_hooks(
    sess: &Arc<Session>,
    turn_context: &Arc<TurnContext>,
-    run_id_suffix: String,
-    tool_name: String,
-    command: String,
-    sandbox_permissions: SandboxPermissions,
-    additional_permissions: Option<PermissionProfile>,
-    justification: Option<String>,
-    guardian_review: Option<PermissionRequestGuardianReview>,
+    hook_request: PermissionRequestHookRequest,
 ) -> Option<PermissionRequestDecision> {
+    let PermissionRequestHookRequest {
+        run_id_suffix,
+        payload,
+        guardian_review,
+    } = hook_request;
+    let PermissionRequestPayload {
+        tool_name,
+        command,
+        sandbox_permissions,
+        additional_permissions,
+        justification,
+    } = payload;
    let request = PermissionRequestRequest {
        session_id: sess.conversation_id,
        turn_id: turn_context.sub_id.clone(),
--- a/codex-rs/core/src/tools/orchestrator.rs
+++ b/codex-rs/core/src/tools/orchestrator.rs
@@ -24,6 +24,7 @@ use crate::tools::network_approval::finish_deferred_network_approval;
 use crate::tools::network_approval::finish_immediate_network_approval;
 use crate::tools::sandboxing::ApprovalCtx;
 use crate::tools::sandboxing::ExecApprovalRequirement;
+use crate::tools::sandboxing::PermissionRequestHookRequest;
 use crate::tools::sandboxing::SandboxAttempt;
 use crate::tools::sandboxing::SandboxOverride;
 use crate::tools::sandboxing::ToolCtx;
@@ -60,6 +61,19 @@ enum ApprovalAttempt {
    Retry,
 }

+#[derive(Clone, Copy)]
+struct ApprovalTelemetry<'a> {
+    otel: &'a SessionTelemetry,
+    tool_name: &'a str,
+    call_id: &'a str,
+}
+
+struct ApprovalRequestCtx<'a> {
+    approval: ApprovalCtx<'a>,
+    routes_to_guardian: bool,
+    telemetry: ApprovalTelemetry<'a>,
+}
+
 impl ToolOrchestrator {
    pub fn new() -> Self {
        Self {
@@ -164,11 +178,15 @@ impl ToolOrchestrator {
                    tool,
                    req,
                    ApprovalAttempt::Initial,
-                    approval_ctx,
-                    turn_ctx,
-                    &otel,
-                    otel_tn,
-                    otel_ci,
+                    ApprovalRequestCtx {
+                        routes_to_guardian: use_guardian,
+                        approval: approval_ctx,
+                        telemetry: ApprovalTelemetry {
+                            otel: &otel,
+                            tool_name: otel_tn,
+                            call_id: otel_ci,
+                        },
+                    },
                )
                .await?;

@@ -325,11 +343,15 @@ impl ToolOrchestrator {
                        tool,
                        req,
                        ApprovalAttempt::Retry,
-                        approval_ctx,
-                        turn_ctx,
-                        &otel,
-                        otel_tn,
-                        otel_ci,
+                        ApprovalRequestCtx {
+                            routes_to_guardian: use_guardian,
+                            approval: approval_ctx,
+                            telemetry: ApprovalTelemetry {
+                                otel: &otel,
+                                tool_name: otel_tn,
+                                call_id: otel_ci,
+                            },
+                        },
                    )
                    .await?;

@@ -405,31 +427,40 @@ impl ToolOrchestrator {
        tool: &mut T,
        req: &Rq,
        approval_attempt: ApprovalAttempt,
-        approval_ctx: ApprovalCtx<'_>,
-        turn_ctx: &crate::codex::TurnContext,
-        otel: &SessionTelemetry,
-        otel_tn: &str,
-        otel_ci: &str,
+        request_ctx: ApprovalRequestCtx<'_>,
    ) -> Result<ReviewDecision, ToolError>
    where
        T: ToolRuntime<Rq, Out>,
    {
-        let guardian_review = if routes_approval_to_guardian(turn_ctx) {
-            match tool.guardian_approval_request(req, &approval_ctx) {
-                Some(request) => Some(
-                    review_approval_request_with_review(
-                        approval_ctx.session,
-                        approval_ctx.turn,
-                        approval_ctx
-                            .guardian_review_id
-                            .clone()
-                            .expect("guardian review id should be present for guardian approvals"),
-                        request,
-                        approval_ctx.retry_reason.clone(),
+        let ApprovalRequestCtx {
+            approval: approval_ctx,
+            routes_to_guardian,
+            telemetry,
+        } = request_ctx;
+        let ApprovalTelemetry {
+            otel,
+            tool_name: otel_tn,
+            call_id: otel_ci,
+        } = telemetry;
+
+        let guardian_review = if routes_to_guardian {
+            if let Some(review_id) = approval_ctx.guardian_review_id.clone() {
+                if let Some(request) = tool.guardian_approval_request(req, &approval_ctx) {
+                    Some(
+                        review_approval_request_with_review(
+                            approval_ctx.session,
+                            approval_ctx.turn,
+                            review_id,
+                            request,
+                            approval_ctx.retry_reason.clone(),
+                        )
+                        .await,
                    )
-                    .await,
-                ),
-                None => None,
+                } else {
+                    None
+                }
+            } else {
+                None
            }
        } else {
            None
@@ -443,15 +474,13 @@ impl ToolOrchestrator {
            match run_permission_request_hooks(
                approval_ctx.session,
                approval_ctx.turn,
-                run_id_suffix,
-                permission_request.tool_name,
-                permission_request.command,
-                permission_request.sandbox_permissions,
-                permission_request.additional_permissions,
-                permission_request.justification,
-                guardian_review
-                    .as_ref()
-                    .map(|review| permission_request_guardian_review(review.review.clone())),
+                PermissionRequestHookRequest {
+                    run_id_suffix,
+                    payload: permission_request,
+                    guardian_review: guardian_review
+                        .as_ref()
+                        .map(|review| permission_request_guardian_review(review.review.clone())),
+                },
            )
            .await
            {
@@ -488,7 +517,7 @@ impl ToolOrchestrator {
        }

        let decision = tool.start_approval_async(req, approval_ctx).await;
-        let otel_source = if routes_approval_to_guardian(turn_ctx) {
+        let otel_source = if routes_to_guardian {
            ToolDecisionSource::AutomatedReviewer
        } else {
            ToolDecisionSource::User
--- a/codex-rs/core/src/tools/sandboxing.rs
+++ b/codex-rs/core/src/tools/sandboxing.rs
@@ -11,6 +11,7 @@ use crate::sandboxing::ExecOptions;
 use crate::sandboxing::SandboxPermissions;
 use crate::state::SessionServices;
 use crate::tools::network_approval::NetworkApprovalSpec;
+use codex_hooks::PermissionRequestGuardianReview;
 use codex_network_proxy::NetworkProxy;
 use codex_protocol::approvals::ExecPolicyAmendment;
 use codex_protocol::approvals::NetworkApprovalContext;
@@ -159,6 +160,13 @@ impl PermissionRequestPayload {
    }
 }

+#[derive(Clone, Debug, PartialEq, Eq)]
+pub(crate) struct PermissionRequestHookRequest {
+    pub run_id_suffix: String,
+    pub payload: PermissionRequestPayload,
+    pub guardian_review: Option<PermissionRequestGuardianReview>,
+}
+
 // Specifies what tool orchestrator should do with a given tool call.
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub(crate) enum ExecApprovalRequirement {
--- a/codex-rs/core/tests/suite/hooks.rs
+++ b/codex-rs/core/tests/suite/hooks.rs
@@ -13,6 +13,7 @@ use codex_protocol::protocol::EventMsg;
 use codex_protocol::protocol::Op;
 use codex_protocol::protocol::RolloutItem;
 use codex_protocol::protocol::RolloutLine;
+use codex_protocol::protocol::SandboxPolicy;
 use codex_protocol::user_input::UserInput;
 use core_test_support::responses::ev_assistant_message;
 use core_test_support::responses::ev_completed;
@@ -579,10 +580,9 @@ async fn stop_hook_can_block_multiple_times_in_same_turn() -> Result<()> {
            }
        })
        .with_config(|config| {
-            config
-                .features
-                .enable(Feature::CodexHooks)
-                .expect("test config should allow feature update");
+            if let Err(error) = config.features.enable(Feature::CodexHooks) {
+                panic!("test config should allow feature update: {error}");
+            }
        });
    let test = builder.build(&server).await?;

@@ -678,10 +678,9 @@ async fn session_start_hook_sees_materialized_transcript_path() -> Result<()> {
            }
        })
        .with_config(|config| {
-            config
-                .features
-                .enable(Feature::CodexHooks)
-                .expect("test config should allow feature update");
+            if let Err(error) = config.features.enable(Feature::CodexHooks) {
+                panic!("test config should allow feature update: {error}");
+            }
        });
    let test = builder.build(&server).await?;

@@ -1096,8 +1095,10 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() ->
    let server = start_mock_server().await;
    let call_id = "permissionrequest-shell-command";
    let marker = std::env::temp_dir().join("permissionrequest-shell-command-marker");
-    let command = format!("printf allowed > {}", marker.display());
-    let args = serde_json::json!({ "command": command });
+    let command = format!("rm -f {}", marker.display());
+    let args = serde_json::json!({
+        "command": ["rm", "-f", marker.display().to_string()],
+    });
    let responses = mount_sse_sequence(
        &server,
        vec![
@@ -1105,7 +1106,7 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() ->
                ev_response_created("resp-1"),
                core_test_support::responses::ev_function_call(
                    call_id,
-                    "shell_command",
+                    "shell",
                    &serde_json::to_string(&args)?,
                ),
                ev_completed("resp-1"),
@@ -1138,31 +1139,21 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() ->
        });
    let test = builder.build(&server).await?;

-    if marker.exists() {
-        fs::remove_file(&marker).context("remove leftover permission request marker")?;
-    }
+    fs::write(&marker, "seed").context("create permission request marker")?;

    test.submit_turn_with_policies(
        "run the shell command after hook approval",
        AskForApproval::OnRequest,
-        codex_protocol::protocol::SandboxPolicy::DangerFullAccess,
+        SandboxPolicy::DangerFullAccess,
    )
    .await?;

    let requests = responses.requests();
    assert_eq!(requests.len(), 2);
-    let output_item = requests[1].function_call_output(call_id);
-    let output = output_item
-        .get("output")
-        .and_then(Value::as_str)
-        .expect("shell command output string");
+    requests[1].function_call_output(call_id);
    assert!(
-        output.contains("allowed"),
-        "shell command output should reach the model after hook approval",
-    );
-    assert!(
-        marker.exists(),
-        "approved command should create marker file"
+        !marker.exists(),
+        "approved command should remove marker file"
    );

    let hook_inputs = read_permission_request_hook_inputs(test.codex_home_path())?;
@@ -1184,20 +1175,37 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() ->
    Ok(())
 }

-#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-async fn permission_request_hook_receives_guardian_review_before_fallback() -> Result<()> {
+#[test]
+fn permission_request_hook_receives_guardian_review_before_fallback() -> Result<()> {
+    std::thread::Builder::new()
+        .stack_size(32 * 1024 * 1024)
+        .spawn(|| {
+            tokio::runtime::Builder::new_current_thread()
+                .enable_all()
+                .build()
+                .expect("build current-thread tokio runtime")
+                .block_on(permission_request_hook_receives_guardian_review_before_fallback_inner())
+        })
+        .expect("spawn guardian permission request hook test thread")
+        .join()
+        .expect("guardian permission request hook test thread panicked")
+}
+
+async fn permission_request_hook_receives_guardian_review_before_fallback_inner() -> Result<()> {
    skip_if_no_network!(Ok(()));

    let server = start_mock_server().await;
    let call_id = "permissionrequest-guardian-review";
    let marker = std::env::temp_dir().join("permissionrequest-guardian-review-marker");
-    let command = format!("printf guardian > {}", marker.display());
-    let args = serde_json::json!({ "command": command });
+    let command = format!("rm -f {}", marker.display());
+    let args = serde_json::json!({
+        "command": ["rm", "-f", marker.display().to_string()],
+    });
    let guardian_assessment = serde_json::json!({
        "risk_level": "medium",
        "user_authorization": "high",
        "outcome": "allow",
-        "rationale": "The user asked to run this local marker command.",
+        "rationale": "The user asked to delete this local marker file.",
    })
    .to_string();
    let responses = mount_sse_sequence(
@@ -1207,7 +1215,7 @@ async fn permission_request_hook_receives_guardian_review_before_fallback() -> R
                ev_response_created("resp-1"),
                core_test_support::responses::ev_function_call(
                    call_id,
-                    "shell_command",
+                    "shell",
                    &serde_json::to_string(&args)?,
                ),
                ev_completed("resp-1"),
@@ -1235,26 +1243,25 @@ async fn permission_request_hook_receives_guardian_review_before_fallback() -> R
            }
        })
        .with_config(|config| {
-            config
-                .features
-                .enable(Feature::CodexHooks)
-                .expect("test config should allow feature update");
-            config
-                .features
-                .enable(Feature::GuardianApproval)
-                .expect("test config should allow feature update");
+            if let Err(error) = config.features.enable(Feature::CodexHooks) {
+                panic!("test config should allow feature update: {error}");
+            }
+            if let Err(error) = config.features.enable(Feature::ExecPermissionApprovals) {
+                panic!("test config should allow feature update: {error}");
+            }
+            if let Err(error) = config.features.enable(Feature::GuardianApproval) {
+                panic!("test config should allow feature update: {error}");
+            }
            config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
        });
    let test = builder.build(&server).await?;

-    if marker.exists() {
-        fs::remove_file(&marker).context("remove leftover guardian review marker")?;
-    }
+    fs::write(&marker, "seed").context("create guardian review marker")?;

    test.submit_turn_with_policies(
        "run the shell command after guardian review",
        AskForApproval::OnRequest,
-        codex_protocol::protocol::SandboxPolicy::DangerFullAccess,
+        SandboxPolicy::DangerFullAccess,
    )
    .await?;

@@ -1265,12 +1272,20 @@ async fn permission_request_hook_receives_guardian_review_before_fallback() -> R
        "guardian decision should be reused instead of running a second review",
    );
    assert!(
-        marker.exists(),
-        "guardian-approved fallback should create marker file",
+        requests[2]
+            .input()
+            .iter()
+            .any(|item| item.get("type").and_then(Value::as_str) == Some("function_call_output")),
+        "guardian-approved fallback should continue with tool output",
+    );
+    assert!(
+        !marker.exists(),
+        "guardian-approved fallback should remove marker file",
    );

    let hook_inputs = read_permission_request_hook_inputs(test.codex_home_path())?;
    assert_eq!(hook_inputs.len(), 1);
+    assert_eq!(hook_inputs[0]["tool_input"]["command"], command);
    assert_eq!(
        hook_inputs[0]["guardian_review"],
        serde_json::json!({
@@ -1278,7 +1293,7 @@ async fn permission_request_hook_receives_guardian_review_before_fallback() -> R
            "decision": "allow",
            "risk_level": "medium",
            "user_authorization": "high",
-            "rationale": "The user asked to run this local marker command.",
+            "rationale": "The user asked to delete this local marker file.",
        })
    );