Skip to content

refactor(agent): replace read tool with mbtx file reads - #1359

Open
bobzhang wants to merge 3 commits into
mainfrom
codex/replace-read-tool-with-mbtx
Open

refactor(agent): replace read tool with mbtx file reads#1359
bobzhang wants to merge 3 commits into
mainfrom
codex/replace-read-tool-with-mbtx

Conversation

@bobzhang

@bobzhang bobzhang commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Agents currently have two ways to read files: the standalone read tool and mbtx. Remove read from the main, worker, explore, review, and audit toolsets, and route file reads through mbtx.

Add a compact system-prompt example that selects an inclusive line range, uses an array comprehension to format original line numbers (1 | ...), and prints each excerpt directly. File errors propagate normally, and selected lines are printed in full. Update skill and bundled-documentation instructions, generated prompts, registry expectations, and the eval harness. Keep the historical read-result decoder and renderer so saved transcripts still display correctly.

Validation:

  • moon info && moon fmt
  • just check
  • just test (native, JS, and offline CLI cases)
  • just build
  • All five prompt copies are identical. Extracted the exact compact snippet and ran it through the real sandboxed mbtx tool using a temporary test harness: numbered output, lines 80–119, empty files, past-EOF ranges, and missing-file error propagation passed. The temporary harness was removed after verification.

@bobzhang

bobzhang commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

Holding this one. The direction is right — mbtx is strictly more expressive than a fixed-shape read, and we do want one way to read a file, not two — but we want to improve the mbtx experience and grow the supporting libraries first. Once that's in place, removing read is the natural last step. It isn't ready yet.

Evidence behind both halves of that, from an A/B on this branch.

The removal itself does not cost artifact quality

Four runs building a TOML v1.0.0 parser from an empty module (2 per arm: this branch vs its merge-base), deepseek-v4-pro, --thinking high, --max-steps 150, scored against a held-out 73-document corpus cross-checked with CPython tomllib. No run saw the corpus, and none hit the step ceiling.

base_r1 base_r2 pr_r1 pr_r2
valid accepted 35/35 35/35 31/35 34/35
invalid rejected 38/38 37/38 38/38 37/38
classification 73/73 72/73 69/73 71/73
values exact 31/32 31/32 27/28 31/31

All four produced a working parser and passed their own gates. n=2 per arm can't support a few points on a 73-point scale, and none of the parser bugs trace back to file reading: both baseline runs hit the same naive float conversion (6.626e-346.626000000000003e-34), while pr_r1 has an off-by-one in blank-line skipping (one blank line rejected, two accepted) and pr_r2 misses [x.y.z.w] followed by [x].

Wall clock did not separate the arms either. Three of four runs called review and all three hit the 15-minute deadline and errored; subtracting that dead time gives 3,137 s vs 3,047 s — a wash. (That review timeout is a separate problem, unrelated to this PR.)

What isn't ready

The prompt block is not being followed. Across both PR-arm runs, in 23 snippets that read a file:

  • the taught excerpt helper was used 0 times
  • 8 of 23 printed a whole file with no range at all
  • 8 of 23 reported a line total

That last one matters because it used to be free. read ended every result with start_line / shown_lines / total_lines / truncated, so the model always knew whether its window covered the file. The snippet prints only the range, and a past-EOF read comes back as a bare path with no indication anything was wrong, while an empty file renders as a phantom 1 | line.

The unbounded whole-file reads are the sharper risk, and the eval understates it: these runs worked in a fresh empty module with tiny files. In this repo, reading a whole .mbt of any size or a generated .mbti runs into mbtx's 48 KB OutputCapBytes, which kills the program (kill_when_full), so the model gets a truncated body plus an exit code that isn't the program's. read capped at 12 KB by default and returned a clean prefix with truncated=true. That guarantee should come back as tested code before the tool providing it goes away.

Two smaller things in the same category: a Windows path pasted into a snippet is either a compile error ("C:\Users\..."Invalid escape sequence: \U) or silently wrong ("C:\temp\notes.md"C:<TAB>emp<NL>otes.md), where a JSON path argument had no such hazard; and deleting read.mbt drops ~15 tests (ranges, past-EOF, empty files, truncation, directories, workspace-relative paths) with nothing replacing them — the snippet lives only in prose across five prompt files, so nothing in CI would notice if it stopped working.

Cost side, for completeness

Reads through mbtx measured 542 ms median (n=19) against 1 ms for read (n=15). Almost all of that is #1360mbtx allocates a fresh --target-dir per call, so dependencies recompile every time (537 ms cold vs 279 ms warm with a new body, 44 ms warm with the same body; moonrun itself is 40 ms). Not inherent, and worth fixing regardless of what happens to read.

Also worth noting the fixed overhead moved the wrong way: identical task and workspace, first request was 20,025 prompt tokens on base against 20,094 here. The 28-line block across five prompts costs slightly more than the tool schema it removes — which a library import would reverse.

Plan

  1. mbtx experience work, starting with the build cache in mbtx: reuse a turn-scoped build cache instead of a fresh --target-dir per call #1360.
  2. Libraries — a tested excerpt/read_lines among them, so the caps, the line total, per-file error isolation and path handling are code with tests, and the prompt is one line instead of 28.
  3. Then remove read, which is this PR rebased on top.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant