diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index db66301230..385fe89ceb 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -42,6 +42,7 @@ use super::GuardianAssessmentOutcome; use super::GuardianRejectionCircuitBreakerAction; use super::GuardianRejectionCircuitBreakerPolicy; use super::GuardianReviewContext; +use super::approval_request::guardian_approval_request_to_json; use super::approval_request::guardian_assessment_action; use super::approval_request::guardian_request_target_item_id; use super::approval_request::guardian_request_turn_id; @@ -299,9 +300,9 @@ pub(crate) async fn record_guardian_denial_for_test( record_guardian_denial(session, turn, turn_id).await; } -/// This function always fails closed: timeouts, review-session failures, and -/// parse failures all block execution, but timeouts are still surfaced to the -/// caller as distinct from explicit guardian denials. +/// Runs Guardian unless an installed extension explicitly claims the review. +/// Guardian timeouts, review-session failures, and parse failures all block +/// execution, with timeouts surfaced separately from explicit denials. async fn run_guardian_review( session: Arc, context: GuardianReviewContext, @@ -311,6 +312,33 @@ async fn run_guardian_review( options: GuardianReviewOptions, ) -> ReviewDecision { let turn = Arc::clone(context.turn()); + if !turn + .config + .config_layer_stack + .requirements() + .auto_review_required_for_model(&turn.model_info.slug) + && options + .external_cancel + .as_ref() + .is_none_or(|cancel| !cancel.is_cancelled()) + && let Ok(action) = guardian_approval_request_to_json(&request) + && let Some(decision) = session + .services + .extensions + .approval_review( + &session.services.session_extension_data, + &session.services.thread_extension_data, + &action.to_string(), + ) + .await + { + if decision == ReviewDecision::Approved { + record_guardian_non_denial(&session, guardian_request_turn_id(&request, &turn.sub_id)) + .await; + } + return decision; + } + let GuardianReviewOptions { plugin_attribution_override, approval_request_source, diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 2e93782c3a..73decd4504 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -1306,7 +1306,7 @@ async fn maybe_request_mcp_tool_approval( let strict_auto_review = match turn_state { Some(turn_state) => turn_state.lock().await.strict_auto_review_enabled(), None => false, - } || sess.extension_strict_auto_review_enabled().await; + }; let approvals_reviewer = connectors::mcp_approvals_reviewer_from_layers( &config.config_layer_stack, config.approvals_reviewer, diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index f47dc8a510..1690591239 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -833,9 +833,7 @@ async fn review_guardian_mcp_elicitation( request.server_name.as_str(), elicitation_connector_id(&request.elicitation), ); - if !crate::guardian::routes_approval_policy_to_guardian(approval_policy, approvals_reviewer) - && !session.extension_strict_auto_review_enabled().await - { + if !crate::guardian::routes_approval_policy_to_guardian(approval_policy, approvals_reviewer) { return Ok(None); } diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 68feceedf1..fb12a66355 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2512,9 +2512,7 @@ impl Session { }); }; - if crate::guardian::routes_approval_to_guardian(turn_context.as_ref()) - || self.extension_strict_auto_review_enabled().await - { + if crate::guardian::routes_approval_to_guardian(turn_context.as_ref()) { let originating_turn_state = { let active = self.active_turn.lock().await; active.as_ref().map(|active| Arc::clone(&active.turn_state)) @@ -2853,41 +2851,11 @@ impl Session { pub(crate) async fn active_turn_context_and_strict_auto_review( &self, ) -> Option<(Arc, bool)> { - let extension_strict_auto_review = self.extension_strict_auto_review_enabled().await; let active = self.active_turn.lock().await; let active = active.as_ref()?; let turn_context = Arc::clone(&active.task.as_ref()?.turn_context); let ts = active.turn_state.lock().await; - Some(( - turn_context, - ts.strict_auto_review_enabled() || extension_strict_auto_review, - )) - } - - pub(crate) async fn extension_strict_auto_review_enabled(&self) -> bool { - if self - .services - .extensions - .approval_requirement(&self.services.thread_extension_data) - != codex_extension_api::ApprovalRequirement::RequireAutomaticReview - { - return false; - } - - let state = self.state.lock().await; - let configuration = &state.session_configuration; - !configuration.trusted_guardian_reviewer - && configuration - .original_config_do_not_use - .features - .enabled(Feature::GuardianApproval) - && configuration - .original_config_do_not_use - .config_layer_stack - .requirements() - .approvals_reviewer - .can_set(&ApprovalsReviewer::AutoReview) - .is_ok() + Some((turn_context, ts.strict_auto_review_enabled())) } pub(crate) async fn granted_session_permissions( diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 83383ea259..21ca88c384 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -282,8 +282,7 @@ impl<'a> ToolRuntime for UnifiedExecRunt let mut launch = network.remote_launch_config().await.map_err(|err| { ToolError::Codex(CodexErr::Io(io::Error::other(err.to_string()))) })?; - if (routes_approval_to_guardian(&ctx.step_context.turn) - || ctx.session.extension_strict_auto_review_enabled().await) + if routes_approval_to_guardian(&ctx.step_context.turn) && network.remote_policy_decider().is_some() { let timeout = ctx diff --git a/codex-rs/core/tests/suite/auto_review.rs b/codex-rs/core/tests/suite/auto_review.rs index b41668935c..d83561155e 100644 --- a/codex-rs/core/tests/suite/auto_review.rs +++ b/codex-rs/core/tests/suite/auto_review.rs @@ -3,9 +3,9 @@ use std::sync::Arc; use std::time::Duration; use anyhow::Result; -use codex_extension_api::ApprovalPolicyContributor; -use codex_extension_api::ApprovalRequirement; +use codex_extension_api::ApprovalReviewContributor; use codex_extension_api::ExtensionData; +use codex_extension_api::ExtensionFuture; use codex_extension_api::ExtensionRegistryBuilder; use codex_features::Feature; use codex_login::CodexAuth; @@ -25,6 +25,7 @@ use codex_protocol::openai_models::default_input_modalities; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; +use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::ThreadSettingsOverrides; use codex_protocol::request_permissions::PermissionGrantScope; use codex_protocol::request_permissions::RequestPermissionsResponse; @@ -39,6 +40,7 @@ use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; use core_test_support::skip_if_no_network; use core_test_support::skip_if_sandbox; +use core_test_support::skip_if_wine_exec; use core_test_support::test_codex::TestCodex; use core_test_support::test_codex::local_selections; use core_test_support::test_codex::test_codex; @@ -54,22 +56,104 @@ use wiremock::ResponseTemplate; use wiremock::matchers::method; use wiremock::matchers::path; -struct AutomaticReviewRequested; +struct ApprovedReviewContributor; -struct ThreadApprovalPolicyContributor; - -impl ApprovalPolicyContributor for ThreadApprovalPolicyContributor { - fn approval_requirement(&self, thread_store: &ExtensionData) -> ApprovalRequirement { - if thread_store.get::().is_some() { - ApprovalRequirement::RequireAutomaticReview - } else { - ApprovalRequirement::Default - } +impl ApprovalReviewContributor for ApprovedReviewContributor { + fn contribute<'a>( + &'a self, + _session_store: &'a ExtensionData, + _thread_store: &'a ExtensionData, + prompt: &'a str, + ) -> ExtensionFuture<'a, Option> { + Box::pin(async move { + assert!(prompt.contains("\"tool\":\"request_permissions\"")); + Some(ReviewDecision::Approved) + }) } } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn remote_model_override_uses_catalog_model_for_extension_strict_auto_review() -> Result<()> { +async fn approval_review_contributor_skips_existing_guardian_model_call() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_wine_exec!(Ok(()), "request_permissions requires host-native paths"); + + let server = MockServer::start().await; + let permissions_call_id = "extension-approved-permissions"; + let permissions_args = json!({ + "reason": "grant low-risk network access", + "permissions": { + "network": { + "enabled": true, + }, + }, + }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-parent-1"), + ev_function_call( + permissions_call_id, + "request_permissions", + &serde_json::to_string(&permissions_args)?, + ), + ev_completed("resp-parent-1"), + ]), + sse(vec![ + ev_response_created("resp-parent-2"), + ev_assistant_message("msg-parent", "done"), + ev_completed("resp-parent-2"), + ]), + ], + ) + .await; + + let mut extensions = ExtensionRegistryBuilder::new(); + extensions.approval_review_contributor(Arc::new(ApprovedReviewContributor)); + let mut builder = test_codex() + .with_extensions(Arc::new(extensions.build())) + .with_config(|config| { + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + config + .features + .enable(Feature::RequestPermissionsTool) + .expect("test config should allow feature update"); + }); + let test = builder.build_with_auto_env(&server).await?; + test.codex + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { + text: "request low-risk network access".into(), + text_elements: Vec::new(), + }])) + .await?; + + loop { + match wait_for_event(&test.codex, |_| true).await { + EventMsg::GuardianAssessment(event) => { + panic!("approved extension review should not start Guardian: {event:?}") + } + EventMsg::RequestPermissions(event) => { + panic!("approved extension review should not prompt the user: {event:?}") + } + EventMsg::TurnComplete(_) => break, + _ => {} + } + } + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + assert!( + requests[1] + .function_call_output(permissions_call_id) + .to_string() + .contains("enabled") + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn remote_model_override_uses_catalog_model_for_strict_auto_review() -> Result<()> { skip_if_no_network!(Ok(())); skip_if_sandbox!(Ok(())); @@ -136,10 +220,7 @@ async fn remote_model_override_uses_catalog_model_for_extension_strict_auto_revi ) .await; - let mut extensions = ExtensionRegistryBuilder::new(); - extensions.approval_policy_contributor(Arc::new(ThreadApprovalPolicyContributor)); let mut builder = test_codex() - .with_extensions(Arc::new(extensions.build())) .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) .with_config(|config| { config.model = Some("gpt-5.4".to_string()); @@ -226,16 +307,13 @@ async fn remote_model_override_uses_catalog_model_for_extension_strict_auto_revi panic!("expected request_permissions before completion"); }; assert_eq!(permissions_request.call_id, permissions_call_id); - codex - .thread_extension_data() - .insert(AutomaticReviewRequested); codex .submit(Op::RequestPermissionsResponse { id: permissions_request.call_id, response: RequestPermissionsResponse { permissions: permissions_request.permissions, scope: PermissionGrantScope::Turn, - strict_auto_review: false, + strict_auto_review: true, }, }) .await?; diff --git a/codex-rs/ext/extension-api/src/contributors.rs b/codex-rs/ext/extension-api/src/contributors.rs index 5b779b7bbb..ac3fcb1a2f 100644 --- a/codex-rs/ext/extension-api/src/contributors.rs +++ b/codex-rs/ext/extension-api/src/contributors.rs @@ -316,26 +316,6 @@ pub trait ToolLifecycleContributor: Send + Sync { } } -/// Approval behavior requested by an extension for the current thread. -#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] -pub enum ApprovalRequirement { - /// Preserve the host's configured approval behavior. - #[default] - Default, - /// Ask the host to require automatic review before an action executes. - RequireAutomaticReview, -} - -/// Contributes thread-scoped approval requirements from extension-owned state. -/// -/// Implementations should keep this callback cheap and must not perform I/O. -/// The host remains responsible for enforcing managed policy and selecting the -/// trusted reviewer that satisfies the requested requirement. -pub trait ApprovalPolicyContributor: Send + Sync { - /// Returns the current approval requirement for the supplied thread. - fn approval_requirement(&self, thread_store: &ExtensionData) -> ApprovalRequirement; -} - /// Extension contribution that can claim rendered approval-review prompts. pub trait ApprovalReviewContributor: Send + Sync { fn contribute<'a>( diff --git a/codex-rs/ext/extension-api/src/lib.rs b/codex-rs/ext/extension-api/src/lib.rs index 77adecfd54..69b4903af2 100644 --- a/codex-rs/ext/extension-api/src/lib.rs +++ b/codex-rs/ext/extension-api/src/lib.rs @@ -35,8 +35,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::ApprovalPolicyContributor; -pub use contributors::ApprovalRequirement; pub use contributors::ApprovalReviewContributor; pub use contributors::ConfigContributor; pub use contributors::ContextContributor; diff --git a/codex-rs/ext/extension-api/src/registry.rs b/codex-rs/ext/extension-api/src/registry.rs index 671d05b058..f34c88cc3b 100644 --- a/codex-rs/ext/extension-api/src/registry.rs +++ b/codex-rs/ext/extension-api/src/registry.rs @@ -2,8 +2,6 @@ use std::sync::Arc; use codex_protocol::protocol::ReviewDecision; -use crate::ApprovalPolicyContributor; -use crate::ApprovalRequirement; use crate::ApprovalReviewContributor; use crate::ConfigContributor; use crate::ContextContributor; @@ -35,7 +33,6 @@ impl Default for ExtensionRegistryBuilder { config_contributors: Vec::new(), token_usage_contributors: Vec::new(), skill_invocation_contributors: Vec::new(), - approval_policy_contributors: Vec::new(), approval_review_contributors: Vec::new(), context_contributors: Vec::new(), mcp_server_contributors: Vec::new(), @@ -66,11 +63,6 @@ impl ExtensionRegistryBuilder { Arc::clone(&self.registry.event_sink) } - /// Registers one thread-scoped approval-policy contributor. - pub fn approval_policy_contributor(&mut self, contributor: Arc) { - self.registry.approval_policy_contributors.push(contributor); - } - /// Registers one approval-review contributor. pub fn approval_review_contributor(&mut self, contributor: Arc) { self.registry.approval_review_contributors.push(contributor); @@ -161,7 +153,6 @@ pub struct ExtensionRegistry { tool_contributors: Vec>, tool_lifecycle_contributors: Vec>, turn_item_contributors: Vec>, - approval_policy_contributors: Vec>, approval_review_contributors: Vec>, } @@ -196,18 +187,6 @@ impl ExtensionRegistry { &self.skill_invocation_contributors } - /// Resolves the strongest approval requirement for the current thread. - pub fn approval_requirement(&self, thread_store: &ExtensionData) -> ApprovalRequirement { - if self.approval_policy_contributors.iter().any(|contributor| { - contributor.approval_requirement(thread_store) - == ApprovalRequirement::RequireAutomaticReview - }) { - ApprovalRequirement::RequireAutomaticReview - } else { - ApprovalRequirement::Default - } - } - /// Claims the first rendered approval-review prompt accepted by an /// installed contributor. pub async fn approval_review( diff --git a/codex-rs/ext/extension-api/tests/registry.rs b/codex-rs/ext/extension-api/tests/registry.rs index 1b7cb899df..e5579e5453 100644 --- a/codex-rs/ext/extension-api/tests/registry.rs +++ b/codex-rs/ext/extension-api/tests/registry.rs @@ -3,8 +3,6 @@ use std::sync::Arc; use std::sync::Mutex; -use codex_extension_api::ApprovalPolicyContributor; -use codex_extension_api::ApprovalRequirement; use codex_extension_api::ApprovalReviewContributor; use codex_extension_api::ConfigContributor; use codex_extension_api::ContextContributor; @@ -117,12 +115,6 @@ impl ToolContributor for AllContributors { impl ToolLifecycleContributor for AllContributors {} -impl ApprovalPolicyContributor for AllContributors { - fn approval_requirement(&self, _thread_store: &ExtensionData) -> ApprovalRequirement { - ApprovalRequirement::RequireAutomaticReview - } -} - impl TurnItemContributor for AllContributors { fn contribute<'a>( &'a self, @@ -165,7 +157,6 @@ async fn build_round_trips_every_contributor_category() { builder.tool_contributor(contributor.clone()); builder.tool_lifecycle_contributor(contributor.clone()); builder.turn_item_contributor(contributor.clone()); - builder.approval_policy_contributor(contributor.clone()); builder.approval_review_contributor(contributor); let registry = builder.build(); @@ -179,10 +170,6 @@ 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); - assert_eq!( - registry.approval_requirement(&ExtensionData::new("thread")), - ApprovalRequirement::RequireAutomaticReview - ); assert_eq!( registry .approval_review( diff --git a/codex-rs/ext/guardian-v2/src/extension.rs b/codex-rs/ext/guardian-v2/src/extension.rs index b656111b4c..a9b6696988 100644 --- a/codex-rs/ext/guardian-v2/src/extension.rs +++ b/codex-rs/ext/guardian-v2/src/extension.rs @@ -4,8 +4,7 @@ use std::time::SystemTime; use codex_core::ThreadManager; use codex_core::config::Config; -use codex_extension_api::ApprovalPolicyContributor; -use codex_extension_api::ApprovalRequirement; +use codex_extension_api::ApprovalReviewContributor; use codex_extension_api::ExtensionData; use codex_extension_api::ExtensionEventSink; use codex_extension_api::ExtensionFuture; @@ -28,6 +27,7 @@ use codex_model_provider::create_model_provider; use codex_protocol::ThreadId; use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ReasoningEffort; +use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::TruncationPolicy; use codex_protocol::security_risk::SecurityRiskScore; use serde_json::json; @@ -198,7 +198,9 @@ impl ThreadLifecycleContributor for GuardianV2Extension { input: ThreadStartInput<'a, Config>, ) -> ExtensionFuture<'a, ()> { Box::pin(async move { - if !input.config.features.enabled(Feature::GuardianV2) { + if !input.config.features.enabled(Feature::GuardianV2) + || !input.config.features.enabled(Feature::GuardianApproval) + { return; } @@ -250,23 +252,22 @@ impl ThreadLifecycleContributor for GuardianV2Extension { } } -impl ApprovalPolicyContributor for GuardianV2Extension { - fn approval_requirement(&self, thread_store: &ExtensionData) -> ApprovalRequirement { - if thread_store.get::().is_none() { - return ApprovalRequirement::Default; - } +impl ApprovalReviewContributor for GuardianV2Extension { + fn contribute<'a>( + &'a self, + _session_store: &'a ExtensionData, + thread_store: &'a ExtensionData, + _prompt: &'a str, + ) -> ExtensionFuture<'a, Option> { + Box::pin(async move { + thread_store.get::()?; - match thread_store.get::() { - Some(score) - if score - .scores - .get("action_risk") - .is_some_and(|score| *score >= ACTION_RISK_REVIEW_THRESHOLD) => - { - ApprovalRequirement::RequireAutomaticReview - } - _ => ApprovalRequirement::Default, - } + thread_store + .get::() + .and_then(|score| score.scores.get("action_risk").copied()) + .filter(|score| *score < ACTION_RISK_REVIEW_THRESHOLD) + .map(|_| ReviewDecision::Approved) + }) } } @@ -442,7 +443,7 @@ pub fn install( thread_manager, }); registry.thread_lifecycle_contributor(extension.clone()); - registry.approval_policy_contributor(extension.clone()); + registry.approval_review_contributor(extension.clone()); registry.tool_lifecycle_contributor(extension); } diff --git a/codex-rs/ext/guardian-v2/src/extension_tests.rs b/codex-rs/ext/guardian-v2/src/extension_tests.rs index 3a64ff0f07..59009373e9 100644 --- a/codex-rs/ext/guardian-v2/src/extension_tests.rs +++ b/codex-rs/ext/guardian-v2/src/extension_tests.rs @@ -4,7 +4,6 @@ use std::time::Duration; use anyhow::Result; use codex_core::config::Config; -use codex_extension_api::ApprovalRequirement; use codex_extension_api::ConversationHistorySnapshot; use codex_extension_api::ExtensionData; use codex_extension_api::ExtensionRegistry; @@ -24,6 +23,7 @@ use codex_protocol::ResponseItemId; use codex_protocol::models::ContentItem; use codex_protocol::models::FunctionCallOutputPayload; use codex_protocol::models::ReasoningItemReasoningSummary; +use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::TruncationPolicy; use codex_protocol::security_risk::SecurityRiskScore; @@ -143,8 +143,10 @@ async fn sample_conversation_history( let session_store = ExtensionData::new("session-1"); let thread_store = test.codex.thread_extension_data(); assert_eq!( - registry.approval_requirement(thread_store), - ApprovalRequirement::Default + registry + .approval_review(&session_store, thread_store, "review action") + .await, + None ); registry.thread_lifecycle_contributors()[0] .on_thread_start(ThreadStartInput { @@ -238,6 +240,7 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result< let (request, test, registry) = sample_conversation_history(conversation_history, r#"{"path":"README.md"}"#).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"); assert_eq!( @@ -292,8 +295,10 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result< ); assert!(score.sampled_at.is_some()); assert_eq!( - registry.approval_requirement(thread_store), - ApprovalRequirement::RequireAutomaticReview + registry + .approval_review(&session_store, thread_store, "review action") + .await, + None ); test.codex.ensure_rollout_materialized().await; test.codex.flush_rollout().await?; @@ -315,18 +320,22 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result< sampled_at: None, }); assert_eq!( - registry.approval_requirement(thread_store), - ApprovalRequirement::Default + registry + .approval_review(&session_store, thread_store, "review action") + .await, + Some(ReviewDecision::Approved) ); let disabled_thread_store = ExtensionData::new("disabled-thread"); disabled_thread_store.insert(SecurityRiskScore { - scores: BTreeMap::from([("action_risk".to_string(), 0.8)]), + scores: BTreeMap::from([("action_risk".to_string(), 0.25)]), sampled_at: None, }); assert_eq!( - registry.approval_requirement(&disabled_thread_store), - ApprovalRequirement::Default + registry + .approval_review(&session_store, &disabled_thread_store, "review action") + .await, + None ); Ok(())