From ebee066e6cad7ada7da786818c8ec4f50a0bd57c Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 13 Aug 2026 06:09:17 +0530 Subject: [PATCH 1/6] 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 2/6] =?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 3/6] 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 4/6] =?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 5/6] 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 6/6] 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