Просмотр исходного кода

fix: preserve BOM in text tool round-trips (#23797)

Luke Parker 4 месяцев назад
Родитель
Сommit
8113a4360e

+ 11 - 6
packages/opencode/src/format/index.ts

@@ -25,7 +25,7 @@ export type Status = z.infer<typeof Status>
 export interface Interface {
   readonly init: () => Effect.Effect<void>
   readonly status: () => Effect.Effect<Status[]>
-  readonly file: (filepath: string) => Effect.Effect<void>
+  readonly file: (filepath: string) => Effect.Effect<boolean>
 }
 
 export class Service extends Context.Service<Service, Interface>()("@opencode/Format") {}
@@ -70,16 +70,19 @@ export const layer = Layer.effect(
               }
             }),
           )
-          return checks.filter((x) => x.cmd).map((x) => ({ item: x.item, cmd: x.cmd! }))
+          return checks
+            .filter((x): x is { item: Formatter.Info; cmd: string[] } => x.cmd !== false)
+            .map((x) => ({ item: x.item, cmd: x.cmd }))
         }
 
         function formatFile(filepath: string) {
           return Effect.gen(function* () {
             log.info("formatting", { file: filepath })
-            const ext = path.extname(filepath)
+            const formatters = yield* Effect.promise(() => getFormatter(path.extname(filepath)))
 
-            for (const { item, cmd } of yield* Effect.promise(() => getFormatter(ext))) {
-              if (cmd === false) continue
+            if (!formatters.length) return false
+
+            for (const { item, cmd } of formatters) {
               log.info("running", { command: cmd })
               const replaced = cmd.map((x) => x.replace("$FILE", filepath))
               const dir = yield* InstanceState.directory
@@ -113,6 +116,8 @@ export const layer = Layer.effect(
                 })
               }
             }
+
+            return true
           })
         }
 
@@ -188,7 +193,7 @@ export const layer = Layer.effect(
 
     const file = Effect.fn("Format.file")(function* (filepath: string) {
       const { formatFile } = yield* InstanceState.get(state)
-      yield* formatFile(filepath)
+      return yield* formatFile(filepath)
     })
 
     return Service.of({ init, status, file })

+ 11 - 7
packages/opencode/src/patch/index.ts

@@ -3,6 +3,7 @@ import * as path from "path"
 import * as fs from "fs/promises"
 import { readFileSync } from "fs"
 import { Log } from "../util"
+import * as Bom from "../util/bom"
 
 const log = Log.create({ service: "patch" })
 
@@ -305,18 +306,19 @@ export function maybeParseApplyPatch(
 interface ApplyPatchFileUpdate {
   unified_diff: string
   content: string
+  bom: boolean
 }
 
 export function deriveNewContentsFromChunks(filePath: string, chunks: UpdateFileChunk[]): ApplyPatchFileUpdate {
   // Read original file content
-  let originalContent: string
+  let originalContent: ReturnType<typeof Bom.split>
   try {
-    originalContent = readFileSync(filePath, "utf-8")
+    originalContent = Bom.split(readFileSync(filePath, "utf-8"))
   } catch (error) {
     throw new Error(`Failed to read file ${filePath}: ${error}`, { cause: error })
   }
 
-  let originalLines = originalContent.split("\n")
+  let originalLines = originalContent.text.split("\n")
 
   // Drop trailing empty element for consistent line counting
   if (originalLines.length > 0 && originalLines[originalLines.length - 1] === "") {
@@ -331,14 +333,16 @@ export function deriveNewContentsFromChunks(filePath: string, chunks: UpdateFile
     newLines.push("")
   }
 
-  const newContent = newLines.join("\n")
+  const next = Bom.split(newLines.join("\n"))
+  const newContent = next.text
 
   // Generate unified diff
-  const unifiedDiff = generateUnifiedDiff(originalContent, newContent)
+  const unifiedDiff = generateUnifiedDiff(originalContent.text, newContent)
 
   return {
     unified_diff: unifiedDiff,
     content: newContent,
+    bom: originalContent.bom || next.bom,
   }
 }
 
@@ -553,13 +557,13 @@ export async function applyHunksToFiles(hunks: Hunk[]): Promise<AffectedPaths> {
             await fs.mkdir(moveDir, { recursive: true })
           }
 
-          await fs.writeFile(hunk.move_path, fileUpdate.content, "utf-8")
+          await fs.writeFile(hunk.move_path, Bom.join(fileUpdate.content, fileUpdate.bom), "utf-8")
           await fs.unlink(hunk.path)
           modified.push(hunk.move_path)
           log.info(`Moved file: ${hunk.path} -> ${hunk.move_path}`)
         } else {
           // Regular update
-          await fs.writeFile(hunk.path, fileUpdate.content, "utf-8")
+          await fs.writeFile(hunk.path, Bom.join(fileUpdate.content, fileUpdate.bom), "utf-8")
           modified.push(hunk.path)
           log.info(`Updated file: ${hunk.path}`)
         }

+ 22 - 10
packages/opencode/src/tool/apply_patch.ts

@@ -14,6 +14,7 @@ import { AppFileSystem } from "@opencode-ai/shared/filesystem"
 import DESCRIPTION from "./apply_patch.txt"
 import { File } from "../file"
 import { Format } from "../format"
+import * as Bom from "@/util/bom"
 
 const PatchParams = z.object({
   patchText: z.string().describe("The full patch text that describes all changes to be made"),
@@ -59,6 +60,7 @@ export const ApplyPatchTool = Tool.define(
         diff: string
         additions: number
         deletions: number
+        bom: boolean
       }> = []
 
       let totalDiff = ""
@@ -72,11 +74,12 @@ export const ApplyPatchTool = Tool.define(
             const oldContent = ""
             const newContent =
               hunk.contents.length === 0 || hunk.contents.endsWith("\n") ? hunk.contents : `${hunk.contents}\n`
-            const diff = trimDiff(createTwoFilesPatch(filePath, filePath, oldContent, newContent))
+            const next = Bom.split(newContent)
+            const diff = trimDiff(createTwoFilesPatch(filePath, filePath, oldContent, next.text))
 
             let additions = 0
             let deletions = 0
-            for (const change of diffLines(oldContent, newContent)) {
+            for (const change of diffLines(oldContent, next.text)) {
               if (change.added) additions += change.count || 0
               if (change.removed) deletions += change.count || 0
             }
@@ -84,11 +87,12 @@ export const ApplyPatchTool = Tool.define(
             fileChanges.push({
               filePath,
               oldContent,
-              newContent,
+              newContent: next.text,
               type: "add",
               diff,
               additions,
               deletions,
+              bom: next.bom,
             })
 
             totalDiff += diff + "\n"
@@ -104,13 +108,16 @@ export const ApplyPatchTool = Tool.define(
               )
             }
 
-            const oldContent = yield* afs.readFileString(filePath)
+            const source = yield* Bom.readFile(afs, filePath)
+            const oldContent = source.text
             let newContent = oldContent
+            let bom = source.bom
 
             // Apply the update chunks to get new content
             try {
               const fileUpdate = Patch.deriveNewContentsFromChunks(filePath, hunk.chunks)
               newContent = fileUpdate.content
+              bom = fileUpdate.bom
             } catch (error) {
               return yield* Effect.fail(new Error(`apply_patch verification failed: ${error}`))
             }
@@ -136,6 +143,7 @@ export const ApplyPatchTool = Tool.define(
               diff,
               additions,
               deletions,
+              bom,
             })
 
             totalDiff += diff + "\n"
@@ -143,8 +151,8 @@ export const ApplyPatchTool = Tool.define(
           }
 
           case "delete": {
-            const contentToDelete = yield* afs
-              .readFileString(filePath)
+            const source = yield* Bom
+              .readFile(afs, filePath)
               .pipe(
                 Effect.catch((error) =>
                   Effect.fail(
@@ -154,6 +162,7 @@ export const ApplyPatchTool = Tool.define(
                   ),
                 ),
               )
+            const contentToDelete = source.text
             const deleteDiff = trimDiff(createTwoFilesPatch(filePath, filePath, contentToDelete, ""))
 
             const deletions = contentToDelete.split("\n").length
@@ -166,6 +175,7 @@ export const ApplyPatchTool = Tool.define(
               diff: deleteDiff,
               additions: 0,
               deletions,
+              bom: source.bom,
             })
 
             totalDiff += deleteDiff + "\n"
@@ -207,12 +217,12 @@ export const ApplyPatchTool = Tool.define(
           case "add":
             // Create parent directories (recursive: true is safe on existing/root dirs)
 
-            yield* afs.writeWithDirs(change.filePath, change.newContent)
+            yield* afs.writeWithDirs(change.filePath, Bom.join(change.newContent, change.bom))
             updates.push({ file: change.filePath, event: "add" })
             break
 
           case "update":
-            yield* afs.writeWithDirs(change.filePath, change.newContent)
+            yield* afs.writeWithDirs(change.filePath, Bom.join(change.newContent, change.bom))
             updates.push({ file: change.filePath, event: "change" })
             break
 
@@ -220,7 +230,7 @@ export const ApplyPatchTool = Tool.define(
             if (change.movePath) {
               // Create parent directories (recursive: true is safe on existing/root dirs)
 
-              yield* afs.writeWithDirs(change.movePath!, change.newContent)
+              yield* afs.writeWithDirs(change.movePath!, Bom.join(change.newContent, change.bom))
               yield* afs.remove(change.filePath)
               updates.push({ file: change.filePath, event: "unlink" })
               updates.push({ file: change.movePath, event: "add" })
@@ -234,7 +244,9 @@ export const ApplyPatchTool = Tool.define(
         }
 
         if (edited) {
-          yield* format.file(edited)
+          if (yield* format.file(edited)) {
+            yield* Bom.syncFile(afs, edited, change.bom)
+          }
           yield* bus.publish(File.Event.Edited, { file: edited })
         }
       }

+ 20 - 9
packages/opencode/src/tool/edit.ts

@@ -18,6 +18,7 @@ import { Instance } from "../project/instance"
 import { Snapshot } from "@/snapshot"
 import { assertExternalDirectoryEffect } from "./external-directory"
 import { AppFileSystem } from "@opencode-ai/shared/filesystem"
+import * as Bom from "@/util/bom"
 
 function normalizeLineEndings(text: string): string {
   return text.replaceAll("\r\n", "\n")
@@ -84,7 +85,11 @@ export const EditTool = Tool.define(
             Effect.gen(function* () {
               if (params.oldString === "") {
                 const existed = yield* afs.existsSafe(filePath)
-                contentNew = params.newString
+                const source = existed ? yield* Bom.readFile(afs, filePath) : { bom: false, text: "" }
+                const next = Bom.split(params.newString)
+                const desiredBom = source.bom || next.bom
+                contentOld = source.text
+                contentNew = next.text
                 diff = trimDiff(createTwoFilesPatch(filePath, filePath, contentOld, contentNew))
                 yield* ctx.ask({
                   permission: "edit",
@@ -95,8 +100,10 @@ export const EditTool = Tool.define(
                     diff,
                   },
                 })
-                yield* afs.writeWithDirs(filePath, params.newString)
-                yield* format.file(filePath)
+                yield* afs.writeWithDirs(filePath, Bom.join(contentNew, desiredBom))
+                if (yield* format.file(filePath)) {
+                  contentNew = yield* Bom.syncFile(afs, filePath, desiredBom)
+                }
                 yield* bus.publish(File.Event.Edited, { file: filePath })
                 yield* bus.publish(FileWatcher.Event.Updated, {
                   file: filePath,
@@ -108,13 +115,16 @@ export const EditTool = Tool.define(
               const info = yield* afs.stat(filePath).pipe(Effect.catch(() => Effect.succeed(undefined)))
               if (!info) throw new Error(`File ${filePath} not found`)
               if (info.type === "Directory") throw new Error(`Path is a directory, not a file: ${filePath}`)
-              contentOld = yield* afs.readFileString(filePath)
+              const source = yield* Bom.readFile(afs, filePath)
+              contentOld = source.text
 
               const ending = detectLineEnding(contentOld)
               const old = convertToLineEnding(normalizeLineEndings(params.oldString), ending)
-              const next = convertToLineEnding(normalizeLineEndings(params.newString), ending)
+              const replacement = convertToLineEnding(normalizeLineEndings(params.newString), ending)
 
-              contentNew = replace(contentOld, old, next, params.replaceAll)
+              const next = Bom.split(replace(contentOld, old, replacement, params.replaceAll))
+              const desiredBom = source.bom || next.bom
+              contentNew = next.text
 
               diff = trimDiff(
                 createTwoFilesPatch(
@@ -134,14 +144,15 @@ export const EditTool = Tool.define(
                 },
               })
 
-              yield* afs.writeWithDirs(filePath, contentNew)
-              yield* format.file(filePath)
+              yield* afs.writeWithDirs(filePath, Bom.join(contentNew, desiredBom))
+              if (yield* format.file(filePath)) {
+                contentNew = yield* Bom.syncFile(afs, filePath, desiredBom)
+              }
               yield* bus.publish(File.Event.Edited, { file: filePath })
               yield* bus.publish(FileWatcher.Event.Updated, {
                 file: filePath,
                 event: "change",
               })
-              contentNew = yield* afs.readFileString(filePath)
               diff = trimDiff(
                 createTwoFilesPatch(
                   filePath,

+ 11 - 4
packages/opencode/src/tool/write.ts

@@ -13,6 +13,7 @@ import { AppFileSystem } from "@opencode-ai/shared/filesystem"
 import { Instance } from "../project/instance"
 import { trimDiff } from "./edit"
 import { assertExternalDirectoryEffect } from "./external-directory"
+import * as Bom from "@/util/bom"
 
 const MAX_PROJECT_DIAGNOSTICS_FILES = 5
 
@@ -38,9 +39,13 @@ export const WriteTool = Tool.define(
           yield* assertExternalDirectoryEffect(ctx, filepath)
 
           const exists = yield* fs.existsSafe(filepath)
-          const contentOld = exists ? yield* fs.readFileString(filepath) : ""
+          const source = exists ? yield* Bom.readFile(fs, filepath) : { bom: false, text: "" }
+          const next = Bom.split(params.content)
+          const desiredBom = source.bom || next.bom
+          const contentOld = source.text
+          const contentNew = next.text
 
-          const diff = trimDiff(createTwoFilesPatch(filepath, filepath, contentOld, params.content))
+          const diff = trimDiff(createTwoFilesPatch(filepath, filepath, contentOld, contentNew))
           yield* ctx.ask({
             permission: "edit",
             patterns: [path.relative(Instance.worktree, filepath)],
@@ -51,8 +56,10 @@ export const WriteTool = Tool.define(
             },
           })
 
-          yield* fs.writeWithDirs(filepath, params.content)
-          yield* format.file(filepath)
+          yield* fs.writeWithDirs(filepath, Bom.join(contentNew, desiredBom))
+          if (yield* format.file(filepath)) {
+            yield* Bom.syncFile(fs, filepath, desiredBom)
+          }
           yield* bus.publish(File.Event.Edited, { file: filepath })
           yield* bus.publish(FileWatcher.Event.Updated, {
             file: filepath,

+ 31 - 0
packages/opencode/src/util/bom.ts

@@ -0,0 +1,31 @@
+import { Effect } from "effect"
+import { AppFileSystem } from "@opencode-ai/shared/filesystem"
+
+const BOM_CODE = 0xfeff
+const BOM = String.fromCharCode(BOM_CODE)
+
+export function split(text: string) {
+  if (text.charCodeAt(0) !== BOM_CODE) return { bom: false, text }
+  return { bom: true, text: text.slice(1) }
+}
+
+export function join(text: string, bom: boolean) {
+  const stripped = split(text).text
+  if (!bom) return stripped
+  return BOM + stripped
+}
+
+export const readFile = Effect.fn("Bom.readFile")(function* (fs: AppFileSystem.Interface, filePath: string) {
+  return split(new TextDecoder("utf-8", { ignoreBOM: true }).decode(yield* fs.readFile(filePath)))
+})
+
+export const syncFile = Effect.fn("Bom.syncFile")(function* (
+  fs: AppFileSystem.Interface,
+  filePath: string,
+  bom: boolean,
+) {
+  const current = yield* readFile(fs, filePath)
+  if (current.bom === bom) return current.text
+  yield* fs.writeWithDirs(filePath, join(current.text, bom))
+  return current.text
+})

+ 31 - 3
packages/opencode/test/format/format.test.ts

@@ -126,6 +126,24 @@ describe("Format", () => {
 
   it.live("service initializes without error", () => provideTmpdirInstance(() => Format.Service.use(() => Effect.void)))
 
+  it.live("file() returns false when no formatter runs", () =>
+    provideTmpdirInstance(
+      (dir) =>
+        Effect.gen(function* () {
+          const file = `${dir}/test.txt`
+          yield* Effect.promise(() => Bun.write(file, "x"))
+
+          const formatted = yield* Format.Service.use((fmt) => fmt.file(file))
+          expect(formatted).toBe(false)
+        }),
+      {
+        config: {
+          formatter: false,
+        },
+      },
+    ),
+  )
+
   it.live("status() initializes formatter state per directory", () =>
     Effect.gen(function* () {
       const a = yield* provideTmpdirInstance(() => Format.Service.use((fmt) => fmt.status()), {
@@ -219,7 +237,7 @@ describe("Format", () => {
           yield* Format.Service.use((fmt) =>
             Effect.gen(function* () {
               yield* fmt.init()
-              yield* fmt.file(file)
+              expect(yield* fmt.file(file)).toBe(true)
             }),
           )
 
@@ -229,11 +247,21 @@ describe("Format", () => {
         config: {
           formatter: {
             first: {
-              command: ["sh", "-c", 'sleep 0.05; v=$(cat "$1"); printf \'%sA\' "$v" > "$1"', "sh", "$FILE"],
+              command: [
+                "node",
+                "-e",
+                "const fs = require('fs'); const file = process.argv[1]; fs.writeFileSync(file, fs.readFileSync(file, 'utf8') + 'A')",
+                "$FILE",
+              ],
               extensions: [".seq"],
             },
             second: {
-              command: ["sh", "-c", 'v=$(cat "$1"); printf \'%sB\' "$v" > "$1"', "sh", "$FILE"],
+              command: [
+                "node",
+                "-e",
+                "const fs = require('fs'); const file = process.argv[1]; fs.writeFileSync(file, fs.readFileSync(file, 'utf8') + 'B')",
+                "$FILE",
+              ],
               extensions: [".seq"],
             },
           },

+ 28 - 0
packages/opencode/test/tool/apply_patch.test.ts

@@ -195,6 +195,34 @@ describe("tool.apply_patch freeform", () => {
     })
   })
 
+  test("does not invent a first-line diff for BOM files", async () => {
+    await using fixture = await tmpdir()
+    const { ctx, calls } = makeCtx()
+
+    await Instance.provide({
+      directory: fixture.path,
+      fn: async () => {
+        const bom = String.fromCharCode(0xfeff)
+        const target = path.join(fixture.path, "example.cs")
+        await fs.writeFile(target, `${bom}using System;\n\nclass Test {}\n`, "utf-8")
+
+        const patchText = "*** Begin Patch\n*** Update File: example.cs\n@@\n class Test {}\n+class Next {}\n*** End Patch"
+
+        await execute({ patchText }, ctx)
+
+        expect(calls.length).toBe(1)
+        const shown = calls[0].metadata.files[0]?.patch ?? ""
+        expect(shown).not.toContain(bom)
+        expect(shown).not.toContain("-using System;")
+        expect(shown).not.toContain("+using System;")
+
+        const content = await fs.readFile(target, "utf-8")
+        expect(content.charCodeAt(0)).toBe(0xfeff)
+        expect(content.slice(1)).toBe("using System;\n\nclass Test {}\nclass Next {}\n")
+      },
+    })
+  })
+
   test("inserts lines with insert-only hunk", async () => {
     await using fixture = await tmpdir()
     const { ctx } = makeCtx()

+ 63 - 0
packages/opencode/test/tool/edit.test.ts

@@ -96,6 +96,37 @@ describe("tool.edit", () => {
       })
     })
 
+    test("preserves BOM when oldString is empty on existing files", async () => {
+      await using tmp = await tmpdir()
+      const filepath = path.join(tmp.path, "existing.cs")
+      const bom = String.fromCharCode(0xfeff)
+      await fs.writeFile(filepath, `${bom}using System;\n`, "utf-8")
+
+      await Instance.provide({
+        directory: tmp.path,
+        fn: async () => {
+          const edit = await resolve()
+          const result = await Effect.runPromise(
+            edit.execute(
+              {
+                filePath: filepath,
+                oldString: "",
+                newString: "using Up;\n",
+              },
+              ctx,
+            ),
+          )
+
+          expect(result.metadata.diff).toContain("-using System;")
+          expect(result.metadata.diff).toContain("+using Up;")
+
+          const content = await fs.readFile(filepath, "utf-8")
+          expect(content.charCodeAt(0)).toBe(0xfeff)
+          expect(content.slice(1)).toBe("using Up;\n")
+        },
+      })
+    })
+
     test("creates new file with nested directories", async () => {
       await using tmp = await tmpdir()
       const filepath = path.join(tmp.path, "nested", "dir", "file.txt")
@@ -183,6 +214,38 @@ describe("tool.edit", () => {
       })
     })
 
+    test("replaces the first visible line in BOM files", async () => {
+      await using tmp = await tmpdir()
+      const filepath = path.join(tmp.path, "existing.cs")
+      const bom = String.fromCharCode(0xfeff)
+      await fs.writeFile(filepath, `${bom}using System;\nclass Test {}\n`, "utf-8")
+
+      await Instance.provide({
+        directory: tmp.path,
+        fn: async () => {
+          const edit = await resolve()
+          const result = await Effect.runPromise(
+            edit.execute(
+              {
+                filePath: filepath,
+                oldString: "using System;",
+                newString: "using Up;",
+              },
+              ctx,
+            ),
+          )
+
+          expect(result.metadata.diff).toContain("-using System;")
+          expect(result.metadata.diff).toContain("+using Up;")
+          expect(result.metadata.diff).not.toContain(bom)
+
+          const content = await fs.readFile(filepath, "utf-8")
+          expect(content.charCodeAt(0)).toBe(0xfeff)
+          expect(content.slice(1)).toBe("using Up;\nclass Test {}\n")
+        },
+      })
+    })
+
     test("throws error when file does not exist", async () => {
       await using tmp = await tmpdir()
       const filepath = path.join(tmp.path, "nonexistent.txt")

+ 48 - 0
packages/opencode/test/tool/write.test.ts

@@ -114,6 +114,54 @@ describe("tool.write", () => {
       ),
     )
 
+    it.live("preserves BOM when overwriting existing files", () =>
+      provideTmpdirInstance((dir) =>
+        Effect.gen(function* () {
+          const filepath = path.join(dir, "existing.cs")
+          const bom = String.fromCharCode(0xfeff)
+          yield* Effect.promise(() => fs.writeFile(filepath, `${bom}using System;\n`, "utf-8"))
+
+          yield* run({ filePath: filepath, content: "using Up;\n" })
+
+          const content = yield* Effect.promise(() => fs.readFile(filepath, "utf-8"))
+          expect(content.charCodeAt(0)).toBe(0xfeff)
+          expect(content.slice(1)).toBe("using Up;\n")
+        }),
+      ),
+    )
+
+    it.live("restores BOM after formatter strips it", () =>
+      provideTmpdirInstance(
+        (dir) =>
+          Effect.gen(function* () {
+            const filepath = path.join(dir, "formatted.cs")
+            const bom = String.fromCharCode(0xfeff)
+            yield* Effect.promise(() => fs.writeFile(filepath, `${bom}using System;\n`, "utf-8"))
+
+            yield* run({ filePath: filepath, content: "using Up;\n" })
+
+            const content = yield* Effect.promise(() => fs.readFile(filepath, "utf-8"))
+            expect(content.charCodeAt(0)).toBe(0xfeff)
+            expect(content.slice(1)).toBe("using Up;\n")
+          }),
+        {
+          config: {
+            formatter: {
+              stripbom: {
+                extensions: [".cs"],
+                command: [
+                  "node",
+                  "-e",
+                  "const fs = require('fs'); const file = process.argv[1]; let text = fs.readFileSync(file, 'utf8'); if (text.charCodeAt(0) === 0xfeff) text = text.slice(1); fs.writeFileSync(file, text, 'utf8')",
+                  "$FILE",
+                ],
+              },
+            },
+          },
+        },
+      ),
+    )
+
     it.live("returns diff in metadata for existing files", () =>
       provideTmpdirInstance((dir) =>
         Effect.gen(function* () {