From 4e48cd02da734b0dfb51726d264133e63951308d Mon Sep 17 00:00:00 2001 From: jif Date: Fri, 4 Sep 2026 11:48:30 +0000 Subject: [PATCH] Honor model-provided Guardian review policies (#42744) ## What changed - Add optional per-model Guardian modes for computer use, shell, code mode, file changes, MCP, network, and permission requests. - Apply disabled, synchronous, or adaptive review by action category while preserving legacy configuration when model policy is absent. - Bind cached risk scores to the active model policy, thread settings, and environment selections so outdated scores cannot approve later actions. - Keep unknown review modes on the synchronous path and ignore unknown policy fields for forward compatibility. ## Testing - Cover policy serialization, legacy fallback, scoped tool classification, disabled categories, and score invalidation after policy changes. GitOrigin-RevId: f334a5f4fffb198da66cde3a400233e0df650516 --- .../app-server/tests/common/models_cache.rs | 1 + .../app-server/tests/suite/v2/guardian_v2.rs | 4 + .../codex-api/tests/models_integration.rs | 1 + codex-rs/core/src/session/session.rs | 3 + codex-rs/core/src/session/step_activation.rs | 10 +- codex-rs/core/src/session/turn.rs | 9 +- codex-rs/core/tests/suite/auto_review.rs | 1 + codex-rs/core/tests/suite/model_switching.rs | 2 + codex-rs/core/tests/suite/models_cache_ttl.rs | 1 + codex-rs/core/tests/suite/personality.rs | 2 + codex-rs/core/tests/suite/remote_models.rs | 3 + codex-rs/core/tests/suite/rmcp_client.rs | 1 + .../tests/suite/spawn_agent_description.rs | 1 + codex-rs/core/tests/suite/view_image.rs | 1 + .../src/contributors/approval_review.rs | 5 +- .../src/async_scorer/authorization.rs | 16 +- .../guardian-v2/src/async_scorer/config.rs | 57 +++--- .../src/async_scorer/config_tests.rs | 15 +- .../guardian-v2/src/async_scorer/coverage.rs | 164 ++++++++++++++++ .../guardian-v2/src/async_scorer/extension.rs | 174 +++++++++-------- .../src/async_scorer/extension_tests.rs | 178 +++++++++++++----- .../ext/guardian-v2/src/async_scorer/mod.rs | 1 + codex-rs/models-manager/src/model_info.rs | 1 + codex-rs/protocol/src/openai_models.rs | 14 +- .../protocol/src/openai_models/guardian.rs | 127 +++++++++++++ .../src/openai_models/guardian_tests.rs | 48 +++++ 26 files changed, 654 insertions(+), 186 deletions(-) create mode 100644 codex-rs/ext/guardian-v2/src/async_scorer/coverage.rs create mode 100644 codex-rs/protocol/src/openai_models/guardian.rs create mode 100644 codex-rs/protocol/src/openai_models/guardian_tests.rs diff --git a/codex-rs/app-server/tests/common/models_cache.rs b/codex-rs/app-server/tests/common/models_cache.rs index e3de19b9d0..cc69f9951a 100644 --- a/codex-rs/app-server/tests/common/models_cache.rs +++ b/codex-rs/app-server/tests/common/models_cache.rs @@ -69,6 +69,7 @@ fn preset_to_info(preset: &ModelPreset, priority: i32) -> ModelInfo { used_fallback_model_metadata: false, supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: false, node_repl_disabled: false, auto_review_model_override: None, diff --git a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs index 6558213dec..ba6343ae3b 100644 --- a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs +++ b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs @@ -116,6 +116,10 @@ async fn resumed_thread_does_not_wait_for_guardian_websocket_warmup() -> Result< .with_extra_config("[features.guardianv2]\nenabled = true") .enable_feature(Feature::GuardianApproval) .write(codex_home.path())?; + let config = load_default_config_for_test(&codex_home).await; + let mut model = codex_core::test_support::construct_model_info_offline(MODEL, &config); + model.node_repl_auto_review_required = true; + write_models_cache_with_models(codex_home.path(), vec![model])?; let thread_id = create_fake_rollout( codex_home.path(), "2025-01-05T12-00-00", diff --git a/codex-rs/codex-api/tests/models_integration.rs b/codex-rs/codex-api/tests/models_integration.rs index b3092a1aaf..288dd123e4 100644 --- a/codex-rs/codex-api/tests/models_integration.rs +++ b/codex-rs/codex-api/tests/models_integration.rs @@ -102,6 +102,7 @@ async fn models_client_hits_models_endpoint() { used_fallback_model_metadata: false, supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: true, node_repl_disabled: true, auto_review_model_override: None, diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 11bb7667c4..56c0107c35 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -832,6 +832,9 @@ impl Session { thread_extension_init.insert(codex_extension_api::ThreadOriginator( session_configuration.originator.clone(), )); + // Publish the already resolved model before extensions make startup decisions. + // Turn construction refreshes this attachment when the selected model changes. + thread_extension_init.insert(model_info); let mcp_thread_init = thread_extension_init.clone(); let thread_extension_data = codex_extension_api::ExtensionData::new_with_init( thread_id.to_string(), diff --git a/codex-rs/core/src/session/step_activation.rs b/codex-rs/core/src/session/step_activation.rs index 5447fc3066..8dc090315a 100644 --- a/codex-rs/core/src/session/step_activation.rs +++ b/codex-rs/core/src/session/step_activation.rs @@ -107,12 +107,18 @@ fn check_legacy_model_safety( // TurnMetadataState pins both node REPL flags. Guardian prompt/evidence // construction also reads node_repl_auto_review_required from the turn. if retained_models.iter().any(|model| { - model.node_repl_auto_review_required != destination.node_repl_auto_review_required + model.computer_use_review_required() != destination.computer_use_review_required() }) { return Err( "the destination changes the admitted node REPL review requirement".to_string(), ); } + if retained_models + .iter() + .any(|model| model.guardian != destination.guardian) + { + return Err("the destination changes the admitted Guardian coverage".to_string()); + } if retained_models .iter() .any(|model| model.node_repl_disabled != destination.node_repl_disabled) @@ -130,7 +136,7 @@ fn check_legacy_model_safety( return Err("the destination changes the explicit Guardian reviewer model".to_string()); } - if admitted_config.features.enabled(Feature::GuardianV2) + if (admitted_config.features.enabled(Feature::GuardianV2) || destination.guardian.is_some()) && admitted_config.features.enabled(Feature::GuardianApproval) { // GuardianV2Extension::on_tool_start reads the parent ModelInfo from diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index c49579cb22..98b98db3dc 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -1069,14 +1069,7 @@ async fn track_turn_resolved_config_analytics( .services .thread_extension_data .get::() - .is_some_and(|state| { - state.computer_use_only - || !turn_context - .config - .config_layer_stack - .requirements() - .auto_review_required_for_model(&turn_context.model_info().slug) - }), + .is_some(), sandbox_network_access: turn_context.network_sandbox_policy().is_enabled(), collaboration_mode: turn_context.mode(), personality: turn_context.personality(), diff --git a/codex-rs/core/tests/suite/auto_review.rs b/codex-rs/core/tests/suite/auto_review.rs index d481d262d3..b6fae525e5 100644 --- a/codex-rs/core/tests/suite/auto_review.rs +++ b/codex-rs/core/tests/suite/auto_review.rs @@ -650,6 +650,7 @@ fn remote_model_with_auto_review_override(slug: &str, review_model: &str) -> Mod supports_image_detail_original: false, context_window: Some(272_000), max_context_window: None, + guardian: None, auto_compact_token_limit: None, comp_hash: None, effective_context_window_percent: 95, diff --git a/codex-rs/core/tests/suite/model_switching.rs b/codex-rs/core/tests/suite/model_switching.rs index 0ed6cdf0e0..025d93ded9 100644 --- a/codex-rs/core/tests/suite/model_switching.rs +++ b/codex-rs/core/tests/suite/model_switching.rs @@ -121,6 +121,7 @@ fn test_model_info( used_fallback_model_metadata: false, supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: false, node_repl_disabled: false, auto_review_model_override: None, @@ -1358,6 +1359,7 @@ async fn model_switch_to_smaller_model_updates_token_context_window() -> Result< used_fallback_model_metadata: false, supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: false, node_repl_disabled: false, auto_review_model_override: None, diff --git a/codex-rs/core/tests/suite/models_cache_ttl.rs b/codex-rs/core/tests/suite/models_cache_ttl.rs index ea663b2ac0..7dea480f5b 100644 --- a/codex-rs/core/tests/suite/models_cache_ttl.rs +++ b/codex-rs/core/tests/suite/models_cache_ttl.rs @@ -539,6 +539,7 @@ fn test_remote_model(slug: &str, priority: i32) -> ModelInfo { used_fallback_model_metadata: false, supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: false, node_repl_disabled: false, auto_review_model_override: None, diff --git a/codex-rs/core/tests/suite/personality.rs b/codex-rs/core/tests/suite/personality.rs index fd3d7e9a05..3de3c34eec 100644 --- a/codex-rs/core/tests/suite/personality.rs +++ b/codex-rs/core/tests/suite/personality.rs @@ -673,6 +673,7 @@ async fn remote_model_friendly_personality_instructions_with_feature() -> anyhow used_fallback_model_metadata: false, supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: false, node_repl_disabled: false, auto_review_model_override: None, @@ -803,6 +804,7 @@ async fn user_turn_personality_remote_model_template_includes_update_message() - used_fallback_model_metadata: false, supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: false, node_repl_disabled: false, auto_review_model_override: None, diff --git a/codex-rs/core/tests/suite/remote_models.rs b/codex-rs/core/tests/suite/remote_models.rs index ba73e2ebf6..545ca6c7d0 100644 --- a/codex-rs/core/tests/suite/remote_models.rs +++ b/codex-rs/core/tests/suite/remote_models.rs @@ -610,6 +610,7 @@ async fn remote_models_remote_model_uses_unified_exec() -> Result<()> { used_fallback_model_metadata: false, supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: false, node_repl_disabled: false, auto_review_model_override: None, @@ -879,6 +880,7 @@ async fn remote_models_apply_legacy_instructions() -> Result<()> { used_fallback_model_metadata: false, supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: false, node_repl_disabled: false, auto_review_model_override: None, @@ -1466,6 +1468,7 @@ fn test_remote_model_with_policy( used_fallback_model_metadata: false, supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: false, node_repl_disabled: false, auto_review_model_override: None, diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 87f47b497e..1c2412902d 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -2771,6 +2771,7 @@ async fn stdio_image_responses_are_sanitized_for_text_only_model() -> anyhow::Re used_fallback_model_metadata: false, supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: false, node_repl_disabled: false, auto_review_model_override: None, diff --git a/codex-rs/core/tests/suite/spawn_agent_description.rs b/codex-rs/core/tests/suite/spawn_agent_description.rs index dab508f9db..712e35bb36 100644 --- a/codex-rs/core/tests/suite/spawn_agent_description.rs +++ b/codex-rs/core/tests/suite/spawn_agent_description.rs @@ -91,6 +91,7 @@ fn test_model_info( used_fallback_model_metadata: false, supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: false, node_repl_disabled: false, auto_review_model_override: None, diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index aeb606cb86..a919b19159 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -1623,6 +1623,7 @@ async fn view_image_tool_returns_unsupported_message_for_text_only_model() -> an used_fallback_model_metadata: false, supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: false, node_repl_disabled: false, auto_review_model_override: None, diff --git a/codex-rs/ext/extension-api/src/contributors/approval_review.rs b/codex-rs/ext/extension-api/src/contributors/approval_review.rs index cabd93293c..38693fac8e 100644 --- a/codex-rs/ext/extension-api/src/contributors/approval_review.rs +++ b/codex-rs/ext/extension-api/src/contributors/approval_review.rs @@ -11,10 +11,7 @@ use crate::ConversationHistorySnapshot; use crate::ExtensionData; /// Thread-local state installed only after Guardian V2's async classifier initializes. -pub struct GuardianV2Enabled { - /// Computer-use classification remains active for models requiring synchronous review. - pub computer_use_only: bool, -} +pub struct GuardianV2Enabled; /// Classification returned by an approval reviewer. #[derive(Clone, Debug, Eq, PartialEq)] diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/authorization.rs b/codex-rs/ext/guardian-v2/src/async_scorer/authorization.rs index 2dcf369a34..8bee74b5b3 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/authorization.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/authorization.rs @@ -1,12 +1,15 @@ -//! Binds cached classifier results to the user authorization they evaluated. +//! Binds cached classifier results to the user authorization and model policy they evaluated. use codex_core::CodexThread; use codex_core::GuardianAuthorizationVersion; -#[derive(Clone, Copy, Debug, Eq, PartialEq)] +#[derive(Clone, Debug, PartialEq)] pub(super) struct ScoreAuthorization { + pub(super) settings: codex_protocol::protocol::ThreadSettingsSnapshot, + pub(super) environments: Vec, pub(super) local: GuardianAuthorizationVersion, pub(super) root: Option, + pub(super) model: Option>, } impl ScoreAuthorization { @@ -16,8 +19,17 @@ impl ScoreAuthorization { .await .map(|snapshot| snapshot.authorization_version); Self { + settings: thread.thread_settings_snapshot().await, + environments: thread + .config_snapshot() + .await + .environment_selections() + .to_vec(), local: thread.guardian_authorization_version().await, root, + model: thread + .thread_extension_data() + .get::(), } } } diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/config.rs b/codex-rs/ext/guardian-v2/src/async_scorer/config.rs index 842cb4412e..1230c15135 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/config.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/config.rs @@ -5,6 +5,7 @@ use codex_features::GuardianV2TranscriptConfigToml; use codex_protocol::openai_models::GuardianV2ModelConfig; use codex_protocol::openai_models::ReasoningEffort; +use super::coverage::GuardianPolicy; use super::transcript::MAX_MESSAGE_ENTRY_TOKENS; use super::transcript::MAX_MESSAGE_TRANSCRIPT_TOKENS; use super::transcript::MAX_RECENT_NON_USER_ENTRIES; @@ -24,12 +25,6 @@ const DEFAULT_MAX_TOOL_CALL_LAG: usize = 2; pub(crate) const DEFAULT_CLASSIFIER_INSTRUCTIONS: &str = include_str!("classifier_instructions.md"); pub(crate) const CLASSIFICATION_OUTPUT_INSTRUCTIONS: &str = "Your first output token is the entire classification: `high` for high risk or `low` for low risk. Output that token immediately and nothing else."; -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub(crate) enum GuardianV2ReviewScope { - Standard { sandboxed_exec_commands: bool }, - ComputerUseOnly, -} - #[derive(Clone, Debug, PartialEq)] pub(crate) struct GuardianV2Config { local_overrides: GuardianV2ConfigToml, @@ -43,7 +38,9 @@ pub(crate) struct GuardianV2Config { pub(crate) max_classifier_instruction_tokens: Option, pub(crate) reuse_parent_compaction: bool, pub(crate) max_parent_compaction_tokens: usize, - pub(crate) review_scope: GuardianV2ReviewScope, + pub(super) policy: GuardianPolicy, + force_synchronous_review: bool, + scoring_disabled: bool, pub(crate) transcript: TranscriptConfig, } @@ -67,7 +64,28 @@ impl GuardianV2Config { None => GuardianV2ConfigToml::default(), }; - Self::from_overrides(configured) + let mut resolved = Self::from_overrides(configured)?; + resolved.force_synchronous_review = config + .config_layer_stack + .requirements() + .approvals_reviewer + .can_set(&codex_protocol::config_types::ApprovalsReviewer::User) + .is_err(); + resolved.scoring_disabled = !config.features.enabled(codex_features::Feature::GuardianV2); + Ok(resolved) + } + + pub(super) fn policy_for_model( + &self, + model: Option<&codex_protocol::openai_models::ModelInfo>, + ) -> GuardianPolicy { + let mut policy = self.policy.for_model(model); + if self.force_synchronous_review + || self.scoring_disabled && model.is_none_or(|model| model.guardian.is_none()) + { + policy.disable_scoring(); + } + policy } pub(crate) fn with_model_defaults( @@ -147,6 +165,9 @@ impl GuardianV2Config { let mut resolved = Self::from_overrides(configured)?; resolved.local_overrides = self.local_overrides.clone(); + resolved.policy = self.policy.clone(); + resolved.force_synchronous_review = self.force_synchronous_review; + resolved.scoring_disabled = self.scoring_disabled; Ok(resolved) } @@ -228,6 +249,7 @@ impl GuardianV2Config { ); } + let policy = GuardianPolicy::from_legacy(configured.review_scope.as_ref()); Ok(Self { local_overrides: configured.clone(), persist_scores: configured.persist_scores.unwrap_or(false), @@ -243,22 +265,9 @@ impl GuardianV2Config { max_classifier_instruction_tokens, reuse_parent_compaction: configured.reuse_parent_compaction.unwrap_or(true), max_parent_compaction_tokens, - review_scope: if configured - .review_scope - .as_ref() - .and_then(|review_scope| review_scope.computer_use_only) - .unwrap_or(true) - { - GuardianV2ReviewScope::ComputerUseOnly - } else { - GuardianV2ReviewScope::Standard { - sandboxed_exec_commands: configured - .review_scope - .as_ref() - .and_then(|review_scope| review_scope.sandboxed_exec_commands) - .unwrap_or(false), - } - }, + policy, + force_synchronous_review: false, + scoring_disabled: false, transcript: TranscriptConfig { sources: transcript_config .and_then(|transcript| transcript.sources.clone()) diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/config_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/config_tests.rs index 84c6a81cdb..63e32d6d59 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/config_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/config_tests.rs @@ -9,14 +9,14 @@ use pretty_assertions::assert_eq; use super::CLASSIFICATION_OUTPUT_INSTRUCTIONS; use super::DEFAULT_CLASSIFIER_INSTRUCTIONS; use super::GuardianV2Config; -use super::GuardianV2ReviewScope; +use crate::async_scorer::coverage::GuardianPolicy; use crate::async_scorer::transcript::truncate_entry; #[test] fn review_scope_is_computer_use_only_by_default() { let config = GuardianV2Config::from_overrides(GuardianV2ConfigToml::default()).unwrap(); - assert_eq!(config.review_scope, GuardianV2ReviewScope::ComputerUseOnly); + assert_eq!(config.policy, GuardianPolicy::from_legacy(/*scope*/ None)); } #[test] @@ -31,10 +31,11 @@ fn sandboxed_exec_commands_can_be_included() { .unwrap(); assert_eq!( - config.review_scope, - GuardianV2ReviewScope::Standard { - sandboxed_exec_commands: true, - } + config.policy, + GuardianPolicy::from_legacy(Some(&GuardianV2ReviewScopeConfigToml { + computer_use_only: Some(false), + sandboxed_exec_commands: Some(true), + })) ); } @@ -49,7 +50,7 @@ fn computer_use_only_takes_precedence_over_sandboxed_exec_commands() { }) .unwrap(); - assert_eq!(config.review_scope, GuardianV2ReviewScope::ComputerUseOnly); + assert_eq!(config.policy, GuardianPolicy::from_legacy(/*scope*/ None)); } #[test] diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/coverage.rs b/codex-rs/ext/guardian-v2/src/async_scorer/coverage.rs new file mode 100644 index 0000000000..ac925c18b3 --- /dev/null +++ b/codex-rs/ext/guardian-v2/src/async_scorer/coverage.rs @@ -0,0 +1,164 @@ +//! Owns Guardian's category policy and translates legacy config at the boundary. +//! Scoring and approval consume the same policy; host-required reviews still take precedence. + +use codex_extension_api::ToolPayload; +use codex_features::GuardianV2ReviewScopeConfigToml; +use codex_protocol::ToolName; +use codex_protocol::openai_models::GuardianModelPolicy; +use codex_protocol::openai_models::GuardianReviewMode; +use codex_protocol::openai_models::GuardianScope; +use codex_protocol::openai_models::ModelInfo; + +use GuardianReviewMode::Adaptive; +use GuardianReviewMode::Disabled; +use GuardianReviewMode::Synchronous; + +#[derive(Clone, Copy, Debug, PartialEq)] +pub(super) enum UnscoredAction { + Ignore, + AgeScore, + InvalidateScore, +} + +/// Canonical runtime policy, including the existing scorer's compatibility behavior. +#[derive(Clone, Debug, PartialEq)] +pub(super) struct GuardianPolicy { + pub(super) categories: GuardianModelPolicy, + pub(super) other_tools: GuardianReviewMode, + pub(super) unscored_action: UnscoredAction, + pub(super) initial_cua_call: bool, + sandboxed_exec_commands: bool, +} + +impl GuardianPolicy { + pub(super) fn from_legacy(scope: Option<&GuardianV2ReviewScopeConfigToml>) -> Self { + let computer_use_only = scope + .and_then(|scope| scope.computer_use_only) + .unwrap_or(/*default*/ true); + let other = if computer_use_only { + Synchronous + } else { + Adaptive + }; + Self { + categories: GuardianModelPolicy { + computer_use: Some(Adaptive), + shell: Some(other), + code_mode: Some(other), + file_changes: Some(other), + mcp: Some(other), + network: Some(other), + permissions: Some(other), + }, + other_tools: other, + unscored_action: if computer_use_only { + UnscoredAction::Ignore + } else { + UnscoredAction::AgeScore + }, + initial_cua_call: computer_use_only, + sandboxed_exec_commands: !computer_use_only + && scope + .and_then(|scope| scope.sandboxed_exec_commands) + .unwrap_or(/*default*/ false), + } + } + + pub(super) fn for_model(&self, model: Option<&ModelInfo>) -> Self { + match model.and_then(|model| model.guardian.as_ref()) { + Some(categories) => Self { + categories: categories.clone(), + other_tools: Disabled, + unscored_action: UnscoredAction::InvalidateScore, + initial_cua_call: categories.computer_use == Some(Adaptive), + sandboxed_exec_commands: true, + }, + None => { + let mut policy = self.clone(); + if policy.initial_cua_call + && model.is_some_and(|model| !model.node_repl_auto_review_required) + { + policy.categories.computer_use = Some(Synchronous); + policy.initial_cua_call = false; + policy.unscored_action = UnscoredAction::InvalidateScore; + } + policy + } + } + } + + pub(super) fn mode(&self, scope: GuardianScope) -> GuardianReviewMode { + self.categories.review_mode(scope) + } + + pub(super) fn scoring_enabled(&self) -> bool { + [ + self.categories.computer_use, + self.categories.shell, + self.categories.code_mode, + self.categories.file_changes, + self.categories.mcp, + self.categories.network, + self.categories.permissions, + ] + .contains(&Some(Adaptive)) + } + + pub(super) fn disable_scoring(&mut self) { + for mode in [ + &mut self.categories.computer_use, + &mut self.categories.shell, + &mut self.categories.code_mode, + &mut self.categories.file_changes, + &mut self.categories.mcp, + &mut self.categories.network, + &mut self.categories.permissions, + ] { + if *mode == Some(Adaptive) { + *mode = Some(Synchronous); + } + } + self.other_tools = Synchronous; + } + + pub(super) fn enforce_required_model(&mut self) { + let computer_use = self.categories.computer_use; + self.disable_scoring(); + if self.initial_cua_call { + self.categories.computer_use = computer_use; + } + } + + pub(super) fn scores_tool( + &self, + tool: &ToolName, + payload: &ToolPayload, + scope: Option, + ) -> bool { + if scope.map_or(self.other_tools, |scope| self.mode(scope)) != Adaptive { + return false; + } + if self.sandboxed_exec_commands + || !tool.is_default_namespace() + || tool.name != "exec_command" + { + return true; + } + matches!(payload, ToolPayload::Function { arguments } + if serde_json::from_str::(arguments).ok().is_some_and(|arguments| { + arguments.get("sandbox_permissions").and_then(serde_json::Value::as_str) + == Some("require_escalated") + })) + } + + pub(super) fn review_scope(action: &serde_json::Value) -> Option { + 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)), + } + } +} diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs index 30159d0ee5..64b8bafb3f 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -31,8 +31,6 @@ use codex_extension_api::ThreadOriginator; use codex_extension_api::ThreadStartInput; use codex_extension_api::ToolLifecycleContributor; use codex_extension_api::ToolLifecycleFuture; -use codex_extension_api::ToolName; -use codex_extension_api::ToolPayload; use codex_extension_api::ToolStartInput; use codex_features::Feature; use codex_guardian_context::ContextTarget; @@ -43,7 +41,8 @@ use codex_model_provider::create_model_provider; use codex_protocol::ThreadId; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::mcp::is_node_repl_backed_server; -use codex_protocol::mcp::is_node_repl_backed_tool; +use codex_protocol::openai_models::GuardianReviewMode; +use codex_protocol::openai_models::GuardianScope; use codex_protocol::openai_models::ModelInfo; use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::TruncationPolicy; @@ -54,7 +53,8 @@ use super::action::GuardianAction; use super::action::RenderedAction; use super::authorization::ScoreAuthorization; use super::config::GuardianV2Config; -use super::config::GuardianV2ReviewScope; +use super::coverage::GuardianPolicy; +use super::coverage::UnscoredAction; use super::metrics::REVIEW_FALLBACK_METRIC; use super::metrics::TOOL_CALL_LAG_METRIC; use super::metrics::record_classification; @@ -71,38 +71,6 @@ use super::trusted_skills::TrustedSkillInvocations; use super::trusted_skills::TrustedSkillRoots; use super::trusted_tools::trusted_tool_context; -fn should_classify_tool( - tool_name: &ToolName, - payload: &ToolPayload, - review_scope: GuardianV2ReviewScope, -) -> bool { - let GuardianV2ReviewScope::Standard { - sandboxed_exec_commands, - } = review_scope - else { - return is_node_repl_backed_tool(&tool_name.name, tool_name.namespace.as_deref()); - }; - if sandboxed_exec_commands - || !tool_name.is_default_namespace() - || tool_name.name != "exec_command" - { - return true; - } - - matches!( - payload, - ToolPayload::Function { arguments } - if serde_json::from_str::(arguments) - .ok() - .is_some_and(|arguments| { - arguments - .get("sandbox_permissions") - .and_then(serde_json::Value::as_str) - == Some("require_escalated") - }) - ) -} - /// Explains why Guardian v2 requires synchronous approval review. #[derive(Debug, Eq, PartialEq)] pub enum StrictReviewReason { @@ -141,12 +109,11 @@ impl ThreadLifecycleContributor for GuardianV2Extension { input: ThreadStartInput<'a, Config>, ) -> ExtensionFuture<'a, ()> { Box::pin(async move { - if !input.config.features.enabled(Feature::GuardianV2) - || !input.config.features.enabled(Feature::GuardianApproval) - { + if !input.config.features.enabled(Feature::GuardianApproval) { return; } + let model = input.thread_store.get::(); let thread_id = input.thread_store.level_id().to_string(); let guardian_config = match GuardianV2Config::resolve(input.config) { Ok(config) => config, @@ -159,6 +126,17 @@ impl ThreadLifecycleContributor for GuardianV2Extension { return; } }; + let mut policy = guardian_config.policy_for_model(model.as_deref()); + if model.as_ref().is_some_and(|model| { + input + .config + .config_layer_stack + .requirements() + .auto_review_required_for_model(&model.slug) + }) { + policy.enforce_required_model(); + } + let scoring_enabled = policy.scoring_enabled(); let luna_compaction_hash = if let Some(thread_manager) = self.thread_manager.upgrade() { thread_manager .get_models_manager() @@ -192,7 +170,7 @@ impl ThreadLifecycleContributor for GuardianV2Extension { metrics: input.extension_metrics.clone(), }; - if guardian_config.transcript.include_images { + if scoring_enabled && guardian_config.transcript.include_images { input .thread_store .get_or_init(NodeReplReviewEvidence::default) @@ -202,10 +180,6 @@ impl ThreadLifecycleContributor for GuardianV2Extension { let sampler = input .thread_store .get_or_init(|| LunaSampler::new(sampler_config)); - let guardian_v2_enabled = GuardianV2Enabled { - computer_use_only: guardian_config.review_scope - == GuardianV2ReviewScope::ComputerUseOnly, - }; input.thread_store.insert(guardian_config); input.thread_store.insert(GuardianV2ScoreProgress { metrics: input.extension_metrics.clone(), @@ -218,11 +192,14 @@ impl ThreadLifecycleContributor for GuardianV2Extension { input .thread_store .insert(TrustedSkillRoots::from_config(input.config)); - input.thread_store.insert(guardian_v2_enabled); + if scoring_enabled { + input.thread_store.insert(GuardianV2Enabled); + } // Keep the sampler available for later automatic review, but do not // prewarm while User approval mode or Full Access is selected. - if input.config.approvals_reviewer == ApprovalsReviewer::AutoReview + if scoring_enabled + && input.config.approvals_reviewer == ApprovalsReviewer::AutoReview && !has_full_access( input.config.permissions.approval_policy.value(), &input.config.permissions.effective_permission_profile(), @@ -273,30 +250,22 @@ impl ApprovalReviewContributor for GuardianV2Extension { extension_metrics: Option>, ) -> ExtensionFuture<'a, Option> { Box::pin(async move { - thread_store.get::()?; + let model = thread_store.get::(); let guardian_config = thread_store.get::()?; + let policy = guardian_config.policy_for_model(model.as_deref()); + let action = serde_json::from_str::(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 thread_context_enabled = guardian_evidence.uses_thread_owned_context(); let mut initial_cua_call = false; - if guardian_config.review_scope == GuardianV2ReviewScope::ComputerUseOnly { - let Ok(action) = serde_json::from_str::(prompt) else { - record_fast_decision(extension_metrics.as_deref(), "deferred", "out_of_scope"); - return None; - }; - if action.get("tool").and_then(serde_json::Value::as_str) != Some("mcp_tool_call") - || !action - .get("server") - .and_then(serde_json::Value::as_str) - .is_some_and(is_node_repl_backed_server) - || !thread_store - .get::() - .is_some_and(|model| model.node_repl_auto_review_required) - { - record_fast_decision(extension_metrics.as_deref(), "deferred", "out_of_scope"); - return None; - } - // Legacy mode keeps the first REPL execution's review bypass. - // Thread-owned context first checks whether Luna can use the checkpoint. + 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 @@ -319,7 +288,14 @@ impl ApprovalReviewContributor for GuardianV2Extension { } else if thread_store.get::().is_some() { let manager = self.thread_manager.upgrade()?; let thread_id = ThreadId::from_string(thread_store.level_id()).ok()?; - let thread = manager.get_thread(thread_id).await.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::()?; if config @@ -345,10 +321,6 @@ impl ApprovalReviewContributor for GuardianV2Extension { record_fast_decision(extension_metrics.as_deref(), "deferred", "scoring_failure"); return None; }; - let root_authorization = thread - .guardian_root_snapshot() - .await - .map(|snapshot| snapshot.authorization_version); let history = thread.conversation_history_snapshot().await; if thread_context_enabled { let sampler = thread_store.get::()?; @@ -366,10 +338,7 @@ impl ApprovalReviewContributor for GuardianV2Extension { record_fast_decision(extension_metrics.as_deref(), "approved", "initial_cua_call"); return Some(ReviewDecision::Approved); } - let current_authorization = ScoreAuthorization { - local: guardian_evidence.authorization_version(history.as_ref()), - root: root_authorization, - }; + let current_authorization = ScoreAuthorization::current(&thread).await; if !current_authorization.local.retained_context_complete || current_authorization .root @@ -473,6 +442,10 @@ impl GuardianV2Extension { } async fn score_tool(&self, input: ToolStartInput<'_>) { + // Polling a code cell does not introduce another action or age its score. + if input.tool_name.is_default_namespace() && input.tool_name.name == "wait" { + return; + } let classification_started_at = Instant::now(); let Some(sampler) = input.thread_store.get::() else { return; @@ -483,11 +456,34 @@ impl GuardianV2Extension { let Some(score_progress) = input.thread_store.get::() else { return; }; - if !should_classify_tool(input.tool_name, input.payload, guardian_config.review_scope) { - if guardian_config.review_scope != GuardianV2ReviewScope::ComputerUseOnly { - score_progress - .latest_tool_call - .fetch_add(/*val*/ 1, Ordering::Relaxed); + let parent_model = input.thread_store.get::(); + let policy = guardian_config.policy_for_model(parent_model.as_deref()); + if !policy.scoring_enabled() { + input.thread_store.remove::(); + } + let mcp_server = input + .mcp_tool + .map(|tool| tool.tool_info().server_name.as_str()); + let scope = mcp_server + .map(GuardianScope::for_mcp_server) + .or_else(|| GuardianScope::for_tool(input.tool_name)); + if !policy.scores_tool(input.tool_name, input.payload, scope) { + match policy.unscored_action { + UnscoredAction::Ignore => {} + UnscoredAction::AgeScore => { + score_progress + .latest_tool_call + .fetch_add(/*val*/ 1, Ordering::Relaxed); + } + UnscoredAction::InvalidateScore => { + let index = score_progress + .latest_tool_call + .fetch_add(/*val*/ 1, Ordering::Relaxed) + .saturating_add(/*rhs*/ 1); + score_progress + .latest_failed_tool_call + .fetch_max(index, Ordering::Release); + } } return; } @@ -505,7 +501,7 @@ impl GuardianV2Extension { let tool_call_index = score_progress .latest_tool_call .fetch_add(/*val*/ 1, Ordering::Relaxed) - .saturating_add(1); + .saturating_add(/*rhs*/ 1); let event_sink = Arc::clone(&self.event_sink); let thread_id = input.thread_store.level_id().to_owned(); let turn_id = input.turn_id.to_owned(); @@ -546,23 +542,17 @@ impl GuardianV2Extension { }; // Use the live reviewer, not the startup config or per-app reviewer overrides. let snapshot = thread.config_snapshot().await; - let parent_model = input.thread_store.get::(); if snapshot.full_access || thread.approvals_reviewer_for_turn(input.turn_id).await == ApprovalsReviewer::User - || (guardian_config.review_scope == GuardianV2ReviewScope::ComputerUseOnly - && !parent_model - .as_ref() - .is_some_and(|model| model.node_repl_auto_review_required)) { - // A skipped call invalidates older scores, including ones still in flight - // when switching to a model that does not require REPL review. + // A skipped call invalidates older scores, including ones still in flight. score_progress .latest_failed_tool_call .fetch_max(tool_call_index, Ordering::Release); return; } - // Computer-use-only scores cannot approve other tools for required models. - if guardian_config.review_scope != GuardianV2ReviewScope::ComputerUseOnly + // A required model keeps synchronous review outside its CUA allowance. + if !(scope == Some(GuardianScope::ComputerUse) && policy.initial_cua_call) && parent_model.as_ref().is_some_and(|model| { config .config_layer_stack @@ -573,6 +563,7 @@ impl GuardianV2Extension { input.thread_store.remove::(); return; } + input.thread_store.insert(GuardianV2Enabled); let model_defaults = parent_model .as_ref() .and_then(|model| model.model_messages.as_ref()) @@ -689,6 +680,7 @@ impl GuardianV2Extension { .transcript .images(input.conversation_history.review_items(), node_repl_images); + let score_authorization = ScoreAuthorization::current(&thread).await; tokio::spawn(async move { let mut truncations = ClassificationTruncations::default(); let trusted_tool_context = match mcp_tool.as_ref() { @@ -715,6 +707,8 @@ impl GuardianV2Extension { let score_authorization = ScoreAuthorization { local: authorization_version, root: root_authorization_version, + model: parent_model.clone(), + ..score_authorization }; let transcript = match guardian_config.transcript.build_context( ContextTarget::Async, diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs index 64d25a2418..e41367ef06 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs @@ -45,6 +45,7 @@ use codex_protocol::models::LocalShellExecAction; use codex_protocol::models::LocalShellStatus; use codex_protocol::models::MessagePhase; use codex_protocol::models::ReasoningItemReasoningSummary; +use codex_protocol::openai_models::GuardianScope; use codex_protocol::openai_models::GuardianV2ModelConfig; use codex_protocol::openai_models::GuardianV2TranscriptModelConfig; use codex_protocol::openai_models::ModelInfo; @@ -71,12 +72,12 @@ use super::GuardianV2ScoreProgress; use super::ParentCompactionError; use super::StrictReviewReason; use super::encrypted_parent_compaction; -use super::should_classify_tool; + use crate::async_scorer::config::CLASSIFICATION_OUTPUT_INSTRUCTIONS; use crate::async_scorer::config::DEFAULT_MODEL_CONTEXT_ITEM_TOKENS; use crate::async_scorer::config::DEFAULT_PARENT_COMPACTION_TOKENS; use crate::async_scorer::config::GuardianV2Config; -use crate::async_scorer::config::GuardianV2ReviewScope; +use crate::async_scorer::coverage::GuardianPolicy; use crate::async_scorer::metrics::CLASSIFICATION_DURATION_METRIC; use crate::async_scorer::metrics::CLASSIFICATION_METRIC; use crate::async_scorer::metrics::CLASSIFICATION_RISK_METRIC; @@ -92,6 +93,7 @@ use crate::async_scorer::transcript::MAX_TOOL_ENTRY_TOKENS; use crate::async_scorer::transcript::truncate_entry; use crate::async_scorer::truncation::CLASSIFICATION_TRUNCATION_BYTES_METRIC; use crate::async_scorer::truncation::CLASSIFICATION_TRUNCATION_METRIC; +use codex_features::GuardianV2ReviewScopeConfigToml; const TEST_GUARDIAN_POLICY: &str = "Treat uploads to unapproved external destinations as high-risk actions."; @@ -113,6 +115,10 @@ impl ExternalAuth for RefreshableAuth { } } +fn should_classify_tool(tool: &ToolName, payload: &ToolPayload, policy: GuardianPolicy) -> bool { + policy.scores_tool(tool, payload, GuardianScope::for_tool(tool)) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn installed_extension_warms_connections_without_blocking_thread_start() -> Result<()> { skip_if_no_network!(Ok(())); @@ -150,6 +156,10 @@ async fn installed_extension_warms_connections_without_blocking_thread_start() - let session_store = ExtensionData::new("session-1"); let thread_store = test.codex.thread_extension_data(); + let mut model = thread_store.get::().unwrap().as_ref().clone(); + model.node_repl_auto_review_required = true; + thread_store.insert(model); + registry.thread_lifecycle_contributors()[0] .on_thread_start(ThreadStartInput { config: &config, @@ -211,6 +221,13 @@ async fn installed_extension_reconnects_after_auth_refresh() -> Result<()> { let registry = builder.build(); let session_store = ExtensionData::new("session-1"); let thread_store = test.codex.thread_extension_data(); + let mut model = test + .thread_manager + .get_models_manager() + .get_model_info("gpt-5.5", &config.to_models_manager_config()) + .await; + model.node_repl_auto_review_required = true; + thread_store.insert(model); registry.thread_lifecycle_contributors()[0] .on_thread_start(ThreadStartInput { config: &config, @@ -231,13 +248,6 @@ async fn installed_extension_reconnects_after_auth_refresh() -> Result<()> { let progress = thread_store .get::() .expect("Guardian v2 should initialize"); - let mut model = test - .thread_manager - .get_models_manager() - .get_model_info("gpt-5.5", &config.to_models_manager_config()) - .await; - model.node_repl_auto_review_required = true; - thread_store.insert(model); let turn_store = ExtensionData::new("turn-1"); let tool_name = ToolName::namespaced("mcp__node_repl__", "js"); let payload = ToolPayload::Function { @@ -455,40 +465,42 @@ async fn sandboxed_shell_classification_respects_review_scope() -> Result<()> { }; let tool_name = ToolName::plain("exec_command"); - let standard_scope = GuardianV2ReviewScope::Standard { - sandboxed_exec_commands: false, - }; + let standard_scope = GuardianPolicy::from_legacy(Some(&GuardianV2ReviewScopeConfigToml { + computer_use_only: Some(false), + sandboxed_exec_commands: Some(false), + })); assert!(!should_classify_tool( &tool_name, &sandboxed, - standard_scope, + standard_scope.clone(), )); assert!(!should_classify_tool( &tool_name, &additional_permissions, - standard_scope, + standard_scope.clone(), )); assert!(should_classify_tool( &tool_name, &unsandboxed, - standard_scope, + standard_scope.clone(), )); assert!(should_classify_tool( &tool_name, &sandboxed, - GuardianV2ReviewScope::Standard { - sandboxed_exec_commands: true, - }, + GuardianPolicy::from_legacy(Some(&GuardianV2ReviewScopeConfigToml { + computer_use_only: Some(false), + sandboxed_exec_commands: Some(true), + })), )); assert!(should_classify_tool( &ToolName::plain("read_file"), &sandboxed, - standard_scope, + standard_scope.clone(), )); assert!(should_classify_tool( &ToolName::namespaced("mcp", "exec_command"), &sandboxed, - standard_scope, + standard_scope.clone(), )); skip_if_no_network!(Ok(())); @@ -550,7 +562,11 @@ fn computer_use_only_classification_recognizes_direct_and_code_mode_tools() { (ToolName::plain("exec_command"), false), ] { assert_eq!( - should_classify_tool(&tool_name, &payload, GuardianV2ReviewScope::ComputerUseOnly), + should_classify_tool( + &tool_name, + &payload, + GuardianPolicy::from_legacy(/*scope*/ None) + ), expected, "unexpected classification scope for {tool_name}" ); @@ -563,12 +579,7 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> { let fixture = GuardianFailureFixture::new().await?; let thread_store = fixture.test.codex.thread_extension_data(); - let mut model = fixture - .test - .thread_manager - .get_models_manager() - .get_model_info("gpt-5.5", &fixture.test.config.to_models_manager_config()) - .await; + let mut model = thread_store.get::().unwrap().as_ref().clone(); model.node_repl_auto_review_required = true; thread_store.insert(model); let mut config = thread_store @@ -576,7 +587,7 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> { .expect("Guardian v2 should have initialized") .as_ref() .clone(); - config.review_scope = GuardianV2ReviewScope::ComputerUseOnly; + config.policy = GuardianPolicy::from_legacy(/*scope*/ None); thread_store.insert(config); thread_store.insert(SecurityRiskScore { scores: BTreeMap::from([("action_risk".to_owned(), 0.25)]), @@ -588,6 +599,10 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> { let progress = thread_store .get::() .expect("Guardian v2 should track score progress per thread"); + // The seeded low score belongs to the model selected above. + let authorization = + super::super::authorization::ScoreAuthorization::current(&fixture.test.codex).await; + *progress.authorization.lock().unwrap() = Some(authorization); let latest_tool_call = progress.latest_tool_call.load(Ordering::Acquire); let turn_store = ExtensionData::new("turn-1"); let ordinary_tool = ToolName::namespaced("mcp__ordinary__", "write_record"); @@ -665,22 +680,31 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> { None, "malformed approval actions must not reuse a browser score" ); - thread_store.remove::(); + let mut changed_model = fixture + .test + .thread_manager + .get_models_manager() + .get_model_info("gpt-5.5", &fixture.test.config.to_models_manager_config()) + .await; + changed_model.guardian = Some(codex_protocol::openai_models::GuardianModelPolicy { + computer_use: Some(codex_protocol::openai_models::GuardianReviewMode::Adaptive), + ..Default::default() + }); + thread_store.insert(changed_model); assert_eq!( fixture .registry .fast_approval_decision( &fixture.session_store, thread_store, - &json!({"tool": "mcp_tool_call", "server": "node_repl"}).to_string(), - thread_store - .get::() - .map(|metrics| metrics as Arc), + &json!({"tool": "mcp_tool_call", "server": "node_repl", "tool_name": "js"}) + .to_string(), + /*extension_metrics*/ None, ) .await, - None + None, + "a score from the previous model policy must not approve a call" ); - let fast_decisions = thread_store .get::() .unwrap() @@ -704,11 +728,11 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> { fast_decision_metric("deferred", "out_of_scope"), fast_decision_metric("deferred", "out_of_scope"), fast_decision_metric("deferred", "out_of_scope"), - fast_decision_metric("deferred", "missing_score"), ] ); let mut model = thread_store.get::().unwrap().as_ref().clone(); + model.guardian = None; // Exercise the legacy fallback after the catalog case above. model.node_repl_auto_review_required = false; thread_store.insert(model.clone()); fixture @@ -1100,6 +1124,58 @@ struct GuardianFailureFixture { session_store: ExtensionData, } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn disabled_code_mode_invalidates_cached_scores() -> Result<()> { + skip_if_no_network!(Ok(())); + let fixture = GuardianFailureFixture::new().await?; + let thread_store = fixture.test.codex.thread_extension_data(); + let mut model = thread_store + .get::() + .expect("resolved model") + .as_ref() + .clone(); + model.guardian = Some(codex_protocol::openai_models::GuardianModelPolicy { + computer_use: Some(codex_protocol::openai_models::GuardianReviewMode::Adaptive), + ..Default::default() + }); + thread_store.insert(model); + let progress = thread_store + .get::() + .expect("score progress"); + let before = ( + progress.latest_tool_call.load(Ordering::Acquire), + progress.latest_failed_tool_call.load(Ordering::Acquire), + ); + fixture.score_tool(ToolName::plain("wait")).await; + assert_eq!( + ( + progress.latest_tool_call.load(Ordering::Acquire), + progress.latest_failed_tool_call.load(Ordering::Acquire), + ), + before, + ); + fixture.score_tool(ToolName::plain("exec")).await; + assert_eq!( + ( + progress.latest_tool_call.load(Ordering::Acquire), + progress.latest_failed_tool_call.load(Ordering::Acquire), + ), + (before.0 + 1, before.0 + 1), + ); + // An MCP tool with the same name remains in the MCP category. + fixture + .score_tool(ToolName::namespaced("mcp__ordinary", "wait")) + .await; + assert_eq!( + ( + progress.latest_tool_call.load(Ordering::Acquire), + progress.latest_failed_tool_call.load(Ordering::Acquire), + ), + (before.0 + 2, before.0 + 2), + ); + Ok(()) +} + impl GuardianFailureFixture { async fn new() -> Result { Self::with_config("").await @@ -2289,9 +2365,10 @@ async fn contributor_skips_required_models_in_standard_scope() -> Result<()> { .expect("Guardian v2 should have initialized") .as_ref() .clone(); - guardian_config.review_scope = GuardianV2ReviewScope::Standard { - sandboxed_exec_commands: false, - }; + guardian_config.policy = GuardianPolicy::from_legacy(Some(&GuardianV2ReviewScopeConfigToml { + computer_use_only: Some(false), + sandboxed_exec_commands: Some(false), + })); thread_store.insert(guardian_config); let mut model_info = test @@ -2501,16 +2578,18 @@ async fn assert_compaction_approval_policy(thread_context_enabled: bool) -> Resu fixture.test.codex.guardian_authorization_version().await, authorization ); + let score_authorization = super::ScoreAuthorization::current(&fixture.test.codex).await; + *thread_store + .get::() + .expect("score progress") + .authorization + .lock() + .unwrap() = Some(score_authorization); // No new sample runs: only the enabled path rejects cached and initial-call approvals. - for (scope, prompt) in [ + for (computer_use_only, prompt) in [ + (false, "review action"), ( - GuardianV2ReviewScope::Standard { - sandboxed_exec_commands: true, - }, - "review action", - ), - ( - GuardianV2ReviewScope::ComputerUseOnly, + true, r#"{"tool":"mcp_tool_call","server":"node_repl","connector_id":"node_repl","tool_name":"js"}"#, ), ] { @@ -2518,7 +2597,10 @@ async fn assert_compaction_approval_policy(thread_context_enabled: bool) -> Resu .get::() .expect("Guardian configuration")) .clone(); - config.review_scope = scope; + config.policy = GuardianPolicy::from_legacy(Some(&GuardianV2ReviewScopeConfigToml { + computer_use_only: Some(computer_use_only), + sandboxed_exec_commands: Some(true), + })); thread_store.insert(config); thread_store .get::() diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/mod.rs b/codex-rs/ext/guardian-v2/src/async_scorer/mod.rs index 8a44c2f609..4b57eeda00 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/mod.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/mod.rs @@ -1,6 +1,7 @@ mod action; mod authorization; mod config; +mod coverage; mod extension; mod metrics; mod review_evidence; diff --git a/codex-rs/models-manager/src/model_info.rs b/codex-rs/models-manager/src/model_info.rs index b79c67b2cf..fd9e2615c1 100644 --- a/codex-rs/models-manager/src/model_info.rs +++ b/codex-rs/models-manager/src/model_info.rs @@ -179,6 +179,7 @@ pub fn model_info_from_slug(slug: &str) -> ModelInfo { used_fallback_model_metadata: true, // this is the fallback model metadata supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: false, node_repl_disabled: false, auto_review_model_override: None, diff --git a/codex-rs/protocol/src/openai_models.rs b/codex-rs/protocol/src/openai_models.rs index 9bd7e1b1b6..649b8e2125 100644 --- a/codex-rs/protocol/src/openai_models.rs +++ b/codex-rs/protocol/src/openai_models.rs @@ -33,6 +33,12 @@ use crate::config_types::ServiceTier; use crate::config_types::Verbosity; use crate::protocol::MultiAgentVersion; +#[path = "openai_models/guardian.rs"] +mod guardian; +pub use guardian::GuardianModelPolicy; +pub use guardian::GuardianReviewMode; +pub use guardian::GuardianScope; + #[path = "openai_models/guardian_v2.rs"] mod guardian_v2; @@ -390,6 +396,11 @@ const fn is_true(value: &bool) -> bool { /// Model metadata returned by the Codex backend `/models` endpoint. #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] pub struct ModelInfo { + /// Model-owned approval coverage. Absent preserves legacy settings; an empty map disables + /// ordinary Guardian review. Keys are computer_use, shell, code_mode, file_changes, mcp, network, + /// and permissions. This does not override mandatory safety or administrator requirements. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub guardian: Option, pub slug: String, pub display_name: String, pub description: Option, @@ -961,7 +972,7 @@ mod tests { ); } - fn test_model(spec: Option) -> ModelInfo { + pub(super) fn test_model(spec: Option) -> ModelInfo { ModelInfo { slug: "test-model".to_string(), display_name: "Test Model".to_string(), @@ -999,6 +1010,7 @@ mod tests { used_fallback_model_metadata: false, supports_search_tool: false, use_responses_lite: false, + guardian: None, node_repl_auto_review_required: false, node_repl_disabled: false, auto_review_model_override: None, diff --git a/codex-rs/protocol/src/openai_models/guardian.rs b/codex-rs/protocol/src/openai_models/guardian.rs new file mode 100644 index 0000000000..26d2819ccd --- /dev/null +++ b/codex-rs/protocol/src/openai_models/guardian.rs @@ -0,0 +1,127 @@ +//! Model-owned Guardian coverage. Missing policy preserves legacy behavior; +//! unknown modes retain synchronous review and never enable the fast path. + +use schemars::JsonSchema; +use serde::Deserialize; +use serde::Serialize; +use ts_rs::TS; + +use super::ModelInfo; +use crate::ToolName; +use crate::mcp::is_node_repl_backed_server; +use crate::mcp::is_node_repl_backed_tool; + +/// How Guardian handles an action when the user selects automatic approval. +#[derive(Debug, Serialize, Deserialize, Clone, Copy, PartialEq, Eq, TS, JsonSchema)] +#[serde(rename_all = "snake_case")] +pub enum GuardianReviewMode { + Disabled, + Synchronous, + /// Use a current low-risk score; otherwise run synchronous review. + Adaptive, + #[serde(other)] + Unknown, +} + +/// A complete model policy. Omitted scopes are disabled; unknown fields are ignored. +#[derive(Debug, Default, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] +pub struct GuardianModelPolicy { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub computer_use: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub shell: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub code_mode: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub file_changes: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub mcp: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub network: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub permissions: Option, +} + +impl GuardianModelPolicy { + pub fn review_mode(&self, scope: GuardianScope) -> GuardianReviewMode { + match scope { + GuardianScope::ComputerUse => self.computer_use, + GuardianScope::Shell => self.shell, + GuardianScope::CodeMode => self.code_mode, + GuardianScope::FileChanges => self.file_changes, + GuardianScope::Mcp => self.mcp, + GuardianScope::Network => self.network, + GuardianScope::Permissions => self.permissions, + } + .unwrap_or(GuardianReviewMode::Disabled) + } +} + +/// Approval categories understood by this client. Future catalog keys are ignored. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum GuardianScope { + ComputerUse, + Shell, + CodeMode, + FileChanges, + Mcp, + Network, + Permissions, +} + +impl GuardianScope { + pub fn for_mcp_server(server: &str) -> Self { + if is_node_repl_backed_server(server) { + Self::ComputerUse + } else { + Self::Mcp + } + } + + pub fn for_tool(tool: &ToolName) -> Option { + if is_node_repl_backed_tool(&tool.name, tool.namespace.as_deref()) { + return Some(Self::ComputerUse); + } + if tool + .namespace + .as_deref() + .is_some_and(|namespace| namespace.starts_with("mcp__")) + || tool.name.starts_with("mcp__") + { + return Some(Self::Mcp); + } + if !tool.is_default_namespace() { + return None; + } + match tool.name.as_str() { + "shell" | "shell_command" | "exec_command" | "write_stdin" | "execve" => { + Some(Self::Shell) + } + "exec" | "wait" => Some(Self::CodeMode), + "apply_patch" => Some(Self::FileChanges), + "request_permissions" => Some(Self::Permissions), + _ => None, + } + } +} + +impl ModelInfo { + /// An absent map uses legacy config; an omitted scope in a supplied map is disabled. + pub fn guardian_review_mode(&self, scope: GuardianScope) -> Option { + self.guardian + .as_ref() + .map(|policy| policy.review_mode(scope)) + } + + /// The legacy metadata bit remains the transport understood by older CUA servers. + pub fn computer_use_review_required(&self) -> bool { + self.guardian_review_mode(GuardianScope::ComputerUse) + .map_or(self.node_repl_auto_review_required, |mode| { + mode != GuardianReviewMode::Disabled + }) + } +} + +#[cfg(test)] +#[path = "guardian_tests.rs"] +mod tests; diff --git a/codex-rs/protocol/src/openai_models/guardian_tests.rs b/codex-rs/protocol/src/openai_models/guardian_tests.rs new file mode 100644 index 0000000000..5c0ded0c61 --- /dev/null +++ b/codex-rs/protocol/src/openai_models/guardian_tests.rs @@ -0,0 +1,48 @@ +use super::super::tests::test_model; +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn guardian_policy_is_optional_and_tolerates_future_config() -> anyhow::Result<()> { + let model = test_model(/*spec*/ None); + let mut wire = serde_json::to_value(&model)?; + assert!( + !wire + .as_object() + .expect("model object") + .contains_key("guardian") + ); + assert_eq!(serde_json::from_value::(wire.clone())?, model); + + wire["guardian"] = serde_json::json!({ + "computer_use": "adaptive", "shell": "future_mode", "future_scope": {"version": 2}, + "mcp": null, "code_mode": "adaptive", + }); + let model: ModelInfo = serde_json::from_value(wire)?; + assert_eq!( + model.guardian, + Some(GuardianModelPolicy { + computer_use: Some(GuardianReviewMode::Adaptive), + shell: Some(GuardianReviewMode::Unknown), + code_mode: Some(GuardianReviewMode::Adaptive), + ..Default::default() + }) + ); + assert!(model.computer_use_review_required()); + assert_eq!( + model.guardian_review_mode(GuardianScope::Mcp), + Some(GuardianReviewMode::Disabled) + ); + Ok(()) +} + +#[test] +fn guardian_policy_overrides_the_legacy_cua_bit_only_when_present() { + let mut model = test_model(/*spec*/ None); + model.node_repl_auto_review_required = true; + assert!(model.computer_use_review_required()); + model.guardian = Some(GuardianModelPolicy::default()); + assert!(!model.computer_use_review_required()); + model.guardian.as_mut().expect("policy").computer_use = Some(GuardianReviewMode::Synchronous); + assert!(model.computer_use_review_required()); +}