mirror of
https://github.com/deepset-ai/haystack.git
synced 2026-10-12 02:12:50 +00:00
fix: Better HITL tool execution decision and tool call consistency (#12345)
This commit is contained in:
Sebastian Husch Lee
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user