mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Disable Guardian V2 for managed automatic reviewers (#39005)
## Why Guardian V2 should not override a managed approval-reviewer policy that excludes the user reviewer. ## What changed - Force `features.guardianv2` off when `allowed_approvals_reviewers` does not include `user`, for both local requirements and cloud config bundles. - Preserve Guardian V2 when the user reviewer remains allowed or when only legacy Guardian feature settings are present. - Return the effective disabled value through `config/read`. ## Testing Added coverage for cloud requirements and the app-server config RPC. GitOrigin-RevId: 82b6d0aaf47fbaa58ae349d4e56777cd442f5807
This commit is contained in:
@@ -231,6 +231,40 @@ service_tier = "fast"
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn config_read_disables_guardian_v2_when_managed_config_requires_guardian_v1() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
write_config(&codex_home, "[features]\nguardianv2 = true\n")?;
|
||||
std::fs::write(
|
||||
codex_home.path().join("requirements.toml"),
|
||||
"allowed_approvals_reviewers = [\"auto_review\"]\n",
|
||||
)?;
|
||||
|
||||
let mut mcp = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.build_initialized_with_timeout(DEFAULT_READ_TIMEOUT)
|
||||
.await?;
|
||||
|
||||
let request_id = mcp
|
||||
.send_config_read_request(ConfigReadParams {
|
||||
include_layers: false,
|
||||
cwd: None,
|
||||
})
|
||||
.await?;
|
||||
let ConfigReadResponse { config, .. } =
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(request_id)).await??;
|
||||
|
||||
assert_eq!(
|
||||
config
|
||||
.additional
|
||||
.get("features")
|
||||
.and_then(|features| features.get("guardianv2")),
|
||||
Some(&json!(false))
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn config_read_returns_effective_and_layers() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
use codex_features::FeatureToml;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::config_types::ForcedLoginMethod;
|
||||
use codex_protocol::config_types::SandboxMode;
|
||||
@@ -1433,6 +1434,14 @@ impl ConfigRequirementsToml {
|
||||
apply_exact!(check_for_update_on_startup);
|
||||
apply_exact!(allow_login_shell);
|
||||
|
||||
if self
|
||||
.allowed_approvals_reviewers
|
||||
.as_ref()
|
||||
.is_some_and(|reviewers| !reviewers.contains(&ApprovalsReviewer::User))
|
||||
{
|
||||
config.features.get_or_insert_default().guardianv2 = Some(FeatureToml::Enabled(false));
|
||||
}
|
||||
|
||||
if let Some(enabled) = self.feedback.as_ref().and_then(|feedback| feedback.enabled) {
|
||||
config.feedback.get_or_insert_default().enabled = Some(enabled);
|
||||
}
|
||||
|
||||
@@ -2,7 +2,9 @@ use codex_config::ConfigRequirements;
|
||||
use codex_config::RequirementSource;
|
||||
use codex_config::Sourced;
|
||||
use codex_config::config_toml::ConfigToml;
|
||||
use codex_config::types::ApprovalsReviewer;
|
||||
use codex_config::types::FeedbackConfigToml;
|
||||
use codex_features::FeatureToml;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use std::path::Path;
|
||||
|
||||
@@ -31,6 +33,13 @@ pub(super) fn apply_to_config(
|
||||
apply_exact!(model_catalog_json);
|
||||
apply_exact!(check_for_update_on_startup);
|
||||
apply_exact!(allow_login_shell);
|
||||
if requirements
|
||||
.approvals_reviewer
|
||||
.can_set(&ApprovalsReviewer::User)
|
||||
.is_err()
|
||||
{
|
||||
config.features.get_or_insert_default().guardianv2 = Some(FeatureToml::Enabled(false));
|
||||
}
|
||||
apply_feedback_requirement(
|
||||
&mut config.feedback,
|
||||
requirements.feedback.as_ref(),
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
use anyhow::Result;
|
||||
use codex_config::CloudConfigBundleLoader;
|
||||
use codex_config::test_support::CloudConfigBundleFixture;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
@@ -69,3 +70,41 @@ async fn refreshed_cloud_bundle_updates_later_sessions() -> Result<()> {
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn managed_guardian_v1_requirements_disable_guardian_v2() -> Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
|
||||
for (requirements, guardian_v2_enabled) in [
|
||||
(r#"allowed_approvals_reviewers = ["auto_review"]"#, false),
|
||||
(
|
||||
r#"allowed_approvals_reviewers = ["guardian_subagent"]"#,
|
||||
false,
|
||||
),
|
||||
(
|
||||
r#"allowed_approvals_reviewers = ["auto_review", "user"]"#,
|
||||
true,
|
||||
),
|
||||
("[features]\nguardian_approval = true\n", true),
|
||||
("[features]\nauto_review = true\n", true),
|
||||
] {
|
||||
let mut builder = test_codex()
|
||||
.with_pre_build_hook(|home| {
|
||||
std::fs::write(home.join("config.toml"), "[features]\nguardianv2 = true\n")
|
||||
.expect("Guardian v2 configuration should be written");
|
||||
})
|
||||
.with_cloud_config_bundle(
|
||||
CloudConfigBundleFixture::loader_with_enterprise_requirement(requirements),
|
||||
);
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
|
||||
assert!(test.config.features.enabled(Feature::GuardianApproval));
|
||||
assert_eq!(
|
||||
test.config.features.enabled(Feature::GuardianV2),
|
||||
guardian_v2_enabled,
|
||||
"{requirements}"
|
||||
);
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user