From 68c9556cdf14c069e289904ed087f62205c4ba91 Mon Sep 17 00:00:00 2001 From: willwang-openai Date: Tue, 1 Sep 2026 22:26:39 +0000 Subject: [PATCH] Upgrade Git marketplaces from merged configuration (#42149) ## Why Configured Git marketplaces may be defined outside the user config, but those definitions could not download or update their snapshots. ## What changed - Select marketplace source, ref, and sparse-path settings from the effective merged configuration for startup synchronization and explicit upgrades. - Preserve the initiating operation's configuration loader and overrides when reloading before activation. - Roll back activation if the marketplace definition changed or configuration can no longer be loaded, without copying settings into the user config. ## Testing - Cover system-defined marketplaces, CLI overrides, startup fallback after invalid user configuration, and rollback after configuration changes. GitOrigin-RevId: 53d1eecf68a91ce2aa8430e5749d78697bfcd821 --- codex-rs/app-server/README.md | 10 +- codex-rs/app-server/src/in_process.rs | 3 +- codex-rs/app-server/src/lib.rs | 10 +- codex-rs/app-server/src/message_processor.rs | 16 +- .../src/message_processor_tracing_tests.rs | 3 +- .../app-server/src/plugin_config_reload.rs | 27 +++ .../marketplace_processor.rs | 4 + .../tests/suite/v2/marketplace_upgrade.rs | 44 ++++- codex-rs/cli/src/cloud_config.rs | 17 +- codex-rs/cli/src/marketplace_cmd.rs | 32 +++- codex-rs/cli/tests/marketplace_upgrade.rs | 49 ++++- codex-rs/core-plugins/src/lib.rs | 1 + codex-rs/core-plugins/src/manager.rs | 7 + codex-rs/core-plugins/src/manager_tests.rs | 10 +- .../src/marketplace_policy_tests.rs | 10 +- .../core-plugins/src/marketplace_upgrade.rs | 48 +++-- .../src/marketplace_upgrade_tests.rs | 178 +++++++++++++++--- 17 files changed, 387 insertions(+), 82 deletions(-) create mode 100644 codex-rs/app-server/src/plugin_config_reload.rs diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 8048e850e8..552c91e2b5 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -339,8 +339,14 @@ sources before returning; ordinary listing schedules the same work in the backgr Remote catalog settings and feature gating remain request-wide rather than being selected from the requested repos. Search continues to report `enabled: false`. -Marketplace definitions can come from system configuration, but configured Git -marketplaces currently require an existing downloaded snapshot. +Marketplace definitions can come from system configuration. Startup synchronization +and `marketplace/upgrade` download or update configured Git marketplaces using the +merged source, ref, and sparse-path settings. Snapshot metadata stays with the +downloaded files; configuration is not copied into the user layer. Pure catalog +listing does not wait for missing snapshots to download. +Activation reloads configuration with the operation's original load settings and +rolls back if the marketplace definition changed or the reload fails. User files +ignored at startup remain ignored during this check. `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 diff --git a/codex-rs/app-server/src/in_process.rs b/codex-rs/app-server/src/in_process.rs index 12f82e6b9e..18b732ae1b 100644 --- a/codex-rs/app-server/src/in_process.rs +++ b/codex-rs/app-server/src/in_process.rs @@ -63,6 +63,7 @@ use crate::outgoing_message::OutgoingEnvelope; use crate::outgoing_message::OutgoingMessage; use crate::outgoing_message::OutgoingMessageSender; use crate::outgoing_message::QueuedOutgoingMessage; +use crate::plugin_config_reload::PluginStartupConfig; use crate::transport::CHANNEL_CAPACITY; use crate::transport::OutboundConnectionState; use crate::transport::route_outgoing_envelope; @@ -486,7 +487,7 @@ async fn start_uninitialized(args: InProcessStartArgs) -> IoResult>, pub(crate) rpc_transport: AppServerRpcTransport, pub(crate) remote_control_handle: Option, - pub(crate) plugin_startup_tasks: crate::PluginStartupTasks, + /// `None` skips startup tasks; otherwise preserve the initial config-loading path. + pub(crate) plugin_startup_tasks: Option, } impl MessageProcessor { @@ -519,14 +522,23 @@ impl MessageProcessor { Arc::clone(&skills_watcher), turn_cost_worker.as_ref().map(TurnCostWorker::handle), ); - if matches!(plugin_startup_tasks, crate::PluginStartupTasks::Start) { + if let Some(startup_config) = plugin_startup_tasks { // Keep plugin startup warmups aligned at app-server startup. + let reload_config = match startup_config { + PluginStartupConfig::Current => { + plugin_config_reload::for_cwd(config_manager.clone(), config.cwd.clone()) + } + PluginStartupConfig::Defaults => { + plugin_config_reload::defaults(config_manager.clone()) + } + }; let on_effective_plugins_changed = plugin_processor.effective_plugins_changed_callback(); thread_manager .plugins_manager() .maybe_start_plugin_startup_tasks_for_config( &config.plugins_config_input(), + reload_config, Some(on_effective_plugins_changed), ); } diff --git a/codex-rs/app-server/src/message_processor_tracing_tests.rs b/codex-rs/app-server/src/message_processor_tracing_tests.rs index 842563e62b..82c1ad0127 100644 --- a/codex-rs/app-server/src/message_processor_tracing_tests.rs +++ b/codex-rs/app-server/src/message_processor_tracing_tests.rs @@ -5,6 +5,7 @@ use crate::analytics_utils::analytics_events_client_from_config; use crate::config_manager::ConfigManager; use crate::outgoing_message::ConnectionId; use crate::outgoing_message::OutgoingMessageSender; +use crate::plugin_config_reload::PluginStartupConfig; use crate::transport::AppServerTransport; use anyhow::Result; use app_test_support::create_mock_responses_server_repeating_assistant; @@ -269,7 +270,7 @@ async fn build_test_processor( code_mode_session_provider: None, rpc_transport: AppServerRpcTransport::Stdio, remote_control_handle: None, - plugin_startup_tasks: crate::PluginStartupTasks::Start, + plugin_startup_tasks: Some(PluginStartupConfig::Current), })); (processor, outgoing_rx) } diff --git a/codex-rs/app-server/src/plugin_config_reload.rs b/codex-rs/app-server/src/plugin_config_reload.rs new file mode 100644 index 0000000000..13842d1ba0 --- /dev/null +++ b/codex-rs/app-server/src/plugin_config_reload.rs @@ -0,0 +1,27 @@ +//! Adapts existing config loaders for blocking plugin-upgrade workers. +//! Reloads must use the same normal or fallback entry point as the initial load. + +use crate::config_manager::ConfigManager; +use codex_core_plugins::ConfigLayerReload; +use codex_utils_absolute_path::AbsolutePathBuf; +use std::sync::Arc; + +/// The config-loading path used to select marketplaces for startup tasks. +pub(crate) enum PluginStartupConfig { + Current, + Defaults, +} + +pub(crate) fn for_cwd(manager: ConfigManager, cwd: AbsolutePathBuf) -> ConfigLayerReload { + let runtime = tokio::runtime::Handle::current(); + Arc::new(move || runtime.block_on(manager.load_config_layers_for_cwd(cwd.clone()))) +} + +pub(crate) fn defaults(manager: ConfigManager) -> ConfigLayerReload { + let runtime = tokio::runtime::Handle::current(); + Arc::new(move || { + runtime + .block_on(manager.load_default_config()) + .map(|config| config.config_layer_stack) + }) +} diff --git a/codex-rs/app-server/src/request_processors/marketplace_processor.rs b/codex-rs/app-server/src/request_processors/marketplace_processor.rs index 77291f6e7c..31ff3c8229 100644 --- a/codex-rs/app-server/src/request_processors/marketplace_processor.rs +++ b/codex-rs/app-server/src/request_processors/marketplace_processor.rs @@ -1,4 +1,5 @@ use super::*; +use crate::plugin_config_reload; #[derive(Clone)] pub(crate) struct MarketplaceRequestProcessor { @@ -78,11 +79,14 @@ impl MarketplaceRequestProcessor { let plugins_manager = self.thread_manager.plugins_manager(); let MarketplaceUpgradeParams { marketplace_name } = params; let plugins_input = config.plugins_config_input(); + let reload_config = + plugin_config_reload::for_cwd(self.config_manager.clone(), config.cwd.clone()); let outcome = tokio::task::spawn_blocking(move || { plugins_manager.upgrade_configured_marketplaces_for_config( &plugins_input, marketplace_name.as_deref(), + &reload_config, ) }) .await diff --git a/codex-rs/app-server/tests/suite/v2/marketplace_upgrade.rs b/codex-rs/app-server/tests/suite/v2/marketplace_upgrade.rs index 1f41391a01..ce6bdbcf67 100644 --- a/codex-rs/app-server/tests/suite/v2/marketplace_upgrade.rs +++ b/codex-rs/app-server/tests/suite/v2/marketplace_upgrade.rs @@ -19,6 +19,7 @@ use codex_config::record_user_marketplace; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use tempfile::TempDir; +use test_case::test_case; use tokio::time::timeout; #[cfg(windows)] @@ -152,17 +153,17 @@ async fn marketplace_upgrade_all_configured_git_marketplaces() -> Result<()> { debug_source.path(), Some(&debug_new_revision), )?; - record_git_marketplace( - codex_home.path(), - "tools", - tools_source.path(), - Some(&tools_new_revision), - )?; + let tools_override = format!( + "marketplaces.tools={{ source_type = \"git\", source = {}, ref = {} }}", + serde_json::to_string(&tools_source.path().to_string_lossy())?, + serde_json::to_string(&tools_new_revision)?, + ); disable_plugin_startup_tasks(codex_home.path())?; let config_before = std::fs::read_to_string(codex_home.path().join("config.toml"))?; let mut mcp = TestAppServer::builder() .with_codex_home(codex_home.path()) + .with_args(&["--config", &tools_override]) .without_auto_env() .build_initialized() .await?; @@ -204,9 +205,12 @@ async fn marketplace_upgrade_all_configured_git_marketplaces() -> Result<()> { Ok(()) } +#[test_case(false; "user config")] +#[test_case(true; "invalid user config fallback")] #[tokio::test] -async fn automatic_upgrade_isolates_git_while_explicit_install_preserves_configuration() --> Result<()> { +async fn automatic_upgrade_isolates_git_while_explicit_install_preserves_configuration( + invalid_user_config: bool, +) -> Result<()> { let codex_home = TempDir::new()?; let source = TempDir::new()?; let plugin_source = TempDir::new()?; @@ -266,14 +270,30 @@ async fn automatic_upgrade_isolates_git_while_explicit_install_preserves_configu let manual_plugin_alias = "https://manual.example/plugin.git"; let manual_plugin_rewrite = format!("url.{}.insteadOf", plugin_source.path().display()); - let mut server = TestAppServer::builder() + let config_before = std::fs::read_to_string(&config_path)?; + let mut builder = TestAppServer::builder() .with_codex_home(codex_home.path()) .with_plugin_startup_tasks() .with_env_overrides(&[ ("GIT_CONFIG_COUNT", Some("1")), ("GIT_CONFIG_KEY_0", Some(&manual_plugin_rewrite)), ("GIT_CONFIG_VALUE_0", Some(manual_plugin_alias)), - ]) + ]); + if invalid_user_config { + // Runtime definitions remain available when startup ignores malformed user config. + std::fs::write(&config_path, "invalid = [")?; + let marketplace_override = format!( + "marketplaces.trusted={{ source_type = 'git', source = {} }}", + serde_json::to_string(&source_path)?, + ); + builder = builder.with_args(&[ + "--config", + &marketplace_override, + "--config", + "plugins={\"toolkit@trusted\"={enabled=true}}", + ]); + } + let mut server = builder .build_initialized_with_timeout(DEFAULT_TIMEOUT) .await?; let plugin_root = codex_home @@ -286,6 +306,10 @@ async fn automatic_upgrade_isolates_git_while_explicit_install_preserves_configu }) .await?; + if invalid_user_config { + // Subsequent explicit operations must pick up a repaired user config normally. + std::fs::write(&config_path, config_before)?; + } let marketplace_path = marketplace_install_root(codex_home.path()) .join("trusted/.agents/plugins/marketplace.json"); std::fs::write( diff --git a/codex-rs/cli/src/cloud_config.rs b/codex-rs/cli/src/cloud_config.rs index 3ac9047e95..4ca60ac503 100644 --- a/codex-rs/cli/src/cloud_config.rs +++ b/codex-rs/cli/src/cloud_config.rs @@ -16,6 +16,17 @@ pub(crate) async fn load_config( config_overrides: &CliConfigOverrides, loader_overrides: LoaderOverrides, ) -> Result { + config_builder(config_overrides, loader_overrides) + .await? + .build() + .await + .context("failed to load configuration") +} + +pub(crate) async fn config_builder( + config_overrides: &CliConfigOverrides, + loader_overrides: LoaderOverrides, +) -> Result { let cli_overrides = config_overrides .parse_overrides() .map_err(anyhow::Error::msg)?; @@ -41,12 +52,10 @@ pub(crate) async fn load_config( .await .context("failed to initialize cloud configuration authentication")?; - ConfigBuilder::default() + Ok(ConfigBuilder::default() .codex_home(codex_home.to_path_buf()) .cli_overrides(cli_overrides) .loader_overrides(loader_overrides) .cloud_config_bundle(cloud_config_bundle) - .build() - .await - .context("failed to load configuration") + .fallback_cwd(Some(cwd.to_path_buf()))) } diff --git a/codex-rs/cli/src/marketplace_cmd.rs b/codex-rs/cli/src/marketplace_cmd.rs index 82bb1ffb4d..8c78a80258 100644 --- a/codex-rs/cli/src/marketplace_cmd.rs +++ b/codex-rs/cli/src/marketplace_cmd.rs @@ -128,13 +128,14 @@ impl MarketplaceCli { subcommand, } = self; - let config = - cloud_config::load_config(&config_overrides, LoaderOverrides::default()).await?; + let builder = + cloud_config::config_builder(&config_overrides, LoaderOverrides::default()).await?; + let config = builder.clone().build().await?; match subcommand { MarketplaceSubcommand::Add(args) => run_add(config, args).await?, MarketplaceSubcommand::List(args) => run_list(config, args).await?, - MarketplaceSubcommand::Upgrade(args) => run_upgrade(config, args).await?, + MarketplaceSubcommand::Upgrade(args) => run_upgrade(config, args, builder).await?, MarketplaceSubcommand::Remove(args) => run_remove(config, args).await?, } @@ -358,16 +359,33 @@ fn configured_marketplace_sources_by_root( .collect() } -async fn run_upgrade(config: Config, args: UpgradeMarketplaceArgs) -> Result<()> { +async fn run_upgrade( + config: Config, + args: UpgradeMarketplaceArgs, + builder: codex_core::config::ConfigBuilder, +) -> Result<()> { let UpgradeMarketplaceArgs { marketplace_name, json, } = args; let manager = plugins_manager_for_config(&config, load_cli_auth_manager(&config).await?); let plugins_input = config.plugins_config_input(); - let outcome = manager - .upgrade_configured_marketplaces_for_config(&plugins_input, marketplace_name.as_deref()) - .map_err(anyhow::Error::msg)?; + let runtime = tokio::runtime::Handle::current(); + let reload_config: codex_core_plugins::ConfigLayerReload = std::sync::Arc::new(move || { + runtime + .block_on(builder.clone().build()) + .map(|config| config.config_layer_stack) + }); + let requested_name = marketplace_name.clone(); + let outcome = tokio::task::spawn_blocking(move || { + manager.upgrade_configured_marketplaces_for_config( + &plugins_input, + requested_name.as_deref(), + &reload_config, + ) + }) + .await? + .map_err(anyhow::Error::msg)?; if json { print_upgrade_outcome_json(&outcome) } else { diff --git a/codex-rs/cli/tests/marketplace_upgrade.rs b/codex-rs/cli/tests/marketplace_upgrade.rs index a2cf11f4af..bfc21b2df2 100644 --- a/codex-rs/cli/tests/marketplace_upgrade.rs +++ b/codex-rs/cli/tests/marketplace_upgrade.rs @@ -1,4 +1,6 @@ use anyhow::Result; +use codex_core_plugins::installed_marketplaces::marketplace_install_root; +use codex_utils_absolute_path::AbsolutePathBuf; use predicates::str::contains; use pretty_assertions::assert_eq; use serde_json::json; @@ -27,22 +29,65 @@ async fn marketplace_upgrade_runs_under_plugin() -> Result<()> { #[tokio::test] async fn marketplace_upgrade_json_prints_upgrade_outcome() -> Result<()> { let codex_home = TempDir::new()?; + let source = TempDir::new()?; + let manifest = r#"{"name":"debug","plugins":[]}"#; + std::fs::create_dir_all(source.path().join(".agents/plugins"))?; + std::fs::write( + source.path().join(".agents/plugins/marketplace.json"), + manifest, + )?; + for args in [ + vec!["init", "--quiet"], + vec!["add", "."], + vec![ + "-c", + "user.name=Codex Tests", + "-c", + "user.email=codex@example.com", + "commit", + "-m", + "marketplace", + ], + ] { + let output = std::process::Command::new("git") + .current_dir(source.path()) + .args(args) + .output()?; + anyhow::ensure!( + output.status.success(), + "git fixture failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + } + let source_arg = serde_json::to_string(&source.path().to_string_lossy())?; + let marketplace_override = + format!("marketplaces.debug={{source_type=\"git\",source={source_arg}}}"); let assert = codex_command(codex_home.path())? + .current_dir(codex_home.path()) + .args(["--config", &marketplace_override]) .args(["plugin", "marketplace", "upgrade", "--json"]) .assert() .success(); let stdout = assert.get_output().stdout.as_slice(); let actual: serde_json::Value = serde_json::from_slice(stdout)?; + let installed_root = AbsolutePathBuf::try_from( + marketplace_install_root(&codex_home.path().canonicalize()?).join("debug"), + )?; assert_eq!( actual, json!({ - "selectedMarketplaces": [], - "upgradedRoots": [], + "selectedMarketplaces": ["debug"], + "upgradedRoots": [installed_root], "errors": [], }) ); + assert_eq!( + std::fs::read_to_string(installed_root.join(".agents/plugins/marketplace.json"))?, + manifest + ); + assert!(!codex_home.path().join("config.toml").exists()); Ok(()) } diff --git a/codex-rs/core-plugins/src/lib.rs b/codex-rs/core-plugins/src/lib.rs index 539e628284..ed440b5d97 100644 --- a/codex-rs/core-plugins/src/lib.rs +++ b/codex-rs/core-plugins/src/lib.rs @@ -89,6 +89,7 @@ pub use manager::RemotePluginOperationError; pub use manager::RemotePluginOperationErrorKind; pub use manager::RemotePluginUninstallOutcome; pub use marketplace_policy::allowed_configured_marketplace_names; +pub use marketplace_upgrade::ConfigLayerReload; pub use marketplace_upgrade::ConfiguredMarketplaceUpgradeError as PluginMarketplaceUpgradeError; pub use marketplace_upgrade::ConfiguredMarketplaceUpgradeOutcome as PluginMarketplaceUpgradeOutcome; pub use plugin_metrics::PluginMeasurementDefinition; diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index a83f0eb459..112eeb0d87 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -58,6 +58,7 @@ use crate::marketplace::list_marketplaces_with_home; use crate::marketplace::plugin_interface_with_marketplace_category; use crate::marketplace_policy::MarketplacePolicy; use crate::marketplace_policy::configured_plugins_from_stack; +use crate::marketplace_upgrade::ConfigLayerReload; use crate::marketplace_upgrade::ConfiguredMarketplaceUpgradeError; use crate::marketplace_upgrade::ConfiguredMarketplaceUpgradeOutcome; use crate::marketplace_upgrade::upgrade_configured_git_marketplaces_with_mode; @@ -2734,6 +2735,7 @@ impl PluginsManager { pub fn maybe_start_plugin_startup_tasks_for_config( self: &Arc, config: &PluginsConfigInput, + reload_config: ConfigLayerReload, on_effective_plugins_changed: Option, ) { if config.plugins_enabled { @@ -2765,6 +2767,7 @@ impl PluginsManager { &config, /*marketplace_name*/ None, PluginGitMode::Automatic, + &reload_config, ); match outcome { Ok(outcome) => { @@ -2860,11 +2863,13 @@ impl PluginsManager { &self, config: &PluginsConfigInput, marketplace_name: Option<&str>, + reload_config: &ConfigLayerReload, ) -> Result { self.upgrade_configured_marketplaces_for_config_with_mode( config, marketplace_name, PluginGitMode::Manual, + reload_config, ) } @@ -2874,12 +2879,14 @@ impl PluginsManager { config: &PluginsConfigInput, marketplace_name: Option<&str>, mode: PluginGitMode, + reload_config: &ConfigLayerReload, ) -> Result { let mut outcome = upgrade_configured_git_marketplaces_with_mode( self.codex_home.as_path(), &config.config_layer_stack, marketplace_name, mode, + reload_config, ); if let Some(marketplace_name) = marketplace_name && outcome.selected_marketplaces.is_empty() diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 767d7a1d97..e7963d443f 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -5313,8 +5313,14 @@ source = "{remote_repo_url}" let manager = test_plugins_manager(tmp.path().to_path_buf()); let config = load_config(tmp.path(), tmp.path()).await; + let reload_stack = config.config_layer_stack.clone(); + let reload_config: ConfigLayerReload = Arc::new(move || Ok(reload_stack.clone())); let initial_upgrade = manager - .upgrade_configured_marketplaces_for_config(&config, /*marketplace_name*/ None) + .upgrade_configured_marketplaces_for_config( + &config, + /*marketplace_name*/ None, + &reload_config, + ) .expect("initial marketplace install should succeed"); assert_eq!(initial_upgrade.errors, Vec::new()); assert_eq!(initial_upgrade.upgraded_roots.len(), 1); @@ -5350,7 +5356,7 @@ source = "{remote_repo_url}" run_git(&remote_repo, &["add", "."]); run_git(&remote_repo, &["commit", "-m", "update plugin"]); let upgrade = manager - .upgrade_configured_marketplaces_for_config(&config, Some("debug")) + .upgrade_configured_marketplaces_for_config(&config, Some("debug"), &reload_config) .expect("marketplace upgrade should succeed"); assert_eq!(upgrade.errors, Vec::new()); assert_eq!(upgrade.upgraded_roots.len(), 1); diff --git a/codex-rs/core-plugins/src/marketplace_policy_tests.rs b/codex-rs/core-plugins/src/marketplace_policy_tests.rs index 9f56f8fccd..a2bef00663 100644 --- a/codex-rs/core-plugins/src/marketplace_policy_tests.rs +++ b/codex-rs/core-plugins/src/marketplace_policy_tests.rs @@ -690,6 +690,8 @@ enabled = true #[test] fn blocked_or_reserved_upgrade_is_rejected_before_marketplace_installation() { + let reload_config: crate::ConfigLayerReload = + std::sync::Arc::new(|| panic!("blocked or reserved marketplace must not reload config")); let codex_home = TempDir::new().expect("create Codex home"); let config_file = AbsolutePathBuf::try_from(codex_home.path().join("config.toml")) .expect("absolute config path"); @@ -708,7 +710,12 @@ source = "https://github.com/example/blocked.git" )), ); - let outcome = upgrade_configured_git_marketplaces(codex_home.path(), &stack, Some("debug")); + let outcome = upgrade_configured_git_marketplaces( + codex_home.path(), + &stack, + Some("debug"), + &reload_config, + ); assert_eq!(outcome.selected_marketplaces, vec!["debug".to_string()]); assert_eq!(outcome.upgraded_roots, Vec::new()); @@ -731,6 +738,7 @@ source = "https://github.com/example/blocked.git" codex_home.path(), &stack, Some(crate::OPENAI_BUNDLED_MARKETPLACE_NAME), + &reload_config, ); assert!(outcome.errors[0].message.contains("is reserved")); assert!(!marketplace_install_root(codex_home.path()).exists()); diff --git a/codex-rs/core-plugins/src/marketplace_upgrade.rs b/codex-rs/core-plugins/src/marketplace_upgrade.rs index 91e7d8e814..9bb06788b4 100644 --- a/codex-rs/core-plugins/src/marketplace_upgrade.rs +++ b/codex-rs/core-plugins/src/marketplace_upgrade.rs @@ -13,15 +13,18 @@ use crate::marketplace::validate_marketplace_root; use crate::marketplace_add::MarketplaceSource; use crate::marketplace_policy::MarketplacePolicy; use crate::marketplace_policy::validate_marketplace_name_for_add; -use codex_config::CONFIG_TOML_FILE; use codex_config::ConfigLayerStack; use codex_config::types::MarketplaceConfig; use codex_config::types::MarketplaceSourceType; use codex_plugin::validate_plugin_segment; use codex_utils_absolute_path::AbsolutePathBuf; use std::path::Path; +use std::sync::Arc; use std::time::Duration; +/// Reloads configuration with the initiating operation's settings. Called only on a blocking worker. +pub type ConfigLayerReload = Arc std::io::Result + Send + Sync>; + const MARKETPLACE_UPGRADE_GIT_TIMEOUT: Duration = Duration::from_secs(30); #[derive(Debug, Clone, PartialEq, Eq)] @@ -71,12 +74,14 @@ pub fn upgrade_configured_git_marketplaces( codex_home: &Path, config_layer_stack: &ConfigLayerStack, marketplace_name: Option<&str>, + reload_config: &ConfigLayerReload, ) -> ConfiguredMarketplaceUpgradeOutcome { upgrade_configured_git_marketplaces_with_mode( codex_home, config_layer_stack, marketplace_name, PluginGitMode::Manual, + reload_config, ) } @@ -86,6 +91,7 @@ pub(crate) fn upgrade_configured_git_marketplaces_with_mode( config_layer_stack: &ConfigLayerStack, marketplace_name: Option<&str>, mode: PluginGitMode, + reload_config: &ConfigLayerReload, ) -> ConfiguredMarketplaceUpgradeOutcome { let loaded = load_configured_git_marketplaces(config_layer_stack); let marketplaces = loaded @@ -131,6 +137,7 @@ pub(crate) fn upgrade_configured_git_marketplaces_with_mode( codex_home, &install_root, &marketplace, + reload_config, normalized_source.as_ref(), mode, ) { @@ -155,10 +162,8 @@ pub(crate) fn upgrade_configured_git_marketplaces_with_mode( fn load_configured_git_marketplaces( config_layer_stack: &ConfigLayerStack, ) -> ConfiguredGitMarketplaceLoadOutcome { - let Some(user_config) = config_layer_stack.effective_user_config() else { - return ConfiguredGitMarketplaceLoadOutcome::default(); - }; - let Some(marketplaces) = user_config + let effective_config = config_layer_stack.effective_config(); + let Some(marketplaces) = effective_config .get("marketplaces") .and_then(toml::Value::as_table) else { @@ -221,6 +226,7 @@ fn upgrade_configured_git_marketplace( codex_home: &Path, install_root: &Path, marketplace: &ConfiguredGitMarketplace, + reload_config: &ConfigLayerReload, normalized_source: Option<&MarketplaceSource>, mode: PluginGitMode, ) -> Result, String> { @@ -284,7 +290,7 @@ fn upgrade_configured_git_marketplace( } write_installed_marketplace_metadata(staged_dir.path(), marketplace, &activated_revision)?; activate_marketplace_root(&destination, staged_dir, &previous_snapshot, || { - ensure_configured_git_marketplace_unchanged(codex_home, marketplace) + ensure_configured_git_marketplace_unchanged(reload_config, marketplace) })?; AbsolutePathBuf::try_from(destination) @@ -292,10 +298,10 @@ fn upgrade_configured_git_marketplace( .map_err(|err| format!("upgraded marketplace path is not absolute: {err}")) } fn ensure_configured_git_marketplace_unchanged( - codex_home: &Path, + reload_config: &ConfigLayerReload, expected: &ConfiguredGitMarketplace, ) -> Result<(), String> { - let current = read_configured_git_marketplace(codex_home, &expected.name)?; + let current = read_configured_git_marketplace(reload_config, &expected.name)?; match current { Some(current) if current == *expected => Ok(()), Some(_) => Err(format!( @@ -310,27 +316,15 @@ fn ensure_configured_git_marketplace_unchanged( } fn read_configured_git_marketplace( - codex_home: &Path, + reload_config: &ConfigLayerReload, marketplace_name: &str, ) -> Result, String> { - let config_path = codex_home.join(CONFIG_TOML_FILE); - let raw_config = match std::fs::read_to_string(&config_path) { - Ok(raw_config) => raw_config, - Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(None), - Err(err) => { - return Err(format!( - "failed to read user config {} while checking marketplace auto-upgrade: {err}", - config_path.display() - )); - } - }; - let config: toml::Value = toml::from_str(&raw_config).map_err(|err| { - format!( - "failed to parse user config {} while checking marketplace auto-upgrade: {err}", - config_path.display() - ) - })?; - let Some(marketplace) = config + let effective_config = reload_config() + .map_err(|err| { + format!("failed to reload config while checking marketplace upgrade: {err}") + })? + .effective_config(); + let Some(marketplace) = effective_config .get("marketplaces") .and_then(toml::Value::as_table) .and_then(|marketplaces| marketplaces.get(marketplace_name)) diff --git a/codex-rs/core-plugins/src/marketplace_upgrade_tests.rs b/codex-rs/core-plugins/src/marketplace_upgrade_tests.rs index 3a4c28d7aa..7b01cee678 100644 --- a/codex-rs/core-plugins/src/marketplace_upgrade_tests.rs +++ b/codex-rs/core-plugins/src/marketplace_upgrade_tests.rs @@ -1,9 +1,9 @@ use super::*; use crate::PluginGitMode; -use codex_config::ConfigLayerEntry; -use codex_config::ConfigLayerSource; -use codex_config::ConfigRequirements; -use codex_config::ConfigRequirementsToml; +use codex_config::CONFIG_TOML_FILE; +use codex_config::LoaderOverrides; +use codex_config::loader::load_config_layers_state; +use codex_exec_server::LOCAL_FS; use pretty_assertions::assert_eq; use std::path::Path; use std::path::PathBuf; @@ -31,7 +31,7 @@ last_revision = "abc123" .expect("write config"); assert_eq!( - read_configured_git_marketplace(codex_home.path(), "good") + read_configured_git_marketplace(&config_reloader(codex_home.path()), "good") .expect("read configured marketplace"), Some(ConfiguredGitMarketplace { name: "good".to_string(), @@ -65,12 +65,14 @@ source = {good_url:?} "# ); std::fs::write(codex_home.path().join(CONFIG_TOML_FILE), &config).expect("write config"); - let stack = config_layer_stack(codex_home.path(), &config); + let reload_config = config_reloader(codex_home.path()); + let stack = reload_config().expect("load config"); let outcome = upgrade_configured_git_marketplaces( codex_home.path(), &stack, /*marketplace_name*/ None, + &reload_config, ); assert_eq!( @@ -103,12 +105,14 @@ fn automatic_marketplace_git_ignores_inherited_repository_configuration() { let config = format!("[marketplaces.trusted]\nsource_type = \"git\"\nsource = {source:?}\n"); std::fs::write(codex_home.join(CONFIG_TOML_FILE), &config).expect("write config"); - let stack = config_layer_stack(&codex_home, &config); + let reload_config = config_reloader(&codex_home); + let stack = reload_config().expect("load config"); let outcome = upgrade_configured_git_marketplaces_with_mode( &codex_home, &stack, /*marketplace_name*/ None, PluginGitMode::Automatic, + &reload_config, ); assert_eq!( outcome, @@ -255,6 +259,7 @@ ref = "missing-ref" codex_home.path(), &install_root, &marketplace, + &config_reloader(codex_home.path()), Some(&normalized_source), PluginGitMode::Manual, ) @@ -299,6 +304,7 @@ fn up_to_date_fast_path_validates_marketplace_name() { codex_home.path(), &install_root, &marketplace, + &config_reloader(codex_home.path()), Some(&normalized_source), PluginGitMode::Manual, ) @@ -380,21 +386,149 @@ fn stale_activation_restores_newer_concurrently_installed_marketplace() { )); } -fn config_layer_stack(codex_home: &Path, config: &str) -> ConfigLayerStack { - let config_file = - AbsolutePathBuf::try_from(codex_home.join(CONFIG_TOML_FILE)).expect("absolute config path"); - ConfigLayerStack::new( - vec![ConfigLayerEntry::new( - ConfigLayerSource::User { - file: config_file, - profile: None, - }, - toml::from_str(config).expect("parse config"), - )], - ConfigRequirements::default(), - ConfigRequirementsToml::default(), - ) - .expect("build config layer stack") +fn config_reloader(codex_home: &Path) -> ConfigLayerReload { + let codex_home = codex_home.to_path_buf(); + let options = LoaderOverrides { + system_config_path: Some(codex_home.join("system.toml")), + managed_config_path: Some(codex_home.join("managed.toml")), + system_requirements_path: Some(codex_home.join("requirements.toml")), + ..LoaderOverrides::without_managed_config_for_tests() + }; + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("create config loading runtime"); + Arc::new(move || { + runtime.block_on(load_config_layers_state( + LOCAL_FS.as_ref(), + &codex_home, + Some(AbsolutePathBuf::try_from(codex_home.join("project"))?), + &[], + options.clone(), + &codex_config::NoopThreadConfigLoader, + )) + }) +} + +fn system_marketplace_stack(codex_home: &Path, system: &str, user: &str) -> ConfigLayerStack { + let system_file = AbsolutePathBuf::try_from(codex_home.join("system.toml")).unwrap(); + std::fs::write(&system_file, system).unwrap(); + if !user.is_empty() { + std::fs::write(codex_home.join(CONFIG_TOML_FILE), user).unwrap(); + } + config_reloader(codex_home)().expect("load system and user config") +} + +#[test] +fn system_marketplace_downloads_and_updates_without_writing_user_config() { + for user_config in ["", "[marketplaces.good]\nsparse_paths = []\n"] { + let codex_home = TempDir::new().unwrap(); + let remote = TempDir::new().unwrap(); + init_marketplace_repo(remote.path(), "good"); + run_git(remote.path(), &["checkout", "-b", "release"]); + std::fs::write(remote.path().join("marker"), "release").unwrap(); + run_git(remote.path(), &["add", "."]); + run_git(remote.path(), &["commit", "-m", "release"]); + run_git(remote.path(), &["checkout", "-b", "other"]); + std::fs::write(remote.path().join("marker"), "wrong branch").unwrap(); + run_git(remote.path(), &["commit", "-am", "other"]); + let source = url::Url::from_directory_path(remote.path()) + .unwrap() + .to_string(); + let system = format!( + "[marketplaces.good]\nsource_type = \"git\"\nsource = {source:?}\nref = \"release\"\n" + ); + let stack = system_marketplace_stack(codex_home.path(), &system, user_config); + let reload_config = config_reloader(codex_home.path()); + let root = + AbsolutePathBuf::try_from(marketplace_install_root(codex_home.path()).join("good")) + .unwrap(); + for marker in ["release", "updated"] { + if marker == "updated" { + run_git(remote.path(), &["checkout", "release"]); + std::fs::write(remote.path().join("marker"), marker).unwrap(); + run_git(remote.path(), &["commit", "-am", "update release"]); + } + assert_eq!( + upgrade_configured_git_marketplaces_with_mode( + codex_home.path(), + &stack, + Some("good"), + PluginGitMode::Automatic, + &reload_config, + ), + ConfiguredMarketplaceUpgradeOutcome { + selected_marketplaces: vec!["good".to_string()], + upgraded_roots: vec![root.clone()], + errors: Vec::new(), + }, + ); + assert_eq!( + std::fs::read_to_string(root.join("marker")).unwrap(), + marker + ); + } + assert_eq!( + std::fs::read_to_string(codex_home.path().join("system.toml")).unwrap(), + system + ); + let user_file = codex_home.path().join(CONFIG_TOML_FILE); + if user_config.is_empty() { + assert!(!user_file.exists()); + } else { + assert_eq!(std::fs::read_to_string(user_file).unwrap(), user_config); + } + } +} + +#[test] +fn changed_config_rolls_back_marketplace_activation() { + let codex_home = TempDir::new().unwrap(); + let remote = TempDir::new().unwrap(); + init_marketplace_repo(remote.path(), "good"); + let source = url::Url::from_directory_path(remote.path()) + .unwrap() + .to_string(); + let system = format!("[marketplaces.good]\nsource_type = \"git\"\nsource = {source:?}\n"); + let stack = system_marketplace_stack(codex_home.path(), &system, ""); + let reload_config = config_reloader(codex_home.path()); + let initial = upgrade_configured_git_marketplaces( + codex_home.path(), + &stack, + Some("good"), + &reload_config, + ); + assert!(initial.all_succeeded()); + let root = &initial.upgraded_roots[0]; + let original_metadata = std::fs::read(root.join(".codex-marketplace-install.json")).unwrap(); + std::fs::write(remote.path().join("new-file"), "new revision").unwrap(); + run_git(remote.path(), &["add", "."]); + run_git(remote.path(), &["commit", "-m", "update"]); + for (file, changed_config) in [ + ("system.toml", format!("{system}ref = \"different\"\n")), + ("system.toml", String::new()), + ( + CONFIG_TOML_FILE, + "[marketplaces.good]\nref = \"user-override\"\n".to_string(), + ), + (CONFIG_TOML_FILE, "invalid = [".to_string()), + ] { + std::fs::write(codex_home.path().join("system.toml"), &system).unwrap(); + std::fs::write(codex_home.path().join(file), changed_config).unwrap(); + let outcome = upgrade_configured_git_marketplaces( + codex_home.path(), + &stack, + Some("good"), + &reload_config, + ); + assert_eq!(outcome.upgraded_roots, Vec::new()); + assert_eq!(outcome.errors.len(), 1); + assert!(!root.join("new-file").exists()); + assert_eq!( + std::fs::read(root.join(".codex-marketplace-install.json")).unwrap(), + original_metadata + ); + } } fn init_marketplace_repo(repo: &Path, marketplace_name: &str) {