Skip to content

Commit fe122a9

Browse files
sahrizviclaude
sahrizvi
and
claude
committed
fix(core): address bot review — tally lifetime, legacy offsets, portability
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) <[email protected]>
1 parent 87e9504 commit fe122a9

4 files changed

Lines changed: 209 additions & 154 deletions

File tree

‎packages/core/src/ripgrep.ts‎

Lines changed: 121 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -348,122 +348,129 @@ export const layer = Layer.effect(
348348
Effect.catchTag("Ripgrep.InvalidPatternError", (cause) => Effect.fail(failure(cause.message, cause))),
349349
),
350350
// altimate_change start — upstream_fix: tally skipped records for one aggregate warning.
351-
// Upstream returns `run(...)` directly; the body exists only to hold the tally, which must be
352-
// per invocation and never per layer so two concurrent searches cannot share it.
353-
grep: (input) => {
354-
const skipped: { count: number; samples: string[] } = { count: 0, samples: [] }
355-
return run<RawMatchData>({
356-
// altimate_change end
357-
...input,
358-
args: [
359-
"--no-config",
360-
"--json",
361-
"--hidden",
362-
"--no-messages",
363-
// altimate_change start — upstream_fix: preserve all debug rg search --glob entries
364-
...(typeof input.include === "string"
365-
? [`--glob=${input.include}`]
366-
: (input.include ?? []).map((pattern) => `--glob=${pattern}`)),
367-
// altimate_change end
368-
"--glob=!**/.git/**",
369-
"--",
370-
input.pattern,
371-
input.file ?? ".",
372-
],
373-
// altimate_change start — upstream_fix: a bad record skips itself, never the search.
374-
// `parse` runs inside `Stream.mapEffect`, so ANY failure here aborts the whole stream and
375-
// discards every match already collected from unrelated files. A record is independent of
376-
// its neighbours, so none of the three ways one can be unusable — oversized, unparseable
377-
// JSON, or schema-rejected — justifies destroying the rest of the search.
378-
parse: (line) => {
379-
const bytes = Buffer.byteLength(line, "utf8")
380-
// Captured during the walk so the aggregate warning can name a file when one is
381-
// recoverable. Malformed JSON has no path by definition, hence "when present".
382-
let where: string | undefined
383-
return Effect.gen(function* () {
384-
// Checked before JSON.parse purely to bound parse cost; `Stream.splitLines` has
385-
// already materialized the line, so this cannot bound memory. See MAX_RECORD_BYTES.
386-
if (bytes > MAX_RECORD_BYTES)
387-
return yield* Effect.fail(failure(`record exceeded ${MAX_RECORD_BYTES} bytes`))
388-
const json = yield* Effect.try({
389-
try: () => JSON.parse(line) as unknown,
390-
catch: (cause) => failure("unparseable JSON", cause),
391-
})
392-
if (!json || typeof json !== "object" || !("type" in json))
393-
return yield* Effect.fail(failure("record has no type"))
394-
// Captured before the type check so an unrecognised record can still name its file.
395-
const pathField = readProp(readProp(json, "data"), "path")
396-
const pathText = readProp(pathField, "text")
397-
if (typeof pathText === "string") where = pathText
398-
// Control records are expected and simply carry no match. An unrecognised type is a
399-
// protocol surprise and is counted rather than dropped on the floor, so a ripgrep
400-
// change cannot quietly turn every match into "no matches".
401-
if (json.type !== "match")
402-
return typeof json.type === "string" && CONTROL_TYPES.has(json.type)
403-
? undefined
404-
: yield* Effect.fail(failure(`unrecognised record type ${JSON.stringify(json.type)}`))
405-
const match = yield* Schema.decodeUnknownEffect(RawMatch)(normalizeMatch(json)).pipe(
406-
Effect.mapError((cause) => failure("unexpected match shape", cause)),
351+
// Upstream returns `run(...)` directly. `Effect.suspend` — rather than a plain block body —
352+
// is what makes the tally per EXECUTION: an Effect is a value that can be run more than once
353+
// and concurrently, so a tally captured when the Effect is built would accumulate across runs
354+
// and over-report.
355+
//
356+
// The marked region covers the whole implementation rather than just these lines, because the
357+
// wrapper re-indents every line of the body: an upstream change anywhere in here genuinely
358+
// needs manual reconciliation, which is exactly what the marker is for.
359+
grep: (input) =>
360+
Effect.suspend(() => {
361+
const skipped: { count: number; samples: string[] } = { count: 0, samples: [] }
362+
return run<RawMatchData>({
363+
...input,
364+
args: [
365+
"--no-config",
366+
"--json",
367+
"--hidden",
368+
"--no-messages",
369+
// altimate_change start — upstream_fix: preserve all debug rg search --glob entries
370+
...(typeof input.include === "string"
371+
? [`--glob=${input.include}`]
372+
: (input.include ?? []).map((pattern) => `--glob=${pattern}`)),
373+
// altimate_change end
374+
"--glob=!**/.git/**",
375+
"--",
376+
input.pattern,
377+
input.file ?? ".",
378+
],
379+
// altimate_change start — upstream_fix: a bad record skips itself, never the search.
380+
// `parse` runs inside `Stream.mapEffect`, so ANY failure here aborts the whole stream and
381+
// discards every match already collected from unrelated files. A record is independent of
382+
// its neighbours, so none of the three ways one can be unusable — oversized, unparseable
383+
// JSON, or schema-rejected — justifies destroying the rest of the search.
384+
parse: (line) => {
385+
const bytes = Buffer.byteLength(line, "utf8")
386+
// Captured during the walk so the aggregate warning can name a file when one is
387+
// recoverable. Malformed JSON has no path by definition, hence "when present".
388+
let where: string | undefined
389+
return Effect.gen(function* () {
390+
// Checked before JSON.parse purely to bound parse cost; `Stream.splitLines` has
391+
// already materialized the line, so this cannot bound memory. See MAX_RECORD_BYTES.
392+
if (bytes > MAX_RECORD_BYTES)
393+
return yield* Effect.fail(failure(`record exceeded ${MAX_RECORD_BYTES} bytes`))
394+
const json = yield* Effect.try({
395+
try: () => JSON.parse(line) as unknown,
396+
catch: (cause) => failure("unparseable JSON", cause),
397+
})
398+
if (!json || typeof json !== "object" || !("type" in json))
399+
return yield* Effect.fail(failure("record has no type"))
400+
// Captured before the type check so an unrecognised record can still name its file.
401+
const pathField = readProp(readProp(json, "data"), "path")
402+
const pathText = readProp(pathField, "text")
403+
if (typeof pathText === "string") where = pathText
404+
// Control records are expected and simply carry no match. An unrecognised type is a
405+
// protocol surprise and is counted rather than dropped on the floor, so a ripgrep
406+
// change cannot quietly turn every match into "no matches".
407+
if (json.type !== "match")
408+
return typeof json.type === "string" && CONTROL_TYPES.has(json.type)
409+
? undefined
410+
: yield* Effect.fail(failure(`unrecognised record type ${JSON.stringify(json.type)}`))
411+
const match = yield* Schema.decodeUnknownEffect(RawMatch)(normalizeMatch(json)).pipe(
412+
Effect.mapError((cause) => failure("unexpected match shape", cause)),
413+
)
414+
// `normalizeMatch` already caps submatches and line text, so nothing is re-trimmed.
415+
return { ...match.data, path: { text: match.data.path.text.replace(/^\.[\\/]/, "") } }
416+
}).pipe(
417+
Effect.catch((cause) =>
418+
Effect.sync(() => {
419+
skipped.count++
420+
if (skipped.samples.length < SKIP_SAMPLES)
421+
skipped.samples.push(where ? `${cause.message} (${where})` : cause.message)
422+
return undefined
423+
}),
424+
),
407425
)
408-
// `normalizeMatch` already caps submatches and line text, so nothing is re-trimmed.
409-
return { ...match.data, path: { text: match.data.path.text.replace(/^\.[\\/]/, "") } }
410-
}).pipe(
411-
Effect.catch((cause) =>
412-
Effect.sync(() => {
413-
skipped.count++
414-
if (skipped.samples.length < SKIP_SAMPLES)
415-
skipped.samples.push(where ? `${cause.message} (${where})` : cause.message)
416-
return undefined
417-
}),
418-
),
419-
)
420-
},
421-
// altimate_change end
422-
}).pipe(
423-
// altimate_change start — upstream_fix: one aggregate warning per search, not per record.
424-
// A systematic protocol mismatch rejects every record in the tree, and a per-record log
425-
// would bury the machine in noise while still answering with an innocent-looking empty
426-
// result — the very failure this change exists to prevent.
427-
Effect.tap(() =>
428-
skipped.count > 0
429-
? Effect.logWarning("skipped unusable ripgrep records", {
430-
skipped: skipped.count,
431-
reasons: skipped.samples,
426+
},
427+
// altimate_change end
428+
}).pipe(
429+
// altimate_change start — upstream_fix: one aggregate warning per search, not per record.
430+
// A systematic protocol mismatch rejects every record in the tree, and a per-record log
431+
// would bury the machine in noise while still answering with an innocent-looking empty
432+
// result — the very failure this change exists to prevent.
433+
// `onExit` rather than `tap`: a search that fails or is interrupted part-way is exactly
434+
// when the diagnostic matters, and `tap` would discard the tally in both cases.
435+
Effect.onExit(() =>
436+
skipped.count > 0
437+
? Effect.logWarning("skipped unusable ripgrep records", {
438+
skipped: skipped.count,
439+
reasons: skipped.samples,
440+
})
441+
: Effect.void,
442+
),
443+
// altimate_change end
444+
Effect.map((result) =>
445+
result.items.map((match) => {
446+
const relative = match.path.text
447+
.replace(/^(?:\.[\\/])+/u, "")
448+
.replace(/^[\\/]+/u, "")
449+
.replaceAll("\\", "/")
450+
const absolute = path.resolve(input.cwd, relative)
451+
return new Match({
452+
entry: new Entry({
453+
path: RelativePath.make(relative),
454+
type: "file",
455+
mime: FSUtil.mimeType(absolute),
456+
}),
457+
line: match.line_number,
458+
offset: match.absolute_offset,
459+
// altimate_change start — upstream_fix: capped at parse time, see LINE_TEXT_CAP.
460+
// Re-applied here so the cap still holds if the parser ever stops trimming.
461+
text: capLineText(match.lines.text),
462+
// altimate_change end
463+
submatches: match.submatches.map((submatch) => ({
464+
text: submatch.match.text,
465+
start: submatch.start,
466+
end: submatch.end,
467+
})),
432468
})
433-
: Effect.void,
434-
),
435-
// altimate_change end
436-
Effect.map((result) =>
437-
result.items.map((match) => {
438-
const relative = match.path.text
439-
.replace(/^(?:\.[\\/])+/u, "")
440-
.replace(/^[\\/]+/u, "")
441-
.replaceAll("\\", "/")
442-
const absolute = path.resolve(input.cwd, relative)
443-
return new Match({
444-
entry: new Entry({
445-
path: RelativePath.make(relative),
446-
type: "file",
447-
mime: FSUtil.mimeType(absolute),
448-
}),
449-
line: match.line_number,
450-
offset: match.absolute_offset,
451-
// altimate_change start — upstream_fix: capped at parse time, see LINE_TEXT_CAP.
452-
// Re-applied here so the cap still holds if the parser ever stops trimming.
453-
text: capLineText(match.lines.text),
454-
// altimate_change end
455-
submatches: match.submatches.map((submatch) => ({
456-
text: submatch.match.text,
457-
start: submatch.start,
458-
end: submatch.end,
459-
})),
460-
})
461-
}),
462-
),
463-
// altimate_change start — upstream_fix: closes the block body opened for the skip tally.
464-
)
465-
},
466-
// altimate_change end
469+
}),
470+
),
471+
)
472+
}),
473+
// altimate_change end — closes the grep block opened above for the skip tally.
467474
})
468475
}),
469476
)

0 commit comments

Comments
 (0)