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.
2026-03-03 15:43:11 +08:00 · 2026-03-02 19:30:34 +08:00
parent 2dce4b3e8f
commit a58aa26a30
3 changed files with 258 additions and 16 deletions
--- 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}` };
  }
--- 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<void>
  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<void>
      }
    }

+    // 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<void>
    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<void>
  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
--- 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) {