fix(provider): preserve reasoning_content on assistant messages with tool_calls (DeepSeek) - #9620
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf30fe26fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if from_response.is_empty() && !turn_thinking_content.is_empty() { | ||
| turn_thinking_content.clone() | ||
| } else { |
There was a problem hiding this comment.
When a stream emits a thinking-only chunk and later a tool-call chunk with no thinking, that earlier chunk has already been stored by the num_tool_requests == 0 path. Cloning turn_thinking_content here creates a second thinking-only assistant message before the tool request; with the paired openai.rs change that no longer clears pending reasoning between assistant messages, format_messages_with_options concatenates both copies and sends duplicated reasoning_content on the next tool call. This affects the DeepSeek streaming scenario this change targets and can make the echoed reasoning differ from what the provider originally produced (or unnecessarily double the context).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7fcb294. The thinking_content variable now only comes from the current response — no longer falls back to turn_thinking_content. Intermediate Thinking messages were already pushed by the num_tool_requests == 0 path.
cf30fe2 to
8a458e1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a458e1860
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !pending_assistant_reasoning.is_empty() { | ||
| reasoning_text = | ||
| merge_reasoning_text(&pending_assistant_reasoning, &reasoning_text); | ||
| pending_assistant_reasoning.clear(); | ||
| // Do NOT clear pending_assistant_reasoning here. It is cleared |
There was a problem hiding this comment.
When a streamed assistant chunk contains both additional Thinking and text before a later tool-call chunk, this merges the old pending prefix with the chunk's reasoning only for the current text message, but leaves pending_assistant_reasoning at the old prefix. The following tool-call-only assistant message then receives only that stale prefix (e.g. Thinking("a"), then Thinking("b") + Text, then ToolRequest formats the tool call with reasoning_content: "a" instead of "ab"), so DeepSeek can still reject or receive incomplete echoed reasoning for streams that interleave reasoning with visible text before tool calls.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7fcb294. After merging pending_assistant_reasoning with the current chunk's reasoning, pending_assistant_reasoning now advances to the merged value (reasoning_text.clone()). This ensures the next assistant message (e.g. tool-call) receives the full reasoning, not a stale prefix.
8a458e1 to
9a237df
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a237dff69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if from_response.is_empty() && !turn_thinking_content.is_empty() { | ||
| turn_thinking_content.clone() |
There was a problem hiding this comment.
For Anthropic/Claude thinking streams, response_to_streaming_message emits the signed Thinking block as its own assistant message before yielding the later tool request, and formats/anthropic.rs serializes signed thinking blocks unconditionally. This fallback therefore stores the same thinking once in the original thinking-only assistant message and again inside the tool-request assistant message, so the next Anthropic request replays a duplicated signed thinking block rather than the exact assistant history the provider produced. Fresh evidence beyond the earlier OpenAI-focused duplicate-reasoning comment is the Anthropic streaming/parser path that yields thinking separately and has no de-duplication before formatting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. The turn_thinking_content fallback on reasoning_content is intentional and necessary for OpenAI-compatible providers (DeepSeek, Kimi) where MOIM injections or other user messages can clear pending_assistant_reasoning between the thinking-only and tool-call messages in the same turn. Without this fallback, the tool-call message loses reasoning_content and DeepSeek returns 400.
For Anthropic, the concern about duplicate thinking blocks is valid but should be handled in formats/anthropic.rs (de-duplication at serialization time), not by removing the fallback which is critical for the OpenAI path. The Anthropic formatter already handles signed thinking blocks differently from the OpenAI formatter, so a formatter-level de-dup check is the right layer for this.
9a237df to
7fcb294
Compare
paulomac1000
left a comment
paulomac1000
left a comment
There was a problem hiding this comment.
✅ Verified — fix works
Tested against custom_deepseek + deepseek-v4-flash. The 400 error reasoning_content must be passed back to the API is resolved. Normal multi-turn tool conversations now work. Thank you for the clean PR!
⚠️ Two remaining edge cases (follow-up, not blocking)
Our analysis (goose run -t --resume with extensive logging on v1.37.0) identified two additional code paths where reasoning_content can still be dropped. These are rare edge cases — the PR already fixes the common one — but worth tracking:
1. RedactedThinking silent drop (~openai.rs:210)
When format_messages_with_options() encounters a RedactedThinking variant, it skips it entirely — no reasoning_content is emitted for that assistant message. This can happen when DeepSeek's content filter intervenes mid-thinking.
// Current behavior: RedactedThinking → no reasoning_content emitted at all2. merge_split_tool_call_messages() strict matching (~openai.rs:290-360)
When multiple tool-calls arrive in separate messages with non-identical reasoning_content (e.g., DeepSeek streams Thinking("a") → ToolCall(shell) → Thinking("b") → ToolCall(read)), the merge function only combines them when reasoning_content matches exactly. Mismatched reasoning can result in dropped content.
Suggestion
Merge this PR as-is (it fixes the primary issue affecting all known reporters: #9397, #9402, #9434, #9333, #9200). Then open a follow-up issue tracking the two edge cases above for a future improvement.
Again — great work on the clean resubmission. Appreciate the effort! 🚀
shafqatevo
left a comment
shafqatevo
left a comment
There was a problem hiding this comment.
Thank you for testing and the thorough analysis! 🙏
Agreed — merge as-is for the primary fix, and I'll open a follow-up issue for the two edge cases:
-
RedactedThinking silent drop: When DeepSeek's content filter intervenes mid-thinking and produces
RedactedThinking,format_messages_with_options()skips it entirely. Fix: treatRedactedThinkingsimilarly toThinking— extract whatever partial reasoning is available and include it inreasoning_content. -
merge_split strict matching: When DeepSeek streams
Thinking("a") → ToolCall → Thinking("b") → ToolCall, the merge function only combines whenreasoning_contentmatches exactly. Mismatched reasoning across tool-call splits results in unmerged (and potentially dropped) content. Fix: use the first message's reasoning as the canonical value for the merged message, or concatenate distinct reasoning blocks.
I'll open the issue now.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fff87012f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if from_response.is_empty() && !turn_thinking_content.is_empty() { | ||
| turn_thinking_content.clone() | ||
| } else { | ||
| from_response |
There was a problem hiding this comment.
When the final streamed tool-call chunk contains any Thinking of its own, this branch ignores the earlier turn_thinking_content instead of combining it. That still loses reasoning for DeepSeek/Kimi histories where MOIM inserts a user message before the latest assistant tool call: format_messages_with_options clears pending reasoning at that user boundary, so the stored tool-call message only echoes the suffix from from_response rather than the full reasoning accumulated earlier in the turn.
Useful? React with 👍 / 👎.
|
@shafqatevo can you look at the codex? |
fd98ab1 to
6aeb48f
Compare
|
Thanks for the work on this Closing for two reasons:
|
5 participants
Summary
Fixes the DeepSeek 400 error: "The
reasoning_contentin the thinking mode must be passed back to the API" — reported in #9402, #9397, #9200.When DeepSeek (or similar providers with
preserves_thinking: true) streams a response containing tool calls, thereasoning_contentfield was missing from the assistant message sent back on the next API turn, causing a 400 error.Root Cause
Two bugs combined:
Bug 1 —
openai.rs(format_messages_with_options):pending_assistant_reasoningwas cleared unconditionally after merging into any assistant message — including text-only messages that precede tool-call messages in the same turn. Fix: only clear on non-assistant messages (the true turn boundary).Bug 2 —
agent.rs(agent streaming loop):When DeepSeek streams a response with tool calls, thinking content arrives in intermediate streaming chunks before the tool-call chunk. The agent stored these as separate thinking-only messages, but created the tool-request message without any thinking content. If any other message (MOIM injection, text content, etc.) appeared between the thinking and tool-request messages,
format_messages_with_optionshad no reasoning to set on the tool-call message.Fix: accumulate thinking from all intermediate streamed messages in
turn_thinking_contentand clone it onto tool-request messages.Testing
deepseek-v4-flash) — tool call + tool result round-trip succeeds without 400 errorformat_messages_with_optionsfixFiles Changed
crates/goose/src/agents/agent.rs— accumulateturn_thinking_contentacross streamed messages, attach to tool-request messagescrates/goose/src/providers/formats/openai.rs— don't clearpending_assistant_reasoningon merge (defense-in-depth) + 2 regression testsClean PR replacing #9404 which was closed due to unrelated fork contamination.