fix: degrade change review instead of failing

src/review-checkpoints.ts

@@ -108,7 +108,14 @@ export function createReviewCheckpointManager(): ReviewCheckpointManager {
       const baselineRef = effectiveSince === "workspace_open" ? state.openRef : state.baselineRef;
       const baseline = (await git(state.gitRoot, ["rev-parse", "--verify", `${baselineRef}^{commit}`])).stdout.trim();
       const current = await createWorkingTreeSnapshot(state.gitRoot);
-      const patch = (await git(state.gitRoot, ["diff", "--binary", "--no-color", baseline, current], {
+      const patch = (await git(state.gitRoot, [
+        "diff",
+        "--no-color",
+        "--no-ext-diff",
+        "--no-textconv",
+        baseline,
+        current,
+      ], {
         maxBuffer: 50 * 1024 * 1024,
       })).stdout;
       const numstat = (await git(state.gitRoot, ["diff", "--numstat", "-z", baseline, current], {

src/server.ts

@@ -49,7 +49,10 @@ import {
   type McpSessionCloseResult,
 } from "./mcp-sessions.js";
 import { ProcessSessionManager, type ProcessSnapshot } from "./process-sessions.js";
-import { createReviewCheckpointManager } from "./review-checkpoints.js";
+import {
+  createReviewCheckpointManager,
+  type ReviewChangesResult,
+} from "./review-checkpoints.js";
 import { openAiConversationScopeId } from "./request-meta.js";
 import { shutdownHttpServer } from "./server-shutdown.js";
 import { formatPathForPrompt } from "./skills.js";
@@ -1308,11 +1311,39 @@ export function createMcpServer(
       async ({ workspaceId }) => {
         const startedAt = performance.now();
         const workspace = workspaces.getWorkspace(workspaceId);
-        const review = await reviewCheckpoints.reviewChanges({
-          workspaceId,
-          root: workspace.root,
-          markReviewed: true,
-        });
+
+        let review: ReviewChangesResult;
+        try {
+          review = await reviewCheckpoints.reviewChanges({
+            workspaceId,
+            root: workspace.root,
+            markReviewed: true,
+          });
+        } catch (error) {
+          const message = error instanceof Error ? error.message : String(error);
+          const content = [textBlock(`show_changes failed: ${message}`)];
+          logFailedToolResponse(config, {
+            tool: "show_changes",
+            workspaceId,
+          }, content, startedAt);
+          return {
+            isError: true,
+            content,
+            _meta: {
+              tool: "show_changes",
+              card: {
+                workspaceId,
+                summary: { files: 0, additions: 0, removals: 0 },
+                files: [],
+                payload: {},
+                error: message,
+              },
+            },
+            structuredContent: {
+              result: contentText(content),
+            },
+          };
+        }
 
         const content = [textBlock(review.result)];
         logToolCall(config, {

src/ui/card-types.ts

@@ -42,6 +42,7 @@ export interface ToolResultCard {
     managed?: boolean;
   };
   status?: string;
+  error?: string;
   summary?: Record<string, unknown>;
   files?: Array<{
     path?: string;
@@ -180,7 +181,9 @@ export function isExpandableCard(card: ToolResultCard): boolean {
     );
   }
 
-  if (isReviewTool(card.tool)) return Boolean(card.files?.length || card.payload?.patch);
+  if (isReviewTool(card.tool)) {
+    return Boolean(card.files?.length || card.payload?.patch || card.error);
+  }
   if (isPatchTool(card.tool)) return Boolean(card.payload?.patch);
 
   return Boolean(card.payload);

src/ui/patch-display.test.ts

@@ -4,6 +4,7 @@ import {
   getPatchDisplayParts,
   getRenderedFileChangeKind,
   getRenderedFileChangePathDisplay,
+  parseReviewPatchFiles,
 } from "./patch-display.js";
 
 assert.deepEqual(getPatchDisplayParts({}), {
@@ -203,3 +204,103 @@ assert.deepEqual(
     tone: "edit",
   },
 );
+
+const reviewPatch = `diff --git a/src/a.ts b/src/a.ts
+index 1111111..2222222 100644
+--- a/src/a.ts
++++ b/src/a.ts
+@@ -1,3 +1,3 @@
+-const x = 1;
++const x = 2;
+`;
+
+const parsedReview = parseReviewPatchFiles(reviewPatch);
+assert.equal(parsedReview.ok, true);
+assert.equal(parsedReview.files.length, 1);
+assert.equal(parsedReview.files[0]?.name, "src/a.ts");
+assert.equal(parsedReview.files[0]?.hunks.length, 1);
+assert.equal(parsedReview.files[0]?.hunks[0]?.additionLines, 1);
+assert.equal(parsedReview.files[0]?.hunks[0]?.deletionLines, 1);
+
+assert.deepEqual(parseReviewPatchFiles(undefined), { files: [], binaryFiles: new Set(), ok: true });
+{
+  const whitespaceOnly = parseReviewPatchFiles("   \n  ");
+  assert.deepEqual(whitespaceOnly.files, []);
+  assert.equal(whitespaceOnly.ok, true);
+}
+assert.equal(parseReviewPatchFiles("garbage that is not a patch").ok, true);
+
+const crlfReviewPatch = reviewPatch.replace(/\n/g, "\r\n");
+assert.equal(parseReviewPatchFiles(crlfReviewPatch).ok, true);
+assert.equal(parseReviewPatchFiles(crlfReviewPatch).files.length, 1);
+
+const binaryPatch = `diff --git a/logo.png b/logo.png
+index 1111111..2222222 100644
+Binary files a/logo.png and b/logo.png differ
+`;
+{
+  const parsed = parseReviewPatchFiles(binaryPatch);
+  assert.equal(parsed.ok, true);
+  assert.equal(parsed.files.length, 1);
+  assert.equal(parsed.files[0]?.hunks.length, 0);
+  assert.deepEqual([...parsed.binaryFiles], ["logo.png"]);
+}
+
+const renamePatch = `diff --git a/old.txt b/new.txt
+similarity index 100%
+rename from old.txt
+rename to new.txt
+`;
+{
+  const parsed = parseReviewPatchFiles(renamePatch);
+  assert.equal(parsed.ok, true);
+  assert.equal(parsed.files[0]?.type, "rename-pure");
+  assert.deepEqual([...parsed.binaryFiles], []);
+}
+
+const modePatch = `diff --git a/run.sh b/run.sh
+old mode 100644
+new mode 100755
+`;
+{
+  const parsed = parseReviewPatchFiles(modePatch);
+  assert.equal(parsed.ok, true);
+  assert.equal(parsed.files[0]?.hunks.length, 0);
+  assert.equal(parsed.files[0]?.prevMode, "100644");
+  assert.equal(parsed.files[0]?.mode, "100755");
+  assert.deepEqual([...parsed.binaryFiles], []);
+}
+
+const trailingSpacePatch = `diff --git a/f.txt b/f.txt
+index 1111111..2222222 100644
+--- a/f.txt
++++ b/f.txt
+@@ -1 +1 @@
+-old
++new 
+`;
+{
+  const parsed = parseReviewPatchFiles(trailingSpacePatch);
+  assert.equal(parsed.ok, true);
+  assert.equal(parsed.files[0]?.additionLines[0], "new ");
+}
+
+const binarySpacePathPatch = `diff --git a/logo mark.png b/logo mark.png
+index 1111111..2222222 100644
+Binary files a/logo mark.png and b/logo mark.png differ
+`;
+{
+  const parsed = parseReviewPatchFiles(binarySpacePathPatch);
+  assert.equal(parsed.ok, true);
+  assert.deepEqual([...parsed.binaryFiles], ["logo mark.png"]);
+}
+
+const quotedBinaryPatch = `diff --git a/"logo mark.png" b/"logo mark.png"
+index 1111111..2222222 100644
+Binary files a/"logo mark.png" and b/"logo mark.png" differ
+`;
+{
+  const parsed = parseReviewPatchFiles(quotedBinaryPatch);
+  assert.equal(parsed.ok, true);
+  assert.deepEqual([...parsed.binaryFiles], ["logo mark.png"]);
+}

src/ui/patch-display.ts

@@ -1,5 +1,12 @@
+import { parsePatchFiles, type FileDiffMetadata } from "@pierre/diffs";
 import type { ToolResultCard } from "./card-types.js";
 
+export interface ReviewPatchParse {
+  files: FileDiffMetadata[];
+  binaryFiles: Set<string>;
+  ok: boolean;
+}
+
 export type FileChangeKind =
   | "added"
   | "edited"
@@ -160,6 +167,38 @@ export function fileChangeKindLabel(kind: FileChangeKind): string {
   return kind === "unknown" ? "Changed" : fileChangeLabels[kind];
 }
 
+/**
+ * Parse a review patch without ever throwing, so a malformed or unexpected
+ * patch cannot take down the whole card render. A parse failure surfaces as
+ * `ok: false` so the caller can fall back to a file-summary-only card.
+ *
+ * Diffs without hunks are normal for renames, mode changes, and binary
+ * files. Binary files are detected from their explicit git markers
+ * ("Binary files ... differ" or "GIT binary patch") so hunkless renames and
+ * mode changes are not mistaken for binary content.
+ */
+export function parseReviewPatchFiles(patch: string | undefined): ReviewPatchParse {
+  if (!patch) return { files: [], binaryFiles: new Set(), ok: true };
+  const normalized = patch.replace(/\r\n/g, "\n").replace(/^\n+|\n+$/g, "");
+  if (/^\s*$/.test(normalized)) return { files: [], binaryFiles: new Set(), ok: true };
+
+  try {
+    const files = parsePatchFiles(normalized, "review", true).flatMap(
+      (parsedPatch) => parsedPatch.files,
+    );
+    const binaryFiles = new Set<string>();
+    for (const section of patch.split(/^diff --git /m)) {
+      if (!section || !/Binary files|GIT binary patch/.test(section)) continue;
+      const firstLine = section.split("\n")[0] ?? "";
+      const binaryPath = firstLine.match(/ b\/(.+?)\s*$/)?.[1]?.replace(/^"|"$/g, "");
+      if (binaryPath) binaryFiles.add(binaryPath);
+    }
+    return { files, binaryFiles, ok: true };
+  } catch {
+    return { files: [], binaryFiles: new Set(), ok: false };
+  }
+}
+
 function countChangedFiles(files: NonNullable<ToolResultCard["files"]>): number {
   const paths = new Set<string>();
   let unnamedFiles = 0;