From fab8dc9e6fe565c56c02ad32830c6598b3d125cb Mon Sep 17 00:00:00 2001
From: Dax Raad <d@ironbay.co>
Date: Wed, 25 Jun 2025 19:22:54 -0400
Subject: [PATCH] more edit tool fixes

---
 packages/opencode/src/tool/edit.ts       | 32 +++++++--
 packages/opencode/test/tool/edit.test.ts | 90 ++++++++++++++++++++++++
 2 files changed, 116 insertions(+), 6 deletions(-)

diff --git a/packages/opencode/src/tool/edit.ts b/packages/opencode/src/tool/edit.ts
index f23d46dd..b451ef55 100644
--- a/packages/opencode/src/tool/edit.ts
+++ b/packages/opencode/src/tool/edit.ts
@@ -33,6 +33,10 @@ export const EditTool = Tool.define({
       throw new Error("filePath is required")
     }
 
+    if (params.oldString === params.newString) {
+      throw new Error("oldString and newString must be different")
+    }
+
     const app = App.info()
     const filepath = path.isAbsolute(params.filePath)
       ? params.filePath
@@ -182,7 +186,10 @@ export const BlockAnchorReplacer: Replacer = function* (content, find) {
 
         let matchEndIndex = matchStartIndex
         for (let k = 0; k <= j - i; k++) {
-          matchEndIndex += originalLines[i + k].length + 1
+          matchEndIndex += originalLines[i + k].length
+          if (k < j - i) {
+            matchEndIndex += 1 // Add newline character except for the last line
+          }
         }
 
         yield content.substring(matchStartIndex, matchEndIndex)
@@ -216,10 +223,14 @@ export const WhitespaceNormalizedReplacer: Replacer = function* (
         const pattern = words
           .map((word) => word.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"))
           .join("\\s+")
-        const regex = new RegExp(pattern)
-        const match = line.match(regex)
-        if (match) {
-          yield match[0]
+        try {
+          const regex = new RegExp(pattern)
+          const match = line.match(regex)
+          if (match) {
+            yield match[0]
+          }
+        } catch (e) {
+          // Invalid regex pattern, skip
         }
       }
     }
@@ -269,7 +280,7 @@ export const IndentationFlexibleReplacer: Replacer = function* (content, find) {
 
 export const EscapeNormalizedReplacer: Replacer = function* (content, find) {
   const unescapeString = (str: string): string => {
-    return str.replace(/\\+(n|t|r|'|"|`|\\|\n|\$)/g, (match, capturedChar) => {
+    return str.replace(/\\(n|t|r|'|"|`|\\|\n|\$)/g, (match, capturedChar) => {
       switch (capturedChar) {
         case "n":
           return "\n"
@@ -363,6 +374,11 @@ export const ContextAwareReplacer: Replacer = function* (content, find) {
     return
   }
 
+  // Remove trailing empty line if present
+  if (findLines[findLines.length - 1] === "") {
+    findLines.pop()
+  }
+
   const contentLines = content.split("\n")
 
   // Extract first and last lines as context anchors
@@ -454,6 +470,10 @@ export function replace(
   newString: string,
   replaceAll = false,
 ): string {
+  if (oldString === newString) {
+    throw new Error("oldString and newString must be different")
+  }
+  
   for (const replacer of [
     SimpleReplacer,
     LineTrimmedReplacer,
diff --git a/packages/opencode/test/tool/edit.test.ts b/packages/opencode/test/tool/edit.test.ts
index d0cb5cc8..f5558eaa 100644
--- a/packages/opencode/test/tool/edit.test.ts
+++ b/packages/opencode/test/tool/edit.test.ts
@@ -302,6 +302,96 @@ const testCases: TestCase[] = [
     find: ["function test() {", "return 'value';", "}"].join("\n"),
     replace: ["function test() {", "return 'new value';", "}"].join("\n"),
   },
+
+  // Test for same oldString and newString (should fail)
+  {
+    content: 'console.log("test");',
+    find: 'console.log("test");',
+    replace: 'console.log("test");',
+    fail: true,
+  },
+
+  // Additional tests for fixes made
+
+  // WhitespaceNormalizedReplacer - test regex special characters that could cause errors
+  {
+    content: 'const pattern = "test[123]";',
+    find: 'test[123]',
+    replace: 'test[456]',
+  },
+  {
+    content: 'const regex = "^start.*end$";',
+    find: '^start.*end$',
+    replace: '^begin.*finish$',
+  },
+
+  // EscapeNormalizedReplacer - test single backslash vs double backslash
+  {
+    content: 'const path = "C:\\Users";',
+    find: 'const path = "C:\\Users";',
+    replace: 'const path = "D:\\Users";',
+  },
+  {
+    content: 'console.log("Line1\\nLine2");',
+    find: 'console.log("Line1\\nLine2");',
+    replace: 'console.log("First\\nSecond");',
+  },
+
+  // BlockAnchorReplacer - test edge case with exact newline boundaries
+  {
+    content: ["function test() {", "  return true;", "}"].join("\n"),
+    find: ["function test() {", "  // middle", "}"].join("\n"),
+    replace: ["function test() {", "  return false;", "}"].join("\n"),
+  },
+
+  // ContextAwareReplacer - test with trailing newline in find string
+  {
+    content: [
+      "class Test {",
+      "  method1() {",
+      "    return 1;",
+      "  }",
+      "}",
+    ].join("\n"),
+    find: [
+      "class Test {",
+      "  // different content",
+      "}",
+      "", // trailing empty line
+    ].join("\n"),
+    replace: ["class Test {", "  method2() { return 2; }", "}"].join("\n"),
+  },
+
+  // Test validation for empty strings with same oldString and newString
+  {
+    content: "",
+    find: "",
+    replace: "",
+    fail: true,
+  },
+
+  // Test multiple occurrences with replaceAll=false (should fail)
+  {
+    content: ["const a = 1;", "const b = 1;", "const c = 1;"].join("\n"),
+    find: "= 1",
+    replace: "= 2",
+    all: false,
+    fail: true,
+  },
+
+  // Test whitespace normalization with multiple spaces and tabs mixed
+  {
+    content: "if\t \t( \tcondition\t )\t{",
+    find: "if ( condition ) {",
+    replace: "if (newCondition) {",
+  },
+
+  // Test escape sequences in template literals
+  {
+    content: "const msg = `Hello\\tWorld`;",
+    find: "const msg = `Hello\\tWorld`;",
+    replace: "const msg = `Hi\\tWorld`;",
+  },
 ]
 
 describe("EditTool Replacers", () => {