fix(installer): seed schema-declared core keys on --yes installs and propagate core --set overrides to module config spreads - #2633
Conversation
…propagate core --set overrides to module config spreads Fresh --yes installs seed core config from a hardcoded five-key literal without reading core's schema, and --yes updates skip core once seeded — so any core key declared after that literal was written is silently dropped on every --yes install, and a subsequent --set core.<key> then mis-files to team scope because the user toml never learned the key. Separately, --set core.<key> --action update patches the central tomls but leaves every module's spread copy of the value stale in _bmad/<module>/config.yaml, so skills keep reading the old value until some later install regenerates the spread. This half is live on main today with existing core keys (e.g. --set core.user_name). - ui.js: after the --yes core seed, backfill any schema-declared core keys missing from the assembled config. Value precedence per key: prior [core] answer, then a prior answer under a module section (key promoted module -> core), then the schema default. The legacy _hoistCoreKeysFromLegacyModuleConfigs cannot cover this: it only runs on the pre-central-toml load path, never for v6 installs. - set-overrides.js: a core --set now also refreshes the spread copies in every installed module's config.yaml, not just core's own. - test: new suite covering --yes seeding of a schema-declared key, prior [core] answer preservation, module -> core promotion keeping the user's prior value, [core]-beats-module precedence, and core --set propagation to spread copies (plus non-module dirs and module-scoped --set staying untouched). Prerequisite for bmad-code-org#2609, which declares the first core key outside the --yes literal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe installer now backfills missing core configuration keys during ChangesInstaller core configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant InstallerUI
participant ExistingConfig
participant CoreSchema
InstallerUI->>ExistingConfig: load prior core and module values
InstallerUI->>CoreSchema: read core-skills/module.yaml
CoreSchema-->>InstallerUI: return schema keys and defaults
InstallerUI->>InstallerUI: backfill missing core values
Possibly related PRs
Suggested reviewers: 🚥 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.
🧹 Nitpick comments (4)
tools/installer/ui.js (2)
879-907: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the
tryto schema loading, and prefer the publicexistingConfigaccessor.The
catchcurrently swallows any failure in the seeding loop too (not just an unreadable schema), so a real defect in the backfill silently degrades to "no core keys seeded" with no signal. Consider wrapping only the read/parse and letting loop errors surface (or at least logging a warning). Separately,configCollector._existingConfigreaches into a private field ofOfficialModules; the publicexistingConfiggetter (exercised intest/test-installation-components.jsaround lines 2986-3003) is the stable contract.As per path instructions for
tools/**: "Build script/tooling. Check error handling and proper exit codes."🤖 Prompt for 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. In `@tools/installer/ui.js` around lines 879 - 907, Narrow the try/catch around yaml loading and parsing only, allowing errors in the core seeding loop to surface rather than being silently swallowed; preserve the fallback behavior for unreadable schema files. In the same block, replace the private configCollector._existingConfig access with the public configCollector.existingConfig getter, keeping the existing core backfill logic unchanged.Source: Path instructions
898-902: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
replaceAllfor{directory_name}to match the interactive path.
String.prototype.replacewith a string pattern substitutes only the first occurrence;buildQuestion()intools/installer/modules/official-modules.js(lines 1910-1939) usesreplaceAll, so a schema default containing the placeholder twice would resolve differently under--yes.♻️ Proposed fix
- if (typeof def === 'string') def = def.replace('{directory_name}', path.basename(directory)); + if (typeof def === 'string') def = def.replaceAll('{directory_name}', path.basename(directory));🤖 Prompt for 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. In `@tools/installer/ui.js` around lines 898 - 902, Update the default-value substitution in the undefined-value branch to use replaceAll for every `{directory_name}` placeholder, matching buildQuestion() behavior in the interactive path; preserve the existing directory basename replacement and non-string handling.test/test-installation-components.js (1)
3664-3701: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the backfill's failure branch and CLI-flag precedence.
The four scenarios cover the happy paths well. Two gaps remain on the code this suite is guarding: (1) the
catchincollectModuleConfigsthat swallows an unreadable/absentcore-skills/module.yaml— a fixture pointinggetSourcePathat an empty tree would pin "install still proceeds with the seeded core config"; (2)--yescombined with--user-name/--output-folder, where the backfill must not overwrite CLI-provided values.🤖 Prompt for 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. In `@test/test-installation-components.js` around lines 3664 - 3701, Extend the test coverage around collectModuleConfigs with two cases: use a fixture whose getSourcePath points to an empty tree so the core-skills/module.yaml read enters the catch path, then assert installation still proceeds with the seeded core configuration; also test --yes together with --user-name and --output-folder, asserting the backfill preserves both CLI-provided values instead of overwriting them.tools/installer/set-overrides.js (1)
299-307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the
nonModuleDirslist instead of re-declaring it.The same exclusion set (minus
core) is hardcoded intools/installer/core/installer.jsaround lines 1051-1076. Two copies will drift the moment a new reserved directory is added, and a drifted copy here means core--setstarts rewriting a non-moduleconfig.yaml. Exporting one constant from a shared module and importing it in both places keeps the "what counts as a module dir" rule single-sourced.🤖 Prompt for 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. In `@tools/installer/set-overrides.js` around lines 299 - 307, The module-directory exclusion list is duplicated and can drift between installers. Extract the shared non-module directory set into an exported constant, then update the core `moduleCode === 'core'` logic and the corresponding logic in `core/installer.js` to import and reuse it, preserving the existing exclusions including `core` where required.
🤖 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.
Nitpick comments:
In `@test/test-installation-components.js`:
- Around line 3664-3701: Extend the test coverage around collectModuleConfigs
with two cases: use a fixture whose getSourcePath points to an empty tree so the
core-skills/module.yaml read enters the catch path, then assert installation
still proceeds with the seeded core configuration; also test --yes together with
--user-name and --output-folder, asserting the backfill preserves both
CLI-provided values instead of overwriting them.
In `@tools/installer/set-overrides.js`:
- Around line 299-307: The module-directory exclusion list is duplicated and can
drift between installers. Extract the shared non-module directory set into an
exported constant, then update the core `moduleCode === 'core'` logic and the
corresponding logic in `core/installer.js` to import and reuse it, preserving
the existing exclusions including `core` where required.
In `@tools/installer/ui.js`:
- Around line 879-907: Narrow the try/catch around yaml loading and parsing
only, allowing errors in the core seeding loop to surface rather than being
silently swallowed; preserve the fallback behavior for unreadable schema files.
In the same block, replace the private configCollector._existingConfig access
with the public configCollector.existingConfig getter, keeping the existing core
backfill logic unchanged.
- Around line 898-902: Update the default-value substitution in the
undefined-value branch to use replaceAll for every `{directory_name}`
placeholder, matching buildQuestion() behavior in the interactive path; preserve
the existing directory basename replacement and non-string handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cf17c30e-45c6-4138-b3ad-46be5a59c2d3
📒 Files selected for processing (3)
test/test-installation-components.jstools/installer/set-overrides.jstools/installer/ui.js
…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>
Greptile SummaryThis PR addresses two related installer regressions around
Confidence Score: 5/5Safe to merge; changes are narrowly scoped, well-guarded, and fully exercised by the new test suite. Both new code paths — the --yes backfill in ui.js and the core --set spread in set-overrides.js — are correctly implemented. The backfill uses _existingConfig (populated by loadExistingConfig) for precedence lookup, the key-in-core guard correctly preserves already-seeded values, and the schema-unreadable path emits a visible warning rather than silently failing. The spread refresh correctly excludes NON_MODULE_DIRS plus core's own directory to avoid double-patching, and the header-preservation loop handles all standard YAML comment/blank-line arrangements. Suite 49 covers every stated regression case plus edge guards. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| tools/installer/non-module-dirs.js | New file exporting NON_MODULE_DIRS constant; cleanly de-duplicates three prior inline Set literals across the codebase. |
| tools/installer/set-overrides.js | Core --set now propagates to every non-excluded module config.yaml; header-preservation loop is correct, double-patch guard via excluded Set is sound. |
| tools/installer/ui.js | Backfill reads core schema after --yes seed; precedence chain (prior core → prior module → default) is correct; {directory_name} substitution mirrors existing buildQuestion() behavior; warn-on-unreadable-schema is correctly scoped to just the try/parse block. |
| tools/installer/core/installer.js | Two inline nonModuleDirs Sets replaced with shared NON_MODULE_DIRS import; no logic changes. |
| tools/installer/modules/official-modules.js | One inline nonModuleDirs Set replaced with shared NON_MODULE_DIRS import in loadExistingConfig's legacy path; no logic changes. |
| test/test-installation-components.js | Suite 49 comprehensively covers both new behaviors with fresh, update, promotion, CLI, unreadable-schema, non-module-dir exclusion, and module-scoped-non-propagation cases. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[collectModuleConfigs called with --yes] --> B{CLI flags provided?}
B -- Yes --> C[loadExistingConfig → _existingConfig]
B -- No --> D[loadExistingConfig → _existingConfig]
C --> E[Seed collectedConfig.core from CLI flags]
D --> F{collectedConfig.core empty?}
F -- Yes, fresh install --> G[Seed hardcoded 5-key core defaults]
F -- No --> H[Keep existing collectedConfig.core]
E --> I
G --> I
H --> I
I[--yes backfill: read core-skills/module.yaml] --> J{Schema readable?}
J -- No --> K[prompts.log.warn, skip backfill]
J -- Yes --> L[For each schema key not in collectedConfig.core]
L --> M{Prior value in _existingConfig.core?}
M -- Yes --> N[Use prior core value]
M -- No --> O{Prior value in any module section?}
O -- Yes --> P[Use promoted module value]
O -- No --> Q[Use schema default, expand directory_name]
N --> R[Set core key]
P --> R
Q --> R
R --> S[collectAllConfigurations: core skipped, modules collected]
K --> S
subgraph applySetOverrides [set-overrides.js: core --set]
T[Write value to config.user.toml] --> U[Build yamlTargets: core/config.yaml first]
U --> V[Enumerate _bmad dirs excluding NON_MODULE_DIRS + core]
V --> W[Add each module/config.yaml to yamlTargets]
W --> X[For each target: parse YAML, patch key, preserve header, write]
end
Reviews (2): Last reviewed commit: "fix(installer): address review-bot findi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- ui.js: {directory_name} resolves with replaceAll, matching
buildQuestion() on the interactive path; the try now wraps only the
schema read/parse (a defect in the seeding loop surfaces instead of
silently degrading) and the catch logs a warning so a broken core
module.yaml is visible; the backfill reads the public existingConfig
getter instead of the private field
- set-overrides.js / core/installer.js / modules/official-modules.js:
the reserved-directory exclusion set is now a single shared
NON_MODULE_DIRS constant (new tools/installer/non-module-dirs.js)
instead of four independent copies that could drift
- test: two more Suite 49 cases — --yes with --user-name/--output-folder
keeps CLI values while still seeding schema-declared keys, and an
unreadable core module.yaml skips the backfill without failing the
install
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bot findings dispositioned in c6b9446 — all four CodeRabbit nitpicks fixed, plus both optional test suggestions taken:
Full |
|
@coderabbitai review |
|
@greptileai review |
✅ Action performedReview finished.
|
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 `@tools/installer/ui.js`:
- Line 908: Update the replacement in the definition handling around def so
replaceAll uses a callback returning path.basename(directory), ensuring
directory names containing replacement-pattern characters remain literal.
🪄 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: 565caa69-62a0-4c8d-a6ff-7acb5e5a2f45
📒 Files selected for processing (6)
test/test-installation-components.jstools/installer/core/installer.jstools/installer/modules/official-modules.jstools/installer/non-module-dirs.jstools/installer/set-overrides.jstools/installer/ui.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/installer/set-overrides.js
| } | ||
| if (value === undefined) { | ||
| let def = item.default; | ||
| if (typeof def === 'string') def = def.replaceAll('{directory_name}', path.basename(directory)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
node - <<'NODE'
const template = 'project-{directory_name}';
const name = '$&';
const unsafe = template.replaceAll('{directory_name}', name);
const safe = template.replaceAll('{directory_name}', () => name);
if (unsafe === safe) process.exit(1);
console.log({ unsafe, safe });
NODERepository: bmad-code-org/BMAD-METHOD
Length of output: 222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file:"
fd -a 'ui\.js$' . | sed 's#^\./##' | grep 'tools/installer/ui.js' || true
echo
echo "Relevant lines around collectModuleConfigs:"
wc -l tools/installer/ui.js
sed -n '860,925p' tools/installer/ui.js | cat -n
echo
echo "Search for project_name / def directory_name usage:"
rg -n "project_name|directory_name|collectModuleConfigs|replaceAll\\('\\{directory_name\\}'" tools/installer/ui.jsRepository: bmad-code-org/BMAD-METHOD
Length of output: 4700
Use a replacer callback for literal directory names.
String.prototype.replaceAll('{directory_name}', path.basename(directory)) treats $&, $', $` , and $$ as replacement instructions, so a directory name like "$&" leaves {directory_name} in project_name. Use def.replaceAll('{directory_name}', () => path.basename(directory)).
🤖 Prompt for 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.
In `@tools/installer/ui.js` at line 908, Update the replacement in the definition
handling around def so replaceAll uses a callback returning
path.basename(directory), ensuring directory names containing
replacement-pattern characters remain literal.
1 participant
What
Two related installer fixes around core config keys, prototyped and requested by @jheyworth in the review of #2609. This PR gates #2609, which will declare the first core key that the
--yesliteral doesn't know about.--yesinstalls now seed schema-declared core keys. Fresh--yesinstalls seed core from a hardcoded five-key literal inui.jswithout reading core's schema, and--yesupdates skip core once seeded — so any newly declared core key was silently dropped on every--yesinstall, and a later--set core.<key>mis-filed to team scope becauseconfig.user.tomlnever learned the key.--set core.<key>now refreshes the spread copies. Core values are spread into every module'sconfig.yamlat generate time and skills read their own module's copy — but--set core.<key> --action updateonly patched the central tomls, leaving every spread copy stale until a later install regenerated it. This half is live on main today with existing core keys (e.g.--set core.user_name).How
tools/installer/ui.js— after the--yescore seed, backfill any core keys declared insrc/core-skills/module.yamlbut absent from the assembled config. Value precedence per key: prior[core]answer → prior answer under a module section (key promoted module → core) → schema default. The legacy_hoistCoreKeysFromLegacyModuleConfigs()cannot cover the promotion case: it only runs on the pre-central-toml load path, never for v6 installs.tools/installer/set-overrides.js— a core--setnow patches the spread copy in every installed module'sconfig.yaml, not just core's own. Non-module dirs (_config,docs, etc.) are excluded; module-scoped--setbehavior is unchanged.Testing
New Test Suite 49 in
test/test-installation-components.jswith the three regression cases requested in the #2609 review, plus edge guards:--yesinstall seeds a schema-declared core key with its schema default--yesupdate preserves a prior[core]answer instead of resetting to the default--yesupdate migrates a prior module-section value for a key promoted to core (and a prior[core]answer wins over a module-section one)--setroutes toconfig.user.tomland immediately refreshes the spread copies incore/,bmm/, and an external module'sconfig.yaml, preserving the generated-file banner header--setdoes not propagate to other modulesnpm testrun locally: refs, install (403/403), urls, channels, skills, eslint, markdownlint, and prettier all pass. Thetest:rendererstep fails on this machine on pristine main as well (local Python is 3.9;render.pyimportstomllib, stdlib only in 3.11+) — environmental, unrelated to this change; CI should confirm.🤖 Generated with Claude Code