From 7432b2b1cddbdc3f5a75508678afb1547035b393 Mon Sep 17 00:00:00 2001 From: Jessica Mulein Date: Thu, 28 May 2026 07:29:09 -0700 Subject: [PATCH 1/7] fix(tools): normalize JSON-encoded tool args from local LLMs --- cecli/tools/read_range.py | 27 +++++++--- cecli/tools/update_todo_list.py | 80 ++++++++++++++-------------- cecli/tools/utils/helpers.py | 48 +++++++++++++++++ tests/tools/test_get_lines.py | 50 +++++++++++++++++ tests/tools/test_update_todo_list.py | 78 +++++++++++++++++++++++++++ 5 files changed, 235 insertions(+), 48 deletions(-) create mode 100644 tests/tools/test_update_todo_list.py diff --git a/cecli/tools/read_range.py b/cecli/tools/read_range.py index a9eaab3abfc..954eebac9d3 100644 --- a/cecli/tools/read_range.py +++ b/cecli/tools/read_range.py @@ -6,13 +6,23 @@ from cecli.tools.utils.base_tool import BaseTool from cecli.tools.utils.helpers import ( ToolError, + coerce_dict_item, handle_tool_error, is_provided, + normalize_json_array, resolve_paths, ) from cecli.tools.utils.output import color_markers, tool_footer, tool_header +def normalize_show_ops(show) -> List[dict]: + """Accept show as list, dict, JSON string, or list of JSON strings (LLM quirk).""" + return [ + coerce_dict_item(op, param_name="show operation") + for op in normalize_json_array(show, param_name="show") + ] + + class Tool(BaseTool): NORM_NAME = "readrange" TRACK_INVOCATIONS = False @@ -89,12 +99,8 @@ def execute(cls, coder, show, **kwargs): error_outputs = [] try: - # 1. Validate show parameter - if not isinstance(show, list): - show = [show] if isinstance(show, dict) else show - - if len(show) == 0: - raise ToolError("show array cannot be empty") + # 1. Validate show parameter (models sometimes double-encode show as JSON text) + show = normalize_show_ops(show) all_outputs = [] already_up_to_details = [] @@ -579,7 +585,14 @@ def format_output(cls, coder, mcp_server, tool_response): tool_header(coder=coder, mcp_server=mcp_server, tool_response=tool_response) - show_ops = params.get("show", []) + raw_show = params.get("show", []) + try: + show_ops = normalize_show_ops(raw_show) if raw_show else [] + except ToolError as err: + coder.io.tool_error(str(err)) + tool_footer(coder=coder, tool_response=tool_response) + return + if show_ops: coder.io.tool_output("") for i, show_op in enumerate(show_ops): diff --git a/cecli/tools/update_todo_list.py b/cecli/tools/update_todo_list.py index 223c85256a1..b713a3b953a 100644 --- a/cecli/tools/update_todo_list.py +++ b/cecli/tools/update_todo_list.py @@ -1,8 +1,44 @@ from cecli.tools.utils.base_tool import BaseTool -from cecli.tools.utils.helpers import ToolError, format_tool_result, handle_tool_error +from cecli.tools.utils.helpers import ( + ToolError, + coerce_dict_item, + format_tool_result, + handle_tool_error, + normalize_json_array, +) from cecli.tools.utils.output import tool_footer, tool_header +def normalize_task_items(tasks) -> list[dict]: + """Accept tasks as list, dict, JSON string, or list of JSON strings (LLM quirk).""" + normalized = normalize_json_array(tasks, param_name="tasks", allow_empty=True) + result: list[dict] = [] + for item in normalized: + if isinstance(item, dict): + result.append(item) + else: + result.append( + coerce_dict_item(item, param_name="task") + if isinstance(item, str) and item.strip().startswith("{") + else {"task": str(item), "done": False, "current": False} + ) + return result + + +def format_task_lines(tasks) -> tuple[list[str], list[str]]: + """Return (done_tasks, remaining_tasks) display lines for todo items.""" + done_tasks: list[str] = [] + remaining_tasks: list[str] = [] + for task_item in normalize_task_items(tasks): + if task_item.get("done", False): + done_tasks.append(f"✓ {task_item['task']}") + elif task_item.get("current", False): + remaining_tasks.append(f"→ {task_item['task']}") + else: + remaining_tasks.append(f"○ {task_item['task']}") + return done_tasks, remaining_tasks + + class Tool(BaseTool): NORM_NAME = "updatetodolist" SCHEMA = { @@ -71,26 +107,7 @@ def execute(cls, coder, tasks, append=False, change_id=None, dry_run=False, **kw todo_file_path = coder.local_agent_folder("todo.txt") abs_path = coder.abs_root_path(todo_file_path) - # Format tasks into string - done_tasks = [] - remaining_tasks = [] - - for task_item in tasks: - if not isinstance(task_item, dict): - task_item = { - "task": str(task_item), - "done": False, - "current": False, - } - - if task_item.get("done", False): - done_tasks.append(f"✓ {task_item['task']}") - else: - # Check if this is the current task - if task_item.get("current", False): - remaining_tasks.append(f"→ {task_item['task']}") - else: - remaining_tasks.append(f"○ {task_item['task']}") + done_tasks, remaining_tasks = format_task_lines(tasks) # Build formatted content content_lines = [] @@ -199,26 +216,7 @@ def format_output(cls, coder, mcp_server, tool_response): tasks = params.get("tasks", []) if tasks: - # Format tasks for display - done_tasks = [] - remaining_tasks = [] - - for task_item in tasks: - if not isinstance(task_item, dict): - task_item = { - "task": str(task_item), - "done": False, - "current": False, - } - - if task_item.get("done", False): - done_tasks.append(f"✓ {task_item['task']}") - else: - # Check if this is the current task - if task_item.get("current", False): - remaining_tasks.append(f"→ {task_item['task']}") - else: - remaining_tasks.append(f"○ {task_item['task']}") + done_tasks, remaining_tasks = format_task_lines(tasks) # Display formatted todo list if done_tasks: diff --git a/cecli/tools/utils/helpers.py b/cecli/tools/utils/helpers.py index d3c219383bb..974f7104eba 100644 --- a/cecli/tools/utils/helpers.py +++ b/cecli/tools/utils/helpers.py @@ -1,4 +1,5 @@ import difflib +import json import os import re import traceback @@ -338,6 +339,53 @@ def format_tool_result( return result_for_llm +def normalize_json_array(value, *, param_name: str = "items", allow_empty: bool = False) -> list: + """ + Coerce tool args that should be arrays but sometimes arrive as JSON strings. + + Local models occasionally double-encode array parameters as JSON text. + """ + if isinstance(value, str): + text = value.strip() + if not text: + if allow_empty: + return [] + raise ToolError(f"{param_name} array cannot be empty") + try: + value = json.loads(text) + except json.JSONDecodeError as err: + raise ToolError(f"Invalid {param_name} parameter JSON: {err}") from err + + if isinstance(value, dict): + value = [value] + + if not isinstance(value, list): + raise ToolError(f"{param_name} must be an array, got {type(value).__name__}") + + if len(value) == 0 and not allow_empty: + raise ToolError(f"{param_name} array cannot be empty") + + return value + + +def coerce_dict_item(item, *, param_name: str = "item") -> dict: + """Coerce one array element to a dict (object items may also be JSON strings).""" + if isinstance(item, dict): + return item + if isinstance(item, str): + text = item.strip() + if not text: + raise ToolError(f"{param_name} cannot be empty") + try: + parsed = json.loads(text) + except json.JSONDecodeError as err: + raise ToolError(f"Invalid {param_name} JSON: {err}") from err + if isinstance(parsed, dict): + return parsed + raise ToolError(f"Each {param_name} must be an object") + raise ToolError(f"Invalid {param_name} type: {type(item).__name__}") + + # Example usage within a hypothetical tool: # try: # abs_path, rel_path, original_content = validate_file_for_edit(coder, file_path) diff --git a/tests/tools/test_get_lines.py b/tests/tools/test_get_lines.py index 8c7fd7705b3..a3d45f69aea 100644 --- a/tests/tools/test_get_lines.py +++ b/tests/tools/test_get_lines.py @@ -1,10 +1,12 @@ from pathlib import Path from types import SimpleNamespace from unittest.mock import Mock +import json import pytest from cecli.tools import read_range +from cecli.tools.read_range import normalize_show_ops class DummyIO: @@ -25,6 +27,8 @@ def __init__(self, root): self.root = str(root) self.repo = SimpleNamespace(root=str(root)) self.io = DummyIO() + self.pretty = False + self.verbose = False import uuid self.uuid = str(uuid.uuid4()) # Generate unique UUID for each instance @@ -142,3 +146,49 @@ def test_multiline_pattern_search(coder_with_file): assert "Retrieved context for 1 operation(s)" in result coder.io.tool_error.assert_not_called() + + +def test_normalize_show_ops_accepts_json_string(): + ops = normalize_show_ops( + '[{"file_path": "docs/ROADMAP.md", "start_text": "@000", "end_text": "\\n"}]' + ) + assert len(ops) == 1 + assert ops[0]["file_path"] == "docs/ROADMAP.md" + assert ops[0]["start_text"] == "@000" + + +def test_execute_accepts_show_as_json_string(coder_with_file): + coder, _file_path = coder_with_file + show_json = json.dumps( + [{"file_path": "example.txt", "start_text": "beta", "end_text": "beta"}] + ) + + result = read_range.Tool.execute(coder, show=show_json) + + assert "Retrieved context for 1 operation(s)" in result + coder.io.tool_error.assert_not_called() + + +def test_format_output_accepts_show_as_json_string(coder_with_file): + coder, _file_path = coder_with_file + args = json.dumps( + { + "show": ( + '[{"file_path": "example.txt", "start_text": "alpha", "end_text": "gamma"}]' + ) + } + ) + tool_response = SimpleNamespace( + function=SimpleNamespace(name="ReadRange", arguments=args) + ) + + read_range.Tool.format_output( + coder, + mcp_server=SimpleNamespace(name="test"), + tool_response=tool_response, + ) + + output_text = "\n".join(call.args[0] for call in coder.io.tool_output.call_args_list) + assert "example.txt" in output_text + assert "alpha" in output_text + coder.io.tool_error.assert_not_called() diff --git a/tests/tools/test_update_todo_list.py b/tests/tools/test_update_todo_list.py new file mode 100644 index 00000000000..3e4da1ef4c4 --- /dev/null +++ b/tests/tools/test_update_todo_list.py @@ -0,0 +1,78 @@ +import json +from types import SimpleNamespace +from unittest.mock import Mock + +import pytest + +from cecli.tools import update_todo_list +from cecli.tools.update_todo_list import format_task_lines, normalize_task_items +from cecli.tools.utils.helpers import normalize_json_array + + +def test_normalize_json_array_parses_string(): + items = normalize_json_array('[{"task": "a", "done": false}]', param_name="tasks") + assert len(items) == 1 + assert items[0]["task"] == "a" + + +def test_format_task_lines_accepts_json_string(): + tasks_json = json.dumps( + [ + {"task": "Draft roadmap items in docs/ROADMAP.md", "done": False}, + {"task": "Ship fix", "done": True}, + ] + ) + done, remaining = format_task_lines(tasks_json) + assert len(remaining) == 1 + assert "Draft roadmap" in remaining[0] + assert len(done) == 1 + assert "Ship fix" in done[0] + assert all(len(line) > 5 for line in remaining + done) + + +def test_normalize_task_items_does_not_split_characters(): + tasks_json = json.dumps([{"task": "Only one task", "done": False}]) + items = normalize_task_items(tasks_json) + assert len(items) == 1 + assert items[0]["task"] == "Only one task" + + +class DummyIO: + def __init__(self): + self.tool_output = Mock() + self.tool_error = Mock() + self.tool_warning = Mock() + + +class DummyCoder: + def __init__(self): + self.io = DummyIO() + self.pretty = False + self.verbose = False + + +def test_format_output_accepts_tasks_as_json_string(): + coder = DummyCoder() + args = json.dumps( + { + "tasks": ( + '[{"task": "Draft roadmap items", "done": false}, ' + '{"task": "Write tests", "done": true}]' + ) + } + ) + tool_response = SimpleNamespace( + function=SimpleNamespace(name="UpdateTodoList", arguments=args) + ) + + update_todo_list.Tool.format_output( + coder, + mcp_server=SimpleNamespace(name="test"), + tool_response=tool_response, + ) + + output_text = "\n".join(call.args[0] for call in coder.io.tool_output.call_args_list) + assert "Draft roadmap items" in output_text + assert "Write tests" in output_text + assert output_text.count("○ ") <= 2 + coder.io.tool_error.assert_not_called() From 86f47c4846a418785221c7d02392e1076ba425dd Mon Sep 17 00:00:00 2001 From: Jessica Mulein Date: Thu, 28 May 2026 07:37:07 -0700 Subject: [PATCH 2/7] fix(tools): harden todo JSON coercion and format_output errors Mirror ReadRange format_output ToolError handling; fall back to plain task text when brace-prefixed strings are not JSON. Tighten tests and fix import order for pre-commit. Co-authored-by: Cursor --- cecli/tools/update_todo_list.py | 34 +++++++++++++++--------- tests/tools/test_get_lines.py | 2 +- tests/tools/test_update_todo_list.py | 39 +++++++++++++++++++++++++--- 3 files changed, 59 insertions(+), 16 deletions(-) diff --git a/cecli/tools/update_todo_list.py b/cecli/tools/update_todo_list.py index b713a3b953a..70f012cb221 100644 --- a/cecli/tools/update_todo_list.py +++ b/cecli/tools/update_todo_list.py @@ -9,20 +9,25 @@ from cecli.tools.utils.output import tool_footer, tool_header +def coerce_task_item(item) -> dict: + """Coerce one task entry to a dict; JSON strings fall back to plain task text on parse failure.""" + if isinstance(item, dict): + return item + if isinstance(item, str): + text = item.strip() + if text.startswith("{"): + try: + return coerce_dict_item(item, param_name="task") + except ToolError: + pass + return {"task": str(item), "done": False, "current": False} + return {"task": str(item), "done": False, "current": False} + + def normalize_task_items(tasks) -> list[dict]: """Accept tasks as list, dict, JSON string, or list of JSON strings (LLM quirk).""" normalized = normalize_json_array(tasks, param_name="tasks", allow_empty=True) - result: list[dict] = [] - for item in normalized: - if isinstance(item, dict): - result.append(item) - else: - result.append( - coerce_dict_item(item, param_name="task") - if isinstance(item, str) and item.strip().startswith("{") - else {"task": str(item), "done": False, "current": False} - ) - return result + return [coerce_task_item(item) for item in normalized] def format_task_lines(tasks) -> tuple[list[str], list[str]]: @@ -216,7 +221,12 @@ def format_output(cls, coder, mcp_server, tool_response): tasks = params.get("tasks", []) if tasks: - done_tasks, remaining_tasks = format_task_lines(tasks) + try: + done_tasks, remaining_tasks = format_task_lines(tasks) + except ToolError as err: + coder.io.tool_error(str(err)) + tool_footer(coder=coder, tool_response=tool_response) + return # Display formatted todo list if done_tasks: diff --git a/tests/tools/test_get_lines.py b/tests/tools/test_get_lines.py index a3d45f69aea..a49663f801c 100644 --- a/tests/tools/test_get_lines.py +++ b/tests/tools/test_get_lines.py @@ -1,7 +1,7 @@ +import json from pathlib import Path from types import SimpleNamespace from unittest.mock import Mock -import json import pytest diff --git a/tests/tools/test_update_todo_list.py b/tests/tools/test_update_todo_list.py index 3e4da1ef4c4..44e8ada48c8 100644 --- a/tests/tools/test_update_todo_list.py +++ b/tests/tools/test_update_todo_list.py @@ -5,8 +5,12 @@ import pytest from cecli.tools import update_todo_list -from cecli.tools.update_todo_list import format_task_lines, normalize_task_items -from cecli.tools.utils.helpers import normalize_json_array +from cecli.tools.update_todo_list import ( + coerce_task_item, + format_task_lines, + normalize_task_items, +) +from cecli.tools.utils.helpers import ToolError, normalize_json_array def test_normalize_json_array_parses_string(): @@ -15,6 +19,11 @@ def test_normalize_json_array_parses_string(): assert items[0]["task"] == "a" +def test_normalize_json_array_rejects_invalid_json(): + with pytest.raises(ToolError, match="Invalid tasks parameter JSON"): + normalize_json_array("not json", param_name="tasks") + + def test_format_task_lines_accepts_json_string(): tasks_json = json.dumps( [ @@ -37,6 +46,11 @@ def test_normalize_task_items_does_not_split_characters(): assert items[0]["task"] == "Only one task" +def test_coerce_task_item_plain_string_starting_with_brace(): + item = coerce_task_item("{not valid json") + assert item == {"task": "{not valid json", "done": False, "current": False} + + class DummyIO: def __init__(self): self.tool_output = Mock() @@ -74,5 +88,24 @@ def test_format_output_accepts_tasks_as_json_string(): output_text = "\n".join(call.args[0] for call in coder.io.tool_output.call_args_list) assert "Draft roadmap items" in output_text assert "Write tests" in output_text - assert output_text.count("○ ") <= 2 + assert output_text.count("○ ") == 1 + assert "○ Draft roadmap items" in output_text + assert "✓ Write tests" in output_text coder.io.tool_error.assert_not_called() + + +def test_format_output_reports_invalid_tasks_json(): + coder = DummyCoder() + args = json.dumps({"tasks": "not json"}) + tool_response = SimpleNamespace( + function=SimpleNamespace(name="UpdateTodoList", arguments=args) + ) + + update_todo_list.Tool.format_output( + coder, + mcp_server=SimpleNamespace(name="test"), + tool_response=tool_response, + ) + + coder.io.tool_error.assert_called_once() + assert "Invalid tasks parameter JSON" in coder.io.tool_error.call_args.args[0] From 41d80862c612a3dc63d705dc12e0cadffaf0111b Mon Sep 17 00:00:00 2001 From: Jessica Mulein Date: Thu, 28 May 2026 08:15:40 -0700 Subject: [PATCH 3/7] chore: apply black preview formatting and harden format_output Match CI pre-commit (black --line-length 100 --preview). Mirror ReadRange Invalid Tool JSON handling in UpdateTodoList.format_output; add tests for mixed JSON-string task rows and malformed tool arguments. Co-authored-by: Cursor --- cecli/tools/read_range.py | 17 +++++++++++--- cecli/tools/update_todo_list.py | 18 ++++++++++---- cecli/tools/utils/helpers.py | 9 ++++++- tests/tools/test_get_lines.py | 14 +++-------- tests/tools/test_update_todo_list.py | 35 +++++++++++++++++++++++----- 5 files changed, 67 insertions(+), 26 deletions(-) diff --git a/cecli/tools/read_range.py b/cecli/tools/read_range.py index 954eebac9d3..8e429251621 100644 --- a/cecli/tools/read_range.py +++ b/cecli/tools/read_range.py @@ -330,12 +330,19 @@ def execute(cls, coder, show, **kwargs): # first, falling back to 20 equally-spaced lines for non-code files if (start_text == "@000" or end_text == "000@") and (e_idx - s_idx > 200): preview = cls._get_range_preview( - abs_path, coder.io, start_idx=s_idx, end_idx=e_idx, line_numbers=True + abs_path, + coder.io, + start_idx=s_idx, + end_idx=e_idx, + line_numbers=True, ) if show_index > 0: all_outputs.append("") all_outputs.append(preview) - cls._last_invocation[abs_path] = {"start_idx": s_idx, "end_idx": e_idx} + cls._last_invocation[abs_path] = { + "start_idx": s_idx, + "end_idx": e_idx, + } continue # Store the found indices for future disambiguation @@ -661,7 +668,11 @@ def _get_range_preview(cls, abs_path, io, start_idx, end_idx, line_numbers=True) from cecli.repomap import RepoMap stub = RepoMap.get_file_stub( - abs_path, io, start_line=start_idx, end_line=end_idx, line_numbers=line_numbers + abs_path, + io, + start_line=start_idx, + end_line=end_idx, + line_numbers=line_numbers, ) # If get_file_stub returned a useful structural outline, wrap it with headers diff --git a/cecli/tools/update_todo_list.py b/cecli/tools/update_todo_list.py index 70f012cb221..9f7fc399b6c 100644 --- a/cecli/tools/update_todo_list.py +++ b/cecli/tools/update_todo_list.py @@ -1,3 +1,5 @@ +import json + from cecli.tools.utils.base_tool import BaseTool from cecli.tools.utils.helpers import ( ToolError, @@ -59,7 +61,10 @@ class Tool(BaseTool): "items": { "type": "object", "properties": { - "task": {"type": "string", "description": "The task description."}, + "task": { + "type": "string", + "description": "The task description.", + }, "done": { "type": "boolean", "description": "Whether the task is completed.", @@ -208,16 +213,19 @@ def execute(cls, coder, tasks, append=False, change_id=None, dry_run=False, **kw @classmethod def format_output(cls, coder, mcp_server, tool_response): - import json - from cecli.tools.utils.output import color_markers color_start, color_end = color_markers(coder) tool_header(coder=coder, mcp_server=mcp_server, tool_response=tool_response) - # Parse the parameters to display formatted todo list - params = json.loads(tool_response.function.arguments) + try: + params = json.loads(tool_response.function.arguments) + except json.JSONDecodeError: + coder.io.tool_error("Invalid Tool JSON") + tool_footer(coder=coder, tool_response=tool_response) + return + tasks = params.get("tasks", []) if tasks: diff --git a/cecli/tools/utils/helpers.py b/cecli/tools/utils/helpers.py index 974f7104eba..0779c1b5195 100644 --- a/cecli/tools/utils/helpers.py +++ b/cecli/tools/utils/helpers.py @@ -273,7 +273,14 @@ def parse_arg_as_list(arg): def apply_change( - coder, abs_path, rel_path, original_content, new_content, change_type, metadata, change_id=None + coder, + abs_path, + rel_path, + original_content, + new_content, + change_type, + metadata, + change_id=None, ): """ Writes the new content, tracks the change, and updates coder state. diff --git a/tests/tools/test_get_lines.py b/tests/tools/test_get_lines.py index a49663f801c..dee5517cb32 100644 --- a/tests/tools/test_get_lines.py +++ b/tests/tools/test_get_lines.py @@ -159,9 +159,7 @@ def test_normalize_show_ops_accepts_json_string(): def test_execute_accepts_show_as_json_string(coder_with_file): coder, _file_path = coder_with_file - show_json = json.dumps( - [{"file_path": "example.txt", "start_text": "beta", "end_text": "beta"}] - ) + show_json = json.dumps([{"file_path": "example.txt", "start_text": "beta", "end_text": "beta"}]) result = read_range.Tool.execute(coder, show=show_json) @@ -172,15 +170,9 @@ def test_execute_accepts_show_as_json_string(coder_with_file): def test_format_output_accepts_show_as_json_string(coder_with_file): coder, _file_path = coder_with_file args = json.dumps( - { - "show": ( - '[{"file_path": "example.txt", "start_text": "alpha", "end_text": "gamma"}]' - ) - } - ) - tool_response = SimpleNamespace( - function=SimpleNamespace(name="ReadRange", arguments=args) + {"show": '[{"file_path": "example.txt", "start_text": "alpha", "end_text": "gamma"}]'} ) + tool_response = SimpleNamespace(function=SimpleNamespace(name="ReadRange", arguments=args)) read_range.Tool.format_output( coder, diff --git a/tests/tools/test_update_todo_list.py b/tests/tools/test_update_todo_list.py index 44e8ada48c8..d7356dd2e9a 100644 --- a/tests/tools/test_update_todo_list.py +++ b/tests/tools/test_update_todo_list.py @@ -39,6 +39,17 @@ def test_format_task_lines_accepts_json_string(): assert all(len(line) > 5 for line in remaining + done) +def test_normalize_task_items_accepts_list_of_json_strings(): + tasks = [ + '{"task": "First", "done": false}', + {"task": "Second", "done": True}, + ] + items = normalize_task_items(tasks) + assert len(items) == 2 + assert items[0]["task"] == "First" + assert items[1]["task"] == "Second" + + def test_normalize_task_items_does_not_split_characters(): tasks_json = json.dumps([{"task": "Only one task", "done": False}]) items = normalize_task_items(tasks_json) @@ -75,9 +86,7 @@ def test_format_output_accepts_tasks_as_json_string(): ) } ) - tool_response = SimpleNamespace( - function=SimpleNamespace(name="UpdateTodoList", arguments=args) - ) + tool_response = SimpleNamespace(function=SimpleNamespace(name="UpdateTodoList", arguments=args)) update_todo_list.Tool.format_output( coder, @@ -94,11 +103,10 @@ def test_format_output_accepts_tasks_as_json_string(): coder.io.tool_error.assert_not_called() -def test_format_output_reports_invalid_tasks_json(): +def test_format_output_reports_invalid_tool_arguments_json(): coder = DummyCoder() - args = json.dumps({"tasks": "not json"}) tool_response = SimpleNamespace( - function=SimpleNamespace(name="UpdateTodoList", arguments=args) + function=SimpleNamespace(name="UpdateTodoList", arguments="not json") ) update_todo_list.Tool.format_output( @@ -107,5 +115,20 @@ def test_format_output_reports_invalid_tasks_json(): tool_response=tool_response, ) + coder.io.tool_error.assert_called_once() + assert coder.io.tool_error.call_args.args[0] == "Invalid Tool JSON" + + +def test_format_output_reports_invalid_tasks_json(): + coder = DummyCoder() + args = json.dumps({"tasks": "not json"}) + tool_response = SimpleNamespace(function=SimpleNamespace(name="UpdateTodoList", arguments=args)) + + update_todo_list.Tool.format_output( + coder, + mcp_server=SimpleNamespace(name="test"), + tool_response=tool_response, + ) + coder.io.tool_error.assert_called_once() assert "Invalid tasks parameter JSON" in coder.io.tool_error.call_args.args[0] From 5c3b41a1e04f8de7b04be21d956fc78910643be0 Mon Sep 17 00:00:00 2001 From: Jessica Mulein Date: Thu, 28 May 2026 09:14:44 -0700 Subject: [PATCH 4/7] fix(tools): join char-split JSON arrays from local LLMs Some models emit tool array args as one string per character (e.g. UpdateTodoList tasks). Rejoin and parse before normalize_task_items / ReadRange show ops. Co-authored-by: Cursor --- cecli/tools/utils/helpers.py | 36 +++++++++++++++++- tests/tools/test_get_lines.py | 10 +++++ tests/tools/test_update_todo_list.py | 55 ++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 1 deletion(-) diff --git a/cecli/tools/utils/helpers.py b/cecli/tools/utils/helpers.py index 0779c1b5195..f990a0cb44f 100644 --- a/cecli/tools/utils/helpers.py +++ b/cecli/tools/utils/helpers.py @@ -346,12 +346,46 @@ def format_tool_result( return result_for_llm +def _try_join_char_split_json_array(items: list) -> list | None: + """ + Some local models emit a JSON array as one string per character in tool args. + + Example: tasks=["[", "{", "\\"", "t", "a", "s", "k", "\\"", ...] instead of + tasks='[{"task": "...", "done": false}]'. + """ + if len(items) < 8: + return None + if not all(isinstance(x, str) for x in items): + return None + joined = "".join(items).strip() + if not joined.startswith(("[", "{")): + return None + try: + parsed = json.loads(joined) + except json.JSONDecodeError: + return None + if isinstance(parsed, dict): + return [parsed] + if isinstance(parsed, list): + return parsed + return None + + def normalize_json_array(value, *, param_name: str = "items", allow_empty: bool = False) -> list: """ Coerce tool args that should be arrays but sometimes arrive as JSON strings. - Local models occasionally double-encode array parameters as JSON text. + Local models occasionally double-encode array parameters as JSON text, or emit + arrays as per-character string lists (see ``_try_join_char_split_json_array``). """ + if isinstance(value, list): + coerced = _try_join_char_split_json_array(value) + if coerced is not None: + value = coerced + elif len(value) == 1 and isinstance(value[0], str): + # Single element wrapping the whole JSON array/object as a string. + value = value[0] + if isinstance(value, str): text = value.strip() if not text: diff --git a/tests/tools/test_get_lines.py b/tests/tools/test_get_lines.py index dee5517cb32..55aaf0e619e 100644 --- a/tests/tools/test_get_lines.py +++ b/tests/tools/test_get_lines.py @@ -184,3 +184,13 @@ def test_format_output_accepts_show_as_json_string(coder_with_file): assert "example.txt" in output_text assert "alpha" in output_text coder.io.tool_error.assert_not_called() + + +def test_normalize_show_ops_joins_char_split_json_list(): + show_json = json.dumps( + [{"file_path": "docs/ROADMAP.md", "start_text": "@000", "end_text": "\\n"}] + ) + ops = normalize_show_ops(list(show_json)) + assert len(ops) == 1 + assert ops[0]["file_path"] == "docs/ROADMAP.md" + assert ops[0]["start_text"] == "@000" diff --git a/tests/tools/test_update_todo_list.py b/tests/tools/test_update_todo_list.py index d7356dd2e9a..8769b4783ff 100644 --- a/tests/tools/test_update_todo_list.py +++ b/tests/tools/test_update_todo_list.py @@ -50,6 +50,35 @@ def test_normalize_task_items_accepts_list_of_json_strings(): assert items[1]["task"] == "Second" +def test_normalize_json_array_joins_char_split_json_list(): + """Ollama sometimes sends tasks as a list of single-character strings.""" + chars = list( + '[{"task": "Explore the codebase", "done": false, "current": true},' + '{"task": "Draft roadmap", "done": false}]' + ) + items = normalize_json_array(chars, param_name="tasks") + assert len(items) == 2 + assert items[0]["task"] == "Explore the codebase" + assert items[1]["task"] == "Draft roadmap" + + +def test_normalize_json_array_unwraps_single_element_json_string_list(): + wrapped = [ + '[{"task": "Only task", "done": false}]', + ] + items = normalize_json_array(wrapped, param_name="tasks") + assert len(items) == 1 + assert items[0]["task"] == "Only task" + + +def test_normalize_task_items_from_char_split_list(): + chars = list(json.dumps([{"task": "Ship tests", "done": True}])) + items = normalize_task_items(chars) + assert len(items) == 1 + assert items[0]["task"] == "Ship tests" + assert items[0]["done"] is True + + def test_normalize_task_items_does_not_split_characters(): tasks_json = json.dumps([{"task": "Only one task", "done": False}]) items = normalize_task_items(tasks_json) @@ -103,6 +132,32 @@ def test_format_output_accepts_tasks_as_json_string(): coder.io.tool_error.assert_not_called() +def test_format_output_accepts_char_split_tasks_list(): + """Reproduces BrightVision bug: tasks array is one JSON character per element.""" + coder = DummyCoder() + tasks_json = ( + '[{"task": "Explore the codebase", "done": false, "current": true},' + '{"task": "Draft roadmap items", "done": false}]' + ) + args = json.dumps({"tasks": list(tasks_json)}) + tool_response = SimpleNamespace(function=SimpleNamespace(name="UpdateTodoList", arguments=args)) + + update_todo_list.Tool.format_output( + coder, + mcp_server=SimpleNamespace(name="test"), + tool_response=tool_response, + ) + + output_text = "\n".join(call.args[0] for call in coder.io.tool_output.call_args_list) + assert "Explore the codebase" in output_text + assert "Draft roadmap items" in output_text + assert output_text.count("○ ") == 1 + assert "→ Explore the codebase" in output_text + assert "○ Draft roadmap items" in output_text + assert all(len(line.strip()) > 3 for line in output_text.splitlines() if line.startswith("○ ")) + coder.io.tool_error.assert_not_called() + + def test_format_output_reports_invalid_tool_arguments_json(): coder = DummyCoder() tool_response = SimpleNamespace( From 3dcb9e99e769694c6a31afc85cf9b209a5fe883b Mon Sep 17 00:00:00 2001 From: Jessica Mulein Date: Thu, 28 May 2026 10:49:54 -0700 Subject: [PATCH 5/7] fix(tools): Grep search coercion and newline-broken tool JSON - normalize_search_operations for char-split and bare pattern strings - _repair_local_model_json_text / _try_parse_json_value for ReadRange show and other double-encoded arrays (literal newline after ':') - Tests: test_grep.py, test_get_lines.py, test_update_todo_list.py Co-authored-by: Cursor --- cecli/tools/grep.py | 21 +++++--- cecli/tools/utils/helpers.py | 81 +++++++++++++++++++++++++--- tests/tools/test_get_lines.py | 36 +++++++++++++ tests/tools/test_grep.py | 54 +++++++++++++++++++ tests/tools/test_update_todo_list.py | 10 ++++ 5 files changed, 188 insertions(+), 14 deletions(-) diff --git a/cecli/tools/grep.py b/cecli/tools/grep.py index 03f51d57275..d1c7e8b979a 100644 --- a/cecli/tools/grep.py +++ b/cecli/tools/grep.py @@ -7,6 +7,7 @@ from cecli.helpers.hashline import strip_hashline from cecli.run_cmd import run_cmd_subprocess from cecli.tools.utils.base_tool import BaseTool +from cecli.tools.utils.helpers import ToolError, normalize_search_operations from cecli.tools.utils.output import color_markers, tool_footer, tool_header @@ -93,9 +94,11 @@ def execute( Search for lines matching patterns in files within the project repository. Uses rg (ripgrep), ag (the silver searcher), or grep, whichever is available. """ - if not isinstance(searches, list): - # Handle legacy single-search call if necessary, or just error - return "Error: 'searches' parameter must be an array." + try: + searches = normalize_search_operations(searches) + except ToolError as err: + coder.io.tool_error(str(err)) + return f"Error: {err}" repo = coder.repo if not repo: @@ -109,7 +112,7 @@ def execute( all_results = [] for search_op in searches: - pattern = strip_hashline(search_op.get("pattern")) + pattern = strip_hashline(str(search_op.get("pattern") or "")) file_pattern = search_op.get("file_pattern", "*") directory = search_op.get("directory", search_op.get("path", ".")) use_regex = search_op.get("use_regex", False) @@ -244,11 +247,17 @@ def format_output(cls, coder, mcp_server, tool_response): tool_header(coder=coder, mcp_server=mcp_server, tool_response=tool_response) # Output each search operation with the requested format - searches = params.get("searches", []) + try: + searches = normalize_search_operations(params.get("searches", [])) + except ToolError as err: + coder.io.tool_error(str(err)) + tool_footer(coder=coder, mcp_server=mcp_server, tool_response=tool_response) + return + if searches: coder.io.tool_output("") for i, search_op in enumerate(searches): - pattern = search_op.get("pattern", "") + pattern = str(search_op.get("pattern") or "") file_pattern = search_op.get("file_pattern", "*") directory = search_op.get("directory", ".") use_regex = search_op.get("use_regex", False) diff --git a/cecli/tools/utils/helpers.py b/cecli/tools/utils/helpers.py index f990a0cb44f..3a18bf4e325 100644 --- a/cecli/tools/utils/helpers.py +++ b/cecli/tools/utils/helpers.py @@ -346,6 +346,35 @@ def format_tool_result( return result_for_llm +def _repair_local_model_json_text(text: str) -> str: + """ + Repair common local-model breakage in double-encoded tool JSON. + + Models sometimes emit a literal newline between ``:`` and the opening quote + of a string value (e.g. ``"end_text":\\n",`` instead of ``"end_text": "",``). + """ + repaired = re.sub(r':\s*\n\s*",', ': "",', text) + repaired = re.sub(r':\s*\n\s*"}', ': ""}', repaired) + return repaired + + +def _try_parse_json_value(text: str): + """Parse JSON text, including repairs for common local-model tool-arg quirks.""" + text = text.strip() + if not text: + return None + for candidate in (text, _repair_local_model_json_text(text)): + try: + return json.loads(candidate) + except json.JSONDecodeError: + continue + if len(text) >= 8: + coerced = _try_join_char_split_json_array(list(text)) + if coerced is not None: + return coerced + return None + + def _try_join_char_split_json_array(items: list) -> list | None: """ Some local models emit a JSON array as one string per character in tool args. @@ -392,10 +421,13 @@ def normalize_json_array(value, *, param_name: str = "items", allow_empty: bool if allow_empty: return [] raise ToolError(f"{param_name} array cannot be empty") - try: - value = json.loads(text) - except json.JSONDecodeError as err: - raise ToolError(f"Invalid {param_name} parameter JSON: {err}") from err + parsed = _try_parse_json_value(text) + if parsed is None: + try: + parsed = json.loads(text) + except json.JSONDecodeError as err: + raise ToolError(f"Invalid {param_name} parameter JSON: {err}") from err + value = parsed if isinstance(value, dict): value = [value] @@ -417,16 +449,49 @@ def coerce_dict_item(item, *, param_name: str = "item") -> dict: text = item.strip() if not text: raise ToolError(f"{param_name} cannot be empty") - try: - parsed = json.loads(text) - except json.JSONDecodeError as err: - raise ToolError(f"Invalid {param_name} JSON: {err}") from err + parsed = _try_parse_json_value(text) + if parsed is None: + try: + parsed = json.loads(text) + except json.JSONDecodeError as err: + raise ToolError(f"Invalid {param_name} JSON: {err}") from err if isinstance(parsed, dict): return parsed raise ToolError(f"Each {param_name} must be an object") raise ToolError(f"Invalid {param_name} type: {type(item).__name__}") +def normalize_search_operations(searches, *, param_name: str = "searches") -> list[dict]: + """ + Coerce Grep ``searches`` arrays from local-model quirks (JSON strings, char-split). + + Each element becomes a dict with a non-empty ``pattern``. + """ + if isinstance(searches, list) and len(searches) == 1 and isinstance(searches[0], str): + lone = searches[0].strip() + if lone and not lone.startswith(("[", "{")): + searches = [{"pattern": lone}] + raw = normalize_json_array(searches, param_name=param_name, allow_empty=False) + out: list[dict] = [] + for i, item in enumerate(raw): + label = f"{param_name}[{i}]" + if isinstance(item, str): + text = item.strip() + if not text: + raise ToolError(f"{label} cannot be empty") + if text.startswith("{"): + item = coerce_dict_item(text, param_name=label) + else: + out.append({"pattern": text}) + continue + op = coerce_dict_item(item, param_name=label) + pattern = op.get("pattern") + if pattern is None or (isinstance(pattern, str) and not pattern.strip()): + raise ToolError(f"{label} missing pattern") + out.append(op) + return out + + # Example usage within a hypothetical tool: # try: # abs_path, rel_path, original_content = validate_file_for_edit(coder, file_path) diff --git a/tests/tools/test_get_lines.py b/tests/tools/test_get_lines.py index 55aaf0e619e..79239d396ce 100644 --- a/tests/tools/test_get_lines.py +++ b/tests/tools/test_get_lines.py @@ -194,3 +194,39 @@ def test_normalize_show_ops_joins_char_split_json_list(): assert len(ops) == 1 assert ops[0]["file_path"] == "docs/ROADMAP.md" assert ops[0]["start_text"] == "@000" + + +def _broken_readrange_show_json(*, file_path: str, start_text: str) -> str: + """BrightVision dogfood: ReadRange show double-encoded with a newline after ':' .""" + return ( + f'[{{"end_text":\n' + f'", "file_path": "{file_path}", "start_text": "{start_text}"}}]' + ) + + +def test_normalize_show_ops_repairs_literal_newline_after_colon(): + broken = _broken_readrange_show_json(file_path="docs/ROADMAP.md", start_text="@000") + ops = normalize_show_ops(broken) + assert len(ops) == 1 + assert ops[0]["file_path"] == "docs/ROADMAP.md" + assert ops[0]["start_text"] == "@000" + assert ops[0]["end_text"] == "" + + +def test_format_output_repairs_broken_show_json_string(coder_with_file): + """format_output must not fail JSON coercion when show is a broken JSON string.""" + coder, _file_path = coder_with_file + broken_show = _broken_readrange_show_json(file_path="example.txt", start_text="alpha") + args = json.dumps({"show": broken_show}) + tool_response = SimpleNamespace(function=SimpleNamespace(name="ReadRange", arguments=args)) + + read_range.Tool.format_output( + coder, + mcp_server=SimpleNamespace(name="test"), + tool_response=tool_response, + ) + + output_text = "\n".join(call.args[0] for call in coder.io.tool_output.call_args_list) + assert "example.txt" in output_text + assert "alpha" in output_text + coder.io.tool_error.assert_not_called() diff --git a/tests/tools/test_grep.py b/tests/tools/test_grep.py index f6d8278a4b8..40b014bf62c 100644 --- a/tests/tools/test_grep.py +++ b/tests/tools/test_grep.py @@ -54,3 +54,57 @@ def test_dash_prefixed_pattern_is_searched_literally(search_term, tmp_path, monk assert "Matches for" in result assert search_term in result coder.io.tool_error.assert_not_called() + + +def test_normalize_char_split_searches_array(tmp_path, monkeypatch): + sample = tmp_path / "example.txt" + sample.write_text("hello world\n") + + coder = SimpleNamespace( + repo=SimpleNamespace(root=str(tmp_path)), + io=SimpleNamespace( + tool_error=Mock(), + tool_output=Mock(), + tool_warning=Mock(), + ), + verbose=False, + root=str(tmp_path), + tui=lambda: None, + ) + + monkeypatch.setattr(grep.Tool, "_find_search_tool", lambda: ("rg", shutil.which("rg"))) + if shutil.which("rg") is None: + pytest.skip("rg is required") + + char_split = list('{"pattern": "hello", "file_pattern": "*.txt"}') + + result = grep.Tool.execute(coder, searches=char_split) + + assert "hello" in result + coder.io.tool_error.assert_not_called() + + +def test_bare_string_search_pattern(tmp_path, monkeypatch): + sample = tmp_path / "example.txt" + sample.write_text("findme here\n") + + coder = SimpleNamespace( + repo=SimpleNamespace(root=str(tmp_path)), + io=SimpleNamespace( + tool_error=Mock(), + tool_output=Mock(), + tool_warning=Mock(), + ), + verbose=False, + root=str(tmp_path), + tui=lambda: None, + ) + + monkeypatch.setattr(grep.Tool, "_find_search_tool", lambda: ("rg", shutil.which("rg"))) + if shutil.which("rg") is None: + pytest.skip("rg is required") + + result = grep.Tool.execute(coder, searches=["findme"]) + + assert "findme" in result + coder.io.tool_error.assert_not_called() diff --git a/tests/tools/test_update_todo_list.py b/tests/tools/test_update_todo_list.py index 8769b4783ff..ffef9d45e31 100644 --- a/tests/tools/test_update_todo_list.py +++ b/tests/tools/test_update_todo_list.py @@ -71,6 +71,16 @@ def test_normalize_json_array_unwraps_single_element_json_string_list(): assert items[0]["task"] == "Only task" +def test_normalize_json_array_repairs_literal_newline_after_colon(): + """ReadRange/Grep: local models break JSON with a newline between ':' and '\"'.""" + broken = '[{"end_text":\n", "file_path": "docs/ROADMAP.md", "start_text": "@000"}]' + items = normalize_json_array(broken, param_name="show") + assert len(items) == 1 + assert items[0]["file_path"] == "docs/ROADMAP.md" + assert items[0]["start_text"] == "@000" + assert items[0]["end_text"] == "" + + def test_normalize_task_items_from_char_split_list(): chars = list(json.dumps([{"task": "Ship tests", "done": True}])) items = normalize_task_items(chars) From 8190f0d000c5c54afe215a7335e094df13ab3d96 Mon Sep 17 00:00:00 2001 From: Jessica Mulein Date: Thu, 28 May 2026 11:36:45 -0700 Subject: [PATCH 6/7] Repomap path resolve, grep hints, JSON repair; quiet session load --- cecli/repomap.py | 26 +++++++++++++++++++----- cecli/sessions.py | 8 +++++--- cecli/tools/grep.py | 8 ++++++-- cecli/tools/utils/helpers.py | 30 ++++++++++++++++++++++++++++ tests/basic/test_repomap.py | 14 +++++++++++++ tests/basic/test_sessions.py | 15 ++++++++++++++ tests/tools/test_grep.py | 11 ++++++++++ tests/tools/test_update_todo_list.py | 8 ++++++++ 8 files changed, 110 insertions(+), 10 deletions(-) diff --git a/cecli/repomap.py b/cecli/repomap.py index 8c0f379d21c..0afc009042d 100644 --- a/cecli/repomap.py +++ b/cecli/repomap.py @@ -350,6 +350,12 @@ def get_repo_map( "has_chat_files": bool(chat_files), } + def _resolve_abs_fname(self, fname: str) -> str: + """Normalize repo file paths for existence checks and tag parsing.""" + if os.path.isabs(fname): + return os.path.normpath(fname) + return os.path.normpath(os.path.join(self.root, fname)) + def get_rel_fname(self, fname): try: return os.path.relpath(fname, self.root) @@ -746,6 +752,7 @@ def get_ranked_tags( num_fnames = len(fnames) fname_index = 0 + skipped_missing = 0 for fname in fnames: if self.verbose: self.io.tool_output(f"Processing {fname}") @@ -756,20 +763,24 @@ def get_ranked_tags( else: self.io.update_spinner(f"{UPDATING_REPO_MAP_MESSAGE}: {fname}") + abs_fname = self._resolve_abs_fname(fname) try: - file_ok = os.path.isfile(fname) + file_ok = os.path.isfile(abs_fname) except OSError: file_ok = False if not file_ok: + skipped_missing += 1 if fname not in self.warned_files: - self.io.tool_warning(f"Repo-map can't include {fname}") - self.io.tool_output( - "Has it been deleted from the file system but not from git?" - ) self.warned_files.add(fname) + if skipped_missing <= 2: + self.io.tool_warning( + f"Repo-map skipping missing file: {abs_fname}" + " (removed on disk or not yet written)." + ) continue + fname = abs_fname # dump(fname) rel_fname = self.get_rel_fname(fname) current_pers = 0.0 # Start with 0 personalization score @@ -843,6 +854,11 @@ def get_ranked_tags( if tag.specific_kind == "import": file_imports[rel_fname].add(tag.name) + if skipped_missing > 2: + self.io.tool_output( + f"Repo-map skipped {skipped_missing} paths that are not readable on disk." + ) + self.io.profile("Process Files") if self.use_enhanced_map and len(file_imports) > 0: diff --git a/cecli/sessions.py b/cecli/sessions.py index f1ee5a12570..2939431575c 100644 --- a/cecli/sessions.py +++ b/cecli/sessions.py @@ -88,7 +88,7 @@ def list_sessions(self) -> List[Dict]: return sessions - async def load_session(self, session_identifier: str, switch=True) -> bool: + async def load_session(self, session_identifier: str, switch=True, quiet: bool = False) -> bool: """Load a saved session by name or file path.""" if not session_identifier: self.io.tool_error("Please provide a session name or file path.") @@ -103,12 +103,14 @@ async def load_session(self, session_identifier: str, switch=True) -> bool: with open(session_file, "r", encoding="utf-8") as f: session_data = json.load(f) except Exception as e: - self.io.tool_error(f"Error loading session: {e}") + if not quiet: + self.io.tool_error(f"Error loading session: {e}") return False # Verify session format if not isinstance(session_data, dict) or "version" not in session_data: - self.io.tool_error("Invalid session format.") + if not quiet: + self.io.tool_error("Invalid session format.") return False # Apply session data diff --git a/cecli/tools/grep.py b/cecli/tools/grep.py index d1c7e8b979a..07a50de2ac0 100644 --- a/cecli/tools/grep.py +++ b/cecli/tools/grep.py @@ -7,7 +7,7 @@ from cecli.helpers.hashline import strip_hashline from cecli.run_cmd import run_cmd_subprocess from cecli.tools.utils.base_tool import BaseTool -from cecli.tools.utils.helpers import ToolError, normalize_search_operations +from cecli.tools.utils.helpers import ToolError, grep_error_hint, normalize_search_operations from cecli.tools.utils.output import color_markers, tool_footer, tool_header @@ -202,7 +202,11 @@ def execute( elif exit_status == 1: all_results.append(f"No matches found for '{pattern}'.") else: - all_results.append(f"Error searching for '{pattern}': {output_content}") + msg = f"Error searching for '{pattern}': {output_content}" + hint = grep_error_hint(pattern, output_content) + if hint: + msg = f"{msg.rstrip()}{hint}" + all_results.append(msg) except Exception as e: all_results.append(f"Error executing search for '{pattern}': {str(e)}") diff --git a/cecli/tools/utils/helpers.py b/cecli/tools/utils/helpers.py index 3a18bf4e325..914eb834e5b 100644 --- a/cecli/tools/utils/helpers.py +++ b/cecli/tools/utils/helpers.py @@ -368,6 +368,25 @@ def _try_parse_json_value(text: str): return json.loads(candidate) except json.JSONDecodeError: continue + if "}{" in text: + from cecli import utils as cecli_utils + + chunks = cecli_utils.split_concatenated_json(text) + if len(chunks) == 1: + try: + return json.loads(chunks[0]) + except json.JSONDecodeError: + pass + elif len(chunks) > 1: + parsed = [] + for chunk in chunks: + try: + parsed.append(json.loads(chunk)) + except json.JSONDecodeError: + parsed = None + break + if parsed is not None: + return parsed if len(text) >= 8: coerced = _try_join_char_split_json_array(list(text)) if coerced is not None: @@ -375,6 +394,17 @@ def _try_parse_json_value(text: str): return None +def grep_error_hint(pattern: str, grep_output: str) -> str: + """Actionable hint when BSD grep rejects a regex pattern.""" + combined = f"{pattern}\n{grep_output}" + if "(?" in pattern or "repetition-operator operand invalid" in combined: + return ( + " Hint: BSD grep does not support PCRE lookahead (e.g. `(?!…)`). " + "Use a simpler pattern, set use_regex=false, or install ripgrep (rg)." + ) + return "" + + def _try_join_char_split_json_array(items: list) -> list | None: """ Some local models emit a JSON array as one string per character in tool args. diff --git a/tests/basic/test_repomap.py b/tests/basic/test_repomap.py index cae2c122ad0..53079604a04 100644 --- a/tests/basic/test_repomap.py +++ b/tests/basic/test_repomap.py @@ -50,6 +50,20 @@ def test_get_repo_map(self): # close the open cache files, so Windows won't error del repo_map + def test_repomap_resolves_relative_paths(self): + """Relative paths from git status must resolve against repo_root.""" + with IgnorantTemporaryDirectory() as temp_dir: + py_file = os.path.join(temp_dir, "module.py") + with open(py_file, "w", encoding="utf-8") as f: + f.write("def helper():\n return 1\n") + + io = InputOutput() + repo_map = RepoMap(main_model=self.GPT35, io=io, repo_root=temp_dir) + ranked = repo_map.get_ranked_tags([], ["module.py"], set(), set(), progress=False) + assert ranked is not None + assert len(ranked) > 0 + del repo_map + def test_repo_map_refresh_files(self): with GitTemporaryDirectory() as temp_dir: repo = git.Repo(temp_dir, odbt=git.GitCmdObjectDB) diff --git a/tests/basic/test_sessions.py b/tests/basic/test_sessions.py index c6611e12909..4248c98b82b 100644 --- a/tests/basic/test_sessions.py +++ b/tests/basic/test_sessions.py @@ -57,6 +57,21 @@ def session_manager(mock_coder): return SessionManager(mock_coder, mock_coder.io) +@pytest.mark.asyncio +async def test_load_session_quiet_skips_tool_error_on_invalid_json( + session_manager, mock_coder, tmp_path +): + """BrightVision auto-load uses quiet=True when restore is best-effort.""" + session_dir = tmp_path / ".cecli" / "sessions" + os.makedirs(session_dir, exist_ok=True) + mock_coder.abs_root_path.side_effect = lambda x: str(tmp_path / x) + bad = session_dir / "bad.json" + bad.write_text("not json", encoding="utf-8") + + assert await session_manager.load_session(str(bad), switch=False, quiet=True) is False + mock_coder.io.tool_error.assert_not_called() + + def test_save_session(session_manager, mock_coder, tmp_path): """Test saving a session.""" session_dir = tmp_path / ".cecli" / "sessions" diff --git a/tests/tools/test_grep.py b/tests/tools/test_grep.py index 40b014bf62c..715b3fb7399 100644 --- a/tests/tools/test_grep.py +++ b/tests/tools/test_grep.py @@ -108,3 +108,14 @@ def test_bare_string_search_pattern(tmp_path, monkeypatch): assert "findme" in result coder.io.tool_error.assert_not_called() + + +def test_grep_error_hint_for_bsd_lookahead_failure(): + from cecli.tools.utils.helpers import grep_error_hint + + hint = grep_error_hint( + r"\| \*\*\d+\*\* \| (?!\*\*Done\*\*)", + "grep: repetition-operator operand invalid\n", + ) + assert "lookahead" in hint + assert "ripgrep" in hint diff --git a/tests/tools/test_update_todo_list.py b/tests/tools/test_update_todo_list.py index ffef9d45e31..9b7fe2d5b55 100644 --- a/tests/tools/test_update_todo_list.py +++ b/tests/tools/test_update_todo_list.py @@ -71,6 +71,14 @@ def test_normalize_json_array_unwraps_single_element_json_string_list(): assert items[0]["task"] == "Only task" +def test_normalize_json_array_parses_concatenated_json_objects(): + glued = '{"path": "."}{"path": "docs"}' + items = normalize_json_array(glued, param_name="paths") + assert len(items) == 2 + assert items[0]["path"] == "." + assert items[1]["path"] == "docs" + + def test_normalize_json_array_repairs_literal_newline_after_colon(): """ReadRange/Grep: local models break JSON with a newline between ':' and '\"'.""" broken = '[{"end_text":\n", "file_path": "docs/ROADMAP.md", "start_text": "@000"}]' From 33e13eb79b6ecd753e4246c6dfbe86ea5e182b27 Mon Sep 17 00:00:00 2001 From: Jessica Mulein Date: Thu, 28 May 2026 12:13:22 -0700 Subject: [PATCH 7/7] fix(tools): merge glued tool JSON fragments from local models MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DeepSeek and similar models emit {…}{}{…} tool args; merge into one object before dispatch instead of triplicate ls/Git/Grep calls. Fix Grep format_output tool_footer TypeError on parse errors. Tests in test_tool_arguments.py. Co-authored-by: Cursor --- cecli/coders/agent_coder.py | 22 ++------- cecli/coders/base_coder.py | 11 ++++- cecli/tools/grep.py | 14 +++--- cecli/tools/utils/helpers.py | 68 ++++++++++++++++++++++++++++ tests/tools/test_tool_arguments.py | 72 ++++++++++++++++++++++++++++++ 5 files changed, 162 insertions(+), 25 deletions(-) create mode 100644 tests/tools/test_tool_arguments.py diff --git a/cecli/coders/agent_coder.py b/cecli/coders/agent_coder.py index 9c5e5816a03..f5e3bf0331e 100644 --- a/cecli/coders/agent_coder.py +++ b/cecli/coders/agent_coder.py @@ -727,25 +727,9 @@ async def _execute_local_tools(self, tool_calls_list): continue if args_string: - json_chunks = utils.split_concatenated_json(args_string) - for chunk in json_chunks: - try: - parsed_args_list.append(json.loads(chunk)) - except json.JSONDecodeError as e: - self.model_kwargs = {} - self.io.tool_warning( - f"Malformed JSON arguments in tool {tool_name}: {chunk}" - ) - tool_responses.append( - { - "role": "tool", - "tool_call_id": tool_call.id, - "content": ( - f"Malformed JSON arguments in tool {tool_name}: {str(e)}" - ), - } - ) - continue + from cecli.tools.utils.helpers import parse_tool_arguments + + parsed_args_list = [parse_tool_arguments(args_string)] if not parsed_args_list and not args_string: parsed_args_list.append({}) all_results_content = [] diff --git a/cecli/coders/base_coder.py b/cecli/coders/base_coder.py index 2b2fbdb40be..e052cb68d45 100755 --- a/cecli/coders/base_coder.py +++ b/cecli/coders/base_coder.py @@ -2645,7 +2645,16 @@ def _expand_concatenated_json(self, tool_calls): expanded_tool_calls.append(tool_call) continue - # We have concatenated JSON, so expand it into multiple tool calls. + from cecli.tools.utils.helpers import merge_glued_json_objects + + merged = merge_glued_json_objects(json_chunks) + if merged is not None: + new_tool_call = copy_tool_call(tool_call) + new_tool_call.function.arguments = json.dumps(merged) + expanded_tool_calls.append(new_tool_call) + continue + + # Non-object glued JSON: expand into multiple tool calls (legacy). for i, chunk in enumerate(json_chunks): if not chunk.strip(): continue diff --git a/cecli/tools/grep.py b/cecli/tools/grep.py index 07a50de2ac0..155dbab8104 100644 --- a/cecli/tools/grep.py +++ b/cecli/tools/grep.py @@ -7,7 +7,12 @@ from cecli.helpers.hashline import strip_hashline from cecli.run_cmd import run_cmd_subprocess from cecli.tools.utils.base_tool import BaseTool -from cecli.tools.utils.helpers import ToolError, grep_error_hint, normalize_search_operations +from cecli.tools.utils.helpers import ( + ToolError, + grep_error_hint, + normalize_search_operations, + parse_tool_arguments, +) from cecli.tools.utils.output import color_markers, tool_footer, tool_header @@ -242,9 +247,8 @@ def format_output(cls, coder, mcp_server, tool_response): """Format output for Grep tool.""" color_start, color_end = color_markers(coder) - try: - params = json.loads(tool_response.function.arguments) - except json.JSONDecodeError: + params = parse_tool_arguments(tool_response.function.arguments or "") + if not params and (tool_response.function.arguments or "").strip(): coder.io.tool_error("Invalid Tool JSON") return @@ -255,7 +259,7 @@ def format_output(cls, coder, mcp_server, tool_response): searches = normalize_search_operations(params.get("searches", [])) except ToolError as err: coder.io.tool_error(str(err)) - tool_footer(coder=coder, mcp_server=mcp_server, tool_response=tool_response) + tool_footer(coder=coder, tool_response=tool_response) return if searches: diff --git a/cecli/tools/utils/helpers.py b/cecli/tools/utils/helpers.py index 914eb834e5b..1006fa64cb3 100644 --- a/cecli/tools/utils/helpers.py +++ b/cecli/tools/utils/helpers.py @@ -358,6 +358,74 @@ def _repair_local_model_json_text(text: str) -> str: return repaired +def merge_glued_json_objects(chunks: list[str]) -> dict | None: + """ + Merge consecutive JSON object strings from glued local-model tool args. + + Example: ``{"limit": 15}{}{"path": "."}`` → ``{"limit": 15, "path": "."}``. + Returns ``None`` when chunks are not all mergeable objects (caller may split). + """ + merged: dict = {} + saw_non_empty = False + for chunk in chunks: + text = chunk.strip() + if not text: + continue + obj = _try_parse_json_value(text) + if obj is None: + try: + obj = json.loads(text) + except json.JSONDecodeError: + return None + if isinstance(obj, list): + return None + if not isinstance(obj, dict): + return None + if obj: + merged.update(obj) + saw_non_empty = True + if saw_non_empty or merged == {}: + return merged + return None + + +def parse_tool_arguments(args_string: str) -> dict: + """Parse tool-call arguments, merging glued ``{…}{} {…}`` object fragments.""" + text = (args_string or "").strip() + if not text: + return {} + try: + parsed = json.loads(text) + if isinstance(parsed, dict): + return parsed + except json.JSONDecodeError: + pass + + parsed = _try_parse_json_value(text) + if isinstance(parsed, dict): + return parsed + + from cecli import utils as cecli_utils + + chunks = cecli_utils.split_concatenated_json(text) + if len(chunks) <= 1: + if not chunks: + return {} + lone = _try_parse_json_value(chunks[0]) + if isinstance(lone, dict): + return lone + try: + single = json.loads(chunks[0]) + except json.JSONDecodeError: + return {} + return single if isinstance(single, dict) else {} + + merged = merge_glued_json_objects(chunks) + if merged is not None: + return merged + return {} + + def _try_parse_json_value(text: str): """Parse JSON text, including repairs for common local-model tool-arg quirks.""" text = text.strip() diff --git a/tests/tools/test_tool_arguments.py b/tests/tools/test_tool_arguments.py new file mode 100644 index 00000000000..6efc6f2c282 --- /dev/null +++ b/tests/tools/test_tool_arguments.py @@ -0,0 +1,72 @@ +"""Glued local-model tool JSON argument parsing.""" + +import json +from types import SimpleNamespace +from unittest.mock import Mock + +import pytest + +from cecli.coders.base_coder import Coder +from cecli.tools.grep import Tool as GrepTool +from cecli.tools.utils.helpers import merge_glued_json_objects, parse_tool_arguments + + +def test_parse_tool_arguments_merges_glued_objects_with_empty_fragments(): + raw = '{"limit": 15}{}{"path": "."}' + assert parse_tool_arguments(raw) == {"limit": 15, "path": "."} + + +def test_parse_tool_arguments_merges_grep_style_glued_args(): + raw = ( + '{"limit": 15}{}{"searches": [{"file_pattern": "*.md", ' + '"pattern": "TODO|FIXME", "use_regex": true}]}' + ) + out = parse_tool_arguments(raw) + assert out["limit"] == 15 + assert out["searches"][0]["pattern"] == "TODO|FIXME" + + +def test_merge_glued_returns_none_for_non_object_chunks(): + assert merge_glued_json_objects(['["a"]', '{"b": 1}']) is None + + +def test_expand_concatenated_json_merges_instead_of_splitting(monkeypatch): + """Dogfood: DeepSeek ``{…}{}{…}`` must not become three tool calls.""" + + class MiniCoder(Coder): + def __init__(self): + pass + + coder = MiniCoder.__new__(MiniCoder) + tool_call = SimpleNamespace( + id="call-1", + function=SimpleNamespace( + name="ls", + arguments='{"limit": 15}{}{"path": "."}', + ), + ) + expanded = coder._expand_concatenated_json([tool_call]) + assert len(expanded) == 1 + assert json.loads(expanded[0].function.arguments) == {"limit": 15, "path": "."} + assert expanded[0].id == "call-1" + + +def test_grep_format_output_empty_searches_does_not_crash_tool_footer(): + coder = SimpleNamespace( + io=SimpleNamespace(tool_error=Mock(), tool_output=Mock(), tool_warning=Mock()), + verbose=False, + pretty=False, + tui=lambda: None, + ) + tool_response = SimpleNamespace( + function=SimpleNamespace( + name="Grep", + arguments='{"limit": 15}{}{"searches": []}', + ), + ) + GrepTool.format_output( + coder, + mcp_server=SimpleNamespace(name="Local"), + tool_response=tool_response, + ) + assert coder.io.tool_error.called