Parse reasoning text content (#2277)
Sometimes COT is returns as text content instead of `ReasoningText`. We should parse it but not serialize back on requests. --------- Co-authored-by: Ahmed Ibrahim <aibrahim@openai.com>
This commit is contained in:
@@ -1606,6 +1606,7 @@ async fn handle_response_item(
|
|||||||
for item in content {
|
for item in content {
|
||||||
let text = match item {
|
let text = match item {
|
||||||
ReasoningItemContent::ReasoningText { text } => text,
|
ReasoningItemContent::ReasoningText { text } => text,
|
||||||
|
ReasoningItemContent::Text { text } => text,
|
||||||
};
|
};
|
||||||
let event = Event {
|
let event = Event {
|
||||||
id: sub_id.to_string(),
|
id: sub_id.to_string(),
|
||||||
|
|||||||
@@ -45,7 +45,7 @@ pub enum ResponseItem {
|
|||||||
Reasoning {
|
Reasoning {
|
||||||
id: String,
|
id: String,
|
||||||
summary: Vec<ReasoningItemReasoningSummary>,
|
summary: Vec<ReasoningItemReasoningSummary>,
|
||||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
#[serde(default, skip_serializing_if = "should_serialize_reasoning_content")]
|
||||||
content: Option<Vec<ReasoningItemContent>>,
|
content: Option<Vec<ReasoningItemContent>>,
|
||||||
encrypted_content: Option<String>,
|
encrypted_content: Option<String>,
|
||||||
},
|
},
|
||||||
@@ -81,6 +81,15 @@ pub enum ResponseItem {
|
|||||||
Other,
|
Other,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn should_serialize_reasoning_content(content: &Option<Vec<ReasoningItemContent>>) -> bool {
|
||||||
|
match content {
|
||||||
|
Some(content) => !content
|
||||||
|
.iter()
|
||||||
|
.any(|c| matches!(c, ReasoningItemContent::ReasoningText { .. })),
|
||||||
|
None => false,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
impl From<ResponseInputItem> for ResponseItem {
|
impl From<ResponseInputItem> for ResponseItem {
|
||||||
fn from(item: ResponseInputItem) -> Self {
|
fn from(item: ResponseInputItem) -> Self {
|
||||||
match item {
|
match item {
|
||||||
@@ -142,6 +151,7 @@ pub enum ReasoningItemReasoningSummary {
|
|||||||
#[serde(tag = "type", rename_all = "snake_case")]
|
#[serde(tag = "type", rename_all = "snake_case")]
|
||||||
pub enum ReasoningItemContent {
|
pub enum ReasoningItemContent {
|
||||||
ReasoningText { text: String },
|
ReasoningText { text: String },
|
||||||
|
Text { text: String },
|
||||||
}
|
}
|
||||||
|
|
||||||
impl From<Vec<InputItem>> for ResponseInputItem {
|
impl From<Vec<InputItem>> for ResponseInputItem {
|
||||||
|
|||||||
@@ -133,6 +133,7 @@ impl ChatWidget<'_> {
|
|||||||
fn on_agent_message(&mut self, message: String) {
|
fn on_agent_message(&mut self, message: String) {
|
||||||
let sink = AppEventHistorySink(self.app_event_tx.clone());
|
let sink = AppEventHistorySink(self.app_event_tx.clone());
|
||||||
let finished = self.stream.apply_final_answer(&message, &sink);
|
let finished = self.stream.apply_final_answer(&message, &sink);
|
||||||
|
self.last_stream_kind = Some(StreamKind::Answer);
|
||||||
self.handle_if_stream_finished(finished);
|
self.handle_if_stream_finished(finished);
|
||||||
self.mark_needs_redraw();
|
self.mark_needs_redraw();
|
||||||
}
|
}
|
||||||
@@ -145,9 +146,10 @@ impl ChatWidget<'_> {
|
|||||||
self.handle_streaming_delta(StreamKind::Reasoning, delta);
|
self.handle_streaming_delta(StreamKind::Reasoning, delta);
|
||||||
}
|
}
|
||||||
|
|
||||||
fn on_agent_reasoning_final(&mut self) {
|
fn on_agent_reasoning_final(&mut self, text: String) {
|
||||||
let sink = AppEventHistorySink(self.app_event_tx.clone());
|
let sink = AppEventHistorySink(self.app_event_tx.clone());
|
||||||
let finished = self.stream.finalize(StreamKind::Reasoning, false, &sink);
|
let finished = self.stream.apply_final_reasoning(&text, &sink);
|
||||||
|
self.last_stream_kind = Some(StreamKind::Reasoning);
|
||||||
self.handle_if_stream_finished(finished);
|
self.handle_if_stream_finished(finished);
|
||||||
self.mark_needs_redraw();
|
self.mark_needs_redraw();
|
||||||
}
|
}
|
||||||
@@ -633,9 +635,9 @@ impl ChatWidget<'_> {
|
|||||||
| EventMsg::AgentReasoningRawContentDelta(AgentReasoningRawContentDeltaEvent {
|
| EventMsg::AgentReasoningRawContentDelta(AgentReasoningRawContentDeltaEvent {
|
||||||
delta,
|
delta,
|
||||||
}) => self.on_agent_reasoning_delta(delta),
|
}) => self.on_agent_reasoning_delta(delta),
|
||||||
EventMsg::AgentReasoning(AgentReasoningEvent { .. })
|
EventMsg::AgentReasoning(AgentReasoningEvent { text })
|
||||||
| EventMsg::AgentReasoningRawContent(AgentReasoningRawContentEvent { .. }) => {
|
| EventMsg::AgentReasoningRawContent(AgentReasoningRawContentEvent { text }) => {
|
||||||
self.on_agent_reasoning_final()
|
self.on_agent_reasoning_final(text)
|
||||||
}
|
}
|
||||||
EventMsg::AgentReasoningSectionBreak(_) => self.on_reasoning_section_break(),
|
EventMsg::AgentReasoningSectionBreak(_) => self.on_reasoning_section_break(),
|
||||||
EventMsg::TaskStarted => self.on_task_started(),
|
EventMsg::TaskStarted => self.on_task_started(),
|
||||||
|
|||||||
@@ -0,0 +1,10 @@
|
|||||||
|
---
|
||||||
|
source: tui/src/chatwidget/tests.rs
|
||||||
|
assertion_line: 886
|
||||||
|
expression: combined
|
||||||
|
---
|
||||||
|
thinking
|
||||||
|
I will first analyze the request.
|
||||||
|
|
||||||
|
codex
|
||||||
|
Here is the result.
|
||||||
@@ -0,0 +1,9 @@
|
|||||||
|
---
|
||||||
|
source: tui/src/chatwidget/tests.rs
|
||||||
|
expression: combined
|
||||||
|
---
|
||||||
|
thinking
|
||||||
|
I will first analyze the request.
|
||||||
|
|
||||||
|
codex
|
||||||
|
Here is the result.
|
||||||
@@ -12,6 +12,7 @@ use codex_core::plan_tool::UpdatePlanArgs;
|
|||||||
use codex_core::protocol::AgentMessageDeltaEvent;
|
use codex_core::protocol::AgentMessageDeltaEvent;
|
||||||
use codex_core::protocol::AgentMessageEvent;
|
use codex_core::protocol::AgentMessageEvent;
|
||||||
use codex_core::protocol::AgentReasoningDeltaEvent;
|
use codex_core::protocol::AgentReasoningDeltaEvent;
|
||||||
|
use codex_core::protocol::AgentReasoningEvent;
|
||||||
use codex_core::protocol::ApplyPatchApprovalRequestEvent;
|
use codex_core::protocol::ApplyPatchApprovalRequestEvent;
|
||||||
use codex_core::protocol::Event;
|
use codex_core::protocol::Event;
|
||||||
use codex_core::protocol::EventMsg;
|
use codex_core::protocol::EventMsg;
|
||||||
@@ -24,6 +25,7 @@ use codex_core::protocol::TaskCompleteEvent;
|
|||||||
use crossterm::event::KeyCode;
|
use crossterm::event::KeyCode;
|
||||||
use crossterm::event::KeyEvent;
|
use crossterm::event::KeyEvent;
|
||||||
use crossterm::event::KeyModifiers;
|
use crossterm::event::KeyModifiers;
|
||||||
|
use insta::assert_snapshot;
|
||||||
use pretty_assertions::assert_eq;
|
use pretty_assertions::assert_eq;
|
||||||
use std::fs::File;
|
use std::fs::File;
|
||||||
use std::io::BufRead;
|
use std::io::BufRead;
|
||||||
@@ -413,46 +415,6 @@ async fn binary_size_transcript_matches_ideal_fixture() {
|
|||||||
assert_eq!(visible_after, ideal);
|
assert_eq!(visible_after, ideal);
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
|
||||||
fn final_longer_answer_after_single_char_delta_is_complete() {
|
|
||||||
let (mut chat, rx, _op_rx) = make_chatwidget_manual();
|
|
||||||
|
|
||||||
// Simulate a stray delta without newline (e.g., punctuation).
|
|
||||||
chat.handle_codex_event(Event {
|
|
||||||
id: "sub-x".into(),
|
|
||||||
msg: EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { delta: "?".into() }),
|
|
||||||
});
|
|
||||||
|
|
||||||
// Now send the full final answer with no newline.
|
|
||||||
let full = "Hi! How can I help with codex-rs today? Want me to explore the repo, run tests, or work on a specific change?";
|
|
||||||
chat.handle_codex_event(Event {
|
|
||||||
id: "sub-x".into(),
|
|
||||||
msg: EventMsg::AgentMessage(AgentMessageEvent {
|
|
||||||
message: full.into(),
|
|
||||||
}),
|
|
||||||
});
|
|
||||||
|
|
||||||
// Drain and assert the full message appears in history.
|
|
||||||
let cells = drain_insert_history(&rx);
|
|
||||||
let mut found = false;
|
|
||||||
for lines in &cells {
|
|
||||||
let s = lines
|
|
||||||
.iter()
|
|
||||||
.flat_map(|l| l.spans.iter())
|
|
||||||
.map(|sp| sp.content.clone())
|
|
||||||
.collect::<String>();
|
|
||||||
if s.contains(full) {
|
|
||||||
found = true;
|
|
||||||
break;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
assert!(
|
|
||||||
found,
|
|
||||||
"expected full final message to be flushed to history, cells={:?}",
|
|
||||||
cells.len()
|
|
||||||
);
|
|
||||||
}
|
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn apply_patch_events_emit_history_cells() {
|
fn apply_patch_events_emit_history_cells() {
|
||||||
let (mut chat, rx, _op_rx) = make_chatwidget_manual();
|
let (mut chat, rx, _op_rx) = make_chatwidget_manual();
|
||||||
@@ -923,3 +885,91 @@ fn multiple_agent_messages_in_single_turn_emit_multiple_headers() {
|
|||||||
let second_idx = combined.find("Second message").unwrap();
|
let second_idx = combined.find("Second message").unwrap();
|
||||||
assert!(first_idx < second_idx, "messages out of order: {combined}");
|
assert!(first_idx < second_idx, "messages out of order: {combined}");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn final_reasoning_then_message_without_deltas_are_rendered() {
|
||||||
|
let (mut chat, rx, _op_rx) = make_chatwidget_manual();
|
||||||
|
|
||||||
|
// No deltas; only final reasoning followed by final message.
|
||||||
|
chat.handle_codex_event(Event {
|
||||||
|
id: "s1".into(),
|
||||||
|
msg: EventMsg::AgentReasoning(AgentReasoningEvent {
|
||||||
|
text: "I will first analyze the request.".into(),
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
chat.handle_codex_event(Event {
|
||||||
|
id: "s1".into(),
|
||||||
|
msg: EventMsg::AgentMessage(AgentMessageEvent {
|
||||||
|
message: "Here is the result.".into(),
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
|
||||||
|
// Drain history and snapshot the combined visible content.
|
||||||
|
let cells = drain_insert_history(&rx);
|
||||||
|
let combined = cells
|
||||||
|
.iter()
|
||||||
|
.map(|lines| lines_to_single_string(lines))
|
||||||
|
.collect::<String>();
|
||||||
|
assert_snapshot!(combined);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn deltas_then_same_final_message_are_rendered_snapshot() {
|
||||||
|
let (mut chat, rx, _op_rx) = make_chatwidget_manual();
|
||||||
|
|
||||||
|
// Stream some reasoning deltas first.
|
||||||
|
chat.handle_codex_event(Event {
|
||||||
|
id: "s1".into(),
|
||||||
|
msg: EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent {
|
||||||
|
delta: "I will ".into(),
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
chat.handle_codex_event(Event {
|
||||||
|
id: "s1".into(),
|
||||||
|
msg: EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent {
|
||||||
|
delta: "first analyze the ".into(),
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
chat.handle_codex_event(Event {
|
||||||
|
id: "s1".into(),
|
||||||
|
msg: EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent {
|
||||||
|
delta: "request.".into(),
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
chat.handle_codex_event(Event {
|
||||||
|
id: "s1".into(),
|
||||||
|
msg: EventMsg::AgentReasoning(AgentReasoningEvent {
|
||||||
|
text: "request.".into(),
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
|
||||||
|
// Then stream answer deltas, followed by the exact same final message.
|
||||||
|
chat.handle_codex_event(Event {
|
||||||
|
id: "s1".into(),
|
||||||
|
msg: EventMsg::AgentMessageDelta(AgentMessageDeltaEvent {
|
||||||
|
delta: "Here is the ".into(),
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
chat.handle_codex_event(Event {
|
||||||
|
id: "s1".into(),
|
||||||
|
msg: EventMsg::AgentMessageDelta(AgentMessageDeltaEvent {
|
||||||
|
delta: "result.".into(),
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
|
||||||
|
chat.handle_codex_event(Event {
|
||||||
|
id: "s1".into(),
|
||||||
|
msg: EventMsg::AgentMessage(AgentMessageEvent {
|
||||||
|
message: "Here is the result.".into(),
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
|
||||||
|
// Snapshot the combined visible content to ensure we render as expected
|
||||||
|
// when deltas are followed by the identical final message.
|
||||||
|
let cells = drain_insert_history(&rx);
|
||||||
|
let combined = cells
|
||||||
|
.iter()
|
||||||
|
.map(|lines| lines_to_single_string(lines))
|
||||||
|
.collect::<String>();
|
||||||
|
assert_snapshot!(combined);
|
||||||
|
}
|
||||||
|
|||||||
@@ -143,6 +143,10 @@ impl StreamController {
|
|||||||
};
|
};
|
||||||
let cfg = self.config.clone();
|
let cfg = self.config.clone();
|
||||||
let state = self.state_mut(kind);
|
let state = self.state_mut(kind);
|
||||||
|
// Record that at least one delta was received for this stream
|
||||||
|
if !delta.is_empty() {
|
||||||
|
state.has_seen_delta = true;
|
||||||
|
}
|
||||||
state.collector.push_delta(delta);
|
state.collector.push_delta(delta);
|
||||||
if delta.contains('\n') {
|
if delta.contains('\n') {
|
||||||
let newly_completed = state.collector.commit_complete_lines(&cfg);
|
let newly_completed = state.collector.commit_complete_lines(&cfg);
|
||||||
@@ -263,19 +267,41 @@ impl StreamController {
|
|||||||
/// Apply a full final answer: replace queued content with only the remaining tail,
|
/// Apply a full final answer: replace queued content with only the remaining tail,
|
||||||
/// then finalize immediately and notify completion.
|
/// then finalize immediately and notify completion.
|
||||||
pub(crate) fn apply_final_answer(&mut self, message: &str, sink: &impl HistorySink) -> bool {
|
pub(crate) fn apply_final_answer(&mut self, message: &str, sink: &impl HistorySink) -> bool {
|
||||||
self.begin(StreamKind::Answer, sink);
|
self.apply_full_final(StreamKind::Answer, message, true, sink)
|
||||||
if !message.is_empty() {
|
}
|
||||||
let mut msg_with_nl = message.to_string();
|
|
||||||
if !msg_with_nl.ends_with('\n') {
|
pub(crate) fn apply_final_reasoning(&mut self, message: &str, sink: &impl HistorySink) -> bool {
|
||||||
msg_with_nl.push('\n');
|
self.apply_full_final(StreamKind::Reasoning, message, false, sink)
|
||||||
|
}
|
||||||
|
|
||||||
|
fn apply_full_final(
|
||||||
|
&mut self,
|
||||||
|
kind: StreamKind,
|
||||||
|
message: &str,
|
||||||
|
immediate: bool,
|
||||||
|
sink: &impl HistorySink,
|
||||||
|
) -> bool {
|
||||||
|
self.begin(kind, sink);
|
||||||
|
|
||||||
|
{
|
||||||
|
let state = self.state_mut(kind);
|
||||||
|
// Only inject the final full message if we have not seen any deltas for this stream.
|
||||||
|
// If deltas were received, rely on the collector's existing buffer to avoid duplication.
|
||||||
|
if !state.has_seen_delta && !message.is_empty() {
|
||||||
|
// normalize to end with newline
|
||||||
|
let mut msg = message.to_owned();
|
||||||
|
if !msg.ends_with('\n') {
|
||||||
|
msg.push('\n');
|
||||||
|
}
|
||||||
|
|
||||||
|
// replace while preserving already committed count
|
||||||
|
let committed = state.collector.committed_count();
|
||||||
|
state
|
||||||
|
.collector
|
||||||
|
.replace_with_and_mark_committed(&msg, committed);
|
||||||
}
|
}
|
||||||
let state = self.state_mut(StreamKind::Answer);
|
|
||||||
let already_committed = state.collector.committed_count();
|
|
||||||
// Preserve previously committed count so finalize emits only the remaining tail.
|
|
||||||
state
|
|
||||||
.collector
|
|
||||||
.replace_with_and_mark_committed(&msg_with_nl, already_committed);
|
|
||||||
}
|
}
|
||||||
self.finalize(StreamKind::Answer, true, sink)
|
|
||||||
|
self.finalize(kind, immediate, sink)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -11,6 +11,7 @@ pub(crate) enum StreamKind {
|
|||||||
pub(crate) struct StreamState {
|
pub(crate) struct StreamState {
|
||||||
pub(crate) collector: MarkdownStreamCollector,
|
pub(crate) collector: MarkdownStreamCollector,
|
||||||
pub(crate) streamer: AnimatedLineStreamer,
|
pub(crate) streamer: AnimatedLineStreamer,
|
||||||
|
pub(crate) has_seen_delta: bool,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl StreamState {
|
impl StreamState {
|
||||||
@@ -18,11 +19,13 @@ impl StreamState {
|
|||||||
Self {
|
Self {
|
||||||
collector: MarkdownStreamCollector::new(),
|
collector: MarkdownStreamCollector::new(),
|
||||||
streamer: AnimatedLineStreamer::new(),
|
streamer: AnimatedLineStreamer::new(),
|
||||||
|
has_seen_delta: false,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
pub(crate) fn clear(&mut self) {
|
pub(crate) fn clear(&mut self) {
|
||||||
self.collector.clear();
|
self.collector.clear();
|
||||||
self.streamer.clear();
|
self.streamer.clear();
|
||||||
|
self.has_seen_delta = false;
|
||||||
}
|
}
|
||||||
pub(crate) fn step(&mut self) -> crate::markdown_stream::StepResult {
|
pub(crate) fn step(&mut self) -> crate::markdown_stream::StepResult {
|
||||||
self.streamer.step()
|
self.streamer.step()
|
||||||
|
|||||||
Reference in New Issue
Block a user