mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
## Background Codex can use **Auto Review** for permission requests. Instead of asking the user immediately, Codex starts a separate locked-down reviewer session called **Guardian**, which returns a structured `allow` or `deny` assessment. The Guardian reviewer is itself a Codex session, so its model request can fail for transient infrastructure reasons such as model overload, HTTP connection failure, or response-stream disconnect. Today, any such failure immediately ends the Auto Review attempt and blocks the action. This PR adds bounded retries for failures that the existing protocol explicitly identifies as transient. Linear context: [CA-539](https://linear.app/openai/issue/CA-539/retry-auto-review-infrastructure-failures-and-fall-back-to-manual) ## What changes A Guardian review can now make at most **three total attempts**: 1. Run the review normally. 2. Retry after a jittered delay of roughly 180–220 ms if the first attempt fails with an eligible error. 3. Retry after a jittered delay of roughly 360–440 ms if the second attempt also fails with an eligible error. All attempts share the original review deadline. Jitter spreads retries from concurrent clients to reduce synchronized load during broader outages. The retries do not reset the user's maximum wait time, and the backoff waits terminate early if the review is cancelled or the deadline expires. Before retrying, the existing Guardian session lifecycle decides whether the session remains usable. Healthy trunks are reused, broken trunks are removed by the existing cleanup path, and ephemeral sessions continue to clean themselves up. The review still emits one logical lifecycle to clients. Recoverable intermediate failures do not produce warnings or terminal events. ## Retry policy ### Retried up to twice - model/server overload - HTTP connection failure - response-stream connection failure - response-stream disconnect - internal server error - a final reviewer message that cannot be parsed as the required Guardian assessment ### Not retried - bad or invalid requests - authentication failures - usage limits - cyber-policy failures - errors without a structured category - a request that already exhausted the lower-level Responses retry budget - a completed Guardian turn with no assessment payload - prompt-construction failures - Guardian review timeout - cancellation or abort - a valid `deny` assessment The session-error classification uses `ErrorEvent.codex_error_info`; it does not inspect error-message strings. ## Implementation notes - `wait_for_guardian_review` preserves the complete `ErrorEvent`, including structured `codex_error_info`. - Guardian session failures preserve the original message and optional structured `CodexErrorInfo`. - The retry policy classifies the explicitly transient `CodexErrorInfo` variants; unknown, absent, and deterministic categories are not retried. - The Guardian session manager receives the caller's deadline rather than creating a new timeout per attempt. - Analytics record the final `attempt_count`. - Retry orchestration does not add a separate session-cleanup protocol; it relies on the existing trunk and ephemeral lifecycle decisions. ## Automated testing Focused Guardian coverage verifies: - every supported transient `CodexErrorInfo` is classified as retryable, while absent and non-transient categories are not; - structured transient session failure -> retry -> approval with the healthy trunk reused; - two invalid Guardian responses -> third attempt -> approval, with exactly three requests; - three invalid responses -> existing fail-closed result, with exactly three requests and one terminal lifecycle; - valid denial, missing payload, invalid request, timeout, cancellation, and prompt/session construction failures are not retried; - retry eligibility ends after the third attempt; - retry delays use the shared exponential backoff helper and remain within the expected jitter bounds; - cancellation and deadline expiry interrupt the backoff wait; - healthy trunks are reused across retryable failures; - broken event streams remove the trunk through the existing lifecycle cleanup; - an ephemeral retry does not disturb a concurrent trunk review. Validation performed: - `just test -p codex-core guardian_review_ guardian_ephemeral_retry_preserves_parallel_trunk_and_fork_history run_review_removes_trunk_when_event_stream_is_broken` — **42 passed**; - `just test -p codex-analytics` — **71 passed**; - scoped Clippy fixes for `codex-core` and `codex-analytics` passed. A prior full `codex-core` run had unrelated environment-sensitive failures outside Guardian coverage. ## Manual QA The focused integration tests use the local mock Responses server to inspect exact request counts and emitted lifecycle events. They confirm that retries are internal, a successful later attempt supplies the final decision, non-retryable failures issue only one request, and exhausted retries emit only one terminal result.
177 lines
6.6 KiB
Rust
177 lines
6.6 KiB
Rust
//! Guardian review decides whether an `on-request` approval should be granted
|
|
//! automatically instead of shown to the user.
|
|
//!
|
|
//! 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 metrics;
|
|
mod prompt;
|
|
mod review;
|
|
mod review_session;
|
|
|
|
use std::time::Duration;
|
|
|
|
use codex_protocol::protocol::GuardianAssessmentDecisionSource;
|
|
use codex_protocol::protocol::GuardianAssessmentOutcome;
|
|
use serde::Deserialize;
|
|
use serde::Serialize;
|
|
|
|
pub(crate) use approval_request::GuardianApprovalRequest;
|
|
pub(crate) use approval_request::GuardianMcpAnnotations;
|
|
pub(crate) use approval_request::GuardianNetworkAccessTrigger;
|
|
#[cfg(test)]
|
|
pub(crate) use approval_request::guardian_approval_request_to_json;
|
|
pub(crate) use review::guardian_rejection_message;
|
|
pub(crate) use review::guardian_timeout_message;
|
|
pub(crate) use review::is_guardian_reviewer_source;
|
|
pub(crate) use review::new_guardian_review_id;
|
|
#[cfg(test)]
|
|
pub(crate) use review::record_guardian_denial_for_test;
|
|
pub(crate) use review::review_approval_request;
|
|
#[cfg(test)]
|
|
pub(crate) use review::review_approval_request_with_cancel;
|
|
pub(crate) use review::routes_approval_to_guardian;
|
|
pub(crate) use review::routes_approval_to_guardian_with_reviewer;
|
|
pub(crate) use review::spawn_approval_request_review;
|
|
pub(crate) use review_session::GuardianReviewSessionManager;
|
|
pub(crate) use review_session::prompt_cache_key_override_for_review_session;
|
|
|
|
pub(crate) const GUARDIAN_REVIEW_TIMEOUT: Duration = Duration::from_secs(90);
|
|
pub(crate) const GUARDIAN_REVIEWER_NAME: &str = "guardian";
|
|
pub(crate) const MAX_CONSECUTIVE_GUARDIAN_DENIALS_PER_TURN: u32 = 3;
|
|
pub(crate) const MAX_RECENT_AUTO_REVIEW_DENIALS_PER_TURN: u32 = 10;
|
|
pub(crate) const AUTO_REVIEW_DENIAL_WINDOW_SIZE: usize = 50;
|
|
pub(crate) const AUTO_REVIEW_DENIED_ACTION_APPROVAL_DEVELOPER_PREFIX: &str =
|
|
"The user has manually approved a specific action that was previously `Rejected`.";
|
|
const GUARDIAN_MAX_MESSAGE_TRANSCRIPT_TOKENS: usize = 10_000;
|
|
const GUARDIAN_MAX_TOOL_TRANSCRIPT_TOKENS: usize = 10_000;
|
|
const GUARDIAN_MAX_MESSAGE_ENTRY_TOKENS: usize = 2_000;
|
|
const GUARDIAN_MAX_TOOL_ENTRY_TOKENS: usize = 1_000;
|
|
const GUARDIAN_MAX_ACTION_STRING_TOKENS: usize = 16_000;
|
|
const GUARDIAN_RECENT_ENTRY_LIMIT: usize = 40;
|
|
const TRUNCATION_TAG: &str = "truncated";
|
|
|
|
/// Structured output contract that the guardian reviewer must satisfy.
|
|
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)]
|
|
pub(crate) struct GuardianAssessment {
|
|
pub(crate) risk_level: codex_protocol::protocol::GuardianRiskLevel,
|
|
pub(crate) user_authorization: codex_protocol::protocol::GuardianUserAuthorization,
|
|
pub(crate) outcome: GuardianAssessmentOutcome,
|
|
pub(crate) rationale: String,
|
|
}
|
|
|
|
#[derive(Debug, Clone, PartialEq, Eq)]
|
|
pub(crate) struct GuardianRejection {
|
|
pub(crate) rationale: String,
|
|
pub(crate) source: GuardianAssessmentDecisionSource,
|
|
}
|
|
|
|
#[derive(Debug, Default)]
|
|
pub(crate) struct GuardianRejectionCircuitBreaker {
|
|
turns: std::collections::HashMap<String, GuardianRejectionCircuitBreakerTurn>,
|
|
}
|
|
|
|
#[derive(Debug, Default)]
|
|
struct GuardianRejectionCircuitBreakerTurn {
|
|
consecutive_denials: u32,
|
|
recent_denials: std::collections::VecDeque<bool>,
|
|
interrupt_triggered: bool,
|
|
}
|
|
|
|
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
|
|
pub(crate) enum GuardianRejectionCircuitBreakerAction {
|
|
Continue,
|
|
InterruptTurn {
|
|
consecutive_denials: u32,
|
|
recent_denials: u32,
|
|
},
|
|
}
|
|
|
|
impl GuardianRejectionCircuitBreaker {
|
|
pub(crate) fn clear_turn(&mut self, turn_id: &str) {
|
|
self.turns.remove(turn_id);
|
|
}
|
|
|
|
pub(crate) fn record_denial(&mut self, turn_id: &str) -> GuardianRejectionCircuitBreakerAction {
|
|
let turn = self.turns.entry(turn_id.to_string()).or_default();
|
|
turn.consecutive_denials = turn.consecutive_denials.saturating_add(1);
|
|
Self::record_recent_review(turn, /*denied*/ true);
|
|
let recent_denials = turn.recent_denials.iter().filter(|denied| **denied).count() as u32;
|
|
if !turn.interrupt_triggered
|
|
&& (turn.consecutive_denials >= MAX_CONSECUTIVE_GUARDIAN_DENIALS_PER_TURN
|
|
|| recent_denials >= MAX_RECENT_AUTO_REVIEW_DENIALS_PER_TURN)
|
|
{
|
|
turn.interrupt_triggered = true;
|
|
GuardianRejectionCircuitBreakerAction::InterruptTurn {
|
|
consecutive_denials: turn.consecutive_denials,
|
|
recent_denials,
|
|
}
|
|
} else {
|
|
GuardianRejectionCircuitBreakerAction::Continue
|
|
}
|
|
}
|
|
|
|
pub(crate) fn record_non_denial(&mut self, turn_id: &str) {
|
|
let turn = self.turns.entry(turn_id.to_string()).or_default();
|
|
turn.consecutive_denials = 0;
|
|
Self::record_recent_review(turn, /*denied*/ false);
|
|
}
|
|
|
|
fn record_recent_review(turn: &mut GuardianRejectionCircuitBreakerTurn, denied: bool) {
|
|
turn.recent_denials.push_back(denied);
|
|
if turn.recent_denials.len() > AUTO_REVIEW_DENIAL_WINDOW_SIZE {
|
|
turn.recent_denials.pop_front();
|
|
}
|
|
}
|
|
}
|
|
|
|
#[cfg(test)]
|
|
use approval_request::format_guardian_action_pretty;
|
|
#[cfg(test)]
|
|
use approval_request::guardian_assessment_action;
|
|
#[cfg(test)]
|
|
use approval_request::guardian_request_turn_id;
|
|
#[cfg(test)]
|
|
use prompt::GuardianPromptMode;
|
|
#[cfg(test)]
|
|
use prompt::GuardianTranscriptCursor;
|
|
#[cfg(test)]
|
|
use prompt::GuardianTranscriptEntry;
|
|
#[cfg(test)]
|
|
use prompt::GuardianTranscriptEntryKind;
|
|
#[cfg(test)]
|
|
use prompt::build_guardian_prompt_items;
|
|
#[cfg(test)]
|
|
use prompt::build_guardian_prompt_items_with_parent_turn;
|
|
#[cfg(test)]
|
|
use prompt::collect_guardian_transcript_entries;
|
|
#[cfg(test)]
|
|
use prompt::guardian_output_schema;
|
|
#[cfg(test)]
|
|
pub(crate) use prompt::guardian_policy_prompt;
|
|
#[cfg(test)]
|
|
pub(crate) use prompt::guardian_policy_prompt_with_config;
|
|
#[cfg(test)]
|
|
use prompt::guardian_truncate_text;
|
|
#[cfg(test)]
|
|
use prompt::parse_guardian_assessment;
|
|
#[cfg(test)]
|
|
use prompt::render_guardian_transcript_entries;
|
|
#[cfg(test)]
|
|
use review::GuardianReviewOutcome;
|
|
#[cfg(test)]
|
|
use review::run_guardian_review_session_with_retry as run_guardian_review_session_for_test;
|
|
#[cfg(test)]
|
|
use review_session::build_guardian_review_session_config as build_guardian_review_session_config_for_test;
|
|
|
|
#[cfg(test)]
|
|
mod tests;
|