diff --git a/codex-rs/app-server/tests/suite/v2/guardian_v2_history_tests.rs b/codex-rs/app-server/tests/suite/v2/guardian_v2_history_tests.rs index b40b76472c..ca1454f1d8 100644 --- a/codex-rs/app-server/tests/suite/v2/guardian_v2_history_tests.rs +++ b/codex-rs/app-server/tests/suite/v2/guardian_v2_history_tests.rs @@ -18,8 +18,6 @@ use codex_app_server_protocol::ApprovalsReviewer; use codex_app_server_protocol::AskForApproval; use codex_app_server_protocol::GuardianApprovalReview; use codex_app_server_protocol::GuardianApprovalReviewStatus; -use codex_app_server_protocol::GuardianRiskLevel; -use codex_app_server_protocol::GuardianUserAuthorization; use codex_app_server_protocol::ItemGuardianApprovalReviewCompletedNotification; use codex_app_server_protocol::ThreadCompactStartParams; use codex_app_server_protocol::ThreadCompactStartResponse; @@ -445,8 +443,8 @@ async fn guardians_retain_evidence_after_compaction_and_discard_it_after_rollbac assessment.review, GuardianApprovalReview { status: GuardianApprovalReviewStatus::Denied, - risk_level: Some(GuardianRiskLevel::High), - user_authorization: Some(GuardianUserAuthorization::Unknown), + risk_level: None, + user_authorization: None, rationale: Some(format!("Automatic approval review failed: {reason}")), } ); @@ -475,9 +473,9 @@ async fn guardians_retain_evidence_after_compaction_and_discard_it_after_rollbac }) .expect("declined tool result"); assert!( - output - .to_string() - .contains("This action was rejected due to unacceptable risk."), + output.to_string().contains( + "This is a review failure, not a determination that the action is unsafe." + ), "tool must not execute: {output}", ); assert_eq!( diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index d96af8764c..258fedb847 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -398,6 +398,7 @@ async fn run_review_on_session( GuardianReviewSessionOutcome::SessionFailed { error, error_info: None, + retry_at: None, }, false, analytics_result, @@ -429,6 +430,7 @@ async fn run_review_on_session( GuardianReviewSessionOutcome::SessionFailed { error: error.into(), error_info: None, + retry_at: None, }, false, analytics_result, @@ -721,6 +723,7 @@ async fn run_review_on_session( GuardianReviewSessionOutcome::SessionFailed { error: anyhow!("guardian review input was not started: {submission:?}"), error_info: None, + retry_at: None, }, false, analytics_result, @@ -731,6 +734,7 @@ async fn run_review_on_session( GuardianReviewSessionOutcome::SessionFailed { error: err.into(), error_info: None, + retry_at: None, }, false, analytics_result, @@ -945,6 +949,10 @@ async fn wait_for_guardian_review( GuardianReviewSessionOutcome::SessionFailed { error: anyhow!(error.message), error_info: error.codex_error_info, + retry_at: review_session.session.services.thread_extension_data + .get::() + .filter(|advice| advice.turn_id == expected_turn_id) + .and_then(|advice| advice.retry_at), }, true, true, diff --git a/codex-rs/core/src/guardian/review_session_tests.rs b/codex-rs/core/src/guardian/review_session_tests.rs index 6490c838aa..57c45c1ef2 100644 --- a/codex-rs/core/src/guardian/review_session_tests.rs +++ b/codex-rs/core/src/guardian/review_session_tests.rs @@ -1030,7 +1030,10 @@ async fn wait_for_guardian_review_preserves_structured_session_error() { ) .await; - let GuardianReviewSessionOutcome::SessionFailed { error, error_info } = outcome else { + let GuardianReviewSessionOutcome::SessionFailed { + error, error_info, .. + } = outcome + else { panic!("expected structured session failure"); }; assert_eq!(error.to_string(), "temporary failure"); diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 91be43f71a..c3a51011be 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -2985,7 +2985,7 @@ async fn guardian_review_surfaces_responses_api_errors_in_rejection_reason() -> "denial rationale should not fall back to the generic missing payload error" ); assert!( - rejection.contains("Reason: Automatic approval review failed:") + rejection.starts_with("Automatic approval review failed:") && rejection.contains(error_message), "rejection message should include guardian rationale: {rejection}" ); diff --git a/codex-rs/core/src/responses_retry.rs b/codex-rs/core/src/responses_retry.rs index fb0505411e..462bb95ba8 100644 --- a/codex-rs/core/src/responses_retry.rs +++ b/codex-rs/core/src/responses_retry.rs @@ -39,6 +39,13 @@ impl Default for ResponsesStreamRetryState { } } +/// Server retry advice retained after stream retries are exhausted. The turn ID +/// prevents a reused Guardian session from applying advice from an earlier review. +pub(crate) struct ExhaustedResponseRetry { + pub(crate) turn_id: String, + pub(crate) retry_at: Option, +} + /// Handles a retryable stream error and returns `Ok(())` when the caller should /// retry the request loop. pub(crate) async fn handle_retryable_response_stream_error( @@ -125,6 +132,14 @@ pub(crate) async fn handle_retryable_response_stream_error( return Ok(()); } + sess.services + .thread_extension_data + .insert(ExhaustedResponseRetry { + turn_id: turn_context.sub_id.clone(), + retry_at: err + .retry_delay() + .and_then(|delay| tokio::time::Instant::now().checked_add(delay)), + }); Err(err) } diff --git a/codex-rs/core/tests/suite/guardian_retry.rs b/codex-rs/core/tests/suite/guardian_retry.rs new file mode 100644 index 0000000000..a48ad260c1 --- /dev/null +++ b/codex-rs/core/tests/suite/guardian_retry.rs @@ -0,0 +1,108 @@ +//! Exercises recovery through the parent tool call, reviewer, and executor boundary. + +use anyhow::Result; +use codex_core::config::Constrained; +use codex_protocol::approvals::GuardianAssessmentStatus; +use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::SandboxPolicy; +use core_test_support::responses::*; +use core_test_support::skip_if_no_network; +use core_test_support::skip_if_wine_exec; +use core_test_support::test_codex::test_codex; +use pretty_assertions::assert_eq; +use serde_json::json; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn guardian_retry_executes_only_after_a_completed_approval() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_wine_exec!( + Ok(()), + "Guardian approval actions require host-native paths" + ); + let server = start_mock_server().await; + let mut builder = test_codex().with_config(|config| { + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + config + .set_legacy_sandbox_policy(SandboxPolicy::new_workspace_write_policy()) + .expect("set sandbox policy"); + }); + let test = builder.build_with_auto_env(&server).await?; + let responses = vec![ + sse(vec![ + ev_function_call( + "write-marker", + "exec_command", + &json!({ + "cmd": "echo executed >> guardian-retry.txt", + "sandbox_permissions": "require_escalated", + "justification": "Write the requested marker", + }) + .to_string(), + ), + ev_completed("parent-call"), + ]), + sse_failed( + "first-review", + "rate_limit_exceeded", + "temporary review error", + ), + sse_failed( + "stream-retry", + "rate_limit_exceeded", + "temporary review error", + ), + sse(vec![ + ev_assistant_message( + "approval", + r#"{"risk_level":"low","user_authorization":"high","outcome":"allow","rationale":"requested write"}"#, + ), + ev_completed("review-approved"), + ]), + sse(vec![ev_completed("parent-done")]), + ]; + let requests = mount_sse_sequence(&server, responses).await; + test.codex + .start_or_steer_turn(codex_core::TurnInputRequest::user_input(vec![ + codex_protocol::user_input::UserInput::Text { + text: "Write the marker once".into(), + text_elements: vec![], + }, + ])) + .await?; + let mut reviews = Vec::new(); + let mut warnings = Vec::new(); + loop { + match test.codex.next_event().await?.msg { + EventMsg::GuardianAssessment(review) => reviews.push(review.status), + EventMsg::GuardianWarning(warning) => warnings.push(warning.message), + EventMsg::TurnComplete(_) => break, + _ => {} + } + } + assert_eq!(requests.requests().len(), 5); + assert_eq!( + reviews, + vec![ + GuardianAssessmentStatus::InProgress, + GuardianAssessmentStatus::Approved, + ] + ); + assert_eq!( + warnings.len(), + 1, + "internal retries should not emit terminal warnings" + ); + let contents = test + .fs() + .read_file_text( + &test.workspace_path_uri("guardian-retry.txt")?, + Default::default(), + /*sandbox*/ None, + ) + .await?; + assert_eq!(contents.lines().collect::>(), vec!["executed"]); + Ok(()) +} diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index 4f23375825..1fcc376522 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -78,6 +78,7 @@ mod guardian_context_budget; mod guardian_history; mod guardian_mcp_elicitation; mod guardian_retained_context; +mod guardian_retry; #[cfg(not(target_os = "windows"))] mod guardian_review; #[cfg(not(target_os = "windows"))] diff --git a/codex-rs/ext/guardian-reviewer/src/completion.rs b/codex-rs/ext/guardian-reviewer/src/completion.rs index 2d19b17ad1..0c1e035a24 100644 --- a/codex-rs/ext/guardian-reviewer/src/completion.rs +++ b/codex-rs/ext/guardian-reviewer/src/completion.rs @@ -12,7 +12,6 @@ use codex_protocol::protocol::GuardianRiskLevel; use codex_protocol::protocol::GuardianUserAuthorization; use codex_protocol::protocol::ReviewDecision; -use crate::GuardianAssessment; use crate::GuardianReviewError; use crate::GuardianReviewOutcome; @@ -23,6 +22,11 @@ const REJECTION_INSTRUCTIONS: &str = concat!( "or if the user explicitly approves the action after being informed of the risk. ", "Otherwise, stop and request user input.", ); +const REVIEW_FAILURE_INSTRUCTIONS: &str = concat!( + "The action was not executed because automatic approval review could not be completed. ", + "This is a review failure, not a determination that the action is unsafe. ", + "Do not bypass the approval check; resolve the error or ask the user for guidance.", +); const TIMEOUT_INSTRUCTIONS: &str = concat!( "The automatic permission approval review did not finish before its deadline. ", "Do not assume the action is unsafe based on the timeout alone. ", @@ -111,12 +115,19 @@ pub fn complete_review( | GuardianReviewError::Parse { message } => { analytics.decision = GuardianReviewDecision::Denied; analytics.terminal_status = GuardianReviewTerminalStatus::FailedClosed; - GuardianAssessment { - risk_level: GuardianRiskLevel::High, - user_authorization: GuardianUserAuthorization::Unknown, - outcome: GuardianAssessmentOutcome::Deny, - rationale: format!("Automatic approval review failed: {message}"), - } + let rationale = format!("Automatic approval review failed: {message}"); + // Keep the existing blocked status for client compatibility. + event.status = GuardianAssessmentStatus::Denied; + event.rationale = Some(rationale.clone()); + return ReviewCompletion { + decision: ReviewDecision::denied(format!( + "{rationale}\n{REVIEW_FAILURE_INSTRUCTIONS}" + )), + event, + warning: Some(rationale), + analytics, + assessment_outcome: None, + }; } } } diff --git a/codex-rs/ext/guardian-reviewer/src/outcome.rs b/codex-rs/ext/guardian-reviewer/src/outcome.rs index e376897cbc..03e4bc4fb9 100644 --- a/codex-rs/ext/guardian-reviewer/src/outcome.rs +++ b/codex-rs/ext/guardian-reviewer/src/outcome.rs @@ -3,6 +3,7 @@ use crate::GuardianAssessment; use codex_analytics::GuardianReviewFailureReason; use codex_protocol::protocol::CodexErrorInfo; +use tokio::time::Instant; #[derive(Debug)] pub enum GuardianReviewOutcome { @@ -18,6 +19,7 @@ pub enum GuardianReviewError { Session { message: String, error_info: Option, + retry_at: Option, }, Parse { message: String, @@ -37,13 +39,16 @@ impl GuardianReviewError { Self::Session { message: err.to_string(), error_info: None, + retry_at: None, } } - pub fn session_with_error_info(err: anyhow::Error, error_info: CodexErrorInfo) -> Self { + #[cfg(test)] + pub(crate) fn session_with_error_info(err: anyhow::Error, error_info: CodexErrorInfo) -> Self { Self::Session { message: err.to_string(), error_info: Some(error_info), + retry_at: None, } } @@ -71,6 +76,7 @@ pub enum GuardianReviewSessionOutcome { SessionFailed { error: anyhow::Error, error_info: Option, + retry_at: Option, }, TimedOut, Aborted, @@ -96,14 +102,15 @@ impl From for GuardianReviewOutcome { GuardianReviewSessionOutcome::PromptBuildFailed(error) => { Self::Error(GuardianReviewError::prompt_build(error)) } - GuardianReviewSessionOutcome::SessionFailed { error, error_info } => { - Self::Error(match error_info { - Some(error_info) => { - GuardianReviewError::session_with_error_info(error, error_info) - } - None => GuardianReviewError::session(error), - }) - } + GuardianReviewSessionOutcome::SessionFailed { + error, + error_info, + retry_at, + } => Self::Error(GuardianReviewError::Session { + message: error.to_string(), + error_info, + retry_at, + }), GuardianReviewSessionOutcome::TimedOut => Self::Error(GuardianReviewError::Timeout), GuardianReviewSessionOutcome::Aborted => Self::Error(GuardianReviewError::Cancelled), } diff --git a/codex-rs/ext/guardian-reviewer/src/retry.rs b/codex-rs/ext/guardian-reviewer/src/retry.rs index 90166bfbbe..49dda5d213 100644 --- a/codex-rs/ext/guardian-reviewer/src/retry.rs +++ b/codex-rs/ext/guardian-reviewer/src/retry.rs @@ -39,8 +39,14 @@ where if attempt_count >= max_attempts || !should_retry_guardian_review(&outcome) { return (outcome, analytics_result); } + let retry_at = match &outcome { + GuardianReviewOutcome::Error(GuardianReviewError::Session { retry_at, .. }) => { + *retry_at + } + _ => None, + }; if let Some(error) = - wait_before_guardian_retry(attempt_count, deadline, external_cancel).await + wait_before_guardian_retry(attempt_count, retry_at, deadline, external_cancel).await { return (GuardianReviewOutcome::Error(error), analytics_result); } @@ -50,13 +56,16 @@ where async fn wait_before_guardian_retry( attempt_count: i64, + retry_not_before: Option, deadline: Instant, external_cancel: Option<&CancellationToken>, ) -> Option { let exponential_delay = 200.0 * 2.0_f64.powi(attempt_count.saturating_sub(1) as i32); let jitter = rand::rng().random_range(0.9..1.1); let retry_delay = Duration::from_millis((exponential_delay * jitter) as u64); - let retry_at = (Instant::now() + retry_delay).min(deadline); + let retry_at = (Instant::now() + retry_delay) + .max(retry_not_before.unwrap_or_else(Instant::now)) + .min(deadline); tokio::select! { _ = sleep_until(retry_at) => { (Instant::now() >= deadline).then_some(GuardianReviewError::Timeout) @@ -72,21 +81,43 @@ async fn wait_before_guardian_retry( } fn should_retry_guardian_review(outcome: &GuardianReviewOutcome) -> bool { - matches!( - outcome, - GuardianReviewOutcome::Error( - GuardianReviewError::Session { - error_info: Some( - CodexErrorInfo::ServerOverloaded - | CodexErrorInfo::HttpConnectionFailed { .. } - | CodexErrorInfo::ResponseStreamConnectionFailed { .. } - | CodexErrorInfo::InternalServerError - | CodexErrorInfo::ResponseStreamDisconnected { .. } - ), - .. - } | GuardianReviewError::Parse { .. } - ) - ) + match outcome { + GuardianReviewOutcome::Error(GuardianReviewError::Parse { .. }) => true, + GuardianReviewOutcome::Error(GuardianReviewError::Session { + error_info: Some(error), + .. + }) => match error { + CodexErrorInfo::RateLimitExceeded + | CodexErrorInfo::ServerOverloaded + | CodexErrorInfo::InternalServerError => true, + CodexErrorInfo::HttpConnectionFailed { http_status_code } + | CodexErrorInfo::ResponseStreamConnectionFailed { http_status_code } + | CodexErrorInfo::ResponseStreamDisconnected { http_status_code } + | CodexErrorInfo::ResponseTooManyFailedAttempts { http_status_code } => { + matches!(http_status_code, None | Some(408 | 429 | 500..=599)) + } + CodexErrorInfo::ContextWindowExceeded + | CodexErrorInfo::SessionBudgetExceeded + | CodexErrorInfo::UsageLimitExceeded + | CodexErrorInfo::CyberPolicy + | CodexErrorInfo::MisalignmentPolicyViolation + | CodexErrorInfo::Unauthorized + | CodexErrorInfo::BadRequest + | CodexErrorInfo::SandboxError + | CodexErrorInfo::ActiveTurnNotSteerable { .. } + | CodexErrorInfo::ThreadRollbackFailed + | CodexErrorInfo::Other => false, + }, + GuardianReviewOutcome::Completed(_) + | GuardianReviewOutcome::Error( + GuardianReviewError::PromptBuild { .. } + | GuardianReviewError::Session { + error_info: None, .. + } + | GuardianReviewError::Timeout + | GuardianReviewError::Cancelled, + ) => false, + } } #[cfg(test)] diff --git a/codex-rs/ext/guardian-reviewer/src/retry_tests.rs b/codex-rs/ext/guardian-reviewer/src/retry_tests.rs index 715598c850..d017e3d654 100644 --- a/codex-rs/ext/guardian-reviewer/src/retry_tests.rs +++ b/codex-rs/ext/guardian-reviewer/src/retry_tests.rs @@ -114,6 +114,7 @@ async fn guardian_review_retry_wait_honors_cancellation() { let error = wait_before_guardian_retry( /*attempt_count*/ 1, + /*retry_not_before*/ None, Instant::now() + Duration::from_secs(/*secs*/ 1), Some(&cancel_token), ) @@ -126,6 +127,7 @@ async fn guardian_review_retry_wait_honors_cancellation() { async fn guardian_review_retry_wait_honors_deadline() { let error = wait_before_guardian_retry( /*attempt_count*/ 1, + /*retry_not_before*/ None, Instant::now(), /*external_cancel*/ None, )