studio: route bare tool-call fragments to the call that owns the index now - #8755
Conversation
…x now
`_Turn.merge_structured` already forks a new slot when a second call arrives
with its own id at an index another call opened, so the two argument streams
do not run together. The fork keyed the new call on `(index, call_id)` while
`by_index[index]` still pointed at the first one, and a continuation fragment
carries no id, so `key = index` sent every bare fragment back to the call that
had been superseded.
Providers restart `delta.tool_calls[].index` at 0 for each tool round inside one
response, so this fires whenever a model calls a tool a second time and the
server streams the arguments in fragments, which llama.cpp and vLLM both do.
Round two's tail landed on round one, leaving `{"query":"first"}"second"}` for
the first call and a truncated `{"query":` for the second. Both then executed on
those arguments, and `_normalized_call` stored the unparseable text under
`_raw`, which is what reaches the client and the replayed conversation.
`open_key_by_index` records the slot each index currently points at, so bare
fragments follow the fork. The id-conflict check reads that slot too, since
`by_index[index]` is no longer the open call after one.
for more information, see https://pre-commit.ci
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48a57524ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Thanks, the external-provider fix itself looks correct, but the same reachable defect remains in the built-in GGUF path.
I reproduced this without a model by feeding the real GGUF loop the same four-delta sequence covered by this PR's new test:
The GGUF loop assembled the tool name as Could you please apply the same current-owner mapping to the built-in GGUF accumulator and add focused coverage for both bare continuations and fragments that repeat their call ID? This should remain a localized change, but it closes the sibling path instead of leaving Studio's built-in GGUF backend with the same bug. |
The GGUF loop in LlamaCppBackend.generate_chat_completion_with_tools kept tool_calls_acc keyed on delta.tool_calls[].index alone. llama-server restarts that index at 0 for every tool round, so a second call arriving at an index another call opened overwrote the first call's id and appended its own name and arguments to it. Two web_search rounds assembled into one web_searchweb_search entry that matched no enabled tool, and neither call ran. tool_calls_open_key records the accumulator slot each index points at, so a second call forks its own slot and bare continuation fragments follow the call that owns the index now. A fragment repeating an id goes back to the call that id opened. Final assembly sorts through _structured_call_order_key because the fork keys are (index, id) tuples. Same fix as the external-provider path in _Turn.merge_structured. Covered by test_second_structured_call_at_one_index_keeps_its_own_fragments and test_structured_fragment_naming_its_call_goes_back_to_that_call, both failing before this change with the concatenated tool name.
|
@oobabooga Confirmed and fixed in c1152e3.
Same shape as the Your four-delta sequence is now 276 tests pass across the llama.cpp tool loop, studio tool loop, GGUF non-streaming, external tool edge case, external tools compat and tool confirm/stream suites. Ruff is clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1152e339f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Two follow-ups to the index-collision fix in the GGUF structured accumulator. A call opened with no id sits on the synthetic call_<index> placeholder, and the fork check read that placeholder as a rival id: the real id arriving on a later delta forked a nameless slot, which final assembly dropped, while the original call ran on truncated arguments. tool_calls_synthetic_ids marks the slots still on a placeholder so a first real id updates the open call instead, restoring the later-id path. Assembly now walks tool_calls_acc in first-seen order. Sorting fork keys next to the index they reused put a second-round call at index 0 ahead of a first-round call at index 1, reordering side effects for stateful tools. Covered by test_structured_call_id_arriving_after_the_opening_delta_updates_ that_call and test_structured_call_forked_onto_a_reused_index_executes_last.
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
2 participants
Found while working on #8734. Same defect class, backend side, and independently reachable.
The bug
_Turn.merge_structuredalready forks a new slot when a second call arrives with its own id at an index another call opened, so the two argument streams do not run together. The fork keyed the new call on(index, call_id)whileby_index[index]still pointed at the first one. A continuation fragment carries no id, sokey = indexsent every bare fragment back to the call that had been superseded.Providers restart
delta.tool_calls[].indexat 0 for each tool round inside one response, so this fires whenever a model calls a tool a second time and the server streams the arguments in fragments, which llama.cpp and vLLM both do. Round two's tail landed on round one:Both tools then executed on those arguments, and
_normalized_callstored the unparseable text under_raw, which is what reaches the client and the replayed conversation.The fix
open_key_by_indexrecords the slot each index currently points at, so bare fragments follow the fork. The id-conflict check reads that slot too, sinceby_index[index]is no longer the open call after one.Testing
Manually tested and verified against a real Studio stack driving an OpenAI-compatible server that streams two rounds at index 0 with the arguments split across bare continuation fragments. Before the fix the executed calls receive
{"query": "{\"query\":\"first\"}\"second\"}"}and{"query": "{\"query\":"}; after, they receive{"query": "first"}and{"query": "second"}.test_a_second_call_at_one_index_keeps_its_own_argument_fragmentscovers it and fails onmainwith exactly that concatenation. The tool loop, external tool edge case, llama.cpp tool loop and external tools compat suites all pass (202 tests), and ruff is clean.