-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(core): route id-less continuation chunks to a colliding tool-call opener's slot #6981
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d02834c
62bce69
001a710
ef62d64
6b62711
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -49,7 +49,12 @@ export class StreamingToolCallParser { | |||||||||||||||||||||
| private namelessToolCallIndices = new Set<number>(); | ||||||||||||||||||||||
| /** Map from tool call ID to actual index used for storage */ | ||||||||||||||||||||||
| private idToIndexMap: Map<string, number> = new Map(); | ||||||||||||||||||||||
| /** Remapped slots awaiting a stable ID from a later chunk. */ | ||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Maps a provider index to the actual slot it was remapped to on collision. | ||||||||||||||||||||||
| * Two readers consume it: an id that arrives after its name/args adopts the | ||||||||||||||||||||||
| * slot, and later id-less continuation chunks at the same provider index are | ||||||||||||||||||||||
| * routed to it. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| private pendingIndexRemaps: Map<number, number> = new Map(); | ||||||||||||||||||||||
| /** Counter for generating new indices when collisions occur */ | ||||||||||||||||||||||
| private nextAvailableIndex: number = 0; | ||||||||||||||||||||||
|
|
@@ -110,10 +115,18 @@ export class StreamingToolCallParser { | |||||||||||||||||||||
| if (this.idToIndexMap.has(id)) { | ||||||||||||||||||||||
| // We've seen this ID before, use the existing mapped index | ||||||||||||||||||||||
| actualIndex = this.idToIndexMap.get(id)!; | ||||||||||||||||||||||
| } else if (this.pendingIndexRemaps.has(index)) { | ||||||||||||||||||||||
| // Some providers stream name or arguments before the stable ID. | ||||||||||||||||||||||
| } else if ( | ||||||||||||||||||||||
| this.pendingIndexRemaps.has(index) && | ||||||||||||||||||||||
| !this.toolCallMeta.get(this.pendingIndexRemaps.get(index)!)?.id | ||||||||||||||||||||||
| ) { | ||||||||||||||||||||||
| // Some providers stream name or arguments before the stable ID. Only | ||||||||||||||||||||||
| // adopt the remapped slot while it has not yet been claimed by an id: | ||||||||||||||||||||||
| // 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. | ||||||||||||||||||||||
| // The remap is deliberately left in place — the re-registration on the | ||||||||||||||||||||||
| // common path below keeps `index -> actualIndex` alive so subsequent | ||||||||||||||||||||||
| // id-less continuation chunks at this provider index still resolve here. | ||||||||||||||||||||||
| actualIndex = this.pendingIndexRemaps.get(index)!; | ||||||||||||||||||||||
| this.pendingIndexRemaps.delete(index); | ||||||||||||||||||||||
| this.idToIndexMap.set(id, actualIndex); | ||||||||||||||||||||||
| } else { | ||||||||||||||||||||||
| // New tool call ID | ||||||||||||||||||||||
|
|
@@ -206,7 +219,7 @@ export class StreamingToolCallParser { | |||||||||||||||||||||
| } else { | ||||||||||||||||||||||
| this.namelessToolCallIndices.delete(actualIndex); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if (!meta.id && actualIndex !== index) { | ||||||||||||||||||||||
| if (actualIndex !== index) { | ||||||||||||||||||||||
| this.pendingIndexRemaps.set(index, actualIndex); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return { actualIndex, complete: false }; | ||||||||||||||||||||||
|
|
@@ -236,7 +249,7 @@ export class StreamingToolCallParser { | |||||||||||||||||||||
| meta.name = validName; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if (!meta.id && actualIndex !== index) { | ||||||||||||||||||||||
| if (actualIndex !== index) { | ||||||||||||||||||||||
| this.pendingIndexRemaps.set(index, actualIndex); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
249
to
254
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The common-path remap recording unconditionally overwrites an existing 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
Suggested change
-- qwen3.7-max via Qwen Code /review
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks — I looked at this closely and verified it empirically, and I'm going to keep the unconditional overwrite. The suggested guard ( I added a regression test for exactly the well-formed three-call case (6b62711): |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The new test covers the removed
!meta.idguard (remap recording) but does not exercise the added guard!this.toolCallMeta.get(...)?.idon pending-remap adoption. In this test, step 1 (call_1at index 0) never creates a remap becauseactualIndex === index, so the guardedelse ifbranch is never reached. A thirdid-bearing call at the same provider index would be needed to hit that branch.Failure scenario: If the
!meta.idguard 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.— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: aftercall_2claims the 0->1 remap with its own id, a third call reusing index 0 with a fresh id assertsthird.actualIndex !== 1, which fails if the!toolCallMeta.get(remap)?.idguard is removed. This first test intentionally covers the record path; the third covers the adopt-guard path.