From ff5c5b257a7adf6eab592446407cafb28f405995 Mon Sep 17 00:00:00 2001
From: Brian Madison <bmadcode@gmail.com>
Date: Sat, 25 Apr 2026 21:11:06 -0500
Subject: [PATCH] fix(installer): address PR #2313 review findings

Three issues raised by augmentcode and coderabbit bot reviewers:

1. _removeDeselectedIdes silently swallowed cleanup failures after the
   refactor to cleanupByList. The old per-IDE try/catch logged a warning;
   the new path discarded the result array. Now logs a warning per failed
   ide so failures stay visible.

2. The legacy-dir cleanup hint printed `rm -rf "<path>"/bmad*` which both
   matched bmad-os-* utility skills the user should keep AND missed the
   custom-module skills (e.g. fred-cool-skill) that the new canonical-id
   detection now finds. Findings now carry the exact entry names from the
   scan, and the warning prints one precise rm line per entry.

3. warnPreNativeSkillsLegacy did unguarded fs reads at install start. A
   permission/IO error would have aborted the whole install. Wrapped the
   call site in try/catch so legacy-scan failures only emit a warning.
---
 tools/installer/core/installer.js       | 21 ++++++++++++++++-----
 tools/installer/core/legacy-warnings.js | 10 ++++++++--
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/tools/installer/core/installer.js b/tools/installer/core/installer.js
index abd9ee9a3..a68193bc6 100644
--- a/tools/installer/core/installer.js
+++ b/tools/installer/core/installer.js
@@ -42,10 +42,15 @@ class Installer {
       const officialModules = await OfficialModules.build(config, paths);
       const existingInstall = await ExistingInstall.detect(paths.bmadDir);
 
-      await warnPreNativeSkillsLegacy({
-        projectRoot: paths.projectRoot,
-        existingVersion: existingInstall.installed ? existingInstall.version : null,
-      });
+      try {
+        await warnPreNativeSkillsLegacy({
+          projectRoot: paths.projectRoot,
+          existingVersion: existingInstall.installed ? existingInstall.version : null,
+        });
+      } catch (error) {
+        // Legacy-dir scan is informational; never let it abort install.
+        await prompts.log.warn(`Warning: Could not check for legacy BMAD entries: ${error.message}`);
+      }
 
       if (existingInstall.installed) {
         await this._removeDeselectedModules(existingInstall, config, paths);
@@ -192,9 +197,15 @@ class Installer {
     // Pass the newly-selected list as remainingIdes so cleanupByList skips
     // target_dir wipes for IDEs whose directory is still owned by a peer
     // (e.g. removing 'cursor' while 'gemini' remains — both share .agents/skills).
-    await this.ideManager.cleanupByList(paths.projectRoot, toRemove, {
+    const results = await this.ideManager.cleanupByList(paths.projectRoot, toRemove, {
       remainingIdes: [...newlySelected],
     });
+
+    for (const result of results || []) {
+      if (result && result.success === false) {
+        await prompts.log.warn(`Warning: Failed to remove ${result.ide}: ${result.error || 'unknown error'}`);
+      }
+    }
   }
 
   /**
diff --git a/tools/installer/core/legacy-warnings.js b/tools/installer/core/legacy-warnings.js
index d73bbd9a1..e3098b82b 100644
--- a/tools/installer/core/legacy-warnings.js
+++ b/tools/installer/core/legacy-warnings.js
@@ -92,7 +92,7 @@ async function findStaleLegacyDirs(projectRoot) {
       const entries = await fs.readdir(resolved);
       const bmadEntries = entries.filter((e) => isBmadOwnedEntry(e, canonicalIds));
       if (bmadEntries.length > 0) {
-        findings.push({ path: resolved, displayPath: legacyPath, count: bmadEntries.length });
+        findings.push({ path: resolved, displayPath: legacyPath, count: bmadEntries.length, entries: bmadEntries });
       }
     } catch {
       // Unreadable dir — skip
@@ -127,7 +127,13 @@ async function warnPreNativeSkillsLegacy({ projectRoot, existingVersion } = {})
         `Your AI tool may load these alongside the new skills, causing duplicates. Remove them manually:`,
     );
     for (const finding of staleDirs) {
-      await prompts.log.message(`    rm -rf "${finding.path}"/bmad*    # ${finding.count} stale entr${finding.count === 1 ? 'y' : 'ies'}`);
+      // Print each entry by exact name. A `bmad*` glob would (a) miss
+      // custom-module skills the canonicalId scan now picks up, and
+      // (b) match bmad-os-* utility skills the user should keep.
+      const entries = finding.entries || [];
+      for (const entry of entries) {
+        await prompts.log.message(`    rm -rf "${path.join(finding.path, entry)}"`);
+      }
     }
   } else if (versionTriggered) {
     await prompts.log.message(