mirror of
https://github.com/openai/codex.git
synced 2026-10-19 17:28:46 +00:00
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
This commit is contained in:
@@ -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;
|
||||
|
||||
@@ -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(())
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -128,6 +128,21 @@ impl WorkspaceRoutingResolver for AccountRequestProcessor {
|
||||
}
|
||||
|
||||
impl AccountRequestProcessor {
|
||||
pub(super) async fn read_account_for_background_workspace_routing(
|
||||
&self,
|
||||
) -> Result<Option<AccountRead>, 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
|
||||
{
|
||||
|
||||
+127
-1
@@ -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")]
|
||||
|
||||
Reference in New Issue
Block a user