From 3db10d8793a5ec70313f1b8298bef164106620d8 Mon Sep 17 00:00:00 2001 From: iammuhammadfurqan Date: Thu, 25 Jun 2026 00:49:42 +0500 Subject: [PATCH 1/3] fix: stop silently clobbering tool execution decisions on name collisions 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 #11756 --- haystack/human_in_the_loop/strategies.py | 14 ++++- ...-decision-clobbering-ee2b956add710394.yaml | 9 +++ test/human_in_the_loop/test_strategies.py | 55 +++++++++++++++++++ 3 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/fix-hitl-duplicate-name-decision-clobbering-ee2b956add710394.yaml 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..0bb991dba1f --- /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..e25a6f41e7a 100644 --- a/test/human_in_the_loop/test_strategies.py +++ b/test/human_in_the_loop/test_strategies.py @@ -357,6 +357,61 @@ 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"} + ), + ], + ) + class TestUpdateChatHistory: @pytest.fixture From 481ba7b77c7dea513b5683ea0a7d0569afc44d41 Mon Sep 17 00:00:00 2001 From: iammuhammadfurqan Date: Thu, 25 Jun 2026 16:10:13 +0500 Subject: [PATCH 2/3] docs: use RST double backticks in release note --- ...plicate-name-decision-clobbering-ee2b956add710394.yaml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/releasenotes/notes/fix-hitl-duplicate-name-decision-clobbering-ee2b956add710394.yaml b/releasenotes/notes/fix-hitl-duplicate-name-decision-clobbering-ee2b956add710394.yaml index 0bb991dba1f..8453ea41a6a 100644 --- a/releasenotes/notes/fix-hitl-duplicate-name-decision-clobbering-ee2b956add710394.yaml +++ b/releasenotes/notes/fix-hitl-duplicate-name-decision-clobbering-ee2b956add710394.yaml @@ -1,9 +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 + 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 + 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. From f33c983314abd3e3b140455e68381c2abe3c36c5 Mon Sep 17 00:00:00 2001 From: iammuhammadfurqan Date: Tue, 30 Jun 2026 15:43:07 +0500 Subject: [PATCH 3/3] test: cover non-ambiguous id-less decision path in _apply_tool_execution_decisions Adds a regression test where a decision without a tool_call_id has a unique tool name: the ambiguity guard must not raise and the decision should be applied normally. This exercises the guard's non-raising branch, which the two existing same-name tests (both asserting the raise) did not cover. --- test/human_in_the_loop/test_strategies.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/human_in_the_loop/test_strategies.py b/test/human_in_the_loop/test_strategies.py index e25a6f41e7a..e8f0ca3dc59 100644 --- a/test/human_in_the_loop/test_strategies.py +++ b/test/human_in_the_loop/test_strategies.py @@ -412,6 +412,22 @@ def test_two_teds_same_name_one_with_id_one_without(self): ], ) + 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