fix(archive): copy without staging when Windows EPERM blocks rename

.changeset/archive-eperm-copy-without-staging.md

@@ -0,0 +1,11 @@
+---
+"@fission-ai/openspec": patch
+---
+
+### Bug Fixes
+
+- **Archive** — When Windows `EPERM` blocks renaming a change directory that still has children, copy from the original source instead of requiring a staging rename that fails the same way. That lets archive finish instead of rolling back the spec write and leaving an empty capability directory git cannot see. A staging failure that is not `EPERM`/`EXDEV` still leaves the source untouched.
+
+  The source of that unstaged copy is still the live change directory, which the archive claim does not cover, so cleanup removes only the entries it copied and verified rather than whatever is present when it runs. A file written in that window is left alone and the complete destination is retained for recovery, instead of being deleted without ever reaching the archive.
+
+  Rollback of a newly created spec now also prunes the capability directory it created — and only that one. An empty capability directory that was already there is left in place with its own permissions.

src/core/archive.ts

@@ -21,6 +21,7 @@ import {
   writeUpdatedSpec,
   retireSpec,
   finalizeRetiredSpec,
+  pruneEmptyDirs,
   type SpecUpdate,
 } from './specs-apply.js';
 import { discoverSpecFiles, findUnreadDeltaFiles, hasAnyFileUnder } from '../utils/spec-discovery.js';
@@ -470,14 +471,161 @@ async function assertCopiedDirectoryUnchanged(
 
 /**
  * Move a directory from src to dest. On Windows, fs.rename() can fail with
- * EPERM, and cross-device moves fail with EXDEV. When the source can first be
- * renamed to a private sibling, fall back to a verified copy-then-remove. A
- * source that cannot be staged is left untouched rather than copied and deleted
- * through a path another process may still be editing.
+ * EPERM, and cross-device moves fail with EXDEV. Prefer renaming the source
+ * to a private sibling first, then copy-then-remove. When that staging rename
+ * also fails with EPERM/EXDEV — the usual Windows case for a directory that
+ * still has children, because a watcher holds a directory-enumeration handle —
+ * copy from the original source instead. Fingerprints still abort if the tree
+ * changes mid-copy. A staging failure that is not EPERM/EXDEV still leaves
+ * the source untouched rather than copying through a path we could not claim.
  */
 class MoveDestinationRetainedError extends Error {}
 class RetirementBackupsRetainedError extends Error {}
 
+function isFallbackRenameCode(code: string | undefined): boolean {
+  return code === 'EPERM' || code === 'EXDEV';
+}
+
+/**
+ * Every entry under `root`, deepest first, as paths relative to it.
+ *
+ * The listing is what bounds the removal below. Anything that appears after it
+ * is simply not in the set, so it cannot be deleted by the cleanup.
+ */
+async function listTreeEntriesDeepestFirst(
+  root: string
+): Promise<{ relative: string; isDirectory: boolean }[]> {
+  const entries: { relative: string; isDirectory: boolean }[] = [];
+  const visit = async (dir: string, relativeDir: string): Promise<void> => {
+    for (const entry of await fs.readdir(dir, { withFileTypes: true })) {
+      const relative = relativeDir === '' ? entry.name : path.join(relativeDir, entry.name);
+      if (entry.isDirectory()) {
+        await visit(path.join(dir, entry.name), relative);
+        entries.push({ relative, isDirectory: true });
+      } else {
+        entries.push({ relative, isDirectory: false });
+      }
+    }
+  };
+  await visit(root, '');
+  return entries;
+}
+
+/**
+ * Remove exactly the entries that were copied and verified, deepest first.
+ *
+ * The move is only safe to finish by deleting the source, and the source of the
+ * unstaged fallback is still the live change directory: the archive claim
+ * covers the destination, not it. A recursive remove would delete whatever is
+ * there at that moment, including a file a concurrent writer added after the
+ * final fingerprint - data that never reached the destination.
+ *
+ * Removing a named set instead means a late arrival is never in it. It is left
+ * on disk, and the `rmdir` of its parent fails with ENOTEMPTY, which the caller
+ * reports as a retained destination. The move does not complete silently.
+ *
+ * `entries` must be listed before the final fingerprint, so that an arrival is
+ * either caught by that fingerprint or absent from the set. What this cannot
+ * cover is an edit to a file that is already in the set: the copy holds the
+ * content as of the fingerprint, and the newer bytes go with the source. That
+ * window is the one the staging rename closes, and is why staging is still
+ * preferred whenever the rename is permitted at all.
+ */
+async function removeVerifiedTree(
+  root: string,
+  entries: { relative: string; isDirectory: boolean }[]
+): Promise<void> {
+  for (const entry of entries) {
+    const target = path.join(root, entry.relative);
+    if (entry.isDirectory) {
+      await fs.rmdir(target);
+    } else {
+      await fs.rm(target, { force: true });
+    }
+  }
+  await fs.rmdir(root);
+}
+
+async function copyThenRemoveDirectory(
+  source: string,
+  dest: string,
+  options: {
+    verifyCopiedDestination?: (copiedSource: string) => Promise<void>;
+  },
+  restoreSource?: () => Promise<void>
+): Promise<void> {
+  let destIsOurs = false;
+  let sourceFingerprint: string;
+  try {
+    sourceFingerprint = await fingerprintDirectoryContents(source);
+    await fs.mkdir(dest, { mode: 0o700 });
+    destIsOurs = true;
+    await copyDirContents(source, dest);
+    await options.verifyCopiedDestination?.(source);
+    await assertCopiedDirectoryUnchanged(source, dest, sourceFingerprint);
+  } catch (copyError) {
+    if (destIsOurs) {
+      await fs.rm(dest, { recursive: true, force: true }).catch(() => undefined);
+    }
+    if (restoreSource) {
+      try {
+        await restoreSource();
+      } catch (restoreError) {
+        throw new Error(
+          `${copyError instanceof Error ? copyError.message : String(copyError)} ` +
+            `Could not restore the staged source at ${source} ` +
+            `(${restoreError instanceof Error ? restoreError.message : String(restoreError)}).`
+        );
+      }
+    }
+    if ((copyError as NodeJS.ErrnoException).code === 'EEXIST') {
+      throw new ArchiveBlockedError(
+        'archive_target_exists',
+        `Archive '${path.basename(dest)}' already exists.`
+      );
+    }
+    throw copyError;
+  }
+  let verifiedEntries: { relative: string; isDirectory: boolean }[];
+  try {
+    // Listed before the verification, not after it. A file that arrives before
+    // the fingerprint changes it and aborts the move; one that arrives after is
+    // not in this set. Listing afterwards would leave a window in which an
+    // arrival is both unverified and deletable.
+    verifiedEntries = await listTreeEntriesDeepestFirst(source);
+    await options.verifyCopiedDestination?.(source);
+    await assertCopiedDirectoryUnchanged(source, dest, sourceFingerprint);
+  } catch (verificationError) {
+    await fs.rm(dest, { recursive: true, force: true }).catch(() => undefined);
+    if (restoreSource) {
+      try {
+        await restoreSource();
+      } catch (restoreError) {
+        throw new Error(
+          `${verificationError instanceof Error ? verificationError.message : String(verificationError)} ` +
+            `Could not restore the staged source at ${source} ` +
+            `(${restoreError instanceof Error ? restoreError.message : String(restoreError)}).`
+        );
+      }
+    }
+    throw verificationError;
+  }
+  try {
+    await removeVerifiedTree(source, verifiedEntries);
+  } catch (cleanupError) {
+    // Removal may already have deleted part of the source, or stopped on an
+    // entry that appeared after verification. The destination is now the only
+    // complete copy, so never erase it while trying to make this failed move
+    // look atomic.
+    throw new MoveDestinationRetainedError(
+      `Copied ${source} to ${dest}, but could not remove the source at ` +
+        `${source} completely ` +
+        `(${cleanupError instanceof Error ? cleanupError.message : String(cleanupError)}). ` +
+        'The complete destination was retained for recovery.'
+    );
+  }
+}
+
 async function moveDirectory(
   src: string,
   dest: string,
@@ -497,76 +645,25 @@ async function moveDirectory(
         `Archive '${path.basename(dest)}' already exists.`
       );
     }
-    if (code === 'EPERM' || code === 'EXDEV') {
+    if (isFallbackRenameCode(code)) {
       const stagedSource = path.join(path.dirname(src), `.openspec-move-${randomUUID()}`);
       try {
         await fs.rename(src, stagedSource);
       } catch (stageError) {
+        const stageCode = (stageError as NodeJS.ErrnoException)?.code;
+        if (isFallbackRenameCode(stageCode)) {
+          await copyThenRemoveDirectory(src, dest, options);
+          return;
+        }
         throw new Error(
           `Could not safely stage ${src} before the fallback archive copy ` +
             `(${stageError instanceof Error ? stageError.message : String(stageError)}). ` +
             'No fallback copy was attempted.'
         );
       }
-      let destIsOurs = false;
-      let stagedFingerprint: string;
-      try {
-        stagedFingerprint = await fingerprintDirectoryContents(stagedSource);
-        await fs.mkdir(dest, { mode: 0o700 });
-        destIsOurs = true;
-        await copyDirContents(stagedSource, dest);
-        await options.verifyCopiedDestination?.(stagedSource);
-        await assertCopiedDirectoryUnchanged(stagedSource, dest, stagedFingerprint);
-      } catch (copyError) {
-        if (destIsOurs) {
-          await fs.rm(dest, { recursive: true, force: true }).catch(() => undefined);
-        }
-        try {
-          await fs.rename(stagedSource, src);
-        } catch (restoreError) {
-          throw new Error(
-            `${copyError instanceof Error ? copyError.message : String(copyError)} ` +
-              `Could not restore the staged source at ${stagedSource} ` +
-              `(${restoreError instanceof Error ? restoreError.message : String(restoreError)}).`
-          );
-        }
-        if ((copyError as NodeJS.ErrnoException).code === 'EEXIST') {
-          throw new ArchiveBlockedError(
-            'archive_target_exists',
-            `Archive '${path.basename(dest)}' already exists.`
-          );
-        }
-        throw copyError;
-      }
-      try {
-        await options.verifyCopiedDestination?.(stagedSource);
-        await assertCopiedDirectoryUnchanged(stagedSource, dest, stagedFingerprint);
-      } catch (verificationError) {
-        await fs.rm(dest, { recursive: true, force: true }).catch(() => undefined);
-        try {
-          await fs.rename(stagedSource, src);
-        } catch (restoreError) {
-          throw new Error(
-            `${verificationError instanceof Error ? verificationError.message : String(verificationError)} ` +
-              `Could not restore the staged source at ${stagedSource} ` +
-              `(${restoreError instanceof Error ? restoreError.message : String(restoreError)}).`
-          );
-        }
-        throw verificationError;
-      }
-      try {
-        await fs.rm(stagedSource, { recursive: true, force: true });
-      } catch (cleanupError) {
-        // Recursive removal may already have deleted part of the source. The
-        // destination is now the only complete copy, so never erase it while
-        // trying to make this failed move look atomic.
-        throw new MoveDestinationRetainedError(
-          `Copied ${src} to ${dest}, but could not remove the staged source at ` +
-            `${stagedSource} completely ` +
-            `(${cleanupError instanceof Error ? cleanupError.message : String(cleanupError)}). ` +
-            'The complete destination was retained for recovery.'
-        );
-      }
+      await copyThenRemoveDirectory(stagedSource, dest, options, async () => {
+        await fs.rename(stagedSource, src);
+      });
     } else {
       throw err;
     }
@@ -673,6 +770,14 @@ async function claimArchiveDestination(
 interface SpecSnapshot {
   target: string;
   existed: boolean;
+  /**
+   * The deepest directory at or above the target's parent that already existed
+   * before the mutation. Rollback prunes up to but never past it, so a
+   * capability directory the user already had keeps its permissions and ACLs -
+   * including an intermediate one under a nested capability id, where only the
+   * leaf was created by this write.
+   */
+  pruneBoundary?: string;
   outcome: 'write' | 'retire';
   expectedContent?: Buffer;
   content?: Buffer;
@@ -862,7 +967,31 @@ async function assertDistinctMutationTargets(mutations: SpecMutation[]): Promise
   }
 }
 
-async function captureSpecSnapshots(mutations: SpecMutation[]): Promise<SpecSnapshot[]> {
+/**
+ * The deepest directory at or above `dir` that exists, never going above
+ * `boundaryDir`. Used as the floor for a rollback prune: everything below it
+ * was created by the write being undone, and it was not.
+ */
+async function deepestExistingAncestor(dir: string, boundaryDir: string): Promise<string> {
+  let current = dir;
+  for (;;) {
+    if (current === boundaryDir || !current.startsWith(boundaryDir + path.sep)) {
+      return boundaryDir;
+    }
+    try {
+      await fs.lstat(current);
+      return current;
+    } catch (error) {
+      if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error;
+    }
+    current = path.dirname(current);
+  }
+}
+
+async function captureSpecSnapshots(
+  mutations: SpecMutation[],
+  mainSpecsDir: string
+): Promise<SpecSnapshot[]> {
   return Promise.all(
     mutations.map(async ({ update, outcome, rebuilt }) => {
       try {
@@ -908,6 +1037,10 @@ async function captureSpecSnapshots(mutations: SpecMutation[]): Promise<SpecSnap
             target: update.target,
             existed: false,
             outcome,
+            pruneBoundary: await deepestExistingAncestor(
+              path.dirname(update.target),
+              mainSpecsDir
+            ),
             ...(outcome === 'write' ? { expectedContent: Buffer.from(rebuilt) } : {}),
           };
         }
@@ -917,7 +1050,10 @@ async function captureSpecSnapshots(mutations: SpecMutation[]): Promise<SpecSnap
   );
 }
 
-async function restoreSpecSnapshots(snapshots: SpecSnapshot[]): Promise<void> {
+async function restoreSpecSnapshots(
+  snapshots: SpecSnapshot[],
+  mainSpecsDir: string
+): Promise<void> {
   const errors: Error[] = [];
   for (const snapshot of [...snapshots].reverse()) {
     try {
@@ -1008,6 +1144,13 @@ async function restoreSpecSnapshots(snapshots: SpecSnapshot[]): Promise<void> {
 
       if (!snapshot.existed) {
         await fs.rm(snapshot.target, { force: true });
+        // Only a capability directory this write created is ours to take back.
+        // One the user already had stays, empty or not, with its own mode -
+        // pruneEmptyDirs never removes its boundary.
+        await pruneEmptyDirs(
+          path.dirname(snapshot.target),
+          snapshot.pruneBoundary ?? mainSpecsDir
+        );
         continue;
       }
       if (snapshot.symlink !== undefined) {
@@ -1756,7 +1899,7 @@ export class ArchiveCommand {
               );
             }
           }
-          const specSnapshots = await captureSpecSnapshots(mutations);
+          const specSnapshots = await captureSpecSnapshots(mutations, mainSpecsDir);
           const specSnapshotsByTarget = new Map(
             specSnapshots.map((snapshot) => [snapshot.target, snapshot])
           );
@@ -2028,7 +2171,8 @@ export class ArchiveCommand {
             const rollbackErrors: Error[] = [];
             try {
               await restoreSpecSnapshots(
-                specSnapshots.filter(({ target }) => mutationAttempts.has(target))
+                specSnapshots.filter(({ target }) => mutationAttempts.has(target)),
+                mainSpecsDir
               );
             } catch (rollbackError) {
               rollbackErrors.push(

src/core/specs-apply.ts

@@ -1190,7 +1190,7 @@ async function isInsideRealDir(realPath: string, dir: string): Promise<boolean>
  * needs fd-relative syscalls Node does not expose, and it requires local write
  * access to `openspec/specs` during an archive.
  */
-async function pruneEmptyDirs(startDir: string, boundaryDir: string): Promise<void> {
+export async function pruneEmptyDirs(startDir: string, boundaryDir: string): Promise<void> {
   let boundary: string;
   try {
     boundary = await fs.realpath(boundaryDir);