fix(fs): address symlink/delete/remote safety issues from review (#1012)

- Symlink delete/rename now preserve the link entry (new `preserveSymlink`
  option on resolveAuthorizedPath) instead of operating on the target.
- File-explorer delete force-saves dirty editors before trashing so undo
  restores the user's latest edits, not stale disk content.
- Thread `recursive` through preload → SSH provider so remote directory
  delete works end-to-end.
- Remote deletes now confirm (permanent `rm`, no Trash) and the toast
  reflects that no Trash/Recycle Bin is involved.
- Codex managed-home check canonicalizes the storage root before the
  prefix compare so macOS `/private/var` paths stop being rejected.

src/main/codex-accounts/service.ts

@@ -321,16 +321,6 @@ export class CodexAccountService {
     const resolvedCandidate = resolve(candidatePath)
     const resolvedRoot = resolve(rootPath)
 
-    // Why: in dev mode, userData points to orca-dev/ while production uses
-    // orca/. Accounts created by the packaged app store production paths in
-    // settings. A quick prefix check before realpathSync avoids noisy errors
-    // when dev instances encounter production-rooted managed home paths.
-    if (!resolvedCandidate.startsWith(resolvedRoot + sep)) {
-      throw new Error(
-        `Managed Codex home is outside current storage root (expected under ${resolvedRoot}).`
-      )
-    }
-
     if (!existsSync(resolvedCandidate)) {
       throw new Error('Managed Codex home directory does not exist on disk.')
     }
@@ -340,6 +330,21 @@ export class CodexAccountService {
     // canonical on-disk target rather than trusting persisted text blindly.
     const canonicalCandidate = realpathSync(resolvedCandidate)
     const canonicalRoot = realpathSync(resolvedRoot)
+
+    // Why: the prefix check must compare canonical paths on both sides. On
+    // macOS, userData sits under /var/folders/... which realpath resolves to
+    // /private/var/folders/...; comparing a canonical candidate against a
+    // non-canonical root would spuriously reject every managed home. In dev
+    // mode (orca-dev/ vs orca/) this check also filters out production-rooted
+    // paths before downstream sync runs.
+    if (
+      canonicalCandidate !== canonicalRoot &&
+      !canonicalCandidate.startsWith(canonicalRoot + sep)
+    ) {
+      throw new Error(
+        `Managed Codex home is outside current storage root (expected under ${canonicalRoot}).`
+      )
+    }
     const relativePath = relative(canonicalRoot, canonicalCandidate)
     const escaped =
       relativePath === '' || relativePath.startsWith('..') || relativePath.includes(`..${sep}`)
@@ -370,8 +375,11 @@ export class CodexAccountService {
     // just the home/ leaf leaves an empty <uuid>/ directory behind.
     try {
       const parentDir = resolve(managedHomePath, '..')
-      const root = this.getManagedAccountsRoot()
-      if (parentDir.startsWith(root) && parentDir !== root) {
+      // Why: managedHomePath is already canonicalized by assertManagedHomePath,
+      // so the root must be canonicalized too for the prefix check to work on
+      // macOS where userData resolves through /private/var.
+      const root = realpathSync(this.getManagedAccountsRoot())
+      if (parentDir.startsWith(root + sep) && parentDir !== root) {
         rmSync(parentDir, { recursive: true, force: true })
       }
     } catch {

src/main/ipc/filesystem-auth.ts

@@ -120,12 +120,39 @@ export function isENOENT(error: unknown): boolean {
   )
 }
 
-export async function resolveAuthorizedPath(targetPath: string, store: Store): Promise<string> {
+export type ResolveAuthorizedPathOptions = {
+  /**
+   * When true, canonicalize the parent directory but preserve the leaf so
+   * operations target the symlink itself rather than its destination. Required
+   * for delete and rename — following the symlink would trash or rename the
+   * target file (which can live outside allowed roots, or be another tracked
+   * file a symlink inside the worktree happens to point at).
+   */
+  preserveSymlink?: boolean
+}
+
+export async function resolveAuthorizedPath(
+  targetPath: string,
+  store: Store,
+  options: ResolveAuthorizedPathOptions = {}
+): Promise<string> {
   const resolvedTarget = resolve(targetPath)
   if (!(await isPathAllowedIncludingRegisteredWorktrees(resolvedTarget, store))) {
     throw new Error(PATH_ACCESS_DENIED_MESSAGE)
   }
 
+  if (options.preserveSymlink) {
+    // Canonicalize the parent so symlinks in ancestors cannot redirect us
+    // outside allowed roots, but keep the final segment untouched so callers
+    // (delete/rename) act on the link itself.
+    const realParent = await realpath(dirname(resolvedTarget))
+    const candidateTarget = resolve(realParent, basename(resolvedTarget))
+    if (!(await isPathAllowedIncludingRegisteredWorktrees(candidateTarget, store))) {
+      throw new Error(PATH_ACCESS_DENIED_MESSAGE)
+    }
+    return candidateTarget
+  }
+
   try {
     const realTarget = await realpath(resolvedTarget)
     if (!(await isPathAllowedIncludingRegisteredWorktrees(realTarget, store))) {

src/main/ipc/filesystem-mutations.test.ts

@@ -175,34 +175,39 @@ describe('registerFilesystemMutationHandlers', () => {
     expect(renameMock).not.toHaveBeenCalled()
   })
 
-  it('rejects rename when new path escapes allowed roots', async () => {
+  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
+    // still be rejected so callers cannot redirect rename through it.
     mockRealpath({
-      [path.resolve('/workspace/repo/escape.ts')]: path.resolve('/private/escape.ts')
+      [path.resolve('/workspace/repo/escape-dir')]: path.resolve('/private/escape-dir')
     })
 
     await expect(
       handlers.get('fs:rename')!(null, {
         oldPath: path.resolve('/workspace/repo/old.ts'),
-        newPath: path.resolve('/workspace/repo/escape.ts')
+        newPath: path.resolve('/workspace/repo/escape-dir/new.ts')
       })
     ).rejects.toThrow('Access denied')
 
     expect(renameMock).not.toHaveBeenCalled()
   })
 
-  it('rejects rename when old path escapes allowed roots', async () => {
+  it('renames a symlink without following its target', async () => {
+    // Why: rename must operate on the symlink entry, not its target —
+    // following the link would rename the target file (possibly elsewhere in
+    // the worktree, or outside allowed roots entirely). Even though the
+    // symlink points at /private/secret.ts, renaming the link entry inside
+    // the allowed root is a safe directory-entry mutation.
     mockRealpath({
       [path.resolve('/workspace/repo/symlink.ts')]: path.resolve('/private/secret.ts')
     })
 
-    await expect(
-      handlers.get('fs:rename')!(null, {
-        oldPath: path.resolve('/workspace/repo/symlink.ts'),
-        newPath: path.resolve('/workspace/repo/new.ts')
-      })
-    ).rejects.toThrow('Access denied')
+    const oldPath = path.resolve('/workspace/repo/symlink.ts')
+    const newPath = path.resolve('/workspace/repo/renamed-symlink.ts')
+    await handlers.get('fs:rename')!(null, { oldPath, newPath })
 
-    expect(renameMock).not.toHaveBeenCalled()
+    expect(renameMock).toHaveBeenCalledWith(oldPath, newPath)
   })
 
   // ── Edge cases ─────────────────────────────────────────────────

src/main/ipc/filesystem-mutations.ts

@@ -103,8 +103,14 @@ export function registerFilesystemMutationHandlers(store: Store): void {
         }
         return provider.rename(args.oldPath, args.newPath)
       }
-      const oldPath = await resolveAuthorizedPath(args.oldPath, store)
-      const newPath = await resolveAuthorizedPath(args.newPath, store)
+      // Why: rename() operates on directory entries, not file contents. If
+      // oldPath is a symlink, we must rename the link itself rather than
+      // resolving it to its target — following the link would rename the
+      // target file (potentially elsewhere in the worktree) and leave the
+      // symlink dangling. newPath must also preserve its leaf so we don't
+      // 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 rename(oldPath, newPath)
     }

src/main/ipc/filesystem.ts

@@ -184,15 +184,24 @@ export function registerFilesystemHandlers(store: Store): void {
 
   ipcMain.handle(
     'fs:deletePath',
-    async (_event, args: { targetPath: string; connectionId?: string }): Promise<void> => {
+    async (
+      _event,
+      args: { targetPath: string; connectionId?: string; recursive?: boolean }
+    ): Promise<void> => {
       if (args.connectionId) {
         const provider = getSshFilesystemProvider(args.connectionId)
         if (!provider) {
           throw new Error(`No filesystem provider for connection "${args.connectionId}"`)
         }
-        return provider.deletePath(args.targetPath)
+        return provider.deletePath(args.targetPath, args.recursive)
       }
-      const targetPath = await resolveAuthorizedPath(args.targetPath, store)
+      // Why: deleting must operate on the symlink itself, not its target.
+      // Following the link with realpath() would trash the real file — which
+      // could be another file inside the worktree, or a path outside all
+      // allowed roots that we would never be able to delete again.
+      const targetPath = await resolveAuthorizedPath(args.targetPath, store, {
+        preserveSymlink: true
+      })
 
       // Why: once auto-refresh exists, an external delete can race with a
       // UI-initiated delete. Swallowing ENOENT keeps the action idempotent

src/main/providers/types.ts

@@ -92,7 +92,7 @@ export type IFilesystemProvider = {
   readFile(filePath: string): Promise<FileReadResult>
   writeFile(filePath: string, content: string): Promise<void>
   stat(filePath: string): Promise<FileStat>
-  deletePath(targetPath: string): Promise<void>
+  deletePath(targetPath: string, recursive?: boolean): Promise<void>
   createFile(filePath: string): Promise<void>
   createDir(dirPath: string): Promise<void>
   rename(oldPath: string, newPath: string): Promise<void>

src/preload/api-types.d.ts

@@ -524,7 +524,11 @@ export type PreloadApi = {
     createFile: (args: { filePath: string; connectionId?: string }) => Promise<void>
     createDir: (args: { dirPath: string; connectionId?: string }) => Promise<void>
     rename: (args: { oldPath: string; newPath: string; connectionId?: string }) => Promise<void>
-    deletePath: (args: { targetPath: string; connectionId?: string }) => Promise<void>
+    deletePath: (args: {
+      targetPath: string
+      connectionId?: string
+      recursive?: boolean
+    }) => Promise<void>
     authorizeExternalPath: (args: { targetPath: string }) => Promise<void>
     stat: (args: {
       filePath: string

src/preload/index.ts

@@ -942,8 +942,11 @@ const api = {
       ipcRenderer.invoke('fs:createDir', args),
     rename: (args: { oldPath: string; newPath: string; connectionId?: string }): Promise<void> =>
       ipcRenderer.invoke('fs:rename', args),
-    deletePath: (args: { targetPath: string; connectionId?: string }): Promise<void> =>
-      ipcRenderer.invoke('fs:deletePath', args),
+    deletePath: (args: {
+      targetPath: string
+      connectionId?: string
+      recursive?: boolean
+    }): Promise<void> => ipcRenderer.invoke('fs:deletePath', args),
     authorizeExternalPath: (args: { targetPath: string }): Promise<void> =>
       ipcRenderer.invoke('fs:authorizeExternalPath', args),
     stat: (args: {

src/renderer/src/components/right-sidebar/useFileDeletion.ts

@@ -6,7 +6,10 @@ import { dirname } from '@/lib/path'
 import { getConnectionId } from '@/lib/connection-context'
 import { isPathEqualOrDescendant } from './file-explorer-paths'
 import type { TreeNode } from './file-explorer-types'
-import { requestEditorSaveQuiesce } from '@/components/editor/editor-autosave'
+import {
+  requestEditorFileSave,
+  requestEditorSaveQuiesce
+} from '@/components/editor/editor-autosave'
 import { commitFileExplorerOp } from './fileExplorerUndoRedo'
 
 type UseFileDeletionParams = {
@@ -14,6 +17,7 @@ type UseFileDeletionParams = {
   openFiles: {
     id: string
     filePath: string
+    isDirty?: boolean
   }[]
   closeFile: (fileId: string) => void
   refreshDir: (dirPath: string) => Promise<void>
@@ -49,16 +53,39 @@ export function useFileDeletion({
       }
       inFlightRef.current.add(node.path)
 
+      const connectionId = getConnectionId(activeWorktreeId ?? null) ?? undefined
+      const isRemote = connectionId !== undefined
+
+      // Why: remote deletes go through `rm` on the relay — there is no OS-level
+      // Trash/Recycle Bin, so the operation is permanent. Require an explicit
+      // confirmation in that case because the UI's usual undo cannot restore
+      // directories or binary files.
+      if (isRemote) {
+        const message = node.isDirectory
+          ? `Permanently delete '${node.name}' and all its contents? This cannot be undone.`
+          : `Permanently delete '${node.name}'? This cannot be undone.`
+        if (!window.confirm(message)) {
+          inFlightRef.current.delete(node.path)
+          return
+        }
+      }
+
       try {
         const filesToClose = openFiles.filter((file) =>
           isPathEqualOrDescendant(file.filePath, node.path)
         )
-        // Why: moving a file to Trash/Recycle Bin is another external mutation of
-        // the file path. Let any in-flight autosave finish first so the delete
-        // action cannot be undone by a trailing write that recreates the file.
+        // Why: force-save any dirty buffers before trashing so the undo snapshot
+        // reads the user's latest edits from disk — not an older version that
+        // predates debounced autosave or a buffer with autosave disabled.
+        // Quiesce-only would cancel pending timers and discard those edits.
+        // If a save fails, surface the error and abort the delete instead of
+        // silently trashing the stale on-disk content.
+        const dirtyFiles = filesToClose.filter((file) => file.isDirty)
+        await Promise.all(dirtyFiles.map((file) => requestEditorFileSave({ fileId: file.id })))
+        // After saving, quiesce any remaining scheduled autosaves so trailing
+        // writes cannot recreate the file after it's been trashed.
         await Promise.all(filesToClose.map((file) => requestEditorSaveQuiesce({ fileId: file.id })))
 
-        const connectionId = getConnectionId(activeWorktreeId ?? null) ?? undefined
         const parentDir = dirname(node.path)
         // Why: read file content before deleting so undo can restore it.
         // We capture content first but only commit the undo entry after the
@@ -76,7 +103,11 @@ export function useFileDeletion({
           }
         }
 
-        await window.api.fs.deletePath({ targetPath: node.path, connectionId })
+        await window.api.fs.deletePath({
+          targetPath: node.path,
+          connectionId,
+          recursive: node.isDirectory
+        })
 
         if (undoContent !== undefined) {
           commitFileExplorerOp({
@@ -89,7 +120,11 @@ export function useFileDeletion({
               await refreshDir(parentDir)
             },
             redo: async () => {
-              await window.api.fs.deletePath({ targetPath: node.path, connectionId })
+              await window.api.fs.deletePath({
+                targetPath: node.path,
+                connectionId,
+                recursive: node.isDirectory
+              })
               await refreshDir(parentDir)
             }
           })
@@ -129,10 +164,18 @@ export function useFileDeletion({
         // full-tree reloads (the watcher will also trigger a targeted refresh).
         await refreshDir(dirname(node.path))
 
-        const destination = isWindows ? 'Recycle Bin' : 'Trash'
-        toast.success(`'${node.name}' moved to ${destination}`)
+        // Why: local deletes go to the OS trash and are recoverable; remote
+        // deletes call `rm` on the relay and are permanent. The toast needs
+        // to reflect that so users aren't misled into thinking they can
+        // recover a remote file from a Trash/Recycle Bin that doesn't exist.
+        if (isRemote) {
+          toast.success(`'${node.name}' deleted`)
+        } else {
+          const destination = isWindows ? 'Recycle Bin' : 'Trash'
+          toast.success(`'${node.name}' moved to ${destination}`)
+        }
       } catch (error) {
-        const action = isWindows ? 'move to Recycle Bin' : 'move to Trash'
+        const action = isRemote ? 'delete' : isWindows ? 'move to Recycle Bin' : 'move to Trash'
         toast.error(error instanceof Error ? error.message : `Failed to ${action} '${node.name}'.`)
       } finally {
         inFlightRef.current.delete(node.path)
@@ -144,8 +187,9 @@ export function useFileDeletion({
   const requestDelete = useCallback(
     (node: TreeNode) => {
       setSelectedPath(node.path)
-      // Why: per product decision, skip the confirmation dialog — trashing is
-      // reversible (OS-level trash + in-app undo), so the extra prompt is noise.
+      // Why: local deletes skip confirmation because they're reversible
+      // (OS-level Trash + in-app undo). Remote deletes are permanent, so
+      // runDelete prompts for confirmation internally before calling `rm`.
       void runDelete(node)
     },
     [runDelete, setSelectedPath]