Repository navigation
fix(oaistream): replay ReasoningContent when converting messages - #4485
MysticalMount wants to merge 1 commit into
Conversation
aheritier
added
area/providers/openai
OpenAI-compatible custom providers (e.g. Qwen served via llama.cpp, vLLM, or other endpoints using the DeepSeek-style reasoning_content convention) have their streamed reasoning captured into msg.ReasoningContent, but convertMessagesWithCaps built the outgoing assistant param from Content, FunctionCall, and ToolCalls only -- the stored reasoning was silently dropped when the conversation history was replayed on the next request. openai-go's typed ChatCompletionAssistantMessageParam has no field for reasoning_content (non-standard extension -- see openai/openai-go#558), so it is attached via SetExtraFields when present; nothing is added when the history contains no reasoning. The assistant-turn skip predicate is also taught to keep reasoning-only messages (e.g. a model that exhausted its output budget mid-reasoning) instead of dropping them as empty. Fixes docker#4363 Signed-off-by: Jay <12496124+MysticalMount@users.noreply.github.com>
ab8d68a to
15d5b95
Compare
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
Moving forward with this PR rather than #4365 to consolidate the implementation and discussion. Before merge, please fix the reasoning-only assistant wire format and add serialized-JSON regression coverage, as detailed inline. Please also resolve the compatibility policy for replaying the nonstandard reasoning_content field through the shared converter.
The correctness finding is based on SDK and llama.cpp source inspection, not a live backend reproduction. Please validate against the target backend and rerun required checks after the fixes. Blocking CI was green during the earlier review, but the latest check-runs/status lookups returned HTTP 500, so current CI was not reverified; that is not evidence of failing tests.
Optional cleanup: fold the repro tests into messages_test.go and shorten the explanatory comment to the non-obvious reason for using SDK extra fields. Compaction accounting and overflow recovery remain outside this focused change.
|
|
||
| // Skip invalid assistant messages upfront. This can happen if the model is out of tokens (max_tokens reached) | ||
| if msg.Role == chat.MessageRoleAssistant && len(msg.ToolCalls) == 0 && len(msg.MultiContent) == 0 && strings.TrimSpace(msg.Content) == "" { | ||
| if msg.Role == chat.MessageRoleAssistant && len(msg.ToolCalls) == 0 && len(msg.MultiContent) == 0 && strings.TrimSpace(msg.Content) == "" && strings.TrimSpace(msg.ReasoningContent) == "" { |
There was a problem hiding this comment.
[blocking] Preserve reasoning-only turns with a valid wire-format assistant message.
A chat.Message with assistant role and ReasoningContent: "thinking", but no content or tool calls, now survives this guard. The conversion leaves Content unset, and the pinned openai-go SDK omits both content and tool_calls, yielding a message with only role and reasoning_content. llama.cpp rejects that shape: https://github.com/ggml-org/llama.cpp/blob/448147d42a71d0147c6dc844f99f9ffe7419183a/tools/server/server-common.cpp#L1334-L1337. This is based on source inspection, not a live reproduction.
For a retained reasoning-only turn, explicitly set empty-string content, for example assistantParam.Content.OfString = param.NewOpt(""). Add marshaled-JSON tests asserting content: "" is present. Also cover reasoning alongside text/tool calls, absence of the extension when no reasoning is stored, and empty/whitespace edge cases. The current ExtraFields/message-count assertions do not catch this invalid wire shape. Validate subsequent requests using the retained session history against the target backend.
| // reasoning_content. openai-go's typed ChatCompletionAssistantMessageParam has | ||
| // no such field (non-standard extension - see openai/openai-go#558), so it | ||
| // must go through SetExtraFields. See issue #4363. | ||
| if msg.ReasoningContent != "" { |
There was a problem hiding this comment.
[should-fix] Resolve the destination-specific replay policy explicitly.
This condition checks stored reasoning, not the destination provider/model or a replay setting. The shared converter feeds OpenAI-compatible Chat Completions and DMR, so the nonstandard reasoning_content extension is sent across those paths. Issue #4363 raises configurability; the existing attachment capability overrides do not provide a reasoning-replay escape hatch.
Please either provide a dedicated per-model opt-out/capability with tests, or record the maintainer decision to accept universal default-on replay and the compatibility validation supporting it. This is a compatibility-policy risk, not a demonstrated rejection by another provider.
This branch has not been deployed
2 participants
Fixes #4363
Supersedes #4365 (re-based onto current
mainwith signed commits, extended so reasoning-only assistant messages are no longer dropped as empty).Root cause
convertMessagesWithCapsinpkg/model/provider/oaistream/messages.gobuilt the outgoing assistant message param fromContent,FunctionCall, andToolCallsonly.msg.ReasoningContentwas never read, so reasoning captured from OpenAI-compatible custom providers (e.g. Qwen via llama.cpp, vLLM) was silently dropped when the conversation history was replayed on the next request.The same assistant-turn skip predicate also dropped messages that contain only reasoning (no text, no tool calls — e.g. a model that exhausted its output budget mid-reasoning) before conversion ever saw them.
Fix
SetExtraFields—openai-go's typedChatCompletionAssistantMessageParamhas no field forreasoning_content(non-standard, provider-specific extension; see can @chatcompletion.go support reasoning_content? openai/openai-go#558). Nothing is added when the stored history contains no reasoning.Compatibility note
reasoning_contentis only serialized when a stored message actually carries reasoning. Since this path serves many OpenAI-compatible endpoints (xAI, Mistral, Groq, OpenRouter, DeepSeek, ...), a session that accumulates reasoning on one endpoint will send the field when later routed to any other endpoint through this provider. If a specific endpoint is known to reject unknown fields, preservation could be made per-model opt-out via the existing capability-override mechanism (as the issue itself suggested) — happy to add that as a follow-up if maintainers prefer.Testing
Added
repro_issue4363_test.gocovering: reasoning is carried onto the replayed message, no field is added when reasoning is absent, reasoning survives alongside tool calls, and a reasoning-only assistant message survives conversion.