diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 06d1a78931..456c35869c 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -58,6 +58,11 @@ const GUARDIAN_TIMEOUT_INSTRUCTIONS: &str = concat!( "You may retry once, or ask the user for guidance or explicit approval.", ); +const GUARDIAN_RETRY_WARNING: &str = concat!( + "Automatic approval review hit a retryable reviewer availability failure. ", + "Retrying once before failing closed." +); + pub(crate) fn new_guardian_review_id() -> String { uuid::Uuid::new_v4().to_string() } @@ -75,11 +80,20 @@ pub(crate) async fn guardian_rejection_message(session: &Session, review_id: &st source: GuardianAssessmentDecisionSource::Agent, }); match rejection.source { - GuardianAssessmentDecisionSource::Agent => format!( - "This action was rejected due to unacceptable risk.\nReason: {}\n{}", - rejection.rationale.trim(), - GUARDIAN_REJECTION_INSTRUCTIONS - ), + GuardianAssessmentDecisionSource::Agent => { + let rationale = rejection.rationale.trim(); + if rationale.starts_with("Automatic approval review failed") + || rationale.starts_with("Automatic approval review could not") + { + format!( + "This action was blocked because automatic approval review could not complete safely.\nReason: {rationale}\n{GUARDIAN_REJECTION_INSTRUCTIONS}", + ) + } else { + format!( + "This action was rejected due to unacceptable risk.\nReason: {rationale}\n{GUARDIAN_REJECTION_INSTRUCTIONS}", + ) + } + } } } @@ -130,6 +144,54 @@ impl GuardianReviewError { Self::Cancelled => GuardianReviewFailureReason::Cancelled, } } + + fn is_retryable(&self) -> bool { + match self { + Self::Timeout => true, + Self::Session { message } => session_error_is_retryable(message), + Self::PromptBuild { .. } | Self::Parse { .. } | Self::Cancelled => false, + } + } + + fn retry_reason(&self) -> String { + match self { + Self::Session { message } => format!( + "Previous automatic approval review failed due to reviewer capacity, transport, or session error: {message}. Retry once before treating this as a failed-closed review." + ), + Self::Timeout => concat!( + "Previous automatic approval review timed out. ", + "Retry once before treating this as a failed-closed review." + ) + .to_string(), + Self::PromptBuild { .. } | Self::Parse { .. } | Self::Cancelled => { + "Automatic approval review is not retryable.".to_string() + } + } + } +} + +fn session_error_is_retryable(message: &str) -> bool { + let message = message.to_ascii_lowercase(); + [ + "429", + "500", + "502", + "503", + "504", + "capacity", + "connection", + "internal server error", + "overload", + "rate limit", + "service unavailable", + "temporarily unavailable", + "timed out", + "timeout", + "too many requests", + "transport", + ] + .iter() + .any(|needle| message.contains(needle)) } fn guardian_risk_level_str(level: GuardianRiskLevel) -> &'static str { @@ -335,7 +397,7 @@ async fn run_guardian_review( let schema = guardian_output_schema(); let terminal_action = action_summary.clone(); - let (outcome, analytics_result) = Box::pin(run_guardian_review_session( + let (outcome, analytics_result) = Box::pin(run_guardian_review_session_with_retry( session.clone(), turn.clone(), request, @@ -462,15 +524,26 @@ async fn run_guardian_review( GuardianReviewError::PromptBuild { .. } | GuardianReviewError::Session { .. } | GuardianReviewError::Parse { .. } => { - let message = match &error { - GuardianReviewError::PromptBuild { message } - | GuardianReviewError::Session { message } - | GuardianReviewError::Parse { message } => message, + let rationale = match &error { + GuardianReviewError::PromptBuild { message } => { + format!( + "Automatic approval review could not build the review prompt: {message}" + ) + } + GuardianReviewError::Session { message } => { + format!( + "Automatic approval review failed because the reviewer session returned an error: {message}" + ) + } + GuardianReviewError::Parse { message } => { + format!( + "Automatic approval review returned an unreadable assessment: {message}" + ) + } GuardianReviewError::Timeout | GuardianReviewError::Cancelled => { - "guardian review failed" + "Automatic approval review failed.".to_string() } }; - let rationale = format!("Automatic approval review failed: {message}"); track_guardian_review( session.as_ref(), &review_tracking, @@ -568,6 +641,55 @@ async fn run_guardian_review( } } +async fn run_guardian_review_session_with_retry( + session: Arc, + turn: Arc, + request: GuardianApprovalRequest, + retry_reason: Option, + schema: serde_json::Value, + external_cancel: Option, +) -> (GuardianReviewOutcome, GuardianReviewAnalyticsResult) { + let (first_outcome, first_analytics_result) = Box::pin(run_guardian_review_session( + Arc::clone(&session), + Arc::clone(&turn), + request.clone(), + retry_reason.clone(), + schema.clone(), + external_cancel.clone(), + )) + .await; + + let GuardianReviewOutcome::Error(error) = &first_outcome else { + return (first_outcome, first_analytics_result); + }; + if !error.is_retryable() + || external_cancel + .as_ref() + .is_some_and(CancellationToken::is_cancelled) + { + return (first_outcome, first_analytics_result); + } + + session + .send_event( + turn.as_ref(), + EventMsg::GuardianWarning(WarningEvent { + message: GUARDIAN_RETRY_WARNING.to_string(), + }), + ) + .await; + + run_guardian_review_session( + session, + turn, + request, + Some(error.retry_reason()), + schema, + external_cancel, + ) + .await +} + /// Public entrypoint for approval requests that should be reviewed by guardian. pub(crate) async fn review_approval_request( session: &Arc, @@ -825,4 +947,22 @@ mod review_tests { GuardianReviewFailureReason::SessionError )); } + + #[test] + fn guardian_review_retries_only_availability_failures() { + assert!(GuardianReviewError::Timeout.is_retryable()); + assert!( + GuardianReviewError::session(anyhow::anyhow!( + "503 service unavailable: reviewer capacity exhausted" + )) + .is_retryable() + ); + assert!( + !GuardianReviewError::session(anyhow::anyhow!( + "invalid_request_error: malformed review prompt" + )) + .is_retryable() + ); + assert!(!GuardianReviewError::parse(anyhow::anyhow!("bad JSON")).is_retryable()); + } } diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index ffadd801ca..52501aa516 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -56,6 +56,7 @@ use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_response_created; use core_test_support::responses::mount_response_once; +use core_test_support::responses::mount_response_sequence; use core_test_support::responses::mount_sse_once; use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; @@ -2072,7 +2073,8 @@ async fn guardian_review_surfaces_responses_api_errors_in_rejection_reason() -> let rejection_message = guardian_rejection_message(session.as_ref(), "review-shell-guardian-error").await; assert!( - rejection_message.contains("Reason: Automatic approval review failed:") + rejection_message + .contains("Reason: Automatic approval review failed because the reviewer session") && rejection_message.contains(error_message), "rejection message should include guardian rationale: {rejection_message}" ); @@ -2080,6 +2082,62 @@ async fn guardian_review_surfaces_responses_api_errors_in_rejection_reason() -> Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn guardian_review_retries_reviewer_availability_failure() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let guardian_assessment = serde_json::json!({ + "risk_level": "low", + "user_authorization": "high", + "outcome": "allow", + "rationale": "The command only inspects local git state.", + }) + .to_string(); + let request_log = mount_response_sequence( + &server, + vec![ + wiremock::ResponseTemplate::new(503).set_body_json(serde_json::json!({ + "error": { + "message": "reviewer capacity temporarily unavailable" + } + })), + wiremock::ResponseTemplate::new(200) + .insert_header("content-type", "text/event-stream") + .set_body_string(sse(vec![ + ev_response_created("resp-guardian"), + ev_assistant_message("msg-guardian", &guardian_assessment), + ev_completed("resp-guardian"), + ])), + ], + ) + .await; + + let (session, turn) = guardian_test_session_and_turn(&server).await; + seed_guardian_parent_history(&session, &turn).await; + + let decision = review_approval_request( + &session, + &turn, + "review-shell-guardian-retry".to_string(), + GuardianApprovalRequest::Shell { + id: "shell-guardian-retry".to_string(), + command: vec!["git".to_string(), "status".to_string()], + cwd: test_path_buf("/repo/codex-rs/core").abs(), + sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, + additional_permissions: None, + justification: Some("Inspect repo state before proceeding.".to_string()), + }, + /*retry_reason*/ None, + ) + .await; + + assert_eq!(decision, ReviewDecision::Approved); + assert_eq!(request_log.requests().len(), 2); + + Ok(()) +} + #[tokio::test] async fn guardian_parallel_reviews_fork_from_last_committed_trunk_history() -> anyhow::Result<()> { const TEST_STACK_SIZE_BYTES: usize = 4 * 1024 * 1024;