fix: close Claude skill migration review gaps

Keep repair-owned legacy skills in migration health checks, split migration classification and state assembly into focused helpers, document the post-mkdir safety recheck, and remove an unused link helper.

scripts/lib/install-lifecycle.js

@@ -961,7 +961,7 @@ function prepareRepairMigration(plan) {
     migration,
     plan: {
       ...plan,
-      operations: migration.appliedOperations,
+      operations: migration.finalState.operations,
       statePreview: migration.finalState,
       warnings: [
         ...(Array.isArray(plan.warnings) ? plan.warnings : []),

scripts/lib/install/apply.js

@@ -183,8 +183,9 @@ function applyInstallPlan(plan, dependencies = {}) {
   for (const operation of appliedPlan.operations) {
     assertSafeClaudeSkillOperation(appliedPlan, operation);
     fs.mkdirSync(path.dirname(operation.destinationPath), { recursive: true });
-    // The first check validates the existing chain; this second check validates
-    // every directory created by mkdirSync before any file is written.
+    // Recheck directories that were absent during the first validation. This
+    // narrows the symlink-swap window around mkdirSync, but path checks cannot
+    // eliminate a later TOCTOU race before the file write.
     assertSafeClaudeSkillOperation(appliedPlan, operation);
 
     if (operation.kind === 'merge-json') {

scripts/lib/install/claude-skill-migration.js

@@ -236,84 +236,94 @@ function createFileConflictWarning(destinationPath, retainsLegacy) {
   return `Skipped user-owned Claude skill file ${destinationPath}: the existing file is not recorded in ECC install-state.${legacySuffix}`;
 }
 
-function prepareClaudeSkillMigration(plan) {
-  const target = plan && plan.adapter && plan.adapter.target;
-  if (!CLAUDE_TARGETS.has(target)) {
-    return {
-      enabled: false,
-      appliedOperations: [...plan.operations],
-      skippedOperations: [],
-      warnings: [],
-      bridgeState: plan.statePreview,
-      finalState: plan.statePreview,
-      legacyOperationsToRemove: [],
-      requiresBridgeState: false,
-    };
-  }
+function createDisabledMigration(plan) {
+  return {
+    enabled: false,
+    appliedOperations: [...plan.operations],
+    skippedOperations: [],
+    warnings: [],
+    bridgeState: plan.statePreview,
+    finalState: plan.statePreview,
+    legacyOperationsToRemove: [],
+    requiresBridgeState: false,
+  };
+}
 
-  const previousState = pathExists(plan.installStatePath)
-    ? readInstallState(plan.installStatePath)
-    : null;
-  const currentGroups = groupCurrentSkillOperations(plan);
-  const previous = classifyPreviousOperations(plan, previousState);
-  const skippedOperations = [];
-  const skippedDestinations = new Set();
-  const warnings = [];
+function collectRetainedLegacyOperations(currentGroups, previous) {
   const currentSourceKeys = new Set(
     [...currentGroups.values()]
       .flat()
       .map(({ descriptor }) => descriptor.sourceKey)
   );
-  const retainedLegacyOperations = new Set(
+  return (
     [...previous.legacyBySource.entries()]
       .filter(([sourceKey]) => !currentSourceKeys.has(sourceKey))
       .map(([_sourceKey, operation]) => operation)
   );
+}
 
-  for (const [flatSkillRoot, entries] of currentGroups) {
-    const hasManagedFlatFile = entries.some(({ operation }) => (
-      previous.flatByDestination.has(comparablePath(operation.destinationPath))
-    ));
-    const legacyEntries = previous.legacyBySkillRoot.get(
-      entries[0].descriptor.legacySkillRoot
-    ) || [];
+function classifySkillGroup(flatSkillRoot, entries, previous) {
+  const hasManagedFlatFile = entries.some(({ operation }) => (
+    previous.flatByDestination.has(comparablePath(operation.destinationPath))
+  ));
+  const legacyEntries = previous.legacyBySkillRoot.get(
+    entries[0].descriptor.legacySkillRoot
+  ) || [];
 
-    if (pathExists(flatSkillRoot) && !hasManagedFlatFile) {
-      for (const { operation } of entries) {
-        skippedOperations.push(operation);
-        skippedDestinations.add(comparablePath(operation.destinationPath));
-      }
-      for (const { operation } of legacyEntries) {
-        retainedLegacyOperations.add(operation);
-      }
-      warnings.push(createConflictWarning(
+  if (pathExists(flatSkillRoot) && !hasManagedFlatFile) {
+    return {
+      skippedOperations: entries.map(({ operation }) => operation),
+      warnings: [createConflictWarning(
         entries[0].descriptor.skillName,
         flatSkillRoot,
         legacyEntries.length > 0
-      ));
-      continue;
-    }
-
-    for (const { operation, descriptor } of entries) {
-      const destinationKey = comparablePath(operation.destinationPath);
-      const isPreviouslyManaged = previous.flatByDestination.has(destinationKey);
-      if (!pathExists(operation.destinationPath) || isPreviouslyManaged) {
-        continue;
-      }
-
-      skippedOperations.push(operation);
-      skippedDestinations.add(destinationKey);
-      const legacyOperation = previous.legacyBySource.get(descriptor.sourceKey);
-      if (legacyOperation) {
-        retainedLegacyOperations.add(legacyOperation);
-      }
-      warnings.push(createFileConflictWarning(
-        operation.destinationPath,
-        Boolean(legacyOperation)
-      ));
-    }
+      )],
+      retainedLegacyOperations: legacyEntries.map(({ operation }) => operation),
+    };
   }
 
+  const conflicts = entries.filter(({ operation }) => (
+    pathExists(operation.destinationPath)
+    && !previous.flatByDestination.has(comparablePath(operation.destinationPath))
+  ));
+  return {
+    skippedOperations: conflicts.map(({ operation }) => operation),
+    warnings: conflicts.map(({ operation, descriptor }) => createFileConflictWarning(
+      operation.destinationPath,
+      previous.legacyBySource.has(descriptor.sourceKey)
+    )),
+    retainedLegacyOperations: conflicts
+      .map(({ descriptor }) => previous.legacyBySource.get(descriptor.sourceKey))
+      .filter(Boolean),
+  };
+}
+
+function classifySkillConflicts(currentGroups, previous) {
+  const groupClassifications = [...currentGroups.entries()]
+    .map(([flatSkillRoot, entries]) => classifySkillGroup(
+      flatSkillRoot,
+      entries,
+      previous
+    ));
+  const skippedOperations = groupClassifications
+    .flatMap(classification => classification.skippedOperations);
+  return {
+    skippedOperations,
+    skippedDestinations: new Set(
+      skippedOperations.map(operation => comparablePath(operation.destinationPath))
+    ),
+    warnings: groupClassifications.flatMap(classification => classification.warnings),
+    retainedLegacyOperations: new Set([
+      ...collectRetainedLegacyOperations(currentGroups, previous),
+      ...groupClassifications.flatMap(
+        classification => classification.retainedLegacyOperations
+      ),
+    ]),
+  };
+}
+
+function buildMigrationStates(plan, previousState, previous, classification) {
+  const { skippedDestinations, retainedLegacyOperations } = classification;
   const appliedOperations = plan.operations.filter(operation => (
     !skippedDestinations.has(comparablePath(operation.destinationPath))
   ));
@@ -333,10 +343,7 @@ function prepareClaudeSkillMigration(plan) {
   ];
 
   return {
-    enabled: true,
     appliedOperations,
-    skippedOperations,
-    warnings,
     bridgeState: buildState(plan.statePreview, bridgeOperations),
     finalState: buildState(plan.statePreview, finalOperations),
     legacyOperationsToRemove,
@@ -344,6 +351,37 @@ function prepareClaudeSkillMigration(plan) {
   };
 }
 
+function prepareClaudeSkillMigration(plan) {
+  const target = plan && plan.adapter && plan.adapter.target;
+  if (!CLAUDE_TARGETS.has(target)) {
+    return createDisabledMigration(plan);
+  }
+
+  const previousState = pathExists(plan.installStatePath)
+    ? readInstallState(plan.installStatePath)
+    : null;
+  const currentGroups = groupCurrentSkillOperations(plan);
+  const previous = classifyPreviousOperations(plan, previousState);
+  const classification = classifySkillConflicts(currentGroups, previous);
+  const states = buildMigrationStates(
+    plan,
+    previousState,
+    previous,
+    classification
+  );
+
+  return {
+    enabled: true,
+    appliedOperations: states.appliedOperations,
+    skippedOperations: classification.skippedOperations,
+    warnings: classification.warnings,
+    bridgeState: states.bridgeState,
+    finalState: states.finalState,
+    legacyOperationsToRemove: states.legacyOperationsToRemove,
+    requiresBridgeState: states.requiresBridgeState,
+  };
+}
+
 function cleanupEmptyLegacyParents(filePath, targetRoot) {
   const skillsRoot = path.join(targetRoot, 'skills');
   let currentPath = path.dirname(filePath);

scripts/lib/install/link-rewrite.js

@@ -94,15 +94,6 @@ function resolveInstalledTarget(target, sourceDir, index) {
   return null;
 }
 
-// True when the plan installs `sourceRel` at a different relative path than the
-// source (i.e. a namespace segment was injected, e.g. rules/x -> rules/ecc/x).
-// Callers use this to keep non-namespaced files on the byte-for-byte copy path.
-function isNamespacedSource(sourceRel, index) {
-  const normalizedSource = toPosix(sourceRel);
-  const installedSource = index && index.byFile.get(normalizedSource);
-  return Boolean(installedSource) && installedSource !== normalizedSource;
-}
-
 // Rewrite relative links in a markdown file so they resolve to installed target
 // locations. The source file may itself install at the same relative path; links
 // can still need changes when their targets move, such as rules -> rules/ecc.
@@ -172,6 +163,5 @@ function rewriteRelativeLinks(content, options) {
 
 module.exports = {
   buildInstallIndex,
-  isNamespacedSource,
   rewriteRelativeLinks,
 };

tests/lib/install-claude-skill-migration.test.js

@@ -734,6 +734,46 @@ function runTests() {
     }
   })) passed++; else failed++;
 
+  if (test('rechecks skill directories created between validation and copy', () => {
+    if (process.platform === 'win32') {
+      console.log('    ↷ skipped on Windows: symlink privileges vary');
+      return;
+    }
+
+    const fixture = createFixture({
+      skillFiles: {
+        'SKILL.md': '# Current ECC skill\n',
+      },
+    });
+    const destinationDirectory = path.dirname(fixture.operations[0].destinationPath);
+    const outsideRoot = path.join(fixture.tempDir, 'outside');
+    const originalMkdirSync = fs.mkdirSync;
+
+    try {
+      originalMkdirSync(outsideRoot, { recursive: true });
+      let injectedSymlink = false;
+      fs.mkdirSync = function mkdirAndReplaceWithSymlink(directoryPath, options) {
+        const result = originalMkdirSync(directoryPath, options);
+        if (!injectedSymlink && path.resolve(directoryPath) === path.resolve(destinationDirectory)) {
+          fs.rmSync(destinationDirectory, { recursive: true, force: true });
+          fs.symlinkSync(outsideRoot, destinationDirectory, 'dir');
+          injectedSymlink = true;
+        }
+        return result;
+      };
+
+      assert.throws(
+        () => applyInstallPlan(fixture.plan, { writeInstallState() {} }),
+        /symlinked Claude skill path/
+      );
+      assert.strictEqual(injectedSymlink, true);
+      assert.deepStrictEqual(fs.readdirSync(outsideRoot), []);
+    } finally {
+      fs.mkdirSync = originalMkdirSync;
+      cleanup(fixture.tempDir);
+    }
+  })) passed++; else failed++;
+
   if (test('rejects a dangling destination symlink before copying a Claude skill file', () => {
     if (process.platform === 'win32') {
       console.log('    ↷ skipped on Windows: symlink privileges vary');

tests/lib/install-lifecycle.test.js

@@ -708,12 +708,15 @@ function runTests() {
         projectRoot,
         targets: ['claude'],
       });
-      assert.notStrictEqual(repaired.results[0].status, 'error');
+      assert.strictEqual(repaired.results[0].status, 'repaired');
       assert.ok(repaired.results[0].warnings.some(warning => warning.includes('user-owned')));
       assert.strictEqual(fs.readFileSync(flatSkillPath, 'utf8'), '# User-owned flat skill\n');
       assert.strictEqual(
         fs.readFileSync(legacySkillPath, 'utf8'),
-        '# Previously managed nested skill\n'
+        fs.readFileSync(
+          path.join(REPO_ROOT, 'skills', 'tdd-workflow', 'SKILL.md'),
+          'utf8'
+        )
       );
       const repairedState = JSON.parse(fs.readFileSync(installStatePath, 'utf8'));
       assert.ok(repairedState.operations.some(operation => (

tests/lib/install-link-rewrite.test.js

@@ -10,7 +10,6 @@ const path = require('path');
 
 const {
   buildInstallIndex,
-  isNamespacedSource,
   rewriteRelativeLinks,
 } = require('../../scripts/lib/install/link-rewrite');
 const { createManifestInstallPlan } = require('../../scripts/lib/install-executor');
@@ -148,28 +147,6 @@ function runTests() {
     assert.strictEqual(after, before);
   })) passed++; else failed++;
 
-  // Guards the low-level namespace detector for callers that need to distinguish
-  // identity copies from prefix-injected placements.
-  if (test('isNamespacedSource flags only files whose own install path changed', () => {
-    assert.strictEqual(
-      isNamespacedSource('rules/react/hooks.md', index), true,
-      'a namespaced rule file must be flagged'
-    );
-    assert.strictEqual(
-      isNamespacedSource('skills/react-patterns/SKILL.md', index), false,
-      'a flat-installed skill file must not be flagged'
-    );
-    const identity = buildInstallIndex(identityMappings());
-    assert.strictEqual(
-      isNamespacedSource('skills/react-patterns/SKILL.md', identity), false,
-      'an identity-mapped file must stay on the byte-copy path'
-    );
-    assert.strictEqual(
-      isNamespacedSource('skills/not-in-plan/SKILL.md', index), false,
-      'a file the plan does not install is not namespaced'
-    );
-  })) passed++; else failed++;
-
   // Integration: real repo content + real claude plan. Every rewritten link in
   // the three React skills must resolve to a destination the SAME plan installs.
   if (test('real React skills: rewritten rules links resolve to installed targets', () => {