From 6ecb99ea1acde92814b3a46ab516eca3f0ceaaf0 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Thu, 9 Jul 2026 11:11:37 -0700 Subject: [PATCH] rm struct --- codex-rs/core/src/approval_coordinator.rs | 225 ---------------------- codex-rs/core/src/lib.rs | 1 - codex-rs/core/src/tools/approvals.rs | 220 +++++++++++++++++++++ codex-rs/core/src/tools/mod.rs | 1 + codex-rs/core/src/tools/orchestrator.rs | 9 +- codex-rs/core/src/tools/sandboxing.rs | 2 +- 6 files changed, 226 insertions(+), 232 deletions(-) delete mode 100644 codex-rs/core/src/approval_coordinator.rs create mode 100644 codex-rs/core/src/tools/approvals.rs diff --git a/codex-rs/core/src/approval_coordinator.rs b/codex-rs/core/src/approval_coordinator.rs deleted file mode 100644 index d440722fe9..0000000000 --- a/codex-rs/core/src/approval_coordinator.rs +++ /dev/null @@ -1,225 +0,0 @@ -//! 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 785c83f93f..e57288f05d 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -6,7 +6,6 @@ #![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/approvals.rs b/codex-rs/core/src/tools/approvals.rs new file mode 100644 index 0000000000..6e71bb7311 --- /dev/null +++ b/codex-rs/core/src/tools/approvals.rs @@ -0,0 +1,220 @@ +//! 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(super) type ApprovalAction = crate::guardian::GuardianApprovalRequest; + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(super) enum ApprovalReviewer { + Guardian, + User, +} + +impl ApprovalReviewer { + pub(super) fn for_turn(turn: &TurnContext) -> Self { + Self::for_reviewer(turn, turn.config.approvals_reviewer) + } + + 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)] +enum ApprovalResolutionSource { + Hook, + Guardian, + User, +} + +#[derive(Clone, Debug, PartialEq, Eq)] +struct ApprovalResolution { + decision: ReviewDecision, + rejection: Option, + source: ApprovalResolutionSource, +} + +impl ApprovalResolution { + fn into_tool_result(self) -> Result { + if let Some(rejection) = self.rejection { + Err(ToolError::Rejected(rejection)) + } else { + Ok(self.decision) + } + } +} + +pub(super) async fn resolve_tool_apporval( + 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, + }; + record_resolution(otel, tool_ctx, &resolution); + return resolution.into_tool_result(); + } + Some(PermissionRequestDecision::Deny { message }) => { + let resolution = ApprovalResolution { + decision: ReviewDecision::Denied, + rejection: Some(message), + source: ApprovalResolutionSource::Hook, + }; + record_resolution(otel, tool_ctx, &resolution); + return resolution.into_tool_result(); + } + 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, + }; + record_resolution(otel, tool_ctx, &resolution); + return resolution.into_tool_result(); + } + }; + let decision = review_approval_request( + ctx.session, + ctx.turn, + review_id.clone(), + action, + ctx.retry_reason.clone(), + ) + .await; + 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 = normalize_user_rejection(resolution); + record_resolution(otel, tool_ctx, &resolution); + resolution.into_tool_result() +} + +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/tools/mod.rs b/codex-rs/core/src/tools/mod.rs index 1bc9786217..76a836def3 100644 --- a/codex-rs/core/src/tools/mod.rs +++ b/codex-rs/core/src/tools/mod.rs @@ -1,3 +1,4 @@ +mod approvals; pub(crate) mod code_mode; pub(crate) mod context; pub(crate) mod events; diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 966c2082b2..dab3038b23 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -6,8 +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::approval_coordinator::ApprovalCoordinator; -use crate::approval_coordinator::ApprovalReviewer; +use super::approvals::ApprovalReviewer; +use super::approvals::resolve_tool_apporval; use crate::network_policy_decision::network_approval_context_from_payload; use crate::tools::flat_tool_name; use crate::tools::network_approval::ActiveNetworkApproval; @@ -519,7 +519,7 @@ impl ToolOrchestrator { where T: ToolRuntime, { - ApprovalCoordinator::resolve_tool( + resolve_tool_apporval( tool, req, permission_request_run_id, @@ -528,8 +528,7 @@ impl ToolOrchestrator { reviewer, otel, ) - .await? - .into_tool_result() + .await } } diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 11efb05dfc..700d08abc5 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 = crate::approval_coordinator::ApprovalAction; +pub(crate) type ApprovalAction = super::approvals::ApprovalAction; #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct PermissionRequestPayload {