From 3cdee7913c852eddb01e1ef298926c6fed502b5f Mon Sep 17 00:00:00 2001 From: Dicky Moore Date: Mon, 18 May 2026 13:09:52 +0100 Subject: [PATCH] fix(installer): retain stale modules during updates --- tools/installer/core/installer.js | 121 ++++++++++++++++----- tools/installer/core/manifest-generator.js | 58 ---------- tools/installer/ide/_config-driven.js | 4 +- tools/installer/ui.js | 67 +++++++----- 4 files changed, 139 insertions(+), 111 deletions(-) diff --git a/tools/installer/core/installer.js b/tools/installer/core/installer.js index 3bdbec188..9347e1f0b 100644 --- a/tools/installer/core/installer.js +++ b/tools/installer/core/installer.js @@ -76,25 +76,23 @@ class Installer { const results = []; const addResult = (step, status, detail = '', meta = {}) => results.push({ step, status, detail, ...meta }); - // Capture previously installed skill IDs before they get overwritten - const previousSkillIds = new Set(); - const prevCsvPath = path.join(paths.bmadDir, '_config', 'skill-manifest.csv'); - 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}`); - } - } + // Capture previously installed skill rows before they get overwritten + const preservedModules = originalConfig._preserveModules || []; + const previousSkillManifestRows = await this._readSkillManifestRows(paths.bmadDir); + const previousSkillIds = this._getPreviousSkillIdsForCleanup(previousSkillManifestRows, preservedModules); const allModules = config.modules || []; - await this._installAndConfigure(config, originalConfig, paths, allModules, allModules, addResult, officialModules); + await this._installAndConfigure( + config, + originalConfig, + paths, + allModules, + allModules, + addResult, + officialModules, + previousSkillManifestRows, + ); await this._setupIdes(config, allModules, paths, addResult, previousSkillIds); @@ -213,7 +211,16 @@ class Installer { /** * Install modules, create directories, generate configs and manifests. */ - async _installAndConfigure(config, originalConfig, paths, officialModuleIds, allModules, addResult, officialModules) { + async _installAndConfigure( + config, + originalConfig, + paths, + officialModuleIds, + allModules, + addResult, + officialModules, + previousSkillManifestRows = [], + ) { const isQuickUpdate = config.isQuickUpdate(); const moduleConfigs = officialModules.moduleConfigs; @@ -292,25 +299,29 @@ class Installer { message('Generating manifests...'); const manifestGen = new ManifestGenerator(); + const preservedModules = originalConfig._preserveModules || []; const allModulesForManifest = config.isQuickUpdate() ? originalConfig._existingModules || allModules || [] - : originalConfig._preserveModules - ? [...allModules, ...originalConfig._preserveModules] + : preservedModules.length > 0 + ? [...allModules, ...preservedModules] : allModules || []; let modulesForCsvPreserve; if (config.isQuickUpdate()) { modulesForCsvPreserve = originalConfig._existingModules || allModules || []; } else { - modulesForCsvPreserve = originalConfig._preserveModules ? [...allModules, ...originalConfig._preserveModules] : allModules; + modulesForCsvPreserve = preservedModules.length > 0 ? [...allModules, ...preservedModules] : allModules; } + await this._trackPreservedModuleFiles(paths.bmadDir, preservedModules); + await manifestGen.generateManifests(paths.bmadDir, allModulesForManifest, [...this.installedFiles], { ides: config.ides || [], preservedModules: modulesForCsvPreserve, moduleConfigs, }); + await this._appendPreservedSkillManifestRows(paths.bmadDir, previousSkillManifestRows, preservedModules); // Apply post-install --set TOML patches. Runs after writeCentralConfig // (inside generateManifests above) so the patch operates on the @@ -412,6 +423,62 @@ class Installer { } } + async _readSkillManifestRows(bmadDir) { + const csvPath = path.join(bmadDir, '_config', 'skill-manifest.csv'); + if (!(await fs.pathExists(csvPath))) return []; + + try { + const csvParse = require('csv-parse/sync'); + const content = await fs.readFile(csvPath, 'utf8'); + return csvParse.parse(content, { columns: true, skip_empty_lines: true }); + } catch (error) { + await prompts.log.warn(`Failed to parse skill-manifest.csv: ${error.message}`); + return []; + } + } + + _getPreviousSkillIdsForCleanup(previousRows, preservedModules = []) { + const preservedModuleSet = new Set(preservedModules || []); + const ids = new Set(); + for (const row of previousRows || []) { + if (row.canonicalId && !preservedModuleSet.has(row.module)) { + ids.add(row.canonicalId); + } + } + return ids; + } + + async _appendPreservedSkillManifestRows(bmadDir, previousRows, preservedModules = []) { + if (!previousRows || previousRows.length === 0 || preservedModules.length === 0) return; + + const preservedModuleSet = new Set(preservedModules); + const rowsToPreserve = previousRows.filter((row) => row.canonicalId && row.module && preservedModuleSet.has(row.module)); + if (rowsToPreserve.length === 0) return; + + const csvPath = path.join(bmadDir, '_config', 'skill-manifest.csv'); + if (!(await fs.pathExists(csvPath))) return; + + const currentRows = await this._readSkillManifestRows(bmadDir); + const activeIds = new Set(currentRows.map((row) => row.canonicalId).filter(Boolean)); + const appendedRows = []; + + for (const row of rowsToPreserve) { + if (activeIds.has(row.canonicalId)) continue; + activeIds.add(row.canonicalId); + appendedRows.push( + [row.canonicalId, row.name || row.canonicalId, row.description || '', row.module, row.path || ''] + .map((field) => this.escapeCSVField(field)) + .join(','), + ); + } + + if (appendedRows.length === 0) return; + + const currentContent = await fs.readFile(csvPath, 'utf8'); + const prefix = currentContent.endsWith('\n') ? currentContent : `${currentContent}\n`; + await fs.writeFile(csvPath, prefix + appendedRows.join('\n') + '\n', 'utf8'); + } + /** * Restore custom and modified files that were backed up before the update. * No-op for fresh installs (updateState is null). @@ -598,6 +665,15 @@ class Installer { } } + async _trackPreservedModuleFiles(bmadDir, preservedModules = []) { + for (const moduleName of preservedModules) { + const modulePath = path.join(bmadDir, moduleName); + if (await fs.pathExists(modulePath)) { + await this._trackFilesRecursive(modulePath); + } + } + } + /** * Install official (non-custom) modules. * @param {Object} config - Installation configuration @@ -1270,11 +1346,6 @@ class Installer { } } - for (const moduleName of skippedModules) { - if (quickModules.collectedConfig[moduleName] || !quickModules.existingConfig?.[moduleName]) continue; - quickModules.collectedConfig[moduleName] = { ...quickModules.existingConfig[moduleName] }; - } - if (!promptedForNewFields) { await prompts.log.success('All configuration is up to date, no new options to configure'); } diff --git a/tools/installer/core/manifest-generator.js b/tools/installer/core/manifest-generator.js index dd5f2574b..f7b5d0084 100644 --- a/tools/installer/core/manifest-generator.js +++ b/tools/installer/core/manifest-generator.js @@ -435,14 +435,12 @@ class ManifestGenerator { // this means user-scoped keys (e.g. user_name) could mis-file into the // team config, so the operator should notice. const scopeByModuleKey = {}; - const unresolvedModuleNames = new Set(); // Maps installer moduleName (may be full display name) → module code field // from module.yaml, so TOML sections use [modules.] not [modules.]. const codeByModuleName = {}; for (const moduleName of this.updatedModules) { const moduleYamlPath = await resolveInstalledModuleYaml(moduleName); if (!moduleYamlPath) { - unresolvedModuleNames.add(moduleName); console.warn( `[warn] writeCentralConfig: could not locate module.yaml for '${moduleName}'. ` + `Answers from this module will default to team scope — user-scoped keys may mis-file into config.toml.`, @@ -460,7 +458,6 @@ class ManifestGenerator { } } } catch (error) { - unresolvedModuleNames.add(moduleName); console.warn( `[warn] writeCentralConfig: could not parse module.yaml for '${moduleName}' (${error.message}). ` + `Answers from this module will default to team scope — user-scoped keys may mis-file into config.toml.`, @@ -547,35 +544,9 @@ class ManifestGenerator { userLines.push(''); } - const updatedModuleSet = new Set(this.updatedModules); - const preservedModuleBlocks = { team: [], user: [] }; - const preservedModuleCodes = new Set(); - if (updatedModuleSet.size > 0) { - const collectPreservedModuleBlocks = async (filePath, target) => { - if (!(await fs.pathExists(filePath))) return; - try { - const prev = await fs.readFile(filePath, 'utf8'); - for (const block of extractModuleBlocks(prev)) { - if (!updatedModuleSet.has(block.code)) continue; - if (!unresolvedModuleNames.has(block.code) && moduleConfigs[block.code] && Object.keys(moduleConfigs[block.code]).length > 0) - continue; - preservedModuleCodes.add(block.code); - target.push(block.body); - } - } catch (error) { - console.warn( - `[warn] writeCentralConfig: could not read prior ${path.basename(filePath)} to preserve module config: ${error.message}`, - ); - } - }; - await collectPreservedModuleBlocks(teamPath, preservedModuleBlocks.team); - await collectPreservedModuleBlocks(userPath, preservedModuleBlocks.user); - } - // [modules.] — split per module for (const moduleName of this.updatedModules) { if (moduleName === 'core') continue; - if (unresolvedModuleNames.has(moduleName) && preservedModuleCodes.has(moduleName)) continue; const cfg = moduleConfigs[moduleName]; if (!cfg || Object.keys(cfg).length === 0) continue; // Use the module's code field from module.yaml as the TOML key so the @@ -603,13 +574,6 @@ class ManifestGenerator { } } - for (const body of preservedModuleBlocks.team) { - teamLines.push(body, ''); - } - for (const body of preservedModuleBlocks.user) { - userLines.push(body, ''); - } - // [agents.] — always team (agent roster is organizational). // Freshly collected agents come from module.yaml this run. If a module // was preserved (e.g. during quickUpdate when its source isn't available), @@ -892,26 +856,4 @@ function extractAgentBlocks(tomlContent) { return blocks; } -function extractModuleBlocks(tomlContent) { - const lines = tomlContent.split(/\r?\n/); - const blocks = []; - for (let i = 0; i < lines.length; i++) { - const startMatch = lines[i].match(/^\[modules\.([^\]]+)\]\s*$/); - if (!startMatch) continue; - const code = startMatch[1]; - const bodyLines = [lines[i]]; - i++; - while (i < lines.length && !/^\[[^\]]+\]\s*$/.test(lines[i])) { - bodyLines.push(lines[i]); - i++; - } - i--; - while (bodyLines.length > 0 && bodyLines.at(-1).trim() === '') { - bodyLines.pop(); - } - blocks.push({ code, body: bodyLines.join('\n') }); - } - return blocks; -} - module.exports = { ManifestGenerator }; diff --git a/tools/installer/ide/_config-driven.js b/tools/installer/ide/_config-driven.js index bf6fffbc5..baafef1a6 100644 --- a/tools/installer/ide/_config-driven.js +++ b/tools/installer/ide/_config-driven.js @@ -501,7 +501,7 @@ class ConfigDrivenIdeSetup { // Build removal set: previously installed skills + removals.txt entries let removalSet; - if (options.previousSkillIds && options.previousSkillIds.size > 0) { + if (options.previousSkillIds) { // Install/update flow: use pre-captured skill IDs (before manifest was overwritten) removalSet = new Set(options.previousSkillIds); if (resolvedBmadDir) { @@ -547,7 +547,7 @@ class ConfigDrivenIdeSetup { // previousSkillIds — full uninstall or per-IDE removal via // cleanupByList), don't spare anything; the IDE itself is going away, // so its pointers should go with it. - const isInstallFlow = options.previousSkillIds && options.previousSkillIds.size > 0; + const isInstallFlow = !!options.previousSkillIds; const activeSkillIds = isInstallFlow ? await this._readActiveSkillIds(resolvedBmadDir) : new Set(); const extension = this.installerConfig.commands_extension || '.md'; await this.cleanupCommandPointers( diff --git a/tools/installer/ui.js b/tools/installer/ui.js index 1eb004db9..a107fb0fc 100644 --- a/tools/installer/ui.js +++ b/tools/installer/ui.js @@ -7,7 +7,6 @@ const { CLIUtils } = require('./cli-utils'); const { ExternalModuleManager } = require('./modules/external-manager'); const { resolveModuleVersion } = require('./modules/version-resolver'); const { Manifest } = require('./core/manifest'); -const { resolveInstalledModuleYaml } = require('./project-root'); const { parseChannelOptions, buildPlan, @@ -111,21 +110,42 @@ async function getModuleVersion(moduleCode, { repoUrl = null, registryDefault = * UI utilities for the installer */ class UI { - async _findUnavailableInstalledModules(installedModuleIds = new Set(), bmadDir) { - const unavailable = []; - for (const moduleId of installedModuleIds) { - if (moduleId === 'core' || moduleId === 'bmm') continue; - const moduleYamlPath = await resolveInstalledModuleYaml(moduleId); - if (moduleYamlPath) continue; + async _retainUnavailableInstalledModules(selectedModules, installedModuleIds, bmadDir, options = {}) { + const { OfficialModules } = require('./modules/official-modules'); + const officialCodes = new Set(['core']); - const { CustomModuleManager } = require('./modules/custom-module-manager'); - const customMgr = new CustomModuleManager(); - const localSource = await customMgr.findModuleSourceByCode(moduleId, { bmadDir }); - if (localSource) continue; - - unavailable.push(moduleId); + const builtInModules = (await new OfficialModules().listAvailable()).modules || []; + for (const mod of builtInModules) { + officialCodes.add(mod.id); } - return unavailable; + + const externalManager = new ExternalModuleManager(); + const registryModules = await externalManager.listAvailable(); + for (const mod of registryModules) { + officialCodes.add(mod.code); + } + + const { CustomModuleManager } = require('./modules/custom-module-manager'); + const customMgr = new CustomModuleManager(); + const selectedSet = new Set(selectedModules); + const preserveModules = []; + + for (const moduleId of installedModuleIds) { + if (moduleId === 'core') continue; + if (!selectedSet.has(moduleId) && !options.preserveUnselected) continue; + if (officialCodes.has(moduleId)) continue; + + const customSource = await customMgr.findModuleSourceByCode(moduleId, { bmadDir }); + if (!customSource) { + preserveModules.push(moduleId); + } + } + + const preservedSet = new Set(preserveModules); + return { + selectedModules: selectedModules.filter((moduleId) => !preservedSet.has(moduleId)), + preserveModules, + }; } /** @@ -291,12 +311,15 @@ class UI { selectedModules.unshift('core'); } - const preservedModules = await this._findUnavailableInstalledModules(installedModuleIds, bmadDir); + const retainedModuleResult = await this._retainUnavailableInstalledModules(selectedModules, installedModuleIds, bmadDir, { + preserveUnselected: options.yes && !options.modules, + }); + selectedModules = retainedModuleResult.selectedModules; + const preservedModules = retainedModuleResult.preserveModules; + if (preservedModules.length > 0) { - const preservedSet = new Set(preservedModules); - selectedModules = selectedModules.filter((moduleName) => !preservedSet.has(moduleName)); await prompts.log.warn( - `Retaining ${preservedModules.length} installed module(s) with no source available: ${preservedModules.join(', ')}`, + `Retaining ${preservedModules.length} installed module(s) with no available source: ${preservedModules.join(', ')}`, ); } @@ -319,14 +342,6 @@ class UI { ...options, channelOptions, }); - if (preservedModules.length > 0) { - const preservedConfigLoader = new (require('./modules/official-modules').OfficialModules)({ channelOptions }); - await preservedConfigLoader.loadExistingConfig(confirmedDirectory); - for (const moduleName of preservedModules) { - if (moduleConfigs[moduleName] || !preservedConfigLoader.existingConfig?.[moduleName]) continue; - moduleConfigs[moduleName] = { ...preservedConfigLoader.existingConfig[moduleName] }; - } - } // Warn about --pin/--next flags that refer to modules the user didn't // select, or that target bundled modules (core/bmm) where channel