From baa3df615802b39deb4dbbbb6722f17da41ab3ad Mon Sep 17 00:00:00 2001 From: Alex Daley Date: Tue, 7 Jul 2026 18:40:40 -0400 Subject: [PATCH] Verify remote plugin suggestion completion --- .../tools/handlers/request_plugin_install.rs | 46 +++++++----- .../tests/suite/request_plugin_install.rs | 70 ++++++++++++++----- 2 files changed, 81 insertions(+), 35 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/request_plugin_install.rs b/codex-rs/core/src/tools/handlers/request_plugin_install.rs index e3b9ec07af..f145ec0c9b 100644 --- a/codex-rs/core/src/tools/handlers/request_plugin_install.rs +++ b/codex-rs/core/src/tools/handlers/request_plugin_install.rs @@ -14,6 +14,7 @@ use codex_core_plugins::remote::REMOTE_GLOBAL_MARKETPLACE_NAME; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_rmcp_client::ElicitationAction; use codex_rmcp_client::ElicitationResponse; +use codex_tools::DiscoverablePluginInfo; use codex_tools::DiscoverableTool; use codex_tools::DiscoverableToolAction; use codex_tools::DiscoverableToolType; @@ -300,10 +301,10 @@ impl RequestPluginInstallHandler { remote_plugin_id, connector_ids, selected: user_confirmed, + completed, }], response_action, user_confirmed, - completed, }, ); } @@ -412,12 +413,9 @@ async fn verify_request_plugin_install_completed( }), DiscoverableTool::Plugin(plugin) => { if is_remote_plugin_install_suggestion(&plugin.id) { - let (_, accessible_connectors) = tokio::join!( + let (completed, _) = tokio::join!( refresh_remote_installed_plugins_cache_after_install( - session, - turn, - auth, - plugin.id.as_str(), + session, turn, auth, plugin, ), refresh_missing_requested_connectors( turn, @@ -427,12 +425,7 @@ async fn verify_request_plugin_install_completed( plugin.id.as_str(), ) ); - return accessible_connectors.is_some_and(|accessible_connectors| { - all_requested_connectors_picked_up( - &plugin.app_connector_ids, - &accessible_connectors, - ) - }); + return completed; } session.reload_user_config_layer().await; @@ -459,11 +452,18 @@ async fn refresh_remote_installed_plugins_cache_after_install( session: &crate::session::session::Session, turn: &crate::session::turn_context::TurnContext, auth: Option<&codex_login::CodexAuth>, - tool_id: &str, -) { + plugin: &DiscoverablePluginInfo, +) -> bool { + let Some(remote_plugin_id) = plugin.remote_plugin_id.as_deref() else { + warn!( + "remote plugin install suggestion did not include a remote plugin id for {}", + plugin.id + ); + return false; + }; let plugins_manager = &session.services.plugins_manager; let plugins_config = turn.config.plugins_config_input(); - if let Err(err) = plugins_manager + match plugins_manager .build_and_cache_remote_installed_plugin_marketplaces( &plugins_config, auth, @@ -472,9 +472,19 @@ async fn refresh_remote_installed_plugins_cache_after_install( ) .await { - warn!( - "failed to refresh remote installed plugins cache after plugin install request for {tool_id}: {err:#}" - ); + Ok(marketplaces) => marketplaces + .into_iter() + .flat_map(|marketplace| marketplace.plugins) + .any(|installed_plugin| { + installed_plugin.remote_plugin_id == remote_plugin_id && installed_plugin.installed + }), + Err(err) => { + warn!( + "failed to refresh remote installed plugins cache after plugin install request for {}: {err:#}", + plugin.id + ); + false + } } } diff --git a/codex-rs/core/tests/suite/request_plugin_install.rs b/codex-rs/core/tests/suite/request_plugin_install.rs index b8d8736048..0e9d781294 100644 --- a/codex-rs/core/tests/suite/request_plugin_install.rs +++ b/codex-rs/core/tests/suite/request_plugin_install.rs @@ -414,6 +414,7 @@ async fn legacy_connector_install_emits_attributed_suggestion_outcome() -> Resul "remote_plugin_id": null, "connector_ids": [DISCOVERABLE_GMAIL_ID], "selected": false, + "completed": false, }], "response_action": "decline", "user_confirmed": false, @@ -618,6 +619,7 @@ async fn run_remote_plugin_install_metadata_case() -> Result<()> { "remote_plugin_id": REMOTE_PLUGIN_ID, "connector_ids": [APP_CONNECTOR_ID], "selected": false, + "completed": false, }], "response_action": "decline", "user_confirmed": false, @@ -646,15 +648,37 @@ enum RefreshedAppsTools { Missing, } -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn remote_plugin_install_refreshes_plugin_and_apps_tool_caches() -> Result<()> { - skip_if_no_network!(Ok(())); - - run_remote_plugin_install_refresh_case(RefreshedAppsTools::Available).await?; - run_remote_plugin_install_refresh_case(RefreshedAppsTools::Missing).await +#[derive(Clone, Copy)] +enum RefreshedPluginInventory { + Installed, + Missing, } -async fn run_remote_plugin_install_refresh_case(refreshed_tools: RefreshedAppsTools) -> Result<()> { +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn remote_plugin_completion_uses_refreshed_plugin_inventory() -> Result<()> { + skip_if_no_network!(Ok(())); + + run_remote_plugin_install_refresh_case( + RefreshedPluginInventory::Installed, + RefreshedAppsTools::Available, + ) + .await?; + run_remote_plugin_install_refresh_case( + RefreshedPluginInventory::Installed, + RefreshedAppsTools::Missing, + ) + .await?; + run_remote_plugin_install_refresh_case( + RefreshedPluginInventory::Missing, + RefreshedAppsTools::Available, + ) + .await +} + +async fn run_remote_plugin_install_refresh_case( + refreshed_plugin_inventory: RefreshedPluginInventory, + refreshed_tools: RefreshedAppsTools, +) -> Result<()> { let server = start_mock_server().await; let apps_server = match refreshed_tools { RefreshedAppsTools::Available => { @@ -693,11 +717,20 @@ async fn run_remote_plugin_install_refresh_case(refreshed_tools: RefreshedAppsTo let test = build_test(&server, &apps_server).await?; let elicitation = start_install_turn(&test, "use Calendar").await?; - mount_remote_calendar_installed_plugins(&server).await; - drop(initial_remote_installed_plugins); + if matches!( + refreshed_plugin_inventory, + RefreshedPluginInventory::Installed + ) { + mount_remote_calendar_installed_plugins(&server).await; + drop(initial_remote_installed_plugins); + } resolve_install_elicitation(&test, elicitation, ElicitationAction::Accept).await?; - let completed = matches!(refreshed_tools, RefreshedAppsTools::Available); + let completed = matches!( + refreshed_plugin_inventory, + RefreshedPluginInventory::Installed + ); + let apps_tools_available = matches!(refreshed_tools, RefreshedAppsTools::Available); let outcome_event = wait_for_analytics_event(&server, "codex_plugin_install_suggestion_outcome").await; let thread_id = outcome_event["event_params"]["thread_id"].clone(); @@ -716,6 +749,7 @@ async fn run_remote_plugin_install_refresh_case(refreshed_tools: RefreshedAppsTo "remote_plugin_id": REMOTE_CALENDAR_PLUGIN_ID, "connector_ids": [CALENDAR_CONNECTOR_ID], "selected": true, + "completed": completed, }], "response_action": "accept", "user_confirmed": true, @@ -756,15 +790,17 @@ async fn run_remote_plugin_install_refresh_case(refreshed_tools: RefreshedAppsTo requests[1] .tool_by_name(CALENDAR_NAMESPACE, CALENDAR_CREATE_EVENT_TOOL) .is_some(), - completed, + apps_tools_available, "the resumed router should reflect the refreshed Apps tools" ); - assert!( - !tool_names(&requests[1].body_json()) - .iter() - .any(|name| name == REQUEST_PLUGIN_INSTALL_TOOL_NAME), - "the refreshed installed-plugin cache should filter the cached recommendation" - ); + if completed { + assert!( + !tool_names(&requests[1].body_json()) + .iter() + .any(|name| name == REQUEST_PLUGIN_INSTALL_TOOL_NAME), + "the refreshed installed-plugin cache should filter the cached recommendation" + ); + } Ok(()) }