diff --git a/codex-rs/core/src/approval_coordinator.rs b/codex-rs/core/src/approval_coordinator.rs new file mode 100644 index 0000000000..d440722fe9 --- /dev/null +++ b/codex-rs/core/src/approval_coordinator.rs @@ -0,0 +1,225 @@ +//! Central approval policy-stage execution and reviewer routing. + +use std::sync::Arc; + +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::hook_runtime::run_permission_request_hooks; +use crate::session::session::Session; +use crate::session::turn_context::TurnContext; +use crate::tools::flat_tool_name; +use crate::tools::sandboxing::ApprovalCtx; +use crate::tools::sandboxing::ToolCtx; +use crate::tools::sandboxing::ToolError; +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; + +pub(crate) type ApprovalAction = crate::guardian::GuardianApprovalRequest; + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum ApprovalReviewer { + Guardian, + User, +} + +impl ApprovalReviewer { + pub(crate) fn for_turn(turn: &TurnContext) -> Self { + Self::for_reviewer(turn, turn.config.approvals_reviewer) + } + + pub(crate) fn for_reviewer(turn: &TurnContext, reviewer: ApprovalsReviewer) -> Self { + if Self::routes_to_guardian(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)] +pub(crate) enum ApprovalResolutionSource { + Hook, + Guardian, + User, +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct ApprovalResolution { + pub(crate) decision: ReviewDecision, + pub(crate) rejection: Option, + pub(crate) source: ApprovalResolutionSource, +} + +impl ApprovalResolution { + pub(crate) fn into_tool_result(self) -> Result { + if let Some(rejection) = self.rejection { + Err(ToolError::Rejected(rejection)) + } else { + Ok(self.decision) + } + } +} + +pub(crate) struct ApprovalCoordinator; + +impl ApprovalCoordinator { + pub(crate) async fn resolve_tool( + tool: &mut T, + req: &Rq, + permission_request_run_id: &str, + ctx: ApprovalCtx<'_>, + tool_ctx: &ToolCtx, + reviewer: ApprovalReviewer, + otel: &codex_otel::SessionTelemetry, + ) -> Result + where + T: ToolRuntime, + { + if let Some(permission_request) = tool.permission_request_payload(req) { + match run_permission_request_hooks( + ctx.session, + ctx.turn, + permission_request_run_id, + permission_request, + ) + .await + { + Some(PermissionRequestDecision::Allow) => { + let resolution = ApprovalResolution { + decision: ReviewDecision::Approved, + rejection: None, + source: ApprovalResolutionSource::Hook, + }; + Self::record_resolution(otel, tool_ctx, &resolution); + return Ok(resolution); + } + Some(PermissionRequestDecision::Deny { message }) => { + let resolution = ApprovalResolution { + decision: ReviewDecision::Denied, + rejection: Some(message), + source: ApprovalResolutionSource::Hook, + }; + Self::record_resolution(otel, tool_ctx, &resolution); + return Ok(resolution); + } + None => {} + } + } + + let resolution = match reviewer { + ApprovalReviewer::Guardian => { + let review_id = new_guardian_review_id(); + let action = match tool.approval_action(req, &ctx) { + Ok(action) => action, + Err(err) => { + tracing::error!(%err, "failed to build automatic approval action"); + let resolution = ApprovalResolution { + decision: ReviewDecision::Abort, + rejection: Some( + "automatic approval review could not prepare the action" + .to_string(), + ), + source: ApprovalResolutionSource::Guardian, + }; + Self::record_resolution(otel, tool_ctx, &resolution); + return Ok(resolution); + } + }; + let decision = review_approval_request( + ctx.session, + ctx.turn, + review_id.clone(), + action, + ctx.retry_reason.clone(), + ) + .await; + Self::normalize_guardian(ctx.session, review_id, decision).await + } + ApprovalReviewer::User => ApprovalResolution { + decision: tool.start_approval_async(req, ctx.clone()).await, + rejection: None, + source: ApprovalResolutionSource::User, + }, + }; + let resolution = Self::normalize_user_rejection(resolution); + Self::record_resolution(otel, tool_ctx, &resolution); + Ok(resolution) + } + + async fn normalize_guardian( + session: &Arc, + review_id: String, + decision: ReviewDecision, + ) -> ApprovalResolution { + let rejection = match &decision { + ReviewDecision::Approved + | ReviewDecision::ApprovedForSession + | ReviewDecision::ApprovedExecpolicyAmendment { .. } => None, + ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment, + } if network_policy_amendment.action == NetworkPolicyRuleAction::Allow => None, + ReviewDecision::TimedOut => Some(guardian_timeout_message()), + ReviewDecision::NetworkPolicyAmendment { .. } + | ReviewDecision::Denied + | ReviewDecision::Abort => { + Some(guardian_rejection_message(session.as_ref(), &review_id).await) + } + }; + ApprovalResolution { + decision, + rejection, + source: ApprovalResolutionSource::Guardian, + } + } + + fn normalize_user_rejection(mut resolution: ApprovalResolution) -> ApprovalResolution { + if resolution.source == ApprovalResolutionSource::User { + resolution.rejection = match &resolution.decision { + ReviewDecision::Approved + | ReviewDecision::ApprovedForSession + | ReviewDecision::ApprovedExecpolicyAmendment { .. } => None, + ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment, + } if network_policy_amendment.action == NetworkPolicyRuleAction::Allow => None, + ReviewDecision::NetworkPolicyAmendment { .. } + | ReviewDecision::Denied + | ReviewDecision::Abort => Some("rejected by user".to_string()), + ReviewDecision::TimedOut => Some("approval request timed out".to_string()), + }; + } + resolution + } + + fn record_resolution( + otel: &codex_otel::SessionTelemetry, + tool_ctx: &ToolCtx, + resolution: &ApprovalResolution, + ) { + let source = match resolution.source { + ApprovalResolutionSource::Hook => ToolDecisionSource::Config, + ApprovalResolutionSource::Guardian => ToolDecisionSource::AutomatedReviewer, + ApprovalResolutionSource::User => ToolDecisionSource::User, + }; + let tool_name = flat_tool_name(&tool_ctx.tool_name); + otel.tool_decision( + tool_name.as_ref(), + &tool_ctx.call_id, + &resolution.decision, + source, + ); + } +} diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 91ccbd6e63..93ecea2c64 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -6,6 +6,7 @@ #![deny(clippy::print_stdout, clippy::print_stderr)] mod apply_patch; +mod approval_coordinator; mod apps; mod client; mod client_common; diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index f82ad5bb70..966c2082b2 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -6,12 +6,8 @@ simple sequence for any ToolRuntime: approval → select sandbox → attempt → retry with an escalated sandbox strategy on denial (no re‑approval thanks to caching). */ -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; -use crate::hook_runtime::run_permission_request_hooks; +use crate::approval_coordinator::ApprovalCoordinator; +use crate::approval_coordinator::ApprovalReviewer; use crate::network_policy_decision::network_approval_context_from_payload; use crate::tools::flat_tool_name; use crate::tools::network_approval::ActiveNetworkApproval; @@ -30,13 +26,11 @@ use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::default_exec_approval_requirement; use crate::tools::sandboxing::sandbox_override_for_first_attempt; use crate::tools::sandboxing::unsandboxed_execution_allowed; -use codex_hooks::PermissionRequestDecision; use codex_otel::ToolDecisionSource; use codex_protocol::error::CodexErr; use codex_protocol::error::SandboxErr; use codex_protocol::exec_output::ExecToolCallOutput; use codex_protocol::protocol::AskForApproval; -use codex_protocol::protocol::NetworkPolicyRuleAction; use codex_protocol::protocol::ReviewDecision; use codex_sandboxing::SandboxManager; use codex_sandboxing::SandboxType; @@ -155,8 +149,6 @@ impl ToolOrchestrator { let otel_tn = flat_tool_name(&tool_ctx.tool_name).into_owned(); let otel_ci = &tool_ctx.call_id; let strict_auto_review = tool_ctx.session.strict_auto_review_enabled_for_turn().await; - let use_guardian = routes_approval_to_guardian(turn_ctx) || strict_auto_review; - // 1) Approval let mut already_approved = false; @@ -168,27 +160,23 @@ impl ToolOrchestrator { match &requirement { ExecApprovalRequirement::Skip { .. } => { if strict_auto_review { - let guardian_review_id = Some(new_guardian_review_id()); let approval_ctx = ApprovalCtx { session: &tool_ctx.session, turn: &tool_ctx.turn, call_id: &tool_ctx.call_id, - guardian_review_id: guardian_review_id.clone(), retry_reason: None, network_approval_context: None, }; - let decision = Self::request_approval( + Self::request_approval( tool, req, tool_ctx.call_id.as_str(), approval_ctx, tool_ctx, - /*evaluate_permission_request_hooks*/ false, + ApprovalReviewer::Guardian, &otel, ) .await?; - Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision) - .await?; already_approved = true; } else { otel.tool_decision( @@ -203,28 +191,27 @@ impl ToolOrchestrator { return Err(ToolError::Rejected(reason.clone())); } ExecApprovalRequirement::NeedsApproval { reason, .. } => { - let guardian_review_id = use_guardian.then(new_guardian_review_id); let approval_ctx = ApprovalCtx { session: &tool_ctx.session, turn: &tool_ctx.turn, call_id: &tool_ctx.call_id, - guardian_review_id: guardian_review_id.clone(), retry_reason: reason.clone(), network_approval_context: None, }; - let decision = Self::request_approval( + Self::request_approval( tool, req, tool_ctx.call_id.as_str(), approval_ctx, tool_ctx, - /*evaluate_permission_request_hooks*/ !strict_auto_review, + if strict_auto_review { + ApprovalReviewer::Guardian + } else { + ApprovalReviewer::for_turn(turn_ctx) + }, &otel, ) .await?; - - Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision) - .await?; already_approved = true; } } @@ -400,30 +387,29 @@ impl ToolOrchestrator { && tool.should_bypass_approval(approval_policy, already_approved) && network_approval_context.is_none(); if !bypass_retry_approval { - let guardian_review_id = use_guardian.then(new_guardian_review_id); let approval_ctx = ApprovalCtx { session: &tool_ctx.session, turn: &tool_ctx.turn, call_id: &tool_ctx.call_id, - guardian_review_id: guardian_review_id.clone(), retry_reason: Some(retry_reason), network_approval_context: network_approval_context.clone(), }; let permission_request_run_id = format!("{}:retry", tool_ctx.call_id); - let decision = Self::request_approval( + Self::request_approval( tool, req, &permission_request_run_id, approval_ctx, tool_ctx, - /*evaluate_permission_request_hooks*/ !strict_auto_review, + if strict_auto_review { + ApprovalReviewer::Guardian + } else { + ApprovalReviewer::for_turn(turn_ctx) + }, &otel, ) .await?; - - Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision) - .await?; } let retry_sandbox_requested = !unsandboxed_allowed @@ -527,110 +513,23 @@ impl ToolOrchestrator { permission_request_run_id: &str, approval_ctx: ApprovalCtx<'_>, tool_ctx: &ToolCtx, - evaluate_permission_request_hooks: bool, + reviewer: ApprovalReviewer, otel: &codex_otel::SessionTelemetry, ) -> Result where T: ToolRuntime, { - if evaluate_permission_request_hooks - && let Some(permission_request) = tool.permission_request_payload(req) - { - let tool_name = flat_tool_name(&tool_ctx.tool_name); - match run_permission_request_hooks( - approval_ctx.session, - approval_ctx.turn, - permission_request_run_id, - permission_request, - ) - .await - { - Some(PermissionRequestDecision::Allow) => { - let decision = ReviewDecision::Approved; - otel.tool_decision( - tool_name.as_ref(), - &tool_ctx.call_id, - &decision, - ToolDecisionSource::Config, - ); - return Ok(decision); - } - Some(PermissionRequestDecision::Deny { message }) => { - let decision = ReviewDecision::Denied; - otel.tool_decision( - tool_name.as_ref(), - &tool_ctx.call_id, - &decision, - ToolDecisionSource::Config, - ); - return Err(ToolError::Rejected(message)); - } - None => {} - } - } - - let otel_source = if approval_ctx.guardian_review_id.is_some() { - ToolDecisionSource::AutomatedReviewer - } else { - ToolDecisionSource::User - }; - let decision = if let Some(review_id) = approval_ctx.guardian_review_id.clone() { - match tool.approval_action(req, &approval_ctx) { - Ok(action) => { - review_approval_request( - approval_ctx.session, - approval_ctx.turn, - review_id, - action, - approval_ctx.retry_reason.clone(), - ) - .await - } - Err(err) => { - tracing::error!(%err, "failed to build guardian approval action"); - ReviewDecision::Abort - } - } - } else { - tool.start_approval_async(req, approval_ctx).await - }; - let tool_name = flat_tool_name(&tool_ctx.tool_name); - otel.tool_decision( - tool_name.as_ref(), - &tool_ctx.call_id, - &decision, - otel_source, - ); - Ok(decision) - } - - async fn reject_if_not_approved( - tool_ctx: &ToolCtx, - guardian_review_id: Option<&str>, - decision: ReviewDecision, - ) -> Result<(), ToolError> { - match decision { - ReviewDecision::Denied | ReviewDecision::Abort => { - let reason = if let Some(review_id) = guardian_review_id { - guardian_rejection_message(tool_ctx.session.as_ref(), review_id).await - } else { - "rejected by user".to_string() - }; - Err(ToolError::Rejected(reason)) - } - ReviewDecision::TimedOut => Err(ToolError::Rejected(guardian_timeout_message())), - ReviewDecision::Approved - | ReviewDecision::ApprovedExecpolicyAmendment { .. } - | ReviewDecision::ApprovedForSession => Ok(()), - ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment, - } => match network_policy_amendment.action { - NetworkPolicyRuleAction::Allow => Ok(()), - NetworkPolicyRuleAction::Deny => { - Err(ToolError::Rejected("rejected by user".to_string())) - } - }, - } + ApprovalCoordinator::resolve_tool( + tool, + req, + permission_request_run_id, + approval_ctx, + tool_ctx, + reviewer, + otel, + ) + .await? + .into_tool_result() } } diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 571c5ce6df..11efb05dfc 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -122,18 +122,11 @@ pub(crate) struct ApprovalCtx<'a> { pub session: &'a Arc, pub turn: &'a Arc, pub call_id: &'a str, - /// Guardian review lifecycle ID for this approval, when guardian is reviewing it. - /// - /// This is separate from `call_id`: `call_id` identifies the tool item under - /// review, while this ID identifies the review itself. Keeping both lets - /// denial handling, overrides, and app-server notifications refer to the - /// review without overloading the tool call ID as a review ID. - pub guardian_review_id: Option, pub retry_reason: Option, pub network_approval_context: Option, } -pub(crate) type ApprovalAction = crate::guardian::GuardianApprovalRequest; +pub(crate) type ApprovalAction = crate::approval_coordinator::ApprovalAction; #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct PermissionRequestPayload { diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index a5c82d5767..93379faab1 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -12,6 +12,7 @@ use codex_plugin::PluginHookSource; use codex_plugin::PluginId; use codex_protocol::items::parse_hook_prompt_fragment; use codex_protocol::models::ContentItem; +use codex_protocol::models::NetworkPermissions; use codex_protocol::models::PermissionProfile; use codex_protocol::models::ResponseItem; use codex_protocol::permissions::NetworkSandboxPolicy; @@ -20,6 +21,9 @@ use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::RolloutLine; +use codex_protocol::request_permissions::PermissionGrantScope; +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::hooks::trust_discovered_hooks; @@ -2012,6 +2016,118 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() -> Ok(()) } +#[tokio::test] +async fn permission_request_hook_allow_bypasses_strict_auto_review() -> Result<()> { + skip_if_no_network!(Ok(())); + + 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 requested_permissions = RequestPermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + ..Default::default() + }; + let request_permissions_args = serde_json::json!({ + "reason": "Enable strict auto review", + "permissions": requested_permissions, + }); + let command_args = serde_json::json!({ "command": command }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-strict-hook-1"), + ev_function_call( + permission_call_id, + "request_permissions", + &serde_json::to_string(&request_permissions_args)?, + ), + ev_completed("resp-strict-hook-1"), + ]), + sse(vec![ + ev_response_created("resp-strict-hook-2"), + ev_function_call( + command_call_id, + "shell_command", + &serde_json::to_string(&command_args)?, + ), + ev_completed("resp-strict-hook-2"), + ]), + sse(vec![ + ev_response_created("resp-strict-hook-3"), + ev_assistant_message("msg-strict-hook", "permission hook allowed it"), + ev_completed("resp-strict-hook-3"), + ]), + ], + ) + .await; + + let mut builder = test_codex() + .with_pre_build_hook(|home| { + install_allow_permission_request_hook(home) + .expect("failed to write permission request hook test fixture"); + }) + .with_config(|config| { + trust_discovered_hooks(config); + config + .features + .enable(Feature::RequestPermissionsTool) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + + fs::write(&marker, "seed").context("create strict auto-review marker")?; + test.submit_turn_with_approval_and_permission_profile( + "request strict review, then run the shell command", + AskForApproval::OnRequest, + PermissionProfile::Disabled, + ) + .await?; + + let request = wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::RequestPermissions(_)) + }) + .await; + let EventMsg::RequestPermissions(request) = request else { + panic!("expected request permissions event"); + }; + assert_eq!(request.call_id, permission_call_id); + test.codex + .submit(Op::RequestPermissionsResponse { + id: permission_call_id.to_string(), + response: RequestPermissionsResponse { + permissions: request.permissions, + scope: PermissionGrantScope::Turn, + strict_auto_review: true, + }, + }) + .await?; + + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let requests = responses.requests(); + assert_eq!(requests.len(), 3); + requests[2].function_call_output(command_call_id); + assert!( + !marker.exists(), + "hook-approved command should remove marker without Guardian review" + ); + assert_single_permission_request_hook_input( + test.codex_home_path(), + &command, + /*description*/ None, + )?; + + Ok(()) +} + #[tokio::test] async fn permission_request_hook_allows_apply_patch_with_write_alias() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/protocol/src/request_permissions.rs b/codex-rs/protocol/src/request_permissions.rs index 1752363ec5..f9ade7d149 100644 --- a/codex-rs/protocol/src/request_permissions.rs +++ b/codex-rs/protocol/src/request_permissions.rs @@ -66,7 +66,7 @@ pub struct RequestPermissionsResponse { pub permissions: RequestPermissionProfile, #[serde(default)] pub scope: PermissionGrantScope, - /// Review every subsequent command in this turn before normal sandboxed execution. + /// Review subsequent commands in this turn unless a permission hook resolves the request. #[serde(default, skip_serializing_if = "std::ops::Not::not")] pub strict_auto_review: bool, }