mirror of
https://github.com/openai/codex.git
synced 2026-10-20 00:18:49 +00:00
Centralize app enabled-state evaluation (#36916)
## What changed - Add `AppToolPolicyEvaluator::apply_app_enabled_state` and use it when presenting app lists, building plugin context, and deciding whether app instructions are available. - Preserve each app's source state unless local or managed configuration explicitly overrides it. - Keep connector discovery and post-install refresh checks based on raw accessibility rather than configured enablement. ## Testing - Cover default enablement, per-app overrides, managed disablement, and preservation of unconfigured source state. GitOrigin-RevId: f1a62d55e7cc48b37113848e3baa0d69d8d9c8a8
This commit is contained in:
@@ -1,5 +1,6 @@
|
||||
use super::*;
|
||||
use crate::app_info::app_info_to_api;
|
||||
use codex_connectors::AppToolPolicyEvaluator;
|
||||
|
||||
mod installed;
|
||||
mod read;
|
||||
@@ -250,12 +251,13 @@ impl AppsRequestProcessor {
|
||||
let mut codex_apps_ready = true;
|
||||
let mut last_notified_apps = None;
|
||||
let mut sent_app_list_update = false;
|
||||
let app_policy = AppToolPolicyEvaluator::new(&config.config_layer_stack);
|
||||
|
||||
if accessible_connectors.is_some() || all_connectors.is_some() {
|
||||
let merged = connectors::with_app_enabled_state(
|
||||
merge_loaded_apps(all_connectors.as_deref(), accessible_connectors.as_deref()),
|
||||
&config,
|
||||
);
|
||||
let merged = app_policy.apply_app_enabled_state(merge_loaded_apps(
|
||||
all_connectors.as_deref(),
|
||||
accessible_connectors.as_deref(),
|
||||
));
|
||||
if !force_refetch {
|
||||
last_notified_apps = Some(merged);
|
||||
} else if should_send_app_list_updated_notification(
|
||||
@@ -314,10 +316,10 @@ impl AppsRequestProcessor {
|
||||
} else {
|
||||
accessible_connectors.as_deref()
|
||||
};
|
||||
let merged = connectors::with_app_enabled_state(
|
||||
merge_loaded_apps(all_connectors_for_update, accessible_connectors_for_update),
|
||||
&config,
|
||||
);
|
||||
let merged = app_policy.apply_app_enabled_state(merge_loaded_apps(
|
||||
all_connectors_for_update,
|
||||
accessible_connectors_for_update,
|
||||
));
|
||||
if should_send_app_list_updated_notification(
|
||||
merged.as_slice(),
|
||||
accessible_loaded,
|
||||
|
||||
@@ -6,6 +6,7 @@ use crate::chatgpt_client::chatgpt_get_request_with_timeout;
|
||||
use crate::chatgpt_client::chatgpt_post_request_with_timeout;
|
||||
|
||||
use codex_connectors::AppInfo;
|
||||
use codex_connectors::AppToolPolicyEvaluator;
|
||||
use codex_connectors::ConnectorDirectoryCacheContext;
|
||||
use codex_connectors::ConnectorDirectoryCacheKey;
|
||||
use codex_connectors::ConnectorMetadata;
|
||||
@@ -21,7 +22,6 @@ pub use codex_core::connectors::list_accessible_connectors_from_mcp_tools_with_m
|
||||
pub use codex_core::connectors::list_accessible_connectors_from_mcp_tools_with_options;
|
||||
pub use codex_core::connectors::list_accessible_connectors_from_mcp_tools_with_options_and_status;
|
||||
pub use codex_core::connectors::list_cached_accessible_connectors_from_mcp_tools;
|
||||
pub use codex_core::connectors::with_app_enabled_state;
|
||||
use codex_login::AuthManager;
|
||||
use codex_login::CodexAuth;
|
||||
use codex_plugin::AppConnectorId;
|
||||
@@ -65,12 +65,13 @@ pub async fn list_connectors(config: &Config) -> anyhow::Result<Vec<AppInfo>> {
|
||||
);
|
||||
let connectors = connectors_result?;
|
||||
let accessible = accessible_result?;
|
||||
Ok(with_app_enabled_state(
|
||||
merge_connectors_with_accessible(
|
||||
connectors, accessible, /*all_connectors_loaded*/ true,
|
||||
Ok(
|
||||
AppToolPolicyEvaluator::new(&config.config_layer_stack).apply_app_enabled_state(
|
||||
merge_connectors_with_accessible(
|
||||
connectors, accessible, /*all_connectors_loaded*/ true,
|
||||
),
|
||||
),
|
||||
config,
|
||||
))
|
||||
)
|
||||
}
|
||||
|
||||
pub async fn list_all_connectors(config: &Config) -> anyhow::Result<Vec<AppInfo>> {
|
||||
|
||||
@@ -4,6 +4,8 @@ use codex_config::types::AppToolApproval;
|
||||
use codex_config::types::AppsConfigToml;
|
||||
use serde::Deserialize;
|
||||
|
||||
use crate::AppInfo;
|
||||
|
||||
/// The effective enablement and approval policy for one app tool.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub struct AppToolPolicy {
|
||||
@@ -63,6 +65,21 @@ impl<'a> AppToolPolicyEvaluator<'a> {
|
||||
.unwrap_or(true)
|
||||
}
|
||||
|
||||
/// Applies app policy without overriding source state for unconfigured apps.
|
||||
pub fn apply_app_enabled_state(&self, mut apps: Vec<AppInfo>) -> Vec<AppInfo> {
|
||||
let Some(apps_config) = self.apps_config.as_ref() else {
|
||||
return apps;
|
||||
};
|
||||
|
||||
for app in &mut apps {
|
||||
if apps_config.default.is_some() || apps_config.apps.contains_key(app.id.as_str()) {
|
||||
app.is_enabled = self.app_enabled(app.id.as_str());
|
||||
}
|
||||
}
|
||||
|
||||
apps
|
||||
}
|
||||
|
||||
fn from_parts(
|
||||
apps_config: Option<AppsConfigToml>,
|
||||
requirements_apps_config: Option<&'a AppsRequirementsToml>,
|
||||
|
||||
@@ -189,6 +189,61 @@ fn app_enablement_uses_defaults_and_per_app_overrides() {
|
||||
],
|
||||
[true, false, false]
|
||||
);
|
||||
|
||||
let evaluator = AppToolPolicyEvaluator::from_parts(
|
||||
Some(apps_config),
|
||||
/*requirements_apps_config*/ None,
|
||||
);
|
||||
assert_eq!(
|
||||
evaluator.apply_app_enabled_state(vec![
|
||||
app("calendar", /*enabled*/ false),
|
||||
app("drive", /*enabled*/ true),
|
||||
]),
|
||||
vec![
|
||||
app("calendar", /*enabled*/ true),
|
||||
app("drive", /*enabled*/ false),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn app_enablement_preserves_source_state_and_honors_local_and_managed_overrides() {
|
||||
let apps_config = AppsConfigToml {
|
||||
default: None,
|
||||
apps: HashMap::from([
|
||||
(
|
||||
"calendar".to_string(),
|
||||
AppConfig {
|
||||
enabled: true,
|
||||
..Default::default()
|
||||
},
|
||||
),
|
||||
(
|
||||
"drive".to_string(),
|
||||
AppConfig {
|
||||
enabled: true,
|
||||
..Default::default()
|
||||
},
|
||||
),
|
||||
]),
|
||||
};
|
||||
let requirements = app_enabled_requirement("drive", /*enabled*/ false);
|
||||
let evaluator = AppToolPolicyEvaluator::from_parts(Some(apps_config), Some(&requirements));
|
||||
|
||||
assert_eq!(
|
||||
evaluator.apply_app_enabled_state(vec![
|
||||
app("calendar", /*enabled*/ false),
|
||||
app("drive", /*enabled*/ true),
|
||||
app("slack", /*enabled*/ false),
|
||||
app("gmail", /*enabled*/ true),
|
||||
]),
|
||||
vec![
|
||||
app("calendar", /*enabled*/ true),
|
||||
app("drive", /*enabled*/ false),
|
||||
app("slack", /*enabled*/ false),
|
||||
app("gmail", /*enabled*/ true),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -657,6 +712,26 @@ fn input<'a>(tool_name: &'a str, tool_title: Option<&'a str>) -> AppToolPolicyIn
|
||||
}
|
||||
}
|
||||
|
||||
fn app(id: &str, enabled: bool) -> AppInfo {
|
||||
AppInfo {
|
||||
id: id.to_string(),
|
||||
name: id.to_string(),
|
||||
description: None,
|
||||
logo_url: None,
|
||||
logo_url_dark: None,
|
||||
icon_assets: None,
|
||||
icon_dark_assets: None,
|
||||
distribution_channel: None,
|
||||
branding: None,
|
||||
app_metadata: None,
|
||||
labels: None,
|
||||
install_url: None,
|
||||
is_accessible: true,
|
||||
is_enabled: enabled,
|
||||
plugin_display_names: Vec::new(),
|
||||
}
|
||||
}
|
||||
|
||||
fn policy_from_apps_config(
|
||||
apps_config: Option<&AppsConfigToml>,
|
||||
connector_id: Option<&str>,
|
||||
|
||||
@@ -10,7 +10,6 @@ pub use codex_connectors::AppInfo;
|
||||
pub use codex_connectors::AppMetadata;
|
||||
use codex_connectors::ConnectorDirectoryCacheContext;
|
||||
use codex_connectors::ConnectorDirectoryCacheKey;
|
||||
use codex_connectors::app_is_enabled;
|
||||
use codex_connectors::apps_config_from_layer_stack;
|
||||
use codex_connectors::connector_runtime_context_key;
|
||||
use codex_exec_server::EnvironmentManager;
|
||||
@@ -490,32 +489,6 @@ fn accessible_connectors_for_app_list_from_mcp_tools(mcp_tools: &[ToolInfo]) ->
|
||||
collect_accessible_connectors_from_mcp_tools(non_synthetic_tools)
|
||||
}
|
||||
|
||||
pub fn with_app_enabled_state(mut connectors: Vec<AppInfo>, config: &Config) -> Vec<AppInfo> {
|
||||
let user_apps_config = apps_config_from_layer_stack(&config.config_layer_stack);
|
||||
let requirements_apps_config = config.config_layer_stack.requirements_toml().apps.as_ref();
|
||||
if user_apps_config.is_none() && requirements_apps_config.is_none() {
|
||||
return connectors;
|
||||
}
|
||||
|
||||
for connector in &mut connectors {
|
||||
if let Some(apps_config) = user_apps_config.as_ref()
|
||||
&& (apps_config.default.is_some()
|
||||
|| apps_config.apps.contains_key(connector.id.as_str()))
|
||||
{
|
||||
connector.is_enabled = app_is_enabled(apps_config, Some(connector.id.as_str()));
|
||||
}
|
||||
|
||||
if requirements_apps_config
|
||||
.and_then(|apps| apps.apps.get(connector.id.as_str()))
|
||||
.is_some_and(|app| app.enabled == Some(false))
|
||||
{
|
||||
connector.is_enabled = false;
|
||||
}
|
||||
}
|
||||
|
||||
connectors
|
||||
}
|
||||
|
||||
pub fn with_app_plugin_sources(
|
||||
mut connectors: Vec<AppInfo>,
|
||||
tool_plugin_provenance: &ToolPluginProvenance,
|
||||
|
||||
@@ -1,11 +1,6 @@
|
||||
use super::*;
|
||||
use crate::config::CONFIG_TOML_FILE;
|
||||
use crate::config::ConfigBuilder;
|
||||
use codex_config::AppRequirementToml;
|
||||
use codex_config::AppsRequirementsToml;
|
||||
use codex_config::ConfigLayerStack;
|
||||
use codex_config::ConfigRequirements;
|
||||
use codex_config::ConfigRequirementsToml;
|
||||
use codex_config::test_support::CloudConfigBundleFixture;
|
||||
use codex_config::types::ApprovalsReviewer;
|
||||
use codex_connectors::merge::plugin_connector_to_app_info;
|
||||
@@ -19,31 +14,10 @@ use pretty_assertions::assert_eq;
|
||||
use rmcp::model::JsonObject;
|
||||
use rmcp::model::MetaObject;
|
||||
use rmcp::model::Tool;
|
||||
use std::collections::BTreeMap;
|
||||
use std::collections::HashSet;
|
||||
use std::sync::Arc;
|
||||
use tempfile::tempdir;
|
||||
|
||||
fn app(id: &str) -> AppInfo {
|
||||
AppInfo {
|
||||
id: id.to_string(),
|
||||
name: id.to_string(),
|
||||
description: None,
|
||||
logo_url: None,
|
||||
logo_url_dark: None,
|
||||
icon_assets: None,
|
||||
icon_dark_assets: None,
|
||||
distribution_channel: None,
|
||||
install_url: None,
|
||||
branding: None,
|
||||
app_metadata: None,
|
||||
labels: None,
|
||||
is_accessible: false,
|
||||
is_enabled: true,
|
||||
plugin_display_names: Vec::new(),
|
||||
}
|
||||
}
|
||||
|
||||
fn plugin_names(names: &[&str]) -> Vec<String> {
|
||||
names.iter().map(ToString::to_string).collect()
|
||||
}
|
||||
@@ -452,44 +426,6 @@ approvals_reviewer = "user"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn with_app_enabled_state_preserves_unrelated_disabled_connector() {
|
||||
let codex_home = tempdir().expect("tempdir should succeed");
|
||||
let mut config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.fallback_cwd(Some(codex_home.path().to_path_buf()))
|
||||
.build()
|
||||
.await
|
||||
.expect("config should build");
|
||||
|
||||
let requirements = ConfigRequirementsToml {
|
||||
apps: Some(AppsRequirementsToml {
|
||||
apps: BTreeMap::from([(
|
||||
"connector_drive".to_string(),
|
||||
AppRequirementToml {
|
||||
enabled: Some(false),
|
||||
tools: None,
|
||||
},
|
||||
)]),
|
||||
}),
|
||||
..Default::default()
|
||||
};
|
||||
config.config_layer_stack =
|
||||
ConfigLayerStack::new(Vec::new(), ConfigRequirements::default(), requirements)
|
||||
.expect("requirements stack");
|
||||
|
||||
let mut slack = app("connector_slack");
|
||||
slack.is_enabled = false;
|
||||
|
||||
let mut drive = app("connector_drive");
|
||||
drive.is_enabled = false;
|
||||
|
||||
assert_eq!(
|
||||
with_app_enabled_state(vec![slack.clone(), app("connector_drive")], &config),
|
||||
vec![slack, drive]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn tool_suggest_connector_ids_include_configured_tool_suggest_discoverables() {
|
||||
let codex_home = tempdir().expect("tempdir should succeed");
|
||||
|
||||
@@ -71,6 +71,7 @@ use codex_analytics::InvocationType;
|
||||
use codex_analytics::TurnResolvedConfigFact;
|
||||
use codex_analytics::build_track_events_context;
|
||||
use codex_async_utils::OrCancelExt;
|
||||
use codex_connectors::AppToolPolicyEvaluator;
|
||||
use codex_core_plugins::RecommendedPluginCandidatesInput;
|
||||
use codex_core_skills::injection::InjectedHostSkillPrompts;
|
||||
use codex_extension_api::ExtensionData;
|
||||
@@ -754,7 +755,8 @@ async fn build_skills_and_plugins(
|
||||
.map(|connector_id| connector_id.0.clone()),
|
||||
connectors::accessible_connectors_from_mcp_tools(mcp_tools),
|
||||
);
|
||||
connectors::with_app_enabled_state(connectors, &turn_context.config)
|
||||
AppToolPolicyEvaluator::new(&turn_context.config.config_layer_stack)
|
||||
.apply_app_enabled_state(connectors)
|
||||
} else {
|
||||
Vec::new()
|
||||
};
|
||||
@@ -1476,21 +1478,6 @@ pub(crate) async fn built_tools(
|
||||
let apps_enabled = turn_context.apps_enabled();
|
||||
let accessible_connectors =
|
||||
apps_enabled.then(|| connectors::accessible_connectors_from_mcp_tools(all_mcp_tools));
|
||||
let connectors = if apps_enabled {
|
||||
let connectors = codex_connectors::merge::merge_plugin_connectors_with_accessible(
|
||||
connector_snapshot
|
||||
.connector_ids()
|
||||
.iter()
|
||||
.map(|connector_id| connector_id.0.clone()),
|
||||
accessible_connectors.clone().unwrap_or_default(),
|
||||
);
|
||||
Some(connectors::with_app_enabled_state(
|
||||
connectors,
|
||||
&turn_context.config,
|
||||
))
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let tool_suggest_is_enabled = tool_suggest_enabled(turn_context);
|
||||
let PreparedToolRecommendations {
|
||||
auth,
|
||||
@@ -1510,7 +1497,7 @@ pub(crate) async fn built_tools(
|
||||
.collect::<Vec<_>>();
|
||||
async {
|
||||
if apps_enabled && tool_suggest_is_enabled {
|
||||
if let Some(accessible_connectors) = connectors.as_ref() {
|
||||
if let Some(accessible_connectors) = accessible_connectors.as_ref() {
|
||||
match connectors::list_tool_suggest_discoverable_tools_with_auth(
|
||||
&turn_context.config,
|
||||
sess.services.plugins_manager.as_ref(),
|
||||
|
||||
@@ -19,6 +19,7 @@ use crate::context::world_state::PluginsInstructionsState;
|
||||
use crate::context::world_state::RealtimeState;
|
||||
use crate::context::world_state::ToolsState;
|
||||
use crate::context::world_state::WorldState;
|
||||
use codex_connectors::AppToolPolicyEvaluator;
|
||||
use codex_extension_api::WorldStateContributionInput;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::error::CodexErr;
|
||||
@@ -178,12 +179,12 @@ impl Session {
|
||||
));
|
||||
let apps_available =
|
||||
if turn_context.config.include_apps_instructions && turn_context.apps_enabled() {
|
||||
connectors::with_app_enabled_state(
|
||||
connectors::accessible_connectors_from_mcp_tools(step_context.mcp.tools()),
|
||||
&turn_context.config,
|
||||
)
|
||||
.into_iter()
|
||||
.any(|connector| connector.is_accessible && connector.is_enabled)
|
||||
AppToolPolicyEvaluator::new(&turn_context.config.config_layer_stack)
|
||||
.apply_app_enabled_state(connectors::accessible_connectors_from_mcp_tools(
|
||||
step_context.mcp.tools(),
|
||||
))
|
||||
.into_iter()
|
||||
.any(|connector| connector.is_accessible && connector.is_enabled)
|
||||
} else {
|
||||
false
|
||||
};
|
||||
|
||||
@@ -456,20 +456,15 @@ async fn refresh_missing_requested_connectors(
|
||||
}
|
||||
|
||||
let mcp_tools = mcp.tools();
|
||||
let accessible_connectors = connectors::with_app_enabled_state(
|
||||
connectors::accessible_connectors_from_mcp_tools(mcp_tools),
|
||||
&turn.config,
|
||||
);
|
||||
let accessible_connectors = connectors::accessible_connectors_from_mcp_tools(mcp_tools);
|
||||
if all_requested_connectors_picked_up(expected_connector_ids, &accessible_connectors) {
|
||||
return Some(accessible_connectors);
|
||||
}
|
||||
|
||||
match session.hard_refresh_latest_codex_apps_tools().await {
|
||||
Ok(mcp_tools) => {
|
||||
let accessible_connectors = connectors::with_app_enabled_state(
|
||||
connectors::accessible_connectors_from_mcp_tools(&mcp_tools),
|
||||
&turn.config,
|
||||
);
|
||||
let accessible_connectors =
|
||||
connectors::accessible_connectors_from_mcp_tools(&mcp_tools);
|
||||
connectors::refresh_accessible_connectors_cache_from_mcp_tools(
|
||||
&turn.config,
|
||||
auth,
|
||||
|
||||
Reference in New Issue
Block a user