feedback

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);

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<crate::models::ContentItem>, 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"),
+        }
+    }
+}

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"

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 {

codex-rs/tui/src/app_event.rs

@@ -48,9 +48,4 @@ pub(crate) enum AppEvent {
     },
 
     InsertHistory(Vec<Line<'static>>),
-
-    /// 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,
 }

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();
     }

codex-rs/tui/src/status_indicator_widget.rs

@@ -42,9 +42,6 @@ pub(crate) struct StatusIndicatorWidget {
 
     frame_idx: Arc<AtomicUsize>,
     running: Arc<AtomicBool>,
-    /// 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);