From adbc32b094ea1a6b7721fb0157f09490f5438afa Mon Sep 17 00:00:00 2001 From: xli-oai Date: Wed, 8 Apr 2026 21:41:54 -0700 Subject: [PATCH] Address marketplace add review feedback --- codex-rs/cli/src/marketplace_cmd.rs | 57 +++++++++++++--------- codex-rs/core/src/plugins/manager.rs | 29 +++++------ codex-rs/core/src/plugins/manager_tests.rs | 52 ++++++++++++++++++++ 3 files changed, 101 insertions(+), 37 deletions(-) diff --git a/codex-rs/cli/src/marketplace_cmd.rs b/codex-rs/cli/src/marketplace_cmd.rs index d68ed7b22a..1c788632d0 100644 --- a/codex-rs/cli/src/marketplace_cmd.rs +++ b/codex-rs/cli/src/marketplace_cmd.rs @@ -448,31 +448,14 @@ pub(super) fn replace_marketplace_root(staged_root: &Path, destination: &Path) - if let Some(parent) = destination.parent() { fs::create_dir_all(parent)?; } - - let backup = if destination.exists() { - let parent = destination - .parent() - .context("marketplace destination has no parent")?; - let staging_root = marketplace_staging_root(parent); - fs::create_dir_all(&staging_root)?; - let backup = tempfile::Builder::new() - .prefix("marketplace-backup-") - .tempdir_in(&staging_root)?; - let backup_root = backup.path().join("previous"); - fs::rename(destination, &backup_root)?; - Some((backup, backup_root)) - } else { - None - }; - - if let Err(err) = fs::rename(staged_root, destination) { - if let Some((_, backup_root)) = backup { - let _ = fs::rename(backup_root, destination); - } - return Err(err.into()); + if destination.exists() { + bail!( + "marketplace destination already exists: {}", + destination.display() + ); } - Ok(()) + fs::rename(staged_root, destination).map_err(Into::into) } pub(super) fn marketplace_staging_root(install_root: &Path) -> PathBuf { @@ -552,6 +535,7 @@ impl MarketplaceSource { mod tests { use super::*; use pretty_assertions::assert_eq; + use tempfile::TempDir; #[test] fn github_shorthand_parses_ref_suffix() { @@ -624,4 +608,31 @@ mod tests { } ); } + + #[test] + fn replace_marketplace_root_rejects_existing_destination() { + let temp_dir = TempDir::new().unwrap(); + let staged_root = temp_dir.path().join("staged"); + let destination = temp_dir.path().join("destination"); + fs::create_dir_all(&staged_root).unwrap(); + fs::write(staged_root.join("marker.txt"), "staged").unwrap(); + fs::create_dir_all(&destination).unwrap(); + fs::write(destination.join("marker.txt"), "installed").unwrap(); + + let err = replace_marketplace_root(&staged_root, &destination).unwrap_err(); + + assert!( + err.to_string() + .contains("marketplace destination already exists"), + "unexpected error: {err}" + ); + assert_eq!( + fs::read_to_string(staged_root.join("marker.txt")).unwrap(), + "staged" + ); + assert_eq!( + fs::read_to_string(destination.join("marker.txt")).unwrap(), + "installed" + ); + } } diff --git a/codex-rs/core/src/plugins/manager.rs b/codex-rs/core/src/plugins/manager.rs index db48f105df..de00c06061 100644 --- a/codex-rs/core/src/plugins/manager.rs +++ b/codex-rs/core/src/plugins/manager.rs @@ -1270,7 +1270,16 @@ pub fn record_installed_marketplace_root( fn installed_marketplace_roots(codex_home: &Path) -> Vec { let registry_path = marketplace_registry_path(codex_home); if registry_path.is_file() { - return installed_marketplace_roots_from_registry(®istry_path); + match installed_marketplace_roots_from_registry(®istry_path) { + Ok(roots) => return roots, + Err(err) => { + warn!( + path = %registry_path.display(), + error = %err, + "failed to read installed marketplace registry; falling back to installed marketplace directory scan" + ); + } + } } let install_root = marketplace_install_root(codex_home); @@ -1296,18 +1305,10 @@ fn marketplace_registry_path(codex_home: &Path) -> PathBuf { codex_home.join(".tmp").join(KNOWN_MARKETPLACES_FILE) } -fn installed_marketplace_roots_from_registry(registry_path: &Path) -> Vec { - let registry = match read_marketplace_registry(registry_path) { - Ok(registry) => registry, - Err(err) => { - warn!( - path = %registry_path.display(), - error = %err, - "failed to read installed marketplace registry" - ); - return Vec::new(); - } - }; +fn installed_marketplace_roots_from_registry( + registry_path: &Path, +) -> std::io::Result> { + let registry = read_marketplace_registry(registry_path)?; let mut roots = registry .marketplaces @@ -1322,7 +1323,7 @@ fn installed_marketplace_roots_from_registry(registry_path: &Path) -> Vec>(); roots.sort_unstable_by(|left, right| left.as_path().cmp(right.as_path())); - roots + Ok(roots) } fn read_marketplace_registry(path: &Path) -> std::io::Result { diff --git a/codex-rs/core/src/plugins/manager_tests.rs b/codex-rs/core/src/plugins/manager_tests.rs index 2a27e41ca7..13c3bf1c75 100644 --- a/codex-rs/core/src/plugins/manager_tests.rs +++ b/codex-rs/core/src/plugins/manager_tests.rs @@ -1568,6 +1568,58 @@ plugins = true ); } +#[tokio::test] +async fn list_marketplaces_scans_installed_roots_when_registry_is_malformed() { + let tmp = tempfile::tempdir().unwrap(); + let marketplace_root = marketplace_install_root(tmp.path()).join("debug"); + let plugin_root = marketplace_root.join("plugins/sample"); + let registry_path = tmp.path().join(".tmp/known_marketplaces.json"); + + write_file( + &tmp.path().join(CONFIG_TOML_FILE), + r#"[features] +plugins = true +"#, + ); + fs::create_dir_all(marketplace_root.join(".agents/plugins")).unwrap(); + fs::create_dir_all(plugin_root.join(".codex-plugin")).unwrap(); + fs::write( + marketplace_root.join(".agents/plugins/marketplace.json"), + r#"{ + "name": "debug", + "plugins": [ + { + "name": "sample", + "source": { + "source": "local", + "path": "./plugins/sample" + } + } + ] +}"#, + ) + .unwrap(); + fs::write( + plugin_root.join(".codex-plugin/plugin.json"), + r#"{"name":"sample"}"#, + ) + .unwrap(); + fs::write(registry_path, "{not valid json").unwrap(); + + let config = load_config(tmp.path(), tmp.path()).await; + let marketplaces = PluginsManager::new(tmp.path().to_path_buf()) + .list_marketplaces_for_config(&config, &[]) + .unwrap() + .marketplaces; + + let marketplace = marketplaces + .into_iter() + .find(|marketplace| marketplace.name == "debug") + .expect("installed marketplace should be discovered by disk scan fallback"); + + assert_eq!(marketplace.plugins[0].id, "sample@debug"); +} + #[tokio::test] async fn list_marketplaces_uses_first_duplicate_plugin_entry() { let tmp = tempfile::tempdir().unwrap();