From babf24a55914e0a2fc30184c6e6a9c4dbe2f0ac5 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Mon, 1 Jun 2026 19:17:39 +0000 Subject: [PATCH] codex: address app-server analytics review feedback (#25193) --- codex-rs/analytics/src/events.rs | 1 + codex-rs/app-server/src/analytics_utils.rs | 7 +- codex-rs/app-server/src/in_process.rs | 21 +++- codex-rs/app-server/src/lib.rs | 30 ++++- .../src/message_processor_tracing_tests.rs | 7 +- .../app-server/tests/suite/v2/analytics.rs | 114 ++++++++++++++++-- .../tests/suite/v2/plugin_install.rs | 12 +- .../tests/suite/v2/plugin_uninstall.rs | 2 +- 8 files changed, 167 insertions(+), 27 deletions(-) diff --git a/codex-rs/analytics/src/events.rs b/codex-rs/analytics/src/events.rs index 8624505a90..642c4bd772 100644 --- a/codex-rs/analytics/src/events.rs +++ b/codex-rs/analytics/src/events.rs @@ -43,6 +43,7 @@ use serde::Serialize; #[serde(rename_all = "snake_case")] pub enum AppServerRpcTransport { Stdio, + UnixSocket, Websocket, InProcess, } diff --git a/codex-rs/app-server/src/analytics_utils.rs b/codex-rs/app-server/src/analytics_utils.rs index 24ed12d2ad..2da606d3d5 100644 --- a/codex-rs/app-server/src/analytics_utils.rs +++ b/codex-rs/app-server/src/analytics_utils.rs @@ -7,10 +7,15 @@ use codex_login::AuthManager; pub(crate) fn analytics_events_client_from_config( auth_manager: Arc, config: &Config, + default_analytics_enabled: bool, ) -> AnalyticsEventsClient { AnalyticsEventsClient::new( auth_manager, config.chatgpt_base_url.trim_end_matches('/').to_string(), - config.analytics_enabled, + Some( + config + .analytics_enabled + .unwrap_or(default_analytics_enabled), + ), ) } diff --git a/codex-rs/app-server/src/in_process.rs b/codex-rs/app-server/src/in_process.rs index 91372e2bbb..26bbac25ca 100644 --- a/codex-rs/app-server/src/in_process.rs +++ b/codex-rs/app-server/src/in_process.rs @@ -382,8 +382,11 @@ async fn start_uninitialized(args: InProcessStartArgs) -> IoResult IoResult(channel_capacity); - analytics_events_client - .track_app_server_started(AppServerRpcTransport::InProcess, startup_started.elapsed()); let mut processor_handle = tokio::spawn(async move { + let app_server_started_analytics_events_client = analytics_events_client.clone(); let processor = Arc::new(MessageProcessor::new(MessageProcessorArgs { outgoing: Arc::clone(&processor_outgoing), analytics_events_client, @@ -446,6 +448,7 @@ async fn start_uninitialized(args: InProcessStartArgs) -> IoResult IoResult { + let initialized = + matches!(notification, ClientNotification::Initialized); processor.process_client_notification(notification).await; + if initialized && !app_server_started_tracked { + app_server_started_analytics_events_client + .track_app_server_started( + AppServerRpcTransport::InProcess, + startup_started.elapsed(), + ); + app_server_started_tracked = true; + } } None => { break; diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index 8b95468c67..de274d0df7 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -789,8 +789,11 @@ pub async fn run_main_with_transport_options( let processor_handle = tokio::spawn({ let auth_manager = Arc::clone(&auth_manager); - let analytics_events_client = - analytics_events_client_from_config(Arc::clone(&auth_manager), &config); + let analytics_events_client = analytics_events_client_from_config( + Arc::clone(&auth_manager), + &config, + default_analytics_enabled, + ); let rpc_transport = analytics_rpc_transport(&transport); let outgoing_message_sender = Arc::new(OutgoingMessageSender::new( outgoing_tx, @@ -1090,15 +1093,19 @@ pub async fn run_main_with_transport_options( fn analytics_rpc_transport(transport: &AppServerTransport) -> AppServerRpcTransport { match transport { AppServerTransport::Stdio => AppServerRpcTransport::Stdio, - AppServerTransport::UnixSocket { .. } - | AppServerTransport::WebSocket { .. } - | AppServerTransport::Off => AppServerRpcTransport::Websocket, + AppServerTransport::UnixSocket { .. } => AppServerRpcTransport::UnixSocket, + AppServerTransport::WebSocket { .. } | AppServerTransport::Off => { + AppServerRpcTransport::Websocket + } } } #[cfg(test)] mod tests { use super::LogFormat; + use super::analytics_rpc_transport; + use crate::transport::AppServerTransport; + use codex_analytics::AppServerRpcTransport; use pretty_assertions::assert_eq; #[test] @@ -1118,4 +1125,17 @@ mod tests { assert_eq!(LogFormat::from_env_value(Some("text")), LogFormat::Default); assert_eq!(LogFormat::from_env_value(Some("jsonl")), LogFormat::Default); } + + #[cfg(unix)] + #[test] + fn analytics_rpc_transport_preserves_unix_socket() { + let transport = "unix:///tmp/codex-app-server.sock" + .parse::() + .expect("unix socket transport should parse"); + + assert!(matches!( + analytics_rpc_transport(&transport), + AppServerRpcTransport::UnixSocket + )); + } } diff --git a/codex-rs/app-server/src/message_processor_tracing_tests.rs b/codex-rs/app-server/src/message_processor_tracing_tests.rs index 94b5a1af27..1a3ce2cb6e 100644 --- a/codex-rs/app-server/src/message_processor_tracing_tests.rs +++ b/codex-rs/app-server/src/message_processor_tracing_tests.rs @@ -244,8 +244,11 @@ async fn build_test_processor( Arg0DispatchPaths::default(), Arc::new(codex_config::NoopThreadConfigLoader), ); - let analytics_events_client = - analytics_events_client_from_config(Arc::clone(&auth_manager), config.as_ref()); + let analytics_events_client = analytics_events_client_from_config( + Arc::clone(&auth_manager), + config.as_ref(), + /*default_analytics_enabled*/ true, + ); let outgoing = Arc::new(OutgoingMessageSender::new( outgoing_tx, analytics_events_client.clone(), diff --git a/codex-rs/app-server/tests/suite/v2/analytics.rs b/codex-rs/app-server/tests/suite/v2/analytics.rs index 8aa15834b1..18c116e438 100644 --- a/codex-rs/app-server/tests/suite/v2/analytics.rs +++ b/codex-rs/app-server/tests/suite/v2/analytics.rs @@ -112,6 +112,24 @@ async fn standalone_app_server_startup_tracks_analytics_event() -> Result<()> { Ok(()) } +#[tokio::test] +async fn standalone_app_server_startup_respects_default_disabled_analytics() -> Result<()> { + let server = MockServer::start().await; + let codex_home = TempDir::new()?; + write_mock_responses_config_toml_with_chatgpt_base_url( + codex_home.path(), + &server.uri(), + &server.uri(), + )?; + mount_analytics_endpoint(&server).await; + write_analytics_auth(codex_home.path())?; + + let _mcp = McpProcess::new_without_managed_config(codex_home.path()).await?; + + assert_analytics_event_not_received(&server, Duration::from_secs(1), "codex_app_server_started") + .await +} + #[tokio::test] async fn embedded_app_server_startup_tracks_analytics_event() -> Result<()> { let server = MockServer::start().await; @@ -123,14 +141,53 @@ async fn embedded_app_server_startup_tracks_analytics_event() -> Result<()> { )?; mount_analytics_capture(&server, codex_home.path()).await?; + let client = + in_process::start(in_process_start_args(codex_home.path(), "codex-tui").await?).await?; + + let event = + wait_for_analytics_event(&server, Duration::from_secs(10), "codex_app_server_started") + .await?; + + assert_app_server_started_event(&event, "in_process"); + client.shutdown().await?; + Ok(()) +} + +#[tokio::test] +async fn embedded_app_server_failed_initialize_does_not_track_startup_event() -> Result<()> { + let server = MockServer::start().await; + let codex_home = TempDir::new()?; + write_mock_responses_config_toml_with_chatgpt_base_url( + codex_home.path(), + &server.uri(), + &server.uri(), + )?; + mount_analytics_capture(&server, codex_home.path()).await?; + + let result = + in_process::start(in_process_start_args(codex_home.path(), "bad\rname").await?).await; + let Err(error) = result else { + anyhow::bail!("in-process start should reject an invalid client name"); + }; + assert!( + error + .to_string() + .contains("in-process initialize failed: Invalid clientInfo.name") + ); + + assert_analytics_event_not_received(&server, Duration::from_secs(1), "codex_app_server_started") + .await +} + +async fn in_process_start_args(codex_home: &Path, client_name: &str) -> Result { let loader_overrides = LoaderOverrides::without_managed_config_for_tests(); let config = ConfigBuilder::default() - .codex_home(codex_home.path().to_path_buf()) - .fallback_cwd(Some(codex_home.path().to_path_buf())) + .codex_home(codex_home.to_path_buf()) + .fallback_cwd(Some(codex_home.to_path_buf())) .loader_overrides(loader_overrides.clone()) .build() .await?; - let client = in_process::start(InProcessStartArgs { + Ok(InProcessStartArgs { arg0_paths: Arg0DispatchPaths::default(), config: Arc::new(config), cli_overrides: Vec::new(), @@ -147,7 +204,7 @@ async fn embedded_app_server_startup_tracks_analytics_event() -> Result<()> { enable_codex_api_key_env: false, initialize: InitializeParams { client_info: ClientInfo { - name: "codex-tui".to_string(), + name: client_name.to_string(), title: None, version: "0.1.0".to_string(), }, @@ -155,15 +212,6 @@ async fn embedded_app_server_startup_tracks_analytics_event() -> Result<()> { }, channel_capacity: in_process::DEFAULT_IN_PROCESS_CHANNEL_CAPACITY, }) - .await?; - - let event = - wait_for_analytics_event(&server, Duration::from_secs(10), "codex_app_server_started") - .await?; - - assert_app_server_started_event(&event, "in_process"); - client.shutdown().await?; - Ok(()) } fn assert_app_server_started_event(event: &Value, rpc_transport: &str) { @@ -178,12 +226,27 @@ fn assert_app_server_started_event(event: &Value, rpc_transport: &str) { } pub(crate) async fn mount_analytics_capture(server: &MockServer, codex_home: &Path) -> Result<()> { + mount_analytics_endpoint(server).await; + + let config_path = codex_home.join("config.toml"); + let config = std::fs::read_to_string(&config_path)?; + std::fs::write( + config_path, + format!("{config}\n[analytics]\nenabled = true\n"), + )?; + + write_analytics_auth(codex_home) +} + +async fn mount_analytics_endpoint(server: &MockServer) { Mock::given(method("POST")) .and(path("/codex/analytics-events/events")) .respond_with(ResponseTemplate::new(200)) .mount(server) .await; +} +fn write_analytics_auth(codex_home: &Path) -> Result<()> { write_chatgpt_auth( codex_home, ChatGptAuthFixture::new("chatgpt-token") @@ -196,6 +259,31 @@ pub(crate) async fn mount_analytics_capture(server: &MockServer, codex_home: &Pa Ok(()) } +async fn assert_analytics_event_not_received( + server: &MockServer, + wait_duration: Duration, + event_type: &str, +) -> Result<()> { + tokio::time::sleep(wait_duration).await; + let Some(requests) = server.received_requests().await else { + return Ok(()); + }; + for request in &requests { + if request.method != "POST" || request.url.path() != "/codex/analytics-events/events" { + continue; + } + let payload: Value = serde_json::from_slice(&request.body) + .map_err(|err| anyhow::anyhow!("invalid analytics payload: {err}"))?; + if payload["events"] + .as_array() + .is_some_and(|events| events.iter().any(|event| event["event_type"] == event_type)) + { + anyhow::bail!("received unexpected {event_type} analytics event"); + } + } + Ok(()) +} + pub(crate) async fn wait_for_thread_initialized_payload( server: &MockServer, read_timeout: Duration, diff --git a/codex-rs/app-server/tests/suite/v2/plugin_install.rs b/codex-rs/app-server/tests/suite/v2/plugin_install.rs index 2ccf98fa7f..e1db106831 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_install.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_install.rs @@ -763,6 +763,7 @@ async fn plugin_install_tracks_remote_plugin_analytics_event() -> Result<()> { ) .await; configure_remote_plugin_test(codex_home.path(), &server)?; + enable_analytics(codex_home.path())?; mount_remote_plugin_detail(&server, REMOTE_PLUGIN_ID, "1.2.3", Some(&bundle_url)).await; mount_empty_remote_installed_plugins(&server).await; mount_remote_plugin_install(&server, REMOTE_PLUGIN_ID).await; @@ -1333,7 +1334,16 @@ plugins = true fn write_analytics_config(codex_home: &std::path::Path, base_url: &str) -> std::io::Result<()> { std::fs::write( codex_home.join("config.toml"), - format!("chatgpt_base_url = \"{base_url}\"\n"), + format!("chatgpt_base_url = \"{base_url}\"\n\n[analytics]\nenabled = true\n"), + ) +} + +fn enable_analytics(codex_home: &std::path::Path) -> std::io::Result<()> { + let config_path = codex_home.join("config.toml"); + let config = std::fs::read_to_string(&config_path)?; + std::fs::write( + config_path, + format!("{config}\n[analytics]\nenabled = true\n"), ) } 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 31894d940b..84a39ca2b7 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_uninstall.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_uninstall.rs @@ -87,7 +87,7 @@ async fn plugin_uninstall_tracks_analytics_event() -> Result<()> { std::fs::write( codex_home.path().join("config.toml"), format!( - "chatgpt_base_url = \"{}\"\n\n[features]\nplugins = true\n\n[plugins.\"sample-plugin@debug\"]\nenabled = true\n", + "chatgpt_base_url = \"{}\"\n\n[features]\nplugins = true\n\n[analytics]\nenabled = true\n\n[plugins.\"sample-plugin@debug\"]\nenabled = true\n", analytics_server.uri() ), )?;