From 928c585b04e0c9d29f9ee31b7d5f7507520f8075 Mon Sep 17 00:00:00 2001 From: Taras Romaniv Date: Mon, 30 Mar 2026 23:10:41 +0200 Subject: [PATCH] refactor: extract quick update custom source assembly Move quick-update custom module source collection out of Installer and into CustomModules as assembleQuickUpdateSources(). This keeps discoverPaths() focused on consuming prepared install inputs while making the quick-update source assembly step explicit and easier to evolve. Also: - preserve customModules metadata in manifest regeneration for installed modules - drop stale customModules entries when modules are no longer installed - cover manifest preservation and manifest-backed quick-update sources in tests --- test/test-installation-components.js | 113 +++++++++++++++++---- tools/installer/core/installer.js | 86 ++-------------- tools/installer/core/manifest-generator.js | 5 +- tools/installer/modules/custom-modules.js | 105 +++++++++++++++++++ 4 files changed, 210 insertions(+), 99 deletions(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 0ce75bce4..b548cbabe 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -14,7 +14,9 @@ const path = require('node:path'); const os = require('node:os'); const fs = require('fs-extra'); +const { Installer } = require('../tools/installer/core/installer'); const { ManifestGenerator } = require('../tools/installer/core/manifest-generator'); +const { OfficialModules } = require('../tools/installer/modules/official-modules'); const { IdeManager } = require('../tools/installer/ide/manager'); const { clearCache, loadPlatformCodes } = require('../tools/installer/ide/platform-codes'); @@ -130,13 +132,16 @@ async function createCustomModuleManifestFixture() { const fixtureRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-custom-manifest-')); const bmadDir = path.join(fixtureRoot, '_bmad'); const configDir = path.join(bmadDir, '_config'); + const moduleSourceDir = path.join(fixtureRoot, 'test-module-source'); await fs.ensureDir(configDir); + await fs.ensureDir(moduleSourceDir); const minimalAgent = 'p'; await fs.ensureDir(path.join(bmadDir, 'core', 'agents')); await fs.writeFile(path.join(bmadDir, 'core', 'agents', 'test.md'), minimalAgent); await fs.ensureDir(path.join(bmadDir, 'test-module', 'agents')); await fs.writeFile(path.join(bmadDir, 'test-module', 'agents', 'test.md'), minimalAgent); + await fs.writeFile(path.join(moduleSourceDir, 'module.yaml'), ['code: test-module', 'name: Test Module', ''].join('\n')); await fs.writeFile( path.join(configDir, 'manifest.yaml'), @@ -163,14 +168,14 @@ async function createCustomModuleManifestFixture() { 'customModules:', ' - id: test-module', ' name: "Test Module"', - ' sourcePath: "/tmp/test-module-source"', + ` sourcePath: ${JSON.stringify(moduleSourceDir)}`, 'ides:', ' - codex', '', ].join('\n'), ); - return { root: fixtureRoot, bmadDir, manifestPath: path.join(configDir, 'manifest.yaml') }; + return { root: fixtureRoot, bmadDir, manifestPath: path.join(configDir, 'manifest.yaml'), moduleSourceDir }; } /** @@ -386,9 +391,9 @@ async function runTests() { assert( Array.isArray(opencodeInstaller?.legacy_targets) && - ['.opencode/agents', '.opencode/commands', '.opencode/agent', '.opencode/command'].every((legacyTarget) => - opencodeInstaller.legacy_targets.includes(legacyTarget), - ), + ['.opencode/agents', '.opencode/commands', '.opencode/agent', '.opencode/command'].every((legacyTarget) => + opencodeInstaller.legacy_targets.includes(legacyTarget), + ), 'OpenCode installer cleans split legacy agent and command output', ); @@ -1411,8 +1416,8 @@ async function runTests() { } catch (error) { assert(false, 'Pi native skills test succeeds', error.message); } finally { - if (tempProjectDir28) await fs.remove(tempProjectDir28).catch(() => { }); - if (installedBmadDir28) await fs.remove(path.dirname(installedBmadDir28)).catch(() => { }); + if (tempProjectDir28) await fs.remove(tempProjectDir28).catch(() => {}); + if (installedBmadDir28) await fs.remove(path.dirname(installedBmadDir28)).catch(() => {}); } console.log(''); @@ -1559,7 +1564,7 @@ async function runTests() { } catch (error) { assert(false, 'Unified skill scanner test succeeds', error.message); } finally { - if (tempFixture29) await fs.remove(tempFixture29).catch(() => { }); + if (tempFixture29) await fs.remove(tempFixture29).catch(() => {}); } console.log(''); @@ -1626,7 +1631,7 @@ async function runTests() { } catch (error) { assert(false, 'parseSkillMd validation test succeeds', error.message); } finally { - if (tempFixture30) await fs.remove(tempFixture30).catch(() => { }); + if (tempFixture30) await fs.remove(tempFixture30).catch(() => {}); } console.log(''); @@ -1663,8 +1668,8 @@ async function runTests() { } catch (error) { assert(false, 'Skill-format unique count test succeeds', error.message); } finally { - if (collisionProjectDir) await fs.remove(collisionProjectDir).catch(() => { }); - if (collisionFixtureRoot) await fs.remove(collisionFixtureRoot).catch(() => { }); + if (collisionProjectDir) await fs.remove(collisionProjectDir).catch(() => {}); + if (collisionFixtureRoot) await fs.remove(collisionFixtureRoot).catch(() => {}); } console.log(''); @@ -1754,37 +1759,109 @@ async function runTests() { } catch (error) { assert(false, 'Ona native skills test succeeds', error.message); } finally { - if (tempProjectDir32) await fs.remove(tempProjectDir32).catch(() => { }); - if (installedBmadDir32) await fs.remove(path.dirname(installedBmadDir32)).catch(() => { }); + if (tempProjectDir32) await fs.remove(tempProjectDir32).catch(() => {}); + if (installedBmadDir32) await fs.remove(path.dirname(installedBmadDir32)).catch(() => {}); } console.log(''); // ============================================================ - // Suite 33: Main manifest preserves customModules + // Suite 33: Main manifest preserves active customModules only // ============================================================ - console.log(`${colors.yellow}Test Suite 33: Preserve customModules in main manifest${colors.reset}\n`); + console.log(`${colors.yellow}Test Suite 33: Preserve active customModules in main manifest${colors.reset}\n`); let customManifestFixture = null; try { customManifestFixture = await createCustomModuleManifestFixture(); + const yaml = require('yaml'); + const originalManifest = yaml.parse(await fs.readFile(customManifestFixture.manifestPath, 'utf8')); + originalManifest.customModules.push({ + id: 'removed-module', + name: 'Removed Module', + sourcePath: path.join(customManifestFixture.root, 'removed-module-source'), + }); + await fs.writeFile(customManifestFixture.manifestPath, yaml.stringify(originalManifest), 'utf8'); + const generator33 = new ManifestGenerator(); await generator33.generateManifests(customManifestFixture.bmadDir, ['core', 'test-module'], [], { ides: ['codex'] }); - const yaml = require('yaml'); const updatedManifest = yaml.parse(await fs.readFile(customManifestFixture.manifestPath, 'utf8')); const customModule = updatedManifest.customModules?.find((entry) => entry.id === 'test-module'); assert(Array.isArray(updatedManifest.customModules), 'Main manifest keeps customModules array'); assert(customModule !== undefined, 'Main manifest preserves existing custom module entry'); assert( - customModule && customModule.sourcePath === '/tmp/test-module-source', + customModule && customModule.sourcePath === customManifestFixture.moduleSourceDir, 'Main manifest preserves custom module sourcePath', ); + assert( + !updatedManifest.customModules?.some((entry) => entry.id === 'removed-module'), + 'Main manifest drops stale custom module entries', + ); } catch (error) { assert(false, 'Main manifest preserves customModules test succeeds', error.message); } finally { - if (customManifestFixture?.root) await fs.remove(customManifestFixture.root).catch(() => { }); + if (customManifestFixture?.root) await fs.remove(customManifestFixture.root).catch(() => {}); + } + + console.log(''); + + // ============================================================ + // Suite 34: Quick update uses manifest-backed custom sources + // ============================================================ + console.log(`${colors.yellow}Test Suite 34: Quick update uses manifest-backed custom module sources${colors.reset}\n`); + + let quickUpdateFixture = null; + const originalListAvailable34 = OfficialModules.prototype.listAvailable; + const originalLoadExistingConfig34 = OfficialModules.prototype.loadExistingConfig; + const originalCollectModuleConfigQuick34 = OfficialModules.prototype.collectModuleConfigQuick; + try { + quickUpdateFixture = await createCustomModuleManifestFixture(); + const installer34 = new Installer(); + installer34.externalModuleManager.hasModule = async () => false; + installer34.externalModuleManager.listAvailable = async () => []; + + let capturedInstallConfig34 = null; + installer34.install = async (config) => { + capturedInstallConfig34 = config; + return { success: true }; + }; + + OfficialModules.prototype.listAvailable = async function () { + return { modules: [], customModules: [] }; + }; + OfficialModules.prototype.loadExistingConfig = async function () { + this.collectedConfig = this.collectedConfig || {}; + }; + OfficialModules.prototype.collectModuleConfigQuick = async function (moduleName) { + this.collectedConfig = this.collectedConfig || {}; + if (!this.collectedConfig[moduleName]) { + this.collectedConfig[moduleName] = {}; + } + return false; + }; + + await installer34.quickUpdate({ + directory: quickUpdateFixture.root, + skipPrompts: true, + }); + + const customModule34 = capturedInstallConfig34?._customModuleSources?.get('test-module'); + + assert(capturedInstallConfig34 !== null, 'Quick update forwards config to install'); + assert(customModule34 !== undefined, 'Quick update keeps manifest-backed custom module updateable'); + assert(customModule34 && customModule34.cached === false, 'Quick update uses manifest-backed source before cache'); + assert( + customModule34 && customModule34.sourcePath === quickUpdateFixture.moduleSourceDir, + 'Quick update uses preserved manifest sourcePath for custom modules', + ); + } catch (error) { + assert(false, 'Quick update manifest-backed custom source test succeeds', error.message); + } finally { + OfficialModules.prototype.listAvailable = originalListAvailable34; + OfficialModules.prototype.loadExistingConfig = originalLoadExistingConfig34; + OfficialModules.prototype.collectModuleConfigQuick = originalCollectModuleConfigQuick34; + if (quickUpdateFixture?.root) await fs.remove(quickUpdateFixture.root).catch(() => {}); } console.log(''); diff --git a/tools/installer/core/installer.js b/tools/installer/core/installer.js index 7beeb847d..a0ea9a66e 100644 --- a/tools/installer/core/installer.js +++ b/tools/installer/core/installer.js @@ -1144,86 +1144,12 @@ class Installer { const configuredIdes = existingInstall.ides; const projectRoot = path.dirname(bmadDir); - // Get custom module sources: first from manifest, then from --custom-content, then from cache - const customModuleSources = new Map(); - if (existingInstall.customModules) { - for (const customModule of existingInstall.customModules) { - if (!customModule?.id) continue; - - let sourcePath = customModule.sourcePath; - if (sourcePath && sourcePath.startsWith('_config')) { - sourcePath = path.join(bmadDir, sourcePath); - } else if (!sourcePath && customModule.relativePath) { - sourcePath = path.resolve(projectRoot, customModule.relativePath); - } else if (sourcePath && !path.isAbsolute(sourcePath)) { - sourcePath = path.resolve(sourcePath); - } - - if (!sourcePath || !(await fs.pathExists(sourcePath))) { - continue; - } - - customModuleSources.set(customModule.id, { - id: customModule.id, - name: customModule.name || customModule.id, - sourcePath, - relativePath: customModule.relativePath, - cached: false, - }); - } - } - - if (config.customContent?.sources?.length > 0) { - for (const source of config.customContent.sources) { - if (source.id && source.path && (await fs.pathExists(source.path))) { - customModuleSources.set(source.id, { - id: source.id, - name: source.name || source.id, - sourcePath: source.path, - cached: false, // From CLI, will be re-cached - }); - } - } - } - const cacheDir = path.join(bmadDir, '_config', 'custom'); - if (await fs.pathExists(cacheDir)) { - const cachedModules = await fs.readdir(cacheDir, { withFileTypes: true }); - - for (const cachedModule of cachedModules) { - const moduleId = cachedModule.name; - const cachedPath = path.join(cacheDir, moduleId); - - // Skip if path doesn't exist (broken symlink, deleted dir) - avoids lstat ENOENT - if (!(await fs.pathExists(cachedPath))) { - continue; - } - if (!cachedModule.isDirectory()) { - continue; - } - - // Skip if we already have this module from manifest - if (customModuleSources.has(moduleId)) { - continue; - } - - // Check if this is an external official module - skip cache for those - const isExternal = await this.externalModuleManager.hasModule(moduleId); - if (isExternal) { - continue; - } - - // Check if this is actually a custom module (has module.yaml) - const moduleYamlPath = path.join(cachedPath, 'module.yaml'); - if (await fs.pathExists(moduleYamlPath)) { - customModuleSources.set(moduleId, { - id: moduleId, - name: moduleId, - sourcePath: cachedPath, - cached: true, - }); - } - } - } + const customModuleSources = await this.customModules.assembleQuickUpdateSources( + config, + existingInstall, + bmadDir, + this.externalModuleManager, + ); // Get available modules (what we have source for) const availableModulesData = await new OfficialModules().listAvailable(); diff --git a/tools/installer/core/manifest-generator.js b/tools/installer/core/manifest-generator.js index f96081494..bef6f2d23 100644 --- a/tools/installer/core/manifest-generator.js +++ b/tools/installer/core/manifest-generator.js @@ -377,6 +377,7 @@ class ManifestGenerator { */ async writeMainManifest(cfgDir) { const manifestPath = path.join(cfgDir, 'manifest.yaml'); + const installedModuleSet = new Set(this.modules); // Read existing manifest to preserve install date let existingInstallDate = null; @@ -405,7 +406,9 @@ class ManifestGenerator { } if (existingManifest.customModules && Array.isArray(existingManifest.customModules)) { - existingCustomModules = existingManifest.customModules; + // We filter here so manifest regeneration preserves source metadata only for custom modules that + // are still installed. Without that, customModules can retain stale entries for modules that were removed. + existingCustomModules = existingManifest.customModules.filter((customModule) => installedModuleSet.has(customModule?.id)); } } catch { // If we can't read existing manifest, continue with defaults diff --git a/tools/installer/modules/custom-modules.js b/tools/installer/modules/custom-modules.js index b41bf47b1..3f8b793be 100644 --- a/tools/installer/modules/custom-modules.js +++ b/tools/installer/modules/custom-modules.js @@ -192,6 +192,111 @@ class CustomModules { return this.paths; } + + /** + * Assemble quick-update source candidates before install() hands them to discoverPaths(). + * This exists because discoverPaths() consumes already-prepared quick-update sources, + * while quickUpdate() still has to build that source map from manifest, explicit inputs, + * and cache conventions. + * Precedence: manifest-backed paths, explicit sources override them, then cached modules. + * @param {Object} config - Quick update configuration + * @param {Object} existingInstall - Existing installation snapshot + * @param {string} bmadDir - BMAD directory + * @param {Object} externalModuleManager - External module manager + * @returns {Promise>} Map of custom module ID to source info + */ + async assembleQuickUpdateSources(config, existingInstall, bmadDir, externalModuleManager) { + const projectRoot = path.dirname(bmadDir); + const customModuleSources = new Map(); + + if (existingInstall.customModules) { + for (const customModule of existingInstall.customModules) { + // Skip if no ID - can't reliably track or re-cache without it + if (!customModule?.id) continue; + + let sourcePath = customModule.sourcePath; + if (sourcePath && sourcePath.startsWith('_config')) { + // Paths are relative to BMAD dir, but we want absolute paths for install + sourcePath = path.join(bmadDir, sourcePath); + } else if (!sourcePath && customModule.relativePath) { + // Fall back to relativePath + sourcePath = path.resolve(projectRoot, customModule.relativePath); + } else if (sourcePath && !path.isAbsolute(sourcePath)) { + // If we have a sourcePath but it's not absolute, resolve it relative to project root + sourcePath = path.resolve(projectRoot, sourcePath); + } + + // If we still don't have a valid source path, skip this module + if (!sourcePath || !(await fs.pathExists(sourcePath))) { + continue; + } + + customModuleSources.set(customModule.id, { + id: customModule.id, + name: customModule.name || customModule.id, + sourcePath, + relativePath: customModule.relativePath, + cached: false, + }); + } + } + + if (config.customContent?.sources?.length > 0) { + for (const source of config.customContent.sources) { + if (source.id && source.path) { + customModuleSources.set(source.id, { + id: source.id, + name: source.name || source.id, + sourcePath: source.path, + cached: false, // From CLI, will be re-cached + }); + } + } + } + + const cacheDir = path.join(bmadDir, '_config', 'custom'); + if (!(await fs.pathExists(cacheDir))) { + return customModuleSources; + } + + const cachedModules = await fs.readdir(cacheDir, { withFileTypes: true }); + for (const cachedModule of cachedModules) { + const moduleId = cachedModule.name; + const cachedPath = path.join(cacheDir, moduleId); + + // Skip if path doesn't exist (broken symlink, deleted dir) - avoids lstat ENOENT + if (!(await fs.pathExists(cachedPath))) { + continue; + } + if (!cachedModule.isDirectory()) { + continue; + } + + // Skip if we already have this module from manifest + if (customModuleSources.has(moduleId)) { + continue; + } + + // Check if this is an external official module - skip cache for those + const isExternal = await externalModuleManager.hasModule(moduleId); + if (isExternal) { + continue; + } + + // Check if this is actually a custom module (has module.yaml) + const moduleYamlPath = path.join(cachedPath, 'module.yaml'); + if (await fs.pathExists(moduleYamlPath)) { + customModuleSources.set(moduleId, { + id: moduleId, + name: moduleId, + sourcePath: cachedPath, + cached: true, + }); + } + } + + return customModuleSources; + } } module.exports = { CustomModules };