diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index acbeba6f84..01ae1e95ea 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -46,10 +46,6 @@ pub(crate) use review::is_basic_session_source; pub(crate) use review::new_guardian_review_id; #[cfg(test)] pub(crate) use review::record_guardian_denial_for_test; -#[cfg(test)] -pub(crate) use review::review_approval_request; -#[cfg(test)] -pub(crate) use review::review_approval_request_with_cancel; pub(crate) use review::routes_approval_policy_to_guardian; pub(crate) use review::routes_approval_to_guardian; pub(crate) use review_session::GuardianReviewSessionManager; diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 8b14e449c4..c63586fe89 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -6,13 +6,7 @@ use codex_analytics::GuardianReviewFailureReason; use codex_analytics::GuardianReviewTerminalStatus; use codex_analytics::GuardianReviewTrackContext; use codex_analytics::GuardianReviewedAction; -#[cfg(test)] -use codex_analytics::GuardianV2Event; -#[cfg(test)] -use codex_analytics::GuardianV2EventKind; use codex_core_plugins::PluginCommandAttribution; -#[cfg(test)] -use codex_extension_api::GuardianV2Enabled; use codex_extension_api::ThreadIdleCause; use codex_features::Feature; use codex_protocol::config_types::ApprovalsReviewer; @@ -32,8 +26,6 @@ use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SubAgentSource; use codex_protocol::protocol::TurnAbortReason; use codex_protocol::protocol::WarningEvent; -#[cfg(test)] -use futures::future::BoxFuture; use std::sync::Arc; use std::time::Duration; use tokio::time::Instant; @@ -57,8 +49,6 @@ use super::GuardianAssessmentOutcome; use super::GuardianRejectionCircuitBreakerAction; use super::GuardianRejectionCircuitBreakerPolicy; use super::GuardianReviewContext; -#[cfg(test)] -use super::approval_request::format_guardian_action_compact; use super::approval_request::format_guardian_action_pretty; use super::approval_request::guardian_assessment_action; use super::approval_request::guardian_request_target_item_id; @@ -342,112 +332,10 @@ pub(crate) async fn record_guardian_denial_for_test( record_guardian_denial(session, turn, turn_id).await; } -/// Runs Guardian unless Full Access or an installed extension resolves the review. -/// Guardian timeouts, review-session failures, and parse failures all block -/// execution, with timeouts surfaced separately from explicit denials. -#[cfg(test)] -async fn run_guardian_review( - session: Arc, - context: GuardianReviewContext, - review_id: String, - request: GuardianApprovalRequest, - reasons: ApprovalRequestReasons, - options: GuardianReviewOptions, -) -> ReviewDecision { - let turn = Arc::clone(context.turn()); - if context.environments().has_full_access( - context.approval_policy, - &turn.config.permissions.effective_permission_profile(), - ) { - return if options - .external_cancel - .as_ref() - .is_some_and(CancellationToken::is_cancelled) - { - ReviewDecision::Abort - } else { - ReviewDecision::Approved - }; - } - let requires_synchronous_review = options.require_synchronous_review - || reasons.retry.is_some() - || matches!( - &request, - GuardianApprovalRequest::ExecCommand { - sandbox_permissions, - .. - } if sandbox_permissions.requires_escalated_permissions() - ); - // Guardian V2 may satisfy ordinary reviews, including required-model reviews, but broader - // permission requests, retries, and elicitations requiring synchronous review must not use - // extension fast approval. - if (!turn - .config - .config_layer_stack - .requirements() - .auto_review_required_for_model(&turn.model_info().slug) - || turn.config.features.enabled(Feature::GuardianV2)) - && !requires_synchronous_review - && options - .external_cancel - .as_ref() - .is_none_or(|cancel| !cancel.is_cancelled()) - && let Ok(action) = format_guardian_action_compact(&request) - && let Some(decision) = session - .services - .extensions - .fast_approval_decision( - &session.services.session_extension_data, - &session.services.thread_extension_data, - &action, - Some(crate::session::extension_metrics::from_session_telemetry( - turn.session_telemetry.clone(), - )), - ) - .await - { - if decision == ReviewDecision::Approved { - if session - .services - .thread_extension_data - .get::() - .is_some() - { - session - .services - .analytics_events_client - .track_guardian_v2_event(GuardianV2Event { - thread_id: session.thread_id.to_string(), - turn_id: guardian_request_turn_id(&request, &turn.sub_id).to_owned(), - item_id: guardian_request_target_item_id(&request).map(str::to_owned), - model: Some(turn.model_info().slug.clone()), - occurred_at_ms: codex_analytics::now_unix_millis(), - kind: GuardianV2EventKind::FastDecision { - decision: "approved", - }, - }); - } - record_guardian_non_denial(&session, guardian_request_turn_id(&request, &turn.sub_id)) - .await; - } - return decision; - } - - let runtime = super::runtime::ReviewRuntime { - session, - context, - review_id, - request: request.into(), - reasons, - options, - }; - run_synchronous_review(runtime, /*review_reason*/ None).await -} - /// Runs the existing synchronous reviewer without choosing policy or reusing a score. pub(super) async fn run_synchronous_review( runtime: super::runtime::ReviewRuntime, - review_reason: Option, + review_reason: codex_protocol::approvals::GuardianReviewReason, ) -> ReviewDecision { let super::runtime::ReviewRuntime { session, @@ -524,7 +412,7 @@ pub(super) async fn run_synchronous_review( .send_event( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { - review_reason, + review_reason: Some(review_reason), id: review_id.clone(), target_item_id: target_item_id.clone(), plugin_id: plugin_id.clone(), @@ -564,7 +452,7 @@ pub(super) async fn run_synchronous_review( .send_event( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { - review_reason, + review_reason: Some(review_reason), id: review_id, target_item_id, plugin_id: plugin_id.clone(), @@ -711,7 +599,7 @@ pub(super) async fn run_synchronous_review( .send_event( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { - review_reason, + review_reason: Some(review_reason), id: review_id, target_item_id, plugin_id: plugin_id.clone(), @@ -749,7 +637,7 @@ pub(super) async fn run_synchronous_review( .send_event( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { - review_reason, + review_reason: Some(review_reason), id: review_id, target_item_id, plugin_id: plugin_id.clone(), @@ -835,7 +723,7 @@ pub(super) async fn run_synchronous_review( GuardianAssessmentStatus::Denied }; let assessment_event = GuardianAssessmentEvent { - review_reason, + review_reason: Some(review_reason), id: review_id, target_item_id, plugin_id: plugin_id.clone(), @@ -906,56 +794,6 @@ pub(crate) struct GuardianReviewOptions { pub(crate) require_synchronous_review: bool, } -/// Public entrypoint for approval requests that should be reviewed by guardian. -#[cfg(test)] -pub(crate) async fn review_approval_request( - session: &Arc, - context: impl Into, - review_id: String, - request: GuardianApprovalRequest, - reasons: ApprovalRequestReasons, -) -> ReviewDecision { - // Erase the delegated future to bound its async state and the tool handlers' Send checks. - let review: BoxFuture<'_, ReviewDecision> = Box::pin(run_guardian_review( - Arc::clone(session), - context.into(), - review_id, - request, - reasons, - GuardianReviewOptions { - require_guardian: false, - plugin_attribution_override: None, - approval_request_source: GuardianApprovalRequestSource::MainTurn, - external_cancel: None, - require_synchronous_review: false, - }, - )); - review.await -} - -#[cfg(test)] -pub(crate) async fn review_approval_request_with_cancel( - session: &Arc, - context: impl Into, - review_id: String, - request: GuardianApprovalRequest, - retry_reason: Option, - options: GuardianReviewOptions, -) -> ReviewDecision { - run_guardian_review( - Arc::clone(session), - context.into(), - review_id, - request, - ApprovalRequestReasons { - approval: None, - retry: retry_reason, - }, - options, - ) - .await -} - pub(super) struct GuardianReviewSessionConfig { pub(super) spawn_config: crate::config::Config, pub(super) node_repl_policy: GuardianNodeReplPolicy, diff --git a/codex-rs/core/src/guardian/runtime.rs b/codex-rs/core/src/guardian/runtime.rs index 097172cd46..29772ec5e2 100644 --- a/codex-rs/core/src/guardian/runtime.rs +++ b/codex-rs/core/src/guardian/runtime.rs @@ -89,6 +89,6 @@ impl SynchronousApprovalReviewer for ReviewRuntime { &self, reason: codex_protocol::approvals::GuardianReviewReason, ) -> ExtensionFuture<'_, ReviewDecision> { - Box::pin(run_synchronous_review(self.clone(), Some(reason))) + Box::pin(run_synchronous_review(self.clone(), reason)) } } diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index d4810c480a..ac0745faca 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -1688,8 +1688,8 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() { let cancel_token = CancellationToken::new(); cancel_token.cancel(); - let decision = review_approval_request_with_cancel( - &session, + let decision = super::decide_approval( + Arc::clone(&session), &turn, "review-cancelled-guardian".to_string(), GuardianApprovalRequest::ApplyPatch { @@ -1699,9 +1699,12 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() { patch: "*** Begin Patch\n*** Update File: guardian.txt\n@@\n+hello\n*** End Patch" .to_string(), }, - /*retry_reason*/ None, + ApprovalRequestReasons { + approval: None, + retry: None, + }, GuardianReviewOptions { - require_guardian: false, + require_guardian: true, plugin_attribution_override: None, approval_request_source: GuardianApprovalRequestSource::MainTurn, external_cancel: Some(cancel_token), @@ -1710,7 +1713,7 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() { ) .await; - assert_eq!(decision, ReviewDecision::Abort); + assert_eq!(decision, Some(ReviewDecision::Abort)); let mut guardian_statuses = Vec::new(); let mut warnings = Vec::new(); @@ -3399,14 +3402,12 @@ async fn escalated_retry_bypasses_extension_approval_and_runs_guardian() -> anyh struct AutoApprovingReviewContributor; impl codex_extension_api::ApprovalReviewContributor for AutoApprovingReviewContributor { - fn fast_decision<'a>( + fn decide<'a>( &'a self, - _session_store: &'a codex_extension_api::ExtensionData, - _thread_store: &'a codex_extension_api::ExtensionData, - _prompt: &'a str, - _extension_metrics: Option>, - ) -> codex_extension_api::ExtensionFuture<'a, Option> { - Box::pin(async move { Some(ReviewDecision::Approved) }) + _input: &'a codex_extension_api::ApprovalDecisionInput<'_>, + ) -> codex_extension_api::ExtensionFuture<'a, Option> + { + Box::pin(async { Some(codex_extension_api::ApprovalDecision::Allow) }) } } @@ -4188,3 +4189,29 @@ async fn guardian_review_session_config_uses_default_guardian_policy_without_req )) ); } + +// Keep the existing reviewer tests on the production decision path. +async fn review_approval_request( + session: &Arc, + context: impl Into, + review_id: String, + request: GuardianApprovalRequest, + reasons: ApprovalRequestReasons, +) -> ReviewDecision { + super::decide_approval( + Arc::clone(session), + context, + review_id, + request, + reasons, + GuardianReviewOptions { + require_guardian: true, + plugin_attribution_override: None, + approval_request_source: GuardianApprovalRequestSource::MainTurn, + external_cancel: None, + require_synchronous_review: false, + }, + ) + .await + .expect("Guardian should handle the request") +} diff --git a/codex-rs/ext/extension-api/src/contributors.rs b/codex-rs/ext/extension-api/src/contributors.rs index 0f57a7441e..8c75997b63 100644 --- a/codex-rs/ext/extension-api/src/contributors.rs +++ b/codex-rs/ext/extension-api/src/contributors.rs @@ -4,7 +4,6 @@ use std::sync::Arc; use codex_context_fragments::ContextualUserFragment; use codex_protocol::items::TurnItem; -use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::TokenUsageInfo; use codex_tools::ToolCall; use codex_tools::ToolExecutor; @@ -23,7 +22,6 @@ mod turn_input; mod turn_lifecycle; mod world_state; -pub use approval_review::ApprovalAssessment; pub use approval_review::ApprovalDecision; pub use approval_review::ApprovalDecisionInput; pub use approval_review::ApprovalReviewError; @@ -349,11 +347,8 @@ pub trait ToolLifecycleContributor: Send + Sync { } } -/// Extension contribution for fast approval decisions and full action reviews. -/// -/// Implementations can provide a fast decision from existing evidence, perform -/// a full structured review, or support both paths. Returning `None` leaves the -/// request available to the next contributor or the host's fallback path. +/// Owns the complete approval decision, including whether to consult a reviewer. +/// Returning `None` leaves the request to the next contributor. pub trait ApprovalReviewContributor: Send + Sync { /// Claims one request, including a handoff to the user. fn decide<'a>( @@ -362,25 +357,6 @@ pub trait ApprovalReviewContributor: Send + Sync { ) -> ExtensionFuture<'a, Option> { Box::pin(std::future::ready(None)) } - - /// Returns an available approval decision without performing a full review. - fn fast_decision<'a>( - &'a self, - _session_store: &'a ExtensionData, - _thread_store: &'a ExtensionData, - _prompt: &'a str, - _extension_metrics: Option>, - ) -> ExtensionFuture<'a, Option> { - Box::pin(std::future::ready(None)) - } - - /// Performs a full review of a structured host-owned approval request. - fn full_review<'a>( - &'a self, - _input: &'a ApprovalReviewInput<'_>, - ) -> ExtensionFuture<'a, Option>> { - Box::pin(std::future::ready(None)) - } } /// Ordered post-processing contribution for one parsed turn item. diff --git a/codex-rs/ext/extension-api/src/contributors/approval_review.rs b/codex-rs/ext/extension-api/src/contributors/approval_review.rs index a090962ec8..fe1c342b26 100644 --- a/codex-rs/ext/extension-api/src/contributors/approval_review.rs +++ b/codex-rs/ext/extension-api/src/contributors/approval_review.rs @@ -4,9 +4,6 @@ use std::fmt; use std::sync::Arc; use codex_protocol::ThreadId; -use codex_protocol::approvals::GuardianAssessmentOutcome; -use codex_protocol::approvals::GuardianRiskLevel; -use codex_protocol::approvals::GuardianUserAuthorization; use serde_json::Value; use crate::ConversationHistorySnapshot; @@ -15,19 +12,6 @@ use crate::ExtensionData; /// Thread-local state installed only after Guardian V2's async classifier initializes. pub struct GuardianV2Enabled; -/// Classification returned by an approval reviewer. -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct ApprovalAssessment { - /// Final allow-or-deny decision for the requested action. - pub outcome: GuardianAssessmentOutcome, - /// Risk level assigned to the action by the reviewer. - pub risk_level: GuardianRiskLevel, - /// Whether the conversation authorizes the requested action. - pub user_authorization: GuardianUserAuthorization, - /// Human-readable explanation of the assessment. - pub rationale: String, -} - /// Operational failure returned by an approval reviewer. #[derive(Clone, Debug, Eq, PartialEq)] pub enum ApprovalReviewError { diff --git a/codex-rs/ext/extension-api/src/lib.rs b/codex-rs/ext/extension-api/src/lib.rs index 9e8478679e..84f2ac0b8e 100644 --- a/codex-rs/ext/extension-api/src/lib.rs +++ b/codex-rs/ext/extension-api/src/lib.rs @@ -40,7 +40,6 @@ pub use codex_tools::TurnItemEmissionFuture; pub use codex_tools::TurnItemEmitter; pub use codex_tools::parse_tool_input_schema; pub use codex_tools::parse_tool_input_schema_without_compaction; -pub use contributors::ApprovalAssessment; pub use contributors::ApprovalDecision; pub use contributors::ApprovalDecisionInput; pub use contributors::ApprovalReviewContributor; diff --git a/codex-rs/ext/extension-api/src/registry.rs b/codex-rs/ext/extension-api/src/registry.rs index be706b5061..e4aeba9b5e 100644 --- a/codex-rs/ext/extension-api/src/registry.rs +++ b/codex-rs/ext/extension-api/src/registry.rs @@ -1,16 +1,9 @@ use std::sync::Arc; -use codex_protocol::protocol::ReviewDecision; - -use crate::ApprovalAssessment; use crate::ApprovalReviewContributor; -use crate::ApprovalReviewError; -use crate::ApprovalReviewInput; use crate::ConfigContributor; use crate::ContextContributor; -use crate::ExtensionData; use crate::ExtensionEventSink; -use crate::ExtensionMetrics; use crate::McpServerContributor; use crate::NoopExtensionEventSink; use crate::SkillInvocationContributor; @@ -202,45 +195,6 @@ impl ExtensionRegistry { .any(|contributor| contributor.requires_host_skill_discovery()) } - /// Returns the first full approval assessment claimed by a contributor. - pub async fn full_approval_review( - &self, - input: ApprovalReviewInput<'_>, - ) -> Option> { - for contributor in &self.approval_review_contributors { - if let Some(assessment) = contributor.full_review(&input).await { - return Some(assessment); - } - } - - None - } - - /// Returns the first fast approval decision claimed by a contributor. - pub async fn fast_approval_decision( - &self, - session_store: &ExtensionData, - thread_store: &ExtensionData, - prompt: &str, - extension_metrics: Option>, - ) -> Option { - for contributor in &self.approval_review_contributors { - if let Some(decision) = contributor - .fast_decision( - session_store, - thread_store, - prompt, - extension_metrics.clone(), - ) - .await - { - return Some(decision); - } - } - - None - } - /// Returns the first claimed decision in registration order. pub async fn decide_approval( &self, diff --git a/codex-rs/ext/extension-api/tests/registry.rs b/codex-rs/ext/extension-api/tests/registry.rs index 919a38d8d1..c354628382 100644 --- a/codex-rs/ext/extension-api/tests/registry.rs +++ b/codex-rs/ext/extension-api/tests/registry.rs @@ -3,10 +3,7 @@ use std::sync::Arc; use std::sync::Mutex; -use codex_extension_api::ApprovalAssessment; use codex_extension_api::ApprovalReviewContributor; -use codex_extension_api::ApprovalReviewError; -use codex_extension_api::ApprovalReviewInput; use codex_extension_api::ConfigContributor; use codex_extension_api::ContentItemKind; use codex_extension_api::ContextContributor; @@ -34,21 +31,14 @@ use codex_extension_api::TurnInputContext; use codex_extension_api::TurnInputContributor; use codex_extension_api::TurnItemContributor; use codex_extension_api::TurnLifecycleContributor; -use codex_extension_api::empty_extension_registry; -use codex_protocol::ThreadId; -use codex_protocol::approvals::GuardianAssessmentOutcome; -use codex_protocol::approvals::GuardianRiskLevel; -use codex_protocol::approvals::GuardianUserAuthorization; use codex_protocol::items::HookPromptItem; use codex_protocol::items::TurnItem; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; -use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SubAgentSource; use codex_protocol::protocol::WarningEvent; use pretty_assertions::assert_eq; -use serde_json::json; struct AllContributors; @@ -166,33 +156,22 @@ impl TurnItemContributor for AllContributors { } impl ApprovalReviewContributor for AllContributors { - fn fast_decision<'a>( + fn decide<'a>( &'a self, - _session_store: &'a ExtensionData, - _thread_store: &'a ExtensionData, - _prompt: &'a str, - _extension_metrics: Option>, - ) -> ExtensionFuture<'a, Option> { - Box::pin(async move { - let _self = self; - Some(ReviewDecision::ApprovedForSession) - }) + _input: &'a codex_extension_api::ApprovalDecisionInput<'_>, + ) -> ExtensionFuture<'a, Option> { + Box::pin(async { Some(codex_extension_api::ApprovalDecision::AskUser) }) } +} - fn full_review<'a>( - &'a self, - input: &'a ApprovalReviewInput<'_>, - ) -> ExtensionFuture<'a, Option>> { - assert_eq!( - (input.approval_reason, input.retry_reason), - (Some("A policy rule requires approval."), None) - ); - Box::pin(std::future::ready(Some(Ok(ApprovalAssessment { - outcome: GuardianAssessmentOutcome::Allow, - risk_level: GuardianRiskLevel::Low, - user_authorization: GuardianUserAuthorization::High, - rationale: "approved".to_string(), - })))) +impl codex_extension_api::SynchronousApprovalReviewer for AllContributors { + fn review( + &self, + _reason: codex_protocol::approvals::GuardianReviewReason, + ) -> ExtensionFuture<'_, codex_protocol::protocol::ReviewDecision> { + Box::pin(std::future::ready( + codex_protocol::protocol::ReviewDecision::Approved, + )) } } @@ -223,16 +202,24 @@ async fn build_round_trips_every_contributor_category() { assert_eq!(registry.tool_contributors().len(), 1); assert_eq!(registry.tool_lifecycle_contributors().len(), 1); assert_eq!(registry.turn_item_contributors().len(), 1); + let thread_store = ExtensionData::new("thread"); + let input = codex_extension_api::ApprovalDecisionInput { + approval_id: "approval-1", + action: &serde_json::Value::Null, + thread_id: codex_protocol::ThreadId::new(), + thread_store: &thread_store, + category: codex_protocol::openai_models::GuardianScope::Shell, + approval_policy: codex_protocol::protocol::AskForApproval::OnRequest, + approvals_reviewer: codex_protocol::config_types::ApprovalsReviewer::AutoReview, + require_guardian: false, + require_fresh_review: false, + full_access: false, + metrics: None, + synchronous_reviewer: &AllContributors, + }; assert_eq!( - registry - .fast_approval_decision( - &ExtensionData::new("session"), - &ExtensionData::new("thread"), - "review this", - /*extension_metrics*/ None, - ) - .await, - Some(ReviewDecision::ApprovedForSession) + registry.decide_approval(&input).await, + Some(codex_extension_api::ApprovalDecision::AskUser) ); } @@ -250,86 +237,6 @@ impl ConversationHistorySnapshot for AllContributors { } } -#[tokio::test] -async fn full_approval_review_returns_first_claim_and_short_circuits() { - let assessment = ApprovalAssessment { - outcome: GuardianAssessmentOutcome::Allow, - risk_level: GuardianRiskLevel::Low, - user_authorization: GuardianUserAuthorization::High, - rationale: "approved".to_string(), - }; - let action = json!({ - "tool": "exec_command", - "command": ["echo", "hello"], - }); - let thread_store = ExtensionData::new("thread"); - let input = ApprovalReviewInput { - action: &action, - conversation_history: Arc::new(AllContributors), - thread_id: ThreadId::default(), - thread_store: &thread_store, - turn_id: "turn", - approval_reason: Some("A policy rule requires approval."), - retry_reason: None, - }; - - let calls = Arc::new(Mutex::new(Vec::new())); - let mut builder = ExtensionRegistryBuilder::<()>::new(); - for (name, result) in [ - ("first", None), - ("second", Some(Ok(assessment.clone()))), - ( - "third", - Some(Err(ApprovalReviewError::Failed( - "should not run".to_string(), - ))), - ), - ] { - builder.approval_review_contributor(Arc::new(RecordingStructuredApprovalContributor { - name, - result, - calls: Arc::clone(&calls), - })); - } - - assert_eq!( - builder.build().full_approval_review(input).await, - Some(Ok(assessment)) - ); - assert_eq!( - calls.lock().expect("approval calls lock").as_slice(), - ["first", "second"] - ); -} - -struct RecordingStructuredApprovalContributor { - name: &'static str, - result: Option>, - calls: Arc>>, -} - -impl ApprovalReviewContributor for RecordingStructuredApprovalContributor { - fn full_review<'a>( - &'a self, - input: &'a ApprovalReviewInput<'_>, - ) -> ExtensionFuture<'a, Option>> { - Box::pin(async move { - assert_eq!( - input.action, - &json!({ - "tool": "exec_command", - "command": ["echo", "hello"], - }) - ); - self.calls - .lock() - .expect("approval calls lock should not be poisoned") - .push(self.name); - self.result.clone() - }) - } -} - struct NamedContextContributor(&'static str); impl ContextContributor for NamedContextContributor { @@ -465,92 +372,6 @@ async fn contributors_preserve_registration_order() { ); } -#[derive(Debug, PartialEq, Eq)] -struct ApprovalCall { - contributor: &'static str, - session_id: String, - thread_id: String, - prompt: String, -} - -struct RecordingApprovalContributor { - name: &'static str, - decision: Option, - calls: Arc>>, -} - -impl ApprovalReviewContributor for RecordingApprovalContributor { - fn fast_decision<'a>( - &'a self, - session_store: &'a ExtensionData, - thread_store: &'a ExtensionData, - prompt: &'a str, - _extension_metrics: Option>, - ) -> ExtensionFuture<'a, Option> { - Box::pin(async move { - self.calls - .lock() - .expect("approval calls lock should not be poisoned") - .push(ApprovalCall { - contributor: self.name, - session_id: session_store.level_id().to_string(), - thread_id: thread_store.level_id().to_string(), - prompt: prompt.to_string(), - }); - self.decision.clone() - }) - } -} - -#[tokio::test] -async fn fast_approval_decision_returns_first_claim_and_short_circuits() { - let calls = Arc::new(Mutex::new(Vec::new())); - let mut builder = ExtensionRegistryBuilder::<()>::new(); - for (name, decision) in [ - ("first", None), - ("second", Some(ReviewDecision::Approved)), - ( - "third", - Some(ReviewDecision::denied("rejected by extension")), - ), - ] { - builder.approval_review_contributor(Arc::new(RecordingApprovalContributor { - name, - decision, - calls: Arc::clone(&calls), - })); - } - let registry = builder.build(); - - let decision = registry - .fast_approval_decision( - &ExtensionData::new("session-1"), - &ExtensionData::new("thread-1"), - "allow command?", - /*extension_metrics*/ None, - ) - .await; - - assert_eq!(decision, Some(ReviewDecision::Approved)); - assert_eq!( - calls.lock().expect("approval calls lock").as_slice(), - [ - ApprovalCall { - contributor: "first", - session_id: "session-1".to_string(), - thread_id: "thread-1".to_string(), - prompt: "allow command?".to_string(), - }, - ApprovalCall { - contributor: "second", - session_id: "session-1".to_string(), - thread_id: "thread-1".to_string(), - prompt: "allow command?".to_string(), - }, - ] - ); -} - #[derive(Default)] struct RecordingEventSink { events: Mutex>, @@ -606,23 +427,6 @@ fn custom_event_sink_survives_registry_build() { ); } -#[tokio::test] -async fn empty_registry_does_not_claim_fast_approval_decision() { - let registry = empty_extension_registry::<()>(); - - assert_eq!( - registry - .fast_approval_decision( - &ExtensionData::new("session"), - &ExtensionData::new("thread"), - "unclaimed", - /*extension_metrics*/ None, - ) - .await, - None - ); -} - fn warning_event(id: &str, message: &str) -> Event { Event { id: id.to_string(), diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/coverage.rs b/codex-rs/ext/guardian-v2/src/async_scorer/coverage.rs index ac925c18b3..0301abff57 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/coverage.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/coverage.rs @@ -150,15 +150,4 @@ impl GuardianPolicy { == Some("require_escalated") })) } - - pub(super) fn review_scope(action: &serde_json::Value) -> Option { - match action.get("tool").and_then(serde_json::Value::as_str)? { - "mcp_tool_call" => action - .get("server") - .and_then(serde_json::Value::as_str) - .map(GuardianScope::for_mcp_server), - "network_access" => Some(GuardianScope::Network), - tool => GuardianScope::for_tool(&ToolName::plain(tool)), - } - } } diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs index 428aff3097..08659e95a4 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -15,7 +15,6 @@ use codex_core::config::Config; use codex_core::context::GuardianContextMode; use codex_core::context::GuardianReviewEvidence; use codex_core::context::NodeReplReviewEvidence; -use codex_extension_api::ApprovalReviewContributor; use codex_extension_api::ExtensionData; use codex_extension_api::ExtensionEventSink; use codex_extension_api::ExtensionFuture; @@ -40,10 +39,8 @@ use codex_model_provider::create_model_provider; use codex_protocol::ThreadId; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::mcp::is_node_repl_backed_server; -use codex_protocol::openai_models::GuardianReviewMode; use codex_protocol::openai_models::GuardianScope; use codex_protocol::openai_models::ModelInfo; -use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::has_full_access; use codex_protocol::security_risk::SecurityRiskScore; @@ -51,13 +48,9 @@ use super::action::GuardianAction; use super::action::RenderedAction; use super::authorization::ScoreAuthorization; use super::config::GuardianV2Config; -use super::coverage::GuardianPolicy; use super::coverage::UnscoredAction; -use super::metrics::REVIEW_FALLBACK_METRIC; -use super::metrics::TOOL_CALL_LAG_METRIC; use super::metrics::record_classification; use super::metrics::record_classification_risk; -use super::metrics::record_fast_decision; use super::metrics::sampler_failure_reason; use super::parent_compaction::ParentCompactionError; use super::parent_compaction::select_parent_compaction; @@ -72,14 +65,6 @@ use super::trusted_skills::TrustedSkillInvocations; use super::trusted_skills::TrustedSkillRoots; use super::trusted_tools::trusted_tool_context; -/// Explains why Guardian v2 requires synchronous approval review. -#[derive(Debug, Eq, PartialEq)] -pub enum StrictReviewReason { - ElevatedRisk, - StaleScore, - IncompatibleCompaction, -} - enum ClassificationOutcome { Scored, Superseded, @@ -242,195 +227,6 @@ impl SkillInvocationContributor for GuardianV2Extension { } } -impl ApprovalReviewContributor for GuardianV2Extension { - fn fast_decision<'a>( - &'a self, - _session_store: &'a ExtensionData, - thread_store: &'a ExtensionData, - prompt: &'a str, - extension_metrics: Option>, - ) -> ExtensionFuture<'a, Option> { - Box::pin(async move { - let model = thread_store.get::(); - let guardian_config = thread_store.get::()?; - let policy = guardian_config.policy_for_model(model.as_deref()); - let action = serde_json::from_str::(prompt).unwrap_or_default(); - let scope = GuardianPolicy::review_scope(&action); - if scope.map_or(policy.other_tools, |scope| policy.mode(scope)) - != GuardianReviewMode::Adaptive - { - record_fast_decision(extension_metrics.as_deref(), "deferred", "out_of_scope"); - return None; - } - let guardian_evidence = thread_store.get_or_init(GuardianReviewEvidence::default); - let context_mode = guardian_evidence.context_mode(); - let mut initial_cua_call = false; - if scope == Some(GuardianScope::ComputerUse) && policy.initial_cua_call { - // Thread-owned context checks checkpoint compatibility before the first CUA allowance. - initial_cua_call = action.get("tool_name").and_then(serde_json::Value::as_str) - == Some("js") - && action - .get("connector_id") - .and_then(serde_json::Value::as_str) - == Some("node_repl") - && thread_store - .get::()? - .js_executions - .load(Ordering::Acquire) - == 1; - if initial_cua_call && context_mode == GuardianContextMode::Legacy { - record_fast_decision( - extension_metrics.as_deref(), - "approved", - "initial_cua_call", - ); - return Some(ReviewDecision::Approved); - } - } else if thread_store.get::().is_some() { - let manager = self.thread_manager.upgrade()?; - let thread_id = ThreadId::from_string(thread_store.level_id()).ok()?; - let Ok(thread) = manager.get_thread(thread_id).await else { - record_fast_decision( - extension_metrics.as_deref(), - "deferred", - "scoring_failure", - ); - return None; - }; - let config = thread.config().await; - let model = thread_store.get::()?; - if config - .config_layer_stack - .requirements() - .auto_review_required_for_model(&model.slug) - { - record_fast_decision( - extension_metrics.as_deref(), - "deferred", - "required_model", - ); - return None; - } - } - let Some(score_progress) = thread_store.get::() else { - record_fast_decision(extension_metrics.as_deref(), "deferred", "missing_score"); - return None; - }; - let manager = self.thread_manager.upgrade()?; - let thread_id = ThreadId::from_string(thread_store.level_id()).ok()?; - let Ok(thread) = manager.get_thread(thread_id).await else { - record_fast_decision(extension_metrics.as_deref(), "deferred", "scoring_failure"); - return None; - }; - let history = thread.conversation_history_snapshot().await; - if context_mode == GuardianContextMode::ThreadOwned { - let sampler = thread_store.get::()?; - if select_parent_compaction( - context_mode, - &guardian_config, - history.as_ref(), - &sampler, - /*legacy_model_hash*/ None, - ) - .is_err() - { - thread_store.insert(StrictReviewReason::IncompatibleCompaction); - record_fast_decision( - extension_metrics.as_deref(), - "deferred", - "incompatible_compaction", - ); - return None; - } - } - if initial_cua_call { - record_fast_decision(extension_metrics.as_deref(), "approved", "initial_cua_call"); - return Some(ReviewDecision::Approved); - } - let current_authorization = ScoreAuthorization::current(&thread).await; - if !current_authorization.local.retained_context_complete - || current_authorization - .root - .is_some_and(|root| !root.retained_context_complete) - { - record_fast_decision( - extension_metrics.as_deref(), - "deferred", - "incomplete_authorization", - ); - return None; - } - let scored_authorization = score_progress - .authorization - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - let latest_scored_tool_call = score_progress - .latest_scored_tool_call - .load(Ordering::Acquire); - let tool_call_lag = score_progress - .latest_tool_call - .load(Ordering::Acquire) - .saturating_sub(latest_scored_tool_call); - if let Some(metrics) = &extension_metrics { - metrics.histogram( - TOOL_CALL_LAG_METRIC, - i64::try_from(tool_call_lag).unwrap_or(i64::MAX), - &[], - ); - } - if tool_call_lag > guardian_config.max_tool_call_lag { - thread_store.insert(StrictReviewReason::StaleScore); - if let Some(metrics) = &extension_metrics { - metrics.counter( - REVIEW_FALLBACK_METRIC, - /*inc*/ 1, - &[("fallback_reason", "score_lag")], - ); - } - record_fast_decision(extension_metrics.as_deref(), "deferred", "stale_score"); - return None; - } - if score_progress - .latest_failed_tool_call - .load(Ordering::Acquire) - > latest_scored_tool_call - { - thread_store.insert(StrictReviewReason::ElevatedRisk); - record_fast_decision(extension_metrics.as_deref(), "deferred", "scoring_failure"); - return None; - } - - let Some(score) = thread_store - .get::() - .and_then(|score| score.scores.get("action_risk").copied()) - else { - record_fast_decision(extension_metrics.as_deref(), "deferred", "missing_score"); - return None; - }; - if score < guardian_config.review_threshold { - if scored_authorization.as_ref() != Some(¤t_authorization) { - thread_store.insert(StrictReviewReason::StaleScore); - record_fast_decision( - extension_metrics.as_deref(), - "deferred", - "authorization_changed", - ); - return None; - } - record_fast_decision(extension_metrics.as_deref(), "approved", "low_risk"); - return Some(ReviewDecision::Approved); - } - if score >= guardian_config.review_threshold { - thread_store.insert(StrictReviewReason::ElevatedRisk); - record_fast_decision(extension_metrics.as_deref(), "deferred", "elevated_risk"); - } else { - record_fast_decision(extension_metrics.as_deref(), "deferred", "invalid_score"); - } - None - }) - } -} - impl ToolLifecycleContributor for GuardianV2Extension { fn on_tool_start<'a>(&'a self, input: ToolStartInput<'a>) -> ToolLifecycleFuture<'a> { Box::pin(self.score_tool(input)) @@ -971,13 +767,12 @@ pub fn install( let extension = Arc::new(GuardianV2Extension { auth_manager, event_sink: registry.event_sink(), - thread_manager: thread_manager.clone(), + thread_manager, }); registry.thread_lifecycle_contributor(extension.clone()); registry.approval_review_contributor(Arc::new(super::approval::GuardianApprovalReviewer { - thread_manager, + thread_manager: extension.thread_manager.clone(), })); - registry.approval_review_contributor(extension.clone()); registry.skill_invocation_contributor(extension.clone()); registry.tool_lifecycle_contributor(extension); } diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs index a15355c656..19be463a6b 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs @@ -69,7 +69,6 @@ use serde_json::json; use super::GuardianV2Extension; use super::GuardianV2ScoreProgress; -use super::StrictReviewReason; use crate::async_scorer::config::CLASSIFICATION_OUTPUT_INSTRUCTIONS; use crate::async_scorer::config::DEFAULT_MODEL_CONTEXT_ITEM_TOKENS; @@ -285,14 +284,13 @@ async fn installed_extension_uses_http_after_warm_socket_auth_expires() -> Resul }) .await?; assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - r#"{"tool":"mcp_tool_call","server":"node_repl"}"#, - /*extension_metrics*/ None, - ) - .await, + cached_approval( + ®istry, + thread_store, + r#"{"tool":"mcp_tool_call","server":"node_repl"}"#, + /*metrics*/ None, + ) + .await, Some(ReviewDecision::Approved) ); } @@ -669,33 +667,29 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> { ] { let prompt = action.to_string(); assert_eq!( - fixture - .registry - .fast_approval_decision( - &fixture.session_store, - thread_store, - &prompt, - thread_store - .get::() - .map(|metrics| metrics as Arc), - ) - .await, - expected, - "unexpected fast approval for {action}" - ); - } - assert_eq!( - fixture - .registry - .fast_approval_decision( - &fixture.session_store, + cached_approval( + &fixture.registry, thread_store, - "not valid JSON", + &prompt, thread_store .get::() .map(|metrics| metrics as Arc), ) .await, + expected, + "unexpected fast approval for {action}" + ); + } + assert_eq!( + cached_approval( + &fixture.registry, + thread_store, + "not valid JSON", + thread_store + .get::() + .map(|metrics| metrics as Arc), + ) + .await, None, "malformed approval actions must not reuse a browser score" ); @@ -711,16 +705,13 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> { }); thread_store.insert(changed_model); assert_eq!( - fixture - .registry - .fast_approval_decision( - &fixture.session_store, - thread_store, - &json!({"tool": "mcp_tool_call", "server": "node_repl", "tool_name": "js"}) - .to_string(), - /*extension_metrics*/ None, - ) - .await, + cached_approval( + &fixture.registry, + thread_store, + &json!({"tool": "mcp_tool_call", "server": "node_repl", "tool_name": "js"}).to_string(), + /*metrics*/ None, + ) + .await, None, "a score from the previous model policy must not approve a call" ); @@ -772,15 +763,13 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> { sampled_at: None, }); assert_eq!( - fixture - .registry - .fast_approval_decision( - &fixture.session_store, - thread_store, - r#"{"tool":"mcp_tool_call","server":"node_repl","tool_name":"js"}"#, - /*extension_metrics*/ None, - ) - .await, + cached_approval( + &fixture.registry, + thread_store, + r#"{"tool":"mcp_tool_call","server":"node_repl","tool_name":"js"}"#, + /*metrics*/ None, + ) + .await, None, "switching back to a reviewed model must not revive a skipped score" ); @@ -909,14 +898,13 @@ async fn sample_configured_conversation_history_with_source( thread_store.insert(parent_model); } assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - /*extension_metrics*/ None, - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + /*metrics*/ None, + ) + .await, None ); registry.thread_lifecycle_contributors()[0] @@ -1123,16 +1111,15 @@ impl GuardianFailureFixture { .await?; thread_store.insert(RecordingMetrics::default()); assert_eq!( - self.registry - .fast_approval_decision( - &self.session_store, - thread_store, - "review action", - thread_store - .get::() - .map(|metrics| metrics as Arc), - ) - .await, + cached_approval( + &self.registry, + thread_store, + "review action", + thread_store + .get::() + .map(|metrics| metrics as Arc), + ) + .await, None ); assert!( @@ -1434,7 +1421,6 @@ max_recent_non_user_entries = 8 let retained_action_bytes = i64::try_from(serde_json::to_string_pretty(&action)?.len())?; assert_eq!(action["tool"], "read_file"); - let session_store = ExtensionData::new("session-1"); let thread_store = test.codex.thread_extension_data(); let score_progress = thread_store .get::() @@ -1451,7 +1437,6 @@ max_recent_non_user_entries = 8 } }) .await?; - assert_eq!(thread_store.get::(), None); thread_store.insert(SecurityRiskScore { scores: BTreeMap::from([("action_risk".to_owned(), 0.65)]), call_id: None, @@ -1459,22 +1444,17 @@ max_recent_non_user_entries = 8 sampled_at: None, }); assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - thread_store - .get::() - .map(|metrics| metrics as Arc), - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + thread_store + .get::() + .map(|metrics| metrics as Arc), + ) + .await, None ); - assert_eq!( - thread_store.remove::().as_deref(), - Some(&StrictReviewReason::ElevatedRisk) - ); thread_store.insert(SecurityRiskScore { scores: BTreeMap::from([("action_risk".to_owned(), 0.55)]), call_id: None, @@ -1482,16 +1462,15 @@ max_recent_non_user_entries = 8 sampled_at: None, }); assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - thread_store - .get::() - .map(|metrics| metrics as Arc), - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + thread_store + .get::() + .map(|metrics| metrics as Arc), + ) + .await, Some(ReviewDecision::Approved) ); @@ -1505,16 +1484,15 @@ max_recent_non_user_entries = 8 .latest_tool_call .store(/*val*/ 3, Ordering::Release); assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - thread_store - .get::() - .map(|metrics| metrics as Arc), - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + thread_store + .get::() + .map(|metrics| metrics as Arc), + ) + .await, Some(ReviewDecision::Approved) ); @@ -1524,37 +1502,31 @@ max_recent_non_user_entries = 8 .latest_tool_call .store(/*val*/ 4, Ordering::Release); assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - thread_store - .get::() - .map(|metrics| metrics as Arc), - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + thread_store + .get::() + .map(|metrics| metrics as Arc), + ) + .await, None ); - assert_eq!( - thread_store.remove::().as_deref(), - Some(&StrictReviewReason::StaleScore) - ); score_progress .latest_scored_tool_call .store(/*val*/ 2, Ordering::Release); assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - thread_store - .get::() - .map(|metrics| metrics as Arc), - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + thread_store + .get::() + .map(|metrics| metrics as Arc), + ) + .await, Some(ReviewDecision::Approved) ); @@ -1822,7 +1794,6 @@ async fn contributor_uses_model_defaults_and_preserves_local_overrides() -> Resu .expect("planned action should be a text item"); assert!(action.len() <= TruncationPolicy::Tokens(/*limit*/ 128).byte_budget()); - let session_store = ExtensionData::new("session-1"); let thread_store = test.codex.thread_extension_data(); let guardian_config = thread_store .get::() @@ -1857,14 +1828,13 @@ async fn contributor_uses_model_defaults_and_preserves_local_overrides() -> Resu sampled_at: None, }); assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - /*extension_metrics*/ None, - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + /*metrics*/ None, + ) + .await, Some(ReviewDecision::Approved) ); @@ -1931,7 +1901,6 @@ async fn assert_luna_pool_context(thread_context_enabled: bool) -> Result<()> { ) .await?; let thread_id = test.session_configured.thread_id; - let session_store = ExtensionData::new("session-1"); let thread_store = test.codex.thread_extension_data(); assert_eq!(request["model"], "gpt-5.6-luna"); let classifier_thread_id = request["client_metadata"]["thread_id"] @@ -2043,14 +2012,13 @@ async fn assert_luna_pool_context(thread_context_enabled: bool) -> Result<()> { "risk scores should not be persisted unless explicitly enabled" ); assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - /*extension_metrics*/ None, - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + /*metrics*/ None, + ) + .await, None ); thread_store.insert(SecurityRiskScore { @@ -2060,14 +2028,13 @@ async fn assert_luna_pool_context(thread_context_enabled: bool) -> Result<()> { sampled_at: None, }); assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - /*extension_metrics*/ None, - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + /*metrics*/ None, + ) + .await, None ); @@ -2078,14 +2045,13 @@ async fn assert_luna_pool_context(thread_context_enabled: bool) -> Result<()> { sampled_at: None, }); assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - /*extension_metrics*/ None, - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + /*metrics*/ None, + ) + .await, Some(ReviewDecision::Approved) ); @@ -2097,14 +2063,13 @@ async fn assert_luna_pool_context(thread_context_enabled: bool) -> Result<()> { sampled_at: None, }); assert_eq!( - registry - .fast_approval_decision( - &session_store, - &disabled_thread_store, - "review action", - /*extension_metrics*/ None - ) - .await, + cached_approval( + ®istry, + &disabled_thread_store, + "review action", + /*metrics*/ None + ) + .await, None ); @@ -2254,14 +2219,13 @@ async fn contributor_skips_required_models_in_standard_scope() -> Result<()> { sampled_at: None, }); assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - r#"{"tool":"mcp_tool_call","server":"node_repl"}"#, - /*extension_metrics*/ None, - ) - .await, + cached_approval( + ®istry, + thread_store, + r#"{"tool":"mcp_tool_call","server":"node_repl"}"#, + /*metrics*/ None, + ) + .await, None ); @@ -2316,7 +2280,6 @@ async fn cached_score_survives_compaction_and_internal_context_but_not_user_inpu /*model_defaults*/ None, ) .await?; - let session_store = ExtensionData::new("session-1"); let thread_store = test.codex.thread_extension_data(); let progress = thread_store.get::().unwrap(); tokio::time::timeout(ASYNC_TEST_TIMEOUT, async { @@ -2346,14 +2309,13 @@ async fn cached_score_survives_compaction_and_internal_context_but_not_user_inpu }) .await; assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - /*extension_metrics*/ None, - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + /*metrics*/ None, + ) + .await, Some(ReviewDecision::Approved), ); @@ -2369,14 +2331,13 @@ async fn cached_score_survives_compaction_and_internal_context_but_not_user_inpu }]) .await?; assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - /*extension_metrics*/ None - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + /*metrics*/ None + ) + .await, None, ); Ok(()) @@ -2408,15 +2369,13 @@ async fn assert_compaction_approval_policy(thread_context_enabled: bool) -> Resu sampled_at: None, }); assert_eq!( - fixture - .registry - .fast_approval_decision( - &fixture.session_store, - thread_store, - "review action", - /*extension_metrics*/ None - ) - .await, + cached_approval( + &fixture.registry, + thread_store, + "review action", + /*metrics*/ None + ) + .await, Some(ReviewDecision::Approved) ); let authorization = fixture.test.codex.guardian_authorization_version().await; @@ -2472,21 +2431,15 @@ async fn assert_compaction_approval_policy(thread_context_enabled: bool) -> Resu .js_executions .store(/*val*/ 1, Ordering::Release); assert_eq!( - fixture - .registry - .fast_approval_decision( - &fixture.session_store, - thread_store, - prompt, - /*extension_metrics*/ None - ) - .await, + cached_approval( + &fixture.registry, + thread_store, + prompt, + /*metrics*/ None + ) + .await, (!thread_context_enabled).then_some(ReviewDecision::Approved) ); - assert_eq!( - thread_store.remove::().as_deref(), - thread_context_enabled.then_some(&StrictReviewReason::IncompatibleCompaction) - ); } Ok(()) } @@ -2525,14 +2478,13 @@ async fn contributor_counts_failed_thread_lookups_toward_score_lag() -> Result<( sampled_at: None, }); assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - /*extension_metrics*/ None, - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + /*metrics*/ None, + ) + .await, Some(ReviewDecision::Approved) ); @@ -2563,14 +2515,13 @@ async fn contributor_counts_failed_thread_lookups_toward_score_lag() -> Result<( assert_eq!(score_progress.latest_tool_call.load(Ordering::Acquire), 2); assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - /*extension_metrics*/ None, - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + /*metrics*/ None, + ) + .await, None ); @@ -3098,14 +3049,13 @@ async fn assert_parent_compaction_reuse(thread_context_enabled: bool) -> Result< // Raw injection did not attach producer provenance to the live checkpoint. // The sample's mock snapshot cannot make that live checkpoint safe for approval. assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - /*extension_metrics*/ None, - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + /*metrics*/ None, + ) + .await, (!thread_context_enabled).then_some(ReviewDecision::Approved), ); @@ -3149,14 +3099,13 @@ async fn assert_parent_compaction_reuse(thread_context_enabled: bool) -> Result< } ); assert_eq!( - registry - .fast_approval_decision( - &session_store, - thread_store, - "review action", - /*extension_metrics*/ None, - ) - .await, + cached_approval( + ®istry, + thread_store, + "review action", + /*metrics*/ None, + ) + .await, None ); assert_eq!( @@ -3289,3 +3238,68 @@ async fn contributor_bounds_oversized_actions_and_fairly_truncates_nested_fields Ok(()) } + +struct CacheMiss; +impl codex_extension_api::SynchronousApprovalReviewer for CacheMiss { + fn review( + &self, + _reason: codex_protocol::approvals::GuardianReviewReason, + ) -> codex_extension_api::ExtensionFuture<'_, ReviewDecision> { + Box::pin(async { ReviewDecision::denied("cache miss") }) + } +} + +/// Exercises decision routing; a fresh review is observed as a cache miss. +async fn cached_approval( + registry: &codex_extension_api::ExtensionRegistry, + store: &ExtensionData, + action: &str, + metrics: Option>, +) -> Option { + let action = serde_json::from_str(action).unwrap_or(serde_json::Value::Null); + let category = match review_scope(&action) { + Some(category) => category, + None if store.get::()?.policy.other_tools + == codex_protocol::openai_models::GuardianReviewMode::Adaptive => + { + codex_protocol::openai_models::GuardianScope::Shell + } + None => { + super::super::metrics::record_fast_decision( + metrics.as_deref(), + "deferred", + "out_of_scope", + ); + return None; + } + }; + let input = codex_extension_api::ApprovalDecisionInput { + approval_id: "cache-probe", + action: &action, + thread_id: codex_protocol::ThreadId::from_string(store.level_id()).unwrap(), + thread_store: store, + category, + approval_policy: codex_protocol::protocol::AskForApproval::OnRequest, + approvals_reviewer: ApprovalsReviewer::AutoReview, + require_guardian: false, + require_fresh_review: false, + full_access: false, + metrics, + synchronous_reviewer: &CacheMiss, + }; + match registry.decide_approval(&input).await { + Some(codex_extension_api::ApprovalDecision::Allow) => Some(ReviewDecision::Approved), + _ => None, + } +} + +fn review_scope(action: &serde_json::Value) -> Option { + match action.get("tool").and_then(serde_json::Value::as_str)? { + "mcp_tool_call" => action + .get("server") + .and_then(serde_json::Value::as_str) + .map(GuardianScope::for_mcp_server), + "network_access" => Some(GuardianScope::Network), + tool => GuardianScope::for_tool(&ToolName::plain(tool)), + } +} diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/mod.rs b/codex-rs/ext/guardian-v2/src/async_scorer/mod.rs index cb4b12d55b..82ce4d2036 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/mod.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/mod.rs @@ -15,7 +15,6 @@ mod trusted_tools; pub(crate) use config::DEFAULT_MODEL_CONTEXT_ITEM_TOKENS; pub(crate) use config::GuardianV2Config; -pub use extension::StrictReviewReason; pub(crate) use extension::install; pub(crate) use transcript::MAX_TOOL_ENTRY_TOKENS; pub(crate) use transcript::RenderedContext; diff --git a/codex-rs/ext/guardian-v2/src/lib.rs b/codex-rs/ext/guardian-v2/src/lib.rs index 66d661844a..4563431497 100644 --- a/codex-rs/ext/guardian-v2/src/lib.rs +++ b/codex-rs/ext/guardian-v2/src/lib.rs @@ -15,7 +15,6 @@ use codex_protocol::ThreadId; mod async_scorer; mod sync_reviewer; -pub use async_scorer::StrictReviewReason; pub use sync_reviewer::GuardianExtension as GuardianReviewerExtension; pub use sync_reviewer::GuardianThreadContext as GuardianReviewerThreadContext;