openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-09-10 20:26:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

Improve Guardian retries and review failure reporting (#44482)

Browse Source 
## Why

Transient rate limits can end automatic approval reviews prematurely, and review failures currently report high risk even when no assessment completed.

## What changed

- Retry rate limits and recoverable exhausted-stream errors, while excluding non-transient HTTP failures.
- Preserve server retry delays after stream retries are exhausted and honor them within the review deadline. Scope retry advice to the current turn so reused sessions cannot apply stale delays.
- Keep failed reviews denied, but leave risk and authorization unset and explain that the review could not complete without declaring the action unsafe.

## Testing

Add an integration test covering rate-limit recovery through approval and tool execution, asserting that the action executes exactly once after approval. Update failure assertions to check absent assessment fields and the review-failure explanation.

GitOrigin-RevId: 1163cfde35c6b8eb23b6f24f1f24461b86ded838
This commit is contained in:
jif
2026-09-10 09:42:31 +00:00
committed by copyberry
parent 03f014564d
commit 5d3fe48b08
11 changed files with 226 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
codex-rs/app-server/tests/suite/v2/guardian_v2_history_tests.rs
View File

@@ -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!(

8
codex-rs/core/src/guardian/review_session.rs
View File

@@ -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::<crate::responses_retry::ExhaustedResponseRetry>()
.filter(|advice| advice.turn_id == expected_turn_id)
.and_then(|advice| advice.retry_at),
},
true,
true,

5
codex-rs/core/src/guardian/review_session_tests.rs
View File

@@ -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");

2
codex-rs/core/src/guardian/tests.rs
View File

@@ -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}"
);

15
codex-rs/core/src/responses_retry.rs
View File

@@ -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<tokio::time::Instant>,
}
/// 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)
}

108
codex-rs/core/tests/suite/guardian_retry.rs Normal file
View File

@@ -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<_>>(), vec!["executed"]);
Ok(())
}

1
codex-rs/core/tests/suite/mod.rs
View File

@@ -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"))]

25
codex-rs/ext/guardian-reviewer/src/completion.rs
View File

@@ -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,
};
}
}
}

25
codex-rs/ext/guardian-reviewer/src/outcome.rs
View File

@@ -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<CodexErrorInfo>,
retry_at: Option<Instant>,
},
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<CodexErrorInfo>,
retry_at: Option<Instant>,
},
TimedOut,
Aborted,
@@ -96,14 +102,15 @@ impl From<GuardianReviewSessionOutcome> 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),
}

65
codex-rs/ext/guardian-reviewer/src/retry.rs
View File

@@ -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<Instant>,
deadline: Instant,
external_cancel: Option<&CancellationToken>,
) -> Option<GuardianReviewError> {
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)]

2
codex-rs/ext/guardian-reviewer/src/retry_tests.rs
View File

@@ -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,
)