diff --git a/codex-rs/app-server/tests/suite/v2/plugin_uninstall.rs b/codex-rs/app-server/tests/suite/v2/plugin_uninstall.rs index e0f71f11b5bd..5a05816b0d35 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_uninstall.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_uninstall.rs @@ -196,7 +196,6 @@ async fn plugin_uninstall_writes_remote_plugin_to_cloud_when_remote_plugin_enabl )?; mount_remote_plugin_detail(&server, REMOTE_PLUGIN_ID, "1.0.0", "GLOBAL").await; - mount_empty_remote_installed_plugins(&server).await; Mock::given(method("POST")) .and(path(format!( @@ -269,7 +268,6 @@ async fn plugin_uninstall_uses_detail_scope_for_cache_namespace() -> Result<()> AuthCredentialsStoreMode::File, )?; mount_remote_plugin_detail(&server, REMOTE_PLUGIN_ID, "1.0.0", "WORKSPACE").await; - mount_empty_remote_installed_plugins(&server).await; Mock::given(method("POST")) .and(path(format!( @@ -326,7 +324,7 @@ async fn plugin_uninstall_uses_detail_scope_for_cache_namespace() -> Result<()> } #[tokio::test] -async fn plugin_uninstall_posts_even_when_remote_detail_fetch_fails() -> Result<()> { +async fn plugin_uninstall_rejects_before_post_when_remote_detail_fetch_fails() -> Result<()> { let codex_home = TempDir::new()?; let server = MockServer::start().await; write_remote_plugin_catalog_config( @@ -342,19 +340,6 @@ async fn plugin_uninstall_posts_even_when_remote_detail_fetch_fails() -> Result< AuthCredentialsStoreMode::File, )?; - Mock::given(method("POST")) - .and(path(format!( - "/backend-api/plugins/{REMOTE_PLUGIN_ID}/uninstall" - ))) - .and(header("authorization", "Bearer chatgpt-token")) - .and(header("chatgpt-account-id", "account-123")) - .respond_with( - ResponseTemplate::new(200) - .set_body_string(format!(r#"{{"id":"{REMOTE_PLUGIN_ID}","enabled":false}}"#)), - ) - .mount(&server) - .await; - let legacy_remote_plugin_cache_root = codex_home .path() .join(format!("plugins/cache/chatgpt-global/{REMOTE_PLUGIN_ID}")); @@ -368,22 +353,29 @@ async fn plugin_uninstall_posts_even_when_remote_detail_fetch_fails() -> Result< plugin_id: REMOTE_PLUGIN_ID.to_string(), }) .await?; - let response: JSONRPCResponse = timeout( + let err = timeout( DEFAULT_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), ) .await??; - let response: PluginUninstallResponse = to_response(response)?; - assert_eq!(response, PluginUninstallResponse {}); + assert_eq!(err.error.code, -32600); + assert!(err.error.message.contains("remote plugin catalog request")); + wait_for_remote_plugin_request_count( + &server, + "GET", + &format!("/ps/plugins/{REMOTE_PLUGIN_ID}"), + /*expected_count*/ 1, + ) + .await?; wait_for_remote_plugin_request_count( &server, "POST", &format!("/plugins/{REMOTE_PLUGIN_ID}/uninstall"), - /*expected_count*/ 1, + /*expected_count*/ 0, ) .await?; - assert!(!legacy_remote_plugin_cache_root.exists()); + assert!(legacy_remote_plugin_cache_root.exists()); Ok(()) } @@ -557,24 +549,6 @@ async fn mount_remote_plugin_detail( .await; } -async fn mount_empty_remote_installed_plugins(server: &MockServer) { - Mock::given(method("GET")) - .and(path("/backend-api/ps/plugins/installed")) - .and(header("authorization", "Bearer chatgpt-token")) - .and(header("chatgpt-account-id", "account-123")) - .respond_with(ResponseTemplate::new(200).set_body_string( - r#"{ - "plugins": [], - "pagination": { - "limit": 50, - "next_page_token": null - } -}"#, - )) - .mount(server) - .await; -} - async fn wait_for_remote_plugin_request_count( server: &MockServer, method_name: &str, diff --git a/codex-rs/core-plugins/src/remote.rs b/codex-rs/core-plugins/src/remote.rs index 6c87f4dc3036..5462655b59c1 100644 --- a/codex-rs/core-plugins/src/remote.rs +++ b/codex-rs/core-plugins/src/remote.rs @@ -13,10 +13,8 @@ use std::collections::BTreeMap; use std::collections::BTreeSet; use std::collections::HashSet; use std::fs; -use std::path::Path; use std::path::PathBuf; use std::time::Duration; -use tracing::warn; pub const REMOTE_GLOBAL_MARKETPLACE_NAME: &str = "chatgpt-global"; pub const REMOTE_WORKSPACE_MARKETPLACE_NAME: &str = "chatgpt-workspace"; @@ -435,27 +433,6 @@ async fn fetch_remote_plugin_detail_with_download_url_option( .await } -async fn fetch_remote_plugin_detail_by_id( - config: &RemotePluginServiceConfig, - auth: &CodexAuth, - plugin_id: &str, -) -> Result { - let plugin = fetch_plugin_detail( - config, auth, plugin_id, /*include_download_urls*/ false, - ) - .await?; - let scope = plugin.scope; - build_remote_plugin_detail( - config, - auth, - scope, - scope.marketplace_name().to_string(), - plugin_id, - plugin, - ) - .await -} - async fn build_remote_plugin_detail( config: &RemotePluginServiceConfig, auth: &CodexAuth, @@ -548,6 +525,12 @@ pub async fn uninstall_remote_plugin( plugin_id: &str, ) -> Result<(), RemotePluginCatalogError> { let auth = ensure_chatgpt_auth(auth)?; + let plugin = fetch_plugin_detail( + config, auth, plugin_id, /*include_download_urls*/ false, + ) + .await?; + let marketplace_name = plugin.scope.marketplace_name().to_string(); + let plugin_name = plugin.name; let base_url = config.chatgpt_base_url.trim_end_matches('/'); let url = format!("{base_url}/plugins/{plugin_id}/uninstall"); @@ -568,19 +551,9 @@ pub async fn uninstall_remote_plugin( }); } - let remote_detail = match fetch_remote_plugin_detail_by_id(config, auth, plugin_id).await { - Ok(remote_detail) => Some(remote_detail), - Err(err) => { - warn!( - plugin_id, - "failed to read remote plugin details after uninstall; skipping named cache removal: {err}" - ); - None - } - }; let legacy_plugin_id = plugin_id.to_string(); tokio::task::spawn_blocking(move || { - remove_remote_plugin_cache(codex_home, remote_detail, legacy_plugin_id) + remove_remote_plugin_cache(codex_home, marketplace_name, plugin_name, legacy_plugin_id) }) .await .map_err(|err| { @@ -595,65 +568,46 @@ pub async fn uninstall_remote_plugin( fn remove_remote_plugin_cache( codex_home: PathBuf, - remote_detail: Option, + marketplace_name: String, + plugin_name: String, legacy_plugin_id: String, ) -> Result<(), String> { - if let Some(remote_detail) = remote_detail { - let marketplace_name = remote_detail.marketplace_name; - let plugin_name = remote_detail.summary.name; - let store = PluginStore::try_new(codex_home.clone()) - .map_err(|err| format!("failed to resolve remote plugin cache root: {err}"))?; - let plugin_id = PluginId::new(plugin_name.clone(), marketplace_name.clone()).map_err( - |err| { - format!( - "invalid remote plugin cache id for `{plugin_name}` in `{marketplace_name}`: {err}" - ) - }, - )?; - let plugin_cache_root = store.plugin_base_root(&plugin_id); - store.uninstall(&plugin_id).map_err(|err| { + let store = PluginStore::try_new(codex_home.clone()) + .map_err(|err| format!("failed to resolve remote plugin cache root: {err}"))?; + let plugin_id = + PluginId::new(plugin_name.clone(), marketplace_name.clone()).map_err(|err| { format!( - "failed to remove remote plugin cache entry {}: {err}", - plugin_cache_root.display() + "invalid remote plugin cache id for `{plugin_name}` in `{marketplace_name}`: {err}" ) })?; - - let legacy_remote_plugin_cache_root = codex_home - .join(PLUGINS_CACHE_DIR) - .join(marketplace_name) - .join(legacy_plugin_id); - if legacy_remote_plugin_cache_root != plugin_cache_root.as_path() { - remove_path_if_exists(&legacy_remote_plugin_cache_root)?; - } - return Ok(()); - } - - for scope in RemotePluginScope::all() { - let legacy_remote_plugin_cache_root = codex_home - .join(PLUGINS_CACHE_DIR) - .join(scope.marketplace_name()) - .join(&legacy_plugin_id); - remove_path_if_exists(&legacy_remote_plugin_cache_root)?; - } - Ok(()) -} - -fn remove_path_if_exists(path: &Path) -> Result<(), String> { - if !path.exists() { - return Ok(()); - } - - let result = if path.is_dir() { - fs::remove_dir_all(path) - } else { - fs::remove_file(path) - }; - result.map_err(|err| { + let plugin_cache_root = store.plugin_base_root(&plugin_id); + store.uninstall(&plugin_id).map_err(|err| { format!( "failed to remove remote plugin cache entry {}: {err}", - path.display() + plugin_cache_root.display() ) - }) + })?; + + let legacy_remote_plugin_cache_root = codex_home + .join(PLUGINS_CACHE_DIR) + .join(marketplace_name) + .join(legacy_plugin_id); + if legacy_remote_plugin_cache_root != plugin_cache_root.as_path() + && legacy_remote_plugin_cache_root.exists() + { + let result = if legacy_remote_plugin_cache_root.is_dir() { + fs::remove_dir_all(&legacy_remote_plugin_cache_root) + } else { + fs::remove_file(&legacy_remote_plugin_cache_root) + }; + result.map_err(|err| { + format!( + "failed to remove remote plugin cache entry {}: {err}", + legacy_remote_plugin_cache_root.display() + ) + })?; + } + Ok(()) } fn build_remote_plugin_summary(