fix(loaders): restore a/b prefixes on noprefix patch input (#240)

Co-authored-by: Ben Vinegar <ben@benv.ca>

Author: mo

src/core/loaders.test.ts

@@ -1058,4 +1058,125 @@ describe("loadAppBootstrap", () => {
     expect(bootstrap.changeset.files[0]?.path.endsWith("after.ts")).toBe(true);
     expect(bootstrap.changeset.files[0]?.stats.additions).toBeGreaterThan(0);
   });
+
+  test("loads patch text emitted with diff.noprefix=true (e.g. from `hunk pager` stdin)", async () => {
+    const bootstrap = await loadAppBootstrap({
+      kind: "patch",
+      text: [
+        "diff --git src/example.ts src/example.ts",
+        "index 0000000..1111111 100644",
+        "--- src/example.ts",
+        "+++ src/example.ts",
+        "@@ -1,1 +1,2 @@",
+        " const value = 1;",
+        "+const added = 2;",
+      ].join("\n"),
+      options: { mode: "auto" },
+    });
+
+    expect(bootstrap.changeset.files).toHaveLength(1);
+    expect(bootstrap.changeset.files[0]).toMatchObject({
+      path: "src/example.ts",
+      metadata: { name: "src/example.ts", type: "change" },
+    });
+    expect(bootstrap.changeset.files[0]?.stats.additions).toBe(1);
+  });
+
+  test("loads noprefix rename patches by recovering the rename pair from the headers", async () => {
+    const bootstrap = await loadAppBootstrap({
+      kind: "patch",
+      text: [
+        "diff --git old/path.ts new/path.ts",
+        "similarity index 100%",
+        "rename from old/path.ts",
+        "rename to new/path.ts",
+      ].join("\n"),
+      options: { mode: "auto" },
+    });
+
+    expect(bootstrap.changeset.files).toHaveLength(1);
+    expect(bootstrap.changeset.files[0]).toMatchObject({
+      path: "new/path.ts",
+      previousPath: "old/path.ts",
+      metadata: { type: "rename-pure" },
+    });
+  });
+
+  test("loads quoted noprefix patch text emitted for escaped git paths", async () => {
+    const dir = createTempRepo("hunk-patch-quoted-noprefix-");
+    const fileName = "src\tfile.txt";
+
+    writeFileSync(join(dir, fileName), "one\n");
+    git(dir, "add", ".");
+    git(dir, "commit", "-m", "initial");
+
+    writeFileSync(join(dir, fileName), "two\n");
+    const patchText = git(dir, "-c", "diff.noprefix=true", "diff", "--", fileName);
+
+    expect(patchText).toContain('diff --git "src\\tfile.txt" "src\\tfile.txt"');
+
+    const bootstrap = await loadAppBootstrap({
+      kind: "patch",
+      text: patchText,
+      options: { mode: "auto" },
+    });
+
+    expect(bootstrap.changeset.files).toHaveLength(1);
+    expect(bootstrap.changeset.files[0]).toMatchObject({
+      path: "src\\tfile.txt",
+      metadata: { name: "src\\tfile.txt", type: "change" },
+    });
+    expect(bootstrap.changeset.files[0]?.stats).toEqual({ additions: 1, deletions: 1 });
+  });
+
+  test("does not mangle a deleted SQL `-- comment` line in a noprefix patch", async () => {
+    // The original source line `-- drop table users;` (a SQL comment) is encoded in a unified
+    // diff deletion as `--- drop table users;` — three dashes (one for the deletion marker,
+    // two from the comment) and a space. That looks identical to a `--- a/path` file header
+    // on its own, so the noprefix prefix-restorer must stop rewriting `--- ` lines once the
+    // `+++ ` line of the current block has been emitted.
+    const bootstrap = await loadAppBootstrap({
+      kind: "patch",
+      text: [
+        "diff --git db/schema.sql db/schema.sql",
+        "index 0000000..1111111 100644",
+        "--- db/schema.sql",
+        "+++ db/schema.sql",
+        "@@ -1,3 +1,2 @@",
+        " CREATE TABLE users (id INT);",
+        "--- drop table users;",
+        " CREATE TABLE posts (id INT);",
+      ].join("\n"),
+      options: { mode: "auto" },
+    });
+
+    expect(bootstrap.changeset.files).toHaveLength(1);
+    const file = bootstrap.changeset.files[0]!;
+    expect(file.path).toBe("db/schema.sql");
+    expect(file.stats.deletions).toBe(1);
+    // The deleted content must round-trip as `-- drop table users;` (the original SQL line),
+    // not as `-- a/drop table users;` (the corruption produced when the rewriter is still
+    // active inside the hunk body).
+    expect(file.metadata.deletionLines).toContain("-- drop table users;\n");
+    expect(file.metadata.deletionLines.some((line) => line.includes("a/"))).toBe(false);
+  });
+
+  test("leaves correctly prefixed patches untouched even when paths sit inside an `a/` directory", async () => {
+    const bootstrap = await loadAppBootstrap({
+      kind: "patch",
+      text: [
+        "diff --git a/a/inner.ts b/a/inner.ts",
+        "index 0000000..1111111 100644",
+        "--- a/a/inner.ts",
+        "+++ b/a/inner.ts",
+        "@@ -1,1 +1,2 @@",
+        " const x = 1;",
+        "+const y = 2;",
+      ].join("\n"),
+      options: { mode: "auto" },
+    });
+
+    expect(bootstrap.changeset.files).toHaveLength(1);
+    expect(bootstrap.changeset.files[0]?.path).toBe("a/inner.ts");
+  });
 });

src/core/loaders.ts

@@ -252,6 +252,114 @@ function buildDiffFile(
   };
 }
 
+/**
+ * Re-add Git's `a/` and `b/` path prefixes to patch headers when stdin came from a
+ * `git diff` that was emitted with `diff.noprefix=true` (or otherwise stripped prefixes).
+ *
+ * `@pierre/diffs` requires `a/` and `b/` on `diff --git`, `---`, and `+++` lines and throws
+ * a `TypeError` on the first noprefix header, which leaves the review with zero files. The
+ * git-backed paths force `diff.noprefix=false` when they invoke git internally; this helper
+ * covers the patch path (`hunk patch`, `hunk pager`) where the input was produced by an
+ * outer `git` process we do not control.
+ *
+ * The rewrite is scoped to header lines only: once the `+++ ` line has been emitted for a
+ * block we clear the flag so a deleted line whose content starts with `-- ` (e.g. a removed
+ * SQL/Lua/Haskell comment, which becomes `--- foo` on disk) is not mistaken for a file
+ * header inside the hunk body.
+ */
+function normalizeGitPatchPrefixes(patchText: string) {
+  if (!patchText.includes("diff --git ")) {
+    return patchText;
+  }
+
+  let blockNeedsPrefix = false;
+
+  return patchText
+    .split("\n")
+    .map((line) => {
+      if (line.startsWith("diff --git ")) {
+        const result = rewriteGitDiffHeader(line);
+        blockNeedsPrefix = result.changed;
+        return result.line;
+      }
+
+      if (blockNeedsPrefix && line.startsWith("--- ")) {
+        return rewriteUnifiedFileLine(line, "--- ", "a/");
+      }
+
+      if (blockNeedsPrefix && line.startsWith("+++ ")) {
+        blockNeedsPrefix = false;
+        return rewriteUnifiedFileLine(line, "+++ ", "b/");
+      }
+
+      return line;
+    })
+    .join("\n");
+}
+
+/** Detect prefixed/noprefix `diff --git` lines and rewrite the noprefix form into `a/X b/Y`. */
+function rewriteGitDiffHeader(line: string): { line: string; changed: boolean } {
+  const rest = line.slice("diff --git ".length).trimEnd();
+
+  const quotedMatch = rest.match(/^"((?:\\.|[^"\\])*)" "((?:\\.|[^"\\])*)"$/);
+  if (quotedMatch) {
+    const [, oldPath = "", newPath = ""] = quotedMatch;
+    // Pierre's git header parser does not currently handle the quoted `"a/..." "b/..."`
+    // form, so canonicalize quoted paths to the unquoted form even when prefixes exist.
+    return {
+      line: `diff --git ${withGitPrefix(oldPath, "a/")} ${withGitPrefix(newPath, "b/")}`,
+      changed: true,
+    };
+  }
+
+  const tokens = rest.split(" ");
+
+  // Already prefixed: `a/X b/Y` (covers single-token and equally split multi-token paths).
+  if (tokens.length === 2 && tokens[0]?.startsWith("a/") && tokens[1]?.startsWith("b/")) {
+    return { line, changed: false };
+  }
+  if (tokens.length >= 2 && tokens.length % 2 === 0) {
+    const half = tokens.length / 2;
+    const firstHalf = tokens.slice(0, half).join(" ");
+    const secondHalf = tokens.slice(half).join(" ");
+    if (firstHalf.startsWith("a/") && secondHalf.startsWith("b/")) {
+      return { line, changed: false };
+    }
+    // Non-rename noprefix: identical halves regardless of whether the path contains spaces.
+    if (firstHalf === secondHalf && firstHalf.length > 0) {
+      return { line: `diff --git a/${firstHalf} b/${secondHalf}`, changed: true };
+    }
+  }
+
+  // Two-token rename without prefix and without spaces in either path.
+  if (tokens.length === 2 && tokens[0] && tokens[1]) {
+    return { line: `diff --git a/${tokens[0]} b/${tokens[1]}`, changed: true };
+  }
+
+  // Genuinely ambiguous (rename with spaces and no quoting). Leave untouched and let the
+  // parser surface the existing failure rather than guess at the path split.
+  return { line, changed: false };
+}
+
+/** Return a path with the expected Git side prefix while avoiding double-prefixing. */
+function withGitPrefix(path: string, prefix: "a/" | "b/") {
+  return path.startsWith(prefix) ? path : `${prefix}${path}`;
+}
+
+/** Insert the canonical `a/` or `b/` prefix on a unified-diff header that is missing it. */
+function rewriteUnifiedFileLine(line: string, marker: "--- " | "+++ ", prefix: "a/" | "b/") {
+  const path = line.slice(marker.length);
+  const quotedPath = path.match(/^"((?:\\.|[^"\\])*)"(.*)$/);
+  const pathName = quotedPath?.[1] ?? path;
+  const suffix = quotedPath?.[2] ?? "";
+
+  if (pathName === "/dev/null" || pathName.startsWith("/dev/null\t")) {
+    return line;
+  }
+
+  return `${marker}${withGitPrefix(pathName, prefix)}${suffix}`;
+}
+
 /** Escape only the filename characters that break unified-diff header parsing. */
 function escapeUntrackedPatchPath(path: string) {
   return path
@@ -557,8 +665,8 @@ function normalizePatchChangeset(
   sourceLabel: string,
   agentContext: AgentContext | null,
 ): Changeset {
-  const normalizedPatchText = stripGitLogMetadata(
-    stripTerminalControl(patchText.replaceAll("\r\n", "\n")),
+  const normalizedPatchText = normalizeGitPatchPrefixes(
+    stripGitLogMetadata(stripTerminalControl(patchText.replaceAll("\r\n", "\n"))),
   );
 
   let parsedPatches: ReturnType<typeof parsePatchFiles>;