From cb870a169a07423bb366bb8fd7878190b439da08 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Sun, 12 Apr 2026 14:08:14 -0700 Subject: [PATCH 1/6] fix(sandboxing): reject WSL1 bubblewrap sandboxing (#17559) ## Summary - detect WSL1 before Codex probes or invokes the Linux bubblewrap sandbox - fail early with a clear unsupported-operation message when a command would require bubblewrap on WSL1 - document that WSL2 follows the normal Linux bubblewrap path while WSL1 is unsupported ## Why Codex 0.115.0 made bubblewrap the default Linux sandbox. WSL1 cannot create the user namespaces that bubblewrap needs, so shell commands currently fail later with a raw bwrap namespace error. This makes the unsupported environment explicit and keeps non-bubblewrap paths unchanged. The WSL detection reads /proc/version, lets an explicit WSL marker decide WSL1 vs WSL2+, and only treats a bare Microsoft marker as WSL1 when no explicit WSL version is present. addresses https://github.com/openai/codex/issues/16076 --------- Co-authored-by: Codex --- codex-rs/core/README.md | 6 ++- codex-rs/linux-sandbox/README.md | 8 +++- codex-rs/sandboxing/src/bwrap.rs | 32 +++++++++++++ codex-rs/sandboxing/src/bwrap_tests.rs | 30 ++++++++++++ codex-rs/sandboxing/src/lib.rs | 4 ++ codex-rs/sandboxing/src/manager.rs | 31 ++++++++++++ codex-rs/sandboxing/src/manager_tests.rs | 60 ++++++++++++++++++++++++ 7 files changed, 169 insertions(+), 2 deletions(-) diff --git a/codex-rs/core/README.md b/codex-rs/core/README.md index 63d9cba53b..e1eab34935 100644 --- a/codex-rs/core/README.md +++ b/codex-rs/core/README.md @@ -42,7 +42,11 @@ switches to a no-`--argv0` compatibility path for the inner re-exec. If `bwrap` is missing, it falls back to the vendored bubblewrap path compiled into the binary and Codex surfaces a startup warning through its normal notification path instead of printing directly from the sandbox helper. Codex also surfaces -a startup warning when bubblewrap cannot create user namespaces. +a startup warning when bubblewrap cannot create user namespaces. WSL2 uses the +normal Linux bubblewrap path. WSL1 is not supported for bubblewrap sandboxing +because it cannot create the required user namespaces, so Codex rejects +sandboxed shell commands that would enter the bubblewrap path before invoking +`bwrap`. ### Windows diff --git a/codex-rs/linux-sandbox/README.md b/codex-rs/linux-sandbox/README.md index 340b109101..3a004934b2 100644 --- a/codex-rs/linux-sandbox/README.md +++ b/codex-rs/linux-sandbox/README.md @@ -16,7 +16,10 @@ the helper falls back to the vendored bubblewrap path compiled into this binary. Codex also surfaces a startup warning when `bwrap` is missing so users know it is falling back to the vendored helper. Codex surfaces the same startup warning -path when bubblewrap cannot create user namespaces. +path when bubblewrap cannot create user namespaces. WSL2 follows the normal +Linux bubblewrap path. WSL1 is not supported for bubblewrap sandboxing because +it cannot create the required user namespaces, so Codex rejects sandboxed shell +commands that would enter the bubblewrap path. **Current Behavior** - Legacy `SandboxPolicy` / `sandbox_mode` configs remain supported. @@ -31,6 +34,9 @@ path when bubblewrap cannot create user namespaces. printing directly from the sandbox helper. - If bubblewrap cannot create user namespaces, Codex surfaces a startup warning instead of waiting for a runtime sandbox failure. +- WSL2 uses the normal Linux bubblewrap path. +- WSL1 is not supported for bubblewrap sandboxing; Codex rejects sandboxed + shell commands that would require the bubblewrap path before invoking `bwrap`. - Legacy Landlock + mount protections remain available as an explicit legacy fallback path. - Set `features.use_legacy_landlock = true` (or CLI `-c use_legacy_landlock=true`) diff --git a/codex-rs/sandboxing/src/bwrap.rs b/codex-rs/sandboxing/src/bwrap.rs index 01d53ebbe8..069807f986 100644 --- a/codex-rs/sandboxing/src/bwrap.rs +++ b/codex-rs/sandboxing/src/bwrap.rs @@ -14,6 +14,11 @@ const MISSING_BWRAP_WARNING: &str = concat!( ); const USER_NAMESPACE_WARNING: &str = "Codex's Linux sandbox uses bubblewrap and needs access to create user namespaces."; +pub(crate) const WSL1_BWRAP_WARNING: &str = concat!( + "Codex's Linux sandbox uses bubblewrap, which is not supported on WSL1 ", + "because WSL1 cannot create the required user namespaces. ", + "Use WSL2 for sandboxed shell commands." +); const USER_NAMESPACE_FAILURES: [&str; 4] = [ "loopback: Failed RTM_NEWADDR", "loopback: Failed RTM_NEWLINK", @@ -38,6 +43,10 @@ fn should_warn_about_system_bwrap(sandbox_policy: &SandboxPolicy) -> bool { } fn system_bwrap_warning_for_path(system_bwrap_path: Option<&Path>) -> Option { + if is_wsl1() { + return Some(WSL1_BWRAP_WARNING.to_string()); + } + let Some(system_bwrap_path) = system_bwrap_path else { return Some(MISSING_BWRAP_WARNING.to_string()); }; @@ -68,6 +77,29 @@ fn system_bwrap_has_user_namespace_access(system_bwrap_path: &Path) -> bool { output.status.success() || !is_user_namespace_failure(&output) } +pub(crate) fn is_wsl1() -> bool { + std::fs::read_to_string("/proc/version") + .is_ok_and(|proc_version| proc_version_indicates_wsl1(&proc_version)) +} + +fn proc_version_indicates_wsl1(proc_version: &str) -> bool { + let proc_version = proc_version.to_ascii_lowercase(); + let mut remaining = proc_version.as_str(); + while let Some(marker) = remaining.find("wsl") { + let version_start = marker + "wsl".len(); + let version_digits: String = remaining[version_start..] + .chars() + .take_while(char::is_ascii_digit) + .collect(); + if let Ok(version) = version_digits.parse::() { + return version == 1; + } + remaining = &remaining[version_start..]; + } + + proc_version.contains("microsoft") && !proc_version.contains("microsoft-standard") +} + fn is_user_namespace_failure(output: &Output) -> bool { let stderr = String::from_utf8_lossy(&output.stderr); USER_NAMESPACE_FAILURES diff --git a/codex-rs/sandboxing/src/bwrap_tests.rs b/codex-rs/sandboxing/src/bwrap_tests.rs index b0303e2b78..f36848e1e7 100644 --- a/codex-rs/sandboxing/src/bwrap_tests.rs +++ b/codex-rs/sandboxing/src/bwrap_tests.rs @@ -44,6 +44,36 @@ exit 1 assert_eq!(system_bwrap_warning_for_path(Some(fake_bwrap_path)), None); } +#[test] +fn detects_wsl1_proc_version_formats() { + assert!(proc_version_indicates_wsl1( + "Linux version 4.4.0-22621-Microsoft" + )); + assert!(proc_version_indicates_wsl1( + "Linux version 5.15.0-microsoft-standard-WSL1" + )); + assert!(proc_version_indicates_wsl1( + "Linux version 5.15.0-wsl-microsoft-standard-WSL1" + )); +} + +#[test] +fn does_not_treat_wsl2_or_native_linux_as_wsl1() { + assert!(!proc_version_indicates_wsl1( + "Linux version 6.6.87.2-microsoft-standard-WSL2" + )); + assert!(!proc_version_indicates_wsl1( + "Linux version 6.6.87.2-wsl-microsoft-standard-WSL2" + )); + assert!(!proc_version_indicates_wsl1( + "Linux version 4.19.104-microsoft-standard" + )); + assert!(!proc_version_indicates_wsl1( + "Linux version 6.6.87.2-microsoft-standard-WSL3" + )); + assert!(!proc_version_indicates_wsl1("Linux version 6.8.0")); +} + #[test] fn finds_first_executable_bwrap_in_joined_search_path() { let temp_dir = tempdir().expect("temp dir"); diff --git a/codex-rs/sandboxing/src/lib.rs b/codex-rs/sandboxing/src/lib.rs index 244f65bb0a..38aed6ce24 100644 --- a/codex-rs/sandboxing/src/lib.rs +++ b/codex-rs/sandboxing/src/lib.rs @@ -34,6 +34,10 @@ impl From for CodexErr { SandboxTransformError::MissingLinuxSandboxExecutable => { CodexErr::LandlockSandboxExecutableNotProvided } + #[cfg(target_os = "linux")] + SandboxTransformError::Wsl1UnsupportedForBubblewrap => { + CodexErr::UnsupportedOperation(crate::bwrap::WSL1_BWRAP_WARNING.to_string()) + } #[cfg(not(target_os = "macos"))] SandboxTransformError::SeatbeltUnavailable => CodexErr::UnsupportedOperation( "seatbelt sandbox is only available on macOS".to_string(), diff --git a/codex-rs/sandboxing/src/manager.rs b/codex-rs/sandboxing/src/manager.rs index 4ed413735a..ebff33f105 100644 --- a/codex-rs/sandboxing/src/manager.rs +++ b/codex-rs/sandboxing/src/manager.rs @@ -1,3 +1,7 @@ +#[cfg(target_os = "linux")] +use crate::bwrap::WSL1_BWRAP_WARNING; +#[cfg(target_os = "linux")] +use crate::bwrap::is_wsl1; use crate::landlock::CODEX_LINUX_SANDBOX_ARG0; use crate::landlock::allow_network_for_proxy; use crate::landlock::create_linux_sandbox_command_args_for_policies; @@ -109,6 +113,8 @@ pub struct SandboxTransformRequest<'a> { #[derive(Debug)] pub enum SandboxTransformError { MissingLinuxSandboxExecutable, + #[cfg(target_os = "linux")] + Wsl1UnsupportedForBubblewrap, #[cfg(not(target_os = "macos"))] SeatbeltUnavailable, } @@ -119,6 +125,8 @@ impl std::fmt::Display for SandboxTransformError { Self::MissingLinuxSandboxExecutable => { write!(f, "missing codex-linux-sandbox executable path") } + #[cfg(target_os = "linux")] + Self::Wsl1UnsupportedForBubblewrap => write!(f, "{WSL1_BWRAP_WARNING}"), #[cfg(not(target_os = "macos"))] Self::SeatbeltUnavailable => write!(f, "seatbelt sandbox is only available on macOS"), } @@ -219,6 +227,13 @@ impl SandboxManager { let exe = codex_linux_sandbox_exe .ok_or(SandboxTransformError::MissingLinuxSandboxExecutable)?; let allow_proxy_network = allow_network_for_proxy(enforce_managed_network); + #[cfg(target_os = "linux")] + ensure_linux_bubblewrap_is_supported( + &effective_file_system_policy, + use_legacy_landlock, + allow_proxy_network, + is_wsl1(), + )?; let mut args = create_linux_sandbox_command_args_for_policies( os_argv_to_strings(argv), command.cwd.as_path(), @@ -256,6 +271,22 @@ impl SandboxManager { } } +#[cfg(target_os = "linux")] +fn ensure_linux_bubblewrap_is_supported( + file_system_sandbox_policy: &FileSystemSandboxPolicy, + use_legacy_landlock: bool, + allow_network_for_proxy: bool, + is_wsl1: bool, +) -> Result<(), SandboxTransformError> { + let requires_bubblewrap = !use_legacy_landlock + && (!file_system_sandbox_policy.has_full_disk_write_access() || allow_network_for_proxy); + if is_wsl1 && requires_bubblewrap { + return Err(SandboxTransformError::Wsl1UnsupportedForBubblewrap); + } + + Ok(()) +} + fn os_argv_to_strings(argv: Vec) -> Vec { argv.into_iter() .map(os_string_to_command_component) diff --git a/codex-rs/sandboxing/src/manager_tests.rs b/codex-rs/sandboxing/src/manager_tests.rs index 39531f13b8..f4e019ee28 100644 --- a/codex-rs/sandboxing/src/manager_tests.rs +++ b/codex-rs/sandboxing/src/manager_tests.rs @@ -275,6 +275,66 @@ fn transform_linux_seccomp_request( .expect("transform") } +#[cfg(target_os = "linux")] +#[test] +fn wsl1_rejects_linux_bubblewrap_path() { + let restricted_policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }]); + + assert!(matches!( + super::ensure_linux_bubblewrap_is_supported( + &restricted_policy, + /*use_legacy_landlock*/ false, + /*allow_network_for_proxy*/ false, + /*is_wsl1*/ true, + ), + Err(super::SandboxTransformError::Wsl1UnsupportedForBubblewrap) + )); + assert!(matches!( + super::ensure_linux_bubblewrap_is_supported( + &FileSystemSandboxPolicy::unrestricted(), + /*use_legacy_landlock*/ false, + /*allow_network_for_proxy*/ true, + /*is_wsl1*/ true, + ), + Err(super::SandboxTransformError::Wsl1UnsupportedForBubblewrap) + )); +} + +#[cfg(target_os = "linux")] +#[test] +fn wsl1_allows_non_bubblewrap_linux_paths() { + assert!( + super::ensure_linux_bubblewrap_is_supported( + &FileSystemSandboxPolicy::unrestricted(), + /*use_legacy_landlock*/ false, + /*allow_network_for_proxy*/ false, + /*is_wsl1*/ true, + ) + .is_ok() + ); + + let restricted_policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }]); + assert!( + super::ensure_linux_bubblewrap_is_supported( + &restricted_policy, + /*use_legacy_landlock*/ true, + /*allow_network_for_proxy*/ false, + /*is_wsl1*/ true, + ) + .is_ok() + ); +} + #[cfg(target_os = "linux")] #[test] fn transform_linux_seccomp_preserves_helper_path_in_arg0_when_available() { From d840b247d7a7df2fe6d1eed182852beaa066347e Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Sun, 12 Apr 2026 15:03:14 -0700 Subject: [PATCH 2/6] Mirror user text into realtime (#17520) - Let typed user messages submit while realtime is active and mirror accepted text into the realtime text stream. - Add integration coverage and snapshot for outbound realtime text. --- .../tests/suite/v2/realtime_conversation.rs | 71 +++------- codex-rs/core/src/codex.rs | 46 ++++++- codex-rs/core/src/realtime_conversation.rs | 12 -- .../core/tests/suite/realtime_conversation.rs | 127 ++++++++++++++++++ ..._turn_is_sent_to_realtime_when_active.snap | 10 ++ codex-rs/tui/src/chatwidget.rs | 10 -- codex-rs/tui/src/chatwidget/realtime.rs | 26 ---- 7 files changed, 200 insertions(+), 102 deletions(-) create mode 100644 codex-rs/core/tests/suite/snapshots/all__suite__realtime_conversation__conversation_user_text_turn_is_sent_to_realtime_when_active.snap diff --git a/codex-rs/app-server/tests/suite/v2/realtime_conversation.rs b/codex-rs/app-server/tests/suite/v2/realtime_conversation.rs index efc167c9d0..f4c0f99ae3 100644 --- a/codex-rs/app-server/tests/suite/v2/realtime_conversation.rs +++ b/codex-rs/app-server/tests/suite/v2/realtime_conversation.rs @@ -661,7 +661,7 @@ async fn realtime_conversation_streams_v2_notifications() -> Result<()> { let connections = realtime_server.connections(); assert_eq!(connections.len(), 1); let connection = &connections[0]; - assert_eq!(connection.len(), 4); + assert_eq!(connection.len(), 3); assert_eq!( connection[0].body_json()["type"].as_str(), Some("session.update") @@ -679,10 +679,6 @@ async fn realtime_conversation_streams_v2_notifications() -> Result<()> { .as_str() .context("expected websocket request type")? .to_string(), - connection[3].body_json()["type"] - .as_str() - .context("expected websocket request type")? - .to_string(), ]; request_types.sort(); assert_eq!( @@ -690,7 +686,6 @@ async fn realtime_conversation_streams_v2_notifications() -> Result<()> { [ "conversation.item.create".to_string(), "input_audio_buffer.append".to_string(), - "response.create".to_string(), ] ); @@ -1153,7 +1148,6 @@ async fn webrtc_v2_forwards_audio_and_text_between_client_and_sideband() -> Resu "samples_per_channel": 512 }), ], - vec![], ])]), ) .await?; @@ -1185,7 +1179,6 @@ async fn webrtc_v2_forwards_audio_and_text_between_client_and_sideband() -> Resu let requests = [ harness.sideband_outbound_request(/*request_index*/ 1).await, harness.sideband_outbound_request(/*request_index*/ 2).await, - harness.sideband_outbound_request(/*request_index*/ 3).await, ]; assert!( requests @@ -1208,13 +1201,12 @@ async fn webrtc_v2_forwards_audio_and_text_between_client_and_sideband() -> Resu Ok(()) } -/// Regression coverage for Realtime V2's single-active-response rule. +/// Regression coverage for Realtime V2 text input while a response is active. /// -/// The Realtime API rejects a new `response.create` while a default response is -/// still active, so the input task should queue the second create and flush it -/// only after the server sends `response.done` for the active response. +/// Text input is append-only, so app-server should send the user message without +/// requesting a new realtime response. #[tokio::test] -async fn webrtc_v2_queues_text_response_create_while_response_is_active() -> Result<()> { +async fn webrtc_v2_text_input_is_append_only_while_response_is_active() -> Result<()> { skip_if_no_network!(Ok(())); // Phase 1: script a server-side response that becomes active after the first @@ -1224,7 +1216,6 @@ async fn webrtc_v2_queues_text_response_create_while_response_is_active() -> Res no_main_loop_responses(), realtime_sideband(vec![realtime_sideband_connection(vec![ vec![session_updated("sess_v2_response_queue")], - vec![], vec![ json!({ "type": "response.created", @@ -1240,7 +1231,6 @@ async fn webrtc_v2_queues_text_response_create_while_response_is_active() -> Res "type": "response.done", "response": { "id": "resp_active" } })], - vec![], ])]), ) .await?; @@ -1253,15 +1243,14 @@ async fn webrtc_v2_queues_text_response_create_while_response_is_active() -> Res // notifications; they are the protocol frames app-server sends upstream. assert_v2_session_update(&harness.sideband_outbound_request(/*request_index*/ 0).await)?; - // Phase 2: send the first text turn. It is safe to emit `response.create` - // immediately because no default response is active yet. + // Phase 2: send the first text turn. Text input is append-only, so this + // sends only the user text item. let thread_id = started.started.thread_id.clone(); harness.append_text(thread_id.clone(), "first").await?; assert_v2_user_text_item( &harness.sideband_outbound_request(/*request_index*/ 1).await, "first", ); - assert_v2_response_create(&harness.sideband_outbound_request(/*request_index*/ 2).await); let transcript = harness .read_notification::( "thread/realtime/transcriptUpdated", @@ -1270,39 +1259,28 @@ async fn webrtc_v2_queues_text_response_create_while_response_is_active() -> Res assert_eq!(transcript.text, "active response started"); // Phase 3: send a second text turn while `resp_active` is still open. The - // user message must reach realtime, but `response.create` must not be sent - // yet or the Realtime API rejects it as an active-response conflict. + // user message must reach realtime without requesting another response. harness.append_text(thread_id.clone(), "second").await?; assert_v2_user_text_item( - &harness.sideband_outbound_request(/*request_index*/ 3).await, + &harness.sideband_outbound_request(/*request_index*/ 2).await, "second", ); - // Phase 4: the audio input causes the scripted sideband stream to send - // `response.done`, which clears the active response and flushes the queued - // `response.create` for the second text turn. + // Phase 4: audio still forwards normally after text input. harness.append_audio(thread_id).await?; - // This is the negative check: if the second text turn had emitted - // `response.create` immediately, request 4 would be that create instead of - // the audio append. - let audio = harness.sideband_outbound_request(/*request_index*/ 4).await; + let audio = harness.sideband_outbound_request(/*request_index*/ 3).await; assert_eq!(audio["type"], "input_audio_buffer.append"); assert_eq!(audio["audio"], "BQYH"); - assert_v2_response_create(&harness.sideband_outbound_request(/*request_index*/ 5).await); harness.shutdown().await; Ok(()) } -/// Regression coverage for the same queued `response.create` path when the -/// active Realtime V2 response is cancelled instead of completed. -/// -/// `response.cancelled` should clear the active-response guard exactly like -/// `response.done`, so a text turn queued during the active response still gets -/// one deferred `response.create`. +/// Regression coverage for append-only Realtime V2 text input when the active +/// response is cancelled instead of completed. #[tokio::test] -async fn webrtc_v2_flushes_queued_text_response_create_when_response_is_cancelled() -> Result<()> { +async fn webrtc_v2_text_input_is_append_only_when_response_is_cancelled() -> Result<()> { skip_if_no_network!(Ok(())); // Phase 1: script a server-side response that becomes active after the first @@ -1312,7 +1290,6 @@ async fn webrtc_v2_flushes_queued_text_response_create_when_response_is_cancelle no_main_loop_responses(), realtime_sideband(vec![realtime_sideband_connection(vec![ vec![session_updated("sess_v2_response_cancel_queue")], - vec![], vec![json!({ "type": "response.created", "response": { "id": "resp_cancelled" } @@ -1322,7 +1299,6 @@ async fn webrtc_v2_flushes_queued_text_response_create_when_response_is_cancelle "type": "response.cancelled", "response": { "id": "resp_cancelled" } })], - vec![], ])]), ) .await?; @@ -1331,36 +1307,29 @@ async fn webrtc_v2_flushes_queued_text_response_create_when_response_is_cancelle assert_eq!(started.started.version, RealtimeConversationVersion::V2); assert_v2_session_update(&harness.sideband_outbound_request(/*request_index*/ 0).await)?; - // Phase 2: send the first text turn. It is safe to emit `response.create` - // immediately because no default response is active yet. + // Phase 2: send the first text turn. Text input is append-only, so this + // sends only the user text item. let thread_id = started.started.thread_id.clone(); harness.append_text(thread_id.clone(), "first").await?; assert_v2_user_text_item( &harness.sideband_outbound_request(/*request_index*/ 1).await, "first", ); - assert_v2_response_create(&harness.sideband_outbound_request(/*request_index*/ 2).await); // Phase 3: send a second text turn while `resp_cancelled` is still open. - // The user message must reach realtime, but `response.create` stays queued. + // The user message must reach realtime without requesting another response. harness.append_text(thread_id.clone(), "second").await?; assert_v2_user_text_item( - &harness.sideband_outbound_request(/*request_index*/ 3).await, + &harness.sideband_outbound_request(/*request_index*/ 2).await, "second", ); - // Phase 4: the audio input causes the scripted sideband stream to send - // `response.cancelled`, which clears the active response and flushes the - // queued `response.create` for the second text turn. + // Phase 4: audio still forwards normally after text input. harness.append_audio(thread_id).await?; - // This is the negative check: if the second text turn had emitted - // `response.create` immediately, request 4 would be that create instead of - // the audio append. - let audio = harness.sideband_outbound_request(/*request_index*/ 4).await; + let audio = harness.sideband_outbound_request(/*request_index*/ 3).await; assert_eq!(audio["type"], "input_audio_buffer.append"); assert_eq!(audio["audio"], "BQYH"); - assert_v2_response_create(&harness.sideband_outbound_request(/*request_index*/ 5).await); harness.shutdown().await; Ok(()) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index f44a79528f..c19c4b48fd 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -2265,7 +2265,7 @@ impl Session { } pub(crate) async fn route_realtime_text_input(self: &Arc, text: String) { - handlers::user_input_or_turn( + handlers::user_input_or_turn_inner( self, self.next_internal_sub_id(), Op::UserInput { @@ -2276,6 +2276,7 @@ impl Session { final_output_json_schema: None, responsesapi_client_metadata: None, }, + /*mirror_user_text_to_realtime*/ None, ) .await; } @@ -4910,6 +4911,7 @@ mod handlers { use codex_protocol::config_types::ModeKind; use codex_protocol::config_types::Settings; use codex_protocol::dynamic_tools::DynamicToolResponse; + use codex_protocol::items::UserMessageItem; use codex_protocol::mcp::RequestId as ProtocolRequestId; use codex_protocol::user_input::UserInput; use codex_rmcp_client::ElicitationAction; @@ -4917,6 +4919,7 @@ mod handlers { use serde_json::Value; use std::path::PathBuf; use std::sync::Arc; + use tracing::debug; use tracing::info; use tracing::warn; @@ -4958,6 +4961,21 @@ mod handlers { } pub async fn user_input_or_turn(sess: &Arc, sub_id: String, op: Op) { + user_input_or_turn_inner( + sess, + sub_id, + op, + /*mirror_user_text_to_realtime*/ Some(()), + ) + .await; + } + + pub(super) async fn user_input_or_turn_inner( + sess: &Arc, + sub_id: String, + op: Op, + mirror_user_text_to_realtime: Option<()>, + ) { let (items, updates, responsesapi_client_metadata) = match op { Op::UserTurn { cwd, @@ -5023,7 +5041,7 @@ mod handlers { }; sess.maybe_emit_unknown_model_warning_for_turn(current_context.as_ref()) .await; - match sess + let accepted_items = match sess .steer_input( items.clone(), /*expected_turn_id*/ None, @@ -5031,7 +5049,10 @@ mod handlers { ) .await { - Ok(_) => current_context.session_telemetry.user_prompt(&items), + Ok(_) => { + current_context.session_telemetry.user_prompt(&items); + Some(items) + } Err(SteerInputError::NoActiveTurn(items)) => { if let Some(responsesapi_client_metadata) = responsesapi_client_metadata { current_context @@ -5041,12 +5062,14 @@ mod handlers { current_context.session_telemetry.user_prompt(&items); sess.refresh_mcp_servers_if_requested(¤t_context) .await; + let accepted_items = items.clone(); sess.spawn_task( Arc::clone(¤t_context), items, crate::tasks::RegularTask::new(), ) .await; + Some(accepted_items) } Err(err) => { sess.send_event_raw(Event { @@ -5054,7 +5077,24 @@ mod handlers { msg: EventMsg::Error(err.to_error_event()), }) .await; + None } + }; + if let (Some(items), Some(())) = (accepted_items, mirror_user_text_to_realtime) { + self::mirror_user_text_to_realtime(sess, &items).await; + } + } + + async fn mirror_user_text_to_realtime(sess: &Arc, items: &[UserInput]) { + let text = UserMessageItem::new(items).message(); + if text.is_empty() { + return; + } + if sess.conversation.running_state().await.is_none() { + return; + } + if let Err(err) = sess.conversation.text_in(text).await { + debug!("failed to mirror user text to realtime conversation: {err}"); } } diff --git a/codex-rs/core/src/realtime_conversation.rs b/codex-rs/core/src/realtime_conversation.rs index d22551a573..8200ba4908 100644 --- a/codex-rs/core/src/realtime_conversation.rs +++ b/codex-rs/core/src/realtime_conversation.rs @@ -945,8 +945,6 @@ fn spawn_realtime_input_task(input: RealtimeInputTask) -> JoinHandle<()> { user_text, &writer, &events_tx, - session_kind, - &mut response_create_queue, ) .await } @@ -992,8 +990,6 @@ async fn handle_user_text_input( text: Result, writer: &RealtimeWebsocketWriter, events_tx: &Sender, - session_kind: RealtimeSessionKind, - response_create_queue: &mut RealtimeResponseCreateQueue, ) -> anyhow::Result<()> { let text = text.context("user text input channel closed")?; @@ -1005,14 +1001,6 @@ async fn handle_user_text_input( .await; return Err(mapped_error.into()); } - match session_kind { - RealtimeSessionKind::V1 => {} - RealtimeSessionKind::V2 => { - response_create_queue - .request_create(writer, events_tx, "text") - .await?; - } - } Ok(()) } diff --git a/codex-rs/core/tests/suite/realtime_conversation.rs b/codex-rs/core/tests/suite/realtime_conversation.rs index 4a7230867e..cede36c28a 100644 --- a/codex-rs/core/tests/suite/realtime_conversation.rs +++ b/codex-rs/core/tests/suite/realtime_conversation.rs @@ -1790,6 +1790,126 @@ async fn conversation_startup_context_is_truncated_and_sent_once_per_start() -> Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn conversation_user_text_turn_is_sent_to_realtime_when_active() -> Result<()> { + skip_if_no_network!(Ok(())); + + let api_server = start_mock_server().await; + let response_mock = responses::mount_sse_once( + &api_server, + responses::sse(vec![ + responses::ev_response_created("resp_user_text"), + responses::ev_assistant_message("msg_user_text", "ack"), + responses::ev_completed("resp_user_text"), + ]), + ) + .await; + + let realtime_server = start_websocket_server(vec![vec![ + vec![json!({ + "type": "session.updated", + "session": { "id": "sess_user_text", "instructions": "backend prompt" } + })], + vec![], + ]]) + .await; + + let mut builder = test_codex().with_config({ + let realtime_base_url = realtime_server.uri().to_string(); + move |config| { + config.experimental_realtime_ws_base_url = Some(realtime_base_url); + config.experimental_realtime_ws_startup_context = Some(String::new()); + } + }); + let test = builder.build(&api_server).await?; + + test.codex + .submit(Op::RealtimeConversationStart(ConversationStartParams { + prompt: Some(Some("backend prompt".to_string())), + session_id: None, + transport: None, + voice: None, + })) + .await?; + + let session_updated = wait_for_event_match(&test.codex, |msg| match msg { + EventMsg::RealtimeConversationRealtime(RealtimeConversationRealtimeEvent { + payload: RealtimeEvent::SessionUpdated { session_id, .. }, + }) => Some(session_id.clone()), + _ => None, + }) + .await; + assert_eq!(session_updated, "sess_user_text"); + + let user_text = "typed follow-up for realtime"; + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: user_text.to_string(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + }) + .await?; + + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let realtime_text_request = wait_for_matching_websocket_request( + &realtime_server, + "normal user turn text mirrored to realtime", + |request| websocket_request_text(request).as_deref() == Some(user_text), + ) + .await; + let model_user_texts = response_mock.single_request().message_input_texts("user"); + assert_eq!( + ( + model_user_texts.iter().any(|text| text == user_text), + websocket_request_text(&realtime_text_request), + ), + (true, Some(user_text.to_string())), + ); + let realtime_response_create = timeout(Duration::from_millis(200), async { + wait_for_matching_websocket_request( + &realtime_server, + "unexpected realtime response request for mirrored user text", + |request| request.body_json()["type"].as_str() == Some("response.create"), + ) + .await + }) + .await; + assert!( + realtime_response_create.is_err(), + "mirrored user text should not request a realtime response" + ); + + let realtime_request_body = realtime_text_request.body_json(); + let content = &realtime_request_body["item"]["content"][0]; + let snapshot = format!( + "type: {}\nitem.type: {}\nitem.role: {}\ncontent[0].type: {}\ncontent[0].text: {}\nresponse.create: {}", + realtime_request_body["type"].as_str().unwrap_or_default(), + realtime_request_body["item"]["type"] + .as_str() + .unwrap_or_default(), + realtime_request_body["item"]["role"] + .as_str() + .unwrap_or_default(), + content["type"].as_str().unwrap_or_default(), + content["text"].as_str().unwrap_or_default(), + realtime_response_create.is_ok(), + ); + insta::assert_snapshot!( + "conversation_user_text_turn_is_sent_to_realtime_when_active", + snapshot + ); + + realtime_server.shutdown().await; + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn conversation_mirrors_assistant_message_text_to_realtime_handoff() -> Result<()> { skip_if_no_network!(Ok(())); @@ -2767,6 +2887,7 @@ async fn inbound_handoff_request_steers_active_turn() -> Result<()> { "type": "session.updated", "session": { "id": "sess_steer", "instructions": "backend prompt" } })], + vec![], vec![ json!({ "type": "conversation.input_transcript.delta", @@ -2822,6 +2943,12 @@ async fn inbound_handoff_request_steers_active_turn() -> Result<()> { matches!(event, EventMsg::AgentMessageContentDelta(_)) }) .await; + let _ = wait_for_matching_websocket_request( + &realtime_server, + "first prompt mirrored to realtime", + |request| websocket_request_text(request).as_deref() == Some("first prompt"), + ) + .await; test.codex .submit(Op::RealtimeConversationAudio(ConversationAudioParams { diff --git a/codex-rs/core/tests/suite/snapshots/all__suite__realtime_conversation__conversation_user_text_turn_is_sent_to_realtime_when_active.snap b/codex-rs/core/tests/suite/snapshots/all__suite__realtime_conversation__conversation_user_text_turn_is_sent_to_realtime_when_active.snap new file mode 100644 index 0000000000..9cd2a55303 --- /dev/null +++ b/codex-rs/core/tests/suite/snapshots/all__suite__realtime_conversation__conversation_user_text_turn_is_sent_to_realtime_when_active.snap @@ -0,0 +1,10 @@ +--- +source: core/tests/suite/realtime_conversation.rs +expression: snapshot +--- +type: conversation.item.create +item.type: message +item.role: user +content[0].type: input_text +content[0].text: typed follow-up for realtime +response.create: false diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index fe83404523..7f47edbb99 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -5050,11 +5050,6 @@ impl ChatWidget { { return; } - let Some(user_message) = - self.maybe_defer_user_message_for_realtime(user_message) - else { - return; - }; let should_submit_now = self.is_session_configured() && !self.is_plan_streaming_in_tui(); if should_submit_now { @@ -5085,11 +5080,6 @@ impl ChatWidget { .bottom_pane .take_recent_submission_mention_bindings(), }; - let Some(user_message) = - self.maybe_defer_user_message_for_realtime(user_message) - else { - return; - }; self.queue_user_message(user_message); } InputResult::Command(cmd) => { diff --git a/codex-rs/tui/src/chatwidget/realtime.rs b/codex-rs/tui/src/chatwidget/realtime.rs index f45ae5b9aa..03a59c224c 100644 --- a/codex-rs/tui/src/chatwidget/realtime.rs +++ b/codex-rs/tui/src/chatwidget/realtime.rs @@ -29,7 +29,6 @@ pub(super) struct RealtimeConversationUiState { pub(super) phase: RealtimeConversationPhase, requested_close: bool, session_id: Option, - warned_audio_only_submission: bool, transport: RealtimeConversationUiTransport, #[cfg(not(target_os = "linux"))] pub(super) meter_placeholder_id: Option, @@ -191,28 +190,6 @@ impl ChatWidget { self.last_rendered_user_message_event.as_ref() != Some(&key) } - pub(super) fn maybe_defer_user_message_for_realtime( - &mut self, - user_message: UserMessage, - ) -> Option { - if !self.realtime_conversation.is_live() { - return Some(user_message); - } - - self.restore_user_message_to_composer(user_message); - if !self.realtime_conversation.warned_audio_only_submission { - self.realtime_conversation.warned_audio_only_submission = true; - self.add_info_message( - "Realtime voice mode is audio-only. Use /realtime to stop.".to_string(), - /*hint*/ None, - ); - } else { - self.request_redraw(); - } - - None - } - fn realtime_footer_hint_items() -> Vec<(String, String)> { vec![("/realtime".to_string(), "stop live voice".to_string())] } @@ -238,7 +215,6 @@ impl ChatWidget { self.realtime_conversation.phase = RealtimeConversationPhase::Starting; self.realtime_conversation.requested_close = false; self.realtime_conversation.session_id = None; - self.realtime_conversation.warned_audio_only_submission = false; self.set_footer_hint_override(Some(Self::realtime_footer_hint_items())); match self.config.realtime.transport { RealtimeTransport::Websocket => { @@ -297,7 +273,6 @@ impl ChatWidget { self.realtime_conversation.phase = RealtimeConversationPhase::Inactive; self.realtime_conversation.requested_close = false; self.realtime_conversation.session_id = None; - self.realtime_conversation.warned_audio_only_submission = false; self.realtime_conversation.transport = RealtimeConversationUiTransport::Websocket; } @@ -320,7 +295,6 @@ impl ChatWidget { return; } self.realtime_conversation.session_id = ev.session_id; - self.realtime_conversation.warned_audio_only_submission = false; self.set_footer_hint_override(Some(Self::realtime_footer_hint_items())); if self.realtime_conversation_uses_webrtc() { self.realtime_conversation.phase = RealtimeConversationPhase::Starting; From 0393a485ed0b0577938259de3e3e3a0f02b4dcda Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Sun, 12 Apr 2026 19:32:19 -0300 Subject: [PATCH 3/6] feat(tui): add reverse history search to composer (#17550) ## Problem The TUI had shell-style Up/Down history recall, but `Ctrl+R` did not provide the reverse incremental search workflow users expect from shells. Users needed a way to search older prompts without immediately replacing the current draft, and the interaction needed to handle async persistent history, repeated navigation keys, duplicate prompt text, footer hints, and preview highlighting without making the main composer file even harder to review. https://github.com/user-attachments/assets/5165affd-4c9a-46e9-adbd-89088f5f7b6b image ## Mental model `Ctrl+R` opens a temporary search session owned by the composer. The footer line becomes the search input, the composer body previews the current match only after the query has text, and `Enter` accepts that preview as an editable draft while `Esc` restores the draft that existed before search started. The history layer provides a combined offset space over persistent and local history, but search navigation exposes unique prompt text rather than every physical history row. ## Non-goals This change does not rewrite stored history, change normal Up/Down browsing semantics, add fuzzy matching, or add persistent metadata for attachments in cross-session history. Search deduplication is deliberately scoped to the active Ctrl+R search session and uses exact prompt text, so case, whitespace, punctuation, and attachment-only differences are not normalized. ## Tradeoffs The implementation keeps search state in the existing composer and history state machines instead of adding a new cross-module controller. That keeps ownership local and testable, but it means the composer still coordinates visible search status, draft restoration, footer rendering, cursor placement, and match highlighting while `ChatComposerHistory` owns traversal, async fetch continuation, boundary clamping, and unique-result caching. Unique-result caching stores cloned `HistoryEntry` values so known matches can be revisited without cache lookups; this is simple and robust for interactive search sizes, but it is not a global history index. ## Architecture `ChatComposer` detects `Ctrl+R`, snapshots the current draft, switches the footer to `FooterMode::HistorySearch`, and routes search-mode keys before normal editing. Query edits call `ChatComposerHistory::search` with `restart = true`, which starts from the newest combined-history offset. Repeated `Ctrl+R` or Up searches older; Down searches newer through already discovered unique matches or continues the scan. Persistent history entries still arrive asynchronously through `on_entry_response`, where a pending search either accepts the response, skips a duplicate, or requests the next offset. The composer-facing pieces now live in `codex-rs/tui/src/bottom_pane/chat_composer/history_search.rs`, leaving `chat_composer.rs` responsible for routing and rendering integration instead of owning every search helper inline. `codex-rs/tui/src/bottom_pane/chat_composer_history.rs` remains the owner of stored history, combined offsets, async fetch state, boundary semantics, and duplicate suppression. Match highlighting is computed from the current composer text while search is active and disappears when the match is accepted. ## Observability There are no new logs or telemetry. The practical debug path is state inspection: `ChatComposer.history_search` tells whether the footer query is idle, searching, matched, or unmatched; `ChatComposerHistory.search` tracks selected raw offsets, pending persistent fetches, exhausted directions, and unique match cache state. If a user reports skipped or repeated results, first inspect the exact stored prompt text, the selected offset, whether an async persistent response is still pending, and whether a query edit restarted the search session. ## Tests The change is covered by focused `codex-tui` unit tests for opening search without previewing the latest entry, accepting and canceling search, no-match restoration, boundary clamping, footer hints, case-insensitive highlighting, local duplicate skipping, and persistent duplicate skipping through async responses. Snapshot coverage captures the footer-mode visual changes. Local verification used `just fmt`, `cargo test -p codex-tui history_search`, `cargo test -p codex-tui`, and `just fix -p codex-tui`. --- codex-rs/tui/src/bottom_pane/chat_composer.rs | 481 ++++++--- .../chat_composer/history_search.rs | 958 ++++++++++++++++++ .../src/bottom_pane/chat_composer_history.rs | 929 ++++++++++++++++- codex-rs/tui/src/bottom_pane/footer.rs | 39 +- codex-rs/tui/src/bottom_pane/mod.rs | 37 +- ...er__tests__footer_mode_history_search.snap | 13 + ...__tests__footer_mode_shortcut_overlay.snap | 4 +- ...shortcuts_collaboration_modes_enabled.snap | 5 +- ...tests__footer_shortcuts_shift_and_esc.snap | 4 +- codex-rs/tui/src/bottom_pane/textarea.rs | 78 +- docs/tui-chat-composer.md | 8 + 11 files changed, 2356 insertions(+), 200 deletions(-) create mode 100644 codex-rs/tui/src/bottom_pane/chat_composer/history_search.rs create mode 100644 codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_history_search.snap diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 0a82f7cf80..f66f6605ad 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -28,6 +28,9 @@ //! When recalling a persistent entry, only the text is restored. //! Recalled entries move the cursor to end-of-line so repeated Up/Down presses keep shell-like //! history traversal semantics instead of dropping to column 0. +//! `Ctrl+R` opens a reverse incremental search mode. The footer becomes the search input; once the +//! query is non-empty, the composer body previews the current match. `Enter` accepts the preview as +//! an editable draft and `Esc` restores the draft that was active when search started. //! //! Slash commands are staged for local history instead of being recorded immediately. Command //! recall is a two-phase handoff: stage the submitted slash text here, then record it after @@ -133,6 +136,8 @@ use ratatui::layout::Constraint; use ratatui::layout::Layout; use ratatui::layout::Margin; use ratatui::layout::Rect; +use ratatui::style::Modifier; +use ratatui::style::Style; use ratatui::style::Stylize; use ratatui::text::Line; use ratatui::text::Span; @@ -143,6 +148,7 @@ use ratatui::widgets::WidgetRef; use super::chat_composer_history::ChatComposerHistory; use super::chat_composer_history::HistoryEntry; +use super::chat_composer_history::HistoryEntryResponse; use super::command_popup::CommandItem; use super::command_popup::CommandPopup; use super::command_popup::CommandPopupFlags; @@ -186,6 +192,9 @@ use codex_protocol::user_input::ByteRange; use codex_protocol::user_input::MAX_USER_INPUT_TEXT_CHARS; use codex_protocol::user_input::TextElement; +mod history_search; + +use self::history_search::HistorySearchSession; use crate::app_event::AppEvent; use crate::app_event::ConnectorsSnapshot; use crate::app_event_sender::AppEventSender; @@ -356,6 +365,7 @@ pub(crate) struct ChatComposer { status_line_enabled: bool, // Agent label injected into the footer's contextual row when multi-agent mode is active. active_agent_label: Option, + history_search: Option, } #[derive(Clone, Debug)] @@ -364,6 +374,17 @@ struct FooterFlash { expires_at: Instant, } +#[derive(Clone, Debug)] +struct ComposerDraft { + text: String, + text_elements: Vec, + local_image_paths: Vec, + remote_image_urls: Vec, + mention_bindings: Vec, + pending_pastes: Vec<(String, String)>, + cursor: usize, +} + #[derive(Clone, Debug)] struct ComposerMentionBinding { mention: String, @@ -481,6 +502,7 @@ impl ChatComposer { status_line_value: None, status_line_enabled: false, active_agent_label: None, + history_search: None, }; // Apply configuration via the setter to keep side-effects centralized. this.set_disable_paste_burst(disable_paste_burst); @@ -648,6 +670,10 @@ impl ChatComposer { return None; } + if let Some(pos) = self.history_search_cursor_pos(area) { + return Some(pos); + } + let [_, _, textarea_rect, _] = self.layout_areas(area); let state = *self.textarea_state.borrow(); self.textarea.cursor_pos_with_state(textarea_rect, state) @@ -677,13 +703,22 @@ impl ChatComposer { offset: usize, entry: Option, ) -> bool { - let Some(entry) = self.history.on_entry_response(log_id, offset, entry) else { - return false; - }; - // Persistent ↑/↓ history is text-only (backwards-compatible and avoids persisting - // attachments), but local in-session ↑/↓ history can rehydrate elements and image paths. - self.apply_history_entry(entry); - true + match self + .history + .on_entry_response(log_id, offset, entry, &self.app_event_tx) + { + HistoryEntryResponse::Found(entry) => { + // Persistent ↑/↓ history is text-only (backwards-compatible and avoids persisting + // attachments), but local in-session ↑/↓ history can rehydrate elements and image paths. + self.apply_history_entry(entry); + true + } + HistoryEntryResponse::Search(result) => { + self.apply_history_search_result(result); + true + } + HistoryEntryResponse::Ignored => false, + } } /// Integrate pasted text into the composer. @@ -970,6 +1005,45 @@ impl ChatComposer { self.sync_popups(); } + fn snapshot_draft(&self) -> ComposerDraft { + ComposerDraft { + text: self.textarea.text().to_string(), + text_elements: self.textarea.text_elements(), + local_image_paths: self + .attached_images + .iter() + .map(|img| img.path.clone()) + .collect(), + remote_image_urls: self.remote_image_urls.clone(), + mention_bindings: self.snapshot_mention_bindings(), + pending_pastes: self.pending_pastes.clone(), + cursor: self.textarea.cursor(), + } + } + + fn restore_draft(&mut self, draft: ComposerDraft) { + let ComposerDraft { + text, + text_elements, + local_image_paths, + remote_image_urls, + mention_bindings, + pending_pastes, + cursor, + } = draft; + self.set_remote_image_urls(remote_image_urls); + self.set_text_content_with_mention_bindings( + text, + text_elements, + local_image_paths, + mention_bindings, + ); + self.set_pending_pastes(pending_pastes); + self.textarea + .set_cursor(cursor.min(self.textarea.text().len())); + self.sync_popups(); + } + /// Update the placeholder text without changing input enablement. pub(crate) fn set_placeholder_text(&mut self, placeholder: String) { self.placeholder_text = placeholder; @@ -1235,6 +1309,14 @@ impl ChatComposer { return (InputResult::None, false); } + if self.history_search.is_some() { + return self.handle_history_search_key(key_event); + } + + if Self::is_history_search_key(&key_event) { + return self.begin_history_search(); + } + let result = match &mut self.active_popup { ActivePopup::Command(_) => self.handle_key_event_with_slash_popup(key_event), ActivePopup::File(_) => self.handle_key_event_with_file_popup(key_event), @@ -1248,7 +1330,7 @@ impl ChatComposer { /// Return true if either the slash-command popup or the file-search popup is active. pub(crate) fn popup_active(&self) -> bool { - !matches!(self.active_popup, ActivePopup::None) + self.history_search.is_some() || !matches!(self.active_popup, ActivePopup::None) } /// Handle key event when the slash-command popup is visible. @@ -2900,6 +2982,10 @@ impl ChatComposer { /// modes (Esc hint, overlay, quit reminder) can override that base when /// their conditions are active. fn footer_mode(&self) -> FooterMode { + if self.history_search.is_some() { + return FooterMode::HistorySearch; + } + let base_mode = if self.is_empty() { FooterMode::ComposerEmpty } else { @@ -2907,6 +2993,7 @@ impl ChatComposer { }; match self.footer_mode { + FooterMode::HistorySearch => FooterMode::HistorySearch, FooterMode::EscHint => FooterMode::EscHint, FooterMode::ShortcutOverlay => FooterMode::ShortcutOverlay, FooterMode::QuitShortcutReminder if self.quit_shortcut_hint_visible() => { @@ -2933,6 +3020,17 @@ impl ChatComposer { pub(crate) fn sync_popups(&mut self) { self.sync_slash_command_elements(); + if self.history_search.is_some() { + if self.current_file_query.is_some() { + self.app_event_tx + .send(AppEvent::StartFileSearch(String::new())); + self.current_file_query = None; + } + self.active_popup = ActivePopup::None; + self.dismissed_file_popup_token = None; + self.dismissed_mention_popup_token = None; + return; + } if !self.popups_enabled() { self.active_popup = ActivePopup::None; return; @@ -3500,6 +3598,10 @@ impl Renderable for ChatComposer { return None; } + if let Some(pos) = self.history_search_cursor_pos(area) { + return Some(pos); + } + let [_, _, textarea_rect, _] = self.layout_areas(area); let state = *self.textarea_state.borrow(); self.textarea.cursor_pos_with_state(textarea_rect, state) @@ -3558,13 +3660,15 @@ impl ChatComposer { let show_shortcuts_hint = match footer_props.mode { FooterMode::ComposerEmpty => !self.is_in_paste_burst(), FooterMode::ComposerHasDraft => false, - FooterMode::QuitShortcutReminder + FooterMode::HistorySearch + | FooterMode::QuitShortcutReminder | FooterMode::ShortcutOverlay | FooterMode::EscHint => false, }; let show_queue_hint = match footer_props.mode { FooterMode::ComposerHasDraft => footer_props.is_task_running, - FooterMode::QuitShortcutReminder + FooterMode::HistorySearch + | FooterMode::QuitShortcutReminder | FooterMode::ComposerEmpty | FooterMode::ShortcutOverlay | FooterMode::EscHint => false, @@ -3583,124 +3687,141 @@ impl ChatComposer { } else { popup_rect }; - let available_width = - hint_rect.width.saturating_sub(FOOTER_INDENT_COLS as u16) as usize; - let status_line_active = uses_passive_footer_status_layout(&footer_props); - let combined_status_line = if status_line_active { - passive_footer_status_line(&footer_props).map(ratatui::prelude::Stylize::dim) + if let Some(line) = self.history_search_footer_line() { + render_footer_line(hint_rect, buf, line); } else { - None - }; - let mut truncated_status_line = if status_line_active { - combined_status_line.as_ref().map(|line| { - truncate_line_with_ellipsis_if_overflow(line.clone(), available_width) - }) - } else { - None - }; - let left_mode_indicator = if status_line_active { - None - } else { - self.collaboration_mode_indicator - }; - let mut left_width = if self.footer_flash_visible() { - self.footer_flash - .as_ref() - .map(|flash| flash.line.width() as u16) - .unwrap_or(0) - } else if let Some(items) = self.footer_hint_override.as_ref() { - footer_hint_items_width(items) - } else if status_line_active { - truncated_status_line - .as_ref() - .map(|line| line.width() as u16) - .unwrap_or(0) - } else { - footer_line_width( - &footer_props, - left_mode_indicator, - show_cycle_hint, - show_shortcuts_hint, - show_queue_hint, - ) - }; - let right_line = if status_line_active { - let full = - mode_indicator_line(self.collaboration_mode_indicator, show_cycle_hint); - let compact = mode_indicator_line( - self.collaboration_mode_indicator, - /*show_cycle_hint*/ false, - ); - let full_width = full.as_ref().map(|l| l.width() as u16).unwrap_or(0); - if can_show_left_with_context(hint_rect, left_width, full_width) { - full + let available_width = + hint_rect.width.saturating_sub(FOOTER_INDENT_COLS as u16) as usize; + let status_line_active = uses_passive_footer_status_layout(&footer_props); + let combined_status_line = if status_line_active { + passive_footer_status_line(&footer_props) + .map(ratatui::prelude::Stylize::dim) } else { - compact - } - } else { - Some(context_window_line( - footer_props.context_window_percent, - footer_props.context_window_used_tokens, - )) - }; - let right_width = right_line.as_ref().map(|l| l.width() as u16).unwrap_or(0); - if status_line_active - && let Some(max_left) = max_left_width_for_right(hint_rect, right_width) - && left_width > max_left - && let Some(line) = combined_status_line.as_ref().map(|line| { - truncate_line_with_ellipsis_if_overflow(line.clone(), max_left as usize) - }) - { - left_width = line.width() as u16; - truncated_status_line = Some(line); - } - let can_show_left_and_context = - can_show_left_with_context(hint_rect, left_width, right_width); - let has_override = - self.footer_flash_visible() || self.footer_hint_override.is_some(); - let single_line_layout = if has_override || status_line_active { - None - } else { - match footer_props.mode { - FooterMode::ComposerEmpty | FooterMode::ComposerHasDraft => { - // Both of these modes render the single-line footer style (with - // either the shortcuts hint or the optional queue hint). We still - // want the single-line collapse rules so the mode label can win over - // the context indicator on narrow widths. - Some(single_line_footer_layout( - hint_rect, - right_width, - left_mode_indicator, - show_cycle_hint, - show_shortcuts_hint, - show_queue_hint, - )) + None + }; + let mut truncated_status_line = if status_line_active { + combined_status_line.as_ref().map(|line| { + truncate_line_with_ellipsis_if_overflow(line.clone(), available_width) + }) + } else { + None + }; + let left_mode_indicator = if status_line_active { + None + } else { + self.collaboration_mode_indicator + }; + let mut left_width = if self.footer_flash_visible() { + self.footer_flash + .as_ref() + .map(|flash| flash.line.width() as u16) + .unwrap_or(0) + } else if let Some(items) = self.footer_hint_override.as_ref() { + footer_hint_items_width(items) + } else if status_line_active { + truncated_status_line + .as_ref() + .map(|line| line.width() as u16) + .unwrap_or(0) + } else { + footer_line_width( + &footer_props, + left_mode_indicator, + show_cycle_hint, + show_shortcuts_hint, + show_queue_hint, + ) + }; + let right_line = if status_line_active { + let full = + mode_indicator_line(self.collaboration_mode_indicator, show_cycle_hint); + let compact = mode_indicator_line( + self.collaboration_mode_indicator, + /*show_cycle_hint*/ false, + ); + let full_width = full.as_ref().map(|l| l.width() as u16).unwrap_or(0); + if can_show_left_with_context(hint_rect, left_width, full_width) { + full + } else { + compact } - FooterMode::EscHint - | FooterMode::QuitShortcutReminder - | FooterMode::ShortcutOverlay => None, + } else { + Some(context_window_line( + footer_props.context_window_percent, + footer_props.context_window_used_tokens, + )) + }; + let right_width = right_line.as_ref().map(|l| l.width() as u16).unwrap_or(0); + if status_line_active + && let Some(max_left) = max_left_width_for_right(hint_rect, right_width) + && left_width > max_left + && let Some(line) = combined_status_line.as_ref().map(|line| { + truncate_line_with_ellipsis_if_overflow(line.clone(), max_left as usize) + }) + { + left_width = line.width() as u16; + truncated_status_line = Some(line); } - }; - let show_right = if matches!( - footer_props.mode, - FooterMode::EscHint - | FooterMode::QuitShortcutReminder - | FooterMode::ShortcutOverlay - ) { - false - } else { - single_line_layout - .as_ref() - .map(|(_, show_context)| *show_context) - .unwrap_or(can_show_left_and_context) - }; + let can_show_left_and_context = + can_show_left_with_context(hint_rect, left_width, right_width); + let has_override = + self.footer_flash_visible() || self.footer_hint_override.is_some(); + let single_line_layout = if has_override || status_line_active { + None + } else { + match footer_props.mode { + FooterMode::ComposerEmpty | FooterMode::ComposerHasDraft => { + // Both of these modes render the single-line footer style (with + // either the shortcuts hint or the optional queue hint). We still + // want the single-line collapse rules so the mode label can win over + // the context indicator on narrow widths. + Some(single_line_footer_layout( + hint_rect, + right_width, + left_mode_indicator, + show_cycle_hint, + show_shortcuts_hint, + show_queue_hint, + )) + } + FooterMode::EscHint + | FooterMode::HistorySearch + | FooterMode::QuitShortcutReminder + | FooterMode::ShortcutOverlay => None, + } + }; + let show_right = if matches!( + footer_props.mode, + FooterMode::EscHint + | FooterMode::HistorySearch + | FooterMode::QuitShortcutReminder + | FooterMode::ShortcutOverlay + ) { + false + } else { + single_line_layout + .as_ref() + .map(|(_, show_context)| *show_context) + .unwrap_or(can_show_left_and_context) + }; - if let Some((summary_left, _)) = single_line_layout { - match summary_left { - SummaryLeft::Default => { - if status_line_active { - if let Some(line) = truncated_status_line.clone() { - render_footer_line(hint_rect, buf, line); + if let Some((summary_left, _)) = single_line_layout { + match summary_left { + SummaryLeft::Default => { + if status_line_active { + if let Some(line) = truncated_status_line.clone() { + render_footer_line(hint_rect, buf, line); + } else { + render_footer_from_props( + hint_rect, + buf, + &footer_props, + left_mode_indicator, + show_cycle_hint, + show_shortcuts_hint, + show_queue_hint, + ); + } } else { render_footer_from_props( hint_rect, @@ -3712,47 +3833,37 @@ impl ChatComposer { show_queue_hint, ); } - } else { - render_footer_from_props( - hint_rect, - buf, - &footer_props, - left_mode_indicator, - show_cycle_hint, - show_shortcuts_hint, - show_queue_hint, - ); } + SummaryLeft::Custom(line) => { + render_footer_line(hint_rect, buf, line); + } + SummaryLeft::None => {} } - SummaryLeft::Custom(line) => { + } else if self.footer_flash_visible() { + if let Some(flash) = self.footer_flash.as_ref() { + flash.line.render(inset_footer_hint_area(hint_rect), buf); + } + } else if let Some(items) = self.footer_hint_override.as_ref() { + render_footer_hint_items(hint_rect, buf, items); + } else if status_line_active { + if let Some(line) = truncated_status_line { render_footer_line(hint_rect, buf, line); } - SummaryLeft::None => {} + } else { + render_footer_from_props( + hint_rect, + buf, + &footer_props, + self.collaboration_mode_indicator, + show_cycle_hint, + show_shortcuts_hint, + show_queue_hint, + ); } - } else if self.footer_flash_visible() { - if let Some(flash) = self.footer_flash.as_ref() { - flash.line.render(inset_footer_hint_area(hint_rect), buf); - } - } else if let Some(items) = self.footer_hint_override.as_ref() { - render_footer_hint_items(hint_rect, buf, items); - } else if status_line_active { - if let Some(line) = truncated_status_line { - render_footer_line(hint_rect, buf, line); - } - } else { - render_footer_from_props( - hint_rect, - buf, - &footer_props, - self.collaboration_mode_indicator, - show_cycle_hint, - show_shortcuts_hint, - show_queue_hint, - ); - } - if show_right && let Some(line) = &right_line { - render_context_right(hint_rect, buf, line); + if show_right && let Some(line) = &right_line { + render_context_right(hint_rect, buf, line); + } } } } @@ -3828,10 +3939,44 @@ impl ChatComposer { } else if is_zellij && textarea_is_empty { buf.set_style(textarea_rect, textarea_style); } else if is_zellij { - self.textarea - .render_ref_styled(textarea_rect, buf, &mut state, textarea_style); + let highlight_ranges = self.history_search_highlight_ranges(); + if highlight_ranges.is_empty() { + self.textarea + .render_ref_styled(textarea_rect, buf, &mut state, textarea_style); + } else { + let highlight_style = + textarea_style.add_modifier(Modifier::REVERSED | Modifier::BOLD); + let highlights = highlight_ranges + .into_iter() + .map(|range| (range, highlight_style)) + .collect::>(); + self.textarea.render_ref_styled_with_highlights( + textarea_rect, + buf, + &mut state, + textarea_style, + &highlights, + ); + } } else { - StatefulWidgetRef::render_ref(&(&self.textarea), textarea_rect, buf, &mut state); + let highlight_ranges = self.history_search_highlight_ranges(); + if highlight_ranges.is_empty() { + StatefulWidgetRef::render_ref(&(&self.textarea), textarea_rect, buf, &mut state); + } else { + let highlight_style = + Style::default().add_modifier(Modifier::REVERSED | Modifier::BOLD); + let highlights = highlight_ranges + .into_iter() + .map(|range| (range, highlight_style)) + .collect::>(); + self.textarea.render_ref_styled_with_highlights( + textarea_rect, + buf, + &mut state, + Style::default(), + &highlights, + ); + } } if textarea_is_empty { let text = if self.input_enabled { @@ -4191,6 +4336,20 @@ mod tests { type_chars_humanlike(composer, &['h']); }, ); + + snapshot_composer_state( + "footer_mode_history_search", + /*enhanced_keys_supported*/ true, + |composer| { + composer + .history + .record_local_submission(HistoryEntry::new("cargo test".to_string())); + let _ = composer + .handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::CONTROL)); + let _ = composer + .handle_key_event(KeyEvent::new(KeyCode::Char('c'), KeyModifiers::NONE)); + }, + ); } #[test] diff --git a/codex-rs/tui/src/bottom_pane/chat_composer/history_search.rs b/codex-rs/tui/src/bottom_pane/chat_composer/history_search.rs new file mode 100644 index 0000000000..b4f6121563 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/chat_composer/history_search.rs @@ -0,0 +1,958 @@ +//! Composer-side Ctrl+R reverse history search state and rendering helpers. +//! +//! The persistent and local history stores live in `chat_composer_history`, but the composer owns +//! the active search session because it has to snapshot/restore the editable draft, preview matches +//! in the textarea, and render the footer prompt while the footer line is acting as the search +//! input. +//! +//! This module is responsible for the UI-facing lifecycle of a search session: recognizing the +//! keys that enter and drive search mode, keeping the footer query separate from the textarea +//! preview, restoring the original draft on cancellation or misses, and translating history search +//! results into composer-visible state. It deliberately does not decide which history entries +//! match, how duplicate results are skipped, or when persistent history should be fetched; those +//! traversal invariants stay with `ChatComposerHistory`. +//! +//! A search session starts idle with an empty footer query, so opening Ctrl+R never previews the +//! latest history entry by itself. Typing a query restarts traversal from newest to oldest, +//! repeated Ctrl+R/Up and Ctrl+S/Down move between unique matches, `Enter` accepts the current +//! preview as an editable draft, and `Esc` or Ctrl+C restores the exact draft that existed before +//! search started. + +use std::ops::Range; + +use crossterm::event::KeyCode; +use crossterm::event::KeyEvent; +use crossterm::event::KeyEventKind; +use crossterm::event::KeyModifiers; +use ratatui::layout::Constraint; +use ratatui::layout::Layout; +use ratatui::layout::Rect; +use ratatui::style::Stylize; +use ratatui::text::Line; +use ratatui::text::Span; + +use super::super::chat_composer_history::HistorySearchDirection; +use super::super::chat_composer_history::HistorySearchResult; +use super::super::footer::footer_height; +use super::super::footer::reset_mode_after_activity; +use super::ActivePopup; +use super::ChatComposer; +use super::ComposerDraft; +use super::InputResult; +use crate::app_event::AppEvent; +use crate::key_hint; +use crate::key_hint::has_ctrl_or_alt; +use crate::ui_consts::FOOTER_INDENT_COLS; + +/// Active composer-owned state for one Ctrl+R search interaction. +/// +/// The session is created only by [`ChatComposer::begin_history_search`] and is cleared only by +/// accepting, canceling, or replacing the search mode. It stores the original draft separately from +/// the footer query so transient previews never destroy the user's in-progress composer content. +#[derive(Clone, Debug)] +pub(super) struct HistorySearchSession { + /// Draft to restore when search is canceled or a query has no match. + original_draft: ComposerDraft, + /// Footer-owned query text typed while Ctrl+R search is active. + query: String, + /// User-visible search status used to choose footer hints and composer preview behavior. + status: HistorySearchStatus, +} + +/// User-visible phase of the active Ctrl+R search session. +/// +/// Search keeps the footer query and the composer preview separate: `Idle` leaves the original +/// draft untouched, `Searching` waits for persistent history, `Match` previews a found entry, and +/// `NoMatch` restores the original draft while leaving the search input open for more typing. +#[derive(Clone, Debug)] +enum HistorySearchStatus { + Idle, + Searching, + Match, + NoMatch, +} + +impl ChatComposer { + #[cfg(test)] + pub(super) fn history_search_active(&self) -> bool { + self.history_search.is_some() + } + + /// Returns whether a key event should open reverse history search or step to an older match. + /// + /// The check accepts both normal Ctrl+R reports and the raw control character variant that + /// some terminals emit. Callers should only use this before generic text handling; treating the + /// raw control character as ordinary input would insert an invisible byte into the search query + /// or composer draft. + pub(super) fn is_history_search_key(key_event: &KeyEvent) -> bool { + matches!( + key_event, + KeyEvent { + code: KeyCode::Char(c), + modifiers, + kind: KeyEventKind::Press | KeyEventKind::Repeat, + .. + } if modifiers.contains(KeyModifiers::CONTROL) && c.eq_ignore_ascii_case(&'r') + ) || matches!( + key_event, + KeyEvent { + code: KeyCode::Char('\u{0012}'), + modifiers: KeyModifiers::NONE, + kind: KeyEventKind::Press | KeyEventKind::Repeat, + .. + } + ) + } + + fn is_history_search_forward_key(key_event: &KeyEvent) -> bool { + matches!( + key_event, + KeyEvent { + code: KeyCode::Char(c), + modifiers, + kind: KeyEventKind::Press | KeyEventKind::Repeat, + .. + } if modifiers.contains(KeyModifiers::CONTROL) && c.eq_ignore_ascii_case(&'s') + ) || matches!( + key_event, + KeyEvent { + code: KeyCode::Char('\u{0013}'), + modifiers: KeyModifiers::NONE, + kind: KeyEventKind::Press | KeyEventKind::Repeat, + .. + } + ) + } + + /// Opens footer-owned reverse history search without previewing history yet. + /// + /// Entering search mode first flushes pending paste-burst text, then snapshots the full + /// composer draft, clears any file/search popup state, and resets history traversal. The first + /// visible match is produced only after the footer query becomes non-empty, which keeps Ctrl+R + /// from replacing an empty composer with the latest prompt before the user has searched for + /// anything. + pub(super) fn begin_history_search(&mut self) -> (InputResult, bool) { + if let Some(pasted) = self.paste_burst.flush_before_modified_input() { + self.handle_paste(pasted); + } + self.paste_burst.clear_window_after_non_char(); + + if self.current_file_query.is_some() { + self.app_event_tx + .send(AppEvent::StartFileSearch(String::new())); + self.current_file_query = None; + } + self.active_popup = ActivePopup::None; + self.selected_remote_image_index = None; + self.history_search = Some(HistorySearchSession { + original_draft: self.snapshot_draft(), + query: String::new(), + status: HistorySearchStatus::Idle, + }); + self.history.reset_search(); + (InputResult::None, true) + } + + /// Handles every key while the footer is acting as the history search input. + /// + /// The method consumes search-mode keys before normal composer editing sees them. It guarantees + /// that `Esc` and Ctrl+C restore the original draft, `Enter` only accepts an actual match, plain + /// characters edit the footer query, and navigation keys delegate traversal to + /// `ChatComposerHistory`. Calling this when no search session exists is harmless for ignored + /// keys but would make query-edit branches no-op, so route here only after + /// `history_search.is_some()` has been established. + pub(super) fn handle_history_search_key(&mut self, key_event: KeyEvent) -> (InputResult, bool) { + if key_event.kind == KeyEventKind::Release { + return (InputResult::None, false); + } + + if Self::is_history_search_key(&key_event) || matches!(key_event.code, KeyCode::Up) { + let result = self.history_search_in_direction(HistorySearchDirection::Older); + return (result, true); + } + + if Self::is_history_search_forward_key(&key_event) + || matches!(key_event.code, KeyCode::Down) + { + let result = self.history_search_in_direction(HistorySearchDirection::Newer); + return (result, true); + } + + match key_event { + KeyEvent { + code: KeyCode::Esc, .. + } => { + self.cancel_history_search(); + (InputResult::None, true) + } + KeyEvent { + code: KeyCode::Char(c), + modifiers, + .. + } if modifiers.contains(KeyModifiers::CONTROL) && c.eq_ignore_ascii_case(&'c') => { + self.cancel_history_search(); + (InputResult::None, true) + } + KeyEvent { + code: KeyCode::Char('\u{0003}'), + modifiers: KeyModifiers::NONE, + .. + } => { + self.cancel_history_search(); + (InputResult::None, true) + } + KeyEvent { + code: KeyCode::Enter, + modifiers: KeyModifiers::NONE, + .. + } => { + if self + .history_search + .as_ref() + .is_some_and(|search| matches!(search.status, HistorySearchStatus::Match)) + { + self.history_search = None; + self.history.reset_search(); + self.footer_mode = reset_mode_after_activity(self.footer_mode); + self.move_cursor_to_end(); + } + (InputResult::None, true) + } + KeyEvent { + code: KeyCode::Backspace, + .. + } + | KeyEvent { + code: KeyCode::Char('h'), + modifiers: KeyModifiers::CONTROL, + .. + } => { + if let Some(search) = self.history_search.as_ref() { + let mut query = search.query.clone(); + query.pop(); + self.update_history_search_query(query); + } + (InputResult::None, true) + } + KeyEvent { + code: KeyCode::Char('u'), + modifiers: KeyModifiers::CONTROL, + .. + } => { + self.update_history_search_query(String::new()); + (InputResult::None, true) + } + KeyEvent { + code: KeyCode::Char(ch), + modifiers, + .. + } if !has_ctrl_or_alt(modifiers) => { + if let Some(search) = self.history_search.as_ref() { + let mut query = search.query.clone(); + query.push(ch); + self.update_history_search_query(query); + } + (InputResult::None, true) + } + _ => (InputResult::None, true), + } + } + + fn history_search_in_direction(&mut self, direction: HistorySearchDirection) -> InputResult { + let Some((query, original_draft)) = self + .history_search + .as_ref() + .map(|search| (search.query.clone(), search.original_draft.clone())) + else { + return InputResult::None; + }; + if query.is_empty() { + self.history.reset_search(); + if let Some(search) = self.history_search.as_mut() { + search.status = HistorySearchStatus::Idle; + } + self.restore_draft(original_draft); + return InputResult::None; + } + let result = self.history.search( + &query, + direction, + /*restart*/ false, + &self.app_event_tx, + ); + self.apply_history_search_result(result); + InputResult::None + } + + fn update_history_search_query(&mut self, query: String) { + let Some(original_draft) = self + .history_search + .as_ref() + .map(|search| search.original_draft.clone()) + else { + return; + }; + if let Some(search) = self.history_search.as_mut() { + search.query = query.clone(); + search.status = HistorySearchStatus::Searching; + } + self.restore_draft(original_draft); + if query.is_empty() { + self.history.reset_search(); + if let Some(search) = self.history_search.as_mut() { + search.status = HistorySearchStatus::Idle; + } + return; + } + let result = self.history.search( + &query, + HistorySearchDirection::Older, + /*restart*/ true, + &self.app_event_tx, + ); + self.apply_history_search_result(result); + } + + /// Cancels active history search and restores the draft from before search mode opened. + /// + /// This clears normal history navigation as well as search traversal because previewing a match + /// temporarily updates the shared history cursor. Callers that handle global cancellation, such + /// as Ctrl+C, should use the boolean result to consume the key without also clearing the + /// restored draft or triggering quit/interrupt behavior. + pub(crate) fn cancel_history_search(&mut self) -> bool { + let Some(search) = self.history_search.take() else { + return false; + }; + self.history.reset_navigation(); + self.footer_mode = reset_mode_after_activity(self.footer_mode); + self.restore_draft(search.original_draft); + true + } + + /// Applies a traversal result to the composer preview and search status. + /// + /// `Found` previews the matching entry, `Pending` keeps the footer in a waiting state while an + /// async persistent entry lookup is outstanding, `AtBoundary` preserves the current match, and + /// `NotFound` restores the original draft while keeping the query available for further edits. + /// Treating `AtBoundary` like `NotFound` would produce the visible "no match" flicker at the + /// end of a one-result search and desynchronize Up/Down counts. + pub(super) fn apply_history_search_result(&mut self, result: HistorySearchResult) { + match result { + HistorySearchResult::Found(entry) => { + if let Some(search) = self.history_search.as_mut() { + search.status = HistorySearchStatus::Match; + } + self.apply_history_entry(entry); + } + HistorySearchResult::Pending => { + if let Some(search) = self.history_search.as_mut() { + search.status = HistorySearchStatus::Searching; + } + } + HistorySearchResult::AtBoundary => { + if let Some(search) = self.history_search.as_mut() { + search.status = HistorySearchStatus::Match; + } + } + HistorySearchResult::NotFound => { + let original_draft = self + .history_search + .as_ref() + .map(|search| search.original_draft.clone()); + if let Some(search) = self.history_search.as_mut() { + search.status = HistorySearchStatus::NoMatch; + } + if let Some(original_draft) = original_draft { + self.restore_draft(original_draft); + } + } + } + } + + /// Builds the footer line shown while reverse history search is active. + /// + /// The footer displays the query as the editable field and uses the status to decide whether + /// to show searching, match actions, or no-match feedback. The line is intentionally separate + /// from cursor placement so rendering can fall back to normal footer layout if a small terminal + /// cannot allocate a distinct hint row. + pub(super) fn history_search_footer_line(&self) -> Option> { + let search = self.history_search.as_ref()?; + let mut line = Line::from(vec![ + "reverse-i-search: ".dim(), + search.query.clone().cyan(), + ]); + match search.status { + HistorySearchStatus::Idle => {} + HistorySearchStatus::Searching => line.push_span(" searching".dim()), + HistorySearchStatus::Match => { + line.push_span(" ".dim()); + line.push_span(Self::history_search_action_key_span(KeyCode::Enter)); + line.push_span(" accept".dim()); + line.push_span(" · ".dim()); + line.push_span(Self::history_search_action_key_span(KeyCode::Esc)); + line.push_span(" cancel".dim()); + } + HistorySearchStatus::NoMatch => line.push_span(" no match".red()), + } + Some(line) + } + + fn history_search_action_key_span(key: KeyCode) -> Span<'static> { + Span::from(key_hint::plain(key)).cyan().bold().not_dim() + } + + /// Returns byte ranges that should be highlighted in the current composer preview. + /// + /// Highlights are only exposed while a matched history entry is being previewed. Once the user + /// accepts with `Enter`, the search session is cleared and this returns an empty set so the + /// accepted text becomes an ordinary editable draft again. + pub(super) fn history_search_highlight_ranges(&self) -> Vec> { + let Some(search) = self.history_search.as_ref() else { + return Vec::new(); + }; + if !matches!(search.status, HistorySearchStatus::Match) || search.query.is_empty() { + return Vec::new(); + } + Self::case_insensitive_match_ranges(self.textarea.text(), &search.query) + } + + fn case_insensitive_match_ranges(text: &str, query: &str) -> Vec> { + if query.is_empty() { + return Vec::new(); + } + + let query_lower = query + .chars() + .flat_map(char::to_lowercase) + .collect::(); + if query_lower.is_empty() { + return Vec::new(); + } + + let mut folded = String::new(); + let mut folded_spans: Vec<(Range, Range)> = Vec::new(); + for (original_start, ch) in text.char_indices() { + let original_range = original_start..original_start + ch.len_utf8(); + for lower in ch.to_lowercase() { + let folded_start = folded.len(); + folded.push(lower); + folded_spans.push((folded_start..folded.len(), original_range.clone())); + } + } + + let mut ranges = Vec::new(); + let mut search_from = 0; + while search_from <= folded.len() + && let Some(relative_start) = folded[search_from..].find(&query_lower) + { + let folded_start = search_from + relative_start; + let folded_end = folded_start + query_lower.len(); + if let Some((_, first_original)) = folded_spans.iter().find(|(folded_range, _)| { + folded_range.end > folded_start && folded_range.start < folded_end + }) { + let original_end = folded_spans + .iter() + .rev() + .find(|(folded_range, _)| { + folded_range.end > folded_start && folded_range.start < folded_end + }) + .map(|(_, original_range)| original_range.end) + .unwrap_or(first_original.end); + ranges.push(first_original.start..original_end); + } + search_from = folded_end; + } + ranges + } + + /// Returns the screen cursor position for the footer query when search mode is active. + /// + /// The cursor tracks the end of the footer query rather than the textarea preview. If the + /// footer area is collapsed or too narrow, the x coordinate is clamped inside the hint rect so + /// terminal backends do not receive an off-screen cursor position. + pub(super) fn history_search_cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { + let search = self.history_search.as_ref()?; + let [_, _, _, popup_rect] = self.layout_areas(area); + if popup_rect.is_empty() { + return None; + } + + let footer_props = self.footer_props(); + let footer_hint_height = self + .custom_footer_height() + .unwrap_or_else(|| footer_height(&footer_props)); + let footer_spacing = Self::footer_spacing(footer_hint_height); + let hint_rect = if footer_spacing > 0 && footer_hint_height > 0 { + let [_, hint_rect] = Layout::vertical([ + Constraint::Length(footer_spacing), + Constraint::Length(footer_hint_height), + ]) + .areas(popup_rect); + hint_rect + } else { + popup_rect + }; + if hint_rect.is_empty() { + return None; + } + + let prompt_width = Line::from("reverse-i-search: ").width() as u16; + let query_width = Line::from(search.query.clone()).width() as u16; + let desired_x = hint_rect + .x + .saturating_add(FOOTER_INDENT_COLS as u16) + .saturating_add(prompt_width) + .saturating_add(query_width); + let max_x = hint_rect + .x + .saturating_add(hint_rect.width.saturating_sub(1)); + Some((desired_x.min(max_x), hint_rect.y)) + } +} + +#[cfg(test)] +mod tests { + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + use pretty_assertions::assert_eq; + use ratatui::buffer::Buffer; + use ratatui::layout::Rect; + use ratatui::style::Modifier; + use tokio::sync::mpsc::unbounded_channel; + + use super::super::super::chat_composer_history::HistoryEntry; + use super::super::super::footer::FooterMode; + use super::super::ChatComposer; + use super::HistorySearchStatus; + use crate::app_event::AppEvent; + use crate::app_event_sender::AppEventSender; + use crate::render::renderable::Renderable; + + #[test] + fn history_search_opens_without_previewing_latest_entry() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer + .history + .record_local_submission(HistoryEntry::new("remembered command".to_string())); + composer.set_text_content(String::new(), Vec::new(), Vec::new()); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::CONTROL)); + + assert!(composer.history_search_active()); + assert!(composer.textarea.is_empty()); + assert_eq!(composer.footer_mode(), FooterMode::HistorySearch); + } + + #[test] + fn history_search_match_ranges_are_case_insensitive() { + assert_eq!( + ChatComposer::case_insensitive_match_ranges("git status git", "GIT"), + vec![0..3, 11..14] + ); + assert_eq!( + ChatComposer::case_insensitive_match_ranges("aİ i", "i"), + vec![1..3, 4..5] + ); + assert!(ChatComposer::case_insensitive_match_ranges("git", "").is_empty()); + } + + #[test] + fn history_search_accepts_matching_entry() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer + .history + .record_local_submission(HistoryEntry::new("git status".to_string())); + composer + .history + .record_local_submission(HistoryEntry::new("cargo test".to_string())); + composer.set_text_content("draft".to_string(), Vec::new(), Vec::new()); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::CONTROL)); + assert!(composer.history_search_active()); + assert_eq!(composer.textarea.text(), "draft"); + + for ch in ['g', 'i', 't'] { + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char(ch), KeyModifiers::NONE)); + } + assert_eq!(composer.textarea.text(), "git status"); + assert_eq!(composer.footer_mode(), FooterMode::HistorySearch); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + assert!(!composer.history_search_active()); + assert_eq!(composer.textarea.text(), "git status"); + assert_eq!(composer.textarea.cursor(), composer.textarea.text().len()); + } + + #[test] + fn history_search_stays_on_single_match_at_boundaries() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer.history.record_local_submission(HistoryEntry::new( + "Find and fix a bug in @filename".to_string(), + )); + composer.set_text_content("draft".to_string(), Vec::new(), Vec::new()); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::CONTROL)); + for ch in ['b', 'u', 'g'] { + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char(ch), KeyModifiers::NONE)); + } + assert_eq!(composer.textarea.text(), "Find and fix a bug in @filename"); + + for _ in 0..3 { + let _ = + composer.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::CONTROL)); + } + assert_eq!(composer.textarea.text(), "Find and fix a bug in @filename"); + assert!( + composer + .history_search + .as_ref() + .is_some_and(|search| matches!(search.status, HistorySearchStatus::Match)) + ); + + for _ in 0..3 { + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)); + } + assert_eq!(composer.textarea.text(), "Find and fix a bug in @filename"); + assert!( + composer + .history_search + .as_ref() + .is_some_and(|search| matches!(search.status, HistorySearchStatus::Match)) + ); + } + + #[test] + fn history_search_footer_action_hints_are_emphasized() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ true, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer + .history + .record_local_submission(HistoryEntry::new("cargo test".to_string())); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::CONTROL)); + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('c'), KeyModifiers::NONE)); + + let line = composer + .history_search_footer_line() + .expect("expected history search footer line"); + assert_eq!( + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::>(), + vec![ + "reverse-i-search: ", + "c", + " ", + "enter", + " accept", + " · ", + "esc", + " cancel" + ] + ); + + let query_style = line.spans[1].style; + assert_eq!(query_style.fg, Some(ratatui::style::Color::Cyan)); + + let enter_style = line.spans[3].style; + assert_eq!(enter_style.fg, Some(ratatui::style::Color::Cyan)); + assert!(enter_style.add_modifier.contains(Modifier::BOLD)); + assert!(enter_style.sub_modifier.contains(Modifier::DIM)); + + let accept_style = line.spans[4].style; + assert!(accept_style.add_modifier.contains(Modifier::DIM)); + + let separator_style = line.spans[5].style; + assert!(separator_style.add_modifier.contains(Modifier::DIM)); + + let esc_style = line.spans[6].style; + assert_eq!(esc_style.fg, Some(ratatui::style::Color::Cyan)); + assert!(esc_style.add_modifier.contains(Modifier::BOLD)); + assert!(esc_style.sub_modifier.contains(Modifier::DIM)); + + let cancel_style = line.spans[7].style; + assert!(cancel_style.add_modifier.contains(Modifier::DIM)); + } + + #[test] + fn history_search_highlights_matches_until_accepted() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ true, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer + .history + .record_local_submission(HistoryEntry::new("cargo test".to_string())); + composer + .history + .record_local_submission(HistoryEntry::new("git status".to_string())); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::CONTROL)); + for ch in ['g', 'i', 't'] { + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char(ch), KeyModifiers::NONE)); + } + + let area = Rect::new(0, 0, 60, 8); + let [_, _, textarea_rect, _] = composer.layout_areas(area); + let mut buf = Buffer::empty(area); + composer.render(area, &mut buf); + let x = textarea_rect.x; + let y = textarea_rect.y; + assert_eq!(buf[(x, y)].symbol(), "g"); + for offset in 0..3 { + let modifier = buf[(x + offset, y)].style().add_modifier; + assert!(modifier.contains(Modifier::REVERSED)); + assert!(modifier.contains(Modifier::BOLD)); + } + assert!( + !buf[(x + 3, y)] + .style() + .add_modifier + .contains(Modifier::REVERSED) + ); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + let [_, _, accepted_textarea_rect, _] = composer.layout_areas(area); + let mut accepted_buf = Buffer::empty(area); + composer.render(area, &mut accepted_buf); + for offset in 0..3 { + let modifier = accepted_buf + [(accepted_textarea_rect.x + offset, accepted_textarea_rect.y)] + .style() + .add_modifier; + assert!(!modifier.contains(Modifier::REVERSED)); + assert!(!modifier.contains(Modifier::BOLD)); + } + } + + #[test] + fn history_search_esc_restores_original_draft() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer + .history + .record_local_submission(HistoryEntry::new("remembered command".to_string())); + composer.set_text_content("draft".to_string(), Vec::new(), Vec::new()); + composer.textarea.set_cursor(/*pos*/ 2); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::CONTROL)); + assert_eq!(composer.textarea.text(), "draft"); + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::NONE)); + assert_eq!(composer.textarea.text(), "remembered command"); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE)); + assert!(!composer.history_search_active()); + assert_eq!(composer.textarea.text(), "draft"); + assert_eq!(composer.textarea.cursor(), 2); + } + + #[test] + fn history_search_ctrl_c_restores_original_draft() { + fn composer_with_search_preview() -> ChatComposer { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer + .history + .record_local_submission(HistoryEntry::new("remembered command".to_string())); + composer.set_text_content("draft".to_string(), Vec::new(), Vec::new()); + composer.textarea.set_cursor(/*pos*/ 2); + + let _ = + composer.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::CONTROL)); + let _ = + composer.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::NONE)); + assert_eq!(composer.textarea.text(), "remembered command"); + composer + } + + for cancel_key in [ + KeyEvent::new(KeyCode::Char('c'), KeyModifiers::CONTROL), + KeyEvent::new(KeyCode::Char('\u{0003}'), KeyModifiers::NONE), + ] { + let mut composer = composer_with_search_preview(); + + let _ = composer.handle_key_event(cancel_key); + + assert!(!composer.history_search_active()); + assert_eq!(composer.textarea.text(), "draft"); + assert_eq!(composer.textarea.cursor(), 2); + } + } + + #[test] + fn history_search_flushes_pending_first_char_before_snapshot() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('h'), KeyModifiers::NONE)); + assert!(composer.is_in_paste_burst()); + assert_eq!(composer.textarea.text(), ""); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::CONTROL)); + + assert!(composer.history_search_active()); + assert!(!composer.is_in_paste_burst()); + assert_eq!(composer.textarea.text(), "h"); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE)); + + assert!(!composer.history_search_active()); + assert_eq!(composer.textarea.text(), "h"); + } + + #[test] + fn history_search_flushes_buffered_paste_before_snapshot() { + use std::time::Duration; + use std::time::Instant; + + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + + let mut now = Instant::now(); + for ch in ['p', 'a', 's', 't', 'e'] { + let _ = composer.handle_input_basic_with_time( + KeyEvent::new(KeyCode::Char(ch), KeyModifiers::NONE), + now, + ); + now += Duration::from_millis(1); + } + assert!(composer.is_in_paste_burst()); + assert_eq!(composer.textarea.text(), ""); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::CONTROL)); + + assert!(composer.history_search_active()); + assert!(!composer.is_in_paste_burst()); + assert_eq!(composer.textarea.text(), "paste"); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE)); + + assert!(!composer.history_search_active()); + assert_eq!(composer.textarea.text(), "paste"); + } + + #[test] + fn history_search_esc_resets_normal_history_navigation() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer + .history + .record_local_submission(HistoryEntry::new("oldest matching entry".to_string())); + composer + .history + .record_local_submission(HistoryEntry::new("newest entry".to_string())); + composer.set_text_content(String::new(), Vec::new(), Vec::new()); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::CONTROL)); + for ch in ['m', 'a', 't', 'c', 'h'] { + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char(ch), KeyModifiers::NONE)); + } + assert_eq!(composer.textarea.text(), "oldest matching entry"); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE)); + assert!(!composer.history_search_active()); + assert!(composer.textarea.is_empty()); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE)); + assert_eq!(composer.textarea.text(), "newest entry"); + } + + #[test] + fn history_search_no_match_restores_preview_but_keeps_search_open() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer + .history + .record_local_submission(HistoryEntry::new("git status".to_string())); + composer.set_text_content("draft".to_string(), Vec::new(), Vec::new()); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::CONTROL)); + for ch in ['z', 'z', 'z'] { + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char(ch), KeyModifiers::NONE)); + } + + assert!(composer.history_search_active()); + assert_eq!(composer.textarea.text(), "draft"); + assert_eq!(composer.footer_mode(), FooterMode::HistorySearch); + } +} diff --git a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs index cc212b5dbb..22f4a16a64 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs @@ -1,4 +1,18 @@ +//! The chat composer history module owns shell-style recall and incremental search traversal. +//! +//! It combines persistent cross-session entries with local in-session entries into one offset +//! space. Persistent entries are fetched lazily and re-enter this state machine through +//! [`ChatComposerHistory::on_entry_response`], while local entries are already available with full +//! draft metadata. +//! +//! Ctrl+R search is modeled separately from normal Up/Down navigation because it has different +//! guarantees: query edits restart from the newest match, repeated Older/Newer keys move through +//! unique matching text, pending persistent fetches continue the same scan after the response +//! arrives, and boundary hits must not advance hidden cursor state. Search deduplication is scoped +//! to a single active search session and uses exact prompt text; it does not mutate stored history +//! or change normal history browsing. use std::collections::HashMap; +use std::collections::HashSet; use std::path::PathBuf; use crate::app_event::AppEvent; @@ -26,6 +40,12 @@ pub(crate) struct HistoryEntry { } impl HistoryEntry { + /// Creates a text-only history entry and decodes persisted mention bindings. + /// + /// Persistent history does not store attachment payloads or text-element metadata, so this + /// constructor intentionally leaves those fields empty. Local in-session submissions should be + /// recorded with the full `HistoryEntry` value built by the composer; using `new` for a local + /// image or paste submission would make recall lose placeholder ownership. pub(crate) fn new(text: String) -> Self { let decoded = decode_history_mentions(&text); Self { @@ -107,9 +127,93 @@ pub(crate) struct ChatComposerHistory { /// treated as navigation versus normal cursor movement, together with the /// "cursor at line boundary" check in [`Self::should_handle_navigation`]. last_history_text: Option, + + /// Active incremental history search, if Ctrl+R search mode is open. + search: Option, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum HistorySearchDirection { + /// Traverse toward older history offsets. + Older, + /// Traverse toward newer history offsets. + Newer, +} + +/// Result of a single incremental history search step. +/// +/// `Pending` means a persistent entry lookup has been requested and the caller should keep the +/// visible search session open until [`ChatComposerHistory::on_entry_response`] supplies the next +/// result. `AtBoundary` means the current selected match is still valid but the requested direction +/// has no further unique match; callers should avoid treating it like a query miss. +#[derive(Clone, Debug, PartialEq)] +pub(crate) enum HistorySearchResult { + Found(HistoryEntry), + Pending, + AtBoundary, + NotFound, +} + +/// Result of integrating an asynchronous persistent history response. +/// +/// A response can satisfy normal Up/Down navigation, resume a pending Ctrl+R search scan, or be +/// ignored if it belongs to a stale log or an offset the composer no longer needs. +#[derive(Clone, Debug, PartialEq)] +pub(crate) enum HistoryEntryResponse { + Found(HistoryEntry), + Search(HistorySearchResult), + Ignored, +} + +/// State for one active Ctrl+R search query. +/// +/// The state keeps two cursors: `selected_offset` is the raw combined-history offset used to +/// continue scanning, while `selected_match_index` points into `unique_matches` so already +/// discovered unique results can be revisited without rescanning duplicate offsets. `seen_texts` +/// intentionally keys on exact prompt text because the UI previews and accepts text, not the +/// storage identity of each historical record. +#[derive(Clone, Debug)] +struct HistorySearchState { + query: String, + query_lower: String, + selected_offset: Option, + unique_matches: Vec, + selected_match_index: Option, + seen_texts: HashSet, + awaiting: Option, + exhausted_older: bool, + exhausted_newer: bool, +} + +/// A unique search match cached with enough draft state to be selected again. +/// +/// The vector of these matches is kept in newest-to-oldest offset order. Storing the entry beside +/// the offset avoids depending on later cache lookups when the user moves Newer/Older among matches +/// that have already been discovered. +#[derive(Clone, Debug)] +struct UniqueHistoryMatch { + offset: usize, + entry: HistoryEntry, +} + +/// Persistent-history lookup currently blocking an incremental search scan. +/// +/// The pending request records the direction and boundary behavior that were active when the fetch +/// was issued so the response can either return a unique match or continue scanning as if no async +/// gap had occurred. +#[derive(Clone, Copy, Debug)] +struct PendingHistorySearch { + offset: usize, + direction: HistorySearchDirection, + boundary_if_exhausted: bool, } impl ChatComposerHistory { + /// Creates an empty history state machine with no persistent metadata. + /// + /// The caller must provide session metadata before cross-session history can be fetched, but + /// local in-session entries can still be recorded and traversed. Keeping construction cheap and + /// metadata-free lets the composer reset and reuse this helper across session lifecycles. pub fn new() -> Self { Self { history_log_id: None, @@ -118,10 +222,15 @@ impl ChatComposerHistory { fetched_history: HashMap::new(), history_cursor: None, last_history_text: None, + search: None, } } - /// Update metadata when a new session is configured. + /// Updates persistent history metadata when a new session is configured. + /// + /// This clears fetched entries, local entries, navigation cursors, and active search state + /// because offsets only make sense within one history log snapshot. Reusing old offsets after a + /// log-id change would allow a stale async response to hydrate the wrong prompt. pub fn set_metadata(&mut self, log_id: u64, entry_count: usize) { self.history_log_id = Some(log_id); self.history_entry_count = entry_count; @@ -129,10 +238,13 @@ impl ChatComposerHistory { self.local_history.clear(); self.history_cursor = None; self.last_history_text = None; + self.search = None; } - /// Record a message submitted by the user in the current session so it can - /// be recalled later. + /// Records a current-session submission so it can be recalled with full draft metadata. + /// + /// Empty submissions are ignored, adjacent duplicates are collapsed, and active navigation or + /// search state is reset because a new newest entry changes the combined history offset space. pub fn record_local_submission(&mut self, entry: HistoryEntry) { if entry.text.is_empty() && entry.text_elements.is_empty() @@ -145,6 +257,7 @@ impl ChatComposerHistory { } self.history_cursor = None; self.last_history_text = None; + self.search = None; // Avoid inserting a duplicate if identical to the previous entry. if self.local_history.last().is_some_and(|prev| prev == &entry) { @@ -154,10 +267,24 @@ impl ChatComposerHistory { self.local_history.push(entry); } - /// Reset navigation tracking so the next Up key resumes from the latest entry. + /// Resets normal history navigation so the next Up key resumes from the newest entry. + /// + /// This also clears any active incremental search, since normal browsing and Ctrl+R search + /// maintain different cursor semantics. Failing to clear search here would let an old query + /// influence later Up/Down recall. pub fn reset_navigation(&mut self) { self.history_cursor = None; self.last_history_text = None; + self.search = None; + } + + /// Clears only the active incremental search state. + /// + /// The normal Up/Down navigation cursor and cached persistent entries are left intact. Composer + /// search mode calls this when it accepts a match or returns to an empty query so the next + /// search starts with a fresh unique-result cache. + pub fn reset_search(&mut self) { + self.search = None; } /// Returns whether Up/Down should navigate history for the current textarea state. @@ -190,9 +317,13 @@ impl ChatComposerHistory { matches!(&self.last_history_text, Some(prev) if prev == text) } - /// Handle . Returns true when the key was consumed and the caller - /// should request a redraw. + /// Handles Up by moving toward older entries in the combined history space. + /// + /// Local entries can be returned immediately, while missing persistent entries emit a + /// `GetHistoryEntryRequest` and return `None` until the response arrives. Calling this while + /// Ctrl+R search is active intentionally exits search traversal. pub fn navigate_up(&mut self, app_event_tx: &AppEventSender) -> Option { + self.search = None; let total_entries = self.history_entry_count + self.local_history.len(); if total_entries == 0 { return None; @@ -208,8 +339,13 @@ impl ChatComposerHistory { self.populate_history_at_index(next_idx as usize, app_event_tx) } - /// Handle . + /// Handles Down by moving toward newer entries or clearing the composer past the newest entry. + /// + /// Returning an empty `HistoryEntry` means the user moved past the newest known entry and the + /// caller should clear the composer draft. As with Up, invoking this during Ctrl+R search clears + /// search state and resumes normal shell-style browsing. pub fn navigate_down(&mut self, app_event_tx: &AppEventSender) -> Option { + self.search = None; let total_entries = self.history_entry_count + self.local_history.len(); if total_entries == 0 { return None; @@ -235,30 +371,335 @@ impl ChatComposerHistory { } } - /// Integrate a GetHistoryEntryResponse event. + /// Integrates a persistent history entry response into navigation or active search. + /// + /// Responses with a stale log id are ignored, matching responses update the persistent cache, + /// and pending Ctrl+R searches resume their scan from the returned offset. The caller should + /// route `HistoryEntryResponse::Search` back to the composer search session rather than normal + /// history recall; otherwise an async search hit could be accepted without updating footer + /// status or match highlighting. pub fn on_entry_response( &mut self, log_id: u64, offset: usize, entry: Option, - ) -> Option { + app_event_tx: &AppEventSender, + ) -> HistoryEntryResponse { if self.history_log_id != Some(log_id) { - return None; + return HistoryEntryResponse::Ignored; + } + + let entry = entry.map(HistoryEntry::new); + if let Some(entry) = entry.clone() { + self.fetched_history.insert(offset, entry); + } + + if self + .search + .as_ref() + .and_then(|search| search.awaiting) + .is_some_and(|pending| pending.offset == offset) + { + let pending = self + .search + .as_ref() + .and_then(|search| search.awaiting) + .unwrap_or(PendingHistorySearch { + offset, + direction: HistorySearchDirection::Older, + boundary_if_exhausted: false, + }); + if let Some(entry) = entry + && self.search_matches(&entry) + && self.search_result_is_unique(&entry) + { + return HistoryEntryResponse::Search(self.search_match(offset, entry)); + } + return HistoryEntryResponse::Search(self.advance_search_after( + offset, + pending.direction, + pending.boundary_if_exhausted, + app_event_tx, + )); } - let entry = HistoryEntry::new(entry?); - self.fetched_history.insert(offset, entry.clone()); if self.history_cursor == Some(offset as isize) { + let Some(entry) = entry else { + return HistoryEntryResponse::Ignored; + }; self.last_history_text = Some(entry.text.clone()); - return Some(entry); + return HistoryEntryResponse::Found(entry); + } + + HistoryEntryResponse::Ignored + } + + /// Advance the active Ctrl+R search and return the next visible search state. + /// + /// Callers pass `restart` after opening search or editing the query; that clears the unique + /// match cache and starts from the end of combined history. Repeated calls with the same query + /// and `restart == false` move relative to the current unique match, preserving the selected + /// entry at boundaries. Calling this while a previous persistent lookup is still pending will + /// keep returning `Pending`; otherwise a stale response could race with a newer user action and + /// replace the composer with an unexpected entry. + pub fn search( + &mut self, + query: &str, + direction: HistorySearchDirection, + restart: bool, + app_event_tx: &AppEventSender, + ) -> HistorySearchResult { + let total_entries = self.total_entries(); + if total_entries == 0 { + self.search = Some(HistorySearchState::new(query)); + return HistorySearchResult::NotFound; + } + + let query_changed = self + .search + .as_ref() + .is_none_or(|search| search.query != query); + if !query_changed + && !restart + && self + .search + .as_ref() + .and_then(|search| search.awaiting) + .is_some() + { + return HistorySearchResult::Pending; + } + + if query_changed || restart || self.search.is_none() { + self.search = Some(HistorySearchState::new(query)); + } else if let Some(search) = self.search.as_mut() { + search.awaiting = None; + } + + let boundary_if_exhausted = !restart + && self + .search + .as_ref() + .and_then(|search| search.selected_offset) + .is_some(); + if !restart + && !query_changed + && let Some(result) = self.select_cached_unique_match(direction) + { + return result; + } + if boundary_if_exhausted + && self + .search + .as_ref() + .is_some_and(|search| search.is_exhausted(direction)) + { + return HistorySearchResult::AtBoundary; + } + + let start_offset = + self.search_start_offset(total_entries, direction, query_changed || restart); + let Some(start_offset) = start_offset else { + return self.exhausted_search_result(direction, boundary_if_exhausted); + }; + + let result = + self.advance_search_from(start_offset, direction, boundary_if_exhausted, app_event_tx); + if matches!(result, HistorySearchResult::NotFound) { + self.exhausted_search_result(direction, boundary_if_exhausted) + } else { + result } - None } // --------------------------------------------------------------------- // Internal helpers // --------------------------------------------------------------------- + fn total_entries(&self) -> usize { + self.history_entry_count + self.local_history.len() + } + + fn search_start_offset( + &self, + total_entries: usize, + direction: HistorySearchDirection, + restart: bool, + ) -> Option { + let selected = self + .search + .as_ref() + .and_then(|search| search.selected_offset); + match direction { + HistorySearchDirection::Older => { + if restart { + total_entries.checked_sub(1) + } else { + selected.and_then(|offset| offset.checked_sub(1)) + } + } + HistorySearchDirection::Newer => { + if restart { + Some(0) + } else { + selected + .and_then(|offset| offset.checked_add(1)) + .filter(|offset| *offset < total_entries) + } + } + } + } + + fn advance_search_after( + &mut self, + offset: usize, + direction: HistorySearchDirection, + boundary_if_exhausted: bool, + app_event_tx: &AppEventSender, + ) -> HistorySearchResult { + let next_offset = match direction { + HistorySearchDirection::Older => offset.checked_sub(1), + HistorySearchDirection::Newer => offset + .checked_add(1) + .filter(|next| *next < self.total_entries()), + }; + let Some(next_offset) = next_offset else { + return self.exhausted_search_result(direction, boundary_if_exhausted); + }; + let result = + self.advance_search_from(next_offset, direction, boundary_if_exhausted, app_event_tx); + if matches!(result, HistorySearchResult::NotFound) { + self.exhausted_search_result(direction, boundary_if_exhausted) + } else { + result + } + } + + fn advance_search_from( + &mut self, + mut offset: usize, + direction: HistorySearchDirection, + boundary_if_exhausted: bool, + app_event_tx: &AppEventSender, + ) -> HistorySearchResult { + let total_entries = self.total_entries(); + while offset < total_entries { + if let Some(entry) = self.entry_at_cached_offset(offset) { + if self.search_matches(&entry) && self.search_result_is_unique(&entry) { + return self.search_match(offset, entry); + } + } else if offset < self.history_entry_count + && let Some(log_id) = self.history_log_id + { + if let Some(search) = self.search.as_mut() { + search.awaiting = Some(PendingHistorySearch { + offset, + direction, + boundary_if_exhausted, + }); + } + app_event_tx.send(AppEvent::CodexOp(Op::GetHistoryEntryRequest { + offset, + log_id, + })); + return HistorySearchResult::Pending; + } + + let next_offset = match direction { + HistorySearchDirection::Older => offset.checked_sub(1), + HistorySearchDirection::Newer => { + offset.checked_add(1).filter(|next| *next < total_entries) + } + }; + let Some(next_offset) = next_offset else { + return HistorySearchResult::NotFound; + }; + offset = next_offset; + } + + HistorySearchResult::NotFound + } + + fn entry_at_cached_offset(&self, offset: usize) -> Option { + if offset >= self.history_entry_count { + self.local_history + .get(offset - self.history_entry_count) + .cloned() + } else { + self.fetched_history.get(&offset).cloned() + } + } + + fn search_matches(&self, entry: &HistoryEntry) -> bool { + let Some(search) = self.search.as_ref() else { + return false; + }; + search.query.is_empty() || entry.text.to_lowercase().contains(&search.query_lower) + } + + fn search_result_is_unique(&self, entry: &HistoryEntry) -> bool { + self.search + .as_ref() + .is_none_or(|search| !search.seen_texts.contains(entry.text.as_str())) + } + + fn search_match(&mut self, offset: usize, entry: HistoryEntry) -> HistorySearchResult { + self.history_cursor = Some(offset as isize); + self.last_history_text = Some(entry.text.clone()); + if let Some(search) = self.search.as_mut() { + search.selected_offset = Some(offset); + search.record_match(offset, &entry); + search.awaiting = None; + search.exhausted_older = false; + search.exhausted_newer = false; + } + HistorySearchResult::Found(entry) + } + + fn select_cached_unique_match( + &mut self, + direction: HistorySearchDirection, + ) -> Option { + let next_index = { + let search = self.search.as_ref()?; + let selected_index = search.selected_match_index?; + match direction { + HistorySearchDirection::Older => { + let next_index = selected_index + 1; + (next_index < search.unique_matches.len()).then_some(next_index)? + } + HistorySearchDirection::Newer => selected_index.checked_sub(1)?, + } + }; + + let history_match = self.search.as_ref()?.unique_matches[next_index].clone(); + self.history_cursor = Some(history_match.offset as isize); + self.last_history_text = Some(history_match.entry.text.clone()); + if let Some(search) = self.search.as_mut() { + search.select_match(next_index); + } + Some(HistorySearchResult::Found(history_match.entry)) + } + + fn exhausted_search_result( + &mut self, + direction: HistorySearchDirection, + boundary_if_exhausted: bool, + ) -> HistorySearchResult { + if let Some(search) = self.search.as_mut() { + search.awaiting = None; + if boundary_if_exhausted { + search.mark_exhausted(direction); + } + } + + if boundary_if_exhausted { + HistorySearchResult::AtBoundary + } else { + HistorySearchResult::NotFound + } + } + fn populate_history_at_index( &mut self, global_idx: usize, @@ -287,6 +728,71 @@ impl ChatComposerHistory { } } +impl HistorySearchState { + fn new(query: &str) -> Self { + Self { + query: query.to_string(), + query_lower: query.to_lowercase(), + selected_offset: None, + unique_matches: Vec::new(), + selected_match_index: None, + seen_texts: HashSet::new(), + awaiting: None, + exhausted_older: false, + exhausted_newer: false, + } + } + + fn is_exhausted(&self, direction: HistorySearchDirection) -> bool { + match direction { + HistorySearchDirection::Older => self.exhausted_older, + HistorySearchDirection::Newer => self.exhausted_newer, + } + } + + fn mark_exhausted(&mut self, direction: HistorySearchDirection) { + match direction { + HistorySearchDirection::Older => self.exhausted_older = true, + HistorySearchDirection::Newer => self.exhausted_newer = true, + } + } + + fn record_match(&mut self, offset: usize, entry: &HistoryEntry) { + if let Some(index) = self + .unique_matches + .iter() + .position(|history_match| history_match.offset == offset) + { + self.select_match(index); + return; + } + + self.seen_texts.insert(entry.text.clone()); + let insert_index = self + .unique_matches + .partition_point(|history_match| history_match.offset > offset); + self.unique_matches.insert( + insert_index, + UniqueHistoryMatch { + offset, + entry: entry.clone(), + }, + ); + self.select_match(insert_index); + } + + fn select_match(&mut self, index: usize) { + let Some(history_match) = self.unique_matches.get(index) else { + return; + }; + self.selected_offset = Some(history_match.offset); + self.selected_match_index = Some(index); + self.awaiting = None; + self.exhausted_older = false; + self.exhausted_newer = false; + } +} + #[cfg(test)] mod tests { use super::*; @@ -351,8 +857,13 @@ mod tests { // Inject the async response. assert_eq!( - Some(HistoryEntry::new("latest".to_string())), - history.on_entry_response(/*log_id*/ 1, /*offset*/ 2, Some("latest".into())) + HistoryEntryResponse::Found(HistoryEntry::new("latest".to_string())), + history.on_entry_response( + /*log_id*/ 1, + /*offset*/ 2, + Some("latest".into()), + &tx + ) ); // Next Up should move to offset 1. @@ -372,8 +883,390 @@ mod tests { ); assert_eq!( - Some(HistoryEntry::new("older".to_string())), - history.on_entry_response(/*log_id*/ 1, /*offset*/ 1, Some("older".into())) + HistoryEntryResponse::Found(HistoryEntry::new("older".to_string())), + history.on_entry_response( + /*log_id*/ 1, + /*offset*/ 1, + Some("older".into()), + &tx + ) + ); + } + + #[test] + fn search_matches_local_history_and_stops_at_boundaries() { + let (tx, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx); + + let mut history = ChatComposerHistory::new(); + history.record_local_submission(HistoryEntry::new("git status".to_string())); + history.record_local_submission(HistoryEntry::new("cargo test -p codex-tui".to_string())); + history.record_local_submission(HistoryEntry::new("git diff".to_string())); + + assert_eq!( + HistorySearchResult::Found(HistoryEntry::new("git diff".to_string())), + history.search( + "git", + HistorySearchDirection::Older, + /*restart*/ true, + &tx + ) + ); + assert_eq!( + HistorySearchResult::Found(HistoryEntry::new("git status".to_string())), + history.search( + "git", + HistorySearchDirection::Older, + /*restart*/ false, + &tx + ) + ); + assert_eq!( + HistorySearchResult::AtBoundary, + history.search( + "git", + HistorySearchDirection::Older, + /*restart*/ false, + &tx + ) + ); + assert_eq!( + HistorySearchResult::AtBoundary, + history.search( + "git", + HistorySearchDirection::Older, + /*restart*/ false, + &tx + ) + ); + assert_eq!( + HistorySearchResult::Found(HistoryEntry::new("git diff".to_string())), + history.search( + "git", + HistorySearchDirection::Newer, + /*restart*/ false, + &tx + ) + ); + assert_eq!( + HistorySearchResult::AtBoundary, + history.search( + "git", + HistorySearchDirection::Newer, + /*restart*/ false, + &tx + ) + ); + } + + #[test] + fn search_skips_duplicate_local_matches() { + let (tx, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx); + + let mut history = ChatComposerHistory::new(); + history.record_local_submission(HistoryEntry::new("git status".to_string())); + history.record_local_submission(HistoryEntry::new("cargo test -p codex-tui".to_string())); + history.record_local_submission(HistoryEntry::new("git status".to_string())); + history.record_local_submission(HistoryEntry::new("git diff".to_string())); + + assert_eq!( + HistorySearchResult::Found(HistoryEntry::new("git diff".to_string())), + history.search( + "git", + HistorySearchDirection::Older, + /*restart*/ true, + &tx + ) + ); + assert_eq!( + HistorySearchResult::Found(HistoryEntry::new("git status".to_string())), + history.search( + "git", + HistorySearchDirection::Older, + /*restart*/ false, + &tx + ) + ); + assert_eq!( + HistorySearchResult::AtBoundary, + history.search( + "git", + HistorySearchDirection::Older, + /*restart*/ false, + &tx + ) + ); + assert_eq!( + HistorySearchResult::Found(HistoryEntry::new("git diff".to_string())), + history.search( + "git", + HistorySearchDirection::Newer, + /*restart*/ false, + &tx + ) + ); + assert_eq!( + HistorySearchResult::Found(HistoryEntry::new("git status".to_string())), + history.search( + "git", + HistorySearchDirection::Older, + /*restart*/ false, + &tx + ) + ); + } + + #[test] + fn repeated_boundary_search_does_not_refetch_persistent_history() { + let (tx, mut rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx); + + let mut history = ChatComposerHistory::new(); + history.set_metadata(/*log_id*/ 1, /*entry_count*/ 3); + + assert_eq!( + HistorySearchResult::Pending, + history.search( + "needle", + HistorySearchDirection::Older, + /*restart*/ true, + &tx + ) + ); + let _ = rx.try_recv().expect("expected latest lookup"); + assert_eq!( + HistoryEntryResponse::Search(HistorySearchResult::Found(HistoryEntry::new( + "needle latest".to_string() + ))), + history.on_entry_response( + /*log_id*/ 1, + /*offset*/ 2, + Some("needle latest".into()), + &tx, + ) + ); + + assert_eq!( + HistorySearchResult::Pending, + history.search( + "needle", + HistorySearchDirection::Older, + /*restart*/ false, + &tx + ) + ); + let _ = rx.try_recv().expect("expected next older lookup"); + assert_eq!( + HistoryEntryResponse::Search(HistorySearchResult::Pending), + history.on_entry_response( + /*log_id*/ 1, + /*offset*/ 1, + Some("not a match".into()), + &tx, + ) + ); + let _ = rx.try_recv().expect("expected oldest lookup"); + assert_eq!( + HistoryEntryResponse::Search(HistorySearchResult::AtBoundary), + history.on_entry_response( + /*log_id*/ 1, + /*offset*/ 0, + Some("also not a match".into()), + &tx, + ) + ); + assert!(rx.try_recv().is_err()); + + assert_eq!( + HistorySearchResult::AtBoundary, + history.search( + "needle", + HistorySearchDirection::Older, + /*restart*/ false, + &tx + ) + ); + assert!(rx.try_recv().is_err()); + } + + #[test] + fn search_fetches_persistent_history_until_match() { + let (tx, mut rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx); + + let mut history = ChatComposerHistory::new(); + history.set_metadata(/*log_id*/ 1, /*entry_count*/ 3); + + assert_eq!( + HistorySearchResult::Pending, + history.search( + "older", + HistorySearchDirection::Older, + /*restart*/ true, + &tx + ) + ); + let AppEvent::CodexOp(op) = rx.try_recv().expect("expected latest lookup") else { + panic!("unexpected event variant"); + }; + assert_eq!( + Op::GetHistoryEntryRequest { + log_id: 1, + offset: 2, + }, + op + ); + + assert_eq!( + HistoryEntryResponse::Search(HistorySearchResult::Pending), + history.on_entry_response( + /*log_id*/ 1, + /*offset*/ 2, + Some("latest".into()), + &tx + ) + ); + let AppEvent::CodexOp(op) = rx.try_recv().expect("expected next lookup") else { + panic!("unexpected event variant"); + }; + assert_eq!( + Op::GetHistoryEntryRequest { + log_id: 1, + offset: 1, + }, + op + ); + + assert_eq!( + HistoryEntryResponse::Search(HistorySearchResult::Found(HistoryEntry::new( + "older command".to_string() + ))), + history.on_entry_response( + /*log_id*/ 1, + /*offset*/ 1, + Some("older command".into()), + &tx + ) + ); + } + + #[test] + fn search_skips_duplicate_persistent_matches() { + let (tx, mut rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx); + + let mut history = ChatComposerHistory::new(); + history.set_metadata(/*log_id*/ 1, /*entry_count*/ 4); + + assert_eq!( + HistorySearchResult::Pending, + history.search( + "needle", + HistorySearchDirection::Older, + /*restart*/ true, + &tx + ) + ); + let _ = rx.try_recv().expect("expected latest lookup"); + assert_eq!( + HistoryEntryResponse::Search(HistorySearchResult::Found(HistoryEntry::new( + "needle same".to_string() + ))), + history.on_entry_response( + /*log_id*/ 1, + /*offset*/ 3, + Some("needle same".into()), + &tx, + ) + ); + + assert_eq!( + HistorySearchResult::Pending, + history.search( + "needle", + HistorySearchDirection::Older, + /*restart*/ false, + &tx + ) + ); + let _ = rx.try_recv().expect("expected duplicate lookup"); + assert_eq!( + HistoryEntryResponse::Search(HistorySearchResult::Pending), + history.on_entry_response( + /*log_id*/ 1, + /*offset*/ 2, + Some("needle same".into()), + &tx, + ) + ); + let _ = rx.try_recv().expect("expected next lookup after duplicate"); + assert_eq!( + HistoryEntryResponse::Search(HistorySearchResult::Pending), + history.on_entry_response( + /*log_id*/ 1, + /*offset*/ 1, + Some("not a match".into()), + &tx, + ) + ); + let _ = rx.try_recv().expect("expected oldest lookup"); + assert_eq!( + HistoryEntryResponse::Search(HistorySearchResult::Found(HistoryEntry::new( + "needle older".to_string() + ))), + history.on_entry_response( + /*log_id*/ 1, + /*offset*/ 0, + Some("needle older".into()), + &tx, + ) + ); + assert_eq!( + HistorySearchResult::AtBoundary, + history.search( + "needle", + HistorySearchDirection::Older, + /*restart*/ false, + &tx + ) + ); + assert_eq!( + HistorySearchResult::Found(HistoryEntry::new("needle same".to_string())), + history.search( + "needle", + HistorySearchDirection::Newer, + /*restart*/ false, + &tx + ) + ); + } + + #[test] + fn search_is_case_insensitive_and_empty_query_finds_latest() { + let (tx, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx); + + let mut history = ChatComposerHistory::new(); + history.record_local_submission(HistoryEntry::new("Build Release".to_string())); + + assert_eq!( + HistorySearchResult::Found(HistoryEntry::new("Build Release".to_string())), + history.search( + "release", + HistorySearchDirection::Older, + /*restart*/ true, + &tx + ) + ); + assert_eq!( + HistorySearchResult::Found(HistoryEntry::new("Build Release".to_string())), + history.search( + "", + HistorySearchDirection::Older, + /*restart*/ true, + &tx + ) ); } diff --git a/codex-rs/tui/src/bottom_pane/footer.rs b/codex-rs/tui/src/bottom_pane/footer.rs index 194f662b32..958f5c2ead 100644 --- a/codex-rs/tui/src/bottom_pane/footer.rs +++ b/codex-rs/tui/src/bottom_pane/footer.rs @@ -130,6 +130,8 @@ impl CollaborationModeIndicator { /// (for example, showing `QuitShortcutReminder` only while its timer is active). #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub(crate) enum FooterMode { + /// Single-line incremental history search prompt shown while Ctrl+R search is active. + HistorySearch, /// Transient "press again to quit" reminder (Ctrl+C/Ctrl+D). QuitShortcutReminder, /// Multi-line shortcut overlay shown after pressing `?`. @@ -179,6 +181,7 @@ pub(crate) fn reset_mode_after_activity(current: FooterMode) -> FooterMode { FooterMode::EscHint | FooterMode::ShortcutOverlay | FooterMode::QuitShortcutReminder + | FooterMode::HistorySearch | FooterMode::ComposerHasDraft => FooterMode::ComposerEmpty, other => other, } @@ -188,13 +191,15 @@ pub(crate) fn footer_height(props: &FooterProps) -> u16 { let show_shortcuts_hint = match props.mode { FooterMode::ComposerEmpty => true, FooterMode::ComposerHasDraft => false, - FooterMode::QuitShortcutReminder | FooterMode::ShortcutOverlay | FooterMode::EscHint => { - false - } + FooterMode::HistorySearch + | FooterMode::QuitShortcutReminder + | FooterMode::ShortcutOverlay + | FooterMode::EscHint => false, }; let show_queue_hint = match props.mode { FooterMode::ComposerHasDraft => props.is_task_running, FooterMode::QuitShortcutReminder + | FooterMode::HistorySearch | FooterMode::ComposerEmpty | FooterMode::ShortcutOverlay | FooterMode::EscHint => false, @@ -593,6 +598,7 @@ fn footer_from_props_lines( FooterMode::QuitShortcutReminder => { vec![quit_shortcut_reminder_line(props.quit_shortcut_key)] } + FooterMode::HistorySearch => vec![Line::from("reverse-i-search: ").dim()], FooterMode::ComposerEmpty => { let state = LeftSideState { hint: if show_shortcuts_hint { @@ -666,9 +672,10 @@ pub(crate) fn shows_passive_footer_line(props: &FooterProps) -> bool { match props.mode { FooterMode::ComposerEmpty => true, FooterMode::ComposerHasDraft => !props.is_task_running, - FooterMode::QuitShortcutReminder | FooterMode::ShortcutOverlay | FooterMode::EscHint => { - false - } + FooterMode::HistorySearch + | FooterMode::QuitShortcutReminder + | FooterMode::ShortcutOverlay + | FooterMode::EscHint => false, } } @@ -756,6 +763,7 @@ fn shortcut_overlay_lines(state: ShortcutsState) -> Vec> { let mut paste_image = Line::from(""); let mut external_editor = Line::from(""); let mut edit_previous = Line::from(""); + let mut history_search = Line::from(""); let mut quit = Line::from(""); let mut show_transcript = Line::from(""); let mut change_mode = Line::from(""); @@ -771,6 +779,7 @@ fn shortcut_overlay_lines(state: ShortcutsState) -> Vec> { ShortcutId::PasteImage => paste_image = text, ShortcutId::ExternalEditor => external_editor = text, ShortcutId::EditPrevious => edit_previous = text, + ShortcutId::HistorySearch => history_search = text, ShortcutId::Quit => quit = text, ShortcutId::ShowTranscript => show_transcript = text, ShortcutId::ChangeMode => change_mode = text, @@ -787,6 +796,7 @@ fn shortcut_overlay_lines(state: ShortcutsState) -> Vec> { paste_image, external_editor, edit_previous, + history_search, quit, ]; if change_mode.width() > 0 { @@ -869,6 +879,7 @@ enum ShortcutId { PasteImage, ExternalEditor, EditPrevious, + HistorySearch, Quit, ShowTranscript, ChangeMode, @@ -1027,6 +1038,15 @@ const SHORTCUTS: &[ShortcutDescriptor] = &[ prefix: "", label: "", }, + ShortcutDescriptor { + id: ShortcutId::HistorySearch, + bindings: &[ShortcutBinding { + key: key_hint::ctrl(KeyCode::Char('r')), + condition: DisplayCondition::Always, + }], + prefix: "", + label: " search history", + }, ShortcutDescriptor { id: ShortcutId::Quit, bindings: &[ShortcutBinding { @@ -1086,13 +1106,15 @@ mod tests { let show_shortcuts_hint = match props.mode { FooterMode::ComposerEmpty => true, FooterMode::ComposerHasDraft => false, - FooterMode::QuitShortcutReminder + FooterMode::HistorySearch + | FooterMode::QuitShortcutReminder | FooterMode::ShortcutOverlay | FooterMode::EscHint => false, }; let show_queue_hint = match props.mode { FooterMode::ComposerHasDraft => props.is_task_running, - FooterMode::QuitShortcutReminder + FooterMode::HistorySearch + | FooterMode::QuitShortcutReminder | FooterMode::ComposerEmpty | FooterMode::ShortcutOverlay | FooterMode::EscHint => false, @@ -1227,6 +1249,7 @@ mod tests { && !matches!( props.mode, FooterMode::EscHint + | FooterMode::HistorySearch | FooterMode::QuitShortcutReminder | FooterMode::ShortcutOverlay ); diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index bd3bd946af..0273b8e1af 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -7,9 +7,9 @@ //! Input routing is layered: `BottomPane` decides which local surface receives a key (view vs //! composer), while higher-level intent such as "interrupt" or "quit" is decided by the parent //! widget (`ChatWidget`). This split matters for Ctrl+C/Ctrl+D: the bottom pane gives the active -//! view the first chance to consume Ctrl+C (typically to dismiss itself), and `ChatWidget` may -//! treat an unhandled Ctrl+C as an interrupt or as the first press of a double-press quit -//! shortcut. +//! view the first chance to consume Ctrl+C (typically to dismiss itself), then lets an active +//! composer history search consume Ctrl+C as cancellation, and `ChatWidget` may treat an unhandled +//! Ctrl+C as an interrupt or as the first press of a double-press quit shortcut. //! //! Some UI is time-based rather than input-based, such as the transient "press again to quit" //! hint. The pane schedules redraws so those hints can expire even when the UI is otherwise idle. @@ -458,7 +458,8 @@ impl BottomPane { /// Handles a Ctrl+C press within the bottom pane. /// /// An active modal view is given the first chance to consume the key (typically to dismiss - /// itself). If no view is active, Ctrl+C clears draft composer input. + /// itself). If no view is active, Ctrl+C cancels active history search before falling back to + /// clearing draft composer input. /// /// This method may show the quit shortcut hint as a user-visible acknowledgement that Ctrl+C /// was received, but it does not decide whether the process should exit; `ChatWidget` owns the @@ -475,6 +476,9 @@ impl BottomPane { self.request_redraw(); } event + } else if self.composer.cancel_history_search() { + self.request_redraw(); + CancellationEvent::Handled } else if self.composer_is_empty() { CancellationEvent::NotHandled } else { @@ -1305,6 +1309,31 @@ mod tests { assert_eq!(CancellationEvent::NotHandled, pane.on_ctrl_c()); } + #[test] + fn ctrl_c_cancels_history_search_without_clearing_draft_or_showing_quit_hint() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let mut pane = BottomPane::new(BottomPaneParams { + app_event_tx: tx, + frame_requester: FrameRequester::test_dummy(), + has_input_focus: true, + enhanced_keys_supported: false, + placeholder_text: "Ask Codex to do anything".to_string(), + disable_paste_burst: true, + animations_enabled: true, + skills: Some(Vec::new()), + }); + pane.insert_str("draft"); + + pane.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::CONTROL)); + assert!(pane.composer.popup_active()); + + assert_eq!(CancellationEvent::Handled, pane.on_ctrl_c()); + assert_eq!(pane.composer_text(), "draft"); + assert!(!pane.composer.popup_active()); + assert!(!pane.quit_shortcut_hint_visible()); + } + // live ring removed; related tests deleted. #[test] diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_history_search.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_history_search.snap new file mode 100644 index 0000000000..90cfba24a9 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_history_search.snap @@ -0,0 +1,13 @@ +--- +source: tui/src/bottom_pane/chat_composer.rs +expression: terminal.backend() +--- +" " +"› cargo test " +" " +" " +" " +" " +" " +" " +" reverse-i-search: c enter accept · esc cancel " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_shortcut_overlay.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_shortcut_overlay.snap index 8486a9ec6f..8db427fe3b 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_shortcut_overlay.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_shortcut_overlay.snap @@ -14,5 +14,5 @@ expression: terminal.backend() " shift + enter for newline tab to queue message " " @ for file paths ctrl + v to paste images " " ctrl + g to edit in external editor esc again to edit previous message " -" ctrl + c to exit " -" ctrl + t to view transcript " +" ctrl + r search history ctrl + c to exit " +" ctrl + t to view transcript " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__footer__tests__footer_shortcuts_collaboration_modes_enabled.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__footer__tests__footer_shortcuts_collaboration_modes_enabled.snap index 1bb213bbec..b2b7f1349e 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__footer__tests__footer_shortcuts_collaboration_modes_enabled.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__footer__tests__footer_shortcuts_collaboration_modes_enabled.snap @@ -6,5 +6,6 @@ expression: terminal.backend() " ctrl + j for newline tab to queue message " " @ for file paths ctrl + v to paste images " " ctrl + g to edit in external editor esc esc to edit previous message " -" ctrl + c to exit shift + tab to change mode " -" ctrl + t to view transcript " +" ctrl + r search history ctrl + c to exit " +" shift + tab to change mode " +" ctrl + t to view transcript " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__footer__tests__footer_shortcuts_shift_and_esc.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__footer__tests__footer_shortcuts_shift_and_esc.snap index c1f00d4437..e955715ec0 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__footer__tests__footer_shortcuts_shift_and_esc.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__footer__tests__footer_shortcuts_shift_and_esc.snap @@ -6,5 +6,5 @@ expression: terminal.backend() " shift + enter for newline tab to queue message " " @ for file paths ctrl + v to paste images " " ctrl + g to edit in external editor esc again to edit previous message " -" ctrl + c to exit " -" ctrl + t to view transcript " +" ctrl + r search history ctrl + c to exit " +" ctrl + t to view transcript " diff --git a/codex-rs/tui/src/bottom_pane/textarea.rs b/codex-rs/tui/src/bottom_pane/textarea.rs index 93fe62c585..2af4b3145f 100644 --- a/codex-rs/tui/src/bottom_pane/textarea.rs +++ b/codex-rs/tui/src/bottom_pane/textarea.rs @@ -1366,7 +1366,7 @@ impl TextArea { impl WidgetRef for &TextArea { fn render_ref(&self, area: Rect, buf: &mut Buffer) { let lines = self.wrapped_lines(area.width); - self.render_lines(area, buf, &lines, 0..lines.len(), Style::default()); + self.render_lines(area, buf, &lines, 0..lines.len(), Style::default(), &[]); } } @@ -1380,7 +1380,7 @@ impl StatefulWidgetRef for &TextArea { let start = scroll as usize; let end = (scroll + area.height).min(lines.len() as u16) as usize; - self.render_lines(area, buf, &lines, start..end, Style::default()); + self.render_lines(area, buf, &lines, start..end, Style::default(), &[]); } } @@ -1417,7 +1417,28 @@ impl TextArea { let start = scroll as usize; let end = (scroll + area.height).min(lines.len() as u16) as usize; - self.render_lines(area, buf, &lines, start..end, base_style); + self.render_lines(area, buf, &lines, start..end, base_style, &[]); + } + + /// Render the textarea with `base_style` plus additional render-only highlight ranges. + /// + /// Highlight ranges are byte ranges in `self.text`. They affect only the buffer rendering and + /// do not mutate the editable text, cursor, element metadata, or wrapping cache. + pub(crate) fn render_ref_styled_with_highlights( + &self, + area: Rect, + buf: &mut Buffer, + state: &mut TextAreaState, + base_style: Style, + highlights: &[(Range, Style)], + ) { + let lines = self.wrapped_lines(area.width); + let scroll = self.effective_scroll(area.height, &lines, state.scroll); + state.scroll = scroll; + + let start = scroll as usize; + let end = (scroll + area.height).min(lines.len() as u16) as usize; + self.render_lines(area, buf, &lines, start..end, base_style, highlights); } fn render_lines( @@ -1427,6 +1448,7 @@ impl TextArea { lines: &[Range], range: std::ops::Range, base_style: Style, + highlights: &[(Range, Style)], ) { for (row, idx) in range.enumerate() { let r = &lines[idx]; @@ -1449,6 +1471,19 @@ impl TextArea { let style = base_style.fg(ratatui::style::Color::Cyan); buf.set_string(area.x + x_off, y, styled, style); } + + // Overlay render-only highlight ranges last so transient search highlighting remains + // visible even when it intersects attachment placeholders or other styled elements. + for (highlight_range, style) in highlights { + let overlap_start = highlight_range.start.max(line_range.start); + let overlap_end = highlight_range.end.min(line_range.end); + if overlap_start >= overlap_end { + continue; + } + let highlighted = &self.text[overlap_start..overlap_end]; + let x_off = self.text[line_range.start..overlap_start].width() as u16; + buf.set_string(area.x + x_off, y, highlighted, *style); + } } } @@ -2187,6 +2222,43 @@ mod tests { assert!(state.scroll < effective_lines); } + #[test] + fn render_highlights_apply_style_without_mutating_text() { + let t = ta_with("hello world"); + let area = Rect::new(0, 0, 20, 1); + let mut state = TextAreaState::default(); + let mut buf = Buffer::empty(area); + let highlight_style = Style::default().add_modifier(ratatui::style::Modifier::REVERSED); + + t.render_ref_styled_with_highlights( + area, + &mut buf, + &mut state, + Style::default(), + &[(6..11, highlight_style)], + ); + + assert_eq!(t.text(), "hello world"); + assert!( + !buf[(0, 0)] + .style() + .add_modifier + .contains(ratatui::style::Modifier::REVERSED) + ); + assert!( + buf[(6, 0)] + .style() + .add_modifier + .contains(ratatui::style::Modifier::REVERSED) + ); + assert!( + buf[(10, 0)] + .style() + .add_modifier + .contains(ratatui::style::Modifier::REVERSED) + ); + } + #[test] fn cursor_pos_with_state_basic_and_scroll_behaviors() { // Case 1: No wrapping needed, height fits — scroll ignored, y maps directly. diff --git a/docs/tui-chat-composer.md b/docs/tui-chat-composer.md index a61a0b5beb..0ad5c693b3 100644 --- a/docs/tui-chat-composer.md +++ b/docs/tui-chat-composer.md @@ -65,6 +65,14 @@ Up/Down recall is handled by `ChatComposerHistory` and merges two sources: This distinction keeps the on-disk history backward compatible and avoids persisting attachments, while still providing a richer recall experience for in-session edits. +### Reverse history search (Ctrl+R) + +Ctrl+R enters an incremental reverse search mode without immediately previewing the latest history entry. While search is active, the footer line becomes the editable query field and the composer body is only a preview of the currently matched entry. `Enter` accepts the preview as a normal editable draft, and `Esc` or Ctrl+C restores the exact draft that existed before search started. + +The composer owns the search session because it controls draft snapshots, footer rendering, cursor placement, and preview highlighting. `ChatComposerHistory` owns traversal: it scans persistent and local entries in one offset space, skips duplicate prompt text within a search session, keeps boundary hits on the current match, and resumes scans after asynchronous persistent history responses. + +The search query and composer text intentionally remain separate. A no-match result restores the original draft while leaving the footer query open for more typing, and accepting a match clears the search session so highlight styling disappears from the now-editable composer text. + ## Config gating for reuse `ChatComposer` now supports feature gating via `ChatComposerConfig` From 470510174b3862a8e54ce13c874ba53cf9eaab6c Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Sun, 12 Apr 2026 15:42:09 -0700 Subject: [PATCH 4/6] Remove context status-line meter (#17420) Addresses #17313 Problem: The visual context meter in the status line was confusing and continued to draw negative feedback, and context reporting should remain an explicit opt-in rather than part of the default footer. Solution: Remove the visual meter, restore opt-in context remaining/used percentage items that explicitly say "Context", keep existing context-usage configs working as a hidden alias, and update the setup text and snapshots. --- codex-rs/core/src/config/mod.rs | 3 +- ..._snapshot_uses_runtime_preview_values.snap | 4 +- .../tui/src/bottom_pane/status_line_setup.rs | 76 +++++++++---------- ...line_model_with_reasoning_fast_footer.snap | 2 +- .../tui/src/chatwidget/status_surfaces.rs | 65 ++-------------- .../src/chatwidget/tests/status_and_layout.rs | 38 +++++++--- 6 files changed, 71 insertions(+), 117 deletions(-) diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index eac8f9e361..9b2b06e0a8 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -324,8 +324,7 @@ pub struct Config { /// Ordered list of status line item identifiers for the TUI. /// - /// When unset, the TUI defaults to: `model-with-reasoning`, `context-remaining`, and - /// `current-dir`. + /// When unset, the TUI defaults to: `model-with-reasoning` and `current-dir`. pub tui_status_line: Option>, /// Ordered list of terminal title item identifiers for the TUI. diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__status_line_setup__tests__setup_view_snapshot_uses_runtime_preview_values.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__status_line_setup__tests__setup_view_snapshot_uses_runtime_preview_values.snap index 94d9cd450c..20aca1e334 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__status_line_setup__tests__setup_view_snapshot_uses_runtime_preview_values.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__status_line_setup__tests__setup_view_snapshot_uses_runtime_preview_values.snap @@ -13,9 +13,9 @@ expression: "render_lines(&view, 72)" [x] git-branch Current Git branch (omitted when unavaila… [ ] model-with-reasoning Current model name with reasoning level [ ] project-root Project root directory (omitted when unav… - [ ] context-usage Visual meter of context window usage (omi… + [ ] context-remaining Percentage of context window remaining (o… + [ ] context-used Percentage of context window used (omitte… [ ] five-hour-limit Remaining usage on 5-hour usage limit (om… - [ ] weekly-limit Remaining usage on weekly usage limit (om… gpt-5-codex · ~/codex-rs · jif/statusline-preview Use ↑↓ to navigate, ←→ to move, space to select, enter to confirm, esc diff --git a/codex-rs/tui/src/bottom_pane/status_line_setup.rs b/codex-rs/tui/src/bottom_pane/status_line_setup.rs index ed4f76c1d0..7265d61ad3 100644 --- a/codex-rs/tui/src/bottom_pane/status_line_setup.rs +++ b/codex-rs/tui/src/bottom_pane/status_line_setup.rs @@ -12,7 +12,7 @@ //! - Model information (name, reasoning level) //! - Directory paths (current dir, project root) //! - Git information (branch name) -//! - Context usage (meter, window size) +//! - Context usage (remaining %, used %, window size) //! - Usage limits (5-hour, weekly) //! - Session info (thread title, ID, tokens used) //! - Application version @@ -22,6 +22,7 @@ use ratatui::layout::Rect; use ratatui::text::Line; use std::collections::BTreeMap; use std::collections::HashSet; +use strum::IntoEnumIterator; use strum_macros::Display; use strum_macros::EnumIter; use strum_macros::EnumString; @@ -62,15 +63,14 @@ pub(crate) enum StatusLineItem { /// Current git branch name (if in a repository). GitBranch, - /// Visual meter of context window usage. + /// Percentage of context window remaining. + ContextRemaining, + + /// Percentage of context window used. /// - /// Also accepts legacy `context-remaining` and `context-used` config values. - #[strum( - to_string = "context-usage", - serialize = "context-remaining", - serialize = "context-used" - )] - ContextUsage, + /// Also accepts the legacy `context-usage` config value. + #[strum(to_string = "context-used", serialize = "context-usage")] + ContextUsed, /// Remaining usage on the 5-hour rate limit. FiveHourLimit, @@ -112,8 +112,11 @@ impl StatusLineItem { StatusLineItem::CurrentDir => "Current working directory", StatusLineItem::ProjectRoot => "Project root directory (omitted when unavailable)", StatusLineItem::GitBranch => "Current Git branch (omitted when unavailable)", - StatusLineItem::ContextUsage => { - "Visual meter of context window usage (omitted when unknown)" + StatusLineItem::ContextRemaining => { + "Percentage of context window remaining (omitted when unknown)" + } + StatusLineItem::ContextUsed => { + "Percentage of context window used (omitted when unknown)" } StatusLineItem::FiveHourLimit => { "Remaining usage on 5-hour usage limit (omitted when unavailable)" @@ -137,25 +140,6 @@ impl StatusLineItem { } } -const SELECTABLE_STATUS_LINE_ITEMS: &[StatusLineItem] = &[ - StatusLineItem::ModelName, - StatusLineItem::ModelWithReasoning, - StatusLineItem::CurrentDir, - StatusLineItem::ProjectRoot, - StatusLineItem::GitBranch, - StatusLineItem::ContextUsage, - StatusLineItem::FiveHourLimit, - StatusLineItem::WeeklyLimit, - StatusLineItem::CodexVersion, - StatusLineItem::ContextWindowSize, - StatusLineItem::UsedTokens, - StatusLineItem::TotalInputTokens, - StatusLineItem::TotalOutputTokens, - StatusLineItem::SessionId, - StatusLineItem::FastMode, - StatusLineItem::ThreadTitle, -]; - /// Runtime values used to preview the current status-line selection. #[derive(Clone, Debug, Default, Eq, PartialEq)] pub(crate) struct StatusLinePreviewData { @@ -232,7 +216,7 @@ impl StatusLineSetupView { } } - for item in SELECTABLE_STATUS_LINE_ITEMS.iter().cloned() { + for item in StatusLineItem::iter() { let item_id = item.to_string(); if used_ids.contains(&item_id) { continue; @@ -317,19 +301,27 @@ mod tests { use crate::app_event::AppEvent; #[test] - fn context_usage_is_canonical_and_accepts_legacy_ids() { - assert_eq!(StatusLineItem::ContextUsage.to_string(), "context-usage"); - assert_eq!( - "context-usage".parse::(), - Ok(StatusLineItem::ContextUsage) - ); - assert_eq!( - "context-remaining".parse::(), - Ok(StatusLineItem::ContextUsage) - ); + fn context_used_accepts_context_usage_legacy_id() { + assert_eq!(StatusLineItem::ContextUsed.to_string(), "context-used"); assert_eq!( "context-used".parse::(), - Ok(StatusLineItem::ContextUsage) + Ok(StatusLineItem::ContextUsed) + ); + assert_eq!( + "context-usage".parse::(), + Ok(StatusLineItem::ContextUsed) + ); + } + + #[test] + fn context_remaining_is_separate_selectable_id() { + assert_eq!( + "context-remaining".parse::(), + Ok(StatusLineItem::ContextRemaining) + ); + assert_eq!( + StatusLineItem::ContextRemaining.to_string(), + "context-remaining" ); } diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_line_model_with_reasoning_fast_footer.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_line_model_with_reasoning_fast_footer.snap index c71a570606..2ac794d2f0 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_line_model_with_reasoning_fast_footer.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_line_model_with_reasoning_fast_footer.snap @@ -6,4 +6,4 @@ expression: normalized_backend_snapshot(terminal.backend()) " " "› Ask Codex to do anything " " " -" gpt-5.4 xhigh fast · Context [ ] · /tmp/project " +" gpt-5.4 xhigh fast · Context 0% used · /tmp/project " diff --git a/codex-rs/tui/src/chatwidget/status_surfaces.rs b/codex-rs/tui/src/chatwidget/status_surfaces.rs index 078d316175..a744711c2c 100644 --- a/codex-rs/tui/src/chatwidget/status_surfaces.rs +++ b/codex-rs/tui/src/chatwidget/status_surfaces.rs @@ -450,9 +450,12 @@ impl ChatWidget { Some(format!("{} used", format_tokens_compact(total))) } } - StatusLineItem::ContextUsage => self + StatusLineItem::ContextRemaining => self + .status_line_context_remaining_percent() + .map(|remaining| format!("Context {remaining}% left")), + StatusLineItem::ContextUsed => self .status_line_context_used_percent() - .map(format_context_used_meter), + .map(|used| format!("Context {used}% used")), StatusLineItem::FiveHourLimit => { let window = self .rate_limit_snapshots_by_limit_id @@ -661,61 +664,3 @@ where } (items, invalid) } - -fn format_context_used_meter(used_percent: i64) -> String { - const METER_WIDTH: usize = 5; - const EIGHTHS_PER_CELL: i64 = 8; - const PARTIAL_BLOCKS: [&str; 8] = ["", "▏", "▎", "▍", "▌", "▋", "▊", "▉"]; - - let used_percent = used_percent.clamp(0, 100); - let total_eighths = (used_percent * METER_WIDTH as i64 * EIGHTHS_PER_CELL + 50) / 100; - let filled_cells = (total_eighths / EIGHTHS_PER_CELL) as usize; - let partial_eighths = (total_eighths % EIGHTHS_PER_CELL) as usize; - - let mut meter = String::with_capacity(METER_WIDTH); - meter.push_str(&"█".repeat(filled_cells)); - meter.push_str(PARTIAL_BLOCKS[partial_eighths]); - - let occupied_cells = filled_cells + usize::from(partial_eighths > 0); - meter.push_str(&" ".repeat(METER_WIDTH.saturating_sub(occupied_cells))); - - format!("Context [{meter}]") -} - -#[cfg(test)] -mod tests { - use super::format_context_used_meter; - use pretty_assertions::assert_eq; - - #[test] - fn context_meter_uses_five_cells_with_partial_blocks() { - assert_eq!( - format_context_used_meter(/*used_percent*/ 100), - "Context [█████]" - ); - assert_eq!( - format_context_used_meter(/*used_percent*/ 50), - "Context [██▌ ]" - ); - assert_eq!( - format_context_used_meter(/*used_percent*/ 10), - "Context [▌ ]" - ); - assert_eq!( - format_context_used_meter(/*used_percent*/ 0), - "Context [ ]" - ); - } - - #[test] - fn context_meter_clamps_out_of_range_values() { - assert_eq!( - format_context_used_meter(/*used_percent*/ 125), - "Context [█████]" - ); - assert_eq!( - format_context_used_meter(/*used_percent*/ -1), - "Context [ ]" - ); - } -} diff --git a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs index 8ac21627cd..1c43d2e91d 100644 --- a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs +++ b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs @@ -867,32 +867,50 @@ async fn status_line_invalid_items_warn_once() { } #[tokio::test] -async fn status_line_legacy_context_used_renders_context_meter() { +async fn status_line_context_used_renders_labeled_percent() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; chat.thread_id = Some(ThreadId::new()); chat.config.tui_status_line = Some(vec!["context-used".to_string()]); chat.refresh_status_line(); - assert_eq!(status_line_text(&chat), Some("Context [ ]".to_string())); + assert_eq!(status_line_text(&chat), Some("Context 0% used".to_string())); assert!( drain_insert_history(&mut rx).is_empty(), - "legacy context-used should remain a valid status line item" + "context-used should remain a valid status line item" ); } #[tokio::test] -async fn status_line_legacy_context_remaining_renders_context_meter() { +async fn status_line_context_remaining_renders_labeled_percent() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; chat.thread_id = Some(ThreadId::new()); chat.config.tui_status_line = Some(vec!["context-remaining".to_string()]); chat.refresh_status_line(); - assert_eq!(status_line_text(&chat), Some("Context [ ]".to_string())); + assert_eq!( + status_line_text(&chat), + Some("Context 100% left".to_string()) + ); assert!( drain_insert_history(&mut rx).is_empty(), - "legacy context-remaining should remain a valid status line item" + "context-remaining should remain a valid status line item" + ); +} + +#[tokio::test] +async fn status_line_legacy_context_usage_renders_context_used_percent() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.thread_id = Some(ThreadId::new()); + chat.config.tui_status_line = Some(vec!["context-usage".to_string()]); + + chat.refresh_status_line(); + + assert_eq!(status_line_text(&chat), Some("Context 0% used".to_string())); + assert!( + drain_insert_history(&mut rx).is_empty(), + "legacy context-usage should remain a valid status line item" ); } @@ -995,7 +1013,7 @@ async fn status_line_model_with_reasoning_includes_fast_for_fast_capable_models( chat.config.cwd = test_project_path().abs(); chat.config.tui_status_line = Some(vec![ "model-with-reasoning".to_string(), - "context-usage".to_string(), + "context-used".to_string(), "current-dir".to_string(), ]); chat.set_reasoning_effort(Some(ReasoningEffortConfig::XHigh)); @@ -1008,7 +1026,7 @@ async fn status_line_model_with_reasoning_includes_fast_for_fast_capable_models( assert_eq!( status_line_text(&chat), - Some(format!("gpt-5.4 xhigh fast · Context [ ] · {test_cwd}")) + Some(format!("gpt-5.4 xhigh fast · Context 0% used · {test_cwd}")) ); chat.set_model("gpt-5.3-codex"); @@ -1017,7 +1035,7 @@ async fn status_line_model_with_reasoning_includes_fast_for_fast_capable_models( assert_eq!( status_line_text(&chat), Some(format!( - "gpt-5.3-codex xhigh · Context [ ] · {test_cwd}" + "gpt-5.3-codex xhigh · Context 0% used · {test_cwd}" )) ); } @@ -1141,7 +1159,7 @@ async fn status_line_model_with_reasoning_fast_footer_snapshot() { chat.config.cwd = test_project_path().abs(); chat.config.tui_status_line = Some(vec![ "model-with-reasoning".to_string(), - "context-usage".to_string(), + "context-used".to_string(), "current-dir".to_string(), ]); chat.set_reasoning_effort(Some(ReasoningEffortConfig::XHigh)); From 46ab9974dc14ae0bc626ba8d48cc3839ecd69995 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Sun, 12 Apr 2026 15:50:12 -0700 Subject: [PATCH 5/6] Expose instruction sources (AGENTS.md) via app server (#17506) Addresses #17498 Problem: The TUI derived /status instruction source paths from the local client environment, which could show stale output or incorrect paths when connected to a remote app server. Solution: Add an app-server v2 instructionSources snapshot to thread start/resume/fork responses, default it to an empty list when older servers omit it, and render TUI /status from that server-provided session data. Additional context: The app-server field is intentionally named instructionSources rather than AGENTS.md-specific terminology because the loaded instruction sources can include global instructions, project AGENTS.md files, AGENTS.override.md, user-defined instruction files, and future dynamic sources. --- .../analytics/src/analytics_client_tests.rs | 2 + .../codex_app_server_protocol.schemas.json | 24 ++++++++ .../codex_app_server_protocol.v2.schemas.json | 24 ++++++++ .../schema/json/v2/ThreadForkResponse.json | 8 +++ .../schema/json/v2/ThreadResumeResponse.json | 8 +++ .../schema/json/v2/ThreadStartResponse.json | 8 +++ .../typescript/v2/ThreadForkResponse.ts | 6 +- .../typescript/v2/ThreadResumeResponse.ts | 6 +- .../typescript/v2/ThreadStartResponse.ts | 6 +- .../src/protocol/common.rs | 2 + .../app-server-protocol/src/protocol/v2.rs | 53 +++++++++++++++++ .../app-server/src/codex_message_processor.rs | 33 +++++++++++ codex-rs/app-server/src/thread_state.rs | 1 + .../app-server/tests/suite/v2/thread_start.rs | 59 +++++++++++++++++++ codex-rs/exec/src/lib_tests.rs | 1 + codex-rs/tui/src/app.rs | 3 + codex-rs/tui/src/app_server_session.rs | 13 ++++ codex-rs/tui/src/chatwidget.rs | 22 +++---- codex-rs/tui/src/chatwidget/tests/helpers.rs | 1 + .../chatwidget/tests/status_command_tests.rs | 23 ++++++++ codex-rs/tui/src/status/card.rs | 17 +----- codex-rs/tui/src/status/helpers.rs | 49 +++++---------- codex-rs/tui/src/status/mod.rs | 2 +- 23 files changed, 302 insertions(+), 69 deletions(-) diff --git a/codex-rs/analytics/src/analytics_client_tests.rs b/codex-rs/analytics/src/analytics_client_tests.rs index d86159560b..e5aca39b8e 100644 --- a/codex-rs/analytics/src/analytics_client_tests.rs +++ b/codex-rs/analytics/src/analytics_client_tests.rs @@ -105,6 +105,7 @@ fn sample_thread_start_response(thread_id: &str, ephemeral: bool, model: &str) - model_provider: "openai".to_string(), service_tier: None, cwd: PathBuf::from("/tmp"), + instruction_sources: Vec::new(), approval_policy: AppServerAskForApproval::OnFailure, approvals_reviewer: AppServerApprovalsReviewer::User, sandbox: AppServerSandboxPolicy::DangerFullAccess, @@ -155,6 +156,7 @@ fn sample_thread_resume_response_with_source( model_provider: "openai".to_string(), service_tier: None, cwd: PathBuf::from("/tmp"), + instruction_sources: Vec::new(), approval_policy: AppServerAskForApproval::OnFailure, approvals_reviewer: AppServerApprovalsReviewer::User, sandbox: AppServerSandboxPolicy::DangerFullAccess, diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index dd839cd586..15b31cc0ef 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -12806,6 +12806,14 @@ "cwd": { "type": "string" }, + "instructionSources": { + "default": [], + "description": "Instruction source files currently loaded for this thread.", + "items": { + "type": "string" + }, + "type": "array" + }, "model": { "type": "string" }, @@ -14095,6 +14103,14 @@ "cwd": { "type": "string" }, + "instructionSources": { + "default": [], + "description": "Instruction source files currently loaded for this thread.", + "items": { + "type": "string" + }, + "type": "array" + }, "model": { "type": "string" }, @@ -14386,6 +14402,14 @@ "cwd": { "type": "string" }, + "instructionSources": { + "default": [], + "description": "Instruction source files currently loaded for this thread.", + "items": { + "type": "string" + }, + "type": "array" + }, "model": { "type": "string" }, diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 66b91082e0..dd053d77d2 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -10654,6 +10654,14 @@ "cwd": { "type": "string" }, + "instructionSources": { + "default": [], + "description": "Instruction source files currently loaded for this thread.", + "items": { + "type": "string" + }, + "type": "array" + }, "model": { "type": "string" }, @@ -11943,6 +11951,14 @@ "cwd": { "type": "string" }, + "instructionSources": { + "default": [], + "description": "Instruction source files currently loaded for this thread.", + "items": { + "type": "string" + }, + "type": "array" + }, "model": { "type": "string" }, @@ -12234,6 +12250,14 @@ "cwd": { "type": "string" }, + "instructionSources": { + "default": [], + "description": "Instruction source files currently loaded for this thread.", + "items": { + "type": "string" + }, + "type": "array" + }, "model": { "type": "string" }, diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json index 9b1870198a..5af02d344e 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json @@ -2187,6 +2187,14 @@ "cwd": { "type": "string" }, + "instructionSources": { + "default": [], + "description": "Instruction source files currently loaded for this thread.", + "items": { + "type": "string" + }, + "type": "array" + }, "model": { "type": "string" }, diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json index 5143545ec1..d888485d15 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json @@ -2187,6 +2187,14 @@ "cwd": { "type": "string" }, + "instructionSources": { + "default": [], + "description": "Instruction source files currently loaded for this thread.", + "items": { + "type": "string" + }, + "type": "array" + }, "model": { "type": "string" }, diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json index c07b4d8258..7a0e083093 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json @@ -2187,6 +2187,14 @@ "cwd": { "type": "string" }, + "instructionSources": { + "default": [], + "description": "Instruction source files currently loaded for this thread.", + "items": { + "type": "string" + }, + "type": "array" + }, "model": { "type": "string" }, diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadForkResponse.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadForkResponse.ts index 5e9d460a9a..df6c50227d 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadForkResponse.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadForkResponse.ts @@ -8,7 +8,11 @@ import type { AskForApproval } from "./AskForApproval"; import type { SandboxPolicy } from "./SandboxPolicy"; import type { Thread } from "./Thread"; -export type ThreadForkResponse = { thread: Thread, model: string, modelProvider: string, serviceTier: ServiceTier | null, cwd: string, approvalPolicy: AskForApproval, +export type ThreadForkResponse = { thread: Thread, model: string, modelProvider: string, serviceTier: ServiceTier | null, cwd: string, +/** + * Instruction source files currently loaded for this thread. + */ +instructionSources: Array, approvalPolicy: AskForApproval, /** * Reviewer currently used for approval requests on this thread. */ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadResumeResponse.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadResumeResponse.ts index 9c7ec4527b..3234e8b4b3 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadResumeResponse.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadResumeResponse.ts @@ -8,7 +8,11 @@ import type { AskForApproval } from "./AskForApproval"; import type { SandboxPolicy } from "./SandboxPolicy"; import type { Thread } from "./Thread"; -export type ThreadResumeResponse = { thread: Thread, model: string, modelProvider: string, serviceTier: ServiceTier | null, cwd: string, approvalPolicy: AskForApproval, +export type ThreadResumeResponse = { thread: Thread, model: string, modelProvider: string, serviceTier: ServiceTier | null, cwd: string, +/** + * Instruction source files currently loaded for this thread. + */ +instructionSources: Array, approvalPolicy: AskForApproval, /** * Reviewer currently used for approval requests on this thread. */ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadStartResponse.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadStartResponse.ts index c85f81f52b..e3355b9108 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadStartResponse.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadStartResponse.ts @@ -8,7 +8,11 @@ import type { AskForApproval } from "./AskForApproval"; import type { SandboxPolicy } from "./SandboxPolicy"; import type { Thread } from "./Thread"; -export type ThreadStartResponse = { thread: Thread, model: string, modelProvider: string, serviceTier: ServiceTier | null, cwd: string, approvalPolicy: AskForApproval, +export type ThreadStartResponse = { thread: Thread, model: string, modelProvider: string, serviceTier: ServiceTier | null, cwd: string, +/** + * Instruction source files currently loaded for this thread. + */ +instructionSources: Array, approvalPolicy: AskForApproval, /** * Reviewer currently used for approval requests on this thread. */ diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 3f9ba7bfdf..3450e41534 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -1405,6 +1405,7 @@ mod tests { model_provider: "openai".to_string(), service_tier: None, cwd: PathBuf::from("/tmp"), + instruction_sources: vec![PathBuf::from("/tmp/AGENTS.md")], approval_policy: v2::AskForApproval::OnFailure, approvals_reviewer: v2::ApprovalsReviewer::User, sandbox: v2::SandboxPolicy::DangerFullAccess, @@ -1444,6 +1445,7 @@ mod tests { "modelProvider": "openai", "serviceTier": null, "cwd": "/tmp", + "instructionSources": ["/tmp/AGENTS.md"], "approvalPolicy": "on-failure", "approvalsReviewer": "user", "sandbox": { diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 4ce7e9c0bb..30adc152ea 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -2719,6 +2719,9 @@ pub struct ThreadStartResponse { pub model_provider: String, pub service_tier: Option, pub cwd: PathBuf, + /// Instruction source files currently loaded for this thread. + #[serde(default)] + pub instruction_sources: Vec, #[experimental(nested)] pub approval_policy: AskForApproval, /// Reviewer currently used for approval requests on this thread. @@ -2805,6 +2808,9 @@ pub struct ThreadResumeResponse { pub model_provider: String, pub service_tier: Option, pub cwd: PathBuf, + /// Instruction source files currently loaded for this thread. + #[serde(default)] + pub instruction_sources: Vec, #[experimental(nested)] pub approval_policy: AskForApproval, /// Reviewer currently used for approval requests on this thread. @@ -2882,6 +2888,9 @@ pub struct ThreadForkResponse { pub model_provider: String, pub service_tier: Option, pub cwd: PathBuf, + /// Instruction source files currently loaded for this thread. + #[serde(default)] + pub instruction_sources: Vec, #[experimental(nested)] pub approval_policy: AskForApproval, /// Reviewer currently used for approval requests on this thread. @@ -8506,6 +8515,50 @@ mod tests { assert_eq!(serialized_without_override.get("serviceTier"), None); } + #[test] + fn thread_lifecycle_responses_default_missing_instruction_sources() { + let response = json!({ + "thread": { + "id": "thread-id", + "forkedFromId": null, + "preview": "", + "ephemeral": false, + "modelProvider": "openai", + "createdAt": 1, + "updatedAt": 1, + "status": { "type": "idle" }, + "path": null, + "cwd": "/tmp", + "cliVersion": "0.0.0", + "source": "exec", + "agentNickname": null, + "agentRole": null, + "gitInfo": null, + "name": null, + "turns": [] + }, + "model": "gpt-5", + "modelProvider": "openai", + "serviceTier": null, + "cwd": "/tmp", + "approvalPolicy": "on-failure", + "approvalsReviewer": "user", + "sandbox": { "type": "dangerFullAccess" }, + "reasoningEffort": null + }); + + let start: ThreadStartResponse = + serde_json::from_value(response.clone()).expect("thread/start response"); + let resume: ThreadResumeResponse = + serde_json::from_value(response.clone()).expect("thread/resume response"); + let fork: ThreadForkResponse = + serde_json::from_value(response).expect("thread/fork response"); + + assert_eq!(start.instruction_sources, Vec::::new()); + assert_eq!(resume.instruction_sources, Vec::::new()); + assert_eq!(fork.instruction_sources, Vec::::new()); + } + #[test] fn turn_start_params_preserve_explicit_null_service_tier() { let params: TurnStartParams = serde_json::from_value(json!({ diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 53f4a876c2..6762b9e129 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -239,6 +239,7 @@ use codex_core::sandboxing::SandboxPermissions; use codex_core::windows_sandbox::WindowsSandboxLevelExt; use codex_core::windows_sandbox::WindowsSandboxSetupMode as CoreWindowsSandboxSetupMode; use codex_core::windows_sandbox::WindowsSandboxSetupRequest; +use codex_exec_server::LOCAL_FS; use codex_features::FEATURES; use codex_features::Feature; use codex_features::Stage; @@ -491,6 +492,23 @@ pub(crate) struct CodexMessageProcessorArgs { } impl CodexMessageProcessor { + async fn instruction_sources_from_config(config: &Config) -> Vec { + let mut paths: Vec = config.user_instructions_path.iter().cloned().collect(); + match codex_core::discover_project_doc_paths(config, LOCAL_FS.as_ref()).await { + Ok(project_doc_paths) => { + paths.extend( + project_doc_paths + .into_iter() + .map(|path| path.as_path().to_path_buf()), + ); + } + Err(err) => { + tracing::warn!(error = %err, "failed to discover project docs for thread response"); + } + } + paths + } + pub(crate) fn handle_config_mutation(&self) { self.clear_plugin_related_caches(); } @@ -2312,6 +2330,7 @@ impl CodexMessageProcessor { }; } + let instruction_sources = Self::instruction_sources_from_config(&config).await; let dynamic_tools = dynamic_tools.unwrap_or_default(); let core_dynamic_tools = if dynamic_tools.is_empty() { Vec::new() @@ -2443,6 +2462,7 @@ impl CodexMessageProcessor { model_provider: config_snapshot.model_provider_id, service_tier: config_snapshot.service_tier, cwd: config_snapshot.cwd, + instruction_sources, approval_policy: config_snapshot.approval_policy.into(), approvals_reviewer: config_snapshot.approvals_reviewer.into(), sandbox: config_snapshot.sandbox_policy.into(), @@ -3881,6 +3901,7 @@ impl CodexMessageProcessor { }; let fallback_model_provider = config.model_provider_id.clone(); + let instruction_sources = Self::instruction_sources_from_config(&config).await; let response_history = thread_history.clone(); match self @@ -3961,6 +3982,7 @@ impl CodexMessageProcessor { model_provider: session_configured.model_provider_id, service_tier: session_configured.service_tier, cwd: session_configured.cwd, + instruction_sources, approval_policy: session_configured.approval_policy.into(), approvals_reviewer: session_configured.approvals_reviewer.into(), sandbox: session_configured.sandbox_policy.into(), @@ -4119,6 +4141,12 @@ impl CodexMessageProcessor { mismatch_details.join("; ") ); } + let mut config_for_instruction_sources = self.config.as_ref().clone(); + if let Ok(cwd) = AbsolutePathBuf::try_from(config_snapshot.cwd.clone()) { + config_for_instruction_sources.cwd = cwd; + } + let instruction_sources = + Self::instruction_sources_from_config(&config_for_instruction_sources).await; let thread_summary = match load_thread_summary_for_rollout( &self.config, existing_thread_id, @@ -4156,6 +4184,7 @@ impl CodexMessageProcessor { request_id: request_id.clone(), rollout_path: rollout_path.clone(), config_snapshot, + instruction_sources, thread_summary, }), ); @@ -4431,6 +4460,7 @@ impl CodexMessageProcessor { }; let fallback_model_provider = config.model_provider_id.clone(); + let instruction_sources = Self::instruction_sources_from_config(&config).await; let NewThread { thread_id, @@ -4585,6 +4615,7 @@ impl CodexMessageProcessor { model_provider: session_configured.model_provider_id, service_tier: session_configured.service_tier, cwd: session_configured.cwd, + instruction_sources, approval_policy: session_configured.approval_policy.into(), approvals_reviewer: session_configured.approvals_reviewer.into(), sandbox: session_configured.sandbox_policy.into(), @@ -8156,12 +8187,14 @@ async fn handle_pending_thread_resume_request( reasoning_effort, .. } = pending.config_snapshot; + let instruction_sources = pending.instruction_sources; let response = ThreadResumeResponse { thread, model, model_provider: model_provider_id, service_tier, cwd, + instruction_sources, approval_policy: approval_policy.into(), approvals_reviewer: approvals_reviewer.into(), sandbox: sandbox_policy.into(), diff --git a/codex-rs/app-server/src/thread_state.rs b/codex-rs/app-server/src/thread_state.rs index 805a64bfb5..11d6ad6bb3 100644 --- a/codex-rs/app-server/src/thread_state.rs +++ b/codex-rs/app-server/src/thread_state.rs @@ -27,6 +27,7 @@ pub(crate) struct PendingThreadResumeRequest { pub(crate) request_id: ConnectionRequestId, pub(crate) rollout_path: PathBuf, pub(crate) config_snapshot: ThreadConfigSnapshot, + pub(crate) instruction_sources: Vec, pub(crate) thread_summary: codex_app_server_protocol::Thread, } diff --git a/codex-rs/app-server/tests/suite/v2/thread_start.rs b/codex-rs/app-server/tests/suite/v2/thread_start.rs index 50c373ac93..c0df2cab07 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_start.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_start.rs @@ -29,6 +29,7 @@ use pretty_assertions::assert_eq; use serde_json::Value; use serde_json::json; use std::path::Path; +use std::path::PathBuf; use std::time::Duration; use tempfile::TempDir; use tokio::time::timeout; @@ -162,6 +163,64 @@ async fn thread_start_creates_thread_and_emits_started() -> Result<()> { Ok(()) } +#[tokio::test] +async fn thread_start_response_includes_loaded_instruction_sources() -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + let codex_home = TempDir::new()?; + create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?; + let global_agents_path = codex_home.path().join("AGENTS.md"); + std::fs::write(&global_agents_path, "global instructions")?; + let workspace = TempDir::new()?; + let project_agents_path = workspace.path().join("AGENTS.md"); + std::fs::write(&project_agents_path, "project instructions")?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_thread_start_request(ThreadStartParams { + cwd: Some(workspace.path().display().to_string()), + ..Default::default() + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let ThreadStartResponse { + instruction_sources, + .. + } = to_response::(response)?; + + let instruction_sources = instruction_sources + .into_iter() + .map(normalize_path_for_comparison) + .collect::>(); + let expected_instruction_sources = vec![ + std::fs::canonicalize(global_agents_path)?, + std::fs::canonicalize(project_agents_path)?, + ] + .into_iter() + .map(normalize_path_for_comparison) + .collect::>(); + + assert_eq!(instruction_sources, expected_instruction_sources); + + Ok(()) +} + +#[cfg(windows)] +fn normalize_path_for_comparison(path: PathBuf) -> PathBuf { + let path = path.display().to_string(); + PathBuf::from(path.strip_prefix(r"\\?\").unwrap_or(&path)) +} + +#[cfg(not(windows))] +fn normalize_path_for_comparison(path: PathBuf) -> PathBuf { + path +} + #[tokio::test] async fn thread_start_tracks_thread_initialized_analytics() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; diff --git a/codex-rs/exec/src/lib_tests.rs b/codex-rs/exec/src/lib_tests.rs index 0af5cc5250..519a1d6cb3 100644 --- a/codex-rs/exec/src/lib_tests.rs +++ b/codex-rs/exec/src/lib_tests.rs @@ -410,6 +410,7 @@ fn session_configured_from_thread_response_uses_review_policy_from_response() { model_provider: "openai".to_string(), service_tier: None, cwd: PathBuf::from("/tmp"), + instruction_sources: Vec::new(), approval_policy: codex_app_server_protocol::AskForApproval::OnRequest, approvals_reviewer: codex_app_server_protocol::ApprovalsReviewer::GuardianSubagent, sandbox: codex_app_server_protocol::SandboxPolicy::WorkspaceWrite { diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 7dea884dea..a8db00dc21 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -3062,6 +3062,7 @@ impl App { approvals_reviewer: self.config.approvals_reviewer, sandbox_policy: self.config.permissions.sandbox_policy.get().clone(), cwd: thread.cwd.clone(), + instruction_source_paths: Vec::new(), reasoning_effort: self.chat_widget.current_reasoning_effort(), history_log_id: 0, history_entry_count: 0, @@ -3072,6 +3073,7 @@ impl App { session.thread_name = thread.name.clone(); session.model_provider_id = thread.model_provider.clone(); session.cwd = thread.cwd.clone(); + session.instruction_source_paths = Vec::new(); session.rollout_path = thread.path.clone(); if let Some(model) = read_session_model(&self.config, thread_id, thread.path.as_deref()).await @@ -9365,6 +9367,7 @@ guardian_approval = true approvals_reviewer: ApprovalsReviewer::User, sandbox_policy: SandboxPolicy::new_read_only_policy(), cwd, + instruction_source_paths: Vec::new(), reasoning_effort: None, history_log_id: 0, history_entry_count: 0, diff --git a/codex-rs/tui/src/app_server_session.rs b/codex-rs/tui/src/app_server_session.rs index 779efa02ba..5f75bd390a 100644 --- a/codex-rs/tui/src/app_server_session.rs +++ b/codex-rs/tui/src/app_server_session.rs @@ -132,6 +132,7 @@ pub(crate) struct ThreadSessionState { pub(crate) approvals_reviewer: codex_protocol::config_types::ApprovalsReviewer, pub(crate) sandbox_policy: SandboxPolicy, pub(crate) cwd: PathBuf, + pub(crate) instruction_source_paths: Vec, pub(crate) reasoning_effort: Option, pub(crate) history_log_id: u64, pub(crate) history_entry_count: u64, @@ -993,6 +994,7 @@ async fn thread_session_state_from_thread_start_response( response.approvals_reviewer.to_core(), response.sandbox.to_core(), response.cwd.clone(), + response.instruction_sources.clone(), response.reasoning_effort, config, ) @@ -1015,6 +1017,7 @@ async fn thread_session_state_from_thread_resume_response( response.approvals_reviewer.to_core(), response.sandbox.to_core(), response.cwd.clone(), + response.instruction_sources.clone(), response.reasoning_effort, config, ) @@ -1037,6 +1040,7 @@ async fn thread_session_state_from_thread_fork_response( response.approvals_reviewer.to_core(), response.sandbox.to_core(), response.cwd.clone(), + response.instruction_sources.clone(), response.reasoning_effort, config, ) @@ -1078,6 +1082,7 @@ async fn thread_session_state_from_thread_response( approvals_reviewer: codex_protocol::config_types::ApprovalsReviewer, sandbox_policy: SandboxPolicy, cwd: PathBuf, + instruction_source_paths: Vec, reasoning_effort: Option, config: &Config, ) -> Result { @@ -1102,6 +1107,7 @@ async fn thread_session_state_from_thread_response( approvals_reviewer, sandbox_policy, cwd, + instruction_source_paths, reasoning_effort, history_log_id, history_entry_count, @@ -1326,6 +1332,7 @@ mod tests { model_provider: "openai".to_string(), service_tier: None, cwd: PathBuf::from("/tmp/project"), + instruction_sources: vec![PathBuf::from("/tmp/project/AGENTS.md")], approval_policy: codex_protocol::protocol::AskForApproval::Never.into(), approvals_reviewer: codex_app_server_protocol::ApprovalsReviewer::User, sandbox: codex_protocol::protocol::SandboxPolicy::new_read_only_policy().into(), @@ -1336,6 +1343,10 @@ mod tests { .await .expect("resume response should map"); assert_eq!(started.session.forked_from_id, Some(forked_from_id)); + assert_eq!( + started.session.instruction_source_paths, + response.instruction_sources + ); assert_eq!(started.turns.len(), 1); assert_eq!(started.turns[0], response.thread.turns[0]); } @@ -1365,6 +1376,7 @@ mod tests { codex_protocol::config_types::ApprovalsReviewer::User, SandboxPolicy::new_read_only_policy(), PathBuf::from("/tmp/project"), + Vec::new(), /*reasoning_effort*/ None, &config, ) @@ -1394,6 +1406,7 @@ mod tests { codex_protocol::config_types::ApprovalsReviewer::User, SandboxPolicy::new_read_only_policy(), PathBuf::from("/tmp/project"), + Vec::new(), /*reasoning_effort*/ None, &config, ) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 7f47edbb99..e7ef6a0606 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -942,6 +942,8 @@ pub(crate) struct ChatWidget { current_rollout_path: Option, // Current working directory (if known) current_cwd: Option, + // Instruction source files loaded for the current session, supplied by app-server. + instruction_source_paths: Vec, // Runtime network proxy bind addresses from SessionConfigured. session_network_proxy: Option, // Shared latch so we only warn once about invalid status-line item IDs. @@ -2071,6 +2073,7 @@ impl ChatWidget { } pub(crate) fn handle_thread_session(&mut self, session: ThreadSessionState) { + self.instruction_source_paths = session.instruction_source_paths.clone(); self.on_session_configured(thread_session_state_to_legacy_event(session)); } @@ -4865,6 +4868,7 @@ impl ChatWidget { feedback, current_rollout_path: None, current_cwd, + instruction_source_paths: Vec::new(), session_network_proxy: None, status_line_invalid_items_warned, terminal_title_invalid_items_warned, @@ -7071,8 +7075,8 @@ impl ChatWidget { .values() .cloned() .collect(); - let config = self.config.clone(); - let frame_requester = self.frame_requester.clone(); + let agents_summary = + crate::status::compose_agents_summary(&self.config, &self.instruction_source_paths); let (cell, handle) = crate::status::new_status_output_with_rate_limits_handle( &self.config, self.status_account_display.as_ref(), @@ -7087,21 +7091,9 @@ impl ChatWidget { self.model_display_name(), collaboration_mode, reasoning_effort_override, - "".to_string(), + agents_summary, refreshing_rate_limits, ); - let agents_summary_handle = handle.clone(); - tokio::spawn(async move { - let agents_summary = match crate::status::discover_agents_summary(&config).await { - Ok(summary) => summary, - Err(err) => { - tracing::warn!(error = %err, "failed to discover project docs for /status"); - "".to_string() - } - }; - agents_summary_handle.finish_agents_summary_discovery(agents_summary); - frame_requester.schedule_frame(); - }); if let Some(request_id) = request_id { self.refreshing_status_outputs.push((request_id, handle)); } diff --git a/codex-rs/tui/src/chatwidget/tests/helpers.rs b/codex-rs/tui/src/chatwidget/tests/helpers.rs index d5c6a5e3b4..96fdb0a40b 100644 --- a/codex-rs/tui/src/chatwidget/tests/helpers.rs +++ b/codex-rs/tui/src/chatwidget/tests/helpers.rs @@ -275,6 +275,7 @@ pub(super) async fn make_chatwidget_manual( feedback: codex_feedback::CodexFeedback::new(), current_rollout_path: None, current_cwd: None, + instruction_source_paths: Vec::new(), session_network_proxy: None, status_line_invalid_items_warned: Arc::new(AtomicBool::new(false)), terminal_title_invalid_items_warned: Arc::new(AtomicBool::new(false)), diff --git a/codex-rs/tui/src/chatwidget/tests/status_command_tests.rs b/codex-rs/tui/src/chatwidget/tests/status_command_tests.rs index ba14ceb039..dcb8d82fc8 100644 --- a/codex-rs/tui/src/chatwidget/tests/status_command_tests.rs +++ b/codex-rs/tui/src/chatwidget/tests/status_command_tests.rs @@ -76,6 +76,29 @@ async fn status_command_renders_immediately_without_rate_limit_refresh() { ); } +#[tokio::test] +async fn status_command_renders_instruction_sources_from_thread_session() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.instruction_source_paths = vec![chat.config.cwd.join("AGENTS.md").to_path_buf()]; + + chat.dispatch_command(SlashCommand::Status); + + let rendered = match rx.try_recv() { + Ok(AppEvent::InsertHistoryCell(cell)) => { + lines_to_single_string(&cell.display_lines(/*width*/ 80)) + } + other => panic!("expected status output, got {other:?}"), + }; + assert!( + rendered.contains("Agents.md"), + "expected /status to render app-server instruction sources, got: {rendered}" + ); + assert!( + !rendered.contains("Agents.md "), + "expected /status to avoid stale when app-server provided instruction sources, got: {rendered}" + ); +} + #[tokio::test] async fn status_command_overlapping_refreshes_update_matching_cells_only() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; diff --git a/codex-rs/tui/src/status/card.rs b/codex-rs/tui/src/status/card.rs index dccb86cdc9..340f933689 100644 --- a/codex-rs/tui/src/status/card.rs +++ b/codex-rs/tui/src/status/card.rs @@ -67,20 +67,10 @@ struct StatusRateLimitState { #[derive(Debug, Clone)] pub(crate) struct StatusHistoryHandle { - agents_summary: Arc>, rate_limit_state: Arc>, } impl StatusHistoryHandle { - pub(crate) fn finish_agents_summary_discovery(&self, agents_summary: String) { - #[expect(clippy::expect_used)] - let mut current = self - .agents_summary - .write() - .expect("status history agents summary state poisoned"); - *current = agents_summary; - } - pub(crate) fn finish_rate_limit_refresh( &self, rate_limits: &[RateLimitSnapshotDisplay], @@ -360,13 +350,10 @@ impl StatusHistoryCell { session_id, forked_from, token_usage, - agents_summary: agents_summary.clone(), + agents_summary, rate_limit_state: rate_limit_state.clone(), }, - StatusHistoryHandle { - agents_summary, - rate_limit_state, - }, + StatusHistoryHandle { rate_limit_state }, ) } diff --git a/codex-rs/tui/src/status/helpers.rs b/codex-rs/tui/src/status/helpers.rs index ef28ed7fcf..0ee90d4115 100644 --- a/codex-rs/tui/src/status/helpers.rs +++ b/codex-rs/tui/src/status/helpers.rs @@ -1,15 +1,12 @@ use crate::exec_command::relativize_to_home; use crate::legacy_core::config::Config; -use crate::legacy_core::discover_project_doc_paths; use crate::status::StatusAccountDisplay; use crate::text_formatting; use chrono::DateTime; use chrono::Local; -use codex_exec_server::LOCAL_FS; use codex_protocol::account::PlanType; -use codex_utils_absolute_path::AbsolutePathBuf; -use std::io; use std::path::Path; +use std::path::PathBuf; use unicode_width::UnicodeWidthStr; fn normalize_agents_display_path(path: &Path) -> String { @@ -36,16 +33,8 @@ pub(crate) fn compose_model_display( (model_name.to_string(), details) } -pub(crate) async fn discover_agents_summary(config: &Config) -> io::Result { - let paths = discover_project_doc_paths(config, LOCAL_FS.as_ref()).await?; - Ok(compose_agents_summary(config, &paths)) -} - -pub(crate) fn compose_agents_summary(config: &Config, paths: &[AbsolutePathBuf]) -> String { +pub(crate) fn compose_agents_summary(config: &Config, paths: &[PathBuf]) -> String { let mut rels: Vec = Vec::new(); - if let Some(path) = config.user_instructions_path.as_deref() { - rels.push(format_directory_display(path, /*max_width*/ None)); - } for p in paths { let file_name = p @@ -53,14 +42,14 @@ pub(crate) fn compose_agents_summary(config: &Config, paths: &[AbsolutePathBuf]) .map(|name| name.to_string_lossy().to_string()) .unwrap_or_else(|| "".to_string()); let display = if let Some(parent) = p.parent() { - if parent.as_path() == config.cwd.as_path() { + if parent == config.cwd.as_path() { file_name.clone() } else { let mut cur = config.cwd.as_path(); let mut ups = 0usize; let mut reached = false; while let Some(c) = cur.parent() { - if cur == parent.as_path() { + if cur == parent { reached = true; break; } @@ -199,7 +188,6 @@ mod tests { use crate::legacy_core::LOCAL_PROJECT_DOC_FILENAME; use crate::legacy_core::config::ConfigBuilder; use pretty_assertions::assert_eq; - use std::fs; use tempfile::TempDir; async fn test_config(codex_home: &TempDir, cwd: &TempDir) -> Config { @@ -234,52 +222,43 @@ mod tests { } #[tokio::test] - async fn discover_agents_summary_includes_global_agents_path() { + async fn compose_agents_summary_includes_global_agents_path() { let codex_home = TempDir::new().expect("temp codex home"); let cwd = TempDir::new().expect("temp cwd"); let global_agents_path = codex_home.path().join(DEFAULT_PROJECT_DOC_FILENAME); - fs::write(&global_agents_path, "global instructions").expect("write global agents"); let config = test_config(&codex_home, &cwd).await; assert_eq!( - discover_agents_summary(&config).await.expect("summary"), + compose_agents_summary(&config, std::slice::from_ref(&global_agents_path)), format_directory_display(&global_agents_path, /*max_width*/ None) ); } #[tokio::test] - async fn discover_agents_summary_names_global_agents_override() { + async fn compose_agents_summary_names_global_agents_override() { let codex_home = TempDir::new().expect("temp codex home"); let cwd = TempDir::new().expect("temp cwd"); - fs::write( - codex_home.path().join(DEFAULT_PROJECT_DOC_FILENAME), - "global instructions", - ) - .expect("write global agents"); let override_path = codex_home.path().join(LOCAL_PROJECT_DOC_FILENAME); - fs::write(&override_path, "override instructions").expect("write global override"); let config = test_config(&codex_home, &cwd).await; assert_eq!( - discover_agents_summary(&config).await.expect("summary"), + compose_agents_summary(&config, std::slice::from_ref(&override_path)), format_directory_display(&override_path, /*max_width*/ None) ); } #[tokio::test] - async fn discover_agents_summary_orders_global_before_project_agents() { + async fn compose_agents_summary_orders_global_before_project_agents() { let codex_home = TempDir::new().expect("temp codex home"); let cwd = TempDir::new().expect("temp cwd"); let global_agents_path = codex_home.path().join(DEFAULT_PROJECT_DOC_FILENAME); - fs::write(&global_agents_path, "global instructions").expect("write global agents"); - fs::write( - cwd.path().join(DEFAULT_PROJECT_DOC_FILENAME), - "project instructions", - ) - .expect("write project agents"); + let project_agents_path = cwd.path().join(DEFAULT_PROJECT_DOC_FILENAME); let config = test_config(&codex_home, &cwd).await; - let summary = discover_agents_summary(&config).await.expect("summary"); + let summary = compose_agents_summary( + &config, + &[global_agents_path.clone(), project_agents_path.clone()], + ); let mut paths = summary.split(", "); assert_eq!( paths.next(), diff --git a/codex-rs/tui/src/status/mod.rs b/codex-rs/tui/src/status/mod.rs index 1781104efd..d618863384 100644 --- a/codex-rs/tui/src/status/mod.rs +++ b/codex-rs/tui/src/status/mod.rs @@ -19,7 +19,7 @@ pub(crate) use card::new_status_output; #[cfg(test)] pub(crate) use card::new_status_output_with_rate_limits; pub(crate) use card::new_status_output_with_rate_limits_handle; -pub(crate) use helpers::discover_agents_summary; +pub(crate) use helpers::compose_agents_summary; pub(crate) use helpers::format_directory_display; pub(crate) use helpers::format_tokens_compact; pub(crate) use helpers::plan_type_display_name; From 68a1d82a413630892b64258fd3e79786fc419312 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Sun, 12 Apr 2026 16:06:17 -0700 Subject: [PATCH 6/6] fix(mcp) pause timer for elicitations (#17566) ## Summary Stop counting elicitation time towards mcp tool call time. There are some tradeoffs here, but in general I don't think time spent waiting for elicitations should count towards tool call time, or at least not directly towards timeouts. Elicitations are not exactly like exec_command escalation requests, but I would argue it's ~roughly equivalent. ## Testing - [x] Added unit tests - [x] Tested locally --- .../src/elicitation_client_service.rs | 10 +- codex-rs/rmcp-client/src/rmcp_client.rs | 169 ++++++++++++++++-- 2 files changed, 165 insertions(+), 14 deletions(-) diff --git a/codex-rs/rmcp-client/src/elicitation_client_service.rs b/codex-rs/rmcp-client/src/elicitation_client_service.rs index 1d8fb954cc..49f11f0a76 100644 --- a/codex-rs/rmcp-client/src/elicitation_client_service.rs +++ b/codex-rs/rmcp-client/src/elicitation_client_service.rs @@ -17,6 +17,7 @@ use serde_json::Value; use crate::logging_client_handler::LoggingClientHandler; use crate::rmcp_client::Elicitation; +use crate::rmcp_client::ElicitationPauseState; use crate::rmcp_client::ElicitationResponse; use crate::rmcp_client::SendElicitation; @@ -26,10 +27,15 @@ const MCP_PROGRESS_TOKEN_META_KEY: &str = "progressToken"; pub(crate) struct ElicitationClientService { handler: LoggingClientHandler, send_elicitation: Arc, + pause_state: ElicitationPauseState, } impl ElicitationClientService { - pub(crate) fn new(client_info: ClientInfo, send_elicitation: SendElicitation) -> Self { + pub(crate) fn new( + client_info: ClientInfo, + send_elicitation: SendElicitation, + pause_state: ElicitationPauseState, + ) -> Self { let send_elicitation = Arc::new(send_elicitation); Self { handler: LoggingClientHandler::new( @@ -37,6 +43,7 @@ impl ElicitationClientService { clone_send_elicitation(Arc::clone(&send_elicitation)), ), send_elicitation, + pause_state, } } @@ -47,6 +54,7 @@ impl ElicitationClientService { ) -> Result { let RequestContext { id, meta, .. } = context; let request = restore_context_meta(request, meta); + let _pause = self.pause_state.enter(); (self.send_elicitation)(id, request) .await .map_err(|err| rmcp::ErrorData::internal_error(err.to_string(), None)) diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index 4f23526d11..415354fee4 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -1,11 +1,15 @@ use std::borrow::Cow; use std::collections::HashMap; use std::ffi::OsString; +use std::future::Future; use std::io; use std::path::PathBuf; use std::process::Stdio; use std::sync::Arc; +use std::sync::atomic::AtomicUsize; +use std::sync::atomic::Ordering; use std::time::Duration; +use std::time::Instant; use anyhow::Result; use anyhow::anyhow; @@ -63,6 +67,7 @@ use tokio::io::AsyncBufReadExt; use tokio::io::BufReader; use tokio::process::Command; use tokio::sync::Mutex; +use tokio::sync::watch; use tokio::time; use tracing::info; use tracing::warn; @@ -410,6 +415,93 @@ struct InitializeContext { client_service: ElicitationClientService, } +#[derive(Clone)] +pub(crate) struct ElicitationPauseState { + active_count: Arc, + paused: watch::Sender, +} + +impl ElicitationPauseState { + fn new() -> Self { + let (paused, _rx) = watch::channel(false); + Self { + active_count: Arc::new(AtomicUsize::new(0)), + paused, + } + } + + pub(crate) fn enter(&self) -> ElicitationPauseGuard { + if self.active_count.fetch_add(1, Ordering::AcqRel) == 0 { + self.paused.send_replace(true); + } + ElicitationPauseGuard { + pause_state: self.clone(), + } + } + + fn subscribe(&self) -> watch::Receiver { + self.paused.subscribe() + } +} + +pub(crate) struct ElicitationPauseGuard { + pause_state: ElicitationPauseState, +} + +impl Drop for ElicitationPauseGuard { + fn drop(&mut self) { + if self.pause_state.active_count.fetch_sub(1, Ordering::AcqRel) == 1 { + self.pause_state.paused.send_replace(false); + } + } +} + +async fn active_time_timeout( + duration: Duration, + mut pause_state: watch::Receiver, + operation: Fut, +) -> std::result::Result +where + Fut: Future, +{ + let mut remaining = duration; + tokio::pin!(operation); + + loop { + if *pause_state.borrow_and_update() { + tokio::select! { + result = &mut operation => return Ok(result), + changed = pause_state.changed() => { + if changed.is_err() { + return time::timeout(remaining, operation).await.map_err(|_| ()); + } + let _paused = *pause_state.borrow_and_update(); + } + } + continue; + } + + let active_start = Instant::now(); + tokio::select! { + result = &mut operation => return Ok(result), + _ = time::sleep(remaining) => { + return Err(()); + } + changed = pause_state.changed() => { + if changed.is_err() { + return time::timeout(remaining, operation).await.map_err(|_| ()); + } + if *pause_state.borrow_and_update() { + remaining = remaining.saturating_sub(active_start.elapsed()); + if remaining.is_zero() { + return Err(()); + } + } + } + } + } +} + #[derive(Debug, thiserror::Error)] enum ClientOperationError { #[error(transparent)] @@ -472,6 +564,7 @@ pub struct RmcpClient { transport_recipe: TransportRecipe, initialize_context: Mutex>, session_recovery_lock: Mutex<()>, + elicitation_pause_state: ElicitationPauseState, } impl RmcpClient { @@ -500,6 +593,7 @@ impl RmcpClient { transport_recipe, initialize_context: Mutex::new(None), session_recovery_lock: Mutex::new(()), + elicitation_pause_state: ElicitationPauseState::new(), }) } @@ -528,6 +622,7 @@ impl RmcpClient { transport_recipe, initialize_context: Mutex::new(None), session_recovery_lock: Mutex::new(()), + elicitation_pause_state: ElicitationPauseState::new(), }) } @@ -539,7 +634,11 @@ impl RmcpClient { timeout: Option, send_elicitation: SendElicitation, ) -> Result { - let client_service = ElicitationClientService::new(params.clone(), send_elicitation); + let client_service = ElicitationClientService::new( + params.clone(), + send_elicitation, + self.elicitation_pause_state.clone(), + ); let pending_transport = { let mut guard = self.state.lock().await; match &mut *guard { @@ -1052,16 +1151,28 @@ impl RmcpClient { Fut: std::future::Future>, { let service = self.service().await?; - match Self::run_service_operation_once(Arc::clone(&service), label, timeout, &operation) - .await + match Self::run_service_operation_once( + Arc::clone(&service), + label, + timeout, + self.elicitation_pause_state.clone(), + &operation, + ) + .await { Ok(result) => Ok(result), Err(error) if Self::is_session_expired_404(&error) => { self.reinitialize_after_session_expiry(&service).await?; let recovered_service = self.service().await?; - Self::run_service_operation_once(recovered_service, label, timeout, &operation) - .await - .map_err(Into::into) + Self::run_service_operation_once( + recovered_service, + label, + timeout, + self.elicitation_pause_state.clone(), + &operation, + ) + .await + .map_err(Into::into) } Err(error) => Err(error.into()), } @@ -1071,6 +1182,7 @@ impl RmcpClient { service: Arc>, label: &str, timeout: Option, + pause_state: ElicitationPauseState, operation: &F, ) -> std::result::Result where @@ -1078,13 +1190,15 @@ impl RmcpClient { Fut: std::future::Future>, { match timeout { - Some(duration) => time::timeout(duration, operation(service)) - .await - .map_err(|_| ClientOperationError::Timeout { - label: label.to_string(), - duration, - })? - .map_err(ClientOperationError::from), + Some(duration) => { + active_time_timeout(duration, pause_state.subscribe(), operation(service)) + .await + .map_err(|_| ClientOperationError::Timeout { + label: label.to_string(), + duration, + })? + .map_err(ClientOperationError::from) + } None => operation(service).await.map_err(ClientOperationError::from), } } @@ -1207,3 +1321,32 @@ async fn create_oauth_transport_and_runtime( Ok((transport, runtime)) } + +#[cfg(test)] +mod tests { + use std::time::Duration; + + use pretty_assertions::assert_eq; + use tokio::time; + + use super::*; + + #[tokio::test] + async fn active_time_timeout_pauses_while_elicitation_is_pending() { + let pause_state = ElicitationPauseState::new(); + let pause = pause_state.enter(); + tokio::spawn(async move { + time::sleep(Duration::from_millis(75)).await; + drop(pause); + }); + + let result = + active_time_timeout(Duration::from_millis(50), pause_state.subscribe(), async { + time::sleep(Duration::from_millis(90)).await; + "done" + }) + .await; + + assert_eq!(Ok("done"), result); + } +}