fix(isolation): preserve copy boundary parity

Author: shixi-li

src/copy-containment.js

@@ -66,21 +66,14 @@ function pinRoot(rootPath, label, expectedRoot) {
 }
 
 function assertPinnedRoot(root, label, relativePath) {
-  let canonicalPath;
   let identity;
   try {
-    canonicalPath = fs.realpathSync.native(root.canonicalPath);
-    identity = statIdentity(canonicalPath);
+    identity = statIdentity(root.canonicalPath);
   } catch (err) {
     throw containmentError(relativePath, `${label} root can no longer be resolved`, err);
   }
 
-  if (
-    canonicalPath !== root.canonicalPath ||
-    identity.device !== root.device ||
-    identity.inode !== root.inode ||
-    !identity.directory
-  ) {
+  if (identity.device !== root.device || identity.inode !== root.inode || !identity.directory) {
     throw containmentError(relativePath, `${label} root changed after it was pinned`);
   }
 }
@@ -92,15 +85,11 @@ function validateRelativePath(relativePath) {
   if (relativePath.includes('\0')) {
     throw containmentError(relativePath, 'path contains a null byte');
   }
-  if (
-    path.posix.isAbsolute(relativePath) ||
-    path.win32.isAbsolute(relativePath) ||
-    path.win32.parse(relativePath).root
-  ) {
+  if (path.isAbsolute(relativePath) || path.parse(relativePath).root) {
     throw containmentError(relativePath, 'absolute paths are not allowed');
   }
 
-  const components = relativePath.split(/[\\/]/);
+  const components = relativePath.split(path.sep);
   if (components.some((component) => component === '' || component === '.' || component === '..')) {
     throw containmentError(
       relativePath,
@@ -147,21 +136,17 @@ function resolveDestinationPath(boundary, relativePath) {
     throw containmentError(relativePath, 'destination path escapes its pinned root');
   }
 
-  let existingPath = candidatePath;
-  while (true) {
-    try {
-      fs.lstatSync(existingPath);
-      break;
-    } catch (err) {
-      if (err.code !== 'ENOENT') {
-        throw err;
-      }
-      const parentPath = path.dirname(existingPath);
-      if (parentPath === existingPath) {
-        throw containmentError(relativePath, 'destination has no resolvable ancestor', err);
-      }
-      existingPath = parentPath;
+  let existingPath;
+  try {
+    fs.lstatSync(candidatePath);
+    existingPath = candidatePath;
+  } catch (err) {
+    if (err.code !== 'ENOENT') {
+      throw err;
     }
+    // The copy pipeline creates directories parent-first in phase two, so the
+    // immediate parent must exist before any mkdir/copy effect is attempted.
+    existingPath = path.dirname(candidatePath);
   }
 
   let canonicalExistingPath;
@@ -203,9 +188,23 @@ function isCopyContainmentError(error) {
   return error?.code === CONTAINMENT_ERROR_CODE;
 }
 
+function copyErrorFromPayload(payload) {
+  const error =
+    payload.code === CONTAINMENT_ERROR_CODE
+      ? new CopyContainmentError(payload.relativePath, 'worker rejected an unsafe path')
+      : new Error(payload.message);
+  error.name = payload.name || error.name;
+  error.message = payload.message;
+  if (payload.code) {
+    error.code = payload.code;
+  }
+  return error;
+}
+
 module.exports = {
   CONTAINMENT_ERROR_CODE,
   CopyContainmentError,
+  copyErrorFromPayload,
   createCopyBoundary,
   isCopyContainmentError,
   resolveCopyPath,

src/copy-worker.js

@@ -7,7 +7,6 @@
 
 const { parentPort, workerData } = require('worker_threads');
 const fs = require('fs');
-const path = require('path');
 const {
   createCopyBoundary,
   isCopyContainmentError,
@@ -23,16 +22,8 @@ let error = null;
 
 for (const relativePath of files) {
   try {
-    // Ensure parent directory exists
-    const parentRelativePath = path.dirname(relativePath);
-    if (parentRelativePath !== '.') {
-      const { destinationPath: destDir } = resolveCopyPath(copyBoundary, parentRelativePath);
-      if (!fs.existsSync(destDir)) {
-        fs.mkdirSync(destDir, { recursive: true });
-      }
-    }
-
-    // Copy the file
+    // Phase two creates every parent directory. Re-resolve the source and
+    // destination immediately before the only worker filesystem effect.
     const { sourcePath, destinationPath } = resolveCopyPath(copyBoundary, relativePath);
     fs.copyFileSync(sourcePath, destinationPath);
     copied++;
@@ -50,6 +41,7 @@ for (const relativePath of files) {
       name: err.name,
       code: err.code,
       message: err.message,
+      relativePath: err.relativePath || relativePath,
     };
     break;
   }

src/isolation-manager.js

@@ -35,9 +35,11 @@ const { readRepoSettings } = require('../lib/repo-settings');
 const { provisionClaudeCredentials } = require('./claude-credentials');
 const {
   CopyContainmentError,
+  copyErrorFromPayload,
   createCopyBoundary,
   resolveCopyPath,
   resolveSourcePath,
+  validateRelativePath,
 } = require('./copy-containment');
 
 const DEFAULT_WORKTREE_SETUP_TIMEOUT_MS = 15 * 60 * 1000;
@@ -1323,12 +1325,9 @@ class IsolationManager {
 
     function handleEntry(entry, srcPath, relPath, relativePath, ancestorDirectories) {
       if (entry.isSymbolicLink()) {
-        const targetStats = fs.statSync(srcPath);
+        const resolvedSourcePath = resolveSourcePath(copyBoundary, relPath);
+        const targetStats = fs.statSync(resolvedSourcePath);
         if (targetStats.isDirectory()) {
-          const resolvedSourcePath = resolveSourcePath(copyBoundary, relPath);
-          if (ancestorDirectories.has(resolvedSourcePath)) {
-            throw new CopyContainmentError(relPath, 'source directory symlink creates a cycle');
-          }
           directories.add(relPath);
           collectFiles(relPath, ancestorDirectories);
           return;
@@ -1365,8 +1364,9 @@ class IsolationManager {
           continue;
         }
 
-        const srcPath = path.join(currentSrc, entry.name);
-        const relPath = relativePath ? path.join(relativePath, entry.name) : entry.name;
+        const entryName = validateRelativePath(entry.name);
+        const srcPath = path.join(currentSrc, entryName);
+        const relPath = relativePath ? path.join(relativePath, entryName) : entryName;
 
         try {
           handleEntry(entry, srcPath, relPath, relativePath, childAncestors);
@@ -1428,6 +1428,7 @@ class IsolationManager {
     }
 
     // Spawn workers and wait for completion
+    const workers = [];
     const workerPromises = chunks.map((chunk) => {
       return new Promise((resolve, reject) => {
         const worker = new Worker(workerPath, {
@@ -1438,9 +1439,11 @@ class IsolationManager {
             expectedBoundary: copyBoundary,
           },
         });
+        workers.push(worker);
+        let result = null;
 
-        worker.on('message', (result) => {
-          resolve(result);
+        worker.on('message', (workerResult) => {
+          result = workerResult;
         });
 
         worker.on('error', (err) => {
@@ -1450,6 +1453,12 @@ class IsolationManager {
         worker.on('exit', (code) => {
           if (code !== 0) {
             reject(new Error(`Worker exited with code ${code}`));
+          } else if (!result) {
+            reject(new Error('Copy worker exited without reporting a result'));
+          } else if (result.error) {
+            reject(copyErrorFromPayload(result.error));
+          } else {
+            resolve(result);
           }
         });
       });
@@ -1458,15 +1467,11 @@ class IsolationManager {
     // Wait for all workers to complete (proper async/await - no busy-wait!)
     // FIX: Previous version used busy-wait which blocked the event loop,
     // preventing worker thread messages from being processed (timeout bug)
-    const workerResults = await Promise.all(workerPromises);
-    const failedResult = workerResults.find((result) => result.error);
-    if (failedResult) {
-      const error = new Error(failedResult.error.message);
-      error.name = failedResult.error.name || 'Error';
-      if (failedResult.error.code) {
-        error.code = failedResult.error.code;
-      }
-      throw error;
+    try {
+      await Promise.all(workerPromises);
+    } catch (err) {
+      await Promise.all(workers.map((worker) => worker.terminate().catch(() => undefined)));
+      throw err;
     }
   }