fix(installer): address PR #2353 review comments
Carry forward unknown --set keys across upgrades (CodeRabbit major). Without this, an unknown key like --set bmm.future_thing=hello landed in config.toml on run #1 but was silently dropped on the next install because collectModuleConfig rebuilds collectedConfig from prompt answers only. collectModuleConfig now copies any non-declared keys from _existingConfig into collectedConfig and tracks them in setOverrideKeys so the manifest writer's schema-strict partition keeps them. Guard single-select rendering with Array.isArray (CodeRabbit major): a malformed truthy non-array would have aborted --list-options. Unify core override handling: move the inline post-collection block from ui.js into OfficialModules.applyOverridesAfterSeeding so core and non-core take a single validated path. Removes duplicated schema-load logic and inline requires from ui.js. Remove dead code: findOfficialModuleYaml and readDeclaredKeys in set-overrides.js were exported but never imported. Drop them and their path/fs/yaml/project-root imports — the module is now pure string-parsing with zero deps. Doc fix: change "silently ignored" to "ignored with a warning" for the --action quick-update note (Augment + CodeRabbit). Polish: clearer flag placeholder (--set <module.key=value> instead of the misleading <key=value>), trim-asymmetry rationale comment in parseSetEntry, dedupe rationale in list-options. Tests: +6 cases — collectModuleConfig --set application end-to-end (prompt-skip with template rendering), and carry-forward of unknown keys from _existingConfig. Total 333 passing.
This commit is contained in:
Brian Madison
parent
f33d251790
commit
f1c9e12618
|
|
@ -212,7 +212,7 @@ npx bmad-method install --list-options bmm
|
|||
The legacy core shortcuts (`--user-name`, `--output-folder`, etc.) still work and remain documented for backward compatibility, but `--set core.user_name=...` is equivalent and uses the same code path.
|
||||
|
||||
:::note[Quick-update is unaffected]
|
||||
`bmad install --action quick-update` preserves the existing `config.toml` answers as-is. `--set` flags passed alongside `--action quick-update` are silently ignored. To change a stored value, run `bmad install --action update` (or just `bmad install`) instead.
|
||||
`bmad install --action quick-update` preserves the existing `config.toml` answers as-is. `--set` flags passed alongside `--action quick-update` are ignored — the installer prints a warning so scripted runs surface the mismatch in CI logs. To change a stored value, run `bmad install --action update` (or just `bmad install`) instead.
|
||||
:::
|
||||
|
||||
:::caution[Rate limit on shared IPs]
|
||||
|
|
|
|||
|
|
@ -3084,6 +3084,72 @@ async function runTests() {
|
|||
|
||||
await fs.remove(tmp44).catch(() => {});
|
||||
await fs.remove(tmp44b).catch(() => {});
|
||||
|
||||
// Integration: --set actually applies through collectModuleConfig with skipPrompts.
|
||||
// Constructs OfficialModules directly (no UI), runs the bmm collector, asserts
|
||||
// the override value lands in collectedConfig with the result template rendered.
|
||||
const tmp44c = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-fixture-44c-'));
|
||||
try {
|
||||
const om = new OfficialModules({
|
||||
setOverrides: { bmm: { project_knowledge: 'research', user_skill_level: 'expert' } },
|
||||
});
|
||||
om.skipPrompts = true;
|
||||
om._silentConfig = true;
|
||||
om.modulesToCustomize = new Set();
|
||||
om.allAnswers = {};
|
||||
om._existingConfig = {};
|
||||
await om.collectModuleConfig('bmm', tmp44c, true, true);
|
||||
|
||||
assert(
|
||||
om.collectedConfig.bmm?.project_knowledge === '{project-root}/research',
|
||||
'collectModuleConfig pre-fills bmm.project_knowledge from --set and renders {project-root}/{value}',
|
||||
);
|
||||
assert(
|
||||
om.collectedConfig.bmm?.user_skill_level === 'expert',
|
||||
'collectModuleConfig pre-fills bmm.user_skill_level from --set ({value} template)',
|
||||
);
|
||||
// Unrelated bmm keys still get their schema defaults applied.
|
||||
assert(
|
||||
typeof om.collectedConfig.bmm?.planning_artifacts === 'string',
|
||||
'collectModuleConfig still fills non-overridden bmm keys with schema defaults under skipPrompts',
|
||||
);
|
||||
} catch (error) {
|
||||
console.log(`${colors.red} collectModuleConfig --set integration failed: ${error.message}${colors.reset}`);
|
||||
console.log(error.stack);
|
||||
failed++;
|
||||
}
|
||||
await fs.remove(tmp44c).catch(() => {});
|
||||
|
||||
// Carry-forward: an unknown key persisted by a prior install survives the
|
||||
// next collectModuleConfig even when --set isn't repeated. This is the
|
||||
// "persist across upgrades" contract from #1663 (CodeRabbit major fix).
|
||||
const tmp44d = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-fixture-44d-'));
|
||||
try {
|
||||
const om = new OfficialModules();
|
||||
om.skipPrompts = true;
|
||||
om._silentConfig = true;
|
||||
om.modulesToCustomize = new Set();
|
||||
om.allAnswers = {};
|
||||
// Simulate prior install: future_thing was --set on run #1, persisted to
|
||||
// _bmad/bmm/config.yaml, and is now loaded as _existingConfig.
|
||||
om._existingConfig = { bmm: { future_thing: 'pre-seeded', user_skill_level: 'beginner' } };
|
||||
await om.collectModuleConfig('bmm', tmp44d, true, true);
|
||||
|
||||
assert(om.collectedConfig.bmm?.future_thing === 'pre-seeded', 'collectModuleConfig carries unknown key forward from _existingConfig');
|
||||
assert(
|
||||
om.setOverrideKeys?.bmm?.has('future_thing'),
|
||||
'carried-forward keys are tracked in setOverrideKeys so writeCentralConfig keeps them',
|
||||
);
|
||||
// Declared keys from _existingConfig are NOT carried forward by this
|
||||
// mechanism — they go through normal prompt processing and would be
|
||||
// seeded as defaults via buildQuestion's existingValue lookup.
|
||||
assert(!om.setOverrideKeys?.bmm?.has('user_skill_level'), 'carry-forward leaves declared keys to the normal prompt path');
|
||||
} catch (error) {
|
||||
console.log(`${colors.red} collectModuleConfig carry-forward failed: ${error.message}${colors.reset}`);
|
||||
console.log(error.stack);
|
||||
failed++;
|
||||
}
|
||||
await fs.remove(tmp44d).catch(() => {});
|
||||
} catch (error) {
|
||||
console.log(`${colors.red}Test Suite 44 setup failed: ${error.message}${colors.reset}`);
|
||||
console.log(error.stack);
|
||||
|
|
|
|||
|
|
@ -19,8 +19,8 @@ module.exports = {
|
|||
],
|
||||
['--list-tools', 'Print all supported tool/IDE IDs (with target directories) and exit.'],
|
||||
[
|
||||
'--set <key=value>',
|
||||
'Set a module config option non-interactively. Format: <module>.<key>=<value> (e.g. bmm.project_knowledge=research). Repeatable. Run --list-options to see available keys.',
|
||||
'--set <module.key=value>',
|
||||
'Set a module config option non-interactively (e.g. --set bmm.project_knowledge=research). Repeatable. Run --list-options to see available keys.',
|
||||
(value, prev) => [...(prev || []), value],
|
||||
[],
|
||||
],
|
||||
|
|
|
|||
|
|
@ -33,6 +33,14 @@ async function readModuleCode(yamlPath) {
|
|||
*/
|
||||
async function discoverOfficialModuleYamls() {
|
||||
const found = [];
|
||||
// Dedupe is case-insensitive because module caches occasionally retain a
|
||||
// legacy UPPERCASE-named directory alongside the canonical lowercase one
|
||||
// (same module, different cache key from an older schema). We pick whichever
|
||||
// entry we see first and skip the alternate-case duplicate. NOTE: `--set`
|
||||
// matching itself is case-sensitive (it keys on `moduleName` from the install
|
||||
// flow's selected list, which is always lowercase short codes), so the
|
||||
// surfaced `code` here is what users should type. Don't change to
|
||||
// case-sensitive dedupe without revisiting that contract.
|
||||
const seenCodes = new Set();
|
||||
|
||||
const addFound = async (yamlPath, source, fallbackCode) => {
|
||||
|
|
@ -120,7 +128,7 @@ function formatModuleOptions(code, parsed, source) {
|
|||
lines.push(` ${code}.${key} (${type}${scope}) default: ${defaultStr}`);
|
||||
const promptText = formatPromptText(item);
|
||||
if (promptText) lines.push(` ${promptText}`);
|
||||
if (item['single-select']) {
|
||||
if (Array.isArray(item['single-select'])) {
|
||||
const values = item['single-select'].map((v) => (typeof v === 'object' ? v.value : v)).filter((v) => v !== undefined);
|
||||
if (values.length > 0) lines.push(` values: ${values.join(' | ')}`);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -27,6 +27,57 @@ class OfficialModules {
|
|||
this.setOverrides = options.setOverrides || {};
|
||||
}
|
||||
|
||||
/**
|
||||
* Apply `--set <module>.<key>=<value>` overrides AFTER normal collection.
|
||||
*
|
||||
* Used for modules whose `collectModuleConfig` was skipped — currently only
|
||||
* `core`, when its config was seeded by `--yes` defaults or by legacy
|
||||
* core-shortcut flags (--user-name/--output-folder/etc.). For all other
|
||||
* modules, override handling is integrated into `collectModuleConfig`.
|
||||
*
|
||||
* Validates known keys against the module's `module.yaml` schema (located
|
||||
* via `getModulePath`); unknown keys are warned but persisted, mirroring
|
||||
* the schema-handling for non-core modules. Core's `result:` templates are
|
||||
* all `{value}` (or `{project-root}/{value}` for `output_folder`, which the
|
||||
* legacy flag handlers also write raw), so writing the raw override value
|
||||
* preserves parity with the `--user-name` / `--output-folder` shortcuts.
|
||||
*
|
||||
* @param {string} moduleName
|
||||
*/
|
||||
async applyOverridesAfterSeeding(moduleName) {
|
||||
const overrides = this.setOverrides[moduleName] || {};
|
||||
if (Object.keys(overrides).length === 0) return;
|
||||
|
||||
if (!this.collectedConfig[moduleName]) this.collectedConfig[moduleName] = {};
|
||||
for (const [key, value] of Object.entries(overrides)) {
|
||||
this.collectedConfig[moduleName][key] = value;
|
||||
}
|
||||
|
||||
let schema = null;
|
||||
const schemaPath = path.join(getModulePath(moduleName), 'module.yaml');
|
||||
if (await fs.pathExists(schemaPath)) {
|
||||
try {
|
||||
schema = yaml.parse(await fs.readFile(schemaPath, 'utf8'));
|
||||
} catch {
|
||||
// schema unparseable — skip key-existence validation
|
||||
}
|
||||
}
|
||||
if (!schema || typeof schema !== 'object') return;
|
||||
|
||||
if (!this.setOverrideKeys) this.setOverrideKeys = {};
|
||||
if (!this.setOverrideKeys[moduleName]) this.setOverrideKeys[moduleName] = new Set();
|
||||
for (const key of Object.keys(overrides)) {
|
||||
const decl = schema[key];
|
||||
const isDeclared = decl && typeof decl === 'object' && 'prompt' in decl;
|
||||
if (!isDeclared) {
|
||||
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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Module configurations collected during install.
|
||||
*/
|
||||
|
|
@ -1561,12 +1612,13 @@ class OfficialModules {
|
|||
}
|
||||
// Drop pre-seeded questions so the user is not re-prompted and so
|
||||
// skipPrompts mode doesn't overwrite the override with the default.
|
||||
const remainingQuestions = questions.filter((q) => {
|
||||
const key = q.name.replace(`${moduleName}_`, '');
|
||||
return !seededOverrideKeys.has(key);
|
||||
});
|
||||
questions.length = 0;
|
||||
questions.push(...remainingQuestions);
|
||||
// In-place mutation keeps the rest of this method's `questions` references
|
||||
// pointing at the filtered list without renaming a local through 100+ lines.
|
||||
if (seededOverrideKeys.size > 0) {
|
||||
const remaining = questions.filter((q) => !seededOverrideKeys.has(q.name.replace(`${moduleName}_`, '')));
|
||||
questions.length = 0;
|
||||
questions.push(...remaining);
|
||||
}
|
||||
|
||||
if (seededOverrideKeys.size > 0 && !this._silentConfig) {
|
||||
const list = [...seededOverrideKeys].map((k) => `${moduleName}.${k}`).join(', ');
|
||||
|
|
@ -1791,6 +1843,34 @@ class OfficialModules {
|
|||
}
|
||||
}
|
||||
|
||||
// Carry forward unknown keys persisted by a prior install. Without this,
|
||||
// a value originally set via `--set <module>.future_thing=...` lands in
|
||||
// `_bmad/<module>/config.yaml` on run #1, but the next collectModuleConfig
|
||||
// rebuilds collectedConfig from prompt answers only — the unknown key
|
||||
// would be silently dropped from the regenerated config.toml. Tracking
|
||||
// the carried keys in setOverrideKeys ensures the schema-strict partition
|
||||
// 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);
|
||||
}
|
||||
}
|
||||
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.
|
||||
if (this.collectedConfig[moduleName] && key in this.collectedConfig[moduleName]) continue;
|
||||
if (!this.collectedConfig[moduleName]) this.collectedConfig[moduleName] = {};
|
||||
if (!this.setOverrideKeys) this.setOverrideKeys = {};
|
||||
if (!this.setOverrideKeys[moduleName]) this.setOverrideKeys[moduleName] = new Set();
|
||||
this.collectedConfig[moduleName][key] = value;
|
||||
this.setOverrideKeys[moduleName].add(key);
|
||||
}
|
||||
}
|
||||
|
||||
await this.displayModulePostConfigNotes(moduleName, moduleConfig);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1,8 +1,3 @@
|
|||
const path = require('node:path');
|
||||
const fs = require('./fs-native');
|
||||
const yaml = require('yaml');
|
||||
const { getModulePath, getExternalModuleCachePath } = require('./project-root');
|
||||
|
||||
/**
|
||||
* Parse a single `--set <module>.<key>=<value>` entry.
|
||||
* @param {string} entry - raw flag value
|
||||
|
|
@ -18,6 +13,9 @@ function parseSetEntry(entry) {
|
|||
throw new Error(`--set "${entry}": missing '='. Expected <module>.<key>=<value>`);
|
||||
}
|
||||
const lhs = entry.slice(0, eq);
|
||||
// Note: only the LHS is trimmed. Values may legitimately contain leading
|
||||
// or trailing whitespace (paths with spaces, quoted strings); module / key
|
||||
// names cannot, so it's safe to be strict on the left.
|
||||
const value = entry.slice(eq + 1);
|
||||
const dot = lhs.indexOf('.');
|
||||
if (dot === -1) {
|
||||
|
|
@ -48,48 +46,4 @@ function parseSetEntries(entries) {
|
|||
return overrides;
|
||||
}
|
||||
|
||||
/**
|
||||
* Find a module's source `module.yaml` for officials only.
|
||||
* Returns null for community/custom; we don't validate those.
|
||||
* @param {string} moduleCode
|
||||
* @returns {Promise<string|null>}
|
||||
*/
|
||||
async function findOfficialModuleYaml(moduleCode) {
|
||||
const builtIn = path.join(getModulePath(moduleCode), 'module.yaml');
|
||||
if (await fs.pathExists(builtIn)) return builtIn;
|
||||
|
||||
const externalRoot = getExternalModuleCachePath(moduleCode);
|
||||
if (!(await fs.pathExists(externalRoot))) return null;
|
||||
|
||||
const candidates = [
|
||||
path.join(externalRoot, 'module.yaml'),
|
||||
path.join(externalRoot, 'src', 'module.yaml'),
|
||||
path.join(externalRoot, 'skills', 'module.yaml'),
|
||||
];
|
||||
for (const candidate of candidates) {
|
||||
if (await fs.pathExists(candidate)) return candidate;
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Read declared config keys (those with a `prompt:`) from a module.yaml.
|
||||
* Returns a Set of key names, or null if the file can't be read.
|
||||
*/
|
||||
async function readDeclaredKeys(moduleYamlPath) {
|
||||
try {
|
||||
const parsed = yaml.parse(await fs.readFile(moduleYamlPath, 'utf8'));
|
||||
if (!parsed || typeof parsed !== 'object') return null;
|
||||
const keys = new Set();
|
||||
for (const [key, value] of Object.entries(parsed)) {
|
||||
if (value && typeof value === 'object' && 'prompt' in value) {
|
||||
keys.add(key);
|
||||
}
|
||||
}
|
||||
return keys;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
module.exports = { parseSetEntry, parseSetEntries, findOfficialModuleYaml, readDeclaredKeys };
|
||||
module.exports = { parseSetEntry, parseSetEntries };
|
||||
|
|
|
|||
|
|
@ -797,41 +797,13 @@ class UI {
|
|||
skipPrompts: options.yes || false,
|
||||
});
|
||||
|
||||
// Apply --set overrides for `core` AFTER collectAllConfigurations, since
|
||||
// core is skipped when its config was seeded by `--yes` defaults or by
|
||||
// legacy core-shortcut flags (--user-name/--output-folder/etc.). Without
|
||||
// this step those override values would be silently dropped. Core
|
||||
// result templates are all `{value}` (or `{project-root}/{value}` for
|
||||
// output_folder, which the existing flag handling also writes raw),
|
||||
// so writing the raw value matches the legacy shortcut semantics.
|
||||
const coreOverrides = setOverrides.core || {};
|
||||
if (Object.keys(coreOverrides).length > 0) {
|
||||
if (!configCollector.collectedConfig.core) configCollector.collectedConfig.core = {};
|
||||
for (const [key, value] of Object.entries(coreOverrides)) {
|
||||
configCollector.collectedConfig.core[key] = value;
|
||||
}
|
||||
const yaml = require('yaml');
|
||||
const { getProjectRoot } = require('./project-root');
|
||||
const coreSchemaPath = path.join(getProjectRoot(), 'src', 'core-skills', 'module.yaml');
|
||||
let coreSchema = null;
|
||||
try {
|
||||
coreSchema = yaml.parse(await fs.readFile(coreSchemaPath, 'utf8'));
|
||||
} catch {
|
||||
// schema unavailable — skip key-existence validation
|
||||
}
|
||||
if (coreSchema) {
|
||||
if (!configCollector.setOverrideKeys) configCollector.setOverrideKeys = {};
|
||||
if (!configCollector.setOverrideKeys.core) configCollector.setOverrideKeys.core = new Set();
|
||||
for (const key of Object.keys(coreOverrides)) {
|
||||
if (!(key in coreSchema)) {
|
||||
await prompts.log.warn(
|
||||
`--set core.${key} — '${key}' is not a declared config key for module 'core'; persisted but unused by current install.`,
|
||||
);
|
||||
configCollector.setOverrideKeys.core.add(key);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
// Apply --set overrides for `core` AFTER collectAllConfigurations.
|
||||
// Core is skipped inside collectAllConfigurations when its config was
|
||||
// seeded by `--yes` defaults or by legacy core-shortcut flags
|
||||
// (--user-name/--output-folder/etc.), so its overrides need a separate
|
||||
// application path. Non-core modules apply overrides inside their own
|
||||
// collectModuleConfig run.
|
||||
await configCollector.applyOverridesAfterSeeding('core');
|
||||
|
||||
// Convert per-module override-key Sets to plain string arrays so the value
|
||||
// round-trips cleanly through Config.build / freezing.
|
||||
|
|
|
|||
Loading…
Reference in New Issue