From 751e7edd0b2012ca831c7fdcdd3400631e4cfe14 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 9 Oct 2026 16:27:11 +0000 Subject: [PATCH] Skip background workspace routing reads without ChatGPT auth (#52635) ## Why Background workspace routing reads reload configuration even when signed out or using an API key, although routing requires ChatGPT authentication. ## What changed Check cached authentication before reading accounts for workspace routing at app-server startup and connection initialization. Skip these background reads without ChatGPT authentication, while preserving configuration reloads for explicit `account/read` requests. ## Testing Add regression tests covering signed-out and API-key startup and connection initialization, explicit `account/read` configuration reloads, and routing after ChatGPT sign-in, including propagation of configuration errors. GitOrigin-RevId: 8b90ee609ed24f8618cd21f160eb7001703b8bd1 --- codex-rs/app-server/src/message_processor.rs | 4 + ...ssage_processor_workspace_routing_tests.rs | 152 ++++++++++++++++++ .../request_processors/account_processor.rs | 4 +- .../account_processor/workspace_routing.rs | 19 ++- .../workspace_routing_tests.rs | 128 ++++++++++++++- 5 files changed, 304 insertions(+), 3 deletions(-) create mode 100644 codex-rs/app-server/src/message_processor_workspace_routing_tests.rs diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 7a1c09aa48..cf0e741882 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -1917,3 +1917,7 @@ mod gateway_oauth_tests; #[cfg(test)] #[path = "message_processor_thread_lifecycle_tests.rs"] mod thread_lifecycle_tests; + +#[cfg(test)] +#[path = "message_processor_workspace_routing_tests.rs"] +mod workspace_routing_tests; diff --git a/codex-rs/app-server/src/message_processor_workspace_routing_tests.rs b/codex-rs/app-server/src/message_processor_workspace_routing_tests.rs new file mode 100644 index 0000000000..db478a9960 --- /dev/null +++ b/codex-rs/app-server/src/message_processor_workspace_routing_tests.rs @@ -0,0 +1,152 @@ +//! Exercises background routing call sites and explicit account reads through JSON-RPC. + +use super::message_processor_tracing_tests::TEST_CONNECTION_ID; +use super::message_processor_tracing_tests::read_response; +use super::*; +use codex_app_server_protocol::GetAccountResponse; +use codex_app_server_protocol::InitializeResponse; +use codex_config::CloudConfigBundleLoader; +use codex_config::LoaderOverrides; +use codex_config::types::AuthCredentialsStoreMode; +use codex_login::AuthKeyringBackendKind; +use pretty_assertions::assert_eq; +use serde_json::json; +use test_case::test_case; +use tokio::sync::mpsc; + +#[test_case(None; "signed_out")] +#[test_case(Some("dummy"); "api_key")] +#[tokio::test] +async fn background_routing_call_sites_skip_config(api_key: Option<&str>) -> anyhow::Result<()> { + let home = tempfile::tempdir()?; + if let Some(api_key) = api_key { + codex_login::login_with_api_key( + home.path(), + api_key, + AuthCredentialsStoreMode::File, + AuthKeyringBackendKind::default(), + )?; + } + let config = Arc::new( + codex_core::config::ConfigBuilder::default() + .codex_home(home.path().to_path_buf()) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .build() + .await?, + ); + let auth_manager = Arc::new( + AuthManager::new( + home.path().to_path_buf(), + /*enable_codex_api_key_env*/ false, + AuthCredentialsStoreMode::File, + /*forced_chatgpt_workspace_id*/ None, + Some(config.chatgpt_base_url.clone()), + AuthKeyringBackendKind::default(), + config.auth_config().auth_route_config, + ) + .await, + ); + let (loads_tx, mut loads_rx) = mpsc::unbounded_channel(); + let loader = CloudConfigBundleLoader::from_getter(move || { + loads_tx.send(()).expect("config-load observer is alive"); + async { Ok(None) } + }); + let config_manager = ConfigManager::new_for_tests( + home.path().to_path_buf(), + Vec::new(), + LoaderOverrides::without_managed_config_for_tests(), + loader, + ); + let (outgoing_tx, mut outgoing_rx) = mpsc::channel(/*buffer*/ 16); + let analytics_events_client = AnalyticsEventsClient::disabled(); + let outgoing = Arc::new(OutgoingMessageSender::new( + outgoing_tx, + analytics_events_client.clone(), + )); + let processor = Arc::new(MessageProcessor::new(MessageProcessorArgs { + outgoing, + analytics_events_client, + arg0_paths: Arg0DispatchPaths::default(), + config, + config_manager, + environment_manager: Arc::new(EnvironmentManager::default_for_tests()), + feedback: CodexFeedback::new(), + log_db: None, + state_db: None, + config_warnings: Vec::new(), + session_source: SessionSource::VSCode, + user_verification: Arc::new(crate::user_verification::Service::new(Arc::clone( + &auth_manager, + ))), + auth_manager, + installation_id: "11111111-1111-4111-8111-111111111111".to_string(), + code_mode_session_provider: None, + rpc_transport: AppServerRpcTransport::Stdio, + remote_control_handle: None, + // Isolate routing from unrelated plugin startup configuration loads. + plugin_startup_tasks: None, + })); + // The model catalog has its own legitimate config refresh, outside routing. + processor.models_refresh_worker.shutdown(); + + // Observe the task scheduled by real processor construction, not the helper. + assert!( + tokio::time::timeout(Duration::from_secs(/*secs*/ 1), loads_rx.recv()) + .await + .is_err(), + "startup routing entered configuration loading" + ); + let session = Arc::new(ConnectionSessionState::new( + crate::transport::ConnectionOrigin::Stdio, + )); + processor + .process_request( + TEST_CONNECTION_ID, + serde_json::from_value(json!({ + "id": 1, "method": "initialize", "params": { + "clientInfo": {"name": "workspace-routing-test", "version": "1"} + } + }))?, + &crate::transport::AppServerTransport::Stdio, + Arc::clone(&session), + ) + .await; + let _: InitializeResponse = read_response(&mut outgoing_rx, /*request_id*/ 1).await; + // Complete the same post-initialize hook used by the real transports. + processor + .connection_initialized(TEST_CONNECTION_ID, /*request_attestation*/ false) + .await; + assert!( + tokio::time::timeout(Duration::from_secs(/*secs*/ 1), loads_rx.recv()) + .await + .is_err(), + "connection initialization routing entered configuration loading" + ); + + processor + .process_request( + TEST_CONNECTION_ID, + serde_json::from_value(json!({ + "id": 2, "method": "account/read", "params": {"refreshToken": false} + }))?, + &crate::transport::AppServerTransport::Stdio, + session, + ) + .await; + let response: GetAccountResponse = read_response(&mut outgoing_rx, /*request_id*/ 2).await; + assert_eq!( + response, + GetAccountResponse { + account: api_key.map(|_| codex_app_server_protocol::Account::ApiKey {}), + requires_openai_auth: true, + workspace_routing: None, + } + ); + assert_eq!( + loads_rx.try_recv(), + Ok(()), + "explicit account/read must reload config" + ); + processor.shutdown_threads().await; + Ok(()) +} diff --git a/codex-rs/app-server/src/request_processors/account_processor.rs b/codex-rs/app-server/src/request_processors/account_processor.rs index eeaba7882e..b8d56053b1 100644 --- a/codex-rs/app-server/src/request_processors/account_processor.rs +++ b/codex-rs/app-server/src/request_processors/account_processor.rs @@ -150,7 +150,9 @@ impl AccountRequestProcessor { .set_workspace_routing_resolver(Arc::downgrade(&resolver)); let startup = processor.clone(); tokio::spawn(async move { - let _ = startup.read_account(/*request*/ None).await; + let _ = startup + .read_account_for_background_workspace_routing() + .await; }); processor } diff --git a/codex-rs/app-server/src/request_processors/account_processor/workspace_routing.rs b/codex-rs/app-server/src/request_processors/account_processor/workspace_routing.rs index 3eedf082f5..90c8781b5b 100644 --- a/codex-rs/app-server/src/request_processors/account_processor/workspace_routing.rs +++ b/codex-rs/app-server/src/request_processors/account_processor/workspace_routing.rs @@ -128,6 +128,21 @@ impl WorkspaceRoutingResolver for AccountRequestProcessor { } impl AccountRequestProcessor { + pub(super) async fn read_account_for_background_workspace_routing( + &self, + ) -> Result, AccountReadError> { + if !self + .auth_manager + .auth_cached() + .as_ref() + .is_some_and(CodexAuth::is_chatgpt_auth) + { + return Ok(None); + } + + self.read_account(/*request*/ None).await.map(Some) + } + pub(crate) fn notify_workspace_routing_to_connection(&self, connection_id: ConnectionId) { let processor = self.clone(); let auth_changes = self.auth_manager.auth_change_state_receiver(); @@ -136,7 +151,9 @@ impl AccountRequestProcessor { if auth_changes.borrow().owner_generation != owner_generation { return; } - if let Ok(response) = processor.read_account(/*request*/ None).await + if let Ok(Some(response)) = processor + .read_account_for_background_workspace_routing() + .await && response.workspace_routing.is_some() && auth_changes.borrow().owner_generation == owner_generation { diff --git a/codex-rs/app-server/src/request_processors/account_processor/workspace_routing_tests.rs b/codex-rs/app-server/src/request_processors/account_processor/workspace_routing_tests.rs index a7c1b8c731..cc06d5086e 100644 --- a/codex-rs/app-server/src/request_processors/account_processor/workspace_routing_tests.rs +++ b/codex-rs/app-server/src/request_processors/account_processor/workspace_routing_tests.rs @@ -1,10 +1,136 @@ -//! Origin resolution and strict discovery validation. +//! Origin resolution, strict discovery validation, and background read eligibility. use super::*; use pretty_assertions::assert_eq; use serde_json::Value; use test_case::test_case; +#[test_case(None; "signed_out")] +#[test_case(Some("dummy"); "api_key")] +#[tokio::test] +async fn background_routing_skips_config_until_chatgpt_sign_in( + api_key: Option<&str>, +) -> anyhow::Result<()> { + use app_test_support::ChatGptAuthFixture; + use app_test_support::write_chatgpt_auth; + use codex_config::CloudConfigBundleLoader; + use codex_config::LoaderOverrides; + use codex_config::types::AuthCredentialsStoreMode; + use codex_exec_server::EnvironmentManager; + use codex_login::AuthKeyringBackendKind; + use std::sync::atomic::AtomicUsize; + use std::sync::atomic::Ordering; + + let home = tempfile::tempdir()?; + let backend = wiremock::MockServer::start().await; + app_test_support::mount_workspace_routing(&backend).await; + let loads = Arc::new(AtomicUsize::new(/*v*/ 0)); + let calls = Arc::clone(&loads); + let loader = CloudConfigBundleLoader::from_getter(move || { + calls.fetch_add(/*val*/ 1, Ordering::SeqCst); + async { Ok(None) } + }); + let manager = ConfigManager::new_for_tests( + home.path().to_path_buf(), + vec![( + "chatgpt_base_url".into(), + format!("{}/backend-api", backend.uri()).into(), + )], + LoaderOverrides::without_managed_config_for_tests(), + loader, + ); + let config = Arc::new( + manager + .load_latest_config(Some(home.path().to_path_buf())) + .await?, + ); + if let Some(api_key) = api_key { + codex_login::login_with_api_key( + home.path(), + api_key, + AuthCredentialsStoreMode::File, + AuthKeyringBackendKind::default(), + )?; + } + let auth_manager = Arc::new( + AuthManager::new( + home.path().to_path_buf(), + /*enable_codex_api_key_env*/ false, + AuthCredentialsStoreMode::File, + /*forced_chatgpt_workspace_id*/ None, + Some(config.chatgpt_base_url.clone()), + AuthKeyringBackendKind::default(), + config.auth_config().auth_route_config, + ) + .await, + ); + let threads = Arc::new( + codex_core::test_support::thread_manager_with_models_provider_and_home( + CodexAuth::from_api_key("dummy"), + config.model_provider.clone(), + config.codex_home.to_path_buf(), + Arc::new(EnvironmentManager::default_for_tests()), + ), + ); + let (sender, _receiver) = tokio::sync::mpsc::channel(16); + let outgoing = Arc::new(OutgoingMessageSender::new( + sender, + codex_analytics::AnalyticsEventsClient::disabled(), + )); + let processor = AccountRequestProcessor::new( + Arc::clone(&auth_manager), + threads, + outgoing, + config, + manager, + ); + let initial_loads = loads.load(Ordering::SeqCst); + for _ in 0..3 { + assert!( + processor + .read_account_for_background_workspace_routing() + .await? + .is_none() + ); + } + assert_eq!(loads.load(Ordering::SeqCst), initial_loads); + + processor.read_account(/*request*/ None).await?; + assert!(loads.load(Ordering::SeqCst) > initial_loads); + + write_chatgpt_auth( + home.path(), + ChatGptAuthFixture::new("access-chatgpt").account_id("account-123"), + AuthCredentialsStoreMode::File, + )?; + assert!(auth_manager.reload().await); + let initial_loads = loads.load(Ordering::SeqCst); + let read = processor + .read_account_for_background_workspace_routing() + .await? + .unwrap(); + assert_eq!( + read.workspace_routing, + Some(WorkspaceRouting { + chatgpt_account_id: "account-123".into(), + backend_origin: "https://chatgpt.com".into(), + account_routing_override: "NO_CONSTRAINT".into(), + }) + ); + assert!(loads.load(Ordering::SeqCst) > initial_loads); + + std::fs::write(home.path().join("config.toml"), "invalid = [")?; + assert!(matches!( + processor + .read_account_for_background_workspace_routing() + .await, + Err(AccountReadError::Routing( + WorkspaceRoutingError::RequirementsLoad + )) + )); + Ok(()) +} + #[test_case(Some("https://gov.chatgpt.com/backend-api/"), "NO_CONSTRAINT", "https://gov.chatgpt.com"; "configured_only")] #[test_case(None, "https://gov.chatgpt.com", "https://gov.chatgpt.com"; "discovered_only")] #[test_case(Some("https://GOV.chatgpt.com:443/backend-api/"), "https://gov.chatgpt.com", "https://gov.chatgpt.com"; "matching_effective_port")]