diff --git a/packages/core/src/ripgrep.ts b/packages/core/src/ripgrep.ts index 99c851ed1b..29a95e3c9a 100644 --- a/packages/core/src/ripgrep.ts +++ b/packages/core/src/ripgrep.ts @@ -18,9 +18,37 @@ 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 +// 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({ type: Schema.Literal("match"), data: Schema.Struct({ @@ -40,6 +68,123 @@ 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}=)?$/ + +/** 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 + +/** + * 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") + 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 } +} + +/** + * 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. + * + * 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 + // Rebasing is a claim about coordinates, so it is only made for an offset actually addressable in + // the raw line. `Buffer.subarray` clamps an out-of-range end and truncates a fractional one rather + // than throwing, so an unchecked rebase would quietly repair a corrupt offset into a + // plausible-looking one — and the schema would not catch it, since `NonNegativeInt` happily + // accepts a number past the end of the line. An unaddressable offset therefore marks the whole + // record corrupt (undefined) so it is skipped and counted. Offsets on the `{text}` arm are left + // alone: no rebasing happens there, so no claim is made and ripgrep's values stand as before. + let corrupt = false + const rebase = (offset: unknown): unknown => { + if (!raw) return offset + if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) { + corrupt = true + return offset + } + // Decoding the prefix in isolation is not the same as taking a prefix of the full decode when + // the offset splits a VALID multi-byte sequence: `Buffer.from("éneedle")` sliced at byte 1 + // decodes to U+FFFD, so the offset would rebase to 3 and point at the wrong character in a line + // that decoded cleanly at that spot. Requiring the prefix to actually prefix the decoded line + // catches that; an offset that lands mid-character is not addressable and marks the record + // corrupt. This costs nothing asymptotically — decoding the prefix is already O(offset). + const prefix = raw.subarray(0, offset).toString("utf8") + if (!lines.text.startsWith(prefix)) { + corrupt = true + return offset + } + return Buffer.byteLength(prefix, "utf8") + } + const submatches = readProp(data, "submatches") + const normalized = { + ...json, + data: { + ...data, + // 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.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, + }, + } + return corrupt ? undefined : normalized +} +// altimate_change end + export class Error extends Schema.TaggedErrorClass()("Ripgrep.Error", { message: Schema.String, cause: Schema.optional(Schema.Defect), @@ -226,71 +371,130 @@ 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. `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) => - 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 ?? ".", - ], - parse: (line) => - (Buffer.byteLength(line, "utf8") > MAX_RECORD_BYTES - ? Effect.fail(failure(`Ripgrep JSON record exceeded ${MAX_RECORD_BYTES} bytes`)) - : Effect.try({ + 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("Invalid ripgrep JSON output", cause), + catch: (cause) => failure("unparseable JSON", 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)), + 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 + }), + ), + ) + }, + // 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, + })), + }) }), ), - }).pipe( - 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, - text: match.lines.text.length > 2_000 ? match.lines.text.slice(0, 2_000) + "..." : match.lines.text, - submatches: match.submatches.map((submatch) => ({ - text: submatch.match.text, - start: submatch.start, - end: submatch.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 da8e7519ce..3cbb4d35a4 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 { beforeEach, describe, expect, test as bunTest } from "bun:test" import fs from "fs/promises" import path from "path" -import { Effect } 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" import { RelativePath } from "@opencode-ai/core/schema" import { tmpdir } from "./fixture/tmpdir" import { testEffect } from "./lib/effect" @@ -87,5 +89,351 @@ 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, + }, + }) + + /** + * 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, + }) + }), + ), + ]) + + // The stub is a POSIX shell script and `chmod` is a no-op on win32, so the spawn cannot work + // 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[]) => + 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), + ), + ), + 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) + + stubTest("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")]) + 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 + // file — that keeps the case independent of whether a given ripgrep build emits the match at all. + stubTest("skips an oversized record and keeps parsing the records after it", async () => { + const matches = await Effect.runPromise( + grepWithStubbedRecords([ + matchRecord("a.txt"), + matchRecord("b-huge.txt", { lines: { text: "needle" + "x".repeat(17 * 1024 * 1024) } }), + matchRecord("c.txt"), + ]), + ) + + expect(matches.map((item) => item.entry.path)).toEqual([RelativePath.make("a.txt"), RelativePath.make("c.txt")]) + }) + + // 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. + stubTest("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. + stubTest("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")]) + }) + + // 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". + 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([ + 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") + }) + + // `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) + }) + + // 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: "" } })]), + ) + + // "" 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")]) + }) + + 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==" } })]), + ) + + 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". + 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" } } }), + 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"]) + }) + + 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. + 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 + + 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. + 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) } })]), + ) + + expect(matches[0].text).toHaveLength(2_003) + expect(matches[0].text.endsWith("...")).toBe(true) + }) + + stubTest("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..da9d12cae4 100644 --- a/packages/opencode/src/file/ripgrep.ts +++ b/packages/opencode/src/file/ripgrep.ts @@ -94,6 +94,134 @@ export namespace Ripgrep { const Result = z.union([Begin, Match, End, Summary]) + // altimate_change start — upstream_fix: tolerate ripgrep's `{bytes}` arm and malformed lines. + // + // This mirrors packages/core/src/ripgrep.ts, but deliberately not in every respect. That parser + // streams, so it caps the retained line text and rebases submatch offsets; this one buffers all of + // stdout up front and hands its records straight to the `/find` response, where the raw ripgrep + // shape is the published contract — so it normalises and skips, and leaves the shape alone. Both + // report skipped records once per search rather than once per record. + const BASE64 = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/ + + /** Mirrors packages/core/src/ripgrep.ts. Bounds parse cost per record on this path too. */ + const MAX_RECORD_BYTES = 16 * 1024 * 1024 + + /** Parse one NDJSON record, rewriting `{bytes: base64}` fields into the `{text}` arm. */ + const normalizeRecord = (line: string): unknown => { + let json: unknown + try { + json = JSON.parse(line) + } catch { + return undefined + } + if (!json || typeof json !== "object") return json + const read = (value: unknown, key: string): unknown => + value !== null && typeof value === "object" && key in value ? Reflect.get(value, key) : undefined + const data = read(json, "data") + if (!data || typeof data !== "object") return json + /** Decode a `{text}`/`{bytes}` field, returning the raw buffer so offsets can be rebased. */ + const decode = (value: unknown): { text: string; raw?: Buffer } | undefined => { + if (!value || typeof value !== "object") return undefined + const text = read(value, "text") + if (typeof text === "string") return { text } + const bytes = read(value, "bytes") + // Guarded three ways because `Buffer.from` decodes unconvertible input to an EMPTY buffer + // instead of throwing, which would turn a corrupt record into a schema-valid empty match: + // reject the empty string (a matched line is never empty), check the spelling, then require + // a round-trip so non-canonical padding ("Zh==" and "Zg==" both decode to "f") is rejected. + if (typeof bytes !== "string" || bytes.length === 0 || !BASE64.test(bytes)) return undefined + const decoded = Buffer.from(bytes, "base64") + if (decoded.toString("base64") !== bytes) return undefined + return { text: decoded.toString("utf8"), raw: decoded } + } + const lines = "lines" in data ? decode(read(data, "lines")) : undefined + // Submatch offsets are BYTE offsets into the RAW line, and a lossy decode widens every + // undecodable byte to a 3-byte U+FFFD — so they must be rebased onto the decoded text's own + // UTF-8 encoding or they no longer locate the match. This response shape is published by the + // `/find` route, so unrebased offsets would be newly wrong output rather than a skipped record. + // Mirrors packages/core/src/ripgrep.ts. + const raw = lines?.raw + // Only rebase an offset addressable in the raw line: `Buffer.subarray` clamps an out-of-range + // end and truncates a fractional one rather than throwing, so an unchecked rebase would quietly + // repair a corrupt offset, and the schema would not catch it (`z.number()` accepts any number). + // An unaddressable offset marks the record corrupt so it is skipped and counted instead. + let corrupt = false + const rebase = (offset: unknown): unknown => { + if (!raw || !lines) return offset + if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) { + corrupt = true + return offset + } + // Decoding the prefix alone differs from a prefix of the full decode when the offset splits a + // VALID multi-byte sequence, so require it to actually prefix the decoded line. See + // packages/core/src/ripgrep.ts for the worked example. + const prefix = raw.subarray(0, offset).toString("utf8") + if (!lines.text.startsWith(prefix)) { + corrupt = true + return offset + } + return Buffer.byteLength(prefix, "utf8") + } + const submatches = read(data, "submatches") + // Only rewrite keys the record actually carries — `begin`/`end`/`summary` records reach here too + // and must keep their exact shape, or the strict union below would reject them. + // `path` is deliberately left alone: decoding it is lossy, and a path is an identifier the + // caller reopens, so a U+FFFD-mangled path names a file that does not exist. Such a record + // stays in the `{bytes}` arm and is skipped. See packages/core/src/ripgrep.ts. + const normalized = { + ...json, + data: { + ...data, + ...(lines ? { lines: { text: lines.text } } : {}), + ...(Array.isArray(submatches) + ? { + submatches: submatches.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")), + } + }), + } + : {}), + }, + } + return corrupt ? undefined : normalized + } + + /** + * Turn ripgrep NDJSON lines into match data, skipping records that cannot be used. + * + * `JSON.parse` + a strict `Result.parse` on every line meant one unusable record threw out of + * `search()` and discarded every match already collected from unrelated files — the same defect + * fixed in packages/core/src/ripgrep.ts. Records are independent, so a bad one is dropped and + * counted. 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 export type Match = z.infer export type Begin = z.infer @@ -374,11 +502,9 @@ 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. + return parseRecords(lines) + // 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..765832fad2 --- /dev/null +++ b/packages/opencode/test/file/ripgrep-search.test.ts @@ -0,0 +1,131 @@ +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", () => { + // Explicit timeout: this drives the real binary, and `search()` resolves it through `state()`, + // which downloads a release archive when `rg` is absent from PATH and from Global.Path.bin. + test( + "returns matches from a file whose matched line is not valid UTF-8", + () => + withRepo(async (dir) => { + await fs.writeFile(path.join(dir, "a-plain.txt"), "needle here\n") + // ripgrep emits `{"bytes": ""}` rather than `{"text": ...}` for this line, which the + // strict Zod schema rejected — taking the unrelated matches down with it. + await fs.writeFile(path.join(dir, "b-binary.txt"), Buffer.from("needle \xff\xfe tail\n", "binary")) + await fs.writeFile(path.join(dir, "c-plain.txt"), "needle here\n") + + const matches = await Ripgrep.search({ cwd: dir, pattern: "needle", limit: 10 }) + + expect(matches.map((match) => match.path.text.replace(/^\.\//, "")).sort()).toEqual([ + "a-plain.txt", + "b-binary.txt", + "c-plain.txt", + ]) + const binary = matches.find((match) => match.path.text.includes("b-binary.txt")) + expect(binary?.lines.text).toContain("needle") + expect(binary?.lines.text).toContain("tail") + }), + 120_000, + ) +}) + +// `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") + }) + + // 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("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([]) + }) +}) +// altimate_change end