fix(openai): ensure reasoning_content is present in all assistant messages - #1755
fix(openai): ensure reasoning_content is present in all assistant messages#1755ifdog wants to merge 1 commit into
Conversation
…sages When reasoning_content is enabled (ReasoningFieldContent or ReasoningFieldAll), ensure all assistant messages have a reasoning_content field. Some thinking-enabled providers (e.g., Kimi, DeepSeek) require reasoning_content to be present in every assistant message, even if it's an empty string. Without this, multi-turn tool-calling conversations fail with: "thinking is enabled but reasoning_content is missing in assistant tool call message". This aligns with the existing fix in the DeepSeek transformer.
Greptile SummaryThis PR injects
Confidence Score: 4/5Safe to merge; the change is a narrow, additive guard that injects an empty string into a pointer field only when that field is absent, matching an already-deployed pattern in the DeepSeek transformer. The logic is correct for the primary use-case ( llm/transformer/openai/outbound.go — specifically the Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant OutboundTransformer
participant RequestFromLLM
participant Provider
Caller->>OutboundTransformer: TransformRequest(llmReq)
OutboundTransformer->>RequestFromLLM: RequestFromLLM(llmReq, reasoningField)
RequestFromLLM-->>OutboundTransformer: oaiReq (assistant msgs may have nil ReasoningContent)
alt "reasoningField == ReasoningFieldContent OR ReasoningFieldAll"
loop each message in oaiReq.Messages
alt "role=="assistant" AND ReasoningContent==nil"
OutboundTransformer->>OutboundTransformer: "set ReasoningContent = ptr("")"
end
end
end
OutboundTransformer->>Provider: POST /chat/completions (all assistant msgs have reasoning_content)
Reviews (1): Last reviewed commit: "fix(openai): ensure reasoning_content is..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if reasoningField == ReasoningFieldContent || reasoningField == ReasoningFieldAll { | ||
| for i := range oaiReq.Messages { | ||
| if oaiReq.Messages[i].Role == "assistant" && oaiReq.Messages[i].ReasoningContent == nil { | ||
| emptyStr := "" | ||
| oaiReq.Messages[i].ReasoningContent = &emptyStr | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
ReasoningFieldAll leaves reasoning field unpatched
When reasoningField is ReasoningFieldAll, the loop ensures reasoning_content is non-nil but leaves reasoning as nil on the same message. MessageFromLLMWithConfig only cross-syncs the two fields when at least one is already non-nil (lines 135–140 of outbound_convert.go), so a purely synthetic assistant message (e.g. an injected tool-call turn with no prior reasoning data) ends up with reasoning_content: "" but reasoning: null. Any provider configured with ReasoningFieldAll that applies the same "must be present" contract to the reasoning field would still fail. The DeepSeek transformer avoids this because it always uses ReasoningFieldContent, not ReasoningFieldAll.
| if reasoningField == ReasoningFieldContent || reasoningField == ReasoningFieldAll { | ||
| for i := range oaiReq.Messages { | ||
| if oaiReq.Messages[i].Role == "assistant" && oaiReq.Messages[i].ReasoningContent == nil { | ||
| emptyStr := "" | ||
| oaiReq.Messages[i].ReasoningContent = &emptyStr | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
outbound_reasoning_test.go tests MessageFromLLMWithConfig and RequestFromLLM unit-level behavior, but none of the TransformRequest integration tests verify that an assistant message with a nil ReasoningContent actually exits with reasoning_content: "" in the serialized body. A test case such as "assistant tool-call message with no reasoning becomes reasoning_content: """ would confirm the fix and guard against future regressions.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
ds/mimo 是这样，但是不代表其他都是这样吧，不确定是否有其他兼容性问题。 |
2 participants
When reasoning_content is enabled (ReasoningFieldContent or ReasoningFieldAll), ensure all assistant messages have a reasoning_content field.
Some thinking-enabled providers (e.g., Kimi, DeepSeek) require reasoning_content to be present in every assistant message, even if it's an empty string. Without this, multi-round tool-calling conversations fail with:
This aligns with the existing fix already present in the DeepSeek transformer.