mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Use the shared HTTP client for TUI network checks (#35821)
## Why TUI update checks and local OSS provider detection constructed their own HTTP clients instead of using Codex's shared client behavior. ## What changed - Route update requests through the configured route-aware client pool while retaining the existing default headers and custom CA fallback. - Probe the hardcoded LM Studio and Ollama loopback endpoints with a shared direct client and a per-request timeout. - Limit the legacy invalid-custom-CA fallback to the default routing policy so system-proxy routing still reports certificate configuration errors. ## Testing Add coverage for local provider probes with invalid `CODEX_CA_CERTIFICATE` and `SSL_CERT_FILE` values, and for custom CA fallback under both routing policies. GitOrigin-RevId: b5c230b61e8964b3f1af3395052361ff716d6ce1
This commit is contained in:
Generated
+1
@@ -4186,6 +4186,7 @@ dependencies = [
|
||||
"codex-feedback",
|
||||
"codex-file-search",
|
||||
"codex-git-utils",
|
||||
"codex-http-client",
|
||||
"codex-install-context",
|
||||
"codex-login",
|
||||
"codex-mcp",
|
||||
|
||||
@@ -65,6 +65,81 @@ async fn streams_request_bodies_without_exposing_reqwest_body() {
|
||||
assert!(requests[0].ends_with("\r\n\r\nhello"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn legacy_custom_ca_fallback_is_limited_to_reqwest_default() {
|
||||
const CHILD_POLICY_ENV: &str = "CODEX_HTTP_CLIENT_POOL_INVALID_CA_TEST_POLICY";
|
||||
|
||||
let Ok(policy_name) = std::env::var(CHILD_POLICY_ENV) else {
|
||||
let temp_dir = tempfile::tempdir().expect("temporary directory should be created");
|
||||
let invalid_ca_path = temp_dir.path().join("invalid-ca.pem");
|
||||
std::fs::write(&invalid_ca_path, "not a PEM certificate")
|
||||
.expect("invalid CA fixture should be written");
|
||||
|
||||
for ca_env in ["CODEX_CA_CERTIFICATE", "SSL_CERT_FILE"] {
|
||||
for policy_name in ["reqwest-default", "respect-system-proxy"] {
|
||||
let output = std::process::Command::new(
|
||||
std::env::current_exe().expect("test executable should be available"),
|
||||
)
|
||||
.arg("--exact")
|
||||
.arg("route_aware_client_pool::tests::legacy_custom_ca_fallback_is_limited_to_reqwest_default")
|
||||
.arg("--nocapture")
|
||||
.env_remove("CODEX_CA_CERTIFICATE")
|
||||
.env_remove("SSL_CERT_FILE")
|
||||
.env(ca_env, &invalid_ca_path)
|
||||
.env(CHILD_POLICY_ENV, policy_name)
|
||||
.output()
|
||||
.expect("isolated CA subprocess should run");
|
||||
|
||||
assert!(
|
||||
output.status.success(),
|
||||
"{policy_name} failed with invalid {ca_env}\nstdout:\n{}\nstderr:\n{}",
|
||||
String::from_utf8_lossy(&output.stdout),
|
||||
String::from_utf8_lossy(&output.stderr),
|
||||
);
|
||||
}
|
||||
}
|
||||
return;
|
||||
};
|
||||
|
||||
let outbound_proxy_policy = match policy_name.as_str() {
|
||||
"reqwest-default" => OutboundProxyPolicy::ReqwestDefault,
|
||||
"respect-system-proxy" => OutboundProxyPolicy::RespectSystemProxy,
|
||||
_ => panic!("unexpected test proxy policy: {policy_name}"),
|
||||
};
|
||||
let pool = RouteAwareClientPool::with_chatgpt_cloudflare_cookies(
|
||||
HttpClientFactory::new(outbound_proxy_policy),
|
||||
ClientRouteClass::Other,
|
||||
)
|
||||
.with_legacy_custom_ca_fallback();
|
||||
|
||||
match outbound_proxy_policy {
|
||||
OutboundProxyPolicy::ReqwestDefault => {
|
||||
let (address, server) = spawn_response_server(vec![
|
||||
"HTTP/1.1 200 OK\r\nContent-Length: 0\r\nConnection: close\r\n\r\n".to_string(),
|
||||
]);
|
||||
let response = pool
|
||||
.get(format!("http://{address}/update"))
|
||||
.send()
|
||||
.await
|
||||
.expect("default-routed request should fall back to system roots");
|
||||
|
||||
assert_eq!(response.status(), StatusCode::OK);
|
||||
let requests = server.join().expect("response server should finish");
|
||||
assert_eq!(requests.len(), 1);
|
||||
}
|
||||
OutboundProxyPolicy::RespectSystemProxy => {
|
||||
let error = pool
|
||||
.client_for_url_with_resolver("http://127.0.0.1/update", |_| async {
|
||||
Ok(OutboundProxyRoute::Direct)
|
||||
})
|
||||
.await
|
||||
.expect_err("system-proxy routes should reject invalid custom CAs");
|
||||
|
||||
assert!(matches!(error, RouteAwareClientPoolError::Build(_)));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn without_url_redacts_transport_error_urls() {
|
||||
let listener = TcpListener::bind(("127.0.0.1", 0)).expect("listener should bind");
|
||||
|
||||
@@ -40,6 +40,7 @@ codex-features = { workspace = true }
|
||||
codex-feedback = { workspace = true }
|
||||
codex-file-search = { workspace = true }
|
||||
codex-git-utils = { workspace = true }
|
||||
codex-http-client = { workspace = true }
|
||||
codex-login = { workspace = true }
|
||||
codex-message-history = { workspace = true }
|
||||
codex-model-provider = { workspace = true }
|
||||
|
||||
@@ -4,6 +4,8 @@ use std::sync::LazyLock;
|
||||
use crate::key_hint;
|
||||
use crate::key_hint::KeyBinding;
|
||||
use crate::key_hint::KeyBindingListExt;
|
||||
use codex_http_client::HttpClient;
|
||||
use codex_http_client::HttpClientBuilder;
|
||||
use codex_model_provider_info::DEFAULT_LMSTUDIO_PORT;
|
||||
use codex_model_provider_info::DEFAULT_OLLAMA_PORT;
|
||||
use codex_model_provider_info::LMSTUDIO_OSS_PROVIDER_ID;
|
||||
@@ -314,9 +316,15 @@ pub(crate) struct OssProviderSelection {
|
||||
}
|
||||
|
||||
pub async fn select_oss_provider() -> io::Result<OssProviderSelection> {
|
||||
// These probes intentionally bypass proxy discovery because both targets are
|
||||
// hardcoded plaintext loopback endpoints. Preserve the legacy custom-CA fallback so an
|
||||
// invalid inherited certificate bundle cannot prevent best-effort provider detection.
|
||||
#[allow(deprecated)]
|
||||
let client = HttpClientBuilder::new().build_direct_with_custom_ca_fallback();
|
||||
|
||||
// Check provider statuses first
|
||||
let lmstudio_status = check_lmstudio_status().await;
|
||||
let ollama_status = check_ollama_status().await;
|
||||
let lmstudio_status = check_lmstudio_status(&client).await;
|
||||
let ollama_status = check_ollama_status(&client).await;
|
||||
|
||||
// Autoselect if only one is running
|
||||
match (&lmstudio_status, &ollama_status) {
|
||||
@@ -369,31 +377,31 @@ pub async fn select_oss_provider() -> io::Result<OssProviderSelection> {
|
||||
result
|
||||
}
|
||||
|
||||
async fn check_lmstudio_status() -> ProviderStatus {
|
||||
match check_port_status(DEFAULT_LMSTUDIO_PORT).await {
|
||||
async fn check_lmstudio_status(client: &HttpClient) -> ProviderStatus {
|
||||
match check_port_status(client, DEFAULT_LMSTUDIO_PORT).await {
|
||||
Ok(true) => ProviderStatus::Running,
|
||||
Ok(false) => ProviderStatus::NotRunning,
|
||||
Err(_) => ProviderStatus::Unknown,
|
||||
}
|
||||
}
|
||||
|
||||
async fn check_ollama_status() -> ProviderStatus {
|
||||
match check_port_status(DEFAULT_OLLAMA_PORT).await {
|
||||
async fn check_ollama_status(client: &HttpClient) -> ProviderStatus {
|
||||
match check_port_status(client, DEFAULT_OLLAMA_PORT).await {
|
||||
Ok(true) => ProviderStatus::Running,
|
||||
Ok(false) => ProviderStatus::NotRunning,
|
||||
Err(_) => ProviderStatus::Unknown,
|
||||
}
|
||||
}
|
||||
|
||||
async fn check_port_status(port: u16) -> io::Result<bool> {
|
||||
let client = reqwest::Client::builder()
|
||||
.timeout(Duration::from_secs(2))
|
||||
.build()
|
||||
.map_err(io::Error::other)?;
|
||||
|
||||
async fn check_port_status(client: &HttpClient, port: u16) -> io::Result<bool> {
|
||||
let url = format!("http://localhost:{port}");
|
||||
|
||||
match client.get(&url).send().await {
|
||||
match client
|
||||
.get(&url)
|
||||
.timeout(Duration::from_secs(2))
|
||||
.send()
|
||||
.await
|
||||
{
|
||||
Ok(response) => Ok(response.status().is_success()),
|
||||
Err(_) => Ok(false), // Connection failed = not running
|
||||
}
|
||||
@@ -414,4 +422,54 @@ mod tests {
|
||||
widget.handle_key_event(KeyEvent::new(KeyCode::Char('h'), KeyModifiers::CONTROL));
|
||||
assert_eq!(widget.selected_option, 0);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn localhost_probe_succeeds_with_invalid_inherited_ca_bundle() {
|
||||
const CHILD_ENV: &str = "CODEX_OSS_SELECTION_INVALID_CA_TEST_CHILD";
|
||||
|
||||
if std::env::var_os(CHILD_ENV).is_none() {
|
||||
let temp_dir = tempfile::tempdir().expect("temporary directory should be created");
|
||||
let invalid_ca_path = temp_dir.path().join("invalid-ca.pem");
|
||||
std::fs::write(&invalid_ca_path, "not a PEM certificate")
|
||||
.expect("invalid CA fixture should be written");
|
||||
|
||||
for ca_env in ["CODEX_CA_CERTIFICATE", "SSL_CERT_FILE"] {
|
||||
let output = std::process::Command::new(
|
||||
std::env::current_exe().expect("test executable should be available"),
|
||||
)
|
||||
.arg("--exact")
|
||||
.arg("oss_selection::tests::localhost_probe_succeeds_with_invalid_inherited_ca_bundle")
|
||||
.arg("--nocapture")
|
||||
.env_remove("CODEX_CA_CERTIFICATE")
|
||||
.env_remove("SSL_CERT_FILE")
|
||||
.env(ca_env, &invalid_ca_path)
|
||||
.env(CHILD_ENV, "1")
|
||||
.output()
|
||||
.expect("isolated CA subprocess should run");
|
||||
|
||||
assert!(
|
||||
output.status.success(),
|
||||
"localhost probe failed with invalid {ca_env}\nstdout:\n{}\nstderr:\n{}",
|
||||
String::from_utf8_lossy(&output.stdout),
|
||||
String::from_utf8_lossy(&output.stderr),
|
||||
);
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
let server = wiremock::MockServer::start().await;
|
||||
wiremock::Mock::given(wiremock::matchers::method("GET"))
|
||||
.respond_with(wiremock::ResponseTemplate::new(200))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
#[allow(deprecated)]
|
||||
let client = HttpClientBuilder::new().build_direct_with_custom_ca_fallback();
|
||||
assert!(
|
||||
check_port_status(&client, server.address().port())
|
||||
.await
|
||||
.expect("localhost provider probe should complete")
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -13,7 +13,10 @@ use crate::updates_cache::read_version_info;
|
||||
use crate::updates_cache::version_filepath;
|
||||
use chrono::Duration;
|
||||
use chrono::Utc;
|
||||
use codex_login::default_client::create_client;
|
||||
use codex_http_client::ClientRouteClass;
|
||||
use codex_http_client::HttpClientFactory;
|
||||
use codex_http_client::RouteAwareClientPool;
|
||||
use codex_login::default_client::default_headers;
|
||||
use serde::Deserialize;
|
||||
use std::path::Path;
|
||||
|
||||
@@ -34,11 +37,12 @@ pub fn get_upgrade_version(config: &Config) -> Option<String> {
|
||||
None => true,
|
||||
Some(info) => info.last_checked_at < Utc::now() - Duration::hours(20),
|
||||
} {
|
||||
let http_client_factory = config.http_client_factory();
|
||||
// Refresh the cached latest version in the background so TUI startup
|
||||
// isn’t blocked by a network call. The UI reads the previously cached
|
||||
// value (if any) for this run; the next run shows the banner if needed.
|
||||
tokio::spawn(async move {
|
||||
check_for_update(&version_file, action)
|
||||
check_for_update(&version_file, action, http_client_factory)
|
||||
.await
|
||||
.inspect_err(|e| tracing::error!("Failed to update version: {e}"))
|
||||
});
|
||||
@@ -67,11 +71,21 @@ struct HomebrewCaskInfo {
|
||||
version: String,
|
||||
}
|
||||
|
||||
async fn check_for_update(version_file: &Path, action: Option<UpdateAction>) -> anyhow::Result<()> {
|
||||
async fn check_for_update(
|
||||
version_file: &Path,
|
||||
action: Option<UpdateAction>,
|
||||
http_client_factory: HttpClientFactory,
|
||||
) -> anyhow::Result<()> {
|
||||
let client_pool = RouteAwareClientPool::with_chatgpt_cloudflare_cookies(
|
||||
http_client_factory,
|
||||
ClientRouteClass::Other,
|
||||
)
|
||||
.with_legacy_custom_ca_fallback();
|
||||
let latest_version = match action {
|
||||
Some(UpdateAction::BrewUpgrade) => {
|
||||
let HomebrewCaskInfo { version } = create_client()
|
||||
let HomebrewCaskInfo { version } = client_pool
|
||||
.get(HOMEBREW_CASK_API_URL)
|
||||
.headers(default_headers())
|
||||
.send()
|
||||
.await?
|
||||
.error_for_status()?
|
||||
@@ -82,9 +96,10 @@ async fn check_for_update(version_file: &Path, action: Option<UpdateAction>) ->
|
||||
Some(UpdateAction::NpmGlobalLatest)
|
||||
| Some(UpdateAction::BunGlobalLatest)
|
||||
| Some(UpdateAction::PnpmGlobalLatest) => {
|
||||
let latest_version = fetch_latest_github_release_version().await?;
|
||||
let package_info = create_client()
|
||||
let latest_version = fetch_latest_github_release_version(&client_pool).await?;
|
||||
let package_info = client_pool
|
||||
.get(npm_registry::PACKAGE_URL)
|
||||
.headers(default_headers())
|
||||
.send()
|
||||
.await?
|
||||
.error_for_status()?
|
||||
@@ -94,7 +109,7 @@ async fn check_for_update(version_file: &Path, action: Option<UpdateAction>) ->
|
||||
latest_version
|
||||
}
|
||||
Some(UpdateAction::StandaloneUnix) | Some(UpdateAction::StandaloneWindows) | None => {
|
||||
fetch_latest_github_release_version().await?
|
||||
fetch_latest_github_release_version(&client_pool).await?
|
||||
}
|
||||
};
|
||||
|
||||
@@ -114,11 +129,14 @@ async fn check_for_update(version_file: &Path, action: Option<UpdateAction>) ->
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn fetch_latest_github_release_version() -> anyhow::Result<String> {
|
||||
async fn fetch_latest_github_release_version(
|
||||
client_pool: &RouteAwareClientPool,
|
||||
) -> anyhow::Result<String> {
|
||||
let ReleaseInfo {
|
||||
tag_name: latest_tag_name,
|
||||
} = create_client()
|
||||
} = client_pool
|
||||
.get(LATEST_RELEASE_URL)
|
||||
.headers(default_headers())
|
||||
.send()
|
||||
.await?
|
||||
.error_for_status()?
|
||||
|
||||
Reference in New Issue
Block a user