diff --git a/CHANGELOG.md b/CHANGELOG.md index 5686e28..c8801bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,14 @@ and this project adheres to [Clever Semantic Versioning](https://www.w3.org/subm ### Added +- **Offset-Based Pagination for `file_read` (issue #83)** (`agents/tool.py`, `agents/llm_tools.py`): Adds an optional `offset` parameter so large files can be read across multiple bounded `file_read` calls instead of only ever seeing the first `max_chars` characters. + + `offset` is a character position (consistent with the existing `max_chars` unit) defaulting to `0`; omitting it reproduces the prior behavior unchanged. Combined with `max_chars`, it returns the window `[offset, offset + max_chars)`. Truncated responses now include a `MORE_CONTENT_AT_OFFSET: ` marker in the `[FILE_READ_SUCCESS]` header giving the exact offset for the next call, so the caller never has to compute `offset + max_chars` itself. Requesting an offset at or beyond the end of the file returns a `NO_MORE_CONTENT_AT_OFFSET` success response with empty content rather than an error; a negative or non-integer offset raises `ExecutionError`. + + See `docs/adr/ADR-2033-file-read-offset-pagination.md` for the full set of design decisions (unit choice, continuation-signaling format, out-of-range semantics, and why directory-listing pagination and byte-level seeking are deferred to a future issue). + + **Module:** `src/cleveractors/agents/tool.py` (`ToolAgent._file_read_tool`), `src/cleveractors/agents/llm_tools.py` (`_BUILTIN_TOOL_SCHEMAS["file_read"]`). BDD: scenarios in `features/tool_coverage_gaps.feature`. + - **Real `type: tool` Graph Node Execution via ToolAgent Dispatch (issue #75)** (`langgraph/nodes.py`, `agents/tool.py`, `runtime_dispatch.py`, `langgraph/pure_graph.py`): Wiring that lets graph nodes declared with `type: tool` execute real tool calls through `ToolAgent.invoke()` instead of returning no-ops. `NodeConfig` gains two new fields — `tool: str | None` and `static_config: dict[str, Any]` — populated from the `tool:` and `config:` YAML keys in graph definitions. `Node._execute_tool()` is no longer a stub; it now creates a `ToolAgent` instance, invokes it with the static config merged with the threaded previous node output, and returns a structured tool `ToolMessage`. diff --git a/docs/adr/ADR-2033-file-read-offset-pagination.md b/docs/adr/ADR-2033-file-read-offset-pagination.md new file mode 100644 index 0000000..7da5e73 --- /dev/null +++ b/docs/adr/ADR-2033-file-read-offset-pagination.md @@ -0,0 +1,273 @@ +# ADR-2033: `file_read` Offset-Based Pagination — Specification Extension + +**Status:** accepted + +**Date:** 2026-07-30 + +**Author:** Luis Mendes (CoreRasurae) + +**Issue:** #83 — feat(agents): add offset-based pagination to file_read + +--- + +## Context + +`ToolAgent._file_read_tool` (`cleveractors.agents.tool`) always reads a file +from the beginning: `f.read()` loads the entire file, and the optional +`max_chars` argument (ADR-2030 D-3) only bounds where the returned content +*stops* — it always slices `content[:max_chars]`, i.e. from position 0. +There is no `.seek()`/`.tell()` or equivalent anywhere in the codebase. + +Once a large file is truncated at `max_chars`, there is no way to retrieve +the *next* chunk. The `file_read` tool schema (`llm_tools._BUILTIN_TOOL_SCHEMAS +["file_read"]`) explicitly steers the LLM toward truncated reads for large +files ("Use 8000 for scanning, 16000 for analysis"), but gives it no +built-in mechanism to continue past the truncation point. The only +workaround today is falling back to the `shell` tool (`tail -c +N`, +`sed -n`, `dd skip=...`) — the same class of awkward, error-prone detour +that ADR-2030 D-4 already eliminated for shell-commands-mistakenly-passed- +as-file-paths. An `offset` argument closes this gap: it lets the LLM page +through a large file across multiple `file_read` calls without shell +fallbacks and without re-spending context re-reading content it already +saw. + +This ADR resolves the open design questions the issue identified: the unit +of `offset`, the continuation-signaling format, out-of-range semantics, +whether directory-listing pagination is in scope, and whether byte-level +seeking is required for v1. + +--- + +## Decision + +### D-1: Unit of `offset` — Characters + +**What:** `offset` is measured in characters — the same unit `max_chars` +already uses — not bytes and not lines. + +**Why characters, not bytes:** `_file_read_tool` opens files in text mode +(`open(filepath, "r", encoding="utf-8")`), so `f.read()` already returns a +decoded `str`; every existing size computation (`char_count = len(content)`) +counts characters. UTF-8 is not fixed-width, so a raw byte offset could +land mid-sequence of a multi-byte character, corrupting the decoded +output or raising a decode error exactly at the pagination boundary. +Since content is decoded before any slicing occurs, byte-offset seeking +would require either re-encoding to find a safe boundary or separate +raw-byte I/O — strictly more complexity for no benefit, given `max_chars` +already committed this tool to character-counting. + +**Why not lines:** `max_chars` already counts characters. Giving `offset` +a different unit would force the LLM (and any human reading the output) +to convert between two counting systems just to combine the two +parameters into one window — defeating the purpose of pairing them. + +### D-2: Windowing Semantics — `[offset, offset + max_chars)` + +**What:** `offset` seeks to a character position within the file. Combined +with `max_chars`, the tool returns the window +`content[offset:window_end]`, where: + +```text +window_end = char_count if max_chars is None + = min(char_count, offset + max_chars) otherwise +``` + +**Why:** This collapses to the pre-existing `content[:max_chars]` behavior +exactly when `offset` is omitted (i.e. defaults to `0`) — satisfying the +regression requirement that omitting `offset` reproduces byte-for-byte +identical output to current behavior. It also generalizes cleanly to +pagination: each subsequent call passes `offset = `, producing contiguous, non-overlapping windows across calls. + +### D-3: Continuation Signaling — Header Markers + +**What:** The `[FILE_READ_SUCCESS]` header is extended with two new, +mutually-exclusive markers: + +- `MORE_CONTENT_AT_OFFSET: ` — appended whenever the returned window + does not reach the end of the file (i.e. whenever the existing + `TRUNCATED` condition is true). `` is exactly `window_end` from D-2 — + the offset value to pass in the next call to continue reading where + this call left off. This lets the LLM continue paging without computing + `offset + max_chars` itself. +- `NO_MORE_CONTENT_AT_OFFSET: ` — used only in the + at/beyond-EOF case (D-4); see below. + +`Lines` and `Size` in the header always describe the **whole file**, not +the returned window — this was already true of the pre-existing +`max_chars` behavior (`char_count`/`line_count` are computed from the full +`f.read()` before any slicing) and is preserved unchanged so the LLM +always has a stable reference point for the file's total size, regardless +of which window it currently sees. + +**Example — mid-file page (more content remains):** + +```text +[FILE_READ_SUCCESS]📄 File: big.py | Lines: 900 | Size: 30000 chars | TRUNCATED to 8000 chars | MORE_CONTENT_AT_OFFSET: 8000 +[FILE_CONTENT_START] +... +[FILE_CONTENT_END] +``` + +**Example — final page (window reaches EOF):** + +```text +[FILE_READ_SUCCESS]📄 File: big.py | Lines: 900 | Size: 30000 chars +[FILE_CONTENT_START] +... +[FILE_CONTENT_END] +``` + +No new marker is added when the window reaches EOF — the absence of +`TRUNCATED`/`MORE_CONTENT_AT_OFFSET` already means "you have everything +from here to the end," consistent with how a plain (non-paginated) read +has always signaled completeness. + +### D-4: Out-of-Range Semantics + +- **Negative `offset`** → `ExecutionError` ("offset must be non-negative, + got ``"). +- **Non-integer `offset`** → `ExecutionError` ("offset must be an integer, + got ``"), raised via the same `int(offset)` / `except (TypeError, + ValueError)` pattern `max_chars` already uses. This intentionally + inherits the same coercion behavior as `max_chars` — e.g. a numeric + float like `3.9` is silently truncated to `3` rather than rejected, since + `int()` does not distinguish "integer-valued float" from "float". Only + genuinely non-numeric input (a string, `None`, etc.) raises. Mirroring + the established pattern exactly was chosen over inventing a stricter + check solely for `offset`, to keep the two sibling parameters behaving + identically. +- **`offset` at or beyond end-of-file** (`offset > 0 and offset >= + char_count`) → **not an error.** Returns a `[FILE_READ_SUCCESS]` response + with empty content and a `NO_MORE_CONTENT_AT_OFFSET: ` marker + (alongside the whole-file `Lines`/`Size`, so the LLM can see exactly how + far its offset overshot). + +**Why the EOF case is success, not failure:** `offset == char_count` is the +*expected* terminal state of a correctly-implemented pagination loop — it +is exactly the `MORE_CONTENT_AT_OFFSET` value returned by the previous +call, once the file has no content left. Raising an exception on this +outcome would force every pagination loop to treat its own success +condition as an error to catch. Treating "at or beyond" uniformly (rather +than special-casing `==` separately from `>`) avoids an off-by-one +distinction: an LLM that overshoots gets the same clear, actionable +"nothing more here" signal as one that lands exactly on the boundary, +without needing to special-case either. + +The `offset > 0` guard specifically preserves the regression requirement: +`offset` omitted (defaulting to `0`) on an empty file (`char_count == 0`) +must fall through to the ordinary read path and reproduce the pre-existing +empty-file response unchanged, not the new EOF marker. + +### D-5: Directory Listings — Explicitly Deferred + +**What:** `offset` is accepted and validated even when `file` resolves to +a directory, but has no effect on the directory-listing branch — it is +silently ignored there. + +**Spec relationship:** §4.5.1 permits additional arguments to be silently +ignored when not applicable; this is not a new allowance introduced by +this ADR. + +**Why deferred rather than implemented now:** Paginating a directory +listing is a distinct problem — paging over *N entries*, not *C +characters* — with its own open questions (is entry order stable across +calls? does the window count entries or rendered characters?) that are +outside this issue's scope. Left for a future issue rather than bolted on +as a second, incompatible meaning of the same parameter. + +### D-6: Efficiency — Full-File Read Retained for v1 + +**What:** `_file_read_tool` continues to read the entire file into memory +(`f.read()`) before slicing by character offset. No byte-level +`.seek()`/`.tell()` is introduced. + +**Why:** `max_chars` (ADR-2030 D-3) already reads the whole file before +truncating; `offset` extends the same in-memory slicing step and +introduces no new asymptotic cost class. True seek-based reading would +need to reconcile file-mode seeking (which operates on bytes) with the +character-based unit chosen in D-1 — since UTF-8 is not fixed-width, a +byte seek cannot be mapped to a character offset without decoding from the +start of the file anyway (except for pure-ASCII content), so it would not +actually avoid the full read in the general case. Given this tool already +accepts O(file size) memory/time cost for `max_chars`, doing the same for +`offset` is a deliberate, documented v1 choice rather than an oversight. +Should profiling later show this to be a bottleneck for very large files, +a follow-up ADR can introduce byte-seeking together with a revised offset +unit. + +--- + +## Consequences + +### Positive + +- **Closes the pagination gap:** the LLM can now retrieve the entirety of + a large file across multiple bounded `file_read` calls without ever + falling back to the `shell` tool. +- **No wasted context:** each subsequent call requests only the next + unread window, rather than re-reading (and re-spending context tokens + on) content already seen. +- **Self-describing continuation:** the LLM does not need to compute + `offset + max_chars` itself — `MORE_CONTENT_AT_OFFSET` supplies the exact + next value. +- **Backward compatible:** `offset` defaults to `0` and, when omitted, + produces byte-for-byte identical output to the pre-existing behavior. + +### Negative / Risks + +- **Output format stability:** as already noted in ADR-2030's + Consequences, the header format is not part of the normative + specification (§4.5.1 defines only the base `[FILE_READ_SUCCESS]` / + `[FILE_CONTENT_START]`/`[FILE_CONTENT_END]` envelope). Adding + `MORE_CONTENT_AT_OFFSET`/`NO_MORE_CONTENT_AT_OFFSET` further grows this + non-normative surface; any downstream code parsing `file_read` output + by exact header shape could be affected by future header changes. +- **Full-file read remains O(file size):** per D-6, very large files still + incur a full read on every paginated call, even though only a small + window is returned. This is an accepted, documented trade-off for v1, + not an oversight. + +### Follow-up Required + +- **Directory-listing pagination** (D-5) — left for a future issue. +- **Byte-seeking for large files** (D-6) — only if profiling shows the + full-read-per-call cost is a real bottleneck in practice. + +--- + +## Alternatives Considered + +### A-1: Byte-based offset + +**Rejected because:** see D-1 — would require reconciling a non-fixed-width +encoding (UTF-8) with byte positions, risking mid-character seeks, for no +benefit over the character-based unit `max_chars` already established. + +### A-2: Line-based offset + +**Rejected because:** see D-1 — would force the LLM to convert between two +different counting systems (`max_chars` in characters, a hypothetical +`offset` in lines) to combine them into one window. + +### A-3: Treat offset-beyond-EOF as an `ExecutionError` + +**Rejected because:** see D-4 — this is the expected terminal state of a +correctly-implemented pagination loop, not a caller mistake; making it an +error would force every pagination loop to treat its own success +condition as an exception to catch. + +### A-4: A separate `file_read_page`/pagination-specific tool + +**Rejected because:** mirrors ADR-2030 A-3's reasoning for `max_chars` +itself — a second tool name increases the LLM's tool-selection surface +and cognitive load. Folding `offset` into `file_read` as an optional +parameter keeps one tool name and one mental model for reading files. + +### A-5: Server-side auto-pagination (loop internally, return the whole file assembled across an internal retry loop) + +**Rejected because:** this defeats the entire purpose of bounded reads +established by ADR-2030 D-3 — keeping the LLM in control of how much +content it pulls into its own context per call. Silently assembling the +full file server-side would reintroduce the exact context-overflow risk +`max_chars` was introduced to prevent. diff --git a/features/steps/tool_coverage_gaps_steps.py b/features/steps/tool_coverage_gaps_steps.py index bff29d2..d221e9e 100644 --- a/features/steps/tool_coverage_gaps_steps.py +++ b/features/steps/tool_coverage_gaps_steps.py @@ -1033,6 +1033,145 @@ def step_fr_truncated(context): assert "TRUNCATED" in context.result +# ── file_read offset pagination ───────────────────────────────────────── + + +def _fr_extract_content(response): + return response.split("[FILE_CONTENT_START]\n", 1)[1].rsplit( + "\n[FILE_CONTENT_END]", 1 + )[0] + + +@when("I call file read tool with and without offset zero") +def step_fr_offset_zero(context): + a = ToolAgent("tc", {"tools": ["file_read"]}) + fp = context.td / "fr_offset_zero.txt" + fp.write_text("hello world") + context.result = _run( + a._file_read_tool({"file": str(fp)}, context={"_unsafe_mode": True}) + ) + context.result_with_offset = _run( + a._file_read_tool( + {"file": str(fp), "offset": 0}, context={"_unsafe_mode": True} + ) + ) + context.result_with_null_offset = _run( + a._file_read_tool( + {"file": str(fp), "offset": None}, context={"_unsafe_mode": True} + ) + ) + + +@then("the two results should be identical") +def step_fr_results_identical(context): + assert context.result == context.result_with_offset + assert context.result == context.result_with_null_offset + + +@when("I call file read tool with an offset mid-file") +def step_fr_offset_mid(context): + a = ToolAgent("tc", {"tools": ["file_read"]}) + fp = context.td / "fr_offset_mid.txt" + fp.write_text("abcdefghij") + context.result = _run( + a._file_read_tool( + {"file": str(fp), "offset": 3}, context={"_unsafe_mode": True} + ) + ) + + +@then("the result should contain the windowed content") +def step_fr_windowed_content(context): + assert _fr_extract_content(context.result) == "defghij" + + +@when("I call file read tool twice paginating with offset and max_chars") +def step_fr_paginate(context): + a = ToolAgent("tc", {"tools": ["file_read"]}) + fp = context.td / "fr_paginate.txt" + full_content = "abcdefghijklmnopqrst" # 20 chars, no repeats + fp.write_text(full_content) + context.full_content = full_content + context.page1 = _run( + a._file_read_tool( + {"file": str(fp), "offset": 0, "max_chars": 10}, + context={"_unsafe_mode": True}, + ) + ) + context.page2 = _run( + a._file_read_tool( + {"file": str(fp), "offset": 10, "max_chars": 10}, + context={"_unsafe_mode": True}, + ) + ) + + +@then("the combined pages should equal the full file content") +def step_fr_combined_pages(context): + assert "MORE_CONTENT_AT_OFFSET: 10" in context.page1 + assert "MORE_CONTENT_AT_OFFSET" not in context.page2 + combined = _fr_extract_content(context.page1) + _fr_extract_content(context.page2) + assert combined == context.full_content + + +@when("I call file read tool with a negative offset") +def step_fr_negative_offset(context): + a = ToolAgent("tc", {"tools": ["file_read"]}) + fp = context.td / "fr_negative.txt" + fp.write_text("sample text") + context.error = _run( + _catch( + a._file_read_tool, + {"file": str(fp), "offset": -1}, + {"_unsafe_mode": True}, + ) + ) + + +@when("I call file read tool with a non-integer offset") +def step_fr_non_integer_offset(context): + a = ToolAgent("tc", {"tools": ["file_read"]}) + fp = context.td / "fr_non_integer.txt" + fp.write_text("sample text") + context.error = _run( + _catch( + a._file_read_tool, + {"file": str(fp), "offset": "not_a_number"}, + {"_unsafe_mode": True}, + ) + ) + + +@when("I call file read tool with offset at end of file") +def step_fr_offset_at_eof(context): + a = ToolAgent("tc", {"tools": ["file_read"]}) + fp = context.td / "fr_at_eof.txt" + fp.write_text("0123456789") + context.result = _run( + a._file_read_tool( + {"file": str(fp), "offset": 10}, context={"_unsafe_mode": True} + ) + ) + + +@when("I call file read tool with offset beyond end of file") +def step_fr_offset_beyond_eof(context): + a = ToolAgent("tc", {"tools": ["file_read"]}) + fp = context.td / "fr_beyond_eof.txt" + fp.write_text("0123456789") + context.result = _run( + a._file_read_tool( + {"file": str(fp), "offset": 500}, context={"_unsafe_mode": True} + ) + ) + + +@then("the result should signal no more content") +def step_fr_no_more_content(context): + assert "NO_MORE_CONTENT_AT_OFFSET" in context.result + assert "[FILE_CONTENT_START]\n[FILE_CONTENT_END]" in context.result + + # ── file_read general exception ─────────────────────────────────────── diff --git a/features/tool_coverage_gaps.feature b/features/tool_coverage_gaps.feature index da03374..dae449d 100644 --- a/features/tool_coverage_gaps.feature +++ b/features/tool_coverage_gaps.feature @@ -373,6 +373,41 @@ Feature: Tool Agent Coverage Gaps When I call file read tool with small max_chars limit Then the result should be truncated + Scenario: File read tool offset zero reproduces default behavior + Given a tool agent coverage test environment + When I call file read tool with and without offset zero + Then the two results should be identical + + Scenario: File read tool offset returns content starting at that position + Given a tool agent coverage test environment + When I call file read tool with an offset mid-file + Then the result should contain the windowed content + + Scenario: File read tool pages through a file across two calls with no gap or overlap + Given a tool agent coverage test environment + When I call file read tool twice paginating with offset and max_chars + Then the combined pages should equal the full file content + + Scenario: File read tool validates negative offset + Given a tool agent coverage test environment + When I call file read tool with a negative offset + Then the file read raises offset must be non-negative + + Scenario: File read tool validates non-integer offset + Given a tool agent coverage test environment + When I call file read tool with a non-integer offset + Then the file read raises offset must be an integer + + Scenario: File read tool offset at end of file signals no more content + Given a tool agent coverage test environment + When I call file read tool with offset at end of file + Then the result should signal no more content + + Scenario: File read tool offset beyond end of file signals no more content + Given a tool agent coverage test environment + When I call file read tool with offset beyond end of file + Then the result should signal no more content + Scenario: File read tool raises File read failed for read error Given a tool agent coverage test environment When I call file read tool causing a read error diff --git a/src/cleveractors/agents/llm_tools.py b/src/cleveractors/agents/llm_tools.py index 3d37712..cfbab3b 100644 --- a/src/cleveractors/agents/llm_tools.py +++ b/src/cleveractors/agents/llm_tools.py @@ -97,7 +97,7 @@ _BUILTIN_TOOL_SCHEMAS: dict[str, dict[str, Any]] = { }, }, "file_read": { - "description": "Read a file or list a directory. CRITICAL: Always use max_chars when reading large files (>10KB) or the LLM context will overflow and the request will fail. For .cs/.py source files use max_chars=8000 for classification, max_chars=16000 for analysis, max_chars=32000 only for small files needing full content. For directories, returns formatted listing with file sizes.", + "description": "Read a file or list a directory. CRITICAL: Always use max_chars when reading large files (>10KB) or the LLM context will overflow and the request will fail. For .cs/.py source files use max_chars=8000 for classification, max_chars=16000 for analysis, max_chars=32000 only for small files needing full content. For directories, returns formatted listing with file sizes. To page through a file beyond the first max_chars window, call again with offset set to the MORE_CONTENT_AT_OFFSET value from the previous response.", "parameters": { "type": "object", "properties": { @@ -109,6 +109,10 @@ _BUILTIN_TOOL_SCHEMAS: dict[str, dict[str, Any]] = { "type": "integer", "description": "REQUIRED for files >10KB. Maximum chars to return. Files exceeding this will show 'TRUNCATED'. Use 8000 for scanning, 16000 for analysis. Leaving this unset on a large file WILL cause context overflow and task failure.", }, + "offset": { + "type": "integer", + "description": "Character position to start reading from (default 0). Combine with max_chars to page through a large file: the response's 'MORE_CONTENT_AT_OFFSET: N' marker gives the exact offset to pass on the next call. No effect when reading a directory.", + }, "args": { "type": "array", "items": {"type": "string"}, diff --git a/src/cleveractors/agents/tool.py b/src/cleveractors/agents/tool.py index f36ecd6..556f4af 100644 --- a/src/cleveractors/agents/tool.py +++ b/src/cleveractors/agents/tool.py @@ -617,7 +617,8 @@ class ToolAgent(Agent): async def _file_read_tool( self, args: dict[str, Any], context: Optional[dict[str, Any]] ) -> str: - """File reading tool with directory listing support and optional truncation.""" + """File reading tool with directory listing, optional truncation via + `max_chars`, and offset-based pagination via `offset` (ADR-2033).""" filepath = args.get("file", "") if "args" in args and args["args"]: filepath = args["args"][0] @@ -687,6 +688,16 @@ class ToolAgent(Agent): f"max_chars must be an integer, got {max_chars}" ) from exc + offset = args.get("offset", 0) + if offset is None: + offset = 0 + try: + offset = int(offset) + except (TypeError, ValueError) as exc: + raise ExecutionError(f"offset must be an integer, got {offset}") from exc + if offset < 0: + raise ExecutionError(f"offset must be non-negative, got {offset}") + try: if os.path.isdir(filepath): entries = sorted(os.listdir(filepath)) @@ -708,18 +719,33 @@ class ToolAgent(Agent): char_count = len(content) line_count = content.count("\n") + 1 - truncated = False - if max_chars is not None and char_count > max_chars: - content = content[:max_chars] - truncated = True + # offset == 0 always falls through here, even for an empty file, + # so the no-offset path stays byte-for-byte identical to the + # pre-offset behavior (ADR-2033 D-4). + if offset > 0 and offset >= char_count: + header = ( + f"[FILE_READ_SUCCESS]📄 File: {filepath} | " + f"Lines: {line_count} | Size: {char_count} chars | " + f"NO_MORE_CONTENT_AT_OFFSET: {offset}" + ) + return f"{header}\n[FILE_CONTENT_START]\n[FILE_CONTENT_END]" + + window_end = ( + char_count if max_chars is None else min(char_count, offset + max_chars) + ) + truncated = max_chars is not None and window_end < char_count + content = content[offset:window_end] header = ( f"[FILE_READ_SUCCESS]📄 File: {filepath} | " f"Lines: {line_count} | Size: {char_count} chars" ) if truncated: - header += f" | TRUNCATED to {max_chars} chars" + header += ( + f" | TRUNCATED to {max_chars} chars" + f" | MORE_CONTENT_AT_OFFSET: {window_end}" + ) return f"{header}\n[FILE_CONTENT_START]\n{content}\n[FILE_CONTENT_END]" except FileNotFoundError: parent_dir = os.path.dirname(filepath) or "."