From a58aa26a302cdd9e7ac48874e518247efa933eef Mon Sep 17 00:00:00 2001 From: catlog22 Date: Mon, 2 Mar 2026 19:30:34 +0800 Subject: [PATCH] fix(uninstall): add manifest tracking for skill hub installations Fixes #126: ccw uninstall was not cleaning up skills and commands installed via Skill Hub because cpSync() bypassed manifest tracking. Changes: - Add copyDirectoryWithManifest() helper to install.ts and skill-hub-routes.ts - Track all skill files in manifest during Skill Hub installation (CLI and API) - Add orphan cleanup logic to uninstall.ts for defense in depth - Fix installSkillFromRemote() and installSkillFromRemotePath() to track files Root cause: Skill Hub installation methods used cpSync() which did not track files in manifest, causing skills/commands to remain after uninstall. --- ccw/src/commands/install.ts | 73 +++++++++++++++-- ccw/src/commands/uninstall.ts | 98 +++++++++++++++++++++- ccw/src/core/routes/skill-hub-routes.ts | 103 +++++++++++++++++++++--- 3 files changed, 258 insertions(+), 16 deletions(-) diff --git a/ccw/src/commands/install.ts b/ccw/src/commands/install.ts index 4c431835..47b46528 100644 --- a/ccw/src/commands/install.ts +++ b/ccw/src/commands/install.ts @@ -1,4 +1,4 @@ -import { existsSync, mkdirSync, readdirSync, statSync, copyFileSync, readFileSync, writeFileSync, unlinkSync, rmdirSync, appendFileSync, renameSync, cpSync } from 'fs'; +import { existsSync, mkdirSync, readdirSync, statSync, copyFileSync, readFileSync, writeFileSync, unlinkSync, rmdirSync, appendFileSync, renameSync } from 'fs'; import { join, dirname, basename } from 'path'; import { homedir, platform } from 'os'; import { fileURLToPath } from 'url'; @@ -6,7 +6,7 @@ import { execSync } from 'child_process'; import inquirer from 'inquirer'; import chalk from 'chalk'; import { showHeader, createSpinner, info, warning, error, summaryBox, divider } from '../utils/ui.js'; -import { createManifest, addFileEntry, addDirectoryEntry, saveManifest, findManifest, getAllManifests } from '../core/manifest.js'; +import { createManifest, addFileEntry, addDirectoryEntry, saveManifest, findManifest, getAllManifests, type Manifest, type ManifestWithMetadata } from '../core/manifest.js'; import { validatePath } from '../utils/path-resolver.js'; import type { Ora } from 'ora'; @@ -1081,13 +1081,58 @@ function listLocalSkillHubSkills(): Array<{ id: string; name: string; descriptio return result; } +/** + * Copy directory recursively with manifest tracking + * Similar to copyDirectory() but returns file count for skill-hub installation + * @param src - Source directory + * @param dest - Destination directory + * @param manifest - Manifest to track files + * @returns Count of files and directories copied + */ +function copyDirectoryWithManifest( + src: string, + dest: string, + manifest: any +): { files: number; directories: number } { + let files = 0; + let directories = 0; + + // Create destination directory + if (!existsSync(dest)) { + mkdirSync(dest, { recursive: true }); + directories++; + addDirectoryEntry(manifest, dest); + } + + const entries = readdirSync(src); + + for (const entry of entries) { + const srcPath = join(src, entry); + const destPath = join(dest, entry); + const stat = statSync(srcPath); + + if (stat.isDirectory()) { + const result = copyDirectoryWithManifest(srcPath, destPath, manifest); + files += result.files; + directories += result.directories; + } else { + copyFileSync(srcPath, destPath); + files++; + addFileEntry(manifest, destPath); + } + } + + return { files, directories }; +} + /** * Install a skill from skill-hub to CLI skills directory + * Now tracks installed files in manifest for proper uninstall cleanup */ async function installSkillFromHub( skillId: string, cliType: 'claude' | 'codex' -): Promise<{ success: boolean; message: string }> { +): Promise<{ success: boolean; message: string; filesTracked?: number }> { // Only support local skills for now if (!skillId.startsWith('local-')) { return { @@ -1119,10 +1164,26 @@ async function installSkillFromHub( mkdirSync(targetParent, { recursive: true }); } - // Copy skill directory + // Get or create manifest for global installation (skill-hub always installs to home directory) + const installPath = homedir(); + const existingManifest = findManifest(installPath, 'Global'); + + // Use existing manifest or create new one for tracking skill-hub installations + // Note: ManifestWithMetadata extends Manifest, so we can use it directly + const manifest = existingManifest || createManifest('Global', installPath); + + // Copy skill directory with manifest tracking try { - cpSync(skillDir, targetDir, { recursive: true }); - return { success: true, message: `Skill '${skillName}' installed to ${cliType}` }; + const { files } = copyDirectoryWithManifest(skillDir, targetDir, manifest); + + // Save manifest with tracked files + saveManifest(manifest); + + return { + success: true, + message: `Skill '${skillName}' installed to ${cliType}`, + filesTracked: files, + }; } catch (error) { return { success: false, message: `Failed to install: ${(error as Error).message}` }; } diff --git a/ccw/src/commands/uninstall.ts b/ccw/src/commands/uninstall.ts index 2cd0c37d..d0245909 100644 --- a/ccw/src/commands/uninstall.ts +++ b/ccw/src/commands/uninstall.ts @@ -4,7 +4,7 @@ import { homedir, platform } from 'os'; import inquirer from 'inquirer'; import chalk from 'chalk'; import { showBanner, createSpinner, success, info, warning, error, summaryBox, divider } from '../utils/ui.js'; -import { getAllManifests, deleteManifest } from '../core/manifest.js'; +import { getAllManifests, deleteManifest, getFileReferenceCounts } from '../core/manifest.js'; import { removeGitBashFix } from './install.js'; // Global subdirectories that should be protected when Global installation exists @@ -126,6 +126,7 @@ export async function uninstallCommand(options: UninstallOptions): Promise let removedFiles = 0; let removedDirs = 0; let failedFiles: FileEntry[] = []; + let orphanStats = { removed: 0, scanned: 0 }; try { // Remove files first (in reverse order to handle nested files) @@ -202,6 +203,10 @@ export async function uninstallCommand(options: UninstallOptions): Promise } } + // Orphan cleanup: Scan for skills/commands not tracked in any manifest + // This handles files installed by skill-hub that weren't tracked properly + const orphanStats = await cleanupOrphanFiles(selectedManifest.manifest_id); + spinner.succeed('Uninstall complete!'); } catch (err) { @@ -233,6 +238,10 @@ export async function uninstallCommand(options: UninstallOptions): Promise summaryLines.push(chalk.white(`Global files preserved: ${chalk.cyan(skippedFiles.toString())}`)); } + if (orphanStats.removed > 0) { + summaryLines.push(chalk.white(`Orphan files cleaned: ${chalk.magenta(orphanStats.removed.toString())}`)); + } + if (failedFiles.length > 0) { summaryLines.push(chalk.white(`Failed: ${chalk.red(failedFiles.length.toString())}`)); summaryLines.push(''); @@ -281,6 +290,93 @@ export async function uninstallCommand(options: UninstallOptions): Promise console.log(''); } +/** + * Clean up orphan files in skills/commands directories that weren't tracked in manifest + * This handles files installed by skill-hub that bypassed manifest tracking + * @param excludeManifestId - Manifest ID being uninstalled (to exclude from reference check) + * @returns Count of removed orphan files and total scanned + */ +async function cleanupOrphanFiles(excludeManifestId: string): Promise<{ removed: number; scanned: number }> { + let removed = 0; + let scanned = 0; + const home = homedir(); + + // Directories to scan for orphan files + const scanDirs = [ + { base: join(home, '.claude', 'skills'), type: 'skill' }, + { base: join(home, '.claude', 'commands'), type: 'command' }, + { base: join(home, '.codex', 'skills'), type: 'skill' }, + { base: join(home, '.codex', 'commands'), type: 'command' }, + ]; + + // Get file reference counts excluding the manifest being uninstalled + const fileRefs = getFileReferenceCounts(excludeManifestId); + + for (const { base } of scanDirs) { + if (!existsSync(base)) continue; + + try { + // Recursively scan directory for files + const files = scanDirectoryForFiles(base); + + for (const filePath of files) { + scanned++; + + // Check if file is referenced by any remaining manifest + const normalizedPath = filePath.toLowerCase().replace(/\\/g, '/'); + const refs = fileRefs.get(normalizedPath) || []; + + // If not referenced, it's an orphan - remove it + if (refs.length === 0) { + try { + unlinkSync(filePath); + removed++; + } catch { + // Ignore removal errors (file may be in use) + } + } + } + + // Clean up empty directories after orphan removal + await removeEmptyDirs(base); + } catch { + // Ignore scan errors + } + } + + return { removed, scanned }; +} + +/** + * Recursively scan directory for all files + * @param dirPath - Directory to scan + * @returns Array of file paths + */ +function scanDirectoryForFiles(dirPath: string): string[] { + const files: string[] = []; + + if (!existsSync(dirPath)) return files; + + try { + const entries = readdirSync(dirPath); + + for (const entry of entries) { + const fullPath = join(dirPath, entry); + const stat = statSync(fullPath); + + if (stat.isDirectory()) { + files.push(...scanDirectoryForFiles(fullPath)); + } else { + files.push(fullPath); + } + } + } catch { + // Ignore scan errors + } + + return files; +} + /** * Recursively remove empty directories * @param {string} dirPath - Directory path diff --git a/ccw/src/core/routes/skill-hub-routes.ts b/ccw/src/core/routes/skill-hub-routes.ts index db0b7c74..0cddf8ce 100644 --- a/ccw/src/core/routes/skill-hub-routes.ts +++ b/ccw/src/core/routes/skill-hub-routes.ts @@ -11,11 +11,12 @@ * - GET /api/skill-hub/updates - Check for available updates */ -import { readFileSync, existsSync, readdirSync, statSync, mkdirSync, cpSync, rmSync, writeFileSync } from 'fs'; +import { readFileSync, existsSync, readdirSync, statSync, mkdirSync, cpSync, rmSync, writeFileSync, copyFileSync } from 'fs'; import { join, dirname } from 'path'; import { homedir } from 'os'; import { fileURLToPath } from 'url'; import { validatePath as validateAllowedPath } from '../../utils/path-validator.js'; +import { createManifest, addFileEntry, addDirectoryEntry, saveManifest, findManifest, type Manifest } from '../manifest.js'; import type { RouteContext } from './types.js'; // ES Module __dirname equivalent @@ -711,6 +712,49 @@ function removeInstalledSkill(skillId: string, cliType: CliType): void { // Skill Installation Helpers // ============================================================================ +/** + * Copy directory recursively with manifest tracking + * @param src - Source directory + * @param dest - Destination directory + * @param manifest - Manifest to track files + * @returns Count of files and directories copied + */ +function copyDirectoryWithManifest( + src: string, + dest: string, + manifest: Manifest +): { files: number; directories: number } { + let files = 0; + let directories = 0; + + // Create destination directory + if (!existsSync(dest)) { + mkdirSync(dest, { recursive: true }); + directories++; + addDirectoryEntry(manifest, dest); + } + + const entries = readdirSync(src); + + for (const entry of entries) { + const srcPath = join(src, entry); + const destPath = join(dest, entry); + const stat = statSync(srcPath); + + if (stat.isDirectory()) { + const result = copyDirectoryWithManifest(srcPath, destPath, manifest); + files += result.files; + directories += result.directories; + } else { + copyFileSync(srcPath, destPath); + files++; + addFileEntry(manifest, destPath); + } + } + + return { files, directories }; +} + /** * Install skill from local path */ @@ -767,12 +811,22 @@ async function installSkillFromLocal( return { success: false, message: `Skill '${skillName}' already exists in ${cliType}` }; } - // Copy skill directory - cpSync(localPath, targetSkillDir, { recursive: true }); + // Get or create manifest for global installation (skill-hub always installs to home directory) + const installPath = homedir(); + const existingManifest = findManifest(installPath, 'Global'); + + // Use existing manifest or create new one for tracking skill-hub installations + const manifest = existingManifest || createManifest('Global', installPath); + + // Copy skill directory with manifest tracking + const { files } = copyDirectoryWithManifest(localPath, targetSkillDir, manifest); + + // Save manifest with tracked files + saveManifest(manifest); return { success: true, - message: `Skill '${skillName}' installed to ${cliType}`, + message: `Skill '${skillName}' installed to ${cliType} (${files} files tracked)`, installedPath: targetSkillDir, }; } catch (error) { @@ -836,9 +890,21 @@ async function installSkillFromRemote( return { success: false, message: `Skill '${skillName}' already exists in ${cliType}` }; } + // Get or create manifest for global installation + const installPath = homedir(); + const existingManifest = findManifest(installPath, 'Global'); + const manifest = existingManifest || createManifest('Global', installPath); + // Create skill directory and write SKILL.md mkdirSync(targetSkillDir, { recursive: true }); - writeFileSync(join(targetSkillDir, 'SKILL.md'), skillContent, 'utf8'); + addDirectoryEntry(manifest, targetSkillDir); + + const skillMdPath = join(targetSkillDir, 'SKILL.md'); + writeFileSync(skillMdPath, skillContent, 'utf8'); + addFileEntry(manifest, skillMdPath); + + // Save manifest with tracked files + saveManifest(manifest); // Cache the skill locally try { @@ -855,7 +921,7 @@ async function installSkillFromRemote( return { success: true, - message: `Skill '${skillName}' installed to ${cliType}`, + message: `Skill '${skillName}' installed to ${cliType} (1 file tracked)`, installedPath: targetSkillDir, }; } catch (error) { @@ -897,21 +963,40 @@ async function installSkillFromRemotePath( return { success: false, message: `Skill '${skillName}' already exists in ${cliType}` }; } + // Get or create manifest for global installation + const installPath = homedir(); + const existingManifest = findManifest(installPath, 'Global'); + const manifest = existingManifest || createManifest('Global', installPath); + // Create skill directory mkdirSync(targetSkillDir, { recursive: true }); + addDirectoryEntry(manifest, targetSkillDir); // Download entire skill directory console.log(`[SkillHub] Downloading skill directory: ${skillPath}`); const result = await downloadSkillDirectory(skillPath, targetSkillDir); - if (!result.success || result.files.length === 0) { + // Track downloaded files in manifest + let trackedFiles = 0; + if (result.success && result.files.length > 0) { + for (const file of result.files) { + addFileEntry(manifest, file); + trackedFiles++; + } + } else { // Fallback: download only SKILL.md console.log('[SkillHub] Directory download failed, falling back to SKILL.md only'); const skillMdUrl = buildDownloadUrlFromPath(skillPath); const skillContent = await fetchRemoteSkill(skillMdUrl); - writeFileSync(join(targetSkillDir, 'SKILL.md'), skillContent, 'utf8'); + const skillMdPath = join(targetSkillDir, 'SKILL.md'); + writeFileSync(skillMdPath, skillContent, 'utf8'); + addFileEntry(manifest, skillMdPath); + trackedFiles = 1; } + // Save manifest with tracked files + saveManifest(manifest); + // Cache the skill locally try { ensureSkillHubDirs(); @@ -927,7 +1012,7 @@ async function installSkillFromRemotePath( return { success: true, - message: `Skill '${skillName}' installed to ${cliType} (${result.files.length} files)`, + message: `Skill '${skillName}' installed to ${cliType} (${trackedFiles} files tracked)`, installedPath: targetSkillDir, }; } catch (error) {