diff --git a/packages/core/src/tool/patch.txt b/packages/core/src/tool/patch.txt index 72c0489c0d3..1fd633790cf 100644 --- a/packages/core/src/tool/patch.txt +++ b/packages/core/src/tool/patch.txt @@ -31,3 +31,4 @@ It is important to remember: - You must include a header with your intended action (Add/Delete/Update) - You must prefix new lines with `+` even when creating a new file +- Use only one file operation per resolved path. Combine multiple edits to the same file into one Update File section with multiple @@ chunks. diff --git a/packages/core/src/tool/plugin/patch.ts b/packages/core/src/tool/plugin/patch.ts index c77e01d8b7f..a9b2e47dcc3 100644 --- a/packages/core/src/tool/plugin/patch.ts +++ b/packages/core/src/tool/plugin/patch.ts @@ -112,7 +112,6 @@ export const Plugin = { return yield* new ToolFailure({ message: "patch rejected: empty patch" }) } const prepared: Prepared[] = [] - const updates = new Map() const resolveTarget = Effect.fnUntraced(function* (value: string) { const target = yield* mutation.resolve({ path: value, kind: "file" }) if (!target.externalDirectory) return target @@ -131,6 +130,11 @@ export const Plugin = { for (const hunk of hunks) { yield* Effect.gen(function* () { const target = yield* resolveTarget(hunk.path) + if (prepared.some((change) => change.target.absolute === target.absolute)) { + return yield* new ToolFailure({ + message: `apply_patch verification failed: invalid patch: multiple operations target ${target.absolute}`, + }) + } if (hunk.type === "add") { const content = hunk.contents.endsWith("\n") || hunk.contents === "" ? hunk.contents : `${hunk.contents}\n` @@ -155,20 +159,15 @@ export const Plugin = { prepared.push({ ...hunk, target, before: content.text, after: "" }) return } - const previous = updates.get(target.absolute) - const original = - previous ?? - (yield* Effect.gen(function* () { - const content = yield* FileMutation.readText(environment.files, target.absolute).pipe( - Effect.mapError( - (error) => - new ToolFailure({ - message: `patch verification failed: Failed to read file to update ${target.absolute}: ${errorMessage(error)}`, - }), - ), - ) - return Bom.join(content.text, content.bom) - })) + const content = yield* FileMutation.readText(environment.files, target.absolute).pipe( + Effect.mapError( + (error) => + new ToolFailure({ + message: `patch verification failed: Failed to read file to update ${target.absolute}: ${errorMessage(error)}`, + }), + ), + ) + const original = Bom.join(content.text, content.bom) const before = Bom.split(original).text const update = yield* Effect.try({ try: () => Patch.derive(hunk.path, hunk.chunks, original), @@ -183,7 +182,6 @@ export const Plugin = { after: update.content, moveTarget, }) - if (!moveTarget) updates.set(target.absolute, Patch.joinBom(update.content, update.bom)) }).pipe( Effect.mapError((error) => error instanceof ToolFailure @@ -257,20 +255,9 @@ export const Plugin = { }), { discard: true }, ) - const formatTargets = prepared.reduce((result, change) => { - if (change.type !== "delete") { - result.add( - (change.type === "update" ? change.moveTarget : undefined)?.absolute ?? change.target.absolute, - ) - } - if (change.type === "delete" || (change.type === "update" && change.moveTarget)) { - result.delete(change.target.absolute) - } - return result - }, new Set()) const formatted = new Map() yield* Effect.forEach( - formatTargets, + [...new Set(applied.filter((item) => item.type !== "delete").map((item) => item.target))], (target) => Effect.gen(function* () { const current = yield* FileMutation.readText(environment.files, target).pipe( diff --git a/packages/core/test/tool-patch.test.ts b/packages/core/test/tool-patch.test.ts index dddd3a6ab1a..45bfad41c86 100644 --- a/packages/core/test/tool-patch.test.ts +++ b/packages/core/test/tool-patch.test.ts @@ -323,56 +323,39 @@ describe("PatchTool", () => { }), ) - it.live("does not format a file deleted after an update", () => + it.live("rejects multiple operations on the same resolved path before writing any files", () => withTempTool((directory, registry) => Effect.gen(function* () { - const target = path.join(directory, "removed.txt") - const formatted: string[] = [] - formatFile = (file) => - Effect.sync(() => { - formatted.push(file) - return false - }) + const target = path.join(directory, "duplicate.txt") yield* Effect.promise(() => fs.writeFile(target, "before\n")) - expect( - yield* executeTool( - registry, - call( - "*** Begin Patch\n*** Update File: removed.txt\n@@\n-before\n+after\n*** Delete File: removed.txt\n*** End Patch", - ), - ), - ).toMatchObject({ status: "completed" }) - expect(yield* exists(target)).toBe(false) - expect(formatted).toEqual([]) - }), - ), - ) - - it.live("formats only the destination of a file moved after an update", () => - withTempTool((directory, registry) => - Effect.gen(function* () { - const source = path.join(directory, "old.txt") - const destination = path.join(directory, "moved.txt") - const formatted: string[] = [] - formatFile = (file) => - Effect.promise(async () => { - formatted.push(file) - await fs.writeFile(file, (await fs.readFile(file, "utf8")).toUpperCase()) - return true - }) - yield* Effect.promise(() => fs.writeFile(source, "before\n")) - const settled = yield* executeTool( - registry, - call( - "*** Begin Patch\n*** Update File: old.txt\n@@\n-before\n+updated\n*** Update File: old.txt\n*** Move to: moved.txt\n@@\n-updated\n+after\n*** End Patch", - ), - ) - expect(settled.status).toBe("completed") - if (settled.status !== "completed") return - expect(settled.output.files[1]?.patch).toContain("+AFTER") - expect(yield* exists(source)).toBe(false) - expect(yield* Effect.promise(() => fs.readFile(destination, "utf8"))).toBe("AFTER\n") - expect(formatted).toEqual([destination]) + const operations = [ + "*** Add File: duplicate.txt\n+after", + "*** Update File: duplicate.txt\n@@\n-before\n+after", + "*** Delete File: duplicate.txt", + ] + for (const first of operations) { + for (const second of operations) { + for (const alias of ["duplicate.txt", "./duplicate.txt", target]) { + expect( + yield* executeTool( + registry, + call( + `*** Begin Patch\n*** Add File: earlier.txt\n+earlier\n${first}\n${second.replace("duplicate.txt", alias)}\n*** End Patch`, + ), + ), + ).toEqual({ + status: "error", + error: { + type: "tool.execution", + message: `apply_patch verification failed: invalid patch: multiple operations target ${target}`, + }, + }) + expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("before\n") + expect(yield* exists(path.join(directory, "earlier.txt"))).toBe(false) + } + } + } + expect(assertions).toEqual([]) }), ), ) @@ -689,17 +672,17 @@ describe("PatchTool", () => { ), ) - it.live("applies successive update operations to one file", () => + it.live("applies multiple chunks within one update operation", () => withTempTool((directory, registry) => Effect.gen(function* () { const target = path.join(directory, "successive.txt") yield* Effect.promise(() => fs.writeFile(target, "a\nb\n")) - yield* executeTool( - registry, - call( - "*** Begin Patch\n*** Update File: successive.txt\n@@\n-a\n+A\n*** Update File: successive.txt\n@@\n-b\n+B\n*** End Patch", + expect( + yield* executeTool( + registry, + call("*** Begin Patch\n*** Update File: successive.txt\n@@\n-a\n+A\n@@\n-b\n+B\n*** End Patch"), ), - ) + ).toMatchObject({ status: "completed" }) expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("A\nB\n") }), ),