From ce12cc1a7f39328bafab52b247d8f7c5bcc7c005 Mon Sep 17 00:00:00 2001 From: Brian Madison Date: Tue, 28 Apr 2026 11:24:51 -0500 Subject: [PATCH] fix(installer): address second-round PR #2353 review comments (A) Carry forward unknown core keys in applyOverridesAfterSeeding (CodeRabbit major). Mirrors collectModuleConfig's carry-forward so the skip-collection path used by core (when seeded by --yes / legacy shortcuts) doesn't drop unknown keys on subsequent installs. Without this, `--set core.future=x` on run #1 would silently disappear on the next install. (B) --list-options now exits non-zero on a single-module miss (CodeRabbit major). formatOptionsList returns { text, ok }; install.js exits 1 with text on stderr when ok=false, 0 with text on stdout otherwise. CI scripts catch typos like `--list-options bmn`. (C) Hermetic Suite 44 discovery tests (CodeRabbit minor). Point BMAD_EXTERNAL_MODULES_CACHE at a temp dir and restore in a finally block so test results don't depend on the developer / CI cache state. (D) Case-insensitive --list-options filter (Augment). Discovery already dedupes case-insensitively; the filter now matches the same way, so `--list-options BMM` and `--list-options bmm` both find the bmm built-in. Tests: +7 cases (uppercase listing, ok flag, core carry-forward). Total 340 passing. --- test/test-installation-components.js | 83 ++++++++++++++++----- tools/installer/commands/install.js | 8 +- tools/installer/list-options.js | 18 +++-- tools/installer/modules/official-modules.js | 32 +++++++- 4 files changed, 113 insertions(+), 28 deletions(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index e3bc263ca..77eca1aaa 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -3030,25 +3030,48 @@ async function runTests() { const empty = parseSetEntries(); assert(empty && Object.keys(empty).length === 0, 'parseSetEntries() returns empty object when called without args'); - // discoverOfficialModuleYamls includes core and bmm built-ins. - const discovered = await discoverOfficialModuleYamls(); - const codes = new Set(discovered.map((d) => d.code)); - assert(codes.has('core') && codes.has('bmm'), 'discoverOfficialModuleYamls finds core and bmm built-ins'); - const coreEntry = discovered.find((d) => d.code === 'core'); - assert(coreEntry && coreEntry.source === 'built-in', 'core is reported with source="built-in"'); + // discoverOfficialModuleYamls + formatOptionsList read the on-disk + // external-module cache. Point that env at a temp dir so test results + // don't depend on whatever the developer / CI runner has cached. + const priorCacheEnv44 = process.env.BMAD_EXTERNAL_MODULES_CACHE; + const tempCacheDir44 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-list-options-cache-')); + process.env.BMAD_EXTERNAL_MODULES_CACHE = tempCacheDir44; + try { + // discoverOfficialModuleYamls includes core and bmm built-ins. + const discovered = await discoverOfficialModuleYamls(); + const codes = new Set(discovered.map((d) => d.code)); + assert(codes.has('core') && codes.has('bmm'), 'discoverOfficialModuleYamls finds core and bmm built-ins'); + const coreEntry = discovered.find((d) => d.code === 'core'); + assert(coreEntry && coreEntry.source === 'built-in', 'core is reported with source="built-in"'); - // formatOptionsList rendering: bmm-only filter shows the project_knowledge key from issue #1663. - const bmmListing = await formatOptionsList('bmm'); - assert(bmmListing.includes('bmm.project_knowledge'), '--list-options bmm renders bmm.project_knowledge'); - assert(bmmListing.includes('bmm.user_skill_level'), '--list-options bmm renders bmm.user_skill_level'); - assert(bmmListing.includes('beginner | intermediate | expert'), '--list-options renders single-select choices'); + // formatOptionsList rendering: bmm-only filter shows the project_knowledge key from issue #1663. + const bmmListing = await formatOptionsList('bmm'); + assert(bmmListing.ok === true, '--list-options bmm reports ok: true'); + assert(bmmListing.text.includes('bmm.project_knowledge'), '--list-options bmm renders bmm.project_knowledge'); + assert(bmmListing.text.includes('bmm.user_skill_level'), '--list-options bmm renders bmm.user_skill_level'); + assert(bmmListing.text.includes('beginner | intermediate | expert'), '--list-options renders single-select choices'); - // formatOptionsList for an unknown module gives a helpful message, not "No modules found". - const unknownListing = await formatOptionsList('definitely-not-a-module'); - assert( - unknownListing.includes("No locally-known module.yaml for 'definitely-not-a-module'"), - '--list-options handles unknown module gracefully', - ); + // Case-insensitive match: `--list-options BMM` and `bmm` resolve to the same entry. + const bmmUpperListing = await formatOptionsList('BMM'); + assert(bmmUpperListing.ok === true, '--list-options BMM (uppercase) finds the bmm built-in'); + assert(bmmUpperListing.text.includes('bmm.project_knowledge'), '--list-options BMM renders bmm.project_knowledge'); + + // formatOptionsList for an unknown module gives a helpful message AND ok: false + // so install.js can exit non-zero (CI scripts can detect typos). + const unknownListing = await formatOptionsList('definitely-not-a-module'); + assert(unknownListing.ok === false, '--list-options reports ok: false (non-zero exit signal)'); + assert( + unknownListing.text.includes("No locally-known module.yaml for 'definitely-not-a-module'"), + '--list-options handles unknown module gracefully', + ); + } finally { + if (priorCacheEnv44 === undefined) { + delete process.env.BMAD_EXTERNAL_MODULES_CACHE; + } else { + process.env.BMAD_EXTERNAL_MODULES_CACHE = priorCacheEnv44; + } + await fs.remove(tempCacheDir44).catch(() => {}); + } // partition() in writeCentralConfig respects setOverrideKeys: an unknown key // for a known schema must survive when the user asserted it via --set. @@ -3150,6 +3173,32 @@ async function runTests() { failed++; } await fs.remove(tmp44d).catch(() => {}); + + // applyOverridesAfterSeeding mirrors the carry-forward behavior for the + // skip-collection path used by `core` (when seeded by --yes / legacy + // shortcuts) so unknown core keys persisted on a prior run survive + // subsequent installs even without re-passing --set. + try { + const om = new OfficialModules({ + // No new --set entries this run — only prior persisted unknown. + setOverrides: {}, + }); + om._existingConfig = { core: { future_core_thing: 'persisted-from-run-1' } }; + // Simulate the seeded-core state ui.js leaves behind under --yes. + om.collectedConfig.core = { user_name: 'Brian', project_name: 'demo' }; + await om.applyOverridesAfterSeeding('core'); + + assert( + om.collectedConfig.core?.future_core_thing === 'persisted-from-run-1', + 'applyOverridesAfterSeeding carries unknown core key forward from _existingConfig', + ); + assert(om.setOverrideKeys?.core?.has('future_core_thing'), 'carried-forward core keys are tracked in setOverrideKeys'); + assert(!om.setOverrideKeys?.core?.has('user_name'), 'declared core keys (user_name) are not flagged as overrides'); + } catch (error) { + console.log(`${colors.red} applyOverridesAfterSeeding carry-forward 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 02bc67525..962d0e4bd 100644 --- a/tools/installer/commands/install.js +++ b/tools/installer/commands/install.js @@ -60,8 +60,12 @@ module.exports = { if (options.listOptions !== undefined) { const { formatOptionsList } = require('../list-options'); const moduleArg = options.listOptions === true ? null : options.listOptions; - process.stdout.write((await formatOptionsList(moduleArg)) + '\n'); - process.exit(0); + 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); } // Set debug flag as environment variable for all components diff --git a/tools/installer/list-options.js b/tools/installer/list-options.js index 3009423d1..0f3ae4517 100644 --- a/tools/installer/list-options.js +++ b/tools/installer/list-options.js @@ -143,16 +143,23 @@ function formatModuleOptions(code, parsed, source) { /** * Render `--list-options` output. + * + * Returns `{ text, ok }` so callers can surface a non-zero exit code on + * a typo'd module-code lookup. Discovery dedupes case-insensitively, so + * the lookup is also case-insensitive — typing `--list-options BMM` and + * `--list-options bmm` both find the bmm built-in. + * * @param {string|null} moduleCode - if non-null, restrict to this module - * @returns {Promise} + * @returns {Promise<{text: string, ok: boolean}>} */ async function formatOptionsList(moduleCode) { const discovered = await discoverOfficialModuleYamls(); - const filtered = moduleCode ? discovered.filter((d) => d.code === moduleCode) : discovered; + const needle = moduleCode ? moduleCode.toLowerCase() : null; + const filtered = needle ? discovered.filter((d) => d.code.toLowerCase() === needle) : discovered; if (filtered.length === 0) { if (moduleCode) { - return [ + const text = [ `No locally-known module.yaml for '${moduleCode}'.`, '', 'Built-in modules (core, bmm) are always available. External officials', @@ -162,8 +169,9 @@ async function formatOptionsList(moduleCode) { 'For community or custom modules, read the module.yaml file in that', "module's source repository directly.", ].join('\n'); + return { text, ok: false }; } - return 'No modules found.'; + return { text: 'No modules found.', ok: false }; } const sections = []; @@ -186,7 +194,7 @@ async function formatOptionsList(moduleCode) { ); } - return sections.join('\n'); + return { text: sections.join('\n'), ok: true }; } module.exports = { formatOptionsList, discoverOfficialModuleYamls }; diff --git a/tools/installer/modules/official-modules.js b/tools/installer/modules/official-modules.js index 5a1437337..be5976084 100644 --- a/tools/installer/modules/official-modules.js +++ b/tools/installer/modules/official-modules.js @@ -46,13 +46,18 @@ class OfficialModules { */ async applyOverridesAfterSeeding(moduleName) { const overrides = this.setOverrides[moduleName] || {}; - if (Object.keys(overrides).length === 0) return; + const priorConfig = this._existingConfig?.[moduleName]; + const hasPrior = priorConfig && typeof priorConfig === 'object' && !Array.isArray(priorConfig); + + if (Object.keys(overrides).length === 0 && !hasPrior) return; if (!this.collectedConfig[moduleName]) this.collectedConfig[moduleName] = {}; for (const [key, value] of Object.entries(overrides)) { this.collectedConfig[moduleName][key] = value; } + // Load schema so we can flag unknown keys. If the schema can't be loaded, + // we skip key-existence validation but still apply overrides + carry-forward. let schema = null; const schemaPath = path.join(getModulePath(moduleName), 'module.yaml'); if (await fs.pathExists(schemaPath)) { @@ -64,18 +69,37 @@ class OfficialModules { } if (!schema || typeof schema !== 'object') return; + const declaredKeys = new Set(); + for (const [key, decl] of Object.entries(schema)) { + if (decl && typeof decl === 'object' && 'prompt' in decl) declaredKeys.add(key); + } + if (!this.setOverrideKeys) this.setOverrideKeys = {}; if (!this.setOverrideKeys[moduleName]) this.setOverrideKeys[moduleName] = new Set(); + + // Warn + track unknown keys from this run's --set entries. for (const key of Object.keys(overrides)) { - const decl = schema[key]; - const isDeclared = decl && typeof decl === 'object' && 'prompt' in decl; - if (!isDeclared) { + if (!declaredKeys.has(key)) { await prompts.log.warn( `--set ${moduleName}.${key} — '${key}' is not a declared config key for module '${moduleName}'; persisted but unused by current install.`, ); this.setOverrideKeys[moduleName].add(key); } } + + // Carry forward any non-schema keys persisted by a prior install. Mirrors + // the carry-forward logic in `collectModuleConfig` so the skip-collection + // path (e.g. core under --yes) doesn't drop unknown keys on subsequent + // runs. Without this, `--set core.future=x` lands in config.toml on run #1 + // and would silently disappear on the next install. (#1663 forward-compat) + if (hasPrior) { + for (const [key, value] of Object.entries(priorConfig)) { + if (declaredKeys.has(key)) continue; + if (key in this.collectedConfig[moduleName]) continue; // already set this run + this.collectedConfig[moduleName][key] = value; + this.setOverrideKeys[moduleName].add(key); + } + } } /**