Merge pull request #3365 from aniruddhaadak80/fix/read-coerce-numeric-args

fix(read): accept a numeric string for offset and limit

agent/evolution/executor.py

@@ -249,6 +249,10 @@ class _WorkspaceWriteGuard:
         # The agent runtime calls execute_tool (not execute); route it through
         # our guarded execute so the path checks always run.
         try:
+            params, error = self._inner._coerce_integer_args(params)
+            if error:
+                from agent.tools.base_tool import ToolResult
+                return ToolResult.fail(error)
             return self.execute(params)
         except Exception as e:
             logger.error(f"[Evolution] guarded tool error: {e}")

agent/tools/base_tool.py

@@ -34,6 +34,26 @@ class ToolResult:
         return ToolResult(status="error", result=result, ext_data=ext_data, display=display)
 
 
+def _as_int(value) -> Optional[int]:
+    """An integer argument as an int: accepts "2", " 2 ", "2.0" and 2.0, refuses booleans."""
+    if isinstance(value, bool):
+        return None
+    if isinstance(value, int):
+        return value
+    if isinstance(value, str):
+        value = value.strip()
+        try:
+            return int(value)
+        except ValueError:
+            try:
+                value = float(value)
+            except ValueError:
+                return None
+    if isinstance(value, float) and value.is_integer():
+        return int(value)
+    return None
+
+
 def is_tool_available(tool) -> bool:
     """Availability of a tool that may not implement the check at all.
 
@@ -162,11 +182,30 @@ class BaseTool:
     def execute_tool(self, params: dict) -> ToolResult:
         """Run the tool; an exception becomes a failed result, never None."""
         try:
+            params, error = self._coerce_integer_args(params)
+            if error:
+                return ToolResult.fail(error)
             return self.execute(params)
         except Exception as e:
             logger.error(f"[{self.name}] execution failed: {e}", exc_info=True)
             return ToolResult.fail(f"Error: {type(e).__name__}: {e}")
 
+    def _coerce_integer_args(self, params):
+        """Models often quote integers ("offset": "2"); convert every argument the
+        schema declares as an integer, so each tool only checks its own range."""
+        properties = (self.params or {}).get("properties") or {}
+        if not isinstance(params, dict):
+            return params, None
+        coerced = dict(params)
+        for name, value in params.items():
+            if value is None or (properties.get(name) or {}).get("type") != "integer":
+                continue
+            number = _as_int(value)
+            if number is None:
+                return params, f"Error: {name} must be an integer, got {value!r}"
+            coerced[name] = number
+        return coerced, None
+
     def execute(self, params: dict) -> ToolResult:
         """Specific logic to be implemented by subclasses"""
         raise NotImplementedError

agent/tools/memory/memory_get.py

@@ -79,6 +79,8 @@ class MemoryGetTool(BaseTool):
         
         if not path:
             return ToolResult.fail("Error: path parameter is required")
+        if num_lines is not None and num_lines <= 0:
+            return ToolResult.fail(f"Error: num_lines must be a positive number of lines (got {num_lines})")
         
         try:
             workspace_dir = self.memory_manager.config.get_workspace()

agent/tools/read/read.py

@@ -303,6 +303,13 @@ class Read(BaseTool):
         end_line = total_lines
         user_limited = False
         if limit is not None:
+            # limit=0 used to return an empty page with a "continue" hint that
+            # pointed back at the same line; a negative one sliced from the end.
+            if limit <= 0:
+                return None, (
+                    f"Error: limit must be a positive number of lines (got {limit}). "
+                    f"Omit limit to read the whole file, or pass limit=20 to read 20 lines."
+                )
             end_line = min(start_line + limit, total_lines)
             user_limited = True
 

tests/test_read_zero_limit.py

@@ -0,0 +1,108 @@
+# encoding:utf-8
+"""A non-positive Read ``limit`` is refused: limit=0 used to return an empty
+page whose "continue" hint pointed back at the same line, and a negative one
+sliced from the end of the file."""
+import os
+import re
+import sys
+import tempfile
+import unittest
+
+sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
+
+from agent.tools.read.read import Read
+
+SUGGESTED_OFFSET = re.compile(r"Use offset=(\d+) to continue")
+
+
+class TestReadNonPositiveLimit(unittest.TestCase):
+    def setUp(self):
+        self.work = tempfile.mkdtemp()
+        self.tool = Read({"cwd": self.work})
+        self.path = self._write("f.txt", "\n".join(f"line{i}" for i in range(1, 13)) + "\n")
+
+    def _write(self, name, text):
+        path = os.path.join(self.work, name)
+        with open(path, "w", encoding="utf-8") as f:
+            f.write(text)
+        return path
+
+    @staticmethod
+    def _content(result):
+        return result.result["content"] if isinstance(result.result, dict) else str(result.result)
+
+    def _suggested_offset(self, result):
+        match = SUGGESTED_OFFSET.search(self._content(result))
+        return int(match.group(1)) if match else None
+
+    def test_a_suggested_offset_must_actually_advance_the_read(self):
+        """The invariant behind the bug: no hint may point at the same lines."""
+        first = self.tool.execute({"path": self.path, "limit": 0})
+        suggested = self._suggested_offset(first)
+        self.assertIsNone(
+            suggested,
+            "limit=0 advertised "
+            f"offset={suggested}; re-reading from it returned byte-identical "
+            f"output, so the hint could never make progress: "
+            f"{self._content(first)!r}",
+        )
+
+    def test_zero_limit_returns_no_content_at_all(self):
+        result = self.tool.execute({"path": self.path, "limit": 0})
+        self.assertEqual(result.status, "error", result.result)
+        content = self._content(result)
+        # Not a success with an empty body: that is indistinguishable from an
+        # empty file and invites the model to keep re-asking.
+        self.assertNotIn("more lines in file", content)
+        self.assertNotIn("to continue", content)
+
+    def test_zero_limit_names_the_offending_argument(self):
+        result = self.tool.execute({"path": self.path, "limit": 0})
+        message = self._content(result)
+        self.assertIn("limit", message)
+        self.assertIn("0", message)
+        # Tell the model what to do instead, and that omitting limit reads on.
+        self.assertIn("limit=20", message)
+
+    def test_zero_limit_with_an_offset_is_rejected_too(self):
+        result = self.tool.execute({"path": self.path, "offset": 4, "limit": 0})
+        self.assertEqual(result.status, "error", result.result)
+        self.assertNotIn("to continue", self._content(result))
+
+    def test_negative_limit_is_rejected_instead_of_slicing_from_the_end(self):
+        # limit=-5 used to end_line at -5, so all_lines[0:-5] returned the
+        # first 7 lines of a 12-line file with a success status.
+        result = self.tool.execute({"path": self.path, "limit": -5})
+        self.assertEqual(result.status, "error", result.result)
+        self.assertNotIn("line1|", self._content(result))
+
+    def test_a_positive_limit_still_paginates_unchanged(self):
+        result = self.tool.execute({"path": self.path, "offset": 4, "limit": 2})
+        self.assertEqual(result.status, "success", result.result)
+        self.assertEqual(result.result["content"].split("\n\n")[0], "4|line4\n5|line5")
+        self.assertEqual(result.result["start_line"], 4)
+        self.assertEqual(result.result["output_lines"], 2)
+        # The hint a normal limit is supposed to produce is untouched.
+        self.assertEqual(self._suggested_offset(result), 6)
+        self.assertIn("7 more lines in file", self._content(result))
+
+    def test_a_limit_past_the_end_of_the_file_reads_to_the_end(self):
+        result = self.tool.execute({"path": self.path, "offset": 10, "limit": 500})
+        self.assertEqual(result.status, "success", result.result)
+        self.assertEqual(result.result["output_lines"], 3)
+        self.assertIsNone(self._suggested_offset(result))
+
+    def test_limit_of_one_still_returns_exactly_one_line(self):
+        result = self.tool.execute({"path": self.path, "limit": 1})
+        self.assertEqual(result.status, "success", result.result)
+        self.assertEqual(result.result["content"].split("\n\n")[0], "1|line1")
+        self.assertEqual(self._suggested_offset(result), 2)
+
+    def test_a_positive_limit_from_the_end_still_works(self):
+        result = self.tool.execute({"path": self.path, "offset": -3, "limit": 2})
+        self.assertEqual(result.status, "success", result.result)
+        self.assertEqual(result.result["content"].split("\n\n")[0], "10|line10\n11|line11")
+
+
+if __name__ == "__main__":
+    unittest.main()

tests/test_tool_integer_args.py

@@ -0,0 +1,51 @@
+"""Integer tool arguments a model sends quoted ("offset": "2") are converted once,
+in BaseTool.execute_tool, for every tool that declares them as integers."""
+
+import pytest
+
+from agent.tools.base_tool import BaseTool, ToolResult
+from agent.tools.read.read import Read
+
+
+class _Echo(BaseTool):
+    name = "echo"
+    params = {"type": "object", "properties": {"n": {"type": "integer"}, "s": {"type": "string"}}}
+
+    def execute(self, params):
+        return ToolResult.success(params)
+
+
+@pytest.mark.parametrize("raw, expected", [("2", 2), (" 2 ", 2), ("2.0", 2), (2.0, 2), (2, 2)])
+def test_integer_arguments_are_converted(raw, expected):
+    result = _Echo().execute_tool({"n": raw, "s": "7"})
+    assert result.status == "success"
+    assert result.result == {"n": expected, "s": "7"}
+
+
+@pytest.mark.parametrize("raw", ["two", "2.5", True])
+def test_a_non_integer_is_refused_with_the_argument_name(raw):
+    result = _Echo().execute_tool({"n": raw})
+    assert result.status == "error"
+    assert "n must be an integer" in result.result
+
+
+def test_read_accepts_quoted_offset_and_limit(tmp_path):
+    path = tmp_path / "f.txt"
+    path.write_text("\n".join(f"line{i}" for i in range(1, 13)) + "\n", encoding="utf-8")
+    tool = Read({"cwd": str(tmp_path)})
+
+    quoted = tool.execute_tool({"path": str(path), "offset": "4", "limit": "2"})
+    plain = tool.execute_tool({"path": str(path), "offset": 4, "limit": 2})
+
+    assert quoted.status == "success", quoted.result
+    assert quoted.result["content"] == plain.result["content"]
+    assert quoted.result["content"].startswith("4|line4\n5|line5")
+
+
+def test_memory_get_refuses_a_non_positive_num_lines():
+    from agent.tools.memory.memory_get import MemoryGetTool
+
+    tool = MemoryGetTool.__new__(MemoryGetTool)
+    result = tool.execute_tool({"path": "MEMORY.md", "num_lines": "0"})
+    assert result.status == "error"
+    assert "num_lines must be a positive" in result.result