fix(core): sanitize standalone closing thinking tags - #6854
Merged
Conversation
Collaborator
|
Thanks for the PR! Template looks good ✓ — all required sections present with a detailed reviewer test plan. Problem: observed, not theoretical. Issue #6849 documents a real Direction: aligned. This is a direct follow-up to #6794 (which correctly made leaked tags retryable). The natural next step is narrowing the recovery — retrying an entire turn when only a stray closing tag is wrong wastes tokens, adds latency, and may reproduce the same defect. The telemetry addition is a nice touch for observability. CHANGELOG doesn't reference this area yet, but the related PRs (#6794, #6666) establish it as an active maintenance area. Size: 422 production lines across 8 source files, 1257 test lines across 6 test files. Under the 500-line threshold for core module escalation. All changes are in Approach: scope feels right. The recovery gate is narrow (structured reasoning + standalone closing tag + complete named tool calls + Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必要章节齐全，reviewer test plan 详细。 问题：已观测到的问题，非理论性加固。 Issue #6849 记录了一次真实的 方向：对齐。这是 #6794（正确地将泄漏标签变为可重试错误）的自然后续。下一步应该是缩小恢复范围——当只有一个多余的 closing tag 时重试整轮会浪费 token、增加延迟，且可能复现同样的问题。telemetry 的加入有利于可观测性。 规模：422 行生产代码（8 个源文件），1257 行测试代码（6 个测试文件）。低于核心模块 500 行的上报阈值。所有改动都在 方案：范围合理。恢复 gate 很窄（structured reasoning + 独立 closing tag + 完整具名工具调用 + 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Code Review Reviewed the full diff independently. The implementation follows a clean candidate-buffer-then-decide pattern in The The The telemetry event ( No critical blockers. No AGENTS.md violations. The code is focused, well-tested (704 tests across 6 files, all passing), and the production-to-test ratio (~1:3) is strong. Verification All checks pass on the PR branch:
Real-scenario testing: this is non-UI streaming pipeline code — the defect requires a model to return structured reasoning + standalone 中文说明代码审查 独立审查了完整 diff。
Telemetry 事件（ 无关键阻塞问题。无 AGENTS.md 违规。代码聚焦、测试充分（6 个文件共 704 个测试，全部通过），生产/测试比约 1:3。 验证 PR 分支上所有检查通过：
真实场景测试： 这是非 UI 的 streaming 管线代码——该缺陷需要模型返回特定 stream 模式（structured reasoning + 独立 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — Clean across every stage; ready to merge. This is a well-scoped fix for a real production problem. The recovery gate is conservative (many conditions must hold simultaneously for sanitization to activate), every negative path falls closed to the existing retry, and the telemetry is privacy-safe. The The unit test suite is thorough — 704 tests covering the happy path, cross-chunk tag splitting, and every documented failure mode (nameless tool calls, malformed arguments, conflicting identities, non-tool_calls finish reasons, overlong candidates). Build, typecheck, and lint are all clean. The scope is tight: every changed file serves the stated goal, no drive-by refactors, no speculative features. The 128-char candidate buffer cap and deferred decision at stream finish are good conservative engineering choices. Approving. ✅ 中文说明Confidence: 5/5 — 各阶段均通过，可以合并。 这是一个针对真实生产问题的范围合理的修复。恢复 gate 是保守的（多个条件必须同时满足才会触发清洗），所有负面路径都会 fail closed 到现有的重试机制，telemetry 隐私安全。 单元测试覆盖充分——704 个测试覆盖了正常路径、跨 chunk 标签拆分，以及所有记录的失败模式（无名工具调用、格式错误参数、身份冲突、非 tool_calls finish reason、超长候选）。Build、typecheck 和 lint 均通过。 范围紧凑：每个改动文件都服务于声明的目标，无顺手重构，无投机性功能。128 字符候选缓冲上限和延迟到 stream finish 再做判断都是保守的好工程选择。 批准。✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
previously approved these changes
Jul 14, 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. ✅
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. |
qwen-code-ci-bot
previously requested changes
Jul 14, 2026
Collaborator
Author
|
Closeout summary: I am not loosening the sanitized tool-call argument gate in this automated pass because that changes the PR’s documented fail-closed boundary for malformed/incomplete tool calls; leaving that thread unresolved for maintainer decision. I resolved the remaining suggestions as follow-up/no-action: richer PROTOCOL_TAG_LEAK reasons, redundant local cleanup, and additional branch/telemetry tests would grow diagnostics/test surface beyond the minimal #6849 recovery fix. |
yiliang114
commented
Jul 14, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
Author
There was a problem hiding this comment.
Independent review pass. Logic is self-consistent and mergeable — no P0/P1 found.
- The sanitized tag does not leak into parts/history/JSONL: the candidate path filters visible text before pushing to
pendingUntrustedResponseParts, and the release branch concatenates only non-tag parts. Tests also assert history/JSONL exclude the tag. - Telemetry privacy is sound: the event carries only identifiers +
tag_name+tool_call_count, and the test verifiesnot.toMatch(/response_text|reasoning|tool_name|arguments/).
Two inline notes below. On the prior hasInvalidToolCallArguments Critical: I agree the strictness gap is real but downgrade it to P2/by-design — truncation is caught earlier by hasIncompleteToolCalls (inString/depth) before the gate runs, so the only inputs the gap affects are structurally-complete-but-syntactically-broken args (e.g. {bad}, null, 42), which is the fail-closed boundary already documented in the PR body and review reply. A one-line comment would make that intent durable.
Collaborator
Author
|
Automated follow-up summary:
Both threads are being resolved as non-actionable under the minimal-scope policy. |
Collaborator
|
Traced the candidate state machine by hand across the streaming paths (single-chunk, split-across-chunks, prefix-released-as-text, opening tag, >128 overflow, clean EOF, every finish_reason, invalid args). I could not find a correctness regression — the recovery is strictly additive relative to #6794: for the #6849 shape the pre-PR behavior is already a full-turn On the open Critical ( Triage of the remaining suggestions:
One net-new nit: the stream-EOF guard in Note: this lands in core ( Privacy/telemetry payloads, boundedness (128-char cap + constant-size complete-tag state), single-emit semantics, and false-positive gating all check out. Nice work. |
doudouOUC
reviewed
Jul 14, 2026
Collaborator
Author
|
Automated follow-up summary for the new review:
The thread is being resolved under the minimal-scope policy. |
Collaborator
Author
|
Automated follow-up summary for the latest top-level review:
No further code changes were made. |
wenshao
requested changes
Jul 14, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Not reviewed: coverage — could not read the agents' transcripts (the CLI did not export QWEN_CODE_PROJECT_DIR / QWEN_CODE_SESSION_ID, so this run cannot find the harness's record of what its agents did), so this run cannot show that any of the diff was read.
— Codex $qreview via Qwen Code /review
Collaborator
Author
|
Automated follow-up for the late review round:
Verification on |
wenshao
requested changes
Jul 14, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Not reviewed: streaming state-machine specialist — the agent failed to return substantively after one relaunch.
Not reviewed: build-and-test — npm test timed out at 120 seconds; the full suite and two partial failures could not be attributed.
Not reviewed: test efficacy — all six changed-test probes were inconclusive because the runner failed with ERR_MODULE_NOT_FOUND.
Not reviewed: build-and-test — macOS and Windows Test checks were skipped in CI and the full suite did not complete locally.
Not reviewed: coverage — could not read the agents' transcripts (the CLI did not export QWEN_CODE_PROJECT_DIR / QWEN_CODE_SESSION_ID, so this run cannot find the harness's record of what its agents did), so this run cannot show that any of the diff was read.
— Codex $qreview via Qwen Code /review
qwen-code-ci-bot
previously requested changes
Jul 14, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
This review found no new Critical issues — the PR's sanitization logic is well-structured and thoroughly tested (676 tests pass). However, 6 existing Critical findings from prior review rounds still stand in the code at this commit and must be addressed before merging:
-
converter.ts:1429 — Whitespace-only visible output after structured reasoning is classified as a pending thinking-tag prefix. At finish_reason,
throwProtocolTagLeakfires even though no tag was emitted, wasting a valid turn on a retry. Fix: at finish/EOF, treat a candidate whose text is whitespace-only as ordinary whitespace rather than a protocol leak. -
streamingToolCallParser.ts:202 —
if (validName) meta.name = validName;unconditionally overwrites the function name on any later delta carrying a name. The early-return path at line 178 correctly guards with!meta.name, but the main path does not. Fix:if (validName && !meta.name) meta.name = validName;. -
streamingToolCallParser.ts:76 —
const validName = name?.trim() ? name : undefined;uses the trimmed value for validation but stores the original untrimmedname. A recovered call with leading/trailing spaces in its name would fail the scheduler's registry lookup. Fix:const validName = name?.trim() || undefined;. -
streamingToolCallParser.ts:105 —
conflictingToolCallIdentityis set unconditionally when two different IDs arrive at the same index, even when the existing call is complete and safely relocated. This makes the converter reject otherwise valid tag recovery. Fix: set the flag only when the new ID relabels an ambiguous/incomplete slot. -
pipeline.ts:611 — The pending-tag catch branch converts any transport error to PROTOCOL_TAG_LEAK before checking for StreamContentError. A provider rate-limit error (TPM throttling) would be masked, skipping the dedicated rate-limit backoff path. Fix: move the
StreamContentErrorcheck before the pending-tag conversion. -
streamingToolCallParser.ts:129 — The inline
while (this.buffers.has(actualIndex)) { actualIndex++; }loop can hang on an unsafe integer index (IEEE-754 precision loss makesactualIndex++a no-op). Fix: reject or normalize non-negative unsafe indices before allocation.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
Addressed all six current Critical review findings in
Validation: 356 focused tests passed, targeted ESLint passed, branch-local TypeScript typecheck passed, and |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
previously approved these changes
Jul 15, 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. ✅
qwen-code-ci-bot
dismissed stale reviews from themself
July 15, 2026 00:02
Superseded by fresh triage on commit 61d67a1 — all findings addressed in subsequent commits.
Collaborator
|
@qwen-code /triage |
wenshao
requested changes
Jul 15, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Not reviewed: build-and-test — npm test timed out after 120 seconds before a final aggregate; observed failures were unrelated UI/external-hook tests.
Not reviewed: test efficacy — all six changed-test probes were inconclusive because the reverted-source runner failed with ERR_MODULE_NOT_FOUND.
Not reviewed: coverage — could not read the agents' transcripts (the CLI did not export QWEN_CODE_PROJECT_DIR / QWEN_CODE_SESSION_ID, so this run cannot find the harness's record of what its agents did), so this run cannot show that any of the diff was read.
— Codex $qreview via Qwen Code /review
Collaborator
✅ Local verification report — merge referenceVerified locally at head 1. Focused test suites — 683 passed / 0 failedThe six test files this PR touches, run from
Plus quality gates on the changed source:
2. Independent real-converter harness — 21 checks / 0 failedThe PR's
3. Before/after controlled experiment (proves the fix is load-bearing)The whole feature is new (0 references in
VerdictFrom a verification standpoint this is ready to merge: the recovery path is exercised end-to-end against the real converter, every fail-closed branch still throws Reproduced independently in an isolated worktree; screenshots are rendered from the actual test/harness output. 中文版本（点击展开）✅ 本地验证报告 —— 合并参考在 head 1. 相关测试套件 —— 683 通过 / 0 失败本 PR 改动的六个测试文件（在
外加对改动源码的质量门禁： 2. 独立的真实 converter 验证脚本 —— 21 项检查 / 0 失败PR 的
3. Before/After 对照实验（证明该修复是关键的）整个特性都是新增的（
结论从验证角度看可以合并：兜底路径已针对真实 converter 端到端执行，每个 fail-closed 分支仍会抛出 在独立 worktree 中独立复现；截图由真实的测试 / 脚本输出渲染而来。 |
wenshao
previously approved these changes
Jul 15, 2026
Collaborator
|
@qwen-code /resolve |
Collaborator
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
# Conflicts: # packages/core/src/core/openaiContentGenerator/pipeline.test.ts # packages/core/src/core/openaiContentGenerator/streamingToolCallParser.ts
Collaborator
Author
|
Verified the current head against the latest review. Eight blocking correctness findings still reproduce across the converter, pipeline, and streaming tool-call parser. Addressing them safely needs implementation and regression changes across at least six files, so I am leaving those Critical threads unresolved for maintainer direction instead of expanding this closeout automatically. I did not take the two non-blocking suggestions: the monotonic collision cursor is a performance/generalization change, and the reset test is additional hardening after several review-fix rounds. Both threads are resolved as follow-up to keep this PR scoped. CI is green and the branch is conflict-free, but the eight Critical threads remain blocking. |
Collaborator
Author
|
Addressed the remaining eight Critical findings in 8771e48:
Verification: all 374 related converter, pipeline, and parser tests passed. Prettier, ESLint, and git diff --check also passed. I left the two non-blocking Suggestions out of this patch: a monotonic collision cursor is a general parser performance change, and expanding reset-test coverage is unrelated test hardening. Keeping both out avoids widening this PR. The earlier human-gated status note is superseded by this fix pass. Local targeted typecheck was blocked by missing declarations and dependencies in the isolated dependency snapshot; the reported errors were outside the changed files. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
Collaborator
Author
|
The latest review left two non-blocking suggestions; I am leaving both unchanged to keep this PR bounded:
Current head 8771e48 passes Qwen Code CI attempt 2, including the full test job, no-AK smoke, and web-shell E2E. Attempt 1 only failed on an unrelated flaky timeout in the ZIP-extraction cancellation test. |
qwen-code-ci-bot
approved these changes
Jul 15, 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. ✅
wenshao
approved these changes
Jul 15, 2026
This was referenced Jul 27, 2026
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
5 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 handles one recoverable model protocol error without discarding the whole turn: when structured reasoning is followed by a visible channel containing only a standalone closing
</think>or</thinking>tag and at least one complete named tool call, Qwen Code suppresses the tag and preserves the tool calls.The decision is deferred until a
tool_callsstream finish. Tags split across chunks are recognized, while every other finish reason, opening tags, mixed visible content, incomplete prefixes at EOF, and missing, nameless, incomplete, malformed, or non-object tool calls keep the existing invalid-stream retry behavior. Tag-like prefixes that turn out to be ordinary text are released unchanged.Each recovered response emits one
qwen-code.chat.protocol_tag_sanitizedevent to the debug log, OpenTelemetry log pipeline, and QwenLogger. The event contains only model/request identifiers, the bounded tag name, and the preserved tool-call count; it never includes response text, reasoning, tool names, or arguments.Why it's needed
PR #6794 correctly made leaked protocol tags retryable, but the production sample behind #6849 contains otherwise usable responses: structured reasoning, a standalone closing tag, and complete tool calls. Retrying those turns adds latency and token cost, can reproduce the same model defect, and hides how often Qwen Code repaired the model output.
Reviewer Test Plan
How to verify
finish_reason: tool_calls. Confirm the provider is called once, the tag is absent from emitted parts, in-memory history, and chat-recording JSONL, the tool call is preserved, and one sanitization event is recorded.</thifollowed by non-tag text is emitted unchanged.finish_reason, or any non-tool_callsfinish reason. Confirm these cases still fail closed throughPROTOCOL_TAG_LEAKor the existing malformed-call path and emit no sanitization event.model,prompt_id,response_id,tag_name, andtool_call_countplus existing common telemetry fields.Evidence (Before & After)
N/A — non-UI streaming and telemetry behavior.
Tested on
Environment (optional)
Validated with focused converter, pipeline, GeminiChat, OpenTelemetry, and QwenLogger tests, plus
npm run lint,npm run build,npm run bundle, andnpm run typecheckon Node.js 22.Risk & Scope
tool_callsfinish, at least one completed named tool call, and no nameless, incomplete, malformed, or non-object call. Candidate buffering is capped at 128 characters and complete tags use constant-size state.Linked Issues
Resolves #6849
Related to #2596, #6666, and #6794.
中文说明
这个 PR 做了什么
这个 PR 对一种可恢复的模型协议错误做本地兜底，不再丢弃整轮：当 structured reasoning 之后的可见通道只有一个独立的 closing
</think>或</thinking>标签，并且响应里至少有一个完整、具名的工具调用时，Qwen Code 会过滤这个标签并保留工具调用。判断会延迟到
tool_callsstream finish。跨 chunk 拆分的标签可以被识别；其他所有 finish reason、opening tag、混合可见正文、EOF 时未完成的标签前缀，以及缺失、无名、不完整、参数格式错误或参数非对象的工具调用，仍然沿用现有 invalid-stream 重试。像标签的前缀如果后续证明是普通文本，会原样释放。每个被兜底处理的响应会向 debug log、OpenTelemetry log pipeline 和 QwenLogger 各发送一次
qwen-code.chat.protocol_tag_sanitized事件。事件只包含模型/请求标识、有限枚举的标签名和保留的工具调用数量；不会包含响应正文、reasoning、工具名或参数。为什么需要
PR #6794 正确地把协议标签泄漏变成了可重试错误，但 #6849 对应的生产样例其余部分是可用的：有 structured reasoning、一个独立 closing tag，以及完整工具调用。对这种响应整轮重试会增加延迟和 token 成本，重试后仍可能出现相同模型问题，同时也无法观测 Qwen Code 实际兜底了多少次。
Reviewer Test Plan
如何验证
finish_reason: tool_calls结束的完整具名工具调用。确认 provider 只调用一次，标签不进入输出、内存 history 或 chat-recording JSONL，工具调用被保留，并且只记录一次 sanitization event。</thi之后如果接的是非标签正文，会原样输出普通文本。finish_reason就 clean EOF、以及任何非tool_callsfinish reason 的场景。确认这些情况仍通过PROTOCOL_TAG_LEAK或现有 malformed-call 路径 fail closed，并且不发送 sanitization event。model、prompt_id、response_id、tag_name和tool_call_count。Evidence（Before & After）
N/A —— 这是非 UI 的 streaming 和 telemetry 行为变更。
Tested on
Environment（可选）
在 Node.js 22 上验证了 converter、pipeline、GeminiChat、OpenTelemetry 和 QwenLogger 的相关测试，并运行了
npm run lint、npm run build、npm run bundle和npm run typecheck。Risk & Scope
tool_callsfinish、至少一个已完成的具名工具调用，并且没有无名、不完整、参数格式错误或参数非对象的调用。候选缓冲最多 128 字符，完整标签只保留常量大小状态。Linked Issues
Resolves #6849
Related to #2596, #6666, and #6794.