Rescue chat completion changes (#1846)

https://github.com/openai/codex/pull/1835 has some messed up history.

This adds support for streaming chat completions, which is useful for ollama. We should probably take a very skeptical eye to the code introduced in this PR.

---------

Co-authored-by: Ahmed Ibrahim <aibrahim@openai.com>
This commit is contained in:
easong-openai
2025-08-05 01:56:13 -07:00
committed by GitHub
parent d31e149cb1
commit e0303dbac0
13 changed files with 547 additions and 57 deletions

View File

@@ -138,6 +138,11 @@ impl BottomPane<'_> {
view.handle_key_event(self, key_event);
if !view.is_complete() {
self.active_view = Some(view);
} else if self.is_task_running {
let mut v = StatusIndicatorView::new(self.app_event_tx.clone());
v.update_text("waiting for model".to_string());
self.active_view = Some(Box::new(v));
self.status_view_active = true;
}
self.request_redraw();
InputResult::None
@@ -163,6 +168,12 @@ impl BottomPane<'_> {
CancellationEvent::Handled => {
if !view.is_complete() {
self.active_view = Some(view);
} else if self.is_task_running {
// Modal aborted but task still running restore status indicator.
let mut v = StatusIndicatorView::new(self.app_event_tx.clone());
v.update_text("waiting for model".to_string());
self.active_view = Some(Box::new(v));
self.status_view_active = true;
}
self.show_ctrl_c_quit_hint();
}
@@ -202,15 +213,20 @@ impl BottomPane<'_> {
handled_by_view = true;
}
// Fallback: if the current active view did not consume status updates,
// present an overlay above the composer.
if !handled_by_view {
// Fallback: if the current active view did not consume status updates
// and no modal view is active, present an overlay above the composer.
// If a modal is active, do NOT render the overlay to avoid drawing
// over the dialog.
if !handled_by_view && self.active_view.is_none() {
if self.live_status.is_none() {
self.live_status = Some(StatusIndicatorWidget::new(self.app_event_tx.clone()));
}
if let Some(status) = &mut self.live_status {
status.update_text(text);
}
} else if !handled_by_view {
// Ensure any previous overlay is cleared when a modal becomes active.
self.live_status = None;
}
self.request_redraw();
}
@@ -296,6 +312,8 @@ impl BottomPane<'_> {
// Otherwise create a new approval modal overlay.
let modal = ApprovalModalView::new(request, self.app_event_tx.clone());
self.active_view = Some(Box::new(modal));
// Hide any overlay status while a modal is visible.
self.live_status = None;
self.status_view_active = false;
self.request_redraw()
}
@@ -368,16 +386,18 @@ impl WidgetRef for &BottomPane<'_> {
y_offset = y_offset.saturating_add(1);
}
if let Some(status) = &self.live_status {
let live_h = status.desired_height(area.width).min(area.height);
let live_h = status
.desired_height(area.width)
.min(area.height.saturating_sub(y_offset));
if live_h > 0 {
let live_rect = Rect {
x: area.x,
y: area.y,
y: area.y + y_offset,
width: area.width,
height: live_h,
};
status.render_ref(live_rect, buf);
y_offset = live_h;
y_offset = y_offset.saturating_add(live_h);
}
}
@@ -540,6 +560,122 @@ mod tests {
);
}
#[test]
fn overlay_not_shown_above_approval_modal() {
let (tx_raw, _rx) = channel::<AppEvent>();
let tx = AppEventSender::new(tx_raw);
let mut pane = BottomPane::new(BottomPaneParams {
app_event_tx: tx,
has_input_focus: true,
enhanced_keys_supported: false,
});
// Create an approval modal (active view).
pane.push_approval_request(exec_request());
// Attempt to update status; this should NOT create an overlay while modal is visible.
pane.update_status_text("running command".to_string());
// Render and verify the top row does not include the Working header overlay.
let area = Rect::new(0, 0, 60, 6);
let mut buf = Buffer::empty(area);
(&pane).render_ref(area, &mut buf);
let mut r0 = String::new();
for x in 0..area.width {
r0.push(buf[(x, 0)].symbol().chars().next().unwrap_or(' '));
}
assert!(
!r0.contains("Working"),
"overlay Working header should not render above modal"
);
}
#[test]
fn composer_not_shown_after_denied_if_task_running() {
let (tx_raw, rx) = channel::<AppEvent>();
let tx = AppEventSender::new(tx_raw);
let mut pane = BottomPane::new(BottomPaneParams {
app_event_tx: tx.clone(),
has_input_focus: true,
enhanced_keys_supported: false,
});
// Start a running task so the status indicator replaces the composer.
pane.set_task_running(true);
pane.update_status_text("waiting for model".to_string());
// Push an approval modal (e.g., command approval) which should hide the status view.
pane.push_approval_request(exec_request());
// Simulate pressing 'n' (deny) on the modal.
use crossterm::event::KeyCode;
use crossterm::event::KeyEvent;
use crossterm::event::KeyModifiers;
pane.handle_key_event(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::NONE));
// After denial, since the task is still running, the status indicator
// should be restored as the active view; the composer should NOT be visible.
assert!(
pane.status_view_active,
"status view should be active after denial"
);
assert!(pane.active_view.is_some(), "active view should be present");
// Render and ensure the top row includes the Working header instead of the composer.
// Give the animation thread a moment to tick.
std::thread::sleep(std::time::Duration::from_millis(120));
let area = Rect::new(0, 0, 40, 3);
let mut buf = Buffer::empty(area);
(&pane).render_ref(area, &mut buf);
let mut row0 = String::new();
for x in 0..area.width {
row0.push(buf[(x, 0)].symbol().chars().next().unwrap_or(' '));
}
assert!(
row0.contains("Working"),
"expected Working header after denial: {row0:?}"
);
// Drain the channel to avoid unused warnings.
drop(rx);
}
#[test]
fn status_indicator_visible_during_command_execution() {
let (tx_raw, _rx) = channel::<AppEvent>();
let tx = AppEventSender::new(tx_raw);
let mut pane = BottomPane::new(BottomPaneParams {
app_event_tx: tx,
has_input_focus: true,
enhanced_keys_supported: false,
});
// Begin a task: show initial status.
pane.set_task_running(true);
pane.update_status_text("waiting for model".to_string());
// As a long-running command begins (post-approval), ensure the status
// indicator is visible while we wait for the command to run.
pane.update_status_text("running command".to_string());
// Allow some frames so the animation thread ticks.
std::thread::sleep(std::time::Duration::from_millis(120));
// Render and confirm the line contains the "Working" header.
let area = Rect::new(0, 0, 40, 3);
let mut buf = Buffer::empty(area);
(&pane).render_ref(area, &mut buf);
let mut row0 = String::new();
for x in 0..area.width {
row0.push(buf[(x, 0)].symbol().chars().next().unwrap_or(' '));
}
assert!(
row0.contains("Working"),
"expected Working header: {row0:?}"
);
}
#[test]
fn bottom_padding_present_for_status_view() {
let (tx_raw, _rx) = channel::<AppEvent>();

View File

@@ -9,6 +9,8 @@ use codex_core::protocol::AgentMessageDeltaEvent;
use codex_core::protocol::AgentMessageEvent;
use codex_core::protocol::AgentReasoningDeltaEvent;
use codex_core::protocol::AgentReasoningEvent;
use codex_core::protocol::AgentReasoningRawContentDeltaEvent;
use codex_core::protocol::AgentReasoningRawContentEvent;
use codex_core::protocol::ApplyPatchApprovalRequestEvent;
use codex_core::protocol::ErrorEvent;
use codex_core::protocol::Event;
@@ -61,6 +63,7 @@ pub(crate) struct ChatWidget<'a> {
initial_user_message: Option<UserMessage>,
token_usage: TokenUsage,
reasoning_buffer: String,
content_buffer: String,
// Buffer for streaming assistant answer text; we do not surface partial
// We wait for the final AgentMessage event and then emit the full text
// at once into scrollback so the history contains a single message.
@@ -101,6 +104,24 @@ fn create_initial_user_message(text: String, image_paths: Vec<PathBuf>) -> Optio
}
impl ChatWidget<'_> {
fn emit_stream_header(&mut self, kind: StreamKind) {
use ratatui::text::Line as RLine;
if self.stream_header_emitted {
return;
}
let header = match kind {
StreamKind::Reasoning => RLine::from("thinking".magenta().italic()),
StreamKind::Answer => RLine::from("codex".magenta().bold()),
};
self.app_event_tx
.send(AppEvent::InsertHistory(vec![header]));
self.stream_header_emitted = true;
}
fn finalize_active_stream(&mut self) {
if let Some(kind) = self.current_stream {
self.finalize_stream(kind);
}
}
pub(crate) fn new(
config: Config,
app_event_tx: AppEventSender,
@@ -161,6 +182,7 @@ impl ChatWidget<'_> {
),
token_usage: TokenUsage::default(),
reasoning_buffer: String::new(),
content_buffer: String::new(),
answer_buffer: String::new(),
running_commands: HashMap::new(),
live_builder: RowBuilder::new(80),
@@ -276,6 +298,20 @@ impl ChatWidget<'_> {
self.finalize_stream(StreamKind::Reasoning);
self.request_redraw();
}
EventMsg::AgentReasoningRawContentDelta(AgentReasoningRawContentDeltaEvent {
delta,
}) => {
// Treat raw reasoning content the same as summarized reasoning for UI flow.
self.begin_stream(StreamKind::Reasoning);
self.reasoning_buffer.push_str(&delta);
self.stream_push_and_maybe_commit(&delta);
self.request_redraw();
}
EventMsg::AgentReasoningRawContent(AgentReasoningRawContentEvent { text: _ }) => {
// Finalize the raw reasoning stream just like the summarized reasoning event.
self.finalize_stream(StreamKind::Reasoning);
self.request_redraw();
}
EventMsg::TaskStarted => {
self.bottom_pane.clear_ctrl_c_quit_hint();
self.bottom_pane.set_task_running(true);
@@ -299,6 +335,14 @@ impl ChatWidget<'_> {
EventMsg::Error(ErrorEvent { message }) => {
self.add_to_history(HistoryCell::new_error_event(message.clone()));
self.bottom_pane.set_task_running(false);
self.bottom_pane.clear_live_ring();
self.live_builder = RowBuilder::new(self.live_builder.width());
self.current_stream = None;
self.stream_header_emitted = false;
self.answer_buffer.clear();
self.reasoning_buffer.clear();
self.content_buffer.clear();
self.request_redraw();
}
EventMsg::PlanUpdate(update) => {
// Commit plan updates directly to history (no status-line preview).
@@ -310,6 +354,7 @@ impl ChatWidget<'_> {
cwd,
reason,
}) => {
self.finalize_active_stream();
// Log a background summary immediately so the history is chronological.
let cmdline = strip_bash_lc_and_escape(&command);
let text = format!(
@@ -336,6 +381,7 @@ impl ChatWidget<'_> {
reason,
grant_root,
}) => {
self.finalize_active_stream();
// ------------------------------------------------------------------
// Before we even prompt the user for approval we surface the patch
// summary in the main conversation so that the dialog appears in a
@@ -365,6 +411,10 @@ impl ChatWidget<'_> {
command,
cwd,
}) => {
self.finalize_active_stream();
// Ensure the status indicator is visible while the command runs.
self.bottom_pane
.update_status_text("running command".to_string());
self.running_commands.insert(
call_id,
RunningCommand {
@@ -408,6 +458,7 @@ impl ChatWidget<'_> {
call_id: _,
invocation,
}) => {
self.finalize_active_stream();
self.add_to_history(HistoryCell::new_active_mcp_tool_call(invocation));
}
EventMsg::McpToolCallEnd(McpToolCallEndEvent {
@@ -451,7 +502,9 @@ impl ChatWidget<'_> {
/// Update the live log preview while a task is running.
pub(crate) fn update_latest_log(&mut self, line: String) {
self.bottom_pane.update_status_text(line);
if self.bottom_pane.is_task_running() {
self.bottom_pane.update_status_text(line);
}
}
fn request_redraw(&mut self) {
@@ -478,8 +531,15 @@ impl ChatWidget<'_> {
if self.bottom_pane.is_task_running() {
self.bottom_pane.clear_ctrl_c_quit_hint();
self.submit_op(Op::Interrupt);
self.bottom_pane.set_task_running(false);
self.bottom_pane.clear_live_ring();
self.live_builder = RowBuilder::new(self.live_builder.width());
self.current_stream = None;
self.stream_header_emitted = false;
self.answer_buffer.clear();
self.reasoning_buffer.clear();
self.content_buffer.clear();
self.request_redraw();
CancellationEvent::Ignored
} else if self.bottom_pane.ctrl_c_quit_hint_visible() {
self.submit_op(Op::Shutdown);
@@ -518,6 +578,12 @@ impl ChatWidget<'_> {
impl ChatWidget<'_> {
fn begin_stream(&mut self, kind: StreamKind) {
if let Some(current) = self.current_stream {
if current != kind {
self.finalize_stream(current);
}
}
if self.current_stream != Some(kind) {
self.current_stream = Some(kind);
self.stream_header_emitted = false;
@@ -526,6 +592,7 @@ impl ChatWidget<'_> {
// Ensure the waiting status is visible (composer replaced).
self.bottom_pane
.update_status_text("waiting for model".to_string());
self.emit_stream_header(kind);
}
}