fix(snapshot): fix revert not restoring files due to absolute path issue

packages/opencode/src/snapshot/index.ts

@@ -6,6 +6,7 @@ import { Global } from "../global"
 import z from "zod"
 import { Config } from "../config/config"
 import { Instance } from "../project/instance"
+import { Filesystem } from "../util/filesystem"
 import { Scheduler } from "../scheduler"
 
 export namespace Snapshot {
@@ -133,22 +134,43 @@ export namespace Snapshot {
     for (const item of patches) {
       for (const file of item.files) {
         if (files.has(file)) continue
-        log.info("reverting", { file, hash: item.hash })
-        const result = await $`git --git-dir ${git} --work-tree ${Instance.worktree} checkout ${item.hash} -- ${file}`
-          .quiet()
-          .cwd(Instance.worktree)
-          .nothrow()
+        const relativePath = path.relative(Instance.worktree, file)
+        if (!Filesystem.contains(Instance.worktree, file) || path.isAbsolute(relativePath) || relativePath === "") {
+          log.warn("skipping file outside worktree", { file, relativePath, worktree: Instance.worktree })
+          files.add(file)
+          continue
+        }
+        const gitPath = process.platform === "win32" ? relativePath.replaceAll("\\", "/") : relativePath
+        log.info("reverting", { file, gitPath, hash: item.hash })
+        const result =
+          await $`git --git-dir ${git} --work-tree ${Instance.worktree} checkout ${item.hash} -- ${gitPath}`
+            .quiet()
+            .cwd(Instance.worktree)
+            .nothrow()
         if (result.exitCode !== 0) {
-          const relativePath = path.relative(Instance.worktree, file)
           const checkTree =
-            await $`git --git-dir ${git} --work-tree ${Instance.worktree} ls-tree ${item.hash} -- ${relativePath}`
+            await $`git --git-dir ${git} --work-tree ${Instance.worktree} ls-tree ${item.hash} -- ${gitPath}`
               .quiet()
               .cwd(Instance.worktree)
               .nothrow()
           if (checkTree.exitCode === 0 && checkTree.text().trim()) {
-            log.info("file existed in snapshot but checkout failed, keeping", {
-              file,
-            })
+            log.info("checkout failed, trying git show fallback", { file, gitPath })
+            const content = await $`git --git-dir ${git} show ${item.hash}:${gitPath}`
+              .quiet()
+              .cwd(Instance.worktree)
+              .nothrow()
+            if (content.exitCode === 0) {
+              const dir = path.dirname(file)
+              await fs.mkdir(dir, { recursive: true })
+              await fs.writeFile(file, content.stdout)
+              log.info("restored file via git show", { file })
+            } else {
+              log.warn("failed to restore file, keeping current version", {
+                file,
+                checkoutError: result.stderr.toString(),
+                showError: content.stderr.toString(),
+              })
+            }
           } else {
             log.info("file did not exist in snapshot, deleting", { file })
             await fs.unlink(file).catch(() => {})

packages/opencode/test/snapshot/snapshot.test.ts

@@ -1,5 +1,6 @@
 import { test, expect } from "bun:test"
 import { $ } from "bun"
+import path from "path"
 import { Snapshot } from "../../src/snapshot"
 import { Instance } from "../../src/project/instance"
 import { tmpdir } from "../fixture/fixture"
@@ -992,3 +993,324 @@ test("diffFull with whitespace changes", async () => {
     },
   })
 })
+
+test("patch returns absolute paths and revert handles them correctly", async () => {
+  await using tmp = await bootstrap()
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      const before = await Snapshot.track()
+      expect(before).toBeTruthy()
+
+      await Bun.write(`${tmp.path}/newfile.txt`, "new content")
+      await Bun.write(`${tmp.path}/b.txt`, "modified content")
+
+      const patch = await Snapshot.patch(before!)
+
+      expect(patch.files.length).toBe(2)
+      for (const file of patch.files) {
+        expect(path.isAbsolute(file)).toBe(true)
+        expect(file.startsWith(tmp.path)).toBe(true)
+      }
+
+      await Snapshot.revert([patch])
+
+      expect(await Bun.file(`${tmp.path}/newfile.txt`).exists()).toBe(false)
+      expect(await Bun.file(`${tmp.path}/b.txt`).text()).toBe(tmp.extra.bContent)
+    },
+  })
+})
+
+test("revert restores modified file content correctly", async () => {
+  await using tmp = await bootstrap()
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      const before = await Snapshot.track()
+      expect(before).toBeTruthy()
+
+      const originalA = tmp.extra.aContent
+      const originalB = tmp.extra.bContent
+      await Bun.write(`${tmp.path}/a.txt`, "COMPLETELY DIFFERENT CONTENT A")
+      await Bun.write(`${tmp.path}/b.txt`, "COMPLETELY DIFFERENT CONTENT B")
+
+      expect(await Bun.file(`${tmp.path}/a.txt`).text()).toBe("COMPLETELY DIFFERENT CONTENT A")
+      expect(await Bun.file(`${tmp.path}/b.txt`).text()).toBe("COMPLETELY DIFFERENT CONTENT B")
+
+      await Snapshot.revert([await Snapshot.patch(before!)])
+
+      expect(await Bun.file(`${tmp.path}/a.txt`).text()).toBe(originalA)
+      expect(await Bun.file(`${tmp.path}/b.txt`).text()).toBe(originalB)
+    },
+  })
+})
+
+test("revert handles deeply nested files with absolute paths", async () => {
+  await using tmp = await bootstrap()
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      await $`mkdir -p ${tmp.path}/level1/level2/level3`.quiet()
+      await Bun.write(`${tmp.path}/level1/level2/level3/existing.txt`, "original nested content")
+
+      const before = await Snapshot.track()
+      expect(before).toBeTruthy()
+
+      await Bun.write(`${tmp.path}/level1/level2/level3/existing.txt`, "MODIFIED")
+      await Bun.write(`${tmp.path}/level1/level2/level3/new.txt`, "new nested file")
+      await Bun.write(`${tmp.path}/level1/level2/another.txt`, "another new file")
+
+      const patch = await Snapshot.patch(before!)
+
+      for (const file of patch.files) {
+        expect(path.isAbsolute(file)).toBe(true)
+      }
+
+      await Snapshot.revert([patch])
+
+      expect(await Bun.file(`${tmp.path}/level1/level2/level3/existing.txt`).text()).toBe("original nested content")
+      expect(await Bun.file(`${tmp.path}/level1/level2/level3/new.txt`).exists()).toBe(false)
+      expect(await Bun.file(`${tmp.path}/level1/level2/another.txt`).exists()).toBe(false)
+    },
+  })
+})
+
+test("revert handles files with spaces in names", async () => {
+  await using tmp = await bootstrap()
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      await Bun.write(`${tmp.path}/file with spaces.txt`, "original spaces")
+      await Bun.write(`${tmp.path}/file-with-dashes.txt`, "original dashes")
+
+      const before = await Snapshot.track()
+      expect(before).toBeTruthy()
+
+      await Bun.write(`${tmp.path}/file with spaces.txt`, "MODIFIED spaces")
+      await Bun.write(`${tmp.path}/new file with spaces.txt`, "new file")
+      await $`mkdir -p "${tmp.path}/dir with spaces"`.quiet()
+      await Bun.write(`${tmp.path}/dir with spaces/nested file.txt`, "nested in space dir")
+
+      await Snapshot.revert([await Snapshot.patch(before!)])
+
+      expect(await Bun.file(`${tmp.path}/file with spaces.txt`).text()).toBe("original spaces")
+      expect(await Bun.file(`${tmp.path}/file-with-dashes.txt`).text()).toBe("original dashes")
+      expect(await Bun.file(`${tmp.path}/new file with spaces.txt`).exists()).toBe(false)
+      expect(await Bun.file(`${tmp.path}/dir with spaces/nested file.txt`).exists()).toBe(false)
+    },
+  })
+})
+
+test("revert handles multiple sequential patches correctly", async () => {
+  await using tmp = await bootstrap()
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      const snapshot1 = await Snapshot.track()
+      expect(snapshot1).toBeTruthy()
+
+      await Bun.write(`${tmp.path}/a.txt`, "version 2")
+      await Bun.write(`${tmp.path}/new1.txt`, "new file 1")
+
+      const patch1 = await Snapshot.patch(snapshot1!)
+      const snapshot2 = await Snapshot.track()
+
+      await Bun.write(`${tmp.path}/a.txt`, "version 3")
+      await Bun.write(`${tmp.path}/new2.txt`, "new file 2")
+
+      const patch2 = await Snapshot.patch(snapshot2!)
+
+      expect(await Bun.file(`${tmp.path}/a.txt`).text()).toBe("version 3")
+      expect(await Bun.file(`${tmp.path}/new1.txt`).exists()).toBe(true)
+      expect(await Bun.file(`${tmp.path}/new2.txt`).exists()).toBe(true)
+
+      await Snapshot.revert([patch2])
+      expect(await Bun.file(`${tmp.path}/a.txt`).text()).toBe("version 2")
+      expect(await Bun.file(`${tmp.path}/new2.txt`).exists()).toBe(false)
+
+      await Snapshot.revert([patch1])
+      expect(await Bun.file(`${tmp.path}/a.txt`).text()).toBe(tmp.extra.aContent)
+      expect(await Bun.file(`${tmp.path}/new1.txt`).exists()).toBe(false)
+    },
+  })
+})
+
+test("revert with patches array containing multiple items", async () => {
+  await using tmp = await bootstrap()
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      const snapshot1 = await Snapshot.track()
+      expect(snapshot1).toBeTruthy()
+
+      await Bun.write(`${tmp.path}/file1.txt`, "content 1")
+      const patch1 = await Snapshot.patch(snapshot1!)
+
+      const snapshot2 = await Snapshot.track()
+      await Bun.write(`${tmp.path}/file2.txt`, "content 2")
+      const patch2 = await Snapshot.patch(snapshot2!)
+
+      await Snapshot.revert([patch2, patch1])
+
+      expect(await Bun.file(`${tmp.path}/file1.txt`).exists()).toBe(false)
+      expect(await Bun.file(`${tmp.path}/file2.txt`).exists()).toBe(false)
+    },
+  })
+})
+
+test("revert deletes file created after snapshot", async () => {
+  await using tmp = await bootstrap()
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      const before = await Snapshot.track()
+      expect(before).toBeTruthy()
+
+      await $`mkdir -p ${tmp.path}/newdir/subdir`.quiet()
+      await Bun.write(`${tmp.path}/newdir/subdir/created.txt`, "created after snapshot")
+
+      const patch = await Snapshot.patch(before!)
+      expect(patch.files).toContain(`${tmp.path}/newdir/subdir/created.txt`)
+
+      await Snapshot.revert([patch])
+
+      expect(await Bun.file(`${tmp.path}/newdir/subdir/created.txt`).exists()).toBe(false)
+    },
+  })
+})
+
+test("revert restores deleted file", async () => {
+  await using tmp = await bootstrap()
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      const before = await Snapshot.track()
+      expect(before).toBeTruthy()
+
+      await $`rm ${tmp.path}/a.txt`.quiet()
+      expect(await Bun.file(`${tmp.path}/a.txt`).exists()).toBe(false)
+
+      await Snapshot.revert([await Snapshot.patch(before!)])
+
+      expect(await Bun.file(`${tmp.path}/a.txt`).exists()).toBe(true)
+      expect(await Bun.file(`${tmp.path}/a.txt`).text()).toBe(tmp.extra.aContent)
+    },
+  })
+})
+
+test("patch files relative path computation is correct", async () => {
+  await using tmp = await bootstrap()
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      const before = await Snapshot.track()
+      expect(before).toBeTruthy()
+
+      await Bun.write(`${tmp.path}/test.txt`, "test")
+
+      const patch = await Snapshot.patch(before!)
+
+      for (const file of patch.files) {
+        const relativePath = path.relative(tmp.path, file)
+        expect(relativePath).not.toContain("..")
+        expect(path.isAbsolute(relativePath)).toBe(false)
+      }
+    },
+  })
+})
+
+test("revert works when process cwd differs from worktree", async () => {
+  await using tmp = await bootstrap()
+  await using otherDir = await tmpdir({ git: false })
+
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      const before = await Snapshot.track()
+      expect(before).toBeTruthy()
+
+      await Bun.write(`${tmp.path}/modified.txt`, "modified content")
+      await Bun.write(`${tmp.path}/new.txt`, "new content")
+
+      const patch = await Snapshot.patch(before!)
+
+      const originalCwd = process.cwd()
+      try {
+        process.chdir(otherDir.path)
+        await Snapshot.revert([patch])
+      } finally {
+        process.chdir(originalCwd)
+      }
+
+      expect(await Bun.file(`${tmp.path}/modified.txt`).exists()).toBe(false)
+      expect(await Bun.file(`${tmp.path}/new.txt`).exists()).toBe(false)
+    },
+  })
+})
+
+test("revert skips files outside worktree", async () => {
+  await using tmp = await bootstrap()
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      const before = await Snapshot.track()
+      expect(before).toBeTruthy()
+
+      await Bun.write(`${tmp.path}/safe.txt`, "safe content")
+
+      const patch = await Snapshot.patch(before!)
+      patch.files.push("/tmp/outside-worktree.txt")
+      patch.files.push(`${tmp.path}/../escape-attempt.txt`)
+
+      await Snapshot.revert([patch])
+
+      expect(await Bun.file(`${tmp.path}/safe.txt`).exists()).toBe(false)
+    },
+  })
+})
+
+test("revert skips patch with empty relativePath", async () => {
+  await using tmp = await bootstrap()
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      const before = await Snapshot.track()
+      expect(before).toBeTruthy()
+
+      await Bun.write(`${tmp.path}/normal.txt`, "normal content")
+
+      const patch = await Snapshot.patch(before!)
+      patch.files.push(tmp.path)
+
+      await Snapshot.revert([patch])
+
+      expect(await Bun.file(`${tmp.path}/normal.txt`).exists()).toBe(false)
+    },
+  })
+})
+
+test("revert creates parent directory when using fallback", async () => {
+  await using tmp = await bootstrap()
+  await Instance.provide({
+    directory: tmp.path,
+    fn: async () => {
+      await $`mkdir -p ${tmp.path}/deep/nested/dir`.quiet()
+      await Bun.write(`${tmp.path}/deep/nested/dir/file.txt`, "original content")
+
+      const before = await Snapshot.track()
+      expect(before).toBeTruthy()
+
+      await Bun.write(`${tmp.path}/deep/nested/dir/file.txt`, "modified content")
+
+      const patch = await Snapshot.patch(before!)
+
+      await $`rm -rf ${tmp.path}/deep`.quiet()
+      expect(await Bun.file(`${tmp.path}/deep/nested/dir/file.txt`).exists()).toBe(false)
+
+      await Snapshot.revert([patch])
+
+      expect(await Bun.file(`${tmp.path}/deep/nested/dir/file.txt`).exists()).toBe(true)
+      expect(await Bun.file(`${tmp.path}/deep/nested/dir/file.txt`).text()).toBe("original content")
+    },
+  })
+})