From f3afd59045ff41e17d0a67b22ed2350ed35faafd Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Sun, 26 Jul 2026 03:20:06 -0700 Subject: [PATCH] fix: flatten Claude skill installs (#2582) Flatten managed Claude skill destinations, preserve user-owned conflicts, and migrate legacy nested installs through the lifecycle tooling. --- README.md | 8 + docs/es/README.md | 15 +- scripts/install-apply.js | 28 +- scripts/lib/install-executor.js | 6 + scripts/lib/install-lifecycle.js | 60 +- scripts/lib/install-targets/claude-home.js | 3 +- scripts/lib/install-targets/claude-project.js | 3 +- scripts/lib/install/apply.js | 75 +- scripts/lib/install/claude-skill-migration.js | 415 +++++++++ scripts/lib/install/link-rewrite.js | 24 +- .../install-claude-skill-migration.test.js | 811 ++++++++++++++++++ tests/lib/install-executor.test.js | 4 +- tests/lib/install-lifecycle.test.js | 97 +++ tests/lib/install-link-rewrite.test.js | 50 +- tests/lib/install-targets.test.js | 12 +- tests/lib/locale-install.test.js | 2 +- tests/lib/selective-install.test.js | 12 +- tests/scripts/install-apply.test.js | 114 ++- 18 files changed, 1620 insertions(+), 119 deletions(-) create mode 100644 scripts/lib/install/claude-skill-migration.js create mode 100644 tests/lib/install-claude-skill-migration.test.js diff --git a/README.md b/README.md index 86108562f..6776bd5f9 100644 --- a/README.md +++ b/README.md @@ -375,6 +375,14 @@ npx ecc-install --profile minimal --target claude This profile intentionally excludes `hooks-runtime`. +Claude manual installs place each skill directly under +`~/.claude/skills//` (or `.claude/skills//` for +`claude-project`) so Claude Code can discover it. When upgrading an older ECC +manual install, the installer migrates only nested `skills/ecc/` files recorded +in ECC install-state. If a flat skill directory is user-owned, ECC preserves it, +prints a conflict warning, and keeps any older managed copy tracked for a safe +uninstall instead of overwriting user files. + If you want the normal core profile but need hooks off, use: ```bash diff --git a/docs/es/README.md b/docs/es/README.md index d0e105a26..a6a30dbb0 100644 --- a/docs/es/README.md +++ b/docs/es/README.md @@ -757,16 +757,13 @@ cp -r rules/golang ~/.claude/rules/ecc/ cp -r rules/php ~/.claude/rules/ecc/ cp -r rules/arkts ~/.claude/rules/ecc/ -# Copiar skills primero (superficie principal de flujo de trabajo) -# Recomendado (nuevos usuarios): solo skills generales/básicas -mkdir -p ~/.claude/skills/ecc -cp -r .agents/skills/* ~/.claude/skills/ecc/ -cp -r skills/search-first ~/.claude/skills/ecc/ +# Instalar skills con el instalador consciente de migraciones. +# Conserva skills del usuario, informa conflictos y evita sobrescribirlos. +node scripts/install-apply.js --target claude --modules workflow-quality -# Opcional: añadir skills específicas de framework solo cuando las necesites -# for s in django-patterns django-tdd laravel-patterns springboot-patterns quarkus-patterns; do -# cp -r skills/$s ~/.claude/skills/ecc/ -# done +# Opcional: instalar skills concretas solo cuando las necesites. +node scripts/install-apply.js --target claude --skills search-first +# node scripts/install-apply.js --target claude --skills django-patterns,django-tdd # Opcional: mantener compatibilidad con entradas slash durante la migración mkdir -p ~/.claude/commands diff --git a/scripts/install-apply.js b/scripts/install-apply.js index 9c427cd7b..8f1b41cca 100755 --- a/scripts/install-apply.js +++ b/scripts/install-apply.js @@ -32,8 +32,8 @@ Usage: install.sh [--target <${LEGACY_INSTALL_TARGETS.join('|')}>] [--dry-run] [ install.sh [--dry-run] [--json] --config Targets: - claude (default) - Install ECC into ~/.claude/ with managed rules/skills under rules/ecc and skills/ecc - claude-project - Install ECC into ./.claude/ (per-project) with managed rules/skills under rules/ecc and skills/ecc + claude (default) - Install ECC into ~/.claude/ with managed rules under rules/ecc and flat skills under skills/ + claude-project - Install ECC into ./.claude/ (per-project) with managed rules under rules/ecc and flat skills under skills/ cursor - Install rules, hooks, and bundled Cursor configs to ./.cursor/ antigravity - Install rules, workflows, skills, and agents to ./.agent/ codex - Install shared agents/config into ~/.codex/ @@ -102,7 +102,10 @@ function printHumanPlan(plan, dryRun) { console.log(`Excluded modules: ${plan.excludedModuleIds.join(', ')}`); } } - console.log(`Operations: ${plan.operations.length}`); + console.log(`${dryRun ? 'Operations' : 'Applied operations'}: ${plan.operations.length}`); + if (Array.isArray(plan.skippedOperations) && plan.skippedOperations.length > 0) { + console.log(`Skipped operations: ${plan.skippedOperations.length}`); + } if (plan.warnings.length > 0) { console.log('\nWarnings:'); @@ -111,11 +114,18 @@ function printHumanPlan(plan, dryRun) { } } - console.log('\nPlanned file operations:'); + console.log(`\n${dryRun ? 'Planned' : 'Applied'} file operations:`); for (const operation of plan.operations) { console.log(`- ${operation.sourceRelativePath} -> ${operation.destinationPath}`); } + if (Array.isArray(plan.skippedOperations) && plan.skippedOperations.length > 0) { + console.log('\nSkipped file operations:'); + for (const operation of plan.skippedOperations) { + console.log(`- ${operation.sourceRelativePath} -> ${operation.destinationPath}`); + } + } + if (!dryRun) { console.log(`\nDone. Install-state written to ${plan.installStatePath}`); } @@ -135,7 +145,10 @@ function main() { findDefaultInstallConfigPath, loadInstallConfig, } = require('./lib/install/config'); - const { applyInstallPlan } = require('./lib/install-executor'); + const { + applyInstallPlan, + previewInstallPlan, + } = require('./lib/install-executor'); const { createInstallPlanFromRequest } = require('./lib/install/runtime'); const defaultConfigPath = options.configPath || options.languages.length > 0 ? null @@ -147,13 +160,14 @@ function main() { ...options, config, }); - const plan = createInstallPlanFromRequest(request, { + const rawPlan = createInstallPlanFromRequest(request, { projectRoot: process.cwd(), homeDir: process.env.HOME || os.homedir(), claudeRulesDir: process.env.CLAUDE_RULES_DIR || null, }); if (options.dryRun) { + const plan = previewInstallPlan(rawPlan); if (options.json) { console.log(JSON.stringify({ dryRun: true, plan }, null, 2)); } else { @@ -162,7 +176,7 @@ function main() { return; } - const result = applyInstallPlan(plan); + const result = applyInstallPlan(rawPlan); if (options.json) { console.log(JSON.stringify({ dryRun: false, result }, null, 2)); } else { diff --git a/scripts/lib/install-executor.js b/scripts/lib/install-executor.js index 85257c698..57100cb31 100644 --- a/scripts/lib/install-executor.js +++ b/scripts/lib/install-executor.js @@ -123,6 +123,11 @@ function applyInstallPlan(plan) { return applyPlan(plan); } +function previewInstallPlan(plan) { + const { previewInstallPlan: previewPlan } = require('./install/apply'); + return previewPlan(plan); +} + function buildCopyFileOperation({ moduleId, sourcePath, sourceRelativePath, destinationPath, strategy }) { return { kind: 'copy-file', @@ -802,6 +807,7 @@ module.exports = { SUPPORTED_INSTALL_TARGETS, LEGACY_INSTALL_TARGETS, applyInstallPlan, + previewInstallPlan, createLegacyCompatInstallPlan, createManifestInstallPlan, createLegacyInstallPlan, diff --git a/scripts/lib/install-lifecycle.js b/scripts/lib/install-lifecycle.js index 792ba9935..23ecdf4dd 100644 --- a/scripts/lib/install-lifecycle.js +++ b/scripts/lib/install-lifecycle.js @@ -7,6 +7,10 @@ const { resolveInstallPlan, loadInstallManifests } = require('./install-manifest const { readInstallState, writeInstallState } = require('./install-state'); const { assertWithinTrustedRoot } = require('./path-safety'); const { createManifestInstallPlan } = require('./install-executor'); +const { + prepareClaudeSkillMigration, + removeLegacyClaudeSkillFiles, +} = require('./install/claude-skill-migration'); const { getInstallTargetAdapter, listInstallTargetAdapters } = require('./install-targets/registry'); const OPENCODE_BUILD_ARTIFACT = path.join('.opencode', 'dist'); const OPENCODE_BUILD_SCRIPT = path.join('scripts', 'build-opencode.js'); @@ -951,6 +955,22 @@ function createRepairPlanFromRecord(record, context, options = {}) { }; } +function prepareRepairMigration(plan) { + const migration = prepareClaudeSkillMigration(plan); + return { + migration, + plan: { + ...plan, + operations: migration.finalState.operations, + statePreview: migration.finalState, + warnings: [ + ...(Array.isArray(plan.warnings) ? plan.warnings : []), + ...migration.warnings, + ], + }, + }; +} + function repairInstalledStates(options = {}) { const repoRoot = options.repoRoot || DEFAULT_REPO_ROOT; const manifests = loadInstallManifests({ repoRoot }); @@ -988,9 +1008,10 @@ function repairInstalledStates(options = {}) { const opencodeBuildRepairPath = path.join(context.repoRoot, OPENCODE_BUILD_ARTIFACT); if (needsOpencodeBuild && options.dryRun) { - const desiredPlan = createRepairPlanFromRecord(record, context, { + const rawPlan = createRepairPlanFromRecord(record, context, { exemptValidationCodes: [OPENCODE_PLUGIN_NOT_BUILT_CODE], }); + const { plan: desiredPlan } = prepareRepairMigration(rawPlan); const operationHealth = summarizeManagedOperationHealth(context.repoRoot, desiredPlan.operations); const repairOperations = [...operationHealth.missing.map(entry => ({ ...entry.operation })), ...operationHealth.drifted.map(entry => ({ ...entry.operation }))]; const plannedRepairs = [opencodeBuildRepairPath, ...repairOperations.map(operation => operation.destinationPath)]; @@ -1002,6 +1023,7 @@ function repairInstalledStates(options = {}) { repairedPaths: [], plannedRepairs, stateRefreshed: false, + warnings: desiredPlan.warnings, error: null }; } @@ -1021,7 +1043,11 @@ function repairInstalledStates(options = {}) { } } - const desiredPlan = createRepairPlanFromRecord(record, context); + const rawPlan = createRepairPlanFromRecord(record, context); + const { + migration, + plan: desiredPlan, + } = prepareRepairMigration(rawPlan); const operationHealth = summarizeManagedOperationHealth(context.repoRoot, desiredPlan.operations); if (operationHealth.missingSource.length > 0) { @@ -1031,14 +1057,20 @@ function repairInstalledStates(options = {}) { installStatePath: record.installStatePath, repairedPaths: [], plannedRepairs: [], + warnings: desiredPlan.warnings, error: `Missing source file(s): ${operationHealth.missingSource.map(entry => entry.sourcePath).join(', ')}` }; } const repairOperations = [...operationHealth.missing.map(entry => ({ ...entry.operation })), ...operationHealth.drifted.map(entry => ({ ...entry.operation }))]; - const plannedRepairs = needsOpencodeBuild - ? [opencodeBuildRepairPath, ...repairOperations.map(operation => operation.destinationPath)] - : repairOperations.map(operation => operation.destinationPath); + const legacyMigrationPaths = migration.legacyOperationsToRemove.map( + operation => operation.destinationPath + ); + const plannedRepairs = [...new Set([ + ...(needsOpencodeBuild ? [opencodeBuildRepairPath] : []), + ...repairOperations.map(operation => operation.destinationPath), + ...legacyMigrationPaths, + ])]; if (options.dryRun) { return { @@ -1048,26 +1080,36 @@ function repairInstalledStates(options = {}) { repairedPaths: [], plannedRepairs, stateRefreshed: plannedRepairs.length === 0, + warnings: desiredPlan.warnings, error: null }; } + const hasLegacyMigration = migration.legacyOperationsToRemove.length > 0; + if (migration.requiresBridgeState && (repairOperations.length > 0 || hasLegacyMigration)) { + writeInstallState(desiredPlan.installStatePath, migration.bridgeState); + } + if (repairOperations.length > 0) { for (const operation of repairOperations) { executeRepairOperation(context.repoRoot, operation, record.targetRoot); } - writeInstallState(desiredPlan.installStatePath, desiredPlan.statePreview); - } else { - writeInstallState(desiredPlan.installStatePath, desiredPlan.statePreview); } + if (hasLegacyMigration) { + removeLegacyClaudeSkillFiles(migration, desiredPlan.targetRoot); + } + writeInstallState(desiredPlan.installStatePath, desiredPlan.statePreview); return { adapter: record.adapter, - status: (repairOperations.length > 0 || needsOpencodeBuild) ? 'repaired' : 'ok', + status: (repairOperations.length > 0 || needsOpencodeBuild || hasLegacyMigration) + ? 'repaired' + : 'ok', installStatePath: record.installStatePath, repairedPaths: plannedRepairs, plannedRepairs: [], stateRefreshed: true, + warnings: desiredPlan.warnings, error: null }; } catch (error) { diff --git a/scripts/lib/install-targets/claude-home.js b/scripts/lib/install-targets/claude-home.js index ed5f5f46e..3729b50c8 100644 --- a/scripts/lib/install-targets/claude-home.js +++ b/scripts/lib/install-targets/claude-home.js @@ -27,14 +27,13 @@ function getClaudeManagedDestinationPath(adapter, sourceRelativePath, input) { } if (normalizedSourcePath === 'skills') { - return path.join(targetRoot, 'skills', CLAUDE_ECC_NAMESPACE); + return path.join(targetRoot, 'skills'); } if (normalizedSourcePath.startsWith('skills/')) { return path.join( targetRoot, 'skills', - CLAUDE_ECC_NAMESPACE, normalizedSourcePath.slice('skills/'.length) ); } diff --git a/scripts/lib/install-targets/claude-project.js b/scripts/lib/install-targets/claude-project.js index 150df276f..051b0ae26 100644 --- a/scripts/lib/install-targets/claude-project.js +++ b/scripts/lib/install-targets/claude-project.js @@ -27,14 +27,13 @@ function getClaudeManagedDestinationPath(adapter, sourceRelativePath, input) { } if (normalizedSourcePath === 'skills') { - return path.join(targetRoot, 'skills', CLAUDE_ECC_NAMESPACE); + return path.join(targetRoot, 'skills'); } if (normalizedSourcePath.startsWith('skills/')) { return path.join( targetRoot, 'skills', - CLAUDE_ECC_NAMESPACE, normalizedSourcePath.slice('skills/'.length) ); } diff --git a/scripts/lib/install/apply.js b/scripts/lib/install/apply.js index 6d5bb7171..cf1afb186 100644 --- a/scripts/lib/install/apply.js +++ b/scripts/lib/install/apply.js @@ -5,7 +5,12 @@ const path = require('path'); const { writeInstallState } = require('../install-state'); const { filterMcpConfig, parseDisabledMcpServers } = require('../mcp-config'); -const { buildInstallIndex, isNamespacedSource, rewriteRelativeLinks } = require('./link-rewrite'); +const { + assertSafeClaudeSkillOperation, + prepareClaudeSkillMigration, + removeLegacyClaudeSkillFiles, +} = require('./claude-skill-migration'); +const { buildInstallIndex, rewriteRelativeLinks } = require('./link-rewrite'); function isMarkdownPath(filePath) { return /\.(md|mdx|markdown)$/i.test(String(filePath || '')); @@ -139,13 +144,49 @@ function buildResolvedClaudeHooks(plan) { }; } -function applyInstallPlan(plan) { - const resolvedClaudeHooksPlan = buildResolvedClaudeHooks(plan); - const disabledServers = parseDisabledMcpServers(process.env.ECC_DISABLED_MCPS); - const linkIndex = buildLinkIndexForPlan(plan); +function previewInstallPlan(plan) { + const migration = prepareClaudeSkillMigration(plan); + return { + ...plan, + statePreview: migration.finalState, + plannedOperations: [...plan.operations], + operations: migration.appliedOperations, + skippedOperations: migration.skippedOperations, + warnings: [ + ...(Array.isArray(plan.warnings) ? plan.warnings : []), + ...migration.warnings, + ], + applied: false, + }; +} - for (const operation of plan.operations) { +function applyInstallPlan(plan, dependencies = {}) { + const persistInstallState = dependencies.writeInstallState || writeInstallState; + const migration = prepareClaudeSkillMigration(plan); + const appliedPlan = { + ...plan, + operations: migration.appliedOperations, + }; + const resolvedClaudeHooksPlan = buildResolvedClaudeHooks(appliedPlan); + const disabledServers = parseDisabledMcpServers(process.env.ECC_DISABLED_MCPS); + const linkIndex = buildLinkIndexForPlan(appliedPlan); + const hasLegacyMigration = migration.legacyOperationsToRemove.length > 0; + + if (migration.requiresBridgeState) { + // Own every operation that may be written during a flat-skill migration + // before the first copy. A later failure is retryable and uninstall can + // clean the entire partial install, including non-skill files. During + // legacy migration the bridge also retains the prior managed operations. + persistInstallState(plan.installStatePath, migration.bridgeState); + } + + for (const operation of appliedPlan.operations) { + assertSafeClaudeSkillOperation(appliedPlan, operation); fs.mkdirSync(path.dirname(operation.destinationPath), { recursive: true }); + // 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') { const payload = cloneJsonValue(operation.mergePayload); @@ -174,16 +215,14 @@ function applyInstallPlan(plan) { continue; } - // Namespaced markdown (e.g. skills/ -> skills/ecc/) needs its - // relative cross-directory links rewritten so they resolve after install - // (issue #2340). Files whose install path is unchanged (no namespace - // injected) and all non-markdown files stay on the byte-for-byte copy path. + // Markdown may reference files whose installed paths move, such as rules + // copied under rules/ecc. Rewrite only links that point at installed targets; + // untouched links and non-markdown files stay on the byte-for-byte path. if ( linkIndex && operation.kind === 'copy-file' && operation.sourceRelativePath && isMarkdownPath(operation.destinationPath) - && isNamespacedSource(operation.sourceRelativePath, linkIndex) ) { const rewritten = rewriteRelativeLinks( fs.readFileSync(operation.sourcePath, 'utf8'), @@ -205,14 +244,26 @@ function applyInstallPlan(plan) { ); } - writeInstallState(plan.installStatePath, plan.statePreview); + if (hasLegacyMigration) { + removeLegacyClaudeSkillFiles(migration, plan.targetRoot); + } + persistInstallState(plan.installStatePath, migration.finalState); return { ...plan, + statePreview: migration.finalState, + plannedOperations: [...plan.operations], + operations: migration.appliedOperations, + skippedOperations: migration.skippedOperations, + warnings: [ + ...(Array.isArray(plan.warnings) ? plan.warnings : []), + ...migration.warnings, + ], applied: true, }; } module.exports = { applyInstallPlan, + previewInstallPlan, }; diff --git a/scripts/lib/install/claude-skill-migration.js b/scripts/lib/install/claude-skill-migration.js new file mode 100644 index 000000000..ba22978be --- /dev/null +++ b/scripts/lib/install/claude-skill-migration.js @@ -0,0 +1,415 @@ +'use strict'; + +const fs = require('fs'); +const path = require('path'); + +const { readInstallState } = require('../install-state'); +const { assertWithinTrustedRoot } = require('../path-safety'); + +const CLAUDE_TARGETS = new Set(['claude', 'claude-project']); + +function pathExists(filePath) { + try { + fs.lstatSync(filePath); + return true; + } catch (error) { + if (error && error.code === 'ENOENT') { + return false; + } + throw error; + } +} + +function normalizeSourceRelativePath(sourceRelativePath) { + const slashNormalized = String(sourceRelativePath || '').replace(/\\/g, '/'); + const normalized = path.posix.normalize(slashNormalized).replace(/^\.\//, ''); + if ( + !normalized + || normalized === '.' + || normalized === '..' + || normalized.startsWith('../') + || path.posix.isAbsolute(normalized) + ) { + return null; + } + return normalized; +} + +function comparablePath(filePath) { + const resolvedPath = path.resolve(filePath); + return process.platform === 'win32' ? resolvedPath.toLowerCase() : resolvedPath; +} + +function samePath(leftPath, rightPath) { + return comparablePath(leftPath) === comparablePath(rightPath); +} + +function assertSafeSkillPath(targetPath, targetRoot, action) { + const resolvedRoot = path.resolve(targetRoot); + const resolvedTarget = path.resolve(targetPath); + const relativePath = path.relative(resolvedRoot, resolvedTarget); + if ( + relativePath === '' + || relativePath.startsWith('..') + || path.isAbsolute(relativePath) + ) { + throw new Error( + `Refusing to ${action} outside the install root: '${targetPath}' is not within '${targetRoot}'.` + ); + } + + let currentPath = resolvedRoot; + for (const segment of relativePath.split(path.sep)) { + currentPath = path.join(currentPath, segment); + let stats; + try { + stats = fs.lstatSync(currentPath); + } catch (error) { + if (error && error.code === 'ENOENT') { + break; + } + throw error; + } + if (stats.isSymbolicLink()) { + throw new Error( + `Refusing to ${action} through symlinked Claude skill path: '${currentPath}'.` + ); + } + } + + if (pathExists(targetRoot)) { + assertWithinTrustedRoot(targetPath, targetRoot, action); + } +} + +function describeClaudeSkillOperation(targetRoot, operation) { + if (!operation || operation.kind !== 'copy-file') { + return null; + } + + const sourceRelativePath = normalizeSourceRelativePath(operation.sourceRelativePath); + if (!sourceRelativePath) { + return null; + } + + const sourceParts = sourceRelativePath.split('/'); + if (sourceParts[0] !== 'skills' || sourceParts.length < 3 || !sourceParts[1]) { + return null; + } + + const skillName = sourceParts[1]; + const relativeParts = sourceParts.slice(2); + const flatSkillRoot = path.join(targetRoot, 'skills', skillName); + const legacySkillRoot = path.join(targetRoot, 'skills', 'ecc', skillName); + + return { + sourceKey: sourceRelativePath, + skillName, + flatSkillRoot, + flatDestinationPath: path.join(flatSkillRoot, ...relativeParts), + legacySkillRoot, + legacyDestinationPath: path.join(legacySkillRoot, ...relativeParts), + }; +} + +function assertSafeClaudeSkillOperation(plan, operation) { + const target = plan && plan.adapter && plan.adapter.target; + if (!CLAUDE_TARGETS.has(target)) { + return; + } + const descriptor = describeClaudeSkillOperation(plan.targetRoot, operation); + if (!descriptor || !samePath(operation.destinationPath, descriptor.flatDestinationPath)) { + return; + } + assertSafeSkillPath( + operation.destinationPath, + plan.targetRoot, + 'install Claude skill' + ); +} + +function isManagedOperation(operation) { + return operation && operation.ownership === 'managed'; +} + +function uniqueOperations(operations) { + const seen = new Set(); + return operations.filter(operation => { + const key = [ + operation.kind, + normalizeSourceRelativePath(operation.sourceRelativePath) || operation.sourceRelativePath, + comparablePath(operation.destinationPath), + ].join('\0'); + if (seen.has(key)) { + return false; + } + seen.add(key); + return true; + }); +} + +function buildState(statePreview, operations) { + return { + ...statePreview, + operations: uniqueOperations(operations).map(operation => ({ ...operation })), + }; +} + +function groupCurrentSkillOperations(plan) { + const groups = new Map(); + for (const operation of plan.operations) { + const descriptor = describeClaudeSkillOperation(plan.targetRoot, operation); + if (!descriptor || !samePath(operation.destinationPath, descriptor.flatDestinationPath)) { + continue; + } + + assertSafeSkillPath( + operation.destinationPath, + plan.targetRoot, + 'install Claude skill' + ); + + const current = groups.get(descriptor.flatSkillRoot) || []; + current.push({ operation, descriptor }); + groups.set(descriptor.flatSkillRoot, current); + } + return groups; +} + +function classifyPreviousOperations(plan, previousState) { + const flatByDestination = new Map(); + const legacyBySource = new Map(); + const legacyBySkillRoot = new Map(); + + for (const operation of (previousState && previousState.operations) || []) { + if (!isManagedOperation(operation)) { + continue; + } + const descriptor = describeClaudeSkillOperation(plan.targetRoot, operation); + if (!descriptor) { + continue; + } + + if (samePath(operation.destinationPath, descriptor.flatDestinationPath)) { + assertSafeSkillPath( + operation.destinationPath, + plan.targetRoot, + 'inspect managed Claude skill' + ); + flatByDestination.set(comparablePath(operation.destinationPath), operation); + continue; + } + + if (!samePath(operation.destinationPath, descriptor.legacyDestinationPath)) { + continue; + } + + assertSafeSkillPath( + operation.destinationPath, + plan.targetRoot, + 'migrate managed Claude skill' + ); + legacyBySource.set(descriptor.sourceKey, operation); + const current = legacyBySkillRoot.get(descriptor.legacySkillRoot) || []; + current.push({ operation, descriptor }); + legacyBySkillRoot.set(descriptor.legacySkillRoot, current); + } + + return { + flatByDestination, + legacyBySource, + legacyBySkillRoot, + }; +} + +function createConflictWarning(skillName, flatSkillRoot, retainsLegacy) { + const legacySuffix = retainsLegacy + ? ' The existing ECC-managed nested copy was retained and remains tracked for uninstall.' + : ''; + return `Skipped Claude skill '${skillName}' at ${flatSkillRoot}: the flat skill directory is user-owned because it is not recorded in ECC install-state.${legacySuffix}`; +} + +function createFileConflictWarning(destinationPath, retainsLegacy) { + const legacySuffix = retainsLegacy + ? ' The matching ECC-managed nested file was retained and remains tracked for uninstall.' + : ''; + return `Skipped user-owned Claude skill file ${destinationPath}: the existing file is not recorded in ECC install-state.${legacySuffix}`; +} + +function createDisabledMigration(plan) { + return { + enabled: false, + appliedOperations: [...plan.operations], + skippedOperations: [], + warnings: [], + bridgeState: plan.statePreview, + finalState: plan.statePreview, + legacyOperationsToRemove: [], + requiresBridgeState: false, + }; +} + +function collectRetainedLegacyOperations(currentGroups, previous) { + const currentSourceKeys = new Set( + [...currentGroups.values()] + .flat() + .map(({ descriptor }) => descriptor.sourceKey) + ); + return ( + [...previous.legacyBySource.entries()] + .filter(([sourceKey]) => !currentSourceKeys.has(sourceKey)) + .map(([_sourceKey, operation]) => operation) + ); +} + +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) { + return { + skippedOperations: entries.map(({ operation }) => operation), + warnings: [createConflictWarning( + entries[0].descriptor.skillName, + flatSkillRoot, + legacyEntries.length > 0 + )], + 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)) + )); + const legacyOperations = [...previous.legacyBySource.values()]; + const legacyOperationsToRemove = legacyOperations.filter(operation => ( + !retainedLegacyOperations.has(operation) + )); + const finalOperations = [ + ...plan.statePreview.operations.filter(operation => ( + !skippedDestinations.has(comparablePath(operation.destinationPath)) + )), + ...retainedLegacyOperations, + ]; + const bridgeOperations = [ + ...((previousState && previousState.operations) || []), + ...appliedOperations, + ]; + + return { + appliedOperations, + bridgeState: buildState(plan.statePreview, bridgeOperations), + finalState: buildState(plan.statePreview, finalOperations), + legacyOperationsToRemove, + requiresBridgeState: appliedOperations.length > 0, + }; +} + +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); + + while (!samePath(currentPath, skillsRoot)) { + assertSafeSkillPath(currentPath, targetRoot, 'clean Claude skill migration'); + if (!pathExists(currentPath) || fs.readdirSync(currentPath).length > 0) { + return; + } + fs.rmdirSync(currentPath); + currentPath = path.dirname(currentPath); + } +} + +function removeLegacyClaudeSkillFiles(migration, targetRoot) { + for (const operation of migration.legacyOperationsToRemove) { + assertSafeSkillPath( + operation.destinationPath, + targetRoot, + 'migrate managed Claude skill' + ); + fs.rmSync(operation.destinationPath, { force: true }); + cleanupEmptyLegacyParents(operation.destinationPath, targetRoot); + } +} + +module.exports = { + assertSafeClaudeSkillOperation, + prepareClaudeSkillMigration, + removeLegacyClaudeSkillFiles, +}; diff --git a/scripts/lib/install/link-rewrite.js b/scripts/lib/install/link-rewrite.js index adfb612dd..2a06fcc10 100644 --- a/scripts/lib/install/link-rewrite.js +++ b/scripts/lib/install/link-rewrite.js @@ -22,7 +22,7 @@ function stripTrailingSlash(value) { // `fileMappings` is a list of { sourceRel, destRel } where both are paths // relative to the repo root and the install root respectively. The directory // map is derived by walking shared ancestors of each source/dest pair, which is -// exact for prefix-insertion namespacing (e.g. `skills/x` -> `skills/ecc/x`): +// exact for prefix-insertion namespacing (e.g. `rules/x` -> `rules/ecc/x`): // the path suffix below the inserted segment is preserved, so ancestor `k` // of the source maps to the dest with the matching number of trailing // segments removed. @@ -94,27 +94,16 @@ 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. skills/x -> skills/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 single namespaced markdown file so they resolve -// to the file's installed location. Returns the content unchanged when the -// file itself was not namespaced or when no link needs adjustment. Pure: no IO. +// 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. +// Pure: no IO. function rewriteRelativeLinks(content, options) { const { sourceRel, index } = options || {}; const normalizedSource = toPosix(sourceRel); const installedSource = index && index.byFile.get(normalizedSource); - // Only rewrite when the file's own install path gained/changed a namespace - // segment. If it lands at the same relative path, every link recomputes to - // itself, so there is nothing to do. - if (!installedSource || installedSource === normalizedSource) { + if (!installedSource) { return content; } @@ -174,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 new file mode 100644 index 000000000..9fd3defe5 --- /dev/null +++ b/tests/lib/install-claude-skill-migration.test.js @@ -0,0 +1,811 @@ +'use strict'; + +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); + +const { applyInstallPlan } = require('../../scripts/lib/install/apply'); +const { readInstallState, writeInstallState } = require('../../scripts/lib/install-state'); +const { uninstallInstalledStates } = require('../../scripts/lib/install-lifecycle'); + +function createTempDir(prefix) { + return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); +} + +function cleanup(dirPath) { + fs.rmSync(dirPath, { recursive: true, force: true }); +} + +function createOperation(moduleId, sourceRoot, sourceRelativePath, destinationPath) { + return { + kind: 'copy-file', + moduleId, + sourcePath: path.join(sourceRoot, sourceRelativePath), + sourceRelativePath, + destinationPath, + strategy: 'preserve-relative-path', + ownership: 'managed', + scaffoldOnly: false, + }; +} + +function createFixture(options = {}) { + const tempDir = createTempDir('claude-skill-migration-'); + const homeDir = path.join(tempDir, 'home'); + const projectRoot = path.join(tempDir, 'project'); + const sourceRoot = path.join(tempDir, 'source'); + const target = options.target || 'claude'; + const targetRoot = target === 'claude' + ? path.join(homeDir, '.claude') + : path.join(projectRoot, '.claude'); + const installStatePath = path.join(targetRoot, 'ecc', 'install-state.json'); + const skillFiles = options.skillFiles || { + 'SKILL.md': '# Current ECC skill\n', + 'references/guide.md': '# Current ECC guide\n', + }; + + for (const [relativePath, content] of Object.entries(skillFiles)) { + const sourcePath = path.join(sourceRoot, 'skills', 'demo-skill', relativePath); + fs.mkdirSync(path.dirname(sourcePath), { recursive: true }); + fs.writeFileSync(sourcePath, content); + } + + const operations = Object.keys(skillFiles).map(relativePath => createOperation( + 'workflow-quality', + sourceRoot, + path.join('skills', 'demo-skill', relativePath), + path.join(targetRoot, 'skills', 'demo-skill', relativePath) + )); + const statePreview = { + schemaVersion: 'ecc.install.v1', + installedAt: new Date().toISOString(), + target: { + id: target === 'claude' ? 'claude-home' : 'claude-project', + target, + kind: target === 'claude' ? 'home' : 'project', + root: targetRoot, + installStatePath, + }, + request: { + profile: null, + modules: ['workflow-quality'], + includeComponents: [], + excludeComponents: [], + legacyLanguages: [], + legacyMode: false, + }, + resolution: { + selectedModules: ['workflow-quality'], + skippedModules: [], + }, + source: { + repoVersion: null, + repoCommit: null, + manifestVersion: 1, + }, + operations: operations.map(operation => ({ ...operation })), + }; + + return { + tempDir, + homeDir, + projectRoot, + sourceRoot, + target, + targetRoot, + installStatePath, + operations, + plan: { + mode: 'manifest', + target, + adapter: { + id: target === 'claude' ? 'claude-home' : 'claude-project', + target, + kind: target === 'claude' ? 'home' : 'project', + }, + targetRoot, + installRoot: targetRoot, + installStatePath, + operations, + statePreview, + warnings: [], + }, + }; +} + +function legacyDestinationPath(targetRoot, operation) { + const sourceParts = operation.sourceRelativePath.split(path.sep); + return path.join(targetRoot, 'skills', 'ecc', ...sourceParts.slice(1)); +} + +function seedLegacyInstall(fixture, options = {}) { + const legacyOperations = fixture.operations.map((operation, index) => { + const destinationPath = legacyDestinationPath(fixture.targetRoot, operation); + fs.mkdirSync(path.dirname(destinationPath), { recursive: true }); + fs.writeFileSync(destinationPath, `# Legacy managed file ${index}\n`); + return { + ...operation, + sourceRelativePath: options.windowsSourcePaths + ? operation.sourceRelativePath.split(path.sep).join('\\') + : operation.sourceRelativePath, + destinationPath, + }; + }); + + writeInstallState(fixture.installStatePath, { + ...fixture.plan.statePreview, + operations: legacyOperations, + }); + return legacyOperations; +} + +function runUninstall(fixture) { + return uninstallInstalledStates({ + homeDir: fixture.homeDir, + projectRoot: fixture.projectRoot, + targets: [fixture.target], + }); +} + +function test(name, fn) { + try { + fn(); + console.log(` \u2713 ${name}`); + return true; + } catch (error) { + console.log(` \u2717 ${name}`); + console.log(` Error: ${error.stack || error.message}`); + return false; + } +} + +function runTests() { + console.log('\n=== Testing Claude flat-skill migration ===\n'); + let passed = 0; + let failed = 0; + + for (const target of ['claude', 'claude-project']) { + if (test(`migrates state-managed nested skills for ${target} without deleting untracked files`, () => { + const fixture = createFixture({ target }); + try { + const legacyOperations = seedLegacyInstall(fixture, { + windowsSourcePaths: target === 'claude-project', + }); + const untrackedPath = path.join( + fixture.targetRoot, + 'skills', + 'ecc', + 'demo-skill', + 'user-notes.md' + ); + fs.writeFileSync(untrackedPath, '# User notes\n'); + + applyInstallPlan(fixture.plan); + + for (const operation of fixture.operations) { + assert.strictEqual( + fs.readFileSync(operation.destinationPath, 'utf8'), + fs.readFileSync(operation.sourcePath, 'utf8') + ); + } + for (const operation of legacyOperations) { + assert.ok(!fs.existsSync(operation.destinationPath), operation.destinationPath); + } + assert.strictEqual(fs.readFileSync(untrackedPath, 'utf8'), '# User notes\n'); + + const state = readInstallState(fixture.installStatePath); + assert.ok(state.operations.some(operation => ( + operation.destinationPath === fixture.operations[0].destinationPath + ))); + assert.ok(!state.operations.some(operation => ( + operation.destinationPath.includes(path.join('skills', 'ecc', 'demo-skill')) + ))); + + const rerun = applyInstallPlan(fixture.plan); + assert.deepStrictEqual(rerun.skippedOperations, []); + assert.strictEqual(fs.readFileSync(untrackedPath, 'utf8'), '# User notes\n'); + + const uninstall = runUninstall(fixture); + assert.strictEqual(uninstall.summary.errorCount, 0); + assert.ok(!fs.existsSync(fixture.operations[0].destinationPath)); + assert.strictEqual(fs.readFileSync(untrackedPath, 'utf8'), '# User notes\n'); + } finally { + cleanup(fixture.tempDir); + } + })) passed++; else failed++; + } + + if (test('selective migration preserves unrelated legacy skills and uninstall ownership', () => { + const fixture = createFixture(); + try { + const legacyOperations = seedLegacyInstall(fixture); + const otherSourceRelativePath = path.join('skills', 'other-skill', 'SKILL.md'); + const otherSourcePath = path.join(fixture.sourceRoot, otherSourceRelativePath); + const otherLegacyPath = path.join( + fixture.targetRoot, + 'skills', + 'ecc', + 'other-skill', + 'SKILL.md' + ); + fs.mkdirSync(path.dirname(otherSourcePath), { recursive: true }); + fs.mkdirSync(path.dirname(otherLegacyPath), { recursive: true }); + fs.writeFileSync(otherSourcePath, '# Other source\n'); + fs.writeFileSync(otherLegacyPath, '# Other legacy managed skill\n'); + const otherLegacyOperation = createOperation( + 'other-module', + fixture.sourceRoot, + otherSourceRelativePath, + otherLegacyPath + ); + writeInstallState(fixture.installStatePath, { + ...fixture.plan.statePreview, + operations: [...legacyOperations, otherLegacyOperation], + }); + + applyInstallPlan(fixture.plan); + + assert.ok(legacyOperations.every(operation => !fs.existsSync(operation.destinationPath))); + assert.strictEqual( + fs.readFileSync(otherLegacyPath, 'utf8'), + '# Other legacy managed skill\n' + ); + const state = readInstallState(fixture.installStatePath); + assert.ok(state.operations.some(operation => ( + operation.destinationPath === otherLegacyPath + ))); + + const uninstall = runUninstall(fixture); + assert.strictEqual(uninstall.summary.errorCount, 0); + assert.ok(!fs.existsSync(otherLegacyPath)); + } finally { + cleanup(fixture.tempDir); + } + })) passed++; else failed++; + + if (test('reruns a completed migration idempotently and remains uninstallable', () => { + const fixture = createFixture(); + try { + const legacyOperations = seedLegacyInstall(fixture); + applyInstallPlan(fixture.plan); + const stateAfterMigration = readInstallState(fixture.installStatePath); + + const rerun = applyInstallPlan(fixture.plan); + const stateAfterRerun = readInstallState(fixture.installStatePath); + + assert.deepStrictEqual(rerun.skippedOperations, []); + assert.ok(!rerun.warnings.some(warning => ( + warning.includes('user-owned') || warning.includes('nested copy') + ))); + assert.deepStrictEqual(stateAfterRerun, stateAfterMigration); + assert.ok(fixture.operations.every(operation => ( + fs.readFileSync(operation.destinationPath, 'utf8') + === fs.readFileSync(operation.sourcePath, 'utf8') + ))); + assert.ok(legacyOperations.every(operation => !fs.existsSync(operation.destinationPath))); + assert.ok(!fs.existsSync(path.join(fixture.targetRoot, 'skills', 'ecc'))); + + const uninstall = runUninstall(fixture); + assert.strictEqual(uninstall.summary.errorCount, 0); + assert.ok(fixture.operations.every(operation => !fs.existsSync(operation.destinationPath))); + } finally { + cleanup(fixture.tempDir); + } + })) passed++; else failed++; + + if (test('preserves a user-owned flat skill and keeps legacy ownership for uninstall', () => { + const fixture = createFixture(); + try { + const legacyOperations = seedLegacyInstall(fixture); + const userSkillPath = fixture.operations[0].destinationPath; + fs.mkdirSync(path.dirname(userSkillPath), { recursive: true }); + fs.writeFileSync(userSkillPath, '# User-owned flat skill\n'); + + const result = applyInstallPlan(fixture.plan); + + assert.strictEqual(fs.readFileSync(userSkillPath, 'utf8'), '# User-owned flat skill\n'); + assert.ok(legacyOperations.every(operation => fs.existsSync(operation.destinationPath))); + assert.ok(result.warnings.some(warning => ( + warning.includes('demo-skill') && warning.includes('user-owned') + )), JSON.stringify(result.warnings)); + assert.strictEqual(result.operations.length, 0); + assert.strictEqual(result.skippedOperations.length, fixture.operations.length); + + const state = readInstallState(fixture.installStatePath); + assert.ok(legacyOperations.every(legacyOperation => ( + state.operations.some(operation => operation.destinationPath === legacyOperation.destinationPath) + ))); + assert.ok(!state.operations.some(operation => ( + operation.destinationPath === fixture.operations[0].destinationPath + ))); + + const uninstall = runUninstall(fixture); + assert.strictEqual(uninstall.summary.errorCount, 0); + assert.strictEqual(fs.readFileSync(userSkillPath, 'utf8'), '# User-owned flat skill\n'); + assert.ok(legacyOperations.every(operation => !fs.existsSync(operation.destinationPath))); + } finally { + cleanup(fixture.tempDir); + } + })) passed++; else failed++; + + if (test('does not claim or merge into a user-owned flat skill on first install', () => { + const fixture = createFixture(); + try { + const userSkillPath = fixture.operations[0].destinationPath; + fs.mkdirSync(path.dirname(userSkillPath), { recursive: true }); + fs.writeFileSync(userSkillPath, '# User-owned flat skill\n'); + + const result = applyInstallPlan(fixture.plan); + + assert.strictEqual(fs.readFileSync(userSkillPath, 'utf8'), '# User-owned flat skill\n'); + assert.ok(!fs.existsSync(fixture.operations[1].destinationPath)); + assert.ok(result.warnings.some(warning => warning.includes('user-owned'))); + assert.strictEqual(result.operations.length, 0); + assert.strictEqual(result.skippedOperations.length, fixture.operations.length); + assert.deepStrictEqual(readInstallState(fixture.installStatePath).operations, []); + + const uninstall = runUninstall(fixture); + assert.strictEqual(uninstall.summary.errorCount, 0); + assert.strictEqual(fs.readFileSync(userSkillPath, 'utf8'), '# User-owned flat skill\n'); + } finally { + cleanup(fixture.tempDir); + } + })) passed++; else failed++; + + if (test('updates recorded flat files but preserves conflicting unrecorded files', () => { + const initial = createFixture({ + skillFiles: { + 'SKILL.md': '# Initial ECC skill\n', + }, + }); + let expanded; + try { + applyInstallPlan(initial.plan); + expanded = createFixture({ + skillFiles: { + 'SKILL.md': '# Updated ECC skill\n', + 'references/guide.md': '# ECC guide\n', + 'references/new.md': '# New managed file\n', + }, + }); + const expandedOriginalTargetRoot = expanded.targetRoot; + expanded.homeDir = initial.homeDir; + expanded.projectRoot = initial.projectRoot; + expanded.targetRoot = initial.targetRoot; + expanded.installStatePath = initial.installStatePath; + expanded.operations = expanded.operations.map(operation => ({ + ...operation, + destinationPath: path.join( + initial.targetRoot, + path.relative(expandedOriginalTargetRoot, operation.destinationPath) + ), + })); + expanded.plan = { + ...expanded.plan, + targetRoot: initial.targetRoot, + installRoot: initial.targetRoot, + installStatePath: initial.installStatePath, + operations: expanded.operations, + statePreview: { + ...expanded.plan.statePreview, + target: { + ...expanded.plan.statePreview.target, + root: initial.targetRoot, + installStatePath: initial.installStatePath, + }, + operations: expanded.operations, + }, + }; + + const userGuidePath = expanded.operations[1].destinationPath; + fs.mkdirSync(path.dirname(userGuidePath), { recursive: true }); + fs.writeFileSync(userGuidePath, '# User guide\n'); + + const result = applyInstallPlan(expanded.plan); + + assert.strictEqual( + fs.readFileSync(expanded.operations[0].destinationPath, 'utf8'), + '# Updated ECC skill\n' + ); + assert.strictEqual(fs.readFileSync(userGuidePath, 'utf8'), '# User guide\n'); + assert.strictEqual( + fs.readFileSync(expanded.operations[2].destinationPath, 'utf8'), + '# New managed file\n' + ); + assert.ok(result.warnings.some(warning => warning.includes('guide.md'))); + + const state = readInstallState(initial.installStatePath); + assert.ok(state.operations.some(operation => ( + operation.destinationPath === expanded.operations[0].destinationPath + ))); + assert.ok(!state.operations.some(operation => ( + operation.destinationPath === userGuidePath + ))); + assert.ok(state.operations.some(operation => ( + operation.destinationPath === expanded.operations[2].destinationPath + ))); + } finally { + cleanup(initial.tempDir); + if (expanded) { + cleanup(expanded.tempDir); + } + } + })) passed++; else failed++; + + if (test('tracks a partial migration so retry and uninstall remain safe', () => { + const fixture = createFixture(); + try { + const legacyOperations = seedLegacyInstall(fixture); + const missingSourcePlan = { + ...fixture.plan, + operations: fixture.operations.map((operation, index) => ( + index === 1 + ? { ...operation, sourcePath: path.join(fixture.sourceRoot, 'missing.md') } + : operation + )), + }; + + assert.throws(() => applyInstallPlan(missingSourcePlan), /ENOENT/); + assert.ok(legacyOperations.every(operation => fs.existsSync(operation.destinationPath))); + assert.ok(fs.existsSync(fixture.operations[0].destinationPath)); + assert.ok(!fs.existsSync(fixture.operations[1].destinationPath)); + const bridgeState = readInstallState(fixture.installStatePath); + assert.ok(legacyOperations.every(legacyOperation => ( + bridgeState.operations.some(operation => ( + operation.destinationPath === legacyOperation.destinationPath + )) + ))); + assert.ok(fixture.operations.every(flatOperation => ( + bridgeState.operations.some(operation => ( + operation.destinationPath === flatOperation.destinationPath + )) + ))); + + const retry = applyInstallPlan(fixture.plan); + assert.deepStrictEqual(retry.skippedOperations, []); + assert.ok(fixture.operations.every(operation => fs.existsSync(operation.destinationPath))); + assert.ok(legacyOperations.every(operation => !fs.existsSync(operation.destinationPath))); + + const uninstall = runUninstall(fixture); + assert.strictEqual(uninstall.summary.errorCount, 0); + assert.ok(fixture.operations.every(operation => !fs.existsSync(operation.destinationPath))); + } finally { + cleanup(fixture.tempDir); + } + })) passed++; else failed++; + + if (test('tracks a partial first install so retry does not misclassify it as user-owned', () => { + const fixture = createFixture(); + try { + const missingSourcePlan = { + ...fixture.plan, + operations: fixture.operations.map((operation, index) => ( + index === 1 + ? { ...operation, sourcePath: path.join(fixture.sourceRoot, 'missing.md') } + : operation + )), + }; + + assert.throws(() => applyInstallPlan(missingSourcePlan), /ENOENT/); + assert.ok(fs.existsSync(fixture.operations[0].destinationPath)); + assert.ok(!fs.existsSync(fixture.operations[1].destinationPath)); + const bridgeState = readInstallState(fixture.installStatePath); + assert.ok(fixture.operations.every(flatOperation => ( + bridgeState.operations.some(operation => ( + operation.destinationPath === flatOperation.destinationPath + )) + ))); + + const retry = applyInstallPlan(fixture.plan); + assert.deepStrictEqual(retry.skippedOperations, []); + assert.ok(!retry.warnings.some(warning => warning.includes('user-owned'))); + assert.ok(fixture.operations.every(operation => fs.existsSync(operation.destinationPath))); + + const uninstall = runUninstall(fixture); + assert.strictEqual(uninstall.summary.errorCount, 0); + assert.ok(fixture.operations.every(operation => !fs.existsSync(operation.destinationPath))); + } finally { + cleanup(fixture.tempDir); + } + })) passed++; else failed++; + + if (test('tracks non-skill files written before a partial flat-skill install fails', () => { + const fixture = createFixture(); + try { + const ruleSourceRelativePath = path.join('rules', 'common', 'coding.md'); + const ruleSourcePath = path.join(fixture.sourceRoot, ruleSourceRelativePath); + const ruleDestinationPath = path.join( + fixture.targetRoot, + 'rules', + 'ecc', + 'common', + 'coding.md' + ); + fs.mkdirSync(path.dirname(ruleSourcePath), { recursive: true }); + fs.writeFileSync(ruleSourcePath, '# Managed rule\n'); + + const ruleOperation = createOperation( + 'workflow-quality', + fixture.sourceRoot, + ruleSourceRelativePath, + ruleDestinationPath + ); + const missingOperation = createOperation( + 'workflow-quality', + fixture.sourceRoot, + path.join('commands', 'missing.md'), + path.join(fixture.targetRoot, 'commands', 'missing.md') + ); + const operations = [ + fixture.operations[0], + ruleOperation, + missingOperation, + ]; + const partialPlan = { + ...fixture.plan, + operations, + statePreview: { + ...fixture.plan.statePreview, + operations: operations.map(operation => ({ ...operation })), + }, + }; + + assert.throws(() => applyInstallPlan(partialPlan), /ENOENT/); + assert.ok(fs.existsSync(ruleDestinationPath)); + + const bridgeState = readInstallState(fixture.installStatePath); + assert.ok(bridgeState.operations.some(operation => ( + operation.destinationPath === ruleDestinationPath + ))); + + const uninstall = runUninstall(fixture); + assert.strictEqual(uninstall.summary.errorCount, 0); + assert.ok(!fs.existsSync(ruleDestinationPath)); + } finally { + cleanup(fixture.tempDir); + } + })) passed++; else failed++; + + if (test('tracks partial non-skill writes when every flat skill is user-owned', () => { + const fixture = createFixture(); + try { + const userSkillPath = fixture.operations[0].destinationPath; + fs.mkdirSync(path.dirname(userSkillPath), { recursive: true }); + fs.writeFileSync(userSkillPath, '# User skill\n'); + + const ruleSourceRelativePath = path.join('rules', 'common', 'coding.md'); + const ruleSourcePath = path.join(fixture.sourceRoot, ruleSourceRelativePath); + const ruleDestinationPath = path.join( + fixture.targetRoot, + 'rules', + 'ecc', + 'common', + 'coding.md' + ); + fs.mkdirSync(path.dirname(ruleSourcePath), { recursive: true }); + fs.writeFileSync(ruleSourcePath, '# Managed rule\n'); + + const ruleOperation = createOperation( + 'workflow-quality', + fixture.sourceRoot, + ruleSourceRelativePath, + ruleDestinationPath + ); + const missingOperation = createOperation( + 'workflow-quality', + fixture.sourceRoot, + path.join('commands', 'missing.md'), + path.join(fixture.targetRoot, 'commands', 'missing.md') + ); + const operations = [ + ...fixture.operations, + ruleOperation, + missingOperation, + ]; + const partialPlan = { + ...fixture.plan, + operations, + statePreview: { + ...fixture.plan.statePreview, + operations: operations.map(operation => ({ ...operation })), + }, + }; + + assert.throws(() => applyInstallPlan(partialPlan), /ENOENT/); + assert.strictEqual(fs.readFileSync(userSkillPath, 'utf8'), '# User skill\n'); + assert.ok(fs.existsSync(ruleDestinationPath)); + + const bridgeState = readInstallState(fixture.installStatePath); + assert.ok(!bridgeState.operations.some(operation => ( + operation.destinationPath === userSkillPath + ))); + assert.ok(bridgeState.operations.some(operation => ( + operation.destinationPath === ruleDestinationPath + ))); + + const uninstall = runUninstall(fixture); + assert.strictEqual(uninstall.summary.errorCount, 0); + assert.strictEqual(fs.readFileSync(userSkillPath, 'utf8'), '# User skill\n'); + assert.ok(!fs.existsSync(ruleDestinationPath)); + } finally { + cleanup(fixture.tempDir); + } + })) passed++; else failed++; + + if (test('keeps legacy files tracked when the bridge state write fails', () => { + const fixture = createFixture(); + try { + const legacyOperations = seedLegacyInstall(fixture); + const failingStateWriter = filePath => { + assert.strictEqual( + path.resolve(filePath), + path.resolve(fixture.installStatePath) + ); + throw new Error('injected install-state write failure'); + }; + + assert.throws( + () => applyInstallPlan(fixture.plan, { writeInstallState: failingStateWriter }), + /injected install-state write failure/ + ); + + assert.ok(legacyOperations.every(operation => fs.existsSync(operation.destinationPath))); + assert.ok(fixture.operations.every(operation => !fs.existsSync(operation.destinationPath))); + const state = readInstallState(fixture.installStatePath); + assert.ok(state.operations.every(operation => ( + operation.destinationPath.includes(path.join('skills', 'ecc', 'demo-skill')) + ))); + + const retry = applyInstallPlan(fixture.plan); + assert.deepStrictEqual(retry.skippedOperations, []); + const uninstall = runUninstall(fixture); + assert.strictEqual(uninstall.summary.errorCount, 0); + assert.ok(fixture.operations.every(operation => !fs.existsSync(operation.destinationPath))); + } finally { + cleanup(fixture.tempDir); + } + })) passed++; else failed++; + + if (test('keeps both layouts represented if the final state write fails', () => { + const fixture = createFixture(); + let stateWriteCount = 0; + try { + const legacyOperations = seedLegacyInstall(fixture); + const failFinalStateWrite = (filePath, state) => { + assert.strictEqual( + path.resolve(filePath), + path.resolve(fixture.installStatePath) + ); + stateWriteCount += 1; + if (stateWriteCount === 2) { + throw new Error('injected final install-state write failure'); + } + return writeInstallState(fixture.installStatePath, state); + }; + + assert.throws( + () => applyInstallPlan(fixture.plan, { writeInstallState: failFinalStateWrite }), + /injected final install-state write failure/ + ); + assert.ok(fixture.operations.every(operation => fs.existsSync(operation.destinationPath))); + assert.ok(legacyOperations.every(operation => !fs.existsSync(operation.destinationPath))); + + const bridgeState = readInstallState(fixture.installStatePath); + assert.ok(fixture.operations.every(flatOperation => ( + bridgeState.operations.some(operation => ( + operation.destinationPath === flatOperation.destinationPath + )) + ))); + assert.ok(bridgeState.operations.some(operation => ( + operation.destinationPath.includes(path.join('skills', 'ecc', 'demo-skill')) + ))); + + const uninstall = runUninstall(fixture); + assert.strictEqual(uninstall.summary.errorCount, 0); + assert.ok(fixture.operations.every(operation => !fs.existsSync(operation.destinationPath))); + } finally { + cleanup(fixture.tempDir); + } + })) passed++; else failed++; + + if (test('rejects a flat skill symlink that escapes the Claude install root', () => { + if (process.platform === 'win32') { + console.log(' ↷ skipped on Windows: symlink privileges vary'); + return; + } + + const fixture = createFixture(); + try { + const outsideRoot = path.join(fixture.tempDir, 'outside'); + fs.mkdirSync(outsideRoot, { recursive: true }); + const flatSkillRoot = path.join(fixture.targetRoot, 'skills', 'demo-skill'); + fs.mkdirSync(path.dirname(flatSkillRoot), { recursive: true }); + fs.symlinkSync(outsideRoot, flatSkillRoot, 'dir'); + + assert.throws( + () => applyInstallPlan(fixture.plan), + /symlinked Claude skill path/ + ); + assert.deepStrictEqual(fs.readdirSync(outsideRoot), []); + assert.ok(!fs.existsSync(fixture.installStatePath)); + } finally { + cleanup(fixture.tempDir); + } + })) 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'); + return; + } + + const fixture = createFixture({ + skillFiles: { + 'SKILL.md': '# Current ECC skill\n', + }, + }); + try { + const outsideRoot = path.join(fixture.tempDir, 'outside'); + const outsideTarget = path.join(outsideRoot, 'not-created.md'); + fs.mkdirSync(outsideRoot, { recursive: true }); + fs.mkdirSync(path.dirname(fixture.operations[0].destinationPath), { recursive: true }); + fs.symlinkSync(outsideTarget, fixture.operations[0].destinationPath, 'file'); + assert.strictEqual(fs.existsSync(fixture.operations[0].destinationPath), false); + + assert.throws( + () => applyInstallPlan(fixture.plan), + /symlinked Claude skill path/ + ); + assert.ok(!fs.existsSync(outsideTarget)); + assert.ok(!fs.existsSync(fixture.installStatePath)); + } finally { + cleanup(fixture.tempDir); + } + })) passed++; else failed++; + + console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); + process.exit(failed > 0 ? 1 : 0); +} + +runTests(); diff --git a/tests/lib/install-executor.test.js b/tests/lib/install-executor.test.js index b26fea924..7a2a4df1c 100644 --- a/tests/lib/install-executor.test.js +++ b/tests/lib/install-executor.test.js @@ -362,7 +362,7 @@ function runTests() { ))); assert.ok(plan.operations.some(operation => ( operation.sourceRelativePath === path.join('skills', 'demo', 'SKILL.md') - && operation.destinationPath === path.join(homeDir, '.claude', 'skills', 'ecc', 'demo', 'SKILL.md') + && operation.destinationPath === path.join(homeDir, '.claude', 'skills', 'demo', 'SKILL.md') ))); assert.deepStrictEqual(plan.warnings, ['fixture warning']); assert.strictEqual(plan.statePreview.request.profile, 'minimal'); @@ -416,7 +416,7 @@ function runTests() { assert.strictEqual(applied.applied, true); assert.ok(fs.existsSync(path.join(homeDir, '.claude', 'rules', 'ecc', 'common', 'coding-style.md'))); - assert.ok(fs.existsSync(path.join(homeDir, '.claude', 'skills', 'ecc', 'demo', 'SKILL.md'))); + assert.ok(fs.existsSync(path.join(homeDir, '.claude', 'skills', 'demo', 'SKILL.md'))); assert.ok(fs.existsSync(path.join(homeDir, '.claude', 'src', 'app.js'))); assert.ok(fs.existsSync(path.join(homeDir, '.claude', 'standalone.txt'))); assert.ok(fs.existsSync(path.join(homeDir, '.claude', 'plugin.json'))); diff --git a/tests/lib/install-lifecycle.test.js b/tests/lib/install-lifecycle.test.js index c037c0604..555b6af7b 100644 --- a/tests/lib/install-lifecycle.test.js +++ b/tests/lib/install-lifecycle.test.js @@ -634,6 +634,103 @@ function runTests() { } })) passed++; else failed++; + if (test('Claude repair and dry-run preserve user-owned flat skills during legacy migration', () => { + const homeDir = createTempDir('install-lifecycle-home-'); + const projectRoot = createTempDir('install-lifecycle-project-'); + + try { + const targetRoot = path.join(homeDir, '.claude'); + const installStatePath = path.join(targetRoot, 'ecc', 'install-state.json'); + const flatSkillPath = path.join(targetRoot, 'skills', 'tdd-workflow', 'SKILL.md'); + const legacySkillPath = path.join( + targetRoot, + 'skills', + 'ecc', + 'tdd-workflow', + 'SKILL.md' + ); + fs.mkdirSync(path.dirname(flatSkillPath), { recursive: true }); + fs.mkdirSync(path.dirname(legacySkillPath), { recursive: true }); + fs.writeFileSync(flatSkillPath, '# User-owned flat skill\n'); + fs.writeFileSync(legacySkillPath, '# Previously managed nested skill\n'); + + writeState(installStatePath, { + adapter: { id: 'claude-home', target: 'claude', kind: 'home' }, + targetRoot, + installStatePath, + request: { + profile: null, + modules: ['workflow-quality'], + includeComponents: [], + excludeComponents: [], + legacyLanguages: [], + legacyMode: false, + }, + resolution: { + selectedModules: ['platform-configs', 'workflow-quality'], + skippedModules: [], + }, + operations: [{ + kind: 'copy-file', + moduleId: 'workflow-quality', + sourcePath: path.join(REPO_ROOT, 'skills', 'tdd-workflow', 'SKILL.md'), + sourceRelativePath: path.join('skills', 'tdd-workflow', 'SKILL.md'), + destinationPath: legacySkillPath, + strategy: 'preserve-relative-path', + ownership: 'managed', + scaffoldOnly: false, + }], + source: { + repoVersion: CURRENT_PACKAGE_VERSION, + repoCommit: 'abc123', + manifestVersion: CURRENT_MANIFEST_VERSION, + }, + }); + + const dryRun = repairInstalledStates({ + repoRoot: REPO_ROOT, + homeDir, + projectRoot, + targets: ['claude'], + dryRun: true, + }); + assert.ok(!dryRun.results[0].plannedRepairs.includes(flatSkillPath)); + assert.ok(dryRun.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' + ); + + const repaired = repairInstalledStates({ + repoRoot: REPO_ROOT, + homeDir, + projectRoot, + targets: ['claude'], + }); + 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'), + 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 => ( + operation.destinationPath === legacySkillPath + ))); + assert.ok(!repairedState.operations.some(operation => ( + operation.destinationPath === flatSkillPath + ))); + } finally { + cleanup(homeDir); + cleanup(projectRoot); + } + })) passed++; else failed++; + if (test('repair copies missing managed files from recorded source paths', () => { const homeDir = createTempDir('install-lifecycle-home-'); const projectRoot = createTempDir('install-lifecycle-project-'); diff --git a/tests/lib/install-link-rewrite.test.js b/tests/lib/install-link-rewrite.test.js index b4a75115d..943de7ceb 100644 --- a/tests/lib/install-link-rewrite.test.js +++ b/tests/lib/install-link-rewrite.test.js @@ -10,20 +10,19 @@ const path = require('path'); const { buildInstallIndex, - isNamespacedSource, rewriteRelativeLinks, } = require('../../scripts/lib/install/link-rewrite'); const { createManifestInstallPlan } = require('../../scripts/lib/install-executor'); const REPO_ROOT = path.resolve(__dirname, '..', '..'); -// A claude-style namespace placement: skills/ -> skills/ecc/ and +// A claude-style namespace placement: skills/ -> skills/ and // rules/ -> rules/ecc/. Mirrors what the real adapter emits. function claudeNamespaceMappings() { return [ - { sourceRel: 'skills/react-patterns/SKILL.md', destRel: 'skills/ecc/react-patterns/SKILL.md' }, - { sourceRel: 'skills/react-patterns/other.md', destRel: 'skills/ecc/react-patterns/other.md' }, - { sourceRel: 'skills/react-patterns/sub/NOTE.md', destRel: 'skills/ecc/react-patterns/sub/NOTE.md' }, + { sourceRel: 'skills/react-patterns/SKILL.md', destRel: 'skills/react-patterns/SKILL.md' }, + { sourceRel: 'skills/react-patterns/other.md', destRel: 'skills/react-patterns/other.md' }, + { sourceRel: 'skills/react-patterns/sub/NOTE.md', destRel: 'skills/react-patterns/sub/NOTE.md' }, { sourceRel: 'rules/react/hooks.md', destRel: 'rules/ecc/react/hooks.md' }, { sourceRel: 'rules/react/testing.md', destRel: 'rules/ecc/react/testing.md' }, { sourceRel: 'rules/react/coding-style.md', destRel: 'rules/ecc/react/coding-style.md' }, @@ -64,17 +63,17 @@ function runTests() { for (const skill of ['react-patterns', 'react-performance', 'react-testing']) { if (test(`rewrites ../../rules file link for ${skill}`, () => { const idx = buildInstallIndex([ - { sourceRel: `skills/${skill}/SKILL.md`, destRel: `skills/ecc/${skill}/SKILL.md` }, + { sourceRel: `skills/${skill}/SKILL.md`, destRel: `skills/${skill}/SKILL.md` }, { sourceRel: 'rules/react/hooks.md', destRel: 'rules/ecc/react/hooks.md' }, ]); const before = 'See [rules](../../rules/react/hooks.md) for details.'; const after = rewriteRelativeLinks(before, { sourceRel: `skills/${skill}/SKILL.md`, index: idx }); assert.notStrictEqual(after, before, 'rewrite must change the broken link (not vacuous)'); assert.ok( - after.includes('](../../../rules/ecc/react/hooks.md)'), + after.includes('](../../rules/ecc/react/hooks.md)'), `expected corrected link, got: ${after}` ); - assert.ok(!after.includes('](../../rules/'), 'broken depth must be gone'); + assert.ok(!after.includes('](../../rules/react/'), 'un-namespaced rules link must be gone'); })) passed++; else failed++; } @@ -82,7 +81,7 @@ function runTests() { const before = '- Rules: [rules/react/](../../rules/react/)'; const after = rewriteRelativeLinks(before, { sourceRel: 'skills/react-patterns/SKILL.md', index }); assert.notStrictEqual(after, before); - assert.ok(after.includes('](../../../rules/ecc/react/)'), `got: ${after}`); + assert.ok(after.includes('](../../rules/ecc/react/)'), `got: ${after}`); })) passed++; else failed++; if (test('leaves an intra-skill sibling link unchanged', () => { @@ -111,7 +110,7 @@ function runTests() { if (test('preserves a #fragment on a rewritten link', () => { const before = '[hooks](../../rules/react/hooks.md#use-effect)'; const after = rewriteRelativeLinks(before, { sourceRel: 'skills/react-patterns/SKILL.md', index }); - assert.ok(after.includes('](../../../rules/ecc/react/hooks.md#use-effect)'), `got: ${after}`); + assert.ok(after.includes('](../../rules/ecc/react/hooks.md#use-effect)'), `got: ${after}`); })) passed++; else failed++; if (test('does not rewrite links inside fenced code blocks', () => { @@ -123,16 +122,16 @@ function runTests() { ].join('\n'); const after = rewriteRelativeLinks(before, { sourceRel: 'skills/react-patterns/SKILL.md', index }); assert.ok(after.includes('[code](../../rules/react/hooks.md)'), 'code-fence link must be untouched'); - assert.ok(after.includes('[prose](../../../rules/ecc/react/hooks.md)'), 'prose link must be rewritten'); + assert.ok(after.includes('[prose](../../rules/ecc/react/hooks.md)'), 'prose link must be rewritten'); })) passed++; else failed++; if (test('computes depth from path math for a nested skill file', () => { - // skills/react-patterns/sub/NOTE.md -> skills/ecc/react-patterns/sub/NOTE.md + // skills/react-patterns/sub/NOTE.md -> skills/react-patterns/sub/NOTE.md // Source link is ../../../rules/react/hooks.md (3 up from sub/). const before = '[r](../../../rules/react/hooks.md)'; const after = rewriteRelativeLinks(before, { sourceRel: 'skills/react-patterns/sub/NOTE.md', index }); assert.notStrictEqual(after, before, 'nested depth must be recomputed, not hardcoded'); - assert.ok(after.includes('](../../../../rules/ecc/react/hooks.md)'), `got: ${after}`); + assert.ok(after.includes('](../../../rules/ecc/react/hooks.md)'), `got: ${after}`); })) passed++; else failed++; if (test('is a no-op for a non-namespacing (identity) placement', () => { @@ -148,24 +147,6 @@ function runTests() { assert.strictEqual(after, before); })) passed++; else failed++; - // Guards the apply-layer gate: only namespaced files leave the byte-copy - // path, so non-namespaced markdown is still copied verbatim. - if (test('isNamespacedSource flags only files whose install path changed', () => { - assert.strictEqual( - isNamespacedSource('skills/react-patterns/SKILL.md', index), true, - 'a namespaced skill file must 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', () => { @@ -201,13 +182,16 @@ function runTests() { const content = fs.readFileSync(path.join(REPO_ROOT, sourceRel), 'utf8'); assert.ok(content.includes('](../../rules/'), `${sourceRel} should have a broken link pre-fix`); const rewritten = rewriteRelativeLinks(content, { sourceRel, index: realIndex }); - assert.ok(!rewritten.includes('](../../rules/'), `${sourceRel} still has the broken depth`); + assert.ok( + !rewritten.includes('](../../rules/react/'), + `${sourceRel} still links to un-namespaced rules` + ); // Only links we actually changed are validated here; cross-skill links to // skills outside this module subset are legitimately left untouched. const before = extractLinks(content); const after = extractLinks(rewritten); - const installedSkillDir = path.posix.dirname(`skills/ecc/${skill}/SKILL.md`); + const installedSkillDir = path.posix.dirname(`skills/${skill}/SKILL.md`); for (let i = 0; i < after.length; i += 1) { if (after[i] === before[i]) { continue; diff --git a/tests/lib/install-targets.test.js b/tests/lib/install-targets.test.js index 3e192648e..c527d03e6 100644 --- a/tests/lib/install-targets.test.js +++ b/tests/lib/install-targets.test.js @@ -71,7 +71,7 @@ function runTests() { assert.strictEqual(statePath, path.join(homeDir, '.claude', 'ecc', 'install-state.json')); })) passed++; else failed++; - if (test('plans claude rules and skills under ECC-managed subdirectories', () => { + if (test('plans namespaced Claude rules and flat discoverable skills', () => { const repoRoot = path.join(__dirname, '..', '..'); const homeDir = '/Users/example'; @@ -101,9 +101,9 @@ function runTests() { assert.ok( plan.operations.some(operation => ( normalizedRelativePath(operation.sourceRelativePath) === 'skills/tdd-workflow' - && operation.destinationPath === path.join(homeDir, '.claude', 'skills', 'ecc', 'tdd-workflow') + && operation.destinationPath === path.join(homeDir, '.claude', 'skills', 'tdd-workflow') )), - 'Should install bundled Claude skills under skills/ecc' + 'Should install bundled Claude skills under skills' ); })) passed++; else failed++; @@ -884,7 +884,7 @@ function runTests() { assert.ok(byTarget.supports('claude-project')); })) passed++; else failed++; - if (test('plans claude-project rules and skills under project-scope ECC-managed subdirectories', () => { + if (test('plans project-scoped namespaced Claude rules and flat skills', () => { const repoRoot = path.join(__dirname, '..', '..'); const projectRoot = '/workspace/app'; @@ -917,9 +917,9 @@ function runTests() { assert.ok( plan.operations.some(operation => ( normalizedRelativePath(operation.sourceRelativePath) === 'skills/tdd-workflow' - && operation.destinationPath === path.join(projectRoot, '.claude', 'skills', 'ecc', 'tdd-workflow') + && operation.destinationPath === path.join(projectRoot, '.claude', 'skills', 'tdd-workflow') )), - 'Should install bundled skills under project-scope skills/ecc' + 'Should install bundled skills under project-scope skills' ); })) passed++; else failed++; diff --git a/tests/lib/locale-install.test.js b/tests/lib/locale-install.test.js index f65b7777a..0df64f4e1 100644 --- a/tests/lib/locale-install.test.js +++ b/tests/lib/locale-install.test.js @@ -205,7 +205,7 @@ function runTests() { 'Should install Japanese README under docs/ja-JP' ); assert.ok( - !fs.existsSync(path.join(claudeRoot, 'skills', 'ecc', 'configure-ecc', 'SKILL.md')), + !fs.existsSync(path.join(claudeRoot, 'skills', 'configure-ecc', 'SKILL.md')), 'Locale-only install should not install English skills' ); diff --git a/tests/lib/selective-install.test.js b/tests/lib/selective-install.test.js index c680c71b2..97c2e5bc8 100644 --- a/tests/lib/selective-install.test.js +++ b/tests/lib/selective-install.test.js @@ -658,7 +658,7 @@ function runTests() { const claudeRoot = path.join(homeDir, '.claude'); // Security skill should be installed (from --with) - assert.ok(fs.existsSync(path.join(claudeRoot, 'skills', 'ecc', 'security-review', 'SKILL.md')), + assert.ok(fs.existsSync(path.join(claudeRoot, 'skills', 'security-review', 'SKILL.md')), 'Should install security-review skill from --with'); // Core profile modules should be installed assert.ok(fs.existsSync(path.join(claudeRoot, 'rules', 'ecc', 'common', 'coding-style.md')), @@ -697,12 +697,12 @@ function runTests() { const claudeRoot = path.join(homeDir, '.claude'); // Orchestration skills should NOT be installed (from --without) - assert.ok(!fs.existsSync(path.join(claudeRoot, 'skills', 'ecc', 'dmux-workflows', 'SKILL.md')), + assert.ok(!fs.existsSync(path.join(claudeRoot, 'skills', 'dmux-workflows', 'SKILL.md')), 'Should not install orchestration skills'); // Developer profile base modules should be installed assert.ok(fs.existsSync(path.join(claudeRoot, 'rules', 'ecc', 'common', 'coding-style.md')), 'Should install core rules'); - assert.ok(fs.existsSync(path.join(claudeRoot, 'skills', 'ecc', 'tdd-workflow', 'SKILL.md')), + assert.ok(fs.existsSync(path.join(claudeRoot, 'skills', 'tdd-workflow', 'SKILL.md')), 'Should install workflow skills'); const statePath = path.join(claudeRoot, 'ecc', 'install-state.json'); @@ -735,7 +735,7 @@ function runTests() { const claudeRoot = path.join(homeDir, '.claude'); // framework-language skill (from lang:typescript) should be installed - assert.ok(fs.existsSync(path.join(claudeRoot, 'skills', 'ecc', 'coding-standards', 'SKILL.md')), + assert.ok(fs.existsSync(path.join(claudeRoot, 'skills', 'coding-standards', 'SKILL.md')), 'Should install framework-language skills'); // Its dependencies should be installed assert.ok(fs.existsSync(path.join(claudeRoot, 'rules', 'ecc', 'common', 'coding-style.md')), @@ -771,11 +771,11 @@ function runTests() { const claudeRoot = path.join(homeDir, '.claude'); assert.ok( - fs.existsSync(path.join(claudeRoot, 'skills', 'ecc', 'continuous-learning-v2', 'SKILL.md')), + fs.existsSync(path.join(claudeRoot, 'skills', 'continuous-learning-v2', 'SKILL.md')), 'Should install continuous-learning-v2' ); assert.ok( - !fs.existsSync(path.join(claudeRoot, 'skills', 'ecc', 'tdd-workflow', 'SKILL.md')), + !fs.existsSync(path.join(claudeRoot, 'skills', 'tdd-workflow', 'SKILL.md')), 'Should not install unrelated workflow-quality skills' ); diff --git a/tests/scripts/install-apply.test.js b/tests/scripts/install-apply.test.js index 9ef7def47..721503146 100644 --- a/tests/scripts/install-apply.test.js +++ b/tests/scripts/install-apply.test.js @@ -100,8 +100,8 @@ function runTests() { assert.ok(fs.existsSync(path.join(claudeRoot, 'commands', 'plan.md'))); assert.ok(fs.existsSync(path.join(claudeRoot, 'scripts', 'hooks', 'session-end.js'))); assert.ok(fs.existsSync(path.join(claudeRoot, 'scripts', 'lib', 'utils.js'))); - assert.ok(fs.existsSync(path.join(claudeRoot, 'skills', 'ecc', 'tdd-workflow', 'SKILL.md'))); - assert.ok(fs.existsSync(path.join(claudeRoot, 'skills', 'ecc', 'coding-standards', 'SKILL.md'))); + assert.ok(fs.existsSync(path.join(claudeRoot, 'skills', 'tdd-workflow', 'SKILL.md'))); + assert.ok(fs.existsSync(path.join(claudeRoot, 'skills', 'coding-standards', 'SKILL.md'))); assert.ok(fs.existsSync(path.join(claudeRoot, 'plugin.json'))); const statePath = path.join(homeDir, '.claude', 'ecc', 'install-state.json'); @@ -133,23 +133,23 @@ function runTests() { assert.strictEqual(result.code, 0, result.stderr); const claudeRoot = path.join(homeDir, '.claude'); - const skillPath = path.join(claudeRoot, 'skills', 'ecc', 'react-patterns', 'SKILL.md'); + const skillPath = path.join(claudeRoot, 'skills', 'react-patterns', 'SKILL.md'); assert.ok(fs.existsSync(skillPath), 'react-patterns SKILL.md should be installed'); const content = fs.readFileSync(skillPath, 'utf8'); assert.ok( - content.includes('../../../rules/ecc/react/'), + content.includes('../../rules/ecc/react/'), 'source-relative rules link should be rewritten for the ecc/ namespace' ); assert.ok( - !content.includes('](../../rules/'), - 'no un-namespaced ](../../rules/ links should remain' + !content.includes('](../../rules/react/'), + 'no un-namespaced ](../../rules/react/ links should remain' ); // The rewritten link must resolve to a file that actually exists on disk. const linkTarget = path.join( path.dirname(skillPath), - '../../../rules/ecc/react/hooks.md' + '../../rules/ecc/react/hooks.md' ); assert.ok(fs.existsSync(linkTarget), 'rewritten link target should exist'); } finally { @@ -468,11 +468,101 @@ function runTests() { const result = run(['--profile', 'core'], { cwd: projectDir, homeDir }); assert.strictEqual(result.code, 0, result.stderr); + assert.ok(result.stdout.includes('user-owned'), result.stdout); + assert.ok(result.stdout.includes('Skipped operations:'), result.stdout); assert.strictEqual(fs.readFileSync(userRulePath, 'utf8'), '# User custom rule\n'); assert.strictEqual(fs.readFileSync(userSkillPath, 'utf8'), '# User custom skill\n'); assert.ok(fs.existsSync(path.join(claudeRoot, 'rules', 'ecc', 'common', 'coding-style.md'))); - assert.ok(fs.existsSync(path.join(claudeRoot, 'skills', 'ecc', 'tdd-workflow', 'SKILL.md'))); + assert.ok(fs.existsSync(path.join(claudeRoot, 'skills', 'verification-loop', 'SKILL.md'))); + const state = readJson(path.join(claudeRoot, 'ecc', 'install-state.json')); + assert.ok(!state.operations.some(operation => ( + operation.destinationPath.startsWith(path.join(claudeRoot, 'skills', 'tdd-workflow')) + ))); + } finally { + cleanup(homeDir); + cleanup(projectDir); + } + })) passed++; else failed++; + + if (test('reports applied and skipped user-owned Claude skill operations in JSON', () => { + const homeDir = createTempDir('install-apply-home-'); + const projectDir = createTempDir('install-apply-project-'); + + try { + const userSkillPath = path.join( + homeDir, + '.claude', + 'skills', + 'tdd-workflow', + 'SKILL.md' + ); + fs.mkdirSync(path.dirname(userSkillPath), { recursive: true }); + fs.writeFileSync(userSkillPath, '# User custom skill\n'); + + const result = run(['--skills', 'tdd-workflow', '--json'], { + cwd: projectDir, + homeDir, + }); + assert.strictEqual(result.code, 0, result.stderr); + + const payload = JSON.parse(result.stdout); + assert.strictEqual(payload.dryRun, false); + assert.ok(payload.result.plannedOperations.length > 0); + assert.ok(payload.result.operations.length > 0); + assert.ok(payload.result.skippedOperations.length > 0); + assert.strictEqual( + payload.result.operations.length + payload.result.skippedOperations.length, + payload.result.plannedOperations.length + ); + assert.ok(payload.result.skippedOperations.every(operation => ( + operation.destinationPath.startsWith(path.dirname(userSkillPath)) + ))); + assert.ok(!payload.result.operations.some(operation => ( + operation.destinationPath.startsWith(path.dirname(userSkillPath)) + ))); + assert.ok(payload.result.warnings.some(warning => warning.includes('user-owned'))); + assert.strictEqual(fs.readFileSync(userSkillPath, 'utf8'), '# User custom skill\n'); + } finally { + cleanup(homeDir); + cleanup(projectDir); + } + })) passed++; else failed++; + + if (test('dry-run reports the same user-owned Claude skill conflicts as apply', () => { + const homeDir = createTempDir('install-apply-home-'); + const projectDir = createTempDir('install-apply-project-'); + + try { + const userSkillRoot = path.join( + homeDir, + '.claude', + 'skills', + 'tdd-workflow' + ); + const userSkillPath = path.join(userSkillRoot, 'SKILL.md'); + fs.mkdirSync(userSkillRoot, { recursive: true }); + fs.writeFileSync(userSkillPath, '# User custom skill\n'); + + const result = run( + ['--skills', 'tdd-workflow', '--dry-run', '--json'], + { cwd: projectDir, homeDir } + ); + assert.strictEqual(result.code, 0, result.stderr); + + const payload = JSON.parse(result.stdout); + assert.strictEqual(payload.dryRun, true); + assert.ok(payload.plan.plannedOperations.length > 0); + assert.ok(payload.plan.skippedOperations.length > 0); + assert.ok(payload.plan.warnings.some(warning => warning.includes('user-owned'))); + assert.ok(payload.plan.skippedOperations.every(operation => ( + operation.destinationPath.startsWith(userSkillRoot) + ))); + assert.ok(!payload.plan.operations.some(operation => ( + operation.destinationPath.startsWith(userSkillRoot) + ))); + assert.strictEqual(fs.readFileSync(userSkillPath, 'utf8'), '# User custom skill\n'); + assert.ok(!fs.existsSync(path.join(homeDir, '.claude', 'ecc', 'install-state.json'))); } finally { cleanup(homeDir); cleanup(projectDir); @@ -893,8 +983,8 @@ function runTests() { const result = run(['--config', configPath], { cwd: projectDir, homeDir }); assert.strictEqual(result.code, 0, result.stderr); - assert.ok(fs.existsSync(path.join(homeDir, '.claude', 'skills', 'ecc', 'security-review', 'SKILL.md'))); - assert.ok(!fs.existsSync(path.join(homeDir, '.claude', 'skills', 'ecc', 'dmux-workflows', 'SKILL.md'))); + assert.ok(fs.existsSync(path.join(homeDir, '.claude', 'skills', 'security-review', 'SKILL.md'))); + assert.ok(!fs.existsSync(path.join(homeDir, '.claude', 'skills', 'dmux-workflows', 'SKILL.md'))); const state = readJson(path.join(homeDir, '.claude', 'ecc', 'install-state.json')); assert.strictEqual(state.request.profile, 'developer'); @@ -925,8 +1015,8 @@ function runTests() { const result = run([], { cwd: projectDir, homeDir }); assert.strictEqual(result.code, 0, result.stderr); - assert.ok(fs.existsSync(path.join(homeDir, '.claude', 'skills', 'ecc', 'security-review', 'SKILL.md'))); - assert.ok(!fs.existsSync(path.join(homeDir, '.claude', 'skills', 'ecc', 'dmux-workflows', 'SKILL.md'))); + assert.ok(fs.existsSync(path.join(homeDir, '.claude', 'skills', 'security-review', 'SKILL.md'))); + assert.ok(!fs.existsSync(path.join(homeDir, '.claude', 'skills', 'dmux-workflows', 'SKILL.md'))); const state = readJson(path.join(homeDir, '.claude', 'ecc', 'install-state.json')); assert.strictEqual(state.request.profile, 'developer');