diff --git a/packages/opencode/src/tool/apply_patch.ts b/packages/opencode/src/tool/apply_patch.ts index 3f89a63974b0..4a644334f2d7 100644 --- a/packages/opencode/src/tool/apply_patch.ts +++ b/packages/opencode/src/tool/apply_patch.ts @@ -53,6 +53,7 @@ export const ApplyPatchTool = Tool.define( } const instance = yield* InstanceState.context + const originals: Array<{ filePath: string; content: Uint8Array }> = [] // Validate file paths and check permissions const fileChanges: Array<{ @@ -113,6 +114,7 @@ export const ApplyPatchTool = Tool.define( } const source = yield* Bom.readFile(afs, filePath) + originals.push({ filePath, content: source.content }) const oldContent = source.text let newContent = oldContent let bom = source.bom @@ -168,6 +170,7 @@ export const ApplyPatchTool = Tool.define( ), ), ) + originals.push({ filePath, content: source.content }) const contentToDelete = source.text const deleteDiff = trimDiff(createTwoFilesPatch(filePath, filePath, contentToDelete, "")) @@ -214,6 +217,25 @@ export const ApplyPatchTool = Tool.define( }, }) + // Approval can outlive the contents used to prepare the patch. Check every + // source before writing any file so a stale later hunk leaves earlier ones untouched. + for (const original of originals) { + const current = yield* afs + .readFile(original.filePath) + .pipe(Effect.catchReason("PlatformError", "NotFound", () => Effect.succeed(undefined))) + if ( + !current || + current.length !== original.content.length || + !current.every((byte, index) => byte === original.content[index]) + ) { + return yield* Effect.fail( + new Error( + `apply_patch verification failed: File changed since it was read: ${original.filePath}. No patch changes were applied. Read the file again and retry.`, + ), + ) + } + } + // Apply the changes const updates: Array<{ file: string; event: "add" | "change" | "unlink" }> = [] diff --git a/packages/opencode/src/util/bom.ts b/packages/opencode/src/util/bom.ts index f015651e97fd..8c7b74c85c10 100644 --- a/packages/opencode/src/util/bom.ts +++ b/packages/opencode/src/util/bom.ts @@ -16,7 +16,8 @@ export function join(text: string, bom: boolean) { } export const readFile = Effect.fn("Bom.readFile")(function* (fs: FSUtil.Interface, filePath: string) { - return split(new TextDecoder("utf-8", { ignoreBOM: true }).decode(yield* fs.readFile(filePath))) + const content = yield* fs.readFile(filePath) + return { content, ...split(new TextDecoder("utf-8", { ignoreBOM: true }).decode(content)) } }) export const syncFile = Effect.fn("Bom.syncFile")(function* (fs: FSUtil.Interface, filePath: string, bom: boolean) { diff --git a/packages/opencode/test/tool/apply_patch.test.ts b/packages/opencode/test/tool/apply_patch.test.ts index 742036154b5f..4e06bc81ad12 100644 --- a/packages/opencode/test/tool/apply_patch.test.ts +++ b/packages/opencode/test/tool/apply_patch.test.ts @@ -401,6 +401,108 @@ describe("tool.apply_patch freeform", () => { }), ) + for (const operation of ["update", "delete", "move"] as const) { + it.instance(`rejects a stale ${operation} source before applying any file changes`, () => + Effect.gen(function* () { + const test = yield* TestInstance + const first = path.join(test.directory, "first.txt") + const second = path.join(test.directory, "second.txt") + const destination = path.join(test.directory, "moved.txt") + yield* writeText(first, "first\n") + yield* writeText(second, "second\n") + + const hunk = + operation === "delete" + ? "*** Delete File: second.txt\n" + : `*** Update File: second.txt\n${operation === "move" ? "*** Move to: moved.txt\n" : ""}@@\n-second\n+patched\n` + const patchText = `*** Begin Patch\n*** Update File: first.txt\n@@\n-first\n+patched\n${hunk}*** End Patch` + const ctx: ToolCtx = { + ...baseCtx, + ask: () => writeText(second, "edited during approval\n"), + } + + yield* expectFailure(execute({ patchText }, ctx), "File changed since it was read") + expect(yield* readText(first)).toBe("first\n") + expect(yield* readText(second)).toBe("edited during approval\n") + yield* expectReadFailure(destination) + }), + ) + } + + it.instance("rejects a BOM-only change during approval before creating any files", () => + Effect.gen(function* () { + const test = yield* TestInstance + const target = path.join(test.directory, "source.txt") + yield* writeText(target, "original\n") + const ctx: ToolCtx = { + ...baseCtx, + ask: () => writeText(target, "\uFEFForiginal\n"), + } + + yield* expectFailure( + execute( + { + patchText: + "*** Begin Patch\n*** Add File: created.txt\n+created\n*** Update File: source.txt\n@@\n-original\n+patched\n*** End Patch", + }, + ctx, + ), + "File changed since it was read", + ) + expect(yield* readText(target)).toBe("\uFEFForiginal\n") + yield* expectReadFailure(path.join(test.directory, "created.txt")) + }), + ) + + it.instance("rejects a source removed during approval before changing earlier files", () => + Effect.gen(function* () { + const test = yield* TestInstance + const first = path.join(test.directory, "first.txt") + const second = path.join(test.directory, "second.txt") + yield* writeText(first, "first\n") + yield* writeText(second, "second\n") + const ctx: ToolCtx = { + ...baseCtx, + ask: () => Effect.promise(() => fs.unlink(second)), + } + + yield* expectFailure( + execute( + { + patchText: + "*** Begin Patch\n*** Update File: first.txt\n@@\n-first\n+patched\n*** Delete File: second.txt\n*** End Patch", + }, + ctx, + ), + "File changed since it was read", + ) + expect(yield* readText(first)).toBe("first\n") + yield* expectReadFailure(second) + }), + ) + + it.instance("rejects byte changes hidden by UTF-8 replacement during approval", () => + Effect.gen(function* () { + const test = yield* TestInstance + const afs = yield* FSUtil.Service + const target = path.join(test.directory, "source.txt") + yield* afs.writeFile(target, new Uint8Array([0x80, 0x0a])) + const ctx: ToolCtx = { + ...baseCtx, + ask: () => afs.writeFile(target, new Uint8Array([0x81, 0x0a])).pipe(Effect.orDie), + } + + yield* expectFailure( + execute( + { patchText: "*** Begin Patch\n*** Update File: source.txt\n@@\n-\uFFFD\n+patched\n*** End Patch" }, + ctx, + ), + "File changed since it was read", + ) + expect(Array.from(yield* afs.readFile(target))).toEqual([0x81, 0x0a]) + }), + ) + it.instance("supports end of file anchor", () => Effect.gen(function* () { const test = yield* TestInstance