From 44dbae90eb5a2e3e047d9636bf2463f3beb00fa8 Mon Sep 17 00:00:00 2001 From: jameswt-oai Date: Mon, 22 Jun 2026 10:27:23 -0700 Subject: [PATCH] [codex] Centralize Plugin Analytics Metadata (#27102) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR moves construction of `PluginTelemetryMetadata` from loader and model helpers into `PluginsManager`, which already owns installed plugin state and will eventually perform remote identity enrichment. The metadata type remains in `codex-plugin`, and serialized analytics events remain unchanged. ## Before ```mermaid flowchart LR subgraph Events["Analytics event paths"] direction TB Lifecycle["Local install / uninstall"] Config["Enable / disable"] Remote["Remote install"] Used["Plugin used"] end subgraph Construction["Metadata construction"] direction TB Loader["Loader telemetry helpers"] Summary["PluginCapabilitySummary::telemetry_metadata"] Override["Caller adds remote_plugin_id"] end Metadata["PluginTelemetryMetadata"] Lifecycle --> Loader Config --> Loader Remote --> Loader Loader -->|"local events"| Metadata Loader -->|"remote install"| Override Override --> Metadata Used --> Summary Summary --> Metadata ``` Telemetry metadata was constructed through loader helpers, a capability-summary method, and a remote-install call-site override. ## After ```mermaid flowchart LR subgraph Events["Analytics event paths"] direction TB Lifecycle["Local install / uninstall"] Config["Enable / disable"] Remote["Remote install"] Used["Plugin used"] end Manager["PluginsManager — single construction owner"] Metadata["PluginTelemetryMetadata"] Lifecycle --> Manager Config --> Manager Remote -->|"authoritative remote ID"| Manager Used -->|"capability summary"| Manager Manager --> Metadata ``` Every analytics path delegates metadata construction to `PluginsManager`. Remote install still supplies its authoritative backend ID explicitly. ## What Changes - Make loader code return a focused plugin capability summary instead of constructing analytics metadata. - Centralize immutable plugin telemetry metadata construction in `PluginsManager`. - Route local install/uninstall, remote install, enable/disable, and plugin-used emitters through the manager. - Preserve the current serialized analytics contract exactly. Normal metadata still has no remote override. Remote install continues to provide its authoritative backend ID explicitly, so the existing serializer continues reporting that ID through `plugin_id`. Snapshot-based enrichment is intentionally deferred to the final PR. ## Testing - `just test -p codex-core-plugins` (238 tests passed) - `just test -p codex-plugin` (3 tests passed) - Scoped Clippy/compile checks passed for `codex-plugin`, `codex-core-plugins`, `codex-app-server`, and `codex-core`. ## Split Overview ```text main ├── #27093 Debug analytics capture (merged) ├── #27099 Non-mutating plugin smoke (merged) ├── #27100 Remote install/uninstall smoke (merged) └── #27102 Plugin telemetry metadata refactor ← you are here └── #27669 Persist remote plugin identity After #27102 and #27669 merge: └── Final PR: add explicit local and remote IDs to plugin analytics ``` Review order and dependencies: 1. [#27093 Add debug-only analytics event capture](https://github.com/openai/codex/pull/27093) (merged) 2. [#27099 Add a plugin analytics smoke workflow](https://github.com/openai/codex/pull/27099) (merged) 3. [#27100 Add a remote plugin analytics mutation smoke workflow](https://github.com/openai/codex/pull/27100) (merged) 4. This metadata refactor, independent and based on `main` 5. [#27669 Persist remote plugin identity](https://github.com/openai/codex/pull/27669), stacked on this PR 6. Final remote-ID behavior PR, created after the prerequisites merge The original [#26281](https://github.com/openai/codex/pull/26281) remains open as the aggregate reference until the final replacement PR is published. --- .../analytics/src/analytics_client_tests.rs | 9 +- codex-rs/app-server/src/request_processors.rs | 1 - .../request_processors/config_processor.rs | 9 +- .../src/request_processors/plugins.rs | 18 ++-- codex-rs/core-plugins/src/loader.rs | 47 +++------- codex-rs/core-plugins/src/manager.rs | 72 ++++++++++++++-- codex-rs/core-plugins/src/manager_tests.rs | 85 +++++++++++++++++-- codex-rs/core/src/session/turn.rs | 17 ++-- codex-rs/plugin/src/lib.rs | 22 ----- 9 files changed, 189 insertions(+), 91 deletions(-) diff --git a/codex-rs/analytics/src/analytics_client_tests.rs b/codex-rs/analytics/src/analytics_client_tests.rs index a9a1d90e7..091a85e11 100644 --- a/codex-rs/analytics/src/analytics_client_tests.rs +++ b/codex-rs/analytics/src/analytics_client_tests.rs @@ -3504,10 +3504,11 @@ async fn reducer_ingests_plugin_install_failed_fact() { async fn reducer_ingests_plugin_install_failed_fact_without_detail() { let mut reducer = AnalyticsReducer::default(); let mut events = Vec::new(); - let mut plugin = PluginTelemetryMetadata::from_plugin_id( - &PluginId::parse("unknown@openai-curated-remote").expect("valid plugin id"), - ); - plugin.remote_plugin_id = Some("plugins~Plugin_00000000000000000000000000000000".to_string()); + let plugin = PluginTelemetryMetadata { + plugin_id: PluginId::parse("unknown@openai-curated-remote").expect("valid plugin id"), + remote_plugin_id: Some("plugins~Plugin_00000000000000000000000000000000".to_string()), + capability_summary: None, + }; reducer .ingest( diff --git a/codex-rs/app-server/src/request_processors.rs b/codex-rs/app-server/src/request_processors.rs index c8bd48c47..2972d05f0 100644 --- a/codex-rs/app-server/src/request_processors.rs +++ b/codex-rs/app-server/src/request_processors.rs @@ -337,7 +337,6 @@ use codex_core_plugins::PluginUninstallError as CorePluginUninstallError; use codex_core_plugins::PluginsManager; use codex_core_plugins::loader::load_plugin_apps; use codex_core_plugins::loader::load_plugin_mcp_servers; -use codex_core_plugins::loader::plugin_telemetry_metadata_from_root; use codex_core_plugins::manifest::PluginManifestInterface; use codex_core_plugins::marketplace::MarketplaceError; use codex_core_plugins::marketplace::MarketplacePluginSource; diff --git a/codex-rs/app-server/src/request_processors/config_processor.rs b/codex-rs/app-server/src/request_processors/config_processor.rs index 15c3b7c86..50bba5400 100644 --- a/codex-rs/app-server/src/request_processors/config_processor.rs +++ b/codex-rs/app-server/src/request_processors/config_processor.rs @@ -295,15 +295,14 @@ impl ConfigRequestProcessor { &self, pending_changes: std::collections::BTreeMap, ) { + let plugins_manager = self.thread_manager.plugins_manager(); for (plugin_id, enabled) in pending_changes { let Ok(plugin_id) = PluginId::parse(&plugin_id) else { continue; }; - let metadata = codex_core_plugins::loader::installed_plugin_telemetry_metadata( - self.config_manager.codex_home(), - &plugin_id, - ) - .await; + let metadata = plugins_manager + .telemetry_metadata_for_installed_plugin(&plugin_id) + .await; if enabled { self.analytics_events_client.track_plugin_enabled(metadata); } else { diff --git a/codex-rs/app-server/src/request_processors/plugins.rs b/codex-rs/app-server/src/request_processors/plugins.rs index e9bf04476..1e4716a88 100644 --- a/codex-rs/app-server/src/request_processors/plugins.rs +++ b/codex-rs/app-server/src/request_processors/plugins.rs @@ -23,7 +23,6 @@ use codex_mcp::McpOAuthLoginSupport; use codex_mcp::oauth_login_support; use codex_mcp::should_retry_without_scopes; use codex_plugin::PluginId; -use codex_plugin::PluginTelemetryMetadata; use codex_rmcp_client::perform_oauth_login_silent; #[derive(Clone)] @@ -1620,9 +1619,14 @@ impl PluginRequestProcessor { Some(self.effective_plugins_changed_callback()), ); - let mut plugin_metadata = - plugin_telemetry_metadata_from_root(&result.plugin_id, &result.installed_path).await; - plugin_metadata.remote_plugin_id = Some(remote_plugin_id.clone()); + let plugin_metadata = self + .thread_manager + .plugins_manager() + .telemetry_metadata_for_installed_plugin_with_remote_id( + &result.plugin_id, + &remote_plugin_id, + ) + .await; self.analytics_events_client .track_plugin_installed(plugin_metadata); @@ -1713,8 +1717,10 @@ impl PluginRequestProcessor { else { return; }; - let mut plugin = PluginTelemetryMetadata::from_plugin_id(&plugin_id); - plugin.remote_plugin_id = Some(remote_plugin_id.to_string()); + let plugin = self + .thread_manager + .plugins_manager() + .telemetry_metadata_for_plugin_id_with_remote_id(&plugin_id, remote_plugin_id); self.analytics_events_client .track_plugin_install_failed(plugin, error_type.to_string()); } diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index 54eb37168..2a8d44fc9 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -38,7 +38,6 @@ use codex_plugin::PluginCapabilitySummary; use codex_plugin::PluginHookSource; use codex_plugin::PluginId; use codex_plugin::PluginIdError; -use codex_plugin::PluginTelemetryMetadata; use codex_plugin::app_connector_ids_from_declarations; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; @@ -1164,13 +1163,11 @@ fn cleaned_app_category(category: Option) -> Option { .filter(|category| !category.is_empty()) } -pub async fn plugin_telemetry_metadata_from_root( +pub async fn plugin_capability_summary_from_root( plugin_id: &PluginId, plugin_root: &AbsolutePathBuf, -) -> PluginTelemetryMetadata { - let Some(manifest) = load_plugin_manifest(plugin_root.as_path()) else { - return PluginTelemetryMetadata::from_plugin_id(plugin_id); - }; +) -> Option { + let manifest = load_plugin_manifest(plugin_root.as_path())?; let manifest_paths = &manifest.paths; let has_skills = !plugin_skill_roots(plugin_root, manifest_paths).is_empty(); @@ -1192,18 +1189,14 @@ pub async fn plugin_telemetry_metadata_from_root( .await; let app_connector_ids = app_connector_ids_from_declarations(&app_declarations); - PluginTelemetryMetadata { - plugin_id: plugin_id.clone(), - remote_plugin_id: None, - capability_summary: Some(PluginCapabilitySummary { - config_name: plugin_id.as_key(), - display_name: plugin_id.plugin_name.clone(), - description: None, - has_skills, - mcp_server_names, - app_connector_ids, - }), - } + Some(PluginCapabilitySummary { + config_name: plugin_id.as_key(), + display_name: plugin_id.plugin_name.clone(), + description: None, + has_skills, + mcp_server_names, + app_connector_ids, + }) } pub async fn load_plugin_mcp_servers( @@ -1279,24 +1272,6 @@ pub(crate) async fn load_plugin_mcp_servers_from_manifest( mcp_servers } -pub async fn installed_plugin_telemetry_metadata( - codex_home: &Path, - plugin_id: &PluginId, -) -> PluginTelemetryMetadata { - let store = match PluginStore::try_new(codex_home.to_path_buf()) { - Ok(store) => store, - Err(err) => { - warn!("failed to resolve plugin cache root: {err}"); - return PluginTelemetryMetadata::from_plugin_id(plugin_id); - } - }; - let Some(plugin_root) = store.active_plugin_root(plugin_id) else { - return PluginTelemetryMetadata::from_plugin_id(plugin_id); - }; - - plugin_telemetry_metadata_from_root(plugin_id, &plugin_root).await -} - async fn load_mcp_servers_from_file( plugin_root: &Path, mcp_config_path: &AbsolutePathBuf, diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index b502106ea..19b48b2c0 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -6,7 +6,6 @@ use crate::is_openai_curated_marketplace_name; use crate::loader::PluginHookLoadOutcome; use crate::loader::configured_curated_plugin_ids_from_codex_home; use crate::loader::curated_plugin_cache_version; -use crate::loader::installed_plugin_telemetry_metadata; use crate::loader::load_plugin_apps_from_manifest; use crate::loader::load_plugin_hooks; use crate::loader::load_plugin_hooks_from_layer_stack; @@ -15,7 +14,7 @@ use crate::loader::load_plugin_skills; use crate::loader::load_plugins_from_layer_stack; use crate::loader::log_plugin_load_errors; use crate::loader::materialize_marketplace_plugin_source; -use crate::loader::plugin_telemetry_metadata_from_root; +use crate::loader::plugin_capability_summary_from_root; use crate::loader::refresh_curated_plugin_cache; use crate::loader::refresh_non_curated_plugin_cache; use crate::loader::refresh_non_curated_plugin_cache_force_reinstall; @@ -709,6 +708,66 @@ impl PluginsManager { remote_installed_plugins_to_config(plugins, &self.store) } + pub async fn telemetry_metadata_for_installed_plugin( + &self, + plugin_id: &PluginId, + ) -> PluginTelemetryMetadata { + let mut metadata = self.telemetry_metadata_for_plugin_id(plugin_id); + metadata.capability_summary = match self.store.active_plugin_root(plugin_id) { + Some(plugin_root) => plugin_capability_summary_from_root(plugin_id, &plugin_root).await, + None => None, + }; + metadata + } + + pub async fn telemetry_metadata_for_installed_plugin_with_remote_id( + &self, + plugin_id: &PluginId, + remote_plugin_id: &str, + ) -> PluginTelemetryMetadata { + let mut metadata = + self.telemetry_metadata_for_plugin_id_with_remote_id(plugin_id, remote_plugin_id); + metadata.capability_summary = match self.store.active_plugin_root(plugin_id) { + Some(plugin_root) => plugin_capability_summary_from_root(plugin_id, &plugin_root).await, + None => None, + }; + metadata + } + + pub fn telemetry_metadata_for_plugin_id( + &self, + plugin_id: &PluginId, + ) -> PluginTelemetryMetadata { + PluginTelemetryMetadata { + plugin_id: plugin_id.clone(), + remote_plugin_id: None, + capability_summary: None, + } + } + + pub fn telemetry_metadata_for_plugin_id_with_remote_id( + &self, + plugin_id: &PluginId, + remote_plugin_id: &str, + ) -> PluginTelemetryMetadata { + PluginTelemetryMetadata { + remote_plugin_id: Some(remote_plugin_id.to_string()), + ..self.telemetry_metadata_for_plugin_id(plugin_id) + } + } + + pub fn telemetry_metadata_for_capability_summary( + &self, + summary: &PluginCapabilitySummary, + ) -> Option { + let plugin_id = PluginId::parse(&summary.config_name).ok()?; + Some(PluginTelemetryMetadata { + plugin_id, + remote_plugin_id: None, + capability_summary: Some(summary.clone()), + }) + } + pub fn build_remote_installed_plugin_marketplaces_from_cache( &self, visible_marketplaces: &[&str], @@ -1281,7 +1340,7 @@ impl PluginsManager { }; if let Some(analytics_events_client) = analytics_events_client { analytics_events_client.track_plugin_install_failed( - PluginTelemetryMetadata::from_plugin_id(plugin_id), + self.telemetry_metadata_for_plugin_id(plugin_id), error_type.to_string(), ); } @@ -1351,7 +1410,7 @@ impl PluginsManager { }; if let Some(analytics_events_client) = analytics_events_client { analytics_events_client.track_plugin_installed( - plugin_telemetry_metadata_from_root(&result.plugin_id, &result.installed_path) + self.telemetry_metadata_for_installed_plugin(&result.plugin_id) .await, ); } @@ -1392,7 +1451,10 @@ impl PluginsManager { async fn uninstall_plugin_id(&self, plugin_id: PluginId) -> Result<(), PluginUninstallError> { let plugin_telemetry = if self.store.active_plugin_root(&plugin_id).is_some() { - Some(installed_plugin_telemetry_metadata(self.codex_home.as_path(), &plugin_id).await) + Some( + self.telemetry_metadata_for_installed_plugin(&plugin_id) + .await, + ) } else { None }; diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 4f448ff16..50a5fe3e7 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -845,6 +845,81 @@ remote_plugin = true assert_eq!(outcome, PluginLoadOutcome::default()); } +#[tokio::test] +async fn installed_plugin_telemetry_metadata_collects_capabilities() { + let codex_home = TempDir::new().unwrap(); + write_cached_plugin(codex_home.path(), "test", "sample"); + let manager = PluginsManager::new(codex_home.path().to_path_buf()); + let plugin_id = PluginId::parse("sample@test").expect("plugin id should parse"); + + let metadata = manager + .telemetry_metadata_for_installed_plugin(&plugin_id) + .await; + + assert_eq!( + metadata, + PluginTelemetryMetadata { + plugin_id, + remote_plugin_id: None, + capability_summary: Some(PluginCapabilitySummary { + config_name: "sample@test".to_string(), + display_name: "sample".to_string(), + description: None, + has_skills: true, + mcp_server_names: Vec::new(), + app_connector_ids: Vec::new(), + }), + } + ); +} + +#[tokio::test] +async fn installed_plugin_telemetry_metadata_accepts_authoritative_remote_identity() { + let codex_home = TempDir::new().unwrap(); + let manager = PluginsManager::new(codex_home.path().to_path_buf()); + let plugin_id = + PluginId::parse("linear@openai-curated-remote").expect("plugin id should parse"); + + let metadata = manager + .telemetry_metadata_for_installed_plugin_with_remote_id(&plugin_id, "plugins~Plugin_linear") + .await; + + assert_eq!( + metadata, + PluginTelemetryMetadata { + plugin_id, + remote_plugin_id: Some("plugins~Plugin_linear".to_string()), + capability_summary: None, + } + ); +} + +#[test] +fn capability_summary_telemetry_metadata_uses_local_identity() { + let codex_home = TempDir::new().unwrap(); + let manager = PluginsManager::new(codex_home.path().to_path_buf()); + let summary = PluginCapabilitySummary { + config_name: "linear@openai-curated-remote".to_string(), + display_name: "Linear".to_string(), + description: Some("Track work".to_string()), + has_skills: true, + mcp_server_names: vec!["linear".to_string()], + app_connector_ids: vec![AppConnectorId("linear-app".to_string())], + }; + + let metadata = manager.telemetry_metadata_for_capability_summary(&summary); + + assert_eq!( + metadata, + Some(PluginTelemetryMetadata { + plugin_id: PluginId::parse("linear@openai-curated-remote") + .expect("plugin id should parse"), + remote_plugin_id: None, + capability_summary: Some(summary), + }) + ); +} + #[tokio::test] async fn remote_installed_cache_prefers_local_curated_conflicts_when_remote_plugin_disabled() { let codex_home = TempDir::new().unwrap(); @@ -1154,14 +1229,14 @@ async fn plugin_telemetry_metadata_uses_default_mcp_config_path() { }"#, ); - let metadata = plugin_telemetry_metadata_from_root( + let summary = plugin_capability_summary_from_root( &PluginId::parse("sample@test").expect("plugin id should parse"), &plugin_root.abs(), ) .await; assert_eq!( - metadata.capability_summary, + summary, Some(PluginCapabilitySummary { config_name: "sample@test".to_string(), display_name: "sample".to_string(), @@ -1174,7 +1249,7 @@ async fn plugin_telemetry_metadata_uses_default_mcp_config_path() { } #[tokio::test] -async fn plugin_telemetry_metadata_uses_manifest_mcp_server_objects() { +async fn plugin_capability_summary_uses_manifest_mcp_server_objects() { let codex_home = TempDir::new().unwrap(); let plugin_root = codex_home .path() @@ -1195,14 +1270,14 @@ async fn plugin_telemetry_metadata_uses_manifest_mcp_server_objects() { }"#, ); - let metadata = plugin_telemetry_metadata_from_root( + let summary = plugin_capability_summary_from_root( &PluginId::parse("counter-sample@test").expect("plugin id should parse"), &plugin_root.abs(), ) .await; assert_eq!( - metadata.capability_summary, + summary, Some(PluginCapabilitySummary { config_name: "counter-sample@test".to_string(), display_name: "counter-sample".to_string(), diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 9b9c7cc66..81f9f5235 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -656,13 +656,16 @@ async fn build_skills_and_plugins( sess.services .analytics_events_client .track_app_mentioned(tracking.clone(), mentioned_app_invocations); - for plugin in mentioned_plugins - .iter() - .filter_map(crate::plugins::PluginCapabilitySummary::telemetry_metadata) - { - sess.services - .analytics_events_client - .track_plugin_used(tracking.clone(), plugin); + for summary in &mentioned_plugins { + if let Some(plugin) = sess + .services + .plugins_manager + .telemetry_metadata_for_capability_summary(summary) + { + sess.services + .analytics_events_client + .track_plugin_used(tracking.clone(), plugin); + } } let mut injection_items: Vec = match injected_host_skill_prompts { diff --git a/codex-rs/plugin/src/lib.rs b/codex-rs/plugin/src/lib.rs index 7daa78bc0..1a4eb23af 100644 --- a/codex-rs/plugin/src/lib.rs +++ b/codex-rs/plugin/src/lib.rs @@ -76,25 +76,3 @@ pub struct PluginTelemetryMetadata { pub remote_plugin_id: Option, pub capability_summary: Option, } - -impl PluginTelemetryMetadata { - pub fn from_plugin_id(plugin_id: &PluginId) -> Self { - Self { - plugin_id: plugin_id.clone(), - remote_plugin_id: None, - capability_summary: None, - } - } -} - -impl PluginCapabilitySummary { - pub fn telemetry_metadata(&self) -> Option { - PluginId::parse(&self.config_name) - .ok() - .map(|plugin_id| PluginTelemetryMetadata { - plugin_id, - remote_plugin_id: None, - capability_summary: Some(self.clone()), - }) - } -}