From 9654f912d4e13302caab11ead0325c7c4c50b5d1 Mon Sep 17 00:00:00 2001 From: Alex Verkhovsky Date: Sat, 7 Mar 2026 11:34:44 -0700 Subject: [PATCH] fix(installer): address PR review findings from automated reviewers Triage of 18 findings from Augment and CodeRabbit reviews on PR #1841: Source code fixes: - Exclude bmad-os-* from findAncestorConflict to match cleanupTarget - Wrap cleanupCopilotInstructions in try/catch (best-effort, not fatal) - Wrap suspended-platform cleanup in try/catch (failure boundary) - Clean up temp backup dirs in catch block when install aborts - Normalize IDE keys to lowercase before suspended lookup - Delete dead loadCustomInstallerFiles method and stale references - Rename "Roo Cline" to "Roo Code" in both platform-codes.yaml files - Fix Gemini CLI package name (@google/gemini-cli, not @anthropic-ai) Test improvements: - Add name/frontmatter invariant check to 6 missing platform suites - Assert stale bmad-architect skill is removed after cleanup Co-Authored-By: Claude Opus 4.6 --- test/test-installation-components.js | 33 ++++++++++++ tools/cli/installers/lib/core/installer.js | 16 +++++- .../cli/installers/lib/ide/_config-driven.js | 42 ++++++++------- tools/cli/installers/lib/ide/manager.js | 54 +++---------------- .../installers/lib/ide/platform-codes.yaml | 2 +- .../docs/native-skills-migration-checklist.md | 2 +- tools/platform-codes.yaml | 2 +- 7 files changed, 82 insertions(+), 69 deletions(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 4315a2b29..56f37b365 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -495,6 +495,11 @@ async function runTests() { const skillFile9 = path.join(tempProjectDir9, '.claude', 'skills', 'bmad-master', 'SKILL.md'); assert(await fs.pathExists(skillFile9), 'Claude Code install writes SKILL.md directory output'); + // Verify name frontmatter matches directory name + const skillContent9 = await fs.readFile(skillFile9, 'utf8'); + const nameMatch9 = skillContent9.match(/^name:\s*(.+)$/m); + assert(nameMatch9 && nameMatch9[1].trim() === 'bmad-master', 'Claude Code skill name frontmatter matches directory name exactly'); + assert(!(await fs.pathExists(legacyDir9)), 'Claude Code setup removes legacy commands dir'); await fs.remove(tempProjectDir9); @@ -583,6 +588,11 @@ async function runTests() { const skillFile11 = path.join(tempProjectDir11, '.agents', 'skills', 'bmad-master', 'SKILL.md'); assert(await fs.pathExists(skillFile11), 'Codex install writes SKILL.md directory output'); + // Verify name frontmatter matches directory name + const skillContent11 = await fs.readFile(skillFile11, 'utf8'); + const nameMatch11 = skillContent11.match(/^name:\s*(.+)$/m); + assert(nameMatch11 && nameMatch11[1].trim() === 'bmad-master', 'Codex skill name frontmatter matches directory name exactly'); + assert(!(await fs.pathExists(legacyDir11)), 'Codex setup removes legacy prompts dir'); await fs.remove(tempProjectDir11); @@ -668,6 +678,11 @@ async function runTests() { const skillFile13c = path.join(tempProjectDir13c, '.cursor', 'skills', 'bmad-master', 'SKILL.md'); assert(await fs.pathExists(skillFile13c), 'Cursor install writes SKILL.md directory output'); + // Verify name frontmatter matches directory name + const skillContent13c = await fs.readFile(skillFile13c, 'utf8'); + const nameMatch13c = skillContent13c.match(/^name:\s*(.+)$/m); + assert(nameMatch13c && nameMatch13c[1].trim() === 'bmad-master', 'Cursor skill name frontmatter matches directory name exactly'); + assert(!(await fs.pathExists(legacyDir13c)), 'Cursor setup removes legacy commands dir'); await fs.remove(tempProjectDir13c); @@ -1286,6 +1301,11 @@ async function runTests() { const skillFile24 = path.join(tempProjectDir24, '.iflow', 'skills', 'bmad-master', 'SKILL.md'); assert(await fs.pathExists(skillFile24), 'iFlow install writes SKILL.md directory output'); + // Verify name frontmatter matches directory name + const skillContent24 = await fs.readFile(skillFile24, 'utf8'); + const nameMatch24 = skillContent24.match(/^name:\s*(.+)$/m); + assert(nameMatch24 && nameMatch24[1].trim() === 'bmad-master', 'iFlow skill name frontmatter matches directory name exactly'); + assert(!(await fs.pathExists(path.join(tempProjectDir24, '.iflow', 'commands'))), 'iFlow setup removes legacy commands dir'); await fs.remove(tempProjectDir24); @@ -1331,6 +1351,11 @@ async function runTests() { const skillFile25 = path.join(tempProjectDir25, '.qwen', 'skills', 'bmad-master', 'SKILL.md'); assert(await fs.pathExists(skillFile25), 'QwenCoder install writes SKILL.md directory output'); + // Verify name frontmatter matches directory name + const skillContent25 = await fs.readFile(skillFile25, 'utf8'); + const nameMatch25 = skillContent25.match(/^name:\s*(.+)$/m); + assert(nameMatch25 && nameMatch25[1].trim() === 'bmad-master', 'QwenCoder skill name frontmatter matches directory name exactly'); + assert(!(await fs.pathExists(path.join(tempProjectDir25, '.qwen', 'commands'))), 'QwenCoder setup removes legacy commands dir'); await fs.remove(tempProjectDir25); @@ -1387,6 +1412,11 @@ async function runTests() { const skillFile26 = path.join(tempProjectDir26, '.rovodev', 'skills', 'bmad-master', 'SKILL.md'); assert(await fs.pathExists(skillFile26), 'Rovo Dev install writes SKILL.md directory output'); + // Verify name frontmatter matches directory name + const skillContent26 = await fs.readFile(skillFile26, 'utf8'); + const nameMatch26 = skillContent26.match(/^name:\s*(.+)$/m); + assert(nameMatch26 && nameMatch26[1].trim() === 'bmad-master', 'Rovo Dev skill name frontmatter matches directory name exactly'); + assert(!(await fs.pathExists(path.join(tempProjectDir26, '.rovodev', 'workflows'))), 'Rovo Dev setup removes legacy workflows dir'); // Verify prompts.yml cleanup: BMAD entries removed, user entry preserved @@ -1459,6 +1489,9 @@ async function runTests() { const newSkillFile27 = path.join(tempProjectDir27, '.claude', 'skills', 'bmad-master', 'SKILL.md'); assert(await fs.pathExists(newSkillFile27), 'Fresh bmad skills are installed alongside preserved bmad-os-* skills'); + // Stale non-bmad-os skill must have been removed by cleanup + assert(!(await fs.pathExists(regularSkillDir27)), 'Cleanup removes stale non-bmad-os skills'); + await fs.remove(tempProjectDir27); await fs.remove(installedBmadDir27); } catch (error) { diff --git a/tools/cli/installers/lib/core/installer.js b/tools/cli/installers/lib/core/installer.js index 97caedc60..1d9868b60 100644 --- a/tools/cli/installers/lib/core/installer.js +++ b/tools/cli/installers/lib/core/installer.js @@ -713,7 +713,8 @@ class Installer { } // Merge tool selection into config (for both quick update and regular flow) - config.ides = toolSelection.ides; + // Normalize IDE keys to lowercase so they match handler map keys consistently + config.ides = (toolSelection.ides || []).map((ide) => ide.toLowerCase()); config.skipIde = toolSelection.skipIde; const ideConfigurations = toolSelection.configurations; @@ -1354,6 +1355,19 @@ class Installer { } catch { // Ensure the original error is never swallowed by a logging failure } + + // Clean up any temp backup directories that were created before the failure + try { + if (config._tempBackupDir && (await fs.pathExists(config._tempBackupDir))) { + await fs.remove(config._tempBackupDir); + } + if (config._tempModifiedBackupDir && (await fs.pathExists(config._tempModifiedBackupDir))) { + await fs.remove(config._tempModifiedBackupDir); + } + } catch { + // Best-effort cleanup — don't mask the original error + } + throw error; } } diff --git a/tools/cli/installers/lib/ide/_config-driven.js b/tools/cli/installers/lib/ide/_config-driven.js index 325848c77..0a311a68d 100644 --- a/tools/cli/installers/lib/ide/_config-driven.js +++ b/tools/cli/installers/lib/ide/_config-driven.js @@ -793,28 +793,32 @@ LOAD and execute from: {project-root}/{{bmadFolderName}}/{{path}} if (!(await fs.pathExists(filePath))) return; - const content = await fs.readFile(filePath, 'utf8'); - const startIdx = content.indexOf(''); - const endIdx = content.indexOf(''); + try { + const content = await fs.readFile(filePath, 'utf8'); + const startIdx = content.indexOf(''); + const endIdx = content.indexOf(''); - if (startIdx === -1 || endIdx === -1 || endIdx <= startIdx) return; + if (startIdx === -1 || endIdx === -1 || endIdx <= startIdx) return; - const cleaned = content.slice(0, startIdx) + content.slice(endIdx + ''.length); + const cleaned = content.slice(0, startIdx) + content.slice(endIdx + ''.length); - if (cleaned.trim().length === 0) { - await fs.remove(filePath); - const backupPath = `${filePath}.bak`; - if (await fs.pathExists(backupPath)) { - await fs.rename(backupPath, filePath); - if (!options.silent) await prompts.log.message(' Restored copilot-instructions.md from backup'); + if (cleaned.trim().length === 0) { + await fs.remove(filePath); + const backupPath = `${filePath}.bak`; + if (await fs.pathExists(backupPath)) { + await fs.rename(backupPath, filePath); + if (!options.silent) await prompts.log.message(' Restored copilot-instructions.md from backup'); + } + } else { + await fs.writeFile(filePath, cleaned, 'utf8'); + const backupPath = `${filePath}.bak`; + if (await fs.pathExists(backupPath)) await fs.remove(backupPath); } - } else { - await fs.writeFile(filePath, cleaned, 'utf8'); - const backupPath = `${filePath}.bak`; - if (await fs.pathExists(backupPath)) await fs.remove(backupPath); - } - if (!options.silent) await prompts.log.message(' Cleaned BMAD markers from copilot-instructions.md'); + if (!options.silent) await prompts.log.message(' Cleaned BMAD markers from copilot-instructions.md'); + } catch { + if (!options.silent) await prompts.log.warn(' Warning: Could not clean BMAD markers from copilot-instructions.md'); + } } /** @@ -914,7 +918,9 @@ LOAD and execute from: {project-root}/{{bmadFolderName}}/{{path}} try { if (await fs.pathExists(candidatePath)) { const entries = await fs.readdir(candidatePath); - const hasBmad = entries.some((e) => typeof e === 'string' && e.toLowerCase().startsWith('bmad')); + const hasBmad = entries.some( + (e) => typeof e === 'string' && e.toLowerCase().startsWith('bmad') && !e.toLowerCase().startsWith('bmad-os-'), + ); if (hasBmad) { return candidatePath; } diff --git a/tools/cli/installers/lib/ide/manager.js b/tools/cli/installers/lib/ide/manager.js index 7d2330c14..2381bddfa 100644 --- a/tools/cli/installers/lib/ide/manager.js +++ b/tools/cli/installers/lib/ide/manager.js @@ -1,5 +1,3 @@ -const fs = require('fs-extra'); -const path = require('node:path'); const { BMAD_FOLDER_NAME } = require('./shared/path-utils'); const prompts = require('../../../lib/prompts'); @@ -8,8 +6,7 @@ const prompts = require('../../../lib/prompts'); * Dynamically discovers and loads IDE handlers * * Loading strategy: - * All platforms are now config-driven from platform-codes.yaml. - * The custom installer file mechanism is retained for future use but currently has no entries. + * All platforms are config-driven from platform-codes.yaml. */ class IdeManager { constructor() { @@ -43,50 +40,12 @@ class IdeManager { } /** - * Dynamically load all IDE handlers - * 1. Load custom installer files first (kilo.js, rovodev.js) - * 2. Load config-driven handlers from platform-codes.yaml + * Dynamically load all IDE handlers from platform-codes.yaml */ async loadHandlers() { - // Load custom installer files - await this.loadCustomInstallerFiles(); - - // Load config-driven handlers from platform-codes.yaml await this.loadConfigDrivenHandlers(); } - /** - * Load custom installer files (unique installation logic) - * These files have special installation patterns that don't fit the config-driven model - * Note: All custom installers (codex, github-copilot, kilo, rovodev) have been migrated to config-driven (platform-codes.yaml) - */ - async loadCustomInstallerFiles() { - const ideDir = __dirname; - const customFiles = []; - - for (const file of customFiles) { - const filePath = path.join(ideDir, file); - if (!fs.existsSync(filePath)) continue; - - try { - const HandlerModule = require(filePath); - const HandlerClass = HandlerModule.default || Object.values(HandlerModule)[0]; - - if (HandlerClass) { - const instance = new HandlerClass(); - if (instance.name && typeof instance.name === 'string') { - if (typeof instance.setBmadFolderName === 'function') { - instance.setBmadFolderName(this.bmadFolderName); - } - this.handlers.set(instance.name, instance); - } - } - } catch (error) { - await prompts.log.warn(`Warning: Could not load ${file}: ${error.message}`); - } - } - } - /** * Load config-driven handlers from platform-codes.yaml * This creates ConfigDrivenIdeSetup instances for platforms with installer config @@ -98,9 +57,6 @@ class IdeManager { const { ConfigDrivenIdeSetup } = require('./_config-driven'); for (const [platformCode, platformInfo] of Object.entries(platformConfig.platforms)) { - // Skip if already loaded by custom installer - if (this.handlers.has(platformCode)) continue; - // Skip if no installer config (platform may not need installation) if (!platformInfo.installer) continue; @@ -189,7 +145,11 @@ class IdeManager { } // Still clean up legacy artifacts so old broken configs don't linger if (typeof handler.cleanup === 'function') { - await handler.cleanup(projectDir, { silent: true }); + try { + await handler.cleanup(projectDir, { silent: true }); + } catch { + // Best-effort cleanup — don't let stale files block the suspended result + } } return { success: false, ide: ideName, error: 'suspended' }; } diff --git a/tools/cli/installers/lib/ide/platform-codes.yaml b/tools/cli/installers/lib/ide/platform-codes.yaml index efef85579..82d45f562 100644 --- a/tools/cli/installers/lib/ide/platform-codes.yaml +++ b/tools/cli/installers/lib/ide/platform-codes.yaml @@ -205,7 +205,7 @@ platforms: skill_format: true roo: - name: "Roo Cline" + name: "Roo Code" preferred: false category: ide description: "Enhanced Cline fork" diff --git a/tools/docs/native-skills-migration-checklist.md b/tools/docs/native-skills-migration-checklist.md index 5f1bc76a4..2f0f31344 100644 --- a/tools/docs/native-skills-migration-checklist.md +++ b/tools/docs/native-skills-migration-checklist.md @@ -221,7 +221,7 @@ Support assumption: full Agent Skills support. BMAD currently uses a custom inst Support assumption: full Agent Skills support. Gemini CLI docs confirm workspace skills at `.gemini/skills/` and user skills at `~/.gemini/skills/`. Also discovers `.agents/skills/` as an alias. BMAD previously installed TOML files to `.gemini/commands`. -**Install:** `npm install -g @anthropic-ai/gemini-cli` or see [geminicli.com](https://geminicli.com) +**Install:** `npm install -g @google/gemini-cli` or see [geminicli.com](https://geminicli.com) - [x] Confirm Gemini CLI native skills path is `.gemini/skills/{skill-name}/SKILL.md` (per [geminicli.com/docs/cli/skills](https://geminicli.com/docs/cli/skills/)) - [x] Implement native skills output — target_dir `.gemini/skills`, skill_format true, template_type default (replaces TOML templates) diff --git a/tools/platform-codes.yaml b/tools/platform-codes.yaml index 97846a9bd..7458143e7 100644 --- a/tools/platform-codes.yaml +++ b/tools/platform-codes.yaml @@ -50,7 +50,7 @@ platforms: description: "AI development tool" roo: - name: "Roo Cline" + name: "Roo Code" preferred: false category: ide description: "Enhanced Cline fork"