From 3c155e3cbc26642317e77a5cfbeff0c48df0ee03 Mon Sep 17 00:00:00 2001 From: Brian Madison Date: Tue, 7 Apr 2026 01:49:33 -0500 Subject: [PATCH] fix(installer): address Augment review findings - Fix plugins[0] fragility: extract highest version across all plugins in marketplace.json instead of assuming first entry (ui.js, installer.js, manifest.js) - Fix _readMarketplaceVersion ignoring moduleSourcePath: custom modules can now source their own marketplace.json by walking up from source path - Hard-exclude bmad-os-* utility skills in both surgical and legacy cleanup modes, preventing accidental deletion if tracked in manifests - Distinguish missing file vs parse error in skill-manifest.csv reading: warn on corrupt CSV instead of silently skipping cleanup --- tools/installer/core/installer.js | 23 +++++++++++++++----- tools/installer/core/manifest.js | 30 +++++++++++++++++++++++---- tools/installer/ide/_config-driven.js | 5 ++++- tools/installer/ui.js | 19 ++++++++++++++++- 4 files changed, 66 insertions(+), 11 deletions(-) diff --git a/tools/installer/core/installer.js b/tools/installer/core/installer.js index 14bb2ac9c..d75355d72 100644 --- a/tools/installer/core/installer.js +++ b/tools/installer/core/installer.js @@ -39,7 +39,7 @@ class Installer { if (await fs.pathExists(marketplacePath)) { try { const data = JSON.parse(await fs.readFile(marketplacePath, 'utf8')); - return data.plugins?.[0]?.version || ''; + return this._extractMarketplaceVersion(data); } catch { return ''; } @@ -51,6 +51,19 @@ class Installer { return ''; } + /** + * Extract the highest version from marketplace.json plugins array + */ + _extractMarketplaceVersion(data) { + const plugins = data?.plugins; + if (!Array.isArray(plugins) || plugins.length === 0) return ''; + let best = ''; + for (const p of plugins) { + if (p.version && (!best || p.version > best)) best = p.version; + } + return best; + } + /** * Main installation method * @param {Object} config - Installation configuration @@ -95,17 +108,17 @@ class Installer { // Capture previously installed skill IDs before they get overwritten const previousSkillIds = new Set(); const prevCsvPath = path.join(paths.bmadDir, '_config', 'skill-manifest.csv'); - try { - if (await fs.pathExists(prevCsvPath)) { + if (await fs.pathExists(prevCsvPath)) { + try { const csvParse = require('csv-parse/sync'); const content = await fs.readFile(prevCsvPath, 'utf8'); const records = csvParse.parse(content, { columns: true, skip_empty_lines: true }); for (const r of records) { if (r.canonicalId) previousSkillIds.add(r.canonicalId); } + } catch (error) { + await prompts.log.warn(`Failed to parse skill-manifest.csv: ${error.message}`); } - } catch { - // No previous manifest - fresh install } await this._cacheCustomModules(paths, addResult); diff --git a/tools/installer/core/manifest.js b/tools/installer/core/manifest.js index 0fa350279..3da5ad7d3 100644 --- a/tools/installer/core/manifest.js +++ b/tools/installer/core/manifest.js @@ -841,7 +841,7 @@ class Manifest { const yaml = require('yaml'); // All module versions come from .claude-plugin/marketplace.json - const version = await this._readMarketplaceVersion(moduleName); + const version = await this._readMarketplaceVersion(moduleName, moduleSourcePath); // Determine source type if (['core', 'bmm'].includes(moduleName)) { @@ -900,13 +900,29 @@ class Manifest { * @param {string} moduleName - Module code * @returns {string|null} Version or null */ - async _readMarketplaceVersion(moduleName) { + async _readMarketplaceVersion(moduleName, moduleSourcePath = null) { const os = require('node:os'); let marketplacePath; if (['core', 'bmm'].includes(moduleName)) { marketplacePath = path.join(getProjectRoot(), '.claude-plugin', 'marketplace.json'); - } else { + } else if (moduleSourcePath) { + // Walk up from source path to find marketplace.json + let dir = moduleSourcePath; + for (let i = 0; i < 5; i++) { + const candidate = path.join(dir, '.claude-plugin', 'marketplace.json'); + if (await fs.pathExists(candidate)) { + marketplacePath = candidate; + break; + } + const parent = path.dirname(dir); + if (parent === dir) break; + dir = parent; + } + } + + // Fallback to external module cache + if (!marketplacePath) { const cacheDir = path.join(os.homedir(), '.bmad', 'cache', 'external-modules', moduleName); marketplacePath = path.join(cacheDir, '.claude-plugin', 'marketplace.json'); } @@ -914,7 +930,13 @@ class Manifest { try { if (await fs.pathExists(marketplacePath)) { const data = JSON.parse(await fs.readFile(marketplacePath, 'utf8')); - return data.plugins?.[0]?.version || null; + const plugins = data?.plugins; + if (!Array.isArray(plugins) || plugins.length === 0) return null; + let best = null; + for (const p of plugins) { + if (p.version && (!best || p.version > best)) best = p.version; + } + return best; } } catch { // ignore diff --git a/tools/installer/ide/_config-driven.js b/tools/installer/ide/_config-driven.js index 6bb48af2d..ec7dcaad6 100644 --- a/tools/installer/ide/_config-driven.js +++ b/tools/installer/ide/_config-driven.js @@ -428,8 +428,11 @@ class ConfigDrivenIdeSetup { for (const entry of entries) { if (!entry || typeof entry !== 'string') continue; + // Always preserve bmad-os-* utility skills regardless of cleanup mode + if (entry.startsWith('bmad-os-')) continue; + // Surgical removal from set, or legacy prefix matching when set is null - const shouldRemove = removalSet ? removalSet.has(entry) : entry.startsWith('bmad') && !entry.startsWith('bmad-os-'); + const shouldRemove = removalSet ? removalSet.has(entry) : entry.startsWith('bmad'); if (shouldRemove) { try { diff --git a/tools/installer/ui.js b/tools/installer/ui.js index 85f9d07c9..cccf219cc 100644 --- a/tools/installer/ui.js +++ b/tools/installer/ui.js @@ -23,7 +23,7 @@ async function getMarketplaceVersion(moduleCode) { try { if (await fs.pathExists(marketplacePath)) { const data = JSON.parse(await fs.readFile(marketplacePath, 'utf8')); - return data.plugins?.[0]?.version || ''; + return _extractMarketplaceVersion(data); } } catch { // ignore @@ -31,6 +31,23 @@ async function getMarketplaceVersion(moduleCode) { return ''; } +/** + * Extract the highest version from marketplace.json plugins array. + * Handles multiple plugins per file safely. + * @param {Object} data - Parsed marketplace.json + * @returns {string} Version string or empty string + */ +function _extractMarketplaceVersion(data) { + const plugins = data?.plugins; + if (!Array.isArray(plugins) || plugins.length === 0) return ''; + // Use the highest version across all plugins in the file + let best = ''; + for (const p of plugins) { + if (p.version && (!best || p.version > best)) best = p.version; + } + return best; +} + // Separator class for visual grouping in select/multiselect prompts // Note: @clack/prompts doesn't support separators natively, they are filtered out class Separator {