fix(core): refuse tool calls with unparseable JSON arguments instead of executing them [K-02]

core.py

@@ -17,7 +17,7 @@
 from core_context import MAX_CONTEXT_MESSAGES, TOOL_COMPACT_AFTER
 
 
-from utils.json_utils import _escape_invalid_chars_in_json_strings, _repair_tool_call_arguments
+from utils.json_utils import parse_tool_call_arguments
 
 
 
@@ -806,7 +806,7 @@ def _run_conversation_loop(self, user_input: str, image_path: str | None = None)
           {"type": "error", "message": ...}
           {"type": "interrupted"}
         """
-        import time, json as _json
+        import time
 
         if hasattr(self, "_guardrail") and self._guardrail:
             self._guardrail.reset()
@@ -884,8 +884,10 @@ def _run_conversation_loop(self, user_input: str, image_path: str | None = None)
 
         try:
             MAX_ROUNDS = 25  # safety cap — allows complex multi-step tasks
+            MAX_INVALID_JSON_RETRIES = 3
             empty_retries = 0
             length_continuation_count = 0
+            invalid_json_retries = 0
             for _round in range(MAX_ROUNDS):
                 if self._cancel.is_set():
                     yield {"type": "interrupted"}
@@ -1095,18 +1097,16 @@ 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
                 for idx, stc in sorted(_tool_buf.items()):
                     raw_args = stc["args"] or "{}"
-                    try:
-                        args_parsed = _json.loads(raw_args)
-                    except Exception:
-                        repaired = _repair_tool_call_arguments(raw_args, tool_name=stc["name"])
-                        try:
-                            args_parsed = _json.loads(repaired)
-                        except Exception:
-                            args_parsed = {}
+                    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": stc["id"] or stc["name"],
+                        "id": call_id,
                         "name": stc["name"],
                         "arguments": args_parsed,
                     })
@@ -1117,6 +1117,62 @@ def _run_conversation_loop(self, user_input: str, image_path: str | None = None)
                     "tool_calls": calls,
                 })
 
+                # ── Unparseable arguments veto the whole round ───────────────────
+                # Running a tool on silently-emptied arguments makes it act on
+                # missing parameters; the model is told what was wrong instead
+                # and gets a bounded number of rounds to re-issue the call.
+                if invalid_args:
+                    invalid_json_retries += 1
+                    import logging
+                    _log = logging.getLogger(__name__)
+                    _first_reason = next(iter(invalid_args.values()))
+
+                    if invalid_json_retries > MAX_INVALID_JSON_RETRIES:
+                        _log.warning(
+                            f"Tool-call arguments were unparseable {invalid_json_retries} rounds in a row — "
+                            "refusing to execute them and ending the turn."
+                        )
+                        notice = (
+                            "I stopped instead of guessing: the model produced tool-call arguments that are "
+                            f"not valid JSON in {invalid_json_retries} consecutive rounds "
+                            f"({_first_reason}). Nothing was executed with empty arguments — "
+                            "please re-issue the request."
+                        )
+                        self.messages.append({"role": "assistant", "content": notice})
+                        for _tok in notice:
+                            yield {"type": "text", "token": _tok}
+                        return
+
+                    _log.warning(
+                        f"Invalid tool-call arguments (round {invalid_json_retries}/{MAX_INVALID_JSON_RETRIES}), "
+                        f"executing nothing: {_first_reason}"
+                    )
+                    for _tc in calls:
+                        if _tc["id"] in invalid_args:
+                            content = (
+                                f"Error: invalid JSON in arguments for '{_tc['name']}': "
+                                f"{invalid_args[_tc['id']]}. The tool was NOT executed. Re-issue this call "
+                                "with valid JSON; for a tool that takes no parameters use {}."
+                            )
+                        else:
+                            content = (
+                                "Skipped: another tool call in this round had invalid JSON arguments, so "
+                                "nothing was executed. Please re-issue this call after fixing the arguments."
+                            )
+                        yield {"type": "tool_done", "name": _tc["name"], "result": content, "elapsed": 0.0}
+                        self.messages.append({
+                            "role": "tool",
+                            "tool_call_id": _tc["id"],
+                            "name": _tc["name"],
+                            "content": content,
+                        })
+
+                    if self.provider.supports_thinking:
+                        yield {"type": "thinking"}
+                    continue
+
+                invalid_json_retries = 0
+
                 # ── Execute each tool call ────────────────────────────────────────
                 permanent_failure = False
 

tests/test_tool_call_arguments.py

@@ -0,0 +1,196 @@
+"""Tool-call arguments: an unparseable payload must never be executed.
+
+An argument payload that cannot be parsed used to be silently replaced with
+``{}`` and the tool ran anyway, acting on missing parameters. The conversation
+loop must instead refuse the whole round, feed the parse error back to the
+model, and end the turn if the model keeps emitting broken JSON — bounded, so
+it cannot spin forever.
+"""
+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.json_utils import (
+    parse_tool_call_arguments,
+    tool_call_arguments_look_truncated,
+)
+
+# ── argument parsing ─────────────────────────────────────────────────────────
+
+def test_empty_payload_is_a_parameterless_call():
+    assert parse_tool_call_arguments("") == ({}, None)
+    assert parse_tool_call_arguments("   ") == ({}, None)
+    assert parse_tool_call_arguments(None) == ({}, None)
+
+
+def test_repairable_payload_still_parses():
+    args, error = parse_tool_call_arguments('{"path": "a.py", "content": "x",}')
+    assert error is None
+    assert args == {"path": "a.py", "content": "x"}
+
+
+def test_truncated_payload_is_reported_not_repaired():
+    raw = '{"command": "rm -rf /tmp'
+    assert tool_call_arguments_look_truncated(raw)
+    args, error = parse_tool_call_arguments(raw, tool_name="run_terminal")
+    assert args == {}
+    assert error and "truncated" in error
+
+
+def test_unparseable_payload_is_reported():
+    args, error = parse_tool_call_arguments("this is not json", tool_name="read_file")
+    assert args == {}
+    assert error
+
+
+def test_non_object_payload_is_rejected():
+    args, error = parse_tool_call_arguments('["a", "b"]', tool_name="read_file")
+    assert args == {}
+    assert error and "object" in error
+
+
+def test_complete_payloads_are_not_flagged_as_truncated():
+    assert not tool_call_arguments_look_truncated('{"a": 1}')
+    assert not tool_call_arguments_look_truncated('{"a": 1}  ')
+    assert not tool_call_arguments_look_truncated("[]")
+
+
+# ── 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
+        yield from (self._rounds.pop(0) if self._rounds else [])
+
+
+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, executed):
+    """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: []
+
+    def execute(name, args):
+        executed.append((name, args))
+        return "ran"
+
+    agent._execute_tool = execute
+    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_truncated_arguments_are_not_executed_and_model_is_told_why():
+    executed = []
+    agent = _make_agent(
+        [
+            [_tool_chunk(0, "run_terminal", '{"command": "rm -rf /tmp'), _finish()],
+            ["all good now"],
+        ],
+        executed,
+    )
+
+    events = _events(agent)
+
+    assert executed == [], "a truncated argument payload must never reach the tool"
+    results = _tool_results(agent)
+    assert len(results) == 1
+    assert results[0]["tool_call_id"] == "run_terminal-0"
+    assert "invalid JSON" in results[0]["content"]
+    assert "NOT executed" in results[0]["content"]
+    assert "truncated" in results[0]["content"]
+    assert any(e.get("type") == "text" and "all good now" in e.get("token", "") for e in events)
+
+
+def test_valid_sibling_call_is_skipped_when_one_call_has_broken_json():
+    executed = []
+    agent = _make_agent(
+        [
+            [
+                _tool_chunk(0, "run_terminal", '{"command": "ls"}', "call-a"),
+                _tool_chunk(1, "read_file", '{"path": ', "call-b"),
+                _finish(),
+            ],
+            ["done"],
+        ],
+        executed,
+    )
+
+    _events(agent)
+
+    assert executed == [], "no call in a broken round may execute"
+    results = {m["tool_call_id"]: m["content"] for m in _tool_results(agent)}
+    assert set(results) == {"call-a", "call-b"}
+    assert "invalid JSON" in results["call-b"]
+    assert "Skipped" in results["call-a"]
+
+
+def test_turn_ends_after_repeated_broken_json_instead_of_looping():
+    executed = []
+    broken_round = [_tool_chunk(0, "run_terminal", '{"command": "ls'), _finish()]
+    agent = _make_agent([broken_round, broken_round, broken_round, broken_round], executed)
+
+    events = _events(agent)
+
+    assert executed == []
+    assert agent.provider.rounds_started == 4, "3 retries, then the turn must end"
+    assert any(
+        m.get("role") == "assistant" and "I stopped instead of guessing" in (m.get("content") or "")
+        for m in agent.messages
+    )
+    assert any(e.get("type") == "tool_done" for e in events)
+
+
+def test_repairable_arguments_still_execute():
+    executed = []
+    agent = _make_agent(
+        [
+            [_tool_chunk(0, "run_terminal", '{"command": "ls",}'), _finish()],
+            ["finished"],
+        ],
+        executed,
+    )
+
+    _events(agent)
+
+    assert executed == [("run_terminal", {"command": "ls"})]

utils/json_utils.py

@@ -96,4 +96,67 @@ def _repair_tool_call_arguments(raw_args: str, tool_name: str = "?") -> str:
     return "{}"
 
 
+def tool_call_arguments_look_truncated(raw_args: str) -> bool:
+    """True when a tool-call payload was cut off before its JSON object closed.
+
+    A complete object/array always ends in ``}`` or ``]``. Anything else was
+    cut by the output-length limit or by a stream that died mid-call, and a
+    truncation must never be executed as if it were a full argument set.
+    """
+    text = raw_args.strip() if isinstance(raw_args, str) else ""
+    if not text:
+        return False
+    return not text.endswith(("}", "]"))
+
+
+def parse_tool_call_arguments(raw_args: str | None, tool_name: str = "?") -> tuple[dict, str | None]:
+    """Parse one tool call's argument JSON, repairing minor malformations.
+
+    Returns ``(arguments, error)``. ``error`` is ``None`` when the payload was
+    usable; otherwise ``arguments`` is empty and the caller MUST NOT execute the
+    tool. Handing a silently emptied ``{}`` to a tool makes it act on missing
+    parameters — which looks like a tool bug and hides the real cause — while
+    reporting the parse failure back to the model lets it re-issue the call.
+    """
+    if raw_args is None:
+        return {}, None
+    if not isinstance(raw_args, str):
+        raw_args = str(raw_args)
+
+    stripped = raw_args.strip()
+    # Empty payloads are a common model quirk for parameterless tools.
+    if not stripped or stripped == "None":
+        return {}, None
+
+    try:
+        parsed = json.loads(stripped)
+    except Exception as exc:
+        # Refuse a cut-off payload before repair: closing its braces would turn
+        # a half-streamed call (e.g. {"command": "rm -rf /tmp) into a valid,
+        # executable one.
+        if tool_call_arguments_look_truncated(stripped):
+            return {}, (
+                "truncated JSON — the payload was cut off before closing its "
+                f"object: {stripped[:120]!r}"
+            )
+        repaired = _repair_tool_call_arguments(stripped, tool_name=tool_name)
+        # A non-empty payload repaired into "{}" means the repair gave up and
+        # returned its failure sentinel — treat that as unparseable.
+        if repaired == "{}" and re.sub(r"\s+", "", stripped) != "{}":
+            parsed = None
+        else:
+            try:
+                parsed = json.loads(repaired)
+            except Exception:
+                parsed = None
+        if parsed is None:
+            return {}, f"{exc}: {stripped[:120]!r}"
+
+    if not isinstance(parsed, dict):
+        return {}, (
+            f"expected a JSON object, got {type(parsed).__name__}: {stripped[:120]!r}"
+        )
+    return parsed, None
+
+