fix(core): retry leaked JSON tool protocol output - #8301
Conversation
github-actions
Bot
added
the
review/self-reported
Code ReviewVerified locally on PR head OverviewExtends the existing 1. Detection is over-fitted to the single production sample (main concern)
Two of these look likely in practice: a different tool's first argument key ( The 2.
|
|
Thanks for the PR! Template looks good ✓ Problem: observed production bug. Issue #8207 carries the actual leaked payload (subagent dispatch JSON rendered as assistant text), the version (0.21.0-preview.2), the model (qwen3.7-max), and the triggering conditions (6th consecutive tool-call turn, ~35K input tokens, a 429 retry across pool nodes). A maintainer confirmed the gap in source. This is not theoretical. Direction: aligned. This extends the existing Size: 187 production lines (geminiChat.ts: 157+/30−), 689 test lines (geminiChat.test.ts: 685+/4−). Well under the 500-line threshold — no maintainer escalation needed. Approach: the scope feels right. The detector gains a Risk: Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的生产 bug。Issue #8207 附带了真实泄漏的 payload（subagent 调度 JSON 被当作助手文本渲染）、版本（0.21.0-preview.2）、模型（qwen3.7-max）以及触发条件（第 6 轮连续 tool call、约 35K 输入 token、跨池节点 429 重试）。维护者已在源码中确认缺口。非理论性问题。 方向：对齐。在现有 规模：187 行生产代码（geminiChat.ts: 157+/30−），689 行测试（geminiChat.test.ts: 685+/4−）。远低于 500 行阈值，无需维护者升级。 方案：范围合理。检测器新增 风险： 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal. Given the problem — JSON-serialized tool arguments leaking as plain text when the model drops function-calling format — I would extend the existing |
|
Confidence: 4/5 — solid, well-tested fix for a real production bug; the implementation is the minimal change the problem needs, CI is green, and the only reservation is that Stepping back: the problem is real (production payload in #8207, maintainer-confirmed gap), the approach matches my independent proposal almost exactly, and the test suite is thorough enough that I can trace every code path through a test. The The If I had to maintain this in six months, the state machine is clear enough: Non-blocking note: the buffering tradeoff (leading JSON objects/arrays delivered at the terminal event rather than incrementally) is documented in the PR description and is the right call — correctness over latency for an ambiguous prefix. Sandboxed verification would settle the remaining behavioural gap: 中文说明置信度：4/5 —— 针对真实生产 bug 的扎实、充分测试的修复；实现是问题所需的最小变更，CI 绿色，唯一的保留是 回顾全局：问题是真实的（#8207 中的生产 payload，维护者确认的缺口），方案与我的独立提议几乎完全一致，测试套件足够详尽，每条代码路径都能追溯到测试。
如果六个月后维护这段代码，状态机足够清晰： 非阻塞说明：缓冲权衡（前导 JSON 对象/数组在终止事件时一次性交付而非增量交付）已在 PR 描述中记录，是正确的选择——对歧义前缀，正确性优先于延迟。 沙箱验证可以弥补剩余的行为缺口： — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Addressed in f3fd84a. Detection now buffers a leading JSON object or object array independently of its first key, while numeric arrays and Markdown-style brackets release immediately; the closing protocol tags are also recognized before optional trailing prose. I removed the unused released-text parameter from pending-part replay, preserved usage-only chunks without counting them as user-visible fallback progress, and added regressions for |
Code Review (round 2)Verified locally on PR head Previous round — status
Also worth calling out as an unadvertised improvement: the new 1. Critical — the leak scan runs over the whole buffered response, so a legitimate JSON answer that merely mentions the tags is discarded and retriedRemoving the key whitelist widened the buffering trigger to any leading Verified end-to-end through This is reachable, not theoretical. Suggested fixes, in order of preference:
Either way, the mid-buffer match should not be enough on its own. 2. Suggestion — the buffering window is now the whole response for every
|
| Input | Result |
|---|---|
[{…}]\n</parameter>\n</function>\n (production shape) |
LEAK ✅ |
[ { … } ]\n</parameter>\n</function>\n (pretty-printed) |
LEAK ✅ |
[{…}]</parameter></function>Let me continue. |
emitted as text ❌ |
{"name":"x"</parameter></function> (truncated JSON, no closing brace) |
emitted as text ❌ |
```json\n[{…}]\n</parameter></function> |
emitted as text (out of scope, fine) |
PROBE-MISS retried=false lastText="[{\"name\":\"x\"}]</parameter></function>Let me continue."
The (?:\s|$) tail is doing no disambiguation work — </function> already ends in >. Dropping it costs nothing and closes the first miss. The [}\]] prefix requirement is what loses the truncated-payload case; if you keep it, that's a deliberate narrowing worth a negative test so it doesn't read as an oversight.
4. Suggestion — hasCandidateOutput changed more than the rename suggests
isToolCallPreparationOnly returned true only when preparations existed and there was no candidate output and no usageMetadata. hasCandidateOutput drops both the preparations precondition and the usageMetadata term, and both call sites use it inverted. Net effect beyond the intended usage-metadata fix: a chunk with no preparations, no parts and no finish reason used to count as "the stream yielded something" and now does not, at both the main-send site and the fallback site. That makes empty-stream detection stricter (more retries/fallbacks) — probably the behavior you want, but it is a wider change than "usage-only metadata passes through", and only the usage-only half is covered by the renamed test. Worth a line in the PR body and a test for the no-preparations/no-parts chunk. I grepped packages/*/src for other isToolCallPreparationOnly readers — only the two migrated sites (remaining hits are dist/ and coverage artifacts).
5. Minor
- The detector buffer and
pendingProtocolPartsare two copies of the same text kept in sync implicitly.finish()'s andreleaseJsonCandidate()'s return values are now used purely as booleans (protocolTagDetector.finish();with the result discarded at two sites), and the actual text comes from the parked parts. This is correct today only because everyaccept()that returns''is paired with apendingProtocolParts.push(part). TheGEMINI_EMPTY_CONTENT_PLACEHOLDERcontinueright above the loop is exactly the shape that breaks that pairing, and it is safe only because it is excluded from both sides. Given how heavily commented the rest of this file is, this deserves an explicit invariant comment or a restructure where the buffer is the single source of truth. finish()is still called twice on the terminal-chunk-without-parts path (once in the new branch, once at the end of the parts loop after the released parts are re-entered). Harmless — the second call short-circuits — but a one-line comment would save the next reader the trace.pendingProtocolParts.push(...outputParts.splice(0), part)remains the least obvious line in the diff and is still uncommented. Note it also parks an already-approvedfunctionCallfrom earlier in the same chunk, delaying dispatch to the terminal event.releaseJsonCandidate()is public and still undocumented; one line ("a non-text part proved this is a real response — flush the ambiguous JSON buffer") would carry it.- Naming:
LEAKED_TOOL_CALL_TAGSis a big improvement overTOOL_CALL_CLOSING_TAGS.
Test coverage
Good progress: leak-text-and-finishReason-in-the-same-chunk is now covered via finishWithContent, the ordering test asserts usage metadata passthrough, and the expectedTextChunks parameterization documents the latency tradeoff nicely. Remaining gaps:
- A legitimate leading
{…}object with no tags. The ordinary-JSON test only covers arrays, yet{is the trigger that now buffers unconditionally. One case pinning "object streams through, released at finish" is cheap. - The false positives in §1 — whichever way you resolve them, they belong in the suite.
- Negative test for §3's misses, to pin the intended scope.
- Stream ends while buffering with no
finishReason.finish()never runs,pendingProtocolPartsis silently dropped, and the turn fails asNO_FINISH_REASON. Reasonable, still untested. LeadingProtocolTagLeakDetectoris still not exported. Every state-machine case costs a fullsendMessageStreammock round (~2s each with the retry delay). Exporting it would make the matrices in §1 and §3 nearly free — that is what made the probing above practical for me.
Security
No new attack surface; the change is suppress-and-retry. §1 does shift the risk direction, though: with the whitelist removed the failure mode is no longer only "a leak reaches history" but also "a valid response is destroyed", which is the more user-visible of the two.
Verdict: the round-1 findings were addressed well and the direction is right. §1 is the one I'd want fixed before merge — it is verified, reachable through the structured-output path, and it trades a false negative for a false positive that silently deletes correct model output. §3 is a two-character fix. Everything else is polish or tests.
中文摘要
已在 PR head f3fd84a04 本地验证：geminiChat.test.ts 276/276 通过，packages/core/src/core 全量 2640/2640 通过；另将检测器抽出做状态机探测，并通过 sendMessageStream 跑了 3 个集成探针。
上一轮问题：第 1（过度贴合单样本）、第 2（text 参数被忽略）、第 4（usage-only chunk 被吞）已修复；第 3、5、6 仍在。新增亮点：终止 chunk 无 parts 时现在也会调用 finish()，补上了原有的一个漏洞。
- Critical：泄漏判定扫描整个缓冲区，导致合法 JSON 响应被误判丢弃并重试。 去掉 key 白名单后，任何以
{/[{开头的响应都会整体缓冲，finish()在全缓冲区任意位置匹配}</parameter></function>即判为泄漏。实测（探针）：{"verdict":"fail","evidence":"… } </parameter> </function> …"}→retried=true, calls=2，原始响应完全丢失。该路径真实可达：forkedAgent的结构化输出（responseMimeType: 'application/json'）走的就是GeminiChat.sendMessageStream，响应必然以{开头；若内容里引用了协议标签（例如判定/复述本 issue 的场景），会被反复重试直至耗尽。建议：先定位首个 JSON 值的闭合位置（括号配平或对前缀JSON.parse），只接受紧随其后的标签；或至少在请求带responseJsonSchema时完全跳过缓冲。 - 缓冲窗口现在覆盖所有以
{开头的响应（含 thought 全部滞留），且缓冲无上限。修好第 1 点（值闭合即释放）可顺带缓解。 - 仍存在漏检：
</function>后紧跟正文（无空白）不匹配；缺少闭合括号的截断 payload 不匹配。(?:\s|$)尾部约束没有区分作用，可直接去掉。 hasCandidateOutput的语义变化大于重命名：对于「无 preparation、无 parts、无 finishReason」的 chunk，判定从 true 翻转为 false，两个调用点都受影响。方向合理，但超出 PR 描述，建议补测试与说明。- 次要：检测器 buffer 与
pendingProtocolParts是隐式同步的两份同源数据（finish()返回值已退化为布尔），建议写明不变式；finish()仍被调用两次；push(...outputParts.splice(0), part)仍无注释；releaseJsonCandidate()缺文档。 - 测试建议：补合法单对象 JSON 直通用例、第 1/3 点的正反用例、缓冲中无
finishReason结束的用例；并导出LeadingProtocolTagLeakDetector以便直接做状态机单测。
结论：整体方向正确，上一轮问题处理得好。建议合并前修复第 1 点（已实测、可达、会静默删除正确输出），第 3 点改动极小，其余为打磨与测试。
|
Addressed the round-2 findings in
The new regressions were exercised red-first. I also reran the exact 905-byte production response across five chunkings (single chunk, after byte 1, after byte 2, before |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review
| if (escaped) escaped = false; | ||
| else if (char === '\\') escaped = true; | ||
| else if (char === '"') inString = false; |
There was a problem hiding this comment.
[Suggestion] The escape-handling branch (escaped flag) in hasLeakedToolCallTags has no test coverage. The only string-interior test uses a value with no backslash escapes. — Failure scenario: deleting the two escape lines as a mutation causes a false positive on input like {"example":"\"}</parameter></function>"} — the mutant treats \" as the string’s closing quote, sees the following } outside a string, matches the leak regex, and triggers an incorrect retry that discards a valid model response. The present code is correct; this is a coverage gap with a concrete surviving mutant.
Suggested test addition:
it('does not reject closing protocol tags after an escaped quote inside a JSON string', () => {
const text = JSON.stringify({ example: '\\"}</parameter></function>' });
// assert no retry, text preserved
});中文说明
hasLeakedToolCallTags 中的转义处理分支（escaped 标志）没有测试覆盖。唯一的字符串内部测试使用的值不包含反斜杠转义。删除这两行转义代码作为变异，会导致对包含转义引号的输入产生误报，触发不必要的重试并丢弃有效的模型响应。当前代码是正确的，这是一个有具体存活变异的覆盖缺口。
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Valid coverage gap; the current implementation is correct, and the final review found no blocker. This PR has already gone through more than five review/fix rounds with the behavioral paths and CI green, so I am deferring this non-blocking mutation-only test instead of resetting the full review and CI loop again.
Review:
|
| input | detected |
|---|---|
[{…}]\n</parameter>\n</function> |
✅ |
{…}\n\n</parameter></function> |
✅ |
{"a":1}\nDone.\n</parameter>\n</function> |
❌ |
```json\n{…}\n``` \n</parameter></function> |
❌ (never enters json state) |
The false-positive side is correspondingly tight — {"a":1} the model emitted </parameter></function> here is not flagged, and the string-aware scan handles tags inside JSON strings (good, and tested). So the tradeoff is deliberate; it's just worth stating in the code that this matches one production signature rather than a class of them, since a false positive burns protocolTagLeakMaxRetries and then hard-fails the turn.
3. Buffering surface grew a lot, with no cap and no non-terminal flush
Previously only text starting with <analysis / <summary was withheld. Now every response starting with { or [{ is buffered in full until the terminal event, and takePendingProtocolParts also retracts same-chunk thought parts (pendingProtocolParts.push(...outputParts.splice(0), part), ~L4232) — so thinking text stops streaming live for those responses too. Consequences worth considering:
- Unbounded
buffergrowth on a large JSON answer, delivered as one part at the end. A size cap that releases as clean past some threshold would be a cheap safety valve. - The buffer is only flushed on a
finishReasonchunk. If the stream ends without one, the buffered text never reachesallModelParts. That's usually covered by theNO_FINISH_REASONthrow — except whenhasToolCallis true, where validation passes and the buffered text is silently dropped from both the emitted stream and history. - On mid-stream
streamError, the buffered text is likewise absent from the partial assistant turn used by the repair path.
None of these is covered by a test.
4. !hasToolCall on the throw is a good fix, but the leak is now silent
Adding && !hasToolCall (~L4471) correctly stops a retry after a tool call has already been yielded downstream — that would otherwise re-execute the call. Nice catch, and the new test documents it.
The side effect: in that ordering the leaked text is swallowed (pendingProtocolParts = []) and nothing is logged. Every other rejection path in this file emits a debugLogger.warn. A one-line warn here would make the "text vanished but no retry" case diagnosable in the field. Also note the behavior is now order-dependent (leak→tool-call retries, tool-call→leak does not); a comment stating that intent would help the next reader.
5. isToolCallPreparationOnly → hasCandidateOutput is a wider change than the rename suggests
The predicate isn't the inverse of the old one. Two new cases now count as "no user-visible output":
- usage-only chunks (no candidates) — this is the intended fix, and it's fine:
turn.tsonly surfacesusageMetadataon theFinishedevent, which needs afinishReason, so nothing user-visible is lost. - chunks with candidates whose
partsis[]and nofinishReason, regardless of whether preparations are present — previously these counted as output.
Both make the fallback chain more likely to run. The knock-on I'd double-check is currentFallbackYieldedAnyChunk (~L3401) gating popPendingPartialAssistantTurn() in both directions (L3419 and L3468) — the reasoning that a usage-only chunk can never coexist with a pushed partial turn holds as far as I can tell, but it's load-bearing and undocumented.
6. Nits
- Aliasing: at ~L4226 the released text part is pushed by reference (
outputParts.push(...takePendingProtocolParts(), part)), whereas every other release path copies ({ ...part, text }, andtakePendingProtocolPartsitself does{ ...part }). Since the history consolidation later mutates in place (lastPart.text += part.text), a raw reference that was already yielded can have itstextmutated after the fact.{ ...part }here would keep the invariant uniform. hasLeakedToolCallTagshas no comment explaining why it tracks string/escape state — that's the whole point of the function and the reason the} </parameter></function>-inside-a-string test passes. Two lines would help.- The class name
LeadingProtocolTagLeakDetectorand the constantLEAKED_TOOL_CALL_TAGSno longer describe what they do (the latter is really "closing-delimiter-adjacent protocol tags");PROTOCOL_TAG_PREFIXESsits right above and now covers only half the detector's job. - Perf is a non-issue — I benchmarked
hasLeakedToolCallTagsat ~4 ms on a 768 KB payload despite the per-}slice()(V8 sliced strings), so no change needed.
Test coverage
Good breadth on the happy paths: leaked array/object with and without a terminal event, ordinary object array and numeric array (with an explicit assertion that the numeric array streams incrementally — nice), ordering vs. a real structured tool call with and without preparation metadata, tags inside a JSON string, and both tool-call orderings. Converting streamResponse to varargs is a clean way to get there.
Missing, mapping to the findings above:
- a preparation chunk or non-text part arriving mid-payload (finding 1)
- a buffered JSON response whose stream ends without a
finishReason, and one that errors mid-stream (finding 3) finishWithContent: trueis only exercised for the single-object case; the array case only covers a separate terminal event
Security / correctness posture
No injection or data-exfiltration surface; the change is defensive and fails toward retry. The main correctness risk is silent data loss (findings 3 and 4) rather than incorrect output.
Overall: the direction is right and the ordering machinery is carefully done. Finding 1 is the one I'd want addressed before merge — it's a hole in the exact path the PR is built to close.
Real session JSONL verificationI pulled the complete production session artifact from the trace-derived Beijing OSS bucket and replayed it directly through Artifact checks:
Before the fix ( At the current head ( This is in addition to the existing 321/321 affected tests, build, typecheck, formatting/lint checks, and the green GitHub CI/review runs at the same head. |
Local real-stack verification (merge reference)Full end-to-end run on macOS (Node v24), no mocks inside the product: two fresh detached worktrees — before = PR base 1. Before (base
|
yiliang114
added
the
autofix/takeover
Re-verification at
|
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no code changeThis round triaged the feedback newer than the last evaluation. The issue-level The only actionable inline finding is a single automated-reviewer Suggestion Disposition: deferred (no code change). This is a non-blocking, test-only No source files were modified and no commit was created this round. 中文说明Autofix 审查轮次——无代码改动本轮分诊了上次评估之后的新反馈。Issue 级审查线程已结束:审查者已对当前 head( 唯一可执行的行内 finding 是一条自动化审查器的 Suggestion( 处置:延迟处理(无代码改动)。 这是一个非阻断、纯测试的覆盖缺口,生产代码已被确认正确。本 PR 已经历超过五轮审查/修复,因此按照仓库的审查策略,此阶段只落地 Critical 修复(正确性、安全性、数据丢失、回归),其余 Suggestion 延迟到后续处理。维护者(@yiliang114)也已在该线程中明确记录了同样的延迟决定,不希望为一个纯变异测试而重置完整的审查与 CI 循环。该 finding 线程保持打开,并附一条回复记录处置结论,以便该覆盖缺口在后续工作中保持可见。 本轮未修改任何源文件,也未创建任何提交。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: This PR extends LeadingProtocolTagLeakDetector in package... — the agent made no tool call: it read nothing.
中文说明
已审查。 建议见行内评论。 未审查：This PR extends LeadingProtocolTagLeakDetector in package...——该 agent 未发起任何工具调用：它什么都没读。
— qwen3.8-max-preview via Qwen Code /review
| } else if ( | ||
| (char === '}' || char === ']') && | ||
| LEAKED_TOOL_CALL_TAGS.test(text.slice(i)) | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] hasLeakedToolCallTags calls text.slice(i) for every structural } / ] outside a JSON string, allocating a fresh O(n−i) substring on each iteration — the scan is quadratic in the buffered response length. — Concrete cost: this runs from finish() precisely when a response starts with { / [{ (the common structured-output shape that this diff newly buffers). A large minified JSON response (~256KB, ~30K top-level braces) triggers ~30K substring allocations; a probe measured ~3MB of temporary allocation on a 1.26MB / 100K-brace input versus 0MB for a sticky-regex variant. Wall-clock impact is modest today (V8 collects the short-lived slices efficiently), but the allocation work is avoidable. A sticky regex (drop ^, add the y flag, set lastIndex = i) or a startsWith forward scan makes the whole function O(n) with zero intermediate allocations, e.g.:
const LEAKED_TOOL_CALL_TAGS = /[}\]]\s*<\/parameter>\s*<\/function>/iy;
// ...
} else if (char === '}' || char === ']') {
LEAKED_TOOL_CALL_TAGS.lastIndex = i;
if (LEAKED_TOOL_CALL_TAGS.test(text)) return true;
}中文说明
hasLeakedToolCallTags 对 JSON 字符串外的每个结构字符 } / ] 都调用 text.slice(i)，每次分配一个 O(n−i) 的新子串，使扫描相对于缓冲响应长度呈平方复杂度。— 具体代价：该函数仅在响应以 { / [{ 开头时（即常见的结构化输出形态，本 diff 新增的缓冲逻辑）由 finish() 调用。一个较大的压缩 JSON 响应（约 256KB、约 3 万个顶层花括号）会触发约 3 万次子串分配；探针实测在 1.26MB / 10 万个花括号的输入上产生约 3MB 临时分配，而 sticky 正则变体为 0MB。当前墙钟时间影响不大（V8 能高效回收这些短命子串），但这部分分配工作是可以避免的。改用 sticky 正则（去掉 ^、加 y 标志、设置 lastIndex = i）或 startsWith 前向扫描，可使整个函数变为 O(n) 且零中间分配。
— qwen3.8-max-preview via Qwen Code /review
| const parts = takePendingProtocolParts(); | ||
| if (parts.length > 0) { | ||
| content = { |
There was a problem hiding this comment.
[Suggestion] The non-leaked branch of the in-loop finishReason && !content?.parts path is not exercised by any test. — Concrete cost: the only test that sends a finish-only chunk ({ candidates: [{ finishReason: 'STOP' }] }) does so inside the leak test, where the detector is already in the leaked state, so only the pendingProtocolParts = [] branch runs. The else branch here — takePendingProtocolParts() plus the role: content?.role ?? 'model' content reconstruction — has no coverage. A future change that broke this branch would silently drop buffered, non-leaked JSON text from both the emitted stream and getHistory(), with no failing test. Consider adding a test that streams a JSON array without leaked tags across chunks, then a finish-only chunk, and asserts the JSON text appears in the emitted CHUNK events and in history.
中文说明
循环内 finishReason && !content?.parts 路径的“未泄漏”分支没有任何测试覆盖。— 具体代价：唯一发送仅含 finish reason 的数据块（{ candidates: [{ finishReason: 'STOP' }] }）的测试位于泄漏测试中，此时检测器已处于 leaked 状态，因此只会走到 pendingProtocolParts = [] 分支。这里的 else 分支——takePendingProtocolParts() 加上 role: content?.role ?? 'model' 的 content 重建——没有覆盖。如果未来的改动破坏了该分支，缓冲的、未泄漏的 JSON 文本会被静默地从输出流和 getHistory() 中丢弃，且没有任何测试失败。建议新增一个测试：跨多个数据块流式返回不带泄漏标签的 JSON 数组，随后发送一个仅含 finish reason 的数据块，并断言该 JSON 文本出现在输出的 CHUNK 事件以及 history 中。
— qwen3.8-max-preview via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressedFeedback points1. [rc:3696128433] Quadratic allocation in
|
Review (round 4) —
|
| leaked text | retried? |
|---|---|
{"file_path": "a.ts"</parameter></function> |
❌ no |
{"file_path":"a.ts",</parameter></function> |
❌ no |
[{"file_path":"a.ts"}]</function> |
❌ no |
All three are emitted and persisted as assistant text — the exact failure mode #8207 describes, one truncated brace away. This is not a regression (pre-PR they leaked too) and the production signature is covered, so it shouldn't block. For the follow-up: once the detector has committed to the json state, any </parameter> or </function> occurring outside a JSON string is already unambiguous — valid JSON can't contain bare text between tokens — so the closing-brace anchor can be dropped entirely without raising false-positive risk. The existing quote/escape scanner already gives you the "outside a string" test for free.
2. No diagnostics on the suppression path. When a leak is detected the buffered parts — sometimes including a real functionCall, per the retries when a function call interrupts a partial JSON protocol leak test — are dropped with no debugLogger line, while the neighbouring XML-fallback path logs both success and rejection. A one-line debugLogger.warn with buffer length and whether a functionCall was discarded would make this diagnosable from a user's session log instead of requiring a repro.
3. The subtle bits deserve comments. In a file that carries multi-paragraph design notes on every other non-obvious branch, pendingProtocolParts.push(...outputParts.splice(0), part) and the "push part, not {...part, text}" asymmetry are the two lines a future reader is most likely to "simplify" into a duplication bug. Two short comments would pin the invariant.
4. escaped-branch coverage — already triaged and deferred in this thread; noting it only so the follow-up carries all four items together.
Known tradeoff (already documented, no action)
A response whose first non-whitespace text is { or [{ is buffered end-to-end and delivered as a single part at the terminal event, with all intermediate chunks suppressed. For a "reply with JSON only" prompt that means no incremental rendering for the whole response and an unbounded in-memory buffer. The PR body calls this out explicitly and the production payload is only ~905 bytes, so it's the right call for now; if it ever bites, a size cap (release past a few KB — well above any realistic leaked argument blob) is a name-agnostic mitigation that preserves the current detection.
Verdict
✅ Approve. The delta is a clean, correct refactor plus a genuine coverage gain, the full suite is green, and the four items above are follow-up material, not merge blockers.
中文说明
第 4 轮评审
在 4a374ef32 上于独立 worktree 本地验证：geminiChat.test.ts 284/284 通过。另将 LeadingProtocolTagLeakDetector 与 processStreamResponse 的 pending-part 循环抽到独立 harness 做状态机探测，并通过真实 sendMessageStream 跑了 3 个即席集成探针。
与上次评审（95f7e0ceb）的增量
- sticky 正则：
^+slice(i)改为/…/iy+lastIndex = i，语义等价且正确。失败的 stickytest()会把lastIndex复位为 0，且每次调用前都重新赋值，不存在状态残留；函数同步且不递归，模块级可变正则安全。8 种形态验证结论一致。 - finish-only 分支的新测试：覆盖了此前未测的
finishReason && !content?.parts的else分支，harness 复放确认 JSON 完整释放、pending 归零。
增量部分无 Critical 问题，可以合并。
全量复查
- 「buffer 非空 ⟹ pending 非空」不变量成立，因此释放时 push 原始
part（而非{...part, text}）是正确的，不会重复输出（已用拆分的 Markdown 链接用例验证）。 - 四处释放点均会把 pending 清空；泄漏抛出点在 post-loop drain 之后，不会既释放又抛出。
- 空字符串 delta（OpenAI 兼容 SSE 常见）会暂存但在下一个非空文本或 finish 处恢复，无文本丢失。
isToolCallPreparationOnly→hasCandidateOutput是语义扩大而非纯重命名：usage-only chunk 不再置位streamYieldedAnyChunk，从而不阻断 fallback——这正是本次意图，且已有测试固定。
非阻断的后续项（不要为此重置本 PR）
- 检测对同类泄漏的近似形态很脆弱：正则要求标签前紧邻
}/]，因此参数对象未闭合的泄漏会漏检。真实链路探针结果：{"file_path": "a.ts"</parameter></function>、{"file_path":"a.ts",</parameter></function>、[{"file_path":"a.ts"}]</function>三者均不重试，直接作为文本输出并持久化。这不是回归（改前同样泄漏），生产特征也已覆盖，故不阻断。后续可考虑：进入json状态后，出现在 JSON 字符串之外的</parameter>或</function>本身就已无歧义（合法 JSON 的 token 之间不可能有裸文本），可直接去掉闭合括号锚点而不增加误报；现有的引号/转义扫描已经提供了「是否在字符串内」的判定。 - 抑制路径没有任何日志：泄漏时丢弃的 pending parts 有时包含真实
functionCall，却没有debugLogger记录，而相邻的 XML fallback 路径成功与拒绝都会记日志。建议补一行 warn（buffer 长度 + 是否丢弃了 functionCall）。 - 两处易被「优化」成 bug 的代码建议加注释：
pendingProtocolParts.push(...outputParts.splice(0), part)与「pushpart而非{...part, text}」的不对称。 escaped分支覆盖：本线程已决定延后，仅一并列出便于后续统一处理。
已知权衡（已在 PR 描述中说明，无需处理）
首个非空白文本为 { 或 [{ 的响应会整体缓冲到终止事件才一次性输出，期间所有 chunk 被抑制。对「只输出 JSON」类提示意味着全程无增量渲染，且缓冲无上限。PR 已显式记录该权衡，生产 payload 仅约 905 字节，当前取舍合理；若日后成为问题，加一个体积上限（超过数 KB 即释放）是与参数名无关且不影响现有检测的缓解手段。
结论
✅ 同意合并。 增量是干净正确的重构加一处真实覆盖增益，全量测试通过，上述四项属后续跟进而非合并阻断。
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action required — PR approvedThe round-4 review from @wenshao is an explicit APPROVE on head
No code changes are warranted this round. 中文说明无需操作 — PR 已获批准第 4 轮评审（@wenshao，基于
本轮无需任何代码变更。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| protocolTagDetector.finish(); | ||
| if (protocolTagDetector.leaked) { | ||
| pendingProtocolParts = []; |
There was a problem hiding this comment.
[Suggestion] The leaked branch of this post-stream synthetic-chunk block is not exercised by any test. — Concrete cost: a stream that emits leaked JSON (an object array followed by the closing parameter-end / function-end protocol tags) plus a functionCall part, and then ends with no finishReason, reaches this block as the only code path that calls finish() and clears the leak. Probe-confirmed: neutralizing the if (protocolTagDetector.leaked) { pendingProtocolParts = []; } guard ships the leaked protocol tags to the user instead of retrying, and the full 284-test suite still passes with that mutant — no existing test discriminates this branch. The closest test, "retries leaked JSON before a structured tool call", sends a finish reason, so it exercises the in-loop finishReason guard rather than this post-stream one. Suggested fix: add a case mirroring "preserves leading JSON when a tool call ends without a finish reason" but with leaked protocol tags in the buffered JSON and no finish reason, asserting that a retry occurs and no leaked text is emitted.
中文说明
[Suggestion] 这段「流结束后合成 chunk」代码块中的 leaked 分支没有任何测试覆盖。— 具体代价：当一个流先输出泄漏的 JSON（一个对象数组，后跟 parameter 结束标签 / function 结束标签这类协议标签）外加一个 functionCall part，然后在没有 finishReason 的情况下结束时，只有这个代码块会调用 finish() 并清除泄漏。经探针验证：把 if (protocolTagDetector.leaked) { pendingProtocolParts = []; } 这个保护置为无效后，泄漏的协议标签会直接输出给用户而不是触发重试，并且整套 284 个测试在该 mutant 下仍然全部通过——没有任何现有测试能区分这个分支。最接近的测试 "retries leaked JSON before a structured tool call" 会发送 finish reason，因此它走的是循环内的 finishReason 保护，而非这个流结束后的分支。建议修复：参照 "preserves leading JSON when a tool call ends without a finish reason" 增加一个用例，但让缓冲的 JSON 中带泄漏的协议标签且不带 finish reason，断言会触发重试且不输出任何泄漏文本。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review — fix(core): retry leaked JSON tool protocol output
Verdict: COMMENT (downgraded from Approve: CI still running)
Summary
This PR extends LeadingProtocolTagLeakDetector in geminiChat.ts to catch a production variant where the model serializes tool-call arguments as plain-text JSON followed by leaked protocol close tags. The failed attempt is routed through the existing protocol-leak retry path. Detection stays at the shared stream boundary, buffering a leading JSON object or object array without guessing argument names, preserving part ordering while ambiguous, and releasing ordinary JSON or real structured tool-call events unchanged.
Review Details
- 12 review agents launched in parallel (issue fidelity, line-by-line, removed-behavior, etc.)
- Verification rejected both initial findings (JSON buffering delay is documented design decision; post-stream pending parts handled by existing NO_FINISH_REASON path)
- Reverse audit: two consecutive dry rounds confirmed convergence
- Coverage: 3/3 chunks, 15 agents, all did work
- 0 confirmed findings, 0 inline comments
Conclusion
No blockers. The PR faithfully fixes #8207 with a well-tested approach (321 affected unit tests). The buffering tradeoff is acknowledged in the PR description. Approve once CI passes.
— qwen3.7-max via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[rc:3696470416] Post-stream
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action requiredThe latest review round (round 2) from the automated reviewer returned a COMMENT with zero findings. There are no inline comments, no failed checks, and no still-red checks to address. The reviewer noted that 中文说明无需操作自动化审查者的最新审查轮次（第 2 轮）返回了一条 COMMENT（评论），包含零个发现。没有行内评论、没有失败的检查、也没有持续红色的检查需要处理。 审查者指出 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 48 passed · 1 failed · 49 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：48 通过 · 1 失败 · 49 总计 Verification reportPR #8301 — deep verification reportVerdict: The central claim is decisively load-bearing: the A/B flips cleanly (
中文摘要
Central claim + A/BCentral claim. A model response that is a plain-text JSON object/object-array of tool arguments followed by leaked close tags </parameter></function> — with Method (control validity). Base cell =
Witnesses: Secondary claims (covered by the 285-test suite at head, not independently A/B'd — scope choice). (a) Non-leak shapes are preserved and numeric/markdown/prose keep streaming immediately — independently re-verified by the wire oracle below. (b) The CorrectionsNone. This is a first verification round (no FindingsF1 — Medium (completeness): a prose gap between the JSON payload and the leaked close tags escapes the detectorSeverity rationale. The production repro from #8207 (JSON immediately followed by the close tags) is fixed and proven load-bearing above, so this is not a regression or a data-loss path. It is a completeness gap in a defence-in-depth guard: the same root cause (tool protocol rendered as plain text instead of a structured call) one shape down. The fix's own new tests pin the reported shape by construction; this sibling is exactly what they do not pin. It is rated Medium rather than Critical because (i) the immediate-tags shape is the documented production signature and is closed, (ii) the escape requires the model to interleave a natural-language sentence between the JSON args and the close tags — a less probable but plausible degeneration, and (iii) the PR's scope statement arguably carves it out. The sibling-sweep rule still requires surfacing it, and a clean fix exists. Root cause. Reproducing (head, clean tree). The boundary probe ( Blast radius. Same code path as the central fix ( Measured candidate fix (behaviour-preserving; not applied to the PR)Broaden the matcher to fire on an out-of-string leaked close tag anywhere in the buffered JSON, not only immediately after -const LEAKED_TOOL_CALL_TAGS = /[}\]]\s*<\/parameter>\s*<\/function>/iy;
+const LEAKED_CLOSE_TAG = /<\/(?:parameter|function)>/iy;
...
- } else if (char === '}' || char === ']') {
- LEAKED_TOOL_CALL_TAGS.lastIndex = i;
- if (LEAKED_TOOL_CALL_TAGS.test(text)) return true;
+ } else if (char === '<') {
+ LEAKED_CLOSE_TAG.lastIndex = i;
+ if (LEAKED_CLOSE_TAG.test(text)) return true;
}Measured on the same harnesses (scratch copy, then restored — the PR tree was left pristine):
Witness Not covered
MethodologyEnvironment: the CI verify container ( Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
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. ✅
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action requiredThis review round found no actionable feedback:
The only issue-level comment is an automated CI verification status notification, which requires no code change. 中文说明无需操作本轮审查未发现需要处理的反馈：
唯一的 issue 级别评论是自动化 CI 验证状态通知，无需进行代码更改。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Local real-stack verification at
|
| commit | contains hasLeakedToolCallTags |
|
|---|---|---|
| before | 184365390 (merge base) |
no |
| after | 26a99e433 = merge of 45ed8f111 into 184365390 |
yes (dist/chunks/chunk-3YWUEUAM.js) |
The two trees differ only by this PR: git diff --stat 184365390 26a99e433 → geminiChat.ts + geminiChat.test.ts, 842 insertions / 34 deletions.
Result: the production bug reproduces on base and is fixed on head
Before — the leaked JSON payload and the </parameter> / </function> tags are rendered as an assistant message. One upstream request, no retry.
After — the failed attempt emits nothing, the turn is retried, and only the successful response reaches the UI.
Provider request log for the same prompt:
base: 1 A attempt=1 A/leaked-json
head: 1 A attempt=1 A/leaked-json
2 A attempt=2 A/retry-clean <-- retry only on head
Session persistence (~/.qwen/projects/<slug>/chats/<id>.jsonl), read back from disk:
base assistant parts: [thought] | "[{\"name\":\"create_node\", ... }]\n\n</parameter>\n</function>\n"
file contains "</parameter>": true
file contains "create_node": true
head assistant parts: "RETRY-OK: dispatching the two subagents through the tool channel."
file contains "</parameter>": false
file contains "create_node": false
So the leak is gone from the UI and from the recording, and the discarded attempt's thought part is dropped with it.
Non-regression: four shapes that must not change
Each was run through the same real TUI on both trees, and the persisted assistant parts were diffed.
| # | Shape | base | head | persisted output |
|---|---|---|---|---|
| B | legit JSON object array as the answer | rendered | rendered | identical (12 parts) |
| C | numeric array [1, 2, 3, 5, 8, 13, 21] |
rendered | rendered | identical (1 part) |
| D | leading JSON text → real structured tool call | text, tool runs, final | text, tool runs, final | identical after normalising the fixture path |
| G | legit JSON whose string value contains "}</parameter></function>" |
rendered | rendered | identical (5 parts), no retry |
G is the false-positive path I flagged in round 2 — the quote/escape-aware scan holds in the real stack: one upstream request, no PROTOCOL_TAG_LEAK.
D confirms ordering and tool execution survive the buffering path:
The buffering tradeoff, measured
The PR describes the tradeoff qualitatively ("may be delivered at the terminal event rather than incrementally"). I measured it: the mock sends every content chunk, then stalls 12 s before the terminal finish frame, and the harness samples the pane every 500 ms.
| Payload | base: first pixel | head: first pixel |
|---|---|---|
numeric array ([1, 2, …) |
555 ms | 549 ms — unchanged, still incremental |
JSON object array ([{…) |
551 ms | 12 814 ms — held until the terminal event |
At t≈6 s, mid-stall, with all content already sent upstream:
| before — text already on screen | after — still spinning |
|---|---|
 |
 |
Reading of this: the delay is bounded by the terminal event and loses nothing — scenario B's final render and persisted bytes are identical on both trees. It only affects responses whose first non-whitespace character is { or [{, and numeric arrays and Markdown brackets are provably unaffected. For a coding agent that is a rare answer shape, and the cost is perceived latency on a shape that is far more often a leak than an answer. I consider it an acceptable trade; it is worth keeping in mind if someone later reports "my JSON answers feel less streamy".
Checks
packages/core/src/core/geminiChat.test.tson head: 285/285 passed (27.4 s), including the new post-stream-guard case added in45ed8f111.npm ci+npm run bundle: clean on both trees.- Production code is unchanged since my round-4 approval at
4a374ef32;45ed8f111adds only a test.
Verdict
Good to merge. The reported production failure reproduces on base and is fixed on head; the fix does not alter any of the four adjacent shapes I could construct; the one behavioural cost is a bounded, lossless streaming delay on a narrow input shape, now quantified above.
中文说明
在 45ed8f111 上的本地真实环境验证 —— 合并参考
macOS（Node v24.18.1）全链路运行，产品内部不打任何 mock。基于 GitHub merge ref 拉出两个全新的 detached worktree，各自 npm ci + 完整 npm run bundle，以真实交互式 TUI 启动（tmux 120×40 中运行 node dist/cli.js --yolo，$HOME/$QWEN_HOME/$TMPDIR 全隔离，通过 OPENAI_* 鉴权）。唯一的替身在上游：一个本地 OpenAI 兼容 SSE 服务，回放 #8207 的真实响应形态并记录每一次请求。
| commit | 是否包含 hasLeakedToolCallTags |
|
|---|---|---|
| 修复前 | 184365390（merge base） |
否 |
| 修复后 | 26a99e433 = 45ed8f111 合入 184365390 |
是（dist/chunks/chunk-3YWUEUAM.js） |
两棵树仅相差本 PR：git diff --stat 184365390 26a99e433 → geminiChat.ts + geminiChat.test.ts，842 增 / 34 删。
结论：生产 bug 在 base 复现，在 head 已修复
修复前 —— 泄漏的 JSON 与 </parameter> / </function> 标签被当作助手消息渲染出来，只发出 1 次上游请求，无重试。
修复后 —— 失败轮次不输出任何内容，触发重试，只有成功响应到达 UI。
上游请求日志（同一条 prompt）：
base: 1 A attempt=1 A/leaked-json
head: 1 A attempt=1 A/leaked-json
2 A attempt=2 A/retry-clean <-- 仅 head 有重试
从磁盘读回的会话持久化（~/.qwen/projects/<slug>/chats/<id>.jsonl）：
base assistant parts: [thought] | "[{\"name\":\"create_node\", ... }]\n\n</parameter>\n</function>\n"
文件中含 "</parameter>": true
文件中含 "create_node": true
head assistant parts: "RETRY-OK: dispatching the two subagents through the tool channel."
文件中含 "</parameter>": false
文件中含 "create_node": false
也就是说，泄漏内容在 UI 和 recording 中都消失了，被丢弃轮次的 thought part 也一并丢掉。
非回归：四种必须保持不变的形态
每种都在两棵树上跑同一套真实 TUI，并对持久化的 assistant parts 做 diff。
| # | 形态 | base | head | 持久化输出 |
|---|---|---|---|---|
| B | 合法 JSON 对象数组作为回答 | 正常渲染 | 正常渲染 | 完全一致（12 parts） |
| C | 数字数组 [1, 2, 3, 5, 8, 13, 21] |
正常渲染 | 正常渲染 | 完全一致（1 part） |
| D | 先输出 JSON 文本 → 再真实结构化工具调用 | 文本、工具执行、最终回复 | 同左 | 归一化 fixture 路径后完全一致 |
| G | 合法 JSON，其字符串值中含 "}</parameter></function>" |
正常渲染 | 正常渲染 | 完全一致（5 parts），无重试 |
G 正是我在 round 2 提出的误判路径 —— 引号/转义感知的扫描在真实链路上成立：仅 1 次上游请求，未触发 PROTOCOL_TAG_LEAK。
D 证明缓冲路径不破坏顺序，工具也照常执行。
缓冲权衡的实测数据
PR 只定性描述了这个权衡（"可能在终止事件时一次性输出，而不是增量输出"）。我把它测了出来：mock 发完全部 content chunk 后，在终止 finish 帧前停顿 12 秒，harness 每 500ms 采样一次界面。
| Payload | base：首次可见 | head：首次可见 |
|---|---|---|
数字数组（[1, 2, …） |
555 ms | 549 ms —— 未变，仍是增量输出 |
JSON 对象数组（[{…） |
551 ms | 12 814 ms —— 被暂存到终止事件 |
第 6 秒（停顿中途、上游内容已全部发完）的截图对比见上方英文部分。
我的判断：这个延迟以终止事件为上界，且不丢任何内容 —— 场景 B 在两棵树上的最终渲染和持久化字节完全一致。它只影响首个非空白字符为 { 或 [{ 的响应，数字数组和 Markdown 方括号已证明不受影响。对编码 agent 来说这是很少见的回答形态，代价只是这种形态下的感知延迟，而这种形态更多情况下本来就是泄漏而非正常回答。我认为这个取舍可以接受；如果日后有人反馈"JSON 回答的流式感变差了"，可以回头看这里。
检查项
- head 上
packages/core/src/core/geminiChat.test.ts：285/285 通过（27.4 s），包含45ed8f111新增的 post-stream guard 用例。 - 两棵树的
npm ci+npm run bundle均干净通过。 - 自我 round-4 批准的
4a374ef32起，生产代码未变；45ed8f111只加了测试。
结论
可以合并。 上报的生产故障在 base 复现、在 head 修复；该修复未改变我所能构造的四种相邻形态的任何行为；唯一的行为代价是一个有界、无损的流式延迟，作用于很窄的输入形态，且已在上文量化。
|
Released in v0.21.4. |
5 participants
What this PR does
This PR prevents a model response that contains a JSON-array tool payload followed by leaked
</parameter></function>protocol tags from reaching the UI, conversation history, or session recording. The failed attempt is routed through the existing protocol-leak retry path instead.Detection stays at the shared stream boundary. It buffers a leading JSON object or object array without guessing argument names, preserves part ordering while the response is ambiguous, and releases ordinary JSON or real structured tool-call events unchanged. Numeric arrays and Markdown-style bracketed text continue streaming immediately.
Why it's needed
In a production session, the model returned
finish_reason=stopwith a plain-text JSON array containing two subagent argument objects and the closing tool protocol tags, but no structured tool call. The existing guard handled leading XML-style protocol tags, so this JSON-shaped variant was displayed and persisted as assistant text instead of being retried.Reviewer Test Plan
How to verify
</parameter></function>, both with a separate terminal event and with the finish reason on the content chunk. Cover an arbitrary first argument key, a direct object, and trailing prose. Confirm that the failed attempt emits no parts, writes no history or recording entry, and the retry exposes only the successful response.Evidence (Before & After)
Before: the production-shaped response was emitted as assistant text and persisted without a retry.
After: the same 905-character response produces
PROTOCOL_TAG_LEAK, emits zero parts from the failed attempt, leaves history and recording unchanged, retries once, and exposes only the successful response.Tested on
Environment (optional)
Node.js 22, no sandbox. Verified with 321 affected unit tests, the full repository build and typecheck, ESLint, Prettier, and
git diff --check.Risk & Scope
Linked Issues
Fixes #8207
中文说明
本 PR 的改动
本 PR 防止模型将 JSON 数组形式的工具参数以及泄漏的
</parameter></function>协议结束标签输出到 UI、会话历史或 session 记录中。失败轮次会复用现有的协议泄漏重试流程。检测位于共享流处理边界，不再猜测参数名，而是暂存以 JSON 对象或对象数组开头的响应；在响应尚未判定时保持各 part 的顺序，并原样放行普通 JSON 或真实的结构化工具调用事件。数字数组和 Markdown 风格的方括号文本仍会立即流式输出。
为什么需要
一个生产会话中，模型以
finish_reason=stop返回了包含两个 subagent 参数对象的纯文本 JSON 数组以及工具协议结束标签，但没有返回结构化工具调用。现有保护只处理以 XML 风格协议标签开头的响应，因此这个 JSON 变体被当作助手文本展示并持久化，没有触发重试。Reviewer Test Plan
验证方式
</parameter></function>，分别覆盖独立终止事件、finish reason 与内容同块、任意首个参数键、直接对象和尾随文本。确认失败轮次不输出任何 part、不写入历史或 recording，并且重试后只暴露成功响应。修复前后证据
修复前：生产形态的响应会直接作为助手文本输出并持久化，不触发重试。
修复后：相同的 905 字符响应触发
PROTOCOL_TAG_LEAK，失败轮次输出 0 个 part，history 和 recording 不变，随后重试且只暴露成功响应。测试环境
Node.js 22，无 sandbox。已通过 321 个受影响单元测试、全仓 build 与 typecheck、ESLint、Prettier 和
git diff --check。风险与范围
关联 Issue
Fixes #8207