From 4de6a5f60a5e11aefbd25e62588158508ccbf79c Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Wed, 22 Jul 2026 13:54:33 -0500 Subject: [PATCH] fix(core): improve patch errors --- packages/core/src/tool/patch.ts | 68 ++++++++++---- packages/core/test/patch.test.ts | 12 ++- packages/core/test/tool-patch.test.ts | 126 ++++++++++++++++++++++++-- packages/util/src/patch.ts | 35 ++++++- 4 files changed, 207 insertions(+), 34 deletions(-) diff --git a/packages/core/src/tool/patch.ts b/packages/core/src/tool/patch.ts index 7a70b3c5c3..8e09354922 100644 --- a/packages/core/src/tool/patch.ts +++ b/packages/core/src/tool/patch.ts @@ -84,11 +84,24 @@ export const Plugin = { execute: (input, context) => { const applied: Array = [] const fail = (path: string, error?: unknown) => { - const prefix = + const detail = error === undefined ? "" : `: ${errorMessage(error)}` + if (applied.length === 0) { + return new ToolFailure({ message: `Unable to apply patch at ${path}${detail}`, error }) + } + return new ToolFailure({ + message: `Patch partially applied before failing at ${path}${detail}. Applied: ${applied.map((item) => item.resource).join(", ")}`, + error, + }) + } + const failMoveRemoval = (source: string, destination: string, error: unknown) => { + const previous = applied.length === 0 - ? `Unable to apply patch at ${path}` - : `Patch partially applied before failing at ${path}. Applied: ${applied.map((item) => item.resource).join(", ")}` - return new ToolFailure({ message: prefix, error }) + ? "" + : `. Applied before move: ${applied.map((item) => item.resource).join(", ")}` + return new ToolFailure({ + message: `Patch partially applied while moving ${source} to ${destination}: wrote ${destination} but failed to remove ${source}: ${errorMessage(error)}${previous}`, + error, + }) } return Effect.gen(function* () { const source = { @@ -99,15 +112,11 @@ export const Plugin = { if (!input.patchText) return yield* new ToolFailure({ message: "patchText is required" }) const hunks = yield* Effect.fromResult(Patch.parse(input.patchText)).pipe( Effect.mapError( - (error) => new ToolFailure({ message: `patch verification failed: ${error.message}` }), + (error) => new ToolFailure({ message: `patch verification failed: ${error.message}`, error }), ), ) if (hunks.length === 0) { - const normalized = input.patchText.replace(/\r\n/g, "\n").replace(/\r/g, "\n").trim() - if (normalized === "*** Begin Patch\n*** End Patch") { - return yield* new ToolFailure({ message: "patch rejected: empty patch" }) - } - return yield* new ToolFailure({ message: "patch verification failed: no hunks found" }) + return yield* new ToolFailure({ message: "patch rejected: empty patch" }) } const prepared: Prepared[] = [] const targets: Target[] = [] @@ -147,7 +156,8 @@ export const Plugin = { Effect.mapError( (error) => new ToolFailure({ - message: `patch verification failed: ${error instanceof Error ? error.message : String(error)}`, + message: `patch verification failed: Failed to read file to delete ${target.canonical}: ${errorMessage(error)}`, + error, }), ), ) @@ -163,7 +173,8 @@ export const Plugin = { Effect.mapError( (error) => new ToolFailure({ - message: `patch verification failed: Failed to read file to update ${target.canonical}: ${error instanceof Error ? error.message : String(error)}`, + message: `patch verification failed: Failed to read file to update ${target.canonical}: ${errorMessage(error)}`, + error, }), ), ) @@ -177,7 +188,8 @@ export const Plugin = { Effect.mapError( (error) => new ToolFailure({ - message: `patch verification failed: Failed to read file to update ${target.canonical}: ${error instanceof Error ? error.message : String(error)}`, + message: `patch verification failed: Failed to read file to update ${target.canonical}: ${errorMessage(error)}`, + error, }), ), ), @@ -186,7 +198,8 @@ export const Plugin = { const before = original.replace(/^\uFEFF/, "") const update = yield* Effect.try({ try: () => Patch.derive(hunk.path, hunk.chunks, original), - catch: (error) => new ToolFailure({ message: `patch verification failed: ${String(error)}` }), + catch: (error) => + new ToolFailure({ message: `patch verification failed: ${errorMessage(error)}`, error }), }) const moveTarget = hunk.movePath ? resolveTarget(location, hunk.movePath) : undefined if (moveTarget) targets.push(moveTarget) @@ -241,7 +254,7 @@ export const Plugin = { change.contents.endsWith("\n") || change.contents === "" ? change.contents : `${change.contents}\n`, - ) + ).pipe(Effect.mapError((error) => fail(change.target.resource, error))) applied.push({ type: change.type, resource: change.target.resource, @@ -250,7 +263,9 @@ export const Plugin = { return } if (change.type === "delete") { - yield* fs.remove(change.target.canonical) + yield* fs + .remove(change.target.canonical) + .pipe(Effect.mapError((error) => fail(change.target.resource, error))) applied.push({ type: change.type, resource: change.target.resource, @@ -259,8 +274,15 @@ export const Plugin = { return } if (change.moveTarget) { - yield* fs.writeWithDirs(change.moveTarget.canonical, change.content) - yield* fs.remove(change.target.canonical) + const moveTarget = change.moveTarget + yield* fs + .writeWithDirs(moveTarget.canonical, change.content) + .pipe(Effect.mapError((error) => fail(moveTarget.resource, error))) + yield* fs.remove(change.target.canonical).pipe( + Effect.mapError((error) => + failMoveRemoval(change.target.resource, moveTarget.resource, error), + ), + ) applied.push({ type: change.type, resource: change.moveTarget.resource, @@ -268,13 +290,15 @@ export const Plugin = { }) return } - yield* fs.writeWithDirs(change.target.canonical, change.content) + yield* fs + .writeWithDirs(change.target.canonical, change.content) + .pipe(Effect.mapError((error) => fail(change.target.resource, error))) applied.push({ type: change.type, resource: change.target.resource, target: change.target.canonical, }) - }).pipe(Effect.mapError((error) => fail(change.path, error))), + }), { discard: true }, ) return { applied, files: patchFiles } @@ -303,6 +327,10 @@ export const Plugin = { }), } +function errorMessage(error: unknown) { + return error instanceof Error ? error.message : String(error) +} + function patchFile(change: Prepared): typeof FileDiff.Info.Type { const target = (change.type === "update" ? change.moveTarget : undefined)?.resource ?? change.target.resource const patch = trimDiff( diff --git a/packages/core/test/patch.test.ts b/packages/core/test/patch.test.ts index 18dd3695ce..3ac8154b3b 100644 --- a/packages/core/test/patch.test.ts +++ b/packages/core/test/patch.test.ts @@ -327,6 +327,12 @@ describe("Patch", () => { ).toThrow("Failed to find expected lines") }) + test("identifies a missing blank line", () => { + expect(() => + Patch.derive("update.txt", [{ oldLines: [""], newLines: ["added"] }], "content\n"), + ).toThrow("Failed to find an expected blank line in update.txt") + }) + test("parses an update without an explicit first chunk header", () => { expect(parse("*** Begin Patch\n*** Update File: file.txt\n import foo\n+bar\n*** End Patch")).toEqual([ { @@ -413,10 +419,10 @@ describe("Patch", () => { test("rejects invalid add and delete lines", () => { expect(() => parse("*** Begin Patch\n*** Add File: file.txt\nbad\n*** End Patch")).toThrow( - "Invalid hunk at line 3: 'bad' is not a valid hunk header", + "Invalid hunk at line 3: Invalid Add File line for 'file.txt': expected a line starting with '+', got 'bad'", ) expect(() => parse("*** Begin Patch\n*** Delete File: file.txt\nbad\n*** End Patch")).toThrow( - "Invalid hunk at line 3: 'bad' is not a valid hunk header", + "Invalid hunk at line 3: Unexpected line after Delete File 'file.txt': 'bad'. Delete hunks do not contain body lines", ) }) @@ -478,6 +484,6 @@ describe("Patch", () => { } expect(() => parse("*** Begin Patch\n*** Update File: old.txt\n*** Move to: \n@@\n-old\n+new\n*** End Patch"), - ).toThrow("Invalid hunk at line 3: '*** Move to:' is not a valid hunk header") + ).toThrow("Invalid hunk at line 3: Move destination for 'old.txt' must not be empty") }) }) diff --git a/packages/core/test/tool-patch.test.ts b/packages/core/test/tool-patch.test.ts index f43b49342c..e63468526e 100644 --- a/packages/core/test/tool-patch.test.ts +++ b/packages/core/test/tool-patch.test.ts @@ -2,6 +2,7 @@ import fs from "fs/promises" import path from "path" import { describe, expect } from "bun:test" import { Effect, Exit, Layer, Schema } from "effect" +import { systemError } from "effect/PlatformError" import { AppNodeBuilder } from "@opencode-ai/core/effect/app-node-builder" import { LayerNode } from "@opencode-ai/util/effect/layer-node" import { FSUtil } from "@opencode-ai/util/fs-util" @@ -28,6 +29,8 @@ const sessionID = SessionV2.ID.make("ses_patch_tool_test") const assertions: PermissionV2.AssertInput[] = [] let denyAction: string | undefined let failRemoveTarget: string | undefined +let failRemoveErrorTarget: string | undefined +let failWriteTarget: string | undefined let readsBeforeEditApproval = 0 let editApproved = false let afterEditApproval = (): Effect.Effect => Effect.void @@ -65,6 +68,8 @@ const reset = () => { assertions.length = 0 denyAction = undefined failRemoveTarget = undefined + failRemoveErrorTarget = undefined + failWriteTarget = undefined readsBeforeEditApproval = 0 editApproved = false afterEditApproval = () => Effect.void @@ -82,8 +87,33 @@ const filesystem = Layer.effect( }).pipe(Effect.andThen(fs.readFile(target))), remove: (target, options) => { if (failRemoveTarget && path.basename(target) === failRemoveTarget) return Effect.die("forced remove failure") + if (failRemoveErrorTarget && path.basename(target) === failRemoveErrorTarget) { + return Effect.fail( + systemError({ + _tag: "Unknown", + module: "FileSystem", + method: "remove", + description: "forced remove failure", + pathOrDescriptor: target, + }), + ) + } return fs.remove(target, options) }, + writeWithDirs: (target, content, mode) => { + if (failWriteTarget && path.basename(target) === failWriteTarget) { + return Effect.fail( + systemError({ + _tag: "Unknown", + module: "FileSystem", + method: "writeWithDirs", + description: "forced write failure", + pathOrDescriptor: target, + }), + ) + } + return fs.writeWithDirs(target, content, mode) + }, }) }), ).pipe(Layer.provide(LayerNode.compile(FSUtil.node))) @@ -488,10 +518,17 @@ describe("PatchTool", () => { it.live("rejects an empty patch", () => withTempTool((_directory, registry) => Effect.gen(function* () { - expect(yield* executeTool(registry, call("*** Begin Patch\n*** End Patch"))).toEqual({ - type: "error", - value: "patch rejected: empty patch", - }) + for (const patchText of [ + "*** Begin Patch\n*** End Patch", + " *** Begin Patch \n *** End Patch ", + "< { registry, call("*** Begin Patch\n*** Update File: unchanged.txt\n@@\n-missing\n+changed\n*** End Patch"), ), - ).toMatchObject({ type: "error", value: expect.stringContaining("Failed to find expected lines") }) + ).toEqual({ + type: "error", + value: "patch verification failed: Failed to find expected lines in unchanged.txt:\nmissing", + }) expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("line1\nline2\n") }), ), @@ -718,12 +758,82 @@ describe("PatchTool", () => { ), ) - it.live("rejects a delete when the target file is missing", () => - withTempTool((_directory, registry) => + it.live("identifies a missing delete target", () => + withTempTool((directory, registry) => Effect.gen(function* () { expect( yield* executeTool(registry, call("*** Begin Patch\n*** Delete File: missing.txt\n*** End Patch")), - ).toMatchObject({ type: "error", value: expect.stringContaining("patch verification failed") }) + ).toMatchObject({ + type: "error", + value: expect.stringContaining( + `patch verification failed: Failed to read file to delete ${path.join(directory, "missing.txt")}: `, + ), + }) + }), + ), + ) + + it.live("reports the failing destination and filesystem error", () => + withTempTool((directory, registry) => + Effect.gen(function* () { + yield* Effect.promise(() => fs.writeFile(path.join(directory, "old.txt"), "before\n")) + failWriteTarget = "new.txt" + expect( + yield* executeTool( + registry, + call( + "*** Begin Patch\n*** Update File: old.txt\n*** Move to: new.txt\n@@\n-before\n+after\n*** End Patch", + ), + ), + ).toEqual({ + type: "error", + value: `Unable to apply patch at new.txt: Unknown: FileSystem.writeWithDirs (${path.join(directory, "new.txt")}): forced write failure`, + }) + expect(yield* Effect.promise(() => fs.readFile(path.join(directory, "old.txt"), "utf8"))).toBe("before\n") + expect(yield* exists(path.join(directory, "new.txt"))).toBe(false) + }), + ), + ) + + it.live("reports the successful prefix and filesystem error", () => + withTempTool((directory, registry) => + Effect.gen(function* () { + failWriteTarget = "second.txt" + expect( + yield* executeTool( + registry, + call( + "*** Begin Patch\n*** Add File: first.txt\n+first\n*** Add File: second.txt\n+second\n*** End Patch", + ), + ), + ).toEqual({ + type: "error", + value: `Patch partially applied before failing at second.txt: Unknown: FileSystem.writeWithDirs (${path.join(directory, "second.txt")}): forced write failure. Applied: first.txt`, + }) + expect(yield* Effect.promise(() => fs.readFile(path.join(directory, "first.txt"), "utf8"))).toBe("first\n") + expect(yield* exists(path.join(directory, "second.txt"))).toBe(false) + }), + ), + ) + + it.live("reports a destination written before move removal fails", () => + withTempTool((directory, registry) => + Effect.gen(function* () { + yield* Effect.promise(() => fs.writeFile(path.join(directory, "old.txt"), "before\n")) + failRemoveErrorTarget = "old.txt" + expect( + yield* executeTool( + registry, + call( + "*** Begin Patch\n*** Update File: old.txt\n*** Move to: new.txt\n@@\n-before\n+after\n*** End Patch", + ), + ), + ).toEqual({ + type: "error", + value: `Patch partially applied while moving old.txt to new.txt: wrote new.txt but failed to remove old.txt: Unknown: FileSystem.remove (${path.join(directory, "old.txt")}): forced remove failure`, + }) + expect(yield* Effect.promise(() => fs.readFile(path.join(directory, "old.txt"), "utf8"))).toBe("before\n") + expect(yield* Effect.promise(() => fs.readFile(path.join(directory, "new.txt"), "utf8"))).toBe("after\n") }), ), ) diff --git a/packages/util/src/patch.ts b/packages/util/src/patch.ts index 843e14efa1..8a94a1948e 100644 --- a/packages/util/src/patch.ts +++ b/packages/util/src/patch.ts @@ -69,7 +69,7 @@ export function parse(patchText: string): Result.Result, Par } if (header.startsWith("*** Add File: ")) { const path = header.slice("*** Add File: ".length).trim() - const parsed = parseAdd(lines, index + 1, end) + const parsed = parseAdd(lines, index + 1, end, path) if ("error" in parsed) return Result.fail(parsed.error) hunks.push({ type: "add", path, contents: parsed.content }) index = parsed.next @@ -77,6 +77,16 @@ export function parse(patchText: string): Result.Result, Par } if (header.startsWith("*** Delete File: ")) { const path = header.slice("*** Delete File: ".length).trim() + const next = lines[index + 1]?.trim() + if (index + 1 < end && next !== undefined && !isBoundary(next)) { + return Result.fail( + new InvalidHunkError({ + line: next, + lineNumber: index + 2, + reason: `Unexpected line after Delete File '${path}': '${next}'. Delete hunks do not contain body lines`, + }), + ) + } hunks.push({ type: "delete", path }) index++ continue @@ -90,7 +100,13 @@ export function parse(patchText: string): Result.Result, Par if (move === "*** Move to:" || move?.startsWith("*** Move to: ")) { movePath = move.slice("*** Move to: ".length).trim() if (!movePath) { - return Result.fail(new InvalidHunkError({ line: lines[next]!.trim(), lineNumber: next + 1 })) + return Result.fail( + new InvalidHunkError({ + line: lines[next]!.trim(), + lineNumber: next + 1, + reason: `Move destination for '${path}' must not be empty`, + }), + ) } next++ } @@ -126,12 +142,20 @@ function parseAdd( lines: ReadonlyArray, start: number, end: number, + path: string, ): { content: string; next: number } | { error: InvalidHunkError } { const content: string[] = [] let index = start while (index < end && !isBoundary(lines[index]!.trim())) { if (!lines[index]!.startsWith("+")) { - return { error: new InvalidHunkError({ line: lines[index]!.trim(), lineNumber: index + 1 }) } + const line = lines[index]!.trim() + return { + error: new InvalidHunkError({ + line, + lineNumber: index + 1, + reason: `Invalid Add File line for '${path}': expected a line starting with '+', got '${line}'`, + }), + } } content.push(lines[index]!.slice(1)) index++ @@ -303,6 +327,11 @@ function computeReplacements(lines: ReadonlyArray, path: string, chunks: if (newLines.at(-1) === "") newLines = newLines.slice(0, -1) found = seek(lines, oldLines, lineIndex, chunk.endOfFile) } + if (found === -1 && chunk.oldLines.every((line) => line === "")) { + const expected = + chunk.oldLines.length === 1 ? "an expected blank line" : `${chunk.oldLines.length} consecutive blank lines` + throw new Error(`Failed to find ${expected} in ${path}`) + } if (found === -1) throw new Error(`Failed to find expected lines in ${path}:\n${chunk.oldLines.join("\n")}`) replacements.push([found, oldLines.length, newLines]) lineIndex = found + oldLines.length