openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-22 17:58:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

guardian: move denial tracking into turn extension state

Browse Source
This commit is contained in:
Dylan Hurd
2026-06-12 12:10:48 -07:00
parent 70c858fc28
commit c7645ca96e
12 changed files with 188 additions and 185 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/Cargo.lock
Generated
+1
View File
@@ -3078,6 +3078,7 @@ dependencies = [
"codex-shell-command",
"codex-utils-absolute-path",
"codex-utils-output-truncation",
"pretty_assertions",
"serde",
"serde_json",
]
codex-rs/core/src/guardian/mod.rs
+3 -62
View File
@@ -34,6 +34,9 @@ pub(crate) use approval_request::guardian_request_turn_id;
pub(crate) use codex_extension_api::ApprovalReviewMcpAnnotations as GuardianMcpAnnotations;
pub(crate) use codex_extension_api::ApprovalReviewNetworkAccessTrigger as GuardianNetworkAccessTrigger;
pub(crate) use codex_extension_api::ApprovalReviewRequest as GuardianApprovalRequest;
pub(crate) use codex_guardian::AUTO_REVIEW_DENIAL_WINDOW_SIZE;
pub(crate) use codex_guardian::GuardianRejectionCircuitBreaker;
pub(crate) use codex_guardian::GuardianRejectionCircuitBreakerAction;
pub(crate) use review::guardian_rejection_message;
pub(crate) use review::guardian_timeout_message;
pub(crate) use review::is_guardian_reviewer_source;
@@ -51,9 +54,6 @@ 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;
@@ -77,65 +77,6 @@ pub(crate) struct GuardianRejection {
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 prompt::GuardianPromptMode;
#[cfg(test)]
codex-rs/core/src/guardian/review.rs
+17 -13
View File
@@ -19,6 +19,7 @@ use codex_protocol::protocol::SubAgentSource;
use codex_protocol::protocol::TurnAbortReason;
use codex_protocol::protocol::WarningEvent;
use std::sync::Arc;
use tokio::sync::Mutex;
use tokio::sync::oneshot;
use tokio::time::Instant;
use tokio::time::sleep_until;
@@ -211,22 +212,25 @@ fn track_guardian_review(
.track_guardian_review(tracking, result, completed_at_ms);
}
async fn record_guardian_non_denial(session: &Arc<Session>, turn_id: &str) {
session
.services
.guardian_rejection_circuit_breaker
fn guardian_rejection_circuit_breaker(
turn: &TurnContext,
) -> Arc<Mutex<super::GuardianRejectionCircuitBreaker>> {
turn.extension_data
.get_or_init(|| Mutex::new(super::GuardianRejectionCircuitBreaker::default()))
}
async fn record_guardian_non_denial(turn: &TurnContext) {
guardian_rejection_circuit_breaker(turn)
.lock()
.await
.record_non_denial(turn_id);
.record_non_denial();
}
async fn record_guardian_denial(session: &Arc<Session>, turn: &Arc<TurnContext>, turn_id: &str) {
let action = session
.services
.guardian_rejection_circuit_breaker
let action = guardian_rejection_circuit_breaker(turn)
.lock()
.await
.record_denial(turn_id);
.record_denial();
let GuardianRejectionCircuitBreakerAction::InterruptTurn {
consecutive_denials,
recent_denials,
@@ -350,7 +354,7 @@ async fn run_guardian_review(
}),
)
.await;
record_guardian_non_denial(&session, &assessment_turn_id).await;
record_guardian_non_denial(&turn).await;
return ReviewDecision::Abort;
}
@@ -443,7 +447,7 @@ async fn run_guardian_review(
}),
)
.await;
record_guardian_non_denial(&session, &assessment_turn_id).await;
record_guardian_non_denial(&turn).await;
return ReviewDecision::TimedOut;
}
GuardianReviewError::Cancelled => {
@@ -478,7 +482,7 @@ async fn run_guardian_review(
}),
)
.await;
record_guardian_non_denial(&session, &assessment_turn_id).await;
record_guardian_non_denial(&turn).await;
return ReviewDecision::Abort;
}
GuardianReviewError::PromptBuild { .. }
@@ -580,7 +584,7 @@ async fn run_guardian_review(
if count_denial_for_circuit_breaker {
record_guardian_denial(&session, &turn, &assessment_turn_id).await;
} else {
record_guardian_non_denial(&session, &assessment_turn_id).await;
record_guardian_non_denial(&turn).await;
}
if approved {
codex-rs/core/src/guardian/tests.rs
-87
View File
@@ -80,93 +80,6 @@ fn fixed_guardian_parent_session_id() -> ThreadId {
.expect("fixed parent session id should be a valid UUID")
}
#[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"),
GuardianRejectionCircuitBreakerAction::Continue
);
assert_eq!(
circuit_breaker.record_denial("turn-1"),
GuardianRejectionCircuitBreakerAction::Continue
);
assert_eq!(
circuit_breaker.record_denial("turn-1"),
GuardianRejectionCircuitBreakerAction::InterruptTurn {
consecutive_denials: 3,
recent_denials: 3,
}
);
assert_eq!(
circuit_breaker.record_denial("turn-1"),
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"),
GuardianRejectionCircuitBreakerAction::Continue
);
circuit_breaker.record_non_denial("turn-1");
assert_eq!(
circuit_breaker.record_denial("turn-1"),
GuardianRejectionCircuitBreakerAction::Continue
);
assert_eq!(
circuit_breaker.record_denial("turn-1"),
GuardianRejectionCircuitBreakerAction::Continue
);
assert_eq!(
circuit_breaker.record_denial("turn-1"),
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"),
GuardianRejectionCircuitBreakerAction::Continue
);
circuit_breaker.record_non_denial("turn-1");
}
assert_eq!(
circuit_breaker.record_denial("turn-1"),
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"),
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"),
GuardianRejectionCircuitBreakerAction::Continue
);
}
async fn guardian_test_session_and_turn(
server: &wiremock::MockServer,
) -> (Arc<Session>, Arc<TurnContext>) {
codex-rs/core/src/session/session.rs
-1
View File
@@ -995,7 +995,6 @@ impl Session {
models_manager: Arc::clone(&models_manager),
tool_approvals: Mutex::new(ApprovalStore::default()),
guardian_rejections: Mutex::new(HashMap::new()),
guardian_rejection_circuit_breaker: Mutex::new(Default::default()),
runtime_handle: tokio::runtime::Handle::current(),
skills_manager,
plugins_manager: Arc::clone(&plugins_manager),
codex-rs/core/src/session/tests.rs
-2
View File
@@ -4969,7 +4969,6 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
models_manager: Arc::clone(&models_manager),
tool_approvals: Mutex::new(ApprovalStore::default()),
guardian_rejections: Mutex::new(std::collections::HashMap::new()),
guardian_rejection_circuit_breaker: Mutex::new(Default::default()),
runtime_handle: tokio::runtime::Handle::current(),
skills_manager,
plugins_manager,
@@ -7044,7 +7043,6 @@ where
models_manager: Arc::clone(&models_manager),
tool_approvals: Mutex::new(ApprovalStore::default()),
guardian_rejections: Mutex::new(std::collections::HashMap::new()),
guardian_rejection_circuit_breaker: Mutex::new(Default::default()),
runtime_handle: tokio::runtime::Handle::current(),
skills_manager,
plugins_manager,
codex-rs/core/src/state/service.rs
-2
View File
@@ -9,7 +9,6 @@ use crate::config::NetworkProxyAuditMetadata;
use crate::config::StartedNetworkProxy;
use crate::exec_policy::ExecPolicyManager;
use crate::guardian::GuardianRejection;
use crate::guardian::GuardianRejectionCircuitBreaker;
use crate::mcp::McpManager;
use crate::tools::code_mode::CodeModeService;
use crate::tools::network_approval::NetworkApprovalService;
@@ -59,7 +58,6 @@ pub(crate) struct SessionServices {
pub(crate) session_telemetry: SessionTelemetry,
pub(crate) tool_approvals: Mutex<ApprovalStore>,
pub(crate) guardian_rejections: Mutex<HashMap<String, GuardianRejection>>,
pub(crate) guardian_rejection_circuit_breaker: Mutex<GuardianRejectionCircuitBreaker>,
pub(crate) runtime_handle: Handle,
pub(crate) skills_manager: Arc<SkillsManager>,
pub(crate) plugins_manager: Arc<PluginsManager>,
codex-rs/core/src/tasks/mod.rs
-17
View File
@@ -337,12 +337,6 @@ impl Session {
let cancellation_token = CancellationToken::new();
let done = Arc::new(Notify::new());
self.services
.guardian_rejection_circuit_breaker
.lock()
.await
.clear_turn(&turn_context.sub_id);
let pending_items = self.input_queue.get_pending_input(&self.active_turn).await;
let turn_state = {
let mut active = self.active_turn.lock().await;
@@ -749,12 +743,6 @@ impl Session {
time_to_first_token_ms,
});
self.send_event(turn_context.as_ref(), event).await;
self.services
.guardian_rejection_circuit_breaker
.lock()
.await
.clear_turn(&turn_context.sub_id);
let cleared_active_turn = {
let mut active = self.active_turn.lock().await;
if let Some(active_turn) = active.as_ref()
@@ -865,11 +853,6 @@ impl Session {
duration_ms,
});
self.send_event(task.turn_context.as_ref(), event).await;
self.services
.guardian_rejection_circuit_breaker
.lock()
.await
.clear_turn(&task.turn_context.sub_id);
}
}
codex-rs/ext/guardian/Cargo.toml
+3 -1
View File
@@ -7,7 +7,6 @@ version.workspace = true
[lib]
name = "codex_guardian"
path = "src/lib.rs"
test = false
doctest = false
[lints]
@@ -22,3 +21,6 @@ codex-utils-absolute-path = { workspace = true }
codex-utils-output-truncation = { workspace = true }
serde = { workspace = true, features = ["derive"] }
serde_json = { workspace = true }
[dev-dependencies]
pretty_assertions = { workspace = true }
codex-rs/ext/guardian/src/circuit_breaker.rs
+58
View File
@@ -0,0 +1,58 @@
use std::collections::VecDeque;
pub const MAX_CONSECUTIVE_GUARDIAN_DENIALS_PER_TURN: u32 = 3;
pub const MAX_RECENT_AUTO_REVIEW_DENIALS_PER_TURN: u32 = 10;
pub const AUTO_REVIEW_DENIAL_WINDOW_SIZE: usize = 50;
/// Per-turn denial history used to stop repeated unsafe approval requests.
#[derive(Debug, Default, PartialEq, Eq)]
pub struct GuardianRejectionCircuitBreaker {
consecutive_denials: u32,
recent_denials: VecDeque<bool>,
interrupt_triggered: bool,
}
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum GuardianRejectionCircuitBreakerAction {
Continue,
InterruptTurn {
consecutive_denials: u32,
recent_denials: u32,
},
}
impl GuardianRejectionCircuitBreaker {
pub fn record_denial(&mut self) -> GuardianRejectionCircuitBreakerAction {
self.consecutive_denials = self.consecutive_denials.saturating_add(1);
self.record_recent_review(/*denied*/ true);
let recent_denials = self.recent_denials.iter().filter(|denied| **denied).count() as u32;
if !self.interrupt_triggered
&& (self.consecutive_denials >= MAX_CONSECUTIVE_GUARDIAN_DENIALS_PER_TURN
|| recent_denials >= MAX_RECENT_AUTO_REVIEW_DENIALS_PER_TURN)
{
self.interrupt_triggered = true;
GuardianRejectionCircuitBreakerAction::InterruptTurn {
consecutive_denials: self.consecutive_denials,
recent_denials,
}
} else {
GuardianRejectionCircuitBreakerAction::Continue
}
}
pub fn record_non_denial(&mut self) {
self.consecutive_denials = 0;
self.record_recent_review(/*denied*/ false);
}
fn record_recent_review(&mut self, denied: bool) {
self.recent_denials.push_back(denied);
if self.recent_denials.len() > AUTO_REVIEW_DENIAL_WINDOW_SIZE {
self.recent_denials.pop_front();
}
}
}
#[cfg(test)]
#[path = "circuit_breaker_tests.rs"]
mod tests;
codex-rs/ext/guardian/src/circuit_breaker_tests.rs
+100
View File
@@ -0,0 +1,100 @@
use std::collections::VecDeque;
use pretty_assertions::assert_eq;
use super::*;
#[test]
fn interrupts_once_after_three_consecutive_denials() {
let mut circuit_breaker = GuardianRejectionCircuitBreaker::default();
assert_eq!(
circuit_breaker.record_denial(),
GuardianRejectionCircuitBreakerAction::Continue
);
assert_eq!(
circuit_breaker.record_denial(),
GuardianRejectionCircuitBreakerAction::Continue
);
assert_eq!(
circuit_breaker.record_denial(),
GuardianRejectionCircuitBreakerAction::InterruptTurn {
consecutive_denials: 3,
recent_denials: 3,
}
);
assert_eq!(
circuit_breaker.record_denial(),
GuardianRejectionCircuitBreakerAction::Continue
);
assert_eq!(
circuit_breaker,
GuardianRejectionCircuitBreaker {
consecutive_denials: 4,
recent_denials: VecDeque::from([true, true, true, true]),
interrupt_triggered: true,
}
);
}
#[test]
fn non_denial_resets_consecutive_denials() {
let mut circuit_breaker = GuardianRejectionCircuitBreaker::default();
assert_eq!(
circuit_breaker.record_denial(),
GuardianRejectionCircuitBreakerAction::Continue
);
circuit_breaker.record_non_denial();
assert_eq!(
circuit_breaker.record_denial(),
GuardianRejectionCircuitBreakerAction::Continue
);
assert_eq!(
circuit_breaker.record_denial(),
GuardianRejectionCircuitBreakerAction::Continue
);
assert_eq!(
circuit_breaker.record_denial(),
GuardianRejectionCircuitBreakerAction::InterruptTurn {
consecutive_denials: 3,
recent_denials: 4,
}
);
}
#[test]
fn interrupts_after_ten_recent_denials() {
let mut circuit_breaker = GuardianRejectionCircuitBreaker::default();
for _ in 0..9 {
assert_eq!(
circuit_breaker.record_denial(),
GuardianRejectionCircuitBreakerAction::Continue
);
circuit_breaker.record_non_denial();
}
assert_eq!(
circuit_breaker.record_denial(),
GuardianRejectionCircuitBreakerAction::InterruptTurn {
consecutive_denials: 1,
recent_denials: 10,
}
);
}
#[test]
fn forgets_denials_outside_recent_review_window() {
let mut circuit_breaker = GuardianRejectionCircuitBreaker::default();
for _ in 0..9 {
assert_eq!(
circuit_breaker.record_denial(),
GuardianRejectionCircuitBreakerAction::Continue
);
circuit_breaker.record_non_denial();
}
for _ in 0..(AUTO_REVIEW_DENIAL_WINDOW_SIZE - 18) {
circuit_breaker.record_non_denial();
}
assert_eq!(
circuit_breaker.record_denial(),
GuardianRejectionCircuitBreakerAction::Continue
);
}
codex-rs/ext/guardian/src/lib.rs
+6
View File
@@ -14,6 +14,7 @@ use codex_protocol::config_types::ApprovalsReviewer;
use codex_protocol::protocol::AskForApproval;
mod approval_request;
mod circuit_breaker;
mod truncation;
pub use approval_request::FormattedGuardianAction;
@@ -23,6 +24,11 @@ pub use approval_request::guardian_assessment_action;
pub use approval_request::guardian_request_target_item_id;
pub use approval_request::guardian_request_turn_id;
pub use approval_request::guardian_reviewed_action;
pub use circuit_breaker::AUTO_REVIEW_DENIAL_WINDOW_SIZE;
pub use circuit_breaker::GuardianRejectionCircuitBreaker;
pub use circuit_breaker::GuardianRejectionCircuitBreakerAction;
pub use circuit_breaker::MAX_CONSECUTIVE_GUARDIAN_DENIALS_PER_TURN;
pub use circuit_breaker::MAX_RECENT_AUTO_REVIEW_DENIALS_PER_TURN;
pub use truncation::guardian_truncate_text;
/// Guardian extension dependencies supplied by the host at construction time.