From 4c1c9bf327e33fc18cf64af54bf706101659511d Mon Sep 17 00:00:00 2001 From: jameswt-oai Date: Mon, 15 Jun 2026 16:09:43 -0700 Subject: [PATCH] [codex] Preserve remote plugin directory order (#28395) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - preserve the plugin directory endpoint's response order while merging installed state - append unmatched installed-only plugins afterward when requested - add focused coverage for directory order and installed-only placement ## Why The remote marketplace merge currently reconstructs plugins through ordered maps and sets, then sorts the result alphabetically by display name. That discards any ordering supplied by the plugin directory endpoint before the list reaches Desktop. ## Implementation Directory plugin IDs are unique, so the merge now iterates the directory vector directly in response order. For each directory plugin, it removes matching installed state from an ID-indexed map and builds the summary. Any entries left in the installed map are installed-only plugins and are appended when `include_installed_only` is enabled. There is no separate rank field, rank map, or final sort. Desktop therefore receives directory order—including any backend ranking—and can preserve it within its existing stable UI state tiers. ## Testing - `just test -p codex-core-plugins` (225 passed) --- codex-rs/core-plugins/src/remote.rs | 49 ++++++-------- codex-rs/core-plugins/src/remote_tests.rs | 78 +++++++++++++++++++++++ 2 files changed, 99 insertions(+), 28 deletions(-) create mode 100644 codex-rs/core-plugins/src/remote_tests.rs diff --git a/codex-rs/core-plugins/src/remote.rs b/codex-rs/core-plugins/src/remote.rs index a2286199e..3a6b5f6d2 100644 --- a/codex-rs/core-plugins/src/remote.rs +++ b/codex-rs/core-plugins/src/remote.rs @@ -27,6 +27,10 @@ mod catalog_cache; mod remote_installed_plugin_sync; mod share; +#[cfg(test)] +#[path = "remote_tests.rs"] +mod tests; + pub use remote_installed_plugin_sync::RemoteInstalledPluginBundleSyncError; pub use remote_installed_plugin_sync::RemoteInstalledPluginBundleSyncOutcome; pub use remote_installed_plugin_sync::RemotePluginCacheMutationGuard; @@ -812,40 +816,29 @@ fn build_remote_marketplace( installed_plugins: Vec, include_installed_only: bool, ) -> Result, RemotePluginCatalogError> { - let directory_plugins = directory_plugins - .into_iter() - .map(|plugin| (plugin.id.clone(), plugin)) - .collect::>(); - let installed_plugins = installed_plugins + let mut installed_plugins = installed_plugins .into_iter() .map(|plugin| (plugin.plugin.id.clone(), plugin)) .collect::>(); - let plugin_ids = directory_plugins - .keys() - .chain( - include_installed_only - .then_some(&installed_plugins) - .into_iter() - .flat_map(|plugins| plugins.keys()), - ) - .cloned() - .collect::>(); - if plugin_ids.is_empty() { + let mut plugins = directory_plugins + .into_iter() + .map(|plugin| { + let installed_plugin = installed_plugins.remove(&plugin.id); + build_remote_plugin_summary(&plugin, installed_plugin.as_ref()) + }) + .collect::, _>>()?; + if include_installed_only { + plugins.extend( + installed_plugins + .into_values() + .map(|plugin| build_remote_plugin_summary(&plugin.plugin, Some(&plugin))) + .collect::, _>>()?, + ); + } + if plugins.is_empty() { return Ok(None); } - let mut plugins = plugin_ids - .into_iter() - .filter_map(|plugin_id| { - let directory_plugin = directory_plugins.get(&plugin_id); - let installed_plugin = installed_plugins.get(&plugin_id); - directory_plugin - .or_else(|| installed_plugin.map(|plugin| &plugin.plugin)) - .map(|plugin| (plugin, installed_plugin)) - }) - .map(|(plugin, installed_plugin)| build_remote_plugin_summary(plugin, installed_plugin)) - .collect::, _>>()?; - sort_remote_plugin_summaries_by_display_name(&mut plugins); Ok(Some(RemoteMarketplace { name: name.to_string(), display_name: display_name.to_string(), diff --git a/codex-rs/core-plugins/src/remote_tests.rs b/codex-rs/core-plugins/src/remote_tests.rs new file mode 100644 index 000000000..cff134b80 --- /dev/null +++ b/codex-rs/core-plugins/src/remote_tests.rs @@ -0,0 +1,78 @@ +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn build_remote_marketplace_preserves_directory_order_and_appends_installed_only_plugins() { + let directory_plugins = vec![ + directory_plugin("plugin-z", "zulu"), + directory_plugin("plugin-m", "mike"), + ]; + let installed_plugins = vec![RemotePluginInstalledItem { + plugin: directory_plugin("plugin-a", "alpha"), + enabled: true, + disabled_skill_names: Vec::new(), + }]; + + let marketplace = build_remote_marketplace( + "marketplace", + "Marketplace", + directory_plugins, + installed_plugins, + /*include_installed_only*/ true, + ) + .expect("marketplace should be valid") + .expect("marketplace should not be empty"); + + assert_eq!( + marketplace + .plugins + .into_iter() + .map(|plugin| plugin.remote_plugin_id) + .collect::>(), + vec!["plugin-z", "plugin-m", "plugin-a"] + ); +} + +fn directory_plugin(id: &str, name: &str) -> RemotePluginDirectoryItem { + RemotePluginDirectoryItem { + id: id.to_string(), + name: name.to_string(), + scope: RemotePluginScope::Global, + discoverability: None, + creator_account_user_id: None, + creator_name: None, + share_url: None, + share_principals: None, + installation_policy: PluginInstallPolicy::Available, + authentication_policy: PluginAuthPolicy::OnUse, + availability: PluginAvailability::Available, + release: RemotePluginReleaseResponse { + version: None, + display_name: name.to_string(), + description: String::new(), + bundle_download_url: None, + app_ids: Vec::new(), + app_manifest: None, + app_templates: Vec::new(), + keywords: Vec::new(), + interface: RemotePluginReleaseInterfaceResponse { + short_description: None, + long_description: None, + developer_name: None, + category: None, + capabilities: Vec::new(), + website_url: None, + privacy_policy_url: None, + terms_of_service_url: None, + brand_color: None, + default_prompt: None, + default_prompts: None, + composer_icon_url: None, + logo_url: None, + screenshot_urls: Vec::new(), + }, + skills: Vec::new(), + mcp_servers: Vec::new(), + }, + } +}