fix(core): keep every tool result paired when a tool-call id collides [K-03]

core.py

@@ -18,6 +18,7 @@
 
 
 from utils.json_utils import parse_tool_call_arguments
+from utils.tool_ids import uniquify_tool_call_ids
 
 
 
@@ -970,18 +971,34 @@ def _run_conversation_loop(self, user_input: str, image_path: str | None = None)
                                         logging.getLogger(__name__).warning("Rate limit hit during stream, rotated to next API key.")
                                         continue
 
-                            # Auto-recover from "tool_calls must be followed by tool messages" (400)
                             _emsg_orig = str(_e)
-                            if not _stream_retried and ("tool_calls" in _emsg_orig or "400" in _emsg_orig):
-                                _stream_retried = True
+                            # Auto-recover from "tool_calls must be followed by tool messages" (400).
+                            # The substring match is broad on purpose (providers word this
+                            # differently), but this recovery is destructive — it removes the
+                            # offending round from history — so it must be earned: drop only
+                            # when something is actually dangling, and re-request only then.
+                            # Otherwise an unrelated error whose text merely contains "400"
+                            # (e.g. "... 400 requests/min", a token count) hijacked the branch,
+                            # dropped nothing, retried with no backoff, and never reached the
+                            # key-rotation / backoff recovery below.
+                            if (
+                                not _stream_retried
+                                and ("tool_calls" in _emsg_orig or "400" in _emsg_orig)
+                            ):
                                 n = self._drop_dangling_tool_calls()
                                 import logging as _logging
+                                if n:
+                                    _stream_retried = True
+                                    _logging.getLogger(__name__).warning(
+                                        f"stream_chat: 400 tool_calls error — dropped {n} dangling messages, retrying"
+                                    )
+                                    full = ""
+                                    _tool_buf = {}
+                                    continue
                                 _logging.getLogger(__name__).warning(
-                                    f"stream_chat: 400 tool_calls error — dropped {n} dangling messages, retrying"
+                                    "stream_chat: 400 tool_calls error but the history has no "
+                                    "dangling tool call to drop — not retrying"
                                 )
-                                full = ""
-                                _tool_buf = {}
-                                continue
 
                             # If rate limit or overloaded/timeout, retry with jittered backoff
                             if (is_rate_limit or is_overloaded) and _stream_call_retries < _MAX_STREAM_RETRIES:
@@ -1097,19 +1114,36 @@ def _run_conversation_loop(self, user_input: str, image_path: str | None = None)
 
                 # ── Build call list from buffered chunks ─────────────────────────
                 calls = []
-                invalid_args: dict[str, str] = {}  # tool_call_id -> reason
+                args_errors: list[str | None] = []
                 for idx, stc in sorted(_tool_buf.items()):
                     raw_args = stc["args"] or "{}"
                     args_parsed, args_error = parse_tool_call_arguments(raw_args, tool_name=stc["name"])
-                    call_id = stc["id"] or stc["name"]
-                    if args_error:
-                        invalid_args[call_id] = args_error
-                        args_parsed = {}
                     calls.append({
-                        "id": call_id,
+                        "id": stc["id"] or stc["name"],
                         "name": stc["name"],
                         "arguments": args_parsed,
                     })
+                    args_errors.append(args_error)
+
+                # A provider may reuse one id — or omit it — for several calls in
+                # a batch. The id keys every downstream result map
+                # (completed_results / blocked_results and the tool_call_id of each
+                # tool message), so a collision silently drops one tool's result and
+                # repeats its sibling's, and strict providers 400 on the duplicate.
+                # Rename duplicates deterministically (never uuid4 — these ids ride
+                # the prompt-cache prefix).
+                if uniquify_tool_call_ids(calls):
+                    import logging
+                    logging.getLogger(__name__).warning(
+                        "Duplicate tool-call id(s) in one round were renamed so every "
+                        "tool result stays paired."
+                    )
+
+                invalid_args: dict[str, str] = {}  # tool_call_id -> reason
+                for call, args_error in zip(calls, args_errors):
+                    if args_error:
+                        invalid_args[call["id"]] = args_error
+                        call["arguments"] = {}
 
                 self.messages.append({
                     "role": "assistant",

tests/test_tool_call_ids.py

@@ -0,0 +1,199 @@
+"""Tool-call id collisions: every tool result must keep its own pairing id.
+
+A provider that reuses one id — or omits it — for several calls in a batch used
+to lose results: the id keys the result maps in the parallel dispatch path, so
+the second call's result overwrote the first's and both tool messages were
+written with the same ``tool_call_id``. The round is now uniquified before
+dispatch, and the 400 "dangling tool_calls" recovery only fires when the
+history actually has something dangling to repair.
+"""
+import os
+import sys
+import threading
+
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+from core import Agent, ToolLoopGuardrail
+from utils.tool_ids import uniquify_tool_call_ids
+
+# ── id uniquification ────────────────────────────────────────────────────────
+
+
+def test_distinct_ids_are_left_alone():
+    calls = [{"id": "a", "name": "read_file"}, {"id": "b", "name": "list_dir"}]
+    assert uniquify_tool_call_ids(calls) == 0
+    assert [c["id"] for c in calls] == ["a", "b"]
+
+
+def test_duplicate_id_gets_a_deterministic_suffix():
+    calls = [{"id": "call_1", "name": "read_file"}, {"id": "call_1", "name": "list_dir"}]
+    assert uniquify_tool_call_ids(calls) == 1
+    assert [c["id"] for c in calls] == ["call_1", "call_1_d2"]
+
+
+def test_third_collision_gets_the_next_suffix():
+    calls = [{"id": "x", "name": "a"}, {"id": "x", "name": "b"}, {"id": "x", "name": "c"}]
+    assert uniquify_tool_call_ids(calls) == 2
+    assert [c["id"] for c in calls] == ["x", "x_d2", "x_d3"]
+
+
+def test_renaming_never_produces_a_collision():
+    calls = [{"id": "x", "name": "a"}, {"id": "x", "name": "b"}, {"id": "x_d2", "name": "c"}]
+    renamed = uniquify_tool_call_ids(calls)
+    ids = [c["id"] for c in calls]
+    assert renamed == 2
+    assert len(set(ids)) == 3, "every call must end up with a distinct id"
+    assert ids[0] == "x", "the first call keeps the id the model sent"
+
+
+def test_blank_ids_are_skipped():
+    calls = [{"id": "", "name": "a"}, {"id": None, "name": "a"}, {"id": "  ", "name": "a"}]
+    assert uniquify_tool_call_ids(calls) == 0
+    assert [c["id"] for c in calls] == ["", None, "  "]
+
+
+def test_renaming_is_deterministic():
+    def run():
+        calls = [{"id": "c", "name": "a"}, {"id": "c", "name": "b"}]
+        uniquify_tool_call_ids(calls)
+        return [c["id"] for c in calls]
+
+    assert run() == run(), "ids ride the prompt-cache prefix — renaming must not be random"
+
+
+# ── the conversation loop ────────────────────────────────────────────────────
+
+
+class _FakeProvider:
+    name = "fake"
+    supports_vision = False
+    supports_thinking = False
+    _model = "fake-model"
+
+    def __init__(self, rounds):
+        self._rounds = list(rounds)
+        self.rounds_started = 0
+
+    def stream_chat(self, messages, tools=None, cancel_event=None):
+        self.rounds_started += 1
+        if not self._rounds:
+            return
+        item = self._rounds.pop(0)
+        if isinstance(item, BaseException):
+            raise item
+        yield from item
+
+
+def _tool_chunk(index, name, args_chunk, call_id=None):
+    return {
+        "__tool_chunk__": True,
+        "index": index,
+        "id": call_id or f"{name}-{index}",
+        "name": name,
+        "args_chunk": args_chunk,
+    }
+
+
+def _finish(reason="tool_calls"):
+    return {"__finish_reason__": reason}
+
+
+def _make_agent(rounds):
+    """Agent with only the attributes the conversation loop actually reads."""
+    agent = Agent.__new__(Agent)
+    agent.provider = _FakeProvider(rounds)
+    agent.messages = [{"role": "system", "content": "test"}]
+    agent._cancel = threading.Event()
+    agent._busy = False
+    agent._router = None
+    agent._guardrail = ToolLoopGuardrail()
+    agent.permission_callback = None
+    agent.tool_progress_callback = None
+    agent._pre_fetch_links = lambda user_input: iter(())
+    agent._refresh_memory_context = lambda *a, **k: None
+    agent._resolve_available_tools = lambda *a, **k: []
+    # Echo the path back so each result is attributable to its own call.
+    agent._execute_tool = lambda name, args: f"read {args.get('path')}"
+    return agent
+
+
+def _events(agent, text="do something"):
+    return list(agent._run_conversation_loop(text))
+
+
+def _tool_results(agent):
+    return [m for m in agent.messages if m.get("role") == "tool"]
+
+
+def test_colliding_ids_keep_every_tool_result():
+    agent = _make_agent(
+        [
+            [
+                _tool_chunk(0, "read_file", '{"path": "a.py"}', call_id="call_dup"),
+                _tool_chunk(1, "read_file", '{"path": "b.py"}', call_id="call_dup"),
+                _finish(),
+            ],
+            ["done"],
+        ]
+    )
+
+    _events(agent)
+
+    results = _tool_results(agent)
+    assert len(results) == 2, "both calls ran, so both results must be in the history"
+    ids = [m["tool_call_id"] for m in results]
+    assert len(set(ids)) == 2, "each tool result must keep its own pairing id"
+
+    # Not just present — paired with the *right* call. Under the collision the
+    # second result overwrote the first and was replayed for both calls.
+    paired = {m["tool_call_id"]: m["content"] for m in results}
+    assert paired[ids[0]] == "read a.py"
+    assert paired[ids[1]] == "read b.py"
+
+    assistant = [
+        m for m in agent.messages
+        if m.get("role") == "assistant" and m.get("tool_calls")
+    ][-1]
+    assert [tc["id"] for tc in assistant["tool_calls"]] == ids
+
+
+def test_unrelated_400_does_not_retry_a_history_with_nothing_dangling():
+    agent = _make_agent([RuntimeError("HTTP 400: maximum context length is 4000 tokens")])
+
+    events = _events(agent)
+
+    assert agent.provider.rounds_started == 1, (
+        "a 400 whose text merely contains '400' has nothing to repair — it must "
+        "not consume the recovery and re-request"
+    )
+    assert any(e.get("type") == "error" for e in events)
+
+
+def test_dangling_round_is_repaired_and_the_request_retried():
+    agent = _make_agent(
+        [
+            RuntimeError(
+                "400 An assistant message with 'tool_calls' must be followed by "
+                "tool messages responding to each 'tool_call_id'"
+            ),
+            ["recovered"],
+        ]
+    )
+    agent.messages += [
+        {"role": "user", "content": "hi"},
+        {
+            "role": "assistant",
+            "content": None,
+            "tool_calls": [{"id": "stale", "name": "read_file", "arguments": {}}],
+        },
+    ]
+
+    events = _events(agent)
+
+    assert agent.provider.rounds_started == 2, "a genuine dangling 400 must still recover"
+    assert not [
+        m for m in agent.messages
+        if m.get("role") == "assistant" and m.get("tool_calls") and m["tool_calls"][0]["id"] == "stale"
+    ], "the dangling round must be dropped before the retry"
+    text = "".join(e.get("token", "") for e in events if e.get("type") == "text")
+    assert "recovered" in text

utils/tool_ids.py

@@ -0,0 +1,50 @@
+"""Deterministic tool-call id uniquification.
+
+Providers occasionally reuse one tool-call id — or omit the id entirely — for
+several calls inside a single assistant turn. The id is the pairing key for
+every tool *result*, so a collision silently drops one tool's result and
+repeats the other's, and strict providers reject the duplicate outright (400).
+"""
+import logging
+
+logger = logging.getLogger(__name__)
+
+
+def uniquify_tool_call_ids(tool_calls: list) -> int:
+    """Give every call in one round a distinct, deterministic id.
+
+    Mutates each entry's ``"id"`` in place. Later collisions get an
+    ``<id>_d<n>`` suffix (n counted from 2, first occurrence wins) —
+    deterministic on purpose: these ids ride the prompt-cache prefix, so a
+    random suffix would break caching. Returns the number of entries renamed.
+
+    Blank / non-string ids are skipped: the caller has already coalesced them
+    to the tool name and there is nothing stable to suffix.
+    """
+    seen: set[str] = set()
+    renamed = 0
+    for call in tool_calls or []:
+        if not isinstance(call, dict):
+            continue
+        cid = call.get("id")
+        cid = cid.strip() if isinstance(cid, str) else ""
+        if not cid:
+            continue
+        if cid not in seen:
+            seen.add(cid)
+            continue
+        # Bounded: at most len(seen) suffixes can already be taken.
+        new_id = next(
+            f"{cid}_d{n}" for n in range(2, len(seen) + 3) if f"{cid}_d{n}" not in seen
+        )
+        seen.add(new_id)
+        call["id"] = new_id
+        renamed += 1
+        logger.warning(
+            "Model reused tool-call id %s in one round; renamed the duplicate to %s "
+            "(tool=%s) so every tool result stays paired.",
+            cid,
+            new_id,
+            call.get("name", "?"),
+        )
+    return renamed