Skip to content

Commit e256bdf

Browse files
authored
Fix is_api_message to correctly exclude reasoning messages (openai#6156)
## Problem The `is_api_message` function in `conversation_history.rs` had a misalignment between its documentation and implementation: - **Comment stated**: "Anything that is not a system message or 'reasoning' message is considered an API message" - **Code behavior**: Was returning `true` for `ResponseItem::Reasoning`, meaning reasoning messages were incorrectly treated as API messages This inconsistency could lead to reasoning messages being persisted in conversation history when they should be filtered out. ## Root Cause Investigation revealed that reasoning messages are explicitly excluded throughout the codebase: 1. **Chat completions API** (lines 267-272 in `chat_completions.rs`) omits reasoning from conversation history: ```rust ResponseItem::Reasoning { .. } | ResponseItem::Other => { // Omit these items from the conversation history. continue; } ``` 2. **Existing tests** like `drops_reasoning_when_last_role_is_user` and `ignores_reasoning_before_last_user` validate that reasoning should be excluded from API payloads ## Solution Fixed the `is_api_message` function to align with its documentation and the rest of the codebase: ```rust // Before: Reasoning was incorrectly returning true ResponseItem::Reasoning { .. } | ResponseItem::WebSearchCall { .. } => true, // After: Reasoning correctly returns false ResponseItem::WebSearchCall { .. } => true, ResponseItem::Reasoning { .. } | ResponseItem::Other => false, ``` ## Testing - Enhanced existing test to verify reasoning messages are properly filtered out - All 264 core tests pass, including 8 chat completions tests that validate reasoning behavior - No regressions introduced This ensures reasoning messages are consistently excluded from API message processing across the entire codebase.
1 parent e733521 commit e256bdf

File tree

1 file changed

+30
-4
lines changed

1 file changed

+30
-4
lines changed

codex-rs/core/src/conversation_history.rs

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -516,8 +516,8 @@ fn truncate_formatted_exec_output(content: &str, total_lines: usize) -> String {
516516
result
517517
}
518518

519-
/// Anything that is not a system message or "reasoning" message is considered
520-
/// an API message.
519+
/// API messages include every non-system item (user/assistant messages, reasoning,
520+
/// tool calls, tool outputs, shell calls, and web-search calls).
521521
fn is_api_message(message: &ResponseItem) -> bool {
522522
match message {
523523
ResponseItem::Message { role, .. } => role.as_str() != "system",
@@ -542,6 +542,8 @@ mod tests {
542542
use codex_protocol::models::LocalShellAction;
543543
use codex_protocol::models::LocalShellExecAction;
544544
use codex_protocol::models::LocalShellStatus;
545+
use codex_protocol::models::ReasoningItemContent;
546+
use codex_protocol::models::ReasoningItemReasoningSummary;
545547
use pretty_assertions::assert_eq;
546548

547549
fn assistant_msg(text: &str) -> ResponseItem {
@@ -570,18 +572,32 @@ mod tests {
570572
}
571573
}
572574

575+
fn reasoning_msg(text: &str) -> ResponseItem {
576+
ResponseItem::Reasoning {
577+
id: String::new(),
578+
summary: vec![ReasoningItemReasoningSummary::SummaryText {
579+
text: "summary".to_string(),
580+
}],
581+
content: Some(vec![ReasoningItemContent::ReasoningText {
582+
text: text.to_string(),
583+
}]),
584+
encrypted_content: None,
585+
}
586+
}
587+
573588
#[test]
574589
fn filters_non_api_messages() {
575590
let mut h = ConversationHistory::default();
576-
// System message is not an API message; Other is ignored.
591+
// System message is not API messages; Other is ignored.
577592
let system = ResponseItem::Message {
578593
id: None,
579594
role: "system".to_string(),
580595
content: vec![ContentItem::OutputText {
581596
text: "ignored".to_string(),
582597
}],
583598
};
584-
h.record_items([&system, &ResponseItem::Other]);
599+
let reasoning = reasoning_msg("thinking...");
600+
h.record_items([&system, &reasoning, &ResponseItem::Other]);
585601

586602
// User and assistant should be retained.
587603
let u = user_msg("hi");
@@ -592,6 +608,16 @@ mod tests {
592608
assert_eq!(
593609
items,
594610
vec![
611+
ResponseItem::Reasoning {
612+
id: String::new(),
613+
summary: vec![ReasoningItemReasoningSummary::SummaryText {
614+
text: "summary".to_string(),
615+
}],
616+
content: Some(vec![ReasoningItemContent::ReasoningText {
617+
text: "thinking...".to_string(),
618+
}]),
619+
encrypted_content: None,
620+
},
595621
ResponseItem::Message {
596622
id: None,
597623
role: "user".to_string(),

0 commit comments

Comments
 (0)