diff --git a/haystack/hooks/human_in_the_loop/strategies.py b/haystack/hooks/human_in_the_loop/strategies.py index 3e1ccbca4f..a324387b69 100644 --- a/haystack/hooks/human_in_the_loop/strategies.py +++ b/haystack/hooks/human_in_the_loop/strategies.py @@ -238,19 +238,9 @@ def _passthrough_tool_call(tool_call: ToolCall) -> ToolExecutionDecision: ) -def _with_tool_call_id(decision: ToolExecutionDecision, tool_call: ToolCall) -> ToolExecutionDecision: - """ - Stamp the ID of the tool call a decision was made about onto that decision, if the strategy did not pass it back. - - A custom `ConfirmationStrategy` receives `tool_call_id` but is free to return a decision without it, and a decision - that carries no ID can only be matched back to its tool call by tool name. Restoring the ID here keeps that - name-based fallback for tool calls that genuinely have no ID. - - :param decision: The decision a confirmation strategy returned. - :param tool_call: The tool call the strategy was asked about. - :returns: The decision, with the tool call's ID set if it had none. - """ - if tool_call.id is None or decision.tool_call_id is not None: +def _bind_decision_to_tool_call(decision: ToolExecutionDecision, tool_call: ToolCall) -> ToolExecutionDecision: + """Bind a confirmation decision to the tool call being processed by its tool_call_id.""" + if decision.tool_call_id == tool_call.id: return decision return replace(decision, tool_call_id=tool_call.id) @@ -404,7 +394,7 @@ def _run_confirmation_strategies( tool_call_id=tool_call.id, confirmation_strategy_context=confirmation_strategy_context, ) - teds.append(_with_tool_call_id(ted, tool_call)) + teds.append(_bind_decision_to_tool_call(decision=ted, tool_call=tool_call)) return teds @@ -474,7 +464,7 @@ async def _run_confirmation_strategies_async( tool_call_id=tool_call.id, confirmation_strategy_context=confirmation_strategy_context, ) - teds.append(_with_tool_call_id(ted, tool_call)) + teds.append(_bind_decision_to_tool_call(decision=ted, tool_call=tool_call)) return teds @@ -494,20 +484,20 @@ def _apply_tool_execution_decisions( - A list of tool call messages for confirmed or modified tool calls. If tool parameters were modified, a user message explaining the modification is included before the tool call message. """ + tool_calls = [tc for message in tool_call_messages for tc in (message.tool_calls or [])] + if len(tool_calls) != len(tool_execution_decisions): + raise ValueError( + f"Expected one ToolExecutionDecision for each tool call, but received {len(tool_execution_decisions)} " + f"decisions for {len(tool_calls)} tool calls." + ) + + # Create lookup for decisions that have a tool_call_id decision_by_id = {d.tool_call_id: d for d in tool_execution_decisions if d.tool_call_id} - # 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. - # Decisions that carry an ID stay out of the name lookup, so they cannot be handed to a different tool call. + + # Create a lookup for decisions that don't have a tool_call_id. We use tool name instead. decisions_without_id = [d for d in tool_execution_decisions if not d.tool_call_id] decision_by_name = {d.tool_name: d for d in decisions_without_id if d.tool_name} - if len(decision_by_name) < len(decisions_without_id): - raise ValueError( - "ToolExecutionDecisions are missing tool_call_id fields and cannot be matched by tool name. A decision " - "without a tool_call_id is matched to its tool call by name, so those names have to be non-empty and " - "unique. Set tool_call_id to match decisions to tool calls reliably." - ) - def make_assistant_message(chat_message: ChatMessage, tool_calls: list[ToolCall]) -> ChatMessage: return ChatMessage.from_assistant( text=chat_message.text, @@ -523,10 +513,14 @@ def _apply_tool_execution_decisions( for chat_msg in tool_call_messages: new_tool_calls = [] for tc in chat_msg.tool_calls or []: - ted = decision_by_id.get(tc.id or "") or decision_by_name.get(tc.tool_name) - if not ted: - # This shouldn't happen, if so something went wrong in _run_confirmation_strategies - continue + ted = decision_by_id.pop(tc.id or "", None) + if ted is None: + ted = decision_by_name.pop(tc.tool_name, None) + if ted is None: + raise ValueError( + f"No unused ToolExecutionDecision matches tool call {tc.tool_name!r} with ID {tc.id!r}. " + "Set tool_call_id to match decisions to tool calls reliably." + ) if not ted.execute: # rejected tool call diff --git a/releasenotes/notes/hitl-mixed-tool-call-id-decisions-a70172176717c38f.yaml b/releasenotes/notes/hitl-mixed-tool-call-id-decisions-a70172176717c38f.yaml index 5ac75ce557..9228d99c1a 100644 --- a/releasenotes/notes/hitl-mixed-tool-call-id-decisions-a70172176717c38f.yaml +++ b/releasenotes/notes/hitl-mixed-tool-call-id-decisions-a70172176717c38f.yaml @@ -2,8 +2,8 @@ fixes: - | Fixed ``ConfirmationHook`` applying a Human-in-the-Loop decision to the wrong tool call when a custom - ``ConfirmationStrategy`` returns decisions without a ``tool_call_id``. The ID of the tool call a strategy was asked - about is now restored on the decision it returns, decisions that carry an ID are no longer matched to a different - tool call by name, and two ID-less decisions for the same tool name raise a ``ValueError`` as documented instead of - both tool calls taking the last decision. The text of that ``ValueError`` changed, since it now also covers an - ID-less decision with an empty tool name. + ``ConfirmationStrategy`` returns a decision with a missing or incorrect ``tool_call_id``. Each decision is now + bound to the tool call for which its strategy ran, and ID-bearing decisions are no longer matched to a different + call by name. Haystack's existing requirement of exactly one decision per tool call is now explicitly enforced. + Each matched decision is consumed after use, so a missing, unused, or reused decision raises a ``ValueError`` + instead of being silently misapplied. diff --git a/test/hooks/human_in_the_loop/test_strategies.py b/test/hooks/human_in_the_loop/test_strategies.py index 7d67edce84..6493d276be 100644 --- a/test/hooks/human_in_the_loop/test_strategies.py +++ b/test/hooks/human_in_the_loop/test_strategies.py @@ -21,6 +21,7 @@ from haystack.hooks.human_in_the_loop import ( ) from haystack.hooks.human_in_the_loop.strategies import ( _apply_tool_execution_decisions, + _bind_decision_to_tool_call, _process_confirmation_strategies, _run_confirmation_strategies, _run_confirmation_strategies_async, @@ -328,110 +329,60 @@ class TestApplyToolExecutionDecisions: ), ] - def test_two_teds_same_name_no_ids(self): - message_with_tool_calls = ChatMessage.from_assistant( - text="I'll extract the information about the people mentioned in the context.", - # Same tool name with different params but missing IDs - tool_calls=[ - ToolCall(tool_name="add_database_tool", arguments={"name": "Malte"}), - ToolCall(tool_name="add_database_tool", arguments={"name": "Milos"}), - ], - ) - # This raises a ValueError because tool_call_id is missing and there are multiple tool calls with the same name - # so we cannot disambiguate which TED applies to which tool call. - with pytest.raises( - ValueError, - match="ToolExecutionDecisions are missing tool_call_id fields and cannot be matched by tool 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"} - ), - ], - ) - def test_two_teds_same_name_no_ids_and_one_with_an_id(self): message_with_tool_calls = ChatMessage.from_assistant( tool_calls=[ - ToolCall(tool_name="get_weather", arguments={"city": "Berlin"}, id="1"), - ToolCall(tool_name="add_database_tool", arguments={"name": "Malte"}), - ToolCall(tool_name="add_database_tool", arguments={"name": "Milos"}), + # Two tool calls with the same name but no IDs + ToolCall("tool", {"value": 1}), + ToolCall("tool", {"value": 2}), + # One tool call with a different name and an ID + ToolCall("other", {}, id="1"), ] ) - # The two decisions without an ID are still ambiguous, even though an unrelated decision in the same batch - # carries one. - with pytest.raises( - ValueError, - match="ToolExecutionDecisions are missing tool_call_id fields and cannot be matched by tool name", - ): + decisions = [ + ToolExecutionDecision("tool", execute=True), + ToolExecutionDecision("tool", execute=False), + ToolExecutionDecision("other", execute=True, tool_call_id="1"), + ] + with pytest.raises(ValueError, match="No unused ToolExecutionDecision matches tool call"): _apply_tool_execution_decisions( - tool_call_messages=[message_with_tool_calls], - tool_execution_decisions=[ - ToolExecutionDecision( - tool_name="get_weather", execute=True, tool_call_id="1", final_tool_params={"city": "Berlin"} - ), - ToolExecutionDecision(tool_name="add_database_tool", execute=False, feedback="Not this one"), - ToolExecutionDecision( - tool_name="add_database_tool", execute=True, final_tool_params={"name": "Milos"} - ), - ], + tool_call_messages=[message_with_tool_calls], tool_execution_decisions=decisions ) def test_ted_with_an_id_is_not_matched_by_name(self): - rejected_tool_call = ToolCall(tool_name="add_database_tool", arguments={"name": "Malte"}) - message_with_tool_calls = ChatMessage.from_assistant( - tool_calls=[ - rejected_tool_call, - ToolCall(tool_name="add_database_tool", arguments={"name": "Milos"}, id="2"), - ] - ) - + # Two tools with the same name, and only one has an ID. + rejected_tool_call = ToolCall("tool", {"value": 1}) + confirmed_tool_call = ToolCall("tool", {"value": 2}, id="2") + message_with_tool_calls = ChatMessage.from_assistant(tool_calls=[rejected_tool_call, confirmed_tool_call]) rejection_messages, new_tool_call_messages = _apply_tool_execution_decisions( tool_call_messages=[message_with_tool_calls], + # Similarly the TEDs match the same level of info as the tool calls so we can disambiguate them. tool_execution_decisions=[ - ToolExecutionDecision(tool_name="add_database_tool", execute=False, feedback="Malte is already in"), - ToolExecutionDecision( - tool_name="add_database_tool", execute=True, tool_call_id="2", final_tool_params={"name": "Milos"} - ), + ToolExecutionDecision("tool", execute=False), + ToolExecutionDecision("tool", execute=True, tool_call_id="2", final_tool_params={"value": 2}), ], ) + assert rejection_messages[0].tool_calls == [rejected_tool_call] + assert new_tool_call_messages[0].tool_calls == [confirmed_tool_call] - assert rejection_messages == [ - ChatMessage.from_assistant(tool_calls=[rejected_tool_call]), - ChatMessage.from_tool(tool_result="Malte is already in", origin=rejected_tool_call, error=True), - ] - assert new_tool_call_messages == [ - ChatMessage.from_assistant( - tool_calls=[ToolCall(tool_name="add_database_tool", arguments={"name": "Milos"}, id="2")] - ) - ] - - def test_ted_with_an_empty_name_and_no_id(self): + def test_decision_with_mismatched_tool_name_raises(self): message_with_tool_calls = ChatMessage.from_assistant( - tool_calls=[ - ToolCall(tool_name="get_weather", arguments={"city": "Berlin"}, id="1"), - ToolCall(tool_name="add_database_tool", arguments={"name": "Malte"}), - ] + tool_calls=[ToolCall("tool", {}), ToolCall("other", {}, id="1")] ) - # Without an ID or a name there is nothing left to match on, so the batch is rejected instead of the decision - # being dropped. - with pytest.raises( - ValueError, - match="ToolExecutionDecisions are missing tool_call_id fields and cannot be matched by tool name", - ): + decisions = [ + ToolExecutionDecision("different_tool", execute=False), + ToolExecutionDecision("other", execute=True, tool_call_id="1"), + ] + with pytest.raises(ValueError, match="No unused ToolExecutionDecision matches tool call"): _apply_tool_execution_decisions( - tool_call_messages=[message_with_tool_calls], - tool_execution_decisions=[ - ToolExecutionDecision( - tool_name="get_weather", execute=True, tool_call_id="1", final_tool_params={"city": "Berlin"} - ), - ToolExecutionDecision(tool_name="", execute=False), - ], + tool_call_messages=[message_with_tool_calls], tool_execution_decisions=decisions + ) + + def test_one_decision_cannot_match_multiple_tool_calls(self): + message = ChatMessage.from_assistant(tool_calls=[ToolCall("tool", {}), ToolCall("tool", {})]) + with pytest.raises(ValueError, match="Expected one ToolExecutionDecision for each tool call"): + _apply_tool_execution_decisions( + tool_call_messages=[message], tool_execution_decisions=[ToolExecutionDecision("tool", execute=True)] ) @@ -823,75 +774,8 @@ class TestAsyncConfirmationStrategies: ] -class IdDroppingConfirmationStrategy: - """A strategy that ignores the tool_call_id it is given, as a custom strategy is free to do.""" - - def run( - self, - tool_name: str, - tool_description: str, - tool_params: dict[str, Any], - tool_call_id: str | None = None, - confirmation_strategy_context: dict[str, Any] | None = None, - ) -> ToolExecutionDecision: - return ToolExecutionDecision(tool_name=tool_name, execute=True, final_tool_params=tool_params) - - async def run_async( - self, - tool_name: str, - tool_description: str, - tool_params: dict[str, Any], - tool_call_id: str | None = None, - confirmation_strategy_context: dict[str, Any] | None = None, - ) -> ToolExecutionDecision: - return ToolExecutionDecision(tool_name=tool_name, execute=True, final_tool_params=tool_params) - - def to_dict(self) -> dict[str, Any]: - return {"type": "test.IdDroppingConfirmationStrategy", "init_parameters": {}} - - @classmethod - def from_dict(cls, data: dict[str, Any]) -> "IdDroppingConfirmationStrategy": - return cls() - - -class TestDecisionsCarryTheirToolCallId: - def test_id_is_restored_when_the_strategy_drops_it(self, tools): - teds = _run_confirmation_strategies( - confirmation_strategies={tools[0].name: IdDroppingConfirmationStrategy()}, - messages_with_tool_calls=[ - ChatMessage.from_assistant( - tool_calls=[ - ToolCall(tools[0].name, {"a": 1, "b": 2}, id="1"), - ToolCall(tools[0].name, {"a": 3, "b": 4}, id="2"), - ] - ) - ], - tools=tools, - ) - assert [ted.tool_call_id for ted in teds] == ["1", "2"] - - def test_id_is_left_alone_when_the_tool_call_has_none(self, tools): - teds = _run_confirmation_strategies( - confirmation_strategies={tools[0].name: IdDroppingConfirmationStrategy()}, - messages_with_tool_calls=[ - ChatMessage.from_assistant(tool_calls=[ToolCall(tools[0].name, {"a": 1, "b": 2})]) - ], - tools=tools, - ) - assert teds[0].tool_call_id is None - - @pytest.mark.asyncio - async def test_id_is_restored_when_the_strategy_drops_it_async(self, tools): - teds = await _run_confirmation_strategies_async( - confirmation_strategies={tools[0].name: IdDroppingConfirmationStrategy()}, - messages_with_tool_calls=[ - ChatMessage.from_assistant( - tool_calls=[ - ToolCall(tools[0].name, {"a": 1, "b": 2}, id="1"), - ToolCall(tools[0].name, {"a": 3, "b": 4}, id="2"), - ] - ) - ], - tools=tools, - ) - assert [ted.tool_call_id for ted in teds] == ["1", "2"] +@pytest.mark.parametrize(("tool_call_id", "decision_id"), [("1", None), ("1", "wrong"), (None, "wrong"), ("1", "1")]) +def test_bind_decision_to_tool_call(tool_call_id: str | None, decision_id: str | None) -> None: + decision = ToolExecutionDecision("tool", execute=True, tool_call_id=decision_id) + bound_decision = _bind_decision_to_tool_call(decision=decision, tool_call=ToolCall("tool", {}, id=tool_call_id)) + assert bound_decision.tool_call_id == tool_call_id