From 66b79633f1088b9b80ff074e6959de7744c4b6e5 Mon Sep 17 00:00:00 2001 From: haydarkadioglu Date: Thu, 1 Oct 2026 13:59:27 +0000 Subject: [PATCH] fix(core): refuse tool calls with unparseable JSON arguments instead of executing them [K-02] --- core.py | 78 ++++++++++-- tests/test_tool_call_arguments.py | 196 ++++++++++++++++++++++++++++++ utils/json_utils.py | 63 ++++++++++ 3 files changed, 326 insertions(+), 11 deletions(-) create mode 100644 tests/test_tool_call_arguments.py diff --git a/core.py b/core.py index 478f4955..b43e7ac9 100644 --- a/core.py +++ b/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 diff --git a/tests/test_tool_call_arguments.py b/tests/test_tool_call_arguments.py new file mode 100644 index 00000000..3f4f63c1 --- /dev/null +++ b/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"})] diff --git a/utils/json_utils.py b/utils/json_utils.py index 21423736..8ecf43c3 100644 --- a/utils/json_utils.py +++ b/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 + +