From 3555dbdda7cf1fe5eb7f7036045b5076d56ca6bd Mon Sep 17 00:00:00 2001 From: oobabooga Date: Thu, 16 Jul 2026 19:47:22 -0300 Subject: [PATCH] Studio: don't drop parallel tool calls after an internal no-op (#7157) --- studio/backend/core/inference/llama_cpp.py | 10 ++- .../core/inference/safetensors_agentic.py | 10 ++- .../core/inference/tool_loop_controller.py | 20 ++++- .../backend/tests/test_llama_cpp_tool_loop.py | 74 +++++++++++++++++++ .../tests/test_safetensors_tool_loop.py | 55 ++++++++++++++ .../tests/test_tool_loop_controller.py | 28 ++++++- 6 files changed, 188 insertions(+), 9 deletions(-) diff --git a/studio/backend/core/inference/llama_cpp.py b/studio/backend/core/inference/llama_cpp.py index 06b68ea831..dadfdfd38d 100644 --- a/studio/backend/core/inference/llama_cpp.py +++ b/studio/backend/core/inference/llama_cpp.py @@ -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: diff --git a/studio/backend/core/inference/safetensors_agentic.py b/studio/backend/core/inference/safetensors_agentic.py index f4c243d1bf..43b72110ff 100644 --- a/studio/backend/core/inference/safetensors_agentic.py +++ b/studio/backend/core/inference/safetensors_agentic.py @@ -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": ""} diff --git a/studio/backend/core/inference/tool_loop_controller.py b/studio/backend/core/inference/tool_loop_controller.py index f595531b90..f7ed450d11 100644 --- a/studio/backend/core/inference/tool_loop_controller.py +++ b/studio/backend/core/inference/tool_loop_controller.py @@ -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." ) diff --git a/studio/backend/tests/test_llama_cpp_tool_loop.py b/studio/backend/tests/test_llama_cpp_tool_loop.py index c3161c5714..bd2c008589 100644 --- a/studio/backend/tests/test_llama_cpp_tool_loop.py +++ b/studio/backend/tests/test_llama_cpp_tool_loop.py @@ -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( diff --git a/studio/backend/tests/test_safetensors_tool_loop.py b/studio/backend/tests/test_safetensors_tool_loop.py index eae1a75161..e3633de289 100644 --- a/studio/backend/tests/test_safetensors_tool_loop.py +++ b/studio/backend/tests/test_safetensors_tool_loop.py @@ -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( + [ + ['{"name":"web_search","arguments":{"query":"x"}}'], + [ + '{"name":"web_search","arguments":{"query":"x"}}' + '{"name":"python","arguments":{"code":"print(1)"}}' + ], + ["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]] = [] diff --git a/studio/backend/tests/test_tool_loop_controller.py b/studio/backend/tests/test_tool_loop_controller.py index 0e8ae798af..496c30ac13 100644 --- a/studio/backend/tests/test_tool_loop_controller.py +++ b/studio/backend/tests/test_tool_loop_controller.py @@ -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() == []