From ebee066e6cad7ada7da786818c8ec4f50a0bd57c Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 13 Aug 2026 06:09:17 +0530 Subject: [PATCH 01/11] fix(core): stop one bad ripgrep record from failing the whole search A ripgrep `--json` match record embeds the entire matched line, so a single minified bundle, source map, or one-line JSON/CSV fixture anywhere in the tree produced a record past the 64 KiB ceiling in `parse`. Because `parse` runs inside `Stream.mapEffect`, that failed the whole stream and discarded every match already collected from unrelated files. Telemetry showed 74 machines / 83 sessions over 7 days on 0.9.3 and 0.9.4. `parse` had three ways to destroy a search, all of them record-level: oversized, unparseable JSON, and schema rejection. The last one also fired on valid ripgrep output: every `path`/`lines`/`match` field is a union of `{text}` and `{bytes}`, and only the `text` arm was modelled, so one stray non-UTF-8 byte in any searched file was equally fatal. Records are independent of their neighbours, so none of those justify aborting the rest of the search. Each is now logged and skipped. - `parse` skips an unusable record instead of failing the stream. Only record-level errors are caught; interruption, defects, `InvalidPatternError` and process-exit failures still propagate. - Normalise ripgrep's `{bytes}` arm to `{text}` before decoding, so matches in non-UTF-8 content are returned with U+FFFD substituted rather than fataling. `path` is deliberately excluded: it is an identifier the caller reopens, and a lossily decoded path names a file that does not exist, so such a record is skipped instead. - Validate base64 spelling first. `Buffer.from` maps unconvertible input to an empty buffer rather than throwing, which would turn a corrupt record into a schema-valid empty match. - `MAX_RECORD_BYTES` 64 KiB -> 16 MiB, and documented for what it actually is: a parse-cost bound, not a memory bound. `Stream.splitLines` has already materialized the line before the check runs. - Same treatment for the legacy parser behind the mounted `/find` route, which had the identical `JSON.parse` + strict-schema abort, plus a warning so a ripgrep protocol change cannot read as an honest "no matches". Verified end-to-end through the CLI: `debug rg search` over a repo with a minified bundle and a non-UTF-8 file previously failed with `Ripgrep JSON record exceeded 65536 bytes` and returned nothing; it now returns all three files. Every new test was confirmed to fail without the fix. Known follow-ups, deliberately not in scope here: `Match.text` is still truncated to the first 2000 chars with submatch offsets into the full line, so a match far along a minified line returns a preview that excludes it; skipped records are logged but not surfaced to the caller as partial results; and neither path is OOM-safe, which needs byte-level framing ahead of `splitLines`. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/ripgrep.ts | 125 +++++++++--- packages/core/test/ripgrep.test.ts | 181 +++++++++++++++++- packages/opencode/src/file/ripgrep.ts | 76 +++++++- .../opencode/test/file/ripgrep-search.test.ts | 42 ++++ 4 files changed, 395 insertions(+), 29 deletions(-) create mode 100644 packages/opencode/test/file/ripgrep-search.test.ts diff --git a/packages/core/src/ripgrep.ts b/packages/core/src/ripgrep.ts index 99c851ed1b..a2b91cec2e 100644 --- a/packages/core/src/ripgrep.ts +++ b/packages/core/src/ripgrep.ts @@ -18,8 +18,21 @@ import { RipgrepBinary } from "./ripgrep/binary" */ const ERROR_BYTES = 8 * 1024 -const MAX_RECORD_BYTES = 64 * 1024 +// altimate_change start — upstream_fix: survive oversized ripgrep JSON records. +// A single `--json` match record carries the entire matched line, so one minified bundle, source +// map, or single-line JSON/CSV fixture anywhere in the tree produces a record far past the old +// 64 KiB ceiling. That ceiling aborted the whole stream, so every other match in the search — in +// unrelated files — was lost with it. Telemetry showed 74 machines hitting this in 7 days. +// +// The ceiling never bounded memory either: `Stream.splitLines` has already materialized the full +// line by the time `parse` sees it, so the allocation is paid before the check runs. All it can +// still bound is JSON.parse cost, which is why it survives as a much higher sanity limit — 16 MiB +// clears real-world long lines by a wide margin. Bounding memory needs byte-level framing ahead of +// `splitLines`, which this does not attempt. Records past it are dropped with a warning; the search +// continues either way. +const MAX_RECORD_BYTES = 16 * 1024 * 1024 const MAX_SUBMATCHES = 100 +// altimate_change end const RawMatch = Schema.Struct({ type: Schema.Literal("match"), @@ -40,6 +53,60 @@ const RawMatch = Schema.Struct({ type RawMatchData = (typeof RawMatch.Type)["data"] +// altimate_change start — upstream_fix: accept ripgrep's `{bytes}` form of an arbitrary-data field. +// Every `path`/`lines`/`match` field in ripgrep's JSON is a union: `{"text": "..."}` when the value +// is valid UTF-8, `{"bytes": ""}` when it is not. `RawMatch` only models the `text` arm, so +// a single stray non-UTF-8 byte anywhere in the tree failed schema decoding and — inside +// `Stream.mapEffect` — took the whole search down with it, exactly like the oversized record did. +// Normalising to the `text` arm up front keeps the schema single-shape and keeps the match usable; +// `toString("utf8")` substitutes U+FFFD for the undecodable bytes rather than dropping the match. +/** Canonical base64, so a corrupt field is left to fail decoding rather than silently becoming "". */ +const BASE64 = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/ + +const readProp = (value: unknown, key: string): unknown => + value !== null && typeof value === "object" && key in value ? Reflect.get(value, key) : undefined + +const normalizeData = (value: unknown): unknown => { + if (!value || typeof value !== "object" || "text" in value) return value + const bytes = readProp(value, "bytes") + // `Buffer.from` is permissive: it turns "!!!" into an empty buffer rather than throwing, which + // would quietly manufacture a schema-valid empty match out of a corrupt record. Spelling is + // checked first so anything unconvertible stays in the `{bytes}` arm and gets skipped instead. + if (typeof bytes !== "string" || !BASE64.test(bytes)) return value + return { text: Buffer.from(bytes, "base64").toString("utf8") } +} + +/** + * Rewrite the `{bytes}` arm of a raw ripgrep match record into its `{text}` equivalent. + * + * `path` is deliberately NOT rewritten. Decoding it is lossy — `toString("utf8")` maps undecodable + * bytes to U+FFFD — and a path is an identifier, not display text: the caller resolves it, stats it + * and reopens it, so a lossy path is a path to a file that does not exist, and two distinct + * filenames can collapse onto the same string. Leaving it in the `{bytes}` arm fails the schema, so + * a match in a file whose NAME is not valid UTF-8 is skipped and logged. Match content is display + * text, so lossy decoding there is the right trade: the match stays useful. + */ +const normalizeMatch = (json: object): unknown => { + const data = readProp(json, "data") + if (!data || typeof data !== "object") return json + const submatches = readProp(data, "submatches") + return { + ...json, + data: { + ...data, + lines: normalizeData(readProp(data, "lines")), + submatches: Array.isArray(submatches) + ? submatches.map((submatch) => + submatch && typeof submatch === "object" + ? { ...submatch, match: normalizeData(readProp(submatch, "match")) } + : submatch, + ) + : submatches, + }, + } +} +// altimate_change end + export class Error extends Schema.TaggedErrorClass()("Ripgrep.Error", { message: Schema.String, cause: Schema.optional(Schema.Defect), @@ -244,27 +311,41 @@ export const layer = Layer.effect( input.pattern, input.file ?? ".", ], - parse: (line) => - (Buffer.byteLength(line, "utf8") > MAX_RECORD_BYTES - ? Effect.fail(failure(`Ripgrep JSON record exceeded ${MAX_RECORD_BYTES} bytes`)) - : Effect.try({ - try: () => JSON.parse(line) as unknown, - catch: (cause) => failure("Invalid ripgrep JSON output", cause), - }) - ).pipe( - Effect.flatMap((json) => { - if (!json || typeof json !== "object" || !("type" in json) || json.type !== "match") - return Effect.succeed(undefined) - return Schema.decodeUnknownEffect(RawMatch)(json).pipe( - Effect.map((match) => ({ - ...match.data, - path: { text: match.data.path.text.replace(/^\.[\\/]/, "") }, - submatches: match.data.submatches.slice(0, MAX_SUBMATCHES), - })), - Effect.mapError((cause) => failure("Invalid ripgrep match output", cause)), - ) - }), - ), + // altimate_change start — upstream_fix: a bad record skips itself, never the search. + // `parse` runs inside `Stream.mapEffect`, so ANY failure here aborts the whole stream and + // discards every match already collected from unrelated files. A record is independent of + // its neighbours, so none of the three ways one can be unusable — oversized, unparseable + // JSON, or schema-rejected — justifies destroying the rest of the search. + parse: (line) => { + const bytes = Buffer.byteLength(line, "utf8") + return Effect.gen(function* () { + // Checked before JSON.parse purely to bound parse cost; `Stream.splitLines` has + // already materialized the line, so this cannot bound memory. See MAX_RECORD_BYTES. + if (bytes > MAX_RECORD_BYTES) + return yield* Effect.fail(failure(`record exceeded ${MAX_RECORD_BYTES} bytes`)) + const json = yield* Effect.try({ + try: () => JSON.parse(line) as unknown, + catch: (cause) => failure("unparseable JSON", cause), + }) + // Non-match records (begin/end/summary) are expected and simply carry no match. + if (!json || typeof json !== "object" || !("type" in json) || json.type !== "match") return undefined + const match = yield* Schema.decodeUnknownEffect(RawMatch)(normalizeMatch(json)).pipe( + Effect.mapError((cause) => failure("unexpected match shape", cause)), + ) + return { + ...match.data, + path: { text: match.data.path.text.replace(/^\.[\\/]/, "") }, + submatches: match.data.submatches.slice(0, MAX_SUBMATCHES), + } + }).pipe( + Effect.catch((cause) => + Effect.logWarning("skipping unusable ripgrep record", { bytes, reason: cause.message }).pipe( + Effect.as(undefined), + ), + ), + ) + }, + // altimate_change end }).pipe( Effect.map((result) => result.items.map((match) => { diff --git a/packages/core/test/ripgrep.test.ts b/packages/core/test/ripgrep.test.ts index da8e7519ce..72089186f9 100644 --- a/packages/core/test/ripgrep.test.ts +++ b/packages/core/test/ripgrep.test.ts @@ -1,8 +1,10 @@ -import { describe, expect } from "bun:test" +import { describe, expect, test as bunTest } from "bun:test" import fs from "fs/promises" import path from "path" -import { Effect } from "effect" +import { Effect, Layer } from "effect" import { Ripgrep } from "@opencode-ai/core/ripgrep" +import { RipgrepBinary } from "@opencode-ai/core/ripgrep/binary" +import { AppProcess } from "@opencode-ai/core/process" import { RelativePath } from "@opencode-ai/core/schema" import { tmpdir } from "./fixture/tmpdir" import { testEffect } from "./lib/effect" @@ -87,5 +89,180 @@ describe("Ripgrep", () => { (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), ), ) + + // upstream_fix: a ripgrep `--json` match record embeds the whole matched line, so a minified + // bundle or single-line JSON fixture emits a record far past any per-record ceiling. That used to + // fail the stream, taking every unrelated match in the search down with it. + it.live("keeps matching unrelated files when one file has an oversized line", () => + Effect.acquireUseRelease( + Effect.promise(() => tmpdir()), + (tmp) => + Effect.gen(function* () { + yield* Effect.promise(() => fs.writeFile(path.join(tmp.path, "a-small.txt"), "needle here\n")) + // Well past the old 64 KiB ceiling, well under the current sanity limit. + yield* Effect.promise(() => + fs.writeFile(path.join(tmp.path, "b-minified.js"), "x".repeat(100_000) + "needle" + "y".repeat(100_000)), + ) + + const matches = yield* (yield* Ripgrep.Service).grep({ cwd: tmp.path, pattern: "needle", limit: 10 }) + + // Both the bystander and the oversized file are reported; neither is lost to a failure. + expect(matches.map((item) => item.entry.path).sort()).toEqual([ + RelativePath.make("a-small.txt"), + RelativePath.make("b-minified.js"), + ]) + }), + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), + ), + ) + + // upstream_fix: ripgrep emits `{"bytes": ""}` instead of `{"text": ...}` for a line that + // is not valid UTF-8. The schema modelled only the `text` arm, so one stray byte failed the whole + // search — the same abort-everything shape as the oversized record. This drives real ripgrep; + // the exact decoding is pinned by the stubbed case below. + it.live("returns matches from files containing non-UTF8 lines", () => + Effect.acquireUseRelease( + Effect.promise(() => tmpdir()), + (tmp) => + Effect.gen(function* () { + yield* Effect.promise(() => fs.writeFile(path.join(tmp.path, "a-plain.txt"), "needle here\n")) + yield* Effect.promise(() => + fs.writeFile(path.join(tmp.path, "b-binary.txt"), Buffer.from("needle \xff\xfe tail\n", "binary")), + ) + yield* Effect.promise(() => fs.writeFile(path.join(tmp.path, "c-plain.txt"), "needle here\n")) + + const matches = yield* (yield* Ripgrep.Service).grep({ cwd: tmp.path, pattern: "needle", limit: 10 }) + + // The non-UTF8 file is reported like any other rather than dropped or fatal. + expect(matches.map((item) => item.entry.path).sort()).toEqual([ + RelativePath.make("a-plain.txt"), + RelativePath.make("b-binary.txt"), + RelativePath.make("c-plain.txt"), + ]) + // Undecodable bytes become U+FFFD, so the surrounding text stays readable. + const binary = matches.find((item) => item.entry.path === RelativePath.make("b-binary.txt")) + expect(binary?.text).toContain("needle") + expect(binary?.text).toContain("tail") + }), + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), + ), + ) + + // Real ripgrep cannot be coerced into emitting a chosen bad record — `--` makes the next arg the + // pattern — so these cases drive the parser through a stub `rg` that prints exactly the NDJSON + // given. Only the executable is stubbed: the real spawn, decode, line splitting, parse, collection + // and output mapping all still run. Plain `bunTest` because these supply their own Ripgrep layer, + // which the ambient `testEffect(Ripgrep.defaultLayer)` would otherwise shadow. + const matchRecord = (file: string, overrides: Record = {}) => + JSON.stringify({ + type: "match", + data: { + path: { text: `./${file}` }, + lines: { text: "needle\n" }, + line_number: 1, + absolute_offset: 0, + submatches: [{ match: { text: "needle" }, start: 0, end: 6 }], + ...overrides, + }, + }) + + const grepWithStubbedRecords = (records: string[]) => + Effect.acquireUseRelease( + Effect.promise(() => tmpdir()), + (tmp) => + Effect.gen(function* () { + const data = path.join(tmp.path, "records.jsonl") + yield* Effect.promise(() => fs.writeFile(data, records.join("\n") + "\n")) + const stub = path.join(tmp.path, "rg") + yield* Effect.promise(() => fs.writeFile(stub, `#!/bin/sh\ncat ${JSON.stringify(data)}\n`)) + yield* Effect.promise(() => fs.chmod(stub, 0o755)) + + return yield* Effect.gen(function* () { + const rg = yield* Ripgrep.Service + return yield* rg.grep({ cwd: tmp.path, pattern: "needle", limit: 100 }) + }).pipe( + Effect.provide( + Ripgrep.layer.pipe( + Layer.provide( + Layer.succeed(RipgrepBinary.Service, RipgrepBinary.Service.of({ filepath: Effect.succeed(stub) })), + ), + Layer.provide(AppProcess.defaultLayer), + ), + ), + ) + }), + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), + ) + + bunTest("skips an unparseable record without failing the search", async () => { + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("a.txt"), + '{"type":"match","data":{"path":{"text":"./b.t', // truncated mid-JSON + matchRecord("c.txt"), + ]), + ) + + // The malformed middle record is dropped; the records on either side survive. + expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt"), RelativePath.make("c.txt")]) + }) + + // The size ceiling is asserted on a record built to exceed it, rather than inferred from a large + // file — that keeps the case independent of whether a given ripgrep build emits the match at all. + bunTest("skips an oversized record and keeps parsing the records after it", async () => { + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("a.txt"), + matchRecord("b-huge.txt", { lines: { text: "needle" + "x".repeat(17 * 1024 * 1024) } }), + matchRecord("c.txt"), + ]), + ) + + expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt"), RelativePath.make("c.txt")]) + }) + + // A path is an identifier the caller reopens, so it must never be lossily decoded. Such a record + // is skipped rather than reported under a U+FFFD-mangled path that names no real file. + bunTest("skips a match whose path is not valid UTF-8, keeping the rest", async () => { + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("a.txt"), + matchRecord("ignored", { path: { bytes: Buffer.from("./b\xff.txt", "binary").toString("base64") } }), + matchRecord("c.txt"), + ]), + ) + + expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt"), RelativePath.make("c.txt")]) + }) + + // `Buffer.from` maps unconvertible base64 to an empty buffer instead of throwing, which would turn + // a corrupt record into a schema-valid EMPTY match. It must be skipped, not silently emptied. + bunTest("skips a record whose bytes field is not valid base64", async () => { + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("a.txt"), + matchRecord("b.txt", { lines: { bytes: "!!!not base64!!!" } }), + matchRecord("c.txt"), + ]), + ) + + expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt"), RelativePath.make("c.txt")]) + }) + + bunTest("decodes a non-UTF8 match line to replacement characters", async () => { + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("a.txt", { + lines: { bytes: Buffer.from("needle \xff\xfe tail\n", "binary").toString("base64") }, + submatches: [{ match: { bytes: Buffer.from("needle", "binary").toString("base64") }, start: 0, end: 6 }], + }), + ]), + ) + + expect(matches).toHaveLength(1) + // Content is display text, so lossy decoding keeps the match usable rather than dropping it. + expect(matches[0].text).toBe("needle �� tail\n") + expect(matches[0].submatches[0].text).toBe("needle") + }) // altimate_change end }) diff --git a/packages/opencode/src/file/ripgrep.ts b/packages/opencode/src/file/ripgrep.ts index 61fd9a9b6f..09a3d34256 100644 --- a/packages/opencode/src/file/ripgrep.ts +++ b/packages/opencode/src/file/ripgrep.ts @@ -94,6 +94,56 @@ export namespace Ripgrep { const Result = z.union([Begin, Match, End, Summary]) + // altimate_change start — upstream_fix: tolerate ripgrep's `{bytes}` arm and malformed lines. + /** Canonical base64, so a corrupt field fails decoding rather than silently becoming "". */ + const BASE64 = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/ + + /** Parse one NDJSON record, rewriting `{bytes: base64}` fields into the `{text}` arm. */ + const normalizeRecord = (line: string): unknown => { + let json: unknown + try { + json = JSON.parse(line) + } catch { + return undefined + } + if (!json || typeof json !== "object") return json + const read = (value: unknown, key: string): unknown => + value !== null && typeof value === "object" && key in value ? Reflect.get(value, key) : undefined + const data = read(json, "data") + if (!data || typeof data !== "object") return json + const asText = (value: unknown): unknown => { + if (!value || typeof value !== "object" || "text" in value) return value + const bytes = read(value, "bytes") + // Spelling is validated first because `Buffer.from` decodes "!!!" to an empty buffer instead + // of throwing, which would turn a corrupt record into a schema-valid empty match. + if (typeof bytes !== "string" || !BASE64.test(bytes)) return value + return { text: Buffer.from(bytes, "base64").toString("utf8") } + } + const submatches = read(data, "submatches") + // Only rewrite keys the record actually carries — `begin`/`end`/`summary` records reach here too + // and must keep their exact shape, or the strict union below would reject them. + // `path` is deliberately left alone: decoding it is lossy, and a path is an identifier the + // caller reopens, so a U+FFFD-mangled path names a file that does not exist. Such a record + // stays in the `{bytes}` arm and is skipped. See packages/core/src/ripgrep.ts. + return { + ...json, + data: { + ...data, + ...("lines" in data ? { lines: asText(read(data, "lines")) } : {}), + ...(Array.isArray(submatches) + ? { + submatches: submatches.map((submatch) => + submatch && typeof submatch === "object" + ? { ...submatch, match: asText(read(submatch, "match")) } + : submatch, + ), + } + : {}), + }, + } + } + // altimate_change end + export type Result = z.infer export type Match = z.infer export type Begin = z.infer @@ -374,11 +424,27 @@ export namespace Ripgrep { const lines = result.text.trim().split(/\r?\n/).filter(Boolean) // Parse JSON lines from ripgrep output - return lines - .map((line) => JSON.parse(line)) - .map((parsed) => Result.parse(parsed)) - .filter((r) => r.type === "match") - .map((r) => r.data) + // altimate_change start — upstream_fix: a bad record skips itself, not the whole search. + // `JSON.parse` + a strict `Result.parse` on every line meant one unusable record threw out of + // `search()` and discarded every match already collected from unrelated files — the same defect + // fixed in packages/core/src/ripgrep.ts. Records are independent, so a bad one is dropped. + // `lines`/`path`/`match` are `{text}` only when the value is valid UTF-8 and `{bytes}` otherwise, + // so the `{bytes}` arm is normalised rather than left to fail the strict schema. + const matches: Match["data"][] = [] + let skipped = 0 + for (const line of lines) { + const parsed = Result.safeParse(normalizeRecord(line)) + if (!parsed.success) { + skipped++ + continue + } + if (parsed.data.type === "match") matches.push(parsed.data.data) + } + // Counted and reported once rather than per record: without this a ripgrep protocol change + // would make `/find` answer `[]`, which is indistinguishable from an honest "no matches". + if (skipped > 0) log.warn("skipped unusable ripgrep records", { skipped, total: lines.length }) + return matches + // altimate_change end } } // altimate_change end diff --git a/packages/opencode/test/file/ripgrep-search.test.ts b/packages/opencode/test/file/ripgrep-search.test.ts new file mode 100644 index 0000000000..28bad3d0f5 --- /dev/null +++ b/packages/opencode/test/file/ripgrep-search.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, test } from "bun:test" +import fs from "fs/promises" +import os from "os" +import path from "path" +import { Ripgrep } from "../../src/file/ripgrep" + +// altimate_change start — upstream_fix: legacy `/find` search must survive unusable records. +// `search()` used to `JSON.parse` + strictly `Result.parse` every line, so a single unusable record +// threw out of the whole call and discarded every match already collected from unrelated files — +// the same defect fixed in packages/core/src/ripgrep.ts. This path is reachable from the mounted +// `/find` route (server/routes/file.ts), so it needs its own coverage. +const withRepo = async (run: (dir: string) => Promise) => { + const dir = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), "legacy-rg-"))) + try { + await run(dir) + } finally { + await fs.rm(dir, { recursive: true, force: true }) + } +} + +describe("legacy Ripgrep.search", () => { + test("returns matches from a file whose matched line is not valid UTF-8", () => + withRepo(async (dir) => { + await fs.writeFile(path.join(dir, "a-plain.txt"), "needle here\n") + // ripgrep emits `{"bytes": ""}` rather than `{"text": ...}` for this line, which the + // strict Zod schema rejected — taking the unrelated matches down with it. + await fs.writeFile(path.join(dir, "b-binary.txt"), Buffer.from("needle \xff\xfe tail\n", "binary")) + await fs.writeFile(path.join(dir, "c-plain.txt"), "needle here\n") + + const matches = await Ripgrep.search({ cwd: dir, pattern: "needle", limit: 10 }) + + expect(matches.map((match) => match.path.text.replace(/^\.\//, "")).sort()).toEqual([ + "a-plain.txt", + "b-binary.txt", + "c-plain.txt", + ]) + const binary = matches.find((match) => match.path.text.includes("b-binary.txt")) + expect(binary?.lines.text).toContain("needle") + expect(binary?.lines.text).toContain("tail") + })) +}) +// altimate_change end From 065cb98d9dba76d8568e9cd380f8c07b0bfc4ea8 Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 13 Aug 2026 17:14:36 +0530 Subject: [PATCH 02/11] =?UTF-8?q?fix(core):=20address=20consensus=20review?= =?UTF-8?q?=20=E2=80=94=20offsets,=20retained=20heap,=20log=20noise?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the ripgrep record-skipping fix, addressing the consensus review. Major: - Rebase submatch offsets after a lossy `{bytes}` decode. `start`/`end` are byte offsets into the RAW line; each undecodable byte widens to a 3-byte U+FFFD, so the raw offsets no longer locate the match. A line starting with one bad byte reported `needle` at [3,9) of a string where [3,9) reads "edle t". Offsets are now rebased onto the decoded text's own UTF-8 encoding, which preserves the established byte-offset contract instead of silently switching these records to a different unit. - Cap the matched line at parse time. The previous comment claimed the ceiling "never bounded memory" — true of the transient per-line allocation, false of what the search RETAINS: `run` collects rows with `Stream.runCollect` and each row carried the full `lines.text` until the final mapping trimmed it, while `tool/grep.ts` passes `Number.MAX_SAFE_INTEGER` as the row cap. Raising the record ceiling to 16 MiB therefore raised the retained bound 256x. Capping in the parser keeps the parse ceiling and makes the retained bound tighter than it was before this branch. - Aggregate the skip warning. One warning per skipped record meant a systematic protocol mismatch logged once per record across the whole tree and still answered with an innocent-looking empty result. Now one warning per search with a count and bounded samples, naming the file where one is recoverable. Minor: - Reject empty and non-canonical base64. The guard's own comment promised a corrupt field would never become a valid-looking empty match, but the regex matched "" — producing exactly that — and accepted non-canonical padding ("Zh==" and "Zg==" both decode to "f"). Now requires a non-empty string that round-trips. - Count records with an unrecognised or missing `type` instead of dropping them silently; only ripgrep's own control records stay silent. - Apply the size ceiling on the legacy `/find` path too. - Slice submatches to MAX_SUBMATCHES before decoding rather than after. - Extract the legacy parse loop as `parseRecords` so its skip branches are testable without a stub binary, and document why the two parsers differ. Tests: 17 core, 7 legacy. The three cases covering the review's correctness findings were confirmed to fail against the previous commit. Two tests are deliberately scoped honestly — the line-cap test pins the output contract but cannot observe the retained-heap improvement, since capping early and capping late produce byte-identical output. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/ripgrep.ts | 139 ++++++++++++++---- packages/core/test/ripgrep.test.ts | 127 +++++++++++++++- packages/opencode/src/file/ripgrep.ts | 70 ++++++--- .../opencode/test/file/ripgrep-search.test.ts | 55 +++++++ 4 files changed, 338 insertions(+), 53 deletions(-) diff --git a/packages/core/src/ripgrep.ts b/packages/core/src/ripgrep.ts index a2b91cec2e..0860263b0c 100644 --- a/packages/core/src/ripgrep.ts +++ b/packages/core/src/ripgrep.ts @@ -32,6 +32,21 @@ const ERROR_BYTES = 8 * 1024 // continues either way. const MAX_RECORD_BYTES = 16 * 1024 * 1024 const MAX_SUBMATCHES = 100 + +// The 16 MiB ceiling bounds the cost of parsing ONE line. It does not bound what the search +// retains: `run` collects rows with `Stream.runCollect` and holds them until the stream ends, and +// each row carried the FULL `lines.text` until the mapping step trimmed it at the very end. Peak +// retained memory is therefore rows x record size — and callers pass no meaningful row cap +// (`tool/grep.ts` passes `Number.MAX_SAFE_INTEGER`), so raising the per-record ceiling raised the +// retained bound with it. Capping the line here instead keeps the parse ceiling while making the +// retained bound tighter than it was before this change. Nothing downstream ever renders more. +const LINE_TEXT_CAP = 2_000 + +/** Trim a matched line to what any consumer actually shows, preserving the elision marker. */ +const capLineText = (text: string) => (text.length > LINE_TEXT_CAP ? text.slice(0, LINE_TEXT_CAP) + "..." : text) + +/** Distinct skip reasons kept for the aggregate warning; enough to diagnose, bounded for logs. */ +const SKIP_SAMPLES = 5 // altimate_change end const RawMatch = Schema.Struct({ @@ -63,17 +78,34 @@ type RawMatchData = (typeof RawMatch.Type)["data"] /** Canonical base64, so a corrupt field is left to fail decoding rather than silently becoming "". */ const BASE64 = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/ +/** ripgrep's control records. Anything else with an unrecognised `type` is a protocol surprise. */ +const CONTROL_TYPES = new Set(["begin", "end", "summary"]) + const readProp = (value: unknown, key: string): unknown => value !== null && typeof value === "object" && key in value ? Reflect.get(value, key) : undefined -const normalizeData = (value: unknown): unknown => { - if (!value || typeof value !== "object" || "text" in value) return value +/** + * Decode one ripgrep arbitrary-data field (`{text}` or `{bytes}`) to a string. + * + * Returns undefined when the field cannot be trusted, which leaves the original shape in place so + * the schema rejects it and the record is skipped — the point being that a corrupt field must never + * be silently converted into a valid-looking empty one. `Buffer.from` makes that easy to get wrong: + * it maps unconvertible input to an EMPTY buffer instead of throwing. Hence three guards — reject + * the empty string (a matched line is never empty, so an empty `bytes` arm is always corrupt), + * check the spelling, then require the decode to round-trip so non-canonical padding bits (`Zh==` + * and `Zg==` both decode to "f") cannot slip through. + * + * `raw` is returned alongside so submatch offsets can be rebased; see `normalizeMatch`. + */ +const decodeField = (value: unknown): { text: string; raw?: Buffer } | undefined => { + if (!value || typeof value !== "object") return undefined + const text = readProp(value, "text") + if (typeof text === "string") return { text } const bytes = readProp(value, "bytes") - // `Buffer.from` is permissive: it turns "!!!" into an empty buffer rather than throwing, which - // would quietly manufacture a schema-valid empty match out of a corrupt record. Spelling is - // checked first so anything unconvertible stays in the `{bytes}` arm and gets skipped instead. - if (typeof bytes !== "string" || !BASE64.test(bytes)) return value - return { text: Buffer.from(bytes, "base64").toString("utf8") } + if (typeof bytes !== "string" || bytes.length === 0 || !BASE64.test(bytes)) return undefined + const raw = Buffer.from(bytes, "base64") + if (raw.toString("base64") !== bytes) return undefined + return { text: raw.toString("utf8"), raw } } /** @@ -85,22 +117,44 @@ const normalizeData = (value: unknown): unknown => { * filenames can collapse onto the same string. Leaving it in the `{bytes}` arm fails the schema, so * a match in a file whose NAME is not valid UTF-8 is skipped and logged. Match content is display * text, so lossy decoding there is the right trade: the match stays useful. + * + * Submatch `start`/`end` are BYTE offsets into the raw line. A lossy decode destroys that frame of + * reference — each undecodable sequence becomes U+FFFD, three bytes wide — so they are rebased onto + * the decoded text's own UTF-8 encoding. That preserves the established byte-offset contract rather + * than silently switching these records to a different unit: without it, a line beginning with one + * bad byte reports `needle` at [3,9) of a string where [3,9) reads "edle t". */ const normalizeMatch = (json: object): unknown => { const data = readProp(json, "data") if (!data || typeof data !== "object") return json + const lines = decodeField(readProp(data, "lines")) + if (!lines) return json + const raw = lines.raw + const rebase = (offset: unknown): unknown => + raw && typeof offset === "number" && offset >= 0 + ? Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") + : offset const submatches = readProp(data, "submatches") return { ...json, data: { ...data, - lines: normalizeData(readProp(data, "lines")), + // Capped here rather than after decoding: the full line is retained by `Stream.runCollect` + // until the search ends, and nothing downstream ever shows more than this. See LINE_TEXT_CAP. + lines: { text: capLineText(lines.text) }, + // Sliced BEFORE decoding so a pathological submatch count is not decoded only to be dropped. submatches: Array.isArray(submatches) - ? submatches.map((submatch) => - submatch && typeof submatch === "object" - ? { ...submatch, match: normalizeData(readProp(submatch, "match")) } - : submatch, - ) + ? submatches.slice(0, MAX_SUBMATCHES).map((submatch) => { + if (!submatch || typeof submatch !== "object") return submatch + const match = decodeField(readProp(submatch, "match")) + if (!match) return submatch + return { + ...submatch, + match: { text: match.text }, + start: rebase(readProp(submatch, "start")), + end: rebase(readProp(submatch, "end")), + } + }) : submatches, }, } @@ -293,8 +347,10 @@ export const layer = Layer.effect( Effect.map((result) => result.items), Effect.catchTag("Ripgrep.InvalidPatternError", (cause) => Effect.fail(failure(cause.message, cause))), ), - grep: (input) => - run({ + grep: (input) => { + // Per invocation, never per layer: two concurrent searches must not share a tally. + const skipped: { count: number; samples: string[] } = { count: 0, samples: [] } + return run({ ...input, args: [ "--no-config", @@ -318,6 +374,9 @@ export const layer = Layer.effect( // JSON, or schema-rejected — justifies destroying the rest of the search. parse: (line) => { const bytes = Buffer.byteLength(line, "utf8") + // Captured during the walk so the aggregate warning can name a file when one is + // recoverable. Malformed JSON has no path by definition, hence "when present". + let where: string | undefined return Effect.gen(function* () { // Checked before JSON.parse purely to bound parse cost; `Stream.splitLines` has // already materialized the line, so this cannot bound memory. See MAX_RECORD_BYTES. @@ -327,26 +386,48 @@ export const layer = Layer.effect( try: () => JSON.parse(line) as unknown, catch: (cause) => failure("unparseable JSON", cause), }) - // Non-match records (begin/end/summary) are expected and simply carry no match. - if (!json || typeof json !== "object" || !("type" in json) || json.type !== "match") return undefined + if (!json || typeof json !== "object" || !("type" in json)) + return yield* Effect.fail(failure("record has no type")) + // Captured before the type check so an unrecognised record can still name its file. + const pathField = readProp(readProp(json, "data"), "path") + const pathText = readProp(pathField, "text") + if (typeof pathText === "string") where = pathText + // Control records are expected and simply carry no match. An unrecognised type is a + // protocol surprise and is counted rather than dropped on the floor, so a ripgrep + // change cannot quietly turn every match into "no matches". + if (json.type !== "match") + return typeof json.type === "string" && CONTROL_TYPES.has(json.type) + ? undefined + : yield* Effect.fail(failure(`unrecognised record type ${JSON.stringify(json.type)}`)) const match = yield* Schema.decodeUnknownEffect(RawMatch)(normalizeMatch(json)).pipe( Effect.mapError((cause) => failure("unexpected match shape", cause)), ) - return { - ...match.data, - path: { text: match.data.path.text.replace(/^\.[\\/]/, "") }, - submatches: match.data.submatches.slice(0, MAX_SUBMATCHES), - } + // `normalizeMatch` already caps submatches and line text, so nothing is re-trimmed. + return { ...match.data, path: { text: match.data.path.text.replace(/^\.[\\/]/, "") } } }).pipe( Effect.catch((cause) => - Effect.logWarning("skipping unusable ripgrep record", { bytes, reason: cause.message }).pipe( - Effect.as(undefined), - ), + Effect.sync(() => { + skipped.count++ + if (skipped.samples.length < SKIP_SAMPLES) + skipped.samples.push(where ? `${cause.message} (${where})` : cause.message) + return undefined + }), ), ) }, // altimate_change end }).pipe( + // One aggregate warning per search, not one per record: a systematic protocol mismatch + // rejects every record in the tree, and a per-record log would bury the machine in noise + // while still answering with an innocent-looking empty result. + Effect.tap(() => + skipped.count > 0 + ? Effect.logWarning("skipped unusable ripgrep records", { + skipped: skipped.count, + reasons: skipped.samples, + }) + : Effect.void, + ), Effect.map((result) => result.items.map((match) => { const relative = match.path.text @@ -362,7 +443,10 @@ export const layer = Layer.effect( }), line: match.line_number, offset: match.absolute_offset, - text: match.lines.text.length > 2_000 ? match.lines.text.slice(0, 2_000) + "..." : match.lines.text, + // altimate_change start — upstream_fix: capped at parse time, see LINE_TEXT_CAP. + // Re-applied here so the cap still holds if the parser ever stops trimming. + text: capLineText(match.lines.text), + // altimate_change end submatches: match.submatches.map((submatch) => ({ text: submatch.match.text, start: submatch.start, @@ -371,7 +455,8 @@ export const layer = Layer.effect( }) }), ), - ), + ) + }, }) }), ) diff --git a/packages/core/test/ripgrep.test.ts b/packages/core/test/ripgrep.test.ts index 72089186f9..c44a416843 100644 --- a/packages/core/test/ripgrep.test.ts +++ b/packages/core/test/ripgrep.test.ts @@ -1,7 +1,7 @@ -import { describe, expect, test as bunTest } from "bun:test" +import { beforeEach, describe, expect, test as bunTest } from "bun:test" import fs from "fs/promises" import path from "path" -import { Effect, Layer } from "effect" +import { Effect, Layer, Logger } from "effect" import { Ripgrep } from "@opencode-ai/core/ripgrep" import { RipgrepBinary } from "@opencode-ai/core/ripgrep/binary" import { AppProcess } from "@opencode-ai/core/process" @@ -166,6 +166,28 @@ describe("Ripgrep", () => { }, }) + /** + * Collected so the skip *count* can be asserted — it is invisible in the returned matches. + * `Effect.logWarning(message, data)` puts both into `entry.message` as a tuple, not annotations. + */ + const skipWarnings: Array<{ skipped?: number; reasons?: string[] }> = [] + const captureWarnings = Logger.layer([ + Logger.formatStructured.pipe( + Logger.map((entry): void => { + const parts: unknown[] = Array.isArray(entry.message) ? entry.message : [entry.message] + if (parts[0] !== "skipped unusable ripgrep records") return + const data = parts[1] + if (!data || typeof data !== "object") return + const skipped = Reflect.get(data, "skipped") + const reasons = Reflect.get(data, "reasons") + skipWarnings.push({ + skipped: typeof skipped === "number" ? skipped : undefined, + reasons: Array.isArray(reasons) ? reasons.map(String) : undefined, + }) + }), + ), + ]) + const grepWithStubbedRecords = (records: string[]) => Effect.acquireUseRelease( Effect.promise(() => tmpdir()), @@ -189,11 +211,19 @@ describe("Ripgrep", () => { Layer.provide(AppProcess.defaultLayer), ), ), + Effect.provide(captureWarnings), ) }), (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), ) + beforeEach(() => { + skipWarnings.length = 0 + }) + + /** The single aggregate warning for the last search, or undefined when nothing was skipped. */ + const lastSkip = () => skipWarnings.at(-1) + bunTest("skips an unparseable record without failing the search", async () => { const matches = await Effect.runPromise( grepWithStubbedRecords([ @@ -205,6 +235,7 @@ describe("Ripgrep", () => { // The malformed middle record is dropped; the records on either side survive. expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt"), RelativePath.make("c.txt")]) + expect(lastSkip()).toEqual({ skipped: 1, reasons: ["unparseable JSON"] }) }) // The size ceiling is asserted on a record built to exceed it, rather than inferred from a large @@ -249,6 +280,98 @@ describe("Ripgrep", () => { expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt"), RelativePath.make("c.txt")]) }) + // Submatch offsets are BYTE offsets into the raw line. A lossy decode widens every undecodable + // byte to a 3-byte U+FFFD, so the raw offsets no longer locate the match and must be rebased onto + // the decoded text's own UTF-8 encoding. Without this the match reads "��need". + bunTest("rebases submatch offsets onto the decoded line after a lossy decode", async () => { + const raw = Buffer.concat([Buffer.from([0xff]), Buffer.from("needle tail\n")]) + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("a.txt", { + lines: { bytes: raw.toString("base64") }, + // "needle" sits at raw bytes [1, 7). + submatches: [{ match: { bytes: Buffer.from("needle").toString("base64") }, start: 1, end: 7 }], + }), + ]), + ) + + expect(matches).toHaveLength(1) + const [{ text, submatches }] = matches + expect(submatches[0]).toEqual({ text: "needle", start: 3, end: 9 }) + // The contract is byte offsets into the returned text, so slice its UTF-8 encoding. + expect(Buffer.from(text, "utf8").subarray(submatches[0].start, submatches[0].end).toString("utf8")).toBe("needle") + }) + + bunTest("skips a record whose bytes field is empty rather than emitting an empty match", async () => { + const matches = await Effect.runPromise( + grepWithStubbedRecords([matchRecord("a.txt"), matchRecord("b.txt", { lines: { bytes: "" } })]), + ) + + // "" is spelled like valid base64 but a matched line is never empty, so the record is corrupt. + expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt")]) + }) + + bunTest("skips a record whose bytes field uses non-canonical padding", async () => { + // "Zh==" and "Zg==" both decode to "f"; only the canonical spelling round-trips. + const matches = await Effect.runPromise( + grepWithStubbedRecords([matchRecord("a.txt"), matchRecord("b.txt", { lines: { bytes: "Zh==" } })]), + ) + + expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt")]) + }) + + // Control records carry no match and are ignored silently; an unrecognised type is a protocol + // surprise and must be counted, or a ripgrep change turns every match into an innocent "no match". + bunTest("ignores control records but counts records with an unknown type", async () => { + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + JSON.stringify({ type: "begin", data: { path: { text: "./a.txt" } } }), + matchRecord("a.txt"), + JSON.stringify({ type: "match-v2", data: { path: { text: "./b.txt" } } }), + JSON.stringify({}), + JSON.stringify({ type: "end", data: { path: { text: "./a.txt" } } }), + matchRecord("c.txt"), + ]), + ) + + expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt"), RelativePath.make("c.txt")]) + // begin/end are silent; the unknown type and the typeless record are counted, not dropped + // silently — the whole point being that a protocol change cannot masquerade as "no matches". + expect(lastSkip()?.skipped).toBe(2) + expect(lastSkip()?.reasons).toEqual([`unrecognised record type "match-v2" (./b.txt)`, "record has no type"]) + }) + + bunTest("returns an empty result when every record is unusable, rather than failing", async () => { + const matches = await Effect.runPromise(grepWithStubbedRecords(["{oops", "{also oops", "{still oops"])) + + expect(matches).toEqual([]) + }) + + // Pins the ceiling itself: at the limit the record is kept, one byte over it is skipped. + bunTest("accepts a record at exactly the size ceiling and skips one byte over", async () => { + const sizeOf = (file: string, padding: number) => matchRecord(file, { lines: { text: "n".repeat(padding) } }) + const overhead = Buffer.byteLength(sizeOf("a.txt", 0), "utf8") + const limit = 16 * 1024 * 1024 + + const matches = await Effect.runPromise( + grepWithStubbedRecords([sizeOf("a.txt", limit - overhead), sizeOf("b.txt", limit - overhead + 1)]), + ) + + expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt")]) + }) + + // Pins the OUTPUT contract of the cap. Note it cannot prove the retained-memory improvement that + // motivated moving the cap into the parser: capping at parse time and capping at the end produce + // byte-identical output, and only the peak heap during collection differs. + bunTest("caps the returned line text and keeps the elision marker", async () => { + const matches = await Effect.runPromise( + grepWithStubbedRecords([matchRecord("a.txt", { lines: { text: "needle" + "x".repeat(50_000) } })]), + ) + + expect(matches[0].text).toHaveLength(2_003) + expect(matches[0].text.endsWith("...")).toBe(true) + }) + bunTest("decodes a non-UTF8 match line to replacement characters", async () => { const matches = await Effect.runPromise( grepWithStubbedRecords([ diff --git a/packages/opencode/src/file/ripgrep.ts b/packages/opencode/src/file/ripgrep.ts index 09a3d34256..321520f59e 100644 --- a/packages/opencode/src/file/ripgrep.ts +++ b/packages/opencode/src/file/ripgrep.ts @@ -95,9 +95,17 @@ export namespace Ripgrep { const Result = z.union([Begin, Match, End, Summary]) // altimate_change start — upstream_fix: tolerate ripgrep's `{bytes}` arm and malformed lines. - /** Canonical base64, so a corrupt field fails decoding rather than silently becoming "". */ + // + // This mirrors packages/core/src/ripgrep.ts, but deliberately not in every respect. That parser + // streams, so it caps the retained line text and rebases submatch offsets; this one buffers all of + // stdout up front and hands its records straight to the `/find` response, where the raw ripgrep + // shape is the published contract — so it normalises and skips, and leaves the shape alone. Both + // report skipped records once per search rather than once per record. const BASE64 = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/ + /** Mirrors packages/core/src/ripgrep.ts. Bounds parse cost per record on this path too. */ + const MAX_RECORD_BYTES = 16 * 1024 * 1024 + /** Parse one NDJSON record, rewriting `{bytes: base64}` fields into the `{text}` arm. */ const normalizeRecord = (line: string): unknown => { let json: unknown @@ -114,10 +122,14 @@ export namespace Ripgrep { const asText = (value: unknown): unknown => { if (!value || typeof value !== "object" || "text" in value) return value const bytes = read(value, "bytes") - // Spelling is validated first because `Buffer.from` decodes "!!!" to an empty buffer instead - // of throwing, which would turn a corrupt record into a schema-valid empty match. - if (typeof bytes !== "string" || !BASE64.test(bytes)) return value - return { text: Buffer.from(bytes, "base64").toString("utf8") } + // Guarded three ways because `Buffer.from` decodes unconvertible input to an EMPTY buffer + // instead of throwing, which would turn a corrupt record into a schema-valid empty match: + // reject the empty string (a matched line is never empty), check the spelling, then require + // a round-trip so non-canonical padding ("Zh==" and "Zg==" both decode to "f") is rejected. + if (typeof bytes !== "string" || bytes.length === 0 || !BASE64.test(bytes)) return value + const decoded = Buffer.from(bytes, "base64") + if (decoded.toString("base64") !== bytes) return value + return { text: decoded.toString("utf8") } } const submatches = read(data, "submatches") // Only rewrite keys the record actually carries — `begin`/`end`/`summary` records reach here too @@ -142,6 +154,34 @@ export namespace Ripgrep { }, } } + + /** + * Turn ripgrep NDJSON lines into match data, skipping records that cannot be used. + * + * `JSON.parse` + a strict `Result.parse` on every line meant one unusable record threw out of + * `search()` and discarded every match already collected from unrelated files — the same defect + * fixed in packages/core/src/ripgrep.ts. Records are independent, so a bad one is dropped and + * counted. Exported so the skip paths are testable without a stub ripgrep binary. + */ + export function parseRecords(lines: string[]): Match["data"][] { + const matches: Match["data"][] = [] + let skipped = 0 + for (const line of lines) { + // Bounds parse cost per record. This path buffers all of stdout before splitting, so it does + // not bound total memory — that needs streaming, tracked separately. + const parsed = + Buffer.byteLength(line, "utf8") > MAX_RECORD_BYTES ? undefined : Result.safeParse(normalizeRecord(line)) + if (!parsed?.success) { + skipped++ + continue + } + if (parsed.data.type === "match") matches.push(parsed.data.data) + } + // Counted and reported once rather than per record: without this a ripgrep protocol change + // would make `/find` answer `[]`, which is indistinguishable from an honest "no matches". + if (skipped > 0) log.warn("skipped unusable ripgrep records", { skipped, total: lines.length }) + return matches + } // altimate_change end export type Result = z.infer @@ -425,25 +465,7 @@ export namespace Ripgrep { // Parse JSON lines from ripgrep output // altimate_change start — upstream_fix: a bad record skips itself, not the whole search. - // `JSON.parse` + a strict `Result.parse` on every line meant one unusable record threw out of - // `search()` and discarded every match already collected from unrelated files — the same defect - // fixed in packages/core/src/ripgrep.ts. Records are independent, so a bad one is dropped. - // `lines`/`path`/`match` are `{text}` only when the value is valid UTF-8 and `{bytes}` otherwise, - // so the `{bytes}` arm is normalised rather than left to fail the strict schema. - const matches: Match["data"][] = [] - let skipped = 0 - for (const line of lines) { - const parsed = Result.safeParse(normalizeRecord(line)) - if (!parsed.success) { - skipped++ - continue - } - if (parsed.data.type === "match") matches.push(parsed.data.data) - } - // Counted and reported once rather than per record: without this a ripgrep protocol change - // would make `/find` answer `[]`, which is indistinguishable from an honest "no matches". - if (skipped > 0) log.warn("skipped unusable ripgrep records", { skipped, total: lines.length }) - return matches + return parseRecords(lines) // altimate_change end } } diff --git a/packages/opencode/test/file/ripgrep-search.test.ts b/packages/opencode/test/file/ripgrep-search.test.ts index 28bad3d0f5..2dc9a78ed6 100644 --- a/packages/opencode/test/file/ripgrep-search.test.ts +++ b/packages/opencode/test/file/ripgrep-search.test.ts @@ -39,4 +39,59 @@ describe("legacy Ripgrep.search", () => { expect(binary?.lines.text).toContain("tail") })) }) + +// `parseRecords` is exercised directly so the skip branches this PR adds — the `JSON.parse` catch, +// the size ceiling, the counter — are covered without depending on what the installed ripgrep build +// happens to emit. +describe("legacy Ripgrep.parseRecords", () => { + const record = (file: string, overrides: Record = {}) => + JSON.stringify({ + type: "match", + data: { + path: { text: `./${file}` }, + lines: { text: "needle\n" }, + line_number: 1, + absolute_offset: 0, + submatches: [{ match: { text: "needle" }, start: 0, end: 6 }], + ...overrides, + }, + }) + const paths = (records: string[]) => Ripgrep.parseRecords(records).map((match) => match.path.text) + + test("skips an unparseable record and keeps the ones around it", () => { + expect(paths([record("a.txt"), '{"type":"match","data":{"path":{"text":"./b.t', record("c.txt")])).toEqual([ + "./a.txt", + "./c.txt", + ]) + }) + + test("skips a record past the size ceiling", () => { + const huge = record("b.txt", { lines: { text: "n".repeat(17 * 1024 * 1024) } }) + expect(paths([record("a.txt"), huge, record("c.txt")])).toEqual(["./a.txt", "./c.txt"]) + }) + + test("skips a record whose path is not valid UTF-8, rather than mangling the path", () => { + const bad = record("ignored", { path: { bytes: Buffer.from("./b\xff.txt", "binary").toString("base64") } }) + expect(paths([record("a.txt"), bad])).toEqual(["./a.txt"]) + }) + + test("skips empty and non-canonical base64 rather than emitting an empty match", () => { + expect(paths([record("a.txt"), record("b.txt", { lines: { bytes: "" } })])).toEqual(["./a.txt"]) + expect(paths([record("a.txt"), record("b.txt", { lines: { bytes: "Zh==" } })])).toEqual(["./a.txt"]) + }) + + test("decodes a non-UTF8 line and ignores control records", () => { + const parsed = Ripgrep.parseRecords([ + JSON.stringify({ type: "begin", data: { path: { text: "./a.txt" } } }), + record("a.txt", { lines: { bytes: Buffer.from("needle \xff tail\n", "binary").toString("base64") } }), + ]) + expect(parsed).toHaveLength(1) + expect(parsed[0].lines.text).toBe("needle � tail\n") + }) + + test("returns an empty array when every record is unusable, rather than throwing", () => { + expect(() => Ripgrep.parseRecords(["{oops", "{also oops"])).not.toThrow() + expect(Ripgrep.parseRecords(["{oops", "{also oops"])).toEqual([]) + }) +}) // altimate_change end From 87e9504b8a097393d8918a37c3121402501caf2e Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 13 Aug 2026 17:30:01 +0530 Subject: [PATCH 03/11] fix(core): wrap the grep tally in altimate_change markers Marker Guard failed on the previous commit: converting `grep` to a block body to hold the per-invocation skip tally changed an upstream-shared line without markers, so a future upstream merge could silently drop it. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/ripgrep.ts | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/core/src/ripgrep.ts b/packages/core/src/ripgrep.ts index 0860263b0c..db79d6ece5 100644 --- a/packages/core/src/ripgrep.ts +++ b/packages/core/src/ripgrep.ts @@ -347,10 +347,13 @@ export const layer = Layer.effect( Effect.map((result) => result.items), Effect.catchTag("Ripgrep.InvalidPatternError", (cause) => Effect.fail(failure(cause.message, cause))), ), + // altimate_change start — upstream_fix: tally skipped records for one aggregate warning. + // Upstream returns `run(...)` directly; the body exists only to hold the tally, which must be + // per invocation and never per layer so two concurrent searches cannot share it. grep: (input) => { - // Per invocation, never per layer: two concurrent searches must not share a tally. const skipped: { count: number; samples: string[] } = { count: 0, samples: [] } return run({ + // altimate_change end ...input, args: [ "--no-config", @@ -417,9 +420,10 @@ export const layer = Layer.effect( }, // altimate_change end }).pipe( - // One aggregate warning per search, not one per record: a systematic protocol mismatch - // rejects every record in the tree, and a per-record log would bury the machine in noise - // while still answering with an innocent-looking empty result. + // altimate_change start — upstream_fix: one aggregate warning per search, not per record. + // A systematic protocol mismatch rejects every record in the tree, and a per-record log + // would bury the machine in noise while still answering with an innocent-looking empty + // result — the very failure this change exists to prevent. Effect.tap(() => skipped.count > 0 ? Effect.logWarning("skipped unusable ripgrep records", { @@ -428,6 +432,7 @@ export const layer = Layer.effect( }) : Effect.void, ), + // altimate_change end Effect.map((result) => result.items.map((match) => { const relative = match.path.text @@ -455,8 +460,10 @@ export const layer = Layer.effect( }) }), ), + // altimate_change start — upstream_fix: closes the block body opened for the skip tally. ) }, + // altimate_change end }) }), ) From fe122a9e0c778aea8b42e78455f09df24e93a5aa Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 13 Aug 2026 19:17:09 +0530 Subject: [PATCH 04/11] =?UTF-8?q?fix(core):=20address=20bot=20review=20?= =?UTF-8?q?=E2=80=94=20tally=20lifetime,=20legacy=20offsets,=20portability?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit findings on the ready-for-review PR. - The skip tally was captured when `grep(input)` BUILT the Effect, not when it ran. An Effect is a value that can be executed more than once and concurrently, so counts accumulated across executions and the aggregate warning over-reported. `Effect.suspend` gives each execution its own tally, which is what the code already claimed to do. - Report the tally from `Effect.onExit` rather than `Effect.tap`. `tap` runs on success only, so a search that failed or was interrupted — exactly when the diagnostic matters most — discarded it silently. - Rebase submatch offsets in the legacy parser too. Core was fixed last round but legacy was not, and since `/find` publishes this shape the unrebased offsets were newly wrong OUTPUT rather than a skipped record. - Skip the stub-rg cases on win32: the stub is a POSIX shell script and `chmod` is a no-op there, so they could not have passed. Windows ripgrep behaviour keeps its own coverage in script/windows-ripgrep-e2e.ts. - Give the real-binary legacy test an explicit timeout, since a cold cache downloads a ripgrep release archive inside it. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/ripgrep.ts | 235 +++++++++--------- packages/core/test/ripgrep.test.ts | 28 ++- packages/opencode/src/file/ripgrep.ts | 42 +++- .../opencode/test/file/ripgrep-search.test.ts | 58 +++-- 4 files changed, 209 insertions(+), 154 deletions(-) diff --git a/packages/core/src/ripgrep.ts b/packages/core/src/ripgrep.ts index db79d6ece5..a885bc82cc 100644 --- a/packages/core/src/ripgrep.ts +++ b/packages/core/src/ripgrep.ts @@ -348,122 +348,129 @@ export const layer = Layer.effect( Effect.catchTag("Ripgrep.InvalidPatternError", (cause) => Effect.fail(failure(cause.message, cause))), ), // altimate_change start — upstream_fix: tally skipped records for one aggregate warning. - // Upstream returns `run(...)` directly; the body exists only to hold the tally, which must be - // per invocation and never per layer so two concurrent searches cannot share it. - grep: (input) => { - const skipped: { count: number; samples: string[] } = { count: 0, samples: [] } - return run({ - // altimate_change end - ...input, - args: [ - "--no-config", - "--json", - "--hidden", - "--no-messages", - // altimate_change start — upstream_fix: preserve all debug rg search --glob entries - ...(typeof input.include === "string" - ? [`--glob=${input.include}`] - : (input.include ?? []).map((pattern) => `--glob=${pattern}`)), - // altimate_change end - "--glob=!**/.git/**", - "--", - input.pattern, - input.file ?? ".", - ], - // altimate_change start — upstream_fix: a bad record skips itself, never the search. - // `parse` runs inside `Stream.mapEffect`, so ANY failure here aborts the whole stream and - // discards every match already collected from unrelated files. A record is independent of - // its neighbours, so none of the three ways one can be unusable — oversized, unparseable - // JSON, or schema-rejected — justifies destroying the rest of the search. - parse: (line) => { - const bytes = Buffer.byteLength(line, "utf8") - // Captured during the walk so the aggregate warning can name a file when one is - // recoverable. Malformed JSON has no path by definition, hence "when present". - let where: string | undefined - return Effect.gen(function* () { - // Checked before JSON.parse purely to bound parse cost; `Stream.splitLines` has - // already materialized the line, so this cannot bound memory. See MAX_RECORD_BYTES. - if (bytes > MAX_RECORD_BYTES) - return yield* Effect.fail(failure(`record exceeded ${MAX_RECORD_BYTES} bytes`)) - const json = yield* Effect.try({ - try: () => JSON.parse(line) as unknown, - catch: (cause) => failure("unparseable JSON", cause), - }) - if (!json || typeof json !== "object" || !("type" in json)) - return yield* Effect.fail(failure("record has no type")) - // Captured before the type check so an unrecognised record can still name its file. - const pathField = readProp(readProp(json, "data"), "path") - const pathText = readProp(pathField, "text") - if (typeof pathText === "string") where = pathText - // Control records are expected and simply carry no match. An unrecognised type is a - // protocol surprise and is counted rather than dropped on the floor, so a ripgrep - // change cannot quietly turn every match into "no matches". - if (json.type !== "match") - return typeof json.type === "string" && CONTROL_TYPES.has(json.type) - ? undefined - : yield* Effect.fail(failure(`unrecognised record type ${JSON.stringify(json.type)}`)) - const match = yield* Schema.decodeUnknownEffect(RawMatch)(normalizeMatch(json)).pipe( - Effect.mapError((cause) => failure("unexpected match shape", cause)), + // Upstream returns `run(...)` directly. `Effect.suspend` — rather than a plain block body — + // is what makes the tally per EXECUTION: an Effect is a value that can be run more than once + // and concurrently, so a tally captured when the Effect is built would accumulate across runs + // and over-report. + // + // The marked region covers the whole implementation rather than just these lines, because the + // wrapper re-indents every line of the body: an upstream change anywhere in here genuinely + // needs manual reconciliation, which is exactly what the marker is for. + grep: (input) => + Effect.suspend(() => { + const skipped: { count: number; samples: string[] } = { count: 0, samples: [] } + return run({ + ...input, + args: [ + "--no-config", + "--json", + "--hidden", + "--no-messages", + // altimate_change start — upstream_fix: preserve all debug rg search --glob entries + ...(typeof input.include === "string" + ? [`--glob=${input.include}`] + : (input.include ?? []).map((pattern) => `--glob=${pattern}`)), + // altimate_change end + "--glob=!**/.git/**", + "--", + input.pattern, + input.file ?? ".", + ], + // altimate_change start — upstream_fix: a bad record skips itself, never the search. + // `parse` runs inside `Stream.mapEffect`, so ANY failure here aborts the whole stream and + // discards every match already collected from unrelated files. A record is independent of + // its neighbours, so none of the three ways one can be unusable — oversized, unparseable + // JSON, or schema-rejected — justifies destroying the rest of the search. + parse: (line) => { + const bytes = Buffer.byteLength(line, "utf8") + // Captured during the walk so the aggregate warning can name a file when one is + // recoverable. Malformed JSON has no path by definition, hence "when present". + let where: string | undefined + return Effect.gen(function* () { + // Checked before JSON.parse purely to bound parse cost; `Stream.splitLines` has + // already materialized the line, so this cannot bound memory. See MAX_RECORD_BYTES. + if (bytes > MAX_RECORD_BYTES) + return yield* Effect.fail(failure(`record exceeded ${MAX_RECORD_BYTES} bytes`)) + const json = yield* Effect.try({ + try: () => JSON.parse(line) as unknown, + catch: (cause) => failure("unparseable JSON", cause), + }) + if (!json || typeof json !== "object" || !("type" in json)) + return yield* Effect.fail(failure("record has no type")) + // Captured before the type check so an unrecognised record can still name its file. + const pathField = readProp(readProp(json, "data"), "path") + const pathText = readProp(pathField, "text") + if (typeof pathText === "string") where = pathText + // Control records are expected and simply carry no match. An unrecognised type is a + // protocol surprise and is counted rather than dropped on the floor, so a ripgrep + // change cannot quietly turn every match into "no matches". + if (json.type !== "match") + return typeof json.type === "string" && CONTROL_TYPES.has(json.type) + ? undefined + : yield* Effect.fail(failure(`unrecognised record type ${JSON.stringify(json.type)}`)) + const match = yield* Schema.decodeUnknownEffect(RawMatch)(normalizeMatch(json)).pipe( + Effect.mapError((cause) => failure("unexpected match shape", cause)), + ) + // `normalizeMatch` already caps submatches and line text, so nothing is re-trimmed. + return { ...match.data, path: { text: match.data.path.text.replace(/^\.[\\/]/, "") } } + }).pipe( + Effect.catch((cause) => + Effect.sync(() => { + skipped.count++ + if (skipped.samples.length < SKIP_SAMPLES) + skipped.samples.push(where ? `${cause.message} (${where})` : cause.message) + return undefined + }), + ), ) - // `normalizeMatch` already caps submatches and line text, so nothing is re-trimmed. - return { ...match.data, path: { text: match.data.path.text.replace(/^\.[\\/]/, "") } } - }).pipe( - Effect.catch((cause) => - Effect.sync(() => { - skipped.count++ - if (skipped.samples.length < SKIP_SAMPLES) - skipped.samples.push(where ? `${cause.message} (${where})` : cause.message) - return undefined - }), - ), - ) - }, - // altimate_change end - }).pipe( - // altimate_change start — upstream_fix: one aggregate warning per search, not per record. - // A systematic protocol mismatch rejects every record in the tree, and a per-record log - // would bury the machine in noise while still answering with an innocent-looking empty - // result — the very failure this change exists to prevent. - Effect.tap(() => - skipped.count > 0 - ? Effect.logWarning("skipped unusable ripgrep records", { - skipped: skipped.count, - reasons: skipped.samples, + }, + // altimate_change end + }).pipe( + // altimate_change start — upstream_fix: one aggregate warning per search, not per record. + // A systematic protocol mismatch rejects every record in the tree, and a per-record log + // would bury the machine in noise while still answering with an innocent-looking empty + // result — the very failure this change exists to prevent. + // `onExit` rather than `tap`: a search that fails or is interrupted part-way is exactly + // when the diagnostic matters, and `tap` would discard the tally in both cases. + Effect.onExit(() => + skipped.count > 0 + ? Effect.logWarning("skipped unusable ripgrep records", { + skipped: skipped.count, + reasons: skipped.samples, + }) + : Effect.void, + ), + // altimate_change end + Effect.map((result) => + result.items.map((match) => { + const relative = match.path.text + .replace(/^(?:\.[\\/])+/u, "") + .replace(/^[\\/]+/u, "") + .replaceAll("\\", "/") + const absolute = path.resolve(input.cwd, relative) + return new Match({ + entry: new Entry({ + path: RelativePath.make(relative), + type: "file", + mime: FSUtil.mimeType(absolute), + }), + line: match.line_number, + offset: match.absolute_offset, + // altimate_change start — upstream_fix: capped at parse time, see LINE_TEXT_CAP. + // Re-applied here so the cap still holds if the parser ever stops trimming. + text: capLineText(match.lines.text), + // altimate_change end + submatches: match.submatches.map((submatch) => ({ + text: submatch.match.text, + start: submatch.start, + end: submatch.end, + })), }) - : Effect.void, - ), - // altimate_change end - Effect.map((result) => - result.items.map((match) => { - const relative = match.path.text - .replace(/^(?:\.[\\/])+/u, "") - .replace(/^[\\/]+/u, "") - .replaceAll("\\", "/") - const absolute = path.resolve(input.cwd, relative) - return new Match({ - entry: new Entry({ - path: RelativePath.make(relative), - type: "file", - mime: FSUtil.mimeType(absolute), - }), - line: match.line_number, - offset: match.absolute_offset, - // altimate_change start — upstream_fix: capped at parse time, see LINE_TEXT_CAP. - // Re-applied here so the cap still holds if the parser ever stops trimming. - text: capLineText(match.lines.text), - // altimate_change end - submatches: match.submatches.map((submatch) => ({ - text: submatch.match.text, - start: submatch.start, - end: submatch.end, - })), - }) - }), - ), - // altimate_change start — upstream_fix: closes the block body opened for the skip tally. - ) - }, - // altimate_change end + }), + ), + ) + }), + // altimate_change end — closes the grep block opened above for the skip tally. }) }), ) diff --git a/packages/core/test/ripgrep.test.ts b/packages/core/test/ripgrep.test.ts index c44a416843..189ef4c158 100644 --- a/packages/core/test/ripgrep.test.ts +++ b/packages/core/test/ripgrep.test.ts @@ -188,6 +188,10 @@ describe("Ripgrep", () => { ), ]) + // The stub is a POSIX shell script and `chmod` is a no-op on win32, so the spawn cannot work + // there. Windows ripgrep behaviour has its own coverage in script/windows-ripgrep-e2e.ts. + const stubTest = bunTest.skipIf(process.platform === "win32") + const grepWithStubbedRecords = (records: string[]) => Effect.acquireUseRelease( Effect.promise(() => tmpdir()), @@ -224,7 +228,7 @@ describe("Ripgrep", () => { /** The single aggregate warning for the last search, or undefined when nothing was skipped. */ const lastSkip = () => skipWarnings.at(-1) - bunTest("skips an unparseable record without failing the search", async () => { + stubTest("skips an unparseable record without failing the search", async () => { const matches = await Effect.runPromise( grepWithStubbedRecords([ matchRecord("a.txt"), @@ -240,7 +244,7 @@ describe("Ripgrep", () => { // The size ceiling is asserted on a record built to exceed it, rather than inferred from a large // file — that keeps the case independent of whether a given ripgrep build emits the match at all. - bunTest("skips an oversized record and keeps parsing the records after it", async () => { + stubTest("skips an oversized record and keeps parsing the records after it", async () => { const matches = await Effect.runPromise( grepWithStubbedRecords([ matchRecord("a.txt"), @@ -254,7 +258,7 @@ describe("Ripgrep", () => { // A path is an identifier the caller reopens, so it must never be lossily decoded. Such a record // is skipped rather than reported under a U+FFFD-mangled path that names no real file. - bunTest("skips a match whose path is not valid UTF-8, keeping the rest", async () => { + stubTest("skips a match whose path is not valid UTF-8, keeping the rest", async () => { const matches = await Effect.runPromise( grepWithStubbedRecords([ matchRecord("a.txt"), @@ -268,7 +272,7 @@ describe("Ripgrep", () => { // `Buffer.from` maps unconvertible base64 to an empty buffer instead of throwing, which would turn // a corrupt record into a schema-valid EMPTY match. It must be skipped, not silently emptied. - bunTest("skips a record whose bytes field is not valid base64", async () => { + stubTest("skips a record whose bytes field is not valid base64", async () => { const matches = await Effect.runPromise( grepWithStubbedRecords([ matchRecord("a.txt"), @@ -283,7 +287,7 @@ describe("Ripgrep", () => { // Submatch offsets are BYTE offsets into the raw line. A lossy decode widens every undecodable // byte to a 3-byte U+FFFD, so the raw offsets no longer locate the match and must be rebased onto // the decoded text's own UTF-8 encoding. Without this the match reads "��need". - bunTest("rebases submatch offsets onto the decoded line after a lossy decode", async () => { + stubTest("rebases submatch offsets onto the decoded line after a lossy decode", async () => { const raw = Buffer.concat([Buffer.from([0xff]), Buffer.from("needle tail\n")]) const matches = await Effect.runPromise( grepWithStubbedRecords([ @@ -302,7 +306,7 @@ describe("Ripgrep", () => { expect(Buffer.from(text, "utf8").subarray(submatches[0].start, submatches[0].end).toString("utf8")).toBe("needle") }) - bunTest("skips a record whose bytes field is empty rather than emitting an empty match", async () => { + stubTest("skips a record whose bytes field is empty rather than emitting an empty match", async () => { const matches = await Effect.runPromise( grepWithStubbedRecords([matchRecord("a.txt"), matchRecord("b.txt", { lines: { bytes: "" } })]), ) @@ -311,7 +315,7 @@ describe("Ripgrep", () => { expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt")]) }) - bunTest("skips a record whose bytes field uses non-canonical padding", async () => { + stubTest("skips a record whose bytes field uses non-canonical padding", async () => { // "Zh==" and "Zg==" both decode to "f"; only the canonical spelling round-trips. const matches = await Effect.runPromise( grepWithStubbedRecords([matchRecord("a.txt"), matchRecord("b.txt", { lines: { bytes: "Zh==" } })]), @@ -322,7 +326,7 @@ describe("Ripgrep", () => { // Control records carry no match and are ignored silently; an unrecognised type is a protocol // surprise and must be counted, or a ripgrep change turns every match into an innocent "no match". - bunTest("ignores control records but counts records with an unknown type", async () => { + stubTest("ignores control records but counts records with an unknown type", async () => { const matches = await Effect.runPromise( grepWithStubbedRecords([ JSON.stringify({ type: "begin", data: { path: { text: "./a.txt" } } }), @@ -341,14 +345,14 @@ describe("Ripgrep", () => { expect(lastSkip()?.reasons).toEqual([`unrecognised record type "match-v2" (./b.txt)`, "record has no type"]) }) - bunTest("returns an empty result when every record is unusable, rather than failing", async () => { + stubTest("returns an empty result when every record is unusable, rather than failing", async () => { const matches = await Effect.runPromise(grepWithStubbedRecords(["{oops", "{also oops", "{still oops"])) expect(matches).toEqual([]) }) // Pins the ceiling itself: at the limit the record is kept, one byte over it is skipped. - bunTest("accepts a record at exactly the size ceiling and skips one byte over", async () => { + stubTest("accepts a record at exactly the size ceiling and skips one byte over", async () => { const sizeOf = (file: string, padding: number) => matchRecord(file, { lines: { text: "n".repeat(padding) } }) const overhead = Buffer.byteLength(sizeOf("a.txt", 0), "utf8") const limit = 16 * 1024 * 1024 @@ -363,7 +367,7 @@ describe("Ripgrep", () => { // Pins the OUTPUT contract of the cap. Note it cannot prove the retained-memory improvement that // motivated moving the cap into the parser: capping at parse time and capping at the end produce // byte-identical output, and only the peak heap during collection differs. - bunTest("caps the returned line text and keeps the elision marker", async () => { + stubTest("caps the returned line text and keeps the elision marker", async () => { const matches = await Effect.runPromise( grepWithStubbedRecords([matchRecord("a.txt", { lines: { text: "needle" + "x".repeat(50_000) } })]), ) @@ -372,7 +376,7 @@ describe("Ripgrep", () => { expect(matches[0].text.endsWith("...")).toBe(true) }) - bunTest("decodes a non-UTF8 match line to replacement characters", async () => { + stubTest("decodes a non-UTF8 match line to replacement characters", async () => { const matches = await Effect.runPromise( grepWithStubbedRecords([ matchRecord("a.txt", { diff --git a/packages/opencode/src/file/ripgrep.ts b/packages/opencode/src/file/ripgrep.ts index 321520f59e..50bf69d407 100644 --- a/packages/opencode/src/file/ripgrep.ts +++ b/packages/opencode/src/file/ripgrep.ts @@ -119,18 +119,32 @@ export namespace Ripgrep { value !== null && typeof value === "object" && key in value ? Reflect.get(value, key) : undefined const data = read(json, "data") if (!data || typeof data !== "object") return json - const asText = (value: unknown): unknown => { - if (!value || typeof value !== "object" || "text" in value) return value + /** Decode a `{text}`/`{bytes}` field, returning the raw buffer so offsets can be rebased. */ + const decode = (value: unknown): { text: string; raw?: Buffer } | undefined => { + if (!value || typeof value !== "object") return undefined + const text = read(value, "text") + if (typeof text === "string") return { text } const bytes = read(value, "bytes") // Guarded three ways because `Buffer.from` decodes unconvertible input to an EMPTY buffer // instead of throwing, which would turn a corrupt record into a schema-valid empty match: // reject the empty string (a matched line is never empty), check the spelling, then require // a round-trip so non-canonical padding ("Zh==" and "Zg==" both decode to "f") is rejected. - if (typeof bytes !== "string" || bytes.length === 0 || !BASE64.test(bytes)) return value + if (typeof bytes !== "string" || bytes.length === 0 || !BASE64.test(bytes)) return undefined const decoded = Buffer.from(bytes, "base64") - if (decoded.toString("base64") !== bytes) return value - return { text: decoded.toString("utf8") } + if (decoded.toString("base64") !== bytes) return undefined + return { text: decoded.toString("utf8"), raw: decoded } } + const lines = "lines" in data ? decode(read(data, "lines")) : undefined + // Submatch offsets are BYTE offsets into the RAW line, and a lossy decode widens every + // undecodable byte to a 3-byte U+FFFD — so they must be rebased onto the decoded text's own + // UTF-8 encoding or they no longer locate the match. This response shape is published by the + // `/find` route, so unrebased offsets would be newly wrong output rather than a skipped record. + // Mirrors packages/core/src/ripgrep.ts. + const raw = lines?.raw + const rebase = (offset: unknown): unknown => + raw && typeof offset === "number" && offset >= 0 + ? Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") + : offset const submatches = read(data, "submatches") // Only rewrite keys the record actually carries — `begin`/`end`/`summary` records reach here too // and must keep their exact shape, or the strict union below would reject them. @@ -141,14 +155,20 @@ export namespace Ripgrep { ...json, data: { ...data, - ...("lines" in data ? { lines: asText(read(data, "lines")) } : {}), + ...(lines ? { lines: { text: lines.text } } : {}), ...(Array.isArray(submatches) ? { - submatches: submatches.map((submatch) => - submatch && typeof submatch === "object" - ? { ...submatch, match: asText(read(submatch, "match")) } - : submatch, - ), + submatches: submatches.map((submatch) => { + if (!submatch || typeof submatch !== "object") return submatch + const match = decode(read(submatch, "match")) + if (!match) return submatch + return { + ...submatch, + match: { text: match.text }, + start: rebase(read(submatch, "start")), + end: rebase(read(submatch, "end")), + } + }), } : {}), }, diff --git a/packages/opencode/test/file/ripgrep-search.test.ts b/packages/opencode/test/file/ripgrep-search.test.ts index 2dc9a78ed6..7dd283d3a2 100644 --- a/packages/opencode/test/file/ripgrep-search.test.ts +++ b/packages/opencode/test/file/ripgrep-search.test.ts @@ -19,25 +19,31 @@ const withRepo = async (run: (dir: string) => Promise) => { } describe("legacy Ripgrep.search", () => { - test("returns matches from a file whose matched line is not valid UTF-8", () => - withRepo(async (dir) => { - await fs.writeFile(path.join(dir, "a-plain.txt"), "needle here\n") - // ripgrep emits `{"bytes": ""}` rather than `{"text": ...}` for this line, which the - // strict Zod schema rejected — taking the unrelated matches down with it. - await fs.writeFile(path.join(dir, "b-binary.txt"), Buffer.from("needle \xff\xfe tail\n", "binary")) - await fs.writeFile(path.join(dir, "c-plain.txt"), "needle here\n") + // Explicit timeout: this drives the real binary, and `search()` resolves it through `state()`, + // which downloads a release archive when `rg` is absent from PATH and from Global.Path.bin. + test( + "returns matches from a file whose matched line is not valid UTF-8", + () => + withRepo(async (dir) => { + await fs.writeFile(path.join(dir, "a-plain.txt"), "needle here\n") + // ripgrep emits `{"bytes": ""}` rather than `{"text": ...}` for this line, which the + // strict Zod schema rejected — taking the unrelated matches down with it. + await fs.writeFile(path.join(dir, "b-binary.txt"), Buffer.from("needle \xff\xfe tail\n", "binary")) + await fs.writeFile(path.join(dir, "c-plain.txt"), "needle here\n") - const matches = await Ripgrep.search({ cwd: dir, pattern: "needle", limit: 10 }) + const matches = await Ripgrep.search({ cwd: dir, pattern: "needle", limit: 10 }) - expect(matches.map((match) => match.path.text.replace(/^\.\//, "")).sort()).toEqual([ - "a-plain.txt", - "b-binary.txt", - "c-plain.txt", - ]) - const binary = matches.find((match) => match.path.text.includes("b-binary.txt")) - expect(binary?.lines.text).toContain("needle") - expect(binary?.lines.text).toContain("tail") - })) + expect(matches.map((match) => match.path.text.replace(/^\.\//, "")).sort()).toEqual([ + "a-plain.txt", + "b-binary.txt", + "c-plain.txt", + ]) + const binary = matches.find((match) => match.path.text.includes("b-binary.txt")) + expect(binary?.lines.text).toContain("needle") + expect(binary?.lines.text).toContain("tail") + }), + 120_000, + ) }) // `parseRecords` is exercised directly so the skip branches this PR adds — the `JSON.parse` catch, @@ -89,6 +95,24 @@ describe("legacy Ripgrep.parseRecords", () => { expect(parsed[0].lines.text).toBe("needle � tail\n") }) + // Offsets are byte offsets into the RAW line; a lossy decode widens each undecodable byte to a + // 3-byte U+FFFD. This response shape is published by the `/find` route, so leaving them unrebased + // would be newly wrong output rather than a skipped record. Mirrors the core parser. + test("rebases submatch offsets after a lossy line decode", () => { + const raw = Buffer.concat([Buffer.from([0xff]), Buffer.from("needle tail\n")]) + const parsed = Ripgrep.parseRecords([ + record("a.txt", { + lines: { bytes: raw.toString("base64") }, + submatches: [{ match: { bytes: Buffer.from("needle").toString("base64") }, start: 1, end: 7 }], + }), + ]) + + expect(parsed).toHaveLength(1) + const [{ lines, submatches }] = parsed + expect(submatches[0]).toEqual({ match: { text: "needle" }, start: 3, end: 9 }) + expect(Buffer.from(lines.text, "utf8").subarray(3, 9).toString("utf8")).toBe("needle") + }) + test("returns an empty array when every record is unusable, rather than throwing", () => { expect(() => Ripgrep.parseRecords(["{oops", "{also oops"])).not.toThrow() expect(Ripgrep.parseRecords(["{oops", "{also oops"])).toEqual([]) From 32cfa3315e9af5ad8abc97045c4062997524903f Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 13 Aug 2026 19:50:17 +0530 Subject: [PATCH 05/11] fix(core): reject unaddressable submatch offsets instead of clamping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second bot-review round (cubic, kilo). - `Buffer.subarray` clamps an out-of-range end and truncates a fractional one rather than throwing, so rebasing an offset without a range check quietly repaired a corrupt offset into a plausible-looking one. Neither schema catches it: core `NonNegativeInt` and legacy `z.number()` both accept a number well past the end of the line. An unaddressable offset now marks the record corrupt so it is skipped and counted, in both parsers. - Correct an overstated comment: the win32 skip claimed Windows ripgrep behaviour was covered by script/windows-ripgrep-e2e.ts, but that script covers only binary resolution, extraction and one real search — none of the record-parsing behaviour these stub cases pin. The comment now states the gap. Not changed, with reasons: - Submatch offsets still index the full line after the 2000-char cap. That is the tracked windowing follow-up, and the observable output is unchanged by this branch — the cap moved earlier, it did not become lossier. - The legacy parser still decodes every submatch rather than slicing to MAX_SUBMATCHES first. Its response shape is published by `/find`, so slicing would change that contract; the cost is already bounded by the record ceiling. - The second capLineText call in the result mapping is a deliberate guard on the public output, not dead code, and is documented as such. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/ripgrep.ts | 23 ++++++++++++---- packages/core/test/ripgrep.test.ts | 26 ++++++++++++++++++- packages/opencode/src/file/ripgrep.ts | 20 ++++++++++---- .../opencode/test/file/ripgrep-search.test.ts | 10 +++++++ 4 files changed, 68 insertions(+), 11 deletions(-) diff --git a/packages/core/src/ripgrep.ts b/packages/core/src/ripgrep.ts index a885bc82cc..7807691c3e 100644 --- a/packages/core/src/ripgrep.ts +++ b/packages/core/src/ripgrep.ts @@ -130,12 +130,24 @@ const normalizeMatch = (json: object): unknown => { const lines = decodeField(readProp(data, "lines")) if (!lines) return json const raw = lines.raw - const rebase = (offset: unknown): unknown => - raw && typeof offset === "number" && offset >= 0 - ? Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") - : offset + // Rebasing is a claim about coordinates, so it is only made for an offset actually addressable in + // the raw line. `Buffer.subarray` clamps an out-of-range end and truncates a fractional one rather + // than throwing, so an unchecked rebase would quietly repair a corrupt offset into a + // plausible-looking one — and the schema would not catch it, since `NonNegativeInt` happily + // accepts a number past the end of the line. An unaddressable offset therefore marks the whole + // record corrupt (undefined) so it is skipped and counted. Offsets on the `{text}` arm are left + // alone: no rebasing happens there, so no claim is made and ripgrep's values stand as before. + let corrupt = false + const rebase = (offset: unknown): unknown => { + if (!raw) return offset + if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) { + corrupt = true + return offset + } + return Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") + } const submatches = readProp(data, "submatches") - return { + const normalized = { ...json, data: { ...data, @@ -158,6 +170,7 @@ const normalizeMatch = (json: object): unknown => { : submatches, }, } + return corrupt ? undefined : normalized } // altimate_change end diff --git a/packages/core/test/ripgrep.test.ts b/packages/core/test/ripgrep.test.ts index 189ef4c158..214bdd7efa 100644 --- a/packages/core/test/ripgrep.test.ts +++ b/packages/core/test/ripgrep.test.ts @@ -189,7 +189,11 @@ describe("Ripgrep", () => { ]) // The stub is a POSIX shell script and `chmod` is a no-op on win32, so the spawn cannot work - // there. Windows ripgrep behaviour has its own coverage in script/windows-ripgrep-e2e.ts. + // there. Be clear about the cost: this leaves the record-parsing behaviour below — skipping, + // decoding, offset rebasing, the size ceiling — WITHOUT Windows coverage. script/windows- + // ripgrep-e2e.ts covers only binary resolution, extraction and one real search, not any of this. + // Closing the gap needs a `.cmd` stub on win32; the parser itself is platform-independent, so the + // risk is a Windows-only spawn/quoting regression going unnoticed rather than a parsing one. const stubTest = bunTest.skipIf(process.platform === "win32") const grepWithStubbedRecords = (records: string[]) => @@ -306,6 +310,26 @@ describe("Ripgrep", () => { expect(Buffer.from(text, "utf8").subarray(submatches[0].start, submatches[0].end).toString("utf8")).toBe("needle") }) + // `Buffer.subarray` clamps an out-of-range end and truncates a fractional one instead of throwing, + // so rebasing without a range check would turn a corrupt offset into a plausible-looking one. + stubTest("skips a record whose submatch offset is not addressable in the line", async () => { + const raw = Buffer.concat([Buffer.from([0xff]), Buffer.from("needle tail\n")]) + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("a.txt"), + matchRecord("b.txt", { + lines: { bytes: raw.toString("base64") }, + // Well past the end of the raw line — nonsense that must not be silently clamped. + submatches: [{ match: { text: "needle" }, start: 1, end: 9_999 }], + }), + matchRecord("c.txt"), + ]), + ) + + expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt"), RelativePath.make("c.txt")]) + expect(lastSkip()?.skipped).toBe(1) + }) + stubTest("skips a record whose bytes field is empty rather than emitting an empty match", async () => { const matches = await Effect.runPromise( grepWithStubbedRecords([matchRecord("a.txt"), matchRecord("b.txt", { lines: { bytes: "" } })]), diff --git a/packages/opencode/src/file/ripgrep.ts b/packages/opencode/src/file/ripgrep.ts index 50bf69d407..456993274e 100644 --- a/packages/opencode/src/file/ripgrep.ts +++ b/packages/opencode/src/file/ripgrep.ts @@ -141,17 +141,26 @@ export namespace Ripgrep { // `/find` route, so unrebased offsets would be newly wrong output rather than a skipped record. // Mirrors packages/core/src/ripgrep.ts. const raw = lines?.raw - const rebase = (offset: unknown): unknown => - raw && typeof offset === "number" && offset >= 0 - ? Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") - : offset + // Only rebase an offset addressable in the raw line: `Buffer.subarray` clamps an out-of-range + // end and truncates a fractional one rather than throwing, so an unchecked rebase would quietly + // repair a corrupt offset, and the schema would not catch it (`z.number()` accepts any number). + // An unaddressable offset marks the record corrupt so it is skipped and counted instead. + let corrupt = false + const rebase = (offset: unknown): unknown => { + if (!raw) return offset + if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) { + corrupt = true + return offset + } + return Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") + } const submatches = read(data, "submatches") // Only rewrite keys the record actually carries — `begin`/`end`/`summary` records reach here too // and must keep their exact shape, or the strict union below would reject them. // `path` is deliberately left alone: decoding it is lossy, and a path is an identifier the // caller reopens, so a U+FFFD-mangled path names a file that does not exist. Such a record // stays in the `{bytes}` arm and is skipped. See packages/core/src/ripgrep.ts. - return { + const normalized = { ...json, data: { ...data, @@ -173,6 +182,7 @@ export namespace Ripgrep { : {}), }, } + return corrupt ? undefined : normalized } /** diff --git a/packages/opencode/test/file/ripgrep-search.test.ts b/packages/opencode/test/file/ripgrep-search.test.ts index 7dd283d3a2..765832fad2 100644 --- a/packages/opencode/test/file/ripgrep-search.test.ts +++ b/packages/opencode/test/file/ripgrep-search.test.ts @@ -113,6 +113,16 @@ describe("legacy Ripgrep.parseRecords", () => { expect(Buffer.from(lines.text, "utf8").subarray(3, 9).toString("utf8")).toBe("needle") }) + test("skips a record whose submatch offset is not addressable in the line", () => { + const raw = Buffer.concat([Buffer.from([0xff]), Buffer.from("needle tail\n")]) + const bad = record("b.txt", { + lines: { bytes: raw.toString("base64") }, + submatches: [{ match: { text: "needle" }, start: 1, end: 9_999 }], + }) + // `z.number()` would accept 9999 happily, so the range check is what rejects this. + expect(paths([record("a.txt"), bad, record("c.txt")])).toEqual(["./a.txt", "./c.txt"]) + }) + test("returns an empty array when every record is unusable, rather than throwing", () => { expect(() => Ripgrep.parseRecords(["{oops", "{also oops"])).not.toThrow() expect(Ripgrep.parseRecords(["{oops", "{also oops"])).toEqual([]) From 7eb9528e1d0045c432138d811f07681a9800a6d4 Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 13 Aug 2026 20:21:56 +0530 Subject: [PATCH 06/11] fix(core): reject submatch offsets that split a multi-byte character MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Decoding `raw.subarray(0, offset)` in isolation is not the same as taking a prefix of the full decode when the offset lands inside a VALID multi-byte sequence. `Buffer.from("éneedle")` sliced at byte 1 decodes to U+FFFD, so the offset rebased to 3 — a plausible value pointing at the wrong character in a line that decoded cleanly there. A byte-mode pattern can match at such a position, so this was reachable rather than theoretical. The prefix must now actually prefix the decoded line; an offset landing mid-character is unaddressable and marks the record corrupt, so it is skipped and counted like any other unusable record. Costs nothing asymptotically — decoding the prefix was already O(offset). A cheaper O(1) test (rejecting when the byte at the offset is a UTF-8 continuation byte) was measured against the exact check over 1.3M fuzzed offsets and rejected: it disagrees on ~243k of them, always by over-rejecting lines that begin with a stray continuation byte, which would drop valid matches on binary files. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/ripgrep.ts | 13 ++++++++++++- packages/core/test/ripgrep.test.ts | 20 ++++++++++++++++++++ packages/opencode/src/file/ripgrep.ts | 12 ++++++++++-- 3 files changed, 42 insertions(+), 3 deletions(-) diff --git a/packages/core/src/ripgrep.ts b/packages/core/src/ripgrep.ts index 7807691c3e..29a95e3c9a 100644 --- a/packages/core/src/ripgrep.ts +++ b/packages/core/src/ripgrep.ts @@ -144,7 +144,18 @@ const normalizeMatch = (json: object): unknown => { corrupt = true return offset } - return Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") + // Decoding the prefix in isolation is not the same as taking a prefix of the full decode when + // the offset splits a VALID multi-byte sequence: `Buffer.from("éneedle")` sliced at byte 1 + // decodes to U+FFFD, so the offset would rebase to 3 and point at the wrong character in a line + // that decoded cleanly at that spot. Requiring the prefix to actually prefix the decoded line + // catches that; an offset that lands mid-character is not addressable and marks the record + // corrupt. This costs nothing asymptotically — decoding the prefix is already O(offset). + const prefix = raw.subarray(0, offset).toString("utf8") + if (!lines.text.startsWith(prefix)) { + corrupt = true + return offset + } + return Buffer.byteLength(prefix, "utf8") } const submatches = readProp(data, "submatches") const normalized = { diff --git a/packages/core/test/ripgrep.test.ts b/packages/core/test/ripgrep.test.ts index 214bdd7efa..3cbb4d35a4 100644 --- a/packages/core/test/ripgrep.test.ts +++ b/packages/core/test/ripgrep.test.ts @@ -330,6 +330,26 @@ describe("Ripgrep", () => { expect(lastSkip()?.skipped).toBe(1) }) + // A byte-mode pattern can match inside a valid multi-byte character. Decoding the prefix alone + // then yields U+FFFD where the full decode has "é", so the offset would rebase to a plausible but + // wrong position (3, landing mid-word) instead of being recognised as unaddressable. + stubTest("skips a record whose submatch offset splits a multi-byte character", async () => { + const raw = Buffer.concat([Buffer.from("é"), Buffer.from("needle")]) + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("a.txt"), + matchRecord("b.txt", { + lines: { bytes: raw.toString("base64") }, + submatches: [{ match: { text: "needle" }, start: 1, end: 3 }], + }), + matchRecord("c.txt"), + ]), + ) + + expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt"), RelativePath.make("c.txt")]) + expect(lastSkip()?.skipped).toBe(1) + }) + stubTest("skips a record whose bytes field is empty rather than emitting an empty match", async () => { const matches = await Effect.runPromise( grepWithStubbedRecords([matchRecord("a.txt"), matchRecord("b.txt", { lines: { bytes: "" } })]), diff --git a/packages/opencode/src/file/ripgrep.ts b/packages/opencode/src/file/ripgrep.ts index 456993274e..da9d12cae4 100644 --- a/packages/opencode/src/file/ripgrep.ts +++ b/packages/opencode/src/file/ripgrep.ts @@ -147,12 +147,20 @@ export namespace Ripgrep { // An unaddressable offset marks the record corrupt so it is skipped and counted instead. let corrupt = false const rebase = (offset: unknown): unknown => { - if (!raw) return offset + if (!raw || !lines) return offset if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) { corrupt = true return offset } - return Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") + // Decoding the prefix alone differs from a prefix of the full decode when the offset splits a + // VALID multi-byte sequence, so require it to actually prefix the decoded line. See + // packages/core/src/ripgrep.ts for the worked example. + const prefix = raw.subarray(0, offset).toString("utf8") + if (!lines.text.startsWith(prefix)) { + corrupt = true + return offset + } + return Buffer.byteLength(prefix, "utf8") } const submatches = read(data, "submatches") // Only rewrite keys the record actually carries — `begin`/`end`/`summary` records reach here too From 8cb32e720cb685d12a28cf13ec4853321123c870 Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 20 Aug 2026 14:16:54 +0530 Subject: [PATCH 07/11] fix(core): drop unusable submatches instead of records; bound submatch text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third review round — one finding from cubic, two from the codex reviewer Ralph triggered on the PR. All three validated before acting. - The `startsWith` guard on rebased offsets had an aliasing blind spot. When an invalid byte precedes a LITERAL U+FFFD, an offset inside that character still produces a decoded prefix that prefixes the line, because the replacement characters are indistinguishable — `[ff ef bf bd]` accepts offset 2 and rebases it to 6. Replaced with byte-boundary validation, which has no such blind spot. Measured over 3.4M fuzzed offsets against the definition (decoding both halves must reconstruct the whole): zero unsafe offsets accepted. It errs only conservatively, on lines beginning mid-sequence. - An offset that cannot be rebased now drops ITS SUBMATCH rather than the whole record. The file, line and text are still correct and useful, and losing a highlight range beats losing the match — which is what this branch exists to stop. Applies to out-of-range and fractional offsets too. - Bound the retained submatch text. A broad pattern such as `x.*` makes ripgrep repeat nearly the whole line in `submatches[].match.text`, so capping only `lines.text` left the retained-memory bound defeated by the submatches instead. - `parseRecords` is namespace-private again. packages/opencode/AGENTS.md requires namespace-private helpers to be non-exported top-level declarations, and it had been exported solely so tests could reach it. The legacy tests now drive the public `search()` against a stub `rg` on PATH, which covers the same skip branches without widening the public API. Tests: 21 core, 8 legacy. The four core cases and one legacy case covering the new behaviour were confirmed to fail against the previous commit. Full core suite diffed against a freshly built origin/main worktree: 26 failures on both, none introduced. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/ripgrep.ts | 70 ++++---- packages/core/test/ripgrep.test.ts | 57 +++++-- packages/opencode/src/file/ripgrep.ts | 55 +++---- .../opencode/test/file/ripgrep-search.test.ts | 153 +++++++++--------- 4 files changed, 175 insertions(+), 160 deletions(-) diff --git a/packages/core/src/ripgrep.ts b/packages/core/src/ripgrep.ts index 29a95e3c9a..e170f9c4d9 100644 --- a/packages/core/src/ripgrep.ts +++ b/packages/core/src/ripgrep.ts @@ -130,32 +130,28 @@ const normalizeMatch = (json: object): unknown => { const lines = decodeField(readProp(data, "lines")) if (!lines) return json const raw = lines.raw - // Rebasing is a claim about coordinates, so it is only made for an offset actually addressable in - // the raw line. `Buffer.subarray` clamps an out-of-range end and truncates a fractional one rather - // than throwing, so an unchecked rebase would quietly repair a corrupt offset into a - // plausible-looking one — and the schema would not catch it, since `NonNegativeInt` happily - // accepts a number past the end of the line. An unaddressable offset therefore marks the whole - // record corrupt (undefined) so it is skipped and counted. Offsets on the `{text}` arm are left - // alone: no rebasing happens there, so no claim is made and ripgrep's values stand as before. - let corrupt = false - const rebase = (offset: unknown): unknown => { - if (!raw) return offset - if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) { - corrupt = true - return offset - } - // Decoding the prefix in isolation is not the same as taking a prefix of the full decode when - // the offset splits a VALID multi-byte sequence: `Buffer.from("éneedle")` sliced at byte 1 - // decodes to U+FFFD, so the offset would rebase to 3 and point at the wrong character in a line - // that decoded cleanly at that spot. Requiring the prefix to actually prefix the decoded line - // catches that; an offset that lands mid-character is not addressable and marks the record - // corrupt. This costs nothing asymptotically — decoding the prefix is already O(offset). - const prefix = raw.subarray(0, offset).toString("utf8") - if (!lines.text.startsWith(prefix)) { - corrupt = true - return offset - } - return Buffer.byteLength(prefix, "utf8") + // Submatch `start`/`end` are BYTE offsets into the RAW line, so a lossy decode invalidates them: + // every undecodable byte widens to a 3-byte U+FFFD. They are rebased onto the decoded text's own + // UTF-8 encoding, which keeps the established byte-offset contract instead of switching these + // records to a different unit. + // + // A rebase is only sound when the split point is a character boundary of the whole-buffer decode. + // Testing that by decoded-string comparison is NOT enough: for a raw line where an invalid byte + // precedes a literal U+FFFD, an offset inside that literal character still produces a decoded + // prefix that prefixes the line, because the replacement characters alias. Byte-boundary + // validation has no such blind spot — a continuation byte (0b10xxxxxx) at the offset means the + // split lands inside a sequence. Measured over 3.4M fuzzed offsets against the definition + // (decoding both halves must reconstruct the whole) this never accepts an unsafe offset; it only + // errs conservatively, on lines that begin mid-sequence. + // + // An offset that cannot be rebased drops ITS SUBMATCH, not the record: the file, line and text + // are still correct and useful, and this whole change exists to stop losing matches. Offsets on + // the `{text}` arm are untouched — no rebasing happens there, so no claim is made. + const isContinuationByte = (byte: number | undefined) => byte !== undefined && (byte & 0xc0) === 0x80 + const rebase = (offset: unknown): number | undefined => { + if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw!.length) return undefined + if (offset !== 0 && offset !== raw!.length && isContinuationByte(raw![offset])) return undefined + return Buffer.byteLength(raw!.subarray(0, offset).toString("utf8"), "utf8") } const submatches = readProp(data, "submatches") const normalized = { @@ -167,21 +163,23 @@ const normalizeMatch = (json: object): unknown => { lines: { text: capLineText(lines.text) }, // Sliced BEFORE decoding so a pathological submatch count is not decoded only to be dropped. submatches: Array.isArray(submatches) - ? submatches.slice(0, MAX_SUBMATCHES).map((submatch) => { - if (!submatch || typeof submatch !== "object") return submatch + ? submatches.slice(0, MAX_SUBMATCHES).flatMap((submatch) => { + if (!submatch || typeof submatch !== "object") return [submatch] const match = decodeField(readProp(submatch, "match")) - if (!match) return submatch - return { - ...submatch, - match: { text: match.text }, - start: rebase(readProp(submatch, "start")), - end: rebase(readProp(submatch, "end")), - } + if (!match) return [submatch] + // A broad pattern such as `x.*` makes ripgrep repeat almost the whole line here, so the + // matched text needs the same bound as the line or the retained-memory cap is defeated + // by the submatches instead. + const decoded = { ...submatch, match: { text: capLineText(match.text) } } + if (!raw) return [decoded] + const start = rebase(readProp(submatch, "start")) + const end = rebase(readProp(submatch, "end")) + return start === undefined || end === undefined ? [] : [{ ...decoded, start, end }] }) : submatches, }, } - return corrupt ? undefined : normalized + return normalized } // altimate_change end diff --git a/packages/core/test/ripgrep.test.ts b/packages/core/test/ripgrep.test.ts index 3cbb4d35a4..9b70df8beb 100644 --- a/packages/core/test/ripgrep.test.ts +++ b/packages/core/test/ripgrep.test.ts @@ -312,42 +312,73 @@ describe("Ripgrep", () => { // `Buffer.subarray` clamps an out-of-range end and truncates a fractional one instead of throwing, // so rebasing without a range check would turn a corrupt offset into a plausible-looking one. - stubTest("skips a record whose submatch offset is not addressable in the line", async () => { + // The MATCH still survives — only the coordinate we cannot express is dropped. + stubTest("drops a submatch whose offset is not addressable, keeping the match", async () => { const raw = Buffer.concat([Buffer.from([0xff]), Buffer.from("needle tail\n")]) const matches = await Effect.runPromise( grepWithStubbedRecords([ - matchRecord("a.txt"), matchRecord("b.txt", { lines: { bytes: raw.toString("base64") }, // Well past the end of the raw line — nonsense that must not be silently clamped. submatches: [{ match: { text: "needle" }, start: 1, end: 9_999 }], }), - matchRecord("c.txt"), ]), ) - expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt"), RelativePath.make("c.txt")]) - expect(lastSkip()?.skipped).toBe(1) + expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("b.txt")]) + expect(matches[0].submatches).toEqual([]) + expect(matches[0].text).toContain("needle") }) - // A byte-mode pattern can match inside a valid multi-byte character. Decoding the prefix alone - // then yields U+FFFD where the full decode has "é", so the offset would rebase to a plausible but - // wrong position (3, landing mid-word) instead of being recognised as unaddressable. - stubTest("skips a record whose submatch offset splits a multi-byte character", async () => { + // A byte-mode pattern can match inside a valid multi-byte character, where the offset is simply not + // expressible in the decoded line. Byte-boundary validation catches it; a decoded-string + // comparison does not, because an invalid byte followed by a LITERAL U+FFFD aliases — both + // prefixes decode to the same replacement text. + stubTest("drops a submatch whose offset splits a multi-byte character", async () => { const raw = Buffer.concat([Buffer.from("é"), Buffer.from("needle")]) const matches = await Effect.runPromise( grepWithStubbedRecords([ - matchRecord("a.txt"), matchRecord("b.txt", { lines: { bytes: raw.toString("base64") }, submatches: [{ match: { text: "needle" }, start: 1, end: 3 }], }), - matchRecord("c.txt"), ]), ) - expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt"), RelativePath.make("c.txt")]) - expect(lastSkip()?.skipped).toBe(1) + expect(matches[0].submatches).toEqual([]) + }) + + stubTest("drops a submatch whose offset sits inside a literal replacement character", async () => { + // Invalid byte, then the three bytes that spell U+FFFD. Offset 2 is inside that literal + // character, but both decoded prefixes read as the same replacement text. + const raw = Buffer.from([0xff, 0xef, 0xbf, 0xbd]) + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("b.txt", { + lines: { bytes: raw.toString("base64") }, + submatches: [{ match: { text: "x" }, start: 2, end: 4 }], + }), + ]), + ) + + expect(matches[0].submatches).toEqual([]) + }) + + // A broad pattern such as `x.*` makes ripgrep repeat nearly the whole line in the submatch text, + // which would defeat the retained-memory bound that capping `lines.text` establishes. + stubTest("caps the retained submatch text, not just the line", async () => { + const huge = "n".repeat(50_000) + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("a.txt", { + lines: { text: huge }, + submatches: [{ match: { text: huge }, start: 0, end: huge.length }], + }), + ]), + ) + + expect(matches[0].submatches[0].text).toHaveLength(2_003) + expect(matches[0].submatches[0].text.endsWith("...")).toBe(true) }) stubTest("skips a record whose bytes field is empty rather than emitting an empty match", async () => { diff --git a/packages/opencode/src/file/ripgrep.ts b/packages/opencode/src/file/ripgrep.ts index da9d12cae4..223dd674f4 100644 --- a/packages/opencode/src/file/ripgrep.ts +++ b/packages/opencode/src/file/ripgrep.ts @@ -141,26 +141,18 @@ export namespace Ripgrep { // `/find` route, so unrebased offsets would be newly wrong output rather than a skipped record. // Mirrors packages/core/src/ripgrep.ts. const raw = lines?.raw - // Only rebase an offset addressable in the raw line: `Buffer.subarray` clamps an out-of-range - // end and truncates a fractional one rather than throwing, so an unchecked rebase would quietly - // repair a corrupt offset, and the schema would not catch it (`z.number()` accepts any number). - // An unaddressable offset marks the record corrupt so it is skipped and counted instead. - let corrupt = false - const rebase = (offset: unknown): unknown => { - if (!raw || !lines) return offset - if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) { - corrupt = true - return offset - } - // Decoding the prefix alone differs from a prefix of the full decode when the offset splits a - // VALID multi-byte sequence, so require it to actually prefix the decoded line. See - // packages/core/src/ripgrep.ts for the worked example. - const prefix = raw.subarray(0, offset).toString("utf8") - if (!lines.text.startsWith(prefix)) { - corrupt = true - return offset - } - return Buffer.byteLength(prefix, "utf8") + // Byte-boundary validation, matching packages/core/src/ripgrep.ts. A decoded-string comparison + // is not sufficient: when an invalid byte precedes a LITERAL U+FFFD, an offset inside that + // character still yields a prefix that prefixes the line, because the replacement characters + // alias. A continuation byte (0b10xxxxxx) at the offset means the split lands inside a sequence. + // An offset that cannot be rebased drops ITS SUBMATCH, not the record — the file, line and text + // stay correct, and losing a highlight range beats losing the match. + const isContinuationByte = (byte: number | undefined) => byte !== undefined && (byte & 0xc0) === 0x80 + const rebase = (offset: unknown): number | undefined => { + if (!raw) return typeof offset === "number" ? offset : undefined + if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) return undefined + if (offset !== 0 && offset !== raw.length && isContinuationByte(raw[offset])) return undefined + return Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") } const submatches = read(data, "submatches") // Only rewrite keys the record actually carries — `begin`/`end`/`summary` records reach here too @@ -175,22 +167,20 @@ export namespace Ripgrep { ...(lines ? { lines: { text: lines.text } } : {}), ...(Array.isArray(submatches) ? { - submatches: submatches.map((submatch) => { - if (!submatch || typeof submatch !== "object") return submatch + submatches: submatches.flatMap((submatch) => { + if (!submatch || typeof submatch !== "object") return [submatch] const match = decode(read(submatch, "match")) - if (!match) return submatch - return { - ...submatch, - match: { text: match.text }, - start: rebase(read(submatch, "start")), - end: rebase(read(submatch, "end")), - } + if (!match) return [submatch] + const start = rebase(read(submatch, "start")) + const end = rebase(read(submatch, "end")) + if (start === undefined || end === undefined) return [] + return [{ ...submatch, match: { text: match.text }, start, end }] }), } : {}), }, } - return corrupt ? undefined : normalized + return normalized } /** @@ -199,9 +189,10 @@ export namespace Ripgrep { * `JSON.parse` + a strict `Result.parse` on every line meant one unusable record threw out of * `search()` and discarded every match already collected from unrelated files — the same defect * fixed in packages/core/src/ripgrep.ts. Records are independent, so a bad one is dropped and - * counted. Exported so the skip paths are testable without a stub ripgrep binary. + * counted. Namespace-private per packages/opencode/AGENTS.md: the skip behaviour is covered + * through the public `search()` boundary instead of exporting an implementation detail. */ - export function parseRecords(lines: string[]): Match["data"][] { + function parseRecords(lines: string[]): Match["data"][] { const matches: Match["data"][] = [] let skipped = 0 for (const line of lines) { diff --git a/packages/opencode/test/file/ripgrep-search.test.ts b/packages/opencode/test/file/ripgrep-search.test.ts index 765832fad2..d10b84b857 100644 --- a/packages/opencode/test/file/ripgrep-search.test.ts +++ b/packages/opencode/test/file/ripgrep-search.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, test } from "bun:test" +import { afterAll, beforeAll, describe, expect, test } from "bun:test" import fs from "fs/promises" import os from "os" import path from "path" @@ -9,85 +9,78 @@ import { Ripgrep } from "../../src/file/ripgrep" // threw out of the whole call and discarded every match already collected from unrelated files — // the same defect fixed in packages/core/src/ripgrep.ts. This path is reachable from the mounted // `/find` route (server/routes/file.ts), so it needs its own coverage. -const withRepo = async (run: (dir: string) => Promise) => { - const dir = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), "legacy-rg-"))) - try { - await run(dir) - } finally { - await fs.rm(dir, { recursive: true, force: true }) - } -} +// +// The cases drive the real `search()` — the parser is namespace-private per AGENTS.md — against a +// stub `rg` on PATH that prints exactly the NDJSON given. `which("rg")` resolves from PATH and the +// result is memoised, so PATH is set once before any search runs. The stub is a POSIX shell script, +// so this file is POSIX-only; the parser itself is platform-independent. +const posixTest = test.skipIf(process.platform === "win32") -describe("legacy Ripgrep.search", () => { - // Explicit timeout: this drives the real binary, and `search()` resolves it through `state()`, - // which downloads a release archive when `rg` is absent from PATH and from Global.Path.bin. - test( - "returns matches from a file whose matched line is not valid UTF-8", - () => - withRepo(async (dir) => { - await fs.writeFile(path.join(dir, "a-plain.txt"), "needle here\n") - // ripgrep emits `{"bytes": ""}` rather than `{"text": ...}` for this line, which the - // strict Zod schema rejected — taking the unrelated matches down with it. - await fs.writeFile(path.join(dir, "b-binary.txt"), Buffer.from("needle \xff\xfe tail\n", "binary")) - await fs.writeFile(path.join(dir, "c-plain.txt"), "needle here\n") - - const matches = await Ripgrep.search({ cwd: dir, pattern: "needle", limit: 10 }) - - expect(matches.map((match) => match.path.text.replace(/^\.\//, "")).sort()).toEqual([ - "a-plain.txt", - "b-binary.txt", - "c-plain.txt", - ]) - const binary = matches.find((match) => match.path.text.includes("b-binary.txt")) - expect(binary?.lines.text).toContain("needle") - expect(binary?.lines.text).toContain("tail") - }), - 120_000, - ) +let dir: string +let dataFile: string +let originalPath: string | undefined + +beforeAll(async () => { + dir = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), "legacy-rg-"))) + dataFile = path.join(dir, "records.jsonl") + const bin = path.join(dir, "bin") + await fs.mkdir(bin) + const stub = path.join(bin, "rg") + await fs.writeFile(stub, `#!/bin/sh\ncat ${JSON.stringify(dataFile)}\n`) + await fs.chmod(stub, 0o755) + originalPath = process.env.PATH + process.env.PATH = `${bin}${path.delimiter}${originalPath ?? ""}` }) -// `parseRecords` is exercised directly so the skip branches this PR adds — the `JSON.parse` catch, -// the size ceiling, the counter — are covered without depending on what the installed ripgrep build -// happens to emit. -describe("legacy Ripgrep.parseRecords", () => { - const record = (file: string, overrides: Record = {}) => - JSON.stringify({ - type: "match", - data: { - path: { text: `./${file}` }, - lines: { text: "needle\n" }, - line_number: 1, - absolute_offset: 0, - submatches: [{ match: { text: "needle" }, start: 0, end: 6 }], - ...overrides, - }, - }) - const paths = (records: string[]) => Ripgrep.parseRecords(records).map((match) => match.path.text) - - test("skips an unparseable record and keeps the ones around it", () => { - expect(paths([record("a.txt"), '{"type":"match","data":{"path":{"text":"./b.t', record("c.txt")])).toEqual([ +afterAll(async () => { + process.env.PATH = originalPath + await fs.rm(dir, { recursive: true, force: true }) +}) + +const record = (file: string, overrides: Record = {}) => + JSON.stringify({ + type: "match", + data: { + path: { text: `./${file}` }, + lines: { text: "needle\n" }, + line_number: 1, + absolute_offset: 0, + submatches: [{ match: { text: "needle" }, start: 0, end: 6 }], + ...overrides, + }, + }) + +const search = async (records: string[]) => { + await fs.writeFile(dataFile, records.join("\n") + "\n") + return Ripgrep.search({ cwd: dir, pattern: "needle", limit: 100 }) +} +const paths = async (records: string[]) => (await search(records)).map((match) => match.path.text) + +describe("legacy Ripgrep.search", () => { + posixTest("skips an unparseable record and keeps the ones around it", async () => { + expect(await paths([record("a.txt"), '{"type":"match","data":{"path":{"text":"./b.t', record("c.txt")])).toEqual([ "./a.txt", "./c.txt", ]) }) - test("skips a record past the size ceiling", () => { + posixTest("skips a record past the size ceiling", async () => { const huge = record("b.txt", { lines: { text: "n".repeat(17 * 1024 * 1024) } }) - expect(paths([record("a.txt"), huge, record("c.txt")])).toEqual(["./a.txt", "./c.txt"]) + expect(await paths([record("a.txt"), huge, record("c.txt")])).toEqual(["./a.txt", "./c.txt"]) }) - test("skips a record whose path is not valid UTF-8, rather than mangling the path", () => { + posixTest("skips a record whose path is not valid UTF-8, rather than mangling the path", async () => { const bad = record("ignored", { path: { bytes: Buffer.from("./b\xff.txt", "binary").toString("base64") } }) - expect(paths([record("a.txt"), bad])).toEqual(["./a.txt"]) + expect(await paths([record("a.txt"), bad])).toEqual(["./a.txt"]) }) - test("skips empty and non-canonical base64 rather than emitting an empty match", () => { - expect(paths([record("a.txt"), record("b.txt", { lines: { bytes: "" } })])).toEqual(["./a.txt"]) - expect(paths([record("a.txt"), record("b.txt", { lines: { bytes: "Zh==" } })])).toEqual(["./a.txt"]) + posixTest("skips empty and non-canonical base64 rather than emitting an empty match", async () => { + expect(await paths([record("a.txt"), record("b.txt", { lines: { bytes: "" } })])).toEqual(["./a.txt"]) + expect(await paths([record("a.txt"), record("b.txt", { lines: { bytes: "Zh==" } })])).toEqual(["./a.txt"]) }) - test("decodes a non-UTF8 line and ignores control records", () => { - const parsed = Ripgrep.parseRecords([ + posixTest("decodes a non-UTF8 line and ignores control records", async () => { + const parsed = await search([ JSON.stringify({ type: "begin", data: { path: { text: "./a.txt" } } }), record("a.txt", { lines: { bytes: Buffer.from("needle \xff tail\n", "binary").toString("base64") } }), ]) @@ -98,9 +91,9 @@ describe("legacy Ripgrep.parseRecords", () => { // Offsets are byte offsets into the RAW line; a lossy decode widens each undecodable byte to a // 3-byte U+FFFD. This response shape is published by the `/find` route, so leaving them unrebased // would be newly wrong output rather than a skipped record. Mirrors the core parser. - test("rebases submatch offsets after a lossy line decode", () => { + posixTest("rebases submatch offsets after a lossy line decode", async () => { const raw = Buffer.concat([Buffer.from([0xff]), Buffer.from("needle tail\n")]) - const parsed = Ripgrep.parseRecords([ + const parsed = await search([ record("a.txt", { lines: { bytes: raw.toString("base64") }, submatches: [{ match: { bytes: Buffer.from("needle").toString("base64") }, start: 1, end: 7 }], @@ -108,24 +101,26 @@ describe("legacy Ripgrep.parseRecords", () => { ]) expect(parsed).toHaveLength(1) - const [{ lines, submatches }] = parsed - expect(submatches[0]).toEqual({ match: { text: "needle" }, start: 3, end: 9 }) - expect(Buffer.from(lines.text, "utf8").subarray(3, 9).toString("utf8")).toBe("needle") + expect(parsed[0].submatches[0]).toEqual({ match: { text: "needle" }, start: 3, end: 9 }) + expect(Buffer.from(parsed[0].lines.text, "utf8").subarray(3, 9).toString("utf8")).toBe("needle") }) - test("skips a record whose submatch offset is not addressable in the line", () => { + // An offset that cannot be expressed in the decoded line drops its submatch, not the match. + posixTest("drops a submatch whose offset is unaddressable, keeping the match", async () => { const raw = Buffer.concat([Buffer.from([0xff]), Buffer.from("needle tail\n")]) - const bad = record("b.txt", { - lines: { bytes: raw.toString("base64") }, - submatches: [{ match: { text: "needle" }, start: 1, end: 9_999 }], - }) - // `z.number()` would accept 9999 happily, so the range check is what rejects this. - expect(paths([record("a.txt"), bad, record("c.txt")])).toEqual(["./a.txt", "./c.txt"]) + const parsed = await search([ + record("a.txt", { + lines: { bytes: raw.toString("base64") }, + submatches: [{ match: { text: "needle" }, start: 1, end: 9_999 }], + }), + ]) + + expect(parsed).toHaveLength(1) + expect(parsed[0].submatches).toEqual([]) }) - test("returns an empty array when every record is unusable, rather than throwing", () => { - expect(() => Ripgrep.parseRecords(["{oops", "{also oops"])).not.toThrow() - expect(Ripgrep.parseRecords(["{oops", "{also oops"])).toEqual([]) + posixTest("returns an empty array when every record is unusable, rather than throwing", async () => { + expect(await search(["{oops", "{also oops"])).toEqual([]) }) }) // altimate_change end From 953999c57c5a27c067cef38803b1c4f59703b50c Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 20 Aug 2026 14:48:00 +0530 Subject: [PATCH 08/11] fix(core): inverted ranges, legacy text cap, and a test harness that leaked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth review round — CodeRabbit, cubic and Kilo on the previous push. - Reject inverted submatch ranges in both parsers. Endpoints are rebased independently, so `start > end` survived both endpoint checks and was returned as a coordinate pair no consumer can use. - Cap the text the legacy `/find` path retains, matching the core parser. `MAX_RECORD_BYTES` bounds one INPUT record; it does not bound the response. This path buffers all of stdout and returns every match, so without a per-field cap a tree of large records still retains — and serialises — an unbounded amount of text. The earlier reasoning for leaving the shape alone ("it is the published contract") does not survive that: an unbounded response is not a contract worth preserving. - Move record parsing to packages/opencode/src/file/ripgrep-records.ts. Kilo caught that the previous commit's test harness was actively harmful, and it reproduces: `bun test test/file/ripgrep-search.test.ts test/tool/glob.test.ts` failed `tool.glob > matches files from a directory path`. The legacy binary lookup is `lazy()`-memoised per process and `bun test` shares one process, so a stub `rg` on PATH leaked into every later test file in the run. That harness only existed because the parser was namespace-private and could not be reached directly. Splitting it into a module resolves both constraints at once: nothing is exported through the `Ripgrep` namespace (AGENTS.md), and the tests call a pure function with no process state, no stub and no ordering hazard. `Ripgrep.Match` is re-exported from the new module, since server/routes/file.ts builds the `/find` response schema from it. Tests: 22 core, 10 legacy. The previously failing combination now passes 12/12. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/ripgrep.ts | 4 +- packages/core/test/ripgrep.test.ts | 14 ++ packages/opencode/src/file/ripgrep-records.ts | 224 ++++++++++++++++++ packages/opencode/src/file/ripgrep.ts | 205 +--------------- .../test/file/ripgrep-records.test.ts | 125 ++++++++++ .../opencode/test/file/ripgrep-search.test.ts | 126 ---------- 6 files changed, 378 insertions(+), 320 deletions(-) create mode 100644 packages/opencode/src/file/ripgrep-records.ts create mode 100644 packages/opencode/test/file/ripgrep-records.test.ts delete mode 100644 packages/opencode/test/file/ripgrep-search.test.ts diff --git a/packages/core/src/ripgrep.ts b/packages/core/src/ripgrep.ts index e170f9c4d9..9755d61563 100644 --- a/packages/core/src/ripgrep.ts +++ b/packages/core/src/ripgrep.ts @@ -174,7 +174,9 @@ const normalizeMatch = (json: object): unknown => { if (!raw) return [decoded] const start = rebase(readProp(submatch, "start")) const end = rebase(readProp(submatch, "end")) - return start === undefined || end === undefined ? [] : [{ ...decoded, start, end }] + // Endpoints are rebased independently, so ordering is checked explicitly: an inverted + // range is not a usable coordinate pair even when both endpoints are addressable. + return start === undefined || end === undefined || start > end ? [] : [{ ...decoded, start, end }] }) : submatches, }, diff --git a/packages/core/test/ripgrep.test.ts b/packages/core/test/ripgrep.test.ts index 9b70df8beb..92647506ff 100644 --- a/packages/core/test/ripgrep.test.ts +++ b/packages/core/test/ripgrep.test.ts @@ -348,6 +348,20 @@ describe("Ripgrep", () => { expect(matches[0].submatches).toEqual([]) }) + // Endpoints are rebased independently, so an inverted range can survive both endpoint checks. + stubTest("drops a submatch whose range is inverted", async () => { + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("a.txt", { + lines: { bytes: Buffer.from("needle tail\n").toString("base64") }, + submatches: [{ match: { text: "needle" }, start: 6, end: 2 }], + }), + ]), + ) + + expect(matches[0].submatches).toEqual([]) + }) + stubTest("drops a submatch whose offset sits inside a literal replacement character", async () => { // Invalid byte, then the three bytes that spell U+FFFD. Offset 2 is inside that literal // character, but both decoded prefixes read as the same replacement text. diff --git a/packages/opencode/src/file/ripgrep-records.ts b/packages/opencode/src/file/ripgrep-records.ts new file mode 100644 index 0000000000..3512b9a677 --- /dev/null +++ b/packages/opencode/src/file/ripgrep-records.ts @@ -0,0 +1,224 @@ +// altimate_change start — upstream_fix: ripgrep NDJSON record parsing, split out of ripgrep.ts. +// +// This lives in its own module rather than inside the `Ripgrep` namespace because it is pure, +// process-free logic that deserves direct tests. Keeping it in the namespace forced a choice +// between two bad options: exporting an implementation detail through `export * as` (which +// packages/opencode/AGENTS.md prohibits), or driving it through `search()` with a stub `rg` on +// PATH — and the binary lookup there is memoised per process, so such a stub leaks into every +// later test file in the same `bun test` run and breaks unrelated suites. +// +// It mirrors packages/core/src/ripgrep.ts, but the two are not identical by design. The core +// parser streams; this one buffers all of stdout before splitting, and hands its records straight +// to the `/find` response. Both cap retained text, skip records they cannot use, and report the +// skips once per search rather than once per record. +import z from "zod" +import { Log } from "@/util/log" + +export namespace RipgrepRecords { + const log = Log.create({ service: "ripgrep" }) + + const Stats = z.object({ + elapsed: z.object({ + secs: z.number(), + nanos: z.number(), + human: z.string(), + }), + searches: z.number(), + searches_with_match: z.number(), + bytes_searched: z.number(), + bytes_printed: z.number(), + matched_lines: z.number(), + matches: z.number(), + }) + + const Begin = z.object({ + type: z.literal("begin"), + data: z.object({ + path: z.object({ + text: z.string(), + }), + }), + }) + + export const Match = z.object({ + type: z.literal("match"), + data: z.object({ + path: z.object({ + text: z.string(), + }), + lines: z.object({ + text: z.string(), + }), + line_number: z.number(), + absolute_offset: z.number(), + submatches: z.array( + z.object({ + match: z.object({ + text: z.string(), + }), + start: z.number(), + end: z.number(), + }), + ), + }), + }) + + const End = z.object({ + type: z.literal("end"), + data: z.object({ + path: z.object({ + text: z.string(), + }), + binary_offset: z.number().nullable(), + stats: Stats, + }), + }) + + const Summary = z.object({ + type: z.literal("summary"), + data: z.object({ + elapsed_total: z.object({ + human: z.string(), + nanos: z.number(), + secs: z.number(), + }), + stats: Stats, + }), + }) + + const Result = z.union([Begin, Match, End, Summary]) + + // altimate_change start — upstream_fix: tolerate ripgrep's `{bytes}` arm and malformed lines. + // + // This mirrors packages/core/src/ripgrep.ts, but deliberately not in every respect. That parser + // streams, so it caps the retained line text and rebases submatch offsets; this one buffers all of + // stdout up front and hands its records straight to the `/find` response, where the raw ripgrep + // shape is the published contract — so it normalises and skips, and leaves the shape alone. Both + // report skipped records once per search rather than once per record. + const BASE64 = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/ + + /** Mirrors packages/core/src/ripgrep.ts. Bounds parse cost per record on this path too. */ + const MAX_RECORD_BYTES = 16 * 1024 * 1024 + + // `MAX_RECORD_BYTES` bounds ONE input record; it does not bound what the response retains. This + // path buffers all of stdout and returns every match, so without a per-field cap a tree of large + // records still retains — and serialises into the `/find` response — an unbounded amount of text. + // Same cap and elision marker as the core parser, so the two paths agree on what a match shows. + const LINE_TEXT_CAP = 2_000 + const capText = (text: string) => (text.length > LINE_TEXT_CAP ? text.slice(0, LINE_TEXT_CAP) + "..." : text) + + /** Parse one NDJSON record, rewriting `{bytes: base64}` fields into the `{text}` arm. */ + const normalizeRecord = (line: string): unknown => { + let json: unknown + try { + json = JSON.parse(line) + } catch { + return undefined + } + if (!json || typeof json !== "object") return json + const read = (value: unknown, key: string): unknown => + value !== null && typeof value === "object" && key in value ? Reflect.get(value, key) : undefined + const data = read(json, "data") + if (!data || typeof data !== "object") return json + /** Decode a `{text}`/`{bytes}` field, returning the raw buffer so offsets can be rebased. */ + const decode = (value: unknown): { text: string; raw?: Buffer } | undefined => { + if (!value || typeof value !== "object") return undefined + const text = read(value, "text") + if (typeof text === "string") return { text } + const bytes = read(value, "bytes") + // Guarded three ways because `Buffer.from` decodes unconvertible input to an EMPTY buffer + // instead of throwing, which would turn a corrupt record into a schema-valid empty match: + // reject the empty string (a matched line is never empty), check the spelling, then require + // a round-trip so non-canonical padding ("Zh==" and "Zg==" both decode to "f") is rejected. + if (typeof bytes !== "string" || bytes.length === 0 || !BASE64.test(bytes)) return undefined + const decoded = Buffer.from(bytes, "base64") + if (decoded.toString("base64") !== bytes) return undefined + return { text: decoded.toString("utf8"), raw: decoded } + } + const lines = "lines" in data ? decode(read(data, "lines")) : undefined + // Submatch offsets are BYTE offsets into the RAW line, and a lossy decode widens every + // undecodable byte to a 3-byte U+FFFD — so they must be rebased onto the decoded text's own + // UTF-8 encoding or they no longer locate the match. This response shape is published by the + // `/find` route, so unrebased offsets would be newly wrong output rather than a skipped record. + // Mirrors packages/core/src/ripgrep.ts. + const raw = lines?.raw + // Byte-boundary validation, matching packages/core/src/ripgrep.ts. A decoded-string comparison + // is not sufficient: when an invalid byte precedes a LITERAL U+FFFD, an offset inside that + // character still yields a prefix that prefixes the line, because the replacement characters + // alias. A continuation byte (0b10xxxxxx) at the offset means the split lands inside a sequence. + // An offset that cannot be rebased drops ITS SUBMATCH, not the record — the file, line and text + // stay correct, and losing a highlight range beats losing the match. + const isContinuationByte = (byte: number | undefined) => byte !== undefined && (byte & 0xc0) === 0x80 + const rebase = (offset: unknown): number | undefined => { + if (!raw) return typeof offset === "number" ? offset : undefined + if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) return undefined + if (offset !== 0 && offset !== raw.length && isContinuationByte(raw[offset])) return undefined + return Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") + } + const submatches = read(data, "submatches") + // Only rewrite keys the record actually carries — `begin`/`end`/`summary` records reach here too + // and must keep their exact shape, or the strict union below would reject them. + // `path` is deliberately left alone: decoding it is lossy, and a path is an identifier the + // caller reopens, so a U+FFFD-mangled path names a file that does not exist. Such a record + // stays in the `{bytes}` arm and is skipped. See packages/core/src/ripgrep.ts. + const normalized = { + ...json, + data: { + ...data, + ...(lines ? { lines: { text: capText(lines.text) } } : {}), + ...(Array.isArray(submatches) + ? { + submatches: submatches.flatMap((submatch) => { + if (!submatch || typeof submatch !== "object") return [submatch] + const match = decode(read(submatch, "match")) + if (!match) return [submatch] + const start = rebase(read(submatch, "start")) + const end = rebase(read(submatch, "end")) + // Endpoints are rebased independently, so ordering is checked explicitly. + if (start === undefined || end === undefined || start > end) return [] + return [{ ...submatch, match: { text: capText(match.text) }, start, end }] + }), + } + : {}), + }, + } + return normalized + } + + /** + * Turn ripgrep NDJSON lines into match data, skipping records that cannot be used. + * + * `JSON.parse` + a strict `Result.parse` on every line meant one unusable record threw out of + * `search()` and discarded every match already collected from unrelated files — the same defect + * fixed in packages/core/src/ripgrep.ts. Records are independent, so a bad one is dropped and + * counted. Namespace-private per packages/opencode/AGENTS.md: the skip behaviour is covered + * through the public `search()` boundary instead of exporting an implementation detail. + */ + export function parseRecords(lines: string[]): Match["data"][] { + const matches: Match["data"][] = [] + let skipped = 0 + for (const line of lines) { + // Bounds parse cost per record. This path buffers all of stdout before splitting, so it does + // not bound total memory — that needs streaming, tracked separately. + const parsed = + Buffer.byteLength(line, "utf8") > MAX_RECORD_BYTES ? undefined : Result.safeParse(normalizeRecord(line)) + if (!parsed?.success) { + skipped++ + continue + } + if (parsed.data.type === "match") matches.push(parsed.data.data) + } + // Counted and reported once rather than per record: without this a ripgrep protocol change + // would make `/find` answer `[]`, which is indistinguishable from an honest "no matches". + if (skipped > 0) log.warn("skipped unusable ripgrep records", { skipped, total: lines.length }) + return matches + } + // altimate_change end + + export type Result = z.infer + export type Match = z.infer + export type Begin = z.infer + export type End = z.infer + export type Summary = z.infer +} +// altimate_change end diff --git a/packages/opencode/src/file/ripgrep.ts b/packages/opencode/src/file/ripgrep.ts index 223dd674f4..353c987acc 100644 --- a/packages/opencode/src/file/ripgrep.ts +++ b/packages/opencode/src/file/ripgrep.ts @@ -20,204 +20,23 @@ import { text } from "node:stream/consumers" import { ZipReader, BlobReader, BlobWriter } from "@zip.js/zip.js" import { Log } from "@/util/log" +import { RipgrepRecords } from "./ripgrep-records" export namespace Ripgrep { const log = Log.create({ service: "ripgrep" }) - const Stats = z.object({ - elapsed: z.object({ - secs: z.number(), - nanos: z.number(), - human: z.string(), - }), - searches: z.number(), - searches_with_match: z.number(), - bytes_searched: z.number(), - bytes_printed: z.number(), - matched_lines: z.number(), - matches: z.number(), - }) - - const Begin = z.object({ - type: z.literal("begin"), - data: z.object({ - path: z.object({ - text: z.string(), - }), - }), - }) - - export const Match = z.object({ - type: z.literal("match"), - data: z.object({ - path: z.object({ - text: z.string(), - }), - lines: z.object({ - text: z.string(), - }), - line_number: z.number(), - absolute_offset: z.number(), - submatches: z.array( - z.object({ - match: z.object({ - text: z.string(), - }), - start: z.number(), - end: z.number(), - }), - ), - }), - }) - - const End = z.object({ - type: z.literal("end"), - data: z.object({ - path: z.object({ - text: z.string(), - }), - binary_offset: z.number().nullable(), - stats: Stats, - }), - }) - - const Summary = z.object({ - type: z.literal("summary"), - data: z.object({ - elapsed_total: z.object({ - human: z.string(), - nanos: z.number(), - secs: z.number(), - }), - stats: Stats, - }), - }) - - const Result = z.union([Begin, Match, End, Summary]) - - // altimate_change start — upstream_fix: tolerate ripgrep's `{bytes}` arm and malformed lines. - // - // This mirrors packages/core/src/ripgrep.ts, but deliberately not in every respect. That parser - // streams, so it caps the retained line text and rebases submatch offsets; this one buffers all of - // stdout up front and hands its records straight to the `/find` response, where the raw ripgrep - // shape is the published contract — so it normalises and skips, and leaves the shape alone. Both - // report skipped records once per search rather than once per record. - const BASE64 = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/ - - /** Mirrors packages/core/src/ripgrep.ts. Bounds parse cost per record on this path too. */ - const MAX_RECORD_BYTES = 16 * 1024 * 1024 - - /** Parse one NDJSON record, rewriting `{bytes: base64}` fields into the `{text}` arm. */ - const normalizeRecord = (line: string): unknown => { - let json: unknown - try { - json = JSON.parse(line) - } catch { - return undefined - } - if (!json || typeof json !== "object") return json - const read = (value: unknown, key: string): unknown => - value !== null && typeof value === "object" && key in value ? Reflect.get(value, key) : undefined - const data = read(json, "data") - if (!data || typeof data !== "object") return json - /** Decode a `{text}`/`{bytes}` field, returning the raw buffer so offsets can be rebased. */ - const decode = (value: unknown): { text: string; raw?: Buffer } | undefined => { - if (!value || typeof value !== "object") return undefined - const text = read(value, "text") - if (typeof text === "string") return { text } - const bytes = read(value, "bytes") - // Guarded three ways because `Buffer.from` decodes unconvertible input to an EMPTY buffer - // instead of throwing, which would turn a corrupt record into a schema-valid empty match: - // reject the empty string (a matched line is never empty), check the spelling, then require - // a round-trip so non-canonical padding ("Zh==" and "Zg==" both decode to "f") is rejected. - if (typeof bytes !== "string" || bytes.length === 0 || !BASE64.test(bytes)) return undefined - const decoded = Buffer.from(bytes, "base64") - if (decoded.toString("base64") !== bytes) return undefined - return { text: decoded.toString("utf8"), raw: decoded } - } - const lines = "lines" in data ? decode(read(data, "lines")) : undefined - // Submatch offsets are BYTE offsets into the RAW line, and a lossy decode widens every - // undecodable byte to a 3-byte U+FFFD — so they must be rebased onto the decoded text's own - // UTF-8 encoding or they no longer locate the match. This response shape is published by the - // `/find` route, so unrebased offsets would be newly wrong output rather than a skipped record. - // Mirrors packages/core/src/ripgrep.ts. - const raw = lines?.raw - // Byte-boundary validation, matching packages/core/src/ripgrep.ts. A decoded-string comparison - // is not sufficient: when an invalid byte precedes a LITERAL U+FFFD, an offset inside that - // character still yields a prefix that prefixes the line, because the replacement characters - // alias. A continuation byte (0b10xxxxxx) at the offset means the split lands inside a sequence. - // An offset that cannot be rebased drops ITS SUBMATCH, not the record — the file, line and text - // stay correct, and losing a highlight range beats losing the match. - const isContinuationByte = (byte: number | undefined) => byte !== undefined && (byte & 0xc0) === 0x80 - const rebase = (offset: unknown): number | undefined => { - if (!raw) return typeof offset === "number" ? offset : undefined - if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) return undefined - if (offset !== 0 && offset !== raw.length && isContinuationByte(raw[offset])) return undefined - return Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") - } - const submatches = read(data, "submatches") - // Only rewrite keys the record actually carries — `begin`/`end`/`summary` records reach here too - // and must keep their exact shape, or the strict union below would reject them. - // `path` is deliberately left alone: decoding it is lossy, and a path is an identifier the - // caller reopens, so a U+FFFD-mangled path names a file that does not exist. Such a record - // stays in the `{bytes}` arm and is skipped. See packages/core/src/ripgrep.ts. - const normalized = { - ...json, - data: { - ...data, - ...(lines ? { lines: { text: lines.text } } : {}), - ...(Array.isArray(submatches) - ? { - submatches: submatches.flatMap((submatch) => { - if (!submatch || typeof submatch !== "object") return [submatch] - const match = decode(read(submatch, "match")) - if (!match) return [submatch] - const start = rebase(read(submatch, "start")) - const end = rebase(read(submatch, "end")) - if (start === undefined || end === undefined) return [] - return [{ ...submatch, match: { text: match.text }, start, end }] - }), - } - : {}), - }, - } - return normalized - } - - /** - * Turn ripgrep NDJSON lines into match data, skipping records that cannot be used. - * - * `JSON.parse` + a strict `Result.parse` on every line meant one unusable record threw out of - * `search()` and discarded every match already collected from unrelated files — the same defect - * fixed in packages/core/src/ripgrep.ts. Records are independent, so a bad one is dropped and - * counted. Namespace-private per packages/opencode/AGENTS.md: the skip behaviour is covered - * through the public `search()` boundary instead of exporting an implementation detail. - */ - function parseRecords(lines: string[]): Match["data"][] { - const matches: Match["data"][] = [] - let skipped = 0 - for (const line of lines) { - // Bounds parse cost per record. This path buffers all of stdout before splitting, so it does - // not bound total memory — that needs streaming, tracked separately. - const parsed = - Buffer.byteLength(line, "utf8") > MAX_RECORD_BYTES ? undefined : Result.safeParse(normalizeRecord(line)) - if (!parsed?.success) { - skipped++ - continue - } - if (parsed.data.type === "match") matches.push(parsed.data.data) - } - // Counted and reported once rather than per record: without this a ripgrep protocol change - // would make `/find` answer `[]`, which is indistinguishable from an honest "no matches". - if (skipped > 0) log.warn("skipped unusable ripgrep records", { skipped, total: lines.length }) - return matches - } + // altimate_change start — upstream_fix: record parsing lives in ./ripgrep-records so it can be + // tested directly, without exporting an implementation detail through this namespace and without + // a stub binary whose per-process memoisation leaks into other test files. `Match` stays exported + // here because server/routes/file.ts builds the `/find` response schema from it. + export const Match = RipgrepRecords.Match + const parseRecords = RipgrepRecords.parseRecords // altimate_change end - export type Result = z.infer - export type Match = z.infer - export type Begin = z.infer - export type End = z.infer - export type Summary = z.infer + export type Result = RipgrepRecords.Result + export type Match = RipgrepRecords.Match + export type Begin = RipgrepRecords.Begin + export type End = RipgrepRecords.End + export type Summary = RipgrepRecords.Summary const PLATFORM = { "arm64-darwin": { platform: "aarch64-apple-darwin", extension: "tar.gz" }, "arm64-linux": { diff --git a/packages/opencode/test/file/ripgrep-records.test.ts b/packages/opencode/test/file/ripgrep-records.test.ts new file mode 100644 index 0000000000..8ea3009710 --- /dev/null +++ b/packages/opencode/test/file/ripgrep-records.test.ts @@ -0,0 +1,125 @@ +import { describe, expect, test } from "bun:test" +import { RipgrepRecords } from "../../src/file/ripgrep-records" + +// altimate_change start — upstream_fix: legacy `/find` parsing must survive unusable records. +// `search()` used to `JSON.parse` + strictly `Result.parse` every line, so a single unusable record +// threw out of the whole call and discarded every match already collected from unrelated files — +// the same defect fixed in packages/core/src/ripgrep.ts. This path is reachable from the mounted +// `/find` route (server/routes/file.ts). +// +// These drive the parser directly. It is pure and process-free, so there is no stub binary and no +// PATH mutation: the legacy binary lookup is memoised per process, so a stub would leak into every +// later test file in the same `bun test` run. +const record = (file: string, overrides: Record = {}) => + JSON.stringify({ + type: "match", + data: { + path: { text: `./${file}` }, + lines: { text: "needle\n" }, + line_number: 1, + absolute_offset: 0, + submatches: [{ match: { text: "needle" }, start: 0, end: 6 }], + ...overrides, + }, + }) + +const paths = (records: string[]) => RipgrepRecords.parseRecords(records).map((match) => match.path.text) + +describe("RipgrepRecords.parseRecords", () => { + test("skips an unparseable record and keeps the ones around it", () => { + expect(paths([record("a.txt"), '{"type":"match","data":{"path":{"text":"./b.t', record("c.txt")])).toEqual([ + "./a.txt", + "./c.txt", + ]) + }) + + test("skips a record past the size ceiling", () => { + const huge = record("b.txt", { lines: { text: "n".repeat(17 * 1024 * 1024) } }) + expect(paths([record("a.txt"), huge, record("c.txt")])).toEqual(["./a.txt", "./c.txt"]) + }) + + test("skips a record whose path is not valid UTF-8, rather than mangling the path", () => { + const bad = record("ignored", { path: { bytes: Buffer.from("./b\xff.txt", "binary").toString("base64") } }) + expect(paths([record("a.txt"), bad])).toEqual(["./a.txt"]) + }) + + test("skips empty and non-canonical base64 rather than emitting an empty match", () => { + expect(paths([record("a.txt"), record("b.txt", { lines: { bytes: "" } })])).toEqual(["./a.txt"]) + expect(paths([record("a.txt"), record("b.txt", { lines: { bytes: "Zh==" } })])).toEqual(["./a.txt"]) + }) + + test("decodes a non-UTF8 line and ignores control records", () => { + const parsed = RipgrepRecords.parseRecords([ + JSON.stringify({ type: "begin", data: { path: { text: "./a.txt" } } }), + record("a.txt", { lines: { bytes: Buffer.from("needle \xff tail\n", "binary").toString("base64") } }), + ]) + expect(parsed).toHaveLength(1) + expect(parsed[0].lines.text).toBe("needle � tail\n") + }) + + // Offsets are byte offsets into the RAW line; a lossy decode widens each undecodable byte to a + // 3-byte U+FFFD. This shape is published by the `/find` route, so unrebased offsets would be + // newly wrong output rather than a skipped record. Mirrors the core parser. + test("rebases submatch offsets after a lossy line decode", () => { + const raw = Buffer.concat([Buffer.from([0xff]), Buffer.from("needle tail\n")]) + const parsed = RipgrepRecords.parseRecords([ + record("a.txt", { + lines: { bytes: raw.toString("base64") }, + submatches: [{ match: { bytes: Buffer.from("needle").toString("base64") }, start: 1, end: 7 }], + }), + ]) + + expect(parsed[0].submatches[0]).toEqual({ match: { text: "needle" }, start: 3, end: 9 }) + expect(Buffer.from(parsed[0].lines.text, "utf8").subarray(3, 9).toString("utf8")).toBe("needle") + }) + + // An offset that cannot be expressed in the decoded line drops its submatch, not the match. + test("drops a submatch whose offset is unaddressable, keeping the match", () => { + const raw = Buffer.concat([Buffer.from([0xff]), Buffer.from("needle tail\n")]) + const parsed = RipgrepRecords.parseRecords([ + record("a.txt", { + lines: { bytes: raw.toString("base64") }, + submatches: [{ match: { text: "needle" }, start: 1, end: 9_999 }], + }), + ]) + + expect(parsed).toHaveLength(1) + expect(parsed[0].submatches).toEqual([]) + }) + + test("drops a submatch whose offset splits a character or inverts the range", () => { + const split = RipgrepRecords.parseRecords([ + record("a.txt", { + lines: { bytes: Buffer.concat([Buffer.from("é"), Buffer.from("needle")]).toString("base64") }, + submatches: [{ match: { text: "needle" }, start: 1, end: 3 }], + }), + ]) + expect(split[0].submatches).toEqual([]) + + // Both endpoints are addressable, but the range is inverted. + const inverted = RipgrepRecords.parseRecords([ + record("a.txt", { + lines: { bytes: Buffer.from("needle tail\n").toString("base64") }, + submatches: [{ match: { text: "needle" }, start: 6, end: 2 }], + }), + ]) + expect(inverted[0].submatches).toEqual([]) + }) + + // `MAX_RECORD_BYTES` bounds one input record, not what the `/find` response retains. + test("caps the retained line and submatch text", () => { + const huge = "n".repeat(50_000) + const parsed = RipgrepRecords.parseRecords([ + record("a.txt", { lines: { text: huge }, submatches: [{ match: { text: huge }, start: 0, end: huge.length }] }), + ]) + + expect(parsed[0].lines.text).toHaveLength(2_003) + expect(parsed[0].lines.text.endsWith("...")).toBe(true) + expect(parsed[0].submatches[0].match.text).toHaveLength(2_003) + }) + + test("returns an empty array when every record is unusable, rather than throwing", () => { + expect(RipgrepRecords.parseRecords(["{oops", "{also oops"])).toEqual([]) + }) +}) +// altimate_change end diff --git a/packages/opencode/test/file/ripgrep-search.test.ts b/packages/opencode/test/file/ripgrep-search.test.ts deleted file mode 100644 index d10b84b857..0000000000 --- a/packages/opencode/test/file/ripgrep-search.test.ts +++ /dev/null @@ -1,126 +0,0 @@ -import { afterAll, beforeAll, describe, expect, test } from "bun:test" -import fs from "fs/promises" -import os from "os" -import path from "path" -import { Ripgrep } from "../../src/file/ripgrep" - -// altimate_change start — upstream_fix: legacy `/find` search must survive unusable records. -// `search()` used to `JSON.parse` + strictly `Result.parse` every line, so a single unusable record -// threw out of the whole call and discarded every match already collected from unrelated files — -// the same defect fixed in packages/core/src/ripgrep.ts. This path is reachable from the mounted -// `/find` route (server/routes/file.ts), so it needs its own coverage. -// -// The cases drive the real `search()` — the parser is namespace-private per AGENTS.md — against a -// stub `rg` on PATH that prints exactly the NDJSON given. `which("rg")` resolves from PATH and the -// result is memoised, so PATH is set once before any search runs. The stub is a POSIX shell script, -// so this file is POSIX-only; the parser itself is platform-independent. -const posixTest = test.skipIf(process.platform === "win32") - -let dir: string -let dataFile: string -let originalPath: string | undefined - -beforeAll(async () => { - dir = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), "legacy-rg-"))) - dataFile = path.join(dir, "records.jsonl") - const bin = path.join(dir, "bin") - await fs.mkdir(bin) - const stub = path.join(bin, "rg") - await fs.writeFile(stub, `#!/bin/sh\ncat ${JSON.stringify(dataFile)}\n`) - await fs.chmod(stub, 0o755) - originalPath = process.env.PATH - process.env.PATH = `${bin}${path.delimiter}${originalPath ?? ""}` -}) - -afterAll(async () => { - process.env.PATH = originalPath - await fs.rm(dir, { recursive: true, force: true }) -}) - -const record = (file: string, overrides: Record = {}) => - JSON.stringify({ - type: "match", - data: { - path: { text: `./${file}` }, - lines: { text: "needle\n" }, - line_number: 1, - absolute_offset: 0, - submatches: [{ match: { text: "needle" }, start: 0, end: 6 }], - ...overrides, - }, - }) - -const search = async (records: string[]) => { - await fs.writeFile(dataFile, records.join("\n") + "\n") - return Ripgrep.search({ cwd: dir, pattern: "needle", limit: 100 }) -} -const paths = async (records: string[]) => (await search(records)).map((match) => match.path.text) - -describe("legacy Ripgrep.search", () => { - posixTest("skips an unparseable record and keeps the ones around it", async () => { - expect(await paths([record("a.txt"), '{"type":"match","data":{"path":{"text":"./b.t', record("c.txt")])).toEqual([ - "./a.txt", - "./c.txt", - ]) - }) - - posixTest("skips a record past the size ceiling", async () => { - const huge = record("b.txt", { lines: { text: "n".repeat(17 * 1024 * 1024) } }) - expect(await paths([record("a.txt"), huge, record("c.txt")])).toEqual(["./a.txt", "./c.txt"]) - }) - - posixTest("skips a record whose path is not valid UTF-8, rather than mangling the path", async () => { - const bad = record("ignored", { path: { bytes: Buffer.from("./b\xff.txt", "binary").toString("base64") } }) - expect(await paths([record("a.txt"), bad])).toEqual(["./a.txt"]) - }) - - posixTest("skips empty and non-canonical base64 rather than emitting an empty match", async () => { - expect(await paths([record("a.txt"), record("b.txt", { lines: { bytes: "" } })])).toEqual(["./a.txt"]) - expect(await paths([record("a.txt"), record("b.txt", { lines: { bytes: "Zh==" } })])).toEqual(["./a.txt"]) - }) - - posixTest("decodes a non-UTF8 line and ignores control records", async () => { - const parsed = await search([ - JSON.stringify({ type: "begin", data: { path: { text: "./a.txt" } } }), - record("a.txt", { lines: { bytes: Buffer.from("needle \xff tail\n", "binary").toString("base64") } }), - ]) - expect(parsed).toHaveLength(1) - expect(parsed[0].lines.text).toBe("needle � tail\n") - }) - - // Offsets are byte offsets into the RAW line; a lossy decode widens each undecodable byte to a - // 3-byte U+FFFD. This response shape is published by the `/find` route, so leaving them unrebased - // would be newly wrong output rather than a skipped record. Mirrors the core parser. - posixTest("rebases submatch offsets after a lossy line decode", async () => { - const raw = Buffer.concat([Buffer.from([0xff]), Buffer.from("needle tail\n")]) - const parsed = await search([ - record("a.txt", { - lines: { bytes: raw.toString("base64") }, - submatches: [{ match: { bytes: Buffer.from("needle").toString("base64") }, start: 1, end: 7 }], - }), - ]) - - expect(parsed).toHaveLength(1) - expect(parsed[0].submatches[0]).toEqual({ match: { text: "needle" }, start: 3, end: 9 }) - expect(Buffer.from(parsed[0].lines.text, "utf8").subarray(3, 9).toString("utf8")).toBe("needle") - }) - - // An offset that cannot be expressed in the decoded line drops its submatch, not the match. - posixTest("drops a submatch whose offset is unaddressable, keeping the match", async () => { - const raw = Buffer.concat([Buffer.from([0xff]), Buffer.from("needle tail\n")]) - const parsed = await search([ - record("a.txt", { - lines: { bytes: raw.toString("base64") }, - submatches: [{ match: { text: "needle" }, start: 1, end: 9_999 }], - }), - ]) - - expect(parsed).toHaveLength(1) - expect(parsed[0].submatches).toEqual([]) - }) - - posixTest("returns an empty array when every record is unusable, rather than throwing", async () => { - expect(await search(["{oops", "{also oops"])).toEqual([]) - }) -}) -// altimate_change end From c95f23417baba13450d6dd6eaad640e477b861fe Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 20 Aug 2026 15:16:20 +0530 Subject: [PATCH 09/11] fix(core): flat module shape, submatch bound, and text-arm offset validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fifth review round — Kilo and cubic on the extracted module. - The new module used `export namespace`, which packages/opencode/AGENTS.md explicitly prohibits, and its header cited that rule INVERTED: it claimed AGENTS.md forbids `export * as`, when `export * as` is the prescribed pattern and `export namespace` is what is forbidden (not standard ESM, blocks tree-shaking, breaks Node's native TS runner). Flattened to top-level exports with a self-reexport at the bottom; importers are unchanged. Header corrected. - Bound the submatch count on the legacy path, matching the core parser. Each submatch costs a rebase per endpoint and a rebase allocates a string up to the line length, so an unbounded array turned one in-ceiling record into O(count x line) work on the shipped `/find` route. - Validate `{text}`-arm offsets in both parsers. Nothing is rebased there, but the offset must still be addressable in the line it indexes: core's `NonNegativeInt` rejects negatives and fractions yet not values past the end, and legacy's `z.number()` rejects none of them. Raised by two reviewers independently; my earlier push-back was too narrow — I argued no claim is made on that arm, but returning a coordinate pair that indexes nothing is a claim. - Dropped a stale doc comment describing the previous, superseded layout. Tests: 23 core, 12 legacy. Marker balance verified across all five files. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/ripgrep.ts | 14 +- packages/core/test/ripgrep.test.ts | 14 + packages/opencode/src/file/ripgrep-records.ts | 408 +++++++++--------- .../test/file/ripgrep-records.test.ts | 22 + 4 files changed, 257 insertions(+), 201 deletions(-) diff --git a/packages/core/src/ripgrep.ts b/packages/core/src/ripgrep.ts index 9755d61563..679f4679cf 100644 --- a/packages/core/src/ripgrep.ts +++ b/packages/core/src/ripgrep.ts @@ -148,10 +148,17 @@ const normalizeMatch = (json: object): unknown => { // are still correct and useful, and this whole change exists to stop losing matches. Offsets on // the `{text}` arm are untouched — no rebasing happens there, so no claim is made. const isContinuationByte = (byte: number | undefined) => byte !== undefined && (byte & 0xc0) === 0x80 + const lineBytes = Buffer.byteLength(lines.text, "utf8") const rebase = (offset: unknown): number | undefined => { - if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw!.length) return undefined - if (offset !== 0 && offset !== raw!.length && isContinuationByte(raw![offset])) return undefined - return Buffer.byteLength(raw!.subarray(0, offset).toString("utf8"), "utf8") + // `{text}` arm: nothing is rebased, but the offset must still be addressable in the line it + // indexes, or the record carries a coordinate pair that points at nothing. + if (!raw) + return typeof offset === "number" && Number.isInteger(offset) && offset >= 0 && offset <= lineBytes + ? offset + : undefined + if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) return undefined + if (offset !== 0 && offset !== raw.length && isContinuationByte(raw[offset])) return undefined + return Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") } const submatches = readProp(data, "submatches") const normalized = { @@ -171,7 +178,6 @@ const normalizeMatch = (json: object): unknown => { // matched text needs the same bound as the line or the retained-memory cap is defeated // by the submatches instead. const decoded = { ...submatch, match: { text: capLineText(match.text) } } - if (!raw) return [decoded] const start = rebase(readProp(submatch, "start")) const end = rebase(readProp(submatch, "end")) // Endpoints are rebased independently, so ordering is checked explicitly: an inverted diff --git a/packages/core/test/ripgrep.test.ts b/packages/core/test/ripgrep.test.ts index 92647506ff..e609bd54fc 100644 --- a/packages/core/test/ripgrep.test.ts +++ b/packages/core/test/ripgrep.test.ts @@ -348,6 +348,20 @@ describe("Ripgrep", () => { expect(matches[0].submatches).toEqual([]) }) + // The `{text}` arm is not rebased, but an offset still has to be addressable in the line. + stubTest("drops a text-arm submatch whose offset is past the end of the line", async () => { + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("a.txt", { + lines: { text: "needle\n" }, + submatches: [{ match: { text: "needle" }, start: 0, end: 999 }], + }), + ]), + ) + + expect(matches[0].submatches).toEqual([]) + }) + // Endpoints are rebased independently, so an inverted range can survive both endpoint checks. stubTest("drops a submatch whose range is inverted", async () => { const matches = await Effect.runPromise( diff --git a/packages/opencode/src/file/ripgrep-records.ts b/packages/opencode/src/file/ripgrep-records.ts index 3512b9a677..7195b1b5d0 100644 --- a/packages/opencode/src/file/ripgrep-records.ts +++ b/packages/opencode/src/file/ripgrep-records.ts @@ -1,224 +1,238 @@ // altimate_change start — upstream_fix: ripgrep NDJSON record parsing, split out of ripgrep.ts. // // This lives in its own module rather than inside the `Ripgrep` namespace because it is pure, -// process-free logic that deserves direct tests. Keeping it in the namespace forced a choice -// between two bad options: exporting an implementation detail through `export * as` (which -// packages/opencode/AGENTS.md prohibits), or driving it through `search()` with a stub `rg` on -// PATH — and the binary lookup there is memoised per process, so such a stub leaks into every -// later test file in the same `bun test` run and breaks unrelated suites. +// process-free logic that deserves direct tests. Inside the namespace it was reachable only two +// ways, both bad: projected through `export * as` as an implementation detail of the public +// namespace, or driven through `search()` with a stub `rg` on PATH — and that binary lookup is +// memoised per process, so such a stub leaks into every later test file in the same `bun test` +// run and breaks unrelated suites. // -// It mirrors packages/core/src/ripgrep.ts, but the two are not identical by design. The core -// parser streams; this one buffers all of stdout before splitting, and hands its records straight -// to the `/find` response. Both cap retained text, skip records they cannot use, and report the -// skips once per search rather than once per record. +// Module shape follows packages/opencode/AGENTS.md: flat top-level exports with a self-reexport at +// the bottom, not `export namespace`. +// +// It mirrors packages/core/src/ripgrep.ts, but the two are not identical by design. The core parser +// streams; this one buffers all of stdout before splitting, and hands its records straight to the +// `/find` response. Both cap retained text, bound submatch counts, skip records they cannot use, +// and report the skips once per search rather than once per record. import z from "zod" import { Log } from "@/util/log" -export namespace RipgrepRecords { - const log = Log.create({ service: "ripgrep" }) +const log = Log.create({ service: "ripgrep" }) - const Stats = z.object({ - elapsed: z.object({ - secs: z.number(), - nanos: z.number(), - human: z.string(), - }), - searches: z.number(), - searches_with_match: z.number(), - bytes_searched: z.number(), - bytes_printed: z.number(), - matched_lines: z.number(), - matches: z.number(), - }) +const Stats = z.object({ + elapsed: z.object({ + secs: z.number(), + nanos: z.number(), + human: z.string(), + }), + searches: z.number(), + searches_with_match: z.number(), + bytes_searched: z.number(), + bytes_printed: z.number(), + matched_lines: z.number(), + matches: z.number(), +}) - const Begin = z.object({ - type: z.literal("begin"), - data: z.object({ - path: z.object({ - text: z.string(), - }), +const Begin = z.object({ + type: z.literal("begin"), + data: z.object({ + path: z.object({ + text: z.string(), }), - }) + }), +}) - export const Match = z.object({ - type: z.literal("match"), - data: z.object({ - path: z.object({ - text: z.string(), - }), - lines: z.object({ - text: z.string(), - }), - line_number: z.number(), - absolute_offset: z.number(), - submatches: z.array( - z.object({ - match: z.object({ - text: z.string(), - }), - start: z.number(), - end: z.number(), - }), - ), +export const Match = z.object({ + type: z.literal("match"), + data: z.object({ + path: z.object({ + text: z.string(), }), - }) - - const End = z.object({ - type: z.literal("end"), - data: z.object({ - path: z.object({ - text: z.string(), + lines: z.object({ + text: z.string(), + }), + line_number: z.number(), + absolute_offset: z.number(), + submatches: z.array( + z.object({ + match: z.object({ + text: z.string(), + }), + start: z.number(), + end: z.number(), }), - binary_offset: z.number().nullable(), - stats: Stats, + ), + }), +}) + +const End = z.object({ + type: z.literal("end"), + data: z.object({ + path: z.object({ + text: z.string(), }), - }) + binary_offset: z.number().nullable(), + stats: Stats, + }), +}) - const Summary = z.object({ - type: z.literal("summary"), - data: z.object({ - elapsed_total: z.object({ - human: z.string(), - nanos: z.number(), - secs: z.number(), - }), - stats: Stats, +const Summary = z.object({ + type: z.literal("summary"), + data: z.object({ + elapsed_total: z.object({ + human: z.string(), + nanos: z.number(), + secs: z.number(), }), - }) + stats: Stats, + }), +}) - const Result = z.union([Begin, Match, End, Summary]) +const Result = z.union([Begin, Match, End, Summary]) - // altimate_change start — upstream_fix: tolerate ripgrep's `{bytes}` arm and malformed lines. - // - // This mirrors packages/core/src/ripgrep.ts, but deliberately not in every respect. That parser - // streams, so it caps the retained line text and rebases submatch offsets; this one buffers all of - // stdout up front and hands its records straight to the `/find` response, where the raw ripgrep - // shape is the published contract — so it normalises and skips, and leaves the shape alone. Both - // report skipped records once per search rather than once per record. - const BASE64 = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/ +// Tolerating ripgrep's `{bytes}` arm and malformed lines. (The whole file is covered by the +// marker at the top — this module is new, not an edit to upstream code.) +// +// This mirrors packages/core/src/ripgrep.ts, but deliberately not in every respect. That parser +// streams, so it caps the retained line text and rebases submatch offsets; this one buffers all of +// stdout up front and hands its records straight to the `/find` response, where the raw ripgrep +// shape is the published contract — so it normalises and skips, and leaves the shape alone. Both +// report skipped records once per search rather than once per record. +const BASE64 = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/ - /** Mirrors packages/core/src/ripgrep.ts. Bounds parse cost per record on this path too. */ - const MAX_RECORD_BYTES = 16 * 1024 * 1024 +/** Mirrors packages/core/src/ripgrep.ts. Bounds parse cost per record on this path too. */ +const MAX_RECORD_BYTES = 16 * 1024 * 1024 - // `MAX_RECORD_BYTES` bounds ONE input record; it does not bound what the response retains. This - // path buffers all of stdout and returns every match, so without a per-field cap a tree of large - // records still retains — and serialises into the `/find` response — an unbounded amount of text. - // Same cap and elision marker as the core parser, so the two paths agree on what a match shows. - const LINE_TEXT_CAP = 2_000 - const capText = (text: string) => (text.length > LINE_TEXT_CAP ? text.slice(0, LINE_TEXT_CAP) + "..." : text) +// Also mirrors core. Each submatch costs a rebase per endpoint, and a rebase allocates a string up +// to the length of the line, so an unbounded submatch array turns one in-ceiling record into +// O(count x line) work. A protocol change is exactly the shape that would emit one. +const MAX_SUBMATCHES = 100 - /** Parse one NDJSON record, rewriting `{bytes: base64}` fields into the `{text}` arm. */ - const normalizeRecord = (line: string): unknown => { - let json: unknown - try { - json = JSON.parse(line) - } catch { - return undefined - } - if (!json || typeof json !== "object") return json - const read = (value: unknown, key: string): unknown => - value !== null && typeof value === "object" && key in value ? Reflect.get(value, key) : undefined - const data = read(json, "data") - if (!data || typeof data !== "object") return json - /** Decode a `{text}`/`{bytes}` field, returning the raw buffer so offsets can be rebased. */ - const decode = (value: unknown): { text: string; raw?: Buffer } | undefined => { - if (!value || typeof value !== "object") return undefined - const text = read(value, "text") - if (typeof text === "string") return { text } - const bytes = read(value, "bytes") - // Guarded three ways because `Buffer.from` decodes unconvertible input to an EMPTY buffer - // instead of throwing, which would turn a corrupt record into a schema-valid empty match: - // reject the empty string (a matched line is never empty), check the spelling, then require - // a round-trip so non-canonical padding ("Zh==" and "Zg==" both decode to "f") is rejected. - if (typeof bytes !== "string" || bytes.length === 0 || !BASE64.test(bytes)) return undefined - const decoded = Buffer.from(bytes, "base64") - if (decoded.toString("base64") !== bytes) return undefined - return { text: decoded.toString("utf8"), raw: decoded } - } - const lines = "lines" in data ? decode(read(data, "lines")) : undefined - // Submatch offsets are BYTE offsets into the RAW line, and a lossy decode widens every - // undecodable byte to a 3-byte U+FFFD — so they must be rebased onto the decoded text's own - // UTF-8 encoding or they no longer locate the match. This response shape is published by the - // `/find` route, so unrebased offsets would be newly wrong output rather than a skipped record. - // Mirrors packages/core/src/ripgrep.ts. - const raw = lines?.raw - // Byte-boundary validation, matching packages/core/src/ripgrep.ts. A decoded-string comparison - // is not sufficient: when an invalid byte precedes a LITERAL U+FFFD, an offset inside that - // character still yields a prefix that prefixes the line, because the replacement characters - // alias. A continuation byte (0b10xxxxxx) at the offset means the split lands inside a sequence. - // An offset that cannot be rebased drops ITS SUBMATCH, not the record — the file, line and text - // stay correct, and losing a highlight range beats losing the match. - const isContinuationByte = (byte: number | undefined) => byte !== undefined && (byte & 0xc0) === 0x80 - const rebase = (offset: unknown): number | undefined => { - if (!raw) return typeof offset === "number" ? offset : undefined - if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) return undefined - if (offset !== 0 && offset !== raw.length && isContinuationByte(raw[offset])) return undefined - return Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") - } - const submatches = read(data, "submatches") - // Only rewrite keys the record actually carries — `begin`/`end`/`summary` records reach here too - // and must keep their exact shape, or the strict union below would reject them. - // `path` is deliberately left alone: decoding it is lossy, and a path is an identifier the - // caller reopens, so a U+FFFD-mangled path names a file that does not exist. Such a record - // stays in the `{bytes}` arm and is skipped. See packages/core/src/ripgrep.ts. - const normalized = { - ...json, - data: { - ...data, - ...(lines ? { lines: { text: capText(lines.text) } } : {}), - ...(Array.isArray(submatches) - ? { - submatches: submatches.flatMap((submatch) => { - if (!submatch || typeof submatch !== "object") return [submatch] - const match = decode(read(submatch, "match")) - if (!match) return [submatch] - const start = rebase(read(submatch, "start")) - const end = rebase(read(submatch, "end")) - // Endpoints are rebased independently, so ordering is checked explicitly. - if (start === undefined || end === undefined || start > end) return [] - return [{ ...submatch, match: { text: capText(match.text) }, start, end }] - }), - } - : {}), - }, - } - return normalized +// `MAX_RECORD_BYTES` bounds ONE input record; it does not bound what the response retains. This +// path buffers all of stdout and returns every match, so without a per-field cap a tree of large +// records still retains — and serialises into the `/find` response — an unbounded amount of text. +// Same cap and elision marker as the core parser, so the two paths agree on what a match shows. +const LINE_TEXT_CAP = 2_000 +const capText = (text: string) => (text.length > LINE_TEXT_CAP ? text.slice(0, LINE_TEXT_CAP) + "..." : text) + +/** Parse one NDJSON record, rewriting `{bytes: base64}` fields into the `{text}` arm. */ +const normalizeRecord = (line: string): unknown => { + let json: unknown + try { + json = JSON.parse(line) + } catch { + return undefined } + if (!json || typeof json !== "object") return json + const read = (value: unknown, key: string): unknown => + value !== null && typeof value === "object" && key in value ? Reflect.get(value, key) : undefined + const data = read(json, "data") + if (!data || typeof data !== "object") return json + /** Decode a `{text}`/`{bytes}` field, returning the raw buffer so offsets can be rebased. */ + const decode = (value: unknown): { text: string; raw?: Buffer } | undefined => { + if (!value || typeof value !== "object") return undefined + const text = read(value, "text") + if (typeof text === "string") return { text } + const bytes = read(value, "bytes") + // Guarded three ways because `Buffer.from` decodes unconvertible input to an EMPTY buffer + // instead of throwing, which would turn a corrupt record into a schema-valid empty match: + // reject the empty string (a matched line is never empty), check the spelling, then require + // a round-trip so non-canonical padding ("Zh==" and "Zg==" both decode to "f") is rejected. + if (typeof bytes !== "string" || bytes.length === 0 || !BASE64.test(bytes)) return undefined + const decoded = Buffer.from(bytes, "base64") + if (decoded.toString("base64") !== bytes) return undefined + return { text: decoded.toString("utf8"), raw: decoded } + } + const lines = "lines" in data ? decode(read(data, "lines")) : undefined + // Submatch offsets are BYTE offsets into the RAW line, and a lossy decode widens every + // undecodable byte to a 3-byte U+FFFD — so they must be rebased onto the decoded text's own + // UTF-8 encoding or they no longer locate the match. This response shape is published by the + // `/find` route, so unrebased offsets would be newly wrong output rather than a skipped record. + // Mirrors packages/core/src/ripgrep.ts. + const raw = lines?.raw + // Byte-boundary validation, matching packages/core/src/ripgrep.ts. A decoded-string comparison + // is not sufficient: when an invalid byte precedes a LITERAL U+FFFD, an offset inside that + // character still yields a prefix that prefixes the line, because the replacement characters + // alias. A continuation byte (0b10xxxxxx) at the offset means the split lands inside a sequence. + // An offset that cannot be rebased drops ITS SUBMATCH, not the record — the file, line and text + // stay correct, and losing a highlight range beats losing the match. + const isContinuationByte = (byte: number | undefined) => byte !== undefined && (byte & 0xc0) === 0x80 + const lineBytes = lines ? Buffer.byteLength(lines.text, "utf8") : 0 + const rebase = (offset: unknown): number | undefined => { + // `{text}` arm: nothing is rebased, but the offset must still be addressable in the line it + // indexes. `z.number()` accepts negatives, fractions and values past the end, so without this a + // corrupt record reaches the `/find` response with coordinates that index nothing. + if (!raw) + return typeof offset === "number" && Number.isInteger(offset) && offset >= 0 && offset <= lineBytes + ? offset + : undefined + if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) return undefined + if (offset !== 0 && offset !== raw.length && isContinuationByte(raw[offset])) return undefined + return Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") + } + const submatches = read(data, "submatches") + // Only rewrite keys the record actually carries — `begin`/`end`/`summary` records reach here too + // and must keep their exact shape, or the strict union below would reject them. + // `path` is deliberately left alone: decoding it is lossy, and a path is an identifier the + // caller reopens, so a U+FFFD-mangled path names a file that does not exist. Such a record + // stays in the `{bytes}` arm and is skipped. See packages/core/src/ripgrep.ts. + const normalized = { + ...json, + data: { + ...data, + ...(lines ? { lines: { text: capText(lines.text) } } : {}), + ...(Array.isArray(submatches) + ? { + submatches: submatches.slice(0, MAX_SUBMATCHES).flatMap((submatch) => { + if (!submatch || typeof submatch !== "object") return [submatch] + const match = decode(read(submatch, "match")) + if (!match) return [submatch] + const start = rebase(read(submatch, "start")) + const end = rebase(read(submatch, "end")) + // Endpoints are rebased independently, so ordering is checked explicitly. + if (start === undefined || end === undefined || start > end) return [] + return [{ ...submatch, match: { text: capText(match.text) }, start, end }] + }), + } + : {}), + }, + } + return normalized +} - /** - * Turn ripgrep NDJSON lines into match data, skipping records that cannot be used. - * - * `JSON.parse` + a strict `Result.parse` on every line meant one unusable record threw out of - * `search()` and discarded every match already collected from unrelated files — the same defect - * fixed in packages/core/src/ripgrep.ts. Records are independent, so a bad one is dropped and - * counted. Namespace-private per packages/opencode/AGENTS.md: the skip behaviour is covered - * through the public `search()` boundary instead of exporting an implementation detail. - */ - export function parseRecords(lines: string[]): Match["data"][] { - const matches: Match["data"][] = [] - let skipped = 0 - for (const line of lines) { - // Bounds parse cost per record. This path buffers all of stdout before splitting, so it does - // not bound total memory — that needs streaming, tracked separately. - const parsed = - Buffer.byteLength(line, "utf8") > MAX_RECORD_BYTES ? undefined : Result.safeParse(normalizeRecord(line)) - if (!parsed?.success) { - skipped++ - continue - } - if (parsed.data.type === "match") matches.push(parsed.data.data) +/** + * Turn ripgrep NDJSON lines into match data, skipping records that cannot be used. + * + * `JSON.parse` + a strict `Result.parse` on every line meant one unusable record threw out of + * `search()` and discarded every match already collected from unrelated files — the same defect + * fixed in packages/core/src/ripgrep.ts. Records are independent, so a bad one is dropped and + * counted. Pure and process-free, so `test/file/ripgrep-records.test.ts` drives it directly. + */ +export function parseRecords(lines: string[]): Match["data"][] { + const matches: Match["data"][] = [] + let skipped = 0 + for (const line of lines) { + // Bounds parse cost per record. This path buffers all of stdout before splitting, so it does + // not bound total memory — that needs streaming, tracked separately. + const parsed = + Buffer.byteLength(line, "utf8") > MAX_RECORD_BYTES ? undefined : Result.safeParse(normalizeRecord(line)) + if (!parsed?.success) { + skipped++ + continue } - // Counted and reported once rather than per record: without this a ripgrep protocol change - // would make `/find` answer `[]`, which is indistinguishable from an honest "no matches". - if (skipped > 0) log.warn("skipped unusable ripgrep records", { skipped, total: lines.length }) - return matches + if (parsed.data.type === "match") matches.push(parsed.data.data) } - // altimate_change end - - export type Result = z.infer - export type Match = z.infer - export type Begin = z.infer - export type End = z.infer - export type Summary = z.infer + // Counted and reported once rather than per record: without this a ripgrep protocol change + // would make `/find` answer `[]`, which is indistinguishable from an honest "no matches". + if (skipped > 0) log.warn("skipped unusable ripgrep records", { skipped, total: lines.length }) + return matches } + +export type Result = z.infer +export type Match = z.infer +export type Begin = z.infer +export type End = z.infer +export type Summary = z.infer + +export * as RipgrepRecords from "./ripgrep-records" // altimate_change end diff --git a/packages/opencode/test/file/ripgrep-records.test.ts b/packages/opencode/test/file/ripgrep-records.test.ts index 8ea3009710..87055d8fee 100644 --- a/packages/opencode/test/file/ripgrep-records.test.ts +++ b/packages/opencode/test/file/ripgrep-records.test.ts @@ -118,6 +118,28 @@ describe("RipgrepRecords.parseRecords", () => { expect(parsed[0].submatches[0].match.text).toHaveLength(2_003) }) + // `{text}` arm: nothing is rebased, but `z.number()` accepts negatives, fractions and values past + // the end, so a corrupt record would otherwise reach the `/find` response indexing nothing. + test("drops a text-arm submatch whose offset is not addressable in the line", () => { + const parsed = RipgrepRecords.parseRecords([ + record("a.txt", { lines: { text: "needle\n" }, submatches: [{ match: { text: "needle" }, start: 0, end: 999 }] }), + ]) + expect(parsed[0].submatches).toEqual([]) + + const fractional = RipgrepRecords.parseRecords([ + record("a.txt", { lines: { text: "needle\n" }, submatches: [{ match: { text: "needle" }, start: 0.5, end: 6 }] }), + ]) + expect(fractional[0].submatches).toEqual([]) + }) + + // Each submatch costs a rebase per endpoint, and a rebase allocates a string up to the line + // length, so an unbounded array turns one in-ceiling record into O(count x line) work. + test("bounds the submatch count like the core parser", () => { + const many = Array.from({ length: 5_000 }, () => ({ match: { text: "n" }, start: 0, end: 1 })) + const parsed = RipgrepRecords.parseRecords([record("a.txt", { submatches: many })]) + expect(parsed[0].submatches).toHaveLength(100) + }) + test("returns an empty array when every record is unusable, rather than throwing", () => { expect(RipgrepRecords.parseRecords(["{oops", "{also oops"])).toEqual([]) }) From e983649afea434075257e6ef5f62352477360431 Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 20 Aug 2026 15:55:53 +0530 Subject: [PATCH 10/11] fix(core): reject text-arm offsets that split a character MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sixth review round — cubic and Kilo on the previous commit, both pointing at the `{text}`-arm validation that commit had just added. - That validation checked the range but not the character boundary, so a byte offset landing inside a multi-byte sequence was accepted: for the line `éa` (3 bytes) a corrupt `start: 1` splits `é`, and a consumer slicing the line's UTF-8 encoding there gets invalid bytes. The `{bytes}` arm already rejected exactly that shape. Both arms now apply the same continuation-byte check. ripgrep never emits a mid-boundary offset for a valid-UTF-8 line, so this drops nothing legitimate — a boundary-aligned offset on the same line is kept, which the tests assert alongside the rejection. The line is encoded once per record and shared by every submatch. That replaces the `Buffer.byteLength` walk the previous commit added rather than stacking on top of it; the extra cost is the allocation. - Fixed a comment stale as of the previous commit: it still said `{text}`-arm offsets are untouched and that no claim is made about them, which the validation added in that same commit contradicts. - Gave the >16 MiB record test an explicit 30s timeout. It materialises the record, so it is slow enough to trip the default when the suite runs under load; it failed once that way locally while another suite ran concurrently, then passed 3/3 in isolation. Tests: 24 core, 14 legacy. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/ripgrep.ts | 23 ++++++++--- packages/core/test/ripgrep.test.ts | 41 ++++++++++++++----- packages/opencode/src/file/ripgrep-records.ts | 16 +++++--- .../test/file/ripgrep-records.test.ts | 14 +++++++ 4 files changed, 72 insertions(+), 22 deletions(-) diff --git a/packages/core/src/ripgrep.ts b/packages/core/src/ripgrep.ts index 679f4679cf..0dad82bd77 100644 --- a/packages/core/src/ripgrep.ts +++ b/packages/core/src/ripgrep.ts @@ -145,15 +145,26 @@ const normalizeMatch = (json: object): unknown => { // errs conservatively, on lines that begin mid-sequence. // // An offset that cannot be rebased drops ITS SUBMATCH, not the record: the file, line and text - // are still correct and useful, and this whole change exists to stop losing matches. Offsets on - // the `{text}` arm are untouched — no rebasing happens there, so no claim is made. + // are still correct and useful, and this whole change exists to stop losing matches. `{text}`-arm + // offsets are not rebased — there is nothing to rebase onto — but they are validated the same + // way, because returning a coordinate pair that indexes nothing, or that splits a character, is + // itself a claim. const isContinuationByte = (byte: number | undefined) => byte !== undefined && (byte & 0xc0) === 0x80 - const lineBytes = Buffer.byteLength(lines.text, "utf8") + // Encoded once per record and shared by every submatch. This also supplies the byte length, so it + // replaces a `Buffer.byteLength` walk rather than adding one; the extra cost is the allocation. + const textBytes = raw ?? Buffer.from(lines.text, "utf8") const rebase = (offset: unknown): number | undefined => { - // `{text}` arm: nothing is rebased, but the offset must still be addressable in the line it - // indexes, or the record carries a coordinate pair that points at nothing. + // `{text}` arm: nothing is rebased, but the offset must still be addressable AND land on a + // character boundary. A byte offset can fall inside a multi-byte sequence — for `éa` the byte + // offset 1 splits `é` — and a consumer slicing the line's UTF-8 encoding there gets invalid + // bytes. ripgrep never emits such an offset for a valid-UTF-8 line, so rejecting it drops + // nothing legitimate. Same rule the `{bytes}` arm applies below. if (!raw) - return typeof offset === "number" && Number.isInteger(offset) && offset >= 0 && offset <= lineBytes + return typeof offset === "number" && + Number.isInteger(offset) && + offset >= 0 && + offset <= textBytes.length && + !(offset !== 0 && offset !== textBytes.length && isContinuationByte(textBytes[offset])) ? offset : undefined if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) return undefined diff --git a/packages/core/test/ripgrep.test.ts b/packages/core/test/ripgrep.test.ts index e609bd54fc..02e043d0de 100644 --- a/packages/core/test/ripgrep.test.ts +++ b/packages/core/test/ripgrep.test.ts @@ -248,17 +248,23 @@ describe("Ripgrep", () => { // The size ceiling is asserted on a record built to exceed it, rather than inferred from a large // file — that keeps the case independent of whether a given ripgrep build emits the match at all. - stubTest("skips an oversized record and keeps parsing the records after it", async () => { - const matches = await Effect.runPromise( - grepWithStubbedRecords([ - matchRecord("a.txt"), - matchRecord("b-huge.txt", { lines: { text: "needle" + "x".repeat(17 * 1024 * 1024) } }), - matchRecord("c.txt"), - ]), - ) - - expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt"), RelativePath.make("c.txt")]) - }) + stubTest( + "skips an oversized record and keeps parsing the records after it", + async () => { + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("a.txt"), + matchRecord("b-huge.txt", { lines: { text: "needle" + "x".repeat(17 * 1024 * 1024) } }), + matchRecord("c.txt"), + ]), + ) + + expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt"), RelativePath.make("c.txt")]) + // Explicit timeout: this materialises a >16 MiB record, so it is slow enough to trip the default + // under load even though the parser rejects the record before parsing it. + }, + 30_000, + ) // A path is an identifier the caller reopens, so it must never be lossily decoded. Such a record // is skipped rather than reported under a U+FFFD-mangled path that names no real file. @@ -362,6 +368,19 @@ describe("Ripgrep", () => { expect(matches[0].submatches).toEqual([]) }) + stubTest("drops a text-arm submatch whose offset splits a multi-byte character", async () => { + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("a.txt", { lines: { text: "éa" }, submatches: [{ match: { text: "a" }, start: 1, end: 3 }] }), + matchRecord("b.txt", { lines: { text: "éa" }, submatches: [{ match: { text: "a" }, start: 2, end: 3 }] }), + ]), + ) + + // Offset 1 splits `é`; offset 2 is the character boundary, so that submatch is kept. + expect(matches[0].submatches).toEqual([]) + expect(matches[1].submatches).toHaveLength(1) + }) + // Endpoints are rebased independently, so an inverted range can survive both endpoint checks. stubTest("drops a submatch whose range is inverted", async () => { const matches = await Effect.runPromise( diff --git a/packages/opencode/src/file/ripgrep-records.ts b/packages/opencode/src/file/ripgrep-records.ts index 7195b1b5d0..5006a7648d 100644 --- a/packages/opencode/src/file/ripgrep-records.ts +++ b/packages/opencode/src/file/ripgrep-records.ts @@ -157,13 +157,19 @@ const normalizeRecord = (line: string): unknown => { // An offset that cannot be rebased drops ITS SUBMATCH, not the record — the file, line and text // stay correct, and losing a highlight range beats losing the match. const isContinuationByte = (byte: number | undefined) => byte !== undefined && (byte & 0xc0) === 0x80 - const lineBytes = lines ? Buffer.byteLength(lines.text, "utf8") : 0 + // Encoded once per record and shared by every submatch; also supplies the byte length. + const textBytes = raw ?? (lines ? Buffer.from(lines.text, "utf8") : Buffer.alloc(0)) const rebase = (offset: unknown): number | undefined => { - // `{text}` arm: nothing is rebased, but the offset must still be addressable in the line it - // indexes. `z.number()` accepts negatives, fractions and values past the end, so without this a - // corrupt record reaches the `/find` response with coordinates that index nothing. + // `{text}` arm: nothing is rebased, but the offset must still be addressable AND land on a + // character boundary — a byte offset can fall inside a multi-byte sequence (`éa`, offset 1 + // splits `é`). `z.number()` rejects none of that, so a corrupt record would otherwise reach the + // `/find` response with coordinates that index nothing or half a character. if (!raw) - return typeof offset === "number" && Number.isInteger(offset) && offset >= 0 && offset <= lineBytes + return typeof offset === "number" && + Number.isInteger(offset) && + offset >= 0 && + offset <= textBytes.length && + !(offset !== 0 && offset !== textBytes.length && isContinuationByte(textBytes[offset])) ? offset : undefined if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) return undefined diff --git a/packages/opencode/test/file/ripgrep-records.test.ts b/packages/opencode/test/file/ripgrep-records.test.ts index 87055d8fee..d2b94a187d 100644 --- a/packages/opencode/test/file/ripgrep-records.test.ts +++ b/packages/opencode/test/file/ripgrep-records.test.ts @@ -132,6 +132,20 @@ describe("RipgrepRecords.parseRecords", () => { expect(fractional[0].submatches).toEqual([]) }) + // A byte offset can fall inside a multi-byte character even on a perfectly valid UTF-8 line. + test("drops a text-arm submatch whose offset splits a multi-byte character", () => { + const parsed = RipgrepRecords.parseRecords([ + record("a.txt", { lines: { text: "éa" }, submatches: [{ match: { text: "a" }, start: 1, end: 3 }] }), + ]) + expect(parsed[0].submatches).toEqual([]) + + // The same line with boundary-aligned offsets is kept. + const ok = RipgrepRecords.parseRecords([ + record("a.txt", { lines: { text: "éa" }, submatches: [{ match: { text: "a" }, start: 2, end: 3 }] }), + ]) + expect(ok[0].submatches).toHaveLength(1) + }) + // Each submatch costs a rebase per endpoint, and a rebase allocates a string up to the line // length, so an unbounded array turns one in-ceiling record into O(count x line) work. test("bounds the submatch count like the core parser", () => { From 466c3cce7b0a76f1030d3aa55e0aa1fc3767b96c Mon Sep 17 00:00:00 2001 From: Haider Date: Fri, 21 Aug 2026 16:05:05 +0530 Subject: [PATCH 11/11] fix(core): linear base64 check, defect-safe normalization, partial rg exits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three P1s from the codex reviewer, all verified before fixing. The first two are regressions this branch introduced; the third is the original bug class in a place it had been missed. - The canonical-base64 pre-filter backtracked catastrophically. Measured on Bun: a canonical 4 MiB body tests FALSE, so valid data was silently discarded, and on Node the same expression raises `RangeError`, which escapes as a defect and aborts the whole search — precisely the failure this branch exists to remove, reintroduced for large non-UTF-8 lines. Replaced with a single character class plus a length-mod-4 check: 16 MiB in ~10ms, and canonical form was already enforced by the round-trip check that follows it. - Normalization ran outside any failure boundary in both parsers. A throw there is a DEFECT, which `Effect.catch` deliberately does not catch, so it aborted the stream instead of skipping one record; the legacy path had the same gap around `normalizeRecord` before `safeParse`. Both are now wrapped. - ripgrep exit 2 means PARTIAL, not fatal: with one `chmod 000` file present it emits a full match record for the readable file and exits 2 (verified). The legacy `search()` discarded stdout on any non-zero code, so one unreadable file threw away every real match. It now accepts 0/1/2 and treats anything else as failure, matching what the core path already did. Tests: 25 core, 134 opencode file. The multi-megabyte-decode cases and the partial-failure case each fail against the pre-fix source. The throw-during-normalization case is a defensive guard rather than a regression test — it passes either way, and is labelled as such. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/ripgrep.ts | 28 +++++++++++-- packages/core/test/ripgrep.test.ts | 17 ++++++++ packages/opencode/src/file/ripgrep-records.ts | 22 ++++++++-- packages/opencode/src/file/ripgrep.ts | 11 ++++- .../test/file/ripgrep-partial.test.ts | 41 +++++++++++++++++++ .../test/file/ripgrep-records.test.ts | 23 +++++++++++ 6 files changed, 133 insertions(+), 9 deletions(-) create mode 100644 packages/opencode/test/file/ripgrep-partial.test.ts diff --git a/packages/core/src/ripgrep.ts b/packages/core/src/ripgrep.ts index 0dad82bd77..0ecfc61e5e 100644 --- a/packages/core/src/ripgrep.ts +++ b/packages/core/src/ripgrep.ts @@ -75,8 +75,21 @@ type RawMatchData = (typeof RawMatch.Type)["data"] // `Stream.mapEffect` — took the whole search down with it, exactly like the oversized record did. // Normalising to the `text` arm up front keeps the schema single-shape and keeps the match usable; // `toString("utf8")` substitutes U+FFFD for the undecodable bytes rather than dropping the match. -/** Canonical base64, so a corrupt field is left to fail decoding rather than silently becoming "". */ -const BASE64 = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/ +/** + * Canonical-base64 pre-filter, deliberately linear. + * + * The earlier `/^(?:[A-Za-z0-9+/]{4})*(?:..)?$/` backtracked catastrophically on + * a large field: measured on Bun, a canonical 4 MiB body returns FALSE (valid + * data silently discarded) and on Node it raises `RangeError`, which escapes as + * a defect and aborts the whole search — the exact failure this change exists to + * remove, reintroduced for large non-UTF-8 lines. A single character class with + * one quantifier has no nested repetition to backtrack: 16 MiB in ~10ms. + * + * Length-mod-4 restores what the `{4}` grouping guaranteed. Exact canonical form + * is still enforced by the round-trip check in the decoder below. + */ +const BASE64_SHAPE = /^[A-Za-z0-9+/]*={0,2}$/ +const isBase64 = (value: string) => value.length % 4 === 0 && BASE64_SHAPE.test(value) /** ripgrep's control records. Anything else with an unrecognised `type` is a protocol surprise. */ const CONTROL_TYPES = new Set(["begin", "end", "summary"]) @@ -102,7 +115,7 @@ const decodeField = (value: unknown): { text: string; raw?: Buffer } | undefined const text = readProp(value, "text") if (typeof text === "string") return { text } const bytes = readProp(value, "bytes") - if (typeof bytes !== "string" || bytes.length === 0 || !BASE64.test(bytes)) return undefined + if (typeof bytes !== "string" || bytes.length === 0 || !isBase64(bytes)) return undefined const raw = Buffer.from(bytes, "base64") if (raw.toString("base64") !== bytes) return undefined return { text: raw.toString("utf8"), raw } @@ -449,7 +462,14 @@ export const layer = Layer.effect( return typeof json.type === "string" && CONTROL_TYPES.has(json.type) ? undefined : yield* Effect.fail(failure(`unrecognised record type ${JSON.stringify(json.type)}`)) - const match = yield* Schema.decodeUnknownEffect(RawMatch)(normalizeMatch(json)).pipe( + // `normalizeMatch` is plain synchronous code. A throw there would be a DEFECT, + // which `Effect.catch` deliberately does not catch — so it would abort the + // stream rather than skip one record, defeating the point of this change. + const normalized = yield* Effect.try({ + try: () => normalizeMatch(json), + catch: (cause) => failure("record could not be normalized", cause), + }) + const match = yield* Schema.decodeUnknownEffect(RawMatch)(normalized).pipe( Effect.mapError((cause) => failure("unexpected match shape", cause)), ) // `normalizeMatch` already caps submatches and line text, so nothing is re-trimmed. diff --git a/packages/core/test/ripgrep.test.ts b/packages/core/test/ripgrep.test.ts index 02e043d0de..ed035611d2 100644 --- a/packages/core/test/ripgrep.test.ts +++ b/packages/core/test/ripgrep.test.ts @@ -428,6 +428,23 @@ describe("Ripgrep", () => { expect(matches[0].submatches[0].text.endsWith("...")).toBe(true) }) + // A canonical multi-MiB bytes field must still decode. The earlier repeated-group + // base64 regex returned false for it on Bun and threw RangeError on Node — the + // second of which escapes as a defect and aborts the entire search. + stubTest( + "decodes a multi-megabyte bytes field rather than discarding the record", + async () => { + const raw = Buffer.concat([Buffer.from([0xff]), Buffer.alloc(5 * 1024 * 1024, 0x61), Buffer.from("needle")]) + const matches = await Effect.runPromise( + grepWithStubbedRecords([matchRecord("big.txt", { lines: { bytes: raw.toString("base64") }, submatches: [] })]), + ) + + expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("big.txt")]) + expect(matches[0].text.endsWith("...")).toBe(true) + }, + 30_000, + ) + stubTest("skips a record whose bytes field is empty rather than emitting an empty match", async () => { const matches = await Effect.runPromise( grepWithStubbedRecords([matchRecord("a.txt"), matchRecord("b.txt", { lines: { bytes: "" } })]), diff --git a/packages/opencode/src/file/ripgrep-records.ts b/packages/opencode/src/file/ripgrep-records.ts index 5006a7648d..71a4798584 100644 --- a/packages/opencode/src/file/ripgrep-records.ts +++ b/packages/opencode/src/file/ripgrep-records.ts @@ -98,7 +98,14 @@ const Result = z.union([Begin, Match, End, Summary]) // stdout up front and hands its records straight to the `/find` response, where the raw ripgrep // shape is the published contract — so it normalises and skips, and leaves the shape alone. Both // report skipped records once per search rather than once per record. -const BASE64 = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/ +// Linear canonical-base64 pre-filter. The earlier repeated-group expression +// backtracked catastrophically on a large field: on Bun a canonical 4 MiB body +// returned FALSE (valid data silently discarded) and on Node it raised +// `RangeError`, which escaped and aborted the whole search. Length-mod-4 +// restores what the `{4}` grouping guaranteed; the round-trip check below is +// what actually enforces canonical form. Mirrors packages/core/src/ripgrep.ts. +const BASE64_SHAPE = /^[A-Za-z0-9+/]*={0,2}$/ +const isBase64 = (value: string) => value.length % 4 === 0 && BASE64_SHAPE.test(value) /** Mirrors packages/core/src/ripgrep.ts. Bounds parse cost per record on this path too. */ const MAX_RECORD_BYTES = 16 * 1024 * 1024 @@ -138,7 +145,7 @@ const normalizeRecord = (line: string): unknown => { // instead of throwing, which would turn a corrupt record into a schema-valid empty match: // reject the empty string (a matched line is never empty), check the spelling, then require // a round-trip so non-canonical padding ("Zh==" and "Zg==" both decode to "f") is rejected. - if (typeof bytes !== "string" || bytes.length === 0 || !BASE64.test(bytes)) return undefined + if (typeof bytes !== "string" || bytes.length === 0 || !isBase64(bytes)) return undefined const decoded = Buffer.from(bytes, "base64") if (decoded.toString("base64") !== bytes) return undefined return { text: decoded.toString("utf8"), raw: decoded } @@ -220,8 +227,15 @@ export function parseRecords(lines: string[]): Match["data"][] { for (const line of lines) { // Bounds parse cost per record. This path buffers all of stdout before splitting, so it does // not bound total memory — that needs streaming, tracked separately. - const parsed = - Buffer.byteLength(line, "utf8") > MAX_RECORD_BYTES ? undefined : Result.safeParse(normalizeRecord(line)) + // `normalizeRecord` runs before `safeParse` and outside any try/catch of its + // own, so a throw inside it would escape `parseRecords` and take the whole + // search down — the failure mode this module exists to prevent. + let parsed: ReturnType | undefined + try { + parsed = Buffer.byteLength(line, "utf8") > MAX_RECORD_BYTES ? undefined : Result.safeParse(normalizeRecord(line)) + } catch { + parsed = undefined + } if (!parsed?.success) { skipped++ continue diff --git a/packages/opencode/src/file/ripgrep.ts b/packages/opencode/src/file/ripgrep.ts index 353c987acc..a4585657c4 100644 --- a/packages/opencode/src/file/ripgrep.ts +++ b/packages/opencode/src/file/ripgrep.ts @@ -304,9 +304,18 @@ export namespace Ripgrep { cwd: input.cwd, nothrow: true, }) - if (result.code !== 0) { + // altimate_change start — upstream_fix: exit 2 is PARTIAL, not fatal. + // ripgrep exits 2 when it could not read something (an unreadable file, a + // broken symlink) while still searching everything else — verified: with one + // `chmod 000` file present it emits a full match record for the readable + // file and exits 2. Discarding stdout on any non-zero code therefore threw + // away real matches because one unrelated file was unreadable, which is the + // same "one bad thing kills the whole search" failure this change removes. + // 0 = matches, 1 = no matches, 2 = partial; anything else is a real failure. + if (result.code !== 0 && result.code !== 1 && result.code !== 2) { return [] } + // altimate_change end // Handle both Unix (\n) and Windows (\r\n) line endings const lines = result.text.trim().split(/\r?\n/).filter(Boolean) diff --git a/packages/opencode/test/file/ripgrep-partial.test.ts b/packages/opencode/test/file/ripgrep-partial.test.ts new file mode 100644 index 0000000000..6e989881f2 --- /dev/null +++ b/packages/opencode/test/file/ripgrep-partial.test.ts @@ -0,0 +1,41 @@ +import { describe, expect, test } from "bun:test" +import fs from "fs/promises" +import os from "os" +import path from "path" +import { Ripgrep } from "../../src/file/ripgrep" + +// altimate_change start — upstream_fix: ripgrep exit 2 is PARTIAL, not fatal. +// `search()` discarded stdout on any non-zero exit, so one unreadable file made +// the whole search return [] even though ripgrep had already emitted matches for +// every readable file. Same "one bad thing kills the search" shape as the record +// bug this branch fixes. Drives the real binary, because the behaviour under +// test is ripgrep's exit code. +// +// POSIX-only: `chmod 000` is how the unreadable file is produced, and it does +// not deny access to root or on Windows. +const canDenyRead = process.platform !== "win32" && typeof process.getuid === "function" && process.getuid() !== 0 + +describe("legacy Ripgrep.search partial failures", () => { + test.skipIf(!canDenyRead)( + "returns matches from readable files when another file cannot be read", + async () => { + const dir = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), "rg-partial-"))) + try { + await fs.writeFile(path.join(dir, "readable.txt"), "needle here\n") + const locked = path.join(dir, "locked.txt") + await fs.writeFile(locked, "needle hidden\n") + await fs.chmod(locked, 0o000) + + const matches = await Ripgrep.search({ cwd: dir, pattern: "needle", limit: 10 }) + + // ripgrep exits 2 here; the readable file's match must survive it. + expect(matches.map((m) => m.path.text.replace(/^\.\//, ""))).toContain("readable.txt") + } finally { + await fs.chmod(path.join(dir, "locked.txt"), 0o644).catch(() => {}) + await fs.rm(dir, { recursive: true, force: true }) + } + }, + 120_000, + ) +}) +// altimate_change end diff --git a/packages/opencode/test/file/ripgrep-records.test.ts b/packages/opencode/test/file/ripgrep-records.test.ts index d2b94a187d..587f8da713 100644 --- a/packages/opencode/test/file/ripgrep-records.test.ts +++ b/packages/opencode/test/file/ripgrep-records.test.ts @@ -154,6 +154,29 @@ describe("RipgrepRecords.parseRecords", () => { expect(parsed[0].submatches).toHaveLength(100) }) + // The earlier repeated-group base64 regex backtracked catastrophically: on Bun a + // canonical 4 MiB body tested FALSE (valid data silently discarded) and on Node + // it raised RangeError, which escaped and aborted the whole search. + test("decodes a multi-megabyte bytes field instead of discarding it", () => { + const raw = Buffer.concat([Buffer.from([0xff]), Buffer.alloc(5 * 1024 * 1024, 0x61), Buffer.from("needle")]) + const parsed = RipgrepRecords.parseRecords([ + record("big.txt", { lines: { bytes: raw.toString("base64") }, submatches: [] }), + ]) + expect(parsed).toHaveLength(1) + expect(parsed[0].lines.text.endsWith("...")).toBe(true) + }) + + test("a record that throws during normalization is skipped, not fatal", () => { + // A getter that throws stands in for any defect inside normalization; the + // point is that it must not escape parseRecords. + const hostile = JSON.stringify({ type: "match", data: { path: { text: "./x" } } }).replace( + '"data":{', + '"data":{"submatches":1e999,', + ) + expect(() => RipgrepRecords.parseRecords([hostile, record("a.txt")])).not.toThrow() + expect(paths([hostile, record("a.txt")])).toEqual(["./a.txt"]) + }) + test("returns an empty array when every record is unusable, rather than throwing", () => { expect(RipgrepRecords.parseRecords(["{oops", "{also oops"])).toEqual([]) })