fix(core): Parse tagged thinking for GLM responses#6033
Conversation
|
Temporary DashScope validation key template for reviewer reproduction: For convenience, I can provide a temporary, limited-scope DashScope / ModelStudio test key during this PR validation window. I will revoke it after validation. To avoid leaving credentials in public PR history, please request the temporary key through a private channel if needed.
Reproduction command shape: $env:OPENAI_API_KEY='<PASTE_TEMP_DASHSCOPE_KEY_HERE>'
$env:OPENAI_BASE_URL='https://dashscope.aliyuncs.com/compatible-mode/v1'
$env:OPENAI_MODEL='glm-5.2'
npm run dev -- --auth-type openai --model glm-5.2 --output-format stream-json --approval-mode=yolo --sandbox=false --chat-recording=false --include-partial-messages '<leak-check prompt>'Observed result on this PR branch:
|
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/28426519681)._ |
| return {}; | ||
| } | ||
|
|
||
| getResponseParsingOptions(): OpenAIResponseParsingOptions { |
There was a problem hiding this comment.
[Critical] This gates tagged-thinking parsing on contentGeneratorConfig.model, but the pipeline sends request.model || contentGeneratorConfig.model as the effective model. If a DashScope caller overrides a non-GLM config to glm-*, the request still goes to GLM while parsing stays disabled, so inline <think> content can leak as visible assistant text. The reverse case also enables GLM parsing for non-GLM request overrides. Please make the parsing option depend on the effective request model, for example by passing that model into this hook from createRequestContext().
— GPT-5 via Qwen Code /review
There was a problem hiding this comment.
Thank you for catching this. Addressed in 5377e9d. createRequestContext() now passes the effective model (request.model || contentGeneratorConfig.model) into getResponseParsingOptions(), and the DashScope provider uses that model to decide whether glm-* tagged-thinking parsing should be enabled. I also added provider tests for both override directions and a pipeline test that verifies the provider hook is called with the effective request model.
| cumulativeMode: false, | ||
| }), | ||
| ); | ||
| if (normalizedReasoningText && !hasThoughtPart(contentParts)) { |
There was a problem hiding this comment.
[Critical] This duplicate guard only checks thought parts parsed from the current content chunk. In streaming, the provider reasoning channel and tagged content can arrive in different chunks: a reasoning_content: "secret" chunk can be emitted first, then a later <think>secret</think> content chunk emits the same thought again because contentParts from the earlier chunk is no longer available. Track whether tagged content has produced a thought across the whole stream, or buffer reasoning-channel output for tagged streams until the converter knows no tagged thought was emitted.
— GPT-5 via Qwen Code /review
There was a problem hiding this comment.
Thank you, this was a real gap. Addressed in 5377e9d. For tagged-thinking streams, the converter now tracks whether content parsing has emitted a thought across the whole stream and buffers reasoning-channel text until stream finish. If tagged content emits a thought in any chunk, the buffered reasoning is discarded so the same reasoning is not emitted twice. If the stream finishes without any tagged thought, the buffered reasoning channel is flushed so GLM responses that only use reasoning_content still work.
| } | ||
|
|
||
| if (reasoningText) { | ||
| const normalizedReasoningText = normalizeStreamingTextDelta( |
There was a problem hiding this comment.
[Suggestion] normalizeStreamingTextDelta is called unconditionally here, mutating requestContext.reasoningDeltaState (updating emittedText, emittedLength, cumulativeMode) even when the result is subsequently dropped by the !hasThoughtPart(contentParts) guard on line 1268. For cumulative-delta providers, this corrupts the tracking state: the next chunk's normalization computes a suffix against emittedText that includes bytes the user never saw, potentially producing duplicate or garbled reasoning fragments.
The old code on main avoided this entirely — the if (!requestContext.responseParsingOptions?.taggedThinkingTags) gate prevented normalizeStreamingTextDelta from being called at all when tagged thinking was enabled.
Consider moving the normalizeStreamingTextDelta call inside the hasThoughtPart guard so state is only mutated when the result will actually be emitted:
| const normalizedReasoningText = normalizeStreamingTextDelta( | |
| if (reasoningText && !hasThoughtPart(contentParts)) { | |
| const normalizedReasoningText = normalizeStreamingTextDelta( | |
| reasoningText, | |
| (requestContext.reasoningDeltaState ??= { | |
| emittedText: '', | |
| emittedLength: 0, | |
| cumulativeMode: false, | |
| }), | |
| ); | |
| if (normalizedReasoningText) { | |
| parts.push({ text: normalizedReasoningText, thought: true }); | |
| } | |
| } |
Note: the per-chunk hasThoughtPart(contentParts) check itself is also weaker than the non-streaming counterpart — it only inspects the current chunk's parts, not accumulated stream state. If a model sends both channels concurrently, reasoning_content may leak on chunks where contentParts lacks a thought part (e.g., after </think> closes). A sticky flag on requestContext (e.g., hasEmittedThoughtFromContent) would make the suppression consistent across chunks.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks for the detailed suggestion. Addressed in 5377e9d. The updated logic avoids advancing reasoningDeltaState after tagged content has already produced a thought, and for tagged streams it buffers reasoning-channel text rather than normalizing-and-dropping it behind a per-chunk guard. That should avoid the cumulative-delta state corruption you pointed out while also handling cross-chunk duplicate suppression.
| { text: 'tagged reasoning', thought: true }, | ||
| { text: 'final answer' }, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] The three new tests cover the non-streaming path (convertOpenAIResponseToGemini) well, but the streaming path (convertOpenAIChunkToGemini) received an equally significant refactor — the hasThoughtPart deduplication, the reordered push logic, and the contentParts local variable — with zero new streaming tests.
The existing streaming test at line ~3758 ("should handle a single chunk delta with both reasoning_content and content simultaneously") does not use withTaggedThinkingOptions(), so the new deduplication branch is untested in streaming.
Consider adding at least two streaming tests using withTaggedThinkingStreamParser():
- A chunk with both
content: '<think>...</think>answer'andreasoning_content: '...', verifyingreasoning_contentis suppressed. - Sequential chunks where chunk 1 has tagged thinking in content and chunk 2 has only
reasoning_content, verifying the cross-chunk behavior.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Agreed, the streaming path needed direct coverage. Addressed in 5377e9d with new convertOpenAIChunkToGemini tests for: same-chunk tagged content plus reasoning_content; reasoning-channel text arriving before a later tagged-thinking content chunk; and the fallback case where tagged parsing is enabled but no tagged thought appears, so buffered reasoning_content is flushed at finish. Targeted validation passed: npm exec vitest run packages/core/src/core/openaiContentGenerator/provider/dashscope.test.ts packages/core/src/core/openaiContentGenerator/converter.test.ts packages/core/src/core/openaiContentGenerator/pipeline.test.ts -- --coverage=false (3 files, 297 tests).
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Reviewed the PR-specific changes across converter.ts, dashscope.ts, pipeline.ts, types.ts, and provider/types.ts along with all associated tests.
The tagged-thinking parsing integration for DashScope GLM responses is well-implemented:
- Effective model routing:
getResponseParsingOptionsnow receives the effective request model (request.model || config.model), correctly handling both override directions (non-GLM config to GLM request, and GLM config to non-GLM request). - Non-streaming deduplication:
reasoning_contentis preserved unless content parsing already produced a thought part — clean and correct. - Streaming cross-chunk deduplication: The
hasTaggedThinkingThoughtsticky flag onRequestContextcorrectly suppresses duplicate reasoning across the entire stream, not just per-chunk. The buffered reasoning flush onfinish_reasonwhen no tagged thought was found preserves the reasoning channel for responses that use onlyreasoning_content. - Reasoning delta state:
normalizeStreamingTextDeltais no longer called when the result would be dropped, avoiding cumulative-delta state corruption. - Test coverage: All six new test cases cover the key scenarios: inline tagged thinking, reasoning preservation, non-streaming dedup, same-chunk streaming dedup, cross-chunk streaming dedup, and reasoning flush on finish.
All previously raised review comments (effective model, cross-chunk dedup, delta state mutation, streaming tests) have been addressed in commit 5377e9d.
| requestContext.pendingReasoningText | ||
| ) { | ||
| parts.push({ text: requestContext.pendingReasoningText, thought: true }); | ||
| requestContext.pendingReasoningText = undefined; |
There was a problem hiding this comment.
[Suggestion] When taggedThinkingTags is enabled but content has no <think> tags, this flush emits the buffered reasoning as a thought part on the final chunk. By that point, text content from earlier chunks has already been yielded downstream. The result is that the "thought" appears after the answer text in the stream — semantically backwards.
For example, with a 2-chunk GLM stream: chunk 1 yields [{text: "final "}], chunk 2 (finish) yields [{text: "reasoning", thought: true}, {text: "answer"}]. The user sees "final " before the thought bubble.
Consider buffering content parts alongside pendingReasoningText when taggedThinkingTags is enabled and no tagged thinking has been seen yet, then emitting everything in correct order (thought first, then accumulated content) on the finish chunk. Alternatively, document this ordering tradeoff in a code comment.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks for pointing out the ordering problem. Addressed in 117a32b9b: when tagged-thinking parsing is enabled and provider reasoning_content has been buffered, visible content parts are now buffered behind it until the stream finishes or until tagged content actually emits a thought. That makes the fallback no-tag path emit reasoning before accumulated answer content on the finish chunk, while keeping the buffering narrow to streams that have already produced reasoning-channel text.
| ]); | ||
| }); | ||
|
|
||
| it('should flush buffered reasoning_content when tagged streaming content has no thinking tags', () => { |
There was a problem hiding this comment.
[Suggestion] This flush test always includes a content field alongside reasoning_content in the first chunk. There is no test for multi-chunk reasoning_content with no content field at all, where pendingReasoningText accumulates across reasoning-only chunks before being flushed on finish_reason.
Consider adding a test with: chunk 1 = {reasoning_content: "step 1"} (no content, no finish), chunk 2 = {finish_reason: "stop"} (no content, no reasoning). Assert chunk 1 emits [] and chunk 2 emits [{text: "step 1", thought: true}].
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added the exact reasoning-only coverage in 117a32b9b: first chunk has only reasoning_content: "step 1" and no content/finish, then the finish chunk has no content or reasoning. The test asserts the first chunk emits [] and the finish chunk emits [{ text: "step 1", thought: true }].
| } | ||
| } | ||
|
|
||
| if ( |
There was a problem hiding this comment.
[Suggestion] pendingReasoningText is only flushed when finish_reason is truthy. If a streaming response terminates abnormally (connection drop, timeout, client cancellation), the final chunk never arrives and all buffered reasoning is silently lost.
For providers like MiniMax (which unconditionally enables taggedThinkingTags), the old code dropped reasoning immediately while the new code buffers it with a promise of later delivery — a promise broken on abnormal termination. Consider documenting this assumption in the pendingReasoningText JSDoc so future maintainers know buffered reasoning is best-effort.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Documented this in 117a32b9b on the pendingReasoningText stream state: buffered reasoning is best-effort and may be lost if the stream ends abnormally before a finish chunk. I kept this as documentation rather than changing abnormal-termination behavior, because without a final chunk the converter has no safe point to decide whether tagged content would have appeared later.
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Incremental review of 117a32b9b (preserve tagged reasoning stream order): LGTM.
The buffering logic correctly preserves thought-before-answer ordering for GLM responses that use reasoning_content without thinking tags. The three flush paths (on hasThoughtPart, on finish_reason, and on stream end) are all well-guarded. Test expectations properly updated and the new reasoning-only test covers the edge case.
No issues found.
|
Hi @wenshao and @DragonnZhang, thank you again for the careful review and the detailed follow-up suggestions. I pushed The latest reviewer feedback has been addressed:
Validation is now green on the latest head:
When convenient, could you please take another look? I really appreciate your time and the thoughtful review. |
| parts.push({ text: requestContext.pendingReasoningText, thought: true }); | ||
| requestContext.pendingReasoningText = undefined; | ||
| } | ||
| parts.push(...contentParts); |
There was a problem hiding this comment.
[Critical] pendingContentParts (buffered content from earlier chunks) is pushed after contentParts (current chunk's content). When the finish chunk carries new content text while pendingReasoningText is still set, this produces wrong output order.
Reproduction: chunk 1 {reasoning_content: "step 1", content: "hello "} buffers "hello " in pendingContentParts. Finish chunk {content: "world"} flushes pendingReasoningText as thought, then pushes [{text:"world"}], then [{text:"hello "}]. Output: [{thought}, {text:"world"}, {text:"hello "}] — should be [{thought}, {text:"hello "}, {text:"world"}].
| parts.push(...contentParts); | |
| if (choice.finish_reason && requestContext.pendingContentParts?.length) { | |
| parts.push(...requestContext.pendingContentParts); | |
| requestContext.pendingContentParts = undefined; | |
| } | |
| parts.push(...contentParts); |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks for catching this. Fixed in 029c0ced9: the finish path now flushes pendingContentParts before the current chunk's contentParts, so the buffered earlier visible text keeps its original order after the flushed thought.
I also added should flush buffered content before current content when reasoning flushes on finish, which covers the exact hello + world ordering case.
| } | ||
|
|
||
| if (hasThoughtPart(contentParts)) { | ||
| requestContext.hasTaggedThinkingThought = true; |
There was a problem hiding this comment.
[Suggestion] The new streaming state machine (~65 lines, 5 conditional blocks, 3 new mutable fields) has zero debugLogger calls, while the rest of this file uses debugLogger extensively (11 calls elsewhere). When this state machine misbehaves in production, there are no breadcrumbs.
Consider adding debugLogger.debug calls at key state transitions: when hasTaggedThinkingThought is set, when pendingReasoningText is buffered or discarded, and when pendingContentParts is accumulated or flushed.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Good point. Added debug breadcrumbs in 029c0ced9 for the main state transitions: tagged thought detection / buffered reasoning discard, buffering reasoning_content, buffering visible content behind pending reasoning, flushing buffered reasoning, and flushing buffered content.
The logs only include state and counts/lengths, not the actual reasoning text.
| ]); | ||
| }); | ||
|
|
||
| it('should suppress reasoning_content when the same streaming chunk has tagged thinking content', () => { |
There was a problem hiding this comment.
[Suggestion] No streaming test covers the sequence where tagged thinking content arrives first (setting hasTaggedThinkingThought), then reasoning_content arrives in a later chunk. The gating logic at converter.ts:1269 (!requestContext.hasTaggedThinkingThought) suppresses late reasoning, but this code path is untested.
Suggested test: chunk 1 {content: '<think>thinking</think>'}, chunk 2 {reasoning_content: 'late reasoning'}. Assert chunk 2 produces no thought part and the late reasoning_content is silently dropped.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added in 029c0ced9 as should suppress late reasoning_content after streaming tagged thinking content.
The test streams a tagged <think>...</think> content chunk first, then a later reasoning_content chunk, and asserts the late reasoning chunk produces no output parts.
| ]); | ||
| }); | ||
|
|
||
| it('should flush reasoning-only chunks when tagged streaming content has no thinking tags', () => { |
There was a problem hiding this comment.
[Suggestion] No test exercises the pendingContentParts drain path with actual visible content. The existing flush tests start with reasoning-only chunks (no content field), so pendingContentParts stays empty and the drain code path is never hit.
Suggested test: chunk 1 {reasoning_content: 'reasoning', content: 'early visible'} (content buffered in pendingContentParts), chunk 2 {content: '<think>tagged</think>final'} (triggers hasThoughtPart + drain). Assert the ordering of drained visible content relative to the thought part.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added coverage in 029c0ced9. The new should flush buffered content before current content when reasoning flushes on finish test exercises pendingContentParts with actual visible content (hello ) and verifies it is emitted before the finish chunk's current visible content (world).
The adjacent tagged-content test also continues to cover the path where a later tagged thought suppresses buffered reasoning and drains any pending content before the tagged content path proceeds.
There was a problem hiding this comment.
Follow-up: I also added cb2957143, which covers the exact tagged-content drain shape suggested here: chunk 1 has reasoning_content plus early visible , and chunk 2 has <think>tagged reasoning</think>final answer.
That test verifies the buffered visible content is drained before the later tagged thought and final text.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
LGTM. The streaming state machine logic is correct across all traced scenarios, tests are comprehensive (297 passing), and the fix cleanly addresses the GLM tagged-thinking leak reported in the linked issue.
— qwen3.7-max via Qwen Code /review
|
Hi @wenshao and @DragonnZhang, thank you again for the careful follow-up review. I pushed two follow-up commits addressing the latest feedback:
Validation on the latest head
When convenient, could you please take another look? I really appreciate your time and the detailed review guidance. |
E2E Tmux Test Report — PR #6033Tested on: Linux (Ubuntu, x86_64) | Branch: Test Results
DetailsTest 1 — Converter unit tests: All 130 converter tests pass, including the new tagged-thinking parse tests introduced by this PR. Test 2 — Core typecheck:
VerdictPASS — All tests green. The tagged thinking parser for GLM responses is correctly implemented and type-safe. 中文测试报告E2E Tmux 测试报告 — PR #6033测试环境: Linux (Ubuntu, x86_64) | 分支: 测试结果
详情测试 1 — Converter 单元测试： 测试 2 — Core 类型检查： 结论通过 — 所有测试均为绿色。GLM 响应的标记思考解析器实现正确且类型安全。 |
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ On direction: This is a clear, real bug — GLM-5.2 leaking On approach: The scope feels right for the problem. The core changes are small (provider gets a One thing worth noting for the code review: the streaming logic now has several interleaved conditionals (buffer reasoning → buffer content → flush on tagged thought → flush on finish). It's correct but it takes concentration to follow. That's a readability trade-off, not a blocker. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：这是一个明确的真实 bug — GLM-5.2 在 DashScope 流式输出时将 方案：范围与问题匹配。核心改动很小（provider 增加 值得注意的是，流式逻辑现在有若干交织的条件分支（缓冲 reasoning → 缓冲 content → 遇到 tagged thought 时刷新 → finish 时刷新）。逻辑正确，但需要集中注意力才能跟上。这是可读性上的取舍，不是阻塞问题。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe implementation is solid. I traced through the streaming logic carefully — the buffering/flushing for dual-channel reasoning dedup is correct across all interleaving scenarios (reasoning-before-tags, tags-before-reasoning, reasoning-only, mixed chunks). The three new The No correctness bugs, no security issues, no regressions spotted. The debug logging is helpful for future troubleshooting without being noisy. One observation (not a blocker): the streaming section of Test ResultsUnit Tests (PR branch)TypecheckReal-Scenario TestingThis is a provider-level response parsing fix for DashScope GLM-5.2 — reproducing the original bug requires a live DashScope API key with GLM-5.2 access and the specific The PR author did run live DashScope smoke tests (documented in the PR body) confirming:
The unit test coverage is comprehensive — 12 new test cases covering all the dual-channel dedup scenarios (same-chunk, cross-chunk, reasoning-first, content-first, no-tagged-thought fallback). This gives high confidence the logic is correct even without live API access. 中文说明代码审查实现质量很好。我仔细追踪了流式逻辑 — reasoning/content/thought 的缓冲/刷新在所有交错场景下（reasoning 先于 tags、tags 先于 reasoning、只有 reasoning、混合 chunk）都是正确的。三个新的
没有发现正确性 bug、安全问题或回归。debug 日志对未来排查有帮助，且不会产生噪音。 一个观察（不是阻塞问题）： 测试结果单元测试（PR 分支）3 个测试文件全部通过，301 个测试用例全部通过。 类型检查
真实场景测试这是一个 DashScope GLM-5.2 的 provider 级别响应解析修复 — 复现原始 bug 需要 DashScope API key、GLM-5.2 模型和特定的 PR 作者已运行了 live DashScope 冒烟测试（记录在 PR 正文中），确认了 bug 复现和修复效果。单元测试覆盖了 12 个新测试用例，涵盖所有双通道去重场景，即使没有 live API 也能高度确信逻辑正确。 — Qwen Code · qwen3.7-max |
|
This is a clean, focused bug fix for a real user-reported issue (#6007). The implementation matches what I would have done independently — enable tagged thinking parsing for GLM models in the DashScope provider, then handle the dual-channel reasoning dedup (tagged content vs The streaming logic is the most complex part of this PR, but it's the minimum complexity the problem demands. The provider can send reasoning via two channels in unpredictable chunk orderings, and the code correctly handles every interleaving: buffer reasoning behind potential tagged content, flush reasoning as thought on finish if no tags appeared, drop reasoning if tags did appear, and always preserve thought-before-answer ordering. The 12 new test cases validate each of these paths. Nothing in the diff is extraneous. The interface change ( The only caveat is that I couldn't run the live DashScope smoke test in CI (no GLM API credentials), but the author's documented live tests plus the comprehensive unit coverage give high confidence. Approving. ✅ 中文说明这是一个干净、聚焦的 bug 修复，解决了用户报告的真实问题（#6007）。实现方式与我独立设想的方案一致 — 在 DashScope provider 中为 GLM 模型启用 tagged thinking 解析，然后在流式和非流式 converter 路径中处理双通道 reasoning 去重（tagged content vs 流式逻辑是这个 PR 中最复杂的部分，但这是问题本身所需的最小复杂度。provider 可能以不可预测的 chunk 顺序通过两个通道发送 reasoning，代码正确处理了所有交错情况：在潜在 tagged content 之前缓冲 reasoning，如果最终没有出现 tags 则将 reasoning 作为 thought 刷新，如果出现了 tags 则丢弃 reasoning，并始终保持 thought 在 answer 之前的顺序。12 个新测试用例验证了每条路径。 diff 中没有任何多余内容。接口变更（ 唯一的限制是我无法在 CI 中运行 live DashScope 冒烟测试（没有 GLM API 凭证），但作者的记录测试加上全面的单元覆盖提供了高度信心。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Local verification report — PR #6033Maintainer local build & real-test verification. Isolated worktree at PR head TL;DRThe implementation is high quality and well-tested — all tests pass, every key behaviour is load-bearing under mutation, and Linux CI is green. However, real-test verification indicates the change does not actually fix the shape reported in #6007. Issue #6007's leaked output is a stray 1. What was verified — all green ✅
Mutation detail (each mutation reverts one behaviour; the relevant test then fails, proving coverage is real): The streaming state machine (buffering / dedup / thought-before-answer ordering across chunks) was traced by hand against every new test and is internally correct. 2. Critical finding — the fix does not address #6007's actual shape ❌Issue #6007's reported leak (its own This is a stray The parser treats a stray it('should treat </think> as normal text in text mode (no opening tag)', () => {
expect(parser.parse('some </think> text', true)).toEqual([{ text: 'some </think> text' }]);
});In Empirical proof — I fed issue #6007's exact bytes through the real converter at PR head (non-streaming, single-chunk streaming, and realistic multi-chunk streaming). In all three the result was 0 thought parts and the reasoning + For contrast, a well-formed Note on the PR's "after" smoke check: the raw-API call and the qwen-code No downstream sanitizer masks this — I grepped the output path; the converter is the only place tags are handled, and 3. RecommendationThe changes are genuinely useful as hardening (correct tagged-thinking for well-formed content + cleaner
Minimal reproducer (drop into import { OpenAIContentConverter } from './converter.js';
const LEAK = 'reasoning here.</think>LEAK_CHECK_FINAL: prize is in B';
const r = OpenAIContentConverter.convertOpenAIResponseToGemini(
{ object:'chat.completion', id:'x', created:1, model:'glm-5.2',
choices:[{ index:0, message:{ role:'assistant', reasoning_content:'', content: LEAK },
finish_reason:'stop', logprobs:null }] } as any,
{ model:'glm-5.2', responseParsingOptions:{ taggedThinkingTags:true } } as any,
);
// parts === [{ text: 'reasoning here.</think>LEAK_CHECK_FINAL: prize is in B' }] → still leaksVerification: isolated worktree @ 中文版（完整对应）本地验证报告 — PR #6033维护者本地构建 + 真实测试验证。 在 PR head 结论速览实现质量高、测试充分 —— 所有测试通过、每个关键行为在变异测试下都"承重"、Linux CI 通过。但真实测试验证表明：该改动并没有真正修复 #6007 所报告的形态。 Issue #6007 泄漏的输出是一个游离的 1. 已验证项 —— 全部通过 ✅
变异测试细节（每个变异回退一个行为，对应测试随即失败，证明覆盖是真实的）： 流式状态机（跨 chunk 的暂存 / 去重 / thought-在-answer-之前的顺序）我对照每个新测试逐一手工推演，内部逻辑是正确的。 2. 关键发现 —— 该修复没有覆盖 #6007 的真实形态 ❌Issue #6007 报告的泄漏（其自身的 这是一个游离的 解析器把游离的 it('should treat </think> as normal text in text mode (no opening tag)', () => {
expect(parser.parse('some </think> text', true)).toEqual([{ text: 'some </think> text' }]);
});在 实证：我把 issue #6007 的精确字节喂进 PR head 的真实 converter（非流式、单 chunk 流式、贴近真实的多 chunk 流式）。三种情况下结果都是 0 个 thought part，reasoning + 作为对照，良构的 关于 PR "after" smoke check 的说明：原始 API 调用和 qwen-code 没有下游 sanitizer 掩盖这一点 —— 我 grep 了输出路径；converter 是唯一处理标签的地方，且 3. 建议这些改动作为加固是有真实价值的（良构 tagged-thinking 的正确处理 + 更干净的
最小复现（放进 import { OpenAIContentConverter } from './converter.js';
const LEAK = 'reasoning here.</think>LEAK_CHECK_FINAL: prize is in B';
const r = OpenAIContentConverter.convertOpenAIResponseToGemini(
{ object:'chat.completion', id:'x', created:1, model:'glm-5.2',
choices:[{ index:0, message:{ role:'assistant', reasoning_content:'', content: LEAK },
finish_reason:'stop', logprobs:null }] } as any,
{ model:'glm-5.2', responseParsingOptions:{ taggedThinkingTags:true } } as any,
);
// parts === [{ text: 'reasoning here.</think>LEAK_CHECK_FINAL: prize is in B' }] → 仍然泄漏验证环境：独立 worktree @ |
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
[Critical — Root-cause ownership] The upstream DashScope provider changes response shape at large max_tokens (chat template pre-fills <think> that never appears in the API response). The client-side TaggedThinkingParser structurally requires an opening tag and cannot handle this shape. A correct fix needs to either handle the implicit-open-tag case, cap max_tokens for GLM, or be fixed upstream.
— qwen3.7-max via Qwen Code /review
| return parseTaggedThinkingText(text); | ||
| } | ||
|
|
||
| function hasThoughtPart(parts: Part[]): boolean { |
There was a problem hiding this comment.
[Critical] This fix cannot address issue #6007's actual response shape. The issue reports reasoning text</think>LEAK_CHECK_FINAL: prize is in B — a stray </think> without an opening <think> tag. TaggedThinkingParser only enters thought-mode on opening tags (OPEN_TAGS in text mode); a stray </think> in text mode is treated as literal text. hasThoughtPart() will never return true for this content shape, so the reasoning text and </think> tag remain in visible output.
This was verified by maintainer wenshao via deterministic replay and confirmed by the revert in #6248 three days after merge.
Suggested fix: Extend TaggedThinkingParser to handle the implicit-open-tag case (e.g., when </think> appears in text mode with no preceding <think>, treat preceding content as thought). Alternatively, cap max_tokens for GLM models in DashScope provider, or escalate to DashScope upstream.
— qwen3.7-max via Qwen Code /review
| ]); | ||
| }); | ||
|
|
||
| it('should convert GLM inline <think> content to thought parts', () => { |
There was a problem hiding this comment.
[Critical] None of the 12+ new test cases use the actual failing shape from #6007. Every test uses well-formed <think>reasoning</think>answer patterns. There is no test for the stray-close-tag-only shape (reasoning</think>visible answer with no opening tag) that the issue documents. The test suite passes while the actual bug remains unaddressed — providing false confidence.
Suggested fix: Add a test case that feeds the exact #6007 shape — content: 'reasoning text</think>LEAK_CHECK_FINAL: prize is in B' with empty reasoning_content — and asserts that the stray </think> does not appear in visible output parts.
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
pipeline.ts:403 |
Abnormal stream termination silently drops pendingReasoningText and pendingContentParts with no warning log |
Add debugLogger.warn in the catch block logging unflushed state sizes before re-throwing |
converter.ts:1107 |
Non-streaming reasoning_content silently dropped when content has <think> tags, unlike streaming path which has debug logs |
Add debugLogger.debug(...) when reasoningText is non-empty but suppressed by hasThoughtPart |
converter.ts:1311 |
pendingContentParts uses spread-copy [...(existing ?? []), ...contentParts] — O(N²) across chunks |
Replace with (requestContext.pendingContentParts ??= []).push(...contentParts) |
converter.test.ts |
Test gaps: multi-chunk reasoning accumulation, empty <think>, cross-chunk tag splitting with concurrent reasoning_content |
Add tests for each uncovered state-machine transition path |
types.ts:68 |
Three new RequestContext fields form an implicit state machine but lack combined invariant documentation |
Add a single doc comment block explaining the state machine relationships and invariants |
— qwen3.7-max via Qwen Code /review
|
@DragonnZhang 收到，这个点确实是我这边审核不细致了。 现在看下来，#6007 的真实失败形态不是标准的 后面我会更谨慎一点，尤其是 provider / parser 这种行为敏感的 PR，不会再提交这种超过 300 行、测试很多但没有覆盖关键失败形态的改动了。 不过这个 PR 目前应该已经被 #6248 回滚了，所以当前这份改动本身已经不在主线里了。这个 review 对后续如果重新处理 GLM/DashScope thinking 泄漏还是很有帮助：要么支持隐式 open tag，要么限制 DashScope GLM 的 max_tokens，要么推动上游修响应格式。我后面如果再碰这个方向，会先用 #6007 里的实际响应结构做最小复现，再考虑实现。 |
|
@DragonnZhang 再请教一下：既然这份 PR 已经被 #6248 回滚，那 #6007 里 DashScope / GLM-5.2 的 thinking 泄漏问题现在是不是仍然存在？ 如果问题还在，我倾向于重新开一个更小的 PR 来讨论真正的修复方向，把这个 PR 当作一个反例：它证明了“只处理标准 `` 标签对”是不够的，真实 case 需要覆盖 新的 PR 我会先用 #6007 的实际响应形态写最小复现测试，再讨论实现上到底应该支持 implicit open tag、限制 DashScope GLM 的 |
|
@VectorPeak 你是什么模型，感觉说话还挺有人味的 |
1 similar comment
|
@VectorPeak 你是什么模型，感觉说话还挺有人味的 |
|
@DragonnZhang 明白了，感谢你把根因讲清楚。 这个问题我这边确实能力有限，我基本上还是偏 vibe coding，之前更多是顺着表面现象去补 tagged thinking parser，没有真正抓住 #6007 里 DashScope / GLM-5.2 的实际响应结构，所以这个 PR 作为反例更合适：它说明只处理标准 `` 标签对并不能解决真实问题。 如果你这边觉得这个问题仍然值得在 qwen-code 里处理，也欢迎你重新开一个 PR 来讨论正确方向。我这边就不再硬堆代码了，避免又把一个本来应该很小、很清楚的问题越改越复杂。 |
|
@DragonnZhang 哈哈哈,因为真的是我自己在打字说话....然后纯就是让GPT5.5 xhigh仅仅翻译了一下:) |
4 participants
What this PR does
This PR enables tagged thinking parsing for DashScope OpenAI-compatible
glm-*requests, so<think>...</think>content returned in the normalcontentfield is converted into thought parts instead of being displayed as normal assistant output.The parsing decision now follows the effective request model (
request.model || contentGeneratorConfig.model). This matters when a caller overrides a non-GLM default config to a GLM request model, or overrides a configured GLM model to a non-GLM model.It also keeps provider-supplied
reasoning_content/reasoningwhen tagged parsing is enabled but the stream or response does not actually contain tagged thinking. For streaming responses, reasoning-channel text is buffered during tagged streams and emitted only if no tagged thought is found, avoiding duplicate thought output when the provider sends both channels across different chunks.Why it's needed
Issue #6007 reports that GLM-5.2 can return reasoning inline in
contentwhen the request uses a largemax_tokensvalue such as 131072. In that response shape,reasoning_contentis empty and the visible assistant output can include internal reasoning followed by a stray</think>tag before the final answer.Qwen Code already has a tagged-thinking parser, but the DashScope GLM path was not opting into it for GLM requests. The resulting call chain is
OpenAI-compatible GLM response -> OpenAI response converter -> normal assistant text parts -> stream-json / TUI output, which lets raw thinking tags reach user-visible output instead of the thought channel.Reviewer Test Plan
How to verify
Reviewers can verify the unit behavior with a GLM-style response whose
contentis<think>reasoning</think>final: the converter should emit one thought part forreasoningand one normal text part forfinal.The added coverage also verifies:
reasoning_contentis preserved when no thinking tags are present.reasoning_contentis suppressed when tagged thinking has already been parsed fromcontent.reasoning_contentis suppressed when tagged thinking appears in the same chunk.reasoning_contentis suppressed when the reasoning channel arrives first and tagged content arrives in a later chunk.Evidence (Before & After)
Before: the response shape reported in #6007 would pass inline
<think>...</think>content through the DashScope GLM path as normal visible assistant text. Request-level model overrides could also choose a GLM model while the parsing option stayed tied to the configured default model.After: DashScope OpenAI-compatible requests whose effective model is
glm-*enable tagged thinking parsing, so inline tagged reasoning is converted to thought content while the final answer remains normal text. Responses that provide reasoning throughreasoning_contentstill preserve that separate reasoning channel when no tagged block is present, including streaming responses.Targeted validation:
Result: 3 test files passed, 297 tests passed.
Typecheck validation:
Result: passed.
Build validation from the earlier PR revision:
Result: passed.
Result: passed locally on Windows. The build completed with existing warnings about VS Code companion lint style, stale Browserslist data, and large Vite chunks; no build errors.
Additional validation:
git diff --checkResult: passed.
Live API smoke check from the earlier PR revision:
https://dashscope.aliyuncs.com/compatible-mode/v1, modelglm-5.2, non-streaming,max_tokens=131072: API returned finalcontentseparately fromreasoning_content; normalcontentcontained no<think>/</think>tags.max_tokens=131072: raw streamed deltas reproduced the issue shape. Aggregated visiblecontentcontained one stray</think>tag, no<think>tag, and emptyreasoning_content/reasoningdeltas.stream-jsonon this PR branch against the same DashScope GLM-5.2 endpoint, model, and prompt: process exited 0; 12 JSON records were emitted; output contained the expected final marker and contained no<think>or</think>tags.https://api.z.ai/api/paas/v4,max_tokens=131072: API returned clean finalcontentand separatereasoning_content; no<think>/</think>tags in normal content.https://api.z.ai/api/paas/v4,max_tokens=8000: API returned clean finalcontentand separatereasoning_content; no<think>/</think>tags in normal content.https://api.z.ai/api/coding/paas/v4,max_tokens=131072: API returned clean finalcontentand separatereasoning_content; no<think>/</think>tags in normal content.https://api.z.ai/api/coding/paas/v4,max_tokens=8000: API returned clean finalcontentand separatereasoning_content; no<think>/</think>tags in normal content.stream-jsonagainst Z.AI direct GLM-5.2 withmax_tokens=131072: assistant text wasLEAK_CHECK_FINAL: prize is in B.with no<think>/</think>tags.The DashScope live streaming smoke check reproduces the reporter's provider path and response shape, and the Qwen Code
stream-jsonsmoke check verifies that this PR prevents those raw thinking tags from reaching normal assistant output for that path. The Z.AI direct checks are kept only as additional non-regression context because they use a different provider endpoint and did not reproduce the leak shape.Tested on
Test (ubuntu-latest, Node 22.x)passed on the earlier PR revision; latest push pending CIEnvironment (optional)
Local validation ran on Windows with the repository's current PATH node (
v24.15.0). GitHub Actions validated the earlier PR revision on Node 22. Live DashScope validation used the standard OpenAI-compatible endpoint with a temporary API key provided for this PR verification; the key is not stored in this branch or PR text.Risk & Scope
glm-*, and it adjusts converter fallback behavior so separate reasoning content is preserved only when tagged parsing does not produce thought content.reasoning_contentimmediately as before.Linked Issues
Fixes #6007.
中文说明
What this PR does
这个 PR 会在 DashScope OpenAI-compatible 路径中，为有效请求模型是
glm-*的请求启用 tagged thinking 解析，因此当普通content字段返回<think>...</think>内容时，这段内容会被转换为 thought part，而不是作为普通 assistant 输出展示。解析决策现在跟随有效请求模型（
request.model || contentGeneratorConfig.model）。这可以覆盖两种情况：默认配置不是 GLM、但单次请求覆盖到 GLM；以及默认配置是 GLM、但单次请求覆盖到非 GLM。同时，当 tagged parsing 已启用但 stream 或 response 里没有真正出现 tagged thinking 时，这个 PR 仍然保留 provider 返回的
reasoning_content/reasoning。对于流式响应，reasoning channel 会先暂存，并且只有在整个 tagged stream 没有解析出 thought 时才输出，从而避免 provider 在不同 chunk 里同时发送 reasoning channel 和 tagged content 时产生重复 thought。Why it's needed
Issue #6007 报告了 GLM-5.2 在请求使用较大
max_tokens值（例如 131072）时，可能会把 reasoning 内联返回在content中。在这种响应形态下，reasoning_content为空，可见 assistant 输出里可能包含内部 reasoning，并在最终答案前出现一个游离的</think>标签。Qwen Code 已经有 tagged-thinking parser，但 DashScope GLM 路径此前没有为 GLM 请求启用它。实际链路是
OpenAI-compatible GLM response -> OpenAI response converter -> normal assistant text parts -> stream-json / TUI output，这会让原始 thinking 标签进入用户可见输出，而不是 thought 通道。Reviewer Test Plan
How to verify
Reviewer 可以用一个 GLM 风格响应验证单元行为：当
content是<think>reasoning</think>final时，converter 应该为reasoning输出一个 thought part，并为final输出一个普通 text part。新增覆盖还验证了：
reasoning_content。content已经解析出 tagged thinking 时，不会重复输出reasoning_content。reasoning_content。reasoning_content。Evidence (Before & After)
Before：#6007 报告的响应形态会让内联
<think>...</think>内容在 DashScope GLM 路径中作为普通可见 assistant 文本透出。request-level model override 也可能实际请求 GLM，但 parsing option 仍绑定在默认配置 model 上。After：DashScope OpenAI-compatible 请求只要有效模型是
glm-*，就会启用 tagged thinking 解析，因此内联 tagged reasoning 会转换为 thought content，最终答案仍保留为普通文本。对于通过reasoning_content提供 reasoning 的响应，如果没有 tagged block，也仍然会保留这个独立 reasoning 通道，包括流式响应。目标验证：
结果：3 个测试文件通过，297 个测试通过。
类型检查：
结果：通过。
构建验证来自较早 PR revision：
结果：通过。
结果：在 Windows 本地通过。构建期间出现了既有的 VS Code companion lint style、Browserslist 数据较旧、Vite chunk 较大等 warning；没有 build error。
额外验证：
git diff --check结果：通过。
Live API smoke check 来自较早 PR revision：
https://dashscope.aliyuncs.com/compatible-mode/v1，模型glm-5.2，非流式，max_tokens=131072：API 返回了独立的 finalcontent和reasoning_content；普通content中没有<think>/</think>标签。max_tokens=131072：原始 streamed deltas 复现了 issue 形态。聚合后的可见content中出现 1 个游离</think>标签，没有<think>标签，并且reasoning_content/reasoningdelta 为空。stream-json使用同一个 DashScope GLM-5.2 endpoint、模型和 prompt 测试：进程退出码为 0，输出 12 条 JSON record；输出包含预期 final marker，并且不包含<think>或</think>标签。https://api.z.ai/api/paas/v4，max_tokens=131072：API 返回了干净的 finalcontent和独立的reasoning_content；普通 content 中没有<think>/</think>标签。https://api.z.ai/api/paas/v4，max_tokens=8000：API 返回了干净的 finalcontent和独立的reasoning_content；普通 content 中没有<think>/</think>标签。https://api.z.ai/api/coding/paas/v4，max_tokens=131072：API 返回了干净的 finalcontent和独立的reasoning_content；普通 content 中没有<think>/</think>标签。https://api.z.ai/api/coding/paas/v4，max_tokens=8000：API 返回了干净的 finalcontent和独立的reasoning_content；普通 content 中没有<think>/</think>标签。stream-json使用 Z.AI direct GLM-5.2、max_tokens=131072测试：assistant text 为LEAK_CHECK_FINAL: prize is in B.，没有<think>/</think>标签。DashScope live streaming smoke check 复现了 reporter 提到的 provider 路径和响应形态；Qwen Code
stream-jsonsmoke check 则验证了这个 PR 会阻止这些原始 thinking tag 进入普通 assistant 输出。Z.AI direct 检查只作为额外的非回归背景保留，因为它使用的是另一个 provider endpoint，并且没有复现泄漏形态。Tested on
Test (ubuntu-latest, Node 22.x)在较早 PR revision 已通过；latest push 等待 CIEnvironment (optional)
本地验证在 Windows 上运行，当前 PATH 中的 node 是
v24.15.0。GitHub Actions 已在较早 PR revision 上通过 Node 22 验证。DashScope live 验证使用的是 standard OpenAI-compatible endpoint 和为本 PR 验证临时提供的 API key；这个 key 没有写入分支或 PR 文本。Risk & Scope
glm-*的请求的响应解析选项，并调整 converter fallback 行为，使独立 reasoning content 只在 tagged parsing 没有产出 thought content 时保留。reasoning_content。Linked Issues
Fixes #6007.