Defensive coercion for JSON-string tool arguments from LLM

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #22436 fix(delegate): salvage #21933 JSON-string batch + diagnostic logging

  merged to main

  files: model_tools.py, tests/tools/test_delegate.py, tools/delegate_tool.py

  Show the diff

  • model_tools.py

    @@ -550,6 +550,16 @@ def coerce_tool_args(tool_name: str, args: Dict[str, Any]) -> Dict[str, Any]:
                         # nullable "null" → None).
                         args[key] = coerced
                         continue
    +                # If the string looks like a JSON array but _coerce_value
    +                # failed to parse it, warn clearly instead of silently wrapping.
    +                if value.strip().startswith("["):
    +                    logger.warning(
    +                        "coerce_tool_args: %s.%s looks like a JSON array string "
    +                        "but could not be parsed — model may have emitted a "
    +                        "JSON-encoded string instead of a native array. "
    +                        "Falling back to single-element list.",
    +                        tool_name, key,
    +                    )
                     args[key] = [value]
                     logger.info(
                         "coerce_tool_args: wrapped bare string in list for %s.%s",
    @@ -637,7 +647,12 @@ def _coerce_json(value: str, expected_python_type: type):
         """
         try:
             parsed = json.loads(value)
    -    except (ValueError, TypeError):
    +    except (ValueError, TypeError) as exc:
    +        logger.warning(
    +            "coerce_tool_args: failed to parse string as JSON for expected type %s: %s",
    +            expected_python_type.__name__,
    +            exc,
    +        )
             return value
         if isinstance(parsed, expected_python_type):
             logger.debug(
    @@ -645,6 +660,11 @@ def _coerce_json(value: str, expected_python_type: type):
                 expected_python_type.__name__,
             )
             return parsed
    +    logger.warning(
    +        "coerce_tool_args: JSON-parsed value is %s, expected %s — skipping coercion",
    +        type(parsed).__name__,
    +        expected_python_type.__name__,
    +    )
         return value
     
     

  • tests/tools/test_delegate.py

    @@ -167,6 +167,63 @@ def test_batch_mode(self, mock_run):
             self.assertEqual(result["results"][1]["summary"], "Result B")
             self.assertIn("total_duration_seconds", result)
     
    +    @patch("tools.delegate_tool._run_single_child")
    +    def test_batch_mode_accepts_json_string_tasks(self, mock_run):
    +        mock_run.side_effect = [
    +            {
    +                "task_index": 0,
    +                "status": "completed",
    +                "summary": "Result A",
    +                "api_calls": 2,
    +                "duration_seconds": 3.0,
    +            },
    +            {
    +                "task_index": 1,
    +                "status": "completed",
    +                "summary": "Result B",
    +                "api_calls": 4,
    +                "duration_seconds": 6.0,
    +            },
    +        ]
    +        parent = _make_mock_parent()
    +        tasks = json.dumps(
    +            [
    +                {"goal": "Research topic A"},
    +                {"goal": "Research topic B"},
    +            ]
    +        )
    +
    +        result = json.loads(delegate_task(tasks=tasks, parent_agent=parent))
    +
    +        self.assertIn("results", result)
    +        self.assertEqual(len(result["results"]), 2)
    +        self.assertEqual(result["results"][0]["summary"], "Result A")
    +        self.assertEqual(result["results"][1]["summary"], "Result B")
    +
    +    @patch("tools.delegate_tool._run_single_child")
    +    def test_batch_mode_rejects_non_object_tasks(self, mock_run):
    +        parent = _make_mock_parent()
    +
    +        result = json.loads(
    +            delegate_task(tasks=["not a task object"], parent_agent=parent)
    +        )
    +
    +        self.assertIn("error", result)
    +        self.assertIn("Task 0 must be an object", result["error"])
    +        mock_run.assert_not_called()
    +
    +    @patch("tools.delegate_tool._run_single_child")
    +    def test_batch_mode_rejects_malformed_json_string_tasks(self, mock_run):
    +        parent = _make_mock_parent()
    +
    +        result = json.loads(
    +            delegate_task(tasks='[{"goal": "bad}', parent_agent=parent)
    +        )
    +
    +        self.assertIn("error", result)
    +        self.assertIn("could not be parsed as JSON", result["error"])
    +        mock_run.assert_not_called()
    +
         @patch("tools.delegate_tool._run_single_child")
         def test_batch_capped_at_3(self, mock_run):
             mock_run.return_value = {

  • tools/delegate_tool.py

    @@ -1867,6 +1867,29 @@ def _run_with_thread_capture():
                 logger.debug("Failed to close child agent after delegation")
     
     
    +def _recover_tasks_from_json_string(
    +    tasks: Any,
    +) -> tuple[Optional[List[Dict[str, Any]]], Optional[str]]:
    +    if not isinstance(tasks, str):
    +        return None, None
    +    raw = tasks.strip()
    +    if not raw:
    +        return None, "Provide either 'goal' (single task) or 'tasks' (batch)."
    +    try:
    +        parsed = json.loads(raw)
    +    except json.JSONDecodeError as exc:
    +        return None, (
    +            "tasks must be a JSON array of task objects; received a string "
    +            f"that could not be parsed as JSON ({exc.msg})."
    +        )
    +    if not isinstance(parsed, list):
    +        return None, (
    +            f"tasks must be a JSON array of task objects; parsed "
    +            f"{type(parsed).__name__} instead."
    +        )
    +    return parsed, None
    +
    +
     def delegate_task(
         goal: Optional[str] = None,
         context: Optional[str] = None,
    @@ -1951,6 +1974,12 @@ def delegate_task(
     
         # Normalize to task list
         max_children = _get_max_concurrent_children()
    +    recovered_tasks, tasks_error = _recover_tasks_from_json_string(tasks)
    +    if tasks_error:
    +        return tool_error(tasks_error)
    +    if recovered_tasks is not None:
    +        tasks = recovered_tasks
    +
         if tasks and isinstance(tasks, list):
             if len(tasks) > max_children:
                 return tool_error(
    @@ -1973,6 +2002,10 @@ def delegate_task(
     
         # Validate each task has a goal
         for i, task in enumerate(task_list):
    +        if not isinstance(task, dict):
    +            return tool_error(
    +                f"Task {i} must be an object, got {type(task).__name__}."
    +            )
             if not task.get("goal", "").strip():
                 return tool_error(f"Task {i} is missing a 'goal'.")
     

• #52618 fix(tools): defensive type coercion in todo_tool for malformed LLM input (#14185)

  merged to main

  files: tests/tools/test_todo_tool_type_coercion.py, tools/todo_tool.py

  Show the diff

  • tests/tools/test_todo_tool_type_coercion.py

    @@ -0,0 +1,133 @@
    +"""Tests for defensive type coercion in todo_tool (issue #14185).
    +
    +Covers three crash patterns:
    +1. todos is a JSON string instead of a list
    +2. todos list contains non-dict items (e.g., bare strings)
    +3. Well-formed input continues to work unchanged
    +"""
    +
    +import json
    +
    +from tools.todo_tool import TodoStore, todo_tool
    +
    +
    +class TestJsonStringCoercion:
    +    """Guard 1: todo_tool() recovers when LLM sends todos as a JSON string."""
    +
    +    def test_json_string_is_parsed_into_list(self):
    +        store = TodoStore()
    +        todos_str = json.dumps([
    +            {"id": "t1", "content": "Do A", "status": "pending"},
    +            {"id": "t2", "content": "Do B", "status": "in_progress"},
    +        ])
    +        result = json.loads(todo_tool(todos=todos_str, store=store))
    +        assert "error" not in result
    +        assert result["summary"]["total"] == 2
    +        assert result["todos"][0]["id"] == "t1"
    +        assert result["todos"][1]["status"] == "in_progress"
    +
    +    def test_unparseable_string_returns_error(self):
    +        store = TodoStore()
    +        result = json.loads(todo_tool(todos="not valid json [", store=store))
    +        assert "error" in result
    +
    +    def test_json_string_that_parses_to_non_list_returns_error(self):
    +        store = TodoStore()
    +        # Valid JSON, but a dict instead of a list
    +        result = json.loads(todo_tool(todos='{"id": "1"}', store=store))
    +        assert "error" in result
    +
    +    def test_non_list_non_string_returns_error(self):
    +        store = TodoStore()
    +        result = json.loads(todo_tool(todos=42, store=store))
    +        assert "error" in result
    +
    +
    +class TestNonDictListItems:
    +    """Guards 2 & 3: _validate and _dedupe_by_id handle non-dict items."""
    +
    +    def test_string_item_in_list_does_not_crash(self):
    +        store = TodoStore()
    +        result = store.write(["not-a-dict"])
    +        assert len(result) == 1
    +        assert result[0]["id"] == "?"
    +        assert result[0]["content"] == "(invalid item)"
    +        assert result[0]["status"] == "pending"
    +
    +    def test_mixed_valid_and_invalid_items(self):
    +        store = TodoStore()
    +        result = store.write([
    +            {"id": "1", "content": "Real task", "status": "pending"},
    +            "garbage",
    +            42,
    +            {"id": "2", "content": "Another task", "status": "completed"},
    +        ])
    +        assert len(result) == 4
    +        # Valid items are preserved
    +        assert result[0]["id"] == "1"
    +        assert result[0]["content"] == "Real task"
    +        assert result[3]["id"] == "2"
    +        # Invalid items get placeholder values
    +        assert result[1]["content"] == "(invalid item)"
    +        assert result[2]["content"] == "(invalid item)"
    +
    +    def test_none_item_in_list(self):
    +        store = TodoStore()
    +        result = store.write([None])
    +        assert len(result) == 1
    +        assert result[0]["id"] == "?"
    +
    +    def test_integer_item_in_list(self):
    +        store = TodoStore()
    +        result = store.write([123])
    +        assert len(result) == 1
    +        assert result[0]["content"] == "(invalid item)"
    +
    +    def test_non_dict_items_via_todo_tool(self):
    +        """End-to-end: non-dict list items produce valid output, not a crash."""
    +        store = TodoStore()
    +        result = json.loads(todo_tool(todos=["bad", "also bad"], store=store))
    +        assert "error" not in result
    +        assert result["summary"]["total"] == 2
    +        assert result["summary"]["pending"] == 2
    +
    +
    +class TestWellFormedInputUnchanged:
    +    """Regression: normal usage must not be affected by the guards."""
    +
    +    def test_normal_write_and_read(self):
    +        store = TodoStore()
    +        items = [
    +            {"id": "a", "content": "First", "status": "pending"},
    +            {"id": "b", "content": "Second", "status": "in_progress"},
    +        ]
    +        result = json.loads(todo_tool(todos=items, store=store))
    +        assert result["summary"]["total"] == 2
    +        assert result["summary"]["pending"] == 1
    +        assert result["summary"]["in_progress"] == 1
    +
    +    def test_merge_mode_still_works(self):
    +        store = TodoStore()
    +        store.write([{"id": "1", "content": "Original", "status": "pending"}])
    +        result = json.loads(todo_tool(
    +            todos=[{"id": "1", "status": "completed"}],
    +            merge=True,
    +            store=store,
    +        ))
    +        assert result["summary"]["completed"] == 1
    +        assert result["todos"][0]["content"] == "Original"
    +
    +    def test_read_mode_still_works(self):
    +        store = TodoStore()
    +        store.write([{"id": "x", "content": "Task", "status": "pending"}])
    +        result = json.loads(todo_tool(store=store))
    +        assert result["summary"]["total"] == 1
    +
    +    def test_dedup_still_works(self):
    +        store = TodoStore()
    +        result = store.write([
    +            {"id": "1", "content": "v1", "status": "pending"},
    +            {"id": "1", "content": "v2", "status": "in_progress"},
    +        ])
    +        assert len(result) == 1
    +        assert result[0]["content"] == "v2"

  • tools/todo_tool.py

    @@ -158,6 +158,9 @@ def _validate(item: Dict[str, Any]) -> Dict[str, str]:
             Ensures required fields exist and status is valid.
             Returns a clean dict with only {id, content, status}.
             """
    +        if not isinstance(item, dict):
    +            return {"id": "?", "content": "(invalid item)", "status": "pending"}
    +
             item_id = str(item.get("id", "")).strip()
             if not item_id:
                 item_id = "?"
    @@ -179,6 +182,10 @@ def _dedupe_by_id(todos: List[Dict[str, Any]]) -> List[Dict[str, Any]]:
             """Collapse duplicate ids, keeping the last occurrence in its position."""
             last_index: Dict[str, int] = {}
             for i, item in enumerate(todos):
    +            if not isinstance(item, dict):
    +                # Non-dict items get a synthetic key so _validate can handle them
    +                last_index[f"__invalid_{i}"] = i
    +                continue
                 item_id = str(item.get("id", "")).strip() or "?"
                 last_index[item_id] = i
             return [todos[i] for i in sorted(last_index.values())]
    @@ -204,6 +211,16 @@ def todo_tool(
             return tool_error("TodoStore not initialized")
     
         if todos is not None:
    +        # Guard: LLM sometimes sends todos as a JSON string instead of a list
    +        if isinstance(todos, str):
    +            try:
    +                todos = json.loads(todos)
    +            except (json.JSONDecodeError, TypeError):
    +                return tool_error("todos must be a list of objects, got unparseable string")
    +        if not isinstance(todos, list):
    +            return tool_error(
    +                f"todos must be a list, got {type(todos).__name__}"
    +            )
             items = store.write(todos, merge)
         else:
             items = store.read()