From b24cc8087abc3de8127da97d15ab56e42dbbd481 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 24 May 2026 17:59:58 -0700 Subject: [PATCH] Allow case-only file explorer renames (#2763) * Allow file explorer case-only renames Reference design doc: docs/file-explorer-case-only-rename.md * Remove case-only rename design doc --- src/main/file-explorer-rename-collision.ts | 51 ++++++++ src/main/ipc/filesystem-mutations.test.ts | 63 +++++++++- src/main/ipc/filesystem-mutations.ts | 3 +- src/main/runtime/orca-runtime-files.test.ts | 133 ++++++++++++++++++-- src/main/runtime/orca-runtime-files.ts | 3 +- 5 files changed, 240 insertions(+), 13 deletions(-) create mode 100644 src/main/file-explorer-rename-collision.ts diff --git a/src/main/file-explorer-rename-collision.ts b/src/main/file-explorer-rename-collision.ts new file mode 100644 index 000000000..073eaf2b4 --- /dev/null +++ b/src/main/file-explorer-rename-collision.ts @@ -0,0 +1,51 @@ +import { lstat } from 'fs/promises' +import type { Stats } from 'fs' +import { basename, dirname } from 'path' + +function isENOENT(error: unknown): boolean { + return ( + error instanceof Error && 'code' in error && (error as NodeJS.ErrnoException).code === 'ENOENT' + ) +} + +function caseFoldFileExplorerBasename(name: string): string { + // Why: APFS may surface canonically equivalent Unicode names in different forms. + // Node has no filesystem-native collation API here, so this is best-effort. + return name.normalize('NFC').toLowerCase() +} + +function hasSameFilesystemIdentity(oldStat: Stats, newStat: Stats): boolean { + return oldStat.dev === newStat.dev && oldStat.ino === newStat.ino +} + +function isCaseOnlySameParentRename(oldPath: string, newPath: string): boolean { + const oldBasename = basename(oldPath) + const newBasename = basename(newPath) + return ( + dirname(oldPath) === dirname(newPath) && + oldBasename !== newBasename && + caseFoldFileExplorerBasename(oldBasename) === caseFoldFileExplorerBasename(newBasename) + ) +} + +export async function assertFileExplorerRenameDestinationAvailable( + oldPath: string, + newPath: string +): Promise { + let newStat: Stats + try { + newStat = await lstat(newPath) + } catch (error) { + if (isENOENT(error)) { + return + } + throw error + } + + const oldStat = await lstat(oldPath) + if (hasSameFilesystemIdentity(oldStat, newStat) && isCaseOnlySameParentRename(oldPath, newPath)) { + return + } + + throw new Error(`A file or folder named '${basename(newPath)}' already exists in this location`) +} diff --git a/src/main/ipc/filesystem-mutations.test.ts b/src/main/ipc/filesystem-mutations.test.ts index 13c4a648a..5ef07164e 100644 --- a/src/main/ipc/filesystem-mutations.test.ts +++ b/src/main/ipc/filesystem-mutations.test.ts @@ -58,6 +58,10 @@ function mockRealpath(mapping: Record) { }) } +function mockStats(dev: number, ino: number) { + return { dev, ino, isDirectory: () => false } +} + describe('registerFilesystemMutationHandlers', () => { beforeEach(() => { handlers.clear() @@ -162,18 +166,22 @@ describe('registerFilesystemMutationHandlers', () => { expect(renameMock).toHaveBeenCalledWith(oldPath, newPath) }) - it('rejects rename when destination already exists', async () => { + it('rejects rename when destination already exists as a true collision', async () => { + const oldPath = path.resolve('/workspace/repo/old.ts') const resolvedNewPath = path.resolve('/workspace/repo/new.ts') lstatMock.mockImplementation(async (p: string) => { + if (p === oldPath) { + return mockStats(1, 10) + } if (p === resolvedNewPath) { - return { isDirectory: () => false } + return mockStats(1, 11) } throw enoent() }) await expect( handlers.get('fs:rename')!(null, { - oldPath: path.resolve('/workspace/repo/old.ts'), + oldPath, newPath: resolvedNewPath }) ).rejects.toThrow("A file or folder named 'new.ts' already exists in this location") @@ -181,6 +189,55 @@ describe('registerFilesystemMutationHandlers', () => { expect(renameMock).not.toHaveBeenCalled() }) + it('allows case-only rename when destination is the same entry in the same parent', async () => { + const oldPath = path.resolve('/workspace/repo/README.md') + const newPath = path.resolve('/workspace/repo/readme.md') + lstatMock.mockImplementation(async (p: string) => { + if (p === oldPath || p === newPath) { + return mockStats(2, 20) + } + throw enoent() + }) + + await handlers.get('fs:rename')!(null, { oldPath, newPath }) + + expect(renameMock).toHaveBeenCalledWith(oldPath, newPath) + }) + + it('rejects hard-link alias rename collisions even when dev and ino match', async () => { + const oldPath = path.resolve('/workspace/repo/README.md') + const newPath = path.resolve('/workspace/repo/README-hardlink.md') + lstatMock.mockImplementation(async (p: string) => { + if (p === oldPath || p === newPath) { + return mockStats(3, 30) + } + throw enoent() + }) + + await expect(handlers.get('fs:rename')!(null, { oldPath, newPath })).rejects.toThrow( + "A file or folder named 'README-hardlink.md' already exists in this location" + ) + + expect(renameMock).not.toHaveBeenCalled() + }) + + it('rejects cross-parent case-only rename collisions even when dev and ino match', async () => { + const oldPath = path.resolve('/workspace/repo/src/README.md') + const newPath = path.resolve('/workspace/repo/docs/readme.md') + lstatMock.mockImplementation(async (p: string) => { + if (p === oldPath || p === newPath) { + return mockStats(4, 40) + } + throw enoent() + }) + + await expect(handlers.get('fs:rename')!(null, { oldPath, newPath })).rejects.toThrow( + "A file or folder named 'readme.md' already exists in this location" + ) + + expect(renameMock).not.toHaveBeenCalled() + }) + it('rejects rename when parent directory escapes allowed roots', async () => { // Why: the parent is still canonicalized (preserveSymlink only preserves // the leaf). A symlinked ancestor that points outside allowed roots must diff --git a/src/main/ipc/filesystem-mutations.ts b/src/main/ipc/filesystem-mutations.ts index cecb1568d..f1f7a30e1 100644 --- a/src/main/ipc/filesystem-mutations.ts +++ b/src/main/ipc/filesystem-mutations.ts @@ -20,6 +20,7 @@ import type { Store } from '../persistence' import { authorizeExternalPath, resolveAuthorizedPath, isENOENT } from './filesystem-auth' import { requireSshFilesystemProvider } from '../providers/ssh-filesystem-dispatch' import { importExternalPathsSsh } from './filesystem-import-ssh' +import { assertFileExplorerRenameDestinationAvailable } from '../file-explorer-rename-collision' /** * Re-throw filesystem errors with user-friendly messages. @@ -118,7 +119,7 @@ export function registerFilesystemMutationHandlers(store: Store): void { // accidentally write into a symlinked destination name. const oldPath = await resolveAuthorizedPath(args.oldPath, store, { preserveSymlink: true }) const newPath = await resolveAuthorizedPath(args.newPath, store, { preserveSymlink: true }) - await assertNotExists(newPath) + await assertFileExplorerRenameDestinationAvailable(oldPath, newPath) await rename(oldPath, newPath) } ) diff --git a/src/main/runtime/orca-runtime-files.test.ts b/src/main/runtime/orca-runtime-files.test.ts index d0fc2c78f..ed4d4d9c8 100644 --- a/src/main/runtime/orca-runtime-files.test.ts +++ b/src/main/runtime/orca-runtime-files.test.ts @@ -3,14 +3,21 @@ import type * as Fs from 'fs' import type * as FsPromises from 'fs/promises' import type * as FilesystemAuth from '../ipc/filesystem-auth' -const { resolveAuthorizedPathMock, statMock, subscribeParcelWatcherMock, watchMock } = vi.hoisted( - () => ({ - resolveAuthorizedPathMock: vi.fn(), - statMock: vi.fn(), - subscribeParcelWatcherMock: vi.fn(), - watchMock: vi.fn() - }) -) +const { + lstatMock, + renameMock, + resolveAuthorizedPathMock, + statMock, + subscribeParcelWatcherMock, + watchMock +} = vi.hoisted(() => ({ + lstatMock: vi.fn(), + renameMock: vi.fn(), + resolveAuthorizedPathMock: vi.fn(), + statMock: vi.fn(), + subscribeParcelWatcherMock: vi.fn(), + watchMock: vi.fn() +})) vi.mock('fs', async () => { const actual = await vi.importActual('fs') @@ -24,6 +31,8 @@ vi.mock('fs/promises', async () => { const actual = await vi.importActual('fs/promises') return { ...actual, + lstat: lstatMock, + rename: renameMock, stat: statMock } }) @@ -48,15 +57,45 @@ vi.mock('../providers/ssh-filesystem-dispatch', () => ({ import { awaitRuntimeFileWatcherUnsubscribes, RuntimeFileCommands } from './orca-runtime-files' +function enoent(): Error { + return Object.assign(new Error('ENOENT'), { code: 'ENOENT' }) +} + +function mockStats(dev: number, ino: number) { + return { dev, ino, isDirectory: () => false } +} + +function createRuntimeFileCommands() { + const store = { + getRepo: vi.fn(() => undefined) + } + const commands = new RuntimeFileCommands({ + getRuntimeId: () => 'runtime-1', + requireStore: () => store, + resolveWorktreeSelector: vi.fn(async () => ({ + id: 'wt-1', + repoId: 'repo-1', + path: '/repo' + })), + resolveRuntimeGitTarget: vi.fn(), + openFile: vi.fn() + } as never) + return { commands, store } +} + describe('RuntimeFileCommands', () => { const originalPlatform = process.platform beforeEach(() => { vi.useFakeTimers() + lstatMock.mockReset() + renameMock.mockReset() resolveAuthorizedPathMock.mockReset() statMock.mockReset() subscribeParcelWatcherMock.mockReset() watchMock.mockReset() + lstatMock.mockRejectedValue(enoent()) + renameMock.mockResolvedValue(undefined) Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform @@ -98,6 +137,84 @@ describe('RuntimeFileCommands', () => { }) }) + it('renames a runtime-local file when destination does not exist', async () => { + const { commands } = createRuntimeFileCommands() + resolveAuthorizedPathMock.mockImplementation(async (p: string) => p) + + await commands.renameFileExplorerPath('id:wt-1', 'old.ts', 'new.ts') + + expect(renameMock).toHaveBeenCalledWith('/repo/old.ts', '/repo/new.ts') + }) + + it('allows runtime-local case-only rename with IPC parity guard behavior', async () => { + const { commands } = createRuntimeFileCommands() + resolveAuthorizedPathMock.mockImplementation(async (p: string) => p) + lstatMock.mockImplementation(async (p: string) => { + if (p === '/repo/README.md' || p === '/repo/readme.md') { + return mockStats(10, 100) + } + throw enoent() + }) + + await commands.renameFileExplorerPath('id:wt-1', 'README.md', 'readme.md') + + expect(renameMock).toHaveBeenCalledWith('/repo/README.md', '/repo/readme.md') + }) + + it('rejects runtime-local true destination collisions', async () => { + const { commands } = createRuntimeFileCommands() + resolveAuthorizedPathMock.mockImplementation(async (p: string) => p) + lstatMock.mockImplementation(async (p: string) => { + if (p === '/repo/old.ts') { + return mockStats(11, 110) + } + if (p === '/repo/new.ts') { + return mockStats(11, 111) + } + throw enoent() + }) + + await expect(commands.renameFileExplorerPath('id:wt-1', 'old.ts', 'new.ts')).rejects.toThrow( + "A file or folder named 'new.ts' already exists in this location" + ) + + expect(renameMock).not.toHaveBeenCalled() + }) + + it('rejects runtime-local hard-link alias collisions', async () => { + const { commands } = createRuntimeFileCommands() + resolveAuthorizedPathMock.mockImplementation(async (p: string) => p) + lstatMock.mockImplementation(async (p: string) => { + if (p === '/repo/README.md' || p === '/repo/README-hardlink.md') { + return mockStats(12, 120) + } + throw enoent() + }) + + await expect( + commands.renameFileExplorerPath('id:wt-1', 'README.md', 'README-hardlink.md') + ).rejects.toThrow("A file or folder named 'README-hardlink.md' already exists in this location") + + expect(renameMock).not.toHaveBeenCalled() + }) + + it('rejects runtime-local cross-parent case-only collisions', async () => { + const { commands } = createRuntimeFileCommands() + resolveAuthorizedPathMock.mockImplementation(async (p: string) => p) + lstatMock.mockImplementation(async (p: string) => { + if (p === '/repo/src/README.md' || p === '/repo/docs/readme.md') { + return mockStats(13, 130) + } + throw enoent() + }) + + await expect( + commands.renameFileExplorerPath('id:wt-1', 'src/README.md', 'docs/readme.md') + ).rejects.toThrow("A file or folder named 'readme.md' already exists in this location") + + expect(renameMock).not.toHaveBeenCalled() + }) + it('uses a conservative Node watcher for Windows runtime file watches', async () => { Object.defineProperty(process, 'platform', { configurable: true, diff --git a/src/main/runtime/orca-runtime-files.ts b/src/main/runtime/orca-runtime-files.ts index f5c5c90b6..d1bc94836 100644 --- a/src/main/runtime/orca-runtime-files.ts +++ b/src/main/runtime/orca-runtime-files.ts @@ -53,6 +53,7 @@ import { getSshFilesystemProvider, SSH_FILESYSTEM_PROVIDER_UNAVAILABLE_MESSAGE } from '../providers/ssh-filesystem-dispatch' +import { assertFileExplorerRenameDestinationAvailable } from '../file-explorer-rename-collision' import { joinWorktreeRelativePath, normalizeRuntimeRelativePath } from './runtime-relative-paths' const MOBILE_FILE_LIST_LIMIT = 5000 @@ -549,7 +550,7 @@ export class RuntimeFileCommands { const store = this.host.requireStore() const oldPath = await resolveAuthorizedPath(oldTarget.path, store, { preserveSymlink: true }) const newPath = await resolveAuthorizedPath(newTarget.path, store, { preserveSymlink: true }) - await assertRuntimePathDoesNotExist(newPath) + await assertFileExplorerRenameDestinationAvailable(oldPath, newPath) await rename(oldPath, newPath) return { ok: true } }