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 <noreply@anthropic.com>
This commit is contained in:
Alex Verkhovsky
parent
e0a8a5a9a1
commit
9654f912d4
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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('<!-- BMAD:START -->');
|
||||
const endIdx = content.indexOf('<!-- BMAD:END -->');
|
||||
try {
|
||||
const content = await fs.readFile(filePath, 'utf8');
|
||||
const startIdx = content.indexOf('<!-- BMAD:START -->');
|
||||
const endIdx = content.indexOf('<!-- BMAD:END -->');
|
||||
|
||||
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 + '<!-- BMAD:END -->'.length);
|
||||
const cleaned = content.slice(0, startIdx) + content.slice(endIdx + '<!-- BMAD:END -->'.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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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' };
|
||||
}
|
||||
|
|
|
|||
|
|
@ -205,7 +205,7 @@ platforms:
|
|||
skill_format: true
|
||||
|
||||
roo:
|
||||
name: "Roo Cline"
|
||||
name: "Roo Code"
|
||||
preferred: false
|
||||
category: ide
|
||||
description: "Enhanced Cline fork"
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -50,7 +50,7 @@ platforms:
|
|||
description: "AI development tool"
|
||||
|
||||
roo:
|
||||
name: "Roo Cline"
|
||||
name: "Roo Code"
|
||||
preferred: false
|
||||
category: ide
|
||||
description: "Enhanced Cline fork"
|
||||
|
|
|
|||
Loading…
Reference in New Issue