Remove legacy Guardian approval review paths (#43462)

## What changed

- Remove the `fast_decision` and `full_review` extension hooks and `ApprovalAssessment`, leaving `ApprovalReviewContributor::decide` as the approval decision interface.
- Remove the duplicate Guardian V2 fast-approval implementation and `StrictReviewReason` state.
- Require an explicit `GuardianReviewReason` for synchronous reviews.

## Testing

Migrate Guardian reviewer tests to the production `decide_approval` path and cached-score tests to the registry's decision interface.

GitOrigin-RevId: 1f9092c0ff5bb1eed0c63fa557fb3807e2e71564
This commit is contained in:
jif authored and copyberry committed 2026-09-07 13:02:46 +00:00
1 parent 8260619cb6
commit db0568dbbb
14 files changed
+342 -968

No files matched your search

-4
View File
@@ -46,10 +46,6 @@ pub(crate) use review::is_basic_session_source;
pub(crate) use review::new_guardian_review_id; pub(crate) use review::new_guardian_review_id;
#[cfg(test)] #[cfg(test)]
pub(crate) use review::record_guardian_denial_for_test; pub(crate) use review::record_guardian_denial_for_test;
#[cfg(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_policy_to_guardian; pub(crate) use review::routes_approval_policy_to_guardian;
pub(crate) use review::routes_approval_to_guardian; pub(crate) use review::routes_approval_to_guardian;
pub(crate) use review_session::GuardianReviewSessionManager; pub(crate) use review_session::GuardianReviewSessionManager;
+6 -168
View File
@@ -6,13 +6,7 @@ use codex_analytics::GuardianReviewFailureReason;
use codex_analytics::GuardianReviewTerminalStatus; use codex_analytics::GuardianReviewTerminalStatus;
use codex_analytics::GuardianReviewTrackContext; use codex_analytics::GuardianReviewTrackContext;
use codex_analytics::GuardianReviewedAction; use codex_analytics::GuardianReviewedAction;
#[cfg(test)]
use codex_analytics::GuardianV2Event;
#[cfg(test)]
use codex_analytics::GuardianV2EventKind;
use codex_core_plugins::PluginCommandAttribution; use codex_core_plugins::PluginCommandAttribution;
#[cfg(test)]
use codex_extension_api::GuardianV2Enabled;
use codex_extension_api::ThreadIdleCause; use codex_extension_api::ThreadIdleCause;
use codex_features::Feature; use codex_features::Feature;
use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::ApprovalsReviewer;
@@ -32,8 +26,6 @@ use codex_protocol::protocol::SessionSource;
use codex_protocol::protocol::SubAgentSource; use codex_protocol::protocol::SubAgentSource;
use codex_protocol::protocol::TurnAbortReason; use codex_protocol::protocol::TurnAbortReason;
use codex_protocol::protocol::WarningEvent; use codex_protocol::protocol::WarningEvent;
#[cfg(test)]
use futures::future::BoxFuture;
use std::sync::Arc; use std::sync::Arc;
use std::time::Duration; use std::time::Duration;
use tokio::time::Instant; use tokio::time::Instant;
@@ -57,8 +49,6 @@ use super::GuardianAssessmentOutcome;
use super::GuardianRejectionCircuitBreakerAction; use super::GuardianRejectionCircuitBreakerAction;
use super::GuardianRejectionCircuitBreakerPolicy; use super::GuardianRejectionCircuitBreakerPolicy;
use super::GuardianReviewContext; use super::GuardianReviewContext;
#[cfg(test)]
use super::approval_request::format_guardian_action_compact;
use super::approval_request::format_guardian_action_pretty; use super::approval_request::format_guardian_action_pretty;
use super::approval_request::guardian_assessment_action; use super::approval_request::guardian_assessment_action;
use super::approval_request::guardian_request_target_item_id; use super::approval_request::guardian_request_target_item_id;
@@ -342,112 +332,10 @@ pub(crate) async fn record_guardian_denial_for_test(
record_guardian_denial(session, turn, turn_id).await; record_guardian_denial(session, turn, turn_id).await;
} }
/// Runs Guardian unless Full Access or an installed extension resolves the review.
/// Guardian timeouts, review-session failures, and parse failures all block
/// execution, with timeouts surfaced separately from explicit denials.
#[cfg(test)]
async fn run_guardian_review(
session: Arc<Session>,
context: GuardianReviewContext,
review_id: String,
request: GuardianApprovalRequest,
reasons: ApprovalRequestReasons,
options: GuardianReviewOptions,
) -> ReviewDecision {
let turn = Arc::clone(context.turn());
if context.environments().has_full_access(
context.approval_policy,
&turn.config.permissions.effective_permission_profile(),
) {
return if options
.external_cancel
.as_ref()
.is_some_and(CancellationToken::is_cancelled)
{
ReviewDecision::Abort
} else {
ReviewDecision::Approved
};
}
let requires_synchronous_review = options.require_synchronous_review
|| reasons.retry.is_some()
|| matches!(
&request,
GuardianApprovalRequest::ExecCommand {
sandbox_permissions,
..
} if sandbox_permissions.requires_escalated_permissions()
);
// Guardian V2 may satisfy ordinary reviews, including required-model reviews, but broader
// permission requests, retries, and elicitations requiring synchronous review must not use
// extension fast approval.
if (!turn
.config
.config_layer_stack
.requirements()
.auto_review_required_for_model(&turn.model_info().slug)
|| turn.config.features.enabled(Feature::GuardianV2))
&& !requires_synchronous_review
&& options
.external_cancel
.as_ref()
.is_none_or(|cancel| !cancel.is_cancelled())
&& let Ok(action) = format_guardian_action_compact(&request)
&& let Some(decision) = session
.services
.extensions
.fast_approval_decision(
&session.services.session_extension_data,
&session.services.thread_extension_data,
&action,
Some(crate::session::extension_metrics::from_session_telemetry(
turn.session_telemetry.clone(),
)),
)
.await
{
if decision == ReviewDecision::Approved {
if session
.services
.thread_extension_data
.get::<GuardianV2Enabled>()
.is_some()
{
session
.services
.analytics_events_client
.track_guardian_v2_event(GuardianV2Event {
thread_id: session.thread_id.to_string(),
turn_id: guardian_request_turn_id(&request, &turn.sub_id).to_owned(),
item_id: guardian_request_target_item_id(&request).map(str::to_owned),
model: Some(turn.model_info().slug.clone()),
occurred_at_ms: codex_analytics::now_unix_millis(),
kind: GuardianV2EventKind::FastDecision {
decision: "approved",
},
});
}
record_guardian_non_denial(&session, guardian_request_turn_id(&request, &turn.sub_id))
.await;
}
return decision;
}
let runtime = super::runtime::ReviewRuntime {
session,
context,
review_id,
request: request.into(),
reasons,
options,
};
run_synchronous_review(runtime, /*review_reason*/ None).await
}
/// Runs the existing synchronous reviewer without choosing policy or reusing a score. /// Runs the existing synchronous reviewer without choosing policy or reusing a score.
pub(super) async fn run_synchronous_review( pub(super) async fn run_synchronous_review(
runtime: super::runtime::ReviewRuntime, runtime: super::runtime::ReviewRuntime,
review_reason: Option<codex_protocol::approvals::GuardianReviewReason>, review_reason: codex_protocol::approvals::GuardianReviewReason,
) -> ReviewDecision { ) -> ReviewDecision {
let super::runtime::ReviewRuntime { let super::runtime::ReviewRuntime {
session, session,
@@ -524,7 +412,7 @@ pub(super) async fn run_synchronous_review(
.send_event( .send_event(
turn.as_ref(), turn.as_ref(),
EventMsg::GuardianAssessment(GuardianAssessmentEvent { EventMsg::GuardianAssessment(GuardianAssessmentEvent {
review_reason, review_reason: Some(review_reason),
id: review_id.clone(), id: review_id.clone(),
target_item_id: target_item_id.clone(), target_item_id: target_item_id.clone(),
plugin_id: plugin_id.clone(), plugin_id: plugin_id.clone(),
@@ -564,7 +452,7 @@ pub(super) async fn run_synchronous_review(
.send_event( .send_event(
turn.as_ref(), turn.as_ref(),
EventMsg::GuardianAssessment(GuardianAssessmentEvent { EventMsg::GuardianAssessment(GuardianAssessmentEvent {
review_reason, review_reason: Some(review_reason),
id: review_id, id: review_id,
target_item_id, target_item_id,
plugin_id: plugin_id.clone(), plugin_id: plugin_id.clone(),
@@ -711,7 +599,7 @@ pub(super) async fn run_synchronous_review(
.send_event( .send_event(
turn.as_ref(), turn.as_ref(),
EventMsg::GuardianAssessment(GuardianAssessmentEvent { EventMsg::GuardianAssessment(GuardianAssessmentEvent {
review_reason, review_reason: Some(review_reason),
id: review_id, id: review_id,
target_item_id, target_item_id,
plugin_id: plugin_id.clone(), plugin_id: plugin_id.clone(),
@@ -749,7 +637,7 @@ pub(super) async fn run_synchronous_review(
.send_event( .send_event(
turn.as_ref(), turn.as_ref(),
EventMsg::GuardianAssessment(GuardianAssessmentEvent { EventMsg::GuardianAssessment(GuardianAssessmentEvent {
review_reason, review_reason: Some(review_reason),
id: review_id, id: review_id,
target_item_id, target_item_id,
plugin_id: plugin_id.clone(), plugin_id: plugin_id.clone(),
@@ -835,7 +723,7 @@ pub(super) async fn run_synchronous_review(
GuardianAssessmentStatus::Denied GuardianAssessmentStatus::Denied
}; };
let assessment_event = GuardianAssessmentEvent { let assessment_event = GuardianAssessmentEvent {
review_reason, review_reason: Some(review_reason),
id: review_id, id: review_id,
target_item_id, target_item_id,
plugin_id: plugin_id.clone(), plugin_id: plugin_id.clone(),
@@ -906,56 +794,6 @@ pub(crate) struct GuardianReviewOptions {
pub(crate) require_synchronous_review: bool, pub(crate) require_synchronous_review: bool,
} }
/// Public entrypoint for approval requests that should be reviewed by guardian.
#[cfg(test)]
pub(crate) async fn review_approval_request(
session: &Arc<Session>,
context: impl Into<GuardianReviewContext>,
review_id: String,
request: GuardianApprovalRequest,
reasons: ApprovalRequestReasons,
) -> ReviewDecision {
// Erase the delegated future to bound its async state and the tool handlers' Send checks.
let review: BoxFuture<'_, ReviewDecision> = Box::pin(run_guardian_review(
Arc::clone(session),
context.into(),
review_id,
request,
reasons,
GuardianReviewOptions {
require_guardian: false,
plugin_attribution_override: None,
approval_request_source: GuardianApprovalRequestSource::MainTurn,
external_cancel: None,
require_synchronous_review: false,
},
));
review.await
}
#[cfg(test)]
pub(crate) async fn review_approval_request_with_cancel(
session: &Arc<Session>,
context: impl Into<GuardianReviewContext>,
review_id: String,
request: GuardianApprovalRequest,
retry_reason: Option<String>,
options: GuardianReviewOptions,
) -> ReviewDecision {
run_guardian_review(
Arc::clone(session),
context.into(),
review_id,
request,
ApprovalRequestReasons {
approval: None,
retry: retry_reason,
},
options,
)
.await
}
pub(super) struct GuardianReviewSessionConfig { pub(super) struct GuardianReviewSessionConfig {
pub(super) spawn_config: crate::config::Config, pub(super) spawn_config: crate::config::Config,
pub(super) node_repl_policy: GuardianNodeReplPolicy, pub(super) node_repl_policy: GuardianNodeReplPolicy,
+1 -1
View File
@@ -89,6 +89,6 @@ impl SynchronousApprovalReviewer for ReviewRuntime {
&self, &self,
reason: codex_protocol::approvals::GuardianReviewReason, reason: codex_protocol::approvals::GuardianReviewReason,
) -> ExtensionFuture<'_, ReviewDecision> { ) -> ExtensionFuture<'_, ReviewDecision> {
Box::pin(run_synchronous_review(self.clone(), Some(reason))) Box::pin(run_synchronous_review(self.clone(), reason))
} }
} }
+39 -12
View File
@@ -1688,8 +1688,8 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() {
let cancel_token = CancellationToken::new(); let cancel_token = CancellationToken::new();
cancel_token.cancel(); cancel_token.cancel();
let decision = review_approval_request_with_cancel( let decision = super::decide_approval(
&session, Arc::clone(&session),
&turn, &turn,
"review-cancelled-guardian".to_string(), "review-cancelled-guardian".to_string(),
GuardianApprovalRequest::ApplyPatch { GuardianApprovalRequest::ApplyPatch {
@@ -1699,9 +1699,12 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() {
patch: "*** Begin Patch\n*** Update File: guardian.txt\n@@\n+hello\n*** End Patch" patch: "*** Begin Patch\n*** Update File: guardian.txt\n@@\n+hello\n*** End Patch"
.to_string(), .to_string(),
}, },
/*retry_reason*/ None, ApprovalRequestReasons {
approval: None,
retry: None,
},
GuardianReviewOptions { GuardianReviewOptions {
require_guardian: false, require_guardian: true,
plugin_attribution_override: None, plugin_attribution_override: None,
approval_request_source: GuardianApprovalRequestSource::MainTurn, approval_request_source: GuardianApprovalRequestSource::MainTurn,
external_cancel: Some(cancel_token), external_cancel: Some(cancel_token),
@@ -1710,7 +1713,7 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() {
) )
.await; .await;
assert_eq!(decision, ReviewDecision::Abort); assert_eq!(decision, Some(ReviewDecision::Abort));
let mut guardian_statuses = Vec::new(); let mut guardian_statuses = Vec::new();
let mut warnings = Vec::new(); let mut warnings = Vec::new();
@@ -3399,14 +3402,12 @@ async fn escalated_retry_bypasses_extension_approval_and_runs_guardian() -> anyh
struct AutoApprovingReviewContributor; struct AutoApprovingReviewContributor;
impl codex_extension_api::ApprovalReviewContributor for AutoApprovingReviewContributor { impl codex_extension_api::ApprovalReviewContributor for AutoApprovingReviewContributor {
fn fast_decision<'a>( fn decide<'a>(
&'a self, &'a self,
_session_store: &'a codex_extension_api::ExtensionData, _input: &'a codex_extension_api::ApprovalDecisionInput<'_>,
_thread_store: &'a codex_extension_api::ExtensionData, ) -> codex_extension_api::ExtensionFuture<'a, Option<codex_extension_api::ApprovalDecision>>
_prompt: &'a str, {
_extension_metrics: Option<Arc<dyn codex_extension_api::ExtensionMetrics>>, Box::pin(async { Some(codex_extension_api::ApprovalDecision::Allow) })
) -> codex_extension_api::ExtensionFuture<'a, Option<ReviewDecision>> {
Box::pin(async move { Some(ReviewDecision::Approved) })
} }
} }
@@ -4188,3 +4189,29 @@ async fn guardian_review_session_config_uses_default_guardian_policy_without_req
)) ))
); );
} }
// Keep the existing reviewer tests on the production decision path.
async fn review_approval_request(
session: &Arc<Session>,
context: impl Into<GuardianReviewContext>,
review_id: String,
request: GuardianApprovalRequest,
reasons: ApprovalRequestReasons,
) -> ReviewDecision {
super::decide_approval(
Arc::clone(session),
context,
review_id,
request,
reasons,
GuardianReviewOptions {
require_guardian: true,
plugin_attribution_override: None,
approval_request_source: GuardianApprovalRequestSource::MainTurn,
external_cancel: None,
require_synchronous_review: false,
},
)
.await
.expect("Guardian should handle the request")
}
+2 -26
View File
@@ -4,7 +4,6 @@ use std::sync::Arc;
use codex_context_fragments::ContextualUserFragment; use codex_context_fragments::ContextualUserFragment;
use codex_protocol::items::TurnItem; use codex_protocol::items::TurnItem;
use codex_protocol::protocol::ReviewDecision;
use codex_protocol::protocol::TokenUsageInfo; use codex_protocol::protocol::TokenUsageInfo;
use codex_tools::ToolCall; use codex_tools::ToolCall;
use codex_tools::ToolExecutor; use codex_tools::ToolExecutor;
@@ -23,7 +22,6 @@ mod turn_input;
mod turn_lifecycle; mod turn_lifecycle;
mod world_state; mod world_state;
pub use approval_review::ApprovalAssessment;
pub use approval_review::ApprovalDecision; pub use approval_review::ApprovalDecision;
pub use approval_review::ApprovalDecisionInput; pub use approval_review::ApprovalDecisionInput;
pub use approval_review::ApprovalReviewError; pub use approval_review::ApprovalReviewError;
@@ -349,11 +347,8 @@ pub trait ToolLifecycleContributor: Send + Sync {
} }
} }
/// Extension contribution for fast approval decisions and full action reviews. /// Owns the complete approval decision, including whether to consult a reviewer.
/// /// Returning `None` leaves the request to the next contributor.
/// Implementations can provide a fast decision from existing evidence, perform
/// a full structured review, or support both paths. Returning `None` leaves the
/// request available to the next contributor or the host's fallback path.
pub trait ApprovalReviewContributor: Send + Sync { pub trait ApprovalReviewContributor: Send + Sync {
/// Claims one request, including a handoff to the user. /// Claims one request, including a handoff to the user.
fn decide<'a>( fn decide<'a>(
@@ -362,25 +357,6 @@ pub trait ApprovalReviewContributor: Send + Sync {
) -> ExtensionFuture<'a, Option<ApprovalDecision>> { ) -> ExtensionFuture<'a, Option<ApprovalDecision>> {
Box::pin(std::future::ready(None)) Box::pin(std::future::ready(None))
} }
/// Returns an available approval decision without performing a full review.
fn fast_decision<'a>(
&'a self,
_session_store: &'a ExtensionData,
_thread_store: &'a ExtensionData,
_prompt: &'a str,
_extension_metrics: Option<Arc<dyn ExtensionMetrics>>,
) -> ExtensionFuture<'a, Option<ReviewDecision>> {
Box::pin(std::future::ready(None))
}
/// Performs a full review of a structured host-owned approval request.
fn full_review<'a>(
&'a self,
_input: &'a ApprovalReviewInput<'_>,
) -> ExtensionFuture<'a, Option<Result<ApprovalAssessment, ApprovalReviewError>>> {
Box::pin(std::future::ready(None))
}
} }
/// Ordered post-processing contribution for one parsed turn item. /// Ordered post-processing contribution for one parsed turn item.
@@ -4,9 +4,6 @@ use std::fmt;
use std::sync::Arc; use std::sync::Arc;
use codex_protocol::ThreadId; use codex_protocol::ThreadId;
use codex_protocol::approvals::GuardianAssessmentOutcome;
use codex_protocol::approvals::GuardianRiskLevel;
use codex_protocol::approvals::GuardianUserAuthorization;
use serde_json::Value; use serde_json::Value;
use crate::ConversationHistorySnapshot; use crate::ConversationHistorySnapshot;
@@ -15,19 +12,6 @@ use crate::ExtensionData;
/// Thread-local state installed only after Guardian V2's async classifier initializes. /// Thread-local state installed only after Guardian V2's async classifier initializes.
pub struct GuardianV2Enabled; pub struct GuardianV2Enabled;
/// Classification returned by an approval reviewer.
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct ApprovalAssessment {
/// Final allow-or-deny decision for the requested action.
pub outcome: GuardianAssessmentOutcome,
/// Risk level assigned to the action by the reviewer.
pub risk_level: GuardianRiskLevel,
/// Whether the conversation authorizes the requested action.
pub user_authorization: GuardianUserAuthorization,
/// Human-readable explanation of the assessment.
pub rationale: String,
}
/// Operational failure returned by an approval reviewer. /// Operational failure returned by an approval reviewer.
#[derive(Clone, Debug, Eq, PartialEq)] #[derive(Clone, Debug, Eq, PartialEq)]
pub enum ApprovalReviewError { pub enum ApprovalReviewError {
-1
View File
@@ -40,7 +40,6 @@ pub use codex_tools::TurnItemEmissionFuture;
pub use codex_tools::TurnItemEmitter; pub use codex_tools::TurnItemEmitter;
pub use codex_tools::parse_tool_input_schema; pub use codex_tools::parse_tool_input_schema;
pub use codex_tools::parse_tool_input_schema_without_compaction; pub use codex_tools::parse_tool_input_schema_without_compaction;
pub use contributors::ApprovalAssessment;
pub use contributors::ApprovalDecision; pub use contributors::ApprovalDecision;
pub use contributors::ApprovalDecisionInput; pub use contributors::ApprovalDecisionInput;
pub use contributors::ApprovalReviewContributor; pub use contributors::ApprovalReviewContributor;
@@ -1,16 +1,9 @@
use std::sync::Arc; use std::sync::Arc;
use codex_protocol::protocol::ReviewDecision;
use crate::ApprovalAssessment;
use crate::ApprovalReviewContributor; use crate::ApprovalReviewContributor;
use crate::ApprovalReviewError;
use crate::ApprovalReviewInput;
use crate::ConfigContributor; use crate::ConfigContributor;
use crate::ContextContributor; use crate::ContextContributor;
use crate::ExtensionData;
use crate::ExtensionEventSink; use crate::ExtensionEventSink;
use crate::ExtensionMetrics;
use crate::McpServerContributor; use crate::McpServerContributor;
use crate::NoopExtensionEventSink; use crate::NoopExtensionEventSink;
use crate::SkillInvocationContributor; use crate::SkillInvocationContributor;
@@ -202,45 +195,6 @@ impl<C: Sync> ExtensionRegistry<C> {
.any(|contributor| contributor.requires_host_skill_discovery()) .any(|contributor| contributor.requires_host_skill_discovery())
} }
/// Returns the first full approval assessment claimed by a contributor.
pub async fn full_approval_review(
&self,
input: ApprovalReviewInput<'_>,
) -> Option<Result<ApprovalAssessment, ApprovalReviewError>> {
for contributor in &self.approval_review_contributors {
if let Some(assessment) = contributor.full_review(&input).await {
return Some(assessment);
}
}
None
}
/// Returns the first fast approval decision claimed by a contributor.
pub async fn fast_approval_decision(
&self,
session_store: &ExtensionData,
thread_store: &ExtensionData,
prompt: &str,
extension_metrics: Option<Arc<dyn ExtensionMetrics>>,
) -> Option<ReviewDecision> {
for contributor in &self.approval_review_contributors {
if let Some(decision) = contributor
.fast_decision(
session_store,
thread_store,
prompt,
extension_metrics.clone(),
)
.await
{
return Some(decision);
}
}
None
}
/// Returns the first claimed decision in registration order. /// Returns the first claimed decision in registration order.
pub async fn decide_approval( pub async fn decide_approval(
&self, &self,
+30 -226
View File
@@ -3,10 +3,7 @@
use std::sync::Arc; use std::sync::Arc;
use std::sync::Mutex; use std::sync::Mutex;
use codex_extension_api::ApprovalAssessment;
use codex_extension_api::ApprovalReviewContributor; use codex_extension_api::ApprovalReviewContributor;
use codex_extension_api::ApprovalReviewError;
use codex_extension_api::ApprovalReviewInput;
use codex_extension_api::ConfigContributor; use codex_extension_api::ConfigContributor;
use codex_extension_api::ContentItemKind; use codex_extension_api::ContentItemKind;
use codex_extension_api::ContextContributor; use codex_extension_api::ContextContributor;
@@ -34,21 +31,14 @@ use codex_extension_api::TurnInputContext;
use codex_extension_api::TurnInputContributor; use codex_extension_api::TurnInputContributor;
use codex_extension_api::TurnItemContributor; use codex_extension_api::TurnItemContributor;
use codex_extension_api::TurnLifecycleContributor; use codex_extension_api::TurnLifecycleContributor;
use codex_extension_api::empty_extension_registry;
use codex_protocol::ThreadId;
use codex_protocol::approvals::GuardianAssessmentOutcome;
use codex_protocol::approvals::GuardianRiskLevel;
use codex_protocol::approvals::GuardianUserAuthorization;
use codex_protocol::items::HookPromptItem; use codex_protocol::items::HookPromptItem;
use codex_protocol::items::TurnItem; use codex_protocol::items::TurnItem;
use codex_protocol::protocol::Event; use codex_protocol::protocol::Event;
use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::ReviewDecision;
use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SessionSource;
use codex_protocol::protocol::SubAgentSource; use codex_protocol::protocol::SubAgentSource;
use codex_protocol::protocol::WarningEvent; use codex_protocol::protocol::WarningEvent;
use pretty_assertions::assert_eq; use pretty_assertions::assert_eq;
use serde_json::json;
struct AllContributors; struct AllContributors;
@@ -166,33 +156,22 @@ impl TurnItemContributor for AllContributors {
} }
impl ApprovalReviewContributor for AllContributors { impl ApprovalReviewContributor for AllContributors {
fn fast_decision<'a>( fn decide<'a>(
&'a self, &'a self,
_session_store: &'a ExtensionData, _input: &'a codex_extension_api::ApprovalDecisionInput<'_>,
_thread_store: &'a ExtensionData, ) -> ExtensionFuture<'a, Option<codex_extension_api::ApprovalDecision>> {
_prompt: &'a str, Box::pin(async { Some(codex_extension_api::ApprovalDecision::AskUser) })
_extension_metrics: Option<Arc<dyn ExtensionMetrics>>,
) -> ExtensionFuture<'a, Option<ReviewDecision>> {
Box::pin(async move {
let _self = self;
Some(ReviewDecision::ApprovedForSession)
})
} }
}
fn full_review<'a>( impl codex_extension_api::SynchronousApprovalReviewer for AllContributors {
&'a self, fn review(
input: &'a ApprovalReviewInput<'_>, &self,
) -> ExtensionFuture<'a, Option<Result<ApprovalAssessment, ApprovalReviewError>>> { _reason: codex_protocol::approvals::GuardianReviewReason,
assert_eq!( ) -> ExtensionFuture<'_, codex_protocol::protocol::ReviewDecision> {
(input.approval_reason, input.retry_reason), Box::pin(std::future::ready(
(Some("A policy rule requires approval."), None) codex_protocol::protocol::ReviewDecision::Approved,
); ))
Box::pin(std::future::ready(Some(Ok(ApprovalAssessment {
outcome: GuardianAssessmentOutcome::Allow,
risk_level: GuardianRiskLevel::Low,
user_authorization: GuardianUserAuthorization::High,
rationale: "approved".to_string(),
}))))
} }
} }
@@ -223,16 +202,24 @@ async fn build_round_trips_every_contributor_category() {
assert_eq!(registry.tool_contributors().len(), 1); assert_eq!(registry.tool_contributors().len(), 1);
assert_eq!(registry.tool_lifecycle_contributors().len(), 1); assert_eq!(registry.tool_lifecycle_contributors().len(), 1);
assert_eq!(registry.turn_item_contributors().len(), 1); assert_eq!(registry.turn_item_contributors().len(), 1);
let thread_store = ExtensionData::new("thread");
let input = codex_extension_api::ApprovalDecisionInput {
approval_id: "approval-1",
action: &serde_json::Value::Null,
thread_id: codex_protocol::ThreadId::new(),
thread_store: &thread_store,
category: codex_protocol::openai_models::GuardianScope::Shell,
approval_policy: codex_protocol::protocol::AskForApproval::OnRequest,
approvals_reviewer: codex_protocol::config_types::ApprovalsReviewer::AutoReview,
require_guardian: false,
require_fresh_review: false,
full_access: false,
metrics: None,
synchronous_reviewer: &AllContributors,
};
assert_eq!( assert_eq!(
registry registry.decide_approval(&input).await,
.fast_approval_decision( Some(codex_extension_api::ApprovalDecision::AskUser)
&ExtensionData::new("session"),
&ExtensionData::new("thread"),
"review this",
/*extension_metrics*/ None,
)
.await,
Some(ReviewDecision::ApprovedForSession)
); );
} }
@@ -250,86 +237,6 @@ impl ConversationHistorySnapshot for AllContributors {
} }
} }
#[tokio::test]
async fn full_approval_review_returns_first_claim_and_short_circuits() {
let assessment = ApprovalAssessment {
outcome: GuardianAssessmentOutcome::Allow,
risk_level: GuardianRiskLevel::Low,
user_authorization: GuardianUserAuthorization::High,
rationale: "approved".to_string(),
};
let action = json!({
"tool": "exec_command",
"command": ["echo", "hello"],
});
let thread_store = ExtensionData::new("thread");
let input = ApprovalReviewInput {
action: &action,
conversation_history: Arc::new(AllContributors),
thread_id: ThreadId::default(),
thread_store: &thread_store,
turn_id: "turn",
approval_reason: Some("A policy rule requires approval."),
retry_reason: None,
};
let calls = Arc::new(Mutex::new(Vec::new()));
let mut builder = ExtensionRegistryBuilder::<()>::new();
for (name, result) in [
("first", None),
("second", Some(Ok(assessment.clone()))),
(
"third",
Some(Err(ApprovalReviewError::Failed(
"should not run".to_string(),
))),
),
] {
builder.approval_review_contributor(Arc::new(RecordingStructuredApprovalContributor {
name,
result,
calls: Arc::clone(&calls),
}));
}
assert_eq!(
builder.build().full_approval_review(input).await,
Some(Ok(assessment))
);
assert_eq!(
calls.lock().expect("approval calls lock").as_slice(),
["first", "second"]
);
}
struct RecordingStructuredApprovalContributor {
name: &'static str,
result: Option<Result<ApprovalAssessment, ApprovalReviewError>>,
calls: Arc<Mutex<Vec<&'static str>>>,
}
impl ApprovalReviewContributor for RecordingStructuredApprovalContributor {
fn full_review<'a>(
&'a self,
input: &'a ApprovalReviewInput<'_>,
) -> ExtensionFuture<'a, Option<Result<ApprovalAssessment, ApprovalReviewError>>> {
Box::pin(async move {
assert_eq!(
input.action,
&json!({
"tool": "exec_command",
"command": ["echo", "hello"],
})
);
self.calls
.lock()
.expect("approval calls lock should not be poisoned")
.push(self.name);
self.result.clone()
})
}
}
struct NamedContextContributor(&'static str); struct NamedContextContributor(&'static str);
impl ContextContributor for NamedContextContributor { impl ContextContributor for NamedContextContributor {
@@ -465,92 +372,6 @@ async fn contributors_preserve_registration_order() {
); );
} }
#[derive(Debug, PartialEq, Eq)]
struct ApprovalCall {
contributor: &'static str,
session_id: String,
thread_id: String,
prompt: String,
}
struct RecordingApprovalContributor {
name: &'static str,
decision: Option<ReviewDecision>,
calls: Arc<Mutex<Vec<ApprovalCall>>>,
}
impl ApprovalReviewContributor for RecordingApprovalContributor {
fn fast_decision<'a>(
&'a self,
session_store: &'a ExtensionData,
thread_store: &'a ExtensionData,
prompt: &'a str,
_extension_metrics: Option<Arc<dyn ExtensionMetrics>>,
) -> ExtensionFuture<'a, Option<ReviewDecision>> {
Box::pin(async move {
self.calls
.lock()
.expect("approval calls lock should not be poisoned")
.push(ApprovalCall {
contributor: self.name,
session_id: session_store.level_id().to_string(),
thread_id: thread_store.level_id().to_string(),
prompt: prompt.to_string(),
});
self.decision.clone()
})
}
}
#[tokio::test]
async fn fast_approval_decision_returns_first_claim_and_short_circuits() {
let calls = Arc::new(Mutex::new(Vec::new()));
let mut builder = ExtensionRegistryBuilder::<()>::new();
for (name, decision) in [
("first", None),
("second", Some(ReviewDecision::Approved)),
(
"third",
Some(ReviewDecision::denied("rejected by extension")),
),
] {
builder.approval_review_contributor(Arc::new(RecordingApprovalContributor {
name,
decision,
calls: Arc::clone(&calls),
}));
}
let registry = builder.build();
let decision = registry
.fast_approval_decision(
&ExtensionData::new("session-1"),
&ExtensionData::new("thread-1"),
"allow command?",
/*extension_metrics*/ None,
)
.await;
assert_eq!(decision, Some(ReviewDecision::Approved));
assert_eq!(
calls.lock().expect("approval calls lock").as_slice(),
[
ApprovalCall {
contributor: "first",
session_id: "session-1".to_string(),
thread_id: "thread-1".to_string(),
prompt: "allow command?".to_string(),
},
ApprovalCall {
contributor: "second",
session_id: "session-1".to_string(),
thread_id: "thread-1".to_string(),
prompt: "allow command?".to_string(),
},
]
);
}
#[derive(Default)] #[derive(Default)]
struct RecordingEventSink { struct RecordingEventSink {
events: Mutex<Vec<(String, String)>>, events: Mutex<Vec<(String, String)>>,
@@ -606,23 +427,6 @@ fn custom_event_sink_survives_registry_build() {
); );
} }
#[tokio::test]
async fn empty_registry_does_not_claim_fast_approval_decision() {
let registry = empty_extension_registry::<()>();
assert_eq!(
registry
.fast_approval_decision(
&ExtensionData::new("session"),
&ExtensionData::new("thread"),
"unclaimed",
/*extension_metrics*/ None,
)
.await,
None
);
}
fn warning_event(id: &str, message: &str) -> Event { fn warning_event(id: &str, message: &str) -> Event {
Event { Event {
id: id.to_string(), id: id.to_string(),
@@ -150,15 +150,4 @@ impl GuardianPolicy {
== Some("require_escalated") == Some("require_escalated")
})) }))
} }
pub(super) fn review_scope(action: &serde_json::Value) -> Option<GuardianScope> {
match action.get("tool").and_then(serde_json::Value::as_str)? {
"mcp_tool_call" => action
.get("server")
.and_then(serde_json::Value::as_str)
.map(GuardianScope::for_mcp_server),
"network_access" => Some(GuardianScope::Network),
tool => GuardianScope::for_tool(&ToolName::plain(tool)),
}
}
} }
@@ -15,7 +15,6 @@ use codex_core::config::Config;
use codex_core::context::GuardianContextMode; use codex_core::context::GuardianContextMode;
use codex_core::context::GuardianReviewEvidence; use codex_core::context::GuardianReviewEvidence;
use codex_core::context::NodeReplReviewEvidence; use codex_core::context::NodeReplReviewEvidence;
use codex_extension_api::ApprovalReviewContributor;
use codex_extension_api::ExtensionData; use codex_extension_api::ExtensionData;
use codex_extension_api::ExtensionEventSink; use codex_extension_api::ExtensionEventSink;
use codex_extension_api::ExtensionFuture; use codex_extension_api::ExtensionFuture;
@@ -40,10 +39,8 @@ use codex_model_provider::create_model_provider;
use codex_protocol::ThreadId; use codex_protocol::ThreadId;
use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::ApprovalsReviewer;
use codex_protocol::mcp::is_node_repl_backed_server; use codex_protocol::mcp::is_node_repl_backed_server;
use codex_protocol::openai_models::GuardianReviewMode;
use codex_protocol::openai_models::GuardianScope; use codex_protocol::openai_models::GuardianScope;
use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ModelInfo;
use codex_protocol::protocol::ReviewDecision;
use codex_protocol::protocol::has_full_access; use codex_protocol::protocol::has_full_access;
use codex_protocol::security_risk::SecurityRiskScore; use codex_protocol::security_risk::SecurityRiskScore;
@@ -51,13 +48,9 @@ use super::action::GuardianAction;
use super::action::RenderedAction; use super::action::RenderedAction;
use super::authorization::ScoreAuthorization; use super::authorization::ScoreAuthorization;
use super::config::GuardianV2Config; use super::config::GuardianV2Config;
use super::coverage::GuardianPolicy;
use super::coverage::UnscoredAction; use super::coverage::UnscoredAction;
use super::metrics::REVIEW_FALLBACK_METRIC;
use super::metrics::TOOL_CALL_LAG_METRIC;
use super::metrics::record_classification; use super::metrics::record_classification;
use super::metrics::record_classification_risk; use super::metrics::record_classification_risk;
use super::metrics::record_fast_decision;
use super::metrics::sampler_failure_reason; use super::metrics::sampler_failure_reason;
use super::parent_compaction::ParentCompactionError; use super::parent_compaction::ParentCompactionError;
use super::parent_compaction::select_parent_compaction; use super::parent_compaction::select_parent_compaction;
@@ -72,14 +65,6 @@ use super::trusted_skills::TrustedSkillInvocations;
use super::trusted_skills::TrustedSkillRoots; use super::trusted_skills::TrustedSkillRoots;
use super::trusted_tools::trusted_tool_context; use super::trusted_tools::trusted_tool_context;
/// Explains why Guardian v2 requires synchronous approval review.
#[derive(Debug, Eq, PartialEq)]
pub enum StrictReviewReason {
ElevatedRisk,
StaleScore,
IncompatibleCompaction,
}
enum ClassificationOutcome { enum ClassificationOutcome {
Scored, Scored,
Superseded, Superseded,
@@ -242,195 +227,6 @@ impl SkillInvocationContributor for GuardianV2Extension {
} }
} }
impl ApprovalReviewContributor for GuardianV2Extension {
fn fast_decision<'a>(
&'a self,
_session_store: &'a ExtensionData,
thread_store: &'a ExtensionData,
prompt: &'a str,
extension_metrics: Option<Arc<dyn ExtensionMetrics>>,
) -> ExtensionFuture<'a, Option<ReviewDecision>> {
Box::pin(async move {
let model = thread_store.get::<ModelInfo>();
let guardian_config = thread_store.get::<GuardianV2Config>()?;
let policy = guardian_config.policy_for_model(model.as_deref());
let action = serde_json::from_str::<serde_json::Value>(prompt).unwrap_or_default();
let scope = GuardianPolicy::review_scope(&action);
if scope.map_or(policy.other_tools, |scope| policy.mode(scope))
!= GuardianReviewMode::Adaptive
{
record_fast_decision(extension_metrics.as_deref(), "deferred", "out_of_scope");
return None;
}
let guardian_evidence = thread_store.get_or_init(GuardianReviewEvidence::default);
let context_mode = guardian_evidence.context_mode();
let mut initial_cua_call = false;
if scope == Some(GuardianScope::ComputerUse) && policy.initial_cua_call {
// Thread-owned context checks checkpoint compatibility before the first CUA allowance.
initial_cua_call = action.get("tool_name").and_then(serde_json::Value::as_str)
== Some("js")
&& action
.get("connector_id")
.and_then(serde_json::Value::as_str)
== Some("node_repl")
&& thread_store
.get::<GuardianV2ScoreProgress>()?
.js_executions
.load(Ordering::Acquire)
== 1;
if initial_cua_call && context_mode == GuardianContextMode::Legacy {
record_fast_decision(
extension_metrics.as_deref(),
"approved",
"initial_cua_call",
);
return Some(ReviewDecision::Approved);
}
} else if thread_store.get::<ModelInfo>().is_some() {
let manager = self.thread_manager.upgrade()?;
let thread_id = ThreadId::from_string(thread_store.level_id()).ok()?;
let Ok(thread) = manager.get_thread(thread_id).await else {
record_fast_decision(
extension_metrics.as_deref(),
"deferred",
"scoring_failure",
);
return None;
};
let config = thread.config().await;
let model = thread_store.get::<ModelInfo>()?;
if config
.config_layer_stack
.requirements()
.auto_review_required_for_model(&model.slug)
{
record_fast_decision(
extension_metrics.as_deref(),
"deferred",
"required_model",
);
return None;
}
}
let Some(score_progress) = thread_store.get::<GuardianV2ScoreProgress>() else {
record_fast_decision(extension_metrics.as_deref(), "deferred", "missing_score");
return None;
};
let manager = self.thread_manager.upgrade()?;
let thread_id = ThreadId::from_string(thread_store.level_id()).ok()?;
let Ok(thread) = manager.get_thread(thread_id).await else {
record_fast_decision(extension_metrics.as_deref(), "deferred", "scoring_failure");
return None;
};
let history = thread.conversation_history_snapshot().await;
if context_mode == GuardianContextMode::ThreadOwned {
let sampler = thread_store.get::<LunaSampler>()?;
if select_parent_compaction(
context_mode,
&guardian_config,
history.as_ref(),
&sampler,
/*legacy_model_hash*/ None,
)
.is_err()
{
thread_store.insert(StrictReviewReason::IncompatibleCompaction);
record_fast_decision(
extension_metrics.as_deref(),
"deferred",
"incompatible_compaction",
);
return None;
}
}
if initial_cua_call {
record_fast_decision(extension_metrics.as_deref(), "approved", "initial_cua_call");
return Some(ReviewDecision::Approved);
}
let current_authorization = ScoreAuthorization::current(&thread).await;
if !current_authorization.local.retained_context_complete
|| current_authorization
.root
.is_some_and(|root| !root.retained_context_complete)
{
record_fast_decision(
extension_metrics.as_deref(),
"deferred",
"incomplete_authorization",
);
return None;
}
let scored_authorization = score_progress
.authorization
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
let latest_scored_tool_call = score_progress
.latest_scored_tool_call
.load(Ordering::Acquire);
let tool_call_lag = score_progress
.latest_tool_call
.load(Ordering::Acquire)
.saturating_sub(latest_scored_tool_call);
if let Some(metrics) = &extension_metrics {
metrics.histogram(
TOOL_CALL_LAG_METRIC,
i64::try_from(tool_call_lag).unwrap_or(i64::MAX),
&[],
);
}
if tool_call_lag > guardian_config.max_tool_call_lag {
thread_store.insert(StrictReviewReason::StaleScore);
if let Some(metrics) = &extension_metrics {
metrics.counter(
REVIEW_FALLBACK_METRIC,
/*inc*/ 1,
&[("fallback_reason", "score_lag")],
);
}
record_fast_decision(extension_metrics.as_deref(), "deferred", "stale_score");
return None;
}
if score_progress
.latest_failed_tool_call
.load(Ordering::Acquire)
> latest_scored_tool_call
{
thread_store.insert(StrictReviewReason::ElevatedRisk);
record_fast_decision(extension_metrics.as_deref(), "deferred", "scoring_failure");
return None;
}
let Some(score) = thread_store
.get::<SecurityRiskScore>()
.and_then(|score| score.scores.get("action_risk").copied())
else {
record_fast_decision(extension_metrics.as_deref(), "deferred", "missing_score");
return None;
};
if score < guardian_config.review_threshold {
if scored_authorization.as_ref() != Some(&current_authorization) {
thread_store.insert(StrictReviewReason::StaleScore);
record_fast_decision(
extension_metrics.as_deref(),
"deferred",
"authorization_changed",
);
return None;
}
record_fast_decision(extension_metrics.as_deref(), "approved", "low_risk");
return Some(ReviewDecision::Approved);
}
if score >= guardian_config.review_threshold {
thread_store.insert(StrictReviewReason::ElevatedRisk);
record_fast_decision(extension_metrics.as_deref(), "deferred", "elevated_risk");
} else {
record_fast_decision(extension_metrics.as_deref(), "deferred", "invalid_score");
}
None
})
}
}
impl ToolLifecycleContributor for GuardianV2Extension { impl ToolLifecycleContributor for GuardianV2Extension {
fn on_tool_start<'a>(&'a self, input: ToolStartInput<'a>) -> ToolLifecycleFuture<'a> { fn on_tool_start<'a>(&'a self, input: ToolStartInput<'a>) -> ToolLifecycleFuture<'a> {
Box::pin(self.score_tool(input)) Box::pin(self.score_tool(input))
@@ -971,13 +767,12 @@ pub fn install(
let extension = Arc::new(GuardianV2Extension { let extension = Arc::new(GuardianV2Extension {
auth_manager, auth_manager,
event_sink: registry.event_sink(), event_sink: registry.event_sink(),
thread_manager: thread_manager.clone(), thread_manager,
}); });
registry.thread_lifecycle_contributor(extension.clone()); registry.thread_lifecycle_contributor(extension.clone());
registry.approval_review_contributor(Arc::new(super::approval::GuardianApprovalReviewer { registry.approval_review_contributor(Arc::new(super::approval::GuardianApprovalReviewer {
thread_manager, thread_manager: extension.thread_manager.clone(),
})); }));
registry.approval_review_contributor(extension.clone());
registry.skill_invocation_contributor(extension.clone()); registry.skill_invocation_contributor(extension.clone());
registry.tool_lifecycle_contributor(extension); registry.tool_lifecycle_contributor(extension);
} }
@@ -69,7 +69,6 @@ use serde_json::json;
use super::GuardianV2Extension; use super::GuardianV2Extension;
use super::GuardianV2ScoreProgress; use super::GuardianV2ScoreProgress;
use super::StrictReviewReason;
use crate::async_scorer::config::CLASSIFICATION_OUTPUT_INSTRUCTIONS; use crate::async_scorer::config::CLASSIFICATION_OUTPUT_INSTRUCTIONS;
use crate::async_scorer::config::DEFAULT_MODEL_CONTEXT_ITEM_TOKENS; use crate::async_scorer::config::DEFAULT_MODEL_CONTEXT_ITEM_TOKENS;
@@ -285,14 +284,13 @@ async fn installed_extension_uses_http_after_warm_socket_auth_expires() -> Resul
}) })
.await?; .await?;
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, r#"{"tool":"mcp_tool_call","server":"node_repl"}"#,
r#"{"tool":"mcp_tool_call","server":"node_repl"}"#, /*metrics*/ None,
/*extension_metrics*/ None, )
) .await,
.await,
Some(ReviewDecision::Approved) Some(ReviewDecision::Approved)
); );
} }
@@ -669,33 +667,29 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> {
] { ] {
let prompt = action.to_string(); let prompt = action.to_string();
assert_eq!( assert_eq!(
fixture cached_approval(
.registry &fixture.registry,
.fast_approval_decision(
&fixture.session_store,
thread_store,
&prompt,
thread_store
.get::<RecordingMetrics>()
.map(|metrics| metrics as Arc<dyn ExtensionMetrics>),
)
.await,
expected,
"unexpected fast approval for {action}"
);
}
assert_eq!(
fixture
.registry
.fast_approval_decision(
&fixture.session_store,
thread_store, thread_store,
"not valid JSON", &prompt,
thread_store thread_store
.get::<RecordingMetrics>() .get::<RecordingMetrics>()
.map(|metrics| metrics as Arc<dyn ExtensionMetrics>), .map(|metrics| metrics as Arc<dyn ExtensionMetrics>),
) )
.await, .await,
expected,
"unexpected fast approval for {action}"
);
}
assert_eq!(
cached_approval(
&fixture.registry,
thread_store,
"not valid JSON",
thread_store
.get::<RecordingMetrics>()
.map(|metrics| metrics as Arc<dyn ExtensionMetrics>),
)
.await,
None, None,
"malformed approval actions must not reuse a browser score" "malformed approval actions must not reuse a browser score"
); );
@@ -711,16 +705,13 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> {
}); });
thread_store.insert(changed_model); thread_store.insert(changed_model);
assert_eq!( assert_eq!(
fixture cached_approval(
.registry &fixture.registry,
.fast_approval_decision( thread_store,
&fixture.session_store, &json!({"tool": "mcp_tool_call", "server": "node_repl", "tool_name": "js"}).to_string(),
thread_store, /*metrics*/ None,
&json!({"tool": "mcp_tool_call", "server": "node_repl", "tool_name": "js"}) )
.to_string(), .await,
/*extension_metrics*/ None,
)
.await,
None, None,
"a score from the previous model policy must not approve a call" "a score from the previous model policy must not approve a call"
); );
@@ -772,15 +763,13 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> {
sampled_at: None, sampled_at: None,
}); });
assert_eq!( assert_eq!(
fixture cached_approval(
.registry &fixture.registry,
.fast_approval_decision( thread_store,
&fixture.session_store, r#"{"tool":"mcp_tool_call","server":"node_repl","tool_name":"js"}"#,
thread_store, /*metrics*/ None,
r#"{"tool":"mcp_tool_call","server":"node_repl","tool_name":"js"}"#, )
/*extension_metrics*/ None, .await,
)
.await,
None, None,
"switching back to a reviewed model must not revive a skipped score" "switching back to a reviewed model must not revive a skipped score"
); );
@@ -909,14 +898,13 @@ async fn sample_configured_conversation_history_with_source(
thread_store.insert(parent_model); thread_store.insert(parent_model);
} }
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", /*metrics*/ None,
/*extension_metrics*/ None, )
) .await,
.await,
None None
); );
registry.thread_lifecycle_contributors()[0] registry.thread_lifecycle_contributors()[0]
@@ -1123,16 +1111,15 @@ impl GuardianFailureFixture {
.await?; .await?;
thread_store.insert(RecordingMetrics::default()); thread_store.insert(RecordingMetrics::default());
assert_eq!( assert_eq!(
self.registry cached_approval(
.fast_approval_decision( &self.registry,
&self.session_store, thread_store,
thread_store, "review action",
"review action", thread_store
thread_store .get::<RecordingMetrics>()
.get::<RecordingMetrics>() .map(|metrics| metrics as Arc<dyn ExtensionMetrics>),
.map(|metrics| metrics as Arc<dyn ExtensionMetrics>), )
) .await,
.await,
None None
); );
assert!( assert!(
@@ -1434,7 +1421,6 @@ max_recent_non_user_entries = 8
let retained_action_bytes = i64::try_from(serde_json::to_string_pretty(&action)?.len())?; let retained_action_bytes = i64::try_from(serde_json::to_string_pretty(&action)?.len())?;
assert_eq!(action["tool"], "read_file"); assert_eq!(action["tool"], "read_file");
let session_store = ExtensionData::new("session-1");
let thread_store = test.codex.thread_extension_data(); let thread_store = test.codex.thread_extension_data();
let score_progress = thread_store let score_progress = thread_store
.get::<GuardianV2ScoreProgress>() .get::<GuardianV2ScoreProgress>()
@@ -1451,7 +1437,6 @@ max_recent_non_user_entries = 8
} }
}) })
.await?; .await?;
assert_eq!(thread_store.get::<StrictReviewReason>(), None);
thread_store.insert(SecurityRiskScore { thread_store.insert(SecurityRiskScore {
scores: BTreeMap::from([("action_risk".to_owned(), 0.65)]), scores: BTreeMap::from([("action_risk".to_owned(), 0.65)]),
call_id: None, call_id: None,
@@ -1459,22 +1444,17 @@ max_recent_non_user_entries = 8
sampled_at: None, sampled_at: None,
}); });
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", thread_store
thread_store .get::<RecordingMetrics>()
.get::<RecordingMetrics>() .map(|metrics| metrics as Arc<dyn ExtensionMetrics>),
.map(|metrics| metrics as Arc<dyn ExtensionMetrics>), )
) .await,
.await,
None None
); );
assert_eq!(
thread_store.remove::<StrictReviewReason>().as_deref(),
Some(&StrictReviewReason::ElevatedRisk)
);
thread_store.insert(SecurityRiskScore { thread_store.insert(SecurityRiskScore {
scores: BTreeMap::from([("action_risk".to_owned(), 0.55)]), scores: BTreeMap::from([("action_risk".to_owned(), 0.55)]),
call_id: None, call_id: None,
@@ -1482,16 +1462,15 @@ max_recent_non_user_entries = 8
sampled_at: None, sampled_at: None,
}); });
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", thread_store
thread_store .get::<RecordingMetrics>()
.get::<RecordingMetrics>() .map(|metrics| metrics as Arc<dyn ExtensionMetrics>),
.map(|metrics| metrics as Arc<dyn ExtensionMetrics>), )
) .await,
.await,
Some(ReviewDecision::Approved) Some(ReviewDecision::Approved)
); );
@@ -1505,16 +1484,15 @@ max_recent_non_user_entries = 8
.latest_tool_call .latest_tool_call
.store(/*val*/ 3, Ordering::Release); .store(/*val*/ 3, Ordering::Release);
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", thread_store
thread_store .get::<RecordingMetrics>()
.get::<RecordingMetrics>() .map(|metrics| metrics as Arc<dyn ExtensionMetrics>),
.map(|metrics| metrics as Arc<dyn ExtensionMetrics>), )
) .await,
.await,
Some(ReviewDecision::Approved) Some(ReviewDecision::Approved)
); );
@@ -1524,37 +1502,31 @@ max_recent_non_user_entries = 8
.latest_tool_call .latest_tool_call
.store(/*val*/ 4, Ordering::Release); .store(/*val*/ 4, Ordering::Release);
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", thread_store
thread_store .get::<RecordingMetrics>()
.get::<RecordingMetrics>() .map(|metrics| metrics as Arc<dyn ExtensionMetrics>),
.map(|metrics| metrics as Arc<dyn ExtensionMetrics>), )
) .await,
.await,
None None
); );
assert_eq!(
thread_store.remove::<StrictReviewReason>().as_deref(),
Some(&StrictReviewReason::StaleScore)
);
score_progress score_progress
.latest_scored_tool_call .latest_scored_tool_call
.store(/*val*/ 2, Ordering::Release); .store(/*val*/ 2, Ordering::Release);
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", thread_store
thread_store .get::<RecordingMetrics>()
.get::<RecordingMetrics>() .map(|metrics| metrics as Arc<dyn ExtensionMetrics>),
.map(|metrics| metrics as Arc<dyn ExtensionMetrics>), )
) .await,
.await,
Some(ReviewDecision::Approved) Some(ReviewDecision::Approved)
); );
@@ -1822,7 +1794,6 @@ async fn contributor_uses_model_defaults_and_preserves_local_overrides() -> Resu
.expect("planned action should be a text item"); .expect("planned action should be a text item");
assert!(action.len() <= TruncationPolicy::Tokens(/*limit*/ 128).byte_budget()); assert!(action.len() <= TruncationPolicy::Tokens(/*limit*/ 128).byte_budget());
let session_store = ExtensionData::new("session-1");
let thread_store = test.codex.thread_extension_data(); let thread_store = test.codex.thread_extension_data();
let guardian_config = thread_store let guardian_config = thread_store
.get::<crate::async_scorer::config::GuardianV2Config>() .get::<crate::async_scorer::config::GuardianV2Config>()
@@ -1857,14 +1828,13 @@ async fn contributor_uses_model_defaults_and_preserves_local_overrides() -> Resu
sampled_at: None, sampled_at: None,
}); });
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", /*metrics*/ None,
/*extension_metrics*/ None, )
) .await,
.await,
Some(ReviewDecision::Approved) Some(ReviewDecision::Approved)
); );
@@ -1931,7 +1901,6 @@ async fn assert_luna_pool_context(thread_context_enabled: bool) -> Result<()> {
) )
.await?; .await?;
let thread_id = test.session_configured.thread_id; let thread_id = test.session_configured.thread_id;
let session_store = ExtensionData::new("session-1");
let thread_store = test.codex.thread_extension_data(); let thread_store = test.codex.thread_extension_data();
assert_eq!(request["model"], "gpt-5.6-luna"); assert_eq!(request["model"], "gpt-5.6-luna");
let classifier_thread_id = request["client_metadata"]["thread_id"] let classifier_thread_id = request["client_metadata"]["thread_id"]
@@ -2043,14 +2012,13 @@ async fn assert_luna_pool_context(thread_context_enabled: bool) -> Result<()> {
"risk scores should not be persisted unless explicitly enabled" "risk scores should not be persisted unless explicitly enabled"
); );
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", /*metrics*/ None,
/*extension_metrics*/ None, )
) .await,
.await,
None None
); );
thread_store.insert(SecurityRiskScore { thread_store.insert(SecurityRiskScore {
@@ -2060,14 +2028,13 @@ async fn assert_luna_pool_context(thread_context_enabled: bool) -> Result<()> {
sampled_at: None, sampled_at: None,
}); });
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", /*metrics*/ None,
/*extension_metrics*/ None, )
) .await,
.await,
None None
); );
@@ -2078,14 +2045,13 @@ async fn assert_luna_pool_context(thread_context_enabled: bool) -> Result<()> {
sampled_at: None, sampled_at: None,
}); });
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", /*metrics*/ None,
/*extension_metrics*/ None, )
) .await,
.await,
Some(ReviewDecision::Approved) Some(ReviewDecision::Approved)
); );
@@ -2097,14 +2063,13 @@ async fn assert_luna_pool_context(thread_context_enabled: bool) -> Result<()> {
sampled_at: None, sampled_at: None,
}); });
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, &disabled_thread_store,
&disabled_thread_store, "review action",
"review action", /*metrics*/ None
/*extension_metrics*/ None )
) .await,
.await,
None None
); );
@@ -2254,14 +2219,13 @@ async fn contributor_skips_required_models_in_standard_scope() -> Result<()> {
sampled_at: None, sampled_at: None,
}); });
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, r#"{"tool":"mcp_tool_call","server":"node_repl"}"#,
r#"{"tool":"mcp_tool_call","server":"node_repl"}"#, /*metrics*/ None,
/*extension_metrics*/ None, )
) .await,
.await,
None None
); );
@@ -2316,7 +2280,6 @@ async fn cached_score_survives_compaction_and_internal_context_but_not_user_inpu
/*model_defaults*/ None, /*model_defaults*/ None,
) )
.await?; .await?;
let session_store = ExtensionData::new("session-1");
let thread_store = test.codex.thread_extension_data(); let thread_store = test.codex.thread_extension_data();
let progress = thread_store.get::<GuardianV2ScoreProgress>().unwrap(); let progress = thread_store.get::<GuardianV2ScoreProgress>().unwrap();
tokio::time::timeout(ASYNC_TEST_TIMEOUT, async { tokio::time::timeout(ASYNC_TEST_TIMEOUT, async {
@@ -2346,14 +2309,13 @@ async fn cached_score_survives_compaction_and_internal_context_but_not_user_inpu
}) })
.await; .await;
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", /*metrics*/ None,
/*extension_metrics*/ None, )
) .await,
.await,
Some(ReviewDecision::Approved), Some(ReviewDecision::Approved),
); );
@@ -2369,14 +2331,13 @@ async fn cached_score_survives_compaction_and_internal_context_but_not_user_inpu
}]) }])
.await?; .await?;
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", /*metrics*/ None
/*extension_metrics*/ None )
) .await,
.await,
None, None,
); );
Ok(()) Ok(())
@@ -2408,15 +2369,13 @@ async fn assert_compaction_approval_policy(thread_context_enabled: bool) -> Resu
sampled_at: None, sampled_at: None,
}); });
assert_eq!( assert_eq!(
fixture cached_approval(
.registry &fixture.registry,
.fast_approval_decision( thread_store,
&fixture.session_store, "review action",
thread_store, /*metrics*/ None
"review action", )
/*extension_metrics*/ None .await,
)
.await,
Some(ReviewDecision::Approved) Some(ReviewDecision::Approved)
); );
let authorization = fixture.test.codex.guardian_authorization_version().await; let authorization = fixture.test.codex.guardian_authorization_version().await;
@@ -2472,21 +2431,15 @@ async fn assert_compaction_approval_policy(thread_context_enabled: bool) -> Resu
.js_executions .js_executions
.store(/*val*/ 1, Ordering::Release); .store(/*val*/ 1, Ordering::Release);
assert_eq!( assert_eq!(
fixture cached_approval(
.registry &fixture.registry,
.fast_approval_decision( thread_store,
&fixture.session_store, prompt,
thread_store, /*metrics*/ None
prompt, )
/*extension_metrics*/ None .await,
)
.await,
(!thread_context_enabled).then_some(ReviewDecision::Approved) (!thread_context_enabled).then_some(ReviewDecision::Approved)
); );
assert_eq!(
thread_store.remove::<StrictReviewReason>().as_deref(),
thread_context_enabled.then_some(&StrictReviewReason::IncompatibleCompaction)
);
} }
Ok(()) Ok(())
} }
@@ -2525,14 +2478,13 @@ async fn contributor_counts_failed_thread_lookups_toward_score_lag() -> Result<(
sampled_at: None, sampled_at: None,
}); });
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", /*metrics*/ None,
/*extension_metrics*/ None, )
) .await,
.await,
Some(ReviewDecision::Approved) Some(ReviewDecision::Approved)
); );
@@ -2563,14 +2515,13 @@ async fn contributor_counts_failed_thread_lookups_toward_score_lag() -> Result<(
assert_eq!(score_progress.latest_tool_call.load(Ordering::Acquire), 2); assert_eq!(score_progress.latest_tool_call.load(Ordering::Acquire), 2);
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", /*metrics*/ None,
/*extension_metrics*/ None, )
) .await,
.await,
None None
); );
@@ -3098,14 +3049,13 @@ async fn assert_parent_compaction_reuse(thread_context_enabled: bool) -> Result<
// Raw injection did not attach producer provenance to the live checkpoint. // Raw injection did not attach producer provenance to the live checkpoint.
// The sample's mock snapshot cannot make that live checkpoint safe for approval. // The sample's mock snapshot cannot make that live checkpoint safe for approval.
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", /*metrics*/ None,
/*extension_metrics*/ None, )
) .await,
.await,
(!thread_context_enabled).then_some(ReviewDecision::Approved), (!thread_context_enabled).then_some(ReviewDecision::Approved),
); );
@@ -3149,14 +3099,13 @@ async fn assert_parent_compaction_reuse(thread_context_enabled: bool) -> Result<
} }
); );
assert_eq!( assert_eq!(
registry cached_approval(
.fast_approval_decision( &registry,
&session_store, thread_store,
thread_store, "review action",
"review action", /*metrics*/ None,
/*extension_metrics*/ None, )
) .await,
.await,
None None
); );
assert_eq!( assert_eq!(
@@ -3289,3 +3238,68 @@ async fn contributor_bounds_oversized_actions_and_fairly_truncates_nested_fields
Ok(()) Ok(())
} }
struct CacheMiss;
impl codex_extension_api::SynchronousApprovalReviewer for CacheMiss {
fn review(
&self,
_reason: codex_protocol::approvals::GuardianReviewReason,
) -> codex_extension_api::ExtensionFuture<'_, ReviewDecision> {
Box::pin(async { ReviewDecision::denied("cache miss") })
}
}
/// Exercises decision routing; a fresh review is observed as a cache miss.
async fn cached_approval(
registry: &codex_extension_api::ExtensionRegistry<Config>,
store: &ExtensionData,
action: &str,
metrics: Option<Arc<dyn ExtensionMetrics>>,
) -> Option<ReviewDecision> {
let action = serde_json::from_str(action).unwrap_or(serde_json::Value::Null);
let category = match review_scope(&action) {
Some(category) => category,
None if store.get::<super::GuardianV2Config>()?.policy.other_tools
== codex_protocol::openai_models::GuardianReviewMode::Adaptive =>
{
codex_protocol::openai_models::GuardianScope::Shell
}
None => {
super::super::metrics::record_fast_decision(
metrics.as_deref(),
"deferred",
"out_of_scope",
);
return None;
}
};
let input = codex_extension_api::ApprovalDecisionInput {
approval_id: "cache-probe",
action: &action,
thread_id: codex_protocol::ThreadId::from_string(store.level_id()).unwrap(),
thread_store: store,
category,
approval_policy: codex_protocol::protocol::AskForApproval::OnRequest,
approvals_reviewer: ApprovalsReviewer::AutoReview,
require_guardian: false,
require_fresh_review: false,
full_access: false,
metrics,
synchronous_reviewer: &CacheMiss,
};
match registry.decide_approval(&input).await {
Some(codex_extension_api::ApprovalDecision::Allow) => Some(ReviewDecision::Approved),
_ => None,
}
}
fn review_scope(action: &serde_json::Value) -> Option<GuardianScope> {
match action.get("tool").and_then(serde_json::Value::as_str)? {
"mcp_tool_call" => action
.get("server")
.and_then(serde_json::Value::as_str)
.map(GuardianScope::for_mcp_server),
"network_access" => Some(GuardianScope::Network),
tool => GuardianScope::for_tool(&ToolName::plain(tool)),
}
}
@@ -15,7 +15,6 @@ mod trusted_tools;
pub(crate) use config::DEFAULT_MODEL_CONTEXT_ITEM_TOKENS; pub(crate) use config::DEFAULT_MODEL_CONTEXT_ITEM_TOKENS;
pub(crate) use config::GuardianV2Config; pub(crate) use config::GuardianV2Config;
pub use extension::StrictReviewReason;
pub(crate) use extension::install; pub(crate) use extension::install;
pub(crate) use transcript::MAX_TOOL_ENTRY_TOKENS; pub(crate) use transcript::MAX_TOOL_ENTRY_TOKENS;
pub(crate) use transcript::RenderedContext; pub(crate) use transcript::RenderedContext;
-1
View File
@@ -15,7 +15,6 @@ use codex_protocol::ThreadId;
mod async_scorer; mod async_scorer;
mod sync_reviewer; mod sync_reviewer;
pub use async_scorer::StrictReviewReason;
pub use sync_reviewer::GuardianExtension as GuardianReviewerExtension; pub use sync_reviewer::GuardianExtension as GuardianReviewerExtension;
pub use sync_reviewer::GuardianThreadContext as GuardianReviewerThreadContext; pub use sync_reviewer::GuardianThreadContext as GuardianReviewerThreadContext;