feat(core): add always_show_recommendation config option - #2609
feat(core): add always_show_recommendation config option#2609will-ai-m wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds the user-scoped ChangesRecommendation preference
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmm-skills/module.yaml`:
- Around line 28-40: Document the user-facing bmm.always_show_recommendation
setting in the appropriate docs/ configuration or installation documentation,
including its default value of "true", installation behavior, workflow-wide
effect, and the override command npx bmad-method install --set
bmm.always_show_recommendation=false.
🪄 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: 987c4dd1-c3db-47b7-afb7-376e8146b534
📒 Files selected for processing (4)
src/bmm-skills/4-implementation/bmad-code-review/SKILL.mdsrc/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.mdsrc/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.mdsrc/bmm-skills/module.yaml
|
Thanks for this; super appreciated! Looks great! I won't have any free time to properly look at this until tomorrow ( tues ), but in the meantime could you:
Happy to chat / answer question in this PR before I have proper time etc do a full review tomorrow |
Addresses CodeRabbit review on bmad-code-org#2609. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
done. @jheyworth |
|
Thanks @will-ai-m ! I can't look at this today unfortunately, but will loot at it in the next day or two and update you in the PR with feedback/ questions etc. |
|
@will-ai-m — thanks for yr patience. Full review below: traced through the installer, checked against all six satellite module repos (gds, cis, wds, tea, bmb, bmad-loop), and tested locally — both your branch as-is and the change I'm suggestiung. I've made all the calls so hopefully there's nothing to debate — just implement. TL;DR: move the key from First, credit where due: your branch works end-to-end exactly as your PR body claims — I installed it fresh with Decisions
ChecklistFirst, the new PR (gates this one):
Then, in this PR:
No longer needed
Tested locally — what I actually ranFresh installs into scratch directories with this repo's installer, plus
So keep your schema shape exactly as-is ( Installer fix — prototype diff for the separate PRVerified working (see "Tested locally") and written to match the surrounding style. Lift it as-is into the new PR or reimplement to taste — either way it needs the two regression tests noted in the checklist. diff --git a/tools/installer/set-overrides.js b/tools/installer/set-overrides.js
index 9349ee2d..cd34327d 100644
--- a/tools/installer/set-overrides.js
+++ b/tools/installer/set-overrides.js
@@ -291,8 +291,22 @@ async function applySetOverrides(overrides, bmadDir) {
// 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)) {
+ // Core overrides also refresh the spread copies: core values are spread
+ // into every module's config.yaml at generate time and skills read their
+ // own module's copy — without this, a core --set would not take effect
+ // until the next install regenerates the spread.
+ const yamlTargets = [path.join(bmadDir, moduleCode, 'config.yaml')];
+ if (moduleCode === 'core') {
+ const nonModuleDirs = new Set(['_config', '_memory', 'memory', 'docs', 'scripts', 'custom', 'core']);
+ const entries = await fs.readdir(bmadDir, { withFileTypes: true });
+ for (const entry of entries) {
+ if (entry.isDirectory() && !nonModuleDirs.has(entry.name)) {
+ yamlTargets.push(path.join(bmadDir, entry.name, 'config.yaml'));
+ }
+ }
+ }
+ for (const moduleYamlPath of yamlTargets) {
+ if (!(await fs.pathExists(moduleYamlPath))) continue;
try {
const text = await fs.readFile(moduleYamlPath, 'utf8');
const parsed = yaml.parse(text);
diff --git a/tools/installer/ui.js b/tools/installer/ui.js
index 7adc867f..ddf5ecd3 100644
--- a/tools/installer/ui.js
+++ b/tools/installer/ui.js
@@ -866,6 +866,47 @@ class UI {
}
}
+ // --yes backfill: seed any core keys declared in the schema but absent from
+ // the config assembled above (the fresh-install defaults, CLI-flag seed, or
+ // carried-forward existing config). Without this, a newly declared core key
+ // is silently dropped on every --yes install — core is skipped by
+ // collectAllConfigurations once seeded, so schema defaults never apply.
+ // Value precedence per key: prior [core] answer, then a prior answer under
+ // a module section (key promoted module → core, e.g. bmm → core), then the
+ // schema default. This mirrors _hoistCoreKeysFromLegacyModuleConfigs, which
+ // only runs on the legacy (pre-central-toml) load path.
+ if (options.yes) {
+ try {
+ const yaml = require('yaml');
+ const { getSourcePath } = require('./project-root');
+ const coreSchema = yaml.parse(await fs.readFile(path.join(getSourcePath('core-skills'), 'module.yaml'), 'utf8'));
+ const core = (configCollector.collectedConfig.core ||= {});
+ const existing = configCollector._existingConfig || {};
+ const existingCore = existing.core && typeof existing.core === 'object' && !Array.isArray(existing.core) ? existing.core : {};
+ for (const [key, item] of Object.entries(coreSchema || {})) {
+ if (!item || typeof item !== 'object' || Array.isArray(item) || !item.prompt || key in core) continue;
+ let value = existingCore[key];
+ if (value === undefined) {
+ for (const [moduleName, cfg] of Object.entries(existing)) {
+ if (moduleName === 'core' || !cfg || typeof cfg !== 'object' || Array.isArray(cfg)) continue;
+ if (cfg[key] !== undefined) {
+ value = cfg[key];
+ break;
+ }
+ }
+ }
+ if (value === undefined) {
+ let def = item.default;
+ if (typeof def === 'string') def = def.replace('{directory_name}', path.basename(directory));
+ value = def;
+ }
+ if (value !== undefined && value !== null && value !== '') core[key] = value;
+ }
+ } catch {
+ // Schema unreadable — keep the seeded config as-is rather than fail the install.
+ }
+ }
+
// Collect all module configs — core is skipped if already seeded above
await configCollector.collectAllConfigurations(modules, directory, {
skipPrompts: options.yes || false, |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Candidate files ==='
fd -t f . src tools test docs | rg '(^|/)(module\.yaml|SKILL\.md|step-01-gather-context\.md|step-04-present\.md|install-bmad\.md|customize-bmad\.md|ui\.js|set-overrides\.js|test-installation-components\.js)$' | sort
printf '%s\n' '=== JavaScript structure ==='
ast-grep outline tools/installer/ui.js --items all --type class,function --view expanded
ast-grep outline tools/installer/set-overrides.js --items all --type function --view expanded
ast-grep outline test/test-installation-components.js --items all --type function --view expanded
printf '%s\n' '=== Current preference and documentation references ==='
rg -n -C 5 --glob 'module.yaml' --glob '*.md' --glob '*.js' \
'(always_show_recommendation|recommendation preference|--set (bmm|core)\.always_show_recommendation)' \
src tools test docs
printf '%s\n' '=== Installer paths cited in the review ==='
rg -n -C 8 --glob '*.js' \
'(collectAllConfigurations|collectedConfig\.core|applySetOverrides|config\.yaml|core-skills)' \
tools/installer
printf '%s\n' '=== Installation test references to core and config spreading ==='
rg -n -C 8 --glob 'test-installation-components.js' \
'(config\.user\.toml|\[core\]|modules\.|config\.yaml|spread|core)' \
testLength of output: 165378 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Core and BMM schema conventions ==='
sed -n '1,75p' src/core-skills/module.yaml
sed -n '1,52p' src/bmm-skills/module.yaml
printf '%s\n' '=== Generated module-config spread ==='
sed -n '950,1025p' tools/installer/core/installer.js
printf '%s\n' '=== Existing core-key hoist migration ==='
sed -n '950,1065p' tools/installer/modules/official-modules.js
printf '%s\n' '=== --set routing and generated-YAML patching ==='
sed -n '237,335p' tools/installer/set-overrides.js
printf '%s\n' '=== Existing focused tests ==='
sed -n '1774,1908p' test/test-installation-components.js
sed -n '2948,3062p' test/test-installation-components.js
sed -n '3168,3265p' test/test-installation-components.js
printf '%s\n' '=== English and French documentation sections ==='
rg -n -C 8 --glob 'install-bmad.md' --glob 'customize-bmad.md' \
'(decision-point recommendations|recommendation preference|Cross-cutting state|--set)' \
docs/how-to docs/fr/how-toLength of output: 57540
✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes.
The separate installer fix, including regression coverage for schema-default seeding, propagated core 🐇🔍 ✏️ Learnings added
|
|
@coderabbitai One correction to your analysis above, verified empirically: The right outcome (user's prior value preserved across the module → core promotion) is achievable though, and I agree it should be asserted. I've updated the review comment above: the prototype diff now includes value migration in the @will-ai-m heads-up: the main review comment was edited — prototype diff is now ~60 lines (was ~40) and the migration behavior improved. Everything else stands. |
|
Tip For best results, initiate chat on the files or code changes.
The proposed 🐇🔎 |
|
Tip For best results, initiate chat on the files or code changes.
Prerequisite: installer fix PR
Follow-up: feature PR
|
|
Tip For best results, initiate chat on the files or code changes.
Once the prerequisite PR and revised 🐇🔎 ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes.
I agree with the sequencing:
The legacy Once the prerequisite PR and revised 🐇🔎 |
|
Tip For best results, initiate chat on the files or code changes.
I agree that the prerequisite installer PR should land first, with regression coverage for:
After that, The legacy 🐇🔎 ✏️ Learnings added
|
|
@will-ai-m any thoughts on the above ? |
|
@jheyworth sorry for the delay, taking care of this now |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses CodeRabbit review on bmad-code-org#2609. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rding Review round on bmad-code-org#2609: the preference is cross-cutting ("Should agents recommend...", wanted for any skill per bmad-code-org#2602), so it belongs in core, where one declaration reaches every module's config.yaml via the core spread — not just bmm skills. - schema block moves from src/bmm-skills/module.yaml to src/core-skills/module.yaml unchanged (scope: user, default "true", string-valued single-select) - canonical rule wording in SKILL.md, step-01, and step-04 (step-04 keeps its parenthetical examples), adding the no-grounds clause so evidence-free asks don't manufacture a justification - docs: key becomes core.always_show_recommendation (incl. the --set example); the what-it-does paragraph moves out of the --set mechanics section into the interactive-install section and says "prompted during interactive install"; customize-bmad names the key explicitly; French docs mirror both edits - tests: assert the key lands in [core] of config.user.toml and is spread into a non-core module's generated config.yaml Merges after bmad-code-org#2633, which makes --yes installs seed schema-declared core keys. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
c8ee914 to
5690ec5
Compare
will-ai-m
changed the title
|
@jheyworth Thanks for the thorough review and the tested prototype — agreed with all the calls, everything is now implemented. Prerequisite PR: opened #2633 with your installer diff (lifted as-is) plus the three requested regression tests — This PR (latest push):
One mechanical note: the branch was rebased onto current main, replacing the GitHub-UI merge commit ( |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Greptile SummaryThis PR introduces an
Confidence Score: 5/5Safe to merge — the change is purely additive, default-on behaviour is fully opt-out, and the only runtime effect is instructional text added to an LLM prompt. Every changed file is either documentation, test assertions, or prompt/instruction text. The new config key is user-scoped and defaults to the safe "on" state that the PR is designed to enable. No installer logic, no runtime code paths, and no schema migrations are touched; the spreading mechanism that carries the key to module configs is tested and pre-existing. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/core-skills/module.yaml | Adds always_show_recommendation as a user-scoped, string-valued single-select key (default "true"); follows the same schema pattern as the other user-scoped keys in this file. |
| src/bmm-skills/4-implementation/bmad-code-review/SKILL.md | Adds always_show_recommendation to the config-loading list and a workflow-wide rule in the "Load Config" step; rule text is clear, includes a no-grounds clause, and defers the final decision to the user. |
| src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md | Adds the recommendation rule to the step RULES block; step-02 and step-03 are verified to have no user decision points, so this is the correct place. |
| src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md | Adds the recommendation rule with additional contextual scope (decision-needed findings, patch findings, next steps), which helpfully anchors the rule to the concrete decision moments in this step. |
| test/test-installation-components.js | Adds two assertions: that always_show_recommendation lands in [core] of config.user.toml, and that it is spread into a non-core module's config.yaml. Both assertions cover the critical propagation path for the new key. |
| docs/how-to/install-bmad.md | Adds a prose paragraph explaining the new option as a sub-item within the existing "per-module config" interactive-install step, and adds a --set override example; the "five things" count remains accurate. |
| docs/fr/how-to/install-bmad.md | French translation mirrors the English additions faithfully; CLI commands are untranslated as expected. |
| docs/how-to/customize-bmad.md | Adds always_show_recommendation to the user-scope description line in the config-file map diagram. |
| docs/fr/how-to/customize-bmad.md | French version of the same one-line config-file map update. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["src/core-skills/module.yaml\nalways_show_recommendation\nscope: user, default: 'true'"] --> B["Interactive Install\n(prompted) or --yes\n(silent default)"]
A --> C["--set core.always_show_recommendation=false\n(override at any time)"]
B --> D["_bmad/config.user.toml\n[core]\nalways_show_recommendation = 'true'"]
C --> D
D --> E["generateModuleConfigs()\ncore keys spread into every module"]
E --> F["_bmad/bmm/config.yaml\nalways_show_recommendation: 'true'"]
F --> G["SKILL.md — Step 4: Load Config\nresolves {always_show_recommendation}"]
G --> H{"always_show_recommendation\nis 'true'?"}
H -- Yes --> I["step-01: at every user decision point\nstate recommended option + 1-2 line justification\nor say 'no grounds to prefer'"]
H -- Yes --> J["step-04: at decision-needed / patch /\nnext-steps choices\nsame recommendation rule"]
H -- No --> K["Present options only\n(no recommendation)"]
Reviews (3): Last reviewed commit: "Merge branch 'main' into feature/always-..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/how-to/install-bmad.md`:
- Line 41: Update the documentation sentence describing
core.always_show_recommendation in docs/how-to/install-bmad.md at lines 41-41 to
state that /bmad-code-review is the current consumer, optionally noting future
expansion; apply the same scope correction in French at
docs/fr/how-to/install-bmad.md lines 41-41, without implying all decision-point
workflows currently support the setting.
🪄 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 Plus
Run ID: 31c368e0-3340-45a0-976c-2c754c773ba0
📒 Files selected for processing (9)
docs/fr/how-to/customize-bmad.mddocs/fr/how-to/install-bmad.mddocs/how-to/customize-bmad.mddocs/how-to/install-bmad.mdsrc/bmm-skills/4-implementation/bmad-code-review/SKILL.mdsrc/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.mdsrc/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.mdsrc/core-skills/module.yamltest/test-installation-components.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md
- src/bmm-skills/4-implementation/bmad-code-review/SKILL.md
- src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md
|
@will-ai-m — outstanding delivery on both PRs. I've verified everything locally before this review, so let me lead with that: Verified locally ✓
The extra edge guards in Suite 49 (precedence, non-module dirs, One final pass, then we mergeThe bot reviews landed after your push, so none are addressed yet. All are small; three are defects in my prototype that you inherited by lifting it as-is, so consider these my bugs to own and yours to fix: #2633 — four fixes:
#2633 — two optional test additions (CodeRabbit; recommended but your call):
#2609 — one word:
Process, same as before: disposition each bot comment in-thread (fixed / skipped-with-reason), then re-request review from both bots on both PRs. Merge order stands: #2633 first, then #2609. Once the pass is done and the bots are green, both are ready from my side. |
Bot-review pass on bmad-code-org#2609: say "workflows wired to the option — currently /bmad-code-review" instead of implying every decision-point workflow already consumes it. English and French mirrors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@jheyworth Final pass done on both PRs: all four #2633 fixes landed in c6b9446 (plus both optional test additions — Suite 49 is now 8 scenarios, 408/408 component tests), and the #2609 docs scope wording is fixed in 1e7b485 (EN + FR). Every bot comment is dispositioned in-thread and fresh reviews are requested from both bots on both PRs. Full |
|
@coderabbitai review |
|
@greptileai review |
✅ Action performedReview finished.
|
|
Thanks for this @will-ai-m !. I will review over the next day or so |
|
@jheyworth bump :) |
|
@will-ai-m am waiting on a an imminent version bump of BMM, before progressing this as I’m guessing that there might be som impact on this PR. I hope that might be over the next few days. |
2 participants
What
Adds an
always_show_recommendationconfig option to core and wires it into/bmad-code-reviewso the agent states a recommended option with a brief justification whenever it asks the user to choose.Why
At decision points the agent presents options without an opinion, leaving the user to weigh trade-offs alone even when the agent has enough context to pre-digest them.
Fixes #2602
How
always_show_recommendationkey insrc/core-skills/module.yaml(user scope, default"true", string-valued single-select to match how--setand carry-forward handle values). Core values are spread into every module's generatedconfig.yaml, so the key reaches skills in any module; behavior wiring is intentionally limited tobmad-code-reviewfor now — later adoption is purely additive. Changeable any time vianpx bmad-method install --set core.always_show_recommendation=falseSKILL.mdand in the RULES of step-01 and step-04 (the two steps with user decision points; step-04 keeps its contextual examples), including a no-grounds clause so evidence-free asks don't manufacture a justification--set core.…example in the overrides section[core]ofconfig.user.tomland is spread into a non-core module's generatedconfig.yamlDepends on #2633 — installer fix that seeds schema-declared core keys on
--yesinstalls and propagates core--setoverrides to module config spreads. This PR merges after it.Testing
Installer component tests (394/394 incl. the two new assertions), refs/urls/channels/skills validators, eslint, markdownlint, and prettier all pass locally.
test:rendererfails on this machine on pristine main as well (local Python 3.9 lackstomllib) — environmental; CI covers it.🤖 Generated with Claude Code