diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index bc08470444..6a4c3c8fb4 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -297,6 +297,16 @@ Example with notification opt-out: ### Plugin configuration scope +Plugin activation and MCP settings use the existing merged configuration, including +system settings and trusted project overrides. `skills/list` resolves plugin skills +independently for each requested working directory. + +Omitted or empty catalog `cwds` exclude project configuration, including the +app-server process's project. + +Marketplace definitions can come from system configuration, but configured Git +marketplaces currently require an existing downloaded snapshot. + `marketplace/remove` rejects removal when the marketplace name is defined in another enabled layer of the operation's loaded config stack. Otherwise it removes the snapshot and any base-user entry; a base-user entry is not required for cleanup. diff --git a/codex-rs/app-server/src/config_manager.rs b/codex-rs/app-server/src/config_manager.rs index acc2972f8d..f62118d292 100644 --- a/codex-rs/app-server/src/config_manager.rs +++ b/codex-rs/app-server/src/config_manager.rs @@ -144,6 +144,14 @@ impl ConfigManager { .await } + /// Loads system, user, and runtime settings without discovering a project + /// from the app-server process's working directory. + pub(crate) async fn load_non_project_config(&self) -> std::io::Result { + let mut manager = self.clone(); + manager.loader_overrides.ignore_project_config = true; + manager.load_latest_config(/*fallback_cwd*/ None).await + } + pub(crate) async fn load_latest_config_for_thread( &self, thread_config: &Config, diff --git a/codex-rs/app-server/src/request_processors/catalog_processor.rs b/codex-rs/app-server/src/request_processors/catalog_processor.rs index 42f36a76dc..9c7bd4e806 100644 --- a/codex-rs/app-server/src/request_processors/catalog_processor.rs +++ b/codex-rs/app-server/src/request_processors/catalog_processor.rs @@ -479,19 +479,12 @@ impl CatalogRequestProcessor { cwds }; - let config = self.load_latest_config(/*fallback_cwd*/ None).await?; let skills_service = self.thread_manager.skills_service(); let plugins_manager = self.thread_manager.plugins_manager(); - if force_reload && config.features.enabled(Feature::Plugins) { + if force_reload { plugins_manager.clear_cache(); skills_service.clear_cache(); } - // Plugin configuration is user-scoped; workspace skill rules are applied below. - let plugins_input = config.plugins_config_input(); - let plugins = plugins_manager.plugins_for_config(&plugins_input).await; - let effective_skill_roots = plugins.effective_plugin_skill_roots(); - let plugin_skill_snapshots = - plugins_manager.plugin_skill_snapshots_for_config(&plugins_input); let fs = self .thread_manager .environment_manager() @@ -502,12 +495,11 @@ impl CatalogRequestProcessor { .map(|(index, cwd)| { let fs = fs.clone(); let skills_request = &skills_request; - let effective_skill_roots = effective_skill_roots.clone(); - let plugin_skill_snapshots = plugin_skill_snapshots.clone(); + let plugins_manager = Arc::clone(&plugins_manager); async move { - let (cwd_abs, config_layer_stack) = match self.resolve_cwd_config(&cwd).await { + let config = match self.load_latest_config(Some(cwd.clone())).await { Ok(resolved) => resolved, - Err(message) => { + Err(error) => { let error_path = cwd.clone(); return ( index, @@ -516,16 +508,20 @@ impl CatalogRequestProcessor { skills: Vec::new(), errors: vec![codex_app_server_protocol::SkillErrorInfo { path: error_path, - message, + message: error.message, }], }, ); } }; + let plugins_input = config.plugins_config_input(); + let plugins = plugins_manager.plugins_for_config(&plugins_input).await; + let plugin_skill_snapshots = + plugins_manager.plugin_skill_snapshots_for_config(&plugins_input); let skills_input = codex_skills_extension::HostSkillsLoadInput::new( - cwd_abs.clone(), - effective_skill_roots, - config_layer_stack, + config.cwd.clone(), + plugins.effective_plugin_skill_roots(), + config.config_layer_stack, ) .with_plugin_skill_snapshots(plugin_skill_snapshots); let snapshot = skills_request diff --git a/codex-rs/app-server/src/request_processors/plugins.rs b/codex-rs/app-server/src/request_processors/plugins.rs index b8041f0023..7e285308b7 100644 --- a/codex-rs/app-server/src/request_processors/plugins.rs +++ b/codex-rs/app-server/src/request_processors/plugins.rs @@ -38,6 +38,7 @@ use codex_rmcp_client::OAuthDiscoveryTimeout; use codex_rmcp_client::StreamableHttpRedirectMode; use codex_rmcp_client::perform_oauth_login_silent; +mod local; mod search; fn plugin_redirect_mode(plugin_root: &Path) -> StreamableHttpRedirectMode { @@ -557,7 +558,7 @@ impl PluginRequestProcessor { let include_local = marketplace_kinds.contains(&PluginListMarketplaceKind::Local); let include_vertical = marketplace_kinds.contains(&PluginListMarketplaceKind::Vertical); - let config = self.load_latest_config(/*fallback_cwd*/ None).await?; + let config = self.load_catalog_config(&roots).await?; let empty_response = || PluginListResponse { marketplaces: Vec::new(), marketplace_load_errors: Vec::new(), @@ -815,7 +816,7 @@ impl PluginRequestProcessor { marketplaces: Vec::new(), marketplace_load_errors: Vec::new(), }; - let config = self.load_latest_config(/*fallback_cwd*/ None).await?; + let config = self.load_catalog_config(&roots).await?; if !config.features.enabled(Feature::Plugins) { return Ok(empty_response()); } diff --git a/codex-rs/app-server/src/request_processors/plugins/local.rs b/codex-rs/app-server/src/request_processors/plugins/local.rs new file mode 100644 index 0000000000..0de209e960 --- /dev/null +++ b/codex-rs/app-server/src/request_processors/plugins/local.rs @@ -0,0 +1,17 @@ +use super::*; + +impl PluginRequestProcessor { + pub(super) async fn load_catalog_config( + &self, + cwds: &[AbsolutePathBuf], + ) -> Result { + if cwds.is_empty() { + self.config_manager + .load_non_project_config() + .await + .map_err(|err| internal_error(format!("failed to reload config: {err}"))) + } else { + self.load_latest_config(/*fallback_cwd*/ None).await + } + } +} diff --git a/codex-rs/app-server/src/request_processors/plugins/search.rs b/codex-rs/app-server/src/request_processors/plugins/search.rs index 842d80b377..6cc25e4b8b 100644 --- a/codex-rs/app-server/src/request_processors/plugins/search.rs +++ b/codex-rs/app-server/src/request_processors/plugins/search.rs @@ -42,7 +42,9 @@ impl PluginRequestProcessor { return Ok(empty_response()); } - let config = self.load_latest_config(/*fallback_cwd*/ None).await?; + let config = self + .load_catalog_config(cwds.as_deref().unwrap_or_default()) + .await?; if !config.features.enabled(Feature::Plugins) { return Ok(empty_response()); } diff --git a/codex-rs/app-server/tests/suite/v2/plugin_list.rs b/codex-rs/app-server/tests/suite/v2/plugin_list.rs index 80ec20880d..f025bb88ea 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_list.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_list.rs @@ -834,14 +834,22 @@ async fn plugin_list_uses_alternate_discoverable_manifest_and_keeps_undiscoverab } #[tokio::test] -async fn plugin_list_accepts_omitted_cwds() -> Result<()> { +async fn plugin_list_omitted_cwds_excludes_server_project_config() -> Result<()> { let codex_home = TempDir::new()?; + let project_marketplace = TempDir::new()?; std::fs::create_dir_all(codex_home.path().join(".agents/plugins"))?; - write_plugins_enabled_config(codex_home.path())?; + std::fs::create_dir_all(codex_home.path().join(".git"))?; + std::fs::create_dir_all(codex_home.path().join(".codex"))?; + std::fs::create_dir_all(project_marketplace.path().join(".agents/plugins"))?; + write_installed_plugin(&codex_home, "home-marketplace", "home-plugin")?; + std::fs::write( + codex_home.path().join("config.toml"), + "[features]\nplugins = true\n[plugins.\"home-plugin@home-marketplace\"]\nenabled = true\n", + )?; std::fs::write( codex_home.path().join(".agents/plugins/marketplace.json"), r#"{ - "name": "codex-curated", + "name": "home-marketplace", "plugins": [ { "name": "home-plugin", @@ -853,6 +861,20 @@ async fn plugin_list_accepts_omitted_cwds() -> Result<()> { ] }"#, )?; + std::fs::write( + project_marketplace + .path() + .join(".agents/plugins/marketplace.json"), + r#"{"name":"project-marketplace","plugins":[{"name":"project-plugin","source":{"source":"local","path":"./project-plugin"}}]}"#, + )?; + let source = serde_json::to_string(&project_marketplace.path().to_string_lossy())?; + std::fs::write( + codex_home.path().join(".codex/config.toml"), + format!( + "[marketplaces.project-marketplace]\nsource_type = \"local\"\nsource = {source}\n\n[plugins.\"home-plugin@home-marketplace\"]\nenabled = false\n" + ), + )?; + set_project_trust_level(codex_home.path(), codex_home.path(), TrustLevel::Trusted)?; let home = codex_home.path().to_string_lossy().into_owned(); let mut mcp = TestAppServer::builder() .with_codex_home(codex_home.path()) @@ -863,15 +885,39 @@ async fn plugin_list_accepts_omitted_cwds() -> Result<()> { .build_initialized_with_timeout(DEFAULT_TIMEOUT) .await?; - let request_id = mcp - .send_plugin_list_request(PluginListParams { - cwds: None, - marketplace_kinds: None, - force_refetch: false, - }) - .await?; - - let _: PluginListResponse = timeout(DEFAULT_TIMEOUT, mcp.read_response(request_id)).await??; + for (cwds, expected) in [ + (None, vec![("home-plugin@home-marketplace", true, true)]), + ( + Some(Vec::new()), + vec![("home-plugin@home-marketplace", true, true)], + ), + ( + Some(vec![AbsolutePathBuf::try_from(codex_home.path())?]), + vec![ + ("home-plugin@home-marketplace", true, false), + ("project-plugin@project-marketplace", false, false), + ], + ), + ] { + let request_id = mcp + .send_plugin_list_request(PluginListParams { + cwds, + marketplace_kinds: Some(vec![PluginListMarketplaceKind::Local]), + force_refetch: false, + }) + .await?; + let response: PluginListResponse = + timeout(DEFAULT_TIMEOUT, mcp.read_response(request_id)).await??; + let mut plugins = response + .marketplaces + .iter() + .flat_map(|marketplace| &marketplace.plugins) + .map(|plugin| (plugin.id.as_str(), plugin.installed, plugin.enabled)) + .collect::>(); + plugins.sort_unstable(); + assert_eq!(plugins, expected); + assert_eq!(response.marketplace_load_errors, Vec::new()); + } Ok(()) } diff --git a/codex-rs/app-server/tests/suite/v2/skills_list.rs b/codex-rs/app-server/tests/suite/v2/skills_list.rs index 0dc72dc0c7..c6880fb0e0 100644 --- a/codex-rs/app-server/tests/suite/v2/skills_list.rs +++ b/codex-rs/app-server/tests/suite/v2/skills_list.rs @@ -39,6 +39,7 @@ use core_test_support::skip_if_remote; use core_test_support::skip_if_wine_exec; use pretty_assertions::assert_eq; use tempfile::TempDir; +use test_case::test_case; use tokio::time::timeout; use wiremock::Mock; use wiremock::MockServer; @@ -794,8 +795,10 @@ async fn skills_list_accepts_relative_cwds() -> Result<()> { Ok(()) } +#[test_case(false; "plugins disabled by default")] +#[test_case(true; "plugins enabled by default")] #[tokio::test] -async fn skills_list_preserves_requested_cwd_order() -> Result<()> { +async fn skills_list_preserves_requested_cwd_order(home_plugins_enabled: bool) -> Result<()> { skip_if_wine_exec!( Ok(()), "skills/list currently requires host-native cwd paths for workspace config" @@ -803,25 +806,32 @@ async fn skills_list_preserves_requested_cwd_order() -> Result<()> { let codex_home = TempDir::new()?; let first_cwd = TempDir::new()?; let second_cwd = TempDir::new()?; + let third_cwd = TempDir::new()?; write_skill(&codex_home, "shared-skill")?; write_cached_local_curated_plugin_with_skill(codex_home.path(), "openai-api-curated")?; std::fs::write( codex_home.path().join("config.toml"), - r#"[features] -plugins = true + format!( + r#"[features] +plugins = {home_plugins_enabled} [plugins."google-calendar@openai-api-curated"] -enabled = true -"#, +enabled = false +"# + ), )?; - for (cwd, plugin_enabled) in [(first_cwd.path(), true), (second_cwd.path(), false)] { + for (cwd, plugins_enabled, plugin_enabled) in [ + (first_cwd.path(), true, true), + (second_cwd.path(), false, true), + (third_cwd.path(), true, false), + ] { std::fs::create_dir_all(cwd.join(".git"))?; std::fs::create_dir_all(cwd.join(".codex"))?; std::fs::write( cwd.join(".codex/config.toml"), format!( - "[plugins.\"google-calendar@openai-api-curated\"]\nenabled = {plugin_enabled}\n" + "[features]\nplugins = {plugins_enabled}\n[plugins.\"google-calendar@openai-api-curated\"]\nenabled = {plugin_enabled}\n" ), )?; set_project_trust_level(codex_home.path(), cwd, TrustLevel::Trusted)?; @@ -836,6 +846,7 @@ enabled = true for (cwd, name) in [ (first_cwd.path(), "first-project-skill"), (second_cwd.path(), "second-project-skill"), + (third_cwd.path(), "third-project-skill"), ] { let cwd = AbsolutePathBuf::try_from(cwd)?; let git_dir = PathUri::from_abs_path(&cwd.join(".git")); @@ -862,29 +873,30 @@ enabled = true .await?; } - for (request_index, force_reload) in [false, false, true].into_iter().enumerate() { - if request_index == 1 { - write_skill(&codex_home, "new-skill")?; - } - + for force_reload in [false, false, true] { let request_id = mcp .send_skills_list_request(SkillsListParams { cwds: vec![ first_cwd.path().to_path_buf(), second_cwd.path().to_path_buf(), + third_cwd.path().to_path_buf(), ], force_reload, }) .await?; let SkillsListResponse { data } = timeout(DEFAULT_TIMEOUT, mcp.read_response(request_id)).await??; - assert_eq!(data.len(), 2); - assert_eq!(data[0].cwd, first_cwd.path()); - assert_eq!(data[1].cwd, second_cwd.path()); - for (entry, project_skill) in data - .iter() - .zip(["first-project-skill", "second-project-skill"]) - { + assert_eq!( + data.iter() + .map(|entry| entry.cwd.as_path()) + .collect::>(), + vec![first_cwd.path(), second_cwd.path(), third_cwd.path()], + ); + for (entry, (project_skill, plugin_enabled)) in data.iter().zip([ + ("first-project-skill", true), + ("second-project-skill", false), + ("third-project-skill", false), + ]) { assert_eq!(entry.errors, Vec::new()); assert!( entry @@ -892,6 +904,10 @@ enabled = true .iter() .any(|skill| skill.name == "shared-skill") ); + let mut expected_skills = vec![project_skill]; + if plugin_enabled { + expected_skills.push("google-calendar:meeting-prep"); + } assert_eq!( entry .skills @@ -902,11 +918,7 @@ enabled = true }) .map(|skill| skill.name.as_str()) .collect::>(), - vec![project_skill, "google-calendar:meeting-prep"] - ); - assert_eq!( - entry.skills.iter().any(|skill| skill.name == "new-skill"), - force_reload + expected_skills ); } } diff --git a/codex-rs/cli/src/marketplace_cmd.rs b/codex-rs/cli/src/marketplace_cmd.rs index 214dc8eb3a..82bb1ffb4d 100644 --- a/codex-rs/cli/src/marketplace_cmd.rs +++ b/codex-rs/cli/src/marketplace_cmd.rs @@ -335,10 +335,8 @@ fn configured_marketplace_sources_by_root( plugins_input: &PluginsConfigInput, ) -> HashMap { let marketplace_sources = configured_marketplace_sources(plugins_input, codex_home); - let Some(user_config) = plugins_input.config_layer_stack.effective_user_config() else { - return HashMap::new(); - }; - let Some(marketplaces) = user_config + let effective_config = plugins_input.config_layer_stack.effective_config(); + let Some(marketplaces) = effective_config .get("marketplaces") .and_then(toml::Value::as_table) else { diff --git a/codex-rs/cli/src/plugin_cmd.rs b/codex-rs/cli/src/plugin_cmd.rs index 92bb78c5d4..73f8a1db8d 100644 --- a/codex-rs/cli/src/plugin_cmd.rs +++ b/codex-rs/cli/src/plugin_cmd.rs @@ -483,10 +483,8 @@ pub(crate) fn configured_marketplace_sources( plugins_input: &PluginsConfigInput, codex_home: &Path, ) -> HashMap { - let Some(user_config) = plugins_input.config_layer_stack.effective_user_config() else { - return HashMap::new(); - }; - let Some(marketplaces) = user_config + let effective_config = plugins_input.config_layer_stack.effective_config(); + let Some(marketplaces) = effective_config .get("marketplaces") .and_then(toml::Value::as_table) else { @@ -726,10 +724,8 @@ pub(crate) fn configured_marketplace_snapshot_issues( load_errors: &[MarketplaceListError], marketplace_name: Option<&str>, ) -> Vec { - let Some(user_config) = plugins_input.config_layer_stack.effective_user_config() else { - return Vec::new(); - }; - let Some(configured_marketplaces) = user_config + let effective_config = plugins_input.config_layer_stack.effective_config(); + let Some(configured_marketplaces) = effective_config .get("marketplaces") .and_then(toml::Value::as_table) else { diff --git a/codex-rs/core-plugins/src/installed_marketplaces.rs b/codex-rs/core-plugins/src/installed_marketplaces.rs index b5bf3b7699..f84290bf43 100644 --- a/codex-rs/core-plugins/src/installed_marketplaces.rs +++ b/codex-rs/core-plugins/src/installed_marketplaces.rs @@ -7,7 +7,7 @@ use codex_utils_absolute_path::AbsolutePathBuf; use tracing::warn; use crate::marketplace::find_marketplace_manifest_path; -use crate::marketplace_policy::project_effective_user_config; +use crate::marketplace_policy::policy_filtered_plugin_config; pub const INSTALLED_MARKETPLACES_DIR: &str = ".tmp/marketplaces"; @@ -19,10 +19,11 @@ pub fn installed_marketplace_roots_from_layer_stack( config_layer_stack: &ConfigLayerStack, codex_home: &Path, ) -> Vec { - let Some(user_config) = project_effective_user_config(config_layer_stack, codex_home) else { + let Some(effective_config) = policy_filtered_plugin_config(config_layer_stack, codex_home) + else { return Vec::new(); }; - let Some(marketplaces_value) = user_config.get("marketplaces") else { + let Some(marketplaces_value) = effective_config.get("marketplaces") else { return Vec::new(); }; let Some(marketplaces) = marketplaces_value.as_table() else { diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index dce8f82ef6..6df014b8db 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -723,7 +723,7 @@ fn is_full_git_sha(value: &str) -> bool { value.len() == 40 && value.chars().all(|ch| ch.is_ascii_hexdigit()) } -fn configured_plugins_from_user_config_value( +fn configured_plugins_from_config_value( user_config: &toml::Value, ) -> HashMap { let Some(plugins_value) = user_config.get("plugins") else { @@ -769,7 +769,7 @@ fn configured_plugins_from_codex_home( } }; - configured_plugins_from_user_config_value(&user_config) + configured_plugins_from_config_value(&user_config) } fn configured_plugin_ids( @@ -1391,7 +1391,7 @@ pub async fn load_plugin_mcp_servers( load_plugin_mcp_servers_with_policy(plugin_root, auth_mode, /*plugin_policy*/ None).await } -/// Loads plugin MCP servers with the effective user policy for an installed plugin. +/// Loads plugin MCP servers with the effective configuration policy for an installed plugin. pub async fn load_configured_plugin_mcp_servers( plugin_root: &Path, auth_mode: Option, @@ -1412,10 +1412,7 @@ pub async fn load_configured_plugin_mcp_servers( pub fn configured_plugin_mcp_server_policies( config_layer_stack: &ConfigLayerStack, ) -> HashMap> { - config_layer_stack - .effective_user_config() - .map(|config| configured_plugins_from_user_config_value(&config)) - .unwrap_or_default() + configured_plugins_from_config_value(&config_layer_stack.effective_config()) .into_iter() .map(|(plugin_id, plugin)| (plugin_id, plugin.mcp_servers)) .collect() diff --git a/codex-rs/core-plugins/src/loader_tests.rs b/codex-rs/core-plugins/src/loader_tests.rs index 7711261ac6..11a80bd927 100644 --- a/codex-rs/core-plugins/src/loader_tests.rs +++ b/codex-rs/core-plugins/src/loader_tests.rs @@ -314,10 +314,16 @@ async fn installed_agent_plugin_uses_isolated_data_root_for_stdio_mcp() { } #[test] -fn configured_plugins_from_stack_merges_user_layers() { +fn configured_plugins_from_stack_merges_enabled_effective_layers() { let temp_dir = TempDir::new().expect("tempdir"); let stack = ConfigLayerStack::new( vec![ + ConfigLayerEntry::new( + ConfigLayerSource::System { + file: user_config_path(&temp_dir, "system.toml"), + }, + toml::from_str("[plugins.system]\nenabled = true\n").expect("system config toml"), + ), user_layer( user_config_path(&temp_dir, "config.toml"), "[plugins.base]\nenabled = true\n", @@ -326,12 +332,36 @@ fn configured_plugins_from_stack_merges_user_layers() { user_config_path(&temp_dir, "work.config.toml"), "[plugins.profile]\nenabled = false\n", ), + ConfigLayerEntry::new( + ConfigLayerSource::Project { + dot_codex_folder: user_config_path(&temp_dir, "project/.codex"), + }, + toml::from_str( + "[plugins.profile]\nenabled = true\n[plugins.profile.mcp_servers.example]\nenabled = false\n", + ) + .expect("project config toml"), + ), + ConfigLayerEntry::new_disabled( + ConfigLayerSource::Project { + dot_codex_folder: user_config_path(&temp_dir, "project/untrusted/.codex"), + }, + toml::from_str("[plugins.untrusted]\nenabled = true\n") + .expect("untrusted project config toml"), + "project is untrusted", + ), ], ConfigRequirements::default(), ConfigRequirementsToml::default(), ) .expect("valid config layer stack"); + let project_mcp_servers = HashMap::from([( + "example".to_string(), + PluginMcpServerConfig { + enabled: false, + ..PluginMcpServerConfig::default() + }, + )]); let plugins = configured_plugins_from_stack(&stack, temp_dir.path()); assert_eq!( @@ -347,12 +377,23 @@ fn configured_plugins_from_stack_merges_user_layers() { ( "profile".to_string(), PluginConfig { - enabled: false, + enabled: true, + mcp_servers: project_mcp_servers.clone(), + }, + ), + ( + "system".to_string(), + PluginConfig { + enabled: true, mcp_servers: HashMap::new(), }, ), ]) ); + assert_eq!( + configured_plugin_mcp_server_policies(&stack).get("profile"), + Some(&project_mcp_servers) + ); } #[tokio::test] diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index 2a7faf2747..69c5e1ddab 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -109,6 +109,7 @@ use std::collections::VecDeque; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; +use std::sync::Mutex; use std::sync::RwLock; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; @@ -125,6 +126,7 @@ static CURATED_REPO_SYNC_STARTED: AtomicBool = AtomicBool::new(false); const FEATURED_PLUGIN_IDS_CACHE_TTL: std::time::Duration = std::time::Duration::from_secs(60 * 60 * 3); const REMOTE_INSTALLED_PLUGIN_SYNC_WAIT_TIMEOUT: Duration = Duration::from_secs(120); +const LOADED_PLUGINS_CACHE_CAPACITY: usize = 8; type EffectivePluginsChangedCallback = Arc; @@ -503,8 +505,8 @@ pub struct PluginsManager { non_curated_cache_refresh_lock: Semaphore, non_curated_cache_refresh_state: RwLock, non_curated_cache_refresh_completion: watch::Sender, - // Remote installed plugins vary by account, so loaded capabilities are cached per auth. - loaded_plugins_cache: RwLock, + // Loaded capabilities vary by effective configuration and, for remote plugins, account. + loaded_plugins_cache: Mutex, loaded_plugins_load_semaphore: Semaphore, skill_root_loader: Arc>, tool_suggest_metadata_cache: ToolSuggestMetadataCache, @@ -528,7 +530,17 @@ struct LoadedPluginsCacheEntry { #[derive(Default)] struct LoadedPluginsCache { generation: u64, - entry: Option, + // Most recently used first. + entries: VecDeque, +} + +impl LoadedPluginsCache { + fn get(&mut self, key: &PluginLoadCacheKey) -> Option<&LoadedPluginsCacheEntry> { + let index = self.entries.iter().position(|entry| &entry.key == key)?; + let entry = self.entries.remove(index)?; + self.entries.push_front(entry); + self.entries.front() + } } #[derive(Clone, PartialEq, Eq)] @@ -611,7 +623,7 @@ impl PluginsManager { NonCuratedCacheRefreshCompletion::default(), ) .0, - loaded_plugins_cache: RwLock::new(LoadedPluginsCache::default()), + loaded_plugins_cache: Mutex::new(LoadedPluginsCache::default()), loaded_plugins_load_semaphore: Semaphore::new(/*permits*/ 1), skill_root_loader, tool_suggest_metadata_cache: ToolSuggestMetadataCache::new(), @@ -696,11 +708,9 @@ impl PluginsManager { RemoteInstalledPluginsAuthIdentity::from_auth(auth.as_ref()), ); self.loaded_plugins_cache - .read() + .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) - .entry - .as_ref() - .filter(|cached| cached.key == key) + .get(&key) .map(|cached| cached.plugin_skill_snapshots.clone()) } @@ -855,12 +865,12 @@ impl PluginsManager { fn clear_loaded_plugins_cache(&self) { self.tool_suggest_metadata_cache.clear(); - let mut cache = match self.loaded_plugins_cache.write() { + let mut cache = match self.loaded_plugins_cache.lock() { Ok(cache) => cache, Err(err) => err.into_inner(), }; cache.generation = cache.generation.wrapping_add(1); - cache.entry = None; + cache.entries.clear(); } fn clear_caches_after_marketplace_source_refresh( @@ -900,16 +910,14 @@ impl PluginsManager { fn cached_loaded_plugins(&self, key: &PluginLoadCacheKey) -> Option> { self.loaded_plugins_cache - .read() + .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) - .entry - .as_ref() - .filter(|cached| cached.key == *key) + .get(key) .map(|cached| cached.plugins.clone()) } fn loaded_plugins_cache_generation(&self) -> u64 { - match self.loaded_plugins_cache.read() { + match self.loaded_plugins_cache.lock() { Ok(cache) => cache.generation, Err(err) => err.into_inner().generation, } @@ -922,16 +930,18 @@ impl PluginsManager { plugins: Vec, plugin_skill_snapshots: SkillRootSnapshots, ) { - let mut cache = match self.loaded_plugins_cache.write() { + let mut cache = match self.loaded_plugins_cache.lock() { Ok(cache) => cache, Err(err) => err.into_inner(), }; if cache.generation == generation { - cache.entry = Some(LoadedPluginsCacheEntry { + cache.entries.retain(|entry| entry.key != key); + cache.entries.push_front(LoadedPluginsCacheEntry { key, plugins, plugin_skill_snapshots, }); + cache.entries.truncate(LOADED_PLUGINS_CACHE_CAPACITY); } } diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 9886cdef83..9ceac10da3 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -2903,7 +2903,7 @@ async fn load_plugins_returns_empty_when_feature_disabled() { } #[tokio::test] -async fn plugin_cache_ignores_unrelated_session_overrides() { +async fn plugin_cache_reuses_effective_configurations() { let codex_home = TempDir::new().unwrap(); let plugin_root = codex_home .path() @@ -2962,16 +2962,44 @@ async fn plugin_cache_ignores_unrelated_session_overrides() { }; let manager = test_plugins_manager(codex_home.path().to_path_buf()); - let first = manager - .plugins_for_config(&config(r#"model = "first""#)) - .await; + let first_config = config(r#"model = "first""#); + let second_config = config( + r#"[plugins."sample@test".mcp_servers.sample] +enabled = false"#, + ); + let first = manager.plugins_for_config(&first_config).await; + let first_snapshots = manager + .plugin_skill_snapshots_for_config(&first_config) + .expect("first configuration snapshots"); + let second = manager.plugins_for_config(&second_config).await; + let second_snapshots = manager + .plugin_skill_snapshots_for_config(&second_config) + .expect("second configuration snapshots"); std::fs::remove_file(plugin_root.join(".mcp.json")).unwrap(); - let second = manager - .plugins_for_config(&config(r#"model = "second""#)) - .await; - assert_eq!(second, first); - assert_eq!(second.plugins()[0].mcp_servers.len(), 1); + assert_eq!( + manager.plugin_skill_snapshots_for_config(&first_config), + Some(first_snapshots), + ); + assert_eq!( + manager + .plugins_for_config(&config(r#"model = "second""#)) + .await, + first, + ); + assert_eq!( + manager.plugin_skill_snapshots_for_config(&second_config), + Some(second_snapshots), + ); + assert_eq!(manager.plugins_for_config(&second_config).await, second); + manager.clear_cache(); + assert_eq!( + [first_config, second_config] + .iter() + .map(|config| manager.plugin_skill_snapshots_for_config(config)) + .collect::>(), + vec![None, None], + ); } #[tokio::test] @@ -3056,6 +3084,57 @@ enabled = true } } +#[test] +fn loaded_plugins_cache_evicts_least_recently_used_configuration() { + let codex_home = TempDir::new().unwrap(); + let manager = test_plugins_manager(codex_home.path().to_path_buf()); + let keys = (0..=LOADED_PLUGINS_CACHE_CAPACITY) + .map(|index| PluginLoadCacheKey { + configured_plugins: HashMap::from([( + format!("plugin-{index}@test"), + PluginConfig { + enabled: true, + mcp_servers: HashMap::new(), + }, + )]), + skill_config_rules: SkillConfigRules::default(), + remote_global_catalog_active: false, + auth_identity: None, + }) + .collect::>(); + let generation = manager.loaded_plugins_cache_generation(); + for key in keys.iter().take(LOADED_PLUGINS_CACHE_CAPACITY) { + manager.cache_loaded_plugins_if_current( + generation, + key.clone(), + Vec::new(), + crate::skill_snapshots::new_plugin_skill_snapshots(), + ); + } + manager.cache_loaded_plugins_if_current( + generation, + keys[LOADED_PLUGINS_CACHE_CAPACITY - 1].clone(), + Vec::new(), + crate::skill_snapshots::new_plugin_skill_snapshots(), + ); + assert_eq!(manager.cached_loaded_plugins(&keys[0]), Some(Vec::new())); + manager.cache_loaded_plugins_if_current( + generation, + keys[LOADED_PLUGINS_CACHE_CAPACITY].clone(), + Vec::new(), + crate::skill_snapshots::new_plugin_skill_snapshots(), + ); + + assert_eq!( + keys.iter() + .map(|key| manager.cached_loaded_plugins(key).is_some()) + .collect::>(), + (0..=LOADED_PLUGINS_CACHE_CAPACITY) + .map(|index| index != 1) + .collect::>(), + ); +} + #[test] fn loaded_plugins_cache_invalidation_rejects_stale_load_completion() { let codex_home = TempDir::new().unwrap(); @@ -6671,7 +6750,7 @@ fn refresh_non_curated_plugin_cache_continues_after_plugin_error() { } #[tokio::test] -async fn load_plugins_ignores_project_config_files() { +async fn load_plugins_uses_project_config_files() { let codex_home = TempDir::new().unwrap(); let project_root = codex_home.path().join("project"); let plugin_root = codex_home @@ -6714,7 +6793,13 @@ async fn load_plugins_ignores_project_config_files() { ) .await; - assert_eq!(plugins, Vec::new()); + assert_eq!( + plugins + .iter() + .map(|plugin| (plugin.config_name.as_str(), plugin.enabled)) + .collect::>(), + vec![("sample@test", true)] + ); } #[tokio::test] diff --git a/codex-rs/core-plugins/src/marketplace_policy.rs b/codex-rs/core-plugins/src/marketplace_policy.rs index 8e8ae238ca..b1ec3eb1a5 100644 --- a/codex-rs/core-plugins/src/marketplace_policy.rs +++ b/codex-rs/core-plugins/src/marketplace_policy.rs @@ -121,12 +121,8 @@ impl MarketplacePolicy { return Ok(()); } - let user_config = config_layer_stack.effective_user_config().ok_or_else(|| { - format!( - "marketplace `{marketplace_name}` must be added to config before plugins can be installed while marketplace source restrictions are enabled" - ) - })?; - let marketplace = user_config + let effective_config = config_layer_stack.effective_config(); + let marketplace = effective_config .get("marketplaces") .and_then(toml::Value::as_table) .and_then(|marketplaces| marketplaces.get(marketplace_name)) @@ -207,28 +203,31 @@ impl AllowedMarketplaceSource { } } -pub(crate) fn project_effective_user_config( +pub(crate) fn policy_filtered_plugin_config( config_layer_stack: &ConfigLayerStack, codex_home: &Path, ) -> Option { - let mut user_config = config_layer_stack.effective_user_config()?; + let mut effective_config = config_layer_stack.effective_config(); + if effective_config.get("plugins").is_none() && effective_config.get("marketplaces").is_none() { + return None; + } let policy = MarketplacePolicy::from_requirements(config_layer_stack.requirements()); let allowed_marketplace_names = - allowed_configured_marketplace_names_with_policy(&user_config, &policy, codex_home); - let configured_marketplace_names = user_config + allowed_configured_marketplace_names_with_policy(&effective_config, &policy, codex_home); + let configured_marketplace_names = effective_config .get("marketplaces") .and_then(toml::Value::as_table) .map(|marketplaces| marketplaces.keys().cloned().collect::>()) .unwrap_or_default(); - if let Some(marketplaces) = user_config + if let Some(marketplaces) = effective_config .get_mut("marketplaces") .and_then(toml::Value::as_table_mut) { marketplaces .retain(|marketplace_name, _| allowed_marketplace_names.contains(marketplace_name)); } - if let Some(plugins) = user_config + if let Some(plugins) = effective_config .get_mut("plugins") .and_then(toml::Value::as_table_mut) { @@ -243,26 +242,24 @@ pub(crate) fn project_effective_user_config( is_openai_curated_marketplace_name(marketplace_name) || !policy.is_restricted() }); } - Some(user_config) + Some(effective_config) } pub fn allowed_configured_marketplace_names( config_layer_stack: &ConfigLayerStack, codex_home: &Path, ) -> HashSet { - let Some(user_config) = config_layer_stack.effective_user_config() else { - return HashSet::new(); - }; + let effective_config = config_layer_stack.effective_config(); let policy = MarketplacePolicy::from_requirements(config_layer_stack.requirements()); - allowed_configured_marketplace_names_with_policy(&user_config, &policy, codex_home) + allowed_configured_marketplace_names_with_policy(&effective_config, &policy, codex_home) } fn allowed_configured_marketplace_names_with_policy( - user_config: &toml::Value, + effective_config: &toml::Value, policy: &MarketplacePolicy, codex_home: &Path, ) -> HashSet { - let Some(marketplaces) = user_config + let Some(marketplaces) = effective_config .get("marketplaces") .and_then(toml::Value::as_table) else { @@ -288,10 +285,11 @@ pub(crate) fn configured_plugins_from_stack( config_layer_stack: &ConfigLayerStack, codex_home: &Path, ) -> HashMap { - let Some(user_config) = project_effective_user_config(config_layer_stack, codex_home) else { + let Some(effective_config) = policy_filtered_plugin_config(config_layer_stack, codex_home) + else { return HashMap::new(); }; - let Some(plugins_value) = user_config.get("plugins") else { + let Some(plugins_value) = effective_config.get("plugins") else { return HashMap::new(); }; match plugins_value.clone().try_into() { diff --git a/codex-rs/core-plugins/src/marketplace_policy_tests.rs b/codex-rs/core-plugins/src/marketplace_policy_tests.rs index 3c3277f390..41fe45d6ed 100644 --- a/codex-rs/core-plugins/src/marketplace_policy_tests.rs +++ b/codex-rs/core-plugins/src/marketplace_policy_tests.rs @@ -220,7 +220,7 @@ restrict_to_allowed_sources = {restricted} } #[test] -fn strict_install_validates_configured_name_source_and_root() { +fn system_marketplace_discovery_and_install_validate_source_and_root() { let codex_home = TempDir::new().expect("create Codex home"); let configured_root = TempDir::new().expect("create configured marketplace"); let other_root = TempDir::new().expect("create other marketplace"); @@ -256,6 +256,18 @@ source = {configured_root:?} config_file, )), ); + let layer = stack.layers_low_to_high().next().expect("configured layer"); + let stack = ConfigLayerStack::new( + vec![ConfigLayerEntry::new( + ConfigLayerSource::System { + file: AbsolutePathBuf::try_from(codex_home.path().join("system.toml")).unwrap(), + }, + layer.config.clone(), + )], + stack.requirements().clone(), + stack.requirements_toml().clone(), + ) + .expect("system config stack"); let policy = MarketplacePolicy::from_requirements(stack.requirements()); let configured_path = AbsolutePathBuf::try_from(configured_root.join(".agents/plugins/marketplace.json")) @@ -263,6 +275,15 @@ source = {configured_root:?} let other_path = AbsolutePathBuf::try_from(other_root.join(".agents/plugins/marketplace.json")) .expect("other marketplace path"); + fs::create_dir_all(configured_path.as_path().parent().unwrap()).unwrap(); + fs::write(&configured_path, r#"{"name":"company","plugins":[]}"#).unwrap(); + assert_eq!( + crate::installed_marketplaces::installed_marketplace_roots_from_layer_stack( + &stack, + codex_home.path() + ), + vec![AbsolutePathBuf::try_from(configured_root).unwrap()], + ); assert_eq!( policy.validate_install(&stack, codex_home.path(), &configured_path, "company"), Ok(()) @@ -508,7 +529,7 @@ enabled = true ); let projected = - project_effective_user_config(&stack, codex_home.path()).expect("project user config"); + policy_filtered_plugin_config(&stack, codex_home.path()).expect("project plugin config"); assert_eq!( projected["marketplaces"] .as_table() @@ -574,7 +595,7 @@ enabled = true ); let projected = - project_effective_user_config(&stack, codex_home.path()).expect("project user config"); + policy_filtered_plugin_config(&stack, codex_home.path()).expect("project plugin config"); assert_eq!( projected["marketplaces"] @@ -640,7 +661,7 @@ enabled = true ); let projected = - project_effective_user_config(&stack, codex_home.path()).expect("project user config"); + policy_filtered_plugin_config(&stack, codex_home.path()).expect("project plugin config"); assert_eq!( projected["marketplaces"] .as_table() diff --git a/codex-rs/core/tests/suite/plugins.rs b/codex-rs/core/tests/suite/plugins.rs index 2e33888a3a..f07245e329 100644 --- a/codex-rs/core/tests/suite/plugins.rs +++ b/codex-rs/core/tests/suite/plugins.rs @@ -5,8 +5,11 @@ use std::time::Duration; use std::time::Instant; use anyhow::Result; +use codex_config::LoaderOverrides; use codex_core::TurnInputRequest; use codex_core::config::Config; +use codex_core::config::ConfigBuilder; +use codex_core::config::set_project_trust_level; use codex_core_plugins::store::PluginStore; use codex_extension_api::ExtensionRegistry; use codex_extension_api::ExtensionRegistryBuilder; @@ -20,6 +23,7 @@ use codex_protocol::auth::AuthMode; use codex_protocol::config_types::CollaborationMode; use codex_protocol::config_types::ModeKind; use codex_protocol::config_types::Settings; +use codex_protocol::config_types::TrustLevel; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; @@ -1244,6 +1248,121 @@ async fn explicit_plugin_mentions_keep_non_conflicting_mcp_for_chatgpt_auth() -> Ok(()) } +#[test_case(TrustLevel::Trusted, true, true, false, &[]; "trusted project disables the plugin")] +#[test_case(TrustLevel::Untrusted, true, true, false, &["echo_tool"]; "untrusted project cannot disable the plugin")] +#[test_case(TrustLevel::Trusted, false, true, true, &["echo"]; "trusted project enables system-disabled server and overrides user tool policy")] +#[test_case(TrustLevel::Untrusted, false, true, true, &[]; "untrusted project cannot enable system-disabled server")] +#[test_case(TrustLevel::Trusted, true, false, true, &[]; "trusted project disables system-enabled server")] +#[test_case(TrustLevel::Untrusted, true, false, true, &["echo_tool"]; "untrusted project preserves system startup and user tool policy")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn system_marketplace_plugin_honors_layered_activation_and_mcp_policy( + trust_level: TrustLevel, + system_enabled: bool, + project_enabled: bool, + plugin_enabled: bool, + expected_tools: &[&str], +) -> Result<()> { + skip_if_no_network!(Ok(())); + let server = start_mock_server().await; + let mock = mount_plugin_tool_search_turn(&server).await; + let codex_home = Arc::new(TempDir::new()?); + let project = TempDir::new()?; + write_plugin_mcp_plugin(codex_home.as_ref(), &stdio_server_bin()?); + let user_config_path = codex_home.path().join("config.toml"); + let user_config = std::fs::read_to_string(&user_config_path)?; + std::fs::write( + &user_config_path, + format!( + "{user_config}\n[plugins.\"{SAMPLE_PLUGIN_CONFIG_NAME}\".mcp_servers.sample]\ndisabled_tools = [\"echo\"]\n" + ), + )?; + let system_config_path = codex_home.path().join("system.toml"); + let marketplace = TempDir::new()?; + std::fs::create_dir_all(marketplace.path().join(".agents/plugins"))?; + std::fs::write( + marketplace.path().join(".agents/plugins/marketplace.json"), + r#"{"name":"test","plugins":[{"name":"sample","source":{"source":"local","path":"./sample"}}]}"#, + )?; + let marketplace_source = toml::Value::String(marketplace.path().to_string_lossy().into_owned()); + std::fs::write( + &system_config_path, + format!( + "[marketplaces.test]\nsource_type = \"local\"\nsource = {marketplace_source}\n[plugins.\"{SAMPLE_PLUGIN_CONFIG_NAME}\".mcp_servers.sample]\nenabled = {system_enabled}\nenabled_tools = [\"echo\", \"echo-tool\"]\n" + ), + )?; + // The cached plugin may activate only through this system-defined marketplace. + // Without the definition, source restrictions exclude it from the real turn. + let requirements_path = codex_home.path().join("requirements.toml"); + std::fs::write( + &requirements_path, + format!( + "[marketplaces]\nrestrict_to_allowed_sources = true\n[marketplaces.allowed_sources.test]\nsource = \"local\"\npath = {marketplace_source}\n" + ), + )?; + std::fs::create_dir_all(project.path().join(".git"))?; + std::fs::create_dir_all(project.path().join(".codex"))?; + std::fs::write( + project.path().join(".codex/config.toml"), + format!( + "[plugins.\"{SAMPLE_PLUGIN_CONFIG_NAME}\"]\nenabled = {plugin_enabled}\n[plugins.\"{SAMPLE_PLUGIN_CONFIG_NAME}\".mcp_servers.sample]\nenabled = {project_enabled}\ndisabled_tools = [\"echo-tool\"]\n" + ), + )?; + set_project_trust_level(codex_home.path(), project.path(), trust_level)?; + // Exercise the real layer loader and trust checks while keeping the test harness's + // mock model provider and automatically selected executor environment. + let layered_config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(project.path().to_path_buf())) + .loader_overrides(LoaderOverrides { + system_config_path: Some(system_config_path), + system_requirements_path: Some(requirements_path), + ..LoaderOverrides::without_managed_config_for_tests() + }) + .build() + .await?; + let mut builder = test_codex() + .with_home(codex_home) + .with_config(move |config| config.config_layer_stack = layered_config.config_layer_stack); + let test = builder.build_with_remote_and_local_env(&server).await?; + let startup = wait_for_event_match(&test.codex, |event| match event { + EventMsg::McpStartupComplete(summary) => Some(summary.clone()), + _ => None, + }) + .await; + let expected_ready = if expected_tools.is_empty() { + vec![] + } else { + vec!["sample"] + }; + assert_eq!( + serde_json::to_value(startup)?, + serde_json::json!({"ready": expected_ready, "failed": [], "cancelled": []}), + ); + test.codex + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Mention { + name: "sample".into(), + path: format!("plugin://{SAMPLE_PLUGIN_CONFIG_NAME}"), + }])) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + let requests = mock.requests(); + let output = requests[1].tool_search_output(PLUGIN_MCP_SEARCH_CALL_ID); + let mut visible_tools = output["tools"] + .as_array() + .into_iter() + .flatten() + .filter(|namespace| namespace["name"] == SAMPLE_PLUGIN_MCP_NAMESPACE) + .flat_map(|namespace| namespace["tools"].as_array().into_iter().flatten()) + .map(|tool| tool["name"].as_str().expect("tool name")) + .collect::>(); + visible_tools.sort_unstable(); + assert_eq!(visible_tools, expected_tools); + Ok(()) +} + #[derive(Clone, Copy)] enum ExplicitMcpRequest { Plugin,