mirror of
https://github.com/openai/codex.git
synced 2026-10-27 18:48:37 +00:00
Extract Guardian review policy into a dedicated crate (#44227)
## What changed Move assessment parsing and schema, model selection, review outcomes, retry handling, and rejection circuit breakers into `codex-guardian-reviewer`. Update core to use the extracted APIs while retaining session execution and decision enforcement in the host. ## Testing Move existing assessment, circuit breaker, and retry tests into the new crate, including coverage for transient errors, cancellation, and deadlines. GitOrigin-RevId: 96ec9989a0066acb012bf3bec0b9f7d8bf11a4ef
This commit is contained in:
@@ -0,0 +1,6 @@
|
||||
load("//:defs.bzl", "codex_rust_crate")
|
||||
|
||||
codex_rust_crate(
|
||||
name = "guardian-reviewer",
|
||||
crate_name = "codex_guardian_reviewer",
|
||||
)
|
||||
@@ -0,0 +1,26 @@
|
||||
[package]
|
||||
edition.workspace = true
|
||||
license.workspace = true
|
||||
name = "codex-guardian-reviewer"
|
||||
version.workspace = true
|
||||
|
||||
[lib]
|
||||
doctest = false
|
||||
name = "codex_guardian_reviewer"
|
||||
path = "src/lib.rs"
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
|
||||
[dependencies]
|
||||
anyhow = { workspace = true }
|
||||
codex-analytics = { workspace = true }
|
||||
codex-protocol = { workspace = true }
|
||||
rand = { workspace = true }
|
||||
serde = { workspace = true }
|
||||
serde_json = { workspace = true }
|
||||
tokio = { workspace = true }
|
||||
tokio-util = { workspace = true }
|
||||
|
||||
[dev-dependencies]
|
||||
pretty_assertions = { workspace = true }
|
||||
@@ -0,0 +1,123 @@
|
||||
//! The production synchronous reviewer's output schema and tolerant JSON parser.
|
||||
//! Parsing defaults and error text are shared unchanged by all reviewer callers.
|
||||
|
||||
use codex_protocol::protocol::GuardianAssessmentOutcome;
|
||||
use codex_protocol::protocol::GuardianRiskLevel;
|
||||
use codex_protocol::protocol::GuardianUserAuthorization;
|
||||
use serde::Deserialize;
|
||||
use serde::Serialize;
|
||||
use serde_json::Value;
|
||||
|
||||
/// Structured output contract that the guardian reviewer must satisfy.
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)]
|
||||
pub struct GuardianAssessment {
|
||||
pub risk_level: codex_protocol::protocol::GuardianRiskLevel,
|
||||
pub user_authorization: codex_protocol::protocol::GuardianUserAuthorization,
|
||||
pub outcome: GuardianAssessmentOutcome,
|
||||
pub rationale: String,
|
||||
}
|
||||
|
||||
/// The model is asked for strict JSON, but we still accept a surrounding prose
|
||||
/// wrapper so transient formatting drift fails less noisily during dogfooding.
|
||||
/// Non-JSON output is still a review failure; this is only a thin recovery path
|
||||
/// for cases where the model wrapped the JSON in extra prose.
|
||||
pub fn parse_guardian_assessment(text: Option<&str>) -> anyhow::Result<GuardianAssessment> {
|
||||
let Some(text) = text else {
|
||||
anyhow::bail!("guardian review completed without an assessment payload");
|
||||
};
|
||||
let parsed_payload =
|
||||
if let Ok(payload) = serde_json::from_str::<GuardianAssessmentPayload>(text) {
|
||||
payload
|
||||
} else if let (Some(start), Some(end)) = (text.find('{'), text.rfind('}'))
|
||||
&& start < end
|
||||
&& let Some(slice) = text.get(start..=end)
|
||||
{
|
||||
serde_json::from_str::<GuardianAssessmentPayload>(slice)?
|
||||
} else {
|
||||
anyhow::bail!("guardian assessment was not valid JSON");
|
||||
};
|
||||
|
||||
let outcome = parsed_payload.outcome;
|
||||
let risk_level = parsed_payload.risk_level.unwrap_or(match outcome {
|
||||
GuardianAssessmentOutcome::Allow => GuardianRiskLevel::Low,
|
||||
GuardianAssessmentOutcome::Deny => GuardianRiskLevel::High,
|
||||
});
|
||||
let rationale = parsed_payload
|
||||
.rationale
|
||||
.filter(|rationale| !rationale.trim().is_empty())
|
||||
.unwrap_or_else(|| match outcome {
|
||||
GuardianAssessmentOutcome::Allow => {
|
||||
"Auto-review returned a low-risk allow decision.".to_string()
|
||||
}
|
||||
GuardianAssessmentOutcome::Deny => {
|
||||
"Auto-review returned a deny decision without a rationale.".to_string()
|
||||
}
|
||||
});
|
||||
|
||||
Ok(GuardianAssessment {
|
||||
risk_level,
|
||||
user_authorization: parsed_payload
|
||||
.user_authorization
|
||||
.unwrap_or(GuardianUserAuthorization::Unknown),
|
||||
outcome,
|
||||
rationale,
|
||||
})
|
||||
}
|
||||
|
||||
#[derive(Deserialize)]
|
||||
struct GuardianAssessmentPayload {
|
||||
risk_level: Option<GuardianRiskLevel>,
|
||||
user_authorization: Option<GuardianUserAuthorization>,
|
||||
outcome: GuardianAssessmentOutcome,
|
||||
rationale: Option<String>,
|
||||
}
|
||||
|
||||
/// JSON schema supplied as `final_output_json_schema` to guide a structured
|
||||
/// final answer from the guardian review session.
|
||||
///
|
||||
/// Keep this next to `guardian_output_contract_prompt()` so the prompt text and
|
||||
/// output schema stay aligned.
|
||||
pub fn guardian_output_schema() -> Value {
|
||||
serde_json::json!({
|
||||
"type": "object",
|
||||
"additionalProperties": false,
|
||||
"properties": {
|
||||
"risk_level": {
|
||||
"type": "string",
|
||||
"enum": ["low", "medium", "high", "critical"]
|
||||
},
|
||||
"user_authorization": {
|
||||
"type": "string",
|
||||
"enum": ["unknown", "low", "medium", "high"]
|
||||
},
|
||||
"outcome": {
|
||||
"type": "string",
|
||||
"enum": ["allow", "deny"]
|
||||
},
|
||||
"rationale": {
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"required": ["outcome"]
|
||||
})
|
||||
}
|
||||
|
||||
/// Prompt fragment that describes the exact JSON contract paired with
|
||||
/// `guardian_output_schema()`.
|
||||
pub fn guardian_output_contract_prompt() -> &'static str {
|
||||
r#"You may use read-only tool checks to gather any additional context you need before deciding. When you are ready to answer, your final message must be strict JSON.
|
||||
|
||||
For low-risk actions, give the final answer directly: {"outcome":"allow"}.
|
||||
|
||||
For anything else, use this JSON schema:
|
||||
{
|
||||
"risk_level": "low" | "medium" | "high" | "critical",
|
||||
"user_authorization": "unknown" | "low" | "medium" | "high",
|
||||
"outcome": "allow" | "deny",
|
||||
"rationale": string
|
||||
}"#
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "assessment_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,83 @@
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn parse_guardian_assessment_extracts_embedded_json() {
|
||||
let parsed = parse_guardian_assessment(Some(
|
||||
"preface {\"risk_level\":\"medium\",\"user_authorization\":\"low\",\"outcome\":\"allow\",\"rationale\":\"ok\"}",
|
||||
))
|
||||
.expect("guardian assessment");
|
||||
|
||||
assert_eq!(
|
||||
parsed,
|
||||
GuardianAssessment {
|
||||
risk_level: GuardianRiskLevel::Medium,
|
||||
user_authorization: GuardianUserAuthorization::Low,
|
||||
outcome: GuardianAssessmentOutcome::Allow,
|
||||
rationale: "ok".to_string(),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_guardian_assessment_treats_bare_allow_as_low_risk() {
|
||||
let parsed =
|
||||
parse_guardian_assessment(Some(r#"{"outcome":"allow"}"#)).expect("guardian assessment");
|
||||
|
||||
assert_eq!(
|
||||
parsed,
|
||||
GuardianAssessment {
|
||||
risk_level: GuardianRiskLevel::Low,
|
||||
user_authorization: GuardianUserAuthorization::Unknown,
|
||||
outcome: GuardianAssessmentOutcome::Allow,
|
||||
rationale: "Auto-review returned a low-risk allow decision.".to_string(),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_guardian_assessment_treats_bare_deny_as_high_risk() {
|
||||
let parsed =
|
||||
parse_guardian_assessment(Some(r#"{"outcome":"deny"}"#)).expect("guardian assessment");
|
||||
|
||||
assert_eq!(
|
||||
parsed,
|
||||
GuardianAssessment {
|
||||
risk_level: GuardianRiskLevel::High,
|
||||
user_authorization: GuardianUserAuthorization::Unknown,
|
||||
outcome: GuardianAssessmentOutcome::Deny,
|
||||
rationale: "Auto-review returned a deny decision without a rationale.".to_string(),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_output_schema_requires_only_outcome_and_allows_optional_details() {
|
||||
let schema = guardian_output_schema();
|
||||
|
||||
assert_eq!(
|
||||
schema,
|
||||
serde_json::json!({
|
||||
"type": "object",
|
||||
"additionalProperties": false,
|
||||
"properties": {
|
||||
"risk_level": {
|
||||
"type": "string",
|
||||
"enum": ["low", "medium", "high", "critical"]
|
||||
},
|
||||
"user_authorization": {
|
||||
"type": "string",
|
||||
"enum": ["unknown", "low", "medium", "high"]
|
||||
},
|
||||
"outcome": {
|
||||
"type": "string",
|
||||
"enum": ["allow", "deny"]
|
||||
},
|
||||
"rationale": {
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"required": ["outcome"]
|
||||
})
|
||||
);
|
||||
}
|
||||
@@ -0,0 +1,90 @@
|
||||
//! Counts policy denials per turn; the host carries out any requested interruption.
|
||||
|
||||
const MAX_CONSECUTIVE_CYBER_GUARDIAN_DENIALS_PER_TURN: u32 = 1;
|
||||
const MAX_CONSECUTIVE_GUARDIAN_DENIALS_PER_TURN: u32 = 3;
|
||||
const MAX_RECENT_CYBER_AUTO_REVIEW_DENIALS_PER_TURN: u32 = 1;
|
||||
const MAX_RECENT_AUTO_REVIEW_DENIALS_PER_TURN: u32 = 10;
|
||||
pub const AUTO_REVIEW_DENIAL_WINDOW_SIZE: usize = 50;
|
||||
|
||||
#[derive(Debug, Default)]
|
||||
pub 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 enum GuardianRejectionCircuitBreakerPolicy {
|
||||
Standard,
|
||||
CyberModel,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
|
||||
pub enum GuardianRejectionCircuitBreakerAction {
|
||||
Continue,
|
||||
InterruptTurn {
|
||||
consecutive_denials: u32,
|
||||
recent_denials: u32,
|
||||
},
|
||||
}
|
||||
|
||||
impl GuardianRejectionCircuitBreaker {
|
||||
pub fn clear_turn(&mut self, turn_id: &str) {
|
||||
self.turns.remove(turn_id);
|
||||
}
|
||||
|
||||
pub fn record_denial(
|
||||
&mut self,
|
||||
turn_id: &str,
|
||||
policy: GuardianRejectionCircuitBreakerPolicy,
|
||||
) -> 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;
|
||||
let (max_consecutive_denials, max_recent_denials) = match policy {
|
||||
GuardianRejectionCircuitBreakerPolicy::Standard => (
|
||||
MAX_CONSECUTIVE_GUARDIAN_DENIALS_PER_TURN,
|
||||
MAX_RECENT_AUTO_REVIEW_DENIALS_PER_TURN,
|
||||
),
|
||||
GuardianRejectionCircuitBreakerPolicy::CyberModel => (
|
||||
MAX_CONSECUTIVE_CYBER_GUARDIAN_DENIALS_PER_TURN,
|
||||
MAX_RECENT_CYBER_AUTO_REVIEW_DENIALS_PER_TURN,
|
||||
),
|
||||
};
|
||||
if !turn.interrupt_triggered
|
||||
&& (turn.consecutive_denials >= max_consecutive_denials
|
||||
|| recent_denials >= max_recent_denials)
|
||||
{
|
||||
turn.interrupt_triggered = true;
|
||||
GuardianRejectionCircuitBreakerAction::InterruptTurn {
|
||||
consecutive_denials: turn.consecutive_denials,
|
||||
recent_denials,
|
||||
}
|
||||
} else {
|
||||
GuardianRejectionCircuitBreakerAction::Continue
|
||||
}
|
||||
}
|
||||
|
||||
pub 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)]
|
||||
#[path = "circuit_breaker_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,107 @@
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn guardian_rejection_circuit_breaker_interrupts_after_three_consecutive_denials() {
|
||||
let mut circuit_breaker = GuardianRejectionCircuitBreaker::default();
|
||||
assert_eq!(
|
||||
circuit_breaker.record_denial("turn-1", GuardianRejectionCircuitBreakerPolicy::Standard),
|
||||
GuardianRejectionCircuitBreakerAction::Continue
|
||||
);
|
||||
assert_eq!(
|
||||
circuit_breaker.record_denial("turn-1", GuardianRejectionCircuitBreakerPolicy::Standard),
|
||||
GuardianRejectionCircuitBreakerAction::Continue
|
||||
);
|
||||
assert_eq!(
|
||||
circuit_breaker.record_denial("turn-1", GuardianRejectionCircuitBreakerPolicy::Standard),
|
||||
GuardianRejectionCircuitBreakerAction::InterruptTurn {
|
||||
consecutive_denials: 3,
|
||||
recent_denials: 3,
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
circuit_breaker.record_denial("turn-1", GuardianRejectionCircuitBreakerPolicy::Standard),
|
||||
GuardianRejectionCircuitBreakerAction::Continue
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_rejection_circuit_breaker_interrupts_cyber_models_after_one_denial() {
|
||||
let mut circuit_breaker = GuardianRejectionCircuitBreaker::default();
|
||||
assert_eq!(
|
||||
circuit_breaker.record_denial("turn-1", GuardianRejectionCircuitBreakerPolicy::CyberModel),
|
||||
GuardianRejectionCircuitBreakerAction::InterruptTurn {
|
||||
consecutive_denials: 1,
|
||||
recent_denials: 1,
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
circuit_breaker.record_denial("turn-1", GuardianRejectionCircuitBreakerPolicy::CyberModel),
|
||||
GuardianRejectionCircuitBreakerAction::Continue
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_rejection_circuit_breaker_resets_consecutive_denials_on_non_denial() {
|
||||
let mut circuit_breaker = GuardianRejectionCircuitBreaker::default();
|
||||
assert_eq!(
|
||||
circuit_breaker.record_denial("turn-1", GuardianRejectionCircuitBreakerPolicy::Standard),
|
||||
GuardianRejectionCircuitBreakerAction::Continue
|
||||
);
|
||||
circuit_breaker.record_non_denial("turn-1");
|
||||
assert_eq!(
|
||||
circuit_breaker.record_denial("turn-1", GuardianRejectionCircuitBreakerPolicy::Standard),
|
||||
GuardianRejectionCircuitBreakerAction::Continue
|
||||
);
|
||||
assert_eq!(
|
||||
circuit_breaker.record_denial("turn-1", GuardianRejectionCircuitBreakerPolicy::Standard),
|
||||
GuardianRejectionCircuitBreakerAction::Continue
|
||||
);
|
||||
assert_eq!(
|
||||
circuit_breaker.record_denial("turn-1", GuardianRejectionCircuitBreakerPolicy::Standard),
|
||||
GuardianRejectionCircuitBreakerAction::InterruptTurn {
|
||||
consecutive_denials: 3,
|
||||
recent_denials: 4,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn auto_review_rejection_circuit_breaker_interrupts_after_ten_recent_denials() {
|
||||
let mut circuit_breaker = GuardianRejectionCircuitBreaker::default();
|
||||
for _ in 0..9 {
|
||||
assert_eq!(
|
||||
circuit_breaker
|
||||
.record_denial("turn-1", GuardianRejectionCircuitBreakerPolicy::Standard),
|
||||
GuardianRejectionCircuitBreakerAction::Continue
|
||||
);
|
||||
circuit_breaker.record_non_denial("turn-1");
|
||||
}
|
||||
assert_eq!(
|
||||
circuit_breaker.record_denial("turn-1", GuardianRejectionCircuitBreakerPolicy::Standard),
|
||||
GuardianRejectionCircuitBreakerAction::InterruptTurn {
|
||||
consecutive_denials: 1,
|
||||
recent_denials: 10,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn auto_review_rejection_circuit_breaker_forgets_denials_outside_recent_review_window() {
|
||||
let mut circuit_breaker = GuardianRejectionCircuitBreaker::default();
|
||||
for _ in 0..9 {
|
||||
assert_eq!(
|
||||
circuit_breaker
|
||||
.record_denial("turn-1", GuardianRejectionCircuitBreakerPolicy::Standard),
|
||||
GuardianRejectionCircuitBreakerAction::Continue
|
||||
);
|
||||
circuit_breaker.record_non_denial("turn-1");
|
||||
}
|
||||
for _ in 0..(AUTO_REVIEW_DENIAL_WINDOW_SIZE - 18) {
|
||||
circuit_breaker.record_non_denial("turn-1");
|
||||
}
|
||||
assert_eq!(
|
||||
circuit_breaker.record_denial("turn-1", GuardianRejectionCircuitBreakerPolicy::Standard),
|
||||
GuardianRejectionCircuitBreakerAction::Continue
|
||||
);
|
||||
}
|
||||
@@ -0,0 +1,27 @@
|
||||
//! Owns synchronous Guardian review policy independently of the host session runtime.
|
||||
//! The host supplies review attempts and enforces the resulting decision on the bound action.
|
||||
|
||||
mod assessment;
|
||||
mod circuit_breaker;
|
||||
mod model;
|
||||
mod outcome;
|
||||
mod retry;
|
||||
|
||||
pub use assessment::GuardianAssessment;
|
||||
pub use assessment::guardian_output_contract_prompt;
|
||||
pub use assessment::guardian_output_schema;
|
||||
pub use assessment::parse_guardian_assessment;
|
||||
pub use circuit_breaker::AUTO_REVIEW_DENIAL_WINDOW_SIZE;
|
||||
pub use circuit_breaker::GuardianRejectionCircuitBreaker;
|
||||
pub use circuit_breaker::GuardianRejectionCircuitBreakerAction;
|
||||
pub use circuit_breaker::GuardianRejectionCircuitBreakerPolicy;
|
||||
pub use model::ReviewModel;
|
||||
pub use model::select_review_model;
|
||||
pub use outcome::GuardianReviewError;
|
||||
pub use outcome::GuardianReviewOutcome;
|
||||
pub use outcome::GuardianReviewSessionOutcome;
|
||||
pub use retry::GuardianReviewSessionLimits;
|
||||
pub use retry::run_with_retry;
|
||||
|
||||
pub const MAX_REVIEW_ATTEMPTS: i64 = 3;
|
||||
pub const REVIEW_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(90);
|
||||
@@ -0,0 +1,76 @@
|
||||
//! Selects the synchronous review model from the parent's metadata and provider catalog.
|
||||
|
||||
use codex_protocol::openai_models::ModelInfo;
|
||||
use codex_protocol::openai_models::ModelPreset;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
|
||||
/// Effective model choice, including the attribution carried by existing review events.
|
||||
pub struct ReviewModel {
|
||||
pub model: String,
|
||||
pub reasoning_effort: Option<ReasoningEffort>,
|
||||
pub default_review_model_id: String,
|
||||
pub catalog_contains_auto_review: bool,
|
||||
pub model_overridden: bool,
|
||||
pub model_override: Option<String>,
|
||||
}
|
||||
|
||||
/// Prefers the review model and low effort when advertised; preserves the parent fallback.
|
||||
pub fn select_review_model(
|
||||
parent_model: &ModelInfo,
|
||||
parent_reasoning_effort: Option<&ReasoningEffort>,
|
||||
default_review_model_id: &str,
|
||||
available_models: &[ModelPreset],
|
||||
) -> ReviewModel {
|
||||
let preferred_reasoning_effort = |supports_low: bool, fallback| {
|
||||
if supports_low {
|
||||
Some(codex_protocol::openai_models::ReasoningEffort::Low)
|
||||
} else {
|
||||
fallback
|
||||
}
|
||||
};
|
||||
let model_override = parent_model.auto_review_model_override.as_deref();
|
||||
let review_model_id = model_override.unwrap_or(default_review_model_id);
|
||||
let review_model = available_models
|
||||
.iter()
|
||||
.find(|preset| preset.model == review_model_id);
|
||||
let guardian_catalog_contains_auto_review = available_models
|
||||
.iter()
|
||||
.any(|preset| preset.model == default_review_model_id);
|
||||
let guardian_review_model_overridden = model_override.is_some();
|
||||
let guardian_review_model_override = model_override.map(str::to_string);
|
||||
let (guardian_model, guardian_reasoning_effort) = if let Some(preset) = review_model {
|
||||
let reasoning_effort = preferred_reasoning_effort(
|
||||
preset
|
||||
.supported_reasoning_efforts
|
||||
.iter()
|
||||
.any(|effort| effort.effort == codex_protocol::openai_models::ReasoningEffort::Low),
|
||||
Some(preset.default_reasoning_effort.clone()),
|
||||
);
|
||||
(review_model_id.to_string(), reasoning_effort)
|
||||
} else {
|
||||
let reasoning_effort = preferred_reasoning_effort(
|
||||
parent_model
|
||||
.supported_reasoning_levels
|
||||
.iter()
|
||||
.any(|preset| preset.effort == codex_protocol::openai_models::ReasoningEffort::Low),
|
||||
parent_reasoning_effort
|
||||
.or(parent_model.default_reasoning_level.as_ref())
|
||||
.cloned(),
|
||||
);
|
||||
(
|
||||
model_override
|
||||
.unwrap_or(parent_model.slug.as_str())
|
||||
.to_string(),
|
||||
reasoning_effort,
|
||||
)
|
||||
};
|
||||
|
||||
ReviewModel {
|
||||
model: guardian_model,
|
||||
reasoning_effort: guardian_reasoning_effort,
|
||||
default_review_model_id: default_review_model_id.to_string(),
|
||||
catalog_contains_auto_review: guardian_catalog_contains_auto_review,
|
||||
model_overridden: guardian_review_model_overridden,
|
||||
model_override: guardian_review_model_override,
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,111 @@
|
||||
//! Distinguishes completed assessments from failures without assigning risk to errors.
|
||||
|
||||
use crate::GuardianAssessment;
|
||||
use codex_analytics::GuardianReviewFailureReason;
|
||||
use codex_protocol::protocol::CodexErrorInfo;
|
||||
|
||||
#[derive(Debug)]
|
||||
pub enum GuardianReviewOutcome {
|
||||
Completed(GuardianAssessment),
|
||||
Error(GuardianReviewError),
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
pub enum GuardianReviewError {
|
||||
PromptBuild {
|
||||
message: String,
|
||||
},
|
||||
Session {
|
||||
message: String,
|
||||
error_info: Option<CodexErrorInfo>,
|
||||
},
|
||||
Parse {
|
||||
message: String,
|
||||
},
|
||||
Timeout,
|
||||
Cancelled,
|
||||
}
|
||||
|
||||
impl GuardianReviewError {
|
||||
pub fn prompt_build(err: anyhow::Error) -> Self {
|
||||
Self::PromptBuild {
|
||||
message: err.to_string(),
|
||||
}
|
||||
}
|
||||
|
||||
pub fn session(err: anyhow::Error) -> Self {
|
||||
Self::Session {
|
||||
message: err.to_string(),
|
||||
error_info: None,
|
||||
}
|
||||
}
|
||||
|
||||
pub fn session_with_error_info(err: anyhow::Error, error_info: CodexErrorInfo) -> Self {
|
||||
Self::Session {
|
||||
message: err.to_string(),
|
||||
error_info: Some(error_info),
|
||||
}
|
||||
}
|
||||
|
||||
pub fn parse(err: anyhow::Error) -> Self {
|
||||
Self::Parse {
|
||||
message: err.to_string(),
|
||||
}
|
||||
}
|
||||
|
||||
pub fn failure_reason(&self) -> GuardianReviewFailureReason {
|
||||
match self {
|
||||
Self::PromptBuild { .. } => GuardianReviewFailureReason::PromptBuildError,
|
||||
Self::Session { .. } => GuardianReviewFailureReason::SessionError,
|
||||
Self::Parse { .. } => GuardianReviewFailureReason::ParseError,
|
||||
Self::Timeout => GuardianReviewFailureReason::Timeout,
|
||||
Self::Cancelled => GuardianReviewFailureReason::Cancelled,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
pub enum GuardianReviewSessionOutcome {
|
||||
Completed(anyhow::Result<Option<String>>),
|
||||
PromptBuildFailed(anyhow::Error),
|
||||
SessionFailed {
|
||||
error: anyhow::Error,
|
||||
error_info: Option<CodexErrorInfo>,
|
||||
},
|
||||
TimedOut,
|
||||
Aborted,
|
||||
}
|
||||
|
||||
impl From<GuardianReviewSessionOutcome> for GuardianReviewOutcome {
|
||||
fn from(outcome: GuardianReviewSessionOutcome) -> Self {
|
||||
match outcome {
|
||||
GuardianReviewSessionOutcome::Completed(Ok(Some(message))) => {
|
||||
match crate::parse_guardian_assessment(Some(&message)) {
|
||||
Ok(assessment) => Self::Completed(assessment),
|
||||
Err(error) => Self::Error(GuardianReviewError::parse(error)),
|
||||
}
|
||||
}
|
||||
GuardianReviewSessionOutcome::Completed(Ok(None)) => {
|
||||
Self::Error(GuardianReviewError::session(anyhow::anyhow!(
|
||||
"guardian review completed without an assessment payload"
|
||||
)))
|
||||
}
|
||||
GuardianReviewSessionOutcome::Completed(Err(error)) => {
|
||||
Self::Error(GuardianReviewError::session(error))
|
||||
}
|
||||
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::TimedOut => Self::Error(GuardianReviewError::Timeout),
|
||||
GuardianReviewSessionOutcome::Aborted => Self::Error(GuardianReviewError::Cancelled),
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,94 @@
|
||||
//! Applies one review's attempt budget, deadline and cancellation to host-owned sessions.
|
||||
//! Resubmissions are independent; this layer keeps no state between approval requests.
|
||||
|
||||
use crate::GuardianReviewError;
|
||||
use crate::GuardianReviewOutcome;
|
||||
use codex_analytics::GuardianReviewAnalyticsResult;
|
||||
use codex_protocol::protocol::CodexErrorInfo;
|
||||
use rand::Rng;
|
||||
use std::future::Future;
|
||||
use std::time::Duration;
|
||||
use tokio::time::Instant;
|
||||
use tokio::time::sleep_until;
|
||||
use tokio_util::sync::CancellationToken;
|
||||
|
||||
pub struct GuardianReviewSessionLimits {
|
||||
pub max_attempts: i64,
|
||||
pub deadline: Instant,
|
||||
}
|
||||
|
||||
/// Retries only recoverable review failures within one shared deadline.
|
||||
pub async fn run_with_retry<F, Attempt>(
|
||||
limits: GuardianReviewSessionLimits,
|
||||
external_cancel: Option<&CancellationToken>,
|
||||
mut run_attempt: F,
|
||||
) -> (GuardianReviewOutcome, GuardianReviewAnalyticsResult)
|
||||
where
|
||||
F: FnMut(Instant) -> Attempt,
|
||||
Attempt: Future<Output = (GuardianReviewOutcome, GuardianReviewAnalyticsResult)>,
|
||||
{
|
||||
let GuardianReviewSessionLimits {
|
||||
max_attempts,
|
||||
deadline,
|
||||
} = limits;
|
||||
assert!(max_attempts > 0, "guardian review must run at least once");
|
||||
let mut attempt_count = 1;
|
||||
loop {
|
||||
let (outcome, mut analytics_result) = run_attempt(deadline).await;
|
||||
analytics_result.attempt_count = attempt_count;
|
||||
if attempt_count >= max_attempts || !should_retry_guardian_review(&outcome) {
|
||||
return (outcome, analytics_result);
|
||||
}
|
||||
if let Some(error) =
|
||||
wait_before_guardian_retry(attempt_count, deadline, external_cancel).await
|
||||
{
|
||||
return (GuardianReviewOutcome::Error(error), analytics_result);
|
||||
}
|
||||
attempt_count += 1;
|
||||
}
|
||||
}
|
||||
|
||||
async fn wait_before_guardian_retry(
|
||||
attempt_count: i64,
|
||||
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);
|
||||
tokio::select! {
|
||||
_ = sleep_until(retry_at) => {
|
||||
(Instant::now() >= deadline).then_some(GuardianReviewError::Timeout)
|
||||
}
|
||||
_ = async {
|
||||
if let Some(cancel_token) = external_cancel {
|
||||
cancel_token.cancelled().await;
|
||||
} else {
|
||||
std::future::pending::<()>().await;
|
||||
}
|
||||
} => Some(GuardianReviewError::Cancelled),
|
||||
}
|
||||
}
|
||||
|
||||
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 { .. }
|
||||
)
|
||||
)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "retry_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,135 @@
|
||||
#[cfg(test)]
|
||||
use super::*;
|
||||
use crate::GuardianAssessment;
|
||||
use codex_analytics::GuardianReviewFailureReason;
|
||||
use codex_protocol::protocol::GuardianAssessmentOutcome;
|
||||
use codex_protocol::protocol::GuardianRiskLevel;
|
||||
use codex_protocol::protocol::GuardianUserAuthorization;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::time::Duration;
|
||||
|
||||
#[test]
|
||||
fn guardian_review_error_reason_distinguishes_error_kinds() {
|
||||
let parse_error = GuardianReviewError::parse(anyhow::anyhow!("bad guardian JSON"));
|
||||
let prompt_error = GuardianReviewError::prompt_build(anyhow::anyhow!("bad prompt/config"));
|
||||
let session_error = GuardianReviewError::session(anyhow::anyhow!("guardian runtime failed"));
|
||||
let structured_session_error = GuardianReviewError::session_with_error_info(
|
||||
anyhow::anyhow!("temporary guardian failure"),
|
||||
CodexErrorInfo::ServerOverloaded,
|
||||
);
|
||||
|
||||
assert!(matches!(
|
||||
parse_error.failure_reason(),
|
||||
GuardianReviewFailureReason::ParseError
|
||||
));
|
||||
assert!(matches!(
|
||||
prompt_error.failure_reason(),
|
||||
GuardianReviewFailureReason::PromptBuildError
|
||||
));
|
||||
assert!(matches!(
|
||||
session_error.failure_reason(),
|
||||
GuardianReviewFailureReason::SessionError
|
||||
));
|
||||
assert!(matches!(
|
||||
structured_session_error.failure_reason(),
|
||||
GuardianReviewFailureReason::SessionError
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_review_retry_only_retries_transient_session_and_parse_errors() {
|
||||
let assessment = GuardianAssessment {
|
||||
risk_level: GuardianRiskLevel::High,
|
||||
user_authorization: GuardianUserAuthorization::Unknown,
|
||||
outcome: GuardianAssessmentOutcome::Deny,
|
||||
rationale: "deny".to_string(),
|
||||
};
|
||||
let transient_error_info = [
|
||||
CodexErrorInfo::ServerOverloaded,
|
||||
CodexErrorInfo::HttpConnectionFailed {
|
||||
http_status_code: Some(502),
|
||||
},
|
||||
CodexErrorInfo::ResponseStreamConnectionFailed {
|
||||
http_status_code: Some(503),
|
||||
},
|
||||
CodexErrorInfo::InternalServerError,
|
||||
CodexErrorInfo::ResponseStreamDisconnected {
|
||||
http_status_code: None,
|
||||
},
|
||||
];
|
||||
let mut outcomes = transient_error_info
|
||||
.into_iter()
|
||||
.map(|error_info| {
|
||||
(
|
||||
GuardianReviewOutcome::Error(GuardianReviewError::session_with_error_info(
|
||||
anyhow::anyhow!("transient session"),
|
||||
error_info,
|
||||
)),
|
||||
true,
|
||||
)
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
outcomes.extend([
|
||||
(GuardianReviewOutcome::Completed(assessment), false),
|
||||
(
|
||||
GuardianReviewOutcome::Error(GuardianReviewError::prompt_build(anyhow::anyhow!(
|
||||
"prompt"
|
||||
))),
|
||||
false,
|
||||
),
|
||||
(
|
||||
GuardianReviewOutcome::Error(GuardianReviewError::session(anyhow::anyhow!("session"))),
|
||||
false,
|
||||
),
|
||||
(
|
||||
GuardianReviewOutcome::Error(GuardianReviewError::session_with_error_info(
|
||||
anyhow::anyhow!("bad request"),
|
||||
CodexErrorInfo::BadRequest,
|
||||
)),
|
||||
false,
|
||||
),
|
||||
(
|
||||
GuardianReviewOutcome::Error(GuardianReviewError::parse(anyhow::anyhow!("parse"))),
|
||||
true,
|
||||
),
|
||||
(
|
||||
GuardianReviewOutcome::Error(GuardianReviewError::Timeout),
|
||||
false,
|
||||
),
|
||||
(
|
||||
GuardianReviewOutcome::Error(GuardianReviewError::Cancelled),
|
||||
false,
|
||||
),
|
||||
]);
|
||||
|
||||
for (outcome, expected) in outcomes {
|
||||
assert_eq!(should_retry_guardian_review(&outcome), expected);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_review_retry_wait_honors_cancellation() {
|
||||
let cancel_token = CancellationToken::new();
|
||||
cancel_token.cancel();
|
||||
|
||||
let error = wait_before_guardian_retry(
|
||||
/*attempt_count*/ 1,
|
||||
Instant::now() + Duration::from_secs(/*secs*/ 1),
|
||||
Some(&cancel_token),
|
||||
)
|
||||
.await;
|
||||
|
||||
assert!(matches!(error, Some(GuardianReviewError::Cancelled)));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_review_retry_wait_honors_deadline() {
|
||||
let error = wait_before_guardian_retry(
|
||||
/*attempt_count*/ 1,
|
||||
Instant::now(),
|
||||
/*external_cancel*/ None,
|
||||
)
|
||||
.await;
|
||||
|
||||
assert!(matches!(error, Some(GuardianReviewError::Timeout)));
|
||||
}
|
||||
Reference in New Issue
Block a user