fix: stop silently clobbering tool execution decisions on name collisions#11758
Open
immuhammadfurqan wants to merge 2 commits into
Open
fix: stop silently clobbering tool execution decisions on name collisions#11758immuhammadfurqan wants to merge 2 commits into
immuhammadfurqan wants to merge 2 commits into
Conversation
…ions The ambiguity guard in _apply_tool_execution_decisions only checked whether *every* decision in the batch lacked a tool_call_id. If any unrelated tool call in the same batch had an id, the guard was skipped entirely, even though id-less duplicate-named calls were still present. The matching fallback (decision_by_name) would then silently resolve to the wrong decision, applying the wrong arguments to the wrong tool call with no error. The guard now checks whether any id-less decision's tool name collides with another decision anywhere in the batch, regardless of whether the colliding decision has an id, and raises ValueError in that case. Fixes deepset-ai#11756
immuhammadfurqan
requested review from
davidsbatista
and removed request for
a team
|
@immuhammadfurqan is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
|
|
Contributor
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issues
Proposed Changes:
_apply_tool_execution_decisionsis supposed to raise aValueErrorwhen it can't unambiguously match aToolExecutionDecisionto a tool call (multiple tool calls share a name and lack atool_call_id). The guard only checkednot decision_by_idacross the entire batch, so if any unrelated tool call elsewhere in the same batch happened to carry an id, the guard was skipped — even though the duplicate-named, id-less calls were still just as ambiguous. Once skipped, the name-based fallback (decision_by_name) silently resolved to whichever decision was last in the dict, applying the wrong arguments to the wrong tool call with no error or warning.This also affects a subtler case: if one of two same-named calls has an id and the other doesn't, the id-less one still falls back to
decision_by_name, which is corrupted by the collision regardless of the other call having an id.This PR replaces the guard with a check that counts tool-name occurrences across the whole batch and raises whenever an id-less decision's name collides with another decision's name anywhere in the batch — whether or not that other decision has an id.
How did you test it?
Added two regression tests: one for the originally reported case (duplicate id-less names plus an unrelated id-bearing call), and one for the case where one of the two same-named calls has an id and the other doesn't. Ran the full
human_in_the_looptest suite (30 passed),ruff check/ruff format, andmypy— all clean.Notes for the reviewer
None.
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.