From 93f6547554bdfc0e832ecf55eab35db3b0c96a93 Mon Sep 17 00:00:00 2001
From: Brian Madison <bmadcode@gmail.com>
Date: Mon, 27 Apr 2026 23:28:41 -0500
Subject: [PATCH] fix(installer): harden config-load against malformed
 config.yaml
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Per augment review on #2348: loadExistingConfig stored any truthy
yaml.parse result (including scalars like '42'), which would later crash
_hoistCoreKeysFromLegacyModuleConfigs at \`key in cfg\` with
"Cannot use 'in' operator to search for ... in 42".

- loadExistingConfig: only keep parses that are plain objects (not
  scalars or arrays). A corrupt config.yaml is now treated the same as
  a parse error — skipped, not crashed-on.
- _hoistCoreKeysFromLegacyModuleConfigs: belt-and-suspenders type guards
  on _existingConfig.core (in case it's populated by some other path)
  and on each module cfg in the loop.
- Test Suite 43 adds 2 assertions covering a scalar core/config.yaml:
  loadExistingConfig must not crash, and bmm.project_name must still
  hoist into a clean core bucket.
---
 test/test-installation-components.js        | 30 +++++++++++++++++++++
 tools/installer/modules/official-modules.js | 15 ++++++++---
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/test/test-installation-components.js b/test/test-installation-components.js
index b7beec9ff..a8bf77756 100644
--- a/test/test-installation-components.js
+++ b/test/test-installation-components.js
@@ -2943,8 +2943,38 @@ async function runTests() {
       'hoist still strips the duplicate from bmm so writeCentralConfig partition stays clean',
     );
 
+    // Malformed config.yaml (parses to a scalar) must not crash loadExistingConfig
+    // or the hoist pass — they should treat it as "no config for that module"
+    // and continue. Regression for augment review on PR #2348.
+    const fixtureRoot43c = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-fixture-43c-'));
+    const bmadDir43c = path.join(fixtureRoot43c, '_bmad');
+    await fs.ensureDir(path.join(bmadDir43c, '_config'));
+    await fs.writeFile(path.join(bmadDir43c, '_config', 'manifest.yaml'), 'modules: []\n', 'utf8');
+    await fs.ensureDir(path.join(bmadDir43c, 'core'));
+    await fs.ensureDir(path.join(bmadDir43c, 'bmm'));
+    // Scalar YAML — yaml.parse returns the literal 42 (truthy non-object).
+    // Pre-fix this crashed _hoistCoreKeysFromLegacyModuleConfigs with
+    // "Cannot use 'in' operator to search for 'project_name' in 42".
+    await fs.writeFile(path.join(bmadDir43c, 'core', 'config.yaml'), '42\n', 'utf8');
+    await fs.writeFile(path.join(bmadDir43c, 'bmm', 'config.yaml'), 'project_name: rescued\n', 'utf8');
+
+    const officialModules43c = new OfficialModules();
+    let crashErr;
+    try {
+      await officialModules43c.loadExistingConfig(fixtureRoot43c);
+    } catch (error) {
+      crashErr = error;
+    }
+    assert(!crashErr, 'loadExistingConfig does not crash on a scalar core/config.yaml', crashErr?.stack);
+
+    assert(
+      officialModules43c.existingConfig.core?.project_name === 'rescued',
+      'scalar core gets replaced with {} and bmm.project_name still hoists in',
+    );
+
     await fs.remove(fixtureRoot43).catch(() => {});
     await fs.remove(fixtureRoot43b).catch(() => {});
+    await fs.remove(fixtureRoot43c).catch(() => {});
   } catch (error) {
     console.log(`${colors.red}Test Suite 43 setup failed: ${error.message}${colors.reset}`);
     console.log(error.stack);
diff --git a/tools/installer/modules/official-modules.js b/tools/installer/modules/official-modules.js
index 40c885360..615daba86 100644
--- a/tools/installer/modules/official-modules.js
+++ b/tools/installer/modules/official-modules.js
@@ -903,7 +903,10 @@ class OfficialModules {
           try {
             const content = await fs.readFile(moduleConfigPath, 'utf8');
             const moduleConfig = yaml.parse(content);
-            if (moduleConfig) {
+            // Only keep plain object parses. A corrupt config.yaml that parses
+            // to a scalar or array would crash later code that does `key in cfg`
+            // / `Object.keys(cfg)`; treat it the same as a parse error.
+            if (moduleConfig && typeof moduleConfig === 'object' && !Array.isArray(moduleConfig)) {
               this._existingConfig[entry.name] = moduleConfig;
               foundAny = true;
             }
@@ -947,9 +950,15 @@ class OfficialModules {
     );
     if (coreKeys.size === 0) return;
 
-    this._existingConfig.core = this._existingConfig.core || {};
+    // Belt-and-suspenders: loadExistingConfig already filters non-object parses,
+    // but anyone calling _hoistCoreKeysFromLegacyModuleConfigs in isolation (or
+    // future code paths populating _existingConfig directly) shouldn't be able
+    // to crash this with a scalar / array.
+    const existingCore = this._existingConfig.core;
+    this._existingConfig.core = existingCore && typeof existingCore === 'object' && !Array.isArray(existingCore) ? existingCore : {};
+
     for (const [moduleName, cfg] of Object.entries(this._existingConfig)) {
-      if (moduleName === 'core' || !cfg || typeof cfg !== 'object') continue;
+      if (moduleName === 'core' || !cfg || typeof cfg !== 'object' || Array.isArray(cfg)) continue;
       for (const key of Object.keys(cfg)) {
         if (!coreKeys.has(key)) continue;
         if (!(key in this._existingConfig.core)) {