From 05983198cd919a477fbaac381ad7fe1388450021 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Sat, 3 Oct 2026 21:09:09 +0900 Subject: [PATCH 1/3] fix: reject stray lines between file sections instead of silently dropping them Between file sections the parser skipped every line that did not start with '*** ', so a file header indented by a space was dropped together with its hunk lines and the patch still reported success. File headers are now recognized after trimming, as in Codex, and any other non-blank line between sections rejects the patch with "is not a valid hunk header". Fixes #45 --- CHANGELOG.md | 4 ++ src/index.ts | 8 ++- test/patch-headers.test.ts | 119 +++++++++++++++++++++++++++++++++++++ 3 files changed, 128 insertions(+), 3 deletions(-) create mode 100644 test/patch-headers.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 11e2fa4..396483f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Recognize indented file headers like Codex, and reject any other line between file sections instead of silently dropping that section while reporting success (#45). + ### Changed - Docs: install from GitHub instead of npm. diff --git a/src/index.ts b/src/index.ts index c47d4f5..b3345b3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -924,8 +924,10 @@ function parsePatch(patchText: string): ParsedPatch[] { const hunks: ParsedPatch[] = []; let index = beginIndex + 1; while (index < endIndex) { - const line = lines[index] ?? ""; - if (!line.startsWith("*** ")) { + // Like Codex, file headers are recognized after trimming, and any other non-blank line between file + // sections is rejected below; skipping it would silently drop the section it introduces. + const line = (lines[index] ?? "").trim(); + if (line === "") { index++; continue; } @@ -936,7 +938,7 @@ function parsePatch(patchText: string): ParsedPatch[] { const contentLines: string[] = []; while (index < endIndex) { const nextLine = lines[index] ?? ""; - if (nextLine.startsWith("*** ")) { + if (nextLine.trim().startsWith("*** ")) { break; } if (!nextLine.startsWith("+")) { diff --git a/test/patch-headers.test.ts b/test/patch-headers.test.ts new file mode 100644 index 0000000..eb9aebb --- /dev/null +++ b/test/patch-headers.test.ts @@ -0,0 +1,119 @@ +import { mkdtemp, readFile, rm, stat, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { applyPatchDetailed, PatchParseError } from "../src/index.js"; + +const tempDirectories: string[] = []; + +async function workspace(files: Record): Promise { + const cwd = await mkdtemp(path.join(tmpdir(), "pi-apply-patch-headers-")); + tempDirectories.push(cwd); + for (const [name, content] of Object.entries(files)) { + await writeFile(path.join(cwd, name), content); + } + return cwd; +} + +async function exists(filePath: string): Promise { + return stat(filePath).then( + () => true, + () => false, + ); +} + +afterEach(async () => { + await Promise.all(tempDirectories.splice(0).map((directory) => rm(directory, { recursive: true, force: true }))); +}); + +describe("apply_patch file headers", () => { + it("#given an update header indented with spaces #when applied #then the file is updated like Codex", async () => { + // given + const cwd = await workspace({ "foo.txt": "old\n" }); + + // when + const result = await applyPatchDetailed( + cwd, + "*** Begin Patch\n *** Update File: foo.txt\n@@\n-old\n+new\n*** End Patch\n", + ); + + // then + expect(result.failures).toEqual([]); + expect(await readFile(path.join(cwd, "foo.txt"), "utf8")).toBe("new\n"); + }); + + it("#given an indented update header after a delete section #when applied #then both files change", async () => { + // given + const cwd = await workspace({ "old.txt": "x\n", "b.txt": "b1\n" }); + + // when + const result = await applyPatchDetailed( + cwd, + "*** Begin Patch\n*** Delete File: old.txt\n *** Update File: b.txt\n@@\n-b1\n+B1\n*** End Patch\n", + ); + + // then + expect(result.failures).toEqual([]); + expect(result.appliedFiles).toEqual(["old.txt", "b.txt"]); + expect(await exists(path.join(cwd, "old.txt"))).toBe(false); + expect(await readFile(path.join(cwd, "b.txt"), "utf8")).toBe("B1\n"); + }); + + it("#given an indented header right after added file content #when applied #then the next section still applies", async () => { + // given + const cwd = await workspace({ "b.txt": "b1\n" }); + + // when + const result = await applyPatchDetailed( + cwd, + "*** Begin Patch\n*** Add File: new.txt\n+hello\n *** Update File: b.txt\n@@\n-b1\n+B1\n*** End Patch\n", + ); + + // then + expect(result.failures).toEqual([]); + expect(await readFile(path.join(cwd, "new.txt"), "utf8")).toBe("hello\n"); + expect(await readFile(path.join(cwd, "b.txt"), "utf8")).toBe("B1\n"); + }); + + it("#given a stray line between file sections #when applied #then the whole patch is rejected and nothing changes", async () => { + // given + const cwd = await workspace({ "old.txt": "x\n", "b.txt": "b1\n" }); + const patch = "*** Begin Patch\n*** Delete File: old.txt\nnow update b\n@@\n-b1\n+B1\n*** End Patch\n"; + + // when + const applying = applyPatchDetailed(cwd, patch); + + // then + await expect(applying).rejects.toThrow(PatchParseError); + await expect(applyPatchDetailed(cwd, patch)).rejects.toThrow("'now update b' is not a valid hunk header"); + expect(await exists(path.join(cwd, "old.txt"))).toBe(true); + expect(await readFile(path.join(cwd, "b.txt"), "utf8")).toBe("b1\n"); + }); + + it("#given a misspelled header as the first section #when applied #then it is rejected instead of reporting no hunks", async () => { + // given + const cwd = await workspace({ "b.txt": "b1\n" }); + + // when + const applying = applyPatchDetailed(cwd, "*** Begin Patch\nUpdate File: b.txt\n@@\n-b1\n+B1\n*** End Patch\n"); + + // then + await expect(applying).rejects.toThrow("'Update File: b.txt' is not a valid hunk header"); + expect(await readFile(path.join(cwd, "b.txt"), "utf8")).toBe("b1\n"); + }); + + it("#given an indented header inside an update hunk #when applied #then it is matched as a context line like Codex", async () => { + // given + const cwd = await workspace({ "notes.md": "intro\n *** Update File: x\nend\n" }); + + // when + const result = await applyPatchDetailed( + cwd, + "*** Begin Patch\n*** Update File: notes.md\n@@\n intro\n *** Update File: x\n-end\n+END\n*** End Patch\n", + ); + + // then + expect(result.failures).toEqual([]); + expect(await readFile(path.join(cwd, "notes.md"), "utf8")).toBe("intro\n *** Update File: x\nEND\n"); + }); +}); From b3efd4ca5d7ab49b8ead0a9eef54e48b1d75a098 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Sat, 3 Oct 2026 21:16:50 +0900 Subject: [PATCH 2/3] fix: list indented file headers in extractPatchedPaths The parser now accepts indented file headers, so the exported path extractor must report them too; a consumer that gates writes per file (a permission prompt, the pending-path display) would otherwise not see a file the patch is about to change. Refs #45 --- src/index.ts | 3 ++- test/patch-headers.test.ts | 14 +++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/index.ts b/src/index.ts index b3345b3..4c9f3ae 100644 --- a/src/index.ts +++ b/src/index.ts @@ -439,7 +439,8 @@ function seekSequence( export function extractPatchedPaths(patchText: string): string[] { const normalized = stripHeredoc(normalizePatchText(patchText)); - const matches = normalized.matchAll(/^\*\*\* (?:(?:Add|Delete|Update) File|Move to): (.+)$/gm); + // Matches every header the parser accepts (it trims them), so a consumer gating writes per file sees them all. + const matches = normalized.matchAll(/^[ \t]*\*\*\* (?:(?:Add|Delete|Update) File|Move to): (.+?)[ \t]*$/gm); return Array.from(matches, (match) => match[1] ?? ""); } diff --git a/test/patch-headers.test.ts b/test/patch-headers.test.ts index eb9aebb..54e5317 100644 --- a/test/patch-headers.test.ts +++ b/test/patch-headers.test.ts @@ -2,7 +2,7 @@ import { mkdtemp, readFile, rm, stat, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; -import { applyPatchDetailed, PatchParseError } from "../src/index.js"; +import { applyPatchDetailed, extractPatchedPaths, PatchParseError } from "../src/index.js"; const tempDirectories: string[] = []; @@ -116,4 +116,16 @@ describe("apply_patch file headers", () => { expect(result.failures).toEqual([]); expect(await readFile(path.join(cwd, "notes.md"), "utf8")).toBe("intro\n *** Update File: x\nEND\n"); }); + + it("#given an indented header the parser accepts #when patched paths are listed #then that file is included", () => { + // given + const patch = + "*** Begin Patch\n*** Delete File: old.txt\n *** Update File: secrets.env \n@@\n-a\n+b\n*** End Patch\n"; + + // when + const paths = extractPatchedPaths(patch); + + // then + expect(paths).toEqual(["old.txt", "secrets.env"]); + }); }); From 36388ed202a216edc06dfd4698a82eeba9354bed Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Sat, 3 Oct 2026 21:19:03 +0900 Subject: [PATCH 3/3] fix: read file headers through one helper in the parser and extractPatchedPaths The parser trimmed headers with String.prototype.trim, but the extractor's regex only allowed spaces and tabs, so a header indented with a no-break space was written but not listed, and a trailing one made the listed path differ from the written one. Both now read headers through parseFileHeader and parseMoveTo (Move to is trimmed at the end, as in Codex), so the listed paths are exactly the paths the patch writes. Refs #45 --- src/index.ts | 50 +++++++++++++++++++++++++++++--------- test/patch-headers.test.ts | 16 +++++++++--- 2 files changed, 50 insertions(+), 16 deletions(-) diff --git a/src/index.ts b/src/index.ts index 4c9f3ae..8d69839 100644 --- a/src/index.ts +++ b/src/index.ts @@ -437,11 +437,35 @@ function seekSequence( return undefined; } +const FILE_HEADER_MARKERS = [ + ["add", "*** Add File: "], + ["delete", "*** Delete File: "], + ["update", "*** Update File: "], +] as const; +const MOVE_TO_MARKER = "*** Move to: "; + +// The parser and extractPatchedPaths both read headers through these two functions, so they share one +// whitespace definition (String.prototype.trim): any path the parser writes is a path consumers that +// gate writes per file (permission prompts) see, spelled the same way. +function parseFileHeader(line: string): { kind: ParsedPatch["type"]; path: string } | undefined { + const trimmed = line.trim(); + for (const [kind, marker] of FILE_HEADER_MARKERS) { + if (trimmed.startsWith(marker)) return { kind, path: trimmed.slice(marker.length) }; + } + return undefined; +} + +function parseMoveTo(line: string): string | undefined { + const trimmed = line.trimEnd(); + return trimmed.startsWith(MOVE_TO_MARKER) ? trimmed.slice(MOVE_TO_MARKER.length) : undefined; +} + export function extractPatchedPaths(patchText: string): string[] { - const normalized = stripHeredoc(normalizePatchText(patchText)); - // Matches every header the parser accepts (it trims them), so a consumer gating writes per file sees them all. - const matches = normalized.matchAll(/^[ \t]*\*\*\* (?:(?:Add|Delete|Update) File|Move to): (.+?)[ \t]*$/gm); - return Array.from(matches, (match) => match[1] ?? ""); + const lines = stripHeredoc(normalizePatchText(patchText)).split("\n"); + return lines.flatMap((line) => { + const path = parseFileHeader(line)?.path ?? parseMoveTo(line.trimStart()); + return path === undefined ? [] : [path]; + }); } function createPatchDiff(oldContent: string, newContent: string): { diff: string; added: number; removed: number } { @@ -932,9 +956,10 @@ function parsePatch(patchText: string): ParsedPatch[] { index++; continue; } + const header = parseFileHeader(line); - if (line.startsWith("*** Add File: ")) { - const filePath = line.slice("*** Add File: ".length); + if (header?.kind === "add") { + const filePath = header.path; index++; const contentLines: string[] = []; while (index < endIndex) { @@ -956,18 +981,19 @@ function parsePatch(patchText: string): ParsedPatch[] { continue; } - if (line.startsWith("*** Delete File: ")) { - hunks.push({ type: "delete", filePath: line.slice("*** Delete File: ".length) }); + if (header?.kind === "delete") { + hunks.push({ type: "delete", filePath: header.path }); index++; continue; } - if (line.startsWith("*** Update File: ")) { - const filePath = line.slice("*** Update File: ".length); + if (header?.kind === "update") { + const filePath = header.path; index++; let movePath: string | undefined; - if ((lines[index] ?? "").startsWith("*** Move to: ")) { - movePath = (lines[index] ?? "").slice("*** Move to: ".length); + const moveTo = parseMoveTo(lines[index] ?? ""); + if (moveTo !== undefined) { + movePath = moveTo; index++; } diff --git a/test/patch-headers.test.ts b/test/patch-headers.test.ts index 54e5317..3ae9cd9 100644 --- a/test/patch-headers.test.ts +++ b/test/patch-headers.test.ts @@ -117,15 +117,23 @@ describe("apply_patch file headers", () => { expect(await readFile(path.join(cwd, "notes.md"), "utf8")).toBe("intro\n *** Update File: x\nEND\n"); }); - it("#given an indented header the parser accepts #when patched paths are listed #then that file is included", () => { + it("#given headers indented or padded with any whitespace #when applied #then the listed paths are exactly the files written", async () => { // given + const cwd = await workspace({ "old.txt": "x\n", "b.txt": "b1\n", "c.txt": "c1\n" }); const patch = - "*** Begin Patch\n*** Delete File: old.txt\n *** Update File: secrets.env \n@@\n-a\n+b\n*** End Patch\n"; + "*** Begin Patch\n*** Delete File: old.txt\n\u00A0*** Update File: b.txt\u00A0\n@@\n-b1\n+B1\n" + + "*** Update File: c.txt\u00A0\n*** Move to: moved.txt \n@@\n-c1\n+C1\n*** End Patch\n"; // when - const paths = extractPatchedPaths(patch); + const listed = extractPatchedPaths(patch); + const result = await applyPatchDetailed(cwd, patch); // then - expect(paths).toEqual(["old.txt", "secrets.env"]); + expect(result.failures).toEqual([]); + expect(listed).toEqual(["old.txt", "b.txt", "c.txt", "moved.txt"]); + expect(result.appliedFiles).toEqual(["old.txt", "b.txt", "moved.txt"]); + expect(await readFile(path.join(cwd, "b.txt"), "utf8")).toBe("B1\n"); + expect(await readFile(path.join(cwd, "moved.txt"), "utf8")).toBe("C1\n"); + expect(await exists(path.join(cwd, "c.txt"))).toBe(false); }); });