diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 1dc5999095..0db99631bb 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -4268,6 +4268,7 @@ dependencies = [ "dirs", "dunce", "futures", + "http 1.4.0", "image", "insta", "itertools 0.14.0", @@ -4280,7 +4281,6 @@ dependencies = [ "ratatui", "ratatui-macros", "regex-lite", - "reqwest 0.12.28", "rmcp", "serde", "serde_json", diff --git a/codex-rs/deny.toml b/codex-rs/deny.toml index 8c57fb289d..2dbb0b0430 100644 --- a/codex-rs/deny.toml +++ b/codex-rs/deny.toml @@ -245,7 +245,6 @@ deny = [ "codex-otel", "codex-protocol", "codex-responses-api-proxy", - "codex-tui", # Third-party crates that own their reqwest integration. These are not part of the # first-party migration count above. "oauth2", diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index e16a9c06c2..8d79afc202 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -87,7 +87,6 @@ ratatui = { workspace = true, features = [ ] } ratatui-macros = { workspace = true } regex-lite = { workspace = true } -reqwest = { workspace = true, features = ["blocking", "json"] } rmcp = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true, features = ["preserve_order"] } @@ -156,6 +155,7 @@ codex-utils-cargo-bin = { workspace = true } assert_matches = { workspace = true } chrono = { workspace = true, features = ["serde"] } futures = { workspace = true } +http = { workspace = true } insta = { workspace = true } pretty_assertions = { workspace = true } rand = { workspace = true } diff --git a/codex-rs/tui/src/app/pets.rs b/codex-rs/tui/src/app/pets.rs index eef38b894e..89e6a6e2a7 100644 --- a/codex-rs/tui/src/app/pets.rs +++ b/codex-rs/tui/src/app/pets.rs @@ -82,18 +82,18 @@ impl App { let frame_requester = tui.frame_requester(); let animations_enabled = self.config.animations; let tx = self.app_event_tx.clone(); - std::mem::drop(tokio::task::spawn_blocking(move || { - let result = crate::pets::ensure_builtin_pack_for_pet(&pet_id, &codex_home) - .and_then(|()| { - crate::pets::AmbientPet::load( - Some(&pet_id), - &codex_home, - frame_requester, - animations_enabled, - ) - }) - .map(Some) - .map_err(|err| err.to_string()); + let pet_http_client = self.chat_widget.pet_http_client.clone(); + std::mem::drop(tokio::spawn(async move { + let result = crate::pets::load_pet_with_assets( + pet_id.clone(), + codex_home, + frame_requester, + animations_enabled, + &pet_http_client, + ) + .await + .map(Some) + .map_err(|err| err.to_string()); tx.send(AppEvent::PetSelectionLoaded { request_id, pet_id, diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 9b716846f6..d56c63d2f5 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -637,6 +637,8 @@ pub(crate) struct ChatWidget { review: ReviewState, // Active hook runs render in a dedicated live cell so they can run alongside tools. active_hook_cell: Option, + // Reused for built-in pet CDN requests so redirects remain route-aware. + pub(crate) pet_http_client: codex_http_client::RouteAwareClientPool, // Ambient companion rendered over the transcript area, never inside the footer rows. ambient_pet: Option, pet_picker_preview_state: crate::pets::PetPickerPreviewState, diff --git a/codex-rs/tui/src/chatwidget/constructor.rs b/codex-rs/tui/src/chatwidget/constructor.rs index eb979ceff1..302a80ab75 100644 --- a/codex-rs/tui/src/chatwidget/constructor.rs +++ b/codex-rs/tui/src/chatwidget/constructor.rs @@ -85,11 +85,16 @@ impl ChatWidget { &chat_keymap.edit_queued_message, current_terminal_info, ); + let pet_http_client = codex_http_client::RouteAwareClientPool::new( + config.http_client_factory(), + codex_http_client::ClientRouteClass::Other, + ); pets::start_configured_pet_load_if_needed( &config, /*ambient_pet_missing*/ true, frame_requester.clone(), app_event_tx.clone(), + pet_http_client.clone(), ); let mut widget = Self { app_event_tx: app_event_tx.clone(), @@ -184,6 +189,7 @@ impl ChatWidget { status_state: StatusState::default(), review: ReviewState::default(), active_hook_cell: None, + pet_http_client, ambient_pet: None, pet_picker_preview_state: crate::pets::PetPickerPreviewState::default(), pet_picker_preview_pet: None, diff --git a/codex-rs/tui/src/chatwidget/pets.rs b/codex-rs/tui/src/chatwidget/pets.rs index 71583414f4..5bfed3d202 100644 --- a/codex-rs/tui/src/chatwidget/pets.rs +++ b/codex-rs/tui/src/chatwidget/pets.rs @@ -26,6 +26,7 @@ pub(super) fn start_configured_pet_load_if_needed( ambient_pet_missing: bool, frame_requester: FrameRequester, app_event_tx: AppEventSender, + pet_http_client: codex_http_client::RouteAwareClientPool, ) { let Some(pet_id) = config.tui_pet.clone() else { return; @@ -36,20 +37,26 @@ pub(super) fn start_configured_pet_load_if_needed( let codex_home = config.codex_home.clone(); let animations_enabled = config.animations; - spawn_pet_load(move || { - let result = crate::pets::ensure_builtin_pack_for_pet(&pet_id, &codex_home) - .and_then(|()| { - crate::pets::AmbientPet::load( - Some(&pet_id), - &codex_home, - frame_requester, - animations_enabled, - ) - }) + let event_pet_id = pet_id.clone(); + spawn_pet_load( + async move { + crate::pets::load_pet_with_assets( + pet_id, + codex_home, + frame_requester, + animations_enabled, + &pet_http_client, + ) + .await .map(Some) - .map_err(|err| err.to_string()); - app_event_tx.send(AppEvent::ConfiguredPetLoaded { pet_id, result }); - }); + .map_err(|err| err.to_string()) + }, + app_event_tx, + move |result| AppEvent::ConfiguredPetLoaded { + pet_id: event_pet_id, + result, + }, + ); } impl ChatWidget { @@ -234,19 +241,22 @@ impl ChatWidget { let codex_home = self.config.codex_home.clone(); let frame_requester = self.frame_requester.clone(); let tx = self.app_event_tx.clone(); - spawn_pet_load(move || { - let result = crate::pets::ensure_builtin_pack_for_pet(&pet_id, &codex_home) - .and_then(|()| { - crate::pets::AmbientPet::load( - Some(&pet_id), - &codex_home, - frame_requester, - /*animations_enabled*/ false, - ) - }) - .map_err(|err| err.to_string()); - tx.send(AppEvent::PetPreviewLoaded { request_id, result }); - }); + let pet_http_client = self.pet_http_client.clone(); + spawn_pet_load( + async move { + crate::pets::load_pet_with_assets( + pet_id, + codex_home, + frame_requester, + /*animations_enabled*/ false, + &pet_http_client, + ) + .await + .map_err(|err| err.to_string()) + }, + tx, + move |result| AppEvent::PetPreviewLoaded { request_id, result }, + ); } pub(crate) fn finish_pet_picker_preview_load( @@ -326,10 +336,34 @@ impl ChatWidget { } } -fn spawn_pet_load(f: impl FnOnce() + Send + 'static) { +fn spawn_pet_load( + future: impl std::future::Future> + Send + 'static, + app_event_tx: AppEventSender, + completion_event: impl FnOnce(Result) -> AppEvent + Send + 'static, +) where + T: Send + 'static, +{ if let Ok(handle) = tokio::runtime::Handle::try_current() { - std::mem::drop(handle.spawn_blocking(f)); + std::mem::drop(handle.spawn(async move { + app_event_tx.send(completion_event(future.await)); + })); } else { - let _ = std::thread::spawn(f); + let _ = std::thread::spawn(move || { + let result = match tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + { + Ok(runtime) => runtime.block_on(future), + Err(err) => { + tracing::warn!(error = %err, "failed to start pet load runtime"); + Err(format!("failed to start pet load runtime: {err}")) + } + }; + app_event_tx.send(completion_event(result)); + }); } } + +#[cfg(test)] +#[path = "pets_tests.rs"] +mod tests; diff --git a/codex-rs/tui/src/chatwidget/pets_tests.rs b/codex-rs/tui/src/chatwidget/pets_tests.rs new file mode 100644 index 0000000000..796339acfc --- /dev/null +++ b/codex-rs/tui/src/chatwidget/pets_tests.rs @@ -0,0 +1,86 @@ +use super::*; +use codex_utils_absolute_path::AbsolutePathBuf; +use pretty_assertions::assert_eq; + +#[test] +fn pet_load_without_runtime_sends_completion_event() { + let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel(); + let app_event_tx = AppEventSender::new(tx); + + spawn_pet_load( + async { Ok::, String>(None) }, + app_event_tx, + |result| AppEvent::ConfiguredPetLoaded { + pet_id: crate::pets::DEFAULT_PET_ID.to_string(), + result, + }, + ); + + match rx.blocking_recv().expect("pet load completion event") { + AppEvent::ConfiguredPetLoaded { pet_id, result } => { + assert_eq!(pet_id, crate::pets::DEFAULT_PET_ID); + assert!(result.expect("successful pet load").is_none()); + } + event => panic!("expected configured pet completion, got {event:?}"), + } +} + +#[tokio::test] +async fn shared_pet_load_uses_cached_builtin_assets() { + let (chat, _tx, _rx, _op_rx) = + crate::chatwidget::tests::make_chatwidget_manual_with_sender().await; + let codex_home = tempfile::tempdir().unwrap(); + crate::pets::write_test_pack(codex_home.path()); + + crate::pets::load_pet_with_assets( + crate::pets::DEFAULT_PET_ID.to_string(), + AbsolutePathBuf::from_absolute_path(codex_home.path()).expect("absolute temporary path"), + chat.frame_requester.clone(), + /*animations_enabled*/ false, + &chat.pet_http_client, + ) + .await + .expect("load cached built-in pet"); + + assert!( + codex_home + .path() + .join("cache") + .join("tui-pets") + .join("frame-cache") + .join(crate::pets::DEFAULT_PET_ID) + .is_dir() + ); +} + +#[tokio::test] +async fn stale_pet_preview_completion_keeps_current_preview() { + let (mut chat, _tx, _rx, _op_rx) = + crate::chatwidget::tests::make_chatwidget_manual_with_sender().await; + chat.pet_picker_preview_request_id = 2; + chat.pet_picker_preview_pet = Some(crate::pets::test_ambient_pet( + chat.frame_requester.clone(), + /*animations_enabled*/ false, + )); + + chat.finish_pet_picker_preview_load(/*request_id*/ 1, Err("stale preview".to_string())); + + assert!(chat.pet_picker_preview_pet.is_some()); + assert_eq!(chat.pet_picker_preview_request_id, 2); +} + +#[tokio::test] +async fn stale_pet_selection_completion_keeps_current_loading_popup() { + let (mut chat, _tx, _rx, _op_rx) = + crate::chatwidget::tests::make_chatwidget_manual_with_sender().await; + let current_request_id = chat.show_pet_selection_loading_popup(); + let stale_request_id = current_request_id.wrapping_sub(/*rhs*/ 1); + + assert!(!chat.finish_pet_selection_loading_popup(stale_request_id)); + assert_eq!( + chat.bottom_pane.active_view_id(), + Some(crate::chatwidget::PET_SELECTION_LOADING_VIEW_ID) + ); + assert!(chat.finish_pet_selection_loading_popup(current_request_id)); + assert_eq!(chat.bottom_pane.active_view_id(), None); +} diff --git a/codex-rs/tui/src/history_cell/tests.rs b/codex-rs/tui/src/history_cell/tests.rs index 9010894d69..dce26f9507 100644 --- a/codex-rs/tui/src/history_cell/tests.rs +++ b/codex-rs/tui/src/history_cell/tests.rs @@ -20,10 +20,10 @@ use codex_protocol::account::PlanType; use codex_protocol::error::UnexpectedResponseError; use codex_protocol::parse_command::ParsedCommand; use dirs::home_dir; +use http::StatusCode; use pretty_assertions::assert_eq; use ratatui::buffer::Buffer; use ratatui::layout::Rect; -use reqwest::StatusCode; use serde_json::json; use std::collections::HashMap; use std::path::PathBuf; diff --git a/codex-rs/tui/src/pets/asset_pack.rs b/codex-rs/tui/src/pets/asset_pack.rs index 7035d1dea2..1b969cebea 100644 --- a/codex-rs/tui/src/pets/asset_pack.rs +++ b/codex-rs/tui/src/pets/asset_pack.rs @@ -11,7 +11,6 @@ //! built-in pet is safe to persist to config. use std::fs; -use std::io::Read; use std::path::Path; use std::path::PathBuf; use std::time::Duration; @@ -19,6 +18,7 @@ use std::time::Duration; use anyhow::Context; use anyhow::Result; use anyhow::bail; +use codex_http_client::RouteAwareClientPool; use url::Url; use uuid::Uuid; @@ -42,44 +42,58 @@ pub(crate) fn builtin_spritesheet_path(codex_home: &Path, file: &str) -> PathBuf /// validates the decoded image dimensions, and installs it atomically. Callers /// should treat any error here as "the asset is unavailable", not as a partial /// install they can safely ignore. -pub(crate) fn ensure_builtin_pet(codex_home: &Path, pet: catalog::BuiltinPet) -> Result<()> { +pub(crate) async fn ensure_builtin_pet( + codex_home: &Path, + pet: catalog::BuiltinPet, + http_client: &RouteAwareClientPool, +) -> Result<()> { let destination = builtin_spritesheet_path(codex_home, pet.spritesheet_file); - if validate_cached_spritesheet(&destination).is_ok() { + let cache_destination = destination.clone(); + let cache_valid = tokio::task::spawn_blocking(move || { + validate_cached_spritesheet(&cache_destination).is_ok() + }) + .await + .context("join pet spritesheet cache validation task")?; + if cache_valid { return Ok(()); } let url = builtin_pet_url(pet)?; - let bytes = download_bytes_with_limit(&url, PET_MAX_DOWNLOAD_BYTES)?; - let parent = destination - .parent() - .context("pet spritesheet path should include an assets directory")?; - fs::create_dir_all(parent).with_context(|| format!("create {}", parent.display()))?; + let bytes = download_bytes_with_limit(http_client, &url, PET_MAX_DOWNLOAD_BYTES).await?; + tokio::task::spawn_blocking(move || { + let parent = destination + .parent() + .context("pet spritesheet path should include an assets directory")?; + fs::create_dir_all(parent).with_context(|| format!("create {}", parent.display()))?; - let staging = destination.with_file_name(format!( - ".{}.download-{}.webp", - pet.spritesheet_file, - Uuid::new_v4() - )); - fs::write(&staging, &bytes).with_context(|| format!("write {}", staging.display()))?; - if let Err(err) = validate_cached_spritesheet(&staging) { - let _ = fs::remove_file(&staging); - return Err(err); - } + let staging = destination.with_file_name(format!( + ".{}.download-{}.webp", + pet.spritesheet_file, + Uuid::new_v4() + )); + fs::write(&staging, &bytes).with_context(|| format!("write {}", staging.display()))?; + if let Err(err) = validate_cached_spritesheet(&staging) { + let _ = fs::remove_file(&staging); + return Err(err); + } - if install_downloaded_spritesheet(&staging, &destination).is_ok() { - return Ok(()); - } + if install_downloaded_spritesheet(&staging, &destination).is_ok() { + return Ok(()); + } - if validate_cached_spritesheet(&destination).is_ok() { - let _ = fs::remove_file(&staging); - return Ok(()); - } + if validate_cached_spritesheet(&destination).is_ok() { + let _ = fs::remove_file(&staging); + return Ok(()); + } - if destination.exists() { - fs::remove_file(&destination) - .with_context(|| format!("remove {}", destination.display()))?; - } - install_downloaded_spritesheet(&staging, &destination) + if destination.exists() { + fs::remove_file(&destination) + .with_context(|| format!("remove {}", destination.display()))?; + } + install_downloaded_spritesheet(&staging, &destination) + }) + .await + .context("join pet spritesheet install task")? } fn builtin_pet_url(pet: catalog::BuiltinPet) -> Result { @@ -92,14 +106,17 @@ fn pack_dir(codex_home: &Path) -> PathBuf { codex_home.join(PET_PACK_DIR).join(PET_PACK_VERSION) } -fn download_bytes_with_limit(url: &str, max_bytes: u64) -> Result> { +async fn download_bytes_with_limit( + http_client: &RouteAwareClientPool, + url: &str, + max_bytes: u64, +) -> Result> { validate_download_url(url)?; - let response = reqwest::blocking::Client::builder() - .timeout(PET_DOWNLOAD_TIMEOUT) - .build() - .context("build pet asset download client")? + let mut response = http_client .get(url) + .timeout(PET_DOWNLOAD_TIMEOUT) .send() + .await .with_context(|| format!("download pet asset from {url}"))? .error_for_status() .with_context(|| format!("download pet asset from {url}"))?; @@ -110,16 +127,29 @@ fn download_bytes_with_limit(url: &str, max_bytes: u64) -> Result> { } let mut bytes = Vec::new(); - response - .take(max_bytes.saturating_add(/*rhs*/ 1)) - .read_to_end(&mut bytes) - .with_context(|| format!("read pet asset download from {url}"))?; - if bytes.len() as u64 > max_bytes { - bail!("pet asset download from {url} exceeded {max_bytes} bytes"); + while let Some(chunk) = response + .chunk() + .await + .with_context(|| format!("read pet asset download from {url}"))? + { + append_download_chunk(&mut bytes, &chunk, max_bytes, url)?; } Ok(bytes) } +fn append_download_chunk( + bytes: &mut Vec, + chunk: &[u8], + max_bytes: u64, + url: &str, +) -> Result<()> { + if (bytes.len() as u64).saturating_add(chunk.len() as u64) > max_bytes { + bail!("pet asset download from {url} exceeded {max_bytes} bytes"); + } + bytes.extend_from_slice(chunk); + Ok(()) +} + fn install_downloaded_spritesheet(staging: &Path, destination: &Path) -> Result<()> { fs::rename(staging, destination).with_context(|| format!("install {}", destination.display())) } @@ -175,6 +205,22 @@ mod tests { ); } + #[test] + fn oversized_download_chunk_is_rejected() { + let url = "https://example.com/pet.webp"; + let mut bytes = Vec::new(); + + append_download_chunk(&mut bytes, b"1234", /*max_bytes*/ 8, url).unwrap(); + let error = append_download_chunk(&mut bytes, b"56789", /*max_bytes*/ 8, url) + .expect_err("chunk should exceed the download limit"); + + assert_eq!( + error.to_string(), + "pet asset download from https://example.com/pet.webp exceeded 8 bytes" + ); + assert_eq!(bytes, b"1234"); + } + #[test] fn write_test_pack_installs_all_builtins() { let dir = tempfile::tempdir().unwrap(); diff --git a/codex-rs/tui/src/pets/mod.rs b/codex-rs/tui/src/pets/mod.rs index 74d8d37380..f12313ff4c 100644 --- a/codex-rs/tui/src/pets/mod.rs +++ b/codex-rs/tui/src/pets/mod.rs @@ -9,9 +9,9 @@ //! resolving a selected pet id, preparing frames for terminal image protocols, //! rendering the ambient sprite and picker preview, and preserving enough //! metadata for `/pets` to behave like a first-class configuration surface. -//! It does not own config persistence or popup orchestration; callers must -//! ensure a built-in asset exists before loading it and must persist the final -//! selection only after the load succeeds. +//! It prepares built-in assets before loading pets, but does not own config +//! persistence or popup orchestration; callers must persist the final selection +//! only after the load succeeds. use std::io::Write; @@ -27,6 +27,10 @@ mod sixel; use anyhow::Context; use anyhow::Result; +use codex_http_client::RouteAwareClientPool; +use codex_utils_absolute_path::AbsolutePathBuf; + +use crate::tui::FrameRequester; pub(crate) use ambient::AmbientPet; pub(crate) use ambient::AmbientPetDraw; @@ -53,20 +57,41 @@ pub(crate) const DISABLED_PET_ID: &str = "disabled"; /// Ensure that a selected built-in pet has a locally cached spritesheet. /// /// Custom pets are intentionally a no-op here because their source of truth is -/// already local. Callers should invoke this before loading a built-in pet for -/// preview or selection; skipping it would make first-use preview and -/// persistence failures depend on deeper image-loading errors instead of the -/// asset-fetch boundary. -pub(crate) fn ensure_builtin_pack_for_pet( +/// already local. Preparing this before loading keeps first-use preview and +/// persistence failures at the asset-fetch boundary rather than surfacing as +/// deeper image-loading errors. +async fn ensure_builtin_pack_for_pet( pet_id: &str, codex_home: &std::path::Path, + http_client: &RouteAwareClientPool, ) -> Result<()> { if let Some(pet) = catalog::builtin_pet(pet_id) { - asset_pack::ensure_builtin_pet(codex_home, pet)?; + asset_pack::ensure_builtin_pet(codex_home, pet, http_client).await?; } Ok(()) } +/// Prepare a pet's built-in assets and load its synchronous state off the runtime. +pub(crate) async fn load_pet_with_assets( + pet_id: String, + codex_home: AbsolutePathBuf, + frame_requester: FrameRequester, + animations_enabled: bool, + http_client: &RouteAwareClientPool, +) -> Result { + ensure_builtin_pack_for_pet(&pet_id, &codex_home, http_client).await?; + tokio::task::spawn_blocking(move || { + AmbientPet::load( + Some(&pet_id), + &codex_home, + frame_requester, + animations_enabled, + ) + }) + .await + .context("join pet load task")? +} + #[derive(Debug)] pub(crate) enum PetImageRenderError { Terminal(std::io::Error),