From 7535acb02fbaed368a1cbf6d33435786163eee36 Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Sun, 26 Jul 2026 05:04:28 -0400 Subject: [PATCH] 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 | 2 +- scripts/lib/install/apply.js | 5 +- scripts/lib/install/claude-skill-migration.js | 166 +++++++++++------- scripts/lib/install/link-rewrite.js | 10 -- .../install-claude-skill-migration.test.js | 40 +++++ tests/lib/install-lifecycle.test.js | 7 +- tests/lib/install-link-rewrite.test.js | 23 --- 7 files changed, 151 insertions(+), 102 deletions(-) diff --git a/scripts/lib/install-lifecycle.js b/scripts/lib/install-lifecycle.js index 64f112a38..23ecdf4dd 100644 --- a/scripts/lib/install-lifecycle.js +++ b/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 : []), diff --git a/scripts/lib/install/apply.js b/scripts/lib/install/apply.js index 25521b626..cf1afb186 100644 --- a/scripts/lib/install/apply.js +++ b/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') { diff --git a/scripts/lib/install/claude-skill-migration.js b/scripts/lib/install/claude-skill-migration.js index 0282385c7..ba22978be 100644 --- a/scripts/lib/install/claude-skill-migration.js +++ b/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); diff --git a/scripts/lib/install/link-rewrite.js b/scripts/lib/install/link-rewrite.js index 8732f7a39..2a06fcc10 100644 --- a/scripts/lib/install/link-rewrite.js +++ b/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, }; diff --git a/tests/lib/install-claude-skill-migration.test.js b/tests/lib/install-claude-skill-migration.test.js index 9d45db8f2..9fd3defe5 100644 --- a/tests/lib/install-claude-skill-migration.test.js +++ b/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'); diff --git a/tests/lib/install-lifecycle.test.js b/tests/lib/install-lifecycle.test.js index bcdc9a359..555b6af7b 100644 --- a/tests/lib/install-lifecycle.test.js +++ b/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 => ( diff --git a/tests/lib/install-link-rewrite.test.js b/tests/lib/install-link-rewrite.test.js index d53248996..943de7ceb 100644 --- a/tests/lib/install-link-rewrite.test.js +++ b/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', () => {