fix(core): preserve reasoning_content in rewind, compression, and merge paths (#3579)#3737
Conversation
…(5 PRs) - BerriAI/litellm#26764 merge-after-nits: multipart user_config/tags JSON decode - BerriAI/litellm#26769 request-changes: FuturMix add bundles unrelated deletions - google-gemini/gemini-cli#26184 merge-after-nits: deleteSession fail-loud on missing file - QwenLM/qwen-code#3737 merge-after-nits: drop strip-thoughts helpers, preserve reasoning_content - aaif-goose/goose#8781 needs-discussion: ACP per-connection inline→spawn ordering question
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
Thanks for the careful follow-up on #3579 — really appreciate the effort here.
A bit of context on how we fixed the recent DeepSeek HTTP 400s: DeepSeek can legitimately return an assistant tool-call turn with empty reasoning_content. Our pipeline filters that empty thought out when storing the turn, so when we rebuild the next request the replayed assistant turn has reasoning_content absent — which DeepSeek then rejects, since thinking-mode requires the field on every prior tool-call turn. #3729 addresses this at the canonical layer: ensureReasoningContentOnToolCalls in provider/deepseek.ts runs on every outgoing assistant message and injects reasoning_content: '' when a turn carries tool_calls but the field is missing. That's scoped to the only shape DeepSeek actually rejects.
This PR is a valuable enhancement on top of that — the rewind handler change and the dead-method cleanup look good and we're happy to take those. But two of the changes don't apply given the canonical fix, and we'd prefer to revert them:
chatCompressionService.ts— the compressed ack is plain text with notool_calls, so DeepSeek's constraint doesn't apply. The{ text: ' ', thought: true }injection is defensive but unnecessary, and the inline comment overstates the requirement.mergeConsecutiveAssistantMessagesinconverter.ts— even if a merged turn ends up withtool_calls, #3729's normalizer fixes it before sending. Duplicating reasoning-merge logic in the converter is redundant and risks drift if the canonical contract changes. If a concrete failure case shows up that isn't covered byensureReasoningContentOnToolCalls, we'd rather extend that one place.
Verdict
REQUEST_CHANGES — once those two are reverted, we're ready to merge. Thanks again!
…nLM#3737) Per tanzhenxin's review: the compressed ack is plain text without tool_calls so the thought-part injection is unnecessary, and the converter reasoning merge is redundant given QwenLM#3729's canonical ensureReasoningContentOnToolCalls in the deepseek provider. Both paths are now handled at the request boundary, not in history transformation.
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
Thanks for the round-2 changes — reverting the chatCompressionService.ts and converter.ts modifications was the right call. The maintainer's #3729 normalizer (ensureReasoningContentOnToolCalls) handles compression and replay at the request boundary, so an overlapping fix here would have just caused merge friction. The PR is now appropriately narrow: one behavioral line in the rewind handler plus symmetric dead-code removal.
Verdict
APPROVE — production code is correct, scope reduction is right, dead-code removal is symmetric.
…ge paths (QwenLM#3579) (QwenLM#3737) * fix(core): preserve reasoning_content in rewind, compression, and merge paths (QwenLM#3579) * chore(core): remove dead stripThoughtsFromHistory methods (QwenLM#3579) * revert(pr): remove redundant reasoning merge per review feedback (QwenLM#3737) Per tanzhenxin's review: the compressed ack is plain text without tool_calls so the thought-part injection is unnecessary, and the converter reasoning merge is redundant given QwenLM#3729's canonical ensureReasoningContentOnToolCalls in the deepseek provider. Both paths are now handled at the request boundary, not in history transformation.
Summary
This PR fixes three remaining paths where
reasoning_contentcould be silently dropped during DeepSeek thinking mode sessions, completing the set of fixes for #3579.After #3590 (session resume / idle) and #3682 (model switch / load_history), these paths were still missing:
AppContainer.tsx):/rewindand double-ESC calledstripThoughtsFromHistory()after truncating historychatCompressionService.ts): The compressed summary turn was created without athoughtpart, breaking DeepSeek's required reasoning chain continuityconverter.ts):mergeConsecutiveAssistantMessagesdiscardedreasoning_contentwhen merging adjacent assistant messagesSecond commit removes the now-unused
stripThoughtsFromHistory()andstripThoughtsFromHistoryKeepRecent()methods fromGeminiClientandGeminiChat, along with their test blocks and stale mock entries.This aligns with the maintainer decision: remove all strip-on-event sites.
Changes
packages/cli/src/ui/AppContainer.tsxstripThoughtsFromHistory()call in the rewind handlerpackages/core/src/services/chatCompressionService.ts{ text: ' ', thought: true }part to the compressed summary turnpackages/core/src/core/openaiContentGenerator/converter.tsreasoning_contentfieldsreasoning_content(string or null)packages/core/src/core/client.tsstripThoughtsFromHistory()wrapper method (dead code)packages/core/src/core/geminiChat.tsstripThoughtsFromHistory()method (dead code)stripThoughtsFromHistoryKeepRecent()method (dead code)Testing
npm run build— passesnpm run typecheck— passesCloses #3579