-
Notifications
You must be signed in to change notification settings - Fork 134
fix(core): stop one bad ripgrep record from failing the whole search #1094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ebee066
065cb98
87e9504
fe122a9
32cfa33
7eb9528
8cb32e7
953999c
e21dc19
c95f234
e983649
466c3cc
d41c0fa
567d073
f673bd3
0dee0f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,275 @@ | ||||||
| // altimate_change start — upstream_fix: ripgrep NDJSON record parsing, split out of ripgrep.ts. | ||||||
| // | ||||||
| // This lives in its own module rather than inside the `Ripgrep` namespace because it is pure, | ||||||
| // process-free logic that deserves direct tests. Inside the namespace it was reachable only two | ||||||
| // ways, both bad: projected through `export * as` as an implementation detail of the public | ||||||
| // namespace, or driven through `search()` with a stub `rg` on PATH — and that binary lookup is | ||||||
| // memoised per process, so such a stub leaks into every later test file in the same `bun test` | ||||||
| // run and breaks unrelated suites. | ||||||
| // | ||||||
| // Module shape follows packages/opencode/AGENTS.md: flat top-level exports with a self-reexport at | ||||||
| // the bottom, not `export namespace`. | ||||||
| // | ||||||
| // It mirrors packages/core/src/ripgrep.ts, but the two are not identical by design. The core parser | ||||||
| // streams; this one buffers all of stdout before splitting, and hands its records straight to the | ||||||
| // `/find` response. Both cap retained text, bound submatch counts, skip records they cannot use, | ||||||
| // and report the skips once per search rather than once per record. | ||||||
| import z from "zod" | ||||||
| import { Log } from "@/util/log" | ||||||
|
|
||||||
| const log = Log.create({ service: "ripgrep" }) | ||||||
|
|
||||||
| const Stats = z.object({ | ||||||
| elapsed: z.object({ | ||||||
| secs: z.number(), | ||||||
| nanos: z.number(), | ||||||
| human: z.string(), | ||||||
| }), | ||||||
| searches: z.number(), | ||||||
| searches_with_match: z.number(), | ||||||
| bytes_searched: z.number(), | ||||||
| bytes_printed: z.number(), | ||||||
| matched_lines: z.number(), | ||||||
| matches: z.number(), | ||||||
| }) | ||||||
|
|
||||||
| const Begin = z.object({ | ||||||
| type: z.literal("begin"), | ||||||
| data: z.object({ | ||||||
| path: z.object({ | ||||||
| text: z.string(), | ||||||
| }), | ||||||
| }), | ||||||
| }) | ||||||
|
|
||||||
| export const Match = z.object({ | ||||||
| type: z.literal("match"), | ||||||
| data: z.object({ | ||||||
| path: z.object({ | ||||||
| text: z.string(), | ||||||
| }), | ||||||
| lines: z.object({ | ||||||
| text: z.string(), | ||||||
| }), | ||||||
| line_number: z.number(), | ||||||
| absolute_offset: z.number(), | ||||||
| submatches: z.array( | ||||||
| z.object({ | ||||||
| match: z.object({ | ||||||
| text: z.string(), | ||||||
| }), | ||||||
| start: z.number(), | ||||||
| end: z.number(), | ||||||
| }), | ||||||
| ), | ||||||
| }), | ||||||
| }) | ||||||
|
|
||||||
| const End = z.object({ | ||||||
| type: z.literal("end"), | ||||||
| data: z.object({ | ||||||
| path: z.object({ | ||||||
| text: z.string(), | ||||||
| }), | ||||||
| binary_offset: z.number().nullable(), | ||||||
| stats: Stats, | ||||||
| }), | ||||||
| }) | ||||||
|
|
||||||
| const Summary = z.object({ | ||||||
| type: z.literal("summary"), | ||||||
| data: z.object({ | ||||||
| elapsed_total: z.object({ | ||||||
| human: z.string(), | ||||||
| nanos: z.number(), | ||||||
| secs: z.number(), | ||||||
| }), | ||||||
| stats: Stats, | ||||||
| }), | ||||||
| }) | ||||||
|
|
||||||
| const Result = z.union([Begin, Match, End, Summary]) | ||||||
|
|
||||||
| // Tolerating ripgrep's `{bytes}` arm and malformed lines. (The whole file is covered by the | ||||||
| // marker at the top — this module is new, not an edit to upstream code.) | ||||||
| // | ||||||
| // This mirrors packages/core/src/ripgrep.ts, but deliberately not in every respect. That parser | ||||||
| // streams, so it caps the retained line text and rebases submatch offsets; this one buffers all of | ||||||
| // stdout up front and hands its records straight to the `/find` response, where the raw ripgrep | ||||||
| // shape is the published contract — so it normalises and skips, and leaves the shape alone. Both | ||||||
| // report skipped records once per search rather than once per record. | ||||||
| // Linear canonical-base64 pre-filter. The earlier repeated-group expression | ||||||
| // backtracked catastrophically on a large field: on Bun a canonical 4 MiB body | ||||||
| // returned FALSE (valid data silently discarded) and on Node it raised | ||||||
| // `RangeError`, which escaped and aborted the whole search. Length-mod-4 | ||||||
| // restores what the `{4}` grouping guaranteed; the round-trip check below is | ||||||
| // what actually enforces canonical form. Mirrors packages/core/src/ripgrep.ts. | ||||||
| const BASE64_SHAPE = /^[A-Za-z0-9+/]*={0,2}$/ | ||||||
| const isBase64 = (value: string) => value.length % 4 === 0 && BASE64_SHAPE.test(value) | ||||||
|
|
||||||
| /** Mirrors packages/core/src/ripgrep.ts. Bounds parse cost per record on this path too. */ | ||||||
| const MAX_RECORD_BYTES = 16 * 1024 * 1024 | ||||||
|
|
||||||
| // Also mirrors core. Each submatch costs a rebase per endpoint, and a rebase allocates a string up | ||||||
| // to the length of the line, so an unbounded submatch array turns one in-ceiling record into | ||||||
| // O(count x line) work. A protocol change is exactly the shape that would emit one. | ||||||
| const MAX_SUBMATCHES = 100 | ||||||
|
|
||||||
| // `MAX_RECORD_BYTES` bounds ONE input record; it does not bound what the response retains. This | ||||||
| // path buffers all of stdout and returns every match, so without a per-field cap a tree of large | ||||||
| // records still retains — and serialises into the `/find` response — an unbounded amount of text. | ||||||
| // Same cap and elision marker as the core parser, so the two paths agree on what a match shows. | ||||||
| /** Distinct skip reasons kept for the aggregate warning; mirrors the core parser. */ | ||||||
| const SKIP_SAMPLES = 5 | ||||||
|
|
||||||
| const LINE_TEXT_CAP = 2_000 | ||||||
| const capText = (text: string) => (text.length > LINE_TEXT_CAP ? text.slice(0, LINE_TEXT_CAP) + "..." : text) | ||||||
|
|
||||||
| /** Parse one NDJSON record, rewriting `{bytes: base64}` fields into the `{text}` arm. */ | ||||||
| const normalizeRecord = (line: string): unknown => { | ||||||
| let json: unknown | ||||||
| try { | ||||||
| json = JSON.parse(line) | ||||||
| } catch { | ||||||
| return undefined | ||||||
| } | ||||||
| if (!json || typeof json !== "object") return json | ||||||
| const read = (value: unknown, key: string): unknown => | ||||||
| value !== null && typeof value === "object" && key in value ? Reflect.get(value, key) : undefined | ||||||
| const data = read(json, "data") | ||||||
| if (!data || typeof data !== "object") return json | ||||||
| /** Decode a `{text}`/`{bytes}` field, returning the raw buffer so offsets can be rebased. */ | ||||||
| const decode = (value: unknown): { text: string; raw?: Buffer } | undefined => { | ||||||
| if (!value || typeof value !== "object") return undefined | ||||||
| const text = read(value, "text") | ||||||
| if (typeof text === "string") return { text } | ||||||
| const bytes = read(value, "bytes") | ||||||
| // Guarded three ways because `Buffer.from` decodes unconvertible input to an EMPTY buffer | ||||||
| // instead of throwing, which would turn a corrupt record into a schema-valid empty match: | ||||||
| // reject the empty string (a matched line is never empty), check the spelling, then require | ||||||
| // a round-trip so non-canonical padding ("Zh==" and "Zg==" both decode to "f") is rejected. | ||||||
| if (typeof bytes !== "string" || bytes.length === 0 || !isBase64(bytes)) return undefined | ||||||
| const decoded = Buffer.from(bytes, "base64") | ||||||
| if (decoded.toString("base64") !== bytes) return undefined | ||||||
| return { text: decoded.toString("utf8"), raw: decoded } | ||||||
| } | ||||||
| const lines = "lines" in data ? decode(read(data, "lines")) : undefined | ||||||
| // Submatch offsets are BYTE offsets into the RAW line, and a lossy decode widens every | ||||||
| // undecodable byte to a 3-byte U+FFFD — so they must be rebased onto the decoded text's own | ||||||
| // UTF-8 encoding or they no longer locate the match. This response shape is published by the | ||||||
| // `/find` route, so unrebased offsets would be newly wrong output rather than a skipped record. | ||||||
| // Mirrors packages/core/src/ripgrep.ts. | ||||||
| const raw = lines?.raw | ||||||
| // Byte-boundary validation, matching packages/core/src/ripgrep.ts. A decoded-string comparison | ||||||
| // is not sufficient: when an invalid byte precedes a LITERAL U+FFFD, an offset inside that | ||||||
| // character still yields a prefix that prefixes the line, because the replacement characters | ||||||
| // alias. A continuation byte (0b10xxxxxx) at the offset means the split lands inside a sequence. | ||||||
| // An offset that cannot be rebased drops ITS SUBMATCH, not the record — the file, line and text | ||||||
| // stay correct, and losing a highlight range beats losing the match. | ||||||
| const isContinuationByte = (byte: number | undefined) => byte !== undefined && (byte & 0xc0) === 0x80 | ||||||
| // Encoded once per record and shared by every submatch; also supplies the byte length. | ||||||
| const textBytes = raw ?? (lines ? Buffer.from(lines.text, "utf8") : Buffer.alloc(0)) | ||||||
| const rebase = (offset: unknown): number | undefined => { | ||||||
| // `{text}` arm: nothing is rebased, but the offset must still be addressable AND land on a | ||||||
| // character boundary — a byte offset can fall inside a multi-byte sequence (`éa`, offset 1 | ||||||
| // splits `é`). `z.number()` rejects none of that, so a corrupt record would otherwise reach the | ||||||
| // `/find` response with coordinates that index nothing or half a character. | ||||||
| if (!raw) | ||||||
| return typeof offset === "number" && | ||||||
| Number.isInteger(offset) && | ||||||
| offset >= 0 && | ||||||
| offset <= textBytes.length && | ||||||
| !(offset !== 0 && offset !== textBytes.length && isContinuationByte(textBytes[offset])) | ||||||
| ? offset | ||||||
| : undefined | ||||||
| if (typeof offset !== "number" || !Number.isInteger(offset) || offset < 0 || offset > raw.length) return undefined | ||||||
| if (offset !== 0 && offset !== raw.length && isContinuationByte(raw[offset])) return undefined | ||||||
| return Buffer.byteLength(raw.subarray(0, offset).toString("utf8"), "utf8") | ||||||
| } | ||||||
| const submatches = read(data, "submatches") | ||||||
| // Only rewrite keys the record actually carries — `begin`/`end`/`summary` records reach here too | ||||||
| // and must keep their exact shape, or the strict union below would reject them. | ||||||
| // `path` is deliberately left alone: decoding it is lossy, and a path is an identifier the | ||||||
| // caller reopens, so a U+FFFD-mangled path names a file that does not exist. Such a record | ||||||
| // stays in the `{bytes}` arm and is skipped. See packages/core/src/ripgrep.ts. | ||||||
| const normalized = { | ||||||
| ...json, | ||||||
| data: { | ||||||
| ...data, | ||||||
| ...(lines ? { lines: { text: capText(lines.text) } } : {}), | ||||||
| ...(Array.isArray(submatches) | ||||||
| ? { | ||||||
| submatches: submatches.slice(0, MAX_SUBMATCHES).flatMap((submatch) => { | ||||||
| if (!submatch || typeof submatch !== "object") return [submatch] | ||||||
| const match = decode(read(submatch, "match")) | ||||||
| if (!match) return [submatch] | ||||||
| const start = rebase(read(submatch, "start")) | ||||||
| const end = rebase(read(submatch, "end")) | ||||||
| // Endpoints are rebased independently, so ordering is checked explicitly. | ||||||
| if (start === undefined || end === undefined || start > end) return [] | ||||||
| return [{ ...submatch, match: { text: capText(match.text) }, start, end }] | ||||||
| }), | ||||||
| } | ||||||
| : {}), | ||||||
| }, | ||||||
| } | ||||||
| return normalized | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Turn ripgrep NDJSON lines into match data, skipping records that cannot be used. | ||||||
| * | ||||||
| * `JSON.parse` + a strict `Result.parse` on every line meant one unusable record threw out of | ||||||
| * `search()` and discarded every match already collected from unrelated files — the same defect | ||||||
| * fixed in packages/core/src/ripgrep.ts. Records are independent, so a bad one is dropped and | ||||||
| * counted. Pure and process-free, so `test/file/ripgrep-records.test.ts` drives it directly. | ||||||
| */ | ||||||
| export function parseRecords(lines: string[]): Match["data"][] { | ||||||
| const matches: Match["data"][] = [] | ||||||
| let skipped = 0 | ||||||
| const reasons: string[] = [] | ||||||
| const skip = (reason: string) => { | ||||||
| skipped++ | ||||||
| if (reasons.length < SKIP_SAMPLES) reasons.push(reason) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: When five records share one skip reason before a different failure, Prompt for AI agents
Suggested change
|
||||||
| } | ||||||
| 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. | ||||||
| if (Buffer.byteLength(line, "utf8") > MAX_RECORD_BYTES) { | ||||||
| skip("oversized") | ||||||
| continue | ||||||
| } | ||||||
| // `normalizeRecord` runs before `safeParse` and outside any try/catch of its own, which is why | ||||||
| // it is wrapped here: a throw would otherwise escape `parseRecords` and take the whole search | ||||||
| // down — the failure mode this module exists to prevent. | ||||||
| let parsed: ReturnType<typeof Result.safeParse> | undefined | ||||||
| try { | ||||||
| parsed = Result.safeParse(normalizeRecord(line)) | ||||||
| } catch (error) { | ||||||
| skip(`normalization threw: ${error instanceof Error ? error.message : String(error)}`) | ||||||
| continue | ||||||
| } | ||||||
| if (!parsed.success) { | ||||||
| skip(parsed.error.issues[0]?.message ?? "unexpected record shape") | ||||||
| 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 | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: skip warning is count-only — no reason samples. The core parser accumulates up to On the const SKIP_SAMPLES = 5
let skipped = 0
const samples: string[] = []
// inside the loop, on failure:
skipped++
if (samples.length < SKIP_SAMPLES) samples.push(reason)
// at the end:
if (skipped > 0) log.warn("skipped unusable ripgrep records", { skipped, total: lines.length, reasons: samples })Skip reasons on this path would be: |
||||||
| // would make `/find` answer `[]`, which is indistinguishable from an honest "no matches". | ||||||
| // | ||||||
| // The count alone cannot tell those apart, which is the whole point of the warning: `skipped: 47` | ||||||
| // could be one odd binary file or every record failing a changed protocol. The sampled reasons | ||||||
| // are what distinguish them, so this mirrors what the core parser already reports. | ||||||
| if (skipped > 0) log.warn("skipped unusable ripgrep records", { skipped, total: lines.length, reasons }) | ||||||
| return matches | ||||||
| } | ||||||
|
|
||||||
| export type Result = z.infer<typeof Result> | ||||||
| export type Match = z.infer<typeof Match> | ||||||
| export type Begin = z.infer<typeof Begin> | ||||||
| export type End = z.infer<typeof End> | ||||||
| export type Summary = z.infer<typeof Summary> | ||||||
|
|
||||||
| export * as RipgrepRecords from "./ripgrep-records" | ||||||
| // altimate_change end | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a match occurs after the first 2,000 characters of a long single-line file, this always retains only the line prefix while the returned submatch keeps its original byte offsets. The
/findresponse therefore omits the searched text entirely and exposes offsets beyondlines.text, making the result unusable for highlighting—the exact minified/source-map inputs this change aims to support. Cap to a window containing the match and rebase its offsets, or drop submatches that no longer address the returned text.Useful? React with 👍 / 👎.