From 5818ed6660e5e79234650538923cc34e01409367 Mon Sep 17 00:00:00 2001 From: xli-oai Date: Thu, 16 Apr 2026 17:06:34 -0700 Subject: [PATCH] Move marketplace add under plugin command (#18116) ## Summary - move the marketplace add CLI from `codex marketplace add` to `codex plugin marketplace add` - keep marketplace config overrides working through the nested plugin command - reject `--sparse` for local marketplace directory sources before the local-source install path bypasses git-source validation ## Validation - `just fmt` - `git diff --check` - `cargo test -p codex-cli` - `cargo test -p codex-core marketplace_add -- --nocapture` - `cargo test -p codex-core install_plugin_updates_config_with_relative_path_and_plugin_key -- --nocapture` - `xli-test-marketplace-cli` local isolated matrix: `T1`, `L1`-`L10` --- codex-rs/cli/src/main.rs | 68 +++++++++++++++++--- codex-rs/cli/tests/marketplace_add.rs | 33 +++++++++- codex-rs/cli/tests/marketplace_upgrade.rs | 36 +++++++++++ codex-rs/core/src/plugins/marketplace_add.rs | 37 +++++++++++ 4 files changed, 163 insertions(+), 11 deletions(-) create mode 100644 codex-rs/cli/tests/marketplace_upgrade.rs diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 25bf80a6e..32fb25ad3 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -111,8 +111,8 @@ enum Subcommand { /// Manage external MCP servers for Codex. Mcp(McpCli), - /// Manage plugin marketplaces for Codex. - Marketplace(MarketplaceCli), + /// Manage Codex plugins. + Plugin(PluginCli), /// Start Codex as an MCP server (stdio). McpServer, @@ -170,6 +170,21 @@ enum Subcommand { Features(FeaturesCli), } +#[derive(Debug, Parser)] +struct PluginCli { + #[clap(flatten)] + pub config_overrides: CliConfigOverrides, + + #[command(subcommand)] + subcommand: PluginSubcommand, +} + +#[derive(Debug, clap::Subcommand)] +enum PluginSubcommand { + /// Manage plugin marketplaces for Codex. + Marketplace(MarketplaceCli), +} + #[derive(Debug, Parser)] struct CompletionCommand { /// Shell to generate completions for @@ -726,17 +741,23 @@ async fn cli_main(arg0_paths: Arg0DispatchPaths) -> anyhow::Result<()> { prepend_config_flags(&mut mcp_cli.config_overrides, root_config_overrides.clone()); mcp_cli.run().await?; } - Some(Subcommand::Marketplace(mut marketplace_cli)) => { + Some(Subcommand::Plugin(plugin_cli)) => { reject_remote_mode_for_subcommand( root_remote.as_deref(), root_remote_auth_token_env.as_deref(), - "marketplace", + "plugin", )?; - prepend_config_flags( - &mut marketplace_cli.config_overrides, - root_config_overrides.clone(), - ); - marketplace_cli.run().await?; + let PluginCli { + mut config_overrides, + subcommand, + } = plugin_cli; + prepend_config_flags(&mut config_overrides, root_config_overrides.clone()); + match subcommand { + PluginSubcommand::Marketplace(mut marketplace_cli) => { + prepend_config_flags(&mut marketplace_cli.config_overrides, config_overrides); + marketplace_cli.run().await?; + } + } } Some(Subcommand::AppServer(app_server_cli)) => { let AppServerCommand { @@ -1690,6 +1711,35 @@ mod tests { assert!(matches!(cli.subcommand, Some(Subcommand::Responses(_)))); } + #[test] + fn plugin_marketplace_add_parses_under_plugin() { + let cli = + MultitoolCli::try_parse_from(["codex", "plugin", "marketplace", "add", "owner/repo"]) + .expect("parse"); + + assert!(matches!(cli.subcommand, Some(Subcommand::Plugin(_)))); + } + + #[test] + fn plugin_marketplace_upgrade_parses_under_plugin() { + let cli = + MultitoolCli::try_parse_from(["codex", "plugin", "marketplace", "upgrade", "debug"]) + .expect("parse"); + + assert!(matches!(cli.subcommand, Some(Subcommand::Plugin(_)))); + } + + #[test] + fn marketplace_no_longer_parses_at_top_level() { + let add_result = + MultitoolCli::try_parse_from(["codex", "marketplace", "add", "owner/repo"]); + assert!(add_result.is_err()); + + let upgrade_result = + MultitoolCli::try_parse_from(["codex", "marketplace", "upgrade", "debug"]); + assert!(upgrade_result.is_err()); + } + fn sample_exit_info(conversation_id: Option<&str>, thread_name: Option<&str>) -> AppExitInfo { let token_usage = TokenUsage { output_tokens: 2, diff --git a/codex-rs/cli/tests/marketplace_add.rs b/codex-rs/cli/tests/marketplace_add.rs index e04cfb579..e4256f52b 100644 --- a/codex-rs/cli/tests/marketplace_add.rs +++ b/codex-rs/cli/tests/marketplace_add.rs @@ -48,7 +48,7 @@ async fn marketplace_add_local_directory_source() -> Result<()> { codex_command(codex_home.path())? .current_dir(source_parent) - .args(["marketplace", "add", source_arg.as_str()]) + .args(["plugin", "marketplace", "add", source_arg.as_str()]) .assert() .success(); @@ -78,7 +78,12 @@ async fn marketplace_add_rejects_local_manifest_file_source() -> Result<()> { let manifest_path = source.path().join(".agents/plugins/marketplace.json"); codex_command(codex_home.path())? - .args(["marketplace", "add", manifest_path.to_str().unwrap()]) + .args([ + "plugin", + "marketplace", + "add", + manifest_path.to_str().unwrap(), + ]) .assert() .failure() .stderr(contains( @@ -87,3 +92,27 @@ async fn marketplace_add_rejects_local_manifest_file_source() -> Result<()> { Ok(()) } + +#[tokio::test] +async fn marketplace_add_rejects_sparse_for_local_directory_source() -> Result<()> { + let codex_home = TempDir::new()?; + let source = TempDir::new()?; + write_marketplace_source(source.path(), "local ref")?; + + codex_command(codex_home.path())? + .args([ + "plugin", + "marketplace", + "add", + "--sparse", + ".agents", + source.path().to_str().unwrap(), + ]) + .assert() + .failure() + .stderr(contains( + "--sparse is only supported for git marketplace sources", + )); + + Ok(()) +} diff --git a/codex-rs/cli/tests/marketplace_upgrade.rs b/codex-rs/cli/tests/marketplace_upgrade.rs new file mode 100644 index 000000000..081203ebe --- /dev/null +++ b/codex-rs/cli/tests/marketplace_upgrade.rs @@ -0,0 +1,36 @@ +use anyhow::Result; +use predicates::str::contains; +use std::path::Path; +use tempfile::TempDir; + +fn codex_command(codex_home: &Path) -> Result { + let mut cmd = assert_cmd::Command::new(codex_utils_cargo_bin::cargo_bin("codex")?); + cmd.env("CODEX_HOME", codex_home); + Ok(cmd) +} + +#[tokio::test] +async fn marketplace_upgrade_runs_under_plugin() -> Result<()> { + let codex_home = TempDir::new()?; + + codex_command(codex_home.path())? + .args(["plugin", "marketplace", "upgrade"]) + .assert() + .success() + .stdout(contains("No configured Git marketplaces to upgrade.")); + + Ok(()) +} + +#[tokio::test] +async fn marketplace_upgrade_no_longer_runs_at_top_level() -> Result<()> { + let codex_home = TempDir::new()?; + + codex_command(codex_home.path())? + .args(["marketplace", "upgrade"]) + .assert() + .failure() + .stderr(contains("unexpected argument 'upgrade' found")); + + Ok(()) +} diff --git a/codex-rs/core/src/plugins/marketplace_add.rs b/codex-rs/core/src/plugins/marketplace_add.rs index 38fc04c19..3ea044583 100644 --- a/codex-rs/core/src/plugins/marketplace_add.rs +++ b/codex-rs/core/src/plugins/marketplace_add.rs @@ -77,6 +77,11 @@ where sparse_paths, } = request; let source = parse_marketplace_source(&source, ref_name)?; + if !sparse_paths.is_empty() && !matches!(source, MarketplaceSource::Git { .. }) { + return Err(MarketplaceAddError::InvalidRequest( + "--sparse is only supported for git marketplace sources".to_string(), + )); + } let install_root = marketplace_install_root(codex_home); fs::create_dir_all(&install_root).map_err(|err| { @@ -287,6 +292,38 @@ mod tests { Ok(()) } + #[test] + fn add_marketplace_sync_rejects_sparse_checkout_for_local_directory_source() -> Result<()> { + let codex_home = TempDir::new()?; + let source_root = TempDir::new()?; + write_marketplace_source(source_root.path(), "local copy")?; + + let err = add_marketplace_sync_with_cloner( + codex_home.path(), + MarketplaceAddRequest { + source: source_root.path().display().to_string(), + ref_name: None, + sparse_paths: vec![".agents".to_string()], + }, + |_url, _ref_name, _sparse_paths, _destination| { + panic!("git cloner should not be called for local marketplace sources") + }, + ) + .unwrap_err(); + + assert_eq!( + err.to_string(), + "--sparse is only supported for git marketplace sources" + ); + assert!( + !codex_home + .path() + .join(codex_config::CONFIG_TOML_FILE) + .exists() + ); + Ok(()) + } + #[test] fn add_marketplace_sync_treats_existing_local_directory_source_as_already_added() -> Result<()> {