chore(agent): phase roadmap from TODO.md, 7 phases (127-133)

Converts the TODO list into the phased-execution roadmap (Protocol B
append): incremental folder summaries on sync (127), the mobile sync
label with the pinned (n/m) count (128), per-chunk embed progress in
the sync label (129), chat answer clusters with the bottom-right
actions row (130), read(offset/limit) line ranges for huge documents
(131), visible chat image-upload feedback (132), and the garbage-PDF
text gate with the page-screenshot vision fallback (133 — includes
the diagnosed root cause of the hnb guide's pre-phase-126 binary
import). Each phase carries its dedicated in-isolation Playwright
suite; TODO.md is cleared (items now live in .agents/phases/todo/).

LLM-Generated: true
This commit is contained in:
2026-09-27 12:34:16 -04:00
parent bdeea71f5d
commit 0c8018971f
35 changed files with 1207 additions and 8 deletions
@@ -0,0 +1,51 @@
# Phase 131 — `read` gains line ranges: grep finds the line, read(offset, limit) reads it
**Source:** `TODO.md` L7 — "The agent is struggling with huge documents. It can search for the text it wants and it finds the text, including line number, but then it gets stuck saying 'However, read takes a path, not a line number.' and can't figure out how to read that part of the document. Does the read tool support that? If not, it should. If it does, improve the description so the agent doesn't get stuck. I have a huge test pdf (the DND player's handbook) in test_sources called 'pdf_tests.tar' that you can use for testing. It's a massive pdf. The question the agent got stuck on was 'What are the components of the find traps spell?' which is toward the end of the players handbook so it will need to search and read specific lines. I've been setting BOR_READ_MAX_CHARS to 24000 to keep things relatively quick so the agent realizes quickly its answer isn't in the first few pages."
(LOCKED A2, owner-confirmed 2026-09-27: the parameter names are `offset`/`limit` — 1-based line number + line count — mirroring the pi-harness `read` tool the model was trained on, the phase-70 "harness-aligned surface" anchor.)
**Story:** n/a (TODO-derived phase, Protocol B append).
**Context:** `app/rag/agent.py` — `AGENT_TOOLS` (L258+: the `ls`/`read`/`grep` pi.dev-shaped schemas; the `read` schema at ~L304 takes ONLY `path`; the `grep` description promises `source/path:line: text` locators), `_execute_tool` (L1266+, the `read` branch at ~L1348: `arg` resolution, the `ALREADY_IN_CONTEXT` dedupe on `holder.read_docs`, the `BOR_READ_MAX_CHARS` truncation with `TRUNCATION_MARKER` + `READ_TRUNCATION_NOTICE` (~L506 — today it points at `grep` and then dead-ends), the `holder.read_truncations` tuples), `AgentHolder` (the `read_docs` / `read_truncations` bookkeeping); `app/rag/llm.py` — `ToolCallPiece` (the `arguments` dict the model sends); `tests/e2e/mock_llm.py` — the scripted tool-call flows (the "use your tools" / multi-read / grep patterns — the module header documents each trigger); `tests/unit/test_agent.py` — the read-branch unit suite (the `ALREADY_IN_CONTEXT` pins, the truncation pins); `test_sources/players_handbook.pdf` (the DND player's handbook from `pdf_tests.tar` — on disk, untracked); `settings.read_max_chars` (`BOR_READ_MAX_CHARS`).
## Objective
The agent stops dead-ending on huge documents: `read` accepts an optional 1-based line `offset` + line-count `limit`, the truncation notice and tool descriptions teach the grep → read(offset/limit) workflow, and the dedupe rules allow the ONE case that matters — re-reading a range of a document whose first read was truncated — while still refusing pointless re-reads of text already in context. The owner's repro (the handbook, "find traps", `BOR_READ_MAX_CHARS=24000`) passes as an integration test.
## Dependencies
- `130_chat_answer_clusters` (todo) — pipeline order only; code dependencies are the completed agent surface (phases 70/72/95/118).
## Design (shared by all tasks — the executor reads this, not the chat)
- **Schema + teaching (task 01):** `read` gains two OPTIONAL integer params — `offset` ("The first line to read, 1-based. Omit it to read from the beginning of the document.") and `limit` ("The maximum number of lines to read. Omit it to read as many lines as fit under the character cap."). `required` stays `["path"]`. The `read` description keeps its existing teaching and GAINS the workflow: for a very large document, use `grep` to find where your text is (it reports line numbers), then `read` that part with `offset` + `limit`; re-reading a document already read in full adds nothing. `READ_TRUNCATION_NOTICE` (L506) is REWRITTEN to end the dead-end: "…The rest is NOT shown. Use grep (pattern) to find what you need — it reports line numbers — then read that part with `offset` (1-based first line) and `limit` (number of lines)." The `grep` description gets the light touch: "read the winner with `read` — for a large document, use the reported line number as `offset`."
- **Execution (task 02):** in `_execute_tool`'s read branch:
- Validation: `offset`/`limit`, when present, must be ints ≥ 1 (NOT bools — the house `isinstance(x, int) and not isinstance(x, bool)` guard); a bad value → a teaching refusal echoing the bad value ("`offset` and `limit` are whole line numbers ≥ 1 — you passed `…`") — counts in nothing, consumes a round (the phase-72 refusal style).
- The dedupe matrix (`known` = the `read_docs` (source, path) set; `truncated` = the set of (source, path) from `holder.read_truncations`):
| call | doc in `known` | previously truncated | result |
|---|---|---|---|
| plain `read(path)` | no | — | full read (truncate at the cap as today) |
| plain `read(path)` | yes | yes | NEW teaching line (no re-emit, no count): "You already have the first {shown} characters of {source}/{path} in your context. To read the rest, find where your text is with grep (it reports line numbers), then call read with `offset` (1-based first line) and `limit` (lines)." — `{shown}` from the matching `read_truncations` tuple |
| plain `read(path)` | yes | no | `ALREADY_IN_CONTEXT` (byte-identical to today) |
| `read(path, offset, limit)` | yes | yes | ALLOWED (the stuck-scenario fix — the range the model names is exactly the part it does NOT have) |
| `read(path, offset, limit)` | yes | no | `ALREADY_IN_CONTEXT` (the full text is already in context — the range is a subset) |
| `read(path, offset, limit)` | no | — | ALLOWED (a range read of an unread doc) |
- `offset > total_lines` (the doc's `len(content.splitlines())`) → teaching refusal: "{source}/{path} is {total_lines} lines long — `offset` {offset} is past the end. Use a smaller offset (grep reports the exact line numbers)." Counts in nothing.
- Result shape: line 1 `Document {source}/{path}:` + line 2 `date: YYYY-MM-DD` stay BYTE-IDENTICAL for plain reads (the E2E mock's `_READ_RESULT_PREFIX` contract); a RANGE read inserts a third line `lines: {offset}-{end} of {total_lines}` (the actual end = min(offset + limit − 1, total_lines); the model sees the true extent), then the image-doc marker (as today), then the slice (`"\n".join(lines[offset-1 : offset-1+limit])`).
- The cap still applies to the SLICE: `len(slice) > settings.read_max_chars` → cut + `TRUNCATION_MARKER` + the (rewritten) notice; the `read_truncations` tuple for a range read is `(arg, cap, len(slice))` — the slice's requested size (documented: for range reads the tuple's `chars_total` is the slice length, not the document's).
- A range read appends the doc to `read_docs` ONCE (the `known` set prevents a re-append) — so a later plain read of a range-read doc is `ALREADY_IN_CONTEXT` (the model has committed to this doc; the range result's `lines:` line told it what it holds).
- **Handbook repro (task 03):** an integration test that imports `test_sources/players_handbook.pdf` as a local source (the mock LLM's deterministic embed/summary fakes — the in-process `tests/fakes.py` pattern; the handbook is ~1.5 MB of text ≈ 1200 chunks — in-process embedding is fast, keep the test under ~60 s) and drives the agent turn "What are the components of the find traps spell?" with `BOR_READ_MAX_CHARS=24000`: a scripted fake (the `test_agent.py` pattern) — round 1 → a `grep` tool call (pattern "find traps"), round 2 (the grep result carries the real line number) → a `read(path, offset=<that line>, limit=40)` call, round 3 → the answer echoing the read result's content (the phase-94 "read document reached the context" echo convention). Assert the echo contains the handbook's actual find-traps text (grep it from the extracted PDF text in the test — do not hardcode the handbook wording).
- **Mock flow + E2E (task 04):** `tests/e2e/mock_llm.py` gains ONE scripted flow (house style — a user-message marker, deterministic reply, module-header docstring entry): the marker `read the lines` → request 1: a `grep` tool call (a canned pattern); request 2 (a `tool`-role grep result with a `source/path:line:` line present): a `read` tool call with `offset` = the parsed line number and `limit` = 30; request 3 (the read result present): the answer "Found it at line N." + the first 80 chars of the read result (the existing read-echo shape). `tests/e2e/test_read_line_ranges.py` (dedicated, in isolation, mock-only): a KB fixture with ONE large doc (> 24000 chars, the marker line `TRAP_COMPONENTS_LINE` near line ~1500), `BOR_READ_MAX_CHARS=24000`; ask a question containing the marker → assert the SSE `tool` frames (grep, then read with `offset`/`limit` args) and the final answer echoing the marker line; a second test: a plain read of the large doc truncates with the REWRITTEN notice (the notice names `offset`/`limit` — assert the `tool_result`/result surface carries it).
## Tasks
1. `01_tool_surface.md` — the schema, the descriptions, the rewritten truncation notice.
2. `02_range_read_execution.md` — the read branch: validation, the dedupe matrix, the slicing, the result shape.
3. `03_handbook_repro.md` — the integration repro (the handbook + the owner's exact question) + the unit matrix.
4. `04_mock_flow_e2e.md` — the mock-LLM `read the lines` flow + the dedicated E2E.
## Testing & Quality
- Unit: the full dedupe matrix (6 rows), the validation refusals, the slice math (boundaries, offset past EOF, 1-line docs), the result shapes (plain reads byte-identical — the existing pins must pass unchanged), the cap on slices.
- Integration: the handbook repro (the owner's scenario, the owner's `BOR_READ_MAX_CHARS=24000`).
- E2E: one dedicated Playwright file, run in isolation, mock-only.
- Coverage: **>90%** on new/modified code (`app/`).
## Completion Criteria
- [ ] `read(path, offset, limit)` returns exactly the named lines with the `lines: a-b of N` header; plain reads are byte-identical to pre-phase
- [ ] a truncated first read followed by a range read SUCCEEDS (the stuck scenario is gone); every other re-read is refused with the teaching
- [ ] the handbook integration test answers the find-traps question from the real PDF text (no hardcoded wording)
- [ ] `uv run pytest --cov=app --cov-report=term-missing` green, `app/` coverage >90%; `uv run pytest tests/e2e/test_read_line_ranges.py -v --no-cov` green in isolation
- [ ] no behavior change in completed phases (the grep/ls surfaces, the plain-read contract, the `ALREADY_IN_CONTEXT` full-read refusal)