Skip to content

fix(openai): handle providers emitting both reasoning and reasoning_content fields #2911

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tusharmath merged 4 commits into from
Apr 9, 2026
Merged

fix(openai): handle providers emitting both reasoning and reasoning_content fields #2911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1ff9b7e
fix(openai): handle providers emitting both reasoning and reasoning_c…
tusharmath Apr 9, 2026
80f1252
fix(openai): merge reasoning fields by longest non-empty value
tusharmath Apr 9, 2026
15c5a3b
test(openai): add unit tests for reasoning field selection logic
tusharmath Apr 9, 2026
4da35c5
[autofix.ci] apply automated fixes
autofix-ci[bot] Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions crates/forge_app/src/dto/openai/fixtures/chutes_completion_response.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"id": "chatcmpl-a9db7cc9005c6568",
"object": "chat.completion.chunk",
"created": 1775233185,
"model": "moonshotai/Kimi-K2.5-TEE",
"choices": [
{
"index": 0,
"delta": { "reasoning": " The", "reasoning_content": " The" },
"logprobs": null,
"finish_reason": null,
"token_ids": null,
"hidden_states": null
}
],
"usage": {
"prompt_tokens": 8764,
"total_tokens": 8765,
"completion_tokens": 1,
"reasoning_tokens": 1
},
"chutes_verification": "6bbeaf04d5800d774cc497b0a619acee"
}
120 changes: 114 additions & 6 deletions crates/forge_app/src/dto/openai/response.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,11 +136,23 @@ pub enum Choice {
},
}

/// A message returned by a provider, used for both streaming deltas and
/// non-streaming responses.
///
/// `reasoning` and `reasoning_content` are kept as separate private fields
/// because some providers (e.g. `moonshotai/Kimi-K2.5-TEE`) emit **both**
/// keys in the same delta object. Using `#[serde(alias)]` would cause a
/// `duplicate_field` error in that case. Use [`ResponseMessage::reasoning`]
/// to read the value in preference order.
#[derive(Debug, Deserialize, Serialize, Clone)]
pub struct ResponseMessage {
pub content: Option<String>,
#[serde(alias = "reasoning_content")]
pub reasoning: Option<String>,
// Private: some providers (e.g. moonshotai/Kimi-K2.5-TEE) emit both keys
// in the same delta object. Exposing them directly would let callers
// accidentally read only one and miss the other. Use `reasoning()` instead,
// which merges them in preference order.
reasoning: Option<String>,
reasoning_content: Option<String>,
pub role: Option<String>,
pub tool_calls: Option<Vec<ToolCall>>,
pub refusal: Option<String>,
Expand All @@ -152,6 +164,28 @@ pub struct ResponseMessage {
pub extra_content: Option<ExtraContent>,
}

impl ResponseMessage {
/// Returns the reasoning text. When both `reasoning` and
/// `reasoning_content` are present, the longer non-empty value is
/// returned; otherwise whichever is non-empty is used.
pub fn reasoning(&self) -> Option<&str> {
match (self.reasoning.as_deref(), self.reasoning_content.as_deref()) {
(Some(a), Some(b)) => {
let a = a.trim();
let b = b.trim();
match (a.is_empty(), b.is_empty()) {
(true, _) => Some(b).filter(|s| !s.is_empty()),
(_, true) => Some(a).filter(|s| !s.is_empty()),
_ => Some(if b.len() > a.len() { b } else { a }),
}
}
(Some(a), None) => Some(a).filter(|s| !s.trim().is_empty()),
(None, Some(b)) => Some(b).filter(|s| !s.trim().is_empty()),
(None, None) => None,
}
}
Comment on lines +171 to +186
Copy link
Copy Markdown
Contributor

@graphite-app graphite-app bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning() method returns inconsistent data: trimmed strings when both fields are present, but untrimmed strings when only one field is present.

When both reasoning and reasoning_content exist (line 173-180), the returned value is the trimmed version (a.trim() or b.trim()). However, when only one field exists (lines 182-183), the original untrimmed string is returned.

For example:

  • reasoning(Some(" hello "), None) returns " hello " (untrimmed)
  • reasoning(Some(" hello "), Some("")) returns "hello" (trimmed)

This inconsistency could cause production issues if downstream code expects consistent whitespace handling.

Fix: Either always return trimmed strings or always return untrimmed strings. For consistency with the filtering logic that checks !s.trim().is_empty(), the method should return trimmed strings in all cases:

pub fn reasoning(&self) -> Option<&str> {
    match (self.reasoning.as_deref(), self.reasoning_content.as_deref()) {
        (Some(a), Some(b)) => {
            let a = a.trim();
            let b = b.trim();
            match (a.is_empty(), b.is_empty()) {
                (true, _) => Some(b).filter(|s| !s.is_empty()),
                (_, true) => Some(a).filter(|s| !s.is_empty()),
                _ => Some(if b.len() > a.len() { b } else { a }),
            }
        }
        (Some(a), None) => {
            let trimmed = a.trim();
            Some(trimmed).filter(|s| !s.is_empty())
        }
        (None, Some(b)) => {
            let trimmed = b.trim();
            Some(trimmed).filter(|s| !s.is_empty())
        }
        (None, None) => None,
    }
}
Suggested change
pub fn reasoning(&self) -> Option<&str> {
match (self.reasoning.as_deref(), self.reasoning_content.as_deref()) {
(Some(a), Some(b)) => {
let a = a.trim();
let b = b.trim();
match (a.is_empty(), b.is_empty()) {
(true, _) => Some(b).filter(|s| !s.is_empty()),
(_, true) => Some(a).filter(|s| !s.is_empty()),
_ => Some(if b.len() > a.len() { b } else { a }),
}
}
(Some(a), None) => Some(a).filter(|s| !s.trim().is_empty()),
(None, Some(b)) => Some(b).filter(|s| !s.trim().is_empty()),
(None, None) => None,
}
}
pub fn reasoning(&self) -> Option<&str> {
match (self.reasoning.as_deref(), self.reasoning_content.as_deref()) {
(Some(a), Some(b)) => {
let a = a.trim();
let b = b.trim();
match (a.is_empty(), b.is_empty()) {
(true, _) => Some(b).filter(|s| !s.is_empty()),
(_, true) => Some(a).filter(|s| !s.is_empty()),
_ => Some(if b.len() > a.len() { b } else { a }),
}
}
(Some(a), None) => {
let trimmed = a.trim();
Some(trimmed).filter(|s| !s.is_empty())
}
(None, Some(b)) => {
let trimmed = b.trim();
Some(trimmed).filter(|s| !s.is_empty())
}
(None, None) => None,
}
}

Spotted by Graphite

Fix in Graphite

Is this helpful? React 👍 or 👎 to let us know.

}

impl From<ReasoningDetail> for forge_domain::ReasoningDetail {
fn from(detail: ReasoningDetail) -> Self {
forge_domain::ReasoningDetail {
Expand Down Expand Up @@ -319,8 +353,8 @@ impl TryFrom<Response> for ChatCompletionMessage {
.clone()
.and_then(|s| FinishReason::from_str(&s).ok()),
);
if let Some(reasoning) = &message.reasoning {
resp = resp.reasoning(Content::full(reasoning.clone()));
if let Some(reasoning) = message.reasoning() {
resp = resp.reasoning(Content::full(reasoning.to_owned()));
}

if let Some(thought_signature) = message
Expand Down Expand Up @@ -387,8 +421,8 @@ impl TryFrom<Response> for ChatCompletionMessage {
.and_then(|s| FinishReason::from_str(&s).ok()),
);

if let Some(reasoning) = &delta.reasoning {
resp = resp.reasoning(Content::part(reasoning.clone()));
if let Some(reasoning) = delta.reasoning() {
resp = resp.reasoning(Content::part(reasoning.to_owned()));
}

if let Some(thought_signature) = delta
Expand Down Expand Up @@ -531,6 +565,66 @@ mod tests {

struct Fixture;

fn response_message(
reasoning: Option<&str>,
reasoning_content: Option<&str>,
) -> ResponseMessage {
ResponseMessage {
content: None,
reasoning: reasoning.map(str::to_owned),
reasoning_content: reasoning_content.map(str::to_owned),
role: None,
tool_calls: None,
refusal: None,
reasoning_details: None,
reasoning_text: None,
reasoning_opaque: None,
extra_content: None,
}
}

#[test]
fn test_reasoning_only_reasoning_field() {
let fixture = response_message(Some("hello"), None);
assert_eq!(fixture.reasoning(), Some("hello"));
}

#[test]
fn test_reasoning_only_reasoning_content_field() {
let fixture = response_message(None, Some("hello"));
assert_eq!(fixture.reasoning(), Some("hello"));
}

#[test]
fn test_reasoning_both_returns_longer() {
let fixture = response_message(Some("short"), Some("much longer text"));
assert_eq!(fixture.reasoning(), Some("much longer text"));
}

#[test]
fn test_reasoning_both_equal_length_returns_reasoning() {
let fixture = response_message(Some("aaa"), Some("bbb"));
assert_eq!(fixture.reasoning(), Some("aaa"));
}

#[test]
fn test_reasoning_both_present_one_empty_returns_non_empty() {
let fixture = response_message(Some(""), Some("content"));
assert_eq!(fixture.reasoning(), Some("content"));
}

#[test]
fn test_reasoning_both_empty_returns_none() {
let fixture = response_message(Some(""), Some(""));
assert_eq!(fixture.reasoning(), None);
}

#[test]
fn test_reasoning_neither_present_returns_none() {
let fixture = response_message(None, None);
assert_eq!(fixture.reasoning(), None);
}

async fn load_fixture(filename: &str) -> serde_json::Value {
let fixture_path = format!("src/dto/openai/fixtures/{}", filename);
let fixture_content = tokio::fs::read_to_string(&fixture_path)
Expand Down Expand Up @@ -571,6 +665,17 @@ mod tests {
assert!(Fixture::test_response_compatibility(event));
}

#[tokio::test]
async fn test_kimi_k2_both_reasoning_keys_event() {
// moonshotai/Kimi-K2.5-TEE emits both "reasoning" and "reasoning_content"
// in the same delta object. This must parse without a duplicate_field error.
let fixture = load_fixture("chutes_completion_response.json").await;
let actual = serde_json::from_value::<Response>(fixture);
assert!(actual.is_ok(), "Failed to parse: {:?}", actual.err());
let completion_result = ChatCompletionMessage::try_from(actual.unwrap());
assert!(completion_result.is_ok());
}

#[test]
fn test_fireworks_response_event_missing_arguments() {
let event = "{\"id\":\"gen-1749331907-SttL6PXleVHnrdLMABfU\",\"provider\":\"Fireworks\",\"model\":\"qwen/qwen3-235b-a22b\",\"object\":\"chat.completion.chunk\",\"created\":1749331907,\"choices\":[{\"index\":0,\"delta\":{\"role\":\"assistant\",\"content\":null,\"tool_calls\":[{\"index\":0,\"id\":\"call_Wl2L8rrzHwrXSeiciIvU65IS\",\"type\":\"function\",\"function\":{\"name\":\"attempt_completion\"}}]},\"finish_reason\":null,\"native_finish_reason\":null,\"logprobs\":null}]}";
Expand Down Expand Up @@ -632,6 +737,7 @@ mod tests {
message: ResponseMessage {
content: Some("test content".to_string()),
reasoning: None,
reasoning_content: None,
role: Some("assistant".to_string()),
tool_calls: None,
refusal: None,
Expand Down Expand Up @@ -669,6 +775,7 @@ mod tests {
delta: ResponseMessage {
content: Some("test content".to_string()),
reasoning: None,
reasoning_content: None,
role: Some("assistant".to_string()),
tool_calls: None,
refusal: None,
Expand Down Expand Up @@ -706,6 +813,7 @@ mod tests {
message: ResponseMessage {
content: Some("Hello, world!".to_string()),
reasoning: None,
reasoning_content: None,
role: Some("assistant".to_string()),
tool_calls: None,
refusal: None,
Expand Down
Loading