From 10df580b772ca1f24407dccaa30f4fd418bdcd67 Mon Sep 17 00:00:00 2001 From: easong-openai Date: Mon, 4 Aug 2025 17:33:59 -0700 Subject: [PATCH] feedback --- codex-rs/core/src/codex.rs | 11 --- codex-rs/core/src/conversation_history.rs | 101 +++++++++++++++++++- codex-rs/tui/Cargo.toml | 4 +- codex-rs/tui/src/app.rs | 1 - codex-rs/tui/src/app_event.rs | 5 - codex-rs/tui/src/bottom_pane/mod.rs | 36 +++---- codex-rs/tui/src/status_indicator_widget.rs | 21 +--- 7 files changed, 123 insertions(+), 56 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 7acfe16af9..8d24356460 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -122,15 +122,6 @@ impl Codex { // experimental resume path (undocumented) let resume_path = config.experimental_resume.clone(); info!("resume_path: {resume_path:?}"); - // Use a bounded channel for submissions to retain backpressure on the - // producer of Ops, but avoid blocking the agent on event delivery to - // the UI. If the event queue fills and is not drained (e.g., if the - // UI forwarder task exits unexpectedly), a bounded channel here would - // cause the agent to block inside `send(event).await`, which in turn - // prevents it from handling interrupts (Ctrl-C) or any further Ops. - // An unbounded channel for events ensures the agent never deadlocks on - // UI delivery; the UI remains responsible for rendering/consuming at - // its own pace. let (tx_sub, rx_sub) = async_channel::bounded(64); let (tx_event, rx_event) = async_channel::unbounded(); @@ -1383,8 +1374,6 @@ async fn try_run_turn( return Ok(output); } ResponseEvent::OutputTextDelta(delta) => { - // Stream assistant text into in-memory conversation history so - // subsequent turns (e.g. tool calls) see the partial message. { let mut st = sess.state.lock().unwrap(); st.history.append_assistant_text(&delta); diff --git a/codex-rs/core/src/conversation_history.rs b/codex-rs/core/src/conversation_history.rs index e6277d1093..c1b13f388a 100644 --- a/codex-rs/core/src/conversation_history.rs +++ b/codex-rs/core/src/conversation_history.rs @@ -48,7 +48,6 @@ impl ConversationHistory { append_text_content(last_content, new_content); } _ => { - // Note agent-loop.ts also does filtering on some of the fields. self.items.push(item.clone()); } } @@ -144,3 +143,103 @@ fn append_text_delta(content: &mut Vec, delta: &str) }); } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::models::ContentItem; + + fn assistant_msg(text: &str) -> ResponseItem { + ResponseItem::Message { + id: None, + role: "assistant".to_string(), + content: vec![ContentItem::OutputText { + text: text.to_string(), + }], + } + } + + fn user_msg(text: &str) -> ResponseItem { + ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ContentItem::OutputText { + text: text.to_string(), + }], + } + } + + #[test] + fn merges_adjacent_assistant_messages() { + let mut h = ConversationHistory::default(); + let a1 = assistant_msg("Hello"); + let a2 = assistant_msg(", world!"); + h.record_items([&a1, &a2]); + + let items = h.contents(); + assert_eq!(items.len(), 1, "adjacent assistant messages should merge"); + if let ResponseItem::Message { role, content, .. } = &items[0] { + assert_eq!(role, "assistant"); + let text = match &content[0] { + ContentItem::OutputText { text } => text, + _ => panic!("expected OutputText"), + }; + assert_eq!(text, "Hello, world!"); + } else { + panic!("expected Message"); + } + } + + #[test] + fn append_assistant_text_creates_and_appends() { + let mut h = ConversationHistory::default(); + h.append_assistant_text("Hello"); + h.append_assistant_text(", world"); + + // Now record a final full assistant message and verify it merges. + let final_msg = assistant_msg("!"); + h.record_items([&final_msg]); + + let items = h.contents(); + assert_eq!(items.len(), 1); + if let ResponseItem::Message { role, content, .. } = &items[0] { + assert_eq!(role, "assistant"); + let text = match &content[0] { + ContentItem::OutputText { text } => text, + _ => panic!("expected OutputText"), + }; + assert_eq!(text, "Hello, world!"); + } else { + panic!("expected Message"); + } + } + + #[test] + fn filters_non_api_messages() { + let mut h = ConversationHistory::default(); + // System message is not an API message; Other is ignored. + let system = ResponseItem::Message { + id: None, + role: "system".to_string(), + content: vec![ContentItem::OutputText { + text: "ignored".to_string(), + }], + }; + h.record_items([&system, &ResponseItem::Other]); + + // User and assistant should be retained. + let u = user_msg("hi"); + let a = assistant_msg("hello"); + h.record_items([&u, &a]); + + let items = h.contents(); + assert_eq!(items.len(), 2); + match (&items[0], &items[1]) { + (ResponseItem::Message { role: r0, .. }, ResponseItem::Message { role: r1, .. }) => { + assert_eq!(r0, "user"); + assert_eq!(r1, "assistant"); + } + _ => panic!("expected two Message items"), + } + } +} diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 041679d6cf..60af056a2d 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -13,7 +13,7 @@ path = "src/lib.rs" [features] # Enable vt100-based tests (emulator) when running with `--features vt100-tests`. -vt100-tests = ["dep:vt100"] +vt100-tests = [] [lints] workspace = true @@ -69,7 +69,6 @@ tui-markdown = "0.3.3" unicode-segmentation = "1.12.0" unicode-width = "0.1" uuid = "1" -vt100 = { version = "0.16.2", optional = true } @@ -78,3 +77,4 @@ insta = "1.43.1" pretty_assertions = "1" rand = "0.8" chrono = { version = "0.4", features = ["serde"] } +vt100 = "0.16.2" diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 39154ded00..1142bd87fc 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -220,7 +220,6 @@ impl App<'_> { AppEvent::Redraw => { std::io::stdout().sync_update(|_| self.draw_next_frame(terminal))??; } - AppEvent::LiveStatusRevealComplete => {} AppEvent::KeyEvent(key_event) => { match key_event { KeyEvent { diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index c9517b59d4..77a600d304 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -48,9 +48,4 @@ pub(crate) enum AppEvent { }, InsertHistory(Vec>), - - /// Emitted by the live status widget when the current text has been fully - /// revealed by the typewriter animation. The app uses this signal to - /// commit the live cell to history and advance to the next entry. - LiveStatusRevealComplete, } diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 66b4b3b7e0..3ecf12b7a7 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -186,32 +186,34 @@ impl BottomPane<'_> { /// the StatusIndicatorView so the input pane shows a single-line status /// like: `▌ Working [·] waiting for model`. pub(crate) fn update_status_text(&mut self, text: String) { + let mut handled_by_view = false; if let Some(view) = self.active_view.as_mut() { - match view.update_status_text(text.clone()) { - bottom_pane_view::ConditionalUpdate::NeedsRedraw => { - self.request_redraw(); - return; - } - bottom_pane_view::ConditionalUpdate::NoRedraw => {} + if matches!( + view.update_status_text(text.clone()), + bottom_pane_view::ConditionalUpdate::NeedsRedraw + ) { + handled_by_view = true; } } else { let mut v = StatusIndicatorView::new(self.app_event_tx.clone()); - v.update_text(text); + v.update_text(text.clone()); self.active_view = Some(Box::new(v)); self.status_view_active = true; - self.request_redraw(); - return; + handled_by_view = true; } - // Fallback: if the current active view does not consume status updates, + // Fallback: if the current active view did not consume status updates, // present an overlay above the composer. - if self.live_status.is_none() { - self.live_status = Some(crate::status_indicator_widget::StatusIndicatorWidget::new( - self.app_event_tx.clone(), - )); - } - if let Some(status) = &mut self.live_status { - status.update_text(text); + if !handled_by_view { + if self.live_status.is_none() { + self.live_status = + Some(crate::status_indicator_widget::StatusIndicatorWidget::new( + self.app_event_tx.clone(), + )); + } + if let Some(status) = &mut self.live_status { + status.update_text(text); + } } self.request_redraw(); } diff --git a/codex-rs/tui/src/status_indicator_widget.rs b/codex-rs/tui/src/status_indicator_widget.rs index 91f6a666a5..c8dc7761f8 100644 --- a/codex-rs/tui/src/status_indicator_widget.rs +++ b/codex-rs/tui/src/status_indicator_widget.rs @@ -42,9 +42,6 @@ pub(crate) struct StatusIndicatorWidget { frame_idx: Arc, running: Arc, - /// Ensure we only notify the app once per target text when the full - /// reveal completes. - completion_sent: AtomicBool, // Keep one sender alive to prevent the channel from closing while the // animation thread is still running. The field itself is currently not // accessed anywhere, therefore the leading underscore silences the @@ -81,7 +78,7 @@ impl StatusIndicatorWidget { reveal_len_at_base: 0, frame_idx, running, - completion_sent: AtomicBool::new(false), + _app_event_tx: app_event_tx, } } @@ -118,7 +115,6 @@ impl StatusIndicatorWidget { self.last_target_len = new_len; self.base_frame = current_frame; self.reveal_len_at_base = shown_now.min(new_len); - self.completion_sent.store(false, Ordering::Relaxed); } /// Reset the animation and start revealing `text` from the beginning. @@ -142,7 +138,6 @@ impl StatusIndicatorWidget { self.base_frame = current_frame; // Start from zero revealed characters for a fresh typewriter cycle. self.reveal_len_at_base = 0; - self.completion_sent.store(false, Ordering::Relaxed); } /// Calculate how many characters should currently be visible given the @@ -254,19 +249,7 @@ impl WidgetRef for StatusIndicatorWidget { } let lines = vec![Line::from(acc)]; - // If the animation for the current target has just finished, notify the app - // so it can commit the cell to history and advance. - { - let current_frame = self.frame_idx.load(std::sync::atomic::Ordering::Relaxed); - let shown = self.current_shown_len(current_frame); - if self.last_target_len > 0 - && shown >= self.last_target_len - && !self.completion_sent.swap(true, Ordering::Relaxed) - { - self._app_event_tx - .send(crate::app_event::AppEvent::LiveStatusRevealComplete); - } - } + // No-op once full text is revealed; the app no longer reacts to a completion event. let paragraph = Paragraph::new(lines); paragraph.render_ref(area, buf);