fix(core): retry malformed JSON protocol leaks - #8289
Conversation
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for this — the underlying fix is well-motivated. #8207 is a real, observed production bug (qwen3.7-max leaking JSON tool-call arguments as plain text when the model drops function-calling format), and extending the existing post-stream recovery architecture with a conservative JSON branch is a sensible direction.
Before we can review the code, the PR body needs to follow the repository's pull request template. Right now it uses free-form headings (Summary / Demo / Validation) and is missing the sections the template requires:
- What this PR does and Why it's needed — in prose, not by file or function name
- Reviewer Test Plan —
How to verify,Evidence (Before & After)(N/Ais fine here since there's no user-visible UI change), and theTested ontable - Risk & Scope — main risk or tradeoff, what's not validated / out of scope, and any breaking changes
- Linked Issues —
Fixes #8207as a closing keyword inside this section - The bilingual 中文说明
<details>block translating the English body paragraph-by-paragraph
Could you restructure the description using the template, @xianjianlf2? It lives here: https://github.com/QwenLM/qwen-code/blob/main/.github/pull_request_template.md
Once the description is in shape, we'll dive into the code review. 🙏
中文说明
感谢贡献——这个修复本身的动机是充分的。#8207 是一个真实观测到的生产环境 bug
（qwen3.7-max 在模型丢失 function-calling 格式时，把 JSON tool-call 参数作为
纯文本泄漏），在现有 post-stream recovery 架构上扩展一个保守的 JSON 分支也是
合理的方向。
但在审查代码之前，PR 描述需要遵循仓库的 pull request 模板。目前使用的是自由
标题（Summary / Demo / Validation），缺少模板要求的章节：
- What this PR does 和 Why it's needed —— 用散文描述，不要按文件或函数名罗列
- Reviewer Test Plan ——
How to verify、Evidence (Before & After)
（因为没有用户可见的 UI 变化，这里可以填N/A）、以及Tested on表格 - Risk & Scope —— 主要风险或权衡、未验证/超出范围的内容、以及破坏性变更
- Linked Issues —— 在此章节内用
Fixes #8207作为关闭关键字 - 双语的 中文说明
<details>块，逐段翻译英文正文
能否用模板重新组织一下描述，@xianjianlf2？模板在这里：
https://github.com/QwenLM/qwen-code/blob/main/.github/pull_request_template.md
描述整理好后，我们会进入代码审查。🙏
— Qwen Code · qwen3.8-max-preview
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
I reproduced the reported payload against this commit. The current recovery misses issue #8207 and also crosses the text-to-execution boundary without a trustworthy signal. Please address the blocking comments below before this lands.
| const recovered = calls.map((call) => { | ||
| if (!isRecord(call) || typeof call['name'] !== 'string') return null; | ||
| const name = call['name']; | ||
| if (!isKnownToolName(name)) return null; |
There was a problem hiding this comment.
[P1] This interprets each item name as the registered tool name, but in #8207 create_node and read_ref are values of the agent call name argument; the intended outer tool is agent. Running the exact reported shape with only agent registered returns recovered: false, so the original leak remains. Please make the regression fixture faithful to the issue and preserve each item name as an argument. Since the text has no trustworthy outer function name, it should not be inferred from this field.
| contentText | ||
| ) { | ||
| const toolRegistry = this.config.getToolRegistry(); | ||
| const recovery = tryRecoverJsonToolCalls(contentText, (name) => |
There was a problem hiding this comment.
[P1] This validates against every loaded registry tool rather than the tools actually declared for this request. As a result, ordinary JSON documentation for a registered tool can become a real call even when that tool was deferred, excluded by a subagent allowlist, or the request explicitly used tools: []; in YOLO mode that can execute immediately. Recovery needs a trustworthy intent signal and must be constrained to the current request tool surface; JSON shape alone is not sufficient.
| const chunkStartsJson = | ||
| chunkText?.trimStart().startsWith('[') || | ||
| chunkText?.trimStart().startsWith('{'); | ||
| if (!hasToolCall && (bufferingJsonText || chunkStartsJson)) { |
There was a problem hiding this comment.
[P1] Buffering can start on a JSON-looking chunk in the middle of a normal answer and then hold every remaining chunk until EOF. It also leaves buffered prefix chunks pending when a later structured function call arrives, so the tool-call/finish chunk is yielded before earlier text. Please restrict detection to the leading response and flush pending chunks in original order before yielding a real structured call.
|
One more detail from the production trace: the visible content was not standalone JSON. The array was followed by closing protocol tags, specifically and , so JSON.parse(trimmed) fails before this code reaches the name check. Those tags are also the trustworthy signal that this is malformed protocol output, while the outer function name is still absent. The smaller safe fix is to buffer this exact shape before UI/history emission and send it through the existing PROTOCOL_TAG_LEAK retry path, with a regression using the array plus closing tags. That fixes the recorded case without guessing agent or turning ordinary JSON into executable calls. |
|
@qwen-code /triage |
xianjianlf2
changed the title
|
@qwen-code /triage |
|
Thanks for iterating on this. I opened #8301 as the maintainer-owned fix based on the full production trace. It treats the JSON array plus closing protocol tags as a failed protocol response and retries it, rather than reconstructing and executing a tool call; it also covers ordinary JSON streaming and structured-call ordering. I’m closing this one as superseded so we can continue the core change in one place. |
3 participants
What this PR does
Retries the malformed production response shape from #8207 when the assistant starts with a JSON array payload and then leaks
</parameter></function>closing protocol tags. The stream now buffers only leading JSON-array-looking text until post-stream validation can decide whether it is the malformed protocol leak; ordinary JSON is flushed as assistant text, and real structured function calls still preserve original chunk order. This intentionally does not infer or execute tool calls from JSON itemnamefields.Why it's needed
The recorded failure was not valid standalone JSON and was not a trustworthy structured tool call. It was leaked protocol output that reached UI/history as visible text, so converting arbitrary JSON shapes into tool calls was too broad and could execute tools outside the current request surface. Routing only the exact malformed protocol shape through the existing
PROTOCOL_TAG_LEAKretry path fixes the observed leak without expanding tool execution semantics.Reviewer Test Plan
How to verify
cd packages/core && npx prettier --check src/core/geminiChat.ts src/core/geminiChat.test.tscd packages/core && npx eslint src/core/geminiChat.ts src/core/geminiChat.test.tscd packages/core && npm run typecheckcd packages/core && npx vitest run src/core/geminiChat.test.ts -t "ordinary leading JSON|leading JSON array followed|flushes buffered leading JSON|protocol-tagged response"Evidence (Before & After)
Before: a leading JSON array followed by leaked
</parameter></function>tags could be emitted and persisted as assistant text. After: the regression fixture retries throughPROTOCOL_TAG_LEAK, does not emit or record the leaked payload, keeps ordinary leading JSON as text, and flushes buffered JSON before a later real structured tool call to preserve event order.Tested on
Environment (optional)
Local macOS worktree under Node/npm from the repository toolchain.
Risk & Scope
Linked Issues
Fixes #8207
中文说明
这个 PR 改为处理线上真实形态：模型输出以 JSON 数组开头，后面泄漏
</parameter></function>协议闭合标签。客户端只在这种精确形态下走已有的PROTOCOL_TAG_LEAK重试路径，不再根据 JSON 里的name字段推断并执行工具，因此不会扩大工具执行面。普通 JSON 文本仍会作为普通助手文本输出。