diff --git a/packages/core/src/tool/patch.ts b/packages/core/src/tool/patch.ts index 5e90c355966..c7307c069ef 100644 --- a/packages/core/src/tool/patch.ts +++ b/packages/core/src/tool/patch.ts @@ -5,6 +5,7 @@ import { ToolFailure } from "@opencode-ai/ai" import { FileDiff } from "@opencode-ai/schema/file-diff" import { createTwoFilesPatch, diffLines } from "diff" import { Effect, Schema } from "effect" +import { PlatformError } from "effect/PlatformError" import path from "path" import { FSUtil } from "@opencode-ai/util/fs-util" import { Location } from "../location" @@ -81,12 +82,11 @@ export const Plugin = { output: Output, execute: (input, context) => { const applied: Array = [] - const fail = (path: string, error?: unknown) => { - const prefix = - 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 }) + const fail = (operation: string, error: unknown) => { + const completed = applied.map((item) => item.resource).join(", ") + return new ToolFailure({ + message: `${operation}: ${errorMessage(error)}${completed ? `. Completed before failure: ${completed}` : ""}`, + }) } return Effect.gen(function* () { const source = { @@ -101,11 +101,7 @@ export const Plugin = { ), ) 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[] = [] @@ -145,7 +141,7 @@ 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 delete ${target.resource}: ${errorMessage(error)}`, }), ), ) @@ -161,7 +157,7 @@ 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)}`, }), ), ) @@ -175,7 +171,7 @@ 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)}`, }), ), ), @@ -184,7 +180,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)}` }), }) const moveTarget = hunk.movePath ? resolveTarget(location, hunk.movePath) : undefined if (moveTarget) targets.push(moveTarget) @@ -211,7 +208,13 @@ export const Plugin = { moveTarget, }) if (!moveTarget) updates.set(target.canonical, Patch.joinBom(update.content, update.bom)) - }).pipe(Effect.mapError((error) => (error instanceof ToolFailure ? error : fail(hunk.path, error)))) + }).pipe( + Effect.mapError((error) => + error instanceof ToolFailure + ? error + : new ToolFailure({ message: `Unable to prepare patch at ${hunk.path}`, error }), + ), + ) } const patchFiles = prepared.map(patchFile) @@ -234,12 +237,16 @@ export const Plugin = { (change) => Effect.gen(function* () { if (change.type === "add") { - yield* fs.writeWithDirs( - change.target.canonical, - change.contents.endsWith("\n") || change.contents === "" - ? change.contents - : `${change.contents}\n`, - ) + yield* fs + .writeWithDirs( + change.target.canonical, + change.contents.endsWith("\n") || change.contents === "" + ? change.contents + : `${change.contents}\n`, + ) + .pipe( + Effect.mapError((error) => fail(`Failed to write ${change.target.resource}`, error)), + ) applied.push({ type: change.type, resource: change.target.resource, @@ -248,7 +255,11 @@ 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(`Failed to delete ${change.target.resource}`, error)), + ) applied.push({ type: change.type, resource: change.target.resource, @@ -257,8 +268,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(`Failed to write ${moveTarget.resource}`, error))) + yield* fs.remove(change.target.canonical).pipe( + Effect.mapError((error) => + fail(`Wrote ${moveTarget.resource} but failed to remove ${change.target.resource}`, error), + ), + ) applied.push({ type: change.type, resource: change.moveTarget.resource, @@ -266,13 +284,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(`Failed to write ${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 } @@ -282,7 +302,11 @@ export const Plugin = { content: toModelOutput(output), metadata: { files: output.files }, })), - Effect.mapError((error) => (error instanceof ToolFailure ? error : fail("patch", error))), + Effect.mapError((error) => + error instanceof ToolFailure + ? error + : new ToolFailure({ message: "Unable to apply patch", error }), + ), ) }, }), @@ -306,6 +330,14 @@ export const Plugin = { }), } +function errorMessage(error: unknown) { + if (error instanceof PlatformError) { + if (error.reason._tag === "NotFound") return "file does not exist" + return error.reason.description ?? error.reason.message + } + 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 18dd3695ce0..bd75b266479 100644 --- a/packages/core/test/patch.test.ts +++ b/packages/core/test/patch.test.ts @@ -246,6 +246,35 @@ describe("Patch", () => { ).toBe("line 1\nLINE 2\nline 3\nLINE 4\n") }) + test("appends a pure-addition chunk to a nonempty file", () => { + expect(Patch.derive("update.txt", [{ oldLines: [], newLines: ["added 1", "added 2"] }], "line 1\nline 2\n").content).toBe( + "line 1\nline 2\nadded 1\nadded 2\n", + ) + }) + + test("applies a pure-addition chunk after an earlier replacement", () => { + expect( + Patch.derive( + "update.txt", + [ + { oldLines: [], newLines: ["after-context", "second-line"] }, + { oldLines: ["line1", "line2", "line3"], newLines: ["line1", "line2-replacement"] }, + ], + "line1\nline2\nline3\n", + ).content, + ).toBe("line1\nline2-replacement\nafter-context\nsecond-line\n") + }) + + test("applies a deletion-only update chunk", () => { + expect( + Patch.derive( + "update.txt", + [{ oldLines: ["line1", "line2", "line3"], newLines: ["line1", "line3"] }], + "line1\nline2\nline3\n", + ).content, + ).toBe("line1\nline3\n") + }) + test("updates empty files and adds a trailing newline", () => { expect(Patch.derive("empty.txt", [{ oldLines: [], newLines: ["First line"] }], "").content).toBe("First line\n") expect(Patch.derive("no-newline.txt", [{ oldLines: ["old"], newLines: ["new"] }], "old").content).toBe("new\n") @@ -327,6 +356,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,11 +448,14 @@ 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", ) + expect(() => + parse("*** Begin Patch\n*** Delete File: file.txt\n*** Frobnicate File: next.txt\n*** End Patch"), + ).toThrow("Invalid hunk at line 3: '*** Frobnicate File: next.txt' is not a valid hunk header") }) test("rejects an empty update hunk", () => { @@ -478,6 +516,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 cfae81eef86..64616e7d2ab 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))) @@ -302,6 +332,27 @@ describe("PatchTool", () => { ), ) + it.live("moves a file without changing its contents", () => + withTempTool((directory, registry) => + Effect.gen(function* () { + const source = path.join(directory, "old.txt") + const destination = path.join(directory, "moved.txt") + yield* Effect.promise(() => fs.writeFile(source, "same\n")) + expect( + yield* executeTool( + registry, + call("*** Begin Patch\n*** Update File: old.txt\n*** Move to: moved.txt\n@@\n same\n*** End Patch"), + ), + ).toMatchObject({ + status: "completed", + content: [{ type: "text", text: "Success. Updated the following files:\nM moved.txt" }], + }) + expect(yield* exists(source)).toBe(false) + expect(yield* Effect.promise(() => fs.readFile(destination, "utf8"))).toBe("same\n") + }), + ), + ) + it.live("moves a symlink without deleting its target", () => withTempTool((directory, registry) => Effect.gen(function* () { @@ -451,10 +502,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({ - status: "error", - error: { type: "tool.execution", message: "patch rejected: empty patch" }, - }) + for (const patchText of [ + "*** Begin Patch\n*** End Patch", + " *** Begin Patch \n *** End Patch ", + "< { ), ).toMatchObject({ status: "error", - error: { message: expect.stringContaining("Failed to find expected lines") }, + error: { + type: "tool.execution", + message: "patch verification failed: Failed to find expected lines in unchanged.txt:\nmissing", + }, }) expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("line1\nline2\n") }), @@ -569,12 +630,83 @@ describe("PatchTool", () => { ), ) - it.live("rejects a delete when the target file is missing", () => + 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({ status: "error", error: { message: expect.stringContaining("patch verification failed") } }) + ).toEqual({ + status: "error", + error: { + type: "tool.execution", + message: "patch verification failed: Failed to delete missing.txt: file does not exist", + }, + }) + }), + ), + ) + + 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({ + status: "error", + error: { type: "tool.execution", message: "Failed to write 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({ + status: "error", + error: { + type: "tool.execution", + message: "Failed to write second.txt: forced write failure. Completed before failure: 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({ + status: "error", + error: { + type: "tool.execution", + message: "Wrote new.txt but failed to remove 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") }), ), ) @@ -628,7 +760,7 @@ describe("PatchTool", () => { registry, call(`*** Begin Patch\n*** Update File: ${target}\n@@\n-before\n+after\n*** End Patch`), ), - ).toMatchObject({ status: "error" }) + ).toMatchObject({ status: "error", error: { type: "permission.rejected" } }) expect(assertions.map((input) => input.action)).toEqual(["external_directory"]) expect(readsBeforeEditApproval).toBe(0) expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("before\n") @@ -645,6 +777,24 @@ describe("PatchTool", () => { ), ) + it.live("preserves edit permission rejection", () => + withTempTool((directory, registry) => + Effect.gen(function* () { + const target = path.join(directory, "target.txt") + yield* Effect.promise(() => fs.writeFile(target, "before\n")) + denyAction = "edit" + expect( + yield* executeTool( + registry, + call("*** Begin Patch\n*** Update File: target.txt\n@@\n-before\n+after\n*** End Patch"), + ), + ).toMatchObject({ status: "error", error: { type: "permission.rejected" } }) + expect(assertions.map((input) => input.action)).toEqual(["edit"]) + expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("before\n") + }), + ), + ) + it.live("treats a sibling path inside the project worktree as internal", () => Effect.acquireUseRelease( Effect.promise(() => tmpdir()), diff --git a/packages/util/src/patch.ts b/packages/util/src/patch.ts index 843e14efa14..54766c5a61e 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,19 @@ 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)) { + if (next.startsWith("*** ")) { + return Result.fail(new InvalidHunkError({ line: next, lineNumber: index + 2 })) + } + 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 +103,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 +145,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 +330,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