diff --git a/codex-rs/app-server/src/extensions.rs b/codex-rs/app-server/src/extensions.rs index c979baacfa..86b49d375a 100644 --- a/codex-rs/app-server/src/extensions.rs +++ b/codex-rs/app-server/src/extensions.rs @@ -171,8 +171,21 @@ pub(crate) fn guardian_agent_spawner( #[cfg(test)] mod tests { + use std::sync::atomic::AtomicUsize; + use std::sync::atomic::Ordering; use std::time::Duration; + use codex_extension_api::ApprovalReviewError; + use codex_extension_api::ApprovalReviewInput; + use codex_extension_api::ApprovalReviewOutcome; + use codex_extension_api::ApprovalReviewRunner; + use codex_extension_api::ApprovalReviewSource; + use codex_extension_api::ExtensionData; + use codex_extension_api::ExtensionFuture; + use codex_protocol::approvals::GuardianAssessmentAction; + use codex_protocol::config_types::ApprovalsReviewer; + use codex_protocol::protocol::AskForApproval; + use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::ThreadGoal as CoreThreadGoal; use codex_protocol::protocol::ThreadGoalStatus; use codex_protocol::protocol::ThreadGoalUpdatedEvent; @@ -182,6 +195,68 @@ mod tests { use super::*; + struct RecordingApprovalRunner(AtomicUsize); + + impl ApprovalReviewRunner for RecordingApprovalRunner { + fn run(&self) -> ExtensionFuture<'_, Result> { + self.0.fetch_add(1, Ordering::Relaxed); + Box::pin(std::future::ready(Ok(ApprovalReviewOutcome::decision( + ReviewDecision::Approved, + )))) + } + } + + #[tokio::test] + async fn installed_guardian_claims_only_auto_review_requests() { + let mut builder = ExtensionRegistryBuilder::::new(); + codex_guardian::install(&mut builder, ()); + let registry = builder.build(); + let session_store = ExtensionData::new("session"); + let thread_store = ExtensionData::new(ThreadId::default().to_string()); + let turn_store = ExtensionData::new("turn"); + let action = GuardianAssessmentAction::RequestPermissions { + reason: Some("run command".to_string()), + permissions: Default::default(), + }; + let approval_policy = AskForApproval::OnRequest; + let runner = RecordingApprovalRunner(AtomicUsize::new(0)); + let input = |reviewer, approval_policy| ApprovalReviewInput { + session_store: &session_store, + thread_store: &thread_store, + turn_store: &turn_store, + review_id: "review", + turn_id: "turn", + target_item_id: None, + prompt: "review command", + action: &action, + reviewer, + approval_policy, + retry_reason: None, + source: ApprovalReviewSource::MainTurn, + runner: &runner, + }; + + assert_eq!( + registry + .approval_review(input(ApprovalsReviewer::AutoReview, &approval_policy)) + .await, + Ok(ApprovalReviewOutcome::decision(ReviewDecision::Approved)) + ); + assert_eq!( + registry + .approval_review(input(ApprovalsReviewer::User, &approval_policy)) + .await, + Ok(ApprovalReviewOutcome::Abstain) + ); + assert_eq!( + registry + .approval_review(input(ApprovalsReviewer::AutoReview, &AskForApproval::Never)) + .await, + Ok(ApprovalReviewOutcome::Abstain) + ); + assert_eq!(runner.0.load(Ordering::Relaxed), 1); + } + #[tokio::test] async fn app_server_event_sink_uses_listener_fifo_for_goal_updates_and_clears() { let (outgoing_tx, _outgoing_rx) = mpsc::channel(4); diff --git a/codex-rs/core/src/tools/approval_dispatch.rs b/codex-rs/core/src/tools/approval_dispatch.rs index fe27f10210..6d5849229c 100644 --- a/codex-rs/core/src/tools/approval_dispatch.rs +++ b/codex-rs/core/src/tools/approval_dispatch.rs @@ -16,7 +16,9 @@ use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use codex_extension_api::ApprovalReviewInput; use codex_extension_api::ApprovalReviewOutcome; +use codex_extension_api::ApprovalReviewRunner; use codex_extension_api::ApprovalReviewSource; +use codex_extension_api::ExtensionFuture; use codex_hooks::PermissionRequestDecision; use codex_otel::ToolDecisionSource; use codex_protocol::protocol::ReviewDecision; @@ -55,6 +57,65 @@ impl AutomatedApprovalDecision { } } +struct CoreApprovalReviewRunner<'a> { + session: &'a std::sync::Arc, + turn: &'a std::sync::Arc, + review_id: &'a str, + request: &'a GuardianApprovalRequest, + retry_reason: Option<&'a str>, + source: ApprovalReviewSource, + cancellation_token: Option<&'a CancellationToken>, +} + +impl ApprovalReviewRunner for CoreApprovalReviewRunner<'_> { + fn run( + &self, + ) -> ExtensionFuture<'_, Result> + { + Box::pin(async move { + let decision = if let Some(cancellation_token) = self.cancellation_token { + let review_rx = crate::guardian::spawn_approval_request_review( + std::sync::Arc::clone(self.session), + std::sync::Arc::clone(self.turn), + self.review_id.to_string(), + self.request.clone(), + self.retry_reason.map(str::to_string), + match self.source { + ApprovalReviewSource::MainTurn => { + codex_analytics::GuardianApprovalRequestSource::MainTurn + } + ApprovalReviewSource::DelegatedSubagent => { + codex_analytics::GuardianApprovalRequestSource::DelegatedSubagent + } + }, + cancellation_token.clone(), + ); + review_rx.await.unwrap_or(ReviewDecision::Denied) + } else { + review_approval_request( + self.session, + self.turn, + self.review_id.to_string(), + self.request.clone(), + self.retry_reason.map(str::to_string), + ) + .await + }; + let denial_message = match decision { + ReviewDecision::Denied | ReviewDecision::Abort => { + Some(guardian_rejection_message(self.session, self.review_id).await) + } + ReviewDecision::TimedOut => Some(guardian_timeout_message()), + _ => None, + }; + Ok(ApprovalReviewOutcome::Decision { + decision, + denial_message, + }) + }) + } +} + #[allow(clippy::too_many_arguments)] pub(crate) async fn request_automated_approval( session: &std::sync::Arc, @@ -70,6 +131,15 @@ pub(crate) async fn request_automated_approval( .map_err(|error| format!("failed to render review prompt: {error}"))? .text; let approval_policy = turn.approval_policy.value(); + let runner = CoreApprovalReviewRunner { + session, + turn, + review_id: &review_id, + request: &request, + retry_reason: retry_reason.as_deref(), + source, + cancellation_token: None, + }; let review_input = ApprovalReviewInput { session_store: &session.services.session_extension_data, thread_store: &session.services.thread_extension_data, @@ -83,6 +153,7 @@ pub(crate) async fn request_automated_approval( approval_policy: &approval_policy, retry_reason: retry_reason.as_deref(), source, + runner: &runner, }; match session .services @@ -99,15 +170,12 @@ pub(crate) async fn request_automated_approval( source: AutomatedApprovalSource::Extension, }), Ok(ApprovalReviewOutcome::Abstain) => { - let decision = - review_approval_request(session, turn, review_id.clone(), request, retry_reason) - .await; - let denial_message = match decision { - ReviewDecision::Denied | ReviewDecision::Abort => { - Some(guardian_rejection_message(session, &review_id).await) - } - ReviewDecision::TimedOut => Some(guardian_timeout_message()), - _ => None, + let ApprovalReviewOutcome::Decision { + decision, + denial_message, + } = runner.run().await.map_err(|error| error.to_string())? + else { + return Err("core approval reviewer unexpectedly abstained".to_string()); }; Ok(AutomatedApprovalDecision { decision, @@ -138,6 +206,15 @@ pub(crate) async fn request_automated_approval_with_cancel( } }; let approval_policy = turn.approval_policy.value(); + let runner = CoreApprovalReviewRunner { + session, + turn, + review_id: &review_id, + request: &request, + retry_reason: retry_reason.as_deref(), + source, + cancellation_token: Some(&cancellation_token), + }; let review_input = ApprovalReviewInput { session_store: &session.services.session_extension_data, thread_store: &session.services.thread_extension_data, @@ -151,6 +228,7 @@ pub(crate) async fn request_automated_approval_with_cancel( approval_policy: &approval_policy, retry_reason: retry_reason.as_deref(), source, + runner: &runner, }; let extension_outcome = tokio::select! { @@ -168,33 +246,23 @@ pub(crate) async fn request_automated_approval_with_cancel( source: AutomatedApprovalSource::Extension, })), Ok(ApprovalReviewOutcome::Abstain) => { - let review_rx = crate::guardian::spawn_approval_request_review( - std::sync::Arc::clone(session), - std::sync::Arc::clone(turn), - review_id.clone(), - request, - retry_reason, - match source { - ApprovalReviewSource::MainTurn => { - codex_analytics::GuardianApprovalRequestSource::MainTurn - } - ApprovalReviewSource::DelegatedSubagent => { - codex_analytics::GuardianApprovalRequestSource::DelegatedSubagent - } - }, - cancellation_token.clone(), - ); - let decision = tokio::select! { + let outcome = tokio::select! { biased; _ = cancellation_token.cancelled() => return None, - decision = review_rx => decision.unwrap_or(ReviewDecision::Denied), + outcome = runner.run() => outcome, }; - let denial_message = match decision { - ReviewDecision::Denied | ReviewDecision::Abort => { - Some(guardian_rejection_message(session, &review_id).await) - } - ReviewDecision::TimedOut => Some(guardian_timeout_message()), - _ => None, + let outcome = match outcome { + Ok(outcome) => outcome, + Err(error) => return Some(Err(error.to_string())), + }; + let ApprovalReviewOutcome::Decision { + decision, + denial_message, + } = outcome + else { + return Some(Err( + "core approval reviewer unexpectedly abstained".to_string() + )); }; Some(Ok(AutomatedApprovalDecision { decision, diff --git a/codex-rs/ext/extension-api/src/approval_review.rs b/codex-rs/ext/extension-api/src/approval_review.rs index 6d3c0d7d5b..c1f0c35805 100644 --- a/codex-rs/ext/extension-api/src/approval_review.rs +++ b/codex-rs/ext/extension-api/src/approval_review.rs @@ -7,6 +7,16 @@ use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ReviewDecision; use crate::ExtensionData; +use crate::ExtensionFuture; + +/// Request-scoped host capability that executes the configured approval review. +/// +/// Contributors decide whether they own a request before invoking this runner. +/// The host retains responsibility for the underlying review runtime while the +/// contributor owns routing policy and the returned decision. +pub trait ApprovalReviewRunner: Send + Sync { + fn run(&self) -> ExtensionFuture<'_, Result>; +} /// Identifies the runtime that originated an approval-review request. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -37,6 +47,7 @@ pub struct ApprovalReviewInput<'a> { pub approval_policy: &'a AskForApproval, pub retry_reason: Option<&'a str>, pub source: ApprovalReviewSource, + pub runner: &'a dyn ApprovalReviewRunner, } /// Result of offering an approval request to one extension contributor. diff --git a/codex-rs/ext/extension-api/src/lib.rs b/codex-rs/ext/extension-api/src/lib.rs index 27814d49a3..1af84950c1 100644 --- a/codex-rs/ext/extension-api/src/lib.rs +++ b/codex-rs/ext/extension-api/src/lib.rs @@ -8,6 +8,7 @@ mod user_instructions; pub use approval_review::ApprovalReviewError; pub use approval_review::ApprovalReviewInput; pub use approval_review::ApprovalReviewOutcome; +pub use approval_review::ApprovalReviewRunner; pub use approval_review::ApprovalReviewSource; pub use capabilities::AgentSpawnFuture; pub use capabilities::AgentSpawner; diff --git a/codex-rs/ext/extension-api/tests/registry.rs b/codex-rs/ext/extension-api/tests/registry.rs index e5b171db58..b41c86d9a4 100644 --- a/codex-rs/ext/extension-api/tests/registry.rs +++ b/codex-rs/ext/extension-api/tests/registry.rs @@ -5,6 +5,7 @@ use codex_extension_api::ApprovalReviewContributor; use codex_extension_api::ApprovalReviewError; use codex_extension_api::ApprovalReviewInput; use codex_extension_api::ApprovalReviewOutcome; +use codex_extension_api::ApprovalReviewRunner; use codex_extension_api::ApprovalReviewSource; use codex_extension_api::ConfigContributor; use codex_extension_api::ContextContributor; @@ -260,6 +261,16 @@ struct RecordingApprovalContributor { calls: Arc>>, } +struct UnusedApprovalRunner; + +impl ApprovalReviewRunner for UnusedApprovalRunner { + fn run(&self) -> ExtensionFuture<'_, Result> { + Box::pin(std::future::ready(Err(ApprovalReviewError::new( + "test contributor should not invoke the runner", + )))) + } +} + impl ApprovalReviewContributor for RecordingApprovalContributor { fn review<'a>( &'a self, @@ -303,6 +314,7 @@ fn approval_review_input<'a>( action: &'a GuardianAssessmentAction, approval_policy: &'a AskForApproval, ) -> ApprovalReviewInput<'a> { + static RUNNER: UnusedApprovalRunner = UnusedApprovalRunner; ApprovalReviewInput { session_store, thread_store, @@ -316,6 +328,7 @@ fn approval_review_input<'a>( approval_policy, retry_reason: Some("initial review timed out"), source: ApprovalReviewSource::DelegatedSubagent, + runner: &RUNNER, } } diff --git a/codex-rs/ext/guardian/src/lib.rs b/codex-rs/ext/guardian/src/lib.rs index a64cf4fed4..240a84768b 100644 --- a/codex-rs/ext/guardian/src/lib.rs +++ b/codex-rs/ext/guardian/src/lib.rs @@ -3,11 +3,16 @@ use std::sync::Arc; use codex_core::config::Config; use codex_extension_api::AgentSpawnFuture; use codex_extension_api::AgentSpawner; +use codex_extension_api::ApprovalReviewContributor; +use codex_extension_api::ApprovalReviewInput; +use codex_extension_api::ApprovalReviewOutcome; use codex_extension_api::ExtensionFuture; use codex_extension_api::ExtensionRegistryBuilder; use codex_extension_api::ThreadLifecycleContributor; use codex_extension_api::ThreadStartInput; use codex_protocol::ThreadId; +use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::protocol::AskForApproval; /// Guardian extension dependencies supplied by the host at construction time. #[derive(Clone, Debug)] @@ -68,10 +73,35 @@ where } } +impl ApprovalReviewContributor for GuardianExtension +where + S: Send + Sync, +{ + fn review<'a>( + &'a self, + input: ApprovalReviewInput<'a>, + ) -> ExtensionFuture<'a, Result> + { + Box::pin(async move { + if input.reviewer != ApprovalsReviewer::AutoReview + || !matches!( + input.approval_policy, + AskForApproval::OnRequest | AskForApproval::Granular(_) + ) + { + return Ok(ApprovalReviewOutcome::Abstain); + } + input.runner.run().await + }) + } +} + /// Installs the guardian contributors into the extension registry. pub fn install(registry: &mut ExtensionRegistryBuilder, agent_spawner: S) where S: Send + Sync + 'static, { - registry.thread_lifecycle_contributor(Arc::new(GuardianExtension::new(agent_spawner))); + let extension = Arc::new(GuardianExtension::new(agent_spawner)); + registry.thread_lifecycle_contributor(extension.clone()); + registry.approval_review_contributor(extension); }