fix(core): reject duplicate patch targets like Codex

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.

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<string, string>()
               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<string>())
               const formatted = new Map<string, string>()
               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(

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")
       }),
     ),