Skip to content

fix(provider): preserve reasoning_content across consecutive assistant messages - #9404

Closed
shafqatevo wants to merge 1 commit into
aaif-goose:mainfrom
shafqatevo:fix/deepseek-reasoning-content-passthrough
Closed

fix(provider): preserve reasoning_content across consecutive assistant messages#9404
shafqatevo wants to merge 1 commit into
aaif-goose:mainfrom
shafqatevo:fix/deepseek-reasoning-content-passthrough

Conversation

@shafqatevo

Copy link
Copy Markdown

Problem

DeepSeek's thinking-mode API returns a 400 when the conversation history contains an assistant message with tool_calls but no reasoning_content:

Request failed: Bad request (400): The reasoning_content in the thinking mode must be passed back to the API.

This happens when DeepSeek streams a response that produces three consecutive assistant messages:

Assistant(Thinking: "reasoning")      -- thinking-only chunk
Assistant(Text: "I'll call a tool…")  -- text content chunk
Assistant(ToolRequest: "shell")       -- tool-call chunk (no Thinking)

In format_messages_with_options, pending_assistant_reasoning accumulates the reasoning from message 1 and merges it into message 2 (the text message). But it then clears the pending buffer — so message 3 (the tool-call message) receives no reasoning_content and DeepSeek rejects the request.

Fixes #9402
Fixes #9397
Follow-up to #8857 (which added the preserves_thinking flag but missed this clearing bug)

Fix

Remove the premature pending_assistant_reasoning.clear() from the merge path in format_messages_with_options. The buffer is already properly cleared at the top of the loop whenever a non-assistant message (user / tool) arrives, which prevents stale reasoning from leaking across turn boundaries.

The change is provider-agnostic — it's correct for all providers that set preserve_thinking_context: true (DeepSeek, Kimi, OpenRouter, vLLM), and has no effect on providers where it's false (OpenAI direct, Anthropic).

Tests

Two new regression tests:

  • test_format_messages_reasoning_survives_text_before_tool_call — the exact failure scenario: thinking-only → text-only → tool-call. Asserts both emitted messages carry reasoning_content.
  • test_format_messages_reasoning_cleared_across_user_message_boundary — verifies stale reasoning does NOT leak across a user message boundary (the original safety property).

All 71 existing format tests continue to pass (the 1 test_create_request_o1_default failure is pre-existing and unrelated).

@paulomac1000

Copy link
Copy Markdown

We reported this exact behavior in #9397 with extensive evidence:

  • Provider comparison: custom_deepseek fails, openrouter works (same model, same tools)
  • RAW LLM log analysis: reasoning_content present in ~50% of assistant messages with custom_deepseek, 100% with openrouter
  • Curl test: DeepSeek API itself handles reasoning round-trip correctly
  • Canary build testing: issue reproduces on 1.35.0-canary+728d72a

The root cause described here — pending_assistant_reasoning.clear() stripping reasoning from tool-call messages when a text-only chunk sits between thinking and tool-call — matches our findings perfectly. Our logs show exactly this pattern: first assistant message has reasoning_content, second (tool-call) does not.

Looking forward to testing this fix. Thank you for the PR!

@shafqatevo
shafqatevo force-pushed the fix/deepseek-reasoning-content-passthrough branch 2 times, most recently from ae1d571 to ff4578b Compare May 24, 2026 17:34
@shafqatevo
shafqatevo force-pushed the fix/deepseek-reasoning-content-passthrough branch from ff4578b to 48964d0 Compare May 25, 2026 19:34
Comment thread documentation/tailwind.config.js
@jamadeo jamadeo closed this May 26, 2026
@jamadeo

jamadeo commented May 26, 2026

Copy link
Copy Markdown
Member

@shafqatevo was your account compromised? the latest commit contains apparently malicious code.

@shafqatevo

shafqatevo commented Jun 1, 2026

Copy link
Copy Markdown
Author

My account was not compromised. The malicious code was not in my commit — it was already in the repo's history. Here's what I found:

The malware lives in commit 48964d0b8, authored by fre$h <anonwurcod@proton.me> with message "fix(agents): serialize per-session agent creation to stop duplicate MCP init (#9357)". Obfuscated JavaScript was appended to the last line of documentation/tailwind.config.js after ~350 characters of whitespace padding to hide it from code review.

Commit ce004f747 (the current upstream main) removed it — same commit message, same author, but sanitized. Someone cleaned this up.

My PR branch (ff4578b) is based on ce004f747 (the clean version), not 48964d0b8. Verified:

  • git merge-base --is-ancestor 48964d0b8 ff4578bNO
  • My PR only touches crates/goose/src/agents/agent.rs and crates/goose/src/providers/formats/openai.rs
  • documentation/tailwind.config.js is not modified by my PR

However, my fork's main branch was still pointing at the infected 48964d0b8. I've since force-pushed the clean upstream main to fix that. This is why 48964d0b8 appeared in the PR's force-push comparison view — GitHub was comparing against the fork's main branch tip.

Summary: The obfuscated code was injected by fre$h @fresh3nough into the repo's main branch at some point, and later removed (likely by a maintainer). My fork inherited the infected commit as its default branch, but my PR branch was always based on the clean version. I've now updated my fork's main to match upstream.

If fre$h is not a known maintainer, this is a supply-chain compromise that should be investigated — the account may have been compromised or is malicious.

@shafqatevo

Copy link
Copy Markdown
Author

@jamadeo Could you please reopen this PR? The malicious code was not from my commit — as I explained above, it was already in the repo history (commit 48964d0b8 by fre$h). My PR branch is based on the clean ce004f747 (upstream main), verified with git merge-base --is-ancestor 48964d0b8 ff4578b → NO.

The fix in this PR is needed for the DeepSeek reasoning_content 400 error (#9402, #9397). Both CI and live testing against DeepSeek pass. Would appreciate if you can take another look. Thanks!

@paulomac1000

Copy link
Copy Markdown

+1 to reopening. This fix is proven needed by live testing on v1.37.0 (see #9397 for detailed analysis). The malware was in the repo history (commit 48964d0b8), not in this PR — verified via git merge-base --is-ancestor.

We've identified 2 additional code paths where reasoning_content gets stripped beyond this fix. Happy to contribute as follow-up commits once reopened.

@jamadeo — please take another look.

@jamadeo

jamadeo commented Jun 5, 2026

Copy link
Copy Markdown
Member

Thanks for reopening the linked issue, but this PR will remain closed. We welcome another fix in a fresh PR.

I'll leave the forensic work to the experts, but to clarify a few facts:

  • 48964d0 did not come from goose's main branch. If main had been force-pushed, all goose contributors would have plainly seen that when they fetched. No rewrite of main ever happened.
  • It is not particularly hard to spoof a commit in git.
  • This PR's branch is from @shafqatevo's fork. The spoofed commit purports to be authored by fresh3nough, but only shafqatevo could have pushed the commit to their branch. In fact, GitHub clearly states that's what happened.

@shafqatevo I recommend looking into https://opensourcemalware.com/blog/polinrider-attack

@aaif-goose aaif-goose locked as resolved and limited conversation to collaborators Jun 5, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

4 participants