[codex] Revert GLM tagged thinking parsing for DashScope#6248
Merged
Conversation
tanzhenxin
marked this pull request as ready for review
July 3, 2026 07:44
tanzhenxin
enabled auto-merge
July 3, 2026 07:46
Collaborator
|
Thanks for the PR! Template looks good ✓ On direction: this is a clean revert of a regression — strong alignment. PR #6033 introduced tagged-thinking parsing for DashScope On approach: the revert is tight — 46 additions vs 638 deletions, all within the converter/pipeline/provider layer. No drive-by refactors, no scope creep. The one non-deletion change (removing the Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：这是回归修复——强对齐。PR #6033 为 DashScope 方案：回滚范围紧凑——46 行增加 vs 638 行删除，全部在 converter/pipeline/provider 层。没有顺手重构，没有范围蔓延。唯一的非删除改动（移除 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
|
Code review: This is a clean revert — the diff removes exactly what PR #6033 added, with no new logic introduced. The restored converter behavior is straightforward: reasoning_content is always extracted into thought parts (for non-tagged providers), and text content is always passed through without buffering. The removed buffering state machine ( The No correctness issues, no security concerns. All 292 unit tests pass across the three affected test files. Typecheck clean. Real-scenario testing: The behavioral difference (buffered vs immediate streaming) is only visible with a DashScope GLM API key, which is not available in this CI environment. For non-GLM models, both builds behave identically — tested with a simple prompt to confirm the CLI still works. Before (installed build) After (this PR) Both respond correctly. The GLM-specific streaming buffering behavior that this PR removes would require DashScope GLM credentials to demonstrate the before/after difference. 中文说明代码审查： 这是一个干净的回滚——diff 精确移除了 PR #6033 添加的内容，没有引入新逻辑。恢复后的 converter 行为很直接：reasoning_content 始终被提取为 thought parts（对于非 tagged provider），文本内容始终直接传递不做缓冲。被移除的缓冲状态机（
没有正确性问题，没有安全隐患。三个受影响的测试文件全部 292 个单元测试通过。Typecheck 通过。 真实场景测试： 行为差异（缓冲 vs 即时流式）仅在使用 DashScope GLM API key 时可见，当前 CI 环境没有该凭证。对于非 GLM 模型，两个构建行为一致——使用简单提示测试确认 CLI 正常工作。 — Qwen Code · qwen3.7-max |
Collaborator
|
This is a textbook revert: it removes exactly what broke, restores the simpler behavior, and doesn't try to sneak in anything else. The original PR #6033 tried to solve #6007 (GLM thinking text leaking as visible output) with a broad opt-in to tagged-thinking parsing for all DashScope GLM models, but that opt-in changed streaming semantics in a way that made long GLM requests appear completely silent in the TUI and caused logging to miss successful responses. The right call here is to revert first and fix #6007 with a more targeted approach later — one that handles the malformed The diff is clean, the streaming path is simpler without the buffering logic, tests pass, typecheck is clean. One thing to note: the original #6007 issue is still open after this revert, so a follow-up fix will be needed for the GLM thinking-text leak at large LGTM — ships the revert cleanly. ✅ 中文说明这是一个教科书式的回滚：精确移除引入问题的改动，恢复更简单的行为，没有夹带任何其他内容。原始 PR #6033 试图用为所有 DashScope GLM 模型广泛启用 tagged-thinking 解析来解决 #6007（GLM thinking 文本泄漏为可见输出），但这个 opt-in 改变了流式语义，导致长时间 GLM 请求在 TUI 中完全无输出，并导致日志漏记成功响应。 正确的做法是先回滚，之后再用更有针对性的方法修复 #6007——直接处理畸形的 Diff 干净，移除缓冲逻辑后流式路径更简单，测试通过，typecheck 通过。需要注意：这个回滚后 #6007 仍然未解决，后续需要一个针对 GLM 大 同意合入——回滚干净利落。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jul 3, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| // When taggedThinkingTags is enabled, thought content is already | ||
| // extracted from the text content via convertOpenAITextToParts. | ||
| // Skip reasoning_content extraction to avoid duplicating thought parts. | ||
| if (!requestContext.responseParsingOptions?.taggedThinkingTags) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The !taggedThinkingTags guard that suppresses reasoning_content extraction has no test coverage. No remaining test sends a response with both taggedThinkingTags: true and a reasoning_content field — the MiniMax tests only use <think> tags in content without reasoning_content. If this guard is accidentally removed or inverted, no test would catch the regression.
Consider adding a test that sends both reasoning_content and taggedThinkingTags: true, asserting that reasoning_content is suppressed:
it('should suppress reasoning_content when taggedThinkingTags is enabled', () => {
const response = converter.convertOpenAIResponseToGemini(
{ /* response with reasoning_content + <think> tags */ },
withTaggedThinkingOptions(),
);
// assert reasoning_content is NOT in parts
});Same gap exists in the streaming path at line 1232.
— qwen3.7-max via Qwen Code /review
| : undefined; | ||
| const responseParsingOptions = | ||
| this.config.provider.getResponseParsingOptions?.(effectiveModel); | ||
| this.config.provider.getResponseParsingOptions?.(); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The removed pipeline test was the only test covering the wiring between provider.getResponseParsingOptions(), the RequestContext.responseParsingOptions field, and the conditional TaggedThinkingParser instantiation. After removal, zero pipeline tests verify that the pipeline calls getResponseParsingOptions and propagates the result into the request context.
Consider adding a lightweight test that mocks getResponseParsingOptions and verifies propagation — even without the model parameter, the wiring itself is worth covering.
— qwen3.7-max via Qwen Code /review
pull Bot
pushed a commit
to Stars1233/qwen-code
that referenced
this pull request
Jul 12, 2026
) Qwen3 can return `<think>` blocks in `content` instead of `reasoning_content`, but the default OpenAI-compatible provider never enabled the existing tagged thinking parser on the response path. This wires that parser back in only for Qwen3 model names, keeping the safer GLM revert intact while restoring the intended fallback for Qwen3 responses. Constraint: Must not reintroduce the DashScope GLM buffering regression reverted in QwenLM#6248 Rejected: Enable tagged-thinking parsing for every default provider response | broader blast radius without maintainer direction Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep response-side tagged-thinking gating aligned with the Qwen3 request-side reasoning mirroring check Tested: `npm run typecheck --workspace=packages/core`; `npm run test --workspace=packages/core -- src/core/openaiContentGenerator/provider/default.test.ts`; `npm run test --workspace=packages/core -- src/core/openaiContentGenerator/converter.test.ts -t 'OpenAI -> Gemini tagged thinking content'` Not-tested: End-to-end live DashScope/Qwen API streaming against a real provider Related: QwenLM#6666
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
This PR reverts PR #6033, which enabled tagged-thinking parsing for DashScope
glm-*responses and added buffering logic for mixed tagged-thinking and reasoning-channel streams.Why it's needed
The reverted change caused a critical regression for GLM on DashScope: raw SSE traffic can continue arriving while Qwen Code withholds converted stream events until the response finishes. The TUI renders converted model events, not raw SSE chunks, so long GLM requests can appear completely silent even though the upstream connection is active.
The same converted-event starvation also affects logging. The logging wrapper resets its idle timer only when a converted response is yielded. If the converter buffers GLM output for more than 5 minutes, the wrapper marks the stream span as idle-timed-out and then skips the eventual success response and OpenAI interaction log, even when the request later completes successfully.
PR #6033 was intended to address #6007, where GLM-5.2 could leak thinking text as visible output with a malformed
</think>shape at largemax_tokens. That problem still needs a safer follow-up, but the tagged-thinking opt-in for DashScope GLM is too broad because it changes streaming semantics and breaks long-running GLM requests.Reviewer Test Plan
How to verify
Reviewers should confirm that DashScope
glm-*no longer opts into tagged-thinking parsing by default, and that reasoning-channel streaming returns to the pre-#6033 behavior instead of being buffered behind an ambiguous tagged-thinking state.Targeted local validation:
Evidence (Before & After)
Before: after #6033, GLM on DashScope could keep the upstream SSE stream active while emitting no converted model chunks to the TUI for a long-running response. If that converted-event gap exceeded 5 minutes, logging treated the stream span as idle-timed-out and skipped the eventual successful response log.
After: DashScope GLM no longer enters the tagged-thinking buffering path added by #6033. Converted events are no longer withheld by that GLM-specific tagged-thinking state machine.
Local validation passed: 3 focused test files passed, 292 tests passed;
packages/coretypecheck passed.Tested on
Environment (optional)
Local validation used Node.js
v22.18.0.Risk & Scope
max_tokensresponse shape.</think>shape directly or avoid the unsafe GLM large-output request shape without buffering normal reasoning-channel streams.Linked Issues
Reverts #6033. References #6007.
中文说明
What this PR does
这个 PR 回滚 PR #6033。PR #6033 为 DashScope
glm-*响应启用了 tagged-thinking 解析，并为 tagged-thinking 与 reasoning channel 混合流增加了缓冲逻辑。Why it's needed
被回滚的改动导致 DashScope 上的 GLM 出现严重回归：上游 SSE 原始数据仍然持续到达，但 Qwen Code 可能在响应结束前不产出转换后的流式事件。TUI 渲染的是转换后的模型事件，不是原始 SSE chunk，因此长时间运行的 GLM 请求会看起来完全没有流式输出，即使上游连接仍然活跃。
同一个转换后事件饥饿问题也会影响日志。logging wrapper 只有在收到转换后的 response 时才会重置 idle timer。如果 converter 为 GLM 缓冲输出超过 5 分钟，wrapper 会把 stream span 标记为 idle timeout，并在请求最终成功完成后跳过 success response 和 OpenAI interaction log。
PR #6033 原本是为了解决 #6007，即 GLM-5.2 在大
max_tokens下可能用畸形的</think>形态把 thinking 文本泄漏到可见输出。这个问题仍然需要一个更安全的后续修复，但为 DashScope GLM 广泛启用 tagged-thinking 解析会改变流式语义，并破坏长时间运行的 GLM 请求。Reviewer Test Plan
How to verify
Reviewer 应确认 DashScope
glm-*不再默认启用 tagged-thinking 解析，并确认 reasoning-channel streaming 恢复到 #6033 之前的行为，而不是被缓存在一个不确定的 tagged-thinking 状态后面。本地定向验证：
Evidence (Before & After)
Before：#6033 之后，DashScope 上的 GLM 可能保持上游 SSE stream 活跃，但长时间不向 TUI 产出转换后的模型 chunk。如果这个转换后事件空窗超过 5 分钟，logging 会把 stream span 当作 idle timeout，并跳过最终成功响应的日志。
After：DashScope GLM 不再进入 #6033 添加的 tagged-thinking 缓冲路径。转换后的事件不再被这个 GLM 专用 tagged-thinking 状态机扣住。
本地验证通过：3 个定向测试文件通过，292 个测试通过；
packages/coretypecheck 通过。Tested on
Environment (optional)
本地验证使用 Node.js
v22.18.0。Risk & Scope
max_tokens响应形态下可能把畸形 tagged-thinking 文本泄漏为可见文本。</think>形态，或者避免 GLM 的不安全大输出请求形态，同时不能缓冲正常的 reasoning-channel stream。Linked Issues
Reverts #6033. References #6007.