Studio: don't drop parallel tool calls after an internal no-op (#7157)
This commit is contained in:
parent
8c83478da0
commit
3555dbdda7
6 changed files with 188 additions and 9 deletions
|
|
@ -82,6 +82,7 @@ from core.inference.tool_call_parser import (
|
|||
)
|
||||
from core.inference.tool_loop_controller import (
|
||||
ToolLoopController,
|
||||
append_deferred_nudges,
|
||||
tool_event_provenance,
|
||||
)
|
||||
from state.tool_approvals import (
|
||||
|
|
@ -10111,6 +10112,9 @@ class LlamaCppBackend:
|
|||
|
||||
assistant_msg: dict = {"role": "assistant", "content": content_text}
|
||||
assistant_appended = False
|
||||
# Collect no-op nudges and flush them after the batch, so a no-op
|
||||
# doesn't abort it and drop the parallel calls that follow.
|
||||
deferred_noop_msgs: list = []
|
||||
|
||||
# The text-path provisional card uses the parser's default id ("call_0");
|
||||
# a Mistral-style call carries its own id and would open a duplicate. Reuse
|
||||
|
|
@ -10153,14 +10157,14 @@ class LlamaCppBackend:
|
|||
"provenance": decision.provenance,
|
||||
}
|
||||
completion = tool_controller.record_noop(decision)
|
||||
conversation.append(completion.model_message())
|
||||
deferred_noop_msgs.append(completion.model_message())
|
||||
if _forced_tool_call_pending:
|
||||
_forced_tool_call_pending = False
|
||||
logger.info(
|
||||
"Suppressed local GGUF tool call as internal no-op: "
|
||||
f"action={decision.action} tool={decision.tool_name}"
|
||||
)
|
||||
break
|
||||
continue
|
||||
|
||||
if not assistant_appended:
|
||||
assistant_msg["tool_calls"] = [decision.as_assistant_tool_call()]
|
||||
|
|
@ -10279,6 +10283,8 @@ class LlamaCppBackend:
|
|||
if _forced_tool_call_pending:
|
||||
_forced_tool_call_pending = False
|
||||
|
||||
append_deferred_nudges(conversation, deferred_noop_msgs)
|
||||
|
||||
# Close provisional cards not resolved by execution/no-op handling.
|
||||
for _pid, _pname in provisional_started_tool_calls.items():
|
||||
if _pid not in resolved_provisional_tool_call_ids:
|
||||
|
|
|
|||
|
|
@ -57,6 +57,7 @@ from core.tool_healing import (
|
|||
)
|
||||
from core.inference.tool_loop_controller import (
|
||||
ToolLoopController,
|
||||
append_deferred_nudges,
|
||||
coerce_tool_arguments,
|
||||
status_for_tool,
|
||||
tool_event_provenance,
|
||||
|
|
@ -1099,6 +1100,9 @@ def run_safetensors_tool_loop(
|
|||
|
||||
assistant_msg: dict = {"role": "assistant", "content": content_text}
|
||||
assistant_appended = False
|
||||
# Collect no-op nudges and flush them after the batch, so a no-op doesn't
|
||||
# abort it and drop the parallel calls that follow.
|
||||
deferred_noop_msgs: list = []
|
||||
|
||||
for tc in tool_calls or []:
|
||||
func = tc.get("function", {}) or {}
|
||||
|
|
@ -1127,12 +1131,12 @@ def run_safetensors_tool_loop(
|
|||
"provenance": decision.provenance,
|
||||
}
|
||||
completion = tool_controller.record_noop(decision)
|
||||
conversation.append(completion.model_message())
|
||||
deferred_noop_msgs.append(completion.model_message())
|
||||
logger.info(
|
||||
"Suppressed local safetensors tool call as internal no-op: "
|
||||
f"action={decision.action} tool={decision.tool_name}"
|
||||
)
|
||||
break
|
||||
continue
|
||||
|
||||
if not assistant_appended:
|
||||
assistant_msg["tool_calls"] = [decision.as_assistant_tool_call()]
|
||||
|
|
@ -1243,6 +1247,8 @@ def run_safetensors_tool_loop(
|
|||
yield completion.tool_end_event()
|
||||
conversation.append(completion.tool_message())
|
||||
|
||||
append_deferred_nudges(conversation, deferred_noop_msgs)
|
||||
|
||||
# Clear the status badge before the next turn.
|
||||
yield {"type": "status", "text": ""}
|
||||
|
||||
|
|
|
|||
|
|
@ -266,6 +266,17 @@ def strip_result_for_model(result: str) -> str:
|
|||
return result
|
||||
|
||||
|
||||
def append_deferred_nudges(conversation: list, msgs: Sequence[dict]) -> None:
|
||||
"""Append a batch's no-op nudges as one deduped ``role=user`` message.
|
||||
|
||||
Deferred to after the batch's tool results so a no-op never splits an
|
||||
assistant's ``tool_calls`` from their ``role=tool`` results.
|
||||
"""
|
||||
contents = list(dict.fromkeys(msg["content"] for msg in msgs))
|
||||
if contents:
|
||||
conversation.append({"role": "user", "content": "\n\n".join(contents)})
|
||||
|
||||
|
||||
def _tool_name_from_schema(tool: Mapping[str, Any]) -> str:
|
||||
function = tool.get("function")
|
||||
if not isinstance(function, Mapping):
|
||||
|
|
@ -277,8 +288,9 @@ def _tool_name_from_schema(tool: Mapping[str, Any]) -> str:
|
|||
def _noop_result(reason: NoopReason, tool_name: str) -> str:
|
||||
if reason == "duplicate":
|
||||
return (
|
||||
"The previous tool request was not executed because this exact "
|
||||
"tool call already completed successfully. Do not repeat the same "
|
||||
f"One earlier request to call tool '{tool_name}' in this batch was "
|
||||
"not executed because an identical call had already completed "
|
||||
"successfully. Do not repeat the same "
|
||||
"tool call. Continue with a different enabled tool if that would "
|
||||
"materially help, or provide the final answer if you have enough "
|
||||
"information."
|
||||
|
|
@ -291,8 +303,8 @@ def _noop_result(reason: NoopReason, tool_name: str) -> str:
|
|||
"the requested final note or answer."
|
||||
)
|
||||
return (
|
||||
f"The previous tool request was not executed because tool "
|
||||
f"'{tool_name}' is not enabled for this request. Provide the "
|
||||
f"One earlier request to call tool '{tool_name}' in this batch was "
|
||||
"not executed because that tool is not enabled for this request. Provide the "
|
||||
"final answer now without calling more tools."
|
||||
)
|
||||
|
||||
|
|
|
|||
|
|
@ -1061,6 +1061,80 @@ def test_same_turn_duplicate_web_search_is_internal_noop(monkeypatch):
|
|||
]
|
||||
|
||||
|
||||
def test_same_turn_duplicate_does_not_drop_later_parallel_call(monkeypatch):
|
||||
# One batch: search(a), search(a) [duplicate], search(b). The duplicate is an
|
||||
# internal no-op, but the distinct search(b) after it must still run, and the
|
||||
# no-op nudge must land after the tool results rather than splitting them.
|
||||
batch = [
|
||||
_sse(
|
||||
{
|
||||
"tool_calls": [
|
||||
{
|
||||
"index": 0,
|
||||
"id": "call_a1",
|
||||
"type": "function",
|
||||
"function": {"name": "web_search", "arguments": json.dumps({"query": "a"})},
|
||||
},
|
||||
{
|
||||
"index": 1,
|
||||
"id": "call_a2",
|
||||
"type": "function",
|
||||
"function": {"name": "web_search", "arguments": json.dumps({"query": "a"})},
|
||||
},
|
||||
{
|
||||
"index": 2,
|
||||
"id": "call_b",
|
||||
"type": "function",
|
||||
"function": {"name": "web_search", "arguments": json.dumps({"query": "b"})},
|
||||
},
|
||||
]
|
||||
}
|
||||
),
|
||||
_done(),
|
||||
]
|
||||
final_stream = [_sse({"content": "Final answer."}), _done()]
|
||||
payloads: list[dict] = []
|
||||
backend = _make_backend(monkeypatch, [batch, final_stream], payloads)
|
||||
|
||||
calls: list[dict] = []
|
||||
|
||||
def fake_execute_tool(name, arguments, **_kwargs):
|
||||
calls.append(arguments)
|
||||
return "search-result"
|
||||
|
||||
monkeypatch.setattr("core.inference.tools.execute_tool", fake_execute_tool)
|
||||
|
||||
events = list(
|
||||
backend.generate_chat_completion_with_tools(
|
||||
messages = [{"role": "user", "content": "search"}],
|
||||
tools = [{"type": "function", "function": {"name": "web_search"}}],
|
||||
max_tool_iterations = 3,
|
||||
)
|
||||
)
|
||||
|
||||
# Both distinct calls ran; the duplicate did not (old `break` dropped search(b)).
|
||||
assert calls == [{"query": "a"}, {"query": "b"}]
|
||||
assert [e.get("tool_call_id") for e in events if e.get("type") == "tool_end"] == [
|
||||
"call_a1",
|
||||
"call_b",
|
||||
]
|
||||
|
||||
# The next generation's conversation must be well-formed: the assistant lists
|
||||
# only the executed calls (no orphan for the duplicate), the two tool results
|
||||
# follow contiguously, and the no-op nudge lands after them, never between.
|
||||
conv = payloads[1]["messages"]
|
||||
asst = next(m for m in conv if m["role"] == "assistant" and m.get("tool_calls"))
|
||||
assert [tc.get("id") for tc in asst["tool_calls"]] == ["call_a1", "call_b"]
|
||||
after = conv[conv.index(asst) + 1 :]
|
||||
assert [m["role"] for m in after[:2]] == ["tool", "tool"]
|
||||
assert [m.get("tool_call_id") for m in after[:2]] == ["call_a1", "call_b"]
|
||||
assert after[2]["role"] == "user" # deferred duplicate nudge, after the results
|
||||
assert after[2]["content"].startswith(
|
||||
"One earlier request to call tool 'web_search' in this batch was not executed"
|
||||
)
|
||||
assert "previous tool request" not in after[2]["content"].lower()
|
||||
|
||||
|
||||
def test_same_turn_repeated_render_html_does_not_emit_second_provisional_start(monkeypatch):
|
||||
same_turn_render_calls = [
|
||||
_sse(
|
||||
|
|
|
|||
|
|
@ -2843,6 +2843,61 @@ class TestLoopBehaviour:
|
|||
]
|
||||
assert len(duplicate_nudges) == 1
|
||||
|
||||
def test_same_turn_duplicate_does_not_drop_later_parallel_call(self):
|
||||
# Turn 1 runs search(x). Turn 2's batch is [search(x) duplicate, python]:
|
||||
# the duplicate is a no-op, but python after it must still run, and the
|
||||
# no-op nudge must land after python's result rather than splitting it.
|
||||
captured_messages: list[list[dict]] = []
|
||||
turns = iter(
|
||||
[
|
||||
['<tool_call>{"name":"web_search","arguments":{"query":"x"}}</tool_call>'],
|
||||
[
|
||||
'<tool_call>{"name":"web_search","arguments":{"query":"x"}}</tool_call>'
|
||||
'<tool_call>{"name":"python","arguments":{"code":"print(1)"}}</tool_call>'
|
||||
],
|
||||
["final"],
|
||||
]
|
||||
)
|
||||
|
||||
def fake_single_turn(messages, active_tools = None):
|
||||
captured_messages.append([dict(m) for m in messages])
|
||||
chunks = next(turns)
|
||||
acc = ""
|
||||
for chunk in chunks:
|
||||
acc += chunk
|
||||
yield acc
|
||||
|
||||
exec_fn = FakeExecuteTool(["search-x", "py-result"])
|
||||
_collect_events(
|
||||
run_safetensors_tool_loop(
|
||||
single_turn = fake_single_turn,
|
||||
messages = [{"role": "user", "content": "hi"}],
|
||||
tools = [
|
||||
{"type": "function", "function": {"name": "web_search"}},
|
||||
{"type": "function", "function": {"name": "python"}},
|
||||
],
|
||||
execute_tool = exec_fn,
|
||||
max_tool_iterations = 4,
|
||||
)
|
||||
)
|
||||
|
||||
# Turn-1 search and turn-2 python both ran; the turn-2 duplicate search did not.
|
||||
assert exec_fn.calls == [
|
||||
("web_search", {"query": "x"}),
|
||||
("python", {"code": "print(1)"}),
|
||||
]
|
||||
|
||||
conv = captured_messages[-1]
|
||||
turn2 = [m for m in conv if m.get("role") == "assistant" and m.get("tool_calls")][-1]
|
||||
assert [tc["function"]["name"] for tc in turn2["tool_calls"]] == ["python"]
|
||||
after = conv[conv.index(turn2) + 1 :]
|
||||
assert after[0]["role"] == "tool" and after[0]["content"] == "py-result"
|
||||
assert after[1]["role"] == "user" # deferred duplicate nudge, after the result
|
||||
assert after[1]["content"].startswith(
|
||||
"One earlier request to call tool 'web_search' in this batch was not executed"
|
||||
)
|
||||
assert "previous tool request" not in after[1]["content"].lower()
|
||||
|
||||
def test_duplicate_tool_call_internal_noop_allows_distinct_followup_tool(self):
|
||||
captured_messages: list[list[dict]] = []
|
||||
captured_tool_names: list[list[str]] = []
|
||||
|
|
|
|||
|
|
@ -13,6 +13,7 @@ if _BACKEND_DIR not in sys.path:
|
|||
|
||||
from core.inference.tool_loop_controller import (
|
||||
ToolLoopController,
|
||||
append_deferred_nudges,
|
||||
canonical_tool_call_key,
|
||||
coerce_tool_arguments,
|
||||
status_for_tool,
|
||||
|
|
@ -21,6 +22,22 @@ from core.inference.tool_loop_controller import (
|
|||
)
|
||||
|
||||
|
||||
def test_append_deferred_nudges_merges_deduped_into_one_message():
|
||||
conversation = [{"role": "assistant", "tool_calls": [1]}, {"role": "tool", "content": "r"}]
|
||||
nudges = [
|
||||
{"role": "user", "content": "duplicate"},
|
||||
{"role": "user", "content": "duplicate"}, # dropped: same content
|
||||
{"role": "user", "content": "disabled foo"},
|
||||
]
|
||||
append_deferred_nudges(conversation, nudges)
|
||||
# One user message, after the results, with distinct contents joined.
|
||||
assert conversation[2:] == [{"role": "user", "content": "duplicate\n\ndisabled foo"}]
|
||||
# Empty is a no-op.
|
||||
before = list(conversation)
|
||||
append_deferred_nudges(conversation, [])
|
||||
assert conversation == before
|
||||
|
||||
|
||||
def _tool(name: str) -> dict:
|
||||
return {"type": "function", "function": {"name": name}}
|
||||
|
||||
|
|
@ -111,6 +128,10 @@ def test_successful_duplicate_is_internal_noop_and_keeps_remaining_tools():
|
|||
assert not duplicate.should_execute
|
||||
assert not duplicate.emit_visible_events
|
||||
duplicate_nudge = completion.model_message()["content"]
|
||||
assert duplicate_nudge.startswith(
|
||||
"One earlier request to call tool 'web_search' in this batch was not executed"
|
||||
)
|
||||
assert "previous tool request" not in duplicate_nudge.lower()
|
||||
assert "already completed successfully" in duplicate_nudge
|
||||
assert "different enabled tool" in duplicate_nudge
|
||||
assert completion.model_message()["role"] == "user"
|
||||
|
|
@ -165,7 +186,12 @@ def test_empty_enabled_tool_list_blocks_all_tool_calls():
|
|||
assert decision.action == "disabled"
|
||||
assert not decision.emit_visible_events
|
||||
assert completion.model_message()["role"] == "user"
|
||||
assert "not enabled" in completion.model_message()["content"]
|
||||
disabled_nudge = completion.model_message()["content"]
|
||||
assert disabled_nudge.startswith(
|
||||
"One earlier request to call tool 'web_search' in this batch was not executed"
|
||||
)
|
||||
assert "previous tool request" not in disabled_nudge.lower()
|
||||
assert "not enabled" in disabled_nudge
|
||||
assert controller.force_final_answer
|
||||
assert controller.active_tools() == []
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue