diff --git a/src/review-checkpoints.ts b/src/review-checkpoints.ts index 0fd8bf36..467b80f4 100644 --- a/src/review-checkpoints.ts +++ b/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], { diff --git a/src/server.ts b/src/server.ts index 840594ab..10527b1f 100644 --- a/src/server.ts +++ b/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, { diff --git a/src/ui/card-types.ts b/src/ui/card-types.ts index 3d238083..a0383922 100644 --- a/src/ui/card-types.ts +++ b/src/ui/card-types.ts @@ -42,6 +42,7 @@ export interface ToolResultCard { managed?: boolean; }; status?: string; + error?: string; summary?: Record; 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); diff --git a/src/ui/patch-display.test.ts b/src/ui/patch-display.test.ts index 612809ff..b39c3b17 100644 --- a/src/ui/patch-display.test.ts +++ b/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"]); +} diff --git a/src/ui/patch-display.ts b/src/ui/patch-display.ts index ec1f7ad2..490a722b 100644 --- a/src/ui/patch-display.ts +++ b/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; + 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(); + 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): number { const paths = new Set(); let unnamedFiles = 0; diff --git a/src/ui/review-payload.tsx b/src/ui/review-payload.tsx index 455e5472..120c2a77 100644 --- a/src/ui/review-payload.tsx +++ b/src/ui/review-payload.tsx @@ -1,12 +1,15 @@ import { useMemo, useState } from "react"; import { createRoot } from "react-dom/client"; -import { parsePatchFiles, type FileDiffMetadata, type FileDiffOptions } from "@pierre/diffs"; +import { type FileDiffMetadata, type FileDiffOptions } from "@pierre/diffs"; import { FileDiff } from "@pierre/diffs/react"; import type { HostContext, ToolResultCard } from "./card-types.js"; import { fileChangeKindLabel, - getRenderedFileChangePathDisplay, + getFileChangeKind, + getFileChangePathDisplay, getRenderedFileChangeKind, + getRenderedFileChangePathDisplay, + parseReviewPatchFiles, type FileChangeKind, } from "./patch-display.js"; import { pierrePrettyScrollbarCss } from "./scrollbar.js"; @@ -50,23 +53,44 @@ function ReviewPayload({ }: PayloadRendererOptions) { const patch = card.payload?.patch; const themeType: ThemeType = hostContext?.theme === "light" ? "light" : "dark"; - const files = useMemo(() => parseFiles(patch), [patch]); + const reviewParse = useMemo(() => parseReviewPatchFiles(patch), [patch]); + const files = reviewParse.files; const visibleFiles = typeof visibleFileCount === "number" ? files.slice(0, visibleFileCount) : files; const [openFiles, setOpenFiles] = useState(() => new Set()); if (errorMessage) return ; - if (!patch) return ; - if (files.length === 0) return ; + if (card.error) return ; + const cardFiles = card.files ?? []; + if (!patch) { + if (cardFiles.length === 0) return ; + return ; + } + if (!reviewParse.ok) return ; + if (files.length === 0) { + if (cardFiles.length === 0) return ; + return ; + } const options = diffOptions(themeType); + const binaryFiles = reviewParse.binaryFiles; if (files.length === 1) { + const fileDiff = files[0]; + if (fileDiff.hunks.length === 0) { + return ( + + ); + } return (
@@ -101,58 +125,103 @@ function ReviewPayload({ return (
- + {isOpen ? ( + + ) : null} + + )} +
+ ); + })} + {cardFiles + .filter((cardFile) => ( + !files.some((fileDiff) => ( + fileDiff.name === cardFile.path || + fileDiff.name === cardFile.previousPath + )) + )) + .map((cardFile, index) => ( +
+ +
+ ))} +
+ + ); +} + +function FallbackReviewList({ card }: { card: ToolResultCard }) { + const files = card.files ?? []; + if (files.length === 0) { + return ; + } + + return ( +
+
+ Diff preview is unavailable — showing the changed files instead. +
+
+ {files.map((file, index) => { + const kind = getFileChangeKind(file); + const pathDisplay = getFileChangePathDisplay(file); + return ( +
+
+ - +{stats.additions} - -{stats.removals} + +{file.additions ?? 0} + -{file.removals ?? 0} - - {isOpen ? ( - - ) : null} +
); })} @@ -161,6 +230,120 @@ function ReviewPayload({ ); } +function BinaryFileList({ + files, + card, + note, +}: { + files: FileDiffMetadata[]; + card: ToolResultCard; + note?: string; +}) { + return ( +
+
+ {files.map((fileDiff, index) => { + const changeKind = getRenderedFileChangeKind( + card.files ?? [], + { path: fileDiff.name, previousPath: fileDiff.prevName, type: fileDiff.type }, + index, + ); + const pathDisplay = getRenderedFileChangePathDisplay( + card.files ?? [], + { path: fileDiff.name, previousPath: fileDiff.prevName }, + index, + ); + return ( +
+ +
+ ); + })} +
+
+ ); +} + +function FileSummaryRow({ + kind, + pathDisplay, + name, + additions, + removals, + note, +}: { + kind: FileChangeKind; + pathDisplay: ReturnType; + name: string; + additions: number; + removals: number; + note?: string; +}) { + const row = ( +
+ + + +{additions} + -{removals} + +
+ ); + return ( +
+ {row} + {note ?
{note}
: null} +
+ ); +} + +function FileSummaryLabel({ + kind, + pathDisplay, + name, +}: { + kind: FileChangeKind; + pathDisplay: ReturnType; + name: string; +}) { + return ( + <> + + {fileChangeSymbol(kind)} + + {pathDisplay?.previous ? ( + + + {pathDisplay.previous} + + → + + {pathDisplay.current} + + + ) : ( + + {pathDisplay?.current ?? name} + + )} + + ); +} + function fileChangeSymbol(kind: FileChangeKind): string { switch (kind) { case "added": @@ -177,11 +360,6 @@ function fileChangeSymbol(kind: FileChangeKind): string { } } -function parseFiles(patch: string | undefined): FileDiffMetadata[] { - if (!patch) return []; - return parsePatchFiles(patch, "review", true).flatMap((parsedPatch) => parsedPatch.files); -} - function diffStats(fileDiff: FileDiffMetadata): { additions: number; removals: number } { return fileDiff.hunks.reduce( (stats, hunk) => ({ diff --git a/src/ui/workspace-app.css b/src/ui/workspace-app.css index bee72228..4f9d24ec 100644 --- a/src/ui/workspace-app.css +++ b/src/ui/workspace-app.css @@ -772,6 +772,31 @@ body { background: var(--tool-card-hover-bg); } +.review-diff-file-header.static, +.review-diff-file-header.static:hover { + cursor: default; + background: transparent; +} + +.review-diff-file-group { + overflow: hidden; + border: 0; + border-radius: 0; +} + +.review-summary-note { + padding: 8px 12px; + border-bottom: 1px solid var(--tool-card-divider); + color: var(--color-text-secondary, #b7b7bf); + font-size: var(--font-text-sm-size, 12px); +} + +.review-binary-note { + padding: 4px 12px 10px 44px; + color: var(--color-text-tertiary, #a3a3aa); + font-size: var(--font-text-sm-size, 12px); +} + .review-diff-file-name, .review-diff-file-stats { overflow: hidden;