Handle marketplace add edge cases

codex-rs/cli/src/marketplace_cmd.rs

@@ -3,6 +3,7 @@ use anyhow::Result;
 use anyhow::bail;
 use clap::Parser;
 use codex_core::config::find_codex_home;
+use codex_core::plugins::OPENAI_CURATED_MARKETPLACE_NAME;
 use codex_core::plugins::marketplace_install_root;
 use codex_core::plugins::validate_marketplace_root;
 use codex_utils_cli::CliConfigOverrides;
@@ -11,6 +12,8 @@ use std::path::Path;
 use std::path::PathBuf;
 use std::process::Command;
 
+mod metadata;
+
 #[derive(Debug, Parser)]
 pub struct MarketplaceCli {
     #[clap(flatten)]
@@ -41,11 +44,15 @@ struct AddMarketplaceArgs {
 }
 
 #[derive(Debug, PartialEq, Eq)]
-enum MarketplaceSource {
-    LocalDirectory(PathBuf),
+pub(super) enum MarketplaceSource {
+    LocalDirectory {
+        path: PathBuf,
+        source_id: String,
+    },
     Git {
         url: String,
         ref_name: Option<String>,
+        source_id: String,
     },
 }
 
@@ -90,20 +97,45 @@ async fn run_add(args: AddMarketplaceArgs) -> Result<()> {
             install_root.display()
         )
     })?;
+    let install_metadata =
+        metadata::MarketplaceInstallMetadata::from_source(&source, &sparse_paths);
+    if let Some(existing_root) =
+        metadata::installed_marketplace_root_for_source(&install_root, &install_metadata.source_id)?
+    {
+        let marketplace_name = validate_marketplace_root(&existing_root).with_context(|| {
+            format!(
+                "failed to validate installed marketplace at {}",
+                existing_root.display()
+            )
+        })?;
+        println!(
+            "Marketplace `{marketplace_name}` is already added from {}.",
+            source.display()
+        );
+        println!("Installed marketplace root: {}", existing_root.display());
+        return Ok(());
+    }
 
+    let staging_root = marketplace_staging_root(&install_root);
+    fs::create_dir_all(&staging_root).with_context(|| {
+        format!(
+            "failed to create marketplace staging directory {}",
+            staging_root.display()
+        )
+    })?;
     let staged_dir = tempfile::Builder::new()
         .prefix("marketplace-add-")
-        .tempdir_in(&install_root)
+        .tempdir_in(&staging_root)
         .with_context(|| {
             format!(
                 "failed to create temporary marketplace directory in {}",
-                install_root.display()
+                staging_root.display()
             )
         })?;
     let staged_root = staged_dir.path().to_path_buf();
 
     match &source {
-        MarketplaceSource::LocalDirectory(path) => {
+        MarketplaceSource::LocalDirectory { path, .. } => {
             copy_dir_recursive(path, &staged_root).with_context(|| {
                 format!(
                     "failed to copy marketplace source {} into {}",
@@ -112,14 +144,22 @@ async fn run_add(args: AddMarketplaceArgs) -> Result<()> {
                 )
             })?;
         }
-        MarketplaceSource::Git { url, ref_name } => {
+        MarketplaceSource::Git { url, ref_name, .. } => {
             clone_git_source(url, ref_name.as_deref(), &sparse_paths, &staged_root)?;
         }
     }
 
     let marketplace_name = validate_marketplace_root(&staged_root)
         .with_context(|| format!("failed to validate marketplace from {}", source.display()))?;
+    if marketplace_name == OPENAI_CURATED_MARKETPLACE_NAME {
+        bail!(
+            "marketplace `{OPENAI_CURATED_MARKETPLACE_NAME}` is reserved and cannot be added from {}",
+            source.display()
+        );
+    }
+    metadata::write_marketplace_source_metadata(&staged_root, &install_metadata)?;
     let destination = install_root.join(safe_marketplace_dir_name(&marketplace_name)?);
+    ensure_marketplace_destination_is_inside_install_root(&install_root, &destination)?;
     replace_marketplace_root(&staged_root, &destination)
         .with_context(|| format!("failed to install marketplace at {}", destination.display()))?;
 
@@ -143,38 +183,69 @@ fn parse_marketplace_source(
 
     let source = expand_home(source);
     let path = PathBuf::from(&source);
-    if path.exists() || looks_like_local_path(&source) {
-        if !path.exists() {
+    let path_exists = path.try_exists().with_context(|| {
+        format!(
+            "failed to access local marketplace source {}",
+            path.display()
+        )
+    })?;
+    if path_exists || looks_like_local_path(&source) {
+        if !path_exists {
             bail!(
                 "local marketplace source does not exist: {}",
                 path.display()
             );
         }
-        if !path.is_dir() {
+        let metadata = path.metadata().with_context(|| {
+            format!("failed to read local marketplace source {}", path.display())
+        })?;
+        if metadata.is_file() {
+            if path
+                .extension()
+                .is_some_and(|extension| extension == "json")
+            {
+                bail!(
+                    "local marketplace JSON files are not supported yet; pass the marketplace root directory containing .agents/plugins/marketplace.json: {}",
+                    path.display()
+                );
+            }
             bail!(
-                "local marketplace source must be a directory containing .agents/plugins/marketplace.json: {}",
+                "local marketplace source file must be a JSON marketplace manifest or a directory containing .agents/plugins/marketplace.json: {}",
+                path.display()
+            );
+        }
+        if !metadata.is_dir() {
+            bail!(
+                "local marketplace source must be a file or directory: {}",
                 path.display()
             );
         }
         let path = path
             .canonicalize()
             .with_context(|| format!("failed to resolve {}", path.display()))?;
-        return Ok(MarketplaceSource::LocalDirectory(path));
+        return Ok(MarketplaceSource::LocalDirectory {
+            source_id: format!("directory:{}", path.display()),
+            path,
+        });
     }
 
     let (base_source, parsed_ref) = split_source_ref(&source);
     let ref_name = explicit_ref.or(parsed_ref);
 
     if is_ssh_git_url(&base_source) || is_http_git_url(&base_source) {
+        let url = normalize_git_url(&base_source);
         return Ok(MarketplaceSource::Git {
-            url: normalize_git_url(&base_source),
+            source_id: git_source_id("git", &url, ref_name.as_deref()),
+            url,
             ref_name,
         });
     }
 
     if looks_like_github_shorthand(&base_source) {
+        let url = format!("https://github.com/{base_source}.git");
         return Ok(MarketplaceSource::Git {
-            url: format!("https://github.com/{base_source}.git"),
+            source_id: git_source_id("github", &base_source, ref_name.as_deref()),
+            url,
             ref_name,
         });
     }
@@ -188,6 +259,14 @@ fn parse_marketplace_source(
     bail!("invalid marketplace source format: {source}");
 }
 
+fn git_source_id(kind: &str, source: &str, ref_name: Option<&str>) -> String {
+    if let Some(ref_name) = ref_name {
+        format!("{kind}:{source}#{ref_name}")
+    } else {
+        format!("{kind}:{source}")
+    }
+}
+
 fn split_source_ref(source: &str) -> (String, Option<String>) {
     if let Some((base, ref_name)) = source.rsplit_once('#') {
         return (base.to_string(), non_empty_ref(ref_name));
@@ -259,7 +338,7 @@ fn is_github_shorthand_segment(segment: &str) -> bool {
             .all(|ch| ch.is_ascii_alphanumeric() || matches!(ch, '-' | '_' | '.'))
 }
 
-fn clone_git_source(
+pub(super) fn clone_git_source(
     url: &str,
     ref_name: Option<&str>,
     sparse_paths: &[String],
@@ -292,9 +371,10 @@ fn clone_git_source(
     Ok(())
 }
 
-fn run_git(args: &[&str], cwd: Option<&Path>) -> Result<()> {
+pub(super) fn run_git(args: &[&str], cwd: Option<&Path>) -> Result<()> {
     let mut command = Command::new("git");
     command.args(args);
+    command.env("GIT_TERMINAL_PROMPT", "0");
     if let Some(cwd) = cwd {
         command.current_dir(cwd);
     }
@@ -317,7 +397,7 @@ fn run_git(args: &[&str], cwd: Option<&Path>) -> Result<()> {
     );
 }
 
-fn copy_dir_recursive(source: &Path, target: &Path) -> Result<()> {
+pub(super) fn copy_dir_recursive(source: &Path, target: &Path) -> Result<()> {
     fs::create_dir_all(target)?;
     for entry in fs::read_dir(source)? {
         let entry = entry?;
@@ -355,19 +435,20 @@ fn copy_symlink_target(source: &Path, target: &Path) -> Result<()> {
     }
 }
 
-fn replace_marketplace_root(staged_root: &Path, destination: &Path) -> Result<()> {
+pub(super) fn replace_marketplace_root(staged_root: &Path, destination: &Path) -> Result<()> {
     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(
-                destination
-                    .parent()
-                    .context("marketplace destination has no parent")?,
-            )?;
+            .tempdir_in(&staging_root)?;
         let backup_root = backup.path().join("previous");
         fs::rename(destination, &backup_root)?;
         Some((backup, backup_root))
@@ -385,6 +466,10 @@ fn replace_marketplace_root(staged_root: &Path, destination: &Path) -> Result<()
     Ok(())
 }
 
+pub(super) fn marketplace_staging_root(install_root: &Path) -> PathBuf {
+    install_root.join(".staging")
+}
+
 fn safe_marketplace_dir_name(marketplace_name: &str) -> Result<String> {
     let safe = marketplace_name
         .chars()
@@ -403,11 +488,47 @@ fn safe_marketplace_dir_name(marketplace_name: &str) -> Result<String> {
     Ok(safe)
 }
 
+fn ensure_marketplace_destination_is_inside_install_root(
+    install_root: &Path,
+    destination: &Path,
+) -> Result<()> {
+    let install_root = install_root.canonicalize().with_context(|| {
+        format!(
+            "failed to resolve marketplace install root {}",
+            install_root.display()
+        )
+    })?;
+    let destination_parent = destination
+        .parent()
+        .context("marketplace destination has no parent")?
+        .canonicalize()
+        .with_context(|| {
+            format!(
+                "failed to resolve marketplace destination parent {}",
+                destination.display()
+            )
+        })?;
+    if !destination_parent.starts_with(&install_root) {
+        bail!(
+            "marketplace destination {} is outside install root {}",
+            destination.display(),
+            install_root.display()
+        );
+    }
+    Ok(())
+}
+
 impl MarketplaceSource {
+    fn source_id(&self) -> &str {
+        match self {
+            Self::LocalDirectory { source_id, .. } | Self::Git { source_id, .. } => source_id,
+        }
+    }
+
     fn display(&self) -> String {
         match self {
-            Self::LocalDirectory(path) => path.display().to_string(),
-            Self::Git { url, ref_name } => {
+            Self::LocalDirectory { path, .. } => path.display().to_string(),
+            Self::Git { url, ref_name, .. } => {
                 if let Some(ref_name) = ref_name {
                     format!("{url}#{ref_name}")
                 } else {
@@ -430,6 +551,7 @@ mod tests {
             MarketplaceSource::Git {
                 url: "https://github.com/owner/repo.git".to_string(),
                 ref_name: Some("main".to_string()),
+                source_id: "github:owner/repo#main".to_string(),
             }
         );
     }
@@ -445,6 +567,7 @@ mod tests {
             MarketplaceSource::Git {
                 url: "https://example.com/team/repo.git".to_string(),
                 ref_name: Some("v1".to_string()),
+                source_id: "git:https://example.com/team/repo.git#v1".to_string(),
             }
         );
     }
@@ -460,6 +583,35 @@ mod tests {
             MarketplaceSource::Git {
                 url: "https://github.com/owner/repo.git".to_string(),
                 ref_name: Some("release".to_string()),
+                source_id: "github:owner/repo#release".to_string(),
+            }
+        );
+    }
+
+    #[test]
+    fn github_shorthand_and_git_url_have_different_source_ids() {
+        let shorthand = parse_marketplace_source("owner/repo", /* explicit_ref */ None).unwrap();
+        let git_url = parse_marketplace_source(
+            "https://github.com/owner/repo.git",
+            /* explicit_ref */ None,
+        )
+        .unwrap();
+
+        assert_ne!(shorthand.source_id(), git_url.source_id());
+        assert_eq!(
+            shorthand,
+            MarketplaceSource::Git {
+                url: "https://github.com/owner/repo.git".to_string(),
+                ref_name: None,
+                source_id: "github:owner/repo".to_string(),
+            }
+        );
+        assert_eq!(
+            git_url,
+            MarketplaceSource::Git {
+                url: "https://github.com/owner/repo.git".to_string(),
+                ref_name: None,
+                source_id: "git:https://github.com/owner/repo.git".to_string(),
             }
         );
     }

codex-rs/cli/src/marketplace_cmd/metadata.rs

@@ -0,0 +1,191 @@
+use super::MarketplaceSource;
+use anyhow::Context;
+use anyhow::Result;
+use anyhow::bail;
+use codex_core::plugins::validate_marketplace_root;
+use std::fs;
+use std::path::Path;
+use std::path::PathBuf;
+
+const MARKETPLACE_ADD_SOURCE_FILE: &str = ".codex-marketplace-source";
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub(super) struct MarketplaceInstallMetadata {
+    pub(super) source_id: String,
+    source: InstalledMarketplaceSource,
+}
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub(super) enum InstalledMarketplaceSource {
+    LocalDirectory {
+        path: PathBuf,
+    },
+    Git {
+        url: String,
+        ref_name: Option<String>,
+        sparse_paths: Vec<String>,
+    },
+}
+
+pub(super) fn installed_marketplace_root_for_source(
+    install_root: &Path,
+    source_id: &str,
+) -> Result<Option<PathBuf>> {
+    let entries = fs::read_dir(install_root).with_context(|| {
+        format!(
+            "failed to read marketplace install directory {}",
+            install_root.display()
+        )
+    })?;
+    for entry in entries {
+        let entry = entry?;
+        if !entry.file_type()?.is_dir() {
+            continue;
+        }
+        let root = entry.path();
+        let metadata_path = root.join(MARKETPLACE_ADD_SOURCE_FILE);
+        if !metadata_path.is_file() {
+            continue;
+        }
+        let metadata = read_marketplace_source_metadata(&root)?;
+        if metadata.source_id == source_id && validate_marketplace_root(&root).is_ok() {
+            return Ok(Some(root));
+        }
+    }
+    Ok(None)
+}
+
+pub(super) fn write_marketplace_source_metadata(
+    root: &Path,
+    metadata: &MarketplaceInstallMetadata,
+) -> Result<()> {
+    let source = match &metadata.source {
+        InstalledMarketplaceSource::LocalDirectory { path } => serde_json::json!({
+            "kind": "directory",
+            "path": path,
+        }),
+        InstalledMarketplaceSource::Git {
+            url,
+            ref_name,
+            sparse_paths,
+        } => serde_json::json!({
+            "kind": "git",
+            "url": url,
+            "ref": ref_name,
+            "sparsePaths": sparse_paths,
+        }),
+    };
+    let content = serde_json::to_string_pretty(&serde_json::json!({
+        "version": 1,
+        "sourceId": metadata.source_id,
+        "source": source,
+    }))?;
+    fs::write(root.join(MARKETPLACE_ADD_SOURCE_FILE), content).with_context(|| {
+        format!(
+            "failed to write marketplace source metadata in {}",
+            root.display()
+        )
+    })
+}
+
+fn read_marketplace_source_metadata(root: &Path) -> Result<MarketplaceInstallMetadata> {
+    let path = root.join(MARKETPLACE_ADD_SOURCE_FILE);
+    let content = fs::read_to_string(&path).with_context(|| {
+        format!(
+            "failed to read marketplace source metadata {}",
+            path.display()
+        )
+    })?;
+    if !content.trim_start().starts_with('{') {
+        return Ok(MarketplaceInstallMetadata {
+            source_id: content.trim().to_string(),
+            source: InstalledMarketplaceSource::LocalDirectory {
+                path: root.to_path_buf(),
+            },
+        });
+    }
+
+    let json: serde_json::Value = serde_json::from_str(&content).with_context(|| {
+        format!(
+            "failed to parse marketplace source metadata {}",
+            path.display()
+        )
+    })?;
+    let source_id = json
+        .get("sourceId")
+        .and_then(serde_json::Value::as_str)
+        .context("marketplace source metadata is missing sourceId")?
+        .to_string();
+    let source = json
+        .get("source")
+        .context("marketplace source metadata is missing source")?;
+    let kind = source
+        .get("kind")
+        .and_then(serde_json::Value::as_str)
+        .context("marketplace source metadata is missing source.kind")?;
+    let source = match kind {
+        "directory" => {
+            let path = source
+                .get("path")
+                .and_then(serde_json::Value::as_str)
+                .context("marketplace directory metadata is missing path")?;
+            InstalledMarketplaceSource::LocalDirectory {
+                path: PathBuf::from(path),
+            }
+        }
+        "git" => {
+            let url = source
+                .get("url")
+                .and_then(serde_json::Value::as_str)
+                .context("marketplace git metadata is missing url")?
+                .to_string();
+            let ref_name = source
+                .get("ref")
+                .and_then(serde_json::Value::as_str)
+                .map(str::to_string);
+            let sparse_paths = source
+                .get("sparsePaths")
+                .and_then(serde_json::Value::as_array)
+                .map(|paths| {
+                    paths
+                        .iter()
+                        .filter_map(serde_json::Value::as_str)
+                        .map(str::to_string)
+                        .collect::<Vec<_>>()
+                })
+                .unwrap_or_default();
+            InstalledMarketplaceSource::Git {
+                url,
+                ref_name,
+                sparse_paths,
+            }
+        }
+        other => bail!("unsupported marketplace source metadata kind `{other}`"),
+    };
+    Ok(MarketplaceInstallMetadata { source_id, source })
+}
+
+impl MarketplaceInstallMetadata {
+    pub(super) fn from_source(source: &MarketplaceSource, sparse_paths: &[String]) -> Self {
+        let source_id = if sparse_paths.is_empty() {
+            source.source_id().to_string()
+        } else {
+            format!(
+                "{}?sparse={}",
+                source.source_id(),
+                serde_json::to_string(sparse_paths).unwrap_or_else(|_| "[]".to_string())
+            )
+        };
+        let source = match source {
+            MarketplaceSource::LocalDirectory { path, .. } => {
+                InstalledMarketplaceSource::LocalDirectory { path: path.clone() }
+            }
+            MarketplaceSource::Git { url, ref_name, .. } => InstalledMarketplaceSource::Git {
+                url: url.clone(),
+                ref_name: ref_name.clone(),
+                sparse_paths: sparse_paths.to_vec(),
+            },
+        };
+        Self { source_id, source }
+    }
+}

codex-rs/cli/tests/marketplace_add.rs

@@ -12,14 +12,11 @@ fn codex_command(codex_home: &Path) -> Result<assert_cmd::Command> {
     Ok(cmd)
 }
 
-#[tokio::test]
-async fn marketplace_add_local_directory_installs_valid_marketplace_root() -> Result<()> {
-    let codex_home = TempDir::new()?;
-    let source = TempDir::new()?;
-    std::fs::create_dir_all(source.path().join(".agents/plugins"))?;
-    std::fs::create_dir_all(source.path().join("plugins/sample/.codex-plugin"))?;
+fn write_marketplace_source(source: &Path, marker: &str) -> Result<()> {
+    std::fs::create_dir_all(source.join(".agents/plugins"))?;
+    std::fs::create_dir_all(source.join("plugins/sample/.codex-plugin"))?;
     std::fs::write(
-        source.path().join(".agents/plugins/marketplace.json"),
+        source.join(".agents/plugins/marketplace.json"),
         r#"{
   "name": "debug",
   "plugins": [
@@ -34,11 +31,18 @@ async fn marketplace_add_local_directory_installs_valid_marketplace_root() -> Re
 }"#,
     )?;
     std::fs::write(
-        source
-            .path()
-            .join("plugins/sample/.codex-plugin/plugin.json"),
+        source.join("plugins/sample/.codex-plugin/plugin.json"),
         r#"{"name":"sample"}"#,
     )?;
+    std::fs::write(source.join("plugins/sample/marker.txt"), marker)?;
+    Ok(())
+}
+
+#[tokio::test]
+async fn marketplace_add_local_directory_installs_valid_marketplace_root() -> Result<()> {
+    let codex_home = TempDir::new()?;
+    let source = TempDir::new()?;
+    write_marketplace_source(source.path(), "first install")?;
 
     let mut add_cmd = codex_command(codex_home.path())?;
     add_cmd
@@ -57,3 +61,35 @@ async fn marketplace_add_local_directory_installs_valid_marketplace_root() -> Re
 
     Ok(())
 }
+
+#[tokio::test]
+async fn marketplace_add_same_source_is_idempotent() -> Result<()> {
+    let codex_home = TempDir::new()?;
+    let source = TempDir::new()?;
+    write_marketplace_source(source.path(), "first install")?;
+
+    codex_command(codex_home.path())?
+        .args(["marketplace", "add", source.path().to_str().unwrap()])
+        .assert()
+        .success()
+        .stdout(contains("Added marketplace `debug`"));
+
+    std::fs::write(
+        source.path().join("plugins/sample/marker.txt"),
+        "source changed after add",
+    )?;
+
+    codex_command(codex_home.path())?
+        .args(["marketplace", "add", source.path().to_str().unwrap()])
+        .assert()
+        .success()
+        .stdout(contains("Marketplace `debug` is already added"));
+
+    let installed_root = marketplace_install_root(codex_home.path()).join("debug");
+    assert_eq!(
+        std::fs::read_to_string(installed_root.join("plugins/sample/marker.txt"))?,
+        "first install"
+    );
+
+    Ok(())
+}