Mirror_Repos/anomalyco_opencode
2
0
Fork 0
Code Issues Pull Requests Actions 71 Packages Projects Releases Wiki Activity

fix(core): improve patch errors

Browse Source
This commit is contained in:
Aiden Cline
2026-07-22 13:54:33 -05:00
parent 2271f9b222
commit 4de6a5f60a
4 changed files with 207 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/core/src/tool/patch.ts
+48 -20
View File
@@ -84,11 +84,24 @@ export const Plugin = {
execute: (input, context) => {
const applied: Array<typeof Applied.Type> = []
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(
packages/core/test/patch.test.ts
+9 -3
View File
@@ -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")
})
})
packages/core/test/tool-patch.test.ts
+118 -8
View File
@@ -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<void> => 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 ",
"<<EOF\n*** Begin Patch\n*** End Patch\nEOF",
"*** Begin Patch\n*** Environment ID: remote\n*** End Patch",
]) {
expect(yield* executeTool(registry, call(patchText))).toEqual({
type: "error",
value: "patch rejected: empty patch",
})
}
}),
),
)
@@ -680,7 +717,10 @@ describe("PatchTool", () => {
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")
}),
),
)
packages/util/src/patch.ts
+32 -3
View File
@@ -69,7 +69,7 @@ export function parse(patchText: string): Result.Result<ReadonlyArray<Hunk>, 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<ReadonlyArray<Hunk>, 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<ReadonlyArray<Hunk>, 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<string>,
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<string>, 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