fix(core): preserve reasoning_content when merging assistant turns - #5815
Conversation
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ On direction: this is a clear bug fix — On approach: the diff is exactly what the problem demands — concatenate Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 方向:这是一个明确的 bug 修复—— 方案:diff 恰好是解决问题所需的最小改动——用与拼接 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
|
Code review: The fix adds reasoning concatenation right after the existing tool_calls merge, using the same Testing: This is an internal converter function — no user-visible TUI change. Verification is through unit tests. Ran the full Before (main — test without fix)After (this PR — all 135 tests)The new test fails on main (confirming the bug exists) and passes with the fix (confirming the fix works). All 134 pre-existing tests remain green. 中文说明代码审查: 修复在现有的 tool_calls 合并之后添加了 reasoning 拼接逻辑,使用了与 content 拼接相同的 测试: 这是内部转换器的函数,没有用户可见的 TUI 变化。通过单元测试验证。在两种配置下分别运行了完整的
— Qwen Code · qwen3.7-max |
|
This is a textbook bug fix: real bug confirmed by failing test on main, minimal fix that mirrors an existing pattern in the same function, targeted test added, zero regressions across 135 tests. The reasoning goes: My independent proposal would have been identical — concatenate reasoning strings with the same Approving. ✅ 中文说明这是一个教科书级的 bug 修复:main 上失败的测试确认了真实 bug 的存在,修复方式极简且复用了同一函数中已有的模式,添加了针对性测试,135 个测试零回归。逻辑链条: 我的独立方案完全一致——用与 content 相同的 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
|
请提供一个截图作为 Before & After 的证据,例如在对话中展示 |
✅ Runtime verification — safe to mergeVerified locally on Linux with a real
Layer 1 — A/B on the real compiled functionDrove the shipped All in-scope scenarios pass; messages without reasoning are unchanged. Layer 2 — vitest A/B (the PR's own test, identical test file on both sides)
→ The new test genuinely catches the bug it targets — the fix is both necessary and sufficient. Layer 3 — full
|
| 被测 converter | should preserve reasoning_content from every merged assistant turn |
|---|---|
| BEFORE(未修复) | × 失败 1 |
| AFTER(已修复) | ✓ 通过 1 |
→ 这个新测试确实能抓到它针对的 bug,说明修复既必要又充分。
第 3 层 — AFTER 上跑完整 converter.test.ts(回归)
Test Files 1 passed (1)
Tests 135 passed (135)
→ 无回归;与 PR 描述里"135 个测试保持全绿"完全一致。
一个范围外的观察(不是本 PR 的阻塞项)
当两个纯 reasoning回合(只有思考、没有可见文本,即各自 content: "")合并时,合并后的 content 会变成 null —— 因为 content 合并用的是 [...].filter(Boolean).join('') || null。这是改动前就存在的(BEFORE/AFTER 一致),而且 reasoning_content 在第一个回合就已经存在,所以本 PR 既没有引入也没有加重 PR 自己提到的会触发 Ollama HTTP 400 的 content: null + reasoning_content 组合。可以考虑单独跟进 content 合并这条路径(对齐单条消息里 reasoningParts.length > 0 ? '' : null 的规则),但与本次改动无关。
结论
正确,可以合并。 修复行为与描述完全一致,新测试有"牙齿",完整测试套件保持全绿,且不含 reasoning 的消息不受影响。👍
✅ Local verification —
|
| # | Check | How | Result |
|---|---|---|---|
| 1 | New regression test, fix present | vitest (real source) |
✅ pass |
| 2 | Same test, fix reverted | vitest |
❌ fails — expected 'First reasoning.' to be 'First reasoning.Second reasoning.' |
| 3 | Same test, fix restored | vitest |
✅ pass |
| 4 | Full converter.test.ts suite |
vitest |
✅ 135/135, no regressions |
| 5 | Compiled dist artifact, fix ON |
node harness on real convertGeminiRequestToOpenAI |
✅ reasoning merged |
| 6 | Compiled dist artifact, fix OFF (rebuilt) |
same harness | ❌ reasoning dropped |
Steps 2 and 6 are the key A/B: toggling only the production hunk flips the outcome at both the TS-source level and the compiled-artifact level — so the fix is provably the cause, and the shipped code (not just the test) carries the corrected behavior.
The compiled-artifact harness ran the real production entry point on two realistic histories:
- A — two consecutive reasoning turns → merged reasoning
"First reasoning.Second reasoning."✅ - B — a
reasoning + tool_callturn followed by areasoning + answerturn (also exercises the post-cleanOrphanedToolCallsmerge) → merged reasoning"Plan: call the tool.Reflect on result."✅
With the fix removed and the artifact rebuilt, both collapse to only the first turn's reasoning ("First reasoning." / "Plan: call the tool.").
Verdict
✅ Correct, causal, and regression-free — LGTM for merge.
Scope note: this converter is an internal request-pipeline transform (it runs on every request to an OpenAI-compatible provider), so it was verified through its real public entry convertGeminiRequestToOpenAI plus the compiled shipped artifact with A/B toggling, rather than a black-box TUI session — consecutive assistant turns arise from history states (e.g. post-tool-cleanup / context compression) that can't be produced deterministically from interactive input.
中文版(点击展开)
✅ 本地验证 —— 合并 assistant 轮次时保留 reasoning_content
在本地对本 PR 做了真实的、带 A/B 开关的测试验证(在 tmux 中运行),并且同时在「源码测试」层面和「编译后的发布产物」层面进行。基于 PR head 2e5152e5 构建 · Linux · Node v22。
本次修复做了什么
converter.ts 中的 mergeConsecutiveAssistantMessages 现在会把每个被合并轮次的 reasoning_content 拼接起来,方式与它原本拼接 content、tool_calls 完全一致。修复前,当两个连续的 assistant/model 轮次被合并时,只有「累加方」那一轮的 reasoning 会保留,被合并掉的那一轮的 reasoning 会被静默丢弃 —— 这与已经会保留 reasoning 的 cleanOrphanedToolCalls 不一致。
验证结果
| # | 检查项 | 方式 | 结果 |
|---|---|---|---|
| 1 | 新增回归测试,带修复 | vitest(真实源码) |
✅ 通过 |
| 2 | 同一测试,回退修复 | vitest |
❌ 失败 —— expected 'First reasoning.' to be 'First reasoning.Second reasoning.' |
| 3 | 同一测试,恢复修复 | vitest |
✅ 通过 |
| 4 | 完整 converter.test.ts 套件 |
vitest |
✅ 135/135,无回归 |
| 5 | 编译后的 dist 产物,开启修复 |
对真实 convertGeminiRequestToOpenAI 跑 node 测试脚本 |
✅ reasoning 已合并 |
| 6 | 编译后的 dist 产物,关闭修复(重新构建) |
同一脚本 | ❌ reasoning 被丢弃 |
第 2 步和第 6 步是关键的 A/B:仅切换生产代码这一处改动,就会在 TS 源码层面和编译产物层面同时改变结果 —— 因此可证明该修复正是问题根因,且最终发布的代码(而不仅仅是测试)确实带上了修复后的行为。
编译产物测试脚本对两段真实历史调用了真实的生产入口:
- A —— 两个连续的 reasoning 轮次 → 合并后的 reasoning 为
"First reasoning.Second reasoning."✅ - B —— 一个
reasoning + tool_call轮次后接一个reasoning + answer轮次(同时覆盖cleanOrphanedToolCalls之后的二次合并)→ 合并后的 reasoning 为"Plan: call the tool.Reflect on result."✅
去掉修复并重新构建产物后,两段都退化为只剩第一轮的 reasoning("First reasoning." / "Plan: call the tool.")。
结论
✅ 修复正确、为问题根因、且无回归 —— 可以合并。
范围说明:该 converter 是请求管线内部的转换逻辑(每次请求 OpenAI 兼容 provider 时都会执行),因此通过其真实公开入口 convertGeminiRequestToOpenAI 加编译后的发布产物、配合 A/B 开关来验证,而非黑盒 TUI 会话 —— 连续的 assistant 轮次来自特定的历史状态(例如工具调用清理后、上下文压缩后),无法通过交互输入稳定复现。
What this PR does
When two assistant turns end up next to each other,
mergeConsecutiveAssistantMessagesfolds them into one. It already concatenates their text content and joins their tool calls, but it keeps only the reasoning of the first turn and throws away the reasoning of the one it merges in. This change concatenatesreasoning_contentfrom both turns the same way the visible content is concatenated, so nothing is lost when the turns collapse.Why it's needed
reasoning_contentis a real field we put on the request: when a model turn carries thoughts but no visible text we deliberately setcontent: ""(notnull) so OpenAI-compatible providers like Ollama don't reject the request with HTTP 400 whilereasoning_contentis present. The sibling helpercleanOrphanedToolCallsis careful to carryreasoning_contentover when it rewrites assistant turns, and the merge step runs right after it (and again after orphan cleanup can make two assistant turns adjacent). So the merge path is the one place that silently drops chain-of-thought that the rest of the pipeline goes out of its way to preserve. After this change the two paths agree.Reviewer Test Plan
How to verify
Added a unit test under the existing
mergeConsecutiveAssistantMessagesdescribe block: two consecutivemodelturns, each with athought: truepart plus visible text. Expected after merge —contentis"First answer.Second answer."andreasoning_contentis"First reasoning.Second reasoning.". Onmainthe test fails with the second turn's reasoning missing ("First reasoning."); with the fix it passes. The wholeconverter.test.tsfile (135 tests) stays green.Evidence (Before & After)
N/A — not a user-visible / TUI change.
Tested on
Environment (optional)
Unit tests only (vitest), no runtime/sandbox needed.
Risk & Scope
reasoning_content; it sets the field only when the combined string is non-empty, so messages without reasoning are byte-for-byte unchanged.reasoning_contentis produced or consumed elsewhere — purely the merge step.Linked Issues
None.
中文说明
这个 PR 做了什么
当两个 assistant 回合相邻时,
mergeConsecutiveAssistantMessages会把它们合成一个。它已经会拼接文本 content、合并 tool calls,但只保留了第一个回合的 reasoning,把被合并进来的那个回合的 reasoning 丢掉了。这里把两个回合的reasoning_content按照合并 content 同样的方式拼接起来,合并时不再丢东西。为什么需要
reasoning_content是我们真正会放进请求里的字段:当一个 model 回合只有思考、没有可见文本时,我们故意把content设成""而不是null,这样 Ollama 这类 OpenAI 兼容 provider 在带着reasoning_content时不会返回 HTTP 400。兄弟函数cleanOrphanedToolCalls在改写 assistant 回合时会特意保留reasoning_content,而合并这一步紧跟在它后面跑(orphan 清理可能让两个 assistant 回合变相邻,之后还会再跑一次合并)。所以合并这一步是整条流水线里唯一会悄悄丢掉思维链的地方。改完之后两条路径就一致了。如何验证
在已有的
mergeConsecutiveAssistantMessagesdescribe 块里加了一个单测:两个连续的model回合,每个都带一个thought: true部分加可见文本。合并后期望content为"First answer.Second answer."、reasoning_content为"First reasoning.Second reasoning."。在main上这个测试会失败,第二个回合的 reasoning 丢失(只剩"First reasoning.");带上修复后通过。整个converter.test.ts(135 个测试)保持全绿。风险
极低。新分支只在两个 assistant 消息合并、且至少一个带
reasoning_content时才执行,并且只在拼出的字符串非空时才写该字段,所以没有 reasoning 的消息一字节都不变。无破坏性改动。