From 7275afc5c7fd2c01e0a3442ead09f44bad52422a Mon Sep 17 00:00:00 2001 From: jif Date: Wed, 16 Sep 2026 14:40:01 +0000 Subject: [PATCH] Centralize Guardian policy resolution in config and protocol (#45957) ## What changed - Extend `GuardianModelPolicy` with controls for uncategorized tools, unscored actions, the initial computer-use call allowance, and sandboxed command coverage. - Add `GuardianPolicyLoader` in `codex-config` to translate legacy settings, preserve catalog policy precedence, and enforce reviewer requirements. Apply live model review requirements through `ConfigRequirements::constrain_guardian_policy`. - Use the shared model policy for Guardian scoring and approval, replacing the extension-local policy wrapper while retaining legacy defaults. ## Testing Add configuration tests for catalog precedence, legacy scope fallback, required-model constraints, and legacy computer-use opt-in and feature gating. Adapt existing extension tests to consume the shared policy. GitOrigin-RevId: 6fed1c3a831964ad28ea7de5cb19e74e323c1204 --- codex-rs/Cargo.lock | 1 + codex-rs/config/src/guardian.rs | 106 +++++++++++ codex-rs/config/src/guardian_tests.rs | 124 +++++++++++++ codex-rs/config/src/lib.rs | 2 + codex-rs/ext/guardian-v2/Cargo.toml | 1 + .../guardian-v2/src/async_scorer/approval.rs | 22 ++- .../guardian-v2/src/async_scorer/config.rs | 45 +++-- .../src/async_scorer/config_tests.rs | 43 ----- .../guardian-v2/src/async_scorer/coverage.rs | 166 +++--------------- .../guardian-v2/src/async_scorer/extension.rs | 6 +- .../src/async_scorer/extension_tests.rs | 51 ++++-- .../src/async_scorer/observation.rs | 7 +- codex-rs/protocol/src/openai_models.rs | 1 + .../protocol/src/openai_models/guardian.rs | 58 +++++- 14 files changed, 390 insertions(+), 243 deletions(-) create mode 100644 codex-rs/config/src/guardian.rs create mode 100644 codex-rs/config/src/guardian_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index d2f5babd5b..1e51bf3834 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3595,6 +3595,7 @@ dependencies = [ "anyhow", "codex-analytics", "codex-api", + "codex-config", "codex-core", "codex-extension-api", "codex-features", diff --git a/codex-rs/config/src/guardian.rs b/codex-rs/config/src/guardian.rs new file mode 100644 index 0000000000..8125c95894 --- /dev/null +++ b/codex-rs/config/src/guardian.rs @@ -0,0 +1,106 @@ +//! Translates legacy Guardian settings and applies live managed constraints. +//! Catalog policies take precedence over legacy scope and feature settings. + +use codex_features::FeatureToml; +use codex_features::GuardianV2ConfigToml; +use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::openai_models::GuardianModelPolicy; +use codex_protocol::openai_models::GuardianReviewMode; +use codex_protocol::openai_models::GuardianUnscoredAction; +use codex_protocol::openai_models::ModelInfo; + +use crate::ConfigRequirements; + +/// Load-boundary adapter for configuration that predates model-owned policies. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct GuardianPolicyLoader { + default: GuardianModelPolicy, + force_synchronous_review: bool, + scoring_disabled: bool, +} + +impl GuardianPolicyLoader { + pub fn new( + legacy: Option<&FeatureToml>, + requirements: &ConfigRequirements, + ) -> Self { + let scope = match legacy { + Some(FeatureToml::Config(config)) => config.review_scope.as_ref(), + Some(FeatureToml::Enabled(_)) | None => None, + }; + let computer_use_only = scope + .and_then(|scope| scope.computer_use_only) + .unwrap_or(/*default*/ true); + let other = if computer_use_only { + GuardianReviewMode::Synchronous + } else { + GuardianReviewMode::Adaptive + }; + let default = GuardianModelPolicy { + computer_use: Some(GuardianReviewMode::Adaptive), + shell: Some(other), + file_changes: Some(other), + mcp: Some(other), + network: Some(other), + permissions: Some(other), + other_tools: other, + unscored_action: if computer_use_only { + GuardianUnscoredAction::Ignore + } else { + GuardianUnscoredAction::AgeScore + }, + initial_cua_call: Some(computer_use_only), + sandboxed_exec_commands: Some( + !computer_use_only + && scope + .and_then(|scope| scope.sandboxed_exec_commands) + .unwrap_or(/*default*/ false), + ), + }; + let enabled = legacy.and_then(FeatureToml::enabled); + Self { + default, + scoring_disabled: !enabled.unwrap_or(/*default*/ false), + force_synchronous_review: requirements + .approvals_reviewer + .can_set(&ApprovalsReviewer::User) + .is_err(), + } + } + + pub fn resolve(&self, model: Option<&ModelInfo>) -> GuardianModelPolicy { + let catalog = model.and_then(|model| model.guardian.as_ref()); + let mut policy = catalog.cloned().unwrap_or_else(|| self.default.clone()); + if catalog.is_none() + && policy.allows_initial_cua_call() + && model.is_some_and(|model| !model.node_repl_auto_review_required) + { + policy.computer_use = Some(GuardianReviewMode::Synchronous); + policy.initial_cua_call = Some(false); + policy.unscored_action = GuardianUnscoredAction::InvalidateScore; + } + // Preserve the legacy feature gate: explicit catalog policies bypass it. + if self.force_synchronous_review || self.scoring_disabled && catalog.is_none() { + policy.disable_scoring(); + } + policy + } +} + +impl ConfigRequirements { + /// Applies live administrator requirements to an already resolved model policy. + pub fn constrain_guardian_policy(&self, policy: &mut GuardianModelPolicy, model: &str) { + if self.auto_review_required_for_model(model) { + let computer_use = policy.computer_use; + let initial_cua_call = policy.allows_initial_cua_call(); + policy.disable_scoring(); + if initial_cua_call { + policy.computer_use = computer_use; + } + } + } +} + +#[cfg(test)] +#[path = "guardian_tests.rs"] +mod tests; diff --git a/codex-rs/config/src/guardian_tests.rs b/codex-rs/config/src/guardian_tests.rs new file mode 100644 index 0000000000..097c510ff9 --- /dev/null +++ b/codex-rs/config/src/guardian_tests.rs @@ -0,0 +1,124 @@ +use super::*; +use crate::FeatureRequirementsToml; +use crate::RequirementSource; +use crate::Sourced; +use pretty_assertions::assert_eq; +use serde_json::json; +use std::collections::BTreeSet; + +#[test] +fn explicit_catalog_policy_preserves_precedence_over_the_legacy_switch() { + let catalog: GuardianModelPolicy = serde_json::from_value(json!({ + "computer_use": "adaptive", "shell": "disabled" + })) + .unwrap(); + for legacy in [json!(false), json!({"enabled": false})] { + let legacy = serde_json::from_value(legacy).unwrap(); + for managed in [None, Some(false), Some(true)] { + let requirements = ConfigRequirements { + feature_requirements: managed.map(|enabled| { + Sourced::new( + FeatureRequirementsToml { + entries: [("guardianv2".to_owned(), enabled)].into(), + }, + RequirementSource::Unknown, + ) + }), + ..Default::default() + }; + let mut model = test_model(); + model.guardian = Some(catalog.clone()); + assert_eq!( + GuardianPolicyLoader::new(Some(&legacy), &requirements).resolve(Some(&model)), + catalog + ); + } + } +} + +#[test] +fn legacy_scope_applies_only_to_models_without_catalog_policy() { + let legacy = serde_json::from_value(json!({ + "enabled": true, + "review_scope": {"computer_use_only": false, "sandboxed_exec_commands": true} + })) + .unwrap(); + let loader = GuardianPolicyLoader::new(Some(&legacy), &ConfigRequirements::default()); + let expected = GuardianModelPolicy { + computer_use: Some(GuardianReviewMode::Adaptive), + shell: Some(GuardianReviewMode::Adaptive), + file_changes: Some(GuardianReviewMode::Adaptive), + mcp: Some(GuardianReviewMode::Adaptive), + network: Some(GuardianReviewMode::Adaptive), + permissions: Some(GuardianReviewMode::Adaptive), + other_tools: GuardianReviewMode::Adaptive, + unscored_action: GuardianUnscoredAction::AgeScore, + initial_cua_call: Some(false), + sandboxed_exec_commands: Some(true), + }; + assert_eq!(loader.resolve(/*model*/ None), expected); + let catalog: GuardianModelPolicy = + serde_json::from_value(json!({"computer_use": "adaptive"})).unwrap(); + let mut model = test_model(); + model.guardian = Some(catalog.clone()); + assert_eq!(loader.resolve(Some(&model)), catalog); +} + +#[test] +fn required_models_preserve_cua_allowance_and_disable_all_tools_scoring() { + let requirements = ConfigRequirements { + auto_review_required_models: Some(Sourced::new( + BTreeSet::from(["protected-model".to_owned()]), + RequirementSource::Unknown, + )), + ..Default::default() + }; + let legacy = serde_json::from_value(json!({ + "enabled": true, "review_scope": {"computer_use_only": false} + })) + .unwrap(); + let cua_only = FeatureToml::Enabled(true); + for legacy in [Some(&cua_only), Some(&legacy)] { + let loader = GuardianPolicyLoader::new(legacy, &requirements); + let ordinary = loader.resolve(/*model*/ None); + let mut expected = ordinary.clone(); + expected.disable_scoring(); + if ordinary.allows_initial_cua_call() { + expected.computer_use = ordinary.computer_use; + } + let mut actual = ordinary; + requirements.constrain_guardian_policy(&mut actual, "provider/protected-model"); + assert_eq!(actual, expected); + } +} + +#[test] +fn legacy_cua_opt_in_and_disabled_feature_preserve_scoring_behavior() { + for legacy in [ + json!(false), + json!({"review_scope": {"computer_use_only": true}}), + json!(true), + ] { + let enabled = legacy == json!(true); + let legacy = serde_json::from_value(legacy).unwrap(); + let loader = GuardianPolicyLoader::new(Some(&legacy), &ConfigRequirements::default()); + for required in [false, true] { + let mut model = test_model(); + model.node_repl_auto_review_required = required; + let policy = loader.resolve(Some(&model)); + assert_eq!(policy.scoring_enabled(), enabled && required); + assert_eq!(model.computer_use_review_required(), required); + } + } +} + +fn test_model() -> ModelInfo { + serde_json::from_value(json!({ + "slug": "test-model", "display_name": "test-model", + "supported_reasoning_levels": [], "shell_type": "shell_command", + "visibility": "list", "supported_in_api": true, "priority": 0, + "support_verbosity": false, "truncation_policy": {"mode": "bytes", "limit": 10000}, + "experimental_supported_tools": [] + })) + .unwrap() +} diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 949e883837..3053aea812 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -14,6 +14,7 @@ mod constraint; mod diagnostics; mod filesystem_constraints; mod fingerprint; +mod guardian; mod hook_config; mod host_name; mod in_app_browser_requirements; @@ -141,6 +142,7 @@ pub use diagnostics::format_config_error; pub use diagnostics::format_config_error_with_source; pub use diagnostics::io_error_from_config_error; pub use fingerprint::version_for_toml; +pub use guardian::GuardianPolicyLoader; pub use hook_config::HookEventsToml; pub use hook_config::HookHandlerConfig; pub use hook_config::HookStateToml; diff --git a/codex-rs/ext/guardian-v2/Cargo.toml b/codex-rs/ext/guardian-v2/Cargo.toml index 35033d756b..4121fc1ce9 100644 --- a/codex-rs/ext/guardian-v2/Cargo.toml +++ b/codex-rs/ext/guardian-v2/Cargo.toml @@ -16,6 +16,7 @@ workspace = true anyhow = { workspace = true } codex-analytics = { workspace = true } codex-api = { workspace = true } +codex-config = { workspace = true } codex-core = { workspace = true } codex-extension-api = { workspace = true } codex-features = { workspace = true } diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs b/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs index 85ef325a6d..2e4c2d24e2 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/approval.rs @@ -3,7 +3,6 @@ use super::authorization::ScoreAuthorization; use super::config::GuardianV2Config; -use super::coverage::GuardianPolicy; use super::metrics::TOOL_CALL_LAG_METRIC; use super::metrics::record_fast_decision; use super::parent_compaction::select_parent_compaction; @@ -18,6 +17,7 @@ use codex_extension_api::ApprovalReviewContributor; use codex_extension_api::ExtensionFuture; use codex_protocol::approvals::GuardianReviewReason; use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::openai_models::GuardianModelPolicy; use codex_protocol::openai_models::GuardianReviewMode; use codex_protocol::openai_models::GuardianScope; use codex_protocol::protocol::AskForApproval; @@ -77,18 +77,22 @@ impl GuardianApprovalReviewer { .map_or_else(|| GuardianV2Config::resolve(&config), Ok) .ok(); let mut policy = guardian_config.as_ref().map_or_else( - || GuardianPolicy::from_legacy(/*scope*/ None).for_model(model.as_deref()), + || { + codex_config::GuardianPolicyLoader::new( + Some(&codex_features::FeatureToml::Enabled(true)), + &codex_config::ConfigRequirements::default(), + ) + .resolve(model.as_deref()) + }, |config| config.policy_for_model(model.as_deref()), ); - if model.as_ref().is_some_and(|model| { + if let Some(model) = model.as_ref() { config .config_layer_stack .requirements() - .auto_review_required_for_model(&model.slug) - }) { - policy.enforce_required_model(); + .constrain_guardian_policy(&mut policy, &model.slug); } - let mode = policy.mode(input.category); + let mode = policy.review_mode(input.category); if mode != GuardianReviewMode::Adaptive { record_fast_decision(input.metrics.as_deref(), "deferred", "out_of_scope"); } @@ -127,7 +131,7 @@ async fn cached_evidence( thread: &CodexThread, input: &ApprovalDecisionInput<'_>, config: &GuardianV2Config, - policy: &GuardianPolicy, + policy: &GuardianModelPolicy, ) -> Result<(), GuardianReviewReason> { let store = input.thread_store; let metrics = input.metrics.as_deref(); @@ -180,7 +184,7 @@ async fn cached_evidence( } let action = input.action; if input.category == GuardianScope::ComputerUse - && policy.initial_cua_call + && policy.allows_initial_cua_call() && action.get("tool_name").and_then(serde_json::Value::as_str) == Some("js") && action .get("connector_id") 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 1230c15135..0125ba2f49 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/config.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/config.rs @@ -1,11 +1,12 @@ +use codex_config::GuardianPolicyLoader; use codex_core::config::Config; use codex_features::FeatureToml; use codex_features::GuardianV2ConfigToml; use codex_features::GuardianV2TranscriptConfigToml; +use codex_protocol::openai_models::GuardianModelPolicy; 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; @@ -38,9 +39,7 @@ 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(super) policy: GuardianPolicy, - force_synchronous_review: bool, - scoring_disabled: bool, + pub(super) policy: GuardianPolicyLoader, pub(crate) transcript: TranscriptConfig, } @@ -64,28 +63,22 @@ impl GuardianV2Config { None => GuardianV2ConfigToml::default(), }; - 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); + let mut resolved = Self::from_overrides(configured.clone())?; + // Config.features can be changed after loading, including for reviewer threads. + let legacy = FeatureToml::Config(GuardianV2ConfigToml { + enabled: Some(config.features.enabled(codex_features::Feature::GuardianV2)), + ..configured + }); + resolved.policy = + GuardianPolicyLoader::new(Some(&legacy), config.config_layer_stack.requirements()); 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 + ) -> GuardianModelPolicy { + self.policy.resolve(model) } pub(crate) fn with_model_defaults( @@ -166,8 +159,6 @@ 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) } @@ -249,7 +240,13 @@ impl GuardianV2Config { ); } - let policy = GuardianPolicy::from_legacy(configured.review_scope.as_ref()); + let policy = GuardianPolicyLoader::new( + Some(&FeatureToml::Config(GuardianV2ConfigToml { + enabled: Some(true), + ..configured.clone() + })), + &codex_config::ConfigRequirements::default(), + ); Ok(Self { local_overrides: configured.clone(), persist_scores: configured.persist_scores.unwrap_or(false), @@ -266,8 +263,6 @@ impl GuardianV2Config { reuse_parent_compaction: configured.reuse_parent_compaction.unwrap_or(true), max_parent_compaction_tokens, 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 63e32d6d59..a3dbe3ca2a 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 @@ -1,5 +1,4 @@ use codex_features::GuardianV2ConfigToml; -use codex_features::GuardianV2ReviewScopeConfigToml; use codex_features::GuardianV2TranscriptConfigToml; use codex_protocol::openai_models::GuardianV2ModelConfig; use codex_protocol::openai_models::GuardianV2TranscriptModelConfig; @@ -9,50 +8,8 @@ use pretty_assertions::assert_eq; use super::CLASSIFICATION_OUTPUT_INSTRUCTIONS; use super::DEFAULT_CLASSIFIER_INSTRUCTIONS; use super::GuardianV2Config; -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.policy, GuardianPolicy::from_legacy(/*scope*/ None)); -} - -#[test] -fn sandboxed_exec_commands_can_be_included() { - let config = GuardianV2Config::from_overrides(GuardianV2ConfigToml { - review_scope: Some(GuardianV2ReviewScopeConfigToml { - computer_use_only: Some(false), - sandboxed_exec_commands: Some(true), - }), - ..Default::default() - }) - .unwrap(); - - assert_eq!( - config.policy, - GuardianPolicy::from_legacy(Some(&GuardianV2ReviewScopeConfigToml { - computer_use_only: Some(false), - sandboxed_exec_commands: Some(true), - })) - ); -} - -#[test] -fn computer_use_only_takes_precedence_over_sandboxed_exec_commands() { - let config = GuardianV2Config::from_overrides(GuardianV2ConfigToml { - review_scope: Some(GuardianV2ReviewScopeConfigToml { - computer_use_only: Some(true), - sandboxed_exec_commands: Some(true), - }), - ..Default::default() - }) - .unwrap(); - - assert_eq!(config.policy, GuardianPolicy::from_legacy(/*scope*/ None)); -} - #[test] fn template_policy_is_substituted_before_the_single_truncation() { for max_tokens in [256, 1_000, 2_000] { diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/coverage.rs b/codex-rs/ext/guardian-v2/src/async_scorer/coverage.rs index 41cc40d58a..21e19b4023 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/coverage.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/coverage.rs @@ -1,151 +1,29 @@ -//! 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. -//! Model policies review nested actions; legacy tool coverage remains unchanged. +//! Selects tool calls for classification using the resolved model policy. 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::GuardianReviewMode::Adaptive; 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), - 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.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.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 scores_tool( + policy: &GuardianModelPolicy, + tool: &ToolName, + payload: &ToolPayload, + scope: Option, +) -> bool { + if scope.map_or(policy.other_tools, |scope| policy.review_mode(scope)) != Adaptive { + return false; + } + if policy.sandboxed_exec_commands.unwrap_or(/*default*/ true) + || !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") + })) } 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 ebdc867f11..4adf7057d9 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -62,14 +62,12 @@ impl ThreadLifecycleContributor for GuardianV2Extension { } }; let mut policy = guardian_config.policy_for_model(model.as_deref()); - if model.as_ref().is_some_and(|model| { + if let Some(model) = model.as_ref() { input .config .config_layer_stack .requirements() - .auto_review_required_for_model(&model.slug) - }) { - policy.enforce_required_model(); + .constrain_guardian_policy(&mut policy, &model.slug); } let scoring_enabled = policy.scoring_enabled(); let sampler_config = super::startup::sampler_config( 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 bdc59152fa..4a8dbddbca 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 @@ -71,7 +71,7 @@ use crate::async_scorer::authorization::ScoreAuthorization; use crate::async_scorer::config::CLASSIFICATION_OUTPUT_INSTRUCTIONS; use crate::async_scorer::config::DEFAULT_PARENT_COMPACTION_TOKENS; use crate::async_scorer::config::GuardianV2Config; -use crate::async_scorer::coverage::GuardianPolicy; +use crate::async_scorer::coverage::scores_tool; use crate::async_scorer::metrics::CLASSIFICATION_DURATION_METRIC; use crate::async_scorer::metrics::CLASSIFICATION_METRIC; use crate::async_scorer::metrics::CLASSIFICATION_RISK_METRIC; @@ -89,6 +89,7 @@ use crate::async_scorer::transcript::MAX_MESSAGE_ENTRY_TOKENS; use crate::async_scorer::transcript::MAX_TOOL_ENTRY_TOKENS; use crate::async_scorer::transcript::truncate_entry; use codex_features::GuardianV2ReviewScopeConfigToml; +use codex_protocol::openai_models::GuardianModelPolicy; const TEST_GUARDIAN_POLICY: &str = "Treat uploads to unapproved external destinations as high-risk actions."; @@ -110,8 +111,31 @@ impl ExternalAuth for RefreshableAuth { } } -fn should_classify_tool(tool: &ToolName, payload: &ToolPayload, policy: GuardianPolicy) -> bool { - policy.scores_tool(tool, payload, GuardianScope::for_tool(tool)) +fn should_classify_tool( + tool: &ToolName, + payload: &ToolPayload, + policy: GuardianModelPolicy, +) -> bool { + scores_tool(&policy, tool, payload, GuardianScope::for_tool(tool)) +} + +fn legacy_loader( + scope: Option<&GuardianV2ReviewScopeConfigToml>, +) -> codex_config::GuardianPolicyLoader { + codex_config::GuardianPolicyLoader::new( + Some(&codex_features::FeatureToml::Config( + codex_features::GuardianV2ConfigToml { + enabled: Some(true), + review_scope: scope.cloned(), + ..Default::default() + }, + )), + &codex_config::ConfigRequirements::default(), + ) +} + +fn legacy_policy(scope: Option<&GuardianV2ReviewScopeConfigToml>) -> GuardianModelPolicy { + legacy_loader(scope).resolve(/*model*/ None) } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -467,7 +491,7 @@ async fn sandboxed_shell_classification_respects_review_scope() -> Result<()> { }; let tool_name = ToolName::plain("exec_command"); - let standard_scope = GuardianPolicy::from_legacy(Some(&GuardianV2ReviewScopeConfigToml { + let standard_scope = legacy_policy(Some(&GuardianV2ReviewScopeConfigToml { computer_use_only: Some(false), sandboxed_exec_commands: Some(false), })); @@ -489,7 +513,7 @@ async fn sandboxed_shell_classification_respects_review_scope() -> Result<()> { assert!(should_classify_tool( &tool_name, &sandboxed, - GuardianPolicy::from_legacy(Some(&GuardianV2ReviewScopeConfigToml { + legacy_policy(Some(&GuardianV2ReviewScopeConfigToml { computer_use_only: Some(false), sandboxed_exec_commands: Some(true), })), @@ -587,11 +611,7 @@ fn computer_use_only_classification_recognizes_direct_and_code_mode_tools() { (ToolName::plain("exec_command"), false), ] { assert_eq!( - should_classify_tool( - &tool_name, - &payload, - GuardianPolicy::from_legacy(/*scope*/ None) - ), + should_classify_tool(&tool_name, &payload, legacy_policy(/*scope*/ None)), expected, "unexpected classification scope for {tool_name}" ); @@ -612,7 +632,7 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> { .expect("Guardian v2 should have initialized") .as_ref() .clone(); - config.policy = GuardianPolicy::from_legacy(/*scope*/ None); + config.policy = legacy_loader(/*scope*/ None); thread_store.insert(config); thread_store.insert(SecurityRiskScore { scores: BTreeMap::from([("action_risk".to_owned(), 0.25)]), @@ -2202,7 +2222,7 @@ async fn contributor_skips_required_models_in_standard_scope() -> Result<()> { .expect("Guardian v2 should have initialized") .as_ref() .clone(); - guardian_config.policy = GuardianPolicy::from_legacy(Some(&GuardianV2ReviewScopeConfigToml { + guardian_config.policy = legacy_loader(Some(&GuardianV2ReviewScopeConfigToml { computer_use_only: Some(false), sandboxed_exec_commands: Some(false), })); @@ -2431,7 +2451,7 @@ async fn assert_compaction_approval_policy(thread_context_enabled: bool) -> Resu .get::() .expect("Guardian configuration")) .clone(); - config.policy = GuardianPolicy::from_legacy(Some(&GuardianV2ReviewScopeConfigToml { + config.policy = legacy_loader(Some(&GuardianV2ReviewScopeConfigToml { computer_use_only: Some(computer_use_only), sandboxed_exec_commands: Some(true), })); @@ -3324,7 +3344,10 @@ async fn cached_approval( 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::()?.policy.other_tools + None if store + .get::()? + .policy_for_model(store.get::().as_deref()) + .other_tools == codex_protocol::openai_models::GuardianReviewMode::Adaptive => { codex_protocol::openai_models::GuardianScope::Shell diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/observation.rs b/codex-rs/ext/guardian-v2/src/async_scorer/observation.rs index aa195d58be..b5a8158f71 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/observation.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/observation.rs @@ -28,7 +28,7 @@ use super::action::GuardianAction; use super::authorization::ScoreAuthorization; use super::classification::Classification; use super::config::GuardianV2Config; -use super::coverage::UnscoredAction; +use super::coverage::scores_tool; use super::extension::GuardianV2Extension; use super::metrics::record_classification; use super::parent_compaction::ParentCompactionError; @@ -36,6 +36,7 @@ use super::parent_compaction::select_parent_compaction; use super::sampler::LunaSampler; use super::score::GuardianV2ScoreProgress; use super::score::record_fail_closed_score; +use codex_protocol::openai_models::GuardianUnscoredAction as UnscoredAction; impl GuardianV2Extension { pub(super) async fn score_tool(&self, input: ToolStartInput<'_>) { @@ -75,7 +76,7 @@ impl GuardianV2Extension { { return; } - if !policy.scores_tool(input.tool_name, input.payload, scope) { + if !scores_tool(&policy, input.tool_name, input.payload, scope) { match policy.unscored_action { UnscoredAction::Ignore => {} UnscoredAction::AgeScore => { @@ -183,7 +184,7 @@ impl GuardianV2Extension { return; } // A required model keeps synchronous review outside its CUA allowance. - if !(scope == Some(GuardianScope::ComputerUse) && policy.initial_cua_call) + if !(scope == Some(GuardianScope::ComputerUse) && policy.allows_initial_cua_call()) && parent_model.as_ref().is_some_and(|model| { config .config_layer_stack diff --git a/codex-rs/protocol/src/openai_models.rs b/codex-rs/protocol/src/openai_models.rs index 0821fe61c0..ec0038959b 100644 --- a/codex-rs/protocol/src/openai_models.rs +++ b/codex-rs/protocol/src/openai_models.rs @@ -39,6 +39,7 @@ mod guardian; pub use guardian::GuardianModelPolicy; pub use guardian::GuardianReviewMode; pub use guardian::GuardianScope; +pub use guardian::GuardianUnscoredAction; #[path = "openai_models/guardian_v2.rs"] mod guardian_v2; diff --git a/codex-rs/protocol/src/openai_models/guardian.rs b/codex-rs/protocol/src/openai_models/guardian.rs index 00c07ed96b..7616092c97 100644 --- a/codex-rs/protocol/src/openai_models/guardian.rs +++ b/codex-rs/protocol/src/openai_models/guardian.rs @@ -12,9 +12,10 @@ 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)] +#[derive(Debug, Default, Serialize, Deserialize, Clone, Copy, PartialEq, Eq, TS, JsonSchema)] #[serde(rename_all = "snake_case")] pub enum GuardianReviewMode { + #[default] Disabled, Synchronous, /// Use a current low-risk score; otherwise run synchronous review. @@ -23,6 +24,17 @@ pub enum GuardianReviewMode { Unknown, } +/// How actions outside adaptive coverage affect cached classification evidence. +#[derive(Debug, Default, Serialize, Deserialize, Clone, Copy, PartialEq, Eq, TS, JsonSchema)] +#[serde(rename_all = "snake_case")] +pub enum GuardianUnscoredAction { + Ignore, + AgeScore, + #[default] + #[serde(other)] + InvalidateScore, +} + /// A complete model policy. Omitted scopes are disabled; unknown fields are ignored. /// Code Mode wrappers have no approval scope; their nested tools follow this policy. #[derive(Debug, Default, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] @@ -39,6 +51,17 @@ pub struct GuardianModelPolicy { pub network: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub permissions: Option, + /// Coverage for tools without an approval category. + #[serde(default)] + pub other_tools: GuardianReviewMode, + #[serde(default)] + pub unscored_action: GuardianUnscoredAction, + /// Omission retains the existing allowance for adaptive computer use. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub initial_cua_call: Option, + /// Whether adaptive shell coverage includes ordinary sandboxed commands. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub sandboxed_exec_commands: Option, } impl GuardianModelPolicy { @@ -53,6 +76,39 @@ impl GuardianModelPolicy { } .unwrap_or(GuardianReviewMode::Disabled) } + + pub fn scoring_enabled(&self) -> bool { + [ + self.computer_use, + self.shell, + self.file_changes, + self.mcp, + self.network, + self.permissions, + ] + .contains(&Some(GuardianReviewMode::Adaptive)) + } + + pub fn disable_scoring(&mut self) { + for mode in [ + &mut self.computer_use, + &mut self.shell, + &mut self.file_changes, + &mut self.mcp, + &mut self.network, + &mut self.permissions, + ] { + if *mode == Some(GuardianReviewMode::Adaptive) { + *mode = Some(GuardianReviewMode::Synchronous); + } + } + self.other_tools = GuardianReviewMode::Synchronous; + } + + pub fn allows_initial_cua_call(&self) -> bool { + self.initial_cua_call + .unwrap_or(self.computer_use == Some(GuardianReviewMode::Adaptive)) + } } /// Approval categories understood by this client. Future catalog keys are ignored.