From 0059db22f8b2dd75debe8a6c3121cc6ec3e8e317 Mon Sep 17 00:00:00 2001 From: Agent Runtime Fixture Date: Tue, 22 Sep 2026 00:12:49 +0100 Subject: [PATCH 1/2] fix(server): truncate unified diffs at hunk boundaries maxLines slicing cut patch text mid-hunk, leaving hunk headers that declared more body lines than were present. @pierre/diffs logged parsePatchContent count mismatches and repaired hunks with shifted boundaries, corrupting the rendered line mapping. Add truncateUnifiedDiff to @mcode/shared: keeps only complete hunks, drops a file header block when none of its hunks fit, and handles no-newline markers, countless/zero-count headers, and blank context lines. Applied to snapshot and git comparison diff reads, including the root-commit fallback. --- .../diffs/snapshots/snapshot-service.ts | 7 +- .../projects/git/git-comparison-service.ts | 10 +- .../src/git/__tests__/truncate-patch.test.ts | 154 ++++++++++++++++++ packages/shared/src/git/truncate-patch.ts | 61 +++++++ packages/shared/src/index.ts | 1 + 5 files changed, 224 insertions(+), 9 deletions(-) create mode 100644 packages/shared/src/git/__tests__/truncate-patch.test.ts create mode 100644 packages/shared/src/git/truncate-patch.ts diff --git a/apps/server/src/features/projects/diffs/snapshots/snapshot-service.ts b/apps/server/src/features/projects/diffs/snapshots/snapshot-service.ts index a85d46388..b1d7213af 100644 --- a/apps/server/src/features/projects/diffs/snapshots/snapshot-service.ts +++ b/apps/server/src/features/projects/diffs/snapshots/snapshot-service.ts @@ -5,6 +5,7 @@ */ import { injectable, inject } from "tsyringe"; +import { truncateUnifiedDiff } from "@mcode/shared"; import * as NodeFSPromises from "node:fs/promises"; import * as NodePath from "node:path"; import * as NodeCrypto from "node:crypto"; @@ -178,9 +179,7 @@ async function executeDiffBatches( return outputs; } -function limitDiffLines(diff: string, maxLines: number | undefined): string { - return maxLines ? diff.split("\n").slice(0, maxLines).join("\n") : diff; -} + function collectDiffStats( outputs: readonly string[], @@ -336,7 +335,7 @@ export class SnapshotService { refAfter, pathspecBatches, ); - return limitDiffLines(outputs.join("\n"), maxLines); + return truncateUnifiedDiff(outputs.join("\n"), maxLines); } catch { return ""; } diff --git a/apps/server/src/features/projects/git/git-comparison-service.ts b/apps/server/src/features/projects/git/git-comparison-service.ts index be4d4290c..f57669e40 100644 --- a/apps/server/src/features/projects/git/git-comparison-service.ts +++ b/apps/server/src/features/projects/git/git-comparison-service.ts @@ -1,5 +1,5 @@ import { inject, injectable } from "tsyringe"; -import { logger } from "@mcode/shared"; +import { logger, truncateUnifiedDiff } from "@mcode/shared"; import type { BranchComparison, GitCommit, @@ -76,13 +76,13 @@ export class GitComparisonService { if (filePath) args.push("--", filePath); const { stdout } = await this.gitExecutor.exec(args, { timeout: 10_000 }); const result = stdout.trim(); - return truncate && maxLines ? result.split("\n").slice(0, maxLines).join("\n") : result; + return truncate ? truncateUnifiedDiff(result, maxLines) : result; }; try { return await readDiff(`${sha}~1..${sha}`, true); } catch { try { - return await readDiff(`${EMPTY_TREE}..${sha}`, false); + return await readDiff(`${EMPTY_TREE}..${sha}`, true); } catch { return ""; } @@ -139,7 +139,7 @@ export class GitComparisonService { try { const { stdout } = await this.gitExecutor.exec(args, { timeout: 10_000 }); const result = stdout.trim(); - return maxLines ? result.split("\n").slice(0, maxLines).join("\n") : result; + return truncateUnifiedDiff(result, maxLines); } catch { return ""; } @@ -205,7 +205,7 @@ export class GitComparisonService { try { const { stdout } = await this.gitExecutor.exec(args, { timeout: 10_000 }); const result = stdout.trim(); - return maxLines ? result.split("\n").slice(0, maxLines).join("\n") : result; + return truncateUnifiedDiff(result, maxLines); } catch { return ""; } diff --git a/packages/shared/src/git/__tests__/truncate-patch.test.ts b/packages/shared/src/git/__tests__/truncate-patch.test.ts new file mode 100644 index 000000000..b8b12acd0 --- /dev/null +++ b/packages/shared/src/git/__tests__/truncate-patch.test.ts @@ -0,0 +1,154 @@ +import { describe, expect, it } from "vitest"; +import { truncateUnifiedDiff } from "../truncate-patch.js"; + +/** Assert every hunk header's declared counts match the body lines present. */ +function expectCompleteHunks(patch: string): void { + const lines = patch.split("\n"); + for (let i = 0; i < lines.length; i++) { + const match = /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/.exec(lines[i]!); + if (!match) continue; + let old = Number(match[2] ?? 1); + let next = Number(match[4] ?? 1); + let j = i + 1; + while (old + next > 0) { + const prefix = lines[j]?.[0]; + expect(prefix, `hunk at line ${i} ends early`).toMatch(/^[-+ ]$/); + if (prefix === " ") { + old--; + next--; + } else if (prefix === "-") old--; + else next--; + j++; + if (lines[j] === "\\ No newline at end of file") j++; + } + } +} + +const PATCH = [ + "diff --git a/a.txt b/a.txt", + "index 1111111..2222222 100644", + "--- a/a.txt", + "+++ b/a.txt", + "@@ -1,3 +1,4 @@", + " one", + "-two", + "+2", + "+2b", + " three", + "@@ -10,3 +11,4 @@", + " ten", + "-eleven", + "+11", + "+11b", + " twelve", + "diff --git a/b.txt b/b.txt", + "--- a/b.txt", + "+++ b/b.txt", + "@@ -1,3 +1,4 @@", + " a", + "-b", + "+c", + "+d", + " e", + "diff --git a/c.txt b/c.txt", + "--- a/c.txt", + "+++ b/c.txt", + "@@ -1,3 +1,4 @@", + " x", + "-y", + "+z", + "+w", + " tail", +].join("\n") + "\n"; + +describe("truncateUnifiedDiff", () => { + it("returns the input when maxLines is undefined or exceeds the length", () => { + expect(truncateUnifiedDiff(PATCH, undefined)).toBe(PATCH); + expect(truncateUnifiedDiff(PATCH, 0)).toBe(PATCH); + expect(truncateUnifiedDiff(PATCH, 10_000)).toBe(PATCH); + }); + + it("drops a hunk cut mid-body and keeps earlier complete hunks", () => { + // Cutting inside b.txt's only hunk drops its headers too. + const lines = PATCH.split("\n"); + const hunkB = lines.indexOf("+++ b/b.txt") + 1; + const truncated = truncateUnifiedDiff(PATCH, hunkB + 3); + expect(truncated).not.toContain("b.txt"); + expect(truncated).toContain("a/a.txt"); + expectCompleteHunks(truncated); + expect(truncated.split("\n").length).toBeLessThanOrEqual(hunkB + 3); + }); + + it("keeps every line budget allows when cuts land on hunk boundaries", () => { + for (let maxLines = 1; maxLines < PATCH.split("\n").length; maxLines++) { + const truncated = truncateUnifiedDiff(PATCH, maxLines); + expect(truncated.split("\n").length).toBeLessThanOrEqual(maxLines); + expectCompleteHunks(truncated); + } + }); + + it("keeps the \\ No newline marker with its hunk", () => { + const noEof = [ + "diff --git a/a.txt b/a.txt", + "--- a/a.txt", + "+++ b/a.txt", + "@@ -1,2 +1,2 @@", + " a", + "-b", + "+c", + "\\ No newline at end of file", + "diff --git a/d.txt b/d.txt", + "--- a/d.txt", + "+++ b/d.txt", + "@@ -1,1 +1,1 @@", + "-q", + "+r", + ].join("\n") + "\n"; + // The marker is line index 7; cutting after it keeps a complete hunk. + const truncated = truncateUnifiedDiff(noEof, 9); + expect(truncated).toContain("\\ No newline at end of file"); + expectCompleteHunks(truncated); + // Cutting before the marker drops the whole hunk and the trailing file. + const tighter = truncateUnifiedDiff(noEof, 7); + expect(tighter).not.toContain("a.txt"); + expect(tighter).not.toContain("d.txt"); + }); + + it("handles headers without counts and zero counts", () => { + const patch = [ + "diff --git a/a.txt b/a.txt", + "--- a/a.txt", + "+++ b/a.txt", + "@@ -1 +1 @@", + "-old", + "+new", + "@@ -5,0 +5 @@", + "+appended", + ].join("\n") + "\n"; + const truncated = truncateUnifiedDiff(patch, 7); + expect(truncated).toContain("@@ -1 +1 @@"); + expect(truncated).not.toContain("@@ -5,0 +5 @@"); + expectCompleteHunks(truncated); + }); + + it("counts bare empty lines as context (diff.suppressBlankEmpty)", () => { + const patch = [ + "diff --git a/a.txt b/a.txt", + "--- a/a.txt", + "+++ b/a.txt", + "@@ -1,3 +1,3 @@", + " one", + "", + " three", + "diff --git a/b.txt b/b.txt", + "--- a/b.txt", + "+++ b/b.txt", + "@@ -1,1 +1,1 @@", + "-x", + "+y", + ].join("\n") + "\n"; + const truncated = truncateUnifiedDiff(patch, 8); + expect(truncated).toContain("a/a.txt"); + expect(truncated).not.toContain("b.txt"); + }); +}); diff --git a/packages/shared/src/git/truncate-patch.ts b/packages/shared/src/git/truncate-patch.ts new file mode 100644 index 000000000..0ea9350c8 --- /dev/null +++ b/packages/shared/src/git/truncate-patch.ts @@ -0,0 +1,61 @@ +const HUNK_HEADER = /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/; +const NO_EOF_NEWLINE = "\\ No newline at end of file"; + +/** + * Truncate a unified diff to at most `maxLines` lines without splitting a + * hunk. A raw line slice can leave a hunk body shorter than its header + * declares; `@pierre/diffs` treats that as malformed and repairs the hunk + * with shifted boundaries, so the rendered line mapping drifts. Only + * complete hunks are kept, and a file header block is emitted only when at + * least one of its hunks survives. + */ +export function truncateUnifiedDiff(diff: string, maxLines: number | undefined): string { + if (!maxLines || maxLines <= 0) return diff; + const lines = diff.split("\n"); + if (lines.length <= maxLines) return diff; + let end = 0; + let i = 0; + while (i < maxLines) { + if (!lines[i]!.startsWith("@@ ")) { + i++; + continue; + } + const body = hunkBodyLength(lines, i); + if (body === undefined || i + 1 + body > maxLines) break; + i += 1 + body; + end = i; + } + return lines.slice(0, end).join("\n"); +} + +/** Body line count of the hunk at `headerIndex`, including `\ No newline` markers. */ +function hunkBodyLength(lines: readonly string[], headerIndex: number): number | undefined { + const match = HUNK_HEADER.exec(lines[headerIndex]!); + if (!match) return undefined; + const remaining = { old: Number(match[2] ?? 1), new: Number(match[4] ?? 1) }; + let i = headerIndex + 1; + while (remaining.old + remaining.new > 0) { + if (!consumeHunkLine(lines[i], remaining)) return undefined; + i++; + if (lines[i] === NO_EOF_NEWLINE) i++; + } + return i - headerIndex - 1; +} + +/** Debit the declared hunk counts for one body line; false when the line cannot belong to the hunk. */ +function consumeHunkLine(line: string | undefined, remaining: { old: number; new: number }): boolean { + // `diff.suppressBlankEmpty` and external diff drivers emit a truly empty + // line for blank context; `git apply` counts it as context too. + const prefix = line === "" ? " " : line?.[0]; + if (prefix === " ") { + remaining.old--; + remaining.new--; + } else if (prefix === "-") { + remaining.old--; + } else if (prefix === "+") { + remaining.new--; + } else { + return false; + } + return remaining.old >= 0 && remaining.new >= 0; +} diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 8270f71eb..536ab9cba 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -17,6 +17,7 @@ export { // Git utilities export { createTextPatch } from "./git/text-patch.js"; +export { truncateUnifiedDiff } from "./git/truncate-patch.js"; export { validateWorktreeName, validateBranchName, From 684a6f0106e13dc0fdab7ba2d7aef53e4d1b8895 Mon Sep 17 00:00:00 2001 From: Agent Runtime Fixture Date: Tue, 22 Sep 2026 00:12:56 +0100 Subject: [PATCH 2/2] fix(web): keep FileDiffMetadata identity stable in ReviewDiffView The fileDiffs memo re-parsed every patch into a fresh FileDiffMetadata on any patches change while stamping the same cacheKey. Pierre's areDiffTargetsEqual treats same-key objects as one render target, so the virtualizer could prepare layout for the old object and commit the new one, tripping the 'rendered a different diff than its prepared layout' assertion during scroll and resize and crashing the panel. FileDiffCache reuses the parsed object while (scope, path, patch) are unchanged and gives every fresh parse a unique serial key, keeping object identity and render-target identity equivalent. --- .../src/components/diff/ReviewDiffView.tsx | 18 +++--- .../src/lib/__tests__/file-diff-cache.test.ts | 57 +++++++++++++++++++ apps/web/src/lib/file-diff-cache.ts | 26 +++++++++ 3 files changed, 93 insertions(+), 8 deletions(-) create mode 100644 apps/web/src/lib/__tests__/file-diff-cache.test.ts create mode 100644 apps/web/src/lib/file-diff-cache.ts diff --git a/apps/web/src/components/diff/ReviewDiffView.tsx b/apps/web/src/components/diff/ReviewDiffView.tsx index aea9e95c8..f720d8112 100644 --- a/apps/web/src/components/diff/ReviewDiffView.tsx +++ b/apps/web/src/components/diff/ReviewDiffView.tsx @@ -1,6 +1,5 @@ import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { - parsePatchFiles, type DiffLineAnnotation, type FileContents, type FileDiffMetadata, @@ -21,6 +20,7 @@ import { import type { ReviewFileChange } from "@mcode/contracts"; import { getTransport } from "@/transport"; import { loadFileDiff } from "@/lib/load-file-diff"; +import { FileDiffCache } from "@/lib/file-diff-cache"; import { parseDiffLines, isMarkdownFile } from "@/lib/diff-parser"; import { parseFirstHunkLine } from "@/lib/parse-first-hunk-line"; import { useShikiTheme } from "@/hooks/useTheme"; @@ -298,18 +298,20 @@ export function ReviewDiffView({ [expanded, editingAnnotation], ); + // Reusing the parsed object keeps each CodeView item's render target + // identical across unrelated state changes; a fresh object under the same + // cacheKey makes pierre's virtualizer commit a diff its layout pass never + // prepared. + const diffCache = useRef(new FileDiffCache()).current; const fileDiffs = useMemo(() => { const out: Record = {}; + const scope = `${threadId}:${source}:${id}:${cacheVersion}`; for (const [path, patch] of Object.entries(patches)) { - const parsed = parsePatchFiles(patch).flatMap((p) => p.files); - if (parsed.length > 0) { - const fileDiff = parsed[0]!; - fileDiff.cacheKey = `${threadId}:${source}:${id}:${cacheVersion}:${path}:${patch.length}`; - out[path] = fileDiff; - } + const fileDiff = diffCache.get(scope, path, patch); + if (fileDiff) out[path] = fileDiff; } return out; - }, [patches, id, source, threadId, cacheVersion]); + }, [patches, id, source, threadId, cacheVersion, diffCache]); // Lazy-load the patch for every expanded file missing one. useEffect(() => { diff --git a/apps/web/src/lib/__tests__/file-diff-cache.test.ts b/apps/web/src/lib/__tests__/file-diff-cache.test.ts new file mode 100644 index 000000000..eaf1459fe --- /dev/null +++ b/apps/web/src/lib/__tests__/file-diff-cache.test.ts @@ -0,0 +1,57 @@ +import { describe, it, expect } from "vitest"; +import { FileDiffCache } from "../file-diff-cache"; + +const PATCH = `diff --git a/a.ts b/a.ts +index 0000000..1111111 100644 +--- a/a.ts ++++ b/a.ts +@@ -1,2 +1,2 @@ + context +-old ++new`; + +const PATCH_2 = `diff --git a/a.ts b/a.ts +index 0000000..1111111 100644 +--- a/a.ts ++++ b/a.ts +@@ -1,2 +1,2 @@ + context +-old ++newer`; + +describe("FileDiffCache", () => { + it("returns the same object while the patch is unchanged", () => { + const cache = new FileDiffCache(); + const first = cache.get("s", "a.ts", PATCH)!; + expect(cache.get("s", "a.ts", PATCH)).toBe(first); + }); + + it("re-parses with a distinct cacheKey when the patch changes", () => { + const cache = new FileDiffCache(); + const first = cache.get("s", "a.ts", PATCH)!; + const second = cache.get("s", "a.ts", PATCH_2)!; + expect(second).not.toBe(first); + expect(second.cacheKey).not.toBe(first.cacheKey); + }); + + it("re-parses with a distinct cacheKey when the scope changes", () => { + const cache = new FileDiffCache(); + const first = cache.get("s1", "a.ts", PATCH)!; + const second = cache.get("s2", "a.ts", PATCH)!; + expect(second).not.toBe(first); + expect(second.cacheKey).not.toBe(first.cacheKey); + }); + + it("does not reuse entries across paths", () => { + const cache = new FileDiffCache(); + const a = cache.get("s", "a.ts", PATCH)!; + const b = cache.get("s", "b.ts", PATCH)!; + expect(a).not.toBe(b); + expect(a.cacheKey).not.toBe(b.cacheKey); + }); + + it("returns undefined for a patch with no files", () => { + const cache = new FileDiffCache(); + expect(cache.get("s", "a.ts", "not a diff")).toBeUndefined(); + }); +}); diff --git a/apps/web/src/lib/file-diff-cache.ts b/apps/web/src/lib/file-diff-cache.ts new file mode 100644 index 000000000..2384887e0 --- /dev/null +++ b/apps/web/src/lib/file-diff-cache.ts @@ -0,0 +1,26 @@ +import { parsePatchFiles, type FileDiffMetadata } from "@pierre/diffs"; + +/** + * Parses per-file patches into pierre FileDiffMetadata with stable object + * identity. pierre's areDiffTargetsEqual treats two objects sharing a + * cacheKey as one render target: handing a CodeView item a re-parsed copy of + * the same patch under the same key lets its virtualizer commit a different + * object than the prepared layout, which throws inside render. Reuse the + * parsed object while the patch is unchanged, and give every fresh parse a + * unique key so object identity and render-target identity stay equivalent. + */ +export class FileDiffCache { + private readonly entries = new Map(); + private serial = 0; + + /** Parse `patch` for `path`, reusing the previous result when nothing changed. */ + get(scope: string, path: string, patch: string): FileDiffMetadata | undefined { + const cached = this.entries.get(path); + if (cached && cached.scope === scope && cached.patch === patch) return cached.fileDiff; + const fileDiff = parsePatchFiles(patch).flatMap((file) => file.files)[0]; + if (!fileDiff) return undefined; + fileDiff.cacheKey = `${scope}:${path}:${++this.serial}`; + this.entries.set(path, { scope, patch, fileDiff }); + return fileDiff; + } +}