fix(tools): close ROADMAP-02/03/04 — markdown escaping, bounded reads, finite results

agentkit/tools/builtin/calculator.py

@@ -1,12 +1,18 @@
 """Safe arithmetic calculator tool powered by strict Python AST parsing."""
 
 import ast
+import math
 
 from agentkit.tools.registry import tool
 
 MAX_EXPRESSION_LEN = 1000
 MAX_AST_NODES = 100
 MAX_EXPONENT = 1000
+#: When the base magnitude grows too, an exponent well below MAX_EXPONENT can already
+#: overflow a float (``1000.0 ** 100`` is ~1e300; ``1999.0 ** 101`` is inf), so pairs of an
+#: oversized base and a still-large exponent are rejected before the operation runs.
+MAX_POW_BASE_MAGNITUDE = 1000
+MAX_POW_PAIR_EXPONENT = 100
 
 #: Rough ceiling on the bit length of an exponentiation result. 10**1000 is ~3322 bits and
 #: still cheap once, but the ReAct loop can repeat the call, so the result size itself is
@@ -23,6 +29,11 @@ def _evaluate_node(node: ast.AST) -> int | float:
         # Explicitly reject boolean, string, None, complex, etc.
         if isinstance(node.value, bool) or not isinstance(node.value, (int, float)):
             raise ValueError(f"Disallowed operand type: {type(node.value).__name__}")
+        # A float literal that overflows Python's double range (e.g. ``1e999``) silently
+        # becomes inf at parse time; anchoring finiteness here keeps it out of every
+        # subsequent operation.
+        if isinstance(node.value, float) and not math.isfinite(node.value):
+            raise ValueError("Numeric literal is not finite (out of range)")
         return node.value
 
     if isinstance(node, ast.UnaryOp):
@@ -56,7 +67,9 @@ def _evaluate_node(node: ast.AST) -> int | float:
                 raise ValueError("Modulo by zero")
             return left % right
         if isinstance(node.op, ast.Pow):
-            if abs(right) > MAX_EXPONENT or (abs(left) > 1000 and right > 100):
+            if abs(right) > MAX_EXPONENT or (
+                abs(left) > MAX_POW_BASE_MAGNITUDE and right > MAX_POW_PAIR_EXPONENT
+            ):
                 raise ValueError(f"Exponent too large (maximum allowed is {MAX_EXPONENT})")
 
             # The exponent bound alone still permits enormous big integers (10**1000 is a
@@ -108,7 +121,16 @@ def safe_calculate(expression: str) -> int | float:
     if len(nodes) > MAX_AST_NODES:
         raise ValueError(f"Expression too complex (exceeds {MAX_AST_NODES} AST nodes).")
 
-    return _evaluate_node(tree)
+    result = _evaluate_node(tree)
+
+    # Any float operand is finite, but sums/products/divisions can still overflow into
+    # inf or collapse into nan. Intercept once at the exit so neither ever reaches the
+    # prompt or history. (The only finite value derivable from an inf intermediate is
+    # inf ** 0 == 1.0, which is the correct x**0 answer rather than a masked overflow.)
+    if isinstance(result, float) and not math.isfinite(result):
+        raise ValueError("Result is not a finite number")
+
+    return result
 
 
 @tool

agentkit/tools/builtin/read_file.py

@@ -50,14 +50,18 @@ def read_sandboxed_file(
     if not target.is_file():
         raise ValueError(f"Target is not a regular file: '{file_path}'")
 
-    file_size = target.stat().st_size
-    if file_size > max_bytes:
-        raise ValueError(
-            f"File '{file_path}' exceeds maximum size limit of {max_bytes} bytes (size: {file_size} bytes)."
-        )
+    # The byte cap is enforced on the bytes actually read, not on a prior st_size: a file
+    # grown between a stat check and a second open would otherwise be read in full (the
+    # check-then-read race). One bounded read of max_bytes + 1 closes that window while
+    # making oversize still fail — and an exact-cap file stays fully readable.
+    with target.open("rb") as handle:
+        data = handle.read(max_bytes + 1)
+
+    if len(data) > max_bytes:
+        raise ValueError(f"File '{file_path}' exceeds maximum size limit of {max_bytes} bytes.")
 
     try:
-        return target.read_text(encoding="utf-8")
+        return data.decode("utf-8")
     except UnicodeDecodeError as exc:
         raise ValueError(f"File '{file_path}' is not a valid UTF-8 text file.") from exc
 

agentkit/tools/builtin/sql_readonly.py

@@ -36,6 +36,26 @@
 DEFAULT_ROW_LIMIT = 50
 DEFAULT_STATEMENT_TIMEOUT_S = 10.0
 
+#: Per-cell character ceiling for rendered table cells. The executor budget
+#: (TOOL_OUTPUT_MAX_CHARS) bounds the whole table afterwards, but one wide column value
+#: should not be able to consume most of that budget — or hide a large payload — behind
+#: row truncation.
+MAX_CELL_CHARS = 200
+
+#: Row-breaking and invisible control characters are rewritten to visible escapes, and
+#: every other C0 control character (plus DEL) is rendered as ``\\uXXXX`` so no control
+#: byte reaches the prompt as structure.
+_CONTROL_CHAR_ESCAPES: dict[int, str] = {
+    ord("\n"): "\\n",
+    ord("\r"): "\\r",
+    ord("\t"): "\\t",
+    **{
+        code: f"\\u{code:04x}"
+        for code in (*range(0x20), 0x7F)
+        if code not in (ord("\n"), ord("\r"), ord("\t"))
+    },
+}
+
 DISALLOWED_KEYWORDS = (
     "insert",
     "update",
@@ -402,17 +422,36 @@ def _dollar_quote_end(sql: str, start: int) -> int | None:
     return len(sql) if closing == -1 else closing + len(tag)
 
 
+def _escape_cell_text(raw_text: str) -> str:
+    """Escape and bound a single table cell so it cannot alter the markdown structure.
+
+    A pipe would forge additional columns, a newline would forge additional rows, and
+    invisible control characters could smuggle prompt structure — the classic stored
+    prompt injection through attacker-influenced database values. Backslash is escaped
+    first so it cannot escape its own escape sequences. Finiteness of the rendered
+    length (not the raw value) is the cap, so the displayed cell is bounded no matter
+    how the escapes inflated the text.
+    """
+    escaped = raw_text.replace("\\", "\\\\").replace("|", "\\|")
+    escaped = escaped.translate(_CONTROL_CHAR_ESCAPES)
+    if len(escaped) > MAX_CELL_CHARS:
+        escaped = escaped[:MAX_CELL_CHARS] + "[Truncated]"
+    return escaped
+
+
+def _format_cell(value: Any) -> str:
+    """Render one SQL result value as a safe markdown table cell."""
+    return "NULL" if value is None else _escape_cell_text(str(value))
+
+
 def format_rows_as_markdown(columns: Sequence[str], rows: Sequence[Any]) -> str:
     """Format tabular SQL results into clean Markdown table string."""
     if not rows:
         return "No rows returned."
 
-    header = "| " + " | ".join(columns) + " |"
+    header = "| " + " | ".join(_format_cell(col) for col in columns) + " |"
     divider = "| " + " | ".join(["---"] * len(columns)) + " |"
-    data_lines = [
-        "| " + " | ".join(str(val) if val is not None else "NULL" for val in row) + " |"
-        for row in rows
-    ]
+    data_lines = ["| " + " | ".join(_format_cell(val) for val in row) + " |" for row in rows]
     return "\n".join([header, divider, *data_lines])
 
 

docs/PROGRESS.md

@@ -76,6 +76,9 @@ This document tracks execution progress across all backlog tasks, including pull
 | FU-85 | AUDIT | Real-Infra Provisioning Failures | `test/real-infra-provisioning-failures` | #90 | 1 | Done | Provisioning errors fail the live SQL-isolation tier instead of skipping it |
 | FU-88 | AUDIT | Audit History Consistency | `docs/audit-history-consistency` | #90 | 1 | Done | Canonical 12-layer/14-commit wording; stale "AST validation" row corrected and guarded |
 | FU-89 | AUDIT | uv Lockfile Policy | `chore/repo-uv-lock` | #90 | 1 | Done | `uv.lock` ignored; pip-based CI does not consume a uv lock |
+| ROADMAP-02 | ROADMAP | SQL Markdown Cell Escaping | `fix/roadmap-02-03-04-tools-hardening` | #120 | 1 | Done | Cells escaped (`\|`, `\n`, `\uXXXX` controls) and length-capped before markdown output; closes #94 |
+| ROADMAP-03 | ROADMAP | read_file Read-After-Stat Race | `fix/roadmap-02-03-04-tools-hardening` | #120 | 1 | Done | Byte cap enforced on a bounded `read(max_bytes + 1)` in one open; closes #95 |
+| ROADMAP-04 | ROADMAP | Calculator Non-Finite Result Guard | `fix/roadmap-02-03-04-tools-hardening` | #120 | 1 | Done | `math.isfinite` result check, inf literals rejected, pow pair guard named; closes #96 |
 
 ---
 
@@ -505,3 +508,9 @@ This document tracks execution progress across all backlog tasks, including pull
   - **#85 (test reliability)** — `tests/real/test_sql_isolation_real.py::readonly_engine` no longer converts `provision_readonly_role()` failures into `pytest.skip`. The upstream `real_database_url` fixture already applies migrations and fails loudly when PostgreSQL is unreachable, so a provisioning error is a defect and now turns CI red instead of leaving the isolation tier green-by-skip.
   - **#88 (docs)** — `docs/AUDIT.md` records the canonical "linear 12-layer stack, merged as 14 commits (12 layer commits plus two progress-bookkeeping commits)" wording instead of "nine branches", and the historical M4-T04 row describes the lexer plus dedicated read-only role instead of "Strict SELECT AST validation". `tests/unit/test_docs_accuracy.py` guards both claims.
   - **#89 (repo hygiene)** — `.gitignore` ignores `uv.lock` with a comment explaining that all three CI jobs install from `pyproject.toml` with pip, so a lockfile is not part of the reproducible build. Adopting uv properly (commit the lock, switch CI to `uv sync --locked`) remains a separate decision.
+
+### PR #120: ROADMAP-02 / ROADMAP-03 / ROADMAP-04 Tools-Hardening Batch (P0 quick fixes)
+- **Status**: Done
+- **PR**: `fix/roadmap-02-03-04-tools-hardening` — PR #120 (three `fix(tools)` commits, squash-merged; docs rows in this commit)
+- **Commits**: 3 (+1 docs)
+- **Notes**: Closed the first three `ROADMAP` issues filed from the 2026-10-27 assessment. **#94** — `format_rows_as_markdown` now routes every cell (and column name) through `_format_cell`/`_escape_cell_text`: backslash first, then `|` → `\|`, newline/CR/tab → `\n`/`\r`/`\t`, every other C0/DEL control character → `\uXXXX`, and a rendered-length cap `MAX_CELL_CHARS` (200) with a `[Truncated]` marker, so a database value can no longer forge table rows, columns or prompt structure (stored prompt injection). **#95** — `read_sandboxed_file` enforces the byte cap on the bytes actually read (`read(max_bytes + 1)` from a single open) instead of a `stat().st_size` check + second open; oversize keeps the same user-visible phrase, an exact-cap file stays fully readable, and a regression test stands in for the race by lying about `st_size` (the old code returned the full oversized file). **#96** — `safe_calculate` rejects non-finite float results (`1e308 * 10 ** 3` no longer renders `inf` into the prompt) and non-finite float literals (`1e999` parses to inf) at the operand boundary; the `Pow` guard's bare `1000`/`100` literals became `MAX_POW_BASE_MAGNITUDE`/`MAX_POW_PAIR_EXPONENT` with unchanged values. 11 tests added (`test_tool_sql_readonly.py`, `test_tool_read_file.py`, `test_tool_calculator.py`); suite moved from 483 to 494 passing.

tests/unit/test_tool_calculator.py

@@ -93,6 +93,29 @@ def test_dos_protection_large_exponents() -> None:
         safe_calculate("10 ** 10000")
 
 
+def test_non_finite_results_rejected() -> None:
+    with pytest.raises(ValueError, match="not a finite number"):
+        safe_calculate("1e308 * 10 ** 3")
+
+    with pytest.raises(ValueError, match="not a finite number"):
+        safe_calculate("1e308 * 2")
+
+
+def test_overflowing_float_literal_rejected() -> None:
+    # A float literal beyond double range silently parses to inf at AST level.
+    with pytest.raises(ValueError, match="not finite"):
+        safe_calculate("1e999")
+
+    with pytest.raises(ValueError, match="not finite"):
+        safe_calculate("-1e999")
+
+
+def test_large_integer_results_still_allowed() -> None:
+    assert safe_calculate("2 ** 1000") == 2**1000
+    assert safe_calculate("10 ** 100") == 10**100
+    assert safe_calculate("1000 ** 100") == 1000**100
+
+
 def test_calculator_tool_function() -> None:
     assert calculator("10 + 20") == "30"
     assert calculator("7 / 2") == "3.5"

tests/unit/test_tool_read_file.py

@@ -1,5 +1,6 @@
 """Unit and security tests for sandboxed file reader tool."""
 
+import os
 from pathlib import Path
 
 import pytest
@@ -90,6 +91,52 @@ def test_max_bytes_limit_enforced(tmp_path: Path) -> None:
         read_sandboxed_file("large.txt", base_dir=tmp_path, max_bytes=100)
 
 
+def test_exact_cap_file_is_readable(tmp_path: Path) -> None:
+    doc = tmp_path / "exact.txt"
+    doc.write_text("A" * 40, encoding="utf-8")
+
+    assert read_sandboxed_file("exact.txt", base_dir=tmp_path, max_bytes=40) == "A" * 40
+
+
+def test_size_cap_enforced_on_the_read_not_on_stat(
+    tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """A file whose stat lies small is still capped by the bytes actually read.
+
+    Regression: the cap used to consult ``stat().st_size`` and then re-open the file, so a
+    file grown between the two calls bypassed the limit completely. Lying about the size is
+    an observable stand-in for that race; the bounded read must reject it regardless.
+    """
+    doc = tmp_path / "growing.txt"
+    doc.write_text("A" * 50, encoding="utf-8")
+
+    real_stat = Path.stat
+
+    def lying_stat(self: Path, *, follow_symlinks: bool = True) -> "os.stat_result":
+        st = real_stat(self, follow_symlinks=follow_symlinks)
+        # Report st_size = 10 while the real content is 50 bytes.
+        return os.stat_result(
+            (
+                st.st_mode,
+                st.st_ino,
+                st.st_dev,
+                st.st_nlink,
+                st.st_uid,
+                st.st_gid,
+                10,
+                st.st_atime,
+                st.st_mtime,
+                st.st_ctime,
+            )
+        )
+
+    monkeypatch.setattr(Path, "stat", lying_stat)
+
+    # The old stat-driven cap would have passed this file through and returned all 50 bytes.
+    with pytest.raises(ValueError, match="exceeds maximum size limit"):
+        read_sandboxed_file("growing.txt", base_dir=tmp_path, max_bytes=40)
+
+
 def test_binary_file_rejected(tmp_path: Path) -> None:
     doc = tmp_path / "binary.bin"
     doc.write_bytes(b"\x80\x81\xff\xfe\x00\x01")

tests/unit/test_tool_sql_readonly.py

@@ -7,7 +7,9 @@
 from sqlalchemy.ext.asyncio import AsyncEngine, create_async_engine
 
 from agentkit.tools.builtin.sql_readonly import (
+    MAX_CELL_CHARS,
     execute_sql_query,
+    format_rows_as_markdown,
     sql_readonly,
     validate_sql_query,
 )
@@ -158,3 +160,67 @@ async def test_sql_tool_registry_integration(
     bad_result = await registry.execute(bad_call)
     assert bad_result.ok is False
     assert "Only SELECT" in (bad_result.error or "")
+
+
+def test_markdown_escapes_pipe_and_backslash_per_cell() -> None:
+    """A pipe in a cell value cannot forge additional columns, and rows stay intact."""
+    output = format_rows_as_markdown(["customer"], [("Al|ice",), ("a\\b",), ("Bob",)])
+
+    assert output.splitlines() == [
+        "| customer |",
+        "| --- |",
+        "| Al\\|ice |",
+        "| a\\\\b |",
+        "| Bob |",
+    ]
+
+
+def test_markdown_escapes_newlines_and_control_characters() -> None:
+    """A newline in a cell value cannot forge additional rows."""
+    output = format_rows_as_markdown(["text"], [("line1\nline2\r\nline3\x07end\x7f",)])
+
+    assert output.splitlines() == [
+        "| text |",
+        "| --- |",
+        "| line1\\nline2\\r\\nline3\\u0007end\\u007f |",
+    ]
+
+
+def test_markdown_null_and_unchanged_basics() -> None:
+    output = format_rows_as_markdown(["a", "b"], [(None, 150.0)])
+
+    assert output.splitlines() == ["| a | b |", "| --- | --- |", "| NULL | 150.0 |"]
+
+
+def test_markdown_truncates_oversized_cell() -> None:
+    output = format_rows_as_markdown(["c"], [("x" * 5_000,)])
+    lines = output.splitlines()
+
+    # One data row, one bounded cell, marker appended inside the cell.
+    assert len(lines) == 3
+    assert lines[-1].endswith("[Truncated] |")
+    assert len(lines[-1]) <= MAX_CELL_CHARS + len("[Truncated]") + 4
+
+
+def test_markdown_escapes_column_names_too() -> None:
+    output = format_rows_as_markdown(["name|extra"], [("v",)])
+
+    assert output.splitlines()[0] == "| name\\|extra |"
+
+
+@pytest.mark.asyncio
+async def test_execute_sql_query_escapes_adversarial_cells(demo_db_engine: AsyncEngine) -> None:
+    async with demo_db_engine.begin() as conn:
+        await conn.execute(
+            text("INSERT INTO orders (customer, amount) VALUES ('Al|ice\nEvil', 7.5)")
+        )
+
+    output = await execute_sql_query(
+        "SELECT customer, amount FROM orders ORDER BY id", engine=demo_db_engine
+    )
+    lines = [line for line in output.strip().split("\n") if line.strip()]
+
+    # Header + divider + 4 data rows: the injected pipe and newline are escapes within
+    # one cell, not structure that adds a line or column.
+    assert len(lines) == 6
+    assert "Al\\|ice\\nEvil" in output