diff --git a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst index d93bfefb7d..10d00f3a35 100644 Binary files a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst and b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst differ diff --git a/codex-rs/app-server-protocol/src/protocol/item_builders_tests.rs b/codex-rs/app-server-protocol/src/protocol/item_builders_tests.rs index 47cc227110..7bcc341c02 100644 --- a/codex-rs/app-server-protocol/src/protocol/item_builders_tests.rs +++ b/codex-rs/app-server-protocol/src/protocol/item_builders_tests.rs @@ -129,6 +129,7 @@ fn guardian_stdin_reviews_preserve_parent_command_history() { assert!(expected[1].items.is_empty()); let assessment = GuardianAssessmentEvent { + review_reason: None, id: "review-stdin".into(), target_item_id: Some("terminal-command".into()), plugin_id: None, @@ -159,6 +160,7 @@ fn guardian_stdin_reviews_preserve_parent_command_history() { ] { items.push(RolloutItem::EventMsg(EventMsg::GuardianAssessment( GuardianAssessmentEvent { + review_reason: None, turn_id: turn_id.into(), status, ..assessment.clone() diff --git a/codex-rs/app-server-protocol/src/protocol/thread_history.rs b/codex-rs/app-server-protocol/src/protocol/thread_history.rs index 965a30d2d0..56c90427e4 100644 --- a/codex-rs/app-server-protocol/src/protocol/thread_history.rs +++ b/codex-rs/app-server-protocol/src/protocol/thread_history.rs @@ -3339,6 +3339,7 @@ mod tests { ..Default::default() }), EventMsg::GuardianAssessment(GuardianAssessmentEvent { + review_reason: None, id: "review-guardian-exec".into(), target_item_id: Some("guardian-exec".into()), plugin_id: Some("sample@openai-curated".into()), @@ -3360,6 +3361,7 @@ mod tests { .expect("guardian action"), }), EventMsg::GuardianAssessment(GuardianAssessmentEvent { + review_reason: None, id: "review-guardian-exec".into(), target_item_id: Some("guardian-exec".into()), plugin_id: Some("sample@openai-curated".into()), @@ -3431,6 +3433,7 @@ mod tests { ..Default::default() }), EventMsg::GuardianAssessment(GuardianAssessmentEvent { + review_reason: None, id: "review-guardian-execve".into(), target_item_id: Some("guardian-execve".into()), plugin_id: Some("sample@openai-curated".into()), diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 7c86ecb84e..95ea7e6548 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -355,10 +355,21 @@ pub(crate) async fn apply_bespoke_event_handling( ); outgoing.send_server_notification(notification).await; if assessment.status == codex_protocol::protocol::GuardianAssessmentStatus::InProgress - && conversation - .thread_extension_data() - .remove::() - .is_some() + && match assessment.review_reason { + Some(reason) => matches!( + reason, + codex_protocol::approvals::GuardianReviewReason::ElevatedRisk + | codex_protocol::approvals::GuardianReviewReason::StaleScore + | codex_protocol::approvals::GuardianReviewReason::IncompatibleCompaction + | codex_protocol::approvals::GuardianReviewReason::ScoringFailure + | codex_protocol::approvals::GuardianReviewReason::AuthorizationChanged + ), + // MCP elicitations migrate to per-approval reasons next. + None => conversation + .thread_extension_data() + .remove::() + .is_some(), + } { outgoing .send_server_notification(ServerNotification::StrictReviewRequired( @@ -2429,6 +2440,7 @@ mod tests { GuardianAssessmentStatus::Aborted => (None, None, None), }; GuardianAssessmentEvent { + review_reason: None, id: format!("review-{id}"), target_item_id: Some(id.to_string()), plugin_id: Some("sample@openai-curated".to_string()), @@ -2498,6 +2510,7 @@ mod tests { &conversation_id, "turn-from-event", &GuardianAssessmentEvent { + review_reason: None, id: "review-1".to_string(), target_item_id: Some("item-1".to_string()), plugin_id: None, @@ -2546,6 +2559,7 @@ mod tests { &conversation_id, "turn-from-event", &GuardianAssessmentEvent { + review_reason: None, id: "review-2".to_string(), target_item_id: Some("item-2".to_string()), plugin_id: None, @@ -2602,6 +2616,7 @@ mod tests { &conversation_id, "turn-from-event", &GuardianAssessmentEvent { + review_reason: None, id: "review-3".to_string(), target_item_id: None, plugin_id: None, diff --git a/codex-rs/core/src/guardian/coverage.rs b/codex-rs/core/src/guardian/coverage.rs new file mode 100644 index 0000000000..f18aa2bf0c --- /dev/null +++ b/codex-rs/core/src/guardian/coverage.rs @@ -0,0 +1,33 @@ +//! Supplies action categories to the review extension and enforces host-owned requirements. + +use super::GuardianApprovalRequest; +use crate::tools::sandboxing::ApprovalAction; +use codex_protocol::openai_models::GuardianScope; + +impl GuardianApprovalRequest { + pub(crate) fn guardian_scope(&self) -> GuardianScope { + match self { + Self::ExecCommand { .. } | Self::WriteStdin { .. } => GuardianScope::Shell, + #[cfg(unix)] + Self::Execve { .. } => GuardianScope::Shell, + Self::ApplyPatch { .. } => GuardianScope::FileChanges, + Self::McpToolCall { server, .. } => GuardianScope::for_mcp_server(server), + Self::NetworkAccess { .. } => GuardianScope::Network, + Self::RequestPermissions { .. } => GuardianScope::Permissions, + } + } +} + +impl ApprovalAction { + pub(crate) fn guardian_scope(&self) -> GuardianScope { + match self { + Self::ExecCommand { .. } | Self::WriteStdin { .. } => GuardianScope::Shell, + #[cfg(unix)] + Self::Execve { .. } => GuardianScope::Shell, + Self::ApplyPatch { .. } => GuardianScope::FileChanges, + Self::McpToolCall { server, .. } => GuardianScope::for_mcp_server(server), + Self::NetworkAccess { .. } => GuardianScope::Network, + Self::RequestPermissions { .. } => GuardianScope::Permissions, + } + } +} diff --git a/codex-rs/core/src/guardian/decision.rs b/codex-rs/core/src/guardian/decision.rs new file mode 100644 index 0000000000..11f9b641f2 --- /dev/null +++ b/codex-rs/core/src/guardian/decision.rs @@ -0,0 +1,184 @@ +//! Calls the decision extension for each approval and enforces host constraints. +//! The synchronous service captures one action; no outcome is stored by tool-call ID. + +use codex_async_utils::THREAD_STACK_SIZE_BYTES; +use codex_extension_api::ApprovalDecision; +use codex_extension_api::ApprovalDecisionInput; +use codex_extension_api::SynchronousApprovalReviewer; +use codex_protocol::approvals::GuardianReviewReason; +use codex_protocol::protocol::ReviewDecision; +use std::sync::Arc; +use tokio::sync::oneshot; +use tokio_util::sync::CancellationToken; + +use super::ApprovalRequestReasons; +use super::GuardianApprovalRequest; +use super::GuardianReviewContext; +use super::GuardianReviewOptions; +use super::review::record_guardian_non_denial; +use super::runtime::ReviewAction; +use super::runtime::ReviewRuntime; +use crate::session::session::Session; + +pub(crate) fn spawn_approval_decision( + session: Arc, + context: impl Into, + review_id: String, + request: impl Into, + reasons: ApprovalRequestReasons, + options: GuardianReviewOptions, +) -> oneshot::Receiver> { + let context: GuardianReviewContext = context.into(); + let request: ReviewAction = request.into(); + let (tx, rx) = oneshot::channel(); + let runtime = session.services.runtime_handle.clone(); + let spawn_result = std::thread::Builder::new() + .name("codex-approval-review".to_string()) + .stack_size(THREAD_STACK_SIZE_BYTES) + .spawn(move || { + let decision = runtime.block_on(decide_approval( + session, context, review_id, request, reasons, options, + )); + let _ = tx.send(decision); + }); + if let Err(err) = spawn_result { + tracing::error!(%err, "failed to spawn automatic approval review worker"); + } + rx +} + +/// `None` requests the existing user flow. No contributor is never an implicit allow. +pub(crate) async fn decide_approval( + session: Arc, + context: impl Into, + review_id: String, + request: impl Into, + reasons: ApprovalRequestReasons, + options: GuardianReviewOptions, +) -> Option { + let context = context.into(); + let request = request.into(); + let turn = context.turn(); + let requirements = turn.config.config_layer_stack.requirements(); + let model_requires_review = + requirements.auto_review_required_for_model(&turn.model_info().slug); + let require_guardian = options.require_guardian + || model_requires_review + || requirements + .approvals_reviewer + .can_set(&codex_protocol::config_types::ApprovalsReviewer::User) + .is_err(); + let require_fresh_review = options.require_synchronous_review + || model_requires_review + && !turn + .config + .features + .enabled(codex_features::Feature::GuardianV2) + || options + .external_cancel + .as_ref() + .is_some_and(CancellationToken::is_cancelled) + || reasons.retry.is_some() + || request.request.as_ref().is_ok_and(|request| { + super::approval_request::format_guardian_action_compact(request).is_err() + }) + || matches!(&request.request, Ok(GuardianApprovalRequest::ExecCommand { sandbox_permissions, .. }) + if sandbox_permissions.requires_escalated_permissions()); + let full_access = context.environments().has_full_access( + context.approval_policy, + &turn.config.permissions.effective_permission_profile(), + ); + if full_access { + return Some(if let Err(decision) = request.validate(&context) { + decision + } else if options + .external_cancel + .as_ref() + .is_some_and(CancellationToken::is_cancelled) + { + ReviewDecision::Abort + } else { + ReviewDecision::Approved + }); + } + let action = match &request.action { + Ok(action) => action, + Err(_) => { + return Some(ReviewDecision::denied( + "automatic approval review could not prepare the action", + )); + } + }; + let runtime = ReviewRuntime { + session: Arc::clone(&session), + context: context.clone(), + review_id: review_id.clone(), + request: request.clone(), + reasons, + options, + }; + let input = ApprovalDecisionInput { + approval_id: &review_id, + action, + thread_id: session.thread_id, + thread_store: &session.services.thread_extension_data, + category: request.category, + approval_policy: context.approval_policy, + approvals_reviewer: context.approvals_reviewer, + require_guardian, + require_fresh_review, + full_access, + metrics: Some(crate::session::extension_metrics::from_session_telemetry( + turn.session_telemetry.clone(), + )), + synchronous_reviewer: &runtime, + }; + match session.services.extensions.decide_approval(&input).await { + Some(ApprovalDecision::Reviewed(decision)) => Some(decision), + Some(ApprovalDecision::Allow) if !require_fresh_review => { + let request = match request.validate(&context) { + Ok(request) => request, + Err(decision) => return Some(decision), + }; + let turn_id = super::approval_request::guardian_request_turn_id(request, &turn.sub_id); + if session + .services + .thread_extension_data + .get::() + .is_some() + { + session + .services + .analytics_events_client + .track_guardian_v2_event(codex_analytics::GuardianV2Event { + thread_id: session.thread_id.to_string(), + turn_id: turn_id.to_owned(), + item_id: super::approval_request::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: codex_analytics::GuardianV2EventKind::FastDecision { + decision: "approved", + }, + }); + } + record_guardian_non_denial(&session, turn_id).await; + Some(ReviewDecision::Approved) + } + Some(ApprovalDecision::Allow) => { + Some(runtime.review(GuardianReviewReason::FreshRequired).await) + } + Some(ApprovalDecision::AskUser) if !require_guardian => None, + None if !require_guardian + && !super::review::routes_approval_policy_to_guardian( + context.approval_policy, + context.approvals_reviewer, + ) => + { + None + } + None | Some(ApprovalDecision::AskUser) => { + Some(runtime.review(GuardianReviewReason::Policy).await) + } + } +} diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 57fdcd7737..3701c86add 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -1,18 +1,10 @@ -//! Guardian review decides whether an `on-request` approval should be granted -//! automatically instead of shown to the user. -//! Full Access (`never` approvals with a disabled sandbox) approves without review. -//! -//! High-level approach: -//! 1. Reconstruct a compact transcript that preserves user intent plus the most -//! relevant recent assistant and tool context. -//! 2. Ask a dedicated guardian review session to assess the exact planned -//! action and return strict JSON. -//! The guardian clones the parent config, so it inherits any managed -//! network proxy / allowlist that the parent turn already had. -//! 3. Fail closed on timeout, execution failure, or malformed output. -//! 4. Apply the guardian's explicit allow/deny outcome. +//! Hosts approval decisions and the isolated synchronous reviewer. +//! The extension chooses policy and evidence; core enforces permissions and mandatory +//! review requirements. Each approval retains its issuing context and cancellation. mod approval_request; +mod coverage; +mod decision; mod feedback; mod metrics; mod prompt; @@ -43,6 +35,8 @@ pub(crate) use approval_request::GuardianMcpAnnotations; pub(crate) use approval_request::GuardianNetworkAccessTrigger; #[cfg(test)] pub(crate) use approval_request::guardian_approval_request_to_json; +pub(crate) use decision::decide_approval; +pub(crate) use decision::spawn_approval_decision; pub(crate) use prompt::BUNDLED_GUARDIAN_POLICY; pub(crate) use prompt::BUNDLED_GUARDIAN_POLICY_TEMPLATE; pub(crate) use prompt::guardian_truncate_text; @@ -52,13 +46,14 @@ 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; 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::spawn_approval_request_review; pub(crate) use review_session::GuardianReviewSessionManager; pub(crate) use review_session::prompt_cache_key_override_for_review_session; +pub(crate) use runtime::ReviewAction; pub(crate) const GUARDIAN_REVIEW_TIMEOUT: Duration = Duration::from_secs(90); pub(crate) const GUARDIAN_REVIEWER_NAME: &str = "guardian"; @@ -83,9 +78,7 @@ const GUARDIAN_RECENT_ENTRY_LIMIT: usize = 40; /// Background network approvals and Unix interception use the active task's resolved settings. /// Startup reviewer prewarming intentionally uses turn-only inputs because it has no issuing step. /// -/// MCP elicitation reviews still use turn-only inputs. -/// TODO(sayan): See if we can find a way to model those as StepContext as well without holding -/// step-scoped things past their lifetime (like MCP bindings) +/// MCP elicitation reviews continue to use turn-only inputs. #[derive(Clone)] pub(crate) struct GuardianReviewContext { /// The response currently handled in this execution context. diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 83384c0c38..3347abe33d 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -7,7 +7,6 @@ use codex_analytics::GuardianReviewTrackContext; use codex_analytics::GuardianReviewedAction; use codex_analytics::GuardianV2Event; use codex_analytics::GuardianV2EventKind; -use codex_async_utils::THREAD_STACK_SIZE_BYTES; use codex_core_plugins::PluginCommandAttribution; use codex_extension_api::GuardianV2Enabled; use codex_extension_api::ThreadIdleCause; @@ -29,10 +28,10 @@ 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::sync::oneshot; use tokio::time::Instant; use tokio::time::sleep_until; use tokio_util::sync::CancellationToken; @@ -268,7 +267,7 @@ fn track_guardian_review( .track_guardian_review(tracking, result, completed_at_ms); } -async fn record_guardian_non_denial(session: &Arc, turn_id: &str) { +pub(super) async fn record_guardian_non_denial(session: &Arc, turn_id: &str) { session .services .guardian_rejection_circuit_breaker @@ -432,21 +431,17 @@ async fn run_guardian_review( session, context, review_id, - request, + request: request.into(), reasons, options, }; - codex_extension_api::SynchronousApprovalReviewer::review( - &runtime, - codex_protocol::approvals::GuardianReviewReason::Policy, - ) - .await + 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: codex_protocol::approvals::GuardianReviewReason, + review_reason: Option, ) -> ReviewDecision { let super::runtime::ReviewRuntime { session, @@ -456,12 +451,17 @@ pub(super) async fn run_synchronous_review( reasons, options, } = runtime; + let request = match request.validate(&context) { + Ok(request) => request.clone(), + Err(decision) => return decision, + }; let turn = Arc::clone(context.turn()); let GuardianReviewOptions { plugin_attribution_override, approval_request_source, external_cancel, require_synchronous_review: _, + require_guardian: _, } = options; // Bound executor attribution and the Guardian session with one review deadline. let deadline = Instant::now() + GUARDIAN_REVIEW_TIMEOUT; @@ -518,6 +518,7 @@ pub(super) async fn run_synchronous_review( .send_event( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { + review_reason, id: review_id.clone(), target_item_id: target_item_id.clone(), plugin_id: plugin_id.clone(), @@ -557,6 +558,7 @@ pub(super) async fn run_synchronous_review( .send_event( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { + review_reason, id: review_id, target_item_id, plugin_id: plugin_id.clone(), @@ -696,6 +698,7 @@ pub(super) async fn run_synchronous_review( .send_event( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { + review_reason, id: review_id, target_item_id, plugin_id: plugin_id.clone(), @@ -733,6 +736,7 @@ pub(super) async fn run_synchronous_review( .send_event( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { + review_reason, id: review_id, target_item_id, plugin_id: plugin_id.clone(), @@ -818,6 +822,7 @@ pub(super) async fn run_synchronous_review( GuardianAssessmentStatus::Denied }; let assessment_event = GuardianAssessmentEvent { + review_reason, id: review_id, target_item_id, plugin_id: plugin_id.clone(), @@ -879,6 +884,8 @@ pub(super) async fn run_synchronous_review( #[derive(Clone)] pub(crate) struct GuardianReviewOptions { + /// Requires Guardian rather than a manual approval; cached evidence may still satisfy it. + pub(crate) require_guardian: bool, pub(crate) plugin_attribution_override: Option, pub(crate) approval_request_source: GuardianApprovalRequestSource, pub(crate) external_cancel: Option, @@ -887,6 +894,7 @@ pub(crate) struct GuardianReviewOptions { } /// 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, @@ -902,6 +910,7 @@ pub(crate) async fn review_approval_request( request, reasons, GuardianReviewOptions { + require_guardian: false, plugin_attribution_override: None, approval_request_source: GuardianApprovalRequestSource::MainTurn, external_cancel: None, @@ -933,32 +942,6 @@ pub(crate) async fn review_approval_request_with_cancel( .await } -pub(crate) fn spawn_approval_request_review( - session: Arc, - context: impl Into, - review_id: String, - request: GuardianApprovalRequest, - reasons: ApprovalRequestReasons, - options: GuardianReviewOptions, -) -> oneshot::Receiver { - let context = context.into(); - let (tx, rx) = oneshot::channel(); - let runtime = session.services.runtime_handle.clone(); - let spawn_result = std::thread::Builder::new() - .name("codex-approval-review".to_string()) - .stack_size(THREAD_STACK_SIZE_BYTES) - .spawn(move || { - let decision = runtime.block_on(run_guardian_review( - session, context, review_id, request, reasons, options, - )); - let _ = tx.send(decision); - }); - if let Err(err) = spawn_result { - tracing::error!(%err, "failed to spawn automatic approval review worker"); - } - rx -} - 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 d4b353371e..093a998a33 100644 --- a/codex-rs/core/src/guardian/runtime.rs +++ b/codex-rs/core/src/guardian/runtime.rs @@ -1,4 +1,4 @@ -//! Binds the existing synchronous reviewer to one captured approval action. +//! Binds the existing synchronous reviewer to one approval action. use codex_extension_api::ExtensionFuture; use codex_extension_api::SynchronousApprovalReviewer; @@ -9,15 +9,77 @@ use super::ApprovalRequestReasons; use super::GuardianApprovalRequest; use super::GuardianReviewContext; use super::GuardianReviewOptions; +use super::approval_request::guardian_approval_request_to_json; use super::review::run_synchronous_review; use crate::session::session::Session; +/// Carries the original action even when the legacy synchronous renderer cannot handle its paths. +/// This lets the extension return AskUser before invoking that renderer. +#[derive(Clone)] +pub(crate) struct ReviewAction { + pub(super) action: Result, + pub(super) category: codex_protocol::openai_models::GuardianScope, + pub(super) request: Result, +} + +impl From for ReviewAction { + fn from(request: GuardianApprovalRequest) -> Self { + Self { + action: guardian_approval_request_to_json(&request).map_err(|error| error.to_string()), + category: request.guardian_scope(), + request: Ok(request), + } + } +} + +impl ReviewAction { + pub(crate) fn from_approval_action( + action: crate::tools::sandboxing::ApprovalAction, + exec_command_cwd_convention: Option, + ) -> Self { + let category = action.guardian_scope(); + let original = serde_json::to_value(&action).map_err(|error| error.to_string()); + match action.into_guardian_request(exec_command_cwd_convention) { + Ok(request) => Self::from(request), + Err(error) => Self { + action: original, + category, + request: Err(error.to_string()), + }, + } + } +} + +impl ReviewAction { + /// Preserve the checks previously made before entering Guardian from tool approvals. + pub(super) fn validate( + &self, + context: &GuardianReviewContext, + ) -> Result<&GuardianApprovalRequest, ReviewDecision> { + let request = self.request.as_ref().map_err(|error| { + tracing::error!(%error, "failed to build automatic approval action"); + ReviewDecision::denied("automatic approval review could not prepare the action") + })?; + if let GuardianApprovalRequest::WriteStdin { environment_id, .. } = request + && !context + .environments() + .turn_environments() + .any(|environment| environment.selection.environment_id == *environment_id) + { + return Err(ReviewDecision::denied( + "automatic approval review cannot access the terminal's environment; select it before retrying", + )); + } + Ok(request) + } +} + #[derive(Clone)] pub(super) struct ReviewRuntime { pub(super) session: Arc, pub(super) context: GuardianReviewContext, pub(super) review_id: String, - pub(super) request: GuardianApprovalRequest, + pub(super) request: ReviewAction, pub(super) reasons: ApprovalRequestReasons, pub(super) options: GuardianReviewOptions, } @@ -27,6 +89,6 @@ impl SynchronousApprovalReviewer for ReviewRuntime { &self, reason: codex_protocol::approvals::GuardianReviewReason, ) -> ExtensionFuture<'_, ReviewDecision> { - Box::pin(run_synchronous_review(self.clone(), reason)) + Box::pin(run_synchronous_review(self.clone(), Some(reason))) } } diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 88f4f17a20..71190f951d 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -1696,6 +1696,7 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() { }, /*retry_reason*/ None, GuardianReviewOptions { + require_guardian: false, plugin_attribution_override: None, approval_request_source: GuardianApprovalRequestSource::MainTurn, external_cancel: Some(cancel_token), diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 1444c1e64c..f4feaafbc2 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -891,6 +891,7 @@ async fn review_guardian_mcp_elicitation( guardian_request, /*retry_reason*/ None, crate::guardian::GuardianReviewOptions { + require_guardian: false, plugin_attribution_override: None, approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn, external_cancel: Some(cancellation_token), @@ -984,6 +985,7 @@ async fn review_guardian_mcp_elicitation( guardian_request, /*retry_reason*/ None, crate::guardian::GuardianReviewOptions { + require_guardian: false, plugin_attribution_override: None, approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn, external_cancel: Some(cancellation_token), diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 24fd9cf629..849f07c935 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2780,7 +2780,6 @@ impl Session { ) -> Option { let turn_context = &step_context.turn; let approval_policy = step_context.settings.approval_policy(); - let approvals_reviewer = step_context.settings.approvals_reviewer(); let Some(environment) = step_context .environments .turn_environments() @@ -2827,7 +2826,6 @@ impl Session { strict_auto_review: false, }); }; - if crate::guardian::routes_approval_policy_to_guardian(approval_policy, approvals_reviewer) { let originating_turn_state = { let active = self.active_turn.lock().await; @@ -2836,13 +2834,13 @@ impl Session { let action = ApprovalAction::RequestPermissions { id: call_id.clone(), turn_id: turn_context.sub_id.clone(), - reason: args.reason, + reason: args.reason.clone(), permissions: requested_permissions.clone(), }; let approval_context = ApprovalContext { review_context: crate::guardian::GuardianReviewContext::from(step_context), cancellation_token: Some(cancellation_token.clone()), - call_id, + call_id: call_id.clone(), tool_name: ToolName::plain("request_permissions"), strict_auto_review: false, approval_reason: None, @@ -2857,54 +2855,53 @@ impl Session { &approval_context, ) => decision, }; - let response = match decision { - ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } => { - RequestPermissionsResponse { - permissions: requested_permissions.clone(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, + if let Some(decision) = decision { + let (permissions, scope) = match decision { + ReviewDecision::Approved + | ReviewDecision::ApprovedExecpolicyAmendment { .. } + | ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment: + NetworkPolicyAmendment { + action: NetworkPolicyRuleAction::Allow, + .. + }, + } => (requested_permissions.clone(), PermissionGrantScope::Turn), + ReviewDecision::ApprovedForSession => { + (requested_permissions.clone(), PermissionGrantScope::Session) } - } - ReviewDecision::ApprovedForSession => RequestPermissionsResponse { - permissions: requested_permissions.clone(), - scope: PermissionGrantScope::Session, + ReviewDecision::ApprovedMcpPolicyAmendment + | ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment: + NetworkPolicyAmendment { + action: NetworkPolicyRuleAction::Deny, + .. + }, + } + | ReviewDecision::Abort + | ReviewDecision::Denied { .. } + | ReviewDecision::TimedOut => ( + RequestPermissionProfile::default(), + PermissionGrantScope::Turn, + ), + }; + let response = RequestPermissionsResponse { + permissions, + scope, strict_auto_review: false, - }, - ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment, - } => match network_policy_amendment.action { - NetworkPolicyRuleAction::Allow => RequestPermissionsResponse { - permissions: requested_permissions.clone(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - }, - NetworkPolicyRuleAction::Deny => RequestPermissionsResponse { - permissions: RequestPermissionProfile::default(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - }, - }, - ReviewDecision::ApprovedMcpPolicyAmendment - | ReviewDecision::Abort - | ReviewDecision::Denied { .. } - | ReviewDecision::TimedOut => RequestPermissionsResponse { - permissions: RequestPermissionProfile::default(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - }, - }; - let response = Self::normalize_request_permissions_response( - requested_permissions, - response, - &context, - ); - self.record_granted_request_permissions_for_turn( - &response, - &environment.selection.environment_id, - originating_turn_state.as_ref(), - ) - .await; - return Some(response); + }; + let response = Self::normalize_request_permissions_response( + requested_permissions, + response, + &context, + ); + self.record_granted_request_permissions_for_turn( + &response, + &environment.selection.environment_id, + originating_turn_state.as_ref(), + ) + .await; + return Some(response); + } } let _elicitation = self.services.elicitations.register(); diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index 7e7c68e13e..8675ead36d 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -5,12 +5,10 @@ use crate::exec_policy::prompt_is_rejected_by_policy; use crate::guardian::GuardianNetworkAccessTrigger; use crate::guardian::GuardianReviewContext; use crate::guardian::GuardianReviewOptions; +use crate::guardian::decide_approval; use crate::guardian::guardian_timeout_message; use crate::guardian::new_guardian_review_id; -use crate::guardian::review_approval_request; -use crate::guardian::review_approval_request_with_cancel; -use crate::guardian::routes_approval_policy_to_guardian; -use crate::guardian::spawn_approval_request_review; +use crate::guardian::spawn_approval_decision; use crate::hook_runtime::run_permission_request_hooks; use crate::mcp_tool_call::request_mcp_tool_user_approval; use crate::sandboxing::SandboxPermissions; @@ -64,12 +62,14 @@ pub(crate) struct ApprovalContext { pub(crate) network_approval_context: Option, } -#[derive(Clone, Debug, PartialEq)] +#[derive(Clone, Debug, PartialEq, serde::Serialize)] +#[serde(tag = "tool", rename_all = "snake_case")] pub(crate) enum ApprovalAction { ExecCommand { id: String, environment_id: String, command: Vec, + #[serde(skip_serializing)] hook_command: String, cwd: PathUri, sandbox_permissions: SandboxPermissions, @@ -107,6 +107,7 @@ pub(crate) enum ApprovalAction { cwd: PathUri, files: Vec, patch: String, + #[serde(skip_serializing)] changes: Arc>, permissions_preapproved: bool, }, @@ -122,6 +123,7 @@ pub(crate) enum ApprovalAction { tool_title: Option, tool_description: Option, annotations: Option, + #[serde(skip_serializing)] hook_tool_name: HookToolName, approval_policy: AskForApproval, reviewer: ApprovalsReviewer, @@ -138,7 +140,9 @@ pub(crate) enum ApprovalAction { protocol: NetworkApprovalProtocol, port: u16, trigger: Option, + #[serde(skip_serializing)] hook_command: String, + #[serde(skip_serializing)] hook_run_id: String, command: Vec, cwd: AbsolutePathBuf, @@ -414,22 +418,6 @@ impl ApprovalAction { } } -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -enum ApprovalReviewer { - Guardian, - User, -} - -impl ApprovalReviewer { - fn for_policy(approval_policy: AskForApproval, reviewer: ApprovalsReviewer) -> Self { - if routes_approval_policy_to_guardian(approval_policy, reviewer) { - Self::Guardian - } else { - Self::User - } - } -} - #[derive(Clone, Copy, Debug, PartialEq, Eq)] enum ApprovalResolutionSource { Hook, @@ -556,51 +544,33 @@ impl Session { action: ApprovalAction, ctx: &ApprovalContext, ) -> ApprovalResolution { - let reviewer = if ctx.strict_auto_review { - ApprovalReviewer::Guardian - } else if let ApprovalAction::McpToolCall { - approval_policy, - reviewer, - .. - } = &action - { - ApprovalReviewer::for_policy(*approval_policy, *reviewer) + if let Some(decision) = self.request_guardian_approval(action.clone(), ctx).await { + ApprovalResolution { + decision, + source: ApprovalResolutionSource::Guardian, + } } else { - ApprovalReviewer::for_policy( - ctx.review_context.approval_policy, - ctx.review_context.approvals_reviewer, - ) - }; - - let decision = match reviewer { - ApprovalReviewer::Guardian => self.request_guardian_approval(action, ctx).await, - ApprovalReviewer::User => self.request_user_approval(&action, ctx).await, - }; - let source = match reviewer { - ApprovalReviewer::Guardian => ApprovalResolutionSource::Guardian, - ApprovalReviewer::User => ApprovalResolutionSource::User, - }; - ApprovalResolution { decision, source } + ApprovalResolution { + decision: self.request_user_approval(&action, ctx).await, + source: ApprovalResolutionSource::User, + } + } } pub(crate) async fn request_guardian_approval( self: &Arc, action: ApprovalAction, ctx: &ApprovalContext, - ) -> ReviewDecision { - // Guardian inherits only the current turn's ready environments. A retained - // terminal handle may outlive its selection, but must not be reviewed in - // a different environment that happens to have the same launch directory. - if let ApprovalAction::WriteStdin { environment_id, .. } = &action - && !ctx - .review_context - .environments() - .turn_environments() - .any(|environment| environment.selection.environment_id == *environment_id) + ) -> Option { + let mut context = ctx.review_context.clone(); + if let ApprovalAction::McpToolCall { + approval_policy, + reviewer, + .. + } = &action { - return ReviewDecision::denied( - "automatic approval review cannot access the terminal's environment; select it before retrying", - ); + context.approval_policy = *approval_policy; + context.approvals_reviewer = *reviewer; } let is_network_approval = matches!(&action, ApprovalAction::NetworkAccess { .. }); let review_id = new_guardian_review_id(); @@ -628,74 +598,67 @@ impl Session { ), _ => None, }; - let action = match action.into_guardian_request(exec_command_cwd_convention) { - Ok(action) => action, - Err(err) => { - tracing::error!(%err, "failed to build automatic approval action"); - return ReviewDecision::denied( - "automatic approval review could not prepare the action", - ); - } - }; + let action = crate::guardian::ReviewAction::from_approval_action( + action, + exec_command_cwd_convention, + ); - if let Some(cancellation_token) = &ctx.cancellation_token { - let review = spawn_approval_request_review( + let reasons = ApprovalRequestReasons { + approval: ctx.approval_reason.clone(), + retry: ctx.retry_reason.clone(), + }; + let options = GuardianReviewOptions { + require_guardian: ctx.strict_auto_review, + plugin_attribution_override: None, + approval_request_source: GuardianApprovalRequestSource::MainTurn, + external_cancel: ctx.cancellation_token.clone(), + require_synchronous_review: false, + }; + if ctx.cancellation_token.is_some() { + spawn_approval_decision( Arc::clone(self), - ctx.review_context.clone(), + context, review_id, action, - ApprovalRequestReasons { - approval: ctx.approval_reason.clone(), - retry: ctx.retry_reason.clone(), - }, - GuardianReviewOptions { - plugin_attribution_override: None, - approval_request_source: GuardianApprovalRequestSource::MainTurn, - external_cancel: Some(cancellation_token.clone()), - require_synchronous_review: false, - }, - ); - review.await.unwrap_or_else(|_| { - ReviewDecision::denied("automatic approval review could not complete") + reasons, + options, + ) + .await + .unwrap_or_else(|_| { + Some(ReviewDecision::denied( + "automatic approval review could not complete", + )) }) } else if is_network_approval { let review_cancel = CancellationToken::new(); let review_cancel_guard = review_cancel.clone().drop_guard(); - let review_session = Arc::clone(self); - let review_context = ctx.review_context.clone(); - let retry_reason = ctx.retry_reason.clone(); - let review = tokio::spawn(async move { - review_approval_request_with_cancel( - &review_session, - review_context, - review_id, - action, - retry_reason, - GuardianReviewOptions { - plugin_attribution_override: None, - approval_request_source: GuardianApprovalRequestSource::MainTurn, - external_cancel: Some(review_cancel), - require_synchronous_review: false, - }, - ) - .await - }); + let review = tokio::spawn(decide_approval( + Arc::clone(self), + context, + review_id, + action, + reasons, + GuardianReviewOptions { + external_cancel: Some(review_cancel), + ..options + }, + )); let decision = review.await.unwrap_or_else(|err| { warn!("network Guardian review task failed: {err}"); - ReviewDecision::denied("automatic approval review could not complete") + Some(ReviewDecision::denied( + "automatic approval review could not complete", + )) }); drop(review_cancel_guard.disarm()); decision } else { - review_approval_request( - self, - ctx.review_context.clone(), + decide_approval( + Arc::clone(self), + context, review_id, action, - ApprovalRequestReasons { - approval: ctx.approval_reason.clone(), - retry: ctx.retry_reason.clone(), - }, + reasons, + options, ) .await } diff --git a/codex-rs/core/src/tools/approvals_tests.rs b/codex-rs/core/src/tools/approvals_tests.rs index 21e1cf8ed1..4ecd7460bf 100644 --- a/codex-rs/core/src/tools/approvals_tests.rs +++ b/codex-rs/core/src/tools/approvals_tests.rs @@ -76,6 +76,109 @@ fn approval_resolution_uses_acting_model_timeout_instructions() { } } +#[cfg(unix)] +#[test_case::test_case(ApprovalsReviewer::User, codex_extension_api::ApprovalDecision::AskUser; "manual prompt")] +#[test_case::test_case(ApprovalsReviewer::AutoReview, codex_extension_api::ApprovalDecision::Allow; "cached allow")] +#[tokio::test] +async fn non_utf8_cwd_preserves_approval_routing( + reviewer: ApprovalsReviewer, + decision: codex_extension_api::ApprovalDecision, +) -> anyhow::Result<()> { + use anyhow::Context; + use codex_extension_api::ApprovalDecision; + use std::os::unix::ffi::OsStringExt; + + struct Contributor { + cwd: codex_utils_path_uri::LegacyAppPathString, + decision: ApprovalDecision, + } + + impl codex_extension_api::ApprovalReviewContributor for Contributor { + fn decide<'a>( + &'a self, + input: &'a codex_extension_api::ApprovalDecisionInput<'_>, + ) -> codex_extension_api::ExtensionFuture<'a, Option> { + Box::pin(async move { + assert_eq!(input.action["cwd"], serde_json::json!(self.cwd)); + Some(self.decision.clone()) + }) + } + } + + let cwd = PathUri::from_abs_path(&AbsolutePathBuf::try_from(PathBuf::from( + std::ffi::OsString::from_vec(b"/tmp/non-utf8-\xe9".to_vec()), + ))?); + let (mut session, turn, events) = make_session_and_context_with_rx().await; + let mut extensions = codex_extension_api::ExtensionRegistryBuilder::new(); + extensions.approval_review_contributor(Arc::new(Contributor { + cwd: codex_utils_path_uri::LegacyAppPathString::from_path_uri(&cwd, PathConvention::Posix)?, + decision, + })); + Arc::get_mut(&mut session) + .context("session is uniquely owned")? + .services + .extensions = Arc::new(extensions.build()); + *session.active_turn.lock().await = Some(crate::state::ActiveTurn::default()); + let mut review_context = GuardianReviewContext::from(&turn); + review_context.approval_policy = AskForApproval::OnRequest; + review_context.approvals_reviewer = reviewer; + let context = ApprovalContext { + review_context, + cancellation_token: None, + call_id: "non-utf8-cwd".to_string(), + tool_name: ToolName::plain("exec_command"), + strict_auto_review: false, + approval_reason: None, + retry_reason: None, + network_approval_context: None, + }; + let action = ApprovalAction::ExecCommand { + id: context.call_id.clone(), + environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), + command: vec!["npm".to_string(), "install".to_string()], + hook_command: "npm install".to_string(), + cwd: cwd.clone(), + sandbox_permissions: if reviewer == ApprovalsReviewer::User { + SandboxPermissions::RequireEscalated + } else { + SandboxPermissions::UseDefault + }, + additional_permissions: None, + justification: None, + tty: false, + proposed_execpolicy_amendment: None, + }; + let approval = session.request_reviewer_approval(action, &context); + tokio::pin!(approval); + let expected = if reviewer == ApprovalsReviewer::User { + tokio::select! { + resolution = &mut approval => panic!("expected a user prompt, got {resolution:?}"), + event = events.recv() => { + let codex_protocol::protocol::EventMsg::ExecApprovalRequest(request) = + event.context("receive user prompt")?.msg + else { + panic!("expected a command approval prompt"); + }; + assert_eq!(request.cwd, codex_utils_path_uri::LegacyAppPathString::from(cwd)); + assert_eq!(request.command, vec!["npm", "install"]); + session.notify_approval(&request.call_id, ReviewDecision::Approved).await; + } + } + ApprovalResolution { + decision: ReviewDecision::Approved, + source: ApprovalResolutionSource::User, + } + } else { + ApprovalResolution { + decision: ReviewDecision::Approved, + source: ApprovalResolutionSource::Guardian, + } + }; + assert_eq!(approval.await, expected); + assert!(events.try_recv().is_err()); + Ok(()) +} + #[tokio::test] async fn explicit_mcp_reviewer_override_takes_precedence_over_action_context() { let (session, turn, events) = make_session_and_context_with_rx().await; diff --git a/codex-rs/core/tests/suite/auto_review.rs b/codex-rs/core/tests/suite/auto_review.rs index 75575a61bb..e529144589 100644 --- a/codex-rs/core/tests/suite/auto_review.rs +++ b/codex-rs/core/tests/suite/auto_review.rs @@ -7,9 +7,7 @@ use anyhow::Result; use codex_config::test_support::CloudConfigBundleFixture; use codex_core::config::Constrained; use codex_extension_api::ApprovalReviewContributor; -use codex_extension_api::ExtensionData; use codex_extension_api::ExtensionFuture; -use codex_extension_api::ExtensionMetrics; use codex_extension_api::ExtensionRegistryBuilder; use codex_features::Feature; use codex_login::CodexAuth; @@ -29,7 +27,6 @@ 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::RequestPermissionProfile; @@ -65,17 +62,14 @@ use wiremock::matchers::path; struct ApprovedReviewContributor; impl ApprovalReviewContributor for ApprovedReviewContributor { - 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> { + input: &'a codex_extension_api::ApprovalDecisionInput<'_>, + ) -> ExtensionFuture<'a, Option> { Box::pin(async move { - assert!(extension_metrics.is_some()); - assert!(prompt.contains("\"tool\":\"request_permissions\"")); - Some(ReviewDecision::Approved) + assert!(input.metrics.is_some()); + assert_eq!(input.action["tool"], "request_permissions"); + Some(codex_extension_api::ApprovalDecision::Allow) }) } } @@ -83,16 +77,13 @@ impl ApprovalReviewContributor for ApprovedReviewContributor { struct EscalationApprovingReviewContributor; impl ApprovalReviewContributor for EscalationApprovingReviewContributor { - 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> { + input: &'a codex_extension_api::ApprovalDecisionInput<'_>, + ) -> ExtensionFuture<'a, Option> { Box::pin(async move { - assert!(prompt.contains("\"sandbox_permissions\":\"require_escalated\"")); - Some(ReviewDecision::Approved) + assert_eq!(input.action["sandbox_permissions"], "require_escalated"); + Some(codex_extension_api::ApprovalDecision::Allow) }) } } diff --git a/codex-rs/core/tests/suite/context_annotations.rs b/codex-rs/core/tests/suite/context_annotations.rs index cd7c1eefe1..69b7d32e68 100644 --- a/codex-rs/core/tests/suite/context_annotations.rs +++ b/codex-rs/core/tests/suite/context_annotations.rs @@ -75,6 +75,7 @@ async fn first_request_item_types_roles_and_content_annotations() -> Result<()> test.codex .submit(Op::ApproveGuardianDeniedAction { event: GuardianAssessmentEvent { + review_reason: None, id: "guardian-review".to_string(), target_item_id: None, plugin_id: None, diff --git a/codex-rs/core/tests/suite/guardian_review.rs b/codex-rs/core/tests/suite/guardian_review.rs index 63a7aabd75..538def3138 100644 --- a/codex-rs/core/tests/suite/guardian_review.rs +++ b/codex-rs/core/tests/suite/guardian_review.rs @@ -1561,14 +1561,16 @@ async fn guardian_timeout_rejects_tool_call_with_acting_model_instructions( struct TimedOutReviewContributor; impl codex_extension_api::ApprovalReviewContributor for TimedOutReviewContributor { - 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 { Some(ReviewDecision::TimedOut) }) + _input: &'a codex_extension_api::ApprovalDecisionInput<'_>, + ) -> codex_extension_api::ExtensionFuture<'a, Option> + { + Box::pin(async { + Some(codex_extension_api::ApprovalDecision::Reviewed( + ReviewDecision::TimedOut, + )) + }) } } diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs b/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs index c5d9ed717c..d783468db3 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs @@ -5,6 +5,7 @@ use super::authorization::ScoreAuthorization; use super::config::GuardianV2Config; use super::coverage::GuardianPolicy; use super::extension::GuardianV2ScoreProgress; +use super::extension::encrypted_parent_compaction; use super::extension::requires_sync_for_compaction; use super::metrics::TOOL_CALL_LAG_METRIC; use super::metrics::record_fast_decision; @@ -139,7 +140,10 @@ async fn cached_evidence( .get::() .ok_or(GuardianReviewReason::MissingScore)?; let history = thread.conversation_history_snapshot().await; - if requires_sync_for_compaction(config, history.as_ref(), &sampler) { + if requires_sync_for_compaction(config, history.as_ref(), &sampler) + || encrypted_parent_compaction(history.items(), config.max_parent_compaction_tokens) + .is_err() + { record_fast_decision(metrics, "deferred", "incompatible_compaction"); return Err(GuardianReviewReason::IncompatibleCompaction); } 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 a12c2018b7..34bb6e5b6a 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -987,7 +987,7 @@ impl GuardianV2Extension { } #[derive(Debug, Eq, PartialEq)] -enum ParentCompactionError { +pub(super) enum ParentCompactionError { Serialization, Oversized, Unusable, @@ -1010,7 +1010,7 @@ pub(super) fn requires_sync_for_compaction( // An unusable latest compaction must never fall back to an older one. Missing // encrypted content is rejected here; only legacy callers may omit that checkpoint. -fn encrypted_parent_compaction<'a>( +pub(super) fn encrypted_parent_compaction<'a>( items: impl Iterator, max_parent_compaction_tokens: usize, ) -> Result, ParentCompactionError> { diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index b05c45ce49..3325a90539 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -204,6 +204,10 @@ pub enum GuardianReviewReason { #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, JsonSchema, TS)] pub struct GuardianAssessmentEvent { + /// Request-scoped trigger; absent in events recorded by older clients. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional)] + pub review_reason: Option, /// Stable identifier for this guardian review lifecycle. pub id: String, /// Thread item being reviewed, when the review maps to a concrete item. diff --git a/codex-rs/tui/src/auto_review_denials.rs b/codex-rs/tui/src/auto_review_denials.rs index c2f7ebbe5d..d6c5ee4229 100644 --- a/codex-rs/tui/src/auto_review_denials.rs +++ b/codex-rs/tui/src/auto_review_denials.rs @@ -94,6 +94,7 @@ mod tests { fn denied_event(id: usize) -> GuardianAssessmentEvent { GuardianAssessmentEvent { + review_reason: None, id: format!("review-{id}"), target_item_id: None, plugin_id: None, diff --git a/codex-rs/tui/src/chatwidget/protocol_requests.rs b/codex-rs/tui/src/chatwidget/protocol_requests.rs index 0d7ef630be..89f78ea6e2 100644 --- a/codex-rs/tui/src/chatwidget/protocol_requests.rs +++ b/codex-rs/tui/src/chatwidget/protocol_requests.rs @@ -95,6 +95,7 @@ impl ChatWidget { }; self.on_guardian_assessment(GuardianAssessmentEvent { + review_reason: None, id, target_item_id: None, plugin_id: None, diff --git a/codex-rs/tui/src/chatwidget/tests/guardian.rs b/codex-rs/tui/src/chatwidget/tests/guardian.rs index d0347860d0..1cac151689 100644 --- a/codex-rs/tui/src/chatwidget/tests/guardian.rs +++ b/codex-rs/tui/src/chatwidget/tests/guardian.rs @@ -3,6 +3,7 @@ use pretty_assertions::assert_eq; fn auto_review_denial_event() -> GuardianAssessmentEvent { GuardianAssessmentEvent { + review_reason: None, id: "auto-review-recent-1".into(), target_item_id: Some("target-auto-review-recent-1".into()), plugin_id: None, @@ -31,6 +32,7 @@ fn guardian_command_event( ) -> GuardianAssessmentEvent { let terminal = status != GuardianAssessmentStatus::InProgress; GuardianAssessmentEvent { + review_reason: None, id: id.to_string(), target_item_id: Some(format!("{id}-target")), plugin_id: None, @@ -244,6 +246,7 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() { }; chat.on_guardian_assessment(GuardianAssessmentEvent { + review_reason: None, id: "guardian-1".into(), target_item_id: Some("guardian-target-1".into()), plugin_id: None, @@ -260,6 +263,7 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() { }); chat.on_warning("Automatic approval review denied (risk: high): The planned action would transmit the full contents of a workspace source file (`core/src/codex.rs`) to `https://example.com`, which is an external and untrusted endpoint."); chat.on_guardian_assessment(GuardianAssessmentEvent { + review_reason: None, id: "guardian-1".into(), target_item_id: Some("guardian-target-1".into()), plugin_id: None, @@ -313,6 +317,7 @@ async fn guardian_approved_exec_is_hidden_from_history() { /*replay_kind*/ None, ); chat.on_guardian_assessment(GuardianAssessmentEvent { + review_reason: None, id: "thread:child-thread:guardian-1".into(), target_item_id: Some("guardian-approved-target".into()), plugin_id: None, @@ -370,6 +375,7 @@ async fn guardian_approved_request_permissions_clears_status_without_history() { }; chat.on_guardian_assessment(GuardianAssessmentEvent { + review_reason: None, id: "guardian-request-permissions".into(), target_item_id: None, plugin_id: None, @@ -396,6 +402,7 @@ async fn guardian_approved_request_permissions_clears_status_without_history() { ); chat.on_guardian_assessment(GuardianAssessmentEvent { + review_reason: None, id: "guardian-request-permissions".into(), target_item_id: None, plugin_id: None, @@ -448,6 +455,7 @@ async fn guardian_timed_out_exec_renders_warning_and_timed_out_request() { }; chat.on_guardian_assessment(GuardianAssessmentEvent { + review_reason: None, id: "guardian-1".into(), target_item_id: Some("guardian-target-1".into()), plugin_id: None, @@ -464,6 +472,7 @@ async fn guardian_timed_out_exec_renders_warning_and_timed_out_request() { }); chat.on_warning("Automatic approval review timed out while evaluating the requested approval."); chat.on_guardian_assessment(GuardianAssessmentEvent { + review_reason: None, id: "guardian-1".into(), target_item_id: Some("guardian-target-1".into()), plugin_id: None, diff --git a/codex-rs/tui/src/chatwidget/tests/mcp_startup.rs b/codex-rs/tui/src/chatwidget/tests/mcp_startup.rs index 95a89273ef..c852182d13 100644 --- a/codex-rs/tui/src/chatwidget/tests/mcp_startup.rs +++ b/codex-rs/tui/src/chatwidget/tests/mcp_startup.rs @@ -534,6 +534,7 @@ async fn mcp_startup_complete_preserves_review_status() { ); chat.on_guardian_assessment(GuardianAssessmentEvent { + review_reason: None, id: "guardian-1".to_string(), target_item_id: Some("guardian-target-1".to_string()), plugin_id: None,