diff --git a/codex-rs/analytics/src/events.rs b/codex-rs/analytics/src/events.rs index 5306749b5c..2cb33836b3 100644 --- a/codex-rs/analytics/src/events.rs +++ b/codex-rs/analytics/src/events.rs @@ -538,6 +538,9 @@ pub struct GuardianReviewAnalyticsResult { pub terminal_status: GuardianReviewTerminalStatus, pub failure_reason: Option, pub attempt_count: i64, + /// An allow was validated against changed parent/root authorization or review context. + /// Retained across attempts; this does not identify the cause of a later cancellation. + pub context_changed: bool, pub risk_level: Option, pub user_authorization: Option, pub outcome: Option, @@ -564,6 +567,7 @@ impl GuardianReviewAnalyticsResult { terminal_status: GuardianReviewTerminalStatus::FailedClosed, failure_reason: None, attempt_count: 1, + context_changed: false, risk_level: None, user_authorization: None, outcome: None, diff --git a/codex-rs/core/src/guardian/review_request.rs b/codex-rs/core/src/guardian/review_request.rs index 43bfae0312..613b7efb61 100644 --- a/codex-rs/core/src/guardian/review_request.rs +++ b/codex-rs/core/src/guardian/review_request.rs @@ -34,12 +34,19 @@ pub(in crate::guardian) struct ReviewAuthorization { history_reset: CancellationToken, } +/// Keeps observed context changes separate from the invalidation outcome. +/// A concurrent stop or history reset can take precedence over changed context. +pub(in crate::guardian) struct ReviewInvalidation { + pub(in crate::guardian) outcome: codex_guardian_reviewer::GuardianReviewSessionOutcome, + pub(in crate::guardian) context_changed: bool, +} + impl ReviewAuthorization { pub(in crate::guardian) async fn invalidation( &self, session: &Session, cancellation: Option<&CancellationToken>, - ) -> Option { + ) -> Option { use codex_guardian_reviewer::GuardianReviewSessionOutcome; let root_snapshot = session @@ -63,7 +70,7 @@ impl ReviewAuthorization { let review_context_changed = root_review_changed || self.review_context_revision != history.guardian_review_context_revision(); // An actual stop or history reset wins over a concurrent evidence change. - if self.history_reset.is_cancelled() + let outcome = if self.history_reset.is_cancelled() || cancellation.is_some_and(CancellationToken::is_cancelled) || root_history_changed { @@ -78,7 +85,11 @@ impl ReviewAuthorization { Some(GuardianReviewSessionOutcome::StaleAuthorization) } else { None - } + }; + outcome.map(|outcome| ReviewInvalidation { + outcome, + context_changed: authorization_changed || review_context_changed, + }) } } diff --git a/codex-rs/core/src/guardian/review_session_setup.rs b/codex-rs/core/src/guardian/review_session_setup.rs index ce3fd4bd13..ed1447f317 100644 --- a/codex-rs/core/src/guardian/review_session_setup.rs +++ b/codex-rs/core/src/guardian/review_session_setup.rs @@ -314,13 +314,15 @@ impl ReviewerRequest for PreparedReview { assessment.outcome == super::super::GuardianAssessmentOutcome::Allow }) && let Some(authorization) = &self.params.authorization - && let Some(outcome) = authorization + && let Some(invalidation) = authorization .invalidation( &self.params.parent_session, self.params.external_cancel.as_ref(), ) .await { + let outcome = invalidation.outcome; + result.analytics.context_changed |= invalidation.context_changed; result.disposition = if matches!(outcome, GuardianReviewSessionOutcome::StaleAuthorization) && result.disposition == SessionDisposition::Reusable @@ -378,7 +380,9 @@ pub(crate) async fn run_guardian_review_session( Arc::make_mut(&mut prepared.context).reviewer_selection = ReviewerSelection::FreshParentCheckpoint; prepared.recovery_requested = Arc::default(); - pool.review(prepared).await + let (outcome, mut analytics) = pool.review(prepared).await; + analytics.context_changed |= result.1.context_changed; + (outcome, analytics) } else { result } diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 991ad9083c..0cc6b1fc4a 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -16,6 +16,7 @@ use crate::session::session::Session; use crate::session::step_context::StepContext; use crate::session::turn_context::TurnContext; use crate::tools::ApprovalContext; +use crate::tools::ApprovalResolutionSource; use crate::tools::context::ToolCallOrigin; use crate::tools::hook_names::HookToolName; use crate::tools::lifecycle::process_mcp_tool_result; @@ -275,7 +276,7 @@ pub(crate) async fn handle_mcp_tool_call( } else { McpToolApprovalPolicy::for_server(approval_mode) }; - if let Some(decision) = maybe_request_mcp_tool_approval( + if let Some(approval) = maybe_request_mcp_tool_approval( &sess, step_context, cancellation_token, @@ -290,7 +291,9 @@ pub(crate) async fn handle_mcp_tool_call( ) .await { - let result = match decision { + let cancellation_outcome = matches!(approval.decision, ReviewDecision::Abort) + .then(|| McpCallMetricOutcome::approval_cancelled(approval.source)); + let result = match approval.decision { decision @ (ReviewDecision::Approved | ReviewDecision::ApprovedForSession | ReviewDecision::ApprovedMcpPolicyAmendment @@ -356,7 +359,8 @@ pub(crate) async fn handle_mcp_tool_call( }; let status = if result.is_ok() { "ok" } else { "error" }; - let outcome = McpCallMetricOutcome::from_status(status); + let outcome = + cancellation_outcome.unwrap_or_else(|| McpCallMetricOutcome::from_status(status)); emit_mcp_call_metrics( &step_context.session_telemetry, &outcome, @@ -1467,6 +1471,12 @@ fn mcp_tool_approval_prompt_options( } } +#[derive(Debug, PartialEq, Eq)] +struct McpToolApproval { + decision: ReviewDecision, + source: Option, +} + #[expect(clippy::too_many_arguments)] async fn maybe_request_mcp_tool_approval( sess: &Arc, @@ -1480,7 +1490,7 @@ async fn maybe_request_mcp_tool_approval( config: &codex_mcp::McpConfig, permission_profile: &PermissionProfile, policy: McpToolApprovalPolicy, -) -> Option { +) -> Option { let turn_context = &step_context.turn; let strict_auto_review = turn_context.strict_auto_review_enabled(); let approvals_reviewer = connectors::mcp_approvals_reviewer_from_layers( @@ -1522,7 +1532,10 @@ async fn maybe_request_mcp_tool_approval( && let Some(key) = session_approval_key.as_ref() && mcp_tool_approval_is_remembered(sess, key).await { - return Some(ReviewDecision::Approved); + return Some(McpToolApproval { + decision: ReviewDecision::Approved, + source: None, + }); } let action = ApprovalAction::McpToolCall { @@ -1563,13 +1576,15 @@ async fn maybe_request_mcp_tool_approval( retry_reason: None, network_approval_context: None, }; - Some( - match sess.request_approval(action, approval_context).await { + let approval = sess.request_approval(action, approval_context).await; + Some(McpToolApproval { + decision: match approval.result { Ok(decision) => decision, Err(ToolError::Rejected(rejection)) => ReviewDecision::denied(rejection), Err(ToolError::Codex(_)) => ReviewDecision::Abort, }, - ) + source: approval.source, + }) } pub(crate) async fn request_mcp_tool_user_approval( diff --git a/codex-rs/core/src/mcp_tool_call/telemetry.rs b/codex-rs/core/src/mcp_tool_call/telemetry.rs index 1614e88a72..76aadd4ef6 100644 --- a/codex-rs/core/src/mcp_tool_call/telemetry.rs +++ b/codex-rs/core/src/mcp_tool_call/telemetry.rs @@ -7,6 +7,8 @@ use codex_protocol::mcp::CallToolResult; use serde_json::Value as JsonValue; use tracing::Span; +use crate::tools::ApprovalResolutionSource; + const MCP_CALL_COUNT_METRIC: &str = "codex.mcp.call"; const MCP_CALL_DURATION_METRIC: &str = "codex.mcp.call.duration_ms"; const MCP_CALL_ERROR_COUNT_METRIC: &str = "codex.mcp.call.error"; @@ -25,6 +27,7 @@ pub(super) struct McpCallMetricOutcome { status: &'static str, error_type: Option<&'static str>, error_code: Option, + approval_source: Option<&'static str>, } impl McpCallMetricOutcome { @@ -33,6 +36,21 @@ impl McpCallMetricOutcome { status, error_type: None, error_code: None, + approval_source: None, + } + } + + pub(super) fn approval_cancelled(source: Option) -> Self { + Self { + status: "error", + error_type: Some("approval"), + error_code: Some("cancelled".to_string()), + approval_source: Some(match source { + Some(ApprovalResolutionSource::Guardian) => "guardian", + Some(ApprovalResolutionSource::User) => "user", + Some(ApprovalResolutionSource::Hook) => "hook", + None => "unknown", + }), } } } @@ -69,6 +87,9 @@ pub(super) fn emit_mcp_call_metrics( let mut error_tags = tags; error_tags.push(("error_type", sanitize_metric_tag_value(error_type))); error_tags.push(("error_code", error_code.to_string())); + if let Some(approval_source) = outcome.approval_source { + error_tags.push(("approval_source", approval_source.to_string())); + } let error_tag_refs: Vec<(&str, &str)> = error_tags .iter() .map(|(key, value)| (*key, value.as_str())) @@ -138,6 +159,7 @@ pub(super) fn mcp_call_metric_outcome( status: "error", error_type: Some(MCP_CALL_ERROR_TYPE_TOOL_RESULT), error_code: Some(sanitize_metric_tag_value(&error_code)), + approval_source: None, } } Ok(_) => McpCallMetricOutcome::from_status("ok"), @@ -145,6 +167,7 @@ pub(super) fn mcp_call_metric_outcome( status: "error", error_type: Some(MCP_CALL_ERROR_TYPE_MCP_REQUEST), error_code: Some(MCP_CALL_ERROR_CODE_UNKNOWN.to_string()), + approval_source: None, }, } } diff --git a/codex-rs/core/src/mcp_tool_call/telemetry_tests.rs b/codex-rs/core/src/mcp_tool_call/telemetry_tests.rs index 8b17d5f6b2..80b67e167d 100644 --- a/codex-rs/core/src/mcp_tool_call/telemetry_tests.rs +++ b/codex-rs/core/src/mcp_tool_call/telemetry_tests.rs @@ -43,6 +43,7 @@ fn mcp_call_metric_outcome_distinguishes_request_and_tool_errors() { status: "ok", error_type: None, error_code: None, + approval_source: None, } ); assert_eq!( @@ -54,6 +55,7 @@ fn mcp_call_metric_outcome_distinguishes_request_and_tool_errors() { status: "error", error_type: Some(MCP_CALL_ERROR_TYPE_TOOL_RESULT), error_code: Some("RATE_LIMITED".to_string()), + approval_source: None, } ); assert_eq!( @@ -62,6 +64,7 @@ fn mcp_call_metric_outcome_distinguishes_request_and_tool_errors() { status: "error", error_type: Some(MCP_CALL_ERROR_TYPE_MCP_REQUEST), error_code: Some(MCP_CALL_ERROR_CODE_UNKNOWN.to_string()), + approval_source: None, } ); } @@ -79,6 +82,7 @@ fn mcp_call_metric_outcome_reports_server_tool_error_codes() { status: "error", error_type: Some(MCP_CALL_ERROR_TYPE_TOOL_RESULT), error_code: Some("arbitrary-user-value".to_string()), + approval_source: None, } ); } @@ -105,6 +109,7 @@ fn mcp_call_metric_outcome_reads_auth_error_code_from_meta() { status: "error", error_type: Some(MCP_CALL_ERROR_TYPE_TOOL_RESULT), error_code: Some("UNAUTHORIZED".to_string()), + approval_source: None, } ); } @@ -123,6 +128,7 @@ fn mcp_call_metric_outcome_bounds_and_sanitizes_error_code() { status: "error", error_type: Some(MCP_CALL_ERROR_TYPE_TOOL_RESULT), error_code: Some(format!("BAD_CODE_{}", "x".repeat(247))), + approval_source: None, } ); } diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 1dc06e3d00..7192660f38 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -2944,7 +2944,13 @@ async fn permission_request_hook_allows_mcp_tool_call() { ) .await; - assert_eq!(decision, Some(ReviewDecision::Approved)); + assert_eq!( + decision, + Some(McpToolApproval { + decision: ReviewDecision::Approved, + source: Some(ApprovalResolutionSource::Hook), + }) + ); let log = std::fs::read_to_string(log_path).expect("read MCP permission hook log"); let inputs = log .lines() @@ -3015,7 +3021,13 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() { ) .await; - assert_eq!(decision, Some(ReviewDecision::Approved)); + assert_eq!( + decision, + Some(McpToolApproval { + decision: ReviewDecision::Approved, + source: Some(ApprovalResolutionSource::Hook), + }) + ); let log = std::fs::read_to_string(log_path).expect("read MCP permission hook log"); let inputs = log .lines() @@ -3102,7 +3114,13 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { ) .await; - assert_eq!(decision, Some(ReviewDecision::Approved)); + assert_eq!( + decision, + Some(McpToolApproval { + decision: ReviewDecision::Approved, + source: None, + }) + ); assert!( !log_path.exists(), "remembered approval should skip PermissionRequest hooks" @@ -3202,7 +3220,11 @@ async fn strict_auto_review_forces_guardian_for_mcp_policy_skip() { ) .await; - let Some(ReviewDecision::Denied { rejection: message }) = decision else { + let Some(McpToolApproval { + decision: ReviewDecision::Denied { rejection: message }, + source: Some(ApprovalResolutionSource::Guardian), + }) = decision + else { panic!("guardian-denied MCP approval should carry a rejection message"); }; assert!(message.contains("Reason: The tool call would expose private calendar data")); @@ -3313,7 +3335,10 @@ async fn assert_mcp_user_approval_persistence( .await .expect("MCP approval task timed out") .expect("MCP approval task failed"), - Some(expected_decision), + Some(McpToolApproval { + decision: expected_decision, + source: Some(ApprovalResolutionSource::User), + }), ); } diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index 16197b56d3..2052f5948c 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -388,12 +388,20 @@ impl ApprovalAction { } #[derive(Clone, Copy, Debug, PartialEq, Eq)] -enum ApprovalResolutionSource { +pub(crate) enum ApprovalResolutionSource { Hook, Guardian, User, } +/// Preserves the approval path even when the decision becomes a tool error. +/// The source identifies the reviewer, not the actor that caused cancellation. +#[must_use] +pub(crate) struct ApprovalResult { + pub(crate) result: Result, + pub(crate) source: Option, +} + #[derive(Clone, Debug, PartialEq, Eq)] struct ApprovalResolution { decision: ReviewDecision, @@ -440,7 +448,7 @@ impl Session { self: &Arc, action: ApprovalAction, ctx: ApprovalContext, - ) -> Result { + ) -> ApprovalResult { // Stdin that exceeds current permissions needs a fresh sandbox approval. // Strict review of ordinary input follows the same routing as ordinary exec. let policy = ctx.review_context.turn().approval_policy(); @@ -450,7 +458,10 @@ impl Session { && let Some(reason) = prompt_is_rejected_by_policy(policy, /*prompt_is_rule*/ false) { - return Err(ToolError::Rejected(reason.to_string())); + return ApprovalResult { + result: Err(ToolError::Rejected(reason.to_string())), + source: None, + }; } let is_mcp_tool_call = matches!(&action, ApprovalAction::McpToolCall { .. }); let is_network_approval = matches!(&action, ApprovalAction::NetworkAccess { .. }); @@ -485,8 +496,12 @@ impl Session { if !is_network_approval { record_resolution(&ctx, &resolution); } + let source = Some(resolution.source); if is_mcp_tool_call && resolution.decision == ReviewDecision::ApprovedMcpPolicyAmendment { - return Ok(resolution.decision); + return ApprovalResult { + result: Ok(resolution.decision), + source, + }; } if is_network_approval { match (&resolution.decision, resolution.source) { @@ -496,17 +511,26 @@ impl Session { }, _, ) if network_policy_amendment.action == NetworkPolicyRuleAction::Deny => { - return Ok(resolution.decision); + return ApprovalResult { + result: Ok(resolution.decision), + source, + }; } (ReviewDecision::Abort, ApprovalResolutionSource::Guardian) => { - return Err(ToolError::Rejected( - "automatic approval review was cancelled".to_string(), - )); + return ApprovalResult { + result: Err(ToolError::Rejected( + "automatic approval review was cancelled".to_string(), + )), + source, + }; } _ => {} } } - resolution.into_tool_result(ctx.review_context.turn().model_info()) + ApprovalResult { + result: resolution.into_tool_result(ctx.review_context.turn().model_info()), + source, + } } async fn request_reviewer_approval( diff --git a/codex-rs/core/src/tools/mod.rs b/codex-rs/core/src/tools/mod.rs index 501ce5fe4e..7523d8b6ec 100644 --- a/codex-rs/core/src/tools/mod.rs +++ b/codex-rs/core/src/tools/mod.rs @@ -29,6 +29,7 @@ use std::borrow::Cow; use crate::session::turn_context::TurnContext; pub(crate) use approvals::ApprovalContext; +pub(crate) use approvals::ApprovalResolutionSource; use codex_features::Feature; use codex_protocol::exec_output::ExecToolCallOutput; use codex_protocol::openai_models::ModelInfo; diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 78924010a2..7eb6860729 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -819,7 +819,7 @@ impl NetworkApprovalService { } else { approval.await }; - let approval_decision = match approval_result { + let approval_decision = match approval_result.result { Ok(decision) => decision, Err(ToolError::Rejected(rejection)) => { if let Some(owner_call) = owner_call.as_ref() { diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index e7f544458b..46fd29d030 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -187,7 +187,8 @@ impl ToolOrchestrator { tool_ctx .session .request_approval(action, approval_ctx) - .await?; + .await + .result?; already_approved = true; } else { otel.tool_decision( @@ -220,7 +221,8 @@ impl ToolOrchestrator { tool_ctx .session .request_approval(action, approval_ctx) - .await?; + .await + .result?; already_approved = true; } } @@ -511,7 +513,8 @@ impl ToolOrchestrator { tool_ctx .session .request_approval(action, approval_ctx) - .await?; + .await + .result?; } let retry_sandbox_requested = !unsandboxed_allowed diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index cda8c59dd8..37a4759b82 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -977,6 +977,7 @@ impl UnifiedExecProcessManager { .session .request_approval(approval, approval_context) .await + .result .map_err(UnifiedExecError::StdinApproval)?; } diff --git a/codex-rs/ext/guardian-reviewer/src/metrics.rs b/codex-rs/ext/guardian-reviewer/src/metrics.rs index 48809cdf94..8813d307e2 100644 --- a/codex-rs/ext/guardian-reviewer/src/metrics.rs +++ b/codex-rs/ext/guardian-reviewer/src/metrics.rs @@ -46,6 +46,7 @@ pub(crate) fn emit_guardian_review_metrics( }; let mut counter_tag_refs = tag_refs.clone(); counter_tag_refs.extend(mcp_tags.iter().map(|(key, value)| (*key, value.as_str()))); + counter_tag_refs.push(("context_changed", bool_tag(result.context_changed))); session_telemetry.counter( GUARDIAN_REVIEW_COUNT_METRIC, /*inc*/ 1, @@ -463,6 +464,7 @@ mod tests { ]); let mut expected_counter_tags = expected_base_tags.clone(); expected_counter_tags.extend(expected_mcp_tags); + expected_counter_tags.insert("context_changed".to_string(), "false".to_string()); assert_eq!((attrs, value), (expected_counter_tags, 1)); assert_eq!( diff --git a/codex-rs/ext/guardian-reviewer/src/retry.rs b/codex-rs/ext/guardian-reviewer/src/retry.rs index 0e2df7e965..7bae2d989b 100644 --- a/codex-rs/ext/guardian-reviewer/src/retry.rs +++ b/codex-rs/ext/guardian-reviewer/src/retry.rs @@ -45,9 +45,12 @@ where } = limits; assert!(max_attempts > 0, "guardian review must run at least once"); let mut attempt_count = 1; + let mut context_changed = false; loop { let (outcome, mut analytics_result, evidence) = run_attempt(deadline).await; analytics_result.attempt_count = attempt_count; + context_changed |= analytics_result.context_changed; + analytics_result.context_changed = context_changed; if attempt_count >= max_attempts || !should_retry_guardian_review(&outcome) { return (outcome, analytics_result, evidence); }