From 9c389780c001dc629083dd3326355c682bb35378 Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Mon, 20 Jul 2026 18:57:28 -0500 Subject: [PATCH] fix(core): improve patch errors --- packages/core/src/patch.ts | 11 ++++++++- packages/core/src/tool/patch.ts | 29 +++++++++++++++++------ packages/core/test/patch.test.ts | 9 +++++-- packages/core/test/tool-patch.test.ts | 34 +++++++++++++++++++++++---- 4 files changed, 68 insertions(+), 15 deletions(-) diff --git a/packages/core/src/patch.ts b/packages/core/src/patch.ts index 404986cbc8..d4c4ab5cf2 100644 --- a/packages/core/src/patch.ts +++ b/packages/core/src/patch.ts @@ -26,7 +26,8 @@ export function parse(patchText: string): ReadonlyArray { const lines = stripHeredoc(patchText.trim()).split("\n") const begin = lines.findIndex((line) => line.trim() === "*** Begin Patch") const end = lines.findIndex((line) => line.trim() === "*** End Patch") - if (begin === -1 || end === -1 || begin >= end) throw new Error("Invalid patch format: missing Begin/End markers") + if (begin === -1) throw new Error("The first line of the patch must be '*** Begin Patch'") + if (end === -1 || begin >= end) throw new Error("The last line of the patch must be '*** End Patch'") const hunks: Hunk[] = [] let index = begin + 1 @@ -72,6 +73,14 @@ export function parse(patchText: string): ReadonlyArray { } index++ } + if (hunks.length === 0) { + const invalid = lines.findIndex((line, index) => index > begin && index < end && line.trim() !== "") + if (invalid !== -1) { + throw new Error( + `Invalid hunk at line ${invalid + 1}: '${lines[invalid]!.trim()}' is not a valid hunk header. Valid hunk headers: '*** Add File: {path}', '*** Delete File: {path}', '*** Update File: {path}'`, + ) + } + } return hunks } diff --git a/packages/core/src/tool/patch.ts b/packages/core/src/tool/patch.ts index 990abcab0e..e4f47739a9 100644 --- a/packages/core/src/tool/patch.ts +++ b/packages/core/src/tool/patch.ts @@ -99,7 +99,10 @@ export const Plugin = { if (!input.patchText) return yield* new ToolFailure({ message: "patchText is required" }) const hunks = yield* Effect.try({ try: () => Patch.parse(input.patchText), - catch: (cause) => new ToolFailure({ message: `patch verification failed: ${String(cause)}` }), + catch: (cause) => + new ToolFailure({ + message: `patch verification failed: ${cause instanceof Error ? cause.message : String(cause)}`, + }), }) if (hunks.length === 0) { const normalized = input.patchText.replace(/\r\n/g, "\n").replace(/\r/g, "\n").trim() @@ -153,15 +156,27 @@ export const Plugin = { prepared.push({ ...hunk, target, before: original.replace(/^\uFEFF/, ""), after: "" }) return } - const stats = yield* fs - .stat(target.canonical) - .pipe(Effect.catch(() => Effect.succeed(undefined))) - if (!stats || stats.type === "Directory") { + const stats = yield* fs.stat(target.canonical).pipe( + Effect.mapError( + (error) => + new ToolFailure({ + message: `patch verification failed: Failed to read file to update ${target.canonical}: ${error instanceof Error ? error.message : String(error)}`, + }), + ), + ) + if (stats.type === "Directory") { return yield* new ToolFailure({ - message: `patch verification failed: Failed to read file to update: ${target.canonical}`, + message: `patch verification failed: Failed to read file to update ${target.canonical}: path is a directory`, }) } - const content = yield* fs.readFile(target.canonical) + const content = yield* fs.readFile(target.canonical).pipe( + Effect.mapError( + (error) => + new ToolFailure({ + message: `patch verification failed: Failed to read file to update ${target.canonical}: ${error instanceof Error ? error.message : String(error)}`, + }), + ), + ) const original = new TextDecoder("utf-8", { ignoreBOM: true }).decode(content) const before = original.replace(/^\uFEFF/, "") const update = yield* Effect.try({ diff --git a/packages/core/test/patch.test.ts b/packages/core/test/patch.test.ts index 7dd332e388..c8124c0377 100644 --- a/packages/core/test/patch.test.ts +++ b/packages/core/test/patch.test.ts @@ -34,8 +34,13 @@ describe("Patch", () => { ]) }) - test("rejects invalid patch format", () => { - expect(() => Patch.parse("This is not a valid patch")).toThrow("Invalid patch format") + test("identifies the missing patch boundary", () => { + expect(() => Patch.parse("This is not a valid patch")).toThrow( + "The first line of the patch must be '*** Begin Patch'", + ) + expect(() => Patch.parse("*** Begin Patch\n*** Add File: add.txt\n+added")).toThrow( + "The last line of the patch must be '*** End Patch'", + ) }) test("strips a heredoc wrapper", () => { diff --git a/packages/core/test/tool-patch.test.ts b/packages/core/test/tool-patch.test.ts index 829b9f6840..bdb24bf3b5 100644 --- a/packages/core/test/tool-patch.test.ts +++ b/packages/core/test/tool-patch.test.ts @@ -433,9 +433,13 @@ describe("PatchTool", () => { it.live("rejects invalid patch format", () => withTempTool((_directory, registry) => Effect.gen(function* () { - expect(yield* executeTool(registry, call("invalid patch"))).toMatchObject({ + expect(yield* executeTool(registry, call("invalid patch"))).toEqual({ type: "error", - value: expect.stringContaining("patch verification failed"), + value: "patch verification failed: The first line of the patch must be '*** Begin Patch'", + }) + expect(yield* executeTool(registry, call("*** Begin Patch\n*** Add File: foo\n+hello"))).toEqual({ + type: "error", + value: "patch verification failed: The last line of the patch must be '*** End Patch'", }) }), ), @@ -460,7 +464,11 @@ describe("PatchTool", () => { registry, call("*** Begin Patch\n*** Frobnicate File: foo\n*** End Patch"), ), - ).toEqual({ type: "error", value: "patch verification failed: no hunks found" }) + ).toEqual({ + type: "error", + value: + "patch verification failed: Invalid hunk at line 2: '*** Frobnicate File: foo' is not a valid hunk header. Valid hunk headers: '*** Add File: {path}', '*** Delete File: {path}', '*** Update File: {path}'", + }) }), ), ) @@ -623,7 +631,7 @@ describe("PatchTool", () => { ) it.live("rejects an update when the target file is missing", () => - withTempTool((_directory, registry) => + withTempTool((directory, registry) => Effect.gen(function* () { expect( yield* executeTool( @@ -632,7 +640,23 @@ describe("PatchTool", () => { ), ).toMatchObject({ type: "error", - value: expect.stringContaining("patch verification failed: Failed to read file to update"), + value: expect.stringContaining( + `patch verification failed: Failed to read file to update ${path.join(directory, "missing.txt")}: `, + ), + }) + }), + ), + ) + + it.live("identifies a directory used as an update target", () => + withTempTool((directory, registry) => + Effect.gen(function* () { + yield* Effect.promise(() => fs.mkdir(path.join(directory, "nested"))) + expect( + yield* executeTool(registry, call("*** Begin Patch\n*** Update File: nested\n@@\n-old\n+new\n*** End Patch")), + ).toEqual({ + type: "error", + value: `patch verification failed: Failed to read file to update ${path.join(directory, "nested")}: path is a directory`, }) }), ),