ldy26098
/
BMAD-METHOD
mirror of https://github.com/bmad-code-org/BMAD-METHOD.git
1
0
Fork 0
Code Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Brian Madison 2026-04-28 11:24:51 -05:00
parent f1c9e12618
commit ce12cc1a7f
4 changed files with 113 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

83
test/test-installation-components.js
View File

@ -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 <unknown> 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);

8
tools/installer/commands/install.js
View File

@ -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

18
tools/installer/list-options.js
View File

@ -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<string>}
* @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 };

32
tools/installer/modules/official-modules.js
View File

@ -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);
}
}
}
/**