core: make patch tool more reliable and consistent with other editing tools

The patch tool now works seamlessly alongside other file editing tools with improved error handling and a more intuitive permission system. Users will experience: - More reliable patch application with better error messages - Consistent permission prompts that match other editing tools - Smoother integration when applying complex multi-file changes - Better feedback on what changes are being made before applying patches This refactoring leverages the robust patch parsing engine while making the tool feel native to the opencode workflow, reducing friction when making bulk changes to your codebase.
2025-12-25 03:34:22 +01:00 · 2025-10-01 06:45:43 -04:00
parent 172aeaaf14
commit 41ce56494b
5 changed files with 1455 additions and 313 deletions
--- a/packages/opencode/test/patch/integration.test.ts
+++ b/packages/opencode/test/patch/integration.test.ts
@@ -0,0 +1,66 @@
+import { describe, test, expect } from "bun:test"
+import { Patch } from "../../src/patch"
+
+describe("Patch integration", () => {
+  test("should be compatible with existing tool system", () => {
+    // Test that our Patch namespace can be imported and used
+    expect(Patch).toBeDefined()
+    expect(Patch.parsePatch).toBeDefined()
+    expect(Patch.applyPatch).toBeDefined()
+    expect(Patch.maybeParseApplyPatch).toBeDefined()
+    expect(Patch.PatchSchema).toBeDefined()
+  })
+  
+  test("should parse patch format compatible with existing tool", () => {
+    const patchText = `*** Begin Patch
+*** Add File: test-integration.txt
+Integration test content
+*** End Patch`
+    
+    const result = Patch.parsePatch(patchText)
+    expect(result.hunks).toHaveLength(1)
+    expect(result.hunks[0].type).toBe("add")
+    expect(result.hunks[0].path).toBe("test-integration.txt")
+    if (result.hunks[0].type === "add") {
+      expect(result.hunks[0].contents).toBe("Integration test content")
+    }
+  })
+  
+  test("should handle complex patch with multiple operations", () => {
+    const patchText = `*** Begin Patch
+*** Add File: new-file.txt
+This is a new file
+with multiple lines
+*** Update File: existing.txt
+@@
+ old content
+-line to remove
+line to add
+ more content
+*** Delete File: old-file.txt
+*** End Patch`
+    
+    const result = Patch.parsePatch(patchText)
+    expect(result.hunks).toHaveLength(3)
+    
+    // Check add operation
+    expect(result.hunks[0].type).toBe("add")
+    if (result.hunks[0].type === "add") {
+      expect(result.hunks[0].contents).toBe("This is a new file\nwith multiple lines")
+    }
+    
+    // Check update operation
+    expect(result.hunks[1].type).toBe("update")
+    if (result.hunks[1].type === "update") {
+      expect(result.hunks[1].path).toBe("existing.txt")
+      expect(result.hunks[1].chunks).toHaveLength(1)
+      expect(result.hunks[1].chunks[0].old_lines).toEqual(["old content", "line to remove", "more content"])
+      expect(result.hunks[1].chunks[0].new_lines).toEqual(["old content", "line to add", "more content"])
+      expect(result.hunks[1].chunks[0].change_context).toBeUndefined()
+    }
+    
+    // Check delete operation
+    expect(result.hunks[2].type).toBe("delete")
+    expect(result.hunks[2].path).toBe("old-file.txt")
+  })
+})
--- a/packages/opencode/test/patch/patch.test.ts
+++ b/packages/opencode/test/patch/patch.test.ts
@@ -0,0 +1,339 @@
+import { describe, test, expect, beforeEach, afterEach } from "bun:test"
+import { Patch } from "../../src/patch"
+import * as fs from "fs/promises"
+import * as path from "path"
+import { tmpdir } from "os"
+
+describe("Patch namespace", () => {
+  let tempDir: string
+  
+  beforeEach(async () => {
+    tempDir = await fs.mkdtemp(path.join(tmpdir(), "patch-test-"))
+  })
+  
+  afterEach(async () => {
+    // Clean up temp directory
+    await fs.rm(tempDir, { recursive: true, force: true })
+  })
+  
+  describe("parsePatch", () => {
+    test("should parse simple add file patch", () => {
+      const patchText = `*** Begin Patch
+*** Add File: test.txt
+Hello World
+*** End Patch`
+      
+      const result = Patch.parsePatch(patchText)
+      expect(result.hunks).toHaveLength(1)
+      expect(result.hunks[0]).toEqual({
+        type: "add",
+        path: "test.txt",
+        contents: "Hello World",
+      })
+    })
+    
+    test("should parse delete file patch", () => {
+      const patchText = `*** Begin Patch
+*** Delete File: old.txt
+*** End Patch`
+      
+      const result = Patch.parsePatch(patchText)
+      expect(result.hunks).toHaveLength(1)
+      const hunk = result.hunks[0]
+      expect(hunk.type).toBe("delete")
+      expect(hunk.path).toBe("old.txt")
+    })
+    
+    test("should parse patch with multiple hunks", () => {
+      const patchText = `*** Begin Patch
+*** Add File: new.txt
+This is a new file
+*** Update File: existing.txt
+@@
+ old line
+-new line
+updated line
+*** End Patch`
+      
+      const result = Patch.parsePatch(patchText)
+      expect(result.hunks).toHaveLength(2)
+      expect(result.hunks[0].type).toBe("add")
+      expect(result.hunks[1].type).toBe("update")
+    })
+    
+    test("should parse file move operation", () => {
+      const patchText = `*** Begin Patch
+*** Update File: old-name.txt
+*** Move to: new-name.txt
+@@
+-Old content
+New content
+*** End Patch`
+      
+      const result = Patch.parsePatch(patchText)
+      expect(result.hunks).toHaveLength(1)
+      const hunk = result.hunks[0]
+      expect(hunk.type).toBe("update")
+      expect(hunk.path).toBe("old-name.txt")
+      if (hunk.type === "update") {
+        expect(hunk.move_path).toBe("new-name.txt")
+      }
+    })
+    
+    test("should throw error for invalid patch format", () => {
+      const invalidPatch = `This is not a valid patch`
+      
+      expect(() => Patch.parsePatch(invalidPatch)).toThrow("Invalid patch format")
+    })
+  })
+  
+  describe("maybeParseApplyPatch", () => {
+    test("should parse direct apply_patch command", () => {
+      const patchText = `*** Begin Patch
+*** Add File: test.txt
+Content
+*** End Patch`
+      
+      const result = Patch.maybeParseApplyPatch(["apply_patch", patchText])
+      expect(result.type).toBe(Patch.MaybeApplyPatch.Body)
+      if (result.type === Patch.MaybeApplyPatch.Body) {
+        expect(result.args.patch).toBe(patchText)
+        expect(result.args.hunks).toHaveLength(1)
+      }
+    })
+    
+    test("should parse applypatch command", () => {
+      const patchText = `*** Begin Patch
+*** Add File: test.txt
+Content
+*** End Patch`
+      
+      const result = Patch.maybeParseApplyPatch(["applypatch", patchText])
+      expect(result.type).toBe(Patch.MaybeApplyPatch.Body)
+    })
+    
+    test("should handle bash heredoc format", () => {
+      const script = `apply_patch <<'PATCH'
+*** Begin Patch
+*** Add File: test.txt
+Content
+*** End Patch
+PATCH`
+      
+      const result = Patch.maybeParseApplyPatch(["bash", "-lc", script])
+      expect(result.type).toBe(Patch.MaybeApplyPatch.Body)
+      if (result.type === Patch.MaybeApplyPatch.Body) {
+        expect(result.args.hunks).toHaveLength(1)
+      }
+    })
+    
+    test("should return NotApplyPatch for non-patch commands", () => {
+      const result = Patch.maybeParseApplyPatch(["echo", "hello"])
+      expect(result.type).toBe(Patch.MaybeApplyPatch.NotApplyPatch)
+    })
+  })
+  
+  describe("applyPatch", () => {
+    test("should add a new file", async () => {
+      const patchText = `*** Begin Patch
+*** Add File: ${tempDir}/new-file.txt
+Hello World
+This is a new file
+*** End Patch`
+      
+      const result = await Patch.applyPatch(patchText)
+      expect(result.added).toHaveLength(1)
+      expect(result.modified).toHaveLength(0)
+      expect(result.deleted).toHaveLength(0)
+      
+      const content = await fs.readFile(result.added[0], "utf-8")
+      expect(content).toBe("Hello World\nThis is a new file")
+    })
+    
+    test("should delete an existing file", async () => {
+      const filePath = path.join(tempDir, "to-delete.txt")
+      await fs.writeFile(filePath, "This file will be deleted")
+      
+      const patchText = `*** Begin Patch
+*** Delete File: ${filePath}
+*** End Patch`
+      
+      const result = await Patch.applyPatch(patchText)
+      expect(result.deleted).toHaveLength(1)
+      expect(result.deleted[0]).toBe(filePath)
+      
+      const exists = await fs.access(filePath).then(() => true).catch(() => false)
+      expect(exists).toBe(false)
+    })
+    
+    test("should update an existing file", async () => {
+      const filePath = path.join(tempDir, "to-update.txt")
+      await fs.writeFile(filePath, "line 1\nline 2\nline 3\n")
+      
+      const patchText = `*** Begin Patch
+*** Update File: ${filePath}
+@@
+ line 1
+-line 2
+line 2 updated
+ line 3
+*** End Patch`
+      
+      const result = await Patch.applyPatch(patchText)
+      expect(result.modified).toHaveLength(1)
+      expect(result.modified[0]).toBe(filePath)
+      
+      const content = await fs.readFile(filePath, "utf-8")
+      expect(content).toBe("line 1\nline 2 updated\nline 3\n")
+    })
+    
+    test("should move and update a file", async () => {
+      const oldPath = path.join(tempDir, "old-name.txt")
+      const newPath = path.join(tempDir, "new-name.txt")
+      await fs.writeFile(oldPath, "old content\n")
+      
+      const patchText = `*** Begin Patch
+*** Update File: ${oldPath}
+*** Move to: ${newPath}
+@@
+-old content
+new content
+*** End Patch`
+      
+      const result = await Patch.applyPatch(patchText)
+      expect(result.modified).toHaveLength(1)
+      expect(result.modified[0]).toBe(newPath)
+      
+      const oldExists = await fs.access(oldPath).then(() => true).catch(() => false)
+      expect(oldExists).toBe(false)
+      
+      const newContent = await fs.readFile(newPath, "utf-8")
+      expect(newContent).toBe("new content\n")
+    })
+    
+    test("should handle multiple operations in one patch", async () => {
+      const file1 = path.join(tempDir, "file1.txt")
+      const file2 = path.join(tempDir, "file2.txt")
+      const file3 = path.join(tempDir, "file3.txt")
+      
+      await fs.writeFile(file1, "content 1")
+      await fs.writeFile(file2, "content 2")
+      
+      const patchText = `*** Begin Patch
+*** Add File: ${file3}
+new file content
+*** Update File: ${file1}
+@@
+-content 1
+updated content 1
+*** Delete File: ${file2}
+*** End Patch`
+      
+      const result = await Patch.applyPatch(patchText)
+      expect(result.added).toHaveLength(1)
+      expect(result.modified).toHaveLength(1)
+      expect(result.deleted).toHaveLength(1)
+    })
+    
+    test("should create parent directories when adding files", async () => {
+      const nestedPath = path.join(tempDir, "deep", "nested", "file.txt")
+      
+      const patchText = `*** Begin Patch
+*** Add File: ${nestedPath}
+Deep nested content
+*** End Patch`
+      
+      const result = await Patch.applyPatch(patchText)
+      expect(result.added).toHaveLength(1)
+      expect(result.added[0]).toBe(nestedPath)
+      
+      const exists = await fs.access(nestedPath).then(() => true).catch(() => false)
+      expect(exists).toBe(true)
+    })
+  })
+  
+  describe("error handling", () => {
+    test("should throw error when updating non-existent file", async () => {
+      const nonExistent = path.join(tempDir, "does-not-exist.txt")
+      
+      const patchText = `*** Begin Patch
+*** Update File: ${nonExistent}
+@@
+-old line
+new line
+*** End Patch`
+      
+      await expect(Patch.applyPatch(patchText)).rejects.toThrow()
+    })
+    
+    test("should throw error when deleting non-existent file", async () => {
+      const nonExistent = path.join(tempDir, "does-not-exist.txt")
+      
+      const patchText = `*** Begin Patch
+*** Delete File: ${nonExistent}
+*** End Patch`
+      
+      await expect(Patch.applyPatch(patchText)).rejects.toThrow()
+    })
+  })
+  
+  describe("edge cases", () => {
+    test("should handle empty files", async () => {
+      const emptyFile = path.join(tempDir, "empty.txt")
+      await fs.writeFile(emptyFile, "")
+      
+      const patchText = `*** Begin Patch
+*** Update File: ${emptyFile}
+@@
+First line
+*** End Patch`
+      
+      const result = await Patch.applyPatch(patchText)
+      expect(result.modified).toHaveLength(1)
+      
+      const content = await fs.readFile(emptyFile, "utf-8")
+      expect(content).toBe("First line\n")
+    })
+    
+    test("should handle files with no trailing newline", async () => {
+      const filePath = path.join(tempDir, "no-newline.txt")
+      await fs.writeFile(filePath, "no newline")
+      
+      const patchText = `*** Begin Patch
+*** Update File: ${filePath}
+@@
+-no newline
+has newline now
+*** End Patch`
+      
+      const result = await Patch.applyPatch(patchText)
+      expect(result.modified).toHaveLength(1)
+      
+      const content = await fs.readFile(filePath, "utf-8")
+      expect(content).toBe("has newline now\n")
+    })
+    
+    test("should handle multiple update chunks in single file", async () => {
+      const filePath = path.join(tempDir, "multi-chunk.txt")
+      await fs.writeFile(filePath, "line 1\nline 2\nline 3\nline 4\n")
+      
+      const patchText = `*** Begin Patch
+*** Update File: ${filePath}
+@@
+ line 1
+-line 2
+LINE 2
+@@
+ line 3
+-line 4
+LINE 4
+*** End Patch`
+      
+      const result = await Patch.applyPatch(patchText)
+      expect(result.modified).toHaveLength(1)
+      
+      const content = await fs.readFile(filePath, "utf-8")
+      expect(content).toBe("line 1\nLINE 2\nline 3\nLINE 4\n")
+    })
+  })
+})
--- a/packages/opencode/test/tool/patch.test.ts
+++ b/packages/opencode/test/tool/patch.test.ts
@@ -0,0 +1,263 @@
+import { describe, expect, test } from "bun:test"
+import path from "path"
+import { PatchTool } from "../../src/tool/patch"
+import { Log } from "../../src/util/log"
+import { Instance } from "../../src/project/instance"
+import { tmpdir } from "../fixture/fixture"
+import * as fs from "fs/promises"
+
+const ctx = {
+  sessionID: "test",
+  messageID: "",
+  toolCallID: "",
+  agent: "build", 
+  abort: AbortSignal.any([]),
+  metadata: () => {},
+}
+
+const patchTool = await PatchTool.init()
+Log.init({ print: false })
+
+describe("tool.patch", () => {
+  test("should validate required parameters", async () => {
+    await Instance.provide({
+      directory: "/tmp",
+      fn: async () => {
+        await expect(
+          patchTool.execute({ patchText: "" }, ctx)
+        ).rejects.toThrow("patchText is required")
+      },
+    })
+  })
+  
+  test("should validate patch format", async () => {
+    await Instance.provide({
+      directory: "/tmp",
+      fn: async () => {
+        await expect(
+          patchTool.execute({ patchText: "invalid patch" }, ctx)
+        ).rejects.toThrow("Failed to parse patch")
+      },
+    })
+  })
+  
+  test("should handle empty patch", async () => {
+    await Instance.provide({
+      directory: "/tmp",
+      fn: async () => {
+        const emptyPatch = `*** Begin Patch
+*** End Patch`
+        
+        await expect(
+          patchTool.execute({ patchText: emptyPatch }, ctx)
+        ).rejects.toThrow("No file changes found in patch")
+      },
+    })
+  })
+  
+  test("should reject files outside working directory", async () => {
+    await Instance.provide({
+      directory: "/tmp",
+      fn: async () => {
+        const maliciousPatch = `*** Begin Patch
+*** Add File: /etc/passwd
+malicious content
+*** End Patch`
+        
+        await expect(
+          patchTool.execute({ patchText: maliciousPatch }, ctx)
+        ).rejects.toThrow("is not in the current working directory")
+      },
+    })
+  })
+  
+  test("should handle simple add file operation", async () => {
+    await using fixture = await tmpdir()
+    
+    await Instance.provide({
+      directory: fixture.path,
+      fn: async () => {
+        const patchText = `*** Begin Patch
+*** Add File: test-file.txt
+Hello World
+This is a test file
+*** End Patch`
+        
+        const result = await patchTool.execute({ patchText }, ctx)
+        
+        expect(result.title).toContain("files changed")
+        expect(result.metadata.diff).toBeDefined()
+        expect(result.output).toContain("Patch applied successfully")
+        
+        // Verify file was created
+        const filePath = path.join(fixture.path, "test-file.txt")
+        const content = await fs.readFile(filePath, "utf-8")
+        expect(content).toBe("Hello World\nThis is a test file")
+      },
+    })
+  })
+  
+  test("should handle file with context update", async () => {
+    await using fixture = await tmpdir()
+    
+    await Instance.provide({
+      directory: fixture.path,
+      fn: async () => {
+        const patchText = `*** Begin Patch
+*** Add File: config.js
+const API_KEY = "test-key"
+const DEBUG = false
+const VERSION = "1.0"
+*** End Patch`
+        
+        const result = await patchTool.execute({ patchText }, ctx)
+        
+        expect(result.title).toContain("files changed")
+        expect(result.metadata.diff).toBeDefined()
+        expect(result.output).toContain("Patch applied successfully")
+        
+        // Verify file was created with correct content
+        const filePath = path.join(fixture.path, "config.js")
+        const content = await fs.readFile(filePath, "utf-8")
+        expect(content).toBe("const API_KEY = \"test-key\"\nconst DEBUG = false\nconst VERSION = \"1.0\"")
+      },
+    })
+  })
+  
+  test("should handle multiple file operations", async () => {
+    await using fixture = await tmpdir()
+    
+    await Instance.provide({
+      directory: fixture.path,
+      fn: async () => {
+        const patchText = `*** Begin Patch
+*** Add File: file1.txt
+Content of file 1
+*** Add File: file2.txt
+Content of file 2
+*** Add File: file3.txt
+Content of file 3
+*** End Patch`
+        
+        const result = await patchTool.execute({ patchText }, ctx)
+        
+        expect(result.title).toContain("3 files changed")
+        expect(result.metadata.diff).toBeDefined()
+        expect(result.output).toContain("Patch applied successfully")
+        
+        // Verify all files were created
+        for (let i = 1; i <= 3; i++) {
+          const filePath = path.join(fixture.path, `file${i}.txt`)
+          const content = await fs.readFile(filePath, "utf-8")
+          expect(content).toBe(`Content of file ${i}`)
+        }
+      },
+    })
+  })
+  
+  test("should create parent directories when adding nested files", async () => {
+    await using fixture = await tmpdir()
+    
+    await Instance.provide({
+      directory: fixture.path,
+      fn: async () => {
+        const patchText = `*** Begin Patch
+*** Add File: deep/nested/file.txt
+Deep nested content
+*** End Patch`
+        
+        const result = await patchTool.execute({ patchText }, ctx)
+        
+        expect(result.title).toContain("files changed")
+        expect(result.output).toContain("Patch applied successfully")
+        
+        // Verify nested file was created
+        const nestedPath = path.join(fixture.path, "deep", "nested", "file.txt")
+        const exists = await fs.access(nestedPath).then(() => true).catch(() => false)
+        expect(exists).toBe(true)
+        
+        const content = await fs.readFile(nestedPath, "utf-8")
+        expect(content).toBe("Deep nested content")
+      },
+    })
+  })
+  
+  test("should generate proper unified diff in metadata", async () => {
+    await using fixture = await tmpdir()
+    
+    await Instance.provide({
+      directory: fixture.path,
+      fn: async () => {
+        // First create a file with simple content
+        const patchText1 = `*** Begin Patch
+*** Add File: test.txt
+line 1
+line 2
+line 3
+*** End Patch`
+        
+        await patchTool.execute({ patchText: patchText1 }, ctx)
+        
+        // Now create an update patch
+        const patchText2 = `*** Begin Patch
+*** Update File: test.txt
+@@
+ line 1
+-line 2
+line 2 updated
+ line 3
+*** End Patch`
+        
+        const result = await patchTool.execute({ patchText: patchText2 }, ctx)
+        
+        expect(result.metadata.diff).toBeDefined()
+        expect(result.metadata.diff).toContain("@@")
+        expect(result.metadata.diff).toContain("-line 2")
+        expect(result.metadata.diff).toContain("+line 2 updated")
+      },
+    })
+  })
+  
+  test("should handle complex patch with multiple operations", async () => {
+    await using fixture = await tmpdir()
+    
+    await Instance.provide({
+      directory: fixture.path,
+      fn: async () => {
+        const patchText = `*** Begin Patch
+*** Add File: new.txt
+This is a new file
+with multiple lines
+*** Add File: existing.txt
+old content
+new line
+more content
+*** Add File: config.json
+{
+  "version": "1.0",
+  "debug": true
+}
+*** End Patch`
+        
+        const result = await patchTool.execute({ patchText }, ctx)
+        
+        expect(result.title).toContain("3 files changed")
+        expect(result.metadata.diff).toBeDefined()
+        expect(result.output).toContain("Patch applied successfully")
+        
+        // Verify all files were created
+        const newPath = path.join(fixture.path, "new.txt")
+        const newContent = await fs.readFile(newPath, "utf-8")
+        expect(newContent).toBe("This is a new file\nwith multiple lines")
+        
+        const existingPath = path.join(fixture.path, "existing.txt")
+        const existingContent = await fs.readFile(existingPath, "utf-8")
+        expect(existingContent).toBe("old content\nnew line\nmore content")
+        
+        const configPath = path.join(fixture.path, "config.json")
+        const configContent = await fs.readFile(configPath, "utf-8")
+        expect(configContent).toBe('{\n  "version": "1.0",\n  "debug": true\n}')
+      },
+    })
+  })
+})