From 8ab1de85012e8c7c9e46b76102d561d6b1e448b4 Mon Sep 17 00:00:00 2001 From: Brian Madison Date: Tue, 28 Apr 2026 19:22:34 -0500 Subject: [PATCH] fix(installer): forward-compat --set keys survive quick-update reinstalls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found via end-to-end smoke test, not flagged by either bot review: `--set bmm.future_thing=x` was persisted to config.toml on install #1 but silently dropped on the next quick-update reinstall, even though the per-module _bmad/bmm/config.yaml retained it. The central manifest's schema-strict partition stripped it because collectModuleConfigQuick (the quick-update helper) never populated setOverrideKeys for carried-forward unknown keys, and quickUpdate's installConfig didn't thread setOverrideKeys into the install call. This is the same bug class as the round-1 fix to collectModuleConfig (CodeRabbit major #3155145084) but for the quick-update code path, which has a separate collection helper. Fix: - Add OfficialModules._trackUnknownKeysAsOverrides(moduleName, schema) helper that walks collectedConfig[moduleName] and adds any non-schema key to setOverrideKeys[moduleName]. Without a schema, every key is treated as unknown (safe fallback for modules with no module.yaml). - Call it from all four return paths in collectModuleConfigQuick: no-schema, parse-failed, hasNoConfig+subheader, silent+no-new-keys, and the regular end-of-method. - Mirror ui.collectModuleConfigs's setOverrideKeys conversion in installer.quickUpdate so the Set→array round-trip lands in Config.build, and writeCentralConfig sees the exemption list. Tests: +4 cases — collectModuleConfigQuick carry-forward of unknown key, declared-key non-tracking under quick-update, and _trackUnknownKeysAsOverrides no-schema fallback. Total 351 passing. E2E smoke verified: --set =x survives install→quick-update, install→regular-update, and install→quick-update→regular-update with a new --set added. --- test/test-installation-components.js | 50 ++++++++++++++++++++ tools/installer/core/installer.js | 14 ++++++ tools/installer/modules/official-modules.js | 51 +++++++++++++++++++++ 3 files changed, 115 insertions(+) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 797b75b11..d25160cce 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -3295,6 +3295,56 @@ async function runTests() { console.log(error.stack); failed++; } + + // collectModuleConfigQuick (the quick-update path) must also track + // non-schema keys as overrides so the manifest writer keeps them through + // the partition. Before this fix, `--set bmm.future_thing=x` landed in + // config.toml on install #1 but was silently dropped on the next + // quick-update reinstall — the per-module config.yaml retained the value + // but the central manifest's schema-strict partition stripped it because + // setOverrideKeys was never populated for carried-forward keys. + const tmp44e = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-fixture-44e-')); + try { + const om = new OfficialModules(); + // Simulate prior install state: future_thing was --set on run #1, lives + // in _bmad/bmm/config.yaml, and is now loaded via loadExistingConfig. + om._existingConfig = { bmm: { future_thing: 'persists', user_skill_level: 'expert' } }; + om.allAnswers = {}; + // collectModuleConfigQuick reads moduleConfig from disk; bmm built-in + // schema is the same one used elsewhere in this suite. + await om.collectModuleConfigQuick('bmm', tmp44e, true); + + assert(om.collectedConfig.bmm?.future_thing === 'persists', 'collectModuleConfigQuick carries unknown bmm.future_thing forward'); + assert( + om.setOverrideKeys?.bmm?.has('future_thing'), + 'collectModuleConfigQuick tracks carried-forward unknown keys in setOverrideKeys (so writeCentralConfig keeps them)', + ); + assert( + !om.setOverrideKeys?.bmm?.has('user_skill_level'), + 'collectModuleConfigQuick does NOT flag declared schema keys (user_skill_level) as overrides', + ); + } catch (error) { + console.log(`${colors.red} collectModuleConfigQuick carry-forward failed: ${error.message}${colors.reset}`); + console.log(error.stack); + failed++; + } + await fs.remove(tmp44e).catch(() => {}); + + // _trackUnknownKeysAsOverrides with no schema treats every key as + // unknown — the fallback when a module has no module.yaml at all. + try { + const om = new OfficialModules(); + om.collectedConfig = { weirdmod: { foo: 1, bar: 2 } }; + om._trackUnknownKeysAsOverrides('weirdmod', null); + assert( + om.setOverrideKeys?.weirdmod?.has('foo') && om.setOverrideKeys.weirdmod.has('bar'), + '_trackUnknownKeysAsOverrides flags every key as unknown when no schema is provided', + ); + } catch (error) { + console.log(`${colors.red} _trackUnknownKeysAsOverrides no-schema 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/core/installer.js b/tools/installer/core/installer.js index 748b1b6f5..ac1af0a58 100644 --- a/tools/installer/core/installer.js +++ b/tools/installer/core/installer.js @@ -1277,6 +1277,19 @@ class Installer { lastModified: new Date().toISOString(), }; + // Convert per-module override-key Sets to plain string arrays so the + // value round-trips cleanly through Config.build / Object.freeze. Mirrors + // ui.collectModuleConfigs's return shape so quick-update's central + // manifest writer applies the same schema-strict partition exemption, + // letting prior `--set .=` forward-compat keys + // survive subsequent quick-updates without re-passing the flag. + const setOverrideKeys = {}; + if (quickModules.setOverrideKeys) { + for (const [moduleCode, keys] of Object.entries(quickModules.setOverrideKeys)) { + setOverrideKeys[moduleCode] = [...keys]; + } + } + // Build config and delegate to install() const installConfig = { directory: projectDir, @@ -1284,6 +1297,7 @@ class Installer { ides: configuredIdes, coreConfig: quickModules.collectedConfig.core, moduleConfigs: quickModules.collectedConfig, + setOverrideKeys, actionType: 'install', _quickUpdate: true, _preserveModules: skippedModules, diff --git a/tools/installer/modules/official-modules.js b/tools/installer/modules/official-modules.js index 4a3a4817a..f400eec9e 100644 --- a/tools/installer/modules/official-modules.js +++ b/tools/installer/modules/official-modules.js @@ -1266,6 +1266,42 @@ class OfficialModules { * @param {boolean} silentMode - If true, only prompt for new/missing fields * @returns {boolean} True if new fields were prompted, false if all fields existed */ + /** + * Track keys carried over from `_existingConfig` (or merged in from elsewhere) + * that are NOT declared in the module's `module.yaml` schema. The manifest + * writer's schema-strict partition would otherwise drop these on the next + * write, silently undoing the user's prior `--set .=` + * forward-compat assertion. Used by both `collectModuleConfig` and + * `collectModuleConfigQuick` so the persistence contract holds across + * regular updates and quick-updates alike. + * + * Without a schema (`moduleSchema = null`), every key is treated as unknown + * since we can't tell what's declared. + */ + _trackUnknownKeysAsOverrides(moduleName, moduleSchema) { + const moduleData = this.collectedConfig?.[moduleName]; + if (!moduleData || typeof moduleData !== 'object') return; + + const declaredKeys = new Set(); + if (moduleSchema && typeof moduleSchema === 'object') { + for (const [key, decl] of Object.entries(moduleSchema)) { + if (key === 'prompt') continue; + if (decl && typeof decl === 'object' && (decl.prompt !== undefined || decl.result !== undefined)) { + declaredKeys.add(key); + } + } + } + + if (!this.setOverrideKeys) this.setOverrideKeys = {}; + if (!this.setOverrideKeys[moduleName]) this.setOverrideKeys[moduleName] = new Set(); + + for (const key of Object.keys(moduleData)) { + if (!declaredKeys.has(key)) { + this.setOverrideKeys[moduleName].add(key); + } + } + } + async collectModuleConfigQuick(moduleName, projectDir, silentMode = true) { this.currentProjectDir = projectDir; // Load existing config if not already loaded @@ -1299,6 +1335,10 @@ class OfficialModules { } this.collectedConfig[moduleName] = { ...this._existingConfig[moduleName] }; } + // Without a schema we can't tell declared from undeclared, so every + // carried-forward key gets the override exemption to survive + // writeCentralConfig's schema-strict partition. + this._trackUnknownKeysAsOverrides(moduleName, null); return false; } @@ -1306,6 +1346,8 @@ class OfficialModules { const moduleConfig = yaml.parse(configContent); if (!moduleConfig) { + // Schema unparseable — same fallback as missing schema. + this._trackUnknownKeysAsOverrides(moduleName, null); return false; } @@ -1324,6 +1366,7 @@ class OfficialModules { const moduleDisplayName = moduleConfig.header || `${moduleName.toUpperCase()} Module`; await prompts.log.step(moduleDisplayName); await prompts.log.message(` \u2713 ${moduleConfig.subheader}`); + this._trackUnknownKeysAsOverrides(moduleName, moduleConfig); return false; // No new fields } @@ -1378,6 +1421,9 @@ class OfficialModules { // Show "no config" message for modules with no new questions (that have config keys) await prompts.log.message(` \u2713 ${moduleName.toUpperCase()} module already up to date`); + // Track non-schema keys so the manifest writer keeps prior `--set` + // assertions through quick-update's schema-strict partition. + this._trackUnknownKeysAsOverrides(moduleName, moduleConfig); return false; // No new fields } @@ -1465,6 +1511,11 @@ class OfficialModules { } } + // Track non-schema keys carried over so writeCentralConfig keeps them + // through the partition (forward-compat contract: a `--set` from a prior + // run survives quick-update reinstalls without re-passing the flag). + this._trackUnknownKeysAsOverrides(moduleName, moduleConfig); + await this.displayModulePostConfigNotes(moduleName, moduleConfig); return newKeys.length > 0 || newStaticKeys.length > 0; // Return true if we had any new fields (interactive or static)