diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 7ff5719390..29c5c08e08 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -347,7 +347,7 @@ async fn run_guardian_review( && let Some(decision) = session .services .extensions - .approval_review( + .fast_approval_decision( &session.services.session_extension_data, &session.services.thread_extension_data, &action.to_string(), diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index af5964ad88..c699f0f187 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -3095,7 +3095,7 @@ async fn escalated_retry_bypasses_extension_approval_and_runs_guardian() -> anyh struct AutoApprovingReviewContributor; impl codex_extension_api::ApprovalReviewContributor for AutoApprovingReviewContributor { - fn contribute<'a>( + fn fast_decision<'a>( &'a self, _session_store: &'a codex_extension_api::ExtensionData, _thread_store: &'a codex_extension_api::ExtensionData, diff --git a/codex-rs/core/tests/suite/auto_review.rs b/codex-rs/core/tests/suite/auto_review.rs index 245473083b..ff454ac429 100644 --- a/codex-rs/core/tests/suite/auto_review.rs +++ b/codex-rs/core/tests/suite/auto_review.rs @@ -65,7 +65,7 @@ use wiremock::matchers::path; struct ApprovedReviewContributor; impl ApprovalReviewContributor for ApprovedReviewContributor { - fn contribute<'a>( + fn fast_decision<'a>( &'a self, _session_store: &'a ExtensionData, _thread_store: &'a ExtensionData, @@ -83,7 +83,7 @@ impl ApprovalReviewContributor for ApprovedReviewContributor { struct EscalationApprovingReviewContributor; impl ApprovalReviewContributor for EscalationApprovingReviewContributor { - fn contribute<'a>( + fn fast_decision<'a>( &'a self, _session_store: &'a ExtensionData, _thread_store: &'a ExtensionData, diff --git a/codex-rs/core/tests/suite/guardian_review.rs b/codex-rs/core/tests/suite/guardian_review.rs index 344987c814..c2429b25f1 100644 --- a/codex-rs/core/tests/suite/guardian_review.rs +++ b/codex-rs/core/tests/suite/guardian_review.rs @@ -1161,7 +1161,7 @@ async fn guardian_timeout_rejects_tool_call_with_acting_model_instructions( struct TimedOutReviewContributor; impl codex_extension_api::ApprovalReviewContributor for TimedOutReviewContributor { - fn contribute<'a>( + fn fast_decision<'a>( &'a self, _session_store: &'a codex_extension_api::ExtensionData, _thread_store: &'a codex_extension_api::ExtensionData, diff --git a/codex-rs/ext/extension-api/src/contributors.rs b/codex-rs/ext/extension-api/src/contributors.rs index 5dfc128de3..a0ab68f560 100644 --- a/codex-rs/ext/extension-api/src/contributors.rs +++ b/codex-rs/ext/extension-api/src/contributors.rs @@ -12,6 +12,7 @@ use codex_tools::ToolExecutor; use crate::ExtensionData; use crate::ExtensionMetrics; +mod approval_review; mod context; mod mcp; mod prompt; @@ -22,6 +23,9 @@ mod turn_input; mod turn_lifecycle; mod world_state; +pub use approval_review::ApprovalAssessment; +pub use approval_review::ApprovalReviewError; +pub use approval_review::ApprovalReviewInput; pub use context::TurnContextContributionInput; pub use mcp::McpServerContribution; pub use mcp::McpServerContributionContext; @@ -326,16 +330,30 @@ pub trait ToolLifecycleContributor: Send + Sync { } } -/// Extension contribution that can claim rendered approval-review prompts. +/// 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. pub trait ApprovalReviewContributor: Send + Sync { - /// Reviews one action using metrics bound to the active turn's model. - fn contribute<'a>( + /// 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>; + _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 new file mode 100644 index 0000000000..5669ada823 --- /dev/null +++ b/codex-rs/ext/extension-api/src/contributors/approval_review.rs @@ -0,0 +1,65 @@ +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; +use crate::ExtensionData; + +/// 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 { + /// The reviewer could not produce a valid assessment. + Failed(String), + /// The request exceeded its host-owned review deadline. + TimedOut, + /// The request was cancelled by its parent approval. + Cancelled, +} + +impl fmt::Display for ApprovalReviewError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Failed(message) => write!(formatter, "approval review failed: {message}"), + Self::TimedOut => formatter.write_str("approval review timed out"), + Self::Cancelled => formatter.write_str("approval review was cancelled"), + } + } +} + +impl std::error::Error for ApprovalReviewError {} + +/// Immutable context for reviewing an existing host-owned approval action. +pub struct ApprovalReviewInput<'a> { + /// Structured action evidence rendered by the host's existing approval path. + pub action: &'a Value, + /// Canonical immutable conversation snapshot captured before review. + pub conversation_history: Arc, + /// Stable host-owned thread identifier. + pub thread_id: ThreadId, + /// Store scoped to this thread runtime. + pub thread_store: &'a ExtensionData, + /// Stable host-owned turn identifier. + pub turn_id: &'a str, + /// Policy reason that caused the host to request approval. + pub approval_reason: Option<&'a str>, + /// Reason an earlier request is being retried, when applicable. + pub retry_reason: Option<&'a str>, +} diff --git a/codex-rs/ext/extension-api/src/lib.rs b/codex-rs/ext/extension-api/src/lib.rs index 04d6caf50e..a166fdd31d 100644 --- a/codex-rs/ext/extension-api/src/lib.rs +++ b/codex-rs/ext/extension-api/src/lib.rs @@ -39,7 +39,10 @@ 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::ApprovalReviewContributor; +pub use contributors::ApprovalReviewError; +pub use contributors::ApprovalReviewInput; pub use contributors::ConfigContributor; pub use contributors::ContextContributor; pub use contributors::ExtensionFuture; diff --git a/codex-rs/ext/extension-api/src/registry.rs b/codex-rs/ext/extension-api/src/registry.rs index be4670cbba..eec5b9f0a7 100644 --- a/codex-rs/ext/extension-api/src/registry.rs +++ b/codex-rs/ext/extension-api/src/registry.rs @@ -2,7 +2,10 @@ 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; @@ -188,9 +191,22 @@ impl ExtensionRegistry { &self.skill_invocation_contributors } - /// Claims the first rendered approval-review prompt accepted by an - /// installed contributor. - pub async fn approval_review( + /// 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, @@ -199,7 +215,7 @@ impl ExtensionRegistry { ) -> Option { for contributor in &self.approval_review_contributors { if let Some(decision) = contributor - .contribute( + .fast_decision( session_store, thread_store, prompt, diff --git a/codex-rs/ext/extension-api/tests/registry.rs b/codex-rs/ext/extension-api/tests/registry.rs index 05fa6f0de2..58469370d7 100644 --- a/codex-rs/ext/extension-api/tests/registry.rs +++ b/codex-rs/ext/extension-api/tests/registry.rs @@ -3,11 +3,15 @@ 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; use codex_extension_api::ContextualUserFragment; +use codex_extension_api::ConversationHistorySnapshot; use codex_extension_api::ExtensionData; use codex_extension_api::ExtensionDataInit; use codex_extension_api::ExtensionEventSink; @@ -17,6 +21,7 @@ use codex_extension_api::ExtensionRegistryBuilder; use codex_extension_api::ExtensionWarning; use codex_extension_api::McpServerContributionContext; use codex_extension_api::PromptFragment; +use codex_extension_api::ResponseItem; use codex_extension_api::SkillInvocationContributor; use codex_extension_api::ThreadLifecycleContributor; use codex_extension_api::TokenUsageContributor; @@ -30,6 +35,10 @@ 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; @@ -39,6 +48,7 @@ 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; @@ -130,7 +140,7 @@ impl TurnItemContributor for AllContributors { } impl ApprovalReviewContributor for AllContributors { - fn contribute<'a>( + fn fast_decision<'a>( &'a self, _session_store: &'a ExtensionData, _thread_store: &'a ExtensionData, @@ -142,6 +152,22 @@ impl ApprovalReviewContributor for AllContributors { Some(ReviewDecision::ApprovedForSession) }) } + + 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(), + })))) + } } #[tokio::test] @@ -173,7 +199,7 @@ async fn build_round_trips_every_contributor_category() { assert_eq!(registry.turn_item_contributors().len(), 1); assert_eq!( registry - .approval_review( + .fast_approval_decision( &ExtensionData::new("session"), &ExtensionData::new("thread"), "review this", @@ -184,6 +210,96 @@ async fn build_round_trips_every_contributor_category() { ); } +impl ConversationHistorySnapshot for AllContributors { + fn history_version(&self) -> u64 { + 0 + } + + fn items(&self) -> Box + Send + '_> { + Box::new(std::iter::empty()) + } +} + +#[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 { @@ -334,7 +450,7 @@ struct RecordingApprovalContributor { } impl ApprovalReviewContributor for RecordingApprovalContributor { - fn contribute<'a>( + fn fast_decision<'a>( &'a self, session_store: &'a ExtensionData, thread_store: &'a ExtensionData, @@ -357,7 +473,7 @@ impl ApprovalReviewContributor for RecordingApprovalContributor { } #[tokio::test] -async fn approval_review_returns_first_claim_and_short_circuits() { +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 [ @@ -377,7 +493,7 @@ async fn approval_review_returns_first_claim_and_short_circuits() { let registry = builder.build(); let decision = registry - .approval_review( + .fast_approval_decision( &ExtensionData::new("session-1"), &ExtensionData::new("thread-1"), "allow command?", @@ -461,12 +577,12 @@ fn custom_event_sink_survives_registry_build() { } #[tokio::test] -async fn empty_registry_does_not_claim_approval_review() { +async fn empty_registry_does_not_claim_fast_approval_decision() { let registry = empty_extension_registry::<()>(); assert_eq!( registry - .approval_review( + .fast_approval_decision( &ExtensionData::new("session"), &ExtensionData::new("thread"), "unclaimed", 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 7585e9a26a..f7ac274988 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -363,7 +363,7 @@ impl ThreadLifecycleContributor for GuardianV2Extension { } impl ApprovalReviewContributor for GuardianV2Extension { - fn contribute<'a>( + fn fast_decision<'a>( &'a self, _session_store: &'a ExtensionData, thread_store: &'a ExtensionData, 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 8f08e4721b..e60b888f0a 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 @@ -170,7 +170,7 @@ async fn installed_extension_reconnects_after_auth_refresh() -> Result<()> { .await?; assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -622,7 +622,7 @@ async fn sample_configured_conversation_history( } assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -751,7 +751,7 @@ impl GuardianFailureFixture { .await?; assert_eq!( self.registry - .approval_review( + .fast_approval_decision( &self.session_store, thread_store, "review action", @@ -1071,7 +1071,7 @@ max_recent_non_user_entries = 8 }); assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -1092,7 +1092,7 @@ max_recent_non_user_entries = 8 }); assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -1115,7 +1115,7 @@ max_recent_non_user_entries = 8 .store(/*val*/ 3, Ordering::Release); assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -1134,7 +1134,7 @@ max_recent_non_user_entries = 8 .store(/*val*/ 4, Ordering::Release); assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -1155,7 +1155,7 @@ max_recent_non_user_entries = 8 .store(/*val*/ 2, Ordering::Release); assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -1447,7 +1447,7 @@ async fn contributor_uses_model_defaults_and_preserves_local_overrides() -> Resu }); assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -1603,7 +1603,7 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result< assert!(score.sampled_at.is_some()); assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -1618,7 +1618,7 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result< }); assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -1634,7 +1634,7 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result< }); assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -1651,7 +1651,7 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result< }); assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, &disabled_thread_store, "review action", @@ -1738,7 +1738,7 @@ async fn contributor_skips_models_requiring_managed_guardian_review() -> Result< }); assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -1818,7 +1818,7 @@ async fn contributor_counts_failed_thread_lookups_toward_score_lag() -> Result<( }); assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -1854,7 +1854,7 @@ 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 - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -2197,7 +2197,7 @@ async fn contributor_reuses_the_latest_compatible_parent_compaction() -> Result< assert_eq!(previous_score.scores.get("action_risk"), Some(&0.0)); assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action", @@ -2241,7 +2241,7 @@ async fn contributor_reuses_the_latest_compatible_parent_compaction() -> Result< ); assert_eq!( registry - .approval_review( + .fast_approval_decision( &session_store, thread_store, "review action",