fix(review): harden Git review checkpoints

src/git.ts

@@ -12,7 +12,7 @@ export interface GitCommandResult {
 export interface GitEligibility {
   ok: boolean;
   gitRoot?: string;
-  reason?: "not_git" | "no_head";
+  reason?: "not_git";
   message?: string;
 }
 
@@ -42,17 +42,6 @@ export async function getGitEligibility(cwd: string): Promise<GitEligibility> {
   }
 
   const gitRoot = (await git(cwd, ["rev-parse", "--show-toplevel"])).stdout.trim();
-  try {
-    await git(gitRoot, ["rev-parse", "--verify", "--quiet", "HEAD^{commit}"]);
-  } catch {
-    return {
-      ok: false,
-      gitRoot,
-      reason: "no_head",
-      message: "repository has no HEAD commit",
-    };
-  }
-
   return { ok: true, gitRoot };
 }
 

src/review-checkpoints.test.ts

@@ -1,6 +1,6 @@
 import assert from "node:assert/strict";
 import { execFile } from "node:child_process";
-import { mkdtemp, rm, writeFile } from "node:fs/promises";
+import { mkdir, mkdtemp, rename, rm, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import test, { type TestContext } from "node:test";
@@ -50,6 +50,82 @@ test("show_changes reports and advances the last-shown checkpoint", async (t) =>
   assert.equal(afterReviewed.patch, "");
 });
 
+test("a nested workspace only reviews changes inside its workspace root", async (t) => {
+  const repositoryRoot = await committedRepository(t);
+  const workspaceRoot = join(repositoryRoot, "packages", "app");
+  const siblingRoot = join(repositoryRoot, "packages", "other");
+  await mkdir(workspaceRoot, { recursive: true });
+  await mkdir(siblingRoot, { recursive: true });
+  await writeFile(join(workspaceRoot, "app.txt"), "app\n");
+  await writeFile(join(siblingRoot, "other.txt"), "other\n");
+  await git(repositoryRoot, ["add", "."]);
+  await git(repositoryRoot, ["commit", "-m", "Add packages"]);
+
+  const manager = createReviewCheckpointManager();
+  await manager.initializeWorkspace({ workspaceId: "ws_nested", root: workspaceRoot });
+
+  await writeFile(join(workspaceRoot, "app.txt"), "app changed\n");
+  await writeFile(join(siblingRoot, "other.txt"), "other changed\n");
+
+  const review = await manager.reviewChanges({
+    workspaceId: "ws_nested",
+    root: workspaceRoot,
+    markReviewed: false,
+  });
+
+  assert.deepEqual(review.files.map((file) => file.path), ["app.txt"]);
+  assert.match(review.patch, /app changed/);
+  assert.doesNotMatch(review.patch, /other changed/);
+});
+
+test("binary changes use a renderable review patch instead of a Git binary patch", async (t) => {
+  const root = await committedRepository(t);
+  await writeFile(join(root, "asset.bin"), Buffer.from([0, 1, 2, 3]));
+  await git(root, ["add", "asset.bin"]);
+  await git(root, ["commit", "-m", "Add binary asset"]);
+
+  const manager = createReviewCheckpointManager();
+  await manager.initializeWorkspace({ workspaceId: "ws_binary", root });
+  await writeFile(join(root, "asset.bin"), Buffer.from([0, 1, 9, 3]));
+
+  const review = await manager.reviewChanges({
+    workspaceId: "ws_binary",
+    root,
+    markReviewed: false,
+  });
+
+  assert.deepEqual(review.files.map((file) => file.path), ["asset.bin"]);
+  assert.match(review.patch, /Binary files/);
+  assert.doesNotMatch(review.patch, /GIT binary patch/);
+});
+
+test("review metadata preserves pure renames from the rendered patch", async (t) => {
+  const root = await committedRepository(t);
+  await writeFile(join(root, "before.txt"), "same content\n");
+  await git(root, ["add", "before.txt"]);
+  await git(root, ["commit", "-m", "Add rename source"]);
+
+  const manager = createReviewCheckpointManager();
+  await manager.initializeWorkspace({ workspaceId: "ws_rename", root });
+  await rename(join(root, "before.txt"), join(root, "after.txt"));
+
+  const review = await manager.reviewChanges({
+    workspaceId: "ws_rename",
+    root,
+    markReviewed: false,
+  });
+
+  assert.deepEqual(review.files, [
+    {
+      path: "after.txt",
+      previousPath: "before.txt",
+      type: "rename-pure",
+      additions: 0,
+      removals: 0,
+    },
+  ]);
+});
+
 test("review checkpoints survive a manager restart", async (t) => {
   const root = await committedRepository(t);
   const manager = createReviewCheckpointManager();
@@ -173,27 +249,21 @@ test("a concurrent review rejects a different root after initialization", async
   }
 });
 
-test("an unborn repository becomes reviewable after its first commit", async (t) => {
+test("an unborn repository is reviewable without creating a commit", async (t) => {
   const root = await unbornRepository(t);
   const manager = createReviewCheckpointManager();
 
   await manager.initializeWorkspace({ workspaceId: "ws_unborn", root });
-  await assert.rejects(
-    () => manager.reviewChanges({ workspaceId: "ws_unborn", root }),
-    /repository has no HEAD commit/,
-  );
+  await writeFile(join(root, "README.md"), "first file\n");
 
-  await writeFile(join(root, "README.md"), "first commit\n");
-  await git(root, ["add", "README.md"]);
-  await git(root, ["commit", "-m", "Initial commit"]);
-
-  const afterFirstCommit = await manager.reviewChanges({
+  const review = await manager.reviewChanges({
     workspaceId: "ws_unborn",
     root,
     markReviewed: false,
   });
-  assert.equal(afterFirstCommit.summary.files, 0);
-  assert.equal(afterFirstCommit.patch, "");
+  assert.deepEqual(review.files.map((file) => file.path), ["README.md"]);
+  assert.equal(review.files[0]?.type, "new");
+  assert.match(review.patch, /first file/);
 });
 
 async function committedRepository(t: TestContext): Promise<string> {

src/review-checkpoints.ts

@@ -1,6 +1,7 @@
 import { mkdtemp, rm } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
+import { parsePatchFiles } from "@pierre/diffs";
 import { git, getGitEligibility, safeWorkspaceRefSegment } from "./git.js";
 
 export type ReviewSince = "last_shown" | "workspace_open";
@@ -47,6 +48,7 @@ export interface ReviewCheckpointManager {
 }
 
 const REVIEW_REF_PREFIX = "refs/devspace/review";
+const REVIEW_DIFF_MAX_BUFFER = 10_000_000;
 
 export function createReviewCheckpointManager(): ReviewCheckpointManager {
   const states = new Map<string, WorkspaceReviewState>();
@@ -107,14 +109,21 @@ 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], {
-        maxBuffer: 50 * 1024 * 1024,
+      const current = await createWorkingTreeSnapshot(state.gitRoot, state.root);
+      const patch = (await git(state.root, [
+        "diff",
+        "--relative",
+        "--patch",
+        "--find-renames",
+        "--no-color",
+        "--no-ext-diff",
+        "--no-textconv",
+        baseline,
+        current,
+      ], {
+        maxBuffer: REVIEW_DIFF_MAX_BUFFER,
       })).stdout;
-      const numstat = (await git(state.gitRoot, ["diff", "--numstat", "-z", baseline, current], {
-        maxBuffer: 50 * 1024 * 1024,
-      })).stdout;
-      const files = parseNumstat(numstat);
+      const files = parseReviewFiles(patch);
       const summary = summarizeFiles(files);
 
       if (markReviewed) {
@@ -139,6 +148,47 @@ export function createReviewCheckpointManager(): ReviewCheckpointManager {
   };
 }
 
+function parseReviewFiles(patch: string): ReviewFile[] {
+  if (patch.length === 0) return [];
+
+  try {
+    return parsePatchFiles(patch, "review", true).flatMap((parsedPatch) =>
+      parsedPatch.files.map((file) => {
+        const stats = file.hunks.reduce(
+          (total, hunk) => ({
+            additions: total.additions + hunk.additionLines,
+            removals: total.removals + hunk.deletionLines,
+          }),
+          { additions: 0, removals: 0 },
+        );
+
+        return {
+          path: file.name,
+          previousPath: file.prevName,
+          type: reviewFileType(file.type),
+          ...stats,
+        };
+      }),
+    );
+  } catch (error) {
+    const detail = error instanceof Error ? error.message : String(error);
+    throw new Error(`Review diff could not be rendered: ${detail}`);
+  }
+}
+
+function reviewFileType(type: string): ReviewFile["type"] {
+  switch (type) {
+    case "rename-pure":
+    case "rename-changed":
+    case "new":
+    case "deleted":
+    case "change":
+      return type;
+    default:
+      return "change";
+  }
+}
+
 function assertWorkspaceRoot(
   state: WorkspaceReviewState | undefined,
   workspaceId: string,
@@ -175,7 +225,7 @@ async function initializeWorkspaceState(
     ]);
 
     if (!openCommit && !baselineCommit) {
-      const initialCommit = await createWorkingTreeSnapshot(eligibility.gitRoot);
+      const initialCommit = await createWorkingTreeSnapshot(eligibility.gitRoot, root);
       await git(eligibility.gitRoot, ["update-ref", state.openRef, initialCommit]);
       await git(eligibility.gitRoot, ["update-ref", state.baselineRef, initialCommit]);
       state.openRefAvailable = true;
@@ -215,17 +265,18 @@ function reviewRefs(
   };
 }
 
-async function createWorkingTreeSnapshot(gitRoot: string): Promise<string> {
+async function createWorkingTreeSnapshot(gitRoot: string, workspaceRoot: string): Promise<string> {
   const tempDir = await mkdtemp(join(tmpdir(), "devspace-review-index-"));
   const indexPath = join(tempDir, "index");
   const env = checkpointEnv(indexPath);
 
   try {
-    await git(gitRoot, ["read-tree", "HEAD"], { env });
-    await git(gitRoot, ["add", "-A", "--", "."], { env });
+    if (await commitForRef(gitRoot, "HEAD")) {
+      await git(gitRoot, ["read-tree", "HEAD"], { env });
+    }
+    await git(workspaceRoot, ["add", "-A", "--", "."], { env });
     const tree = (await git(gitRoot, ["write-tree"], { env })).stdout.trim();
-    const parent = (await git(gitRoot, ["rev-parse", "--verify", "HEAD^{commit}"])).stdout.trim();
-    return (await git(gitRoot, ["commit-tree", tree, "-p", parent, "-m", "DevSpace review snapshot"], { env })).stdout.trim();
+    return (await git(gitRoot, ["commit-tree", tree, "-m", "DevSpace review snapshot"], { env })).stdout.trim();
   } finally {
     await rm(tempDir, { recursive: true, force: true });
   }
@@ -241,56 +292,6 @@ function checkpointEnv(indexPath: string): NodeJS.ProcessEnv {
   };
 }
 
-function parseNumstat(output: string): ReviewFile[] {
-  const fields = output.split("\0").filter((field) => field.length > 0);
-  const files: ReviewFile[] = [];
-
-  for (let index = 0; index < fields.length;) {
-    const header = fields[index++] ?? "";
-    const parts = header.split("\t");
-    const additions = parseStatNumber(parts[0]);
-    const removals = parseStatNumber(parts[1]);
-
-    if (parts.length >= 3) {
-      const path = parts[2] ?? "";
-      if (path) files.push({ path, type: fileType(path, undefined, additions, removals), additions, removals });
-      continue;
-    }
-
-    const previousPath = fields[index++];
-    const path = fields[index++];
-    if (!path) continue;
-
-    files.push({
-      path,
-      previousPath,
-      type: fileType(path, previousPath, additions, removals),
-      additions,
-      removals,
-    });
-  }
-
-  return files;
-}
-
-function parseStatNumber(value: string | undefined): number {
-  if (!value || value === "-") return 0;
-  const parsed = Number(value);
-  return Number.isFinite(parsed) ? parsed : 0;
-}
-
-function fileType(
-  path: string,
-  previousPath: string | undefined,
-  additions: number,
-  removals: number,
-): ReviewFile["type"] {
-  if (previousPath) return additions === 0 && removals === 0 ? "rename-pure" : "rename-changed";
-  if (additions > 0 && removals === 0) return "new";
-  if (additions === 0 && removals > 0) return "deleted";
-  return "change";
-}
-
 function summarizeFiles(files: ReviewFile[]): ReviewSummary {
   return files.reduce<ReviewSummary>(
     (summary, file) => ({

src/ui/review-payload.tsx

@@ -25,6 +25,11 @@ interface MountedPayload {
   unmount(): void;
 }
 
+interface ParsedReviewPatch {
+  files: FileDiffMetadata[];
+  error?: string;
+}
+
 export function mountReviewPayload(
   container: HTMLElement,
   options: PayloadRendererOptions,
@@ -50,13 +55,15 @@ function ReviewPayload({
 }: PayloadRendererOptions) {
   const patch = card.payload?.patch;
   const themeType: ThemeType = hostContext?.theme === "light" ? "light" : "dark";
-  const files = useMemo(() => parseFiles(patch), [patch]);
+  const parsedPatch = useMemo(() => parseFiles(patch), [patch]);
+  const files = parsedPatch.files;
   const visibleFiles = typeof visibleFileCount === "number"
     ? files.slice(0, visibleFileCount)
     : files;
   const [openFiles, setOpenFiles] = useState(() => new Set<string>());
 
   if (errorMessage) return <StatusLine message={errorMessage} tone="error" />;
+  if (parsedPatch.error) return <StatusLine message={parsedPatch.error} tone="error" />;
   if (!patch) return <StatusLine message="Diff payload is not available." />;
   if (files.length === 0) return <StatusLine message="No diff hunks to review." />;
 
@@ -177,9 +184,19 @@ function fileChangeSymbol(kind: FileChangeKind): string {
   }
 }
 
-function parseFiles(patch: string | undefined): FileDiffMetadata[] {
-  if (!patch) return [];
-  return parsePatchFiles(patch, "review", true).flatMap((parsedPatch) => parsedPatch.files);
+function parseFiles(patch: string | undefined): ParsedReviewPatch {
+  if (!patch) return { files: [] };
+
+  try {
+    return {
+      files: parsePatchFiles(patch, "review", true).flatMap((parsedPatch) => parsedPatch.files),
+    };
+  } catch {
+    return {
+      files: [],
+      error: "Review diff could not be rendered.",
+    };
+  }
 }
 
 function diffStats(fileDiff: FileDiffMetadata): { additions: number; removals: number } {