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
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/7345-mcp-read-file-says-whether-a-json-yaml-o → main 2 commits fork 2 threads · 2 unresolved
5 files
changed +156
−4
src/http/mcp/tools/read_file.ts +17 −3
| @@ -2,6 +2,7 @@ import { repoDir } from "../../../git/repos.ts"; | ||
| 2 | 2 | import { runGit } from "../../../git/env.ts"; |
| 3 | 3 | import { isSafeRef, isSafeTreePath } from "../../../git/guard.ts"; |
| 4 | 4 | import { blobForRender, listTree } from "../../../git/read.ts"; |
| 5 | import { checkConfig, configKind, type ConfigKind } from "../../../text/config_check.ts"; | |
| 5 | 6 | import { good, bad, type Tool, REPO_ARG, repoArg, isAnswer } from "../shared.ts"; |
| 6 | 7 | |
| 7 | 8 | export const tool: Tool = { |
| @@ -45,15 +46,28 @@ export const tool: Tool = { | ||
| 45 | 46 | if (blob.kind === "too_large") |
| 46 | 47 | return good({ type: "file", tooLarge: true, size: blob.size }); |
| 47 | 48 | const binary = blob.kind === "binary"; |
| 49 | const content = binary ? "" : new TextDecoder().decode(blob.body); | |
| 50 | const kind = binary ? null : configKind(rawPath); | |
| 51 | const about = kind === null ? undefined : aboutConfig(kind, content); | |
| 48 | 52 | return good({ |
| 49 | 53 | type: "file", |
| 50 | 54 | ref, |
| 51 | 55 | path: rawPath, |
| 52 | 56 | size: blob.size, |
| 53 | 57 | encoding: binary ? "base64" : "utf-8", |
| 54 | content: binary | |
| 55 | ? Buffer.from(blob.body).toString("base64") | |
| 56 | : new TextDecoder().decode(blob.body), | |
| 58 | content: binary ? Buffer.from(blob.body).toString("base64") : content, | |
| 59 | ...(about ? { about } : {}), | |
| 57 | 60 | }); |
| 58 | 61 | }, |
| 59 | 62 | }; |
| 63 | ||
| 64 | function aboutConfig(kind: ConfigKind, content: string) { | |
| 65 | const check = checkConfig(kind, content); | |
| 66 | if (check.ok) return { kind, parses: true }; | |
| 67 | return { | |
| 68 | kind, | |
| 69 | parses: false, | |
| 70 | ...(check.line === undefined ? {} : { line: check.line, column: check.column }), | |
| 71 | message: check.message, | |
| 72 | }; | |
| 73 | } |
src/text/config_check.ts added +74
| @@ -0,0 +1,74 @@ | ||
| 1 | export type ConfigKind = "json" | "yaml" | "toml"; | |
| 2 | export type ConfigCheck = | |
| 3 | | { ok: true } | |
| 4 | | { ok: false; line?: number; column?: number; message: string }; | |
| 5 | ||
| 6 | /** Which parser reads a path, by extension; null for any other file. */ | |
| 7 | export function configKind(path: string): ConfigKind | null { | |
| 8 | const m = /\.(json|ya?ml|toml)$/i.exec(path); | |
| 9 | if (!m) return null; | |
| 10 | const ext = m[1]!.toLowerCase(); | |
| 11 | return ext === "json" ? "json" : ext === "toml" ? "toml" : "yaml"; | |
| 12 | } | |
| 13 | ||
| 14 | const parsers: Record<ConfigKind, (text: string) => unknown> = { | |
| 15 | json: (t) => JSON.parse(t), | |
| 16 | yaml: (t) => Bun.YAML.parse(t), | |
| 17 | toml: (t) => Bun.TOML.parse(t), | |
| 18 | }; | |
| 19 | ||
| 20 | function messageOf(err: unknown): string { | |
| 21 | const m = err instanceof Error ? err.message : String((err as { message?: unknown })?.message ?? err); | |
| 22 | return m.split("\n")[0]!.slice(0, 200); | |
| 23 | } | |
| 24 | ||
| 25 | function failsWith(kind: ConfigKind, text: string, message: string): boolean { | |
| 26 | try { | |
| 27 | parsers[kind](text); | |
| 28 | return false; | |
| 29 | } catch (err) { | |
| 30 | return messageOf(err) === message; | |
| 31 | } | |
| 32 | } | |
| 33 | ||
| 34 | /** Past this size a broken file is reported without a position: finding it costs parses. */ | |
| 35 | export const LOCATE_MAX_CHARS = 64_000; | |
| 36 | ||
| 37 | /** Smallest n in [lo, hi] with `fails(n)`, by bisection (about log2 parses); hi when none is seen. */ | |
| 38 | function firstFailing(lo: number, hi: number, fails: (n: number) => boolean): number { | |
| 39 | while (lo < hi) { | |
outdated
High: this loop parses one prefix per line, and the loop at line 48 parses one prefix per character. That makes the cost O(lines × bytes) of synchronous parsing. I measured 131 s for a 400 KB broken JSON and 16 s for a 40 KB one-line file, against a 5 MB cap in |
||
| 40 | const mid = (lo + hi) >> 1; | |
| 41 | if (fails(mid)) hi = mid; | |
| 42 | else lo = mid + 1; | |
| 43 | } | |
| 44 | return lo; | |
| 45 | } | |
| 46 | ||
| 47 | /** JSON and YAML errors carry no position of ours: bisect for the shortest prefix that fails the same way. */ | |
| 48 | function locate(kind: ConfigKind, text: string, message: string): { line: number; column: number } | null { | |
| 49 | if (text.length > LOCATE_MAX_CHARS) return null; | |
| 50 | const starts = [0]; | |
| 51 | for (let i = 0; i < text.length; i++) if (text[i] === "\n") starts.push(i + 1); | |
| 52 | const lineEnd = (k: number) => (k < starts.length ? starts[k]! - 1 : text.length); | |
| 53 | const line = firstFailing(1, starts.length, (k) => failsWith(kind, text.slice(0, lineEnd(k)), message)); | |
| 54 | const start = starts[line - 1]!; | |
| 55 | const end = lineEnd(line); | |
| 56 | const n = firstFailing(start + 1, Math.max(start + 1, end), (m) => failsWith(kind, text.slice(0, m), message)); | |
| 57 | return { line, column: n - start }; | |
| 58 | } | |
| 59 | ||
| 60 | /** Whether `text` parses as `kind`; when not, the 1-based line and column where it breaks. */ | |
| 61 | export function checkConfig(kind: ConfigKind, text: string): ConfigCheck { | |
| 62 | try { | |
| 63 | parsers[kind](text); | |
| 64 | return { ok: true }; | |
| 65 | } catch (err) { | |
| 66 | const message = messageOf(err); | |
| 67 | if (kind === "toml") { | |
| 68 | const pos = (err as { position?: { line?: unknown; column?: unknown } }).position; | |
| 69 | if (typeof pos?.line === "number" && typeof pos.column === "number") | |
| 70 | return { ok: false, line: pos.line, column: pos.column, message }; | |
| 71 | } | |
| 72 | return { ok: false, ...(locate(kind, text, message) ?? {}), message }; | |
| 73 | } | |
| 74 | } |
test/config-check.test.ts added +30
| @@ -0,0 +1,30 @@ | ||
| 1 | import { expect, test } from "bun:test"; | |
| 2 | import { checkConfig, LOCATE_MAX_CHARS } from "../src/text/config_check.ts"; | |
| 3 | ||
| 4 | test("a small broken JSON file is located by line and column", () => { | |
| 5 | const r = checkConfig("json", '{\n "a": 1,\n "b": ]\n}'); | |
| 6 | expect(r.ok).toBe(false); | |
| 7 | if (!r.ok) expect(r.line).toBe(3); | |
| 8 | }); | |
| 9 | ||
| 10 | test("a large broken file answers quickly", () => { | |
| 11 | const lines = Array.from({ length: 5000 }, (_, i) => ` "k${i}": ${i},`).join("\n"); | |
| 12 | const multi = `{\n${lines}\n "end": ]\n}`; | |
| 13 | const t = performance.now(); | |
| 14 | const r = checkConfig("json", multi); | |
| 15 | expect(r.ok).toBe(false); | |
| 16 | const oneLine = `[${"1,".repeat(50_000)}]]`; | |
| 17 | expect(oneLine.length).toBeGreaterThan(LOCATE_MAX_CHARS); | |
| 18 | const r2 = checkConfig("json", oneLine); | |
| 19 | expect(r2.ok).toBe(false); | |
| 20 | if (!r2.ok) expect(r2.line).toBeUndefined(); | |
| 21 | expect(performance.now() - t).toBeLessThan(1000); | |
| 22 | }); | |
| 23 | ||
| 24 | test("alias-heavy YAML does not stall", () => { | |
| 25 | let s = "a0: &a0 [x, x]\n"; | |
| 26 | for (let i = 1; i < 20000; i++) s += `a${i}: &a${i} [*a${i - 1}, *a${i - 1}]\n`; | |
| 27 | const t = performance.now(); | |
| 28 | checkConfig("yaml", s); | |
| 29 | expect(performance.now() - t).toBeLessThan(1000); | |
| 30 | }); |
test/mcp-read-file.test.ts +13 −1
| @@ -60,7 +60,11 @@ beforeAll(async () => { | ||
| 60 | 60 | const pub = ok(await createRepo(app.db, app.reposRoot, { ownerId: owner.id, name: "pub", visibility: "public" })); |
| 61 | 61 | const priv = ok(await createRepo(app.db, app.reposRoot, { ownerId: owner.id, name: "priv", visibility: "private" })); |
| 62 | 62 | for (const r of [pub, priv]) |
| 63 | ok(await commitToBranch(app, owner, r, { message: "seed", files: [{ path: "src/a.txt", content: "hello\n" }] })); | |
| 63 | ok(await commitToBranch(app, owner, r, { message: "seed", files: [ | |
| 64 | { path: "src/a.txt", content: "hello\n" }, | |
| 65 | { path: "bad.json", content: '{\n "a": }' }, | |
| 66 | { path: "good.json", content: "{}" }, | |
| 67 | ] })); | |
| 64 | 68 | tok["owner"] = createToken(app.db, owner.id, "o", "ro").token; |
| 65 | 69 | tok["stranger"] = createToken(app.db, stranger.id, "s", "ro").token; |
| 66 | 70 | }, 30_000); |
| @@ -92,3 +96,11 @@ test("bad arguments are refused", async () => { | ||
| 92 | 96 | expect((await read(t, { repo: "ada/pub", path: 7 })).error).toContain("invalid"); |
| 93 | 97 | expect((await read(t, { repo: "ada/pub", path: "nope.txt" })).error).toContain("not_found"); |
| 94 | 98 | }); |
| 99 | ||
| 100 | test("a config file says whether it parses; other files carry no about", async () => { | |
| 101 | const t = tok["owner"]!; | |
| 102 | const bad = await read(t, { repo: "ada/pub", path: "bad.json" }); | |
| 103 | expect(bad.value.about).toMatchObject({ kind: "json", parses: false, line: 2 }); | |
| 104 | expect((await read(t, { repo: "ada/pub", path: "good.json" })).value.about).toEqual({ kind: "json", parses: true }); | |
| 105 | expect((await read(t, { repo: "ada/pub", path: "src/a.txt" })).value.about).toBeUndefined(); | |
| 106 | }); |
test/text-config-check.test.ts added +22
| @@ -0,0 +1,22 @@ | ||
| 1 | import { expect, test } from "bun:test"; | |
| 2 | import { checkConfig, configKind } from "../src/text/config_check.ts"; | |
| 3 | ||
| 4 | test("the kind follows the extension", () => { | |
| 5 | expect(configKind("a/b.JSON")).toBe("json"); | |
| 6 | expect(configKind("x.yml")).toBe("yaml"); | |
| 7 | expect(configKind("x.yaml")).toBe("yaml"); | |
| 8 | expect(configKind("x.toml")).toBe("toml"); | |
| 9 | expect(configKind("x.ts")).toBeNull(); | |
| 10 | }); | |
| 11 | ||
| 12 | test("valid files parse", () => { | |
| 13 | expect(checkConfig("json", '{"a":1}')).toEqual({ ok: true }); | |
| 14 | expect(checkConfig("yaml", "a: 1\nb: [1, 2]\n")).toEqual({ ok: true }); | |
| 15 | expect(checkConfig("toml", "a = 1\n")).toEqual({ ok: true }); | |
| 16 | }); | |
| 17 | ||
| 18 | test("a broken file names its line and column", () => { | |
| 19 | expect(checkConfig("json", '{\n "a": 1,\n "b": }')).toMatchObject({ ok: false, line: 3, column: 7 }); | |
| 20 | expect(checkConfig("yaml", 'a: 1\nb: "x')).toMatchObject({ ok: false, line: 2 }); | |
| 21 | expect(checkConfig("toml", "a = 1\nb = \nc = 3")).toMatchObject({ ok: false, line: 3 }); | |
| 22 | }); |
new above
offline
This runs on every read of a
.json/.yaml/.tomlblob up to 5 MB, for any caller who can read the repo, and it runs synchronously in the request. OncecheckConfigis bounded, also skipabout(or its position) above a size cap, so that one read cannot stall the process.