diff --git a/agent/agent_runtime_helpers.py b/agent/agent_runtime_helpers.py index 186da719ef..96f7bb821a 100644 --- a/agent/agent_runtime_helpers.py +++ b/agent/agent_runtime_helpers.py @@ -3678,9 +3678,21 @@ def sanitize_api_messages(messages: List[Dict[str, Any]]) -> List[Dict[str, Any] # tool result. This is the final pre-API chokepoint, so dedup defensively # here even though repair_message_sequence also consumes matched ids. # (a) collapse duplicate tool_calls WITHIN an assistant message - # (b) drop later tool result messages reusing an already-seen id + # (b) drop tool results that answer no OUTSTANDING tool call + # + # (b) tracks outstanding calls rather than every id ever seen, because + # ``tool_call_id`` is NOT globally unique in practice: llama.cpp emits a + # single constant id for every tool call it ever returns (verified: three + # separate completions from one server all carry the same id). A + # seen-once-drop-forever rule reads the SECOND legitimate tool result of + # such a session as a duplicate and deletes it, so from the second tool + # call onward the model never sees any result — it announces its next + # action and the turn dies with the work unfinished. Outstanding-call + # semantics keep both protections intact: a re-emitted result still + # answers no pending call and is still dropped, while a genuine new call + # that reuses the id re-arms that id first. seen_assistant_call_ids: set = set() - seen_result_call_ids: set = set() + outstanding_call_ids: set = set() deduped: List[Dict[str, Any]] = [] removed_dupes = 0 for msg in messages: @@ -3694,6 +3706,7 @@ def sanitize_api_messages(messages: List[Dict[str, Any]]) -> List[Dict[str, Any] continue if cid: seen_assistant_call_ids.add(cid) + outstanding_call_ids.add(cid) kept_tcs.append(tc) if kept_tcs: msg = {**msg, "tool_calls": kept_tcs} @@ -3702,11 +3715,15 @@ def sanitize_api_messages(messages: List[Dict[str, Any]]) -> List[Dict[str, Any] deduped.append(msg) elif role == "tool": cid = (msg.get("tool_call_id") or "").strip() - if cid and cid in seen_result_call_ids: + if cid and cid not in outstanding_call_ids: removed_dupes += 1 continue if cid: - seen_result_call_ids.add(cid) + # Answered: this id is no longer outstanding, so a second + # result replaying it is still caught above. + outstanding_call_ids.discard(cid) + # A reused id must be re-armable by the next assistant call. + seen_assistant_call_ids.discard(cid) deduped.append(msg) else: deduped.append(msg) diff --git a/tests/run_agent/test_message_sequence_repair.py b/tests/run_agent/test_message_sequence_repair.py index 39fae300c5..17a9f78a3c 100644 --- a/tests/run_agent/test_message_sequence_repair.py +++ b/tests/run_agent/test_message_sequence_repair.py @@ -336,6 +336,75 @@ def test_sanitize_preserves_distinct_tool_call_ids(): assert sorted(m["tool_call_id"] for m in out if m.get("role") == "tool") == ["call_A", "call_B"] +# ── tool_call_id reuse by local servers (#70724) ──────────────────────────── +# llama.cpp emits ONE constant tool_call_id for every tool call it returns, so +# ``tool_call_id`` is not globally unique in practice. The #58327 dedup pass +# must key off outstanding calls, not "seen at any point", or every result +# after the first is deleted and the agent stops mid-task. + + +CONSTANT_ID = "ZsSt4SkIFMRz0HtqT7MTlimNvzlKM896" + + +def _call(cid, name="terminal"): + return {"role": "assistant", "content": None, + "tool_calls": [{"id": cid, "type": "function", + "function": {"name": name, "arguments": "{}"}}]} + + +def _result(cid, content): + return {"role": "tool", "tool_call_id": cid, "name": "terminal", + "content": content} + + +def test_sanitize_keeps_results_when_server_reuses_one_tool_call_id(): + """Every answered call survives even when all of them share one id. + + Contract: a tool result is dropped for being unanswerable, never for + reusing an id that an earlier call already retired. + """ + from agent.agent_runtime_helpers import sanitize_api_messages + + messages = [{"role": "user", "content": "do three steps"}] + for i in range(3): + messages.append(_call(CONSTANT_ID)) + messages.append(_result(CONSTANT_ID, f"step {i} output")) + + out = sanitize_api_messages(list(messages)) + results = [m for m in out if m.get("role") == "tool"] + assert [m["content"] for m in results] == [ + "step 0 output", "step 1 output", "step 2 output", + ] + calls = [m for m in out if m.get("role") == "assistant" and m.get("tool_calls")] + assert len(calls) == 3 + + +def test_sanitize_still_drops_replayed_result_for_retired_call(): + """The #58327 protection holds: a second result for an already-answered + call answers nothing outstanding and is still dropped.""" + from agent.agent_runtime_helpers import sanitize_api_messages + + messages = [ + {"role": "user", "content": "hi"}, + _call(CONSTANT_ID), + _result(CONSTANT_ID, "real"), + _result(CONSTANT_ID, "replayed by a retry/resume glitch"), + ] + out = sanitize_api_messages(list(messages)) + assert [m["content"] for m in out if m.get("role") == "tool"] == ["real"] + + +def test_sanitize_drops_result_with_no_preceding_call(): + """A tool result that never had a call is an orphan regardless of id.""" + from agent.agent_runtime_helpers import sanitize_api_messages + + out = sanitize_api_messages([ + {"role": "user", "content": "hi"}, + _result("id_never_requested", "orphan"), + ]) + assert [m for m in out if m.get("role") == "tool"] == [] + + def test_sanitize_drops_empty_tool_calls_array(): """sanitize_api_messages strips ``tool_calls: []`` from assistant messages.