feat(installer): add --set and --list-options for non-interactive config - #2353
Closed
bmadcode wants to merge 7 commits into
Closed
feat(installer): add --set and --list-options for non-interactive config#2353bmadcode wants to merge 7 commits into
bmadcode wants to merge 7 commits into
Conversation
…fig (#1663) `--set <module>.<key>=<value>` (repeatable) sets any module config option non-interactively. Scales to every module without growing the CLI surface per option, and persists into _bmad/config.toml so values survive upgrades. `--list-options [module]` prints every available --set key for built-in and locally-cached official modules (community/custom users read their own module.yaml). Pass a module code to scope the listing. Validation rules, all non-fatal: - Module not in --modules → warn and drop the value. - Key not declared in module.yaml → warn but persist (forward-compat). The manifest writer's schema-strict partition exempts these so they survive into config.toml even though the schema doesn't know them. - Malformed --set syntax → exit non-zero up front. The legacy core shortcuts (--user-name, --output-folder, etc.) remain supported as aliases for `--set core.<key>=<value>`. --set with --action quick-update is ignored with a warning since quick-update preserves the existing answers by design. Files: - tools/installer/set-overrides.js (new): parser - tools/installer/list-options.js (new): discovery + formatter - tools/installer/commands/install.js: flags + early validation - tools/installer/ui.js: parse, warn-on-unselected, thread to OfficialModules - tools/installer/modules/official-modules.js: pre-fill answers, persist unknowns - tools/installer/core/config.js + installer.js: carry setOverrideKeys through - tools/installer/core/manifest-generator.js: partition exempts override keys - test/test-installation-components.js: +15 cases (Suite 44) - docs/how-to/install-bmad.md, README.md: --set as preferred non-interactive path Closes #1663
🤖 Augment PR SummarySummary: Adds scalable non-interactive installer configuration via repeatable Changes:
Technical Notes: 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a repeatable CLI Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(63,81,181,0.5)
participant CLI as CLI
end
rect rgba(0,150,136,0.5)
participant InstallCmd as install.js
participant SetParser as set-overrides.js
participant ListOpts as list-options.js
end
rect rgba(255,193,7,0.5)
participant UI as ui.js
participant Official as official-modules.js
end
rect rgba(233,30,99,0.5)
participant Manifest as manifest-generator.js
participant Config as config.js
participant FS as FileSystem
end
CLI->>InstallCmd: invoke with --set / --list-options
alt --list-options
InstallCmd->>ListOpts: formatOptionsList(module?)
ListOpts-->>InstallCmd: rendered list (ok true|false)
InstallCmd-->>CLI: print and exit
else --set flow
InstallCmd->>SetParser: parseSetEntries(args)
SetParser-->>InstallCmd: setOverrides or error
InstallCmd->>UI: collectModuleConfigs({ setOverrides })
UI->>Official: collectModuleConfig / applyOverridesAfterSeeding
Official->>Config: include setOverrides & setOverrideKeys
UI-->>InstallCmd: { moduleConfigs, setOverrideKeys, setOverrides }
InstallCmd->>Manifest: generateManifests(..., setOverrideKeys)
Manifest->>Manifest: writeCentralConfig(..., setOverrideKeys)
Manifest->>FS: persist _bmad/config.toml (retain asserted unknown keys)
Manifest-->>InstallCmd: success
InstallCmd-->>CLI: finish
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tools/installer/list-options.js (1)
62-63: Sort directory entries for deterministic--list-optionsoutputOn Line 62 and Line 75, iteration order depends on filesystem
readdir()ordering, which can vary by platform. Sorting byentry.namewill make output stable and less noisy for docs/tests.Also applies to: 75-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/installer/list-options.js` around lines 62 - 63, The directory iteration over entries from fs.readdir(srcModulesDir, { withFileTypes: true }) is not deterministic; sort the returned Dirent array by entry.name before the for (const entry of entries) loop so --list-options output is stable, and apply the same fix to the other readdir result used on lines 75–76 (the second Dirent array variable) by sorting it by name prior to iteration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/how-to/install-bmad.md`:
- Around line 214-216: Update the note that currently states `--set` flags are
"silently ignored" for `bmad install --action quick-update` to accurately
reflect the feature contract: say that `--set` flags are ignored but a warning
is emitted (e.g., "ignored; a warning will be printed"). Edit the sentence
referencing `--set` and `--action quick-update` so it warns scripted users they
may see CLI warning output during quick-update, and keep the guidance to use
`--action update` (or `bmad install`) to change stored values.
In `@tools/installer/core/manifest-generator.js`:
- Around line 491-495: The code currently builds overrideKeys from
setOverrideKeys[moduleName] only, which causes previously persisted unknown keys
from moduleConfigs to be stripped on update; modify the logic that constructs
overrideKeys (used in the loop over Object.entries(cfg || {}), referencing
setOverrideKeys and moduleConfigs) to also include any undeclared keys present
in moduleConfigs[moduleName] (i.e., add keys from moduleConfigs[moduleName] that
are not in scopes or coreKeys into overrideKeys) so that persisted unknown keys
are treated as retention-eligible on subsequent updates.
In `@tools/installer/list-options.js`:
- Around line 123-125: The code assumes item['single-select'] is an array when
computing values and calling .map(); guard this by checking
Array.isArray(item['single-select']) before mapping (or coerce a single value
into an array), so that malformed but truthy schemas don't throw and abort
--list-options; if it's not an array, skip building values (and optionally
console.warn with context) and continue so lines.push(...) only runs when values
is a non-empty array.
---
Nitpick comments:
In `@tools/installer/list-options.js`:
- Around line 62-63: The directory iteration over entries from
fs.readdir(srcModulesDir, { withFileTypes: true }) is not deterministic; sort
the returned Dirent array by entry.name before the for (const entry of entries)
loop so --list-options output is stable, and apply the same fix to the other
readdir result used on lines 75–76 (the second Dirent array variable) by sorting
it by name prior to iteration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f1f5b5b-5eab-415d-8772-f6a1ea47eec5
📒 Files selected for processing (11)
README.mddocs/how-to/install-bmad.mdtest/test-installation-components.jstools/installer/commands/install.jstools/installer/core/config.jstools/installer/core/installer.jstools/installer/core/manifest-generator.jstools/installer/list-options.jstools/installer/modules/official-modules.jstools/installer/set-overrides.jstools/installer/ui.js
Carry forward unknown --set keys across upgrades (CodeRabbit major). Without this, an unknown key like --set bmm.future_thing=hello landed in config.toml on run #1 but was silently dropped on the next install because collectModuleConfig rebuilds collectedConfig from prompt answers only. collectModuleConfig now copies any non-declared keys from _existingConfig into collectedConfig and tracks them in setOverrideKeys so the manifest writer's schema-strict partition keeps them. Guard single-select rendering with Array.isArray (CodeRabbit major): a malformed truthy non-array would have aborted --list-options. Unify core override handling: move the inline post-collection block from ui.js into OfficialModules.applyOverridesAfterSeeding so core and non-core take a single validated path. Removes duplicated schema-load logic and inline requires from ui.js. Remove dead code: findOfficialModuleYaml and readDeclaredKeys in set-overrides.js were exported but never imported. Drop them and their path/fs/yaml/project-root imports — the module is now pure string-parsing with zero deps. Doc fix: change "silently ignored" to "ignored with a warning" for the --action quick-update note (Augment + CodeRabbit). Polish: clearer flag placeholder (--set <module.key=value> instead of the misleading <key=value>), trim-asymmetry rationale comment in parseSetEntry, dedupe rationale in list-options. Tests: +6 cases — collectModuleConfig --set application end-to-end (prompt-skip with template rendering), and carry-forward of unknown keys from _existingConfig. Total 333 passing.
Collaborator
Author
Review fixes pushedf1c9e12 addresses the bot reviews on this PR. Inline replies posted on each thread; summary:
Beyond the bot comments, also addressed in the same commit:
End-to-end carry-forward verified manually. Test suite: +6 cases, 333 total passing. @coderabbitai full review please @augmentcode review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
| */ | ||
| async function formatOptionsList(moduleCode) { | ||
| const discovered = await discoverOfficialModuleYamls(); | ||
| const filtered = moduleCode ? discovered.filter((d) => d.code === moduleCode) : discovered; |
There was a problem hiding this comment.
tools/installer/list-options.js:151 — formatOptionsList() filters modules with a case-sensitive d.code === moduleCode, even though discovery/deduping is case-insensitive; --list-options BMM (or any case mismatch) can incorrectly report “No locally-known module.yaml”. Consider normalizing the comparison so --list-options behaves consistently with the discovery logic.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Collaborator
Author
There was a problem hiding this comment.
Resolved in ce12cc1. formatOptionsList now lowercases both sides of the filter:
const needle = moduleCode ? moduleCode.toLowerCase() : null;
const filtered = needle ? discovered.filter((d) => d.code.toLowerCase() === needle) : discovered;--list-options BMM and --list-options bmm now both find the bmm built-in. Test added that exercises the uppercase path.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/installer/core/installer.js (1)
307-312:⚠️ Potential issue | 🟠 Major
quick-updatestill drops carried-forward unknown override keys.At Line 311, manifest generation now relies on
config.setOverrideKeysto preserve undeclared--setkeys. ButquickUpdate()in this file builds itsinstallConfigwithout that field, so a previously persisted forward-compatible key is rewritten away the next time the user runs--action quick-update.Suggested fix
const installConfig = { directory: projectDir, modules: modulesToUpdate, ides: configuredIdes, coreConfig: quickModules.collectedConfig.core, moduleConfigs: quickModules.collectedConfig, actionType: 'install', _quickUpdate: true, _preserveModules: skippedModules, _existingModules: installedModules, channelOptions, + setOverrideKeys: quickModules.setOverrideKeys || {}, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/installer/core/installer.js` around lines 307 - 312, The quickUpdate() flow builds an installConfig that omits persisted forward-compatible override keys so manifestGen.generateManifests later loses them; modify quickUpdate() to copy config.setOverrideKeys (e.g. setOverrideKeys: config.setOverrideKeys || {}) into the installConfig object you pass to the installer/generator (same shape expected by manifestGen.generateManifests), ensuring installConfig includes setOverrideKeys so previously carried-forward unknown --set keys are preserved.
🧹 Nitpick comments (1)
test/test-installation-components.js (1)
2986-3157: Add a regression for--action quick-updateignoring--set.Suite 44 covers parsing and persistence well, but it never exercises the documented quick-update branch where overrides must warn and be ignored. A focused CLI-level assertion here would catch future wiring changes that accidentally start mutating config during quick updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-installation-components.js` around lines 2986 - 3157, Add a focused regression test in Suite 44 that asserts --action quick-update does not apply --set overrides: instantiate OfficialModules with setOverrides (e.g. { bmm: { future_thing: 'x' } }), mark skipPrompts/_silentConfig true, simulate the quick-update path by calling the same code path used for quick updates (invoke collectModuleConfig('bmm', tmpDir, true, true) or the CLI quick-update handler if available) and then assert om.collectedConfig.bmm does NOT contain the override and om.setOverrideKeys.bmm does NOT have('future_thing'); also capture/log output and assert a warning message about ignoring --set during quick-update was emitted so the test verifies both the override is ignored and a warning is produced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/test-installation-components.js`:
- Around line 3033-3047: The test reads the external-module cache causing
nondeterminism; before calling discoverOfficialModuleYamls and
formatOptionsList, save process.env.BMAD_EXTERNAL_MODULES_CACHE, set
process.env.BMAD_EXTERNAL_MODULES_CACHE to a new temp directory, run the
assertions (using discoverOfficialModuleYamls, formatOptionsList, etc.), and in
a finally block restore the original env var (or delete it if undefined) to
ensure hermetic behavior; reference BMAD_EXTERNAL_MODULES_CACHE,
discoverOfficialModuleYamls, and formatOptionsList in the changes.
In `@tools/installer/list-options.js`:
- Around line 153-164: The branch in list-options.js that currently returns a
user-facing string when filtered.length === 0 and moduleCode is set must signal
failure to the process; instead of returning the string, throw an Error (e.g.
new Error(theSameMessage)) from that branch so the error propagates to
tools/installer/commands/install.js and yields a non-zero exit, or (if you
prefer not to throw) set process.exitCode = 1 before returning the message;
update the branch handling filtered.length === 0 (referencing the filtered and
moduleCode variables) to implement one of these fixes so a miss like
--list-options bmn results in a non-zero exit.
In `@tools/installer/modules/official-modules.js`:
- Around line 47-79: When applying overrides in applyOverridesAfterSeeding(),
also carry forward previously persisted unknown keys from
this._existingConfig[moduleName] so they aren't dropped when collection was
skipped: after loading schema (or even if overrides is empty), iterate keys in
this._existingConfig[moduleName] that are not declared in schema and not already
present in this.setOverrides[moduleName], add them into
this.collectedConfig[moduleName], and add them into
this.setOverrideKeys[moduleName] (and log the same warning used for new unknown
keys). Use the existing symbols applyOverridesAfterSeeding, this.setOverrides,
this.collectedConfig, this.setOverrideKeys and this._existingConfig to locate
where to merge these non-schema keys before returning.
---
Outside diff comments:
In `@tools/installer/core/installer.js`:
- Around line 307-312: The quickUpdate() flow builds an installConfig that omits
persisted forward-compatible override keys so manifestGen.generateManifests
later loses them; modify quickUpdate() to copy config.setOverrideKeys (e.g.
setOverrideKeys: config.setOverrideKeys || {}) into the installConfig object you
pass to the installer/generator (same shape expected by
manifestGen.generateManifests), ensuring installConfig includes setOverrideKeys
so previously carried-forward unknown --set keys are preserved.
---
Nitpick comments:
In `@test/test-installation-components.js`:
- Around line 2986-3157: Add a focused regression test in Suite 44 that asserts
--action quick-update does not apply --set overrides: instantiate
OfficialModules with setOverrides (e.g. { bmm: { future_thing: 'x' } }), mark
skipPrompts/_silentConfig true, simulate the quick-update path by calling the
same code path used for quick updates (invoke collectModuleConfig('bmm', tmpDir,
true, true) or the CLI quick-update handler if available) and then assert
om.collectedConfig.bmm does NOT contain the override and om.setOverrideKeys.bmm
does NOT have('future_thing'); also capture/log output and assert a warning
message about ignoring --set during quick-update was emitted so the test
verifies both the override is ignored and a warning is produced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0353ad5-b947-49b8-bc41-d0eef908cf07
📒 Files selected for processing (11)
README.mddocs/how-to/install-bmad.mdtest/test-installation-components.jstools/installer/commands/install.jstools/installer/core/config.jstools/installer/core/installer.jstools/installer/core/manifest-generator.jstools/installer/list-options.jstools/installer/modules/official-modules.jstools/installer/set-overrides.jstools/installer/ui.js
(A) Carry forward unknown core keys in applyOverridesAfterSeeding (CodeRabbit major). Mirrors collectModuleConfig's carry-forward so the skip-collection path used by core (when seeded by --yes / legacy shortcuts) doesn't drop unknown keys on subsequent installs. Without this, `--set core.future=x` on run #1 would silently disappear on the next install. (B) --list-options now exits non-zero on a single-module miss (CodeRabbit major). formatOptionsList returns { text, ok }; install.js exits 1 with text on stderr when ok=false, 0 with text on stdout otherwise. CI scripts catch typos like `--list-options bmn`. (C) Hermetic Suite 44 discovery tests (CodeRabbit minor). Point BMAD_EXTERNAL_MODULES_CACHE at a temp dir and restore in a finally block so test results don't depend on the developer / CI cache state. (D) Case-insensitive --list-options filter (Augment). Discovery already dedupes case-insensitively; the filter now matches the same way, so `--list-options BMM` and `--list-options bmm` both find the bmm built-in. Tests: +7 cases (uppercase listing, ok flag, core carry-forward). Total 340 passing.
Collaborator
Author
Round 2 fixes pushedce12cc1 addresses the four new bot comments:
Tests: +7 cases covering uppercase listing, ok-flag, and core carry-forward. Total 340 passing. @coderabbitai full review please @augmentcode review |
|
Triggering a full review of the updated changes now. 🧠 Learnings used✅ Actions performedFull review triggered. |
| // 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}_`, ''))); |
There was a problem hiding this comment.
--set overrides are pre-seeded into allAnswers and the corresponding questions are removed, but buildQuestion’s dynamic-default function resolves {other_key} via the current prompt batch and then this.collectedConfig[moduleName] (which won’t include the override yet). This can cause dependent defaults to ignore a provided --set value when a module uses same-module placeholder defaults.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Collaborator
Author
There was a problem hiding this comment.
Resolved in fb57c81. Pre-write the raw override values to this.collectedConfig[moduleName] BEFORE the prompt batch runs:
if (seededOverrideKeys.size > 0) {
if (!this.collectedConfig[moduleName]) this.collectedConfig[moduleName] = {};
for (const key of seededOverrideKeys) {
this.collectedConfig[moduleName][key] = moduleOverrides[key];
}
}That way the existing fallback chain in buildQuestion's function default — answers[k] → this.collectedConfig[mod][k] — finds sibling override values when computing dynamic defaults. The post-prompt processing later overwrites these raw values with the template-rendered versions, so the final config still goes through the result-template pipeline (e.g. {project-root}/{value}).
| // schema unparseable — skip key-existence validation | ||
| } | ||
| } | ||
| if (!schema || typeof schema !== 'object') return; |
There was a problem hiding this comment.
If module.yaml is missing/unparseable, this return exits before the prior-config carry-forward logic later in the function, so previously persisted unknown keys could still be dropped in that scenario. That also contradicts the comment above that says carry-forward should still happen when the schema can’t be loaded.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Collaborator
Author
There was a problem hiding this comment.
Resolved in fb57c81. Restructured applyOverridesAfterSeeding so the schema-load is best-effort: when unparseable, declaredKeys is left empty, every prior key counts as undeclared, and the carry-forward block at the bottom still runs. Code now matches the comment.
| ], | ||
| ['--list-tools', 'Print all supported tool/IDE IDs (with target directories) and exit.'], | ||
| [ | ||
| '--set <module.key=value>', |
There was a problem hiding this comment.
The option placeholder string says --set <module.key=value>, but the actual contract/docs/parser are <module>.<key>=<value>; the generated CLI help may mislead users into the wrong format.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Collaborator
Author
There was a problem hiding this comment.
Resolved in fb57c81. Changed the placeholder to <spec> and clarified the description so the help text now reads:
--set <spec> Set a module config option non-interactively. Spec format: <module>.<key>=<value> (e.g. bmm.project_knowledge=research). Repeatable. Run --list-options to see available keys.
Avoids the dotted-placeholder ambiguity while keeping the format explicit.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 55-61: Update the README wording to remove the claim that
`--list-options` shows "every available key" and instead state its actual scope:
that `--list-options` lists built-in options and any locally cached official
modules' options but does not include uncached official modules or
community/custom modules; reference the flags `--list-options` and `--set` in
the revised sentence so users understand the limitation when using `--set
<module>.<key>=<value>`.
In `@tools/installer/list-options.js`:
- Around line 179-197: When a module-scoped listing fails to parse module.yaml,
the function should return { ok: false } instead of true; update the loop that
iterates "for (const { code, yamlPath, source } of filtered)" to set a failure
flag (e.g., parseFailed) when the catch branch runs for a specific module and
moduleCode is set, and after the loop change the final "return { text:
sections.join('\n'), ok: true }" to return ok: false when that flag is set.
Ensure the logic around "parsed" and the push to "sections" remains the same and
reference the same symbols (filtered, code, yamlPath, source, parsed, sections,
moduleCode) so tooling exits non-zero on unreadable module.yaml.
In `@tools/installer/set-overrides.js`:
- Around line 24-29: The parsing currently returns { module: moduleCode, key,
value } without defending against prototype-polluting names; after extracting
moduleCode and key in the same block that throws for empty values, add a guard
that rejects reserved prototype property names (e.g. "__proto__", "prototype",
"constructor") for either moduleCode or key and throw a clear Error (e.g. `--set
"${entry}": reserved segment not allowed`) to prevent assignments like
overrides.__proto__ from mutating Object.prototype; update the validation near
the moduleCode/key extraction so callers that populate the overrides map never
use these reserved segments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46d7c668-e3f4-4195-9e1a-bcb10280a7e9
📒 Files selected for processing (11)
README.mddocs/how-to/install-bmad.mdtest/test-installation-components.jstools/installer/commands/install.jstools/installer/core/config.jstools/installer/core/installer.jstools/installer/core/manifest-generator.jstools/installer/list-options.jstools/installer/modules/official-modules.jstools/installer/set-overrides.jstools/installer/ui.js
(J) Prototype pollution guard (CodeRabbit major). `--set __proto__.x=1` previously mutated Object.prototype because `overrides.__proto__` returned Object.prototype on a plain object, and assigning `[key]=value` polluted every plain object in the process. Verified the attack reproduces on f1c9e12 and is now blocked: parser rejects __proto__/prototype/constructor segments, and the maps are Object.create(null) for defense-in-depth. (I) Non-zero exit when --list-options <module>'s yaml is unparseable (CodeRabbit major). formatOptionsList tracks moduleScopedFailure and returns ok:false in that case; install.js exits 1. (F) Dynamic defaults can now see --set sibling values (Augment medium). buildQuestion's function default falls back to `this.collectedConfig[mod][otherKey]`, but overrides were only in `allAnswers` (local) at default-evaluation time. Pre-write override raw values to collectedConfig before the prompt batch so the fallback resolves. Post-prompt template processing overwrites with the rendered version. (E) applyOverridesAfterSeeding no longer bypasses carry-forward when the schema can't be loaded (Augment low). Restructured: schema-load is now best-effort; without schema, declaredKeys is an empty Set, so all overrides are flagged as "unknown" and carry-forward runs against every prior key. Comment now matches behavior. (G) Flag placeholder --set <spec> instead of <module.key=value> (Augment low) — angle brackets in the placeholder were misleading; the description spells out the spec format. (H) README wording: "every available key" → "locally-known official keys (built-in modules plus any external officials cached on this machine)" (CodeRabbit minor) — accurately reflects scope. Tests: +2 cases for prototype-pollution rejection. Total 343 passing.
Collaborator
Author
Round 3 fixes pushedfb57c81 addresses six new bot comments — including a real prototype pollution vulnerability that I confirmed exploitable on the previous commit before fixing.
Tests: +2 cases for prototype-pollution rejection. 343 total passing. @coderabbitai full review please @augmentcode review |
|
343 passing with 2 new prototype-pollution rejection cases is a solid checkpoint. Triggering the full review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
| const seededOverrideKeys = new Set(); | ||
| const unknownOverrideKeys = []; | ||
| for (const [overrideKey, overrideValue] of Object.entries(moduleOverrides)) { | ||
| if (declaredPromptKeys.has(overrideKey)) { |
There was a problem hiding this comment.
declaredPromptKeys only tracks keys that have item.prompt, so a schema key that’s declared as a static result (no prompt) will be treated as “not a declared config key” and go down the unknown-key warn/persist path. That makes the warning potentially incorrect and can also persist a raw override over a computed static result value.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Collaborator
Author
There was a problem hiding this comment.
Resolved in 7ad054f. Added declaredResultKeys parallel to declaredPromptKeys. A schema entry with result: and no prompt: is now treated as schema-declared:
if (declaredPromptKeys.has(overrideKey) || declaredResultKeys.has(overrideKey)) {
seededOverrideKeys.add(overrideKey);
} else {
unknownOverrideKeys.push([overrideKey, overrideValue]);
}For result-only keys this means: the override is pre-seeded as the answer in allAnswers, the existing result-template loop then renders it ({value} substitutes the user's raw input, cross-references like {project-root} still resolve), and no "not a declared config key" warning fires. Carry-forward block at the bottom of the method also refactored to use new Set([...declaredPromptKeys, ...declaredResultKeys]) instead of re-walking configKeys.
Good catch — the prior behavior was both wrong-warning and a correctness bug (raw value stomped the rendered template).
| if (moduleCode) moduleScopedFailure = true; | ||
| continue; | ||
| } | ||
| if (!parsed || typeof parsed !== 'object') { |
There was a problem hiding this comment.
In formatOptionsList(), if module.yaml parses successfully but to a non-object (e.g., scalar/array), moduleScopedFailure flips but no error text is appended, so --list-options <module> can fail with little/no explanation. Consider emitting an explicit “unexpected module.yaml shape” message similar to the catch branch so CLI/CI logs are diagnosable.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Collaborator
Author
There was a problem hiding this comment.
Resolved in 7ad054f. The non-object branch now emits a diagnostic mirroring the catch branch, and the type guard also covers arrays (which typeof reports as 'object'):
if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) {
sections.push(`${code} (${source}): module.yaml is not a valid object (got ${Array.isArray(parsed) ? 'array' : typeof parsed})`, '');
if (moduleCode) moduleScopedFailure = true;
continue;
}--list-options <module> still exits 1 in this case (per the existing moduleScopedFailure → ok:false path), but now the user sees why. New test covers the diagnostic + ok:false signal end-to-end via BMAD_EXTERNAL_MODULES_CACHE pointing at a temp dir with a scalar module.yaml.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/installer/core/installer.js (1)
307-312:⚠️ Potential issue | 🟠 MajorPropagate quick-update override keys into this new manifest path.
This new plumbing still misses the
quickUpdate()caller:installConfigthere never includesquickModules.setOverrideKeys, so carried-forward unknown keys from prior--setruns still get stripped on a quick-update rewrite because this call falls back to{}.💡 Suggested follow-up
coreConfig: quickModules.collectedConfig.core, moduleConfigs: quickModules.collectedConfig, + setOverrideKeys: Object.fromEntries( + Object.entries(quickModules.setOverrideKeys || {}).map(([moduleCode, keys]) => [moduleCode, [...keys]]), + ), actionType: 'install', _quickUpdate: true,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/installer/core/installer.js` around lines 307 - 312, The generateManifests call is dropping quick-update override keys because installConfig only passes config.setOverrideKeys; update the call-site that invokes manifestGen.generateManifests (the block that currently passes { ides: config.ides || [], preservedModules, moduleConfigs, setOverrideKeys: config.setOverrideKeys || {}, }) to include the quick-update keys as a fallback — e.g., use quickModules.setOverrideKeys when config.setOverrideKeys is empty — so quickUpdate() carried keys (quickModules.setOverrideKeys) are propagated into manifest generation instead of being replaced by an empty object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/how-to/install-bmad.md`:
- Around line 204-205: The sentence describing `--list-options` overstates
availability; update the wording to make the cache requirement explicit by
changing “installed at least once on this machine” to “currently cached official
modules” (or similar phrasing) so the line reads that `--list-options` lists
built-in modules plus any currently cached official modules, and note that
cleared caches or ephemeral CI workers may not show previously installed
officials.
In `@tools/installer/commands/install.js`:
- Around line 60-68: The current handling of the --list-options branch calls
stream.write(...) then immediately process.exit(...), which can truncate
buffered output; change the flow in the options.listOptions block (where
formatOptionsList is called and stream.write is used) to wait for the write to
finish (use the writable stream's callback or await stream.write/draint
handling) and then set process.exitCode to ok ? 0 : 1 and return instead of
calling process.exit; ensure this change is applied to the branch surrounding
formatOptionsList, moduleArg, stream.write and the current process.exit call so
output always flushes before the process ends.
In `@tools/installer/modules/official-modules.js`:
- Around line 1633-1650: The headless/skipPrompts branch currently copies only
non-function defaults which drops same-module dynamic defaults; update the
skipPrompts logic (the block that consumes allAnswers / seededOverrideKeys and
writes into this.collectedConfig) to evaluate function defaults the same way the
interactive path does: ensure seeded override keys are pre-written into
this.collectedConfig[moduleName], then for each schema field if the default is a
function call that function with the same context/answer bag used by
buildQuestion (so it can read {other_key} placeholders and module overrides),
take its returned value (and template-render it if you do that elsewhere), and
write that computed value into both allAnswers and
this.collectedConfig[moduleName] so headless installs preserve dynamic
same-module defaults.
- Around line 23-27: The constructor option setOverrides is stored on
this.setOverrides but the place that instantiates OfficialModules (the call that
currently passes only { channelOptions }) doesn't forward those overrides, so
collectModuleConfig() runs with an empty map and misses CLI --set values; update
the instantiation site that calls new OfficialModules(...) to pass the incoming
setOverrides (e.g. new OfficialModules({ channelOptions, setOverrides })) or
thread the setOverrides through the caller so OfficialModules.constructor
receives it, ensuring collectModuleConfig() can read this.setOverrides for
headless/--yes paths.
---
Outside diff comments:
In `@tools/installer/core/installer.js`:
- Around line 307-312: The generateManifests call is dropping quick-update
override keys because installConfig only passes config.setOverrideKeys; update
the call-site that invokes manifestGen.generateManifests (the block that
currently passes { ides: config.ides || [], preservedModules, moduleConfigs,
setOverrideKeys: config.setOverrideKeys || {}, }) to include the quick-update
keys as a fallback — e.g., use quickModules.setOverrideKeys when
config.setOverrideKeys is empty — so quickUpdate() carried keys
(quickModules.setOverrideKeys) are propagated into manifest generation instead
of being replaced by an empty object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b0713eb-1fb9-418d-a530-db237927b448
📒 Files selected for processing (11)
README.mddocs/how-to/install-bmad.mdtest/test-installation-components.jstools/installer/commands/install.jstools/installer/core/config.jstools/installer/core/installer.jstools/installer/core/manifest-generator.jstools/installer/list-options.jstools/installer/modules/official-modules.jstools/installer/set-overrides.jstools/installer/ui.js
(1) Use process.exitCode instead of process.exit() after --list-options
write (CodeRabbit major). process.exit() forces immediate termination
even with pending I/O, which can truncate buffered writes when stdout
is piped or captured by CI. Await the write callback, set exitCode,
and return so the event loop drains naturally.
(2) Thread setOverrides through Config → OfficialModules.build for
headless callers (CodeRabbit major). Non-UI entry points (direct
installer.install({...}) without going through ui.collectModuleConfigs)
previously got an empty override map. Config now carries setOverrides
and the headless branch of OfficialModules.build also runs
loadExistingConfig + applyOverridesAfterSeeding('core') to mirror the
UI path's semantics. The UI path is unaffected because it takes the
moduleConfigs early-return.
(3) Evaluate function defaults under skipPrompts and accept-defaults
paths (CodeRabbit major). Both branches were dropping function defaults
silently, so any same-module dynamic default (`{other_key}` placeholder
in default:) disappeared under --yes. Two-pass: write non-function
defaults first so the answer bag is populated, then call function
defaults with that bag. Try/catch around the call surfaces resolution
failures as warnings instead of crashing the install.
(4) Track result-only schema keys as declared (Augment medium). A
schema entry with `result:` and no `prompt:` was being classified as
"unknown" when targeted by --set, producing a wrong warning and
overwriting the computed template output with the raw value. Added
declaredResultKeys parallel to declaredPromptKeys; an override on
either is now seeded as the answer so the result template still
renders ({value} substitution preserved). Carry-forward block
refactored to consume the same set.
(5) Diagnose non-object module.yaml under --list-options (Augment low).
The non-object branch silently flipped moduleScopedFailure with no
output. Now emits "module.yaml is not a valid object (got <type>)"
mirroring the catch branch, and the type guard also catches arrays
which typeof reports as 'object'.
(6) Reword --list-options doc cache scope (CodeRabbit minor).
"Installed at least once on this machine" → "currently cached official
modules" with a note that cache can be cleared or absent on ephemeral
CI workers — accurately reflects what the command can discover.
Tests: +4 cases — Config.build setOverrides threading and default,
formatOptionsList non-object yaml diagnostic and ok:false. Total 347
passing.
Collaborator
Author
Round 4 fixes pushed7ad054f addresses the six unaddressed bot comments from the round-3 review pass.
Tests: +4 cases ( CLI smoke: @coderabbitai full review please |
|
347 passing with 4 new cases is a solid checkpoint. Triggering the full review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
| ides: config.ides || [], | ||
| preservedModules: modulesForCsvPreserve, | ||
| moduleConfigs, | ||
| setOverrideKeys: config.setOverrideKeys || {}, |
There was a problem hiding this comment.
tools/installer/core/installer.js:311 — In the headless-collection path (when OfficialModules.build() actually collects configs), override-only keys are accumulated on the officialModules instance, but generateManifests() is currently given config.setOverrideKeys, which may still be {}; this can cause unknown --set keys / carried-forward unknown keys to be dropped during writeCentralConfig()’s schema-strict partitioning.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| let count = 0; | ||
| for (const [key, item] of Object.entries(parsed)) { | ||
| if (!item || typeof item !== 'object' || !('prompt' in item)) continue; |
There was a problem hiding this comment.
tools/installer/list-options.js:123 — formatModuleOptions() only lists schema entries that have prompt, but OfficialModules.collectModuleConfig() now treats result-only schema keys as declared and allows --set to override them; --list-options may therefore omit some valid --set keys and make discovery incomplete.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| 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')) { |
There was a problem hiding this comment.
test/test-installation-components.js:3278 — This assertion block is conditional on nonObjListing.text.includes('fakemod'), which means the test can still pass even if cache discovery stops finding the synthesized fakemod/src/module.yaml, reducing coverage of the “non-object YAML should surface a diagnostic + ok:false” behavior.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/installer/core/manifest-generator.js`:
- Around line 491-494: The filtering currently uses the prototype-sensitive
check "key in scopes" which treats inherited properties (e.g., constructor) as
declared; update the declared-key test in the loop that iterates over
Object.entries(cfg) to use an own-property check such as
Object.prototype.hasOwnProperty.call(scopes, key) (or scopes.hasOwnProperty if
preferred) instead of "key in scopes", leaving the surrounding logic with
onlyDeclaredKeys, overrideKeys, isCore, and coreKeys unchanged; ensure you
reference the variables overrideKeys, setOverrideKeys, moduleName, cfg, isCore,
coreKeys, onlyDeclaredKeys, and scopes when making the change.
In `@tools/installer/list-options.js`:
- Around line 121-136: The loop currently skips any schema entries that lack a
'prompt', which omits result-only keys; change the guard so entries are kept if
they have a prompt OR their key is listed in the module's declared result keys
(e.g. check OfficialModules.declaredResultKeys or the appropriate
declaredResultKeys set) — replace the current check (if (!item || typeof item
!== 'object' || !('prompt' in item)) continue;) with a condition that only
continues when item is missing/invalid OR the key is not in declaredResultKeys
and has no prompt, so result-only keys (by key membership in declaredResultKeys)
are included while preserving the existing formatting logic (inferType,
formatPromptText, single-select handling).
In `@tools/installer/modules/official-modules.js`:
- Around line 75-88: Create a single prompt/result-only classifier function
(e.g., isResultOnlyOrPrompt(schemaEntry) used by both the render and seeded-core
paths) and replace the ad-hoc checks that currently only check for 'prompt' in
the schema (the declaredKeys logic) and the seeded-core logic that treats every
non-prompt entry as seedable; use this shared helper to determine whether a key
is declared/result-only. In the render path, when applying an override from
overrides[moduleName] and the classifier marks it as result-only, skip
templating and only inject the override if the string template contains the
"{value}" placeholder; otherwise treat it as a plain seeded-only result (do not
warn or attempt to render). Also update the warning path that calls
prompts.log.warn and this.setOverrideKeys[moduleName].add(key) to rely on the
same classifier so warnings are only emitted for truly undeclared keys (not for
result-only keys).
- Around line 68-73: When parsing module.yaml (the schemaPath block) swallow the
parse exception into a named error (catch (err)) and surface it before falling
back: log a clear warning including schemaPath and err.message/stack (e.g., via
console.warn or the repo's logger) so users see the parse failure, and set
process.exitCode = 1 to mark the tool run as degraded while still allowing the
installer to continue with no-schema behavior; update the try/catch around
yaml.parse(await fs.readFile(schemaPath, 'utf8')) and assignment to schema
accordingly.
In `@tools/installer/ui.js`:
- Around line 729-735: The loop that warns about ignored --set entries
(iterating Object.keys(setOverrides)) currently does not remove those keys, so
setOverrides still contains values for modules not in selectedModuleSet; update
the logic inside that loop (or immediately after it) to delete each ignored
module key from the setOverrides map/object (e.g., delete
setOverrides[moduleCode] or setOverrides.delete(moduleCode) depending on type)
before returning from promptInstall(), ensuring promptInstall() and downstream
code receive the cleaned setOverrides without rejected overrides.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 795d2c8e-d844-4b7c-8220-62c0dc8e8596
📒 Files selected for processing (11)
README.mddocs/how-to/install-bmad.mdtest/test-installation-components.jstools/installer/commands/install.jstools/installer/core/config.jstools/installer/core/installer.jstools/installer/core/manifest-generator.jstools/installer/list-options.jstools/installer/modules/official-modules.jstools/installer/set-overrides.jstools/installer/ui.js
Comment on lines
+491
to
+494
| 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)) continue; | ||
| if (onlyDeclaredKeys && !(key in scopes) && !overrideKeys.has(key)) continue; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's locate and read the file in question
cat -n tools/installer/core/manifest-generator.js | sed -n '485,505p'Repository: bmad-code-org/BMAD-METHOD
Length of output: 1036
🏁 Script executed:
#!/bin/bash
# Search for scopeByModuleKey definition
rg -n "scopeByModuleKey\s*[=:]" tools/installer/core/manifest-generator.js | head -20Repository: bmad-code-org/BMAD-METHOD
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Search for where scopeByModuleKey is populated
rg -n "scopeByModuleKey\[" tools/installer/core/manifest-generator.jsRepository: bmad-code-org/BMAD-METHOD
Length of output: 360
🏁 Script executed:
#!/bin/bash
# Look at the broader context around line 458-461 to understand the structure
cat -n tools/installer/core/manifest-generator.js | sed -n '440,470p'Repository: bmad-code-org/BMAD-METHOD
Length of output: 1799
🏁 Script executed:
#!/bin/bash
# Let's understand what happens with `key in` for inherited properties
node <<'EOF'
// Simulate the scenario
const scopes = {};
scopes['user-key'] = 'team';
// Check behavior with inherited properties
console.log("'constructor' in scopes:", 'constructor' in scopes);
console.log("'toString' in scopes:", 'toString' in scopes);
console.log("scopes.constructor:", scopes.constructor);
console.log("scopes['constructor']:", scopes['constructor']);
// Check with own property check
console.log("\nUsing hasOwnProperty:");
console.log("Object.prototype.hasOwnProperty.call(scopes, 'constructor'):", Object.prototype.hasOwnProperty.call(scopes, 'constructor'));
console.log("Object.prototype.hasOwnProperty.call(scopes, 'user-key'):", Object.prototype.hasOwnProperty.call(scopes, 'user-key'));
EOFRepository: bmad-code-org/BMAD-METHOD
Length of output: 360
🏁 Script executed:
#!/bin/bash
# Check if Object.entries includes inherited properties
node <<'EOF'
const obj = {};
// Object.entries only returns own enumerable properties
console.log("Object.entries result includes inherited properties:");
console.log(Object.entries(obj));
// Even with inherited properties
const objWithInherited = Object.create({inherited: 'value'});
objWithInherited.own = 'value';
console.log("\nObject.entries with inherited property in prototype:");
console.log(Object.entries(objWithInherited));
// The `in` operator checks both own and inherited
console.log("\n'inherited' in objWithInherited:", 'inherited' in objWithInherited);
console.log("Object.prototype.hasOwnProperty.call(objWithInherited, 'inherited'):", Object.prototype.hasOwnProperty.call(objWithInherited, 'inherited'));
EOFRepository: bmad-code-org/BMAD-METHOD
Length of output: 316
🏁 Script executed:
#!/bin/bash
# Let's verify the actual logic flow with the concern
node <<'EOF'
// Simulate the actual scenario in the code
const scopes = {}; // Empty scopes object (no keys added)
// scopes now has inherited properties from Object.prototype
const cfg = { 'constructor': 'some_value', 'normal_key': 'value' };
console.log("Simulating line 494 logic with inherited properties:");
console.log("When key='constructor':");
console.log(" !(key in scopes):", !('constructor' in scopes));
console.log(" Object.prototype.hasOwnProperty.call(scopes, 'constructor'):", Object.prototype.hasOwnProperty.call(scopes, 'constructor'));
console.log("\nWhen key='normal_key' (not in scopes):");
console.log(" !(key in scopes):", !('normal_key' in scopes));
console.log(" Object.prototype.hasOwnProperty.call(scopes, 'normal_key'):", Object.prototype.hasOwnProperty.call(scopes, 'normal_key'));
// The problem: if onlyDeclaredKeys=true and scopes is empty
const onlyDeclaredKeys = true;
const overrideKeys = new Set();
console.log("\nWith current code using 'in' operator:");
for (const [key, value] of Object.entries(cfg || {})) {
const shouldSkip = onlyDeclaredKeys && !(key in scopes) && !overrideKeys.has(key);
console.log(` key='${key}': shouldSkip=${shouldSkip} (because !(key in scopes)=${!(key in scopes)})`);
}
console.log("\nWith proposed fix using hasOwnProperty:");
for (const [key, value] of Object.entries(cfg || {})) {
const isDeclared = Object.prototype.hasOwnProperty.call(scopes, key);
const shouldSkip = onlyDeclaredKeys && !isDeclared && !overrideKeys.has(key);
console.log(` key='${key}': shouldSkip=${shouldSkip} (because isDeclared=${isDeclared})`);
}
EOFRepository: bmad-code-org/BMAD-METHOD
Length of output: 721
Use own-property checks for declared-key filtering.
On line 494, key in scopes matches inherited prototype keys (like constructor), causing undeclared keys to be incorrectly treated as declared. Replace with an own-property check:
Fix
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;
+ const isDeclared = Object.prototype.hasOwnProperty.call(scopes, key);
+ if (onlyDeclaredKeys && !isDeclared && !overrideKeys.has(key)) continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/installer/core/manifest-generator.js` around lines 491 - 494, The
filtering currently uses the prototype-sensitive check "key in scopes" which
treats inherited properties (e.g., constructor) as declared; update the
declared-key test in the loop that iterates over Object.entries(cfg) to use an
own-property check such as Object.prototype.hasOwnProperty.call(scopes, key) (or
scopes.hasOwnProperty if preferred) instead of "key in scopes", leaving the
surrounding logic with onlyDeclaredKeys, overrideKeys, isCore, and coreKeys
unchanged; ensure you reference the variables overrideKeys, setOverrideKeys,
moduleName, cfg, isCore, coreKeys, onlyDeclaredKeys, and scopes when making the
change.
Comment on lines
+121
to
+136
| let count = 0; | ||
| for (const [key, item] of Object.entries(parsed)) { | ||
| if (!item || typeof item !== 'object' || !('prompt' in item)) continue; | ||
| count++; | ||
| const type = inferType(item); | ||
| const scope = item.scope === 'user' ? ' [user-scope]' : ''; | ||
| const defaultStr = item.default === undefined || item.default === null ? '(none)' : String(item.default); | ||
| lines.push(` ${code}.${key} (${type}${scope}) default: ${defaultStr}`); | ||
| const promptText = formatPromptText(item); | ||
| if (promptText) lines.push(` ${promptText}`); | ||
| if (Array.isArray(item['single-select'])) { | ||
| const values = item['single-select'].map((v) => (typeof v === 'object' ? v.value : v)).filter((v) => v !== undefined); | ||
| if (values.length > 0) lines.push(` values: ${values.join(' | ')}`); | ||
| } | ||
| lines.push(''); | ||
| } |
There was a problem hiding this comment.
Include result-only schema keys in --list-options.
OfficialModules already treats both prompted keys and declaredResultKeys as valid --set targets, but Line 123 drops every schema entry that lacks prompt. That means a result-only key can work during install and still never show up in the discovery output.
Suggested fix
let count = 0;
for (const [key, item] of Object.entries(parsed)) {
- if (!item || typeof item !== 'object' || !('prompt' in item)) continue;
+ if (!item || typeof item !== 'object' || Array.isArray(item)) continue;
+ const isSettable = 'prompt' in item || 'result' in item;
+ if (!isSettable) continue;
count++;
const type = inferType(item);
const scope = item.scope === 'user' ? ' [user-scope]' : '';
const defaultStr = item.default === undefined || item.default === null ? '(none)' : String(item.default);
lines.push(` ${code}.${key} (${type}${scope}) default: ${defaultStr}`);
const promptText = formatPromptText(item);
- if (promptText) lines.push(` ${promptText}`);
+ if (promptText) {
+ lines.push(` ${promptText}`);
+ } else if ('result' in item) {
+ lines.push(' result-only key');
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/installer/list-options.js` around lines 121 - 136, The loop currently
skips any schema entries that lack a 'prompt', which omits result-only keys;
change the guard so entries are kept if they have a prompt OR their key is
listed in the module's declared result keys (e.g. check
OfficialModules.declaredResultKeys or the appropriate declaredResultKeys set) —
replace the current check (if (!item || typeof item !== 'object' || !('prompt'
in item)) continue;) with a condition that only continues when item is
missing/invalid OR the key is not in declaredResultKeys and has no prompt, so
result-only keys (by key membership in declaredResultKeys) are included while
preserving the existing formatting logic (inferType, formatPromptText,
single-select handling).
Comment on lines
+68
to
+73
| if (await fs.pathExists(schemaPath)) { | ||
| try { | ||
| schema = yaml.parse(await fs.readFile(schemaPath, 'utf8')); | ||
| } catch { | ||
| // schema unparseable — fall through to no-schema behavior | ||
| } |
There was a problem hiding this comment.
Surface schema parse failures before falling back.
If module.yaml exists but yaml.parse() fails here, the installer silently drops into no-schema mode and then mislabels valid --set keys as undeclared. A warning before the fallback would make the failure diagnosable instead of looking like user error.
As per coding guidelines, tools/**: Build script/tooling. Check error handling and proper exit codes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/installer/modules/official-modules.js` around lines 68 - 73, When
parsing module.yaml (the schemaPath block) swallow the parse exception into a
named error (catch (err)) and surface it before falling back: log a clear
warning including schemaPath and err.message/stack (e.g., via console.warn or
the repo's logger) so users see the parse failure, and set process.exitCode = 1
to mark the tool run as degraded while still allowing the installer to continue
with no-schema behavior; update the try/catch around yaml.parse(await
fs.readFile(schemaPath, 'utf8')) and assignment to schema accordingly.
Comment on lines
+75
to
+88
| 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); |
There was a problem hiding this comment.
Normalize result-only --set handling across both paths.
Line 78 only treats prompt entries as declared, while Lines 1650-1652 treat every result-only entry as seedable. The render path only consumes the seeded value when the template contains {value}, so derived result-only keys can still be ignored during normal collection, and the seeded-core path can still warn on them as if they were unknown. Please share one prompt/result-only classifier between these paths and bypass templating when a result-only override has no {value} placeholder.
Also applies to: 1602-1623, 1649-1655
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/installer/modules/official-modules.js` around lines 75 - 88, Create a
single prompt/result-only classifier function (e.g.,
isResultOnlyOrPrompt(schemaEntry) used by both the render and seeded-core paths)
and replace the ad-hoc checks that currently only check for 'prompt' in the
schema (the declaredKeys logic) and the seeded-core logic that treats every
non-prompt entry as seedable; use this shared helper to determine whether a key
is declared/result-only. In the render path, when applying an override from
overrides[moduleName] and the classifier marks it as result-only, skip
templating and only inject the override if the string template contains the
"{value}" placeholder; otherwise treat it as a plain seeded-only result (do not
warn or attempt to render). Also update the warning path that calls
prompts.log.warn and this.setOverrideKeys[moduleName].add(key) to rely on the
same classifier so warnings are only emitted for truly undeclared keys (not for
result-only keys).
Comment on lines
+729
to
+735
| 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.`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Actually drop ignored --set modules before returning setOverrides.
This warns that the values will be ignored, but Line 819 still returns the untouched setOverrides map and promptInstall() now threads it downstream in both flows. That leaves rejected overrides in the config state even after the UI says they were dropped.
Suggested fix
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];
}
}Also applies to: 819-819
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/installer/ui.js` around lines 729 - 735, The loop that warns about
ignored --set entries (iterating Object.keys(setOverrides)) currently does not
remove those keys, so setOverrides still contains values for modules not in
selectedModuleSet; update the logic inside that loop (or immediately after it)
to delete each ignored module key from the setOverrides map/object (e.g., delete
setOverrides[moduleCode] or setOverrides.delete(moduleCode) depending on type)
before returning from promptInstall(), ensuring promptInstall() and downstream
code receive the cleaned setOverrides without rejected overrides.
…alls 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.
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/<module>/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 48a7ec8). - `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/<module>/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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1663.
Reporter wanted a
--project-knowledgeCLI flag for non-interactive bmm installs. The discussion on the issue surfaced two paths: per-option flags (doesn't scale — every module'smodule.yamlcan declare arbitrarily many prompts) or a config-file approach. This PR pivots to a third shape that's the best of both: a single repeatable--set <module>.<key>=<value>flag that scales to every module, every option, present and future.--list-options [module]is the partner discovery flag — prints every available--setkey for built-in and locally-cached official modules. Community/custom users read their ownmodule.yamldirectly; we don't enumerate those (no network, no extra plumbing).Design choices
--set k=vover inline JSON / config file--seta key for a module that doesn't declare it yet. Lands inconfig.tomlwith a warning, available when the module gains the key.--set widgets.foo=barwithout--modules widgetswould create an orphan TOML section with no schema; honest warning beats invisible junk.single-selectchoices--list-optionsis local-only~/.bmad/cache/external-modules/. No registry fetch, no clone of unknown modules. Community/custom is on the user.Legacy
--user-name,--output-folder, etc. remain as aliases for--set core.<key>=<value>(zero-churn for existing CI scripts).--setwith--action quick-updatewarns and skips, matching quick-update's "preserve existing answers" semantic.Implementation
tools/installer/set-overrides.js(new): parser, throws on malformed inputtools/installer/list-options.js(new): module.yaml discovery + formattertools/installer/commands/install.js: register flags, early syntax validation,--list-optionsexit path, quick-update warningtools/installer/ui.js: parse--set, warn-on-unselected, post-collection core merge (since core is skipped when seeded by--yesdefaults)tools/installer/modules/official-modules.js: pre-fillallAnswersfrom overrides so the prompt loop and--yesskipPrompts path both see them as already-set; persist unknown keystools/installer/core/{config,installer}.js: carrysetOverrideKeysthrough to manifest writertools/installer/core/manifest-generator.js:partition()exempts override-asserted keys from the schema-strict filter so unknown keys survive intoconfig.tomlTest plan
Suite 44 (15 new cases) covers parsing, discovery, formatter rendering, and the manifest writer's exemption logic. End-to-end smoke verified manually:
--set bmm.project_knowledge=research→[modules.bmm] project_knowledge = "{project-root}/research"(template renders)--set core.user_name=Brian→[core] user_name = "Brian"(overrides--yessystem-username default)--set bmm.user_skill_level=expert→config.user.toml(user-scope respected)--set core.unknown_thing=xyz→ warned, persisted toconfig.toml--set freddy-got-fingered.foo=bar(module not selected) → warned, dropped--set→ exits non-zero with a clear error--setwith--action quick-update→ warning, ignored--list-options bmm→ renders bmm.project_knowledge, bmm.user_skill_level (with single-select choices)Docs
docs/how-to/install-bmad.md— flag reference table updated, new "Module config overrides" section with example, validation rules, and quick-update noteREADME.md— added a--setexample below the existing non-interactive snippetdocs/{cs,fr,vi-vn,zh-cn}/) intentionally not touched — they'll lag behind English until your translation pipeline runs