diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 9a9d4dd9ce..d0b835280c 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -6349,7 +6349,6 @@ impl CodexMessageProcessor { return; } }; - plugins_manager.maybe_start_configured_marketplace_upgrade_for_config(&config); let mut remote_sync_error = None; let auth = self.auth_manager.auth().await; diff --git a/codex-rs/core/src/plugins/manager.rs b/codex-rs/core/src/plugins/manager.rs index 0288ef7c7e..a3e00f171e 100644 --- a/codex-rs/core/src/plugins/manager.rs +++ b/codex-rs/core/src/plugins/manager.rs @@ -102,17 +102,28 @@ struct CachedFeaturedPluginIds { featured_plugin_ids: Vec, } +#[derive(Clone, PartialEq, Eq)] +struct NonCuratedCacheRefreshRequest { + roots: Vec, + mode: NonCuratedCacheRefreshMode, +} + +#[derive(Clone, Copy, PartialEq, Eq)] +enum NonCuratedCacheRefreshMode { + IfVersionChanged, + ForceReinstall, +} + #[derive(Default)] struct NonCuratedCacheRefreshState { - requested_roots: Option>, - last_refreshed_roots: Option>, + requested: Option, + last_refreshed: Option, in_flight: bool, } #[derive(Default)] struct ConfiguredMarketplaceUpgradeState { in_flight: bool, - completed: bool, } fn featured_plugin_ids_cache_key( @@ -1111,7 +1122,7 @@ impl PluginsManager { Ok(state) => state, Err(err) => err.into_inner(), }; - if state.completed || state.in_flight { + if state.in_flight { return; } state.in_flight = true; @@ -1128,35 +1139,17 @@ impl PluginsManager { .name("plugins-marketplace-auto-upgrade".to_string()) .spawn(move || { let outcome = upgrade_configured_git_marketplaces(codex_home.as_path(), &config); - let cache_refresh_succeeded = if outcome.upgraded_roots.is_empty() { - true - } else { - match refresh_non_curated_plugin_cache( - codex_home.as_path(), + if !outcome.upgraded_roots.is_empty() { + manager.maybe_start_non_curated_plugin_cache_force_reinstall_for_roots( &outcome.upgraded_roots, - ) { - Ok(cache_refreshed) => { - if cache_refreshed { - manager.clear_cache(); - } - true - } - Err(err) => { - manager.clear_cache(); - warn!( - "failed to refresh non-curated plugin cache after marketplace auto-upgrade: {err}" - ); - false - } - } - }; + ); + } let mut state = match manager.configured_marketplace_upgrade_state.write() { Ok(state) => state, Err(err) => err.into_inner(), }; state.in_flight = false; - state.completed = outcome.all_succeeded && cache_refresh_succeeded; }) { let mut state = match self.configured_marketplace_upgrade_state.write() { @@ -1171,6 +1164,27 @@ impl PluginsManager { pub fn maybe_start_non_curated_plugin_cache_refresh_for_roots( self: &Arc, roots: &[AbsolutePathBuf], + ) { + self.maybe_start_non_curated_plugin_cache_refresh_for_roots_with_mode( + roots, + NonCuratedCacheRefreshMode::IfVersionChanged, + ); + } + + fn maybe_start_non_curated_plugin_cache_force_reinstall_for_roots( + self: &Arc, + roots: &[AbsolutePathBuf], + ) { + self.maybe_start_non_curated_plugin_cache_refresh_for_roots_with_mode( + roots, + NonCuratedCacheRefreshMode::ForceReinstall, + ); + } + + fn maybe_start_non_curated_plugin_cache_refresh_for_roots_with_mode( + self: &Arc, + roots: &[AbsolutePathBuf], + mode: NonCuratedCacheRefreshMode, ) { let mut roots = roots.to_vec(); roots.sort_unstable(); @@ -1178,6 +1192,7 @@ impl PluginsManager { if roots.is_empty() { return; } + let request = NonCuratedCacheRefreshRequest { roots, mode }; let should_spawn = { let mut state = match self.non_curated_cache_refresh_state.write() { @@ -1185,13 +1200,25 @@ impl PluginsManager { Err(err) => err.into_inner(), }; // Collapse repeated plugin/list requests onto one worker and only queue another pass - // when the requested roots set actually changes. - if state.requested_roots.as_ref() == Some(&roots) - || (!state.in_flight && state.last_refreshed_roots.as_ref() == Some(&roots)) + // when the requested roots set actually changes. Forced reinstall requests are not + // deduped against the last completed pass because the same marketplace root path can + // point at newly activated files after an auto-upgrade. + if state.requested.as_ref() == Some(&request) + || (mode == NonCuratedCacheRefreshMode::IfVersionChanged + && !state.in_flight + && state.last_refreshed.as_ref() == Some(&request)) { return; } - state.requested_roots = Some(roots); + if mode == NonCuratedCacheRefreshMode::IfVersionChanged + && state.requested.as_ref().is_some_and(|requested| { + requested.mode == NonCuratedCacheRefreshMode::ForceReinstall + && requested.roots == request.roots + }) + { + return; + } + state.requested = Some(request); if state.in_flight { false } else { @@ -1213,7 +1240,7 @@ impl PluginsManager { Err(err) => err.into_inner(), }; state.in_flight = false; - state.requested_roots = None; + state.requested = None; warn!("failed to start non-curated plugin cache refresh task: {err}"); } } @@ -1267,15 +1294,15 @@ impl PluginsManager { fn run_non_curated_plugin_cache_refresh_loop(self: Arc) { loop { - let roots = { + let request = { let state = match self.non_curated_cache_refresh_state.read() { Ok(state) => state, Err(err) => err.into_inner(), }; - state.requested_roots.clone() + state.requested.clone() }; - let Some(roots) = roots else { + let Some(request) = request else { let mut state = match self.non_curated_cache_refresh_state.write() { Ok(state) => state, Err(err) => err.into_inner(), @@ -1284,30 +1311,33 @@ impl PluginsManager { return; }; - let refreshed = - match refresh_non_curated_plugin_cache(self.codex_home.as_path(), &roots) { - Ok(cache_refreshed) => { - if cache_refreshed { - self.clear_cache(); - } - true - } - Err(err) => { + let refreshed = match refresh_non_curated_plugin_cache( + self.codex_home.as_path(), + &request.roots, + request.mode, + ) { + Ok(cache_refreshed) => { + if cache_refreshed { self.clear_cache(); - warn!("failed to refresh non-curated plugin cache: {err}"); - false } - }; + true + } + Err(err) => { + self.clear_cache(); + warn!("failed to refresh non-curated plugin cache: {err}"); + false + } + }; let mut state = match self.non_curated_cache_refresh_state.write() { Ok(state) => state, Err(err) => err.into_inner(), }; if refreshed { - state.last_refreshed_roots = Some(roots.clone()); + state.last_refreshed = Some(request.clone()); } - if state.requested_roots.as_ref() == Some(&roots) { - state.requested_roots = None; + if state.requested.as_ref() == Some(&request) { + state.requested = None; state.in_flight = false; return; } @@ -1557,6 +1587,7 @@ fn refresh_curated_plugin_cache( fn refresh_non_curated_plugin_cache( codex_home: &Path, additional_roots: &[AbsolutePathBuf], + mode: NonCuratedCacheRefreshMode, ) -> Result { let configured_non_curated_plugin_ids = non_curated_plugin_ids_from_config_keys(configured_plugins_from_codex_home( @@ -1625,7 +1656,9 @@ fn refresh_non_curated_plugin_cache( continue; }; - if store.active_plugin_version(&plugin_id).as_deref() == Some(plugin_version.as_str()) { + if mode == NonCuratedCacheRefreshMode::IfVersionChanged + && store.active_plugin_version(&plugin_id).as_deref() == Some(plugin_version.as_str()) + { continue; } diff --git a/codex-rs/core/src/plugins/manager_tests.rs b/codex-rs/core/src/plugins/manager_tests.rs index 3b06a08a9f..665dd7dfef 100644 --- a/codex-rs/core/src/plugins/manager_tests.rs +++ b/codex-rs/core/src/plugins/manager_tests.rs @@ -2562,6 +2562,7 @@ enabled = true refresh_non_curated_plugin_cache( tmp.path(), &[AbsolutePathBuf::try_from(repo_root).unwrap()], + NonCuratedCacheRefreshMode::IfVersionChanged, ) .expect("cache refresh should succeed") ); @@ -2614,6 +2615,7 @@ enabled = true refresh_non_curated_plugin_cache( tmp.path(), &[AbsolutePathBuf::try_from(repo_root).unwrap()], + NonCuratedCacheRefreshMode::IfVersionChanged, ) .expect("cache refresh should reinstall missing configured plugin") ); @@ -2667,11 +2669,75 @@ enabled = true !refresh_non_curated_plugin_cache( tmp.path(), &[AbsolutePathBuf::try_from(repo_root).unwrap()], + NonCuratedCacheRefreshMode::IfVersionChanged, ) .expect("cache refresh should be a no-op when configured plugins are current") ); } +#[test] +fn refresh_non_curated_plugin_cache_force_reinstalls_current_local_version() { + let tmp = tempfile::tempdir().unwrap(); + let repo_root = tmp.path().join("repo"); + fs::create_dir_all(repo_root.join(".git")).unwrap(); + fs::create_dir_all(repo_root.join(".agents/plugins")).unwrap(); + write_plugin(&repo_root, "sample-plugin", "sample-plugin"); + fs::write(repo_root.join("sample-plugin/skills/SKILL.md"), "new skill").unwrap(); + write_file( + &repo_root.join(".agents/plugins/marketplace.json"), + r#"{ + "name": "debug", + "plugins": [ + { + "name": "sample-plugin", + "source": { + "source": "local", + "path": "./sample-plugin" + } + } + ] +}"#, + ); + write_plugin( + &tmp.path().join("plugins/cache/debug"), + "sample-plugin/local", + "sample-plugin", + ); + fs::write( + tmp.path() + .join("plugins/cache/debug/sample-plugin/local/skills/SKILL.md"), + "old skill", + ) + .unwrap(); + write_file( + &tmp.path().join(CONFIG_TOML_FILE), + r#"[features] +plugins = true + +[plugins."sample-plugin@debug"] +enabled = true +"#, + ); + + assert!( + refresh_non_curated_plugin_cache( + tmp.path(), + &[AbsolutePathBuf::try_from(repo_root).unwrap()], + NonCuratedCacheRefreshMode::ForceReinstall, + ) + .expect("cache refresh should reinstall unchanged local version") + ); + + assert_eq!( + fs::read_to_string( + tmp.path() + .join("plugins/cache/debug/sample-plugin/local/skills/SKILL.md") + ) + .unwrap(), + "new skill" + ); +} + #[test] fn refresh_non_curated_plugin_cache_ignores_invalid_unconfigured_plugin_versions() { let tmp = tempfile::tempdir().unwrap(); @@ -2716,6 +2782,7 @@ enabled = true refresh_non_curated_plugin_cache( tmp.path(), &[AbsolutePathBuf::try_from(repo_root).unwrap()], + NonCuratedCacheRefreshMode::IfVersionChanged, ) .expect("cache refresh should ignore unrelated invalid plugin manifests") ); diff --git a/codex-rs/core/src/plugins/marketplace_upgrade.rs b/codex-rs/core/src/plugins/marketplace_upgrade.rs index 98e4ab990f..62e960eac3 100644 --- a/codex-rs/core/src/plugins/marketplace_upgrade.rs +++ b/codex-rs/core/src/plugins/marketplace_upgrade.rs @@ -1,3 +1,11 @@ +mod activation; +mod git; + +use self::activation::activate_marketplace_root; +use self::activation::activated_marketplace_metadata_matches; +use self::activation::write_activated_marketplace_metadata; +use self::git::clone_git_source; +use self::git::git_remote_revision; use super::installed_marketplaces::marketplace_install_root; use super::validate_marketplace_root; use codex_config::MarketplaceConfigUpdate; @@ -5,14 +13,12 @@ use codex_config::record_user_marketplace; use codex_config::types::MarketplaceConfig; use codex_config::types::MarketplaceSourceType; use codex_utils_absolute_path::AbsolutePathBuf; +use std::collections::HashMap; use std::path::Path; -use std::process::Command; -use std::process::Output; -use std::process::Stdio; use std::time::Duration; -use tempfile::TempDir; use tracing::warn; +use crate::config::CONFIG_TOML_FILE; use crate::config::Config; const MARKETPLACE_UPGRADE_GIT_TIMEOUT: Duration = Duration::from_secs(30); @@ -78,7 +84,7 @@ fn configured_git_marketplaces(config: &Config) -> Vec }; let marketplaces = match marketplaces_value .clone() - .try_into::>() + .try_into::>() { Ok(marketplaces) => marketplaces, Err(err) => { @@ -89,38 +95,43 @@ fn configured_git_marketplaces(config: &Config) -> Vec let mut configured = marketplaces .into_iter() - .filter_map(|(name, marketplace)| { - let MarketplaceConfig { - last_updated: _, - last_revision, - source_type, - source, - ref_name, - sparse_paths, - } = marketplace; - if source_type != Some(MarketplaceSourceType::Git) { - return None; - } - let Some(source) = source else { - warn!( - marketplace = name, - "ignoring configured Git marketplace without source" - ); - return None; - }; - Some(ConfiguredGitMarketplace { - name, - source, - ref_name, - sparse_paths: sparse_paths.unwrap_or_default(), - last_revision, - }) - }) + .filter_map(|(name, marketplace)| configured_git_marketplace_from_config(name, marketplace)) .collect::>(); configured.sort_unstable_by(|left, right| left.name.cmp(&right.name)); configured } +fn configured_git_marketplace_from_config( + name: String, + marketplace: MarketplaceConfig, +) -> Option { + let MarketplaceConfig { + last_updated: _, + last_revision, + source_type, + source, + ref_name, + sparse_paths, + } = marketplace; + if source_type != Some(MarketplaceSourceType::Git) { + return None; + } + let Some(source) = source else { + warn!( + marketplace = name, + "ignoring configured Git marketplace without source" + ); + return None; + }; + Some(ConfiguredGitMarketplace { + name, + source, + ref_name, + sparse_paths: sparse_paths.unwrap_or_default(), + last_revision, + }) +} + fn upgrade_configured_git_marketplace( codex_home: &Path, install_root: &Path, @@ -137,6 +148,7 @@ fn upgrade_configured_git_marketplace( .join(".agents/plugins/marketplace.json") .is_file() && marketplace.last_revision.as_deref() == Some(remote_revision.as_str()) + && activated_marketplace_metadata_matches(&destination, marketplace, &remote_revision) { return Ok(None); } @@ -158,7 +170,7 @@ fn upgrade_configured_git_marketplace( ) })?; - clone_git_source( + let activated_revision = clone_git_source( &marketplace.source, marketplace.ref_name.as_deref(), &marketplace.sparse_paths, @@ -173,17 +185,19 @@ fn upgrade_configured_git_marketplace( marketplace.name )); } + write_activated_marketplace_metadata(staged_dir.path(), marketplace, &activated_revision)?; let last_updated = chrono::Utc::now().to_rfc3339_opts(chrono::SecondsFormat::Secs, true); let update = MarketplaceConfigUpdate { last_updated: &last_updated, - last_revision: Some(&remote_revision), + last_revision: Some(&activated_revision), source_type: "git", source: &marketplace.source, ref_name: marketplace.ref_name.as_deref(), sparse_paths: &marketplace.sparse_paths, }; activate_marketplace_root(&destination, staged_dir, || { + ensure_configured_git_marketplace_unchanged(codex_home, marketplace)?; record_user_marketplace(codex_home, &marketplace.name, &update).map_err(|err| { format!( "failed to record upgraded marketplace `{}` in user config.toml: {err}", @@ -196,279 +210,66 @@ fn upgrade_configured_git_marketplace( .map(Some) .map_err(|err| format!("upgraded marketplace path is not absolute: {err}")) } - -fn git_remote_revision( - source: &str, - ref_name: Option<&str>, - timeout: Duration, -) -> Result { - if let Some(ref_name) = ref_name - && is_full_git_sha(ref_name) - { - return Ok(ref_name.to_string()); - } - - let ref_name = ref_name.unwrap_or("HEAD"); - let output = run_git_command_with_timeout( - Command::new("git") - .env("GIT_OPTIONAL_LOCKS", "0") - .arg("ls-remote") - .arg(source) - .arg(ref_name), - "git ls-remote marketplace source", - timeout, - )?; - ensure_git_success(&output, "git ls-remote marketplace source")?; - - let stdout = String::from_utf8_lossy(&output.stdout); - let Some(first_line) = stdout.lines().next() else { - return Err("git ls-remote returned empty output for marketplace source".to_string()); - }; - let Some((revision, _)) = first_line.split_once('\t') else { - return Err(format!( - "unexpected git ls-remote output for marketplace source: {first_line}" - )); - }; - let revision = revision.trim(); - if revision.is_empty() { - return Err("git ls-remote returned empty revision for marketplace source".to_string()); - } - Ok(revision.to_string()) -} - -fn is_full_git_sha(value: &str) -> bool { - value.len() == 40 && value.chars().all(|ch| ch.is_ascii_hexdigit()) -} - -fn clone_git_source( - source: &str, - ref_name: Option<&str>, - sparse_paths: &[String], - destination: &Path, - timeout: Duration, +fn ensure_configured_git_marketplace_unchanged( + codex_home: &Path, + expected: &ConfiguredGitMarketplace, ) -> Result<(), String> { - if sparse_paths.is_empty() { - let output = run_git_command_with_timeout( - Command::new("git") - .env("GIT_OPTIONAL_LOCKS", "0") - .arg("clone") - .arg(source) - .arg(destination), - "git clone marketplace source", - timeout, - )?; - ensure_git_success(&output, "git clone marketplace source")?; - if let Some(ref_name) = ref_name { - let output = run_git_command_with_timeout( - Command::new("git") - .env("GIT_OPTIONAL_LOCKS", "0") - .arg("-C") - .arg(destination) - .arg("checkout") - .arg(ref_name), - "git checkout marketplace ref", - timeout, - )?; - ensure_git_success(&output, "git checkout marketplace ref")?; + let current = read_configured_git_marketplace(codex_home, &expected.name)?; + match current { + Some(current) if current == *expected => Ok(()), + Some(_) => Err(format!( + "configured marketplace `{}` changed while auto-upgrade was in flight", + expected.name + )), + None => Err(format!( + "configured marketplace `{}` was removed or is no longer a Git marketplace", + expected.name + )), + } +} + +fn read_configured_git_marketplace( + codex_home: &Path, + 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() + )); } - return Ok(()); - } - - let output = run_git_command_with_timeout( - Command::new("git") - .env("GIT_OPTIONAL_LOCKS", "0") - .arg("clone") - .arg("--filter=blob:none") - .arg("--no-checkout") - .arg(source) - .arg(destination), - "git clone marketplace source", - timeout, - )?; - ensure_git_success(&output, "git clone marketplace source")?; - - let mut sparse_checkout = Command::new("git"); - sparse_checkout - .env("GIT_OPTIONAL_LOCKS", "0") - .arg("-C") - .arg(destination) - .arg("sparse-checkout") - .arg("set") - .args(sparse_paths); - let output = run_git_command_with_timeout( - &mut sparse_checkout, - "git sparse-checkout marketplace source", - timeout, - )?; - ensure_git_success(&output, "git sparse-checkout marketplace source")?; - - let output = run_git_command_with_timeout( - Command::new("git") - .env("GIT_OPTIONAL_LOCKS", "0") - .arg("-C") - .arg(destination) - .arg("checkout") - .arg(ref_name.unwrap_or("HEAD")), - "git checkout marketplace ref", - timeout, - )?; - ensure_git_success(&output, "git checkout marketplace ref") -} - -fn activate_marketplace_root( - destination: &Path, - staged_dir: TempDir, - after_activate: impl FnOnce() -> Result<(), String>, -) -> Result<(), String> { - let staged_root = staged_dir.path(); - let Some(parent) = destination.parent() else { - return Err(format!( - "failed to determine marketplace install parent for {}", - destination.display() - )); }; - std::fs::create_dir_all(parent).map_err(|err| { + let config: toml::Value = toml::from_str(&raw_config).map_err(|err| { format!( - "failed to create marketplace install parent {}: {err}", - parent.display() + "failed to parse user config {} while checking marketplace auto-upgrade: {err}", + config_path.display() ) })?; - - if destination.exists() { - let backup_dir = tempfile::Builder::new() - .prefix("marketplace-backup-") - .tempdir_in(parent) - .map_err(|err| { - format!( - "failed to create marketplace backup directory in {}: {err}", - parent.display() - ) - })?; - let backup_root = backup_dir.path().join("root"); - std::fs::rename(destination, &backup_root).map_err(|err| { - format!( - "failed to move previous marketplace root out of the way at {}: {err}", - destination.display() - ) - })?; - - if let Err(err) = std::fs::rename(staged_root, destination) { - let rollback_result = std::fs::rename(&backup_root, destination); - return match rollback_result { - Ok(()) => Err(format!( - "failed to activate upgraded marketplace at {}: {err}", - destination.display() - )), - Err(rollback_err) => { - let backup_path = backup_dir.keep().join("root"); - Err(format!( - "failed to activate upgraded marketplace at {}: {err}; failed to restore previous marketplace root (left at {}): {rollback_err}", - destination.display(), - backup_path.display() - )) - } - }; - } - - if let Err(err) = after_activate() { - let remove_result = std::fs::remove_dir_all(destination); - let rollback_result = - remove_result.and_then(|()| std::fs::rename(&backup_root, destination)); - return match rollback_result { - Ok(()) => Err(err), - Err(rollback_err) => { - let backup_path = backup_dir.keep().join("root"); - Err(format!( - "{err}; failed to restore previous marketplace root at {} (left at {}): {rollback_err}", - destination.display(), - backup_path.display() - )) - } - }; - } - } else { - std::fs::rename(staged_root, destination).map_err(|err| { - format!( - "failed to activate upgraded marketplace at {}: {err}", - destination.display() - ) - })?; - if let Err(err) = after_activate() { - let remove_result = std::fs::remove_dir_all(destination); - return match remove_result { - Ok(()) => Err(err), - Err(remove_err) => Err(format!( - "{err}; failed to remove newly activated marketplace root at {}: {remove_err}", - destination.display() - )), - }; - } - } - - Ok(()) -} - -fn run_git_command_with_timeout( - command: &mut Command, - context: &str, - timeout: Duration, -) -> Result { - let mut child = command - .stdin(Stdio::null()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - .map_err(|err| format!("failed to run {context}: {err}"))?; - let start = std::time::Instant::now(); - loop { - match child.try_wait() { - Ok(Some(_)) => { - return child - .wait_with_output() - .map_err(|err| format!("failed to wait for {context}: {err}")); - } - Ok(None) => {} - Err(err) => return Err(format!("failed to poll {context}: {err}")), - } - - if start.elapsed() >= timeout { - let _ = child.kill(); - let output = child - .wait_with_output() - .map_err(|err| format!("failed to wait for {context} after timeout: {err}"))?; - let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); - return if stderr.is_empty() { - Err(format!("{context} timed out after {}s", timeout.as_secs())) - } else { - Err(format!( - "{context} timed out after {}s: {stderr}", - timeout.as_secs() - )) - }; - } - - std::thread::sleep(Duration::from_millis(100)); - } -} - -fn ensure_git_success(output: &Output, context: &str) -> Result<(), String> { - if output.status.success() { - return Ok(()); - } - let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); - if stderr.is_empty() { - Err(format!("{context} failed with status {}", output.status)) - } else { - Err(format!( - "{context} failed with status {}: {stderr}", - output.status - )) - } + let Some(marketplaces_value) = config.get("marketplaces") else { + return Ok(None); + }; + let mut marketplaces = marketplaces_value + .clone() + .try_into::>() + .map_err(|err| format!("invalid marketplaces config while checking auto-upgrade: {err}"))?; + let Some(marketplace) = marketplaces.remove(marketplace_name) else { + return Ok(None); + }; + Ok(configured_git_marketplace_from_config( + marketplace_name.to_string(), + marketplace, + )) } #[cfg(test)] mod tests { use super::*; + use std::process::Command; + use crate::config::CONFIG_TOML_FILE; use crate::plugins::test_support::load_plugins_config; use crate::plugins::test_support::write_file; @@ -522,6 +323,7 @@ mod tests { let revision = git_output(source_repo.path(), &["rev-parse", "HEAD"]); let installed_root = marketplace_install_root(codex_home.path()).join("debug"); write_marketplace_repo(&installed_root, "debug", "old"); + write_installed_metadata(&installed_root, source_repo.path(), None, &[], &revision); write_file( &codex_home.path().join(CONFIG_TOML_FILE), &marketplace_config(source_repo.path(), &revision), @@ -543,6 +345,42 @@ mod tests { ); } + #[tokio::test] + async fn upgrade_configured_git_marketplace_reclones_when_install_metadata_differs() { + let codex_home = TempDir::new().unwrap(); + let source_repo = TempDir::new().unwrap(); + write_marketplace_repo(source_repo.path(), "debug", "new"); + init_git_repo(source_repo.path()); + let revision = git_output(source_repo.path(), &["rev-parse", "HEAD"]); + let installed_root = marketplace_install_root(codex_home.path()).join("debug"); + write_marketplace_repo(&installed_root, "debug", "old"); + write_installed_metadata(&installed_root, source_repo.path(), None, &[], &revision); + write_file( + &codex_home.path().join(CONFIG_TOML_FILE), + &marketplace_config_with_ref(source_repo.path(), &revision, &revision), + ); + + let config = load_plugins_config(codex_home.path()).await; + let outcome = upgrade_configured_git_marketplaces(codex_home.path(), &config); + + assert_eq!( + outcome, + ConfiguredMarketplaceUpgradeOutcome { + upgraded_roots: vec![ + AbsolutePathBuf::try_from( + marketplace_install_root(codex_home.path()).join("debug") + ) + .unwrap() + ], + all_succeeded: true, + } + ); + assert_eq!( + std::fs::read_to_string(installed_root.join("plugins/sample/marker.txt")).unwrap(), + "new" + ); + } + #[tokio::test] async fn upgrade_configured_git_marketplace_keeps_existing_root_on_name_mismatch() { let codex_home = TempDir::new().unwrap(); @@ -599,6 +437,45 @@ mod tests { ); } + #[tokio::test] + async fn upgrade_configured_git_marketplace_rolls_back_when_config_changes() { + let codex_home = TempDir::new().unwrap(); + let source_repo = TempDir::new().unwrap(); + write_marketplace_repo(source_repo.path(), "debug", "new"); + init_git_repo(source_repo.path()); + let changed_source_repo = TempDir::new().unwrap(); + write_marketplace_repo(changed_source_repo.path(), "debug", "changed"); + init_git_repo(changed_source_repo.path()); + let installed_root = marketplace_install_root(codex_home.path()).join("debug"); + write_marketplace_repo(&installed_root, "debug", "old"); + write_file( + &codex_home.path().join(CONFIG_TOML_FILE), + &marketplace_config(source_repo.path(), "old-revision"), + ); + let config = load_plugins_config(codex_home.path()).await; + write_file( + &codex_home.path().join(CONFIG_TOML_FILE), + &marketplace_config(changed_source_repo.path(), "changed-revision"), + ); + + let outcome = upgrade_configured_git_marketplaces(codex_home.path(), &config); + + assert_eq!( + outcome, + ConfiguredMarketplaceUpgradeOutcome { + upgraded_roots: Vec::new(), + all_succeeded: false, + } + ); + assert_eq!( + std::fs::read_to_string(installed_root.join("plugins/sample/marker.txt")).unwrap(), + "old" + ); + let config = std::fs::read_to_string(codex_home.path().join(CONFIG_TOML_FILE)).unwrap(); + assert!(config.contains(&changed_source_repo.path().display().to_string())); + assert!(config.contains(r#"last_revision = "changed-revision""#)); + } + #[tokio::test] async fn upgrade_configured_git_marketplaces_ignores_local_unconfigured_marketplace() { let codex_home = TempDir::new().unwrap(); @@ -627,13 +504,6 @@ plugins = true ); } - #[test] - fn full_git_sha_ref_is_already_a_remote_revision() { - assert!(is_full_git_sha("0123456789abcdef0123456789abcdef01234567")); - assert!(!is_full_git_sha("main")); - assert!(!is_full_git_sha("0123456")); - } - fn marketplace_config(source_repo: &Path, last_revision: &str) -> String { format!( r#"[features] @@ -649,6 +519,44 @@ source = "{}" ) } + fn marketplace_config_with_ref( + source_repo: &Path, + last_revision: &str, + ref_name: &str, + ) -> String { + format!( + r#"[features] +plugins = true + +[marketplaces.debug] +last_updated = "2026-04-10T00:00:00Z" +last_revision = "{last_revision}" +source_type = "git" +source = "{}" +ref = "{ref_name}" +"#, + source_repo.display() + ) + } + + fn write_installed_metadata( + root: &Path, + source_repo: &Path, + ref_name: Option<&str>, + sparse_paths: &[String], + revision: &str, + ) { + let marketplace = ConfiguredGitMarketplace { + name: "debug".to_string(), + source: source_repo.display().to_string(), + ref_name: ref_name.map(str::to_string), + sparse_paths: sparse_paths.to_vec(), + last_revision: Some(revision.to_string()), + }; + write_activated_marketplace_metadata(root, &marketplace, revision) + .expect("metadata should write"); + } + fn write_marketplace_repo(root: &Path, marketplace_name: &str, marker: &str) { write_file( &root.join(".agents/plugins/marketplace.json"), diff --git a/codex-rs/core/src/plugins/marketplace_upgrade/activation.rs b/codex-rs/core/src/plugins/marketplace_upgrade/activation.rs new file mode 100644 index 0000000000..f260c8dff3 --- /dev/null +++ b/codex-rs/core/src/plugins/marketplace_upgrade/activation.rs @@ -0,0 +1,167 @@ +use super::ConfiguredGitMarketplace; +use codex_config::types::MarketplaceSourceType; +use serde::Deserialize; +use serde::Serialize; +use std::path::Path; +use std::path::PathBuf; +use tempfile::TempDir; +use tracing::warn; + +const MARKETPLACE_INSTALL_METADATA_FILE: &str = ".codex-marketplace-install.json"; + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +struct ActivatedMarketplaceMetadata { + source_type: MarketplaceSourceType, + source: String, + ref_name: Option, + sparse_paths: Vec, + revision: String, +} + +pub(super) fn activated_marketplace_metadata_matches( + root: &Path, + marketplace: &ConfiguredGitMarketplace, + revision: &str, +) -> bool { + let metadata = match std::fs::read_to_string(activated_marketplace_metadata_path(root)) { + Ok(metadata) => metadata, + Err(_) => return false, + }; + let metadata = match serde_json::from_str::(&metadata) { + Ok(metadata) => metadata, + Err(err) => { + warn!( + marketplace = marketplace.name, + error = %err, + "failed to parse activated marketplace metadata" + ); + return false; + } + }; + metadata == activated_marketplace_metadata(marketplace, revision) +} + +pub(super) fn write_activated_marketplace_metadata( + root: &Path, + marketplace: &ConfiguredGitMarketplace, + revision: &str, +) -> Result<(), String> { + let metadata = activated_marketplace_metadata(marketplace, revision); + let contents = serde_json::to_string_pretty(&metadata) + .map_err(|err| format!("failed to serialize activated marketplace metadata: {err}"))?; + std::fs::write(activated_marketplace_metadata_path(root), contents) + .map_err(|err| format!("failed to write activated marketplace metadata: {err}")) +} + +pub(super) fn activate_marketplace_root( + destination: &Path, + staged_dir: TempDir, + after_activate: impl FnOnce() -> Result<(), String>, +) -> Result<(), String> { + let staged_root = staged_dir.path(); + let Some(parent) = destination.parent() else { + return Err(format!( + "failed to determine marketplace install parent for {}", + destination.display() + )); + }; + std::fs::create_dir_all(parent).map_err(|err| { + format!( + "failed to create marketplace install parent {}: {err}", + parent.display() + ) + })?; + + if destination.exists() { + let backup_dir = tempfile::Builder::new() + .prefix("marketplace-backup-") + .tempdir_in(parent) + .map_err(|err| { + format!( + "failed to create marketplace backup directory in {}: {err}", + parent.display() + ) + })?; + let backup_root = backup_dir.path().join("root"); + std::fs::rename(destination, &backup_root).map_err(|err| { + format!( + "failed to move previous marketplace root out of the way at {}: {err}", + destination.display() + ) + })?; + + if let Err(err) = std::fs::rename(staged_root, destination) { + let rollback_result = std::fs::rename(&backup_root, destination); + return match rollback_result { + Ok(()) => Err(format!( + "failed to activate upgraded marketplace at {}: {err}", + destination.display() + )), + Err(rollback_err) => { + let backup_path = backup_dir.keep().join("root"); + Err(format!( + "failed to activate upgraded marketplace at {}: {err}; failed to restore previous marketplace root (left at {}): {rollback_err}", + destination.display(), + backup_path.display() + )) + } + }; + } + + if let Err(err) = after_activate() { + let remove_result = std::fs::remove_dir_all(destination); + let rollback_result = + remove_result.and_then(|()| std::fs::rename(&backup_root, destination)); + return match rollback_result { + Ok(()) => Err(err), + Err(rollback_err) => { + let backup_path = backup_dir.keep().join("root"); + Err(format!( + "{err}; failed to restore previous marketplace root at {} (left at {}): {rollback_err}", + destination.display(), + backup_path.display() + )) + } + }; + } + + return Ok(()); + } + + std::fs::rename(staged_root, destination).map_err(|err| { + format!( + "failed to activate upgraded marketplace at {}: {err}", + destination.display() + ) + })?; + if let Err(err) = after_activate() { + let remove_result = std::fs::remove_dir_all(destination); + return match remove_result { + Ok(()) => Err(err), + Err(remove_err) => Err(format!( + "{err}; failed to remove newly activated marketplace root at {}: {remove_err}", + destination.display() + )), + }; + } + + Ok(()) +} + +fn activated_marketplace_metadata( + marketplace: &ConfiguredGitMarketplace, + revision: &str, +) -> ActivatedMarketplaceMetadata { + ActivatedMarketplaceMetadata { + source_type: MarketplaceSourceType::Git, + source: marketplace.source.clone(), + ref_name: marketplace.ref_name.clone(), + sparse_paths: marketplace.sparse_paths.clone(), + revision: revision.to_string(), + } +} + +fn activated_marketplace_metadata_path(root: &Path) -> PathBuf { + root.join(MARKETPLACE_INSTALL_METADATA_FILE) +} diff --git a/codex-rs/core/src/plugins/marketplace_upgrade/git.rs b/codex-rs/core/src/plugins/marketplace_upgrade/git.rs new file mode 100644 index 0000000000..f6c54314a8 --- /dev/null +++ b/codex-rs/core/src/plugins/marketplace_upgrade/git.rs @@ -0,0 +1,210 @@ +use std::path::Path; +use std::process::Command; +use std::process::Output; +use std::process::Stdio; +use std::time::Duration; + +pub(super) fn git_remote_revision( + source: &str, + ref_name: Option<&str>, + timeout: Duration, +) -> Result { + if let Some(ref_name) = ref_name + && is_full_git_sha(ref_name) + { + return Ok(ref_name.to_string()); + } + + let ref_name = ref_name.unwrap_or("HEAD"); + let output = run_git_command_with_timeout( + git_command().arg("ls-remote").arg(source).arg(ref_name), + "git ls-remote marketplace source", + timeout, + )?; + ensure_git_success(&output, "git ls-remote marketplace source")?; + + let stdout = String::from_utf8_lossy(&output.stdout); + let Some(first_line) = stdout.lines().next() else { + return Err("git ls-remote returned empty output for marketplace source".to_string()); + }; + let Some((revision, _)) = first_line.split_once('\t') else { + return Err(format!( + "unexpected git ls-remote output for marketplace source: {first_line}" + )); + }; + let revision = revision.trim(); + if revision.is_empty() { + return Err("git ls-remote returned empty revision for marketplace source".to_string()); + } + Ok(revision.to_string()) +} + +pub(super) fn clone_git_source( + source: &str, + ref_name: Option<&str>, + sparse_paths: &[String], + destination: &Path, + timeout: Duration, +) -> Result { + if sparse_paths.is_empty() { + let output = run_git_command_with_timeout( + git_command().arg("clone").arg(source).arg(destination), + "git clone marketplace source", + timeout, + )?; + ensure_git_success(&output, "git clone marketplace source")?; + if let Some(ref_name) = ref_name { + let output = run_git_command_with_timeout( + git_command() + .arg("-C") + .arg(destination) + .arg("checkout") + .arg(ref_name), + "git checkout marketplace ref", + timeout, + )?; + ensure_git_success(&output, "git checkout marketplace ref")?; + } + return git_worktree_revision(destination, timeout); + } + + let output = run_git_command_with_timeout( + git_command() + .arg("clone") + .arg("--filter=blob:none") + .arg("--no-checkout") + .arg(source) + .arg(destination), + "git clone marketplace source", + timeout, + )?; + ensure_git_success(&output, "git clone marketplace source")?; + + let mut sparse_checkout = git_command(); + sparse_checkout + .arg("-C") + .arg(destination) + .arg("sparse-checkout") + .arg("set") + .args(sparse_paths); + let output = run_git_command_with_timeout( + &mut sparse_checkout, + "git sparse-checkout marketplace source", + timeout, + )?; + ensure_git_success(&output, "git sparse-checkout marketplace source")?; + + let output = run_git_command_with_timeout( + git_command() + .arg("-C") + .arg(destination) + .arg("checkout") + .arg(ref_name.unwrap_or("HEAD")), + "git checkout marketplace ref", + timeout, + )?; + ensure_git_success(&output, "git checkout marketplace ref")?; + git_worktree_revision(destination, timeout) +} + +fn git_worktree_revision(destination: &Path, timeout: Duration) -> Result { + let output = run_git_command_with_timeout( + git_command() + .arg("-C") + .arg(destination) + .arg("rev-parse") + .arg("HEAD"), + "git rev-parse marketplace revision", + timeout, + )?; + ensure_git_success(&output, "git rev-parse marketplace revision")?; + + let revision = String::from_utf8_lossy(&output.stdout).trim().to_string(); + if revision.is_empty() { + Err("git rev-parse returned empty revision for marketplace source".to_string()) + } else { + Ok(revision) + } +} + +fn is_full_git_sha(value: &str) -> bool { + value.len() == 40 && value.chars().all(|ch| ch.is_ascii_hexdigit()) +} + +fn git_command() -> Command { + let mut command = Command::new("git"); + command + .env("GIT_OPTIONAL_LOCKS", "0") + .env("GIT_TERMINAL_PROMPT", "0"); + command +} + +fn run_git_command_with_timeout( + command: &mut Command, + context: &str, + timeout: Duration, +) -> Result { + let mut child = command + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .map_err(|err| format!("failed to run {context}: {err}"))?; + let start = std::time::Instant::now(); + loop { + match child.try_wait() { + Ok(Some(_)) => { + return child + .wait_with_output() + .map_err(|err| format!("failed to wait for {context}: {err}")); + } + Ok(None) => {} + Err(err) => return Err(format!("failed to poll {context}: {err}")), + } + + if start.elapsed() >= timeout { + let _ = child.kill(); + let output = child + .wait_with_output() + .map_err(|err| format!("failed to wait for {context} after timeout: {err}"))?; + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + return if stderr.is_empty() { + Err(format!("{context} timed out after {}s", timeout.as_secs())) + } else { + Err(format!( + "{context} timed out after {}s: {stderr}", + timeout.as_secs() + )) + }; + } + + std::thread::sleep(Duration::from_millis(100)); + } +} + +fn ensure_git_success(output: &Output, context: &str) -> Result<(), String> { + if output.status.success() { + return Ok(()); + } + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + if stderr.is_empty() { + Err(format!("{context} failed with status {}", output.status)) + } else { + Err(format!( + "{context} failed with status {}: {stderr}", + output.status + )) + } +} + +#[cfg(test)] +mod tests { + use super::is_full_git_sha; + + #[test] + fn full_git_sha_ref_is_already_a_remote_revision() { + assert!(is_full_git_sha("0123456789abcdef0123456789abcdef01234567")); + assert!(!is_full_git_sha("main")); + assert!(!is_full_git_sha("0123456")); + } +}