Route approvals through the extension decision API (#43432)

## What changed

- Let approval extensions choose between cached approval, synchronous review, and a user prompt for tool and permission requests, while core enforces mandatory Guardian and fresh-review requirements.
- Carry an optional `review_reason` on Guardian assessment events and use it to trigger strict-review notifications, retaining the fallback for older events.
- Require synchronous review when cached evidence has an unusable encrypted parent compaction.

## Testing

Add regression coverage for manual prompts and cached approvals with non-UTF-8 working directories. Update approval contributors in existing tests to use the decision API.

GitOrigin-RevId: a4e66416070bdaa881daa41429c49021629f44c1
This commit is contained in:
jif authored and copyberry committed 2026-09-07 11:07:28 +00:00
1 parent 0df39752cb
commit e1eb98461c
24 files changed
+605 -250

No files matched your search

@@ -129,6 +129,7 @@ fn guardian_stdin_reviews_preserve_parent_command_history() {
assert!(expected[1].items.is_empty()); assert!(expected[1].items.is_empty());
let assessment = GuardianAssessmentEvent { let assessment = GuardianAssessmentEvent {
review_reason: None,
id: "review-stdin".into(), id: "review-stdin".into(),
target_item_id: Some("terminal-command".into()), target_item_id: Some("terminal-command".into()),
plugin_id: None, plugin_id: None,
@@ -159,6 +160,7 @@ fn guardian_stdin_reviews_preserve_parent_command_history() {
] { ] {
items.push(RolloutItem::EventMsg(EventMsg::GuardianAssessment( items.push(RolloutItem::EventMsg(EventMsg::GuardianAssessment(
GuardianAssessmentEvent { GuardianAssessmentEvent {
review_reason: None,
turn_id: turn_id.into(), turn_id: turn_id.into(),
status, status,
..assessment.clone() ..assessment.clone()
@@ -3339,6 +3339,7 @@ mod tests {
..Default::default() ..Default::default()
}), }),
EventMsg::GuardianAssessment(GuardianAssessmentEvent { EventMsg::GuardianAssessment(GuardianAssessmentEvent {
review_reason: None,
id: "review-guardian-exec".into(), id: "review-guardian-exec".into(),
target_item_id: Some("guardian-exec".into()), target_item_id: Some("guardian-exec".into()),
plugin_id: Some("sample@openai-curated".into()), plugin_id: Some("sample@openai-curated".into()),
@@ -3360,6 +3361,7 @@ mod tests {
.expect("guardian action"), .expect("guardian action"),
}), }),
EventMsg::GuardianAssessment(GuardianAssessmentEvent { EventMsg::GuardianAssessment(GuardianAssessmentEvent {
review_reason: None,
id: "review-guardian-exec".into(), id: "review-guardian-exec".into(),
target_item_id: Some("guardian-exec".into()), target_item_id: Some("guardian-exec".into()),
plugin_id: Some("sample@openai-curated".into()), plugin_id: Some("sample@openai-curated".into()),
@@ -3431,6 +3433,7 @@ mod tests {
..Default::default() ..Default::default()
}), }),
EventMsg::GuardianAssessment(GuardianAssessmentEvent { EventMsg::GuardianAssessment(GuardianAssessmentEvent {
review_reason: None,
id: "review-guardian-execve".into(), id: "review-guardian-execve".into(),
target_item_id: Some("guardian-execve".into()), target_item_id: Some("guardian-execve".into()),
plugin_id: Some("sample@openai-curated".into()), plugin_id: Some("sample@openai-curated".into()),
@@ -355,10 +355,21 @@ pub(crate) async fn apply_bespoke_event_handling(
); );
outgoing.send_server_notification(notification).await; outgoing.send_server_notification(notification).await;
if assessment.status == codex_protocol::protocol::GuardianAssessmentStatus::InProgress if assessment.status == codex_protocol::protocol::GuardianAssessmentStatus::InProgress
&& conversation && match assessment.review_reason {
.thread_extension_data() Some(reason) => matches!(
.remove::<StrictReviewReason>() reason,
.is_some() 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::<StrictReviewReason>()
.is_some(),
}
{ {
outgoing outgoing
.send_server_notification(ServerNotification::StrictReviewRequired( .send_server_notification(ServerNotification::StrictReviewRequired(
@@ -2429,6 +2440,7 @@ mod tests {
GuardianAssessmentStatus::Aborted => (None, None, None), GuardianAssessmentStatus::Aborted => (None, None, None),
}; };
GuardianAssessmentEvent { GuardianAssessmentEvent {
review_reason: None,
id: format!("review-{id}"), id: format!("review-{id}"),
target_item_id: Some(id.to_string()), target_item_id: Some(id.to_string()),
plugin_id: Some("sample@openai-curated".to_string()), plugin_id: Some("sample@openai-curated".to_string()),
@@ -2498,6 +2510,7 @@ mod tests {
&conversation_id, &conversation_id,
"turn-from-event", "turn-from-event",
&GuardianAssessmentEvent { &GuardianAssessmentEvent {
review_reason: None,
id: "review-1".to_string(), id: "review-1".to_string(),
target_item_id: Some("item-1".to_string()), target_item_id: Some("item-1".to_string()),
plugin_id: None, plugin_id: None,
@@ -2546,6 +2559,7 @@ mod tests {
&conversation_id, &conversation_id,
"turn-from-event", "turn-from-event",
&GuardianAssessmentEvent { &GuardianAssessmentEvent {
review_reason: None,
id: "review-2".to_string(), id: "review-2".to_string(),
target_item_id: Some("item-2".to_string()), target_item_id: Some("item-2".to_string()),
plugin_id: None, plugin_id: None,
@@ -2602,6 +2616,7 @@ mod tests {
&conversation_id, &conversation_id,
"turn-from-event", "turn-from-event",
&GuardianAssessmentEvent { &GuardianAssessmentEvent {
review_reason: None,
id: "review-3".to_string(), id: "review-3".to_string(),
target_item_id: None, target_item_id: None,
plugin_id: None, plugin_id: None,
+33
View File
@@ -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,
}
}
}
+184
View File
@@ -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<Session>,
context: impl Into<GuardianReviewContext>,
review_id: String,
request: impl Into<super::ReviewAction>,
reasons: ApprovalRequestReasons,
options: GuardianReviewOptions,
) -> oneshot::Receiver<Option<ReviewDecision>> {
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<Session>,
context: impl Into<GuardianReviewContext>,
review_id: String,
request: impl Into<ReviewAction>,
reasons: ApprovalRequestReasons,
options: GuardianReviewOptions,
) -> Option<ReviewDecision> {
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::<codex_extension_api::GuardianV2Enabled>()
.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)
}
}
}
+10 -17
View File
@@ -1,18 +1,10 @@
//! Guardian review decides whether an `on-request` approval should be granted //! Hosts approval decisions and the isolated synchronous reviewer.
//! automatically instead of shown to the user. //! The extension chooses policy and evidence; core enforces permissions and mandatory
//! Full Access (`never` approvals with a disabled sandbox) approves without review. //! review requirements. Each approval retains its issuing context and cancellation.
//!
//! 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.
mod approval_request; mod approval_request;
mod coverage;
mod decision;
mod feedback; mod feedback;
mod metrics; mod metrics;
mod prompt; mod prompt;
@@ -43,6 +35,8 @@ pub(crate) use approval_request::GuardianMcpAnnotations;
pub(crate) use approval_request::GuardianNetworkAccessTrigger; pub(crate) use approval_request::GuardianNetworkAccessTrigger;
#[cfg(test)] #[cfg(test)]
pub(crate) use approval_request::guardian_approval_request_to_json; 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;
pub(crate) use prompt::BUNDLED_GUARDIAN_POLICY_TEMPLATE; pub(crate) use prompt::BUNDLED_GUARDIAN_POLICY_TEMPLATE;
pub(crate) use prompt::guardian_truncate_text; 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; pub(crate) use review::new_guardian_review_id;
#[cfg(test)] #[cfg(test)]
pub(crate) use review::record_guardian_denial_for_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;
pub(crate) use review::review_approval_request_with_cancel; pub(crate) use review::review_approval_request_with_cancel;
pub(crate) use review::routes_approval_policy_to_guardian; pub(crate) use review::routes_approval_policy_to_guardian;
pub(crate) use review::routes_approval_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::GuardianReviewSessionManager;
pub(crate) use review_session::prompt_cache_key_override_for_review_session; 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_REVIEW_TIMEOUT: Duration = Duration::from_secs(90);
pub(crate) const GUARDIAN_REVIEWER_NAME: &str = "guardian"; 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. /// 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. /// Startup reviewer prewarming intentionally uses turn-only inputs because it has no issuing step.
/// ///
/// MCP elicitation reviews still use turn-only inputs. /// MCP elicitation reviews continue to 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)
#[derive(Clone)] #[derive(Clone)]
pub(crate) struct GuardianReviewContext { pub(crate) struct GuardianReviewContext {
/// The response currently handled in this execution context. /// The response currently handled in this execution context.
+19 -36
View File
@@ -7,7 +7,6 @@ use codex_analytics::GuardianReviewTrackContext;
use codex_analytics::GuardianReviewedAction; use codex_analytics::GuardianReviewedAction;
use codex_analytics::GuardianV2Event; use codex_analytics::GuardianV2Event;
use codex_analytics::GuardianV2EventKind; use codex_analytics::GuardianV2EventKind;
use codex_async_utils::THREAD_STACK_SIZE_BYTES;
use codex_core_plugins::PluginCommandAttribution; use codex_core_plugins::PluginCommandAttribution;
use codex_extension_api::GuardianV2Enabled; use codex_extension_api::GuardianV2Enabled;
use codex_extension_api::ThreadIdleCause; use codex_extension_api::ThreadIdleCause;
@@ -29,10 +28,10 @@ use codex_protocol::protocol::SessionSource;
use codex_protocol::protocol::SubAgentSource; use codex_protocol::protocol::SubAgentSource;
use codex_protocol::protocol::TurnAbortReason; use codex_protocol::protocol::TurnAbortReason;
use codex_protocol::protocol::WarningEvent; use codex_protocol::protocol::WarningEvent;
#[cfg(test)]
use futures::future::BoxFuture; use futures::future::BoxFuture;
use std::sync::Arc; use std::sync::Arc;
use std::time::Duration; use std::time::Duration;
use tokio::sync::oneshot;
use tokio::time::Instant; use tokio::time::Instant;
use tokio::time::sleep_until; use tokio::time::sleep_until;
use tokio_util::sync::CancellationToken; use tokio_util::sync::CancellationToken;
@@ -268,7 +267,7 @@ fn track_guardian_review(
.track_guardian_review(tracking, result, completed_at_ms); .track_guardian_review(tracking, result, completed_at_ms);
} }
async fn record_guardian_non_denial(session: &Arc<Session>, turn_id: &str) { pub(super) async fn record_guardian_non_denial(session: &Arc<Session>, turn_id: &str) {
session session
.services .services
.guardian_rejection_circuit_breaker .guardian_rejection_circuit_breaker
@@ -432,21 +431,17 @@ async fn run_guardian_review(
session, session,
context, context,
review_id, review_id,
request, request: request.into(),
reasons, reasons,
options, options,
}; };
codex_extension_api::SynchronousApprovalReviewer::review( run_synchronous_review(runtime, /*review_reason*/ None).await
&runtime,
codex_protocol::approvals::GuardianReviewReason::Policy,
)
.await
} }
/// Runs the existing synchronous reviewer without choosing policy or reusing a score. /// Runs the existing synchronous reviewer without choosing policy or reusing a score.
pub(super) async fn run_synchronous_review( pub(super) async fn run_synchronous_review(
runtime: super::runtime::ReviewRuntime, runtime: super::runtime::ReviewRuntime,
_review_reason: codex_protocol::approvals::GuardianReviewReason, review_reason: Option<codex_protocol::approvals::GuardianReviewReason>,
) -> ReviewDecision { ) -> ReviewDecision {
let super::runtime::ReviewRuntime { let super::runtime::ReviewRuntime {
session, session,
@@ -456,12 +451,17 @@ pub(super) async fn run_synchronous_review(
reasons, reasons,
options, options,
} = runtime; } = runtime;
let request = match request.validate(&context) {
Ok(request) => request.clone(),
Err(decision) => return decision,
};
let turn = Arc::clone(context.turn()); let turn = Arc::clone(context.turn());
let GuardianReviewOptions { let GuardianReviewOptions {
plugin_attribution_override, plugin_attribution_override,
approval_request_source, approval_request_source,
external_cancel, external_cancel,
require_synchronous_review: _, require_synchronous_review: _,
require_guardian: _,
} = options; } = options;
// Bound executor attribution and the Guardian session with one review deadline. // Bound executor attribution and the Guardian session with one review deadline.
let deadline = Instant::now() + GUARDIAN_REVIEW_TIMEOUT; let deadline = Instant::now() + GUARDIAN_REVIEW_TIMEOUT;
@@ -518,6 +518,7 @@ pub(super) async fn run_synchronous_review(
.send_event( .send_event(
turn.as_ref(), turn.as_ref(),
EventMsg::GuardianAssessment(GuardianAssessmentEvent { EventMsg::GuardianAssessment(GuardianAssessmentEvent {
review_reason,
id: review_id.clone(), id: review_id.clone(),
target_item_id: target_item_id.clone(), target_item_id: target_item_id.clone(),
plugin_id: plugin_id.clone(), plugin_id: plugin_id.clone(),
@@ -557,6 +558,7 @@ pub(super) async fn run_synchronous_review(
.send_event( .send_event(
turn.as_ref(), turn.as_ref(),
EventMsg::GuardianAssessment(GuardianAssessmentEvent { EventMsg::GuardianAssessment(GuardianAssessmentEvent {
review_reason,
id: review_id, id: review_id,
target_item_id, target_item_id,
plugin_id: plugin_id.clone(), plugin_id: plugin_id.clone(),
@@ -696,6 +698,7 @@ pub(super) async fn run_synchronous_review(
.send_event( .send_event(
turn.as_ref(), turn.as_ref(),
EventMsg::GuardianAssessment(GuardianAssessmentEvent { EventMsg::GuardianAssessment(GuardianAssessmentEvent {
review_reason,
id: review_id, id: review_id,
target_item_id, target_item_id,
plugin_id: plugin_id.clone(), plugin_id: plugin_id.clone(),
@@ -733,6 +736,7 @@ pub(super) async fn run_synchronous_review(
.send_event( .send_event(
turn.as_ref(), turn.as_ref(),
EventMsg::GuardianAssessment(GuardianAssessmentEvent { EventMsg::GuardianAssessment(GuardianAssessmentEvent {
review_reason,
id: review_id, id: review_id,
target_item_id, target_item_id,
plugin_id: plugin_id.clone(), plugin_id: plugin_id.clone(),
@@ -818,6 +822,7 @@ pub(super) async fn run_synchronous_review(
GuardianAssessmentStatus::Denied GuardianAssessmentStatus::Denied
}; };
let assessment_event = GuardianAssessmentEvent { let assessment_event = GuardianAssessmentEvent {
review_reason,
id: review_id, id: review_id,
target_item_id, target_item_id,
plugin_id: plugin_id.clone(), plugin_id: plugin_id.clone(),
@@ -879,6 +884,8 @@ pub(super) async fn run_synchronous_review(
#[derive(Clone)] #[derive(Clone)]
pub(crate) struct GuardianReviewOptions { 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<PluginCommandAttribution>, pub(crate) plugin_attribution_override: Option<PluginCommandAttribution>,
pub(crate) approval_request_source: GuardianApprovalRequestSource, pub(crate) approval_request_source: GuardianApprovalRequestSource,
pub(crate) external_cancel: Option<CancellationToken>, pub(crate) external_cancel: Option<CancellationToken>,
@@ -887,6 +894,7 @@ pub(crate) struct GuardianReviewOptions {
} }
/// Public entrypoint for approval requests that should be reviewed by guardian. /// Public entrypoint for approval requests that should be reviewed by guardian.
#[cfg(test)]
pub(crate) async fn review_approval_request( pub(crate) async fn review_approval_request(
session: &Arc<Session>, session: &Arc<Session>,
context: impl Into<GuardianReviewContext>, context: impl Into<GuardianReviewContext>,
@@ -902,6 +910,7 @@ pub(crate) async fn review_approval_request(
request, request,
reasons, reasons,
GuardianReviewOptions { GuardianReviewOptions {
require_guardian: false,
plugin_attribution_override: None, plugin_attribution_override: None,
approval_request_source: GuardianApprovalRequestSource::MainTurn, approval_request_source: GuardianApprovalRequestSource::MainTurn,
external_cancel: None, external_cancel: None,
@@ -933,32 +942,6 @@ pub(crate) async fn review_approval_request_with_cancel(
.await .await
} }
pub(crate) fn spawn_approval_request_review(
session: Arc<Session>,
context: impl Into<GuardianReviewContext>,
review_id: String,
request: GuardianApprovalRequest,
reasons: ApprovalRequestReasons,
options: GuardianReviewOptions,
) -> oneshot::Receiver<ReviewDecision> {
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) struct GuardianReviewSessionConfig {
pub(super) spawn_config: crate::config::Config, pub(super) spawn_config: crate::config::Config,
pub(super) node_repl_policy: GuardianNodeReplPolicy, pub(super) node_repl_policy: GuardianNodeReplPolicy,
+65 -3
View File
@@ -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::ExtensionFuture;
use codex_extension_api::SynchronousApprovalReviewer; use codex_extension_api::SynchronousApprovalReviewer;
@@ -9,15 +9,77 @@ use super::ApprovalRequestReasons;
use super::GuardianApprovalRequest; use super::GuardianApprovalRequest;
use super::GuardianReviewContext; use super::GuardianReviewContext;
use super::GuardianReviewOptions; use super::GuardianReviewOptions;
use super::approval_request::guardian_approval_request_to_json;
use super::review::run_synchronous_review; use super::review::run_synchronous_review;
use crate::session::session::Session; 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<serde_json::Value, String>,
pub(super) category: codex_protocol::openai_models::GuardianScope,
pub(super) request: Result<GuardianApprovalRequest, String>,
}
impl From<GuardianApprovalRequest> 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<codex_utils_path_uri::PathConvention>,
) -> 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)] #[derive(Clone)]
pub(super) struct ReviewRuntime { pub(super) struct ReviewRuntime {
pub(super) session: Arc<Session>, pub(super) session: Arc<Session>,
pub(super) context: GuardianReviewContext, pub(super) context: GuardianReviewContext,
pub(super) review_id: String, pub(super) review_id: String,
pub(super) request: GuardianApprovalRequest, pub(super) request: ReviewAction,
pub(super) reasons: ApprovalRequestReasons, pub(super) reasons: ApprovalRequestReasons,
pub(super) options: GuardianReviewOptions, pub(super) options: GuardianReviewOptions,
} }
@@ -27,6 +89,6 @@ impl SynchronousApprovalReviewer for ReviewRuntime {
&self, &self,
reason: codex_protocol::approvals::GuardianReviewReason, reason: codex_protocol::approvals::GuardianReviewReason,
) -> ExtensionFuture<'_, ReviewDecision> { ) -> ExtensionFuture<'_, ReviewDecision> {
Box::pin(run_synchronous_review(self.clone(), reason)) Box::pin(run_synchronous_review(self.clone(), Some(reason)))
} }
} }
+1
View File
@@ -1696,6 +1696,7 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() {
}, },
/*retry_reason*/ None, /*retry_reason*/ None,
GuardianReviewOptions { GuardianReviewOptions {
require_guardian: false,
plugin_attribution_override: None, plugin_attribution_override: None,
approval_request_source: GuardianApprovalRequestSource::MainTurn, approval_request_source: GuardianApprovalRequestSource::MainTurn,
external_cancel: Some(cancel_token), external_cancel: Some(cancel_token),
+2
View File
@@ -891,6 +891,7 @@ async fn review_guardian_mcp_elicitation(
guardian_request, guardian_request,
/*retry_reason*/ None, /*retry_reason*/ None,
crate::guardian::GuardianReviewOptions { crate::guardian::GuardianReviewOptions {
require_guardian: false,
plugin_attribution_override: None, plugin_attribution_override: None,
approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn, approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn,
external_cancel: Some(cancellation_token), external_cancel: Some(cancellation_token),
@@ -984,6 +985,7 @@ async fn review_guardian_mcp_elicitation(
guardian_request, guardian_request,
/*retry_reason*/ None, /*retry_reason*/ None,
crate::guardian::GuardianReviewOptions { crate::guardian::GuardianReviewOptions {
require_guardian: false,
plugin_attribution_override: None, plugin_attribution_override: None,
approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn, approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn,
external_cancel: Some(cancellation_token), external_cancel: Some(cancellation_token),
+47 -50
View File
@@ -2780,7 +2780,6 @@ impl Session {
) -> Option<RequestPermissionsResponse> { ) -> Option<RequestPermissionsResponse> {
let turn_context = &step_context.turn; let turn_context = &step_context.turn;
let approval_policy = step_context.settings.approval_policy(); let approval_policy = step_context.settings.approval_policy();
let approvals_reviewer = step_context.settings.approvals_reviewer();
let Some(environment) = step_context let Some(environment) = step_context
.environments .environments
.turn_environments() .turn_environments()
@@ -2827,7 +2826,6 @@ impl Session {
strict_auto_review: false, strict_auto_review: false,
}); });
}; };
if crate::guardian::routes_approval_policy_to_guardian(approval_policy, approvals_reviewer)
{ {
let originating_turn_state = { let originating_turn_state = {
let active = self.active_turn.lock().await; let active = self.active_turn.lock().await;
@@ -2836,13 +2834,13 @@ impl Session {
let action = ApprovalAction::RequestPermissions { let action = ApprovalAction::RequestPermissions {
id: call_id.clone(), id: call_id.clone(),
turn_id: turn_context.sub_id.clone(), turn_id: turn_context.sub_id.clone(),
reason: args.reason, reason: args.reason.clone(),
permissions: requested_permissions.clone(), permissions: requested_permissions.clone(),
}; };
let approval_context = ApprovalContext { let approval_context = ApprovalContext {
review_context: crate::guardian::GuardianReviewContext::from(step_context), review_context: crate::guardian::GuardianReviewContext::from(step_context),
cancellation_token: Some(cancellation_token.clone()), cancellation_token: Some(cancellation_token.clone()),
call_id, call_id: call_id.clone(),
tool_name: ToolName::plain("request_permissions"), tool_name: ToolName::plain("request_permissions"),
strict_auto_review: false, strict_auto_review: false,
approval_reason: None, approval_reason: None,
@@ -2857,54 +2855,53 @@ impl Session {
&approval_context, &approval_context,
) => decision, ) => decision,
}; };
let response = match decision { if let Some(decision) = decision {
ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } => { let (permissions, scope) = match decision {
RequestPermissionsResponse { ReviewDecision::Approved
permissions: requested_permissions.clone(), | ReviewDecision::ApprovedExecpolicyAmendment { .. }
scope: PermissionGrantScope::Turn, | ReviewDecision::NetworkPolicyAmendment {
strict_auto_review: false, network_policy_amendment:
NetworkPolicyAmendment {
action: NetworkPolicyRuleAction::Allow,
..
},
} => (requested_permissions.clone(), PermissionGrantScope::Turn),
ReviewDecision::ApprovedForSession => {
(requested_permissions.clone(), PermissionGrantScope::Session)
} }
} ReviewDecision::ApprovedMcpPolicyAmendment
ReviewDecision::ApprovedForSession => RequestPermissionsResponse { | ReviewDecision::NetworkPolicyAmendment {
permissions: requested_permissions.clone(), network_policy_amendment:
scope: PermissionGrantScope::Session, NetworkPolicyAmendment {
action: NetworkPolicyRuleAction::Deny,
..
},
}
| ReviewDecision::Abort
| ReviewDecision::Denied { .. }
| ReviewDecision::TimedOut => (
RequestPermissionProfile::default(),
PermissionGrantScope::Turn,
),
};
let response = RequestPermissionsResponse {
permissions,
scope,
strict_auto_review: false, strict_auto_review: false,
}, };
ReviewDecision::NetworkPolicyAmendment { let response = Self::normalize_request_permissions_response(
network_policy_amendment, requested_permissions,
} => match network_policy_amendment.action { response,
NetworkPolicyRuleAction::Allow => RequestPermissionsResponse { &context,
permissions: requested_permissions.clone(), );
scope: PermissionGrantScope::Turn, self.record_granted_request_permissions_for_turn(
strict_auto_review: false, &response,
}, &environment.selection.environment_id,
NetworkPolicyRuleAction::Deny => RequestPermissionsResponse { originating_turn_state.as_ref(),
permissions: RequestPermissionProfile::default(), )
scope: PermissionGrantScope::Turn, .await;
strict_auto_review: false, return Some(response);
}, }
},
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 _elicitation = self.services.elicitations.register(); let _elicitation = self.services.elicitations.register();
+73 -110
View File
@@ -5,12 +5,10 @@ use crate::exec_policy::prompt_is_rejected_by_policy;
use crate::guardian::GuardianNetworkAccessTrigger; use crate::guardian::GuardianNetworkAccessTrigger;
use crate::guardian::GuardianReviewContext; use crate::guardian::GuardianReviewContext;
use crate::guardian::GuardianReviewOptions; use crate::guardian::GuardianReviewOptions;
use crate::guardian::decide_approval;
use crate::guardian::guardian_timeout_message; use crate::guardian::guardian_timeout_message;
use crate::guardian::new_guardian_review_id; use crate::guardian::new_guardian_review_id;
use crate::guardian::review_approval_request; use crate::guardian::spawn_approval_decision;
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::hook_runtime::run_permission_request_hooks; use crate::hook_runtime::run_permission_request_hooks;
use crate::mcp_tool_call::request_mcp_tool_user_approval; use crate::mcp_tool_call::request_mcp_tool_user_approval;
use crate::sandboxing::SandboxPermissions; use crate::sandboxing::SandboxPermissions;
@@ -64,12 +62,14 @@ pub(crate) struct ApprovalContext {
pub(crate) network_approval_context: Option<NetworkApprovalContext>, pub(crate) network_approval_context: Option<NetworkApprovalContext>,
} }
#[derive(Clone, Debug, PartialEq)] #[derive(Clone, Debug, PartialEq, serde::Serialize)]
#[serde(tag = "tool", rename_all = "snake_case")]
pub(crate) enum ApprovalAction { pub(crate) enum ApprovalAction {
ExecCommand { ExecCommand {
id: String, id: String,
environment_id: String, environment_id: String,
command: Vec<String>, command: Vec<String>,
#[serde(skip_serializing)]
hook_command: String, hook_command: String,
cwd: PathUri, cwd: PathUri,
sandbox_permissions: SandboxPermissions, sandbox_permissions: SandboxPermissions,
@@ -107,6 +107,7 @@ pub(crate) enum ApprovalAction {
cwd: PathUri, cwd: PathUri,
files: Vec<PathUri>, files: Vec<PathUri>,
patch: String, patch: String,
#[serde(skip_serializing)]
changes: Arc<HashMap<PathBuf, FileChange>>, changes: Arc<HashMap<PathBuf, FileChange>>,
permissions_preapproved: bool, permissions_preapproved: bool,
}, },
@@ -122,6 +123,7 @@ pub(crate) enum ApprovalAction {
tool_title: Option<String>, tool_title: Option<String>,
tool_description: Option<String>, tool_description: Option<String>,
annotations: Option<crate::guardian::GuardianMcpAnnotations>, annotations: Option<crate::guardian::GuardianMcpAnnotations>,
#[serde(skip_serializing)]
hook_tool_name: HookToolName, hook_tool_name: HookToolName,
approval_policy: AskForApproval, approval_policy: AskForApproval,
reviewer: ApprovalsReviewer, reviewer: ApprovalsReviewer,
@@ -138,7 +140,9 @@ pub(crate) enum ApprovalAction {
protocol: NetworkApprovalProtocol, protocol: NetworkApprovalProtocol,
port: u16, port: u16,
trigger: Option<GuardianNetworkAccessTrigger>, trigger: Option<GuardianNetworkAccessTrigger>,
#[serde(skip_serializing)]
hook_command: String, hook_command: String,
#[serde(skip_serializing)]
hook_run_id: String, hook_run_id: String,
command: Vec<String>, command: Vec<String>,
cwd: AbsolutePathBuf, 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)] #[derive(Clone, Copy, Debug, PartialEq, Eq)]
enum ApprovalResolutionSource { enum ApprovalResolutionSource {
Hook, Hook,
@@ -556,51 +544,33 @@ impl Session {
action: ApprovalAction, action: ApprovalAction,
ctx: &ApprovalContext, ctx: &ApprovalContext,
) -> ApprovalResolution { ) -> ApprovalResolution {
let reviewer = if ctx.strict_auto_review { if let Some(decision) = self.request_guardian_approval(action.clone(), ctx).await {
ApprovalReviewer::Guardian ApprovalResolution {
} else if let ApprovalAction::McpToolCall { decision,
approval_policy, source: ApprovalResolutionSource::Guardian,
reviewer, }
..
} = &action
{
ApprovalReviewer::for_policy(*approval_policy, *reviewer)
} else { } else {
ApprovalReviewer::for_policy( ApprovalResolution {
ctx.review_context.approval_policy, decision: self.request_user_approval(&action, ctx).await,
ctx.review_context.approvals_reviewer, source: ApprovalResolutionSource::User,
) }
}; }
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 }
} }
pub(crate) async fn request_guardian_approval( pub(crate) async fn request_guardian_approval(
self: &Arc<Self>, self: &Arc<Self>,
action: ApprovalAction, action: ApprovalAction,
ctx: &ApprovalContext, ctx: &ApprovalContext,
) -> ReviewDecision { ) -> Option<ReviewDecision> {
// Guardian inherits only the current turn's ready environments. A retained let mut context = ctx.review_context.clone();
// terminal handle may outlive its selection, but must not be reviewed in if let ApprovalAction::McpToolCall {
// a different environment that happens to have the same launch directory. approval_policy,
if let ApprovalAction::WriteStdin { environment_id, .. } = &action reviewer,
&& !ctx ..
.review_context } = &action
.environments()
.turn_environments()
.any(|environment| environment.selection.environment_id == *environment_id)
{ {
return ReviewDecision::denied( context.approval_policy = *approval_policy;
"automatic approval review cannot access the terminal's environment; select it before retrying", context.approvals_reviewer = *reviewer;
);
} }
let is_network_approval = matches!(&action, ApprovalAction::NetworkAccess { .. }); let is_network_approval = matches!(&action, ApprovalAction::NetworkAccess { .. });
let review_id = new_guardian_review_id(); let review_id = new_guardian_review_id();
@@ -628,74 +598,67 @@ impl Session {
), ),
_ => None, _ => None,
}; };
let action = match action.into_guardian_request(exec_command_cwd_convention) { let action = crate::guardian::ReviewAction::from_approval_action(
Ok(action) => action, action,
Err(err) => { exec_command_cwd_convention,
tracing::error!(%err, "failed to build automatic approval action"); );
return ReviewDecision::denied(
"automatic approval review could not prepare the action",
);
}
};
if let Some(cancellation_token) = &ctx.cancellation_token { let reasons = ApprovalRequestReasons {
let review = spawn_approval_request_review( 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), Arc::clone(self),
ctx.review_context.clone(), context,
review_id, review_id,
action, action,
ApprovalRequestReasons { reasons,
approval: ctx.approval_reason.clone(), options,
retry: ctx.retry_reason.clone(), )
}, .await
GuardianReviewOptions { .unwrap_or_else(|_| {
plugin_attribution_override: None, Some(ReviewDecision::denied(
approval_request_source: GuardianApprovalRequestSource::MainTurn, "automatic approval review could not complete",
external_cancel: Some(cancellation_token.clone()), ))
require_synchronous_review: false,
},
);
review.await.unwrap_or_else(|_| {
ReviewDecision::denied("automatic approval review could not complete")
}) })
} else if is_network_approval { } else if is_network_approval {
let review_cancel = CancellationToken::new(); let review_cancel = CancellationToken::new();
let review_cancel_guard = review_cancel.clone().drop_guard(); let review_cancel_guard = review_cancel.clone().drop_guard();
let review_session = Arc::clone(self); let review = tokio::spawn(decide_approval(
let review_context = ctx.review_context.clone(); Arc::clone(self),
let retry_reason = ctx.retry_reason.clone(); context,
let review = tokio::spawn(async move { review_id,
review_approval_request_with_cancel( action,
&review_session, reasons,
review_context, GuardianReviewOptions {
review_id, external_cancel: Some(review_cancel),
action, ..options
retry_reason, },
GuardianReviewOptions { ));
plugin_attribution_override: None,
approval_request_source: GuardianApprovalRequestSource::MainTurn,
external_cancel: Some(review_cancel),
require_synchronous_review: false,
},
)
.await
});
let decision = review.await.unwrap_or_else(|err| { let decision = review.await.unwrap_or_else(|err| {
warn!("network Guardian review task failed: {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()); drop(review_cancel_guard.disarm());
decision decision
} else { } else {
review_approval_request( decide_approval(
self, Arc::clone(self),
ctx.review_context.clone(), context,
review_id, review_id,
action, action,
ApprovalRequestReasons { reasons,
approval: ctx.approval_reason.clone(), options,
retry: ctx.retry_reason.clone(),
},
) )
.await .await
} }
+103
View File
@@ -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<ApprovalDecision>> {
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] #[tokio::test]
async fn explicit_mcp_reviewer_override_takes_precedence_over_action_context() { async fn explicit_mcp_reviewer_override_takes_precedence_over_action_context() {
let (session, turn, events) = make_session_and_context_with_rx().await; let (session, turn, events) = make_session_and_context_with_rx().await;
+11 -20
View File
@@ -7,9 +7,7 @@ use anyhow::Result;
use codex_config::test_support::CloudConfigBundleFixture; use codex_config::test_support::CloudConfigBundleFixture;
use codex_core::config::Constrained; use codex_core::config::Constrained;
use codex_extension_api::ApprovalReviewContributor; use codex_extension_api::ApprovalReviewContributor;
use codex_extension_api::ExtensionData;
use codex_extension_api::ExtensionFuture; use codex_extension_api::ExtensionFuture;
use codex_extension_api::ExtensionMetrics;
use codex_extension_api::ExtensionRegistryBuilder; use codex_extension_api::ExtensionRegistryBuilder;
use codex_features::Feature; use codex_features::Feature;
use codex_login::CodexAuth; 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::AskForApproval;
use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::Op; use codex_protocol::protocol::Op;
use codex_protocol::protocol::ReviewDecision;
use codex_protocol::protocol::ThreadSettingsOverrides; use codex_protocol::protocol::ThreadSettingsOverrides;
use codex_protocol::request_permissions::PermissionGrantScope; use codex_protocol::request_permissions::PermissionGrantScope;
use codex_protocol::request_permissions::RequestPermissionProfile; use codex_protocol::request_permissions::RequestPermissionProfile;
@@ -65,17 +62,14 @@ use wiremock::matchers::path;
struct ApprovedReviewContributor; struct ApprovedReviewContributor;
impl ApprovalReviewContributor for ApprovedReviewContributor { impl ApprovalReviewContributor for ApprovedReviewContributor {
fn fast_decision<'a>( fn decide<'a>(
&'a self, &'a self,
_session_store: &'a ExtensionData, input: &'a codex_extension_api::ApprovalDecisionInput<'_>,
_thread_store: &'a ExtensionData, ) -> ExtensionFuture<'a, Option<codex_extension_api::ApprovalDecision>> {
prompt: &'a str,
extension_metrics: Option<Arc<dyn ExtensionMetrics>>,
) -> ExtensionFuture<'a, Option<ReviewDecision>> {
Box::pin(async move { Box::pin(async move {
assert!(extension_metrics.is_some()); assert!(input.metrics.is_some());
assert!(prompt.contains("\"tool\":\"request_permissions\"")); assert_eq!(input.action["tool"], "request_permissions");
Some(ReviewDecision::Approved) Some(codex_extension_api::ApprovalDecision::Allow)
}) })
} }
} }
@@ -83,16 +77,13 @@ impl ApprovalReviewContributor for ApprovedReviewContributor {
struct EscalationApprovingReviewContributor; struct EscalationApprovingReviewContributor;
impl ApprovalReviewContributor for EscalationApprovingReviewContributor { impl ApprovalReviewContributor for EscalationApprovingReviewContributor {
fn fast_decision<'a>( fn decide<'a>(
&'a self, &'a self,
_session_store: &'a ExtensionData, input: &'a codex_extension_api::ApprovalDecisionInput<'_>,
_thread_store: &'a ExtensionData, ) -> ExtensionFuture<'a, Option<codex_extension_api::ApprovalDecision>> {
prompt: &'a str,
_extension_metrics: Option<Arc<dyn ExtensionMetrics>>,
) -> ExtensionFuture<'a, Option<ReviewDecision>> {
Box::pin(async move { Box::pin(async move {
assert!(prompt.contains("\"sandbox_permissions\":\"require_escalated\"")); assert_eq!(input.action["sandbox_permissions"], "require_escalated");
Some(ReviewDecision::Approved) Some(codex_extension_api::ApprovalDecision::Allow)
}) })
} }
} }
@@ -75,6 +75,7 @@ async fn first_request_item_types_roles_and_content_annotations() -> Result<()>
test.codex test.codex
.submit(Op::ApproveGuardianDeniedAction { .submit(Op::ApproveGuardianDeniedAction {
event: GuardianAssessmentEvent { event: GuardianAssessmentEvent {
review_reason: None,
id: "guardian-review".to_string(), id: "guardian-review".to_string(),
target_item_id: None, target_item_id: None,
plugin_id: None, plugin_id: None,
+9 -7
View File
@@ -1561,14 +1561,16 @@ async fn guardian_timeout_rejects_tool_call_with_acting_model_instructions(
struct TimedOutReviewContributor; struct TimedOutReviewContributor;
impl codex_extension_api::ApprovalReviewContributor for TimedOutReviewContributor { impl codex_extension_api::ApprovalReviewContributor for TimedOutReviewContributor {
fn fast_decision<'a>( fn decide<'a>(
&'a self, &'a self,
_session_store: &'a codex_extension_api::ExtensionData, _input: &'a codex_extension_api::ApprovalDecisionInput<'_>,
_thread_store: &'a codex_extension_api::ExtensionData, ) -> codex_extension_api::ExtensionFuture<'a, Option<codex_extension_api::ApprovalDecision>>
_prompt: &'a str, {
_extension_metrics: Option<Arc<dyn codex_extension_api::ExtensionMetrics>>, Box::pin(async {
) -> codex_extension_api::ExtensionFuture<'a, Option<ReviewDecision>> { Some(codex_extension_api::ApprovalDecision::Reviewed(
Box::pin(async { Some(ReviewDecision::TimedOut) }) ReviewDecision::TimedOut,
))
})
} }
} }
@@ -5,6 +5,7 @@ use super::authorization::ScoreAuthorization;
use super::config::GuardianV2Config; use super::config::GuardianV2Config;
use super::coverage::GuardianPolicy; use super::coverage::GuardianPolicy;
use super::extension::GuardianV2ScoreProgress; use super::extension::GuardianV2ScoreProgress;
use super::extension::encrypted_parent_compaction;
use super::extension::requires_sync_for_compaction; use super::extension::requires_sync_for_compaction;
use super::metrics::TOOL_CALL_LAG_METRIC; use super::metrics::TOOL_CALL_LAG_METRIC;
use super::metrics::record_fast_decision; use super::metrics::record_fast_decision;
@@ -139,7 +140,10 @@ async fn cached_evidence(
.get::<LunaSampler>() .get::<LunaSampler>()
.ok_or(GuardianReviewReason::MissingScore)?; .ok_or(GuardianReviewReason::MissingScore)?;
let history = thread.conversation_history_snapshot().await; 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"); record_fast_decision(metrics, "deferred", "incompatible_compaction");
return Err(GuardianReviewReason::IncompatibleCompaction); return Err(GuardianReviewReason::IncompatibleCompaction);
} }
@@ -987,7 +987,7 @@ impl GuardianV2Extension {
} }
#[derive(Debug, Eq, PartialEq)] #[derive(Debug, Eq, PartialEq)]
enum ParentCompactionError { pub(super) enum ParentCompactionError {
Serialization, Serialization,
Oversized, Oversized,
Unusable, 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 // 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. // 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<Item = &'a ResponseItem>, items: impl Iterator<Item = &'a ResponseItem>,
max_parent_compaction_tokens: usize, max_parent_compaction_tokens: usize,
) -> Result<Option<ResponseItem>, ParentCompactionError> { ) -> Result<Option<ResponseItem>, ParentCompactionError> {
+4
View File
@@ -204,6 +204,10 @@ pub enum GuardianReviewReason {
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, JsonSchema, TS)] #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, JsonSchema, TS)]
pub struct GuardianAssessmentEvent { 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<GuardianReviewReason>,
/// Stable identifier for this guardian review lifecycle. /// Stable identifier for this guardian review lifecycle.
pub id: String, pub id: String,
/// Thread item being reviewed, when the review maps to a concrete item. /// Thread item being reviewed, when the review maps to a concrete item.
+1
View File
@@ -94,6 +94,7 @@ mod tests {
fn denied_event(id: usize) -> GuardianAssessmentEvent { fn denied_event(id: usize) -> GuardianAssessmentEvent {
GuardianAssessmentEvent { GuardianAssessmentEvent {
review_reason: None,
id: format!("review-{id}"), id: format!("review-{id}"),
target_item_id: None, target_item_id: None,
plugin_id: None, plugin_id: None,
@@ -95,6 +95,7 @@ impl ChatWidget {
}; };
self.on_guardian_assessment(GuardianAssessmentEvent { self.on_guardian_assessment(GuardianAssessmentEvent {
review_reason: None,
id, id,
target_item_id: None, target_item_id: None,
plugin_id: None, plugin_id: None,
@@ -3,6 +3,7 @@ use pretty_assertions::assert_eq;
fn auto_review_denial_event() -> GuardianAssessmentEvent { fn auto_review_denial_event() -> GuardianAssessmentEvent {
GuardianAssessmentEvent { GuardianAssessmentEvent {
review_reason: None,
id: "auto-review-recent-1".into(), id: "auto-review-recent-1".into(),
target_item_id: Some("target-auto-review-recent-1".into()), target_item_id: Some("target-auto-review-recent-1".into()),
plugin_id: None, plugin_id: None,
@@ -31,6 +32,7 @@ fn guardian_command_event(
) -> GuardianAssessmentEvent { ) -> GuardianAssessmentEvent {
let terminal = status != GuardianAssessmentStatus::InProgress; let terminal = status != GuardianAssessmentStatus::InProgress;
GuardianAssessmentEvent { GuardianAssessmentEvent {
review_reason: None,
id: id.to_string(), id: id.to_string(),
target_item_id: Some(format!("{id}-target")), target_item_id: Some(format!("{id}-target")),
plugin_id: None, plugin_id: None,
@@ -244,6 +246,7 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() {
}; };
chat.on_guardian_assessment(GuardianAssessmentEvent { chat.on_guardian_assessment(GuardianAssessmentEvent {
review_reason: None,
id: "guardian-1".into(), id: "guardian-1".into(),
target_item_id: Some("guardian-target-1".into()), target_item_id: Some("guardian-target-1".into()),
plugin_id: None, 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_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 { chat.on_guardian_assessment(GuardianAssessmentEvent {
review_reason: None,
id: "guardian-1".into(), id: "guardian-1".into(),
target_item_id: Some("guardian-target-1".into()), target_item_id: Some("guardian-target-1".into()),
plugin_id: None, plugin_id: None,
@@ -313,6 +317,7 @@ async fn guardian_approved_exec_is_hidden_from_history() {
/*replay_kind*/ None, /*replay_kind*/ None,
); );
chat.on_guardian_assessment(GuardianAssessmentEvent { chat.on_guardian_assessment(GuardianAssessmentEvent {
review_reason: None,
id: "thread:child-thread:guardian-1".into(), id: "thread:child-thread:guardian-1".into(),
target_item_id: Some("guardian-approved-target".into()), target_item_id: Some("guardian-approved-target".into()),
plugin_id: None, plugin_id: None,
@@ -370,6 +375,7 @@ async fn guardian_approved_request_permissions_clears_status_without_history() {
}; };
chat.on_guardian_assessment(GuardianAssessmentEvent { chat.on_guardian_assessment(GuardianAssessmentEvent {
review_reason: None,
id: "guardian-request-permissions".into(), id: "guardian-request-permissions".into(),
target_item_id: None, target_item_id: None,
plugin_id: None, plugin_id: None,
@@ -396,6 +402,7 @@ async fn guardian_approved_request_permissions_clears_status_without_history() {
); );
chat.on_guardian_assessment(GuardianAssessmentEvent { chat.on_guardian_assessment(GuardianAssessmentEvent {
review_reason: None,
id: "guardian-request-permissions".into(), id: "guardian-request-permissions".into(),
target_item_id: None, target_item_id: None,
plugin_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 { chat.on_guardian_assessment(GuardianAssessmentEvent {
review_reason: None,
id: "guardian-1".into(), id: "guardian-1".into(),
target_item_id: Some("guardian-target-1".into()), target_item_id: Some("guardian-target-1".into()),
plugin_id: None, 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_warning("Automatic approval review timed out while evaluating the requested approval.");
chat.on_guardian_assessment(GuardianAssessmentEvent { chat.on_guardian_assessment(GuardianAssessmentEvent {
review_reason: None,
id: "guardian-1".into(), id: "guardian-1".into(),
target_item_id: Some("guardian-target-1".into()), target_item_id: Some("guardian-target-1".into()),
plugin_id: None, plugin_id: None,
@@ -534,6 +534,7 @@ async fn mcp_startup_complete_preserves_review_status() {
); );
chat.on_guardian_assessment(GuardianAssessmentEvent { chat.on_guardian_assessment(GuardianAssessmentEvent {
review_reason: None,
id: "guardian-1".to_string(), id: "guardian-1".to_string(),
target_item_id: Some("guardian-target-1".to_string()), target_item_id: Some("guardian-target-1".to_string()),
plugin_id: None, plugin_id: None,