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

fix(opencode): prevent destructive edit matches (#30932)

Shoubhit Dash 3 месяцев назад
Родитель
Сommit
236cfcbbc3
2 измененных файлов с 89 добавлено и 17 удалено
  1. 35 9
      packages/opencode/src/tool/edit.ts
  2. 54 8
      packages/opencode/test/tool/edit.test.ts

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

@@ -89,10 +89,14 @@ export const EditTool = Tool.define(
             Effect.gen(function* () {
               if (params.oldString === "") {
                 const existed = yield* afs.existsSafe(filePath)
-                const source = existed ? yield* Bom.readFile(afs, filePath) : { bom: false, text: "" }
+                if (existed) {
+                  throw new Error(
+                    "oldString cannot be empty when editing an existing file. Provide the exact text to replace, or use write for an intentional full-file replacement.",
+                  )
+                }
                 const next = Bom.split(params.newString)
-                const desiredBom = source.bom || next.bom
-                contentOld = source.text
+                const desiredBom = next.bom
+                contentOld = ""
                 contentNew = next.text
                 diff = trimDiff(createTwoFilesPatch(filePath, filePath, contentOld, contentNew))
                 yield* ctx.ask({
@@ -111,7 +115,7 @@ export const EditTool = Tool.define(
                 yield* events.publish(FileSystem.Event.Edited, { file: filePath })
                 yield* events.publish(Watcher.Event.Updated, {
                   file: filePath,
-                  event: existed ? "change" : "add",
+                  event: "add",
                 })
                 return
               }
@@ -213,8 +217,8 @@ export const EditTool = Tool.define(
 export type Replacer = (content: string, find: string) => Generator<string, void, unknown>
 
 // Similarity thresholds for block anchor fallback matching
-const SINGLE_CANDIDATE_SIMILARITY_THRESHOLD = 0.0
-const MULTIPLE_CANDIDATES_SIMILARITY_THRESHOLD = 0.3
+const SINGLE_CANDIDATE_SIMILARITY_THRESHOLD = 0.65
+const MULTIPLE_CANDIDATES_SIMILARITY_THRESHOLD = 0.65
 
 /**
  * Levenshtein distance algorithm implementation
@@ -296,6 +300,7 @@ export const BlockAnchorReplacer: Replacer = function* (content, find) {
   const firstLineSearch = searchLines[0].trim()
   const lastLineSearch = searchLines[searchLines.length - 1].trim()
   const searchBlockSize = searchLines.length
+  const maxLineDelta = Math.max(1, Math.floor(searchBlockSize * 0.25))
 
   // Collect all candidate positions where both anchors match
   const candidates: Array<{ startLine: number; endLine: number }> = []
@@ -307,7 +312,10 @@ export const BlockAnchorReplacer: Replacer = function* (content, find) {
     // Look for the matching last line after this first line
     for (let j = i + 2; j < originalLines.length; j++) {
       if (originalLines[j].trim() === lastLineSearch) {
-        candidates.push({ startLine: i, endLine: j })
+        const actualBlockSize = j - i + 1
+        if (Math.abs(actualBlockSize - searchBlockSize) <= maxLineDelta) {
+          candidates.push({ startLine: i, endLine: j })
+        }
         break // Only match the first occurrence of the last line
       }
     }
@@ -324,7 +332,7 @@ export const BlockAnchorReplacer: Replacer = function* (content, find) {
     const actualBlockSize = endLine - startLine + 1
 
     let similarity = 0
-    let linesToCheck = Math.min(searchBlockSize - 2, actualBlockSize - 2) // Middle lines only
+    const linesToCheck = Math.min(searchBlockSize - 2, actualBlockSize - 2) // Middle lines only
 
     if (linesToCheck > 0) {
       for (let j = 1; j < searchBlockSize - 1 && j < actualBlockSize - 1; j++) {
@@ -373,7 +381,7 @@ export const BlockAnchorReplacer: Replacer = function* (content, find) {
     const actualBlockSize = endLine - startLine + 1
 
     let similarity = 0
-    let linesToCheck = Math.min(searchBlockSize - 2, actualBlockSize - 2) // Middle lines only
+    const linesToCheck = Math.min(searchBlockSize - 2, actualBlockSize - 2) // Middle lines only
 
     if (linesToCheck > 0) {
       for (let j = 1; j < searchBlockSize - 1 && j < actualBlockSize - 1; j++) {
@@ -675,6 +683,11 @@ export function replace(content: string, oldString: string, newString: string, r
   if (oldString === newString) {
     throw new Error("No changes to apply: oldString and newString are identical.")
   }
+  if (oldString === "") {
+    throw new Error(
+      "oldString cannot be empty when editing an existing file. Provide the exact text to replace, or use write for an intentional full-file replacement.",
+    )
+  }
 
   let notFound = true
 
@@ -693,6 +706,11 @@ export function replace(content: string, oldString: string, newString: string, r
       const index = content.indexOf(search)
       if (index === -1) continue
       notFound = false
+      if (isDisproportionateMatch(search, oldString)) {
+        throw new Error(
+          "Refusing replacement because the matched span is much larger than oldString. Re-read the file and provide the full exact oldString for the intended replacement.",
+        )
+      }
       if (replaceAll) {
         return content.replaceAll(search, newString)
       }
@@ -709,3 +727,11 @@ export function replace(content: string, oldString: string, newString: string, r
   }
   throw new Error("Found multiple matches for oldString. Provide more surrounding context to make the match unique.")
 }
+
+function isDisproportionateMatch(search: string, oldString: string) {
+  const oldLines = oldString.split("\n").length
+  const searchLines = search.split("\n").length
+  if (searchLines >= Math.max(oldLines + 3, oldLines * 2)) return true
+  if (oldLines === 1) return false
+  return search.trim().length > Math.max(oldString.trim().length + 500, oldString.trim().length * 4)
+}

+ 54 - 8
packages/opencode/test/tool/edit.test.ts

@@ -106,21 +106,20 @@ describe("tool.edit", () => {
       }),
     )
 
-    it.instance("preserves BOM when oldString is empty on existing files", () =>
+    it.instance("rejects empty oldString on existing files and leaves content unchanged", () =>
       Effect.gen(function* () {
         const test = yield* TestInstance
         const filepath = path.join(test.directory, "existing.cs")
         const bom = String.fromCharCode(0xfeff)
-        yield* put(filepath, `${bom}using System;\n`)
+        const original = `${bom}using System;\n`
+        yield* put(filepath, original)
 
-        const result = yield* run({ filePath: filepath, oldString: "", newString: "using Up;\n" })
-
-        expect(result.metadata.diff).toContain("-using System;")
-        expect(result.metadata.diff).toContain("+using Up;")
+        expect((yield* fail({ filePath: filepath, oldString: "", newString: "using Up;\n" })).message).toContain(
+          "oldString cannot be empty",
+        )
 
         const content = yield* loadRaw(filepath)
-        expect(content.charCodeAt(0)).toBe(0xfeff)
-        expect(content.slice(1)).toBe("using Up;\n")
+        expect(content).toBe(original)
       }),
     )
 
@@ -213,6 +212,53 @@ describe("tool.edit", () => {
       }),
     )
 
+    it.instance("rejects loose block-anchor matches and leaves content unchanged", () =>
+      Effect.gen(function* () {
+        const test = yield* TestInstance
+        const filepath = path.join(test.directory, "file.ts")
+        const original = [
+          "function configure() {",
+          "  keepImportantState()",
+          "  removeAllUserData()",
+          "  archiveBackups()",
+          "  auditLog()",
+          "}",
+        ].join("\n")
+        yield* put(filepath, original)
+
+        expect(
+          (
+            yield* fail({
+              filePath: filepath,
+              oldString: ["function configure() {", "  const enabled = true", "}"].join("\n"),
+              newString: ["function configure() {", "  const enabled = false", "}"].join("\n"),
+            })
+          ).message,
+        ).toContain("Could not find oldString")
+        expect(yield* load(filepath)).toBe(original)
+      }),
+    )
+
+    it.instance("rejects block-anchor matches with unrelated middle content", () =>
+      Effect.gen(function* () {
+        const test = yield* TestInstance
+        const filepath = path.join(test.directory, "file.ts")
+        const original = ["function configure() {", "  removeAllUserData()", "}"].join("\n")
+        yield* put(filepath, original)
+
+        expect(
+          (
+            yield* fail({
+              filePath: filepath,
+              oldString: ["function configure() {", "  const enabled = true", "}"].join("\n"),
+              newString: ["function configure() {", "  const enabled = false", "}"].join("\n"),
+            })
+          ).message,
+        ).toContain("Could not find oldString")
+        expect(yield* load(filepath)).toBe(original)
+      }),
+    )
+
     it.instance("replaces all occurrences with replaceAll option", () =>
       Effect.gen(function* () {
         const test = yield* TestInstance