From 5b723fcce48c31cd2249a3d74acc1cc46539a560 Mon Sep 17 00:00:00 2001 From: dcm Date: Thu, 15 Jan 2026 16:45:04 +0800 Subject: [PATCH 1/3] fix(snapshot): fix revert not restoring files due to absolute path issue Root cause: Snapshot.patch() returns absolute paths in files array, but git checkout expects paths relative to worktree, causing checkout to fail silently and files not being restored. Changes: - Convert absolute paths to relative using path.relative() - Add safety guards: worktree escape check, empty path check - Normalize path separators for Windows compatibility - Add fs.mkdir() before fs.writeFile() in git show fallback - Add 13 test cases covering the fix and edge cases Fixes TUI Revert not actually reverting file changes in workspace. --- packages/opencode/src/snapshot/index.ts | 42 ++- .../opencode/test/snapshot/snapshot.test.ts | 322 ++++++++++++++++++ 2 files changed, 354 insertions(+), 10 deletions(-) diff --git a/packages/opencode/src/snapshot/index.ts b/packages/opencode/src/snapshot/index.ts index 46c97cf8dfd..8e4f7e6041d 100644 --- a/packages/opencode/src/snapshot/index.ts +++ b/packages/opencode/src/snapshot/index.ts @@ -7,6 +7,7 @@ import z from "zod" import { Config } from "../config/config" import { Instance } from "../project/instance" import { Scheduler } from "../scheduler" +import { Filesystem } from "../util/filesystem" export namespace Snapshot { const log = Log.create({ service: "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(() => {}) diff --git a/packages/opencode/test/snapshot/snapshot.test.ts b/packages/opencode/test/snapshot/snapshot.test.ts index cf933f81286..ededec82f10 100644 --- a/packages/opencode/test/snapshot/snapshot.test.ts +++ b/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" @@ -937,3 +938,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") + }, + }) +}) From 5d299aeba24e93487b5039cf5fddeeb2bcf32b61 Mon Sep 17 00:00:00 2001 From: Admin Date: Mon, 19 Jan 2026 21:57:04 +0800 Subject: [PATCH 2/3] test(snapshot): add extended test cases for revert functionality Based on PR #8631 by @Twisted928, this adds 8 additional test scenarios: - Unicode/Chinese character filenames (skipped - reveals bug) - Emoji filenames (skipped - reveals bug) - Symlink handling - Large file handling (1MB) - Binary file handling - Concurrent operations - Empty file handling - Restore to empty state Co-authored-by: Twisted <46923858+Twisted928@users.noreply.github.com> --- .../opencode/test/snapshot/snapshot.test.ts | 206 ++++++++++++++++++ 1 file changed, 206 insertions(+) diff --git a/packages/opencode/test/snapshot/snapshot.test.ts b/packages/opencode/test/snapshot/snapshot.test.ts index ededec82f10..d7d7d305311 100644 --- a/packages/opencode/test/snapshot/snapshot.test.ts +++ b/packages/opencode/test/snapshot/snapshot.test.ts @@ -1259,3 +1259,209 @@ test("revert creates parent directory when using fallback", async () => { }, }) }) + +test.skip("revert handles files with unicode and chinese characters in names", async () => { + await using tmp = await bootstrap() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + await Bun.write(`${tmp.path}/中文文件.txt`, "original chinese content") + await Bun.write(`${tmp.path}/日本語ファイル.txt`, "original japanese content") + await Bun.write(`${tmp.path}/한국어파일.txt`, "original korean content") + + const before = await Snapshot.track() + expect(before).toBeTruthy() + + await Bun.write(`${tmp.path}/中文文件.txt`, "MODIFIED chinese") + await Bun.write(`${tmp.path}/日本語ファイル.txt`, "MODIFIED japanese") + await Bun.write(`${tmp.path}/新建中文文件.txt`, "new chinese file") + + await Snapshot.revert([await Snapshot.patch(before!)]) + + expect(await Bun.file(`${tmp.path}/中文文件.txt`).text()).toBe("original chinese content") + expect(await Bun.file(`${tmp.path}/日本語ファイル.txt`).text()).toBe("original japanese content") + expect(await Bun.file(`${tmp.path}/한국어파일.txt`).text()).toBe("original korean content") + expect(await Bun.file(`${tmp.path}/新建中文文件.txt`).exists()).toBe(false) + }, + }) +}) + +test.skip("revert handles files with emoji in names", async () => { + await using tmp = await bootstrap() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + await Bun.write(`${tmp.path}/readme-🚀.txt`, "original rocket content") + await Bun.write(`${tmp.path}/config-⚙️.txt`, "original config content") + + const before = await Snapshot.track() + expect(before).toBeTruthy() + + await Bun.write(`${tmp.path}/readme-🚀.txt`, "MODIFIED rocket") + await Bun.write(`${tmp.path}/new-file-✨.txt`, "new sparkle file") + + await Snapshot.revert([await Snapshot.patch(before!)]) + + expect(await Bun.file(`${tmp.path}/readme-🚀.txt`).text()).toBe("original rocket content") + expect(await Bun.file(`${tmp.path}/config-⚙️.txt`).text()).toBe("original config content") + expect(await Bun.file(`${tmp.path}/new-file-✨.txt`).exists()).toBe(false) + }, + }) +}) + +test("revert handles symlinks correctly", async () => { + await using tmp = await bootstrap() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + // Create a target file and symlink + await Bun.write(`${tmp.path}/target.txt`, "target content") + await $`ln -s target.txt ${tmp.path}/link.txt`.quiet().nothrow() + + const before = await Snapshot.track() + expect(before).toBeTruthy() + + // Modify the target file (symlink should reflect changes) + await Bun.write(`${tmp.path}/target.txt`, "MODIFIED target") + + const patch = await Snapshot.patch(before!) + + await Snapshot.revert([patch]) + + expect(await Bun.file(`${tmp.path}/target.txt`).text()).toBe("target content") + }, + }) +}) + +test("revert handles large files correctly", async () => { + await using tmp = await bootstrap() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + // Create a large file (1MB of repeated content) + const largeContent = "x".repeat(1024 * 1024) + await Bun.write(`${tmp.path}/large-file.txt`, largeContent) + + const before = await Snapshot.track() + expect(before).toBeTruthy() + + // Modify the large file + const modifiedContent = "y".repeat(1024 * 1024) + await Bun.write(`${tmp.path}/large-file.txt`, modifiedContent) + + expect(await Bun.file(`${tmp.path}/large-file.txt`).text()).toBe(modifiedContent) + + await Snapshot.revert([await Snapshot.patch(before!)]) + + expect(await Bun.file(`${tmp.path}/large-file.txt`).text()).toBe(largeContent) + }, + }) +}) + +test("revert handles binary files correctly", async () => { + await using tmp = await bootstrap() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + // Create a binary file with specific byte pattern + const originalBytes = new Uint8Array([0x00, 0x01, 0x02, 0x03, 0xff, 0xfe, 0xfd, 0x89, 0x50, 0x4e, 0x47]) + await Bun.write(`${tmp.path}/binary.bin`, originalBytes) + + const before = await Snapshot.track() + expect(before).toBeTruthy() + + // Modify binary file + const modifiedBytes = new Uint8Array([0xff, 0xff, 0x00, 0x00, 0xab, 0xcd, 0xef]) + await Bun.write(`${tmp.path}/binary.bin`, modifiedBytes) + + await Snapshot.revert([await Snapshot.patch(before!)]) + + const restoredContent = await Bun.file(`${tmp.path}/binary.bin`).arrayBuffer() + const restoredBytes = new Uint8Array(restoredContent) + expect(restoredBytes).toEqual(originalBytes) + }, + }) +}) + +test("revert handles concurrent operations correctly", async () => { + await using tmp = await bootstrap() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + // Create multiple files + await Bun.write(`${tmp.path}/file1.txt`, "content1") + await Bun.write(`${tmp.path}/file2.txt`, "content2") + await Bun.write(`${tmp.path}/file3.txt`, "content3") + + const before = await Snapshot.track() + expect(before).toBeTruthy() + + // Modify all files + await Bun.write(`${tmp.path}/file1.txt`, "modified1") + await Bun.write(`${tmp.path}/file2.txt`, "modified2") + await Bun.write(`${tmp.path}/file3.txt`, "modified3") + await Bun.write(`${tmp.path}/file4.txt`, "new file") + await Bun.write(`${tmp.path}/file5.txt`, "another new file") + + // Get multiple patches + const patch1 = await Snapshot.patch(before!) + + // Simulate concurrent revert calls (though they run sequentially in practice) + await Promise.all([ + Snapshot.revert([patch1]), + ]) + + expect(await Bun.file(`${tmp.path}/file1.txt`).text()).toBe("content1") + expect(await Bun.file(`${tmp.path}/file2.txt`).text()).toBe("content2") + expect(await Bun.file(`${tmp.path}/file3.txt`).text()).toBe("content3") + expect(await Bun.file(`${tmp.path}/file4.txt`).exists()).toBe(false) + expect(await Bun.file(`${tmp.path}/file5.txt`).exists()).toBe(false) + }, + }) +}) + +test("revert handles empty files correctly", async () => { + await using tmp = await bootstrap() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + // Create an empty file + await Bun.write(`${tmp.path}/empty.txt`, "") + + const before = await Snapshot.track() + expect(before).toBeTruthy() + + // Add content to the empty file + await Bun.write(`${tmp.path}/empty.txt`, "now has content") + + expect(await Bun.file(`${tmp.path}/empty.txt`).text()).toBe("now has content") + + await Snapshot.revert([await Snapshot.patch(before!)]) + + expect(await Bun.file(`${tmp.path}/empty.txt`).text()).toBe("") + }, + }) +}) + +test("revert restores file to empty state when originally empty", async () => { + await using tmp = await bootstrap() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + // Start with a file that has content + await Bun.write(`${tmp.path}/will-be-emptied.txt`, "some content") + + const before = await Snapshot.track() + expect(before).toBeTruthy() + + // Empty the file + await Bun.write(`${tmp.path}/will-be-emptied.txt`, "") + + const patch = await Snapshot.patch(before!) + + await Snapshot.revert([patch]) + + expect(await Bun.file(`${tmp.path}/will-be-emptied.txt`).text()).toBe("some content") + }, + }) +}) From aa40a3602f6427496c2f24c9d0d7772c4c9d6de2 Mon Sep 17 00:00:00 2001 From: Admin Date: Mon, 19 Jan 2026 22:02:12 +0800 Subject: [PATCH 3/3] fix(snapshot): add unicode/emoji filename support in revert Use Bun.spawn instead of shell template for git commands to properly handle unicode filenames on Windows. The Bun shell ($`) has issues passing non-ASCII characters to git on Windows. Changes: - Add runGitCommand helper using Bun.spawn for reliable unicode support - Configure core.quotepath=false in git init and all git commands - Enable unicode/emoji filename tests (previously skipped) All 8 new test cases now pass: - Unicode/Chinese character filenames - Emoji filenames - Symlink handling - Large file handling (1MB) - Binary file handling - Concurrent operations - Empty file handling - Restore to empty state Co-authored-by: Twisted <46923858+Twisted928@users.noreply.github.com> --- packages/opencode/src/snapshot/index.ts | 83 ++++++++++++++----- .../opencode/test/snapshot/snapshot.test.ts | 4 +- 2 files changed, 63 insertions(+), 24 deletions(-) diff --git a/packages/opencode/src/snapshot/index.ts b/packages/opencode/src/snapshot/index.ts index 8e4f7e6041d..4352ca0a9c6 100644 --- a/packages/opencode/src/snapshot/index.ts +++ b/packages/opencode/src/snapshot/index.ts @@ -62,12 +62,12 @@ export namespace Snapshot { }) .quiet() .nothrow() - // Configure git to not convert line endings on Windows await $`git --git-dir ${git} config core.autocrlf false`.quiet().nothrow() + await $`git --git-dir ${git} config core.quotepath false`.quiet().nothrow() log.info("initialized") } - await $`git --git-dir ${git} --work-tree ${Instance.worktree} add .`.quiet().cwd(Instance.directory).nothrow() - const hash = await $`git --git-dir ${git} --work-tree ${Instance.worktree} write-tree` + await $`git -c core.quotepath=false --git-dir ${git} --work-tree ${Instance.worktree} add .`.quiet().cwd(Instance.directory).nothrow() + const hash = await $`git -c core.quotepath=false --git-dir ${git} --work-tree ${Instance.worktree} write-tree` .quiet() .cwd(Instance.directory) .nothrow() @@ -86,7 +86,7 @@ export namespace Snapshot { const git = gitdir() await $`git --git-dir ${git} --work-tree ${Instance.worktree} add .`.quiet().cwd(Instance.directory).nothrow() const result = - await $`git -c core.autocrlf=false --git-dir ${git} --work-tree ${Instance.worktree} diff --no-ext-diff --name-only ${hash} -- .` + await $`git -c core.autocrlf=false -c core.quotepath=false --git-dir ${git} --work-tree ${Instance.worktree} diff --no-ext-diff --name-only ${hash} -- .` .quiet() .cwd(Instance.directory) .nothrow() @@ -142,33 +142,31 @@ export namespace Snapshot { } 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() + const result = await runGitCommand( + ["checkout", item.hash, "--", gitPath], + { gitDir: git, workTree: Instance.worktree, cwd: Instance.worktree }, + ) if (result.exitCode !== 0) { - const checkTree = - 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()) { + const checkTree = await runGitCommand( + ["ls-tree", item.hash, "--", gitPath], + { gitDir: git, workTree: Instance.worktree, cwd: Instance.worktree }, + ) + if (checkTree.exitCode === 0 && checkTree.stdout.trim()) { 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() + const content = await runGitCommand( + ["show", `${item.hash}:${gitPath}`], + { gitDir: git, cwd: Instance.worktree }, + ) if (content.exitCode === 0) { const dir = path.dirname(file) await fs.mkdir(dir, { recursive: true }) - await fs.writeFile(file, content.stdout) + await fs.writeFile(file, content.stdoutBuffer) 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(), + checkoutError: result.stderr, + showError: content.stderr, }) } } else { @@ -181,6 +179,47 @@ export namespace Snapshot { } } + interface GitCommandOptions { + gitDir?: string + workTree?: string + cwd?: string + } + + interface GitCommandResult { + exitCode: number + stdout: string + stderr: string + stdoutBuffer: Buffer + } + + async function runGitCommand(args: string[], options: GitCommandOptions): Promise { + const gitArgs = ["-c", "core.quotepath=false"] + if (options.gitDir) { + gitArgs.push("--git-dir", options.gitDir) + } + if (options.workTree) { + gitArgs.push("--work-tree", options.workTree) + } + gitArgs.push(...args) + + const proc = Bun.spawn(["git", ...gitArgs], { + cwd: options.cwd, + stdout: "pipe", + stderr: "pipe", + }) + + const exitCode = await proc.exited + const stdoutBuffer = Buffer.from(await new Response(proc.stdout).arrayBuffer()) + const stderr = await new Response(proc.stderr).text() + + return { + exitCode, + stdout: stdoutBuffer.toString("utf-8"), + stderr, + stdoutBuffer, + } + } + export async function diff(hash: string) { const git = gitdir() await $`git --git-dir ${git} --work-tree ${Instance.worktree} add .`.quiet().cwd(Instance.directory).nothrow() diff --git a/packages/opencode/test/snapshot/snapshot.test.ts b/packages/opencode/test/snapshot/snapshot.test.ts index d7d7d305311..7872a7373b9 100644 --- a/packages/opencode/test/snapshot/snapshot.test.ts +++ b/packages/opencode/test/snapshot/snapshot.test.ts @@ -1260,7 +1260,7 @@ test("revert creates parent directory when using fallback", async () => { }) }) -test.skip("revert handles files with unicode and chinese characters in names", async () => { +test("revert handles files with unicode and chinese characters in names", async () => { await using tmp = await bootstrap() await Instance.provide({ directory: tmp.path, @@ -1286,7 +1286,7 @@ test.skip("revert handles files with unicode and chinese characters in names", a }) }) -test.skip("revert handles files with emoji in names", async () => { +test("revert handles files with emoji in names", async () => { await using tmp = await bootstrap() await Instance.provide({ directory: tmp.path,