Address marketplace add review feedback

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"
+        );
+    }
 }

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<AbsolutePathBuf> {
     let registry_path = marketplace_registry_path(codex_home);
     if registry_path.is_file() {
-        return installed_marketplace_roots_from_registry(&registry_path);
+        match installed_marketplace_roots_from_registry(&registry_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<AbsolutePathBuf> {
-    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<Vec<AbsolutePathBuf>> {
+    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<Absolu
         })
         .collect::<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<KnownMarketplacesRegistry> {

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();