diff --git a/haystack/human_in_the_loop/strategies.py b/haystack/human_in_the_loop/strategies.py index 7800fac66d2..03ecd134260 100644 --- a/haystack/human_in_the_loop/strategies.py +++ b/haystack/human_in_the_loop/strategies.py @@ -544,8 +544,18 @@ def _apply_tool_execution_decisions( decision_by_name = {d.tool_name: d for d in tool_execution_decisions if d.tool_name} # Known limitation: If tool calls are missing IDs, we rely on tool names to match decisions to tool calls. - # This can lead to incorrect matches if there are multiple tool calls in the provided messages with duplicate names. - if not decision_by_id and len(decision_by_name) < len(tool_execution_decisions): + # This can lead to incorrect matches if there are multiple tool calls in the provided messages with duplicate + # names. `decision_by_name` is built from *all* decisions, so a name collision corrupts the name-based lookup + # for an id-less decision even if the colliding decision has an id (it's still the id-less one that depends on + # the now-ambiguous `decision_by_name` entry). So we check name duplicates across the whole batch, but only + # raise if at least one decision sharing that name is missing a tool_call_id. + name_occurrences: dict[str, int] = {} + for decision in tool_execution_decisions: + name_occurrences[decision.tool_name] = name_occurrences.get(decision.tool_name, 0) + 1 + + if any( + name_occurrences[decision.tool_name] > 1 for decision in tool_execution_decisions if not decision.tool_call_id + ): raise ValueError( "ToolExecutionDecisions are missing tool_call_id fields and there are multiple tool calls with the same " "name. When multiple tool calls with the same name are present, tool_call_id is required to correctly " diff --git a/releasenotes/notes/fix-hitl-duplicate-name-decision-clobbering-ee2b956add710394.yaml b/releasenotes/notes/fix-hitl-duplicate-name-decision-clobbering-ee2b956add710394.yaml new file mode 100644 index 00000000000..8453ea41a6a --- /dev/null +++ b/releasenotes/notes/fix-hitl-duplicate-name-decision-clobbering-ee2b956add710394.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + Fixed ``human_in_the_loop``'s ``_apply_tool_execution_decisions`` silently applying the wrong + ``ToolExecutionDecision`` to a tool call when multiple tool calls shared the same tool name but lacked a + ``tool_call_id``. Previously, the ambiguity check was skipped if *any* unrelated tool call in the same batch + happened to carry an id, allowing the wrong decision (and therefore the wrong arguments) to be silently + applied. The check now correctly raises a ``ValueError`` whenever a tool name collision affects an id-less + decision, regardless of other unrelated tool calls in the batch. diff --git a/test/human_in_the_loop/test_strategies.py b/test/human_in_the_loop/test_strategies.py index 4fc5feca1ea..e8f0ca3dc59 100644 --- a/test/human_in_the_loop/test_strategies.py +++ b/test/human_in_the_loop/test_strategies.py @@ -357,6 +357,77 @@ def test_two_teds_same_name_no_ids(self): ], ) + def test_two_teds_same_name_no_ids_plus_unrelated_call_with_id(self): + # Same setup as test_two_teds_same_name_no_ids, but with an extra, unrelated tool call that *does* have an + # id. The presence of that unrelated id must not bypass the ambiguity check for the id-less duplicates. + message_with_tool_calls = ChatMessage.from_assistant( + tool_calls=[ + ToolCall(tool_name="add_database_tool", arguments={"name": "Malte"}), + ToolCall(tool_name="add_database_tool", arguments={"name": "Milos"}), + ToolCall(tool_name="other_tool", arguments={}, id="xyz"), + ] + ) + with pytest.raises( + ValueError, + match="ToolExecutionDecisions are missing tool_call_id fields and there are multiple tool calls with the " + "same name", + ): + _apply_tool_execution_decisions( + tool_call_messages=[message_with_tool_calls], + tool_execution_decisions=[ + ToolExecutionDecision( + tool_name="add_database_tool", execute=True, final_tool_params={"name": "Malte"} + ), + ToolExecutionDecision( + tool_name="add_database_tool", execute=True, final_tool_params={"name": "Milos"} + ), + ToolExecutionDecision( + tool_name="other_tool", execute=True, tool_call_id="xyz", final_tool_params={} + ), + ], + ) + + def test_two_teds_same_name_one_with_id_one_without(self): + # One of the two same-named tool calls has an id, the other doesn't. The id-less one still falls back to + # name-based matching against `decision_by_name`, which is corrupted by the name collision, so this must + # also be treated as ambiguous even though only one of the two decisions lacks an id. + message_with_tool_calls = ChatMessage.from_assistant( + tool_calls=[ + ToolCall(tool_name="search", arguments={"q": "a"}), + ToolCall(tool_name="search", arguments={"q": "b"}, id="1"), + ] + ) + with pytest.raises( + ValueError, + match="ToolExecutionDecisions are missing tool_call_id fields and there are multiple tool calls with the " + "same name", + ): + _apply_tool_execution_decisions( + tool_call_messages=[message_with_tool_calls], + tool_execution_decisions=[ + ToolExecutionDecision(tool_name="search", execute=True, final_tool_params={"q": "a"}), + ToolExecutionDecision( + tool_name="search", execute=True, tool_call_id="1", final_tool_params={"q": "b"} + ), + ], + ) + + def test_ted_without_id_unique_name_applies_normally(self): + # An id-less decision whose tool name is unique is not ambiguous, so the guard must not raise: name-based + # matching is unambiguous here. The decision should be applied normally. + message_with_tool_calls = ChatMessage.from_assistant( + tool_calls=[ToolCall(tool_name="search", arguments={"q": "a"})] + ) + rejection_messages, new_tool_call_messages = _apply_tool_execution_decisions( + tool_call_messages=[message_with_tool_calls], + tool_execution_decisions=[ + ToolExecutionDecision(tool_name="search", execute=True, final_tool_params={"q": "a"}) + ], + ) + assert rejection_messages == [] + assert len(new_tool_call_messages) == 1 + assert new_tool_call_messages[0].tool_calls == [ToolCall(tool_name="search", arguments={"q": "a"})] + class TestUpdateChatHistory: @pytest.fixture