Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions src/core/loaders.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -876,4 +876,98 @@ 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("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");
});
});
103 changes: 102 additions & 1 deletion src/core/loaders.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,105 @@ 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/");
}
Comment on lines +206 to +209
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 blockNeedsPrefix is never cleared after the +++ header, corrupting removals of -- comment lines

Once the +++ file header is rewritten, blockNeedsPrefix stays true for the rest of the block. In a unified diff, a removed line whose content starts with -- (SQL/Lua/Haskell/Ada comments are exactly this) produces a diff body line that begins with --- (three dashes + space). This line is indistinguishable from the file header marker by the current startsWith("--- ") check, so it gets an a/ prefix prepended — corrupting the diff before it reaches the parser. A minimal repro: any noprefix diff that removes a SQL comment like -- drop table will have a content line --- drop table, which becomes --- a/drop table after this rewrite. The parser will reject the mangled patch, and normalizePatchChangeset will swallow the error and return files: [].

Setting blockNeedsPrefix = false immediately after the +++ line is processed is sufficient; the ---/+++ pair is always the last pair of header lines before the first @@ hunk marker in any valid git diff output.

Suggested change
if (blockNeedsPrefix && line.startsWith("+++ ")) {
return rewriteUnifiedFileLine(line, "+++ ", "b/");
}
if (blockNeedsPrefix && line.startsWith("+++ ")) {
blockNeedsPrefix = false;
return rewriteUnifiedFileLine(line, "+++ ", "b/");
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/loaders.ts
Line: 201-203

Comment:
**`blockNeedsPrefix` is never cleared after the `+++` header, corrupting removals of `--` comment lines**

Once the `+++ ` file header is rewritten, `blockNeedsPrefix` stays `true` for the rest of the block. In a unified diff, a removed line whose content starts with `-- ` (SQL/Lua/Haskell/Ada comments are exactly this) produces a diff body line that begins with `--- ` (three dashes + space). This line is indistinguishable from the file header marker by the current `startsWith("--- ")` check, so it gets an `a/` prefix prepended — corrupting the diff before it reaches the parser. A minimal repro: any noprefix diff that removes a SQL comment like `-- drop table` will have a content line `--- drop table`, which becomes `--- a/drop table` after this rewrite. The parser will reject the mangled patch, and `normalizePatchChangeset` will swallow the error and return `files: []`.

Setting `blockNeedsPrefix = false` immediately after the `+++ ` line is processed is sufficient; the `---`/`+++` pair is always the last pair of header lines before the first `@@` hunk marker in any valid git diff output.

```suggestion
      if (blockNeedsPrefix && line.startsWith("+++ ")) {
        blockNeedsPrefix = false;
        return rewriteUnifiedFileLine(line, "+++ ", "b/");
      }
```

How can I resolve this? If you propose a fix, please make it concise.


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;
if (oldPath?.startsWith("a/") && newPath?.startsWith("b/")) {
return { line, changed: false };
}
return { line: `diff --git "a/${oldPath}" "b/${newPath}"`, 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 };
}

/** 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);
if (path === "/dev/null" || path.startsWith("/dev/null\t")) {
return line;
}
if (path.startsWith('"')) {
return `${marker}"${prefix}${path.slice(1)}`;
}
return `${marker}${prefix}${path}`;
}

/** Escape only the filename characters that break unified-diff header parsing. */
function escapeUntrackedPatchPath(path: string) {
return path
Expand Down Expand Up @@ -300,7 +399,9 @@ function normalizePatchChangeset(
sourceLabel: string,
agentContext: AgentContext | null,
): Changeset {
const normalizedPatchText = stripTerminalControl(patchText.replaceAll("\r\n", "\n"));
const normalizedPatchText = normalizeGitPatchPrefixes(
stripTerminalControl(patchText.replaceAll("\r\n", "\n")),
);

let parsedPatches: ReturnType<typeof parsePatchFiles>;
try {
Expand Down