From 6f56c2b579300dac97fc2eb0c2bf4c741a5bfb5f Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Sun, 12 Apr 2026 12:10:58 -0700 Subject: [PATCH] Refactor permission request approval flow Co-authored-by: Codex --- codex-rs/core/src/hook_runtime.rs | 25 +++--- codex-rs/core/src/tools/orchestrator.rs | 109 +++++++++++++++--------- codex-rs/core/src/tools/sandboxing.rs | 8 ++ codex-rs/core/tests/suite/hooks.rs | 109 ++++++++++++++---------- 4 files changed, 154 insertions(+), 97 deletions(-) diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index ad3b2529a8..a29f613e9f 100644 --- 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, turn_context: &Arc, - run_id_suffix: String, - tool_name: String, - command: String, - sandbox_permissions: SandboxPermissions, - additional_permissions: Option, - justification: Option, - guardian_review: Option, + hook_request: PermissionRequestHookRequest, ) -> Option { + 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(), diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 87b86fed4e..34672261ca 100644 --- 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 where T: ToolRuntime, { - 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 diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index a3e64f96ea..d49dc4db49 100644 --- 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, +} + // Specifies what tool orchestrator should do with a given tool call. #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum ExecApprovalRequirement { diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 24f0926031..30da127c97 100644 --- 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.", }) );