From 3075061bdd410c02892203523c30feafc111deb7 Mon Sep 17 00:00:00 2001 From: Casey Chow Date: Wed, 20 May 2026 18:04:49 -0400 Subject: [PATCH] feat(plugins): tabulate plugin list output (#23727) ## Summary - render `codex plugin list` as one table per marketplace with the marketplace manifest path shown above each table - surface the installed plugin version in the CLI output by threading `installed_version` through marketplace listing state - narrow the system-root exemption so only known bundled/runtime marketplaces skip missing-manifest failures, and keep `VERSION` empty for cached-but-unconfigured plugins ## Rationale The plugin list UX was hard to scan as a flat list and did not show which installed version was active. This change makes the CLI output easier to read in the real multi-marketplace case, keeps the plugin path visible, fixes the Sapphire regression where bundled/runtime marketplace roots were blocking `plugin list`, and addresses the two review findings that came out of the follow-up deep review. ## Key Decisions - kept the CLI output grouped per marketplace instead of one global table so the marketplace path can live with the rows it owns - kept `VERSION` as the installed version, which means it is empty until a plugin is actually installed - handled the bundled/runtime regression in the CLI snapshot validation path rather than widening app-server protocol or changing marketplace loading behavior - narrowed the exemption to known system marketplace names plus expected system paths, so user-configured marketplaces under those directories still fail loudly - gated `installed_version` on actual installed state so `VERSION` cannot show stale cache state for `not installed` rows ## Validation - `just fmt` - Sapphire: `cargo test -p codex-cli --test plugin_cli` (`14 passed; 0 failed`) - Sapphire smoke test: bundled/runtime roots still work - `cargo run -q -p codex-cli -- plugin add sample@debug` - `cargo run -q -p codex-cli -- plugin list` - verified the bundled/runtime-root scenario no longer errors and shows the expected marketplace table output - Sapphire smoke test: custom marketplace under bundled path still errors - verified `failed to load configured marketplace snapshot(s)` for `custom-marketplace` - Sapphire smoke test: cached-but-unconfigured plugin hides version - verified `sample@debug not installed` renders with an empty `VERSION` column ## Sample Output ```text /tmp/custom-marketplace/plugin.json NAME VERSION STATUS DESCRIPTION sample@debug 1.0.0 enabled Debug sample plugin other@local not installed Local development plugin ``` --- codex-rs/cli/src/plugin_cmd.rs | 114 ++++++++++- codex-rs/cli/tests/plugin_cli.rs | 218 +++++++++++++++++++-- codex-rs/core-plugins/src/manager.rs | 23 ++- codex-rs/core-plugins/src/manager_tests.rs | 8 + 4 files changed, 336 insertions(+), 27 deletions(-) diff --git a/codex-rs/cli/src/plugin_cmd.rs b/codex-rs/cli/src/plugin_cmd.rs index 593fe644b..b0574c091 100644 --- a/codex-rs/cli/src/plugin_cmd.rs +++ b/codex-rs/cli/src/plugin_cmd.rs @@ -5,6 +5,7 @@ use clap::Parser; use codex_core::config::Config; use codex_core::config::find_codex_home; use codex_core_plugins::ConfiguredMarketplace; +use codex_core_plugins::OPENAI_BUNDLED_MARKETPLACE_NAME; use codex_core_plugins::PluginInstallRequest; use codex_core_plugins::PluginsConfigInput; use codex_core_plugins::PluginsManager; @@ -15,10 +16,14 @@ use codex_core_plugins::marketplace::find_marketplace_manifest_path; use codex_plugin::PluginId; use codex_plugin::validate_plugin_segment; use codex_utils_cli::CliConfigOverrides; +use std::path::Path; use std::path::PathBuf; use crate::marketplace_cmd::MarketplaceCli; +const OPENAI_BUNDLED_ALPHA_MARKETPLACE_NAME: &str = "openai-bundled-alpha"; +const OPENAI_PRIMARY_RUNTIME_MARKETPLACE_NAME: &str = "openai-primary-runtime"; + #[derive(Debug, Parser)] #[command(bin_name = "codex plugin")] pub struct PluginCli { @@ -168,9 +173,13 @@ pub async fn run_plugin_list( println!("No marketplace plugins found."); } } else { - for marketplace in marketplaces { - println!("Marketplace `{}`", marketplace.name); - println!("Path: {}", marketplace.path.as_path().display()); + for (index, marketplace) in marketplaces.into_iter().enumerate() { + let mut rows = Vec::new(); + let mut plugin_width = "PLUGIN".len(); + let mut status_width = "STATUS".len(); + let mut installed_version_width = "VERSION".len(); + let mut path_width = "PATH".len(); + for plugin in &marketplace.plugins { let state = if plugin.installed && plugin.enabled { "installed, enabled" @@ -179,7 +188,51 @@ pub async fn run_plugin_list( } else { "not installed" }; - println!(" {} ({state})", plugin.id); + let installed_version = plugin.installed_version.clone().unwrap_or_default(); + let path = match &plugin.source { + codex_core_plugins::marketplace::MarketplacePluginSource::Local { path } => { + path.as_path().display().to_string() + } + codex_core_plugins::marketplace::MarketplacePluginSource::Git { + url, + path, + ref_name, + sha, + } => { + let mut parts = vec![url.clone()]; + if let Some(path) = path { + parts.push(format!("path `{path}`")); + } + if let Some(ref_name) = ref_name { + parts.push(format!("ref `{ref_name}`")); + } + if let Some(sha) = sha { + parts.push(format!("sha `{sha}`")); + } + parts.join(", ") + } + }; + plugin_width = plugin_width.max(plugin.id.len()); + status_width = status_width.max(state.len()); + installed_version_width = installed_version_width.max(installed_version.len()); + path_width = path_width.max(path.len()); + rows.push((plugin.id.clone(), state, installed_version, path)); + } + + if index > 0 { + println!(); + } + println!("Marketplace `{}`", marketplace.name); + println!("{}", marketplace.path.as_path().display()); + println!(); + println!( + "{: manifest_paths.push((configured_name.clone(), path)), - None => issues.push(ConfiguredMarketplaceSnapshotIssue { - marketplace_name: configured_name.clone(), - path: root, - message: "marketplace root does not contain a supported manifest".to_string(), - }), + None => { + if is_implicit_system_marketplace_root(configured_name, codex_home, &root) { + continue; + } + issues.push(ConfiguredMarketplaceSnapshotIssue { + marketplace_name: configured_name.clone(), + path: root, + message: "marketplace root does not contain a supported manifest".to_string(), + }); + } } } @@ -403,3 +461,41 @@ fn configured_marketplace_snapshot_issues( } issues } + +fn is_implicit_system_marketplace_root( + marketplace_name: &str, + _codex_home: &Path, + root: &Path, +) -> bool { + if matches!( + marketplace_name, + OPENAI_BUNDLED_MARKETPLACE_NAME | OPENAI_BUNDLED_ALPHA_MARKETPLACE_NAME + ) && path_ends_with(root, &[".tmp", "bundled-marketplaces", marketplace_name]) + { + return true; + } + + marketplace_name == OPENAI_PRIMARY_RUNTIME_MARKETPLACE_NAME + && path_ends_with( + root, + &[ + "codex-runtimes", + "codex-primary-runtime", + "plugins", + marketplace_name, + ], + ) +} + +fn path_ends_with(path: &Path, suffix: &[&str]) -> bool { + let path_components = path + .components() + .map(|component| component.as_os_str().to_string_lossy().into_owned()) + .collect::>(); + path_components.as_slice().ends_with( + &suffix + .iter() + .map(std::string::ToString::to_string) + .collect::>(), + ) +} diff --git a/codex-rs/cli/tests/plugin_cli.rs b/codex-rs/cli/tests/plugin_cli.rs index 9829e4df0..a50883cd7 100644 --- a/codex-rs/cli/tests/plugin_cli.rs +++ b/codex-rs/cli/tests/plugin_cli.rs @@ -41,10 +41,13 @@ plugins = true } fn write_marketplace_source(source: &Path) -> Result<()> { - std::fs::create_dir_all(source.join(".agents/plugins"))?; - std::fs::create_dir_all(source.join("plugins/sample/.codex-plugin"))?; + std::fs::create_dir_all(source.join(".agents").join("plugins"))?; + std::fs::create_dir_all(source.join("plugins").join("sample").join(".codex-plugin"))?; std::fs::write( - source.join(".agents/plugins/marketplace.json"), + source + .join(".agents") + .join("plugins") + .join("marketplace.json"), r#"{ "name": "debug", "plugins": [ @@ -59,8 +62,12 @@ fn write_marketplace_source(source: &Path) -> Result<()> { }"#, )?; std::fs::write( - source.join("plugins/sample/.codex-plugin/plugin.json"), - r#"{"name":"sample","description":"Sample plugin"}"#, + source + .join("plugins") + .join("sample") + .join(".codex-plugin") + .join("plugin.json"), + r#"{"name":"sample","version":"1.2.3","description":"Sample plugin"}"#, )?; Ok(()) } @@ -104,9 +111,13 @@ fn setup_configured_marketplace_with_malformed_manifest() -> Result<(TempDir, Te let codex_home = TempDir::new()?; let source = TempDir::new()?; write_plugins_enabled_config(codex_home.path())?; - std::fs::create_dir_all(source.path().join(".agents/plugins"))?; + std::fs::create_dir_all(source.path().join(".agents").join("plugins"))?; std::fs::write( - source.path().join(".agents/plugins/marketplace.json"), + source + .path() + .join(".agents") + .join("plugins") + .join("marketplace.json"), "{not valid json", )?; let source_path = source.path().to_string_lossy().into_owned(); @@ -118,6 +129,84 @@ fn setup_configured_marketplace_with_malformed_manifest() -> Result<(TempDir, Te Ok((codex_home, source)) } +fn setup_local_marketplace_with_implicit_system_roots() -> Result<(TempDir, TempDir, TempDir)> { + let (codex_home, source) = setup_local_marketplace()?; + + let bundled_root = codex_home + .path() + .join(".tmp") + .join("bundled-marketplaces") + .join("openai-bundled"); + std::fs::create_dir_all(&bundled_root)?; + let bundled_source = bundled_root.display().to_string(); + record_user_marketplace( + codex_home.path(), + "openai-bundled", + &configured_local_marketplace(&bundled_source), + )?; + + let cache_home = TempDir::new()?; + let runtime_root = cache_home + .path() + .join("codex-runtimes") + .join("codex-primary-runtime") + .join("plugins") + .join("openai-primary-runtime"); + std::fs::create_dir_all(&runtime_root)?; + let runtime_source = runtime_root.display().to_string(); + record_user_marketplace( + codex_home.path(), + "openai-primary-runtime", + &configured_local_marketplace(&runtime_source), + )?; + + Ok((codex_home, source, cache_home)) +} + +fn setup_custom_marketplace_under_implicit_system_root() -> Result<(TempDir, std::path::PathBuf)> { + let codex_home = TempDir::new()?; + write_plugins_enabled_config(codex_home.path())?; + + let custom_root = codex_home + .path() + .join(".tmp") + .join("bundled-marketplaces") + .join("custom-marketplace"); + std::fs::create_dir_all(&custom_root)?; + let custom_source = custom_root.display().to_string(); + record_user_marketplace( + codex_home.path(), + "custom-marketplace", + &configured_local_marketplace(&custom_source), + )?; + + Ok((codex_home, custom_root)) +} + +fn remove_installed_plugin_config(codex_home: &Path, plugin_key: &str) -> Result<()> { + let config_path = codex_home.join(CONFIG_TOML_FILE); + let plugin_header = format!("[plugins.\"{plugin_key}\"]"); + let config = std::fs::read_to_string(&config_path)?; + let mut rewritten = Vec::new(); + let mut skipping = false; + + for line in config.lines() { + if line == plugin_header { + skipping = true; + continue; + } + if skipping && line.starts_with('[') { + skipping = false; + } + if !skipping { + rewritten.push(line); + } + } + + std::fs::write(config_path, format!("{}\n", rewritten.join("\n")))?; + Ok(()) +} + #[tokio::test] async fn marketplace_list_shows_configured_marketplace_names() -> Result<()> { let (codex_home, source) = setup_local_marketplace()?; @@ -133,15 +222,48 @@ async fn marketplace_list_shows_configured_marketplace_names() -> Result<()> { } #[tokio::test] -async fn plugin_list_shows_plugins_grouped_by_marketplace() -> Result<()> { - let (codex_home, _source) = setup_local_marketplace()?; +async fn plugin_list_prints_plugins_in_a_table() -> Result<()> { + let (codex_home, source) = setup_local_marketplace()?; + let marketplace_manifest = source + .path() + .join(".agents") + .join("plugins") + .join("marketplace.json"); + let plugin_path = source.path().join("plugins").join("sample"); codex_command(codex_home.path())? .args(["plugin", "list"]) .assert() .success() .stdout(contains("Marketplace `debug`")) - .stdout(contains("sample@debug (not installed)")); + .stdout(contains("PLUGIN")) + .stdout(contains("STATUS")) + .stdout(contains("VERSION")) + .stdout(contains("PATH")) + .stdout(contains(marketplace_manifest.display().to_string())) + .stdout(contains("sample@debug")) + .stdout(contains("not installed")) + .stdout(contains(plugin_path.display().to_string())); + + Ok(()) +} + +#[tokio::test] +async fn plugin_list_shows_installed_version_when_plugin_is_installed() -> Result<()> { + let (codex_home, _source) = setup_local_marketplace()?; + + codex_command(codex_home.path())? + .args(["plugin", "add", "sample@debug"]) + .assert() + .success(); + + codex_command(codex_home.path())? + .args(["plugin", "list"]) + .assert() + .success() + .stdout(contains("sample@debug")) + .stdout(contains("1.2.3")) + .stdout(contains("installed, enabled")); Ok(()) } @@ -151,10 +273,10 @@ async fn plugin_list_excludes_unconfigured_repo_local_marketplaces() -> Result<( let (codex_home, source) = setup_unconfigured_local_marketplace()?; codex_command_in(codex_home.path(), source.path())? - .args(["plugin", "list"]) + .args(["plugin", "list", "--marketplace", "debug"]) .assert() .success() - .stdout(contains("No marketplace plugins found.")) + .stdout(contains("No plugins found in marketplace `debug`.")) .stdout(predicates::str::is_match("sample@debug").unwrap().not()); Ok(()) @@ -180,6 +302,74 @@ async fn plugin_list_fails_when_configured_marketplace_snapshot_is_missing() -> Ok(()) } +#[tokio::test] +async fn plugin_list_ignores_implicit_system_marketplace_roots_without_manifests() -> Result<()> { + let (codex_home, source, cache_home) = setup_local_marketplace_with_implicit_system_roots()?; + + codex_command(codex_home.path())? + .env("XDG_CACHE_HOME", cache_home.path()) + .args(["plugin", "list"]) + .assert() + .success() + .stdout(contains("Marketplace `debug`")) + .stdout(contains( + source + .path() + .join(".agents") + .join("plugins") + .join("marketplace.json") + .display() + .to_string(), + )) + .stderr( + predicates::str::contains("failed to load configured marketplace snapshot(s):").not(), + ); + + Ok(()) +} + +#[tokio::test] +async fn plugin_list_fails_for_custom_marketplace_under_system_root() -> Result<()> { + let (codex_home, custom_root) = setup_custom_marketplace_under_implicit_system_root()?; + + codex_command(codex_home.path())? + .args(["plugin", "list"]) + .assert() + .failure() + .stderr(contains( + "failed to load configured marketplace snapshot(s):", + )) + .stderr(contains("`custom-marketplace`")) + .stderr(contains(custom_root.display().to_string())) + .stderr(contains( + "marketplace root does not contain a supported manifest", + )); + + Ok(()) +} + +#[tokio::test] +async fn plugin_list_hides_version_for_cached_but_unconfigured_plugin() -> Result<()> { + let (codex_home, _source) = setup_local_marketplace()?; + + codex_command(codex_home.path())? + .args(["plugin", "add", "sample@debug"]) + .assert() + .success(); + + remove_installed_plugin_config(codex_home.path(), "sample@debug")?; + + codex_command(codex_home.path())? + .args(["plugin", "list"]) + .assert() + .success() + .stdout(contains("sample@debug")) + .stdout(contains("not installed")) + .stdout(predicates::str::contains("1.2.3").not()); + + Ok(()) +} + #[tokio::test] async fn plugin_add_and_remove_updates_installed_plugin_config() -> Result<()> { let (codex_home, _source) = setup_local_marketplace()?; @@ -258,7 +448,7 @@ async fn plugin_add_reinstalls_from_configured_marketplace_snapshot() -> Result< assert!( codex_home .path() - .join("plugins/cache/debug/sample/local/.codex-plugin/plugin.json") + .join("plugins/cache/debug/sample/1.2.3/.codex-plugin/plugin.json") .is_file() ); @@ -311,7 +501,7 @@ async fn plugin_add_rejects_cached_plugins_without_authorizing_marketplace_snaps assert!( codex_home .path() - .join("plugins/cache/debug/sample/local/.codex-plugin/plugin.json") + .join("plugins/cache/debug/sample/1.2.3/.codex-plugin/plugin.json") .is_file() ); diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index 56b8c18bd..98b420f5f 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -263,6 +263,7 @@ pub struct ConfiguredMarketplacePlugin { pub id: String, pub name: String, pub local_version: Option, + pub installed_version: Option, pub source: MarketplacePluginSource, pub policy: MarketplacePluginPolicy, pub interface: Option, @@ -1225,15 +1226,21 @@ impl PluginsManager { if !self.restriction_product_matches(plugin.policy.products.as_deref()) { return None; } + let plugin_id = + PluginId::new(plugin.name.clone(), marketplace_name.clone()).ok(); let installed = installed_plugins.contains(&plugin_key); + let installed_version = installed.then_some(()).and_then(|_| { + plugin_id + .as_ref() + .and_then(|plugin_id| self.store.active_plugin_version(plugin_id)) + }); let enabled = enabled_plugins.contains(&plugin_key); let mut interface = plugin.interface; let mut local_version = plugin.local_version; if installed && matches!(&plugin.source, MarketplacePluginSource::Git { .. }) - && let Ok(plugin_id) = - PluginId::new(plugin.name.clone(), marketplace_name.clone()) - && let Some(plugin_root) = self.store.active_plugin_root(&plugin_id) + && let Some(plugin_id) = plugin_id.as_ref() + && let Some(plugin_root) = self.store.active_plugin_root(plugin_id) && let Some(manifest) = load_plugin_manifest(plugin_root.as_path()) { local_version = manifest.version.clone(); @@ -1251,6 +1258,7 @@ impl PluginsManager { // plugin entries from duplicate marketplace files intentionally // resolve to the first discovered source. id: plugin_key, + installed_version, installed, enabled, name: plugin.name, @@ -1298,6 +1306,12 @@ impl PluginsManager { let marketplace_name = plugin.plugin_id.marketplace_name.clone(); let plugin_key = plugin.plugin_id.as_key(); let (installed_plugins, enabled_plugins) = self.configured_plugin_states(config); + let installed = installed_plugins.contains(&plugin_key); + let installed_version = if installed { + self.store.active_plugin_version(&plugin.plugin_id) + } else { + None + }; let plugin = self .read_plugin_detail_for_marketplace_plugin( config, @@ -1309,6 +1323,7 @@ impl PluginsManager { .manifest .as_ref() .and_then(|manifest| manifest.version.clone()), + installed_version, source: plugin.source, policy: plugin.policy, interface: plugin.interface, @@ -1317,7 +1332,7 @@ impl PluginsManager { .as_ref() .map(|manifest| manifest.keywords.clone()) .unwrap_or_default(), - installed: installed_plugins.contains(&plugin_key), + installed, enabled: enabled_plugins.contains(&plugin_key), }, ) diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 53f72ed50..fa0b485fa 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -1599,6 +1599,7 @@ enabled = false id: "enabled-plugin@debug".to_string(), name: "enabled-plugin".to_string(), local_version: None, + installed_version: Some("local".to_string()), source: MarketplacePluginSource::Local { path: AbsolutePathBuf::try_from(tmp.path().join("repo/enabled-plugin")) .unwrap(), @@ -1617,6 +1618,7 @@ enabled = false id: "disabled-plugin@debug".to_string(), name: "disabled-plugin".to_string(), local_version: None, + installed_version: Some("local".to_string()), source: MarketplacePluginSource::Local { path: AbsolutePathBuf::try_from(tmp.path().join("repo/disabled-plugin"),) .unwrap(), @@ -1738,6 +1740,7 @@ plugins = true id: "default-plugin@debug".to_string(), name: "default-plugin".to_string(), local_version: None, + installed_version: None, source: MarketplacePluginSource::Local { path: AbsolutePathBuf::try_from(tmp.path().join("repo/default-plugin")).unwrap(), }, @@ -2170,6 +2173,7 @@ enabled = true id: "toolkit@debug".to_string(), name: "toolkit".to_string(), local_version: None, + installed_version: Some("local".to_string()), source: MarketplacePluginSource::Git { url: missing_remote_repo_url, path: Some("plugins/toolkit".to_string()), @@ -2287,6 +2291,7 @@ plugins = true id: "linear@openai-curated".to_string(), name: "linear".to_string(), local_version: None, + installed_version: None, source: MarketplacePluginSource::Local { path: AbsolutePathBuf::try_from(curated_root.join("plugins/linear")).unwrap(), }, @@ -2582,6 +2587,7 @@ enabled = false id: "dup-plugin@debug".to_string(), name: "dup-plugin".to_string(), local_version: None, + installed_version: None, source: MarketplacePluginSource::Local { path: AbsolutePathBuf::try_from(tmp.path().join("repo-a/from-a")).unwrap(), }, @@ -2613,6 +2619,7 @@ enabled = false id: "b-only-plugin@debug".to_string(), name: "b-only-plugin".to_string(), local_version: None, + installed_version: None, source: MarketplacePluginSource::Local { path: AbsolutePathBuf::try_from(tmp.path().join("repo-b/from-b-only")).unwrap(), }, @@ -2698,6 +2705,7 @@ enabled = true id: "sample-plugin@debug".to_string(), name: "sample-plugin".to_string(), local_version: None, + installed_version: None, source: MarketplacePluginSource::Local { path: AbsolutePathBuf::try_from(tmp.path().join("repo/sample-plugin")).unwrap(), },