From bf390c829d1721d60eb5febd1db9f19f10abb7f3 Mon Sep 17 00:00:00 2001 From: Vyacheslav Kurilyak <320287930+vkurilyak@users.noreply.github.com> Date: Thu, 3 Sep 2026 12:46:50 +0200 Subject: [PATCH] fix: keep one unparseable file from zeroing the whole review - Limit DCS/OSC stripping to same line so binary-looking bytes cannot eat newlines - Fall back to per-file parsing with visible placeholder when whole-patch parse throws - Add regression coverage for newline guarantee and keep-good-files fallback --- src/core/changeset/fromPatch.test.ts | 52 +++++++++ src/core/changeset/fromPatch.ts | 169 +++++++++++++++++++++++++-- src/core/patch/sanitize.test.ts | 12 ++ src/core/patch/sanitize.ts | 7 +- 4 files changed, 229 insertions(+), 11 deletions(-) create mode 100644 src/core/changeset/fromPatch.test.ts diff --git a/src/core/changeset/fromPatch.test.ts b/src/core/changeset/fromPatch.test.ts new file mode 100644 index 000000000..bf47e7439 --- /dev/null +++ b/src/core/changeset/fromPatch.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, test } from "bun:test"; +import { changesetFromPatch } from "./fromPatch"; + +const goodFile = [ + "diff --git a/notes.txt b/notes.txt", + "new file mode 100644", + "index 0000000000..3b18e512d6", + "--- /dev/null", + "+++ b/notes.txt", + "@@ -0,0 +1,2 @@", + "+hello", + "+world", + "", +].join("\n"); + +// A hunk body whose line counts disagree with its header: unparseable, but it +// must not take the rest of the review down with it. +const poisonFile = [ + "diff --git a/evil.bin b/evil.bin", + "new file mode 100644", + "index 0000000000..1234567890", + "--- /dev/null", + "+++ b/evil.bin", + "@@ -0,0 +1,5 @@", + "+hello", + "+world", + "", +].join("\n"); + +describe("changesetFromPatch", () => { + test("parses a clean multi-file patch unchanged", () => { + const changeset = changesetFromPatch( + `${goodFile}${goodFile.replaceAll("notes.txt", "other.txt")}`, + "title", + "label", + null, + ); + expect(changeset.files.length).toBe(2); + expect(changeset.files[0]?.path).toBe("notes.txt"); + expect(changeset.files[1]?.path).toBe("other.txt"); + }); + + test("keeps good files when one file chunk is unparseable", () => { + // Regression test: a single poisoned file used to make the whole-patch + // parse throw, and the catch-all returned zero files for the review. + // The fallback keeps the good file and a placeholder for the bad one. + const changeset = changesetFromPatch(`${goodFile}${poisonFile}`, "title", "label", null); + expect(changeset.files.length).toBe(2); + expect(changeset.files[0]?.path).toBe("notes.txt"); + expect(changeset.files[1]?.path).toBe("evil.bin"); + }); +}); diff --git a/src/core/changeset/fromPatch.ts b/src/core/changeset/fromPatch.ts index b07a1c354..90dcebd64 100644 --- a/src/core/changeset/fromPatch.ts +++ b/src/core/changeset/fromPatch.ts @@ -9,11 +9,18 @@ * Moved-line capture has to run before sanitizing: Git marks moved lines only through SGR * color, which `sanitizePatch` strips on its way to parser-safe text. */ -import { parsePatchFiles } from "@pierre/diffs"; +import { parsePatchFiles, type FileDiffMetadata } from "@pierre/diffs"; import { buildDiffFile, type BuildDiffFileOptions } from "./diffFile"; import { splitPatchIntoFileChunks, findPatchChunk } from "../patch/chunks"; import { sanitizePatch, stripTerminalControl } from "../patch/sanitize"; -import type { Changeset, DiffLineMoveKind, DiffLineMoveKinds, SidecarContext } from "./model"; +import type { SanitizedGitPatch } from "../patch/gitFormat"; +import type { + Changeset, + DiffFile, + DiffLineMoveKind, + DiffLineMoveKinds, + SidecarContext, +} from "./model"; /** Return SGR parameter strings that Git emitted before one diff line marker. */ function leadingSgrParameters(rawLine: string, expectedSign: "+" | "-") { @@ -138,14 +145,16 @@ export function changesetFromPatch( try { parsedPatches = parsePatchFiles(sanitizedPatchText, "patch", true); } catch { - return { - id: `changeset:${Date.now()}`, - sourceLabel, + // One unparseable file must not discard the whole review: fall back to + // per-file parsing so every salvageable file still renders. + return changesetFromPatchChunks( + sanitizedPatch, + lineMoveKinds, title, - summary: sanitizedPatchText.trim() || undefined, - agentSummary: sidecar?.summary, - files: [], - }; + sourceLabel, + sidecar, + perFileOptions, + ); } const metadataFiles = parsedPatches.flatMap((entry) => entry.files); @@ -182,3 +191,145 @@ export function changesetFromPatch( }), }; } + +/** + * Best-effort display name for a chunk the parser rejected. + * + * Headers reaching this fallback are already sanitized toward the `a/X b/Y` + * shape, so the b-side names the file. This is display-only (the placeholder + * carries no hunks), so an even token split is acceptable where Git itself + * would call the rename genuinely ambiguous. + */ +function displayNameForUnparseableChunk(chunk: string, index: number): string { + const firstLine = chunk.split("\n", 1)[0] ?? ""; + const rest = firstLine.startsWith("diff --git ") ? firstLine.slice("diff --git ".length) : ""; + if (rest) { + const tokens = rest.split(" "); + const secondHalf = tokens.slice(Math.ceil(tokens.length / 2)).join(" "); + const name = secondHalf.replace(/^"?b\//, "").replace(/"$/, ""); + if (name) { + return name; + } + } + return `unparseable-file-${index + 1}`; +} + +/** Build a placeholder for one file chunk that failed to parse, keeping its review slot. */ +function buildSkippedUnparseableFile( + name: string, + index: number, + sourceLabel: string, + sidecar: SidecarContext | null, + perFileOptions?: Pick, + lineMoveKinds?: DiffLineMoveKinds, +): DiffFile { + const metadata: FileDiffMetadata = { + name, + type: "change", + hunks: [], + splitLineCount: 0, + unifiedLineCount: 0, + isPartial: true, + additionLines: [], + deletionLines: [], + cacheKey: `${name}:unparseable-skipped`, + }; + return buildDiffFile( + metadata, + `Skipped file with unparseable diff content: ${name}\n`, + index, + sourceLabel, + sidecar, + { + ...perFileOptions, + pathsAreExact: true, + lineMoveKinds, + }, + ); +} + +/** Re-parse per file chunk after a whole-patch parse failure, preserving good files. */ +function changesetFromPatchChunks( + sanitizedPatch: SanitizedGitPatch, + lineMoveKinds: DiffLineMoveKinds[], + title: string, + sourceLabel: string, + sidecar: SidecarContext | null, + perFileOptions?: Pick, +): Changeset { + const chunks = splitPatchIntoFileChunks(sanitizedPatch.text); + const files: DiffFile[] = []; + const summaries: string[] = []; + let index = 0; + + const pushParsedFile = (metadata: FileDiffMetadata) => { + const decodedPaths = sanitizedPatch.filePaths[index]; + const normalizedMetadata = decodedPaths + ? { ...metadata, name: decodedPaths.path, prevName: decodedPaths.previousPath } + : metadata; + files.push( + buildDiffFile( + normalizedMetadata, + findPatchChunk(metadata, chunks, index), + index, + sourceLabel, + sidecar, + { + ...perFileOptions, + pathsAreExact: Boolean(decodedPaths), + lineMoveKinds: hasLineMoveKinds(lineMoveKinds[index]) ? lineMoveKinds[index] : undefined, + }, + ), + ); + index += 1; + }; + + for (const chunk of chunks) { + let parsed: ReturnType | undefined; + try { + parsed = parsePatchFiles(chunk, "patch", true); + } catch { + parsed = undefined; + } + const chunkFiles = + parsed?.flatMap((entry) => { + if (entry.patchMetadata) { + summaries.push(entry.patchMetadata); + } + return entry.files; + }) ?? []; + if (chunkFiles.length === 0) { + // The chunk could not be parsed at all: keep a visible placeholder in + // its slot so the review stays complete instead of silently dropping it. + // Prefer the sanitizer's exact decoded path, then a best-effort read of + // the (already sanitized) header, then a positional fallback name. + const name = + sanitizedPatch.filePaths[index]?.path ?? + displayNameForUnparseableChunk(chunk, index); + files.push( + buildSkippedUnparseableFile( + name, + index, + sourceLabel, + sidecar, + perFileOptions, + hasLineMoveKinds(lineMoveKinds[index]) ? lineMoveKinds[index] : undefined, + ), + ); + index += 1; + continue; + } + for (const metadata of chunkFiles) { + pushParsedFile(metadata); + } + } + + return { + id: `changeset:${Date.now()}`, + sourceLabel, + title, + summary: summaries.join("\n\n") || undefined, + agentSummary: sidecar?.summary, + files, + }; +} diff --git a/src/core/patch/sanitize.test.ts b/src/core/patch/sanitize.test.ts index 40d190e15..1a82c22f0 100644 --- a/src/core/patch/sanitize.test.ts +++ b/src/core/patch/sanitize.test.ts @@ -38,6 +38,18 @@ describe("stripTerminalControl", () => { expect(stripTerminalControl("a\x1bMb")).toBe("ab"); }); + test("does not let OSC sequences span newlines into binary content", () => { + // Binary bytes resembling ESC ] ... BEL must not eat the newline: the + // lone-escape rule may still take the ESC ] bytes, but the line count + // reaching the patch parser must stay intact. + expect(stripTerminalControl("a\x1b]payload\nmore\x07b")).toBe("apayload\nmore\x07b"); + }); + + test("does not let DCS sequences span newlines into binary content", () => { + // Same newline guarantee for ESC P ... ST-looking binary bytes. + expect(stripTerminalControl("a\x1bPdata\nmore\x1b\\b")).toBe("adata\nmoreb"); + }); + test("leaves text with no control sequences unchanged", () => { expect(stripTerminalControl("plain diff text")).toBe("plain diff text"); }); diff --git a/src/core/patch/sanitize.ts b/src/core/patch/sanitize.ts index 79500efea..43056274d 100644 --- a/src/core/patch/sanitize.ts +++ b/src/core/patch/sanitize.ts @@ -17,8 +17,11 @@ export { escapeUntrackedPatchPath } from "../../lib/patchPath"; /** Remove terminal escape sequences so Git-colored pager input still parses as plain patch text. */ export function stripTerminalControl(text: string) { return text - .replace(/\x1bP[\s\S]*?\x1b\\/g, "") - .replace(/\x1b\][\s\S]*?(?:\x07|\x1b\\)/g, "") + // DCS and OSC sequences terminate on the same line: letting them span + // newlines eats binary bytes (for example ESC ] ... BEL inside a binary + // file a VCS emitted as text) and corrupts hunk line counts downstream. + .replace(/\x1bP[^\n]*?\x1b\\/g, "") + .replace(/\x1b\][^\n]*?(?:\x07|\x1b\\)/g, "") .replace(/\x1b\[[0-?]*[ -/]*[@-~]/g, "") .replace(/\x1b[@-_]/g, ""); }