fix(core): re-land malformed stream retry with narrower nameless detection - #6794
Conversation
… drops hasNamelessToolCall() flagged any buffer slot lacking a name, including phantom slots (no id, no argument content) created by trailing structural deltas. That tripped shouldDropMalformedAttempt on legitimate tool-call responses, clearing parts and suppressing finish_reason to undefined, which surfaced as 'Model stream ended without a finish reason' and broke E2E suites depending on tool-call emission. Only treat a nameless tool call as malformed when the provider actually started a real call (assigned id or streamed argument content). Genuine #6666 nameless attempts (id or args but no name) still drop and retry; phantom slots are skipped by getCompletedToolCalls as before.
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. |
|
Thanks for the re-land! This is a well-scoped follow-up to the #6783 revert. Template looks good ✓ — all required sections present. The "Risk & Scope" section from the template is missing but that's a minor omission, not a blocker. Problem: observed regression. The original fix (#6754) was reverted (#6783) because metadata-free phantom tool-call slots suppressed Direction: aligned. Model stream parsing and retry behavior are squarely within core's responsibilities. The narrower approach — rejecting only real nameless calls and the recorded thinking-tag leak, while ignoring phantom slots — is the right call. The explicit non-goals (no generic Size: 275 production lines, 370 test lines, 0 generated/schema lines. All changes within Approach: The scope feels right. The deferred-parts mechanism ( One question: the thinking-tag leak detection adds several regex patterns ( Moving on to code review. 🔍 中文说明感谢重新提交！这是 #6783 revert 后范围控制良好的后续修复。 模板完整 ✓ — 所有必需章节都在。模板中的 "Risk & Scope" 部分缺失，但不阻塞。 问题：已观测到的回归。原始修复（#6754）被 revert（#6783），因为无元数据的 phantom tool-call slot 会抑制合法 tool call 的 方向：对齐。模型流解析和重试行为完全属于核心模块的职责范围。更窄的方案——拒绝真正无 name 的调用和已记录的 thinking-tag 泄漏，同时忽略 phantom slot——是正确的选择。明确的非目标（不处理纯 content 中的通用 规模：275 行生产代码，370 行测试代码，0 行生成/schema 代码。所有变更都在 方案：范围合理。延迟 parts 机制（ 一个问题：thinking-tag 泄漏检测添加了多个正则表达式模式（ 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
|
E2E and verification report for the current branch. Current headCommit: Latest review follow-up
Verification on the current head
Prior independent stream verificationThe previous stable head independently passed these exact-stream cases:
tmux E2E evidenceThe deterministic tmux replay was run from runtime head
Thought-tag leak before a named tool call
Malformed tool-call retry classification
Strict assertions: 12/12 passed. Malformed attempts produced no visible or persisted output; clean responses appeared exactly once; retry classifications were present in debug logs. Full workflow matrixThe full E2E workflow was manually dispatched on pre-review head https://github.com/QwenLM/qwen-code/actions/runs/29222640370 All four jobs passed:
The full workflow was not re-dispatched after the narrow review follow-ups. @wenshao |
Code ReviewClean implementation. The PR does exactly what it says — re-lands the malformed stream retry with narrower detection that distinguishes real nameless calls from phantom slots. Independent proposal vs. PR approach: I would have added nameless-call tracking in the parser, a deferred-parts mechanism for the uncertainty window, and thinking-tag leak detection. The PR's approach matches this closely. The phantom-chunk early-return (skip chunks with no id, no name, empty content at depth 0) is a nice minimal addition that the previous version lacked. No critical issues found. A few observations:
Reuse check: No new utilities or abstractions that duplicate existing code. The regex patterns for thinking-tag detection are specific to this use case. Unit TestsAll 449 tests pass across the three changed test files:
New tests cover the key scenarios: phantom slot acceptance, nameless call rejection, thinking-tag leak detection (including cross-channel and split-tag variants), late-name deduplication, and the retry path through the OpenAI pipeline. Real-Scenario TestingThis PR fixes internal stream parsing behavior (malformed tool calls, thinking-tag leaks) that requires specific model responses or a mock server to trigger. A simple CLI prompt cannot produce these edge cases. Smoke test — verified the CLI starts and responds correctly with the PR's code: Typecheck — The specific malformed-stream scenarios are thoroughly covered by the unit tests above, which simulate the exact failure modes described in the PR (phantom slots, nameless calls, cross-channel thinking-tag leaks). 中文说明代码审查实现干净。PR 完全按照描述执行——用更窄的检测逻辑重新落地 malformed stream 重试，区分真正无 name 的调用和 phantom slot。 独立方案 vs. PR 方案： 我会在解析器中添加无 name 调用跟踪、不确定性窗口的延迟 parts 机制、以及 thinking-tag 泄漏检测。PR 的方案与此接近。phantom-chunk 提前返回（跳过无 id、无 name、空内容且 depth 为 0 的 chunk）是之前版本缺少的精简新增。 未发现关键问题。 几点观察：
复用检查： 没有新增与现有代码重复的工具或抽象。thinking-tag 检测的正则表达式模式是特定于此用例的。 单元测试三个变更测试文件共 449 个测试全部通过：
新测试覆盖了关键场景：phantom slot 接受、无 name 调用拒绝、thinking-tag 泄漏检测（包括跨 channel 和分割标签变体）、延迟 name 去重，以及通过 OpenAI pipeline 的重试路径。 真实场景测试本 PR 修复的是内部流解析行为（malformed tool call、thinking-tag 泄漏），需要特定的模型响应或 mock 服务器才能触发。简单的 CLI 提示无法产生这些边界情况。 冒烟测试 — 验证 CLI 在 PR 代码下能正常启动和响应： 类型检查 — 上述单元测试充分覆盖了 PR 描述的具体失败模式（phantom slot、无 name 调用、跨 channel thinking-tag 泄漏）。 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 5/5 — clean across every stage; the implementation is focused, well-tested, and solves a real regression. This is a solid re-land. The author has iterated across three PRs (#6754 → #6783 revert → this) to get the scope right, and the result is tighter than the original fix. The phantom-slot detection is a minimal early-return, the nameless-call tracking is a clean set-based approach, and the retry loop is genuinely simpler without the The 449 unit tests cover the exact failure modes — phantom slots beside valid calls, real nameless calls, cross-channel thinking-tag leaks, and the retry path through the pipeline. The typecheck and CLI smoke test both pass. No scope creep, no unnecessary abstractions, no drive-by refactors. Every change in the diff serves the stated goal. This is ready to ship. ✅ 中文说明信心度: 5/5 — 各阶段均通过；实现聚焦、测试充分、解决了真实回归。 这是一次扎实的重新提交。作者在三个 PR（#6754 → #6783 revert → 本次）中反复调整范围，最终结果比原始修复更精简。phantom-slot 检测是最小化的提前返回，无 name 调用跟踪是干净的基于集合的方案，重试循环在移除 449 个单元测试覆盖了具体的失败模式——合法调用旁的 phantom slot、真正无 name 的调用、跨 channel thinking-tag 泄漏，以及通过 pipeline 的重试路径。类型检查和 CLI 冒烟测试均通过。 无范围蔓延、无不必要的抽象、无顺手重构。diff 中的每个变更都服务于声明的目标。可以合并。✅ — Qwen Code · qwen3.7-max Reviewed at |
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. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
Code Review — reviewed at
|
| Thread | Status |
|---|---|
| Content-only #6666 shape not handled | Descoped honestly: body now says "Related to #6666" with an explicit non-resolution note |
| Unmatched closing tag after ordinary text retried as leak | Fixed — leak now needs visible-start or matching thought-tag evidence; probe confirms no hold, no retry |
Quadratic pendingTag growth + buffered-part rescans |
Fixed — whitespace no longer grows the pending tag; thought-tag evidence persisted monotonically (hasUntrustedThoughtTag) |
| Unbounded phantom slots + full-map scan per chunk | Fixed — pure phantoms return before state allocation; nameless indices tracked incrementally in a Set |
| No converter→pipeline→GeminiChat integration coverage | Fixed — leak-retry test now drives a real OpenAIContentGenerator with raw SSE chunks; pipeline rethrows InvalidStreamError before generic handling and a test asserts object identity |
| Nameless-with-arguments-no-id untested | Fixed — discards a nameless tool call with arguments but no id |
| Drop test ignored first conversion result | Fixed — renamed to drops parts after a later tool call has no name, first result asserted |
| Index reuse merged arguments across calls | Fixed — differing ID always relocates via findNextUnusedIndex(); occupied call preserved |
Findings
1. [Should-fix] The PROTOCOL_TAG_LEAK retry budget silently doubles (2 → 4), reverting a deliberate decision from #6712.
INVALID_STREAM_RETRY_CONFIG goes from {transientMaxRetries: 4, protocolTagLeakMaxRetries: 2} back to a unified maxRetries: 4. The split budget landed in #6712 one day before #6754, with a comment explaining that tag leaks are model-output validation failures rather than the transient provider anomalies of #6670. This PR removes that comment and cap as part of the re-land, and the tests were knowingly renamed (should keep protocol tag leak retries at the existing budget → should retry protocol tag leaks four times) — but the PR body never mentions the change. For a model that deterministically leaks, failure now costs 5 full generation attempts plus 20s of backoff (2+4+6+8s) instead of 3 attempts plus 6s. Please either restore the split budget or state in the PR body why unifying at 4 is now correct; as written, a future reader will find #6712's rationale erased with no counter-rationale.
2. [Nit] Stale doc comment in packages/core/src/telemetry/types.ts (~line 675).
The ApiRetryEvent docs still describe ContentRetryEvent as "emitted by geminiChat's for-loop … (INVALID_CONTENT_RETRY_OPTIONS, NOT retryWithBackoff)". This PR deletes both that constant and the bounded for-loop (now for (;;) with per-class budgets), so the comment references code that no longer exists. Worth updating in the same PR since it deletes the referent. (For the record, I audited the new for (;;) loop: every continue is guarded by a bounded counter or a one-shot flag, so removing the outer attempt bound is safe — the branch it fed was already documented as unreachable.)
3. [Nit] PR body test-plan numbers are stale relative to the head commit.
"155/155" converter tests and "this PR is draft until E2E is green" predate b64cc65c7 (175 converter tests now; the PR is no longer draft). Minor, but the Reviewer Test Plan should match what a reviewer will actually see.
Risk notes (accepted tradeoffs, no action required)
- Held-parts latency: while a nameless call is pending (ID seen, name not yet) or after thought-tag evidence appears, text parts buffer until disambiguation or finish. Ordering is preserved (probe-verified); worst case is batch-at-finish delivery for that turn.
- Residual false-positive window: reasoning that legitimately quotes a thinking tag combined with an unmatched closing tag in visible text is indistinguishable from the qwen 3.7 max 模型在 content 字段中返回 <think> 标签而非 reasoning_content #6666 leak signature and will retry. Balanced inline tags survive (open/close counting), so this is narrow.
- The real merge gate remains the manually dispatched real-model E2E run — the failure mode that killed fix(core): retry malformed streamed responses #6754 (phantom-slot false positives) is now unit- and probe-covered, but E2E green should stay the final word, per the PR's own plan.
Verdict
The re-land is well executed at this head: the narrowing is surgical (!name && (id || args) semantics, incrementally tracked), the leak scanner is bounded and evidence-gated, prior review feedback is fully and verifiably addressed, and the integration seam (converter throw → pipeline rethrow → GeminiChat instanceof retry) is now covered by a real-stream fixture. Finding 1 (document or restore the leak-retry budget) is the only thing I'd ask to resolve before merge, alongside the already-planned E2E dispatch.
中文摘要
在 b64cc65c7 上复审。本地跑通全部相关套件(601/601)+ typecheck,并用一次性探针验证了:phantom 槽不再误杀(#6783 的确切场景)、迟到的 name 会按序释放暂扣文本、Use </think>… 普通文本不再触发重试、tool_calls 空完成会被扣留重试、ID 冲突改为迁移不再串参、两个二次方性能问题已消除。上一轮 8 条评审意见在该提交全部真实修复且各有回归测试。
遗留两点:(1) 应修: PROTOCOL_TAG_LEAK 重试预算从 2 静默翻倍到 4,抹掉了 #6712 的有意拆分及其注释——确定性泄漏模型的失败成本从 3 次尝试 + 6s 变为 5 次 + 20s,请在 PR 描述中说明理由或恢复拆分;(2) 小问题: telemetry/types.ts 中 ApiRetryEvent 注释仍引用本 PR 已删除的 INVALID_CONTENT_RETRY_OPTIONS 与 for 循环;另 PR 描述中的测试数字(155/155、draft 状态)已过期。真实模型 E2E 仍应为合并门槛。整体质量很高,倾向通过。
|
Review closeout on
Verification run locally:
All review threads are resolved; CI is still running on the new head. |
…-v2' into cx/pr-6794-review-fixes # Conflicts: # packages/core/src/core/invalid-stream-error.ts # packages/core/src/core/openaiContentGenerator/converter.test.ts # packages/core/src/core/openaiContentGenerator/converter.ts
SummaryRe-lands malformed streamed-response retry (after #6783 reverted #6754), with narrower nameless tool-call detection so legitimate tool calls are not misclassified as malformed and wrongly retried. Core paths: Confidence: 3/5 — defer to a maintainer. This re-lands a change that was reverted for breaking tool-call emission across the E2E suite, so it deserves a human sign-off — but it is well-defended: the PR's own suite (472 tests) passes and a real tmux run confirms the malformed-stream retry recovers. sequenceDiagram
participant User as User
participant Chat as GeminiChat
participant Prov as Model Provider
User->>Chat: prompt
Chat->>Prov: stream request (attempt 1)
Prov-->>Chat: content but no finish_reason
Chat->>Chat: InvalidStreamError NO_FINISH_REASON
Chat->>Prov: retry (attempt 2)
Prov-->>Chat: content plus finish_reason stop
Chat->>User: recovered answer
Real-scenario testing (tmux)Drove the installed build (before) and this PR's build (after — Both retry the malformed stream and recover the valid answer ( Scope note (honest): this simple Refreshed for head One thing worth checking (from the run logs)In headless VerdictDefer to the maintainer for the core re-land call. The regression that got #6754 reverted (tool-call emission) is exactly what the narrower detection targets, and the PR's suite covers it (472 pass) — but re-landing a reverted core change deserves a human sign-off. Recommend also confirming the partial-content discard on the — Qwen Code · qwen3.7-max Reviewed at 中文说明摘要:重新落地"流式响应损坏时重试"(#6783 曾 revert 掉 #6754),并收窄 nameless tool-call 检测,避免合法 tool call 被误判为损坏而错误重试。触及 core: 置信度:3/5 —— 转交维护者。 这是重新落地一个曾因"破坏 tool-call 输出"而被 revert 的改动,需人把关;但防护到位:PR 自己的 472 个测试全过,tmux 真机也确认损坏流重试能恢复。 tmux 真机测试:用 mock 的 OpenAI 端点(首个流只发 content、无 诚实说明:这个简单的 一个可分析的改进点:headless 结论:core 重新落地的合并决定转交维护者。让 #6754 被 revert 的回归(tool-call 输出)正是本次收窄检测所针对的,且 PR 单测已覆盖(472 过)——但重新落地被 revert 的 core 改动应由人签字。另建议确认 |
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.
Code Review —
|
Final scope audit and current-head verificationI re-audited the review history against the original malformed-stream fix, the #6783 phantom regression, observed traces, and provider-valid stream shapes. The final head deliberately removes review-driven recovery for synthetic states instead of continuing to expand the parser.
flowchart LR
A["Observed trace / valid provider shape"] --> B["Small validation boundary"]
B --> C["Retry with typed InvalidStreamError"]
D["Reverse order, interleaved tag names, same-index ID takeover"] -. "no producer; removed" .-> E["Scanners, relocation map, Cartesian tests"]
Kept
Removed or declined
These cases were technically constructible in unit tests, but no trace, specification, or checked-in producer required them. The exact threads that led to scanner/routing growth now have technical scope-reset replies. Verification on
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] streamingToolCallParser.ts:94-127 again splices arguments when a different tool-call ID reuses an incomplete index. With call_first at index 0 holding {"fromFirst": and call_second at the same index supplying "fromSecond"}, the metadata is overwritten and the converter emits only call_second({fromFirst: "fromSecond"}); call_first disappears. This directly contradicts Reviewer Test Plan item 4 and the earlier reply that marked the case fixed. Treat every different ID as an identity boundary and isolate or reject it before appending arguments.
— Codex $qreview 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 — no blockers. Suggestions are inline.
Review follow-up — head
|
|
@qwen-code /triage |
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. ✅
🔁 Re-verification report at head
|
Scope retrospectiveA short note on why this PR temporarily grew far beyond its original contract, and why the current smaller implementation is intentional. The expansion came from two implementation decisions:
At the largest point, the PR was approximately The rule used for the final audit was: an edge case needs an observed trace, a specification-permitted input, or a reachable checked-in producer. Nonconforming streams receive one fail-closed boundary rather than recovery state. This does not dismiss the review feedback. Reachable findings—including literal-tag false positives, arguments-first/name-later ordering, real nameless calls, retry propagation, normal cross-delta text boundaries, and metadata replay causing duplicate execution—remain fixed and covered. Detailed scope and verification evidence: #6794 (comment) Latest follow-up audit: #6794 (comment) |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Independent re-review against current head. Pre-checks all green (tsc, eslint, 449/449 tests). The malformed stream retry re-land is well-scoped and the nameless-tool-call / thinking-tag-leak detection paths are correct. The pendingUntrustedResponseParts hold-and-release mechanism properly prevents visible content from being emitted before the stream is classified. for(;;) retry loop with per-type budgets replaces the old ContentRetryOptions path cleanly. No blockers found. Two P2 observations (atVisibleLineStart update scope and per-chunk tag fragment limitation) are intentional or inert.
CI failure investigation: infra flake, not this PR — green after re-run ✅The red ❌ on
Why it can't be this PR:
Resolution: re-run (attempt 2) passed in 3m04s → green job. All checks on the PR are now passing. Unrelated side note for anyone auditing CI health: 中文版本 (Chinese Version)CI 失败调查：基础设施抖动，与本 PR 无关 — 重跑后已全绿 ✅
为什么不可能是本 PR 导致：
处理结果： 重跑（attempt 2）3 分 04 秒通过 → 绿色任务。PR 上所有检查现已全部通过。 附带说明（供关注 CI 健康的人参考）： |
|
@qwen-code /triage |
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. ✅
🔁 Re-verification report at head
|
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. 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
| it('releases inline thinking-tag references in both channels', () => { | ||
| const stream = withStreamParser(); | ||
| const reasoning = converter.convertOpenAIChunkToGemini( |
There was a problem hiding this comment.
[Suggestion] The releases inline thinking-tag references in both channels test covers the hasThinkingTagInReasoning trigger's hold→release path, but no converter-level test covers the nameless-tool-call trigger's hold→release path. A provider that sends a tool-call opener with ID + arguments but no name (parts held), then a later name-only chunk (hold cleared, parts released), exercises pendingUntrustedResponseParts.concat(parts) through a different shouldHoldParts branch than the thinking-tag test. The parser-level accepts and deduplicates a name after complete arguments test confirms hasNamelessToolCall() transitions to false but does not exercise the converter's concat path.
Concrete cost: a bug in the release branch that silently drops held parts for the nameless trigger (e.g. assigning instead of concatenating) would ship undetected.
it('releases held parts when a late name completes a nameless tool call', () => {
const stream = withStreamParser();
const held = converter.convertOpenAIChunkToGemini(
streamChunk('open', {
content: 'visible text',
tool_calls: [{ index: 0, id: 'call_1', function: { arguments: '{"path":"a.ts"}' } }],
}),
stream,
);
expect(held.candidates?.[0]?.content?.parts).toEqual([]);
const released = converter.convertOpenAIChunkToGemini(
streamChunk('name', { tool_calls: [{ index: 0, function: { name: 'read_file' } }] }),
stream,
);
expect(released.candidates?.[0]?.content?.parts).toEqual([{ text: 'visible text' }]);
});— qwen3.7-max via Qwen Code /review
6 participants
What this PR does
Re-lands malformed streamed-response retry handling after #6783 reverted #6754, while keeping the fix bounded to observed or contract-valid stream shapes.
The final implementation:
tool_callsfinish with no completed named call;InvalidStreamErroridentity through the OpenAI pipeline and verifies the converter → pipeline → GeminiChat retry path.This PR intentionally does not parse
<think>tags from content-only responses. That behavior is related to #6666 but requires a separate model/provider policy decision, so this PR does not claim to resolve it.It also does not attempt to recover arbitrary channel reordering, tag-name fragmentation across interleaved channels, or multiple tool-call IDs taking ownership of the same in-flight wire index. No observed trace or provider contract requires those sequences; supporting them previously added a second routing protocol and character-level scanners without improving the original fix.
Why it is needed
The original retry fix correctly rejected malformed streamed responses, but metadata-free phantom slots could suppress
finish_reasonon otherwise valid tool calls. That broke tool-call emission across the E2E suite.This version preserves valid named calls and trailing phantom slots, while real nameless calls and the recorded protocol-tag leak still enter the existing invalid-stream retry path.
Reviewer Test Plan
How to verify
STOP.<think>/</think>references remain visible.tool_callsfinish with no completed named call is malformed instead of accepting fallback text.MALFORMED_TOOL_CALLtravels through converter and pipeline into GeminiChat, uses the existing transient retry budget, and records the correct telemetry type.Evidence on
6f4ffd5ff7git diff --check, and pre-commit formatting/lint: passed.edb7bc23da; this follow-up did not change GeminiChat.edb7bc23da: the first response streamed content plus a real nameless tool call and ended withtool_calls; debug loggedMALFORMED_TOOL_CALL (retry 1/4), the mock received exactly two requests, retry history contained neither the failed text nor call ID, no tool ran, and the TUI displayed onlyRECOVERED_ONLY_6794.RETRYas a rollback signal. This was observed before this scope reset and was not baseline-compared here, so it is not claimed as a regression fixed by this PR.Tested on
Linked Issues
Related to #6666
Re-lands #6754 after #6783
中文说明
本 PR 重新落地 malformed stream 重试，并把范围收回到真实 trace 或协议允许的形状：忽略合法调用旁的 phantom slot；拒绝有 ID/arguments 但始终没有 name 的真实调用；接受 arguments-first、name-later；拒绝已记录的 reasoning-first 跨 channel 标签泄漏；确保错误类型穿过 pipeline 后进入 GeminiChat 重试。
本 PR 不处理纯
content中<think>标签的通用解析，也不再支持没有真实 producer 的 channel 逆序/交错拆标签，以及同一 in-flight index 被多个 ID 反复抢占的路由恢复。这些场景此前导致两个字符 scanner、额外路由表和组合测试膨胀，现已删除。最新 head
6f4ffd5ff7的 converter/parser 222/222、ESLint、core typecheck/build 均通过；此前edb7bc23da的 focused tests 447/447，mock + tmux 真实 nameless tool-call 场景发生一次MALFORMED_TOOL_CALL重试并只显示恢复结果。headless JSON 的失败文本聚合限制仍如实记录，但不在本 PR 范围内。