fix(core): route id-less continuation chunks to a colliding tool-call opener's slot - #6981
Conversation
… opener's slot
StreamingToolCallParser remaps a provider index to a fresh slot when a second
tool call reuses an already-completed index. When that second call's id and
name arrive together on an empty opener delta (the standard OpenAI streaming
shape, function: { name, arguments: "" }), the index -> slot remap was never
recorded, because it was guarded by !meta.id and the id was already set. The
following id-less argument chunks then failed to find the remapped slot and
were routed to a brand-new orphan slot: the tool call was emitted with empty
args {}, the real arguments (in a nameless slot) were dropped by
getCompletedToolCalls, and the nameless slot made converter.ts raise
InvalidStreamError('MALFORMED_TOOL_CALL'), forcing up to four wasted retries of
an otherwise valid response.
Record the remap whenever the actual slot differs from the provider index, not
only before an id is known, so id-less continuations follow it. To keep a
genuinely new tool call from hijacking that slot, only adopt a pending remap
for a new id when the remapped slot has not itself been claimed by an id yet
(the existing id-arrives-late path); otherwise fall through to normal collision
handling.
Adds a regression test for the id+name-on-opener collision shape, which the
existing "late stable ID" collision tests did not cover.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hi @he-yufeng, thanks for the PR — the bug analysis and fix look solid, but the description doesn't follow the PR template. The template requires these headings:
## What this PR does## Why it's needed## Reviewer Test Plan(with### How to verify,### Evidence (Before & After),### Tested on)## Risk & Scope## Linked Issues
Your PR uses ## What, ## Fix, and ## Test instead. Could you restructure the body to match the template? The content is already great — it just needs to be placed under the correct headings so reviewers can find it quickly.
中文说明
你好 @he-yufeng，感谢提交！Bug 分析和修复看起来很好，但 PR 描述没有遵循 PR 模板。模板要求以下标题：
## What this PR does## Why it's needed## Reviewer Test Plan（包含### How to verify、### Evidence (Before & After)、### Tested on）## Risk & Scope## Linked Issues
你的 PR 使用了 ## What、## Fix 和 ## Test。请将正文重新组织到模板的标题下——内容本身已经很好，只需要放在正确的标题下方便 reviewer 查阅。
— Qwen Code · qwen3.7-max
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: chunk 1 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
| it('routes id-less continuation chunks to a slot claimed by a colliding opener delta', () => { | ||
| parser.addChunk(0, '{"a":1}', 'call_1', 'function1'); |
There was a problem hiding this comment.
[Suggestion] The new test covers the removed !meta.id guard (remap recording) but does not exercise the added guard !this.toolCallMeta.get(...)?.id on pending-remap adoption. In this test, step 1 (call_1 at index 0) never creates a remap because actualIndex === index, so the guarded else if branch is never reached. A third id-bearing call at the same provider index would be needed to hit that branch.
Failure scenario: If the !meta.id guard were accidentally removed, all existing tests still pass — a third tool call at the same index would silently hijack the second call's remapped slot, dropping its continuation arguments.
it('does not let a third colliding opener hijack a remapped slot', () => {
parser.addChunk(0, '{"a":1}', 'call_1', 'function1');
parser.addChunk(0, '', 'call_2', 'function2'); // creates remap 0→1
const third = parser.addChunk(0, '', 'call_3', 'function3');
expect(third.actualIndex).toBe(2); // must NOT adopt slot 1
// call_2's continuation still routes correctly
expect(parser.addChunk(0, '{"b":2}').actualIndex).toBe(1);
});— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
The added guard's block direction is covered by the third test, does not let a brand-new tool-call id adopt a remap slot that already has an id: after call_2 claims the 0->1 remap with its own id, a third call reusing index 0 with a fresh id asserts third.actualIndex !== 1, which fails if the !toolCallMeta.get(remap)?.id guard is removed. This first test intentionally covers the record path; the third covers the adopt-guard path.
Qwen Code Review (qwen3.7-max)结论：ISSUES_FOUND — 验证通过，发现 3 个建议级问题（2 高置信 + 1 低置信）和 1 个 Nice to have。 审查摘要PR 修复了 Findings
Rejected / 非问题
|
Adds a case where a brand-new tool-call id reuses an index that already has a 0->N remap to an id-claimed slot: it must not hijack that slot and instead gets its own. This exercises the `!toolCallMeta.get(remap)?.id` guard directly (the prior test only covered id-less continuation routing). Fails without the guard.
|
Thanks for the review. Addressed both points:
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: chunk 1 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
…p doc Per review: add a case for the content-bearing colliding opener (arguments fragment arriving with the id/name, exercising the line-239 remap-record path that the empty-opener test skipped), assert call_1's arguments survive the collision in the id-less-continuation test, and update the pendingIndexRemaps doc to describe its dual role (post-id adoption and id-less continuation routing).
|
Addressed the remaining suggestions (001a710):
Parser suite is now 88/88, |
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. 1 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output. Not reviewed: chunk 1 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
| // once it has one, the remap only exists to route later id-less | ||
| // continuation chunks, so a brand-new id must not hijack that slot. | ||
| actualIndex = this.pendingIndexRemaps.get(index)!; | ||
| this.pendingIndexRemaps.delete(index); |
There was a problem hiding this comment.
[Suggestion] Dead delete — pendingIndexRemaps.delete(index) here is unconditionally re-created at line 220 (empty-chunk early return) or line 250 (normal path), since actualIndex !== index always holds on the adoption path. The delete has no runtime effect.
Concrete cost: An oncall engineer reading this delete would conclude the remap is gone after adoption and waste time tracing a phantom state change. A future refactor adding an early return between this delete and the re-set would silently break id-less continuation routing.
Fix: Remove the this.pendingIndexRemaps.delete(index); line entirely. The downstream pendingIndexRemaps.set(index, actualIndex) at lines 220/250 already overwrites the entry with the same value.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Agreed — removed in ef62d64. You're right that the common-path re-registration re-creates the mapping on the adoption path (actualIndex !== index always holds and isKnownId is false there, so the "ignoring replay chunk" early return isn't reached), so the delete had no runtime effect. I left a comment where it was so the remap's intended lifetime — kept alive for later id-less continuations — reads straight through and nobody re-adds it.
Once an id adopts a remapped slot, the common-path re-registration keeps the index->slot mapping alive for later id-less continuation chunks, so the delete in the adoption branch is immediately re-created and has no runtime effect. Remove it and note why the remap is intentionally kept, so the mapping's lifetime reads straight through.
|
@qwen-code /triage — the description now follows the PR template (all required headings plus the Tested-on table), and I've addressed both inline suggestions: removed the dead remap delete (ef62d64) and pointed to the existing coverage for the adopt-guard block direction. Ready for another look. |
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.
— qwen3.7-max via Qwen Code /review
| meta.name = validName; | ||
| } | ||
| } | ||
| if (!meta.id && actualIndex !== index) { | ||
| if (actualIndex !== index) { | ||
| this.pendingIndexRemaps.set(index, actualIndex); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The common-path remap recording unconditionally overwrites an existing pendingIndexRemaps entry when a third tool call collides at the same provider index. While this 3+ collision scenario is pre-existing (the old code also fails via slot hijacking), guarding the overwrite would prevent the failure without breaking the 2-call fix.
Failure scenario: three tool calls at provider index 0 with content-bearing openers -- call_2 at slot 1 (incomplete args), call_3 at slot 2 (incomplete args). Remap 0->1 is overwritten to 0->2. An id-less continuation for call_2 routes to slot 2 (call_3's incomplete buffer) and is appended there, corrupting call_3's args and leaving call_2 incomplete.
| meta.name = validName; | |
| } | |
| } | |
| if (!meta.id && actualIndex !== index) { | |
| if (actualIndex !== index) { | |
| this.pendingIndexRemaps.set(index, actualIndex); | |
| } | |
| if (actualIndex !== index && !this.pendingIndexRemaps.has(index)) { | |
| this.pendingIndexRemaps.set(index, actualIndex); | |
| } |
-- qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks — I looked at this closely and verified it empirically, and I'm going to keep the unconditional overwrite. The suggested guard (!pendingIndexRemaps.has(index)) keeps the first remap, but an id-less continuation is supposed to belong to the most recent opener at that provider index (the sequential-completion assumption the whole collision path relies on). Keeping the first mapping pins the remap at call_2's slot, so a continuation after call_3's opener is misrouted to call_2.
I added a regression test for exactly the well-formed three-call case (6b62711): call_1@0, call_2 opener→slot 1 with its args, call_3 opener→slot 2, then an id-less {"c":3} that must land on slot 2. It passes with the overwrite and fails with the suggested guard (the continuation lands on slot 1 — the findMostRecentIncompleteIndex fallback doesn't recover it). So the guard would trade the pre-existing 3-call interleaved-corruption case (which it only half-fixes — it moves the corruption from call_2 to call_3) for a regression in the well-formed 3-call case. The interleaved-malformed 3-call case is genuinely pre-existing and out of this PR's 2-call scope.
…peners The unconditional remap overwrite is deliberate: an id-less continuation after the newest colliding opener must route to that opener's slot. A regression test covers the three-call case so the overwrite is not later 'guarded' back into misrouting the third call's continuation to an earlier slot.
|
@qwen-code /triage — description follows the template and the one /review suggestion is addressed (kept the unconditional remap overwrite; the suggested guard regresses the well-formed three-call case, verified by a new test in 6b62711). Ready for another look. |
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ — all required headings present. Problem: this is an observed bug, not theoretical hardening. The PR describes a concrete misrouting path in Direction: aligned — this is a correctness fix in the core streaming parser, squarely within qwen-code's mission. No CHANGELOG reference needed for a parser bugfix. Size: 25 production lines (19+/6−) in Approach: the scope feels right. Three small, related changes — drop the overly restrictive Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 所有必需标题齐全。 问题：这是一个已观测到的 bug，而非理论性加固。PR 描述了 方向：对齐——这是核心流式解析器的正确性修复，完全在 qwen-code 的使命范围内。 规模： 方案：范围合理。三个小而相关的改动——移除过于严格的 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent proposal: given the bug description (id-less continuation chunks misrouted when a provider reuses an index and the opener carries id+name together), I would (1) drop the Comparison with the diff: the PR does exactly this. The three production changes map 1:1 to the proposal above. No simpler path missed, no correctness issues found. Specifics I verified by reading the full
No critical blockers. No AGENTS.md violations. The JSDoc update on Four new tests cover: empty-opener collision + id-less continuation, content-bearing opener collision, adoption-guard block direction, and three-way collision with remap overwrite. Each test asserts both the routed TestingNon-user-visible core parser change — no TUI surface affected. Real-scenario tmux testing: N/A. CI signal on
macOS/Windows/integration tests are skipped — typical for fork PRs where platform-specific runners or secrets are unavailable. The ubuntu unit suite (which includes the parser tests) passed. No failures. 中文说明独立方案： 根据 bug 描述（provider 复用 index 且 opener 同时携带 id+name 时，无 id 的后续 chunk 路由错误），我的方案是：(1) 移除 remap 记录上的 与 diff 对比： PR 的实现与上述方案完全一致。三个生产代码改动与方案一一对应。未发现更简路径，未发现正确性问题。 已验证的关键点：认领守卫正确阻止新 id 劫持已有 id 的 slot，同时保留 id 延迟到达路径的认领能力；移除 无关键阻塞项。无 AGENTS.md 违规。四个新测试覆盖：空 opener 碰撞 + 无 id 后续、带内容 opener 碰撞、认领守卫阻断方向、三次碰撞 remap 覆盖。 测试：非用户可见的核心解析器改动，无 TUI 界面影响。真实场景 tmux 测试：N/A。CI 信号：ubuntu 单元测试通过，macOS/Windows/集成测试跳过（fork PR 典型情况），无失败。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. This is a tight, well-evidenced correctness fix. The bug is real and the reproduction is deterministic — reverting any of the three production changes fails the corresponding test. The fix matches what I'd have written independently: drop the guard that was too restrictive, add the guard that was missing, and remove the dead delete. Twenty-five production lines, four focused tests, no scope creep. The author iterated through three rounds of review feedback (qwen-review suggestions plus inline review comments) and addressed each one — added the block-direction test for the adoption guard, covered the content-bearing opener path, asserted CI is green on ubuntu (the only platform that runs for fork PRs). No user-visible surface, so no tmux evidence needed. LGTM, approving. ✅ 中文说明置信度：5/5 —— 各阶段均无问题，毫不犹豫地合并。 这是一个紧凑、证据充分的正确性修复。bug 真实存在，复现是确定性的——回退三个生产代码改动中的任何一个都会导致对应测试失败。修复与我独立提出的方案一致：移除过于严格的守卫、添加缺失的守卫、删除死代码。25 行生产代码，四个聚焦的测试，无范围蔓延。 作者经过三轮审查反馈迭代，逐一解决了每个建议。最终 diff 小巧、易于推理、易于回退。 CI 在 ubuntu 上通过（fork PR 唯一运行的平台）。无用户可见界面，无需 tmux 证据。 LGTM，批准。✅ — Qwen Code · qwen3.8-max-preview 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. ✅
|
@he-yufeng Thanks for the updates, and for flagging that the review suggestion was addressed (with the regression test in 6b62711 — appreciated). One small note: the @he-yufeng 感谢更新，也谢谢你说明已处理那条 review 建议（还补了 6b62711 的回归测试，赞）。 小提示： |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
Local verification report — merge referenceI built and ran this PR locally as an independent check before merging. Everything below is real output from a trial merge of the PR head onto current Verdict: safe to merge. The bug is real, more common than the description suggests, and all three production changes are correct. One non-blocking gap: one of the three changes has no test coverage — details in §5. Setup
Both touched files are untouched on 1. Test suites
The 14 failing files are byte-identical on both sides and are local-environment failures in my symlinked
2. Mutation matrix — do the new tests actually pin the fix?I reverted one production change at a time and re-ran the 89-test suite.
M6 settles the disagreement in the author's favour. The suggested guard really does regress the well-formed three-call case, and M4 confirms the dead-code claim. Re-adding the deleted line changes nothing, so 3. Real-world impact — end-to-end through the real converterThe PR's evidence is unit-level. I drove the same wire bytes through the actual
The retry cost in the description checks out: 4. Differential fuzz — 20,000 randomized provider streams per variantIdentical generated streams into each parser, comparing slot routing and emitted tool calls:
40% divergence is the main thing I'd want a reviewer to see: this is not an exotic corner case. Any provider that recycles a streaming index with the standard 5. One gap (non-blocking): the L252 change is untested
The line is not dead, though. Add a second still-open tool call at a higher slot and the fallback stops coinciding: parser.addChunk(3, '{"z":', 'call_0', 'fn0'); // slot 3, left open (depth 1)
parser.addChunk(0, '{"a":1}', 'call_1', 'fn1'); // slot 0, complete
parser.addChunk(0, '{"b":', 'call_2', 'fn2'); // collision -> slot 1, content-bearing opener
parser.addChunk(0, '2}'); // id-less continuation
So the change fixes a second, distinct misroute that the PR never claims or demonstrates. Suggest adding the four-chunk stream above as a fifth test — it would make the mutation matrix 3/3 on production changes. Not a merge blocker; the code is correct as written. NitThe description says the parser suite is 中文说明本地验证报告 —— 合并参考在合并前我在本地独立构建并运行了这个 PR。以下全部是把 PR head 试合并到当前 结论：可以安全合并。 Bug 真实存在，且比描述中显得更常见，三处生产代码改动都正确。一个不阻塞的缺口：三处改动中有一处没有任何测试覆盖，详见第 5 节。 环境
两个被修改的文件自 merge-base 起在 1. 测试套件
这 14 个失败文件在两侧完全一致，属于我本地 symlink
2. 变异测试矩阵 —— 新测试是否真的钉住了这个修复？每次只回退一处生产代码改动，然后重跑 89 个测试。
M6 判定了那处分歧，作者是对的。 被建议的守卫确实会让格式良好的三次调用场景回归，而 M4 证实了死代码的说法。 把删掉的那行加回来行为完全不变，所以 3. 真实影响 —— 经由真实 converter 的端到端验证PR 的证据停留在单元层面。我把同样的 wire 字节喂进真实的
描述里说的重试代价也核对无误： 4. 差分模糊测试 —— 每个变体 20,000 条随机 provider 流把完全相同的生成流喂给每个解析器，比对槽位路由和最终发出的工具调用：
40% 的分歧率是我最希望 reviewer 看到的一点：这根本不是罕见边角场景。任何会复用流式 index、且 opener 采用标准 5. 一个缺口（不阻塞）：L252 的改动没有测试覆盖
不过这行并非死代码。只要在更高的槽位上再加一个仍然打开的工具调用，兜底就不再重合： parser.addChunk(3, '{"z":', 'call_0', 'fn0'); // 槽位 3，保持打开（depth 1）
parser.addChunk(0, '{"a":1}', 'call_1', 'fn1'); // 槽位 0，已完成
parser.addChunk(0, '{"b":', 'call_2', 'fn2'); // 冲突 -> 槽位 1，content-bearing opener
parser.addChunk(0, '2}'); // 无 id 的续传块
也就是说这处改动修掉了 PR 从未声称、也未演示的第二个、独立的路由错误。建议把上面这段四块流补成第五个测试 —— 那样变异矩阵在生产代码改动上就是 3/3。这不是合并阻塞项，代码本身是正确的。 小提示描述里写解析器套件是 |
|
Released in v0.21.1. |
4 participants
What this PR does
Fixes a silent tool-call argument-loss bug in
StreamingToolCallParser. When a provider reuses the same streamingindexfor a second tool call whoseidandnamearrive together on an empty opener delta (the standard OpenAI streaming shapefunction: { name, arguments: "" }), theindex -> slotremap was never recorded — it was guarded by!meta.id, and the id was already set. The subsequent id-less argument chunks then failed to find the remapped slot and were routed to a fresh orphan slot: the tool call was emitted with emptyargs {}, the real arguments (in a nameless slot) were dropped bygetCompletedToolCalls, and the nameless slot madeconverter.tsraiseInvalidStreamError('MALFORMED_TOOL_CALL'), forcing up to four wasted retries of an otherwise valid response.The fix records the remap whenever the actual slot differs from the provider index, and guards pending-remap adoption so a brand-new id cannot hijack a slot that already has an id. A follow-up commit also removes the now-dead
pendingIndexRemaps.deletein the adoption branch (the common-path re-registration immediately re-creates it), which a reviewer flagged, and documents why the remap is intentionally kept alive.Why it's needed
Under the standard OpenAI streaming protocol, argument continuation chunks carry neither
idnorname. When a provider recycles an index, these continuations were misrouted, so any tool call in that shape silently lost its arguments and burned the retry budget. This is a real provider-stream shape, not a synthetic edge case, and the existing collision tests only covered the id-arrives-late variant.Reviewer Test Plan
How to verify
Three tests are added:
routes id-less continuation chunks to a slot claimed by a colliding opener delta,routes id-less continuations after a content-bearing colliding opener, anddoes not let a brand-new tool-call id adopt a remap slot that already has an id(the block direction of the adoption guard).Evidence (Before & After)
Non-user-visible core parser change, so the evidence is the test suite rather than a screenshot.
routes id-less continuation chunks...—continuation.actualIndexis an orphan slot and the call is emitted withargs: {}. Reverting the added guard failsdoes not let a brand-new tool-call id adopt...—third.actualIndexis1, hijacking the prior call's slot.Test Files 1 passed,Tests 88 passed. The fullopenaiContentGeneratorsuite stays green.Tested on
macOS verified locally (parser suite 88/88,
prettier/eslintclean); Windows and Linux are exercised by CI.Environment (optional)
N/A — unit tests only.
Risk & Scope
StreamingToolCallParser's index-remap bookkeeping; the id-arrives-late path and public API are untouched.Linked Issues
None — a self-contained streaming-parser correctness fix found while reading the collision/remap code introduced in #6819.