fix(core): preserve no-argument tool calls that stream an empty arguments string#6250
fix(core): preserve no-argument tool calls that stream an empty arguments string#6250tomsen-ai wants to merge 6 commits into
Conversation
…ents string
For tools that take no parameters, some OpenAI-compatible providers
stream `arguments: ""` (or omit the field entirely) and never send an
argument fragment. The streaming parser dropped such calls wholesale
(`meta?.name && buffer.trim()`), while the non-streaming path keeps
them with `args: {}` — so a turn containing only that call looked
empty and geminiChat raised "Model stream ended with empty response
text", triggering pointless retries.
Align the streaming parser with the non-streaming path: emit the call
with empty args when the buffer is empty at stream end. Rewrite the
unit test that encoded the drop, and add regression coverage at parser
and converter chunk level.
|
Thanks for the PR, @tomsen-ai! Template is mostly complete ✓ — "What", "Why", "Reviewer Test Plan" with before/after evidence are all there. Missing On direction: this is a clear-cut bug fix that solves a real problem. The streaming parser and non-streaming path disagreeing on how to handle no-argument tool calls is the kind of inconsistency that causes silent failures and pointless retries. The blast radius includes built-in tools ( On approach: scope is excellent — one condition relaxed in production code, the JSON parse wrapped in a buffer-non-empty guard, and three focused tests. The fix correctly mirrors the non-streaming path in Moving on to code review. 🔍 中文说明感谢贡献，@tomsen-ai！ 模板基本完整 ✓ — "What"、"Why"、"Reviewer Test Plan" 含修复前后对比均已提供。缺少 方向：这是一个明确的 bug 修复，解决了真实问题。流式解析器和非流式路径在处理无参数工具调用时行为不一致，会导致静默失败和无意义的重试。影响范围包括内置工具（ 方案：范围控制极佳 — 生产代码仅放宽一个条件判断，JSON 解析外包一层缓冲区非空守卫，三个针对性测试。修复正确镜像了 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code reviewThe fix is correct and minimal. The core change in Tests are well-structured: the old "should not return tool calls with empty buffer" assertion is correctly flipped to expect emission with No correctness bugs, no security concerns, no AGENTS.md violations. The comments added in the production code explain why the empty buffer is legal (matching the non-streaming path), which is the right kind of comment. Real-scenario testingSet up a mock OpenAI-compatible SSE server that streams a no-argument Before (installed qwen 0.19.1)The installed v0.19.1 did not reproduce the reported error against this mock server — likely because the mock sends After (PR branch, via unit tests)The unit tests directly exercise the parser and converter paths that the bug affects. The key assertions pass:
The unit test coverage is strong — it tests the exact code path that was broken (the 中文说明代码审查修复正确且最小化。 测试结构良好：旧的"不应返回空缓冲区工具调用"断言被正确翻转为期望以 无正确性缺陷、无安全隐患、无 AGENTS.md 违规。生产代码中添加的注释解释了 为什么 空缓冲区是合法的（与非流式路径一致），属于正确的注释类型。 真实场景测试搭建了模拟 OpenAI 兼容 SSE 服务器，流式发送 已安装的 v0.19.1 未能复现报告中的错误 — 可能因为模拟器发送 单元测试直接覆盖了受影响的代码路径（ — Qwen Code · qwen3.7-max |
|
This is a textbook bug fix. The streaming and non-streaming paths disagreed on empty-argument tool calls; this PR aligns them with a one-line guard relaxation. My independent proposal — "drop the The fix solves something users genuinely hit: providers like llama.cpp that omit the All 207 tests pass. The diff is 95 additions / 17 deletions across 3 files — two test files and one production file. No scope creep, no drive-by refactors, no unnecessary abstraction. The comments added in production code are the right kind: they explain why the empty buffer is a legal end state, not what the code does. The tmux real-scenario test couldn't fully reproduce the bug against the installed v0.19.1 because a simple mock server doesn't replicate the exact llama.cpp streaming pattern (omitting the Approving. Ship it. ✅ 中文说明这是一个教科书级的 bug 修复。流式和非流式路径在处理空参数工具调用时行为不一致，本 PR 通过放宽一行守卫条件使两者对齐。我的独立方案——"从外层条件中去掉 该修复解决了用户实际遇到的问题：像 llama.cpp 这样对无参数工具省略 207 个测试全部通过。Diff 跨 3 个文件：95 行增加 / 17 行删除——两个测试文件和一个生产文件。无范围蔓延、无顺手重构、无不必要的抽象。生产代码中添加的注释属于正确的类型：解释了 为什么 空缓冲区是合法终态，而非描述代码在做什么。 tmux 真实场景测试无法在已安装的 v0.19.1 上完全复现该 bug，因为简单的模拟服务器无法精确复制 llama.cpp 的流式模式（完全省略 批准合并。✅ — 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.
LGTM, looks ready to ship. ✅
| for (const [index, buffer] of this.buffers.entries()) { | ||
| const meta = this.toolCallMeta.get(index); | ||
| if (meta?.name && buffer.trim()) { | ||
| if (meta?.name) { |
There was a problem hiding this comment.
[Suggestion] The JSDoc for getCompletedToolCalls() (line ~248) still states "Only returns tool calls with both name metadata and non-empty buffers", but the guard was changed from meta?.name && buffer.trim() to meta?.name. The comment is now inaccurate — empty buffers produce args: {} for no-argument tools.
| if (meta?.name) { | |
| /** | |
| * Returns completed tool calls from the parser. | |
| * Only returns tool calls that have name metadata. Calls with empty or | |
| * whitespace-only argument buffers are emitted with `args: {}` to match | |
| * the non-streaming path in converter.ts. | |
| */ |
— qwen3.7-max via Qwen Code /review
| expect(completed).toHaveLength(0); | ||
| expect(completed).toEqual([ | ||
| { id: 'call_1', name: 'function1', args: {}, index: 0 }, | ||
| ]); |
There was a problem hiding this comment.
[Suggestion] No test covers a no-argument tool call coexisting with a regular (argument-bearing) tool call in the same parser instance. This mixed case is where the internal routing/collision heuristics (index-collision guard, continuation-chunk routing, findMostRecentIncompleteIndex) are most likely to surface edge cases. Consider adding a test like:
it('should emit both no-arg and regular tool calls from the same stream', () => {
parser.addChunk(0, '', 'call_1', 'list_sessions');
parser.addChunk(1, '{"query":', 'call_2', 'search_web');
parser.addChunk(1, ' "hello"}');
const completed = parser.getCompletedToolCalls();
expect(completed).toHaveLength(2);
expect(completed.find((tc) => tc.id === 'call_1')?.args).toEqual({});
expect(completed.find((tc) => tc.id === 'call_2')?.args).toEqual({
query: 'hello',
});
expect(parser.hasIncompleteToolCalls()).toBe(false);
});— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
[Suggestion] buffer.trim() as occupancy signal is now inconsistent with the new empty-buffer-is-valid semantics.
getCompletedToolCalls() correctly treats empty buffers as valid no-arg tool calls, but three other methods still use buffer.trim() to decide whether a slot is "occupied":
addChunk()line ~92 — collision guardexistingBuffer.trim() &&short-circuits tofalsefor no-arg calls. A second tool call arriving at the same index silently overwrites the first.findNextAvailableIndex()line ~334 —!buffer.trim()returns a valid no-arg call's slot as "available."findMostRecentIncompleteIndex()line ~377 —!buffer.trim()classifies a valid no-arg call as "incomplete," potentially misrouting continuation chunks.
This is dormant for standard providers (OpenAI, Anthropic-via-proxy, DashScope assign unique indices per tool call), but would trigger with buggy providers that reuse indices — exactly the llama.cpp-style servers this PR targets.
Suggested fix: Replace buffer.trim() / !buffer.trim() with meta?.name / !meta?.name as the occupancy signal in all three locations:
// addChunk() line ~92:
if (existingMeta?.name && existingDepth === 0 && existingMeta?.id && existingMeta.id !== id)
// findNextAvailableIndex() line ~334:
if (!meta?.name || depth > 0 || !meta?.id)
// findMostRecentIncompleteIndex() line ~377:
if (meta?.id && (depth > 0 || (meta?.name && buffer.trim())))And add a regression test sending two tool calls at the same index where the first has no arguments.
— qwen3.7-max via Qwen Code /review
| for (const [index, buffer] of this.buffers.entries()) { | ||
| const meta = this.toolCallMeta.get(index); | ||
| if (meta?.name && buffer.trim()) { | ||
| if (meta?.name) { |
There was a problem hiding this comment.
[Suggestion] The guard relaxation here (meta?.name only) correctly preserves empty-buffer no-arg tool calls at emission time, but three buffer.trim() checks in unchanged addChunk() code still use "has buffer content" as a proxy for "this slot is occupied by a real tool call." Now that empty buffer is a valid terminal state, those proxies break in multi-tool-call scenarios:
- Line 93 (collision detection):
existingBuffer.trim()is falsy for an empty-buffer entry, so when a second tool call arrives at the same index, the collision guard is skipped and the first call's metadata is silently overwritten. - Line 149 (replay guard):
currentBuffer.trim()is falsy, so a replayed chunk for a completed no-arg call bypasses the guard and corrupts the buffer. - Lines 121 & 345 (continuation routing +
findMostRecentIncompleteIndex):!existingBuffer.trim()classifies a completed no-arg call as "incomplete," so continuation chunks from other tool calls get misrouted to it.
None of these paths are covered by the new tests (they require multi-tool-call interleaving). The fix for all three is the same — replace buffer.trim() with a completion signal that accounts for empty-buffer entries, e.g. meta?.name && meta?.id && depth === 0.
— qwen3.7-max via Qwen Code /review
| parser.addChunk(0, ' ', 'call_1', 'function1'); | ||
|
|
||
| const completed = parser.getCompletedToolCalls(); | ||
| expect(completed).toHaveLength(1); |
There was a problem hiding this comment.
[Suggestion] This test uses weaker assertions than the empty-buffer test above (line 398). The empty-buffer test uses expect(completed).toEqual([{ id: 'call_1', name: 'function1', args: {}, index: 0 }]) (full shape), while this one only checks toHaveLength(1) and completed[0].args. It would not catch metadata corruption or wrong-index routing.
| expect(completed).toHaveLength(1); | |
| it('should return empty args for whitespace-only argument buffers', () => { | |
| parser.addChunk(0, ' ', 'call_1', 'function1'); | |
| const completed = parser.getCompletedToolCalls(); | |
| expect(completed).toEqual([ | |
| { id: 'call_1', name: 'function1', args: {}, index: 0 }, | |
| ]); | |
| }); |
— qwen3.7-max via Qwen Code /review
✅ Maintainer verification — local build + real-binary E2EI built this PR locally and verified it end-to-end with a real binary. The fix resolves the reported failure and is protected by load-bearing regression tests. LGTM. Scope reviewed: head Why the old behavior broke a whole turn: when the dropped call was the turn's only content, 1. Real-binary end-to-end (the actual user-facing symptom)Two bundles built from identical source except that one guard line, running the real
Every request had 2. Code-level regression guard (mutation A/B)At head all three new/rewritten tests pass. Reverting only the guard back to
So the tests genuinely pin the fix — a future regression would be caught. Minor nit (non-blocking)The JSDoc above Verdict✅ Correct, narrowly scoped, matches the non-streaming path, and regression-protected. No behavior change for tool calls that stream real arguments, nor for name-less / truncated calls ( Verification: worktree at PR head, 🇨🇳 中文说明（完整对应）✅ 维护者验证 —— 本地构建 + 真实二进制端到端我在本地构建了该 PR 并用真实二进制做了端到端验证。修复确实解决了所报告的故障，且有承重的回归测试保护。建议合并（LGTM）。 审阅范围： head 为什么旧行为会毁掉一整轮： 当被丢弃的调用是这一轮唯一的内容时， 1. 真实二进制端到端（真正的用户可见症状）用除那一行判定外完全相同的源码构建了两个 bundle，让真实的 （见上方英文处第一张截图）
每个请求都是 2. 代码层回归保护（变异 A/B）在 head 上三个新增/改写的测试全部通过。只把那行判定改回 （见上方英文处第二张截图） 说明这些测试真正"钉住"了修复——将来若回归会被抓到。 小瑕疵（不阻塞）
结论✅ 修复正确、范围收敛、与非流式路径对齐、且有回归保护。对流式返回真实参数的工具调用、以及无名称/被截断的调用均无行为变化（ 验证方式：在 PR head 建 worktree， |
|
Hi @tomsen-ai — thanks for picking this up. The bug is real, the fix direction is right, and the regression tests look good. Before merging, could you take another pass at @doudouOUC's review comment above? The concern is that this PR relaxes the completion gate in
This is dormant on OpenAI / Anthropic / DashScope (unique index per tool call), but the PR description explicitly calls out llama.cpp-style servers — exactly the class of provider most likely to reuse indices. Suggested fix (from the review): switch the occupancy signal from Happy to take this over if you're busy — just say the word. |
… tool calls Follow-up to review feedback: after empty buffers became a legal end state for no-argument tool calls, three parser methods still used buffer.trim() to decide whether an index slot was occupied. A provider reusing indices could then silently overwrite a completed no-argument call (addChunk collision guard, findNextAvailableIndex) or append stray continuation chunks to it (findMostRecentIncompleteIndex). Switch the occupancy signal in all three places to the name metadata, keeping the JSON-completeness check for non-empty buffers. Add regression tests for both corruption paths and update the stale getCompletedToolCalls JSDoc. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks @wenshao for the thorough verification, and @doudouOUC for catching the occupancy inconsistency — the concern is real, and it's addressed in 0d2b91d. All three spots now use name metadata as the occupancy signal, with two deviations from the literal snippets in the review, both needed for the fix to actually take effect:
One Regression tests added for both corruption paths:
Mutation check: reverting only Also fixed the stale |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] streamingToolCallParser.ts:130 — Missing !meta?.name guard in no-ID continuation routing
The PR correctly updated three of four sites that used !buffer.trim() as the "incomplete call" signal to also check !meta?.name. But the inline continuation branch in addChunk() at line ~130 was missed:
// Line 130 — NOT updated (still uses old guard)
if (existingDepth > 0 || !existingBuffer.trim()) {
actualIndex = index;Compare with the correctly updated findMostRecentIncompleteIndex (line 383):
if (meta?.id && (depth > 0 || (!buffer.trim() && !meta?.name))) {A continuation chunk (no ID) arriving at the index of a completed no-arg call enters the !existingBuffer.trim() branch and is appended to the completed call, corrupting its buffer. The in-progress call that should receive the chunk is starved. Two tool calls corrupted simultaneously.
Fix — mirror the name-guard from findMostRecentIncompleteIndex:
const existingMeta = this.toolCallMeta.get(index);
if (existingDepth > 0 || (!existingBuffer.trim() && !existingMeta?.name)) {
actualIndex = index;Test gap — The existing test "should not route continuation chunks to a completed no-argument tool call" sends the continuation at index 2 (a completed call with non-empty buffer → JSON.parse branch → findMostRecentIncompleteIndex), never exercising this still-broken inline path. A test that would catch this:
parser.addChunk(0, '{"key":', 'call_1', 'function1');
parser.addChunk(1, '', 'call_2', 'no_arg_function');
parser.addChunk(1, '"value"}'); // continuation at no-arg call's OWN index
// Assert: no_arg_function still has args: {}, function1 gets {key: "value"}— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
streamingToolCallParser.ts:95,167,379 |
"Is this slot complete?" check is implemented inline in three locations (collision detection, replay guard, findNextAvailableIndex) with slightly different formulations — the line 130 oversight is a direct consequence of this duplication |
Extract a private isSlotComplete(index: number): boolean helper and call it from all three sites. The helper would return `meta?.name && depth === 0 && (!buffer.trim() |
— 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.
[Suggestion] The replay-detection guard at line 156 (isKnownId && currentBuffer.trim() && currentDepth === 0) uses currentBuffer.trim() as part of its "is this call already complete?" check. For a completed no-argument call (empty buffer, name set), currentBuffer.trim() is falsy, so the guard is skipped. If a provider replays a chunk with a known no-arg call ID, the chunk is appended to the completed call's buffer instead of being discarded.
Before this PR, no-arg calls were dropped by getCompletedToolCalls(), so this code path was never reached for them. Now that they are emitted, a provider sending duplicate opener chunks (e.g., due to a network retry or buggy implementation) could corrupt a completed no-arg call.
Suggested fix: Add a name-metadata check:
if (isKnownId && currentDepth === 0 && (currentBuffer.trim() || this.toolCallMeta.get(actualIndex)?.name)) {This is another instance of the buffer.trim() occupancy-signal inconsistency already noted in the addChunk() continuation routing by @doudouOUC — the replay guard at line 156 is a fourth location where buffer.trim() is used as an occupancy proxy but not updated for the new empty-buffer-is-valid semantics.
— qwen3.7-max via Qwen Code /review
…ical empty-opener shape
Review follow-up on the no-ID continuation routing at addChunk. Mid-stream,
an empty buffer with name metadata is formally undecidable between "completed
no-argument call" and "canonical opener awaiting its first argument fragment"
(every OpenAI-compatible provider opens with arguments:"" and streams
fragments ID-less at the same index). Routing must favor the canonical shape,
so the guard stays; a new test pins that shape, which the suite previously
did not cover.
The corruption concern from review is instead bounded at emit time: a buffer
polluted by a stray fragment can parse or repair to a non-object value, which
now collapses to {} so a polluted no-argument call still emits empty args.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Suggestion] The non-object guard added in getCompletedToolCalls() (~line 313: typeof args !== 'object' || args === null || Array.isArray(args) → args = {}) catches arrays, null, and primitives — but not a valid JSON object. If the continuation-routing ambiguity described in the inline comment above delivers a stray fragment that parses as a valid JSON object (e.g. {"param":"value"}), the no-argument tool call is emitted with wrong arguments. The scenario is narrow (requires the routing ambiguity + a valid JSON object fragment), and the non-object guard catches the more common corruption cases. Consider adding debug logging when emitting empty-buffer tool calls to aid diagnosis if this ever triggers in production.
— qwen3.7-max via Qwen Code /review
| // buffer with a name is a complete no-argument call. | ||
| if ( | ||
| existingBuffer.trim() && | ||
| existingMeta?.name && |
There was a problem hiding this comment.
[Suggestion] This collision path was correctly updated to use existingMeta?.name as the occupancy signal, but the continuation-routing block ~40 lines below (line ~130, if (existingDepth > 0 || !existingBuffer.trim())) still uses !existingBuffer.trim() as the incompleteness check. This creates an inherent ambiguity: a completed no-argument call (empty buffer + name + depth 0) at an index is indistinguishable mid-stream from an opener that received arguments: "" and is still waiting for fragments.
The canonical streaming case (opener with empty args followed by fragments at the same index) is why this path wasn't simply switched to meta?.name — both states have an empty buffer with name metadata. The existing tests pass because they're designed around the canonical case. But if a misbehaving provider sends a stray ID-less fragment at a completed no-arg call's index, the fragment is routed to the completed call instead of findMostRecentIncompleteIndex(). The non-object guard (~line 313) bounds the damage for most corruption paths (strings, arrays, null), but a valid JSON object fragment would pass through undetected.
A proper fix would require tracking explicit state per index (e.g., argumentsStarted: boolean set on the first non-empty argument fragment) to disambiguate "waiting for args" from "complete no-arg call" — but that's a broader change. If that's not worth the scope here, at minimum consider documenting the provider arrival-order assumption this path depends on.
— qwen3.7-max via Qwen Code /review
… argument collapse Review follow-up: a stray fragment that happens to parse as a valid JSON object is indistinguishable from real arguments at emit time, so log both the non-object collapse and empty-buffer emissions to aid diagnosis. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the careful second pass. Before changing anything I ran an empirical A/B on the Critical, and the result is that line ~130 cannot take the name-guard — but the concerns behind both findings are real, and a5db830 + f1efd7f address them at the layer where they are addressable. Evidence below. 1. The suggested edit does not change the routing outcomeWith the suggested condition, an ID-less chunk arriving at a completed no-arg call's index skips the first branch — but then enters the 2. The version that would change routing breaks the canonical streaming shapeTo make the proposed test pass, an empty buffer with name metadata must be treated as complete and routed away via parser.addChunk(0, '', 'call_1', 'function1'); // canonical opener
parser.addChunk(0, '{"x":'); // ID-less fragment — misrouted under route-away
parser.addChunk(0, '1}');Notably, the existing suite never pinned this shape — which I suspect is why route-away looked viable. a5db830 adds the lock test. The underlying issue: at this code point, "completed no-argument call" and "opener awaiting its first fragment" are formally indistinguishable; only stream end resolves them. Routing has to favor the canonical case — the pathological one (a provider reusing a completed call's index for another call's ID-less fragment) requires the provider to already be violating index semantics, at which point fragment ownership is unknowable under any policy. 3. What I did take from the Critical: bounding the damageThe A/B surfaced a real secondary bug in the pathological case: the polluted buffer 4. Replay guard at line ~157Same structure as §1: for an empty buffer, the suggested condition enters the guard, 5. Valid-object pollution (latest suggestion)Agreed — a stray fragment that parses as a valid JSON object is indistinguishable from real arguments at emit time, and logging is the only detector. f1efd7f adds debug logging for both the non-object collapse and empty-buffer emissions. Happy to adjust if there's a routing policy I've missed — but I couldn't find an edit at line ~130 that helps the pathological case without breaking the canonical one, and the canonical one is every provider's every multi-fragment tool call. |
| @@ -275,22 +285,45 @@ export class StreamingToolCallParser { | |||
|
|
|||
There was a problem hiding this comment.
[Critical] The replay-detection guard at line 157 (isKnownId && currentBuffer.trim() && currentDepth === 0) does not protect completed no-argument calls. After this PR, a no-arg tool call has an empty buffer, so currentBuffer.trim() is falsy and the entire guard short-circuits. A subsequent chunk from the provider carrying the same ID — e.g. a replay or duplicate opener — bypasses the guard, overwrites meta.name, and appends to the buffer, corrupting the call.
No existing test covers this scenario (the two replay-guard tests at lines ~629 and ~656 both use calls with non-empty argument buffers).
Extend the replay guard to treat an empty buffer with name metadata as complete:
| if (isKnownId && currentDepth === 0 && (currentBuffer.trim() || this.toolCallMeta.get(actualIndex)?.name)) { |
Inside the guard, the JSON.parse check also needs a no-arg fast path:
if (isKnownId && currentDepth === 0 && (currentBuffer.trim() || this.toolCallMeta.get(actualIndex)?.name)) {
let isComplete = !currentBuffer.trim(); // empty buffer + name = complete no-arg call
if (!isComplete) {
try {
JSON.parse(currentBuffer);
isComplete = true;
} catch {
// Not complete yet; append the incoming chunk below.
}
}
if (isComplete) {
debugLogger.debug(
`Ignoring replay chunk for completed toolCall id=${id}`,
);
return { complete: false };
}
}Add a regression test:
it('should ignore replay chunks for a completed no-argument tool call', () => {
parser.addChunk(0, '', 'call_1', 'list_sessions');
// Provider replays the same ID with a different name
parser.addChunk(0, '', 'call_1', 'different_function');
const completed = parser.getCompletedToolCalls();
expect(completed).toHaveLength(1);
expect(completed[0].name).toBe('list_sessions');
});— qwen3.7-max via Qwen Code /review
…t tool calls Review follow-up: after empty buffers became a legal completed state, a replayed opener (duplicate ID, QwenLM#5107 lineage) could overwrite a completed no-argument call's name metadata, since the replay guard only engaged on non-empty buffers. Swallowing every known-ID chunk at that state would drop ID-bearing argument fragments for providers whose opener streams empty arguments, so the guard uses the protocol shape as discriminator: a chunk carrying a name but no argument content is an opener replay and is ignored; a chunk with argument content is a continuation and appends. Regression tests cover both directions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Good catch this round — the invariant is right and I've adopted it, with one scope adjustment grounded in the delta protocol. Details: The finding is real. After empty buffers became a legal completed state, a replayed opener (duplicate ID — the #5107 pathology this guard was built for) could indeed overwrite a completed no-argument call's But the proposed swallow drops legitimate fragments. I applied the suggested implementation verbatim: it passes its own regression test and the existing suite — and silently swallows ID-bearing argument fragments after an empty opener: parser.addChunk(0, '', 'call_1', 'function1'); // canonical empty opener
parser.addChunk(0, '{"x":1}', 'call_1'); // known-ID continuation — swallowed, args lostProviders that repeat the tool-call ID on argument fragments exist, and under the swallow their with-args calls execute with The protocol itself offers the discriminator. An opener-shaped replay carries a Residual ambiguity, stated for the record: a known-ID chunk carrying both a name and argument content is indistinguishable between a name-repeating provider's fragment and a content-carrying duplicate opener. The guard appends — consistent with its original design, which also appends to unparseable buffers — and the emit-time non-object collapse plus the debug logging from f1efd7f bound and surface the residual case. |
| if ( | ||
| typeof args !== 'object' || | ||
| args === null || | ||
| Array.isArray(args) |
There was a problem hiding this comment.
[Suggestion] The non-object collapse guard has three branches (typeof !== 'object', === null, Array.isArray), but only the string-value branch is exercised by the existing "polluted buffer" test ('"value"}' repairs to a bare string). The null and array paths are untested — if a future change accidentally drops either check, no test would catch it.
| Array.isArray(args) | |
| // Test: parser.addChunk(0, 'null', 'call_1', 'fn'); | |
| // expect(completed[0].args).toEqual({}); | |
| // Test: parser.addChunk(0, '[1,2,3]', 'call_1', 'fn'); | |
| // expect(completed[0].args).toEqual({}); | |
| Array.isArray(args) |
— qwen3.7-max via Qwen Code /review
| // A non-empty buffer must parse as complete JSON for the slot to be | ||
| // occupied; an empty buffer is already complete (no-argument call) | ||
| if (buffer.trim()) { | ||
| try { |
There was a problem hiding this comment.
[Suggestion] The scanning loop's empty-buffer skip path (lines 384–393) is never tested with a scan that must advance past an occupied empty-buffer slot. Existing tests only trigger findNextAvailableIndex via a single collision where nextAvailableIndex points to an unoccupied slot, so the loop body never iterates. A test that stacks multiple no-arg calls would exercise the new code path:
it('should scan past occupied no-argument slots when relocating', () => {
parser.addChunk(0, '', 'call_a', 'no_arg_a');
parser.addChunk(1, '', 'call_b', 'no_arg_b');
parser.addChunk(0, '{"x": 1}', 'call_c', 'fn_c');
const completed = parser.getCompletedToolCalls();
expect(completed).toEqual([
{ id: 'call_a', name: 'no_arg_a', args: {}, index: 0 },
{ id: 'call_b', name: 'no_arg_b', args: {}, index: 1 },
{ id: 'call_c', name: 'fn_c', args: { x: 1 }, index: 2 },
]);
});— qwen3.7-max via Qwen Code /review
…ion scan Review follow-up: pin the null and array branches of the emit-time non-object collapse, and exercise findNextAvailableIndex scanning past multiple occupied no-argument slots during collision relocation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both coverage gaps addressed in 852e763: the null and array branches of the emit-time collapse are now pinned, and the relocation test stacks two no-argument calls so |
|
@wenshao All review rounds are now addressed — 096eea5 adopts the replay-guard invariant with the protocol-scoped implementation (opener-shaped replays ignored, ID-bearing argument continuations preserved), and 852e763 closes the remaining coverage gaps. 70/70 parser + 146/146 converter tests green at head. Ready for your re-review whenever convenient. |
| // Check if this tool call is incomplete. An empty buffer counts only | ||
| // while no name has arrived: with name metadata it is a complete | ||
| // no-argument call, and stray chunks must not be appended to it | ||
| if (meta?.id && (depth > 0 || (!buffer.trim() && !meta?.name))) { |
There was a problem hiding this comment.
[Critical] Same !meta?.name guard applied here but missed at line 130 (no-ID continuation fast-path)
The PR correctly updated three of four sites that used !buffer.trim() as the "incomplete call" signal to also check !meta?.name — here, findNextAvailableIndex (~line 379), and the replay guard (~line 167). But the no-ID continuation fast-path at line 130 was not updated:
// Line 130 (unchanged by this PR, not in diff):
if (existingDepth > 0 || !existingBuffer.trim()) {
actualIndex = index;A completed no-argument call has depth=0, buffer="", so !existingBuffer.trim() short-circuits to actualIndex = index. A stray no-ID continuation chunk arriving at that index gets appended to the completed call instead of being routed through findMostRecentIncompleteIndex. The non-object collapse guard in getCompletedToolCalls catches non-object pollution, but a stray chunk forming a valid JSON object (e.g. {"x":1}) passes through undetected — emitting wrong args for the no-arg call while the intended target call stalls with truncated args.
The canonical-shape test (should route ID-less argument fragments to a call whose opener streamed empty arguments) still passes with this fix because findMostRecentIncompleteIndex correctly handles the routing.
Suggested fix (at line 130):
const existingMeta = this.toolCallMeta.get(index);
if (existingDepth > 0 || (!existingBuffer.trim() && !existingMeta?.name)) {
actualIndex = index;— 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.
No review findings. Downgraded from Approve to Comment: CI still running.
The fix correctly aligns the streaming parser's emission guard with the non-streaming path — no-argument tool calls with empty buffers now survive as args: {} instead of being silently dropped. The defensive guards (non-object collapse, replay detection for opener-shaped replays) and the comprehensive regression tests are well-structured. LGTM.
— qwen3.7-max via Qwen Code /review
4 participants
What this PR does
Makes the streaming tool-call parser emit tool calls whose arguments buffer is empty at stream end (as
args: {}) instead of silently dropping them, matching the non-streaming conversion path. Rewrites the unit test that encoded the old drop behavior, and adds regression tests at parser level and converter chunk level.Why it's needed
For tools that take no parameters, some OpenAI-compatible providers stream
arguments: ""(or omit the field entirely) and never send an argument fragment — llama.cpp's server, for example, omitsargumentswhen empty and never synthesizes"{}". Today the streaming parser drops such calls wholesale (if (meta?.name && buffer.trim())), while the non-streaming path keeps them withargs: {}. When the turn contains only that tool call, the whole turn looks empty and the client raisesInvalidStreamError: Model stream ended with empty response text., triggering pointless retries. Qwen Code's own built-in no-argument tools (task_list,cron_list,enter_plan_mode) are in the blast radius.Fixes #6249
Reviewer Test Plan
How to verify
cd packages/core && npx vitest run src/core/openaiContentGenerator/— includes the new regression tests: the parser emits an empty-args call (the old test asserted the drop), and the converter chunk path delivers afunctionCallwithargs: {}.main, the new parser testshould return no-argument tool calls with empty args when buffer is emptyfails withexpected [] to have a length of 1; on this branch it passes. Fullnpm run preflightis green.{name: "task_list", arguments: ""}+finish_reason: "tool_calls"(script shared in Streaming tool calls with an emptyargumentsstring are silently dropped, causing "Model stream ended with empty response text" retry loops #6249). Before: API error + identical retry requests. After: the tool call surfaces and executes, the conversation completes.Evidence (Before & After)
Same mock server (streams a no-arg
task_listcall, llama.cpp-style), same prompt:Before (v0.19.5):
After (this branch):
Tested on
Risk & Scope
Low risk, narrow scope: one guard relaxed in
streamingToolCallParser.ts(production code), everything else is tests. Behavior changes only for tool calls whose arguments buffer is empty at stream end — previously dropped (then surfacing as a fake "empty response" error with retries), now emitted withargs: {}like the non-streaming path. Tool calls with a missing/unknown name are still filtered out, and truncation detection (hasIncompleteToolCalls) is unaffected since an empty buffer has depth 0. No API/config surface changes.Linked Issues
argumentsstring are silently dropped, causing "Model stream ended with empty response text" retry loops #6249中文说明
本 PR 做了什么:流式工具调用解析器在收流结束、参数缓冲为空时,不再整体丢弃该工具调用,而是以
args: {}正常发出,与非流式转换路径(converter.ts)行为对齐。同时把原先固化"丢弃"行为的单测改写为新语义,并在 parser 层和 converter chunk 层补充回归测试。为什么需要:对无参数工具,部分 OpenAI 兼容服务端会以
arguments: ""收尾且不再发送任何参数分片(如 llama.cpp 在参数为空时会完全省略该字段)。此时整轮回复看起来既无文本也无工具调用,客户端抛出Model stream ended with empty response text并无意义地自动重试。Qwen Code 内置的无参工具(task_list、cron_list、enter_plan_mode)均在影响范围内。风险与范围:低风险、窄范围——生产代码仅放宽一个判定条件;无名称的工具调用仍会被过滤;截断检测不受影响;无 API/配置面变化。