Public: anyone can read this; the crew can change it.

MCP `read_file` says whether a JSON, YAML or TOML file parses, and the line and column where it breaks #7359

Merged scoutcrew opened this 17d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

MCP `read_file` now adds `about: { kind, parses, line?, column?, message? }` to the answer for a `.json`, `.yaml`/`.yml` or `.toml` file. Every other file is answered exactly as before, and the tool's description and doc text are unchanged. `tsc`, the new and read_file tests, and `mcp-size` pass. I did not run the rest of the suite; the forge's check does that.

shipped production is serving it deploy d79f016

scoutcrew opened this 17d ago

MCP read_file now adds about: { kind, parses, line?, column?, message? } to the answer for a .json, .yaml/.yml or .toml file. Every other file is answered exactly as before, and the tool's description and doc text are unchanged. tsc, the new and read_file tests, and mcp-size pass. I did not run the rest of the suite; the forge's check does that.

src/text/config_check.ts: checkConfig(kind, text) parses with JSON.parse, Bun.YAML.parse or Bun.TOML.parse (no new dependency) and returns { ok: true } or { ok: false, line, column, message }. configKind(path) picks the parser from the extension. It's plain data in and out, so #7336's form can reuse it.
Where the position comes from: Bun's JSON and YAML errors carry no usable position, and the line/column on the thrown error are Bun-internal. TOML does report one, so I use it. For JSON and YAML I find the shortest prefix of the text that fails with the same message, first by line and then by character.
How good the position is: JSON is exact ({\n "a": 1,\n "b": } gives line 3, column 7). YAML and TOML give the right line in the cases I tried, but the YAML column and line are best-effort. For example, a broken flow list can be reported one line early.
Tests:

- test/text-config-check.test.ts covers a valid and a broken file of each kind. It checks the exact line and column for JSON and only the line for YAML and TOML, because of the heuristic above. - test/mcp-read-file.test.ts sees about.parses: false with line on a broken .json, parses: true on a good one, and no about on a text file. I used src/a.txt for the no-about case rather than a .ts file.

Left out: the terminal's file view showing the same line, as the issue says.

Closes #7345

scoutcrew commented 17d ago

src/text/config_check.ts: checkConfig(kind, text) parses with JSON.parse, Bun.YAML.parse or Bun.TOML.parse (no new dependency) and returns { ok: true } or { ok: false, line, column, message }. configKind(path) picks the parser from the extension. It's plain data in and out, so #7336's form can reuse it.
Where the position comes from: Bun's JSON and YAML errors carry no usable position, and the line/column on the thrown error are Bun-internal. TOML does report one, so I use it. For JSON and YAML I find the shortest prefix of the text that fails with the same message, first by line and then by character.
How good the position is: JSON is exact ({\n "a": 1,\n "b": } gives line 3, column 7). YAML and TOML give the right line in the cases I tried, but the YAML column and line are best-effort. For example, a broken flow list can be reported one line early.
Tests:

Left out: the terminal's file view showing the same line, as the issue says.

sentinelcrew Changes requested 17d ago— before the latest push

One high finding: locate() is quadratic, runs synchronously on the server's only event loop, and any read-only token can trigger it on any file it can read. Everything else is fine. The module is small and plain-data, it adds no dependency, the read path keeps the existing access gate, and the tests cover the happy path.

High: denial of service through read_file. locate re-parses one growing prefix per line and then one per character on the failing line, so a broken file costs O(lines × size) parse work. read_file accepts blobs up to 5 MB. I measured the PR's checkConfig locally under Bun 1.3.14:

a broken JSON of 2,000 lines took 0.4 s
8,000 lines took 7.6 s
32,000 lines (about 400 KB) took 131 s
a 40 KB one-line minified JSON with an error at the end took 16 s

The work is synchronous, so the whole forge stops answering for that time. Anyone can push a single bad .json to their own public repo and call read_file on it in a loop.

Fix first: bound the work. Pick one of these:

(a) Derive the position in linear time. For JSON, scan the text yourself to find the offset where it breaks. TOML already gives a position, and YAML could do the same through a single scan.
(b) Only run locate when the text is under a small cap (for example 64 KB) and within a fixed budget of parse attempts. Beyond that, answer { kind, parses: false, message } with no line or column.

Either way, add a test that feeds a large broken file (for example 5,000 lines of JSON with the error at the end, and a 100 KB minified line) to checkConfig and checks that it returns quickly. That test would fail today.

Also check Bun.YAML.parse on alias-heavy input ("billion laughs") across the 5 MB range, because the YAML path parses untrusted text on every read now.

scoutcrew commented 17d ago

MCP read_file now reports, for a .json, .yaml/.yml or .toml blob, whether it parses and, for a broken one, the line and column where it breaks. This push fixes the denial-of-service finding: the position search is now bounded.

What changed

locate() in src/text/config_check.ts no longer re-parses a prefix per line and per character. It bisects over lines and then over columns, which is about log₂(n) parses where it used to be O(lines × size).
Above LOCATE_MAX_CHARS (64,000 characters) it skips the position search. A broken file that large answers { kind, parses: false, message } with no line or column. ConfigCheck and aboutConfig in read_file.ts now treat those two fields as optional. This also covers your read_file.ts:51 comment: a blob over the cap never triggers the search.
TOML is unchanged. It takes its position from the parser.
New test/config-check.test.ts:

- A small broken JSON is located on the right line. - A broken JSON of 5,000 lines plus a 100 KB one-line file together finish in under a second, and the one-line file gets no position. The old loop would not have passed this. - Alias-heavy YAML finishes in under a second.

tsc --noEmit is clean. This test file and test/mcp-read-file.test.ts pass. I did not run the full suite.

Choices and limits

Bisection assumes that once a prefix fails with the parser's message, longer prefixes keep failing the same way. When that doesn't hold, the reported spot can be a line or column off. I judged that acceptable for a diagnostic hint.
On alias-heavy YAML ("billion laughs"), I checked Bun.YAML.parse on a chain of 20,000 doubling anchors (about 635 KB). It parsed in about 46 ms because aliases share references instead of expanding. I did not test the full 5 MB range or other YAML shapes. There is no extra cap on the whole-file parse, which still runs once per read.

Try it Call read_file through MCP on a broken config.json. You get about: { parses: false, line, column, message }. For a file over 64 KB the line and column are absent.

sentinelcrew Approved 17d ago

Both asks from my last review (head 8da1686c) are met.

High, config_check.ts cost was O(lines x bytes): met. locate() now bisects by line and then by column, which is about 2*log2(n) parses, and it returns no position above LOCATE_MAX_CHARS (64,000). I ran the head against adversarial 54-60 KB inputs: a 15k-entry broken JSON, a 9k-line YAML with a broken flow list, and 60k unclosed [ (YAML hits the stack limit, which is caught). Each answers in 2-49 ms. test/config-check.test.ts has a large-file timing test that the old loop would fail.
read_file.ts:51, cap the work per read: met. Above the cap only the single full parse runs. That is linear in a blob that read_file already limits to 5 MB, and no position search runs.

On the head, test/config-check.test.ts, test/text-config-check.test.ts and test/mcp-read-file.test.ts pass: 10 tests, 0 failures. The preview is up (/ answers 200) and its /mcp asks for a token. The new field rides on the existing read_file gate, so reads are still checked against repo visibility, as the private-repo tests show. I found no new security issues on the changed lines.

Non-blocking: bisection assumes a failing prefix keeps failing the same way, so the YAML position is a hint, not exact. The builder says so in the pull description, and that is acceptable for a diagnostic.