From 1ab4d5f93c57a079aeb59e17990cec4adc31ac3a Mon Sep 17 00:00:00 2001 From: Brian Madison Date: Tue, 28 Apr 2026 19:48:30 -0500 Subject: [PATCH] refactor(installer): simplify --set to a post-install TOML patch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original implementation tried to integrate `--set` with the prompt / result-template / schema-strict-partition system: pre-seeding answers, filtering questions, evaluating function defaults, tracking override keys for partition exemption, mirroring carry-forward in two collection helpers, threading state through Config + ui.js + collection helpers + manifest writer. ~900 lines spawned across 4 review rounds, with bugs the bots kept finding because every change touched a different layer. The simpler model: `--set` is a post-install patch. The installer runs its normal flow untouched, then `applySetOverrides` upserts each value into `_bmad/config.toml` (team scope) or `_bmad/config.user.toml` (user scope) AND into `_bmad//config.yaml` so declared keys carry forward via the existingValue path on the next install. What gets ripped out - All `setOverrides` plumbing through OfficialModules (constructor field, applyOverridesAfterSeeding, _trackUnknownKeysAsOverrides, declaredResultKeys, override classification + pre-write + question-filter + two-pass function-defaults + carry-forward in collectModuleConfig, _trackUnknownKeysAsOverrides calls in collectModuleConfigQuick, headless-branch additions in Installer.build). official-modules.js reset to its pre-#1663 baseline (commit 48a7ec8b). - `setOverrideKeys` field on Config, threading from ui.js, partition exemption parameter on `manifest-generator.writeCentralConfig`. - The "ignored under quick-update" warning in install.js — `--set` is now a uniform post-install patch, so it works the same way for quick-update as for a regular install. What stays - `tools/installer/set-overrides.js` parser with the prototype-pollution guard, prefixed by the new `applySetOverrides` / `upsertTomlKey` / `tomlString` / `tomlHasKey` helpers. - `tools/installer/list-options.js` — small standalone discovery helper, untouched. - The `--set` and `--list-options` CLI flag registration in `commands/install.js`. - ui.js `collectModuleConfigs` retains the early-feedback warning for overrides targeting modules not in the install set (and now also filters them out of `setOverrides` before threading). Routing rules (post-install patch) - If `_bmad/config.user.toml` already has `[section] key`, update it there (so user-scope keys like `core.user_name` and `bmm.user_skill_level` keep their proper file). - Otherwise update `_bmad/config.toml` (team scope, default). - A module without `_bmad//config.yaml` (i.e. not installed) is skipped silently — no orphan `[modules.notamodule]` sections. Tradeoffs documented in `docs/how-to/install-bmad.md` - Values are written verbatim — no `result:` template rendering. Pass `--set bmm.project_knowledge='{project-root}/research'` if you want the rendered form. - Carry-forward is automatic for declared schema keys (per-module yaml → existingValue → prompt default → accepted under --yes). For keys outside any module's schema, the value lands in `config.toml` for the current install but won't be re-emitted on the next install. Re-pass `--set` if you need it sticky. - No "key not in schema" validation — whatever you assert is written. Tests: Suite 44 rewritten. 355 passing (was 351). Coverage now focused on what matters: parser + pollution guard, tomlString escaping, upsertTomlKey across insert/replace/missing-section/empty-file/ preserved-newline cases, applySetOverrides happy path + uninstalled- module skip + missing-user-toml-creation + empty-input no-op, discoverOfficialModuleYamls / formatOptionsList sanity. E2E smoke verified across all 6 scenarios: 1. fresh install with mixed declared + undeclared --set → correct files 2. quick-update no --set → declared keys persist via per-module yaml 3. quick-update WITH --set → applies (used to be warned + dropped) 4. --set for unselected module → warned, no orphan section 5. prototype pollution → exit 1 6. --list-options bmm exit 0, --list-options nope exit 1 Net: -158 lines vs HEAD. The complex integration was load-bearing for edge cases nobody actually needed; the simple post-install patch covers the real use case (script a config value from CI) without the schema gymnastics. --- docs/how-to/install-bmad.md | 51 ++- test/test-installation-components.js | 464 ++++++++------------ tools/installer/commands/install.js | 12 +- tools/installer/core/config.js | 16 +- tools/installer/core/installer.js | 32 +- tools/installer/core/manifest-generator.js | 19 +- tools/installer/modules/official-modules.js | 295 +------------ tools/installer/set-overrides.js | 287 +++++++++++- tools/installer/ui.js | 40 +- 9 files changed, 529 insertions(+), 687 deletions(-) diff --git a/docs/how-to/install-bmad.md b/docs/how-to/install-bmad.md index 4bb940dc8..224704a47 100644 --- a/docs/how-to/install-bmad.md +++ b/docs/how-to/install-bmad.md @@ -117,22 +117,22 @@ Under `--yes`, patch and minor upgrades apply automatically. Majors stay frozen ### Flag reference -| Flag | Purpose | -| ------------------------------------------------------------------------------------------ | ---------------------------------------------------------------------------------- | -| `--yes`, `-y` | Skip all prompts; accept flag values + defaults | -| `--directory ` | Install into this directory (default: current working dir) | -| `--modules ` | Exact module set. Core is auto-added. Not a delta — list everything you want kept. | -| `--tools ` | IDE/tool selection. Required for fresh `--yes` installs. Run `--list-tools` for valid IDs. | -| `--list-tools` | Print all supported tool/IDE IDs (with target directories) and exit. | -| `--action ` | `install`, `update`, or `quick-update`. Defaults based on existing install state. | -| `--custom-source ` | Install custom modules from Git URLs or local paths | -| `--channel ` | Apply to all externals (aliased as `--all-stable` / `--all-next`) | -| `--all-stable` | Alias for `--channel=stable` | -| `--all-next` | Alias for `--channel=next` | -| `--next=` | Put one module on next. Repeatable. | -| `--pin =` | Pin one module to a specific tag. Repeatable. | +| Flag | Purpose | +| ------------------------------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------- | +| `--yes`, `-y` | Skip all prompts; accept flag values + defaults | +| `--directory ` | Install into this directory (default: current working dir) | +| `--modules ` | Exact module set. Core is auto-added. Not a delta — list everything you want kept. | +| `--tools ` | IDE/tool selection. Required for fresh `--yes` installs. Run `--list-tools` for valid IDs. | +| `--list-tools` | Print all supported tool/IDE IDs (with target directories) and exit. | +| `--action ` | `install`, `update`, or `quick-update`. Defaults based on existing install state. | +| `--custom-source ` | Install custom modules from Git URLs or local paths | +| `--channel ` | Apply to all externals (aliased as `--all-stable` / `--all-next`) | +| `--all-stable` | Alias for `--channel=stable` | +| `--all-next` | Alias for `--channel=next` | +| `--next=` | Put one module on next. Repeatable. | +| `--pin =` | Pin one module to a specific tag. Repeatable. | | `--set .=` | Set any module config option non-interactively (preferred — see [Module config overrides](#module-config-overrides)). Repeatable. | -| `--list-options [module]` | Print every `--set` key for built-in and locally-cached official modules, then exit. Pass a module code to scope to one module. | +| `--list-options [module]` | Print every `--set` key for built-in and locally-cached official modules, then exit. Pass a module code to scope to one module. | | `--user-name`, `--communication-language`, `--document-output-language`, `--output-folder` | Legacy shortcuts equivalent to `--set core.=` (still supported) | Precedence when flags overlap: `--pin` beats `--next=` beats `--channel` / `--all-*` beats the registry default (`stable`). @@ -183,9 +183,9 @@ npx bmad-method install --yes --action update \ ### Module config overrides -`--set .=` is the preferred way to provide answers that would otherwise be asked interactively. It's repeatable, scales to every module, and survives across upgrades because the values land in `_bmad/config.toml` next to the rest of your install state. +`--set .=` lets you set any module config option non-interactively. It's repeatable and scales to every module — present and future. The flag is applied as a post-install patch: the installer runs its normal flow first, then `--set` upserts each value into `_bmad/config.toml` (team scope) or `_bmad/config.user.toml` (user scope), and into `_bmad//config.yaml` so declared values carry forward to the next install. -**Example — install bmm without using `docs/` for project knowledge:** +**Example — install bmm with explicit project knowledge and skill level:** ```bash npx bmad-method install --yes \ @@ -203,16 +203,19 @@ npx bmad-method install --list-options bmm `--list-options` (no argument) lists every key the installer can find locally — built-in modules (`core`, `bmm`) plus any currently cached official modules. The cache is per-machine and can be cleared, so previously installed officials won't appear on a fresh checkout or an ephemeral CI worker until they're installed again. Community and custom modules aren't enumerated here; read the module's `module.yaml` directly to see what keys it declares. -**Validation rules:** +**How it works:** -- `` must be in `--modules` (or core, which is always installed). Setting a value for a module you didn't include prints a warning and the value is ignored. -- `` is matched against the module's `module.yaml` declarations. An unknown key prints a warning but still gets persisted to `config.toml` — useful for forward-compatibility or for community modules whose schema isn't validated here. -- `single-select` values are not validated against the allowed choices. The value lands in config as-is, even if it falls outside the module's enumeration. +- **Routing.** The patch step looks for `[modules.] ` (or `[core] `) in `config.user.toml` first; if found there, it updates that file. Otherwise it writes to the team-scope `config.toml`. So user-scope keys (e.g. `core.user_name`, `bmm.user_skill_level`) end up in `config.user.toml` and team-scope keys end up in `config.toml`, matching the partition the installer uses. +- **Verbatim values.** The value is written exactly as you provided it — no `result:` template rendering. To get the rendered form (e.g. `{project-root}/research`), pass it explicitly: `--set bmm.project_knowledge='{project-root}/research'`. +- **Carry-forward, declared keys.** Values for keys declared in `module.yaml` survive subsequent installs because they're also written to `_bmad//config.yaml`, which the installer reads as the prompt default on the next run. +- **Carry-forward, undeclared keys.** A value for a key the module's schema doesn't declare lands in `config.toml` for the current install but won't be re-emitted on the next install (the manifest writer's schema-strict partition drops unknown keys). Re-pass `--set` if you need it sticky, or edit `_bmad/config.toml` directly. +- **No validation.** `single-select` values aren't checked against the allowed choices, and unknown keys aren't rejected — whatever you assert is written. +- **Modules not in `--modules`.** Setting a value for a module you didn't include prints a warning and the value is dropped (no file gets created for an uninstalled module). -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. +The legacy core shortcuts (`--user-name`, `--output-folder`, etc.) still work and remain documented for backward compatibility, but `--set core.user_name=...` is equivalent. -:::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 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. +:::note[Works with quick-update] +`--set` is a post-install patch, so it applies the same way regardless of action type. Under `bmad install --action quick-update` (or `--yes` against an existing install, where quick-update is the default), `--set` patches the central config files at the end just like a regular install. ::: :::caution[Rate limit on shared IPs] diff --git a/test/test-installation-components.js b/test/test-installation-components.js index d25160cce..4447f9010 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -2988,21 +2988,17 @@ async function runTests() { // ============================================================ console.log(`${colors.yellow}Test Suite 44: --set CLI overrides${colors.reset}\n`); try { - const { parseSetEntry, parseSetEntries } = require('../tools/installer/set-overrides'); + const { parseSetEntry, parseSetEntries, applySetOverrides, upsertTomlKey, tomlString } = require('../tools/installer/set-overrides'); const { discoverOfficialModuleYamls, formatOptionsList } = require('../tools/installer/list-options'); - // parseSetEntry — happy path + // ---- Parser ---------------------------------------------------------- const ok = parseSetEntry('bmm.project_knowledge=research'); assert( ok.module === 'bmm' && ok.key === 'project_knowledge' && ok.value === 'research', 'parseSetEntry splits .= correctly', ); + assert(parseSetEntry('bmm.weird=a=b=c').value === 'a=b=c', 'parseSetEntry preserves additional "=" inside the value'); - // parseSetEntry — value containing '=' - const okEq = parseSetEntry('bmm.weird=a=b=c'); - assert(okEq.value === 'a=b=c', 'parseSetEntry preserves additional "=" inside the value'); - - // parseSetEntry — malformed inputs const badInputs = ['no-equals', 'no-dot=value', '=value', '.=value', 'foo.=value', '.bar=value', '']; let allBadThrow = true; for (const bad of badInputs) { @@ -3015,23 +3011,17 @@ async function runTests() { } assert(allBadThrow, `parseSetEntry rejects malformed inputs (${badInputs.length} cases)`); - // parseSetEntries — multiple entries collapse into a {module: {key: value}} map const multi = parseSetEntries(['bmm.project_knowledge=research', 'bmm.user_skill_level=expert', 'core.user_name=Brian']); assert( multi.bmm.project_knowledge === 'research' && multi.bmm.user_skill_level === 'expert' && multi.core.user_name === 'Brian', 'parseSetEntries groups by module', ); - - // parseSetEntries — later entry wins for the same key - const later = parseSetEntries(['bmm.x=first', 'bmm.x=second']); - assert(later.bmm.x === 'second', 'parseSetEntries: later --set entry overrides earlier'); - - // parseSetEntries — non-array / missing input → empty object + assert(parseSetEntries(['bmm.x=first', 'bmm.x=second']).bmm.x === 'second', 'parseSetEntries: later --set entry overrides earlier'); const empty = parseSetEntries(); assert(empty && Object.keys(empty).length === 0, 'parseSetEntries() returns empty object when called without args'); - // parseSetEntries — prototype-pollution guard. `--set __proto__.x=1` would - // otherwise reach `overrides.__proto__[x] = 1` and pollute Object.prototype. + // Prototype-pollution guard. `--set __proto__.x=1` would otherwise reach + // `overrides.__proto__[x] = 1` and pollute every plain object. const polluteProbe = {}; let pollutionThrown = false; try { @@ -3049,40 +3039,188 @@ async function runTests() { } assert(constructorThrown, 'parseSetEntries rejects "constructor" as a key name'); - // discoverOfficialModuleYamls + formatOptionsList read the on-disk - // external-module cache. Point that env at a temp dir so test results - // don't depend on whatever the developer / CI runner has cached. + // ---- tomlString ------------------------------------------------------ + assert(tomlString('hello') === '"hello"', 'tomlString quotes a plain string'); + assert(tomlString('with "quotes"') === String.raw`"with \"quotes\""`, 'tomlString escapes embedded double-quotes'); + assert(tomlString(String.raw`back\slash`) === String.raw`"back\\slash"`, 'tomlString escapes backslashes'); + assert(tomlString('line1\nline2') === String.raw`"line1\nline2"`, 'tomlString escapes newlines'); + + // ---- upsertTomlKey: insert into existing section --------------------- + { + const before = `[core]\nuser_name = "Brian"\n\n[modules.bmm]\nproject_knowledge = "{project-root}/docs"\n`; + const after = upsertTomlKey(before, '[modules.bmm]', 'future_thing', '"persists"'); + assert(after.includes('future_thing = "persists"'), 'upsertTomlKey inserts a new key into an existing section'); + assert(/project_knowledge = "{project-root}\/docs"/.test(after), 'upsertTomlKey preserves existing keys'); + } + + // ---- upsertTomlKey: replace existing key, keep comment tail ---------- + { + const before = `[core]\nuser_name = "old" # set on first install\n`; + const after = upsertTomlKey(before, '[core]', 'user_name', '"Brian"'); + assert(/user_name = "Brian"\s+# set on first install/.test(after), 'upsertTomlKey preserves trailing comments'); + assert(!after.includes('"old"'), 'upsertTomlKey replaces the prior value'); + } + + // ---- upsertTomlKey: section missing → append new section ------------- + { + const before = `[core]\nuser_name = "Brian"\n`; + const after = upsertTomlKey(before, '[modules.bmm]', 'project_knowledge', '"research"'); + assert(after.includes('[modules.bmm]'), 'upsertTomlKey appends a new section when missing'); + assert(after.includes('project_knowledge = "research"'), 'upsertTomlKey appends the key under the new section'); + // Existing section remains untouched + assert(after.indexOf('[core]') < after.indexOf('[modules.bmm]'), 'upsertTomlKey adds the new section AFTER existing content'); + } + + // ---- upsertTomlKey: empty file --------------------------------------- + { + const after = upsertTomlKey('', '[core]', 'user_name', '"Brian"'); + assert(after.startsWith('[core]'), 'upsertTomlKey on an empty string emits the section header'); + assert(after.includes('user_name = "Brian"'), 'upsertTomlKey on an empty string writes the key'); + } + + // ---- upsertTomlKey: trailing newline preserved ----------------------- + { + const withTrailing = upsertTomlKey('[core]\nuser_name = "old"\n', '[core]', 'user_name', '"new"'); + assert(withTrailing.endsWith('\n'), 'upsertTomlKey preserves trailing newline'); + const withoutTrailing = upsertTomlKey('[core]\nuser_name = "old"', '[core]', 'user_name', '"new"'); + assert(!withoutTrailing.endsWith('\n'), 'upsertTomlKey preserves absence of trailing newline'); + } + + // ---- applySetOverrides happy path ------------------------------------ + { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-applyset-')); + const bmadDir = path.join(tmp, '_bmad'); + await fs.ensureDir(bmadDir); + // Seed a realistic post-install state: team config has bmm.project_knowledge, + // user config has core.user_name. The applySetOverrides router should + // route bmm.user_skill_level → user.toml (already there), core.user_name + // update → user.toml (already there), and a brand-new key → team.toml. + await fs.writeFile( + path.join(bmadDir, 'config.toml'), + '[core]\nproject_name = "demo"\n\n[modules.bmm]\nproject_knowledge = "{project-root}/docs"\n', + 'utf8', + ); + await fs.writeFile( + path.join(bmadDir, 'config.user.toml'), + '[core]\nuser_name = "OldName"\n\n[modules.bmm]\nuser_skill_level = "intermediate"\n', + 'utf8', + ); + // Per-module config.yaml stubs are the "is this module installed?" + // signal applySetOverrides uses to skip uninstalled-module overrides. + await fs.ensureDir(path.join(bmadDir, 'core')); + await fs.writeFile(path.join(bmadDir, 'core', 'config.yaml'), 'project_name: demo\n', 'utf8'); + await fs.ensureDir(path.join(bmadDir, 'bmm')); + await fs.writeFile( + path.join(bmadDir, 'bmm', 'config.yaml'), + 'project_knowledge: "{project-root}/docs"\nuser_skill_level: intermediate\n', + 'utf8', + ); + + const overrides = { + core: { user_name: 'Brian' }, + bmm: { user_skill_level: 'expert', future_thing: 'persists' }, + }; + const applied = await applySetOverrides(overrides, bmadDir); + + const team = await fs.readFile(path.join(bmadDir, 'config.toml'), 'utf8'); + const user = await fs.readFile(path.join(bmadDir, 'config.user.toml'), 'utf8'); + + assert(user.includes('user_name = "Brian"'), 'applySetOverrides updates user-scope key in config.user.toml'); + assert(user.includes('user_skill_level = "expert"'), 'applySetOverrides updates pre-existing user-scope key in config.user.toml'); + assert(team.includes('future_thing = "persists"'), 'applySetOverrides routes brand-new key to team config.toml'); + assert(team.includes('project_knowledge = "{project-root}/docs"'), 'applySetOverrides leaves untouched team keys alone'); + assert(!team.includes('user_name = "Brian"'), 'applySetOverrides does NOT duplicate user-scope key into team file'); + + const summary = applied + .map((a) => `${a.module}.${a.key}->${a.scope}`) + .sort() + .join(','); + assert( + summary === 'bmm.future_thing->team,bmm.user_skill_level->user,core.user_name->user', + `applySetOverrides reports correct routing decisions (got: ${summary})`, + ); + + await fs.remove(tmp).catch(() => {}); + } + + // ---- applySetOverrides creates config.user.toml if missing ----------- + { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-applyset-nouser-')); + const bmadDir = path.join(tmp, '_bmad'); + await fs.ensureDir(bmadDir); + await fs.writeFile(path.join(bmadDir, 'config.toml'), '[core]\nuser_name = "Brian"\n', 'utf8'); + await fs.ensureDir(path.join(bmadDir, 'core')); + await fs.writeFile(path.join(bmadDir, 'core', 'config.yaml'), 'user_name: Brian\n', 'utf8'); + // Override targets a key only in team config; routes to team. user.toml + // never gets created in this case (correct — no user-scope writes). + await applySetOverrides({ core: { user_name: 'Updated' } }, bmadDir); + const team = await fs.readFile(path.join(bmadDir, 'config.toml'), 'utf8'); + assert(team.includes('user_name = "Updated"'), 'applySetOverrides updates team key when user.toml is absent'); + assert( + !(await fs.pathExists(path.join(bmadDir, 'config.user.toml'))), + 'applySetOverrides does not create config.user.toml unnecessarily', + ); + await fs.remove(tmp).catch(() => {}); + } + + // ---- applySetOverrides skips modules without per-module config.yaml -- + { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-applyset-skip-')); + const bmadDir = path.join(tmp, '_bmad'); + await fs.ensureDir(bmadDir); + await fs.writeFile(path.join(bmadDir, 'config.toml'), '[core]\nuser_name = "Brian"\n', 'utf8'); + await fs.ensureDir(path.join(bmadDir, 'core')); + await fs.writeFile(path.join(bmadDir, 'core', 'config.yaml'), 'user_name: Brian\n', 'utf8'); + // bmm is not installed (no `_bmad/bmm/config.yaml`). The override for + // bmm should be silently skipped, no `[modules.bmm]` section created. + const applied = await applySetOverrides({ bmm: { foo: 'bar' }, core: { user_name: 'Updated' } }, bmadDir); + const team = await fs.readFile(path.join(bmadDir, 'config.toml'), 'utf8'); + assert(!team.includes('[modules.bmm]'), 'applySetOverrides does NOT create section for uninstalled module'); + assert(team.includes('user_name = "Updated"'), 'applySetOverrides still applies overrides for installed modules'); + assert(applied.length === 1 && applied[0].module === 'core', 'applySetOverrides reports only the installed-module entries'); + await fs.remove(tmp).catch(() => {}); + } + + // ---- applySetOverrides: empty/missing input is a no-op --------------- + { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-applyset-empty-')); + const bmadDir = path.join(tmp, '_bmad'); + await fs.ensureDir(bmadDir); + const empty1 = await applySetOverrides({}, bmadDir); + const empty2 = await applySetOverrides(null, bmadDir); + const empty3 = await applySetOverrides(undefined, bmadDir); + assert( + empty1.length === 0 && empty2.length === 0 && empty3.length === 0, + 'applySetOverrides is a no-op for empty/null/undefined input', + ); + await fs.remove(tmp).catch(() => {}); + } + + // ---- discoverOfficialModuleYamls + formatOptionsList ----------------- + // These read the on-disk external-module cache. Point that env at a temp + // dir so test results don't depend on whatever the developer / CI runner + // has cached. const priorCacheEnv44 = process.env.BMAD_EXTERNAL_MODULES_CACHE; const tempCacheDir44 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-list-options-cache-')); process.env.BMAD_EXTERNAL_MODULES_CACHE = tempCacheDir44; try { - // discoverOfficialModuleYamls includes core and bmm built-ins. const discovered = await discoverOfficialModuleYamls(); const codes = new Set(discovered.map((d) => d.code)); assert(codes.has('core') && codes.has('bmm'), 'discoverOfficialModuleYamls finds core and bmm built-ins'); - const coreEntry = discovered.find((d) => d.code === 'core'); - assert(coreEntry && coreEntry.source === 'built-in', 'core is reported with source="built-in"'); - // formatOptionsList rendering: bmm-only filter shows the project_knowledge key from issue #1663. const bmmListing = await formatOptionsList('bmm'); assert(bmmListing.ok === true, '--list-options bmm reports ok: true'); assert(bmmListing.text.includes('bmm.project_knowledge'), '--list-options bmm renders bmm.project_knowledge'); assert(bmmListing.text.includes('bmm.user_skill_level'), '--list-options bmm renders bmm.user_skill_level'); - assert(bmmListing.text.includes('beginner | intermediate | expert'), '--list-options renders single-select choices'); - // Case-insensitive match: `--list-options BMM` and `bmm` resolve to the same entry. - const bmmUpperListing = await formatOptionsList('BMM'); - assert(bmmUpperListing.ok === true, '--list-options BMM (uppercase) finds the bmm built-in'); - assert(bmmUpperListing.text.includes('bmm.project_knowledge'), '--list-options BMM renders bmm.project_knowledge'); + // Case-insensitive filter. + const bmmUpper = await formatOptionsList('BMM'); + assert(bmmUpper.ok === true && bmmUpper.text.includes('bmm.project_knowledge'), '--list-options is case-insensitive'); - // formatOptionsList for an unknown module gives a helpful message AND ok: false - // so install.js can exit non-zero (CI scripts can detect typos). - const unknownListing = await formatOptionsList('definitely-not-a-module'); - assert(unknownListing.ok === false, '--list-options reports ok: false (non-zero exit signal)'); - assert( - unknownListing.text.includes("No locally-known module.yaml for 'definitely-not-a-module'"), - '--list-options handles unknown module gracefully', - ); + // Unknown module → non-zero exit signal. + const unknown = await formatOptionsList('definitely-not-a-module'); + assert(unknown.ok === false, '--list-options reports ok: false'); + assert(unknown.text.includes('No locally-known module.yaml'), '--list-options unknown explains the miss'); } finally { if (priorCacheEnv44 === undefined) { delete process.env.BMAD_EXTERNAL_MODULES_CACHE; @@ -3091,260 +3229,6 @@ async function runTests() { } await fs.remove(tempCacheDir44).catch(() => {}); } - - // partition() in writeCentralConfig respects setOverrideKeys: an unknown key - // for a known schema must survive when the user asserted it via --set. - const tmp44 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-fixture-44-')); - const bmadDir44 = path.join(tmp44, '_bmad'); - await fs.ensureDir(bmadDir44); - const mg = new ManifestGenerator({ ides: [] }); - mg.updatedModules = ['core', 'bmm']; - - const moduleConfigsForWrite = { - core: { user_name: 'Brian' }, - bmm: { project_knowledge: '/proj/research', future_thing: 'pre-seeded' }, - }; - const setOverrideKeys = { bmm: ['future_thing'] }; - - await mg.writeCentralConfig(bmadDir44, moduleConfigsForWrite, setOverrideKeys); - const teamToml = await fs.readFile(path.join(bmadDir44, 'config.toml'), 'utf8'); - assert(teamToml.includes('project_knowledge = "/proj/research"'), 'writeCentralConfig writes a known schema key'); - assert(teamToml.includes('future_thing = "pre-seeded"'), 'writeCentralConfig keeps an unknown key listed in setOverrideKeys'); - - // Same fixture, no override → unknown key is dropped (control case). - const tmp44b = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-fixture-44b-')); - const bmadDir44b = path.join(tmp44b, '_bmad'); - await fs.ensureDir(bmadDir44b); - const mg2 = new ManifestGenerator({ ides: [] }); - mg2.updatedModules = ['core', 'bmm']; - await mg2.writeCentralConfig(bmadDir44b, moduleConfigsForWrite, {}); - const teamToml2 = await fs.readFile(path.join(bmadDir44b, 'config.toml'), 'utf8'); - assert( - !teamToml2.includes('future_thing'), - 'writeCentralConfig drops an unknown key when not asserted via --set (schema-strict default holds)', - ); - - 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(() => {}); - - // applyOverridesAfterSeeding mirrors the carry-forward behavior for the - // skip-collection path used by `core` (when seeded by --yes / legacy - // shortcuts) so unknown core keys persisted on a prior run survive - // subsequent installs even without re-passing --set. - try { - const om = new OfficialModules({ - // No new --set entries this run — only prior persisted unknown. - setOverrides: {}, - }); - om._existingConfig = { core: { future_core_thing: 'persisted-from-run-1' } }; - // Simulate the seeded-core state ui.js leaves behind under --yes. - om.collectedConfig.core = { user_name: 'Brian', project_name: 'demo' }; - await om.applyOverridesAfterSeeding('core'); - - assert( - om.collectedConfig.core?.future_core_thing === 'persisted-from-run-1', - 'applyOverridesAfterSeeding carries unknown core key forward from _existingConfig', - ); - assert(om.setOverrideKeys?.core?.has('future_core_thing'), 'carried-forward core keys are tracked in setOverrideKeys'); - assert(!om.setOverrideKeys?.core?.has('user_name'), 'declared core keys (user_name) are not flagged as overrides'); - } catch (error) { - console.log(`${colors.red} applyOverridesAfterSeeding carry-forward failed: ${error.message}${colors.reset}`); - console.log(error.stack); - failed++; - } - - // Config.build threads setOverrides through to the headless build path so - // a non-UI caller (`installer.install({ ..., setOverrides })`) can drive - // collection from raw flags. UI path takes the early-return on - // moduleConfigs, so this field is only consumed when build() runs - // collection itself. - try { - const { Config } = require('../tools/installer/core/config'); - const cfg = Config.build({ - directory: '/tmp/anywhere', - modules: ['core', 'bmm'], - ides: [], - actionType: 'install', - coreConfig: { user_name: 'Brian' }, - setOverrides: { bmm: { user_skill_level: 'expert' } }, - }); - assert( - cfg.setOverrides?.bmm?.user_skill_level === 'expert', - 'Config.build carries setOverrides through to the headless install path', - ); - const cfgEmpty = Config.build({ - directory: '/tmp/anywhere', - modules: ['core'], - ides: [], - actionType: 'install', - }); - assert( - cfgEmpty.setOverrides && Object.keys(cfgEmpty.setOverrides).length === 0, - 'Config.build defaults setOverrides to {} when omitted', - ); - } catch (error) { - console.log(`${colors.red} Config.build setOverrides threading failed: ${error.message}${colors.reset}`); - console.log(error.stack); - failed++; - } - - // formatOptionsList: when a cached module.yaml parses to a non-object - // (scalar/array), report a diagnostic so CLI/CI logs explain why the - // listing is empty, and signal failure for module-scoped queries. - try { - const tempCacheNonObj = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-list-options-cache-nonobj-')); - const priorCacheEnvNonObj = process.env.BMAD_EXTERNAL_MODULES_CACHE; - process.env.BMAD_EXTERNAL_MODULES_CACHE = tempCacheNonObj; - try { - const fakeModDir = path.join(tempCacheNonObj, 'fakemod', 'src'); - await fs.ensureDir(fakeModDir); - // module.yaml with scalar content — parses to a string, not an object. - await fs.writeFile(path.join(fakeModDir, 'module.yaml'), 'just-a-scalar-string\n', 'utf8'); - // Synthesize a minimal external-modules registry hint so discovery - // includes this entry. Caches index by directory layout — discovery - // walks subdirectories. The exact layout depends on the cache schema; - // if discovery doesn't pick this up, the test still passes because - // formatOptionsList just won't find it (no false-failure). - const { formatOptionsList } = require('../tools/installer/list-options'); - const nonObjListing = await formatOptionsList('fakemod'); - // Either we got a diagnostic for fakemod, or the entry wasn't - // discovered at all (in which case unknown-module fallback runs). - if (nonObjListing.text.includes('fakemod')) { - assert( - nonObjListing.text.includes('not a valid object') || nonObjListing.text.includes('No locally-known module.yaml'), - 'formatOptionsList prints a diagnostic when module.yaml is a non-object scalar', - ); - assert(nonObjListing.ok === false, 'formatOptionsList reports ok:false for non-object module.yaml'); - } - } finally { - if (priorCacheEnvNonObj === undefined) { - delete process.env.BMAD_EXTERNAL_MODULES_CACHE; - } else { - process.env.BMAD_EXTERNAL_MODULES_CACHE = priorCacheEnvNonObj; - } - await fs.remove(tempCacheNonObj).catch(() => {}); - } - } catch (error) { - console.log(`${colors.red} formatOptionsList non-object diagnostic failed: ${error.message}${colors.reset}`); - 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/commands/install.js b/tools/installer/commands/install.js index 12e00b8a8..1dfe6fb70 100644 --- a/tools/installer/commands/install.js +++ b/tools/installer/commands/install.js @@ -102,13 +102,13 @@ module.exports = { process.exit(0); } - // Handle quick update separately + // Handle quick update separately. --set is a post-install TOML patch so + // it works the same way for quick-update as for a regular install — the + // installer runs, then `applySetOverrides` patches the central config + // files. Pass the parsed overrides through. if (config.actionType === 'quick-update') { - if (options.set && options.set.length > 0) { - await prompts.log.warn( - '--set flags are ignored under quick-update (it preserves existing answers). Re-run with --action update to apply them.', - ); - } + const { parseSetEntries } = require('../set-overrides'); + config.setOverrides = parseSetEntries(options.set || []); const result = await installer.quickUpdate(config); await prompts.log.success('Quick update complete!'); await prompts.log.info(`Updated ${result.moduleCount} modules with preserved settings (${result.modules.join(', ')})`); diff --git a/tools/installer/core/config.js b/tools/installer/core/config.js index f2c35b14c..39617de4c 100644 --- a/tools/installer/core/config.js +++ b/tools/installer/core/config.js @@ -14,7 +14,6 @@ class Config { moduleConfigs, quickUpdate, channelOptions, - setOverrideKeys, setOverrides, }) { this.directory = directory; @@ -28,16 +27,10 @@ class Config { this._quickUpdate = quickUpdate; // channelOptions carry a Map + Set; don't deep-freeze. this.channelOptions = channelOptions || null; - // Per-module list of keys originating from `--set .=` - // that are NOT in the module's prompt schema. The manifest writer keeps - // these through the schema-strict partition so user-asserted overrides - // survive into config.toml even when the schema doesn't declare them. - this.setOverrideKeys = setOverrideKeys || {}; - // Raw `--set` values keyed by module/key. The UI flow applies these in - // `collectModuleConfigs` and populates `moduleConfigs`, so this field is - // primarily for the non-UI path where `OfficialModules.build` runs - // headless collection itself and needs the raw values to pre-seed - // answers and skip prompts. + // Parsed `--set .=` overrides, applied as a TOML + // patch AFTER the install finishes. Shape: { moduleCode: { key: value } }. + // Intentionally NOT integrated with the prompt/template/schema flow; see + // `tools/installer/set-overrides.js` for the rationale and tradeoffs. this.setOverrides = setOverrides || {}; Object.freeze(this); } @@ -64,7 +57,6 @@ class Config { moduleConfigs: userInput.moduleConfigs || null, quickUpdate: userInput._quickUpdate || false, channelOptions: userInput.channelOptions || null, - setOverrideKeys: userInput.setOverrideKeys || {}, setOverrides: userInput.setOverrides || {}, }); } diff --git a/tools/installer/core/installer.js b/tools/installer/core/installer.js index ac1af0a58..4952c89e1 100644 --- a/tools/installer/core/installer.js +++ b/tools/installer/core/installer.js @@ -308,9 +308,21 @@ class Installer { ides: config.ides || [], preservedModules: modulesForCsvPreserve, moduleConfigs, - setOverrideKeys: config.setOverrideKeys || {}, }); + // Apply post-install --set TOML patches. Runs after writeCentralConfig + // (inside generateManifests above) so the patch operates on the + // freshly written `_bmad/config.toml` / `_bmad/config.user.toml`. + // See `tools/installer/set-overrides.js` for routing rules. + if (config.setOverrides && Object.keys(config.setOverrides).length > 0) { + const { applySetOverrides } = require('../set-overrides'); + const applied = await applySetOverrides(config.setOverrides, paths.bmadDir); + if (applied.length > 0) { + const summary = applied.map((a) => `${a.module}.${a.key} → ${a.file}`).join(', '); + await prompts.log.info(`Applied --set overrides: ${summary}`); + } + } + message('Generating help catalog...'); await this.mergeModuleHelpCatalogs(paths.bmadDir, manifestGen.agents); addResult('Help catalog', 'ok'); @@ -1277,19 +1289,6 @@ 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, @@ -1297,7 +1296,10 @@ class Installer { ides: configuredIdes, coreConfig: quickModules.collectedConfig.core, moduleConfigs: quickModules.collectedConfig, - setOverrideKeys, + // Forward `--set` overrides so the post-install patch step + // (`applySetOverrides`) runs at the end of quick-update too. The + // installer.install path applies them after writeCentralConfig. + setOverrides: config.setOverrides || {}, actionType: 'install', _quickUpdate: true, _preserveModules: skippedModules, diff --git a/tools/installer/core/manifest-generator.js b/tools/installer/core/manifest-generator.js index 8993b1d40..f7b5d0084 100644 --- a/tools/installer/core/manifest-generator.js +++ b/tools/installer/core/manifest-generator.js @@ -81,11 +81,7 @@ class ManifestGenerator { await this.collectAgentsFromModuleYaml(); // Write manifest files and collect their paths - const [teamConfigPath, userConfigPath] = await this.writeCentralConfig( - bmadDir, - options.moduleConfigs || {}, - options.setOverrideKeys || {}, - ); + const [teamConfigPath, userConfigPath] = await this.writeCentralConfig(bmadDir, options.moduleConfigs || {}); const manifestFiles = [ await this.writeMainManifest(cfgDir), await this.writeSkillManifest(cfgDir), @@ -429,7 +425,7 @@ class ManifestGenerator { * _bmad/custom/config.toml and _bmad/custom/config.user.toml (never touched by installer). * @returns {string[]} Paths to the written config files */ - async writeCentralConfig(bmadDir, moduleConfigs, setOverrideKeys = {}) { + async writeCentralConfig(bmadDir, moduleConfigs) { const teamPath = path.join(bmadDir, 'config.toml'); const userPath = path.join(bmadDir, 'config.user.toml'); @@ -478,20 +474,17 @@ class ManifestGenerator { // Partition a module's answered config into team vs user buckets. // For non-core modules: strip core keys always; when we know the module's - // own schema, also drop keys it doesn't declare — unless the user - // asserted them via `--set .=`, in which case we - // keep them (warn-and-write semantics from issue #1663). Unknown-schema - // modules (external / marketplace) fall through with their remaining - // answers as team so they don't vanish from the config. + // own schema, also drop keys it doesn't declare. Unknown-schema modules + // (external / marketplace) fall through with their remaining answers as + // team so they don't vanish from the config. const partition = (moduleName, cfg, onlyDeclaredKeys = false) => { const team = {}; const user = {}; const scopes = scopeByModuleKey[moduleName] || {}; const isCore = moduleName === 'core'; - const overrideKeys = new Set(setOverrideKeys[moduleName] || []); for (const [key, value] of Object.entries(cfg || {})) { if (!isCore && coreKeys.has(key)) continue; - if (onlyDeclaredKeys && !(key in scopes) && !overrideKeys.has(key)) continue; + if (onlyDeclaredKeys && !(key in scopes)) continue; if (scopes[key] === 'user') { user[key] = value; } else { diff --git a/tools/installer/modules/official-modules.js b/tools/installer/modules/official-modules.js index f400eec9e..615daba86 100644 --- a/tools/installer/modules/official-modules.js +++ b/tools/installer/modules/official-modules.js @@ -20,88 +20,6 @@ class OfficialModules { // pre-install config collection and the install step agree on which ref // to clone. this.channelOptions = options.channelOptions || null; - // Per-module CLI overrides from `--set .=`. - // Shape: { moduleCode: { key: rawStringValue } }. Keys matching a - // declared prompt skip the prompt; unknown keys are persisted with - // a warning so future / community modules can opt in. - 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] || {}; - const priorConfig = this._existingConfig?.[moduleName]; - const hasPrior = priorConfig && typeof priorConfig === 'object' && !Array.isArray(priorConfig); - - if (Object.keys(overrides).length === 0 && !hasPrior) return; - - if (!this.collectedConfig[moduleName]) this.collectedConfig[moduleName] = {}; - for (const [key, value] of Object.entries(overrides)) { - this.collectedConfig[moduleName][key] = value; - } - - if (!this.setOverrideKeys) this.setOverrideKeys = {}; - if (!this.setOverrideKeys[moduleName]) this.setOverrideKeys[moduleName] = new Set(); - - // Try to load the module's schema. When available we can distinguish - // declared keys from unknown ones; when not (built-in is missing or - // unparseable — rare for `core`), we treat every prior + override key as - // unknown so carry-forward still runs and writeCentralConfig keeps them. - 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 — fall through to no-schema behavior - } - } - const declaredKeys = new Set(); - if (schema && typeof schema === 'object') { - for (const [key, decl] of Object.entries(schema)) { - if (decl && typeof decl === 'object' && 'prompt' in decl) declaredKeys.add(key); - } - } - - // Warn + track unknown keys from this run's --set entries. - for (const key of Object.keys(overrides)) { - if (!declaredKeys.has(key)) { - 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); - } - } - - // Carry forward any non-schema keys persisted by a prior install. Mirrors - // the carry-forward logic in `collectModuleConfig` so the skip-collection - // path (e.g. core under --yes) doesn't drop unknown keys on subsequent - // runs. Without this, `--set core.future=x` lands in config.toml on run #1 - // and would silently disappear on the next install. (#1663 forward-compat) - if (hasPrior) { - for (const [key, value] of Object.entries(priorConfig)) { - if (declaredKeys.has(key)) continue; - if (key in this.collectedConfig[moduleName]) continue; // already set this run - this.collectedConfig[moduleName][key] = value; - this.setOverrideKeys[moduleName].add(key); - } - } } /** @@ -125,15 +43,7 @@ class OfficialModules { * @returns {OfficialModules} */ static async build(config, paths) { - // setOverrides flows through Config so the non-UI / direct-call path - // (`installer.install({ ..., setOverrides })` without going through - // `ui.collectModuleConfigs`) can still apply `--set` values when this - // helper drives headless collection itself. The UI path takes the - // early-return below because `moduleConfigs` is already populated. - const instance = new OfficialModules({ - channelOptions: config.channelOptions, - setOverrides: config.setOverrides, - }); + const instance = new OfficialModules({ channelOptions: config.channelOptions }); // Pre-collected by UI or quickUpdate — store and load existing for path-change detection if (config.moduleConfigs) { @@ -153,22 +63,10 @@ class OfficialModules { const toCollect = config.hasCoreConfig() ? config.modules.filter((m) => m !== 'core') : [...config.modules]; - // Load existing config so applyOverridesAfterSeeding (called below for - // core, and inside collectModuleConfig for the rest) can carry forward - // previously persisted unknown keys. - await instance.loadExistingConfig(paths.projectRoot); - await instance.collectAllConfigurations(toCollect, paths.projectRoot, { skipPrompts: config.skipPrompts, }); - // Mirror the UI path: when core was seeded by config (legacy core-shortcut - // flags or `--yes` defaults), `collectAllConfigurations` skips it, so its - // `--set core.=` overrides need a separate application pass. - if (config.hasCoreConfig()) { - await instance.applyOverridesAfterSeeding('core'); - } - return instance; } @@ -1266,42 +1164,6 @@ 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 @@ -1335,10 +1197,6 @@ 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; } @@ -1346,8 +1204,6 @@ class OfficialModules { const moduleConfig = yaml.parse(configContent); if (!moduleConfig) { - // Schema unparseable — same fallback as missing schema. - this._trackUnknownKeysAsOverrides(moduleName, null); return false; } @@ -1366,7 +1222,6 @@ 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 } @@ -1421,9 +1276,6 @@ 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 } @@ -1511,11 +1363,6 @@ 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) @@ -1650,13 +1497,6 @@ class OfficialModules { const questions = []; const staticAnswers = {}; const configKeys = Object.keys(moduleConfig).filter((key) => key !== 'prompt'); - const declaredPromptKeys = new Set(); - // Schema-declared keys that have a `result` template but no `prompt`. These - // are computed at install time from the answer bag (e.g. `{value}` / - // `{other_key}` substitution) rather than asked. A `--set` for one of - // these is a real override of the *output* — pre-seed it as an answer so - // the existing result-template loop renders the user-supplied value. - const declaredResultKeys = new Set(); for (const key of configKeys) { const item = moduleConfig[key]; @@ -1670,13 +1510,11 @@ class OfficialModules { if (!item.prompt && item.result) { // Add to static answers with a marker value staticAnswers[`${moduleName}_${key}`] = undefined; - declaredResultKeys.add(key); continue; } // Handle interactive values (with prompt) if (item.prompt) { - declaredPromptKeys.add(key); const question = await this.buildQuestion(moduleName, key, item, moduleConfig); if (question) { questions.push(question); @@ -1684,68 +1522,8 @@ class OfficialModules { } } - // Apply --set .= overrides for this module. - // - Known prompt key → answer pre-filled, prompt skipped (interactive + --yes). - // - Known result-only → pre-seeded as the answer for the result template - // (raw value substitutes into `{value}` so `result:` placeholders still - // render). Treated as schema-declared, so no warning and no need to - // exempt from the manifest writer's schema-strict partition. - // - Unknown → warn, then write directly to collectedConfig at - // end of this method. The corresponding key is also tracked on - // this.setOverrideKeys so writeCentralConfig keeps it through the - // partition. - const moduleOverrides = this.setOverrides[moduleName] || {}; - const seededOverrideKeys = new Set(); - const unknownOverrideKeys = []; - for (const [overrideKey, overrideValue] of Object.entries(moduleOverrides)) { - if (declaredPromptKeys.has(overrideKey) || declaredResultKeys.has(overrideKey)) { - seededOverrideKeys.add(overrideKey); - } else { - unknownOverrideKeys.push([overrideKey, overrideValue]); - } - } - - if (unknownOverrideKeys.length > 0) { - for (const [overrideKey] of unknownOverrideKeys) { - await prompts.log.warn( - `--set ${moduleName}.${overrideKey} — '${overrideKey}' is not a declared config key for module '${moduleName}'; persisted but unused by current install.`, - ); - } - } - - // Collect all answers (static + prompted). Pre-seed override answers - // so the prompt loop and skipPrompts path both see them as already-set. + // Collect all answers (static + prompted) let allAnswers = { ...staticAnswers }; - for (const key of seededOverrideKeys) { - allAnswers[`${moduleName}_${key}`] = moduleOverrides[key]; - } - // Pre-write raw override values into collectedConfig so dynamic-default - // resolvers (`buildQuestion`'s function default) can see them when a - // sibling key uses a `{other_key}` placeholder. The fallback chain in - // that closure is: current prompt batch → `this.collectedConfig[mod]`, - // and overridden keys are removed from the prompt batch — without this - // pre-write the placeholder lookup would miss them. The raw value is - // overwritten with the template-rendered version after prompts complete. - if (seededOverrideKeys.size > 0) { - if (!this.collectedConfig[moduleName]) this.collectedConfig[moduleName] = {}; - for (const key of seededOverrideKeys) { - this.collectedConfig[moduleName][key] = moduleOverrides[key]; - } - } - // Drop pre-seeded questions so the user is not re-prompted and so - // skipPrompts mode doesn't overwrite the override with the default. - // 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(', '); - await prompts.log.info(`Applying --set overrides: ${list}`); - } // If there are questions to ask, prompt for accepting defaults vs customizing if (questions.length > 0) { @@ -1754,27 +1532,13 @@ class OfficialModules { // Skip prompts mode: use all defaults without asking if (this.skipPrompts) { await prompts.log.info(`Using default configuration for ${moduleDisplayName}`); - // Two passes: write static defaults first so dynamic-default functions - // can resolve sibling `{other_key}` placeholders against a populated - // answer bag. Without this second pass, function defaults are dropped - // entirely under --yes, even though the interactive prompt UI would - // have evaluated them — headless installs would silently lose - // same-module computed defaults. + // Use defaults for all questions for (const question of questions) { const hasDefault = question.default !== undefined && question.default !== null && question.default !== ''; if (hasDefault && typeof question.default !== 'function') { allAnswers[question.name] = question.default; } } - for (const question of questions) { - if (typeof question.default === 'function') { - try { - allAnswers[question.name] = question.default(allAnswers); - } catch (error) { - await prompts.log.warn(`Could not evaluate dynamic default for ${question.name} under --yes: ${error.message}`); - } - } - } } else { if (!this._silentConfig) await prompts.log.step(`Configuring ${moduleDisplayName}`); let useDefaults = true; @@ -1806,24 +1570,14 @@ class OfficialModules { Object.assign(allAnswers, promptedAnswers); } - // For questions with defaults that weren't asked, we need to process them with their default values. - // Same two-pass treatment as the skipPrompts branch: write static - // defaults first, then evaluate function defaults against the - // populated answer bag so sibling-dependent placeholders resolve. + // For questions with defaults that weren't asked, we need to process them with their default values const questionsWithDefaults = questions.filter((q) => q.default !== undefined && q.default !== null && q.default !== ''); for (const question of questionsWithDefaults) { - if (typeof question.default !== 'function') { - allAnswers[question.name] = question.default; - } - } - for (const question of questionsWithDefaults) { + // Skip function defaults - these are dynamic and will be evaluated later if (typeof question.default === 'function') { - try { - allAnswers[question.name] = question.default(allAnswers); - } catch (error) { - await prompts.log.warn(`Could not evaluate dynamic default for ${question.name}: ${error.message}`); - } + continue; } + allAnswers[question.name] = question.default; } } else { const promptedAnswers = await prompts.prompt(questions); @@ -1976,41 +1730,6 @@ class OfficialModules { } } - // Persist any unknown --set keys for this module (warn-and-write semantics). - // The keys are also tracked so writeCentralConfig keeps them through the - // schema-strict partition for officials. - if (unknownOverrideKeys.length > 0) { - if (!this.collectedConfig[moduleName]) this.collectedConfig[moduleName] = {}; - if (!this.setOverrideKeys) this.setOverrideKeys = {}; - if (!this.setOverrideKeys[moduleName]) this.setOverrideKeys[moduleName] = new Set(); - for (const [overrideKey, overrideValue] of unknownOverrideKeys) { - this.collectedConfig[moduleName][overrideKey] = overrideValue; - this.setOverrideKeys[moduleName].add(overrideKey); - } - } - - // 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, ...declaredResultKeys]); - 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 27a7476eb..9349ee2d6 100644 --- a/tools/installer/set-overrides.js +++ b/tools/installer/set-overrides.js @@ -1,12 +1,31 @@ +// `--set .=` is a post-install patch. The installer runs +// its normal flow and writes `_bmad/config.toml`, `_bmad/config.user.toml`, +// and `_bmad//config.yaml`; afterwards `applySetOverrides` upserts +// each override into those files. +// +// This is intentionally NOT integrated with the prompt/template/schema +// system. Tradeoffs: +// - No `result:` template rendering: `--set bmm.project_knowledge=research` +// writes "research" verbatim. Pass `--set bmm.project_knowledge='{project-root}/research'` +// if you want the rendered form. +// - Carry-forward across installs is best-effort: declared schema keys +// persist via the existingValue path on the next interactive run; values +// for keys outside any module's schema may need to be re-passed on each +// install (or edited directly in `_bmad/config.toml`). +// - No "key not in schema" validation: whatever you assert, we write. +// // Names that, when used as object keys, can mutate `Object.prototype` and -// cascade into every plain-object lookup in the process. The whole `--set` -// pipeline assigns into plain `{}` maps keyed by user input, so a flag like -// `--set __proto__.polluted=1` would otherwise reach -// `overrides.__proto__[key] = value`, which assigns onto Object.prototype. -// We reject both segments at parse time and harden the maps in -// `parseSetEntries` with `Object.create(null)`. +// cascade into every plain-object lookup in the process. The `--set` pipeline +// assigns into plain `{}` maps keyed by user input, so `--set __proto__.x=1` +// would otherwise reach `overrides.__proto__[x] = 1` and pollute every plain +// object. We reject the names at parse time and harden the maps in +// `parseSetEntries` with `Object.create(null)` for defense-in-depth. const PROTOTYPE_POLLUTING_NAMES = new Set(['__proto__', 'prototype', 'constructor']); +const path = require('node:path'); +const fs = require('./fs-native'); +const yaml = require('yaml'); + /** * Parse a single `--set .=` entry. * @param {string} entry - raw flag value @@ -45,12 +64,9 @@ function parseSetEntry(entry) { /** * Parse repeated `--set` entries into a `{ module: { key: value } }` map. - * Later entries overwrite earlier ones for the same key. - * - * Both the outer map and the per-module inner maps are `Object.create(null)` - * so that even if a future caller bypasses `parseSetEntry`'s reserved-name - * check, lookups can't traverse `Object.prototype` and pollution-style writes - * land on the map's own properties (not the global prototype). + * Later entries overwrite earlier ones for the same key. Both the outer + * map and the per-module inner maps are `Object.create(null)` so callers + * that bypass `parseSetEntry`'s name check still can't pollute prototypes. * * @param {string[]} entries * @returns {Object>} @@ -66,4 +82,249 @@ function parseSetEntries(entries) { return overrides; } -module.exports = { parseSetEntry, parseSetEntries }; +/** + * Encode a JS string as a TOML basic string (double-quoted with escapes). + * @param {string} value + */ +function tomlString(value) { + const s = String(value); + // Per the TOML spec, basic strings escape `\`, `"`, and control characters. + return ( + '"' + + s + .replaceAll('\\', '\\\\') + .replaceAll('"', String.raw`\"`) + .replaceAll('\b', String.raw`\b`) + .replaceAll('\f', String.raw`\f`) + .replaceAll('\n', String.raw`\n`) + .replaceAll('\r', String.raw`\r`) + .replaceAll('\t', String.raw`\t`) + + '"' + ); +} + +/** + * Section header for a given module code. + * - `core` → `[core]` + * - `` → `[modules.]` + * + * Mirrors the layout `manifest-generator.writeCentralConfig` produces. + */ +function sectionHeader(moduleCode) { + return moduleCode === 'core' ? '[core]' : `[modules.${moduleCode}]`; +} + +/** + * Insert or update `key = value` inside a TOML section, returning the new + * file content. The format produced by the installer is regular and small + * enough that a line scanner is more reliable than pulling in a TOML + * round-tripper that would normalize the file's existing whitespace and + * comment structure. + * + * - If `[section]` exists and contains `key`, replace the value on that + * line (preserving any inline comment after the value). + * - If `[section]` exists but `key` doesn't, append `key = value` at the + * end of the section (before the next `[...]` header or EOF, skipping + * trailing blank lines so the section stays tidy). + * - If `[section]` doesn't exist, append a new section block at EOF. + * + * @param {string} content existing file content (may be empty) + * @param {string} section exact `[section]` header to target + * @param {string} key + * @param {string} valueToml already TOML-encoded value (e.g. `"foo"`) + * @returns {string} new content + */ +function upsertTomlKey(content, section, key, valueToml) { + const lines = content.split('\n'); + // Track whether the file already ended with a newline so we can preserve + // that. `split('\n')` on `"a\n"` yields `['a', '']`, which gives us the + // marker we need. + const hadTrailingNewline = lines.length > 0 && lines.at(-1) === ''; + if (hadTrailingNewline) lines.pop(); + + // Locate the target section. + const sectionStart = lines.findIndex((line) => line.trim() === section); + if (sectionStart === -1) { + // Section doesn't exist — append a new block. Pad with a blank line if + // the file is non-empty so sections stay visually separated. + if (lines.length > 0 && lines.at(-1).trim() !== '') lines.push(''); + lines.push(section, `${key} = ${valueToml}`); + return lines.join('\n') + (hadTrailingNewline ? '\n' : ''); + } + + // Find the section's end (next `[...]` header or EOF). + let sectionEnd = lines.length; + for (let i = sectionStart + 1; i < lines.length; i++) { + if (/^\s*\[/.test(lines[i])) { + sectionEnd = i; + break; + } + } + + // Look for the key inside the section. Match ` = ...` allowing + // optional leading whitespace; preserve the comment tail (`# ...`) if any. + const keyPattern = new RegExp(`^(\\s*)${escapeRegExp(key)}\\s*=\\s*(.*)$`); + for (let i = sectionStart + 1; i < sectionEnd; i++) { + const match = lines[i].match(keyPattern); + if (match) { + const indent = match[1]; + // Preserve trailing comment if present. We split on the first `#` that + // is preceded by whitespace — TOML strings can't contain unescaped `#` + // in basic-string form so this is safe for the values we emit. + const tail = match[2]; + const commentIdx = tail.search(/\s+#/); + const commentSuffix = commentIdx === -1 ? '' : tail.slice(commentIdx); + lines[i] = `${indent}${key} = ${valueToml}${commentSuffix}`; + return lines.join('\n') + (hadTrailingNewline ? '\n' : ''); + } + } + + // Section exists but key doesn't. Insert before the next section header, + // skipping trailing blank lines inside the current section so the new + // entry sits with its siblings. + let insertAt = sectionEnd; + while (insertAt > sectionStart + 1 && lines[insertAt - 1].trim() === '') { + insertAt--; + } + lines.splice(insertAt, 0, `${key} = ${valueToml}`); + return lines.join('\n') + (hadTrailingNewline ? '\n' : ''); +} + +function escapeRegExp(s) { + return s.replaceAll(/[.*+?^${}()|[\]\\]/g, String.raw`\$&`); +} + +/** + * Look up `[section] key` in a TOML file. Returns true if the file exists, + * the section is present, and `key` is set within it. Used by + * `applySetOverrides` to route an override to the file that already owns + * the key (so user-scope keys land in `config.user.toml`, team-scope keys + * land in `config.toml`). + */ +async function tomlHasKey(filePath, section, key) { + if (!(await fs.pathExists(filePath))) return false; + const content = await fs.readFile(filePath, 'utf8'); + const lines = content.split('\n'); + const sectionStart = lines.findIndex((line) => line.trim() === section); + if (sectionStart === -1) return false; + const keyPattern = new RegExp(`^\\s*${escapeRegExp(key)}\\s*=`); + for (let i = sectionStart + 1; i < lines.length; i++) { + if (/^\s*\[/.test(lines[i])) return false; + if (keyPattern.test(lines[i])) return true; + } + return false; +} + +/** + * Apply parsed `--set` overrides to the central TOML files written by the + * installer. Called at the end of an install / quick-update. + * + * Routing per (module, key): + * 1. If `_bmad/config.user.toml` already has `[section] key`, update there + * (user-scope key like `core.user_name`, `bmm.user_skill_level`). + * 2. Otherwise update `_bmad/config.toml` (team scope, the default). + * + * The schema-correct user/team partition lives in `manifest-generator`. We + * intentionally don't re-read module schemas here — the only goal is to + * match the file the installer just wrote the key to. For brand-new keys + * (not in either file yet), team scope is the safe default. + * + * @param {Object>} overrides + * @param {string} bmadDir absolute path to `_bmad/` + * @returns {Promise>} + * a list of applied entries (for caller logging) + */ +async function applySetOverrides(overrides, bmadDir) { + const applied = []; + if (!overrides || typeof overrides !== 'object') return applied; + + const teamPath = path.join(bmadDir, 'config.toml'); + const userPath = path.join(bmadDir, 'config.user.toml'); + + for (const moduleCode of Object.keys(overrides)) { + // Skip overrides for modules not actually installed. The installer writes + // `_bmad//config.yaml` for every installed module (including core), + // so its presence is a reliable "is this module here?" signal that works + // for both fresh installs and quick-updates without coupling to caller- + // supplied module lists. + const moduleConfigYaml = path.join(bmadDir, moduleCode, 'config.yaml'); + if (!(await fs.pathExists(moduleConfigYaml))) { + continue; + } + + const section = sectionHeader(moduleCode); + const moduleOverrides = overrides[moduleCode] || {}; + for (const key of Object.keys(moduleOverrides)) { + const value = moduleOverrides[key]; + const valueToml = tomlString(value); + + const userOwnsIt = await tomlHasKey(userPath, section, key); + const targetPath = userOwnsIt ? userPath : teamPath; + + // The team file always exists post-install; the user file only exists + // if the install wrote at least one user-scope key. If we're routing to + // it but it doesn't exist yet, create it with a minimal header so it + // has the same shape as installer-written user toml. + let content = ''; + if (await fs.pathExists(targetPath)) { + content = await fs.readFile(targetPath, 'utf8'); + } else { + content = '# Personal overrides for _bmad/config.toml.\n'; + } + + const next = upsertTomlKey(content, section, key, valueToml); + await fs.writeFile(targetPath, next, 'utf8'); + applied.push({ + module: moduleCode, + key, + scope: userOwnsIt ? 'user' : 'team', + file: path.basename(targetPath), + }); + } + + // Also patch the per-module yaml (`_bmad//config.yaml`). The + // installer reads this file as `_existingConfig` on subsequent runs and + // surfaces declared values as prompt defaults — under `--yes` those + // defaults are accepted, so patching here gives `--set` natural + // carry-forward for declared keys without needing schema-strict + // partition exemptions in the manifest writer. For undeclared keys the + // value lives in the per-module yaml but won't be re-emitted into + // config.toml on the next install (the schema-strict partition drops + // it); re-pass `--set` if you need it sticky. + const moduleYamlPath = path.join(bmadDir, moduleCode, 'config.yaml'); + if (await fs.pathExists(moduleYamlPath)) { + try { + const text = await fs.readFile(moduleYamlPath, 'utf8'); + const parsed = yaml.parse(text); + if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { + // Preserve the installer's banner header (everything up to the + // first non-comment line) so `_bmad//config.yaml` keeps + // its provenance comments after we round-trip it. + const headerLines = []; + for (const line of text.split('\n')) { + if (line.startsWith('#') || line.trim() === '') { + headerLines.push(line); + } else { + break; + } + } + for (const key of Object.keys(moduleOverrides)) { + parsed[key] = moduleOverrides[key]; + } + const body = yaml.stringify(parsed, { indent: 2, lineWidth: 0, minContentWidth: 0 }); + const header = headerLines.length > 0 ? headerLines.join('\n') + '\n' : ''; + await fs.writeFile(moduleYamlPath, header + body, 'utf8'); + } + } catch { + // Per-module yaml unparseable — skip silently. The central toml was + // already patched above, which is the user-visible state for the + // current install. Carry-forward will fail next install but the + // current install reflects the override. + } + } + } + + return applied; +} + +module.exports = { parseSetEntry, parseSetEntries, applySetOverrides, upsertTomlKey, tomlString }; diff --git a/tools/installer/ui.js b/tools/installer/ui.js index c20446587..5770206ef 100644 --- a/tools/installer/ui.js +++ b/tools/installer/ui.js @@ -288,7 +288,7 @@ class UI { // Get tool selection const toolSelection = await this.promptToolSelection(confirmedDirectory, options); - const { moduleConfigs, setOverrideKeys, setOverrides } = await this.collectModuleConfigs(confirmedDirectory, selectedModules, { + const { moduleConfigs, setOverrides } = await this.collectModuleConfigs(confirmedDirectory, selectedModules, { ...options, channelOptions, }); @@ -314,7 +314,6 @@ class UI { skipIde: toolSelection.skipIde, coreConfig: moduleConfigs.core || {}, moduleConfigs: moduleConfigs, - setOverrideKeys, setOverrides, skipPrompts: options.yes || false, channelOptions, @@ -367,7 +366,7 @@ class UI { await this._interactiveChannelGate({ options, channelOptions, selectedModules }); let toolSelection = await this.promptToolSelection(confirmedDirectory, options); - const { moduleConfigs, setOverrideKeys, setOverrides } = await this.collectModuleConfigs(confirmedDirectory, selectedModules, { + const { moduleConfigs, setOverrides } = await this.collectModuleConfigs(confirmedDirectory, selectedModules, { ...options, channelOptions, }); @@ -393,7 +392,6 @@ class UI { skipIde: toolSelection.skipIde, coreConfig: moduleConfigs.core || {}, moduleConfigs: moduleConfigs, - setOverrideKeys, setOverrides, skipPrompts: options.yes || false, channelOptions, @@ -715,9 +713,12 @@ class UI { async collectModuleConfigs(directory, modules, options = {}) { const { OfficialModules } = require('./modules/official-modules'); - // Parse --set entries up front so we can both (a) hand them to the config - // collector to skip prompts, and (b) warn about modules referenced in --set - // that aren't part of this install (those values are dropped, not persisted). + // Parse --set up front purely to surface user-error before the install + // burns time on the network / filesystem. The actual application happens + // in installer.install() as a post-write TOML patch — see + // `tools/installer/set-overrides.js`. We also warn about overrides + // targeting modules the user didn't include, since those will silently + // miss the file the patch step looks for. let setOverrides = {}; try { setOverrides = parseSetEntries(options.set || []); @@ -725,16 +726,20 @@ class UI { // install.js validated already; rethrow as-is for the user. throw error; } + // Drop overrides for modules that aren't in the install set so the + // post-install patch step doesn't create orphan sections in config.toml + // for modules that were never installed. const selectedModuleSet = new Set(['core', ...modules]); for (const moduleCode of Object.keys(setOverrides)) { if (!selectedModuleSet.has(moduleCode)) { await prompts.log.warn( `--set ${moduleCode}.* — module '${moduleCode}' is not in the install set; values will be ignored. Add it to --modules to apply.`, ); + delete setOverrides[moduleCode]; } } - const configCollector = new OfficialModules({ channelOptions: options.channelOptions, setOverrides }); + const configCollector = new OfficialModules({ channelOptions: options.channelOptions }); // Seed core config from CLI options if provided if (options.userName || options.communicationLanguage || options.documentOutputLanguage || options.outputFolder) { @@ -799,24 +804,7 @@ class UI { skipPrompts: options.yes || false, }); - // 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. - const setOverrideKeys = {}; - if (configCollector.setOverrideKeys) { - for (const [moduleCode, keys] of Object.entries(configCollector.setOverrideKeys)) { - setOverrideKeys[moduleCode] = [...keys]; - } - } - - return { moduleConfigs: configCollector.collectedConfig, setOverrideKeys, setOverrides }; + return { moduleConfigs: configCollector.collectedConfig, setOverrides }; } /**