diff --git a/docs/how-to/install-bmad.md b/docs/how-to/install-bmad.md index 4fbca560b..4122ecf46 100644 --- a/docs/how-to/install-bmad.md +++ b/docs/how-to/install-bmad.md @@ -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] diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 2facf8aed..e3bc263ca 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -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); diff --git a/tools/installer/commands/install.js b/tools/installer/commands/install.js index d752b0471..02bc67525 100644 --- a/tools/installer/commands/install.js +++ b/tools/installer/commands/install.js @@ -19,8 +19,8 @@ module.exports = { ], ['--list-tools', 'Print all supported tool/IDE IDs (with target directories) and exit.'], [ - '--set ', - 'Set a module config option non-interactively. Format: .= (e.g. bmm.project_knowledge=research). Repeatable. Run --list-options to see available keys.', + '--set ', + '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], [], ], diff --git a/tools/installer/list-options.js b/tools/installer/list-options.js index 37742143e..3009423d1 100644 --- a/tools/installer/list-options.js +++ b/tools/installer/list-options.js @@ -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(' | ')}`); } diff --git a/tools/installer/modules/official-modules.js b/tools/installer/modules/official-modules.js index 964e5d248..5a1437337 100644 --- a/tools/installer/modules/official-modules.js +++ b/tools/installer/modules/official-modules.js @@ -27,6 +27,57 @@ class OfficialModules { this.setOverrides = options.setOverrides || {}; } + /** + * Apply `--set .=` 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 .future_thing=...` lands in + // `_bmad//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); } diff --git a/tools/installer/set-overrides.js b/tools/installer/set-overrides.js index b5db532ae..f47ae553a 100644 --- a/tools/installer/set-overrides.js +++ b/tools/installer/set-overrides.js @@ -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 .=` entry. * @param {string} entry - raw flag value @@ -18,6 +13,9 @@ function parseSetEntry(entry) { throw new Error(`--set "${entry}": missing '='. Expected .=`); } 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} - */ -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 }; diff --git a/tools/installer/ui.js b/tools/installer/ui.js index 22b94ec7e..12867dfd5 100644 --- a/tools/installer/ui.js +++ b/tools/installer/ui.js @@ -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.