agent/codex
1
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

[codex] Clarify plugin load and runtime capability stages (#28472)

Browse Source 
## Summary

Plugin loading and auth projection both previously produced
`PluginLoadOutcome`. That made an unfiltered load result look like
runtime-ready capabilities and generated capability summaries before
auth routing had run.

This change keeps loaded plugin records in the cache, applies the
current auth policy in `PluginsManager`, and only then builds
`PluginLoadOutcome` and its summaries. Auth changes still reuse the
cached disk load and re-resolve apps and MCP servers without reloading
plugins.

The updated tests cover cached auth changes and verify that capability
summaries match the effective app/MCP surface.

## Testing

- `just test -p codex-core-plugins`
- `just test -p codex-plugin`
- `just fix -p codex-core-plugins`
This commit is contained in:
xl-openai
2026-06-16 04:57:21 -07:00
committed by GitHub
Unverified
parent ef8eb8bdd9
commit de1f77bfdd
5 changed files with 127 additions and 97 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core-plugins/src/loader.rs
+18 -14
View File
@@ -33,7 +33,6 @@ use codex_plugin::PluginCapabilitySummary;
use codex_plugin::PluginHookSource;
use codex_plugin::PluginId;
use codex_plugin::PluginIdError;
use codex_plugin::PluginLoadOutcome;
use codex_plugin::PluginTelemetryMetadata;
use codex_plugin::app_connector_ids_from_declarations;
use codex_protocol::protocol::Product;
@@ -81,12 +80,8 @@ enum NonCuratedCacheRefreshMode {
ForceReinstall,
}
pub fn log_plugin_load_errors(outcome: &PluginLoadOutcome<McpServerConfig>) {
for plugin in outcome
.plugins()
.iter()
.filter(|plugin| plugin.error.is_some())
{
pub(crate) fn log_plugin_load_errors(plugins: &[LoadedPlugin<McpServerConfig>]) {
for plugin in plugins.iter().filter(|plugin| plugin.error.is_some()) {
if let Some(error) = plugin.error.as_deref() {
warn!(
plugin = plugin.config_name,
@@ -110,14 +105,15 @@ struct PluginAppConfig {
category: Option<String>,
}
/// Load configured plugins without applying auth-dependent runtime policies.
#[instrument(level = "trace", skip_all)]
pub async fn load_plugins_from_layer_stack(
pub(crate) async fn load_plugins_from_layer_stack(
config_layer_stack: &ConfigLayerStack,
extra_plugins: HashMap<String, PluginConfig>,
store: &PluginStore,
restriction_product: Option<Product>,
prefer_remote_curated_conflicts: bool,
) -> PluginLoadOutcome<McpServerConfig> {
) -> Vec<LoadedPlugin<McpServerConfig>> {
let skill_config_rules = skill_config_rules_from_stack(config_layer_stack);
load_plugins_from_layer_stack_with_scope(
config_layer_stack,
@@ -138,7 +134,7 @@ async fn load_plugins_from_layer_stack_with_scope(
store: &PluginStore,
prefer_remote_curated_conflicts: bool,
scope: PluginLoadScope<'_>,
) -> PluginLoadOutcome<McpServerConfig> {
) -> Vec<LoadedPlugin<McpServerConfig>> {
let configured_plugins = merge_configured_plugins_with_remote_installed(
configured_plugins_from_stack(config_layer_stack),
extra_plugins,
@@ -167,7 +163,7 @@ async fn load_plugins_from_layer_stack_with_scope(
plugins.push(loaded_plugin);
}
PluginLoadOutcome::from_plugins(plugins)
plugins
}
/// Load hooks from enabled plugins without loading their skills, MCP servers, or apps.
@@ -177,7 +173,7 @@ pub async fn load_plugin_hooks_from_layer_stack(
store: &PluginStore,
prefer_remote_curated_conflicts: bool,
) -> PluginHookLoadOutcome {
let outcome = load_plugins_from_layer_stack_with_scope(
let plugins = load_plugins_from_layer_stack_with_scope(
config_layer_stack,
extra_plugins,
store,
@@ -186,8 +182,16 @@ pub async fn load_plugin_hooks_from_layer_stack(
)
.await;
PluginHookLoadOutcome {
hook_sources: outcome.effective_plugin_hook_sources(),
hook_load_warnings: outcome.effective_plugin_hook_warnings(),
hook_sources: plugins
.iter()
.filter(|plugin| plugin.is_active())
.flat_map(|plugin| plugin.hook_sources.iter().cloned())
.collect(),
hook_load_warnings: plugins
.iter()
.filter(|plugin| plugin.is_active())
.flat_map(|plugin| plugin.hook_load_warnings.iter().cloned())
.collect(),
}
}
codex-rs/core-plugins/src/loader_tests.rs
+4 -13
View File
@@ -173,9 +173,8 @@ enabled = true
)
.await;
let validation_state = |outcome: &PluginLoadOutcome<McpServerConfig>| {
outcome
.plugins()
let validation_state = |plugins: &[LoadedPlugin<McpServerConfig>]| {
plugins
.iter()
.map(|plugin| {
(
@@ -183,22 +182,15 @@ enabled = true
plugin.enabled,
plugin.root.clone(),
plugin.error.clone(),
plugin.hook_sources.clone(),
plugin.hook_load_warnings.clone(),
)
})
.collect::<Vec<_>>()
};
assert_eq!(validation_state(&hooks_only), validation_state(&full));
assert_eq!(
hooks_only.effective_plugin_hook_sources(),
full.effective_plugin_hook_sources()
);
assert_eq!(
hooks_only.effective_plugin_hook_warnings(),
full.effective_plugin_hook_warnings()
);
let full_valid = full
.plugins()
.iter()
.find(|plugin| plugin.config_name == "valid@test")
.expect("full load should include valid plugin");
@@ -208,7 +200,6 @@ enabled = true
assert!(!full_valid.apps.is_empty());
let hooks_only_valid = hooks_only
.plugins()
.iter()
.find(|plugin| plugin.config_name == "valid@test")
.expect("hooks-only load should include valid plugin");
codex-rs/core-plugins/src/manager.rs
+52 -57
View File
@@ -1,3 +1,4 @@
use super::LoadedPlugin;
use super::PluginLoadOutcome;
use crate::app_mcp_routing::apply_app_mcp_routing_policy;
use crate::installed_marketplaces::installed_marketplace_roots_from_layer_stack;
@@ -208,23 +209,6 @@ fn featured_plugin_ids_cache_key(
}
}
fn project_plugin_load_outcome_for_auth(
outcome: PluginLoadOutcome,
auth_mode: Option<AuthMode>,
) -> PluginLoadOutcome {
let mut plugins = outcome.plugins().to_vec();
for plugin in &mut plugins {
let plugin_active = plugin.is_active();
apply_app_mcp_routing_policy(
&mut plugin.apps,
&mut plugin.mcp_servers,
auth_mode,
plugin_active,
);
}
PluginLoadOutcome::from_plugins(plugins)
}
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct PluginInstallRequest {
pub plugin_name: String,
@@ -336,8 +320,9 @@ pub struct PluginsManager {
featured_plugin_ids_cache: RwLock<Option<CachedFeaturedPluginIds>>,
configured_marketplace_upgrade_state: RwLock<ConfiguredMarketplaceUpgradeState>,
non_curated_cache_refresh_state: RwLock<NonCuratedCacheRefreshState>,
enabled_outcome_cache: RwLock<EnabledOutcomeCache>,
enabled_outcome_load_semaphore: Semaphore,
// Keep the cache auth-independent so auth changes only need to resolve capabilities again.
loaded_plugins_cache: RwLock<LoadedPluginsCache>,
loaded_plugins_load_semaphore: Semaphore,
remote_installed_plugins_cache: RwLock<Option<Vec<RemoteInstalledPlugin>>>,
remote_installed_plugins_cache_refresh_state: RwLock<RemoteInstalledPluginsCacheRefreshState>,
global_remote_catalog_cache_refresh_state: RwLock<GlobalRemoteCatalogCacheRefreshState>,
@@ -347,15 +332,15 @@ pub struct PluginsManager {
}
#[derive(Clone)]
struct CachedPluginLoadOutcome {
struct LoadedPluginsCacheEntry {
key: PluginLoadCacheKey,
outcome: PluginLoadOutcome,
plugins: Vec<LoadedPlugin>,
}
#[derive(Default)]
struct EnabledOutcomeCache {
struct LoadedPluginsCache {
generation: u64,
outcome: Option<CachedPluginLoadOutcome>,
entry: Option<LoadedPluginsCacheEntry>,
}
#[derive(Clone, PartialEq, Eq)]
@@ -390,8 +375,8 @@ impl PluginsManager {
ConfiguredMarketplaceUpgradeState::default(),
),
non_curated_cache_refresh_state: RwLock::new(NonCuratedCacheRefreshState::default()),
enabled_outcome_cache: RwLock::new(EnabledOutcomeCache::default()),
enabled_outcome_load_semaphore: Semaphore::new(/*permits*/ 1),
loaded_plugins_cache: RwLock::new(LoadedPluginsCache::default()),
loaded_plugins_load_semaphore: Semaphore::new(/*permits*/ 1),
remote_installed_plugins_cache: RwLock::new(None),
remote_installed_plugins_cache_refresh_state: RwLock::new(
RemoteInstalledPluginsCacheRefreshState::default(),
@@ -466,19 +451,19 @@ impl PluginsManager {
skill_config_rules: skill_config_rules_from_stack(&config.config_layer_stack),
remote_plugin_enabled: config.remote_plugin_enabled,
};
if !force_reload && let Some(outcome) = self.cached_enabled_outcome(&cache_key) {
return self.project_plugins_for_auth(outcome);
if !force_reload && let Some(plugins) = self.cached_loaded_plugins(&cache_key) {
return self.resolve_loaded_plugins_for_auth(plugins);
}
let Ok(_load_permit) = self.enabled_outcome_load_semaphore.acquire().await else {
let Ok(_load_permit) = self.loaded_plugins_load_semaphore.acquire().await else {
warn!("plugin load semaphore closed");
return PluginLoadOutcome::default();
};
if !force_reload && let Some(outcome) = self.cached_enabled_outcome(&cache_key) {
return self.project_plugins_for_auth(outcome);
if !force_reload && let Some(plugins) = self.cached_loaded_plugins(&cache_key) {
return self.resolve_loaded_plugins_for_auth(plugins);
}
let cache_generation = self.enabled_outcome_cache_generation();
let outcome = load_plugins_from_layer_stack(
let cache_generation = self.loaded_plugins_cache_generation();
let plugins = load_plugins_from_layer_stack(
&config.config_layer_stack,
self.remote_installed_plugin_configs(),
&self.store,
@@ -486,17 +471,27 @@ impl PluginsManager {
config.remote_plugin_enabled,
)
.await;
log_plugin_load_errors(&outcome);
self.cache_enabled_outcome_if_current(cache_generation, cache_key, outcome.clone());
self.project_plugins_for_auth(outcome)
log_plugin_load_errors(&plugins);
self.cache_loaded_plugins_if_current(cache_generation, cache_key, plugins.clone());
self.resolve_loaded_plugins_for_auth(plugins)
}
fn project_plugins_for_auth(&self, outcome: PluginLoadOutcome) -> PluginLoadOutcome {
project_plugin_load_outcome_for_auth(outcome, self.auth_mode())
fn resolve_loaded_plugins_for_auth(&self, mut plugins: Vec<LoadedPlugin>) -> PluginLoadOutcome {
let auth_mode = self.auth_mode();
for plugin in &mut plugins {
let plugin_active = plugin.is_active();
apply_app_mcp_routing_policy(
&mut plugin.apps,
&mut plugin.mcp_servers,
auth_mode,
plugin_active,
);
}
PluginLoadOutcome::from_plugins(plugins)
}
pub fn clear_cache(&self) {
self.clear_enabled_outcome_cache();
self.clear_loaded_plugins_cache();
let mut featured_plugin_ids_cache = match self.featured_plugin_ids_cache.write() {
Ok(cache) => cache,
Err(err) => err.into_inner(),
@@ -504,13 +499,13 @@ impl PluginsManager {
*featured_plugin_ids_cache = None;
}
fn clear_enabled_outcome_cache(&self) {
let mut cache = match self.enabled_outcome_cache.write() {
fn clear_loaded_plugins_cache(&self) {
let mut cache = match self.loaded_plugins_cache.write() {
Ok(cache) => cache,
Err(err) => err.into_inner(),
};
cache.generation = cache.generation.wrapping_add(1);
cache.outcome = None;
cache.entry = None;
}
/// Load plugins for a config layer stack without touching the plugins cache.
@@ -522,7 +517,7 @@ impl PluginsManager {
if !config.plugins_enabled {
return PluginLoadOutcome::default();
}
let outcome = load_plugins_from_layer_stack(
let plugins = load_plugins_from_layer_stack(
config_layer_stack,
self.remote_installed_plugin_configs(),
&self.store,
@@ -530,7 +525,7 @@ impl PluginsManager {
config.remote_plugin_enabled,
)
.await;
self.project_plugins_for_auth(outcome)
self.resolve_loaded_plugins_for_auth(plugins)
}
/// Resolve plugin hooks for a config layer stack without loading other plugin capabilities.
@@ -562,41 +557,41 @@ impl PluginsManager {
.effective_plugin_skill_roots()
}
fn cached_enabled_outcome(&self, key: &PluginLoadCacheKey) -> Option<PluginLoadOutcome> {
match self.enabled_outcome_cache.read() {
fn cached_loaded_plugins(&self, key: &PluginLoadCacheKey) -> Option<Vec<LoadedPlugin>> {
match self.loaded_plugins_cache.read() {
Ok(cache) => cache
.outcome
.entry
.as_ref()
.filter(|cached| cached.key == *key)
.map(|cached| cached.outcome.clone()),
.map(|cached| cached.plugins.clone()),
Err(err) => err
.into_inner()
.outcome
.entry
.as_ref()
.filter(|cached| cached.key == *key)
.map(|cached| cached.outcome.clone()),
.map(|cached| cached.plugins.clone()),
}
}
fn enabled_outcome_cache_generation(&self) -> u64 {
match self.enabled_outcome_cache.read() {
fn loaded_plugins_cache_generation(&self) -> u64 {
match self.loaded_plugins_cache.read() {
Ok(cache) => cache.generation,
Err(err) => err.into_inner().generation,
}
}
fn cache_enabled_outcome_if_current(
fn cache_loaded_plugins_if_current(
&self,
generation: u64,
key: PluginLoadCacheKey,
outcome: PluginLoadOutcome,
plugins: Vec<LoadedPlugin>,
) {
let mut cache = match self.enabled_outcome_cache.write() {
let mut cache = match self.loaded_plugins_cache.write() {
Ok(cache) => cache,
Err(err) => err.into_inner(),
};
if cache.generation == generation {
cache.outcome = Some(CachedPluginLoadOutcome { key, outcome });
cache.entry = Some(LoadedPluginsCacheEntry { key, plugins });
}
}
@@ -687,7 +682,7 @@ impl PluginsManager {
}
*cache = Some(plugins);
drop(cache);
self.clear_enabled_outcome_cache();
self.clear_loaded_plugins_cache();
true
}
@@ -701,7 +696,7 @@ impl PluginsManager {
}
*cache = None;
drop(cache);
self.clear_enabled_outcome_cache();
self.clear_loaded_plugins_cache();
true
}
codex-rs/core-plugins/src/manager_tests.rs
+50 -12
View File
@@ -359,7 +359,7 @@ enabled = true
}
#[tokio::test]
async fn plugin_auth_projection_reprojects_cached_outcome_when_auth_changes() {
async fn plugin_auth_projection_reprojects_cached_plugins_when_auth_changes() {
let codex_home = TempDir::new().unwrap();
write_auth_projection_plugin(codex_home.path(), "sample", /*include_app*/ true);
write_auth_projection_plugin(codex_home.path(), "docs", /*include_app*/ false);
@@ -379,6 +379,27 @@ async fn plugin_auth_projection_reprojects_cached_outcome_when_auth_changes() {
chatgpt_outcome.effective_apps(),
vec![AppConnectorId("connector_sample".to_string())]
);
assert_eq!(
chatgpt_outcome.capability_summaries(),
&[
PluginCapabilitySummary {
config_name: "docs@test".to_string(),
display_name: "docs".to_string(),
description: None,
has_skills: false,
mcp_server_names: vec!["docs".to_string()],
app_connector_ids: Vec::new(),
},
PluginCapabilitySummary {
config_name: "sample@test".to_string(),
display_name: "sample".to_string(),
description: None,
has_skills: false,
mcp_server_names: Vec::new(),
app_connector_ids: vec![AppConnectorId("connector_sample".to_string())],
},
]
);
assert!(manager.set_auth_mode(Some(AuthMode::ApiKey)));
let api_key_outcome = manager.plugins_for_config(&config).await;
@@ -388,6 +409,27 @@ async fn plugin_auth_projection_reprojects_cached_outcome_when_auth_changes() {
vec!["docs".to_string(), "sample".to_string()]
);
assert!(api_key_outcome.effective_apps().is_empty());
assert_eq!(
api_key_outcome.capability_summaries(),
&[
PluginCapabilitySummary {
config_name: "docs@test".to_string(),
display_name: "docs".to_string(),
description: None,
has_skills: false,
mcp_server_names: vec!["docs".to_string()],
app_connector_ids: Vec::new(),
},
PluginCapabilitySummary {
config_name: "sample@test".to_string(),
display_name: "sample".to_string(),
description: None,
has_skills: false,
mcp_server_names: vec!["sample".to_string()],
app_connector_ids: Vec::new(),
},
]
);
}
fn write_plugin_with_version(
@@ -1781,7 +1823,7 @@ async fn plugin_cache_ignores_unrelated_session_overrides() {
}
#[test]
fn plugin_cache_invalidation_rejects_stale_load_completion() {
fn loaded_plugins_cache_invalidation_rejects_stale_load_completion() {
let codex_home = TempDir::new().unwrap();
let manager = PluginsManager::new(codex_home.path().to_path_buf());
let cache_key = PluginLoadCacheKey {
@@ -1789,16 +1831,12 @@ fn plugin_cache_invalidation_rejects_stale_load_completion() {
skill_config_rules: SkillConfigRules::default(),
remote_plugin_enabled: false,
};
let stale_generation = manager.enabled_outcome_cache_generation();
let stale_generation = manager.loaded_plugins_cache_generation();
manager.clear_enabled_outcome_cache();
manager.cache_enabled_outcome_if_current(
stale_generation,
cache_key.clone(),
PluginLoadOutcome::default(),
);
manager.clear_loaded_plugins_cache();
manager.cache_loaded_plugins_if_current(stale_generation, cache_key.clone(), Vec::new());
assert_eq!(manager.cached_enabled_outcome(&cache_key), None);
assert_eq!(manager.cached_loaded_plugins(&cache_key), None);
}
#[tokio::test]
@@ -4265,7 +4303,7 @@ async fn load_plugins_ignores_project_config_files() {
)
.expect("config layer stack should build");
let outcome = load_plugins_from_layer_stack(
let plugins = load_plugins_from_layer_stack(
&stack,
std::collections::HashMap::new(),
&PluginStore::new(codex_home.path().to_path_buf()),
@@ -4274,7 +4312,7 @@ async fn load_plugins_ignores_project_config_files() {
)
.await;
assert_eq!(outcome, PluginLoadOutcome::default());
assert_eq!(plugins, Vec::new());
}
#[tokio::test]
codex-rs/plugin/src/load_outcome.rs
+3 -1
View File
@@ -83,7 +83,9 @@ pub fn prompt_safe_plugin_description(description: Option<&str>) -> Option<Strin
)
}
/// Outcome of loading configured plugins (skills roots, MCP, apps, errors).
/// Runtime view of loaded plugins and their derived capability summaries.
///
/// Callers must apply any runtime capability policies before constructing this outcome.
#[derive(Debug, Clone, PartialEq)]
pub struct PluginLoadOutcome<M> {
plugins: Vec<LoadedPlugin<M>>,