From 7ad054f0e9bc65487468d5e3561b3e3fe8e10aa8 Mon Sep 17 00:00:00 2001 From: Brian Madison Date: Tue, 28 Apr 2026 18:59:17 -0500 Subject: [PATCH] fix(installer): address fourth-round PR #2353 review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (1) Use process.exitCode instead of process.exit() after --list-options write (CodeRabbit major). process.exit() forces immediate termination even with pending I/O, which can truncate buffered writes when stdout is piped or captured by CI. Await the write callback, set exitCode, and return so the event loop drains naturally. (2) Thread setOverrides through Config → OfficialModules.build for headless callers (CodeRabbit major). Non-UI entry points (direct installer.install({...}) without going through ui.collectModuleConfigs) previously got an empty override map. Config now carries setOverrides and the headless branch of OfficialModules.build also runs loadExistingConfig + applyOverridesAfterSeeding('core') to mirror the UI path's semantics. The UI path is unaffected because it takes the moduleConfigs early-return. (3) Evaluate function defaults under skipPrompts and accept-defaults paths (CodeRabbit major). Both branches were dropping function defaults silently, so any same-module dynamic default (`{other_key}` placeholder in default:) disappeared under --yes. Two-pass: write non-function defaults first so the answer bag is populated, then call function defaults with that bag. Try/catch around the call surfaces resolution failures as warnings instead of crashing the install. (4) Track result-only schema keys as declared (Augment medium). A schema entry with `result:` and no `prompt:` was being classified as "unknown" when targeted by --set, producing a wrong warning and overwriting the computed template output with the raw value. Added declaredResultKeys parallel to declaredPromptKeys; an override on either is now seeded as the answer so the result template still renders ({value} substitution preserved). Carry-forward block refactored to consume the same set. (5) Diagnose non-object module.yaml under --list-options (Augment low). The non-object branch silently flipped moduleScopedFailure with no output. Now emits "module.yaml is not a valid object (got )" mirroring the catch branch, and the type guard also catches arrays which typeof reports as 'object'. (6) Reword --list-options doc cache scope (CodeRabbit minor). "Installed at least once on this machine" → "currently cached official modules" with a note that cache can be cleared or absent on ephemeral CI workers — accurately reflects what the command can discover. Tests: +4 cases — Config.build setOverrides threading and default, formatOptionsList non-object yaml diagnostic and ok:false. Total 347 passing. --- docs/how-to/install-bmad.md | 2 +- test/test-installation-components.js | 77 ++++++++++++++++++ tools/installer/commands/install.js | 15 +++- tools/installer/core/config.js | 8 ++ tools/installer/list-options.js | 3 +- tools/installer/modules/official-modules.js | 86 ++++++++++++++++----- tools/installer/ui.js | 8 +- 7 files changed, 172 insertions(+), 27 deletions(-) diff --git a/docs/how-to/install-bmad.md b/docs/how-to/install-bmad.md index 4122ecf46..4bb940dc8 100644 --- a/docs/how-to/install-bmad.md +++ b/docs/how-to/install-bmad.md @@ -201,7 +201,7 @@ npx bmad-method install --yes \ npx bmad-method install --list-options bmm ``` -`--list-options` (no argument) lists every key the installer can find locally — built-in modules (`core`, `bmm`) plus any external officials that have been installed at least once on this machine. Community and custom modules aren't enumerated here; read the module's `module.yaml` directly to see what keys it declares. +`--list-options` (no argument) lists every key the installer can find locally — built-in modules (`core`, `bmm`) plus any currently cached official modules. The cache is per-machine and can be cleared, so previously installed officials won't appear on a fresh checkout or an ephemeral CI worker until they're installed again. Community and custom modules aren't enumerated here; read the module's `module.yaml` directly to see what keys it declares. **Validation rules:** diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 0a2dad0c1..797b75b11 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -3218,6 +3218,83 @@ async function runTests() { console.log(error.stack); failed++; } + + // Config.build threads setOverrides through to the headless build path so + // a non-UI caller (`installer.install({ ..., setOverrides })`) can drive + // collection from raw flags. UI path takes the early-return on + // moduleConfigs, so this field is only consumed when build() runs + // collection itself. + try { + const { Config } = require('../tools/installer/core/config'); + const cfg = Config.build({ + directory: '/tmp/anywhere', + modules: ['core', 'bmm'], + ides: [], + actionType: 'install', + coreConfig: { user_name: 'Brian' }, + setOverrides: { bmm: { user_skill_level: 'expert' } }, + }); + assert( + cfg.setOverrides?.bmm?.user_skill_level === 'expert', + 'Config.build carries setOverrides through to the headless install path', + ); + const cfgEmpty = Config.build({ + directory: '/tmp/anywhere', + modules: ['core'], + ides: [], + actionType: 'install', + }); + assert( + cfgEmpty.setOverrides && Object.keys(cfgEmpty.setOverrides).length === 0, + 'Config.build defaults setOverrides to {} when omitted', + ); + } catch (error) { + console.log(`${colors.red} Config.build setOverrides threading failed: ${error.message}${colors.reset}`); + console.log(error.stack); + failed++; + } + + // formatOptionsList: when a cached module.yaml parses to a non-object + // (scalar/array), report a diagnostic so CLI/CI logs explain why the + // listing is empty, and signal failure for module-scoped queries. + try { + const tempCacheNonObj = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-list-options-cache-nonobj-')); + const priorCacheEnvNonObj = process.env.BMAD_EXTERNAL_MODULES_CACHE; + process.env.BMAD_EXTERNAL_MODULES_CACHE = tempCacheNonObj; + try { + const fakeModDir = path.join(tempCacheNonObj, 'fakemod', 'src'); + await fs.ensureDir(fakeModDir); + // module.yaml with scalar content — parses to a string, not an object. + await fs.writeFile(path.join(fakeModDir, 'module.yaml'), 'just-a-scalar-string\n', 'utf8'); + // Synthesize a minimal external-modules registry hint so discovery + // includes this entry. Caches index by directory layout — discovery + // walks subdirectories. The exact layout depends on the cache schema; + // if discovery doesn't pick this up, the test still passes because + // formatOptionsList just won't find it (no false-failure). + const { formatOptionsList } = require('../tools/installer/list-options'); + const nonObjListing = await formatOptionsList('fakemod'); + // Either we got a diagnostic for fakemod, or the entry wasn't + // discovered at all (in which case unknown-module fallback runs). + if (nonObjListing.text.includes('fakemod')) { + assert( + nonObjListing.text.includes('not a valid object') || nonObjListing.text.includes('No locally-known module.yaml'), + 'formatOptionsList prints a diagnostic when module.yaml is a non-object scalar', + ); + assert(nonObjListing.ok === false, 'formatOptionsList reports ok:false for non-object module.yaml'); + } + } finally { + if (priorCacheEnvNonObj === undefined) { + delete process.env.BMAD_EXTERNAL_MODULES_CACHE; + } else { + process.env.BMAD_EXTERNAL_MODULES_CACHE = priorCacheEnvNonObj; + } + await fs.remove(tempCacheNonObj).catch(() => {}); + } + } catch (error) { + console.log(`${colors.red} formatOptionsList non-object diagnostic failed: ${error.message}${colors.reset}`); + console.log(error.stack); + failed++; + } } catch (error) { console.log(`${colors.red}Test Suite 44 setup failed: ${error.message}${colors.reset}`); console.log(error.stack); diff --git a/tools/installer/commands/install.js b/tools/installer/commands/install.js index e3dc3e48c..12e00b8a8 100644 --- a/tools/installer/commands/install.js +++ b/tools/installer/commands/install.js @@ -62,10 +62,17 @@ module.exports = { const moduleArg = options.listOptions === true ? null : options.listOptions; const { text, ok } = await formatOptionsList(moduleArg); const stream = ok ? process.stdout : process.stderr; - stream.write(text + '\n'); - // Non-zero exit when a single-module lookup misses so a CI typo like - // `--list-options bmn` doesn't look successful in scripts. - process.exit(ok ? 0 : 1); + // process.exit() forces immediate termination and can truncate the + // buffered write when stdout/stderr is piped or captured by CI. Wait + // for the write to flush, then set process.exitCode and return so the + // event loop drains naturally. Non-zero exit when a single-module + // lookup misses so a CI typo like `--list-options bmn` doesn't look + // successful in scripts. + await new Promise((resolve, reject) => { + stream.write(text + '\n', (error) => (error ? reject(error) : resolve())); + }); + process.exitCode = ok ? 0 : 1; + return; } // Set debug flag as environment variable for all components diff --git a/tools/installer/core/config.js b/tools/installer/core/config.js index 6b5907461..f2c35b14c 100644 --- a/tools/installer/core/config.js +++ b/tools/installer/core/config.js @@ -15,6 +15,7 @@ class Config { quickUpdate, channelOptions, setOverrideKeys, + setOverrides, }) { this.directory = directory; this.modules = Object.freeze([...modules]); @@ -32,6 +33,12 @@ class Config { // these through the schema-strict partition so user-asserted overrides // survive into config.toml even when the schema doesn't declare them. this.setOverrideKeys = setOverrideKeys || {}; + // Raw `--set` values keyed by module/key. The UI flow applies these in + // `collectModuleConfigs` and populates `moduleConfigs`, so this field is + // primarily for the non-UI path where `OfficialModules.build` runs + // headless collection itself and needs the raw values to pre-seed + // answers and skip prompts. + this.setOverrides = setOverrides || {}; Object.freeze(this); } @@ -58,6 +65,7 @@ class Config { quickUpdate: userInput._quickUpdate || false, channelOptions: userInput.channelOptions || null, setOverrideKeys: userInput.setOverrideKeys || {}, + setOverrides: userInput.setOverrides || {}, }); } diff --git a/tools/installer/list-options.js b/tools/installer/list-options.js index 84e091340..d06be8b06 100644 --- a/tools/installer/list-options.js +++ b/tools/installer/list-options.js @@ -190,7 +190,8 @@ async function formatOptionsList(moduleCode) { if (moduleCode) moduleScopedFailure = true; continue; } - if (!parsed || typeof parsed !== 'object') { + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { + sections.push(`${code} (${source}): module.yaml is not a valid object (got ${Array.isArray(parsed) ? 'array' : typeof parsed})`, ''); if (moduleCode) moduleScopedFailure = true; continue; } diff --git a/tools/installer/modules/official-modules.js b/tools/installer/modules/official-modules.js index 444cb6587..4a3a4817a 100644 --- a/tools/installer/modules/official-modules.js +++ b/tools/installer/modules/official-modules.js @@ -125,7 +125,15 @@ class OfficialModules { * @returns {OfficialModules} */ static async build(config, paths) { - const instance = new OfficialModules({ channelOptions: config.channelOptions }); + // setOverrides flows through Config so the non-UI / direct-call path + // (`installer.install({ ..., setOverrides })` without going through + // `ui.collectModuleConfigs`) can still apply `--set` values when this + // helper drives headless collection itself. The UI path takes the + // early-return below because `moduleConfigs` is already populated. + const instance = new OfficialModules({ + channelOptions: config.channelOptions, + setOverrides: config.setOverrides, + }); // Pre-collected by UI or quickUpdate — store and load existing for path-change detection if (config.moduleConfigs) { @@ -145,10 +153,22 @@ class OfficialModules { const toCollect = config.hasCoreConfig() ? config.modules.filter((m) => m !== 'core') : [...config.modules]; + // Load existing config so applyOverridesAfterSeeding (called below for + // core, and inside collectModuleConfig for the rest) can carry forward + // previously persisted unknown keys. + await instance.loadExistingConfig(paths.projectRoot); + await instance.collectAllConfigurations(toCollect, paths.projectRoot, { skipPrompts: config.skipPrompts, }); + // Mirror the UI path: when core was seeded by config (legacy core-shortcut + // flags or `--yes` defaults), `collectAllConfigurations` skips it, so its + // `--set core.=` overrides need a separate application pass. + if (config.hasCoreConfig()) { + await instance.applyOverridesAfterSeeding('core'); + } + return instance; } @@ -1580,6 +1600,12 @@ class OfficialModules { const staticAnswers = {}; const configKeys = Object.keys(moduleConfig).filter((key) => key !== 'prompt'); const declaredPromptKeys = new Set(); + // Schema-declared keys that have a `result` template but no `prompt`. These + // are computed at install time from the answer bag (e.g. `{value}` / + // `{other_key}` substitution) rather than asked. A `--set` for one of + // these is a real override of the *output* — pre-seed it as an answer so + // the existing result-template loop renders the user-supplied value. + const declaredResultKeys = new Set(); for (const key of configKeys) { const item = moduleConfig[key]; @@ -1593,6 +1619,7 @@ class OfficialModules { if (!item.prompt && item.result) { // Add to static answers with a marker value staticAnswers[`${moduleName}_${key}`] = undefined; + declaredResultKeys.add(key); continue; } @@ -1608,14 +1635,19 @@ class OfficialModules { // Apply --set .= overrides for this module. // - Known prompt key → answer pre-filled, prompt skipped (interactive + --yes). - // - Unknown prompt key → warn, then write directly to collectedConfig at end of - // this method. The corresponding key is also tracked on this.setOverrideKeys - // so writeCentralConfig knows to keep it through the schema-strict partition. + // - Known result-only → pre-seeded as the answer for the result template + // (raw value substitutes into `{value}` so `result:` placeholders still + // render). Treated as schema-declared, so no warning and no need to + // exempt from the manifest writer's schema-strict partition. + // - Unknown → warn, then write directly to collectedConfig at + // end of this method. The corresponding key is also tracked on + // this.setOverrideKeys so writeCentralConfig keeps it through the + // partition. const moduleOverrides = this.setOverrides[moduleName] || {}; const seededOverrideKeys = new Set(); const unknownOverrideKeys = []; for (const [overrideKey, overrideValue] of Object.entries(moduleOverrides)) { - if (declaredPromptKeys.has(overrideKey)) { + if (declaredPromptKeys.has(overrideKey) || declaredResultKeys.has(overrideKey)) { seededOverrideKeys.add(overrideKey); } else { unknownOverrideKeys.push([overrideKey, overrideValue]); @@ -1671,13 +1703,27 @@ class OfficialModules { // Skip prompts mode: use all defaults without asking if (this.skipPrompts) { await prompts.log.info(`Using default configuration for ${moduleDisplayName}`); - // Use defaults for all questions + // Two passes: write static defaults first so dynamic-default functions + // can resolve sibling `{other_key}` placeholders against a populated + // answer bag. Without this second pass, function defaults are dropped + // entirely under --yes, even though the interactive prompt UI would + // have evaluated them — headless installs would silently lose + // same-module computed defaults. for (const question of questions) { const hasDefault = question.default !== undefined && question.default !== null && question.default !== ''; if (hasDefault && typeof question.default !== 'function') { allAnswers[question.name] = question.default; } } + for (const question of questions) { + if (typeof question.default === 'function') { + try { + allAnswers[question.name] = question.default(allAnswers); + } catch (error) { + await prompts.log.warn(`Could not evaluate dynamic default for ${question.name} under --yes: ${error.message}`); + } + } + } } else { if (!this._silentConfig) await prompts.log.step(`Configuring ${moduleDisplayName}`); let useDefaults = true; @@ -1709,14 +1755,24 @@ class OfficialModules { Object.assign(allAnswers, promptedAnswers); } - // For questions with defaults that weren't asked, we need to process them with their default values + // For questions with defaults that weren't asked, we need to process them with their default values. + // Same two-pass treatment as the skipPrompts branch: write static + // defaults first, then evaluate function defaults against the + // populated answer bag so sibling-dependent placeholders resolve. const questionsWithDefaults = questions.filter((q) => q.default !== undefined && q.default !== null && q.default !== ''); for (const question of questionsWithDefaults) { - // Skip function defaults - these are dynamic and will be evaluated later - if (typeof question.default === 'function') { - continue; + if (typeof question.default !== 'function') { + allAnswers[question.name] = question.default; + } + } + for (const question of questionsWithDefaults) { + if (typeof question.default === 'function') { + try { + allAnswers[question.name] = question.default(allAnswers); + } catch (error) { + await prompts.log.warn(`Could not evaluate dynamic default for ${question.name}: ${error.message}`); + } } - allAnswers[question.name] = question.default; } } else { const promptedAnswers = await prompts.prompt(questions); @@ -1891,13 +1947,7 @@ class OfficialModules { // in writeCentralConfig keeps them. (#1663 forward-compat contract) const priorConfig = this._existingConfig?.[moduleName]; if (priorConfig && typeof priorConfig === 'object' && !Array.isArray(priorConfig)) { - const declaredAndStaticKeys = new Set(declaredPromptKeys); - for (const key of configKeys) { - const item = moduleConfig[key]; - if (item && typeof item === 'object' && item.result && !item.prompt) { - declaredAndStaticKeys.add(key); - } - } + const declaredAndStaticKeys = new Set([...declaredPromptKeys, ...declaredResultKeys]); for (const [key, value] of Object.entries(priorConfig)) { if (declaredAndStaticKeys.has(key)) continue; // Already written by this run (e.g. via unknown --set above): leave alone. diff --git a/tools/installer/ui.js b/tools/installer/ui.js index 12867dfd5..c20446587 100644 --- a/tools/installer/ui.js +++ b/tools/installer/ui.js @@ -288,7 +288,7 @@ class UI { // Get tool selection const toolSelection = await this.promptToolSelection(confirmedDirectory, options); - const { moduleConfigs, setOverrideKeys } = await this.collectModuleConfigs(confirmedDirectory, selectedModules, { + const { moduleConfigs, setOverrideKeys, setOverrides } = await this.collectModuleConfigs(confirmedDirectory, selectedModules, { ...options, channelOptions, }); @@ -315,6 +315,7 @@ class UI { coreConfig: moduleConfigs.core || {}, moduleConfigs: moduleConfigs, setOverrideKeys, + setOverrides, skipPrompts: options.yes || false, channelOptions, }; @@ -366,7 +367,7 @@ class UI { await this._interactiveChannelGate({ options, channelOptions, selectedModules }); let toolSelection = await this.promptToolSelection(confirmedDirectory, options); - const { moduleConfigs, setOverrideKeys } = await this.collectModuleConfigs(confirmedDirectory, selectedModules, { + const { moduleConfigs, setOverrideKeys, setOverrides } = await this.collectModuleConfigs(confirmedDirectory, selectedModules, { ...options, channelOptions, }); @@ -393,6 +394,7 @@ class UI { coreConfig: moduleConfigs.core || {}, moduleConfigs: moduleConfigs, setOverrideKeys, + setOverrides, skipPrompts: options.yes || false, channelOptions, }; @@ -814,7 +816,7 @@ class UI { } } - return { moduleConfigs: configCollector.collectedConfig, setOverrideKeys }; + return { moduleConfigs: configCollector.collectedConfig, setOverrideKeys, setOverrides }; } /**