fix(installer): forward-compat --set keys survive quick-update reinstalls
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 <unknown>=x survives install→quick-update, install→regular-update, and install→quick-update→regular-update with a new --set added.
This commit is contained in:
parent
7ad054f0e9
commit
8ab1de8501
|
|
@ -3295,6 +3295,56 @@ async function runTests() {
|
||||||
console.log(error.stack);
|
console.log(error.stack);
|
||||||
failed++;
|
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) {
|
} catch (error) {
|
||||||
console.log(`${colors.red}Test Suite 44 setup failed: ${error.message}${colors.reset}`);
|
console.log(`${colors.red}Test Suite 44 setup failed: ${error.message}${colors.reset}`);
|
||||||
console.log(error.stack);
|
console.log(error.stack);
|
||||||
|
|
|
||||||
|
|
@ -1277,6 +1277,19 @@ class Installer {
|
||||||
lastModified: new Date().toISOString(),
|
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 <module>.<key>=<value>` 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()
|
// Build config and delegate to install()
|
||||||
const installConfig = {
|
const installConfig = {
|
||||||
directory: projectDir,
|
directory: projectDir,
|
||||||
|
|
@ -1284,6 +1297,7 @@ class Installer {
|
||||||
ides: configuredIdes,
|
ides: configuredIdes,
|
||||||
coreConfig: quickModules.collectedConfig.core,
|
coreConfig: quickModules.collectedConfig.core,
|
||||||
moduleConfigs: quickModules.collectedConfig,
|
moduleConfigs: quickModules.collectedConfig,
|
||||||
|
setOverrideKeys,
|
||||||
actionType: 'install',
|
actionType: 'install',
|
||||||
_quickUpdate: true,
|
_quickUpdate: true,
|
||||||
_preserveModules: skippedModules,
|
_preserveModules: skippedModules,
|
||||||
|
|
|
||||||
|
|
@ -1266,6 +1266,42 @@ class OfficialModules {
|
||||||
* @param {boolean} silentMode - If true, only prompt for new/missing fields
|
* @param {boolean} silentMode - If true, only prompt for new/missing fields
|
||||||
* @returns {boolean} True if new fields were prompted, false if all fields existed
|
* @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 <module>.<key>=<value>`
|
||||||
|
* 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) {
|
async collectModuleConfigQuick(moduleName, projectDir, silentMode = true) {
|
||||||
this.currentProjectDir = projectDir;
|
this.currentProjectDir = projectDir;
|
||||||
// Load existing config if not already loaded
|
// Load existing config if not already loaded
|
||||||
|
|
@ -1299,6 +1335,10 @@ class OfficialModules {
|
||||||
}
|
}
|
||||||
this.collectedConfig[moduleName] = { ...this._existingConfig[moduleName] };
|
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;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -1306,6 +1346,8 @@ class OfficialModules {
|
||||||
const moduleConfig = yaml.parse(configContent);
|
const moduleConfig = yaml.parse(configContent);
|
||||||
|
|
||||||
if (!moduleConfig) {
|
if (!moduleConfig) {
|
||||||
|
// Schema unparseable — same fallback as missing schema.
|
||||||
|
this._trackUnknownKeysAsOverrides(moduleName, null);
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -1324,6 +1366,7 @@ class OfficialModules {
|
||||||
const moduleDisplayName = moduleConfig.header || `${moduleName.toUpperCase()} Module`;
|
const moduleDisplayName = moduleConfig.header || `${moduleName.toUpperCase()} Module`;
|
||||||
await prompts.log.step(moduleDisplayName);
|
await prompts.log.step(moduleDisplayName);
|
||||||
await prompts.log.message(` \u2713 ${moduleConfig.subheader}`);
|
await prompts.log.message(` \u2713 ${moduleConfig.subheader}`);
|
||||||
|
this._trackUnknownKeysAsOverrides(moduleName, moduleConfig);
|
||||||
return false; // No new fields
|
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)
|
// 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`);
|
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
|
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);
|
await this.displayModulePostConfigNotes(moduleName, moduleConfig);
|
||||||
|
|
||||||
return newKeys.length > 0 || newStaticKeys.length > 0; // Return true if we had any new fields (interactive or static)
|
return newKeys.length > 0 || newStaticKeys.length > 0; // Return true if we had any new fields (interactive or static)
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue