mirror of
https://github.com/openai/codex.git
synced 2026-11-05 20:10:33 +00:00
Resolve managed app reviewers during config loading
This commit is contained in:
@@ -17,7 +17,12 @@ use codex_config::ConfigLayerStack;
|
||||
use codex_config::ConfigLayerStackOrdering;
|
||||
use codex_config::ConfigRequirementsToml;
|
||||
use codex_config::config_toml::ConfigToml;
|
||||
use codex_config::configured_app_approvals_reviewers;
|
||||
use codex_config::merge_toml_values;
|
||||
use codex_config::resolve_app_approvals_reviewers;
|
||||
use codex_config::resolve_approvals_reviewer;
|
||||
use codex_config::types::AppConfig as AppConfigToml;
|
||||
use codex_config::types::ApprovalsReviewer;
|
||||
use codex_core::config::deserialize_config_toml_with_base;
|
||||
use codex_core::config::edit::ConfigEdit;
|
||||
use codex_core::config::edit::ConfigEditsBuilder;
|
||||
@@ -125,9 +130,35 @@ impl ConfigManager {
|
||||
};
|
||||
|
||||
let effective = layers.effective_config();
|
||||
let effective_config_toml: ConfigToml = effective
|
||||
let mut effective_config_toml: ConfigToml = effective
|
||||
.try_into()
|
||||
.map_err(|err| ConfigManagerError::toml("invalid configuration", err))?;
|
||||
let requirements = layers.requirements();
|
||||
let global_reviewer = resolve_approvals_reviewer(
|
||||
effective_config_toml.approvals_reviewer,
|
||||
ApprovalsReviewer::User,
|
||||
&requirements.approvals_reviewer,
|
||||
);
|
||||
let configured_app_reviewers =
|
||||
configured_app_approvals_reviewers(effective_config_toml.apps.as_ref());
|
||||
let app_reviewers = resolve_app_approvals_reviewers(
|
||||
&configured_app_reviewers,
|
||||
global_reviewer,
|
||||
&requirements.approvals_reviewer,
|
||||
&requirements.app_approvals_reviewers,
|
||||
);
|
||||
if !app_reviewers.is_empty() {
|
||||
let apps = effective_config_toml.apps.get_or_insert_default();
|
||||
for (app_id, reviewer) in app_reviewers {
|
||||
apps.apps
|
||||
.entry(app_id)
|
||||
.or_insert_with(|| AppConfigToml {
|
||||
enabled: true,
|
||||
..Default::default()
|
||||
})
|
||||
.approvals_reviewer = Some(reviewer);
|
||||
}
|
||||
}
|
||||
|
||||
let json_value = serde_json::to_value(&effective_config_toml)
|
||||
.map_err(|err| ConfigManagerError::json("failed to serialize configuration", err))?;
|
||||
|
||||
@@ -2,6 +2,7 @@ use super::*;
|
||||
use anyhow::Result;
|
||||
use codex_app_server_protocol::AppConfig;
|
||||
use codex_app_server_protocol::AppToolApproval;
|
||||
use codex_app_server_protocol::ApprovalsReviewer;
|
||||
use codex_app_server_protocol::AppsConfig;
|
||||
use codex_app_server_protocol::AskForApproval;
|
||||
use codex_config::CloudConfigBundleLoader;
|
||||
@@ -297,6 +298,80 @@ async fn write_value_supports_nested_app_paths() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn read_reports_app_reviewer_after_app_requirements_are_applied() -> Result<()> {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
std::fs::write(
|
||||
tmp.path().join(CONFIG_TOML_FILE),
|
||||
r#"
|
||||
approvals_reviewer = "user"
|
||||
|
||||
[apps.calendar]
|
||||
approvals_reviewer = "user"
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let service = ConfigManager::new_for_tests(
|
||||
tmp.path().to_path_buf(),
|
||||
vec![],
|
||||
LoaderOverrides::without_managed_config_for_tests(),
|
||||
CloudConfigBundleFixture::loader_with_enterprise_requirement(
|
||||
r#"
|
||||
allowed_approvals_reviewers = ["user", "auto_review"]
|
||||
|
||||
[apps.calendar.allowed_approvals_reviewers]
|
||||
user = false
|
||||
auto_review = true
|
||||
|
||||
[apps.drive.allowed_approvals_reviewers]
|
||||
user = false
|
||||
auto_review = true
|
||||
"#,
|
||||
),
|
||||
);
|
||||
|
||||
let read = service
|
||||
.read(ConfigReadParams {
|
||||
include_layers: true,
|
||||
cwd: None,
|
||||
})
|
||||
.await
|
||||
.expect("config read succeeds");
|
||||
|
||||
assert_eq!(
|
||||
read.config
|
||||
.apps
|
||||
.as_ref()
|
||||
.and_then(|apps| apps.apps.get("calendar"))
|
||||
.and_then(|app| app.approvals_reviewer),
|
||||
Some(ApprovalsReviewer::AutoReview)
|
||||
);
|
||||
assert_eq!(
|
||||
read.config
|
||||
.apps
|
||||
.as_ref()
|
||||
.and_then(|apps| apps.apps.get("drive"))
|
||||
.map(|app| (app.enabled, app.approvals_reviewer)),
|
||||
Some((true, Some(ApprovalsReviewer::AutoReview)))
|
||||
);
|
||||
assert!(
|
||||
read.layers
|
||||
.expect("layers should be included")
|
||||
.iter()
|
||||
.any(|layer| {
|
||||
layer
|
||||
.config
|
||||
.get("apps")
|
||||
.and_then(|apps| apps.get("calendar"))
|
||||
.and_then(|calendar| calendar.get("approvals_reviewer"))
|
||||
== Some(&serde_json::json!("user"))
|
||||
}),
|
||||
"raw config layers should preserve the configured reviewer"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn write_value_supports_custom_mcp_server_default_tool_approval_mode() -> Result<()> {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
|
||||
@@ -10,6 +10,7 @@ use serde::de::Error as _;
|
||||
use serde::de::value::Error as ValueDeserializerError;
|
||||
use serde::de::value::StrDeserializer;
|
||||
use std::collections::BTreeMap;
|
||||
use std::collections::BTreeSet;
|
||||
use std::fmt;
|
||||
use wildmatch::WildMatchPattern;
|
||||
|
||||
@@ -20,6 +21,7 @@ use crate::ConstraintError;
|
||||
use crate::ManagedHooksRequirementsToml;
|
||||
use crate::mcp_types::AppToolApproval;
|
||||
use crate::permissions_toml::PermissionProfileToml;
|
||||
use crate::types::AppsConfigToml;
|
||||
use crate::types::WindowsSandboxModeToml;
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
@@ -223,6 +225,58 @@ impl ConfigRequirements {
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns explicit per-app reviewer choices from ordinary config layers.
|
||||
pub fn configured_app_approvals_reviewers(
|
||||
apps: Option<&AppsConfigToml>,
|
||||
) -> BTreeMap<String, ApprovalsReviewer> {
|
||||
apps.into_iter()
|
||||
.flat_map(|apps| &apps.apps)
|
||||
.filter_map(|(app_id, app)| {
|
||||
app.approvals_reviewer
|
||||
.map(|reviewer| (app_id.clone(), reviewer))
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// Resolves a configured reviewer and fallback against one managed constraint.
|
||||
pub fn resolve_approvals_reviewer(
|
||||
configured: Option<ApprovalsReviewer>,
|
||||
fallback: ApprovalsReviewer,
|
||||
constraint: &ConstrainedWithSource<ApprovalsReviewer>,
|
||||
) -> ApprovalsReviewer {
|
||||
configured
|
||||
.into_iter()
|
||||
.chain([fallback])
|
||||
.find(|reviewer| constraint.can_set(reviewer).is_ok())
|
||||
.unwrap_or_else(|| constraint.value())
|
||||
}
|
||||
|
||||
/// Resolves effective reviewers for apps with an explicit reviewer or managed
|
||||
/// app-specific constraint.
|
||||
pub fn resolve_app_approvals_reviewers(
|
||||
configured: &BTreeMap<String, ApprovalsReviewer>,
|
||||
global_reviewer: ApprovalsReviewer,
|
||||
global_constraint: &ConstrainedWithSource<ApprovalsReviewer>,
|
||||
app_constraints: &BTreeMap<String, ConstrainedWithSource<ApprovalsReviewer>>,
|
||||
) -> BTreeMap<String, ApprovalsReviewer> {
|
||||
configured
|
||||
.keys()
|
||||
.chain(app_constraints.keys())
|
||||
.cloned()
|
||||
.collect::<BTreeSet<_>>()
|
||||
.into_iter()
|
||||
.map(|app_id| {
|
||||
let constraint = app_constraints.get(&app_id).unwrap_or(global_constraint);
|
||||
let reviewer = resolve_approvals_reviewer(
|
||||
configured.get(&app_id).copied(),
|
||||
global_reviewer,
|
||||
constraint,
|
||||
);
|
||||
(app_id, reviewer)
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
#[derive(Deserialize, Debug, Clone, PartialEq, Eq)]
|
||||
#[serde(untagged)]
|
||||
pub enum McpServerIdentity {
|
||||
|
||||
@@ -77,6 +77,9 @@ pub use config_requirements::SandboxModeRequirement;
|
||||
pub use config_requirements::Sourced;
|
||||
pub use config_requirements::WebSearchModeRequirement;
|
||||
pub use config_requirements::WindowsRequirementsToml;
|
||||
pub use config_requirements::configured_app_approvals_reviewers;
|
||||
pub use config_requirements::resolve_app_approvals_reviewers;
|
||||
pub use config_requirements::resolve_approvals_reviewer;
|
||||
pub use config_requirements::sandbox_mode_requirement_for_permission_profile;
|
||||
pub use constraint::Constrained;
|
||||
pub use constraint::ConstraintError;
|
||||
|
||||
@@ -9710,6 +9710,51 @@ async fn root_approvals_reviewer_falls_back_when_disallowed_by_requirements() ->
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn app_approvals_reviewer_falls_back_when_disallowed_by_requirements() -> std::io::Result<()>
|
||||
{
|
||||
let codex_home = TempDir::new()?;
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
r#"approvals_reviewer = "user"
|
||||
|
||||
[apps.calendar]
|
||||
approvals_reviewer = "user"
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let config = ConfigBuilder::without_managed_config_for_tests()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.fallback_cwd(Some(codex_home.path().to_path_buf()))
|
||||
.cloud_config_bundle(
|
||||
CloudConfigBundleFixture::loader_with_enterprise_requirement(
|
||||
r#"allowed_approvals_reviewers = ["user", "auto_review"]
|
||||
|
||||
[apps.calendar.allowed_approvals_reviewers]
|
||||
user = false
|
||||
auto_review = true
|
||||
"#,
|
||||
),
|
||||
)
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert_eq!(
|
||||
config.app_approvals_reviewers.get("calendar"),
|
||||
Some(&ApprovalsReviewer::AutoReview)
|
||||
);
|
||||
assert!(
|
||||
config.startup_warnings.iter().any(|warning| {
|
||||
warning.contains(
|
||||
"Configured value for `apps.calendar.approvals_reviewer` is disallowed by requirements",
|
||||
)
|
||||
}),
|
||||
"{:?}",
|
||||
config.startup_warnings
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn profile_approvals_reviewer_falls_back_when_disallowed_by_requirements()
|
||||
-> std::io::Result<()> {
|
||||
|
||||
@@ -633,6 +633,13 @@ pub struct Config {
|
||||
/// ARC.
|
||||
pub approvals_reviewer: ApprovalsReviewer,
|
||||
|
||||
/// Effective reviewer overrides for individual apps.
|
||||
pub app_approvals_reviewers: BTreeMap<String, ApprovalsReviewer>,
|
||||
|
||||
/// Configured app reviewer values used to recompute effective overrides
|
||||
/// when the thread-level reviewer changes.
|
||||
configured_app_approvals_reviewers: BTreeMap<String, ApprovalsReviewer>,
|
||||
|
||||
/// enforce_residency means web traffic cannot be routed outside of a
|
||||
/// particular geography. HTTP clients should direct their requests
|
||||
/// using backend-specific headers or URLs to enforce this.
|
||||
@@ -1282,6 +1289,19 @@ impl ConfigBuilder {
|
||||
}
|
||||
|
||||
impl Config {
|
||||
pub(crate) fn set_approvals_reviewer(&mut self, approvals_reviewer: ApprovalsReviewer) {
|
||||
self.approvals_reviewer = approvals_reviewer;
|
||||
self.app_approvals_reviewers = codex_config::resolve_app_approvals_reviewers(
|
||||
&self.configured_app_approvals_reviewers,
|
||||
approvals_reviewer,
|
||||
&self.config_layer_stack.requirements().approvals_reviewer,
|
||||
&self
|
||||
.config_layer_stack
|
||||
.requirements()
|
||||
.app_approvals_reviewers,
|
||||
);
|
||||
}
|
||||
|
||||
pub(crate) fn multi_agent_version_from_features(&self) -> MultiAgentVersion {
|
||||
if self.features.enabled(Feature::MultiAgentV2) {
|
||||
MultiAgentVersion::V2
|
||||
@@ -2553,7 +2573,7 @@ impl Config {
|
||||
let ConfigRequirements {
|
||||
approval_policy: mut constrained_approval_policy,
|
||||
approvals_reviewer: mut constrained_approvals_reviewer,
|
||||
app_approvals_reviewers: _,
|
||||
app_approvals_reviewers: constrained_app_approvals_reviewers,
|
||||
permission_profile: mut constrained_permission_profile,
|
||||
windows_sandbox_mode: mut constrained_windows_sandbox_mode,
|
||||
web_search_mode: mut constrained_web_search_mode,
|
||||
@@ -3320,6 +3340,34 @@ impl Config {
|
||||
&mut constrained_approvals_reviewer,
|
||||
&mut startup_warnings,
|
||||
)?;
|
||||
let configured_app_approvals_reviewers =
|
||||
codex_config::configured_app_approvals_reviewers(cfg.apps.as_ref());
|
||||
let app_approvals_reviewers = codex_config::resolve_app_approvals_reviewers(
|
||||
&configured_app_approvals_reviewers,
|
||||
constrained_approvals_reviewer.value(),
|
||||
&constrained_approvals_reviewer,
|
||||
&constrained_app_approvals_reviewers,
|
||||
);
|
||||
for (app_id, configured_reviewer) in &configured_app_approvals_reviewers {
|
||||
let constraint = constrained_app_approvals_reviewers
|
||||
.get(app_id)
|
||||
.unwrap_or(&constrained_approvals_reviewer);
|
||||
if let Err(err) = constraint.can_set(configured_reviewer) {
|
||||
let fallback_value = app_approvals_reviewers
|
||||
.get(app_id)
|
||||
.copied()
|
||||
.unwrap_or_else(|| constrained_approvals_reviewer.value());
|
||||
tracing::warn!(
|
||||
error = %err,
|
||||
?fallback_value,
|
||||
requirement_source = ?constraint.source,
|
||||
"configured app approvals reviewer is disallowed by requirements; falling back to required value for apps.{app_id}.approvals_reviewer"
|
||||
);
|
||||
startup_warnings.push(format!(
|
||||
"Configured value for `apps.{app_id}.approvals_reviewer` is disallowed by requirements; falling back to required value {fallback_value:?}. Details: {err}"
|
||||
));
|
||||
}
|
||||
}
|
||||
let permission_profile_was_constrained = apply_requirement_constrained_value(
|
||||
"permission_profile",
|
||||
permission_profile,
|
||||
@@ -3430,6 +3478,8 @@ impl Config {
|
||||
explicit_permission_profile_mode,
|
||||
custom_permission_profiles,
|
||||
approvals_reviewer: constrained_approvals_reviewer.value(),
|
||||
app_approvals_reviewers,
|
||||
configured_app_approvals_reviewers,
|
||||
enforce_residency: enforce_residency.value,
|
||||
notify: cfg.notify,
|
||||
user_instructions,
|
||||
|
||||
@@ -581,29 +581,10 @@ pub(crate) fn mcp_approvals_reviewer(
|
||||
return config.approvals_reviewer;
|
||||
}
|
||||
|
||||
let app_reviewer = read_user_apps_config(config).and_then(|apps_config| {
|
||||
connector_id
|
||||
.and_then(|connector_id| apps_config.apps.get(connector_id))
|
||||
.and_then(|app| app.approvals_reviewer)
|
||||
});
|
||||
let reviewer_constraint = config
|
||||
.config_layer_stack
|
||||
.requirements()
|
||||
.approvals_reviewer_constraint_for_app(connector_id);
|
||||
|
||||
app_approvals_reviewer(config, app_reviewer, reviewer_constraint)
|
||||
}
|
||||
|
||||
fn app_approvals_reviewer(
|
||||
config: &Config,
|
||||
configured: Option<ApprovalsReviewer>,
|
||||
reviewer_constraint: &codex_config::ConstrainedWithSource<ApprovalsReviewer>,
|
||||
) -> ApprovalsReviewer {
|
||||
configured
|
||||
.into_iter()
|
||||
.chain([config.approvals_reviewer])
|
||||
.find(|reviewer| reviewer_constraint.can_set(reviewer).is_ok())
|
||||
.unwrap_or_else(|| reviewer_constraint.value())
|
||||
connector_id
|
||||
.and_then(|connector_id| config.app_approvals_reviewers.get(connector_id))
|
||||
.copied()
|
||||
.unwrap_or(config.approvals_reviewer)
|
||||
}
|
||||
|
||||
fn read_apps_config(config: &Config) -> Option<AppsConfigToml> {
|
||||
|
||||
@@ -576,6 +576,45 @@ auto_review = {auto_review}
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn app_approvals_reviewer_is_recomputed_when_global_reviewer_changes() {
|
||||
let codex_home = tempdir().expect("tempdir should succeed");
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
r#"approvals_reviewer = "user"
|
||||
"#,
|
||||
)
|
||||
.expect("write config");
|
||||
let mut config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.cloud_config_bundle(
|
||||
CloudConfigBundleFixture::loader_with_enterprise_requirement(
|
||||
r#"
|
||||
allowed_approvals_reviewers = ["user", "auto_review"]
|
||||
|
||||
[apps.calendar.allowed_approvals_reviewers]
|
||||
user = true
|
||||
auto_review = true
|
||||
"#,
|
||||
),
|
||||
)
|
||||
.build()
|
||||
.await
|
||||
.expect("config should build");
|
||||
|
||||
assert_eq!(
|
||||
mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("calendar")),
|
||||
ApprovalsReviewer::User
|
||||
);
|
||||
|
||||
config.set_approvals_reviewer(ApprovalsReviewer::AutoReview);
|
||||
|
||||
assert_eq!(
|
||||
mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("calendar")),
|
||||
ApprovalsReviewer::AutoReview
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn requirements_disabled_connector_overrides_enabled_connector() {
|
||||
let mut effective_apps = AppsConfigToml {
|
||||
|
||||
@@ -421,7 +421,7 @@ impl Session {
|
||||
per_turn_config.model_reasoning_summary = session_configuration.model_reasoning_summary;
|
||||
per_turn_config.service_tier = session_configuration.service_tier.clone();
|
||||
per_turn_config.personality = session_configuration.personality;
|
||||
per_turn_config.approvals_reviewer = session_configuration.approvals_reviewer;
|
||||
per_turn_config.set_approvals_reviewer(session_configuration.approvals_reviewer);
|
||||
session_configuration
|
||||
.apply_permission_profile_to_permissions(&mut per_turn_config.permissions);
|
||||
let permission_profile = session_configuration.permission_profile();
|
||||
|
||||
Reference in New Issue
Block a user