fix(setup): stop registering BMAD config; the installer owns it (#258, #259)#297
Conversation
…#259) bmad-loop-setup wrote _bmad/config.yaml, _bmad/config.user.yaml and a root _bmad/module-help.csv — the pre-v6.10 layout. BMAD's resolver never reads any of them: resolve_config.py merges four TOML layers, and /bmad-help reads a catalog assembled from per-module _bmad/<module>/module-help.csv. The scripts produced dead files. The fix is subtraction, not a TOML port. Since v6.10.0 bmad-loop is an installer-installed module (bmad-modules.yaml, marketplace-plugin), so the BMAD installer already stages _bmad/bmad-loop/, writes the manifests, and rebuilds the help catalog on every install and quick-update. It also regenerates the central config.toml wholesale, discarding anything written there from outside — and writeCentralConfig skips modules with no prompt answers, which ours has none of. There was never a table for us to write. Upstream is retiring setup-skill config writing rather than porting it (bmad-builder#96, BMAD-METHOD#2591). - merge-config.py, merge-help-csv.py and cleanup-legacy.py are removed. Setup writes exactly one file under _bmad/, the per-module module-help.csv, which is byte-for-byte what the installer places there, survives its reruns, and is idempotent. - SKILL.md keeps only what the installer structurally cannot do: install or upgrade the orchestrator tool, run bmad-loop init, preflight with validate. It reads user_name/communication_language through BMAD's own resolve_config.py instead of collecting and storing them. - assets/module.yaml, assets/module-help.csv and the trailing -setup in the directory name stay untouched: plugin-resolver.js strategy 2 needs all three or official-modules.js throws and the module stops installing. Nothing reads them at runtime, so a new test pins them. - #259 closes with them. No inline-dependency script is invoked with bare python3 any more, because none ships; the one remaining script call is the resolver, via uv run per upstream's own docs. Also drops the bmad-auto rename compatibility, dead since 0.8.0 and confirmed to have no installs left in the wild. init no longer strips bmad_auto-marked hooks, deletes bmad-auto-* skill dirs, carries .automator/policy.toml forward, or prints the leftover-state note. That removal also retires cleanup-legacy.py's last legitimate caller: its other invocation targeted _bmad/bmad-loop/, which is now the installer's staged skill payload and its copy source for .claude/skills. test_setup_cleanup_scripts.py guarded "setup must never delete live BMAD config" (#64/#73); with no deleting code shipping, the regression is impossible. It is replaced by test_setup_skill_contract.py, covering the installer's resolver contract and prose drift back toward the legacy paths. The strip-hook tests' one unique invariant — a foreign Stop hook coexisting with our relay — was already covered by test_merge_hooks_preserves_existing. Verified end-to-end against a real v6.10.1 _bmad/: 44 pre-existing files byte-identical, exactly one file created, idempotent over two runs, and resolve_config.py's output byte-identical before and after. Double-init regression: registers once, reports already-registered on re-run, foreign hook and permissions intact. Suite 2960 passed / 1 skipped; trunk check --no-fix clean.
WalkthroughThe setup flow now installs and initializes the orchestrator, delegates BMAD registration to the BMAD installer, preserves existing policy files, removes legacy migration behavior, and adds packaged-skill contract tests with updated documentation. ChangesSetup ownership and migration removal
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_install.py (1)
224-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the removed legacy-policy migration explicitly.
This only proves an existing new policy is preserved. Add a legacy
.automator/policy.tomlfixture and assertinstall_into()writes the template rather than copying its contents, while leaving the legacy file untouched. Based on the PR objective: legacy.automator/policy.tomlmust not be migrated.🤖 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 `@tests/test_install.py` around lines 224 - 232, Add explicit legacy-policy coverage to test_install_does_not_clobber_existing_policy by creating a .automator/policy.toml fixture with distinct contents, then assert install_into() creates the new .bmad-loop policy from the template rather than copying the legacy contents, while the legacy file remains unchanged.
🤖 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 `@CHANGELOG.md`:
- Around line 83-90: Update the final migration guidance in the “Removed”
changelog entry to state that projects still using bmad-auto must migrate on a
0.8.x release before installing 0.9.0. Do not leave the instruction implying
migration can occur after upgrading to 0.9.0 unless a manual migration path is
documented.
In `@docs/setup-guide.md`:
- Around line 373-382: Rename the step-6 heading to cover all projects with a
_bmad/ tree, rather than limiting it to BMAD-installer projects. Keep the
existing installer-managed and /bmad-loop-setup-only cleanup instructions
unchanged.
In `@src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md`:
- Line 14: Pin every BMAD installer invocation in the bmad-loop setup
instructions to the approved explicit bmad-method version, replacing unversioned
commands while preserving the existing install flow. Apply this at
src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md lines 14, 32, and 148; all
three sites require the same command update.
In `@src/bmad_loop/data/skills/README.md`:
- Around line 37-44: Update the CLI examples and installation description in the
README to document all supported `.agents/skills/` targets: codex, gemini,
copilot, and antigravity. Keep claude associated with `.claude/skills/`, and
ensure the setup guidance consistently reflects these adapter mappings.
---
Nitpick comments:
In `@tests/test_install.py`:
- Around line 224-232: Add explicit legacy-policy coverage to
test_install_does_not_clobber_existing_policy by creating a
.automator/policy.toml fixture with distinct contents, then assert
install_into() creates the new .bmad-loop policy from the template rather than
copying the legacy contents, while the legacy file remains unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bdbd112b-6dc6-4134-8a5f-4cde40d250e1
📒 Files selected for processing (13)
.claude-plugin/marketplace.jsonCHANGELOG.mdREADME.mddocs/setup-guide.mdsrc/bmad_loop/data/skills/README.mdsrc/bmad_loop/data/skills/bmad-loop-setup/SKILL.mdsrc/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.pysrc/bmad_loop/data/skills/bmad-loop-setup/scripts/merge-config.pysrc/bmad_loop/data/skills/bmad-loop-setup/scripts/merge-help-csv.pysrc/bmad_loop/install.pytests/test_install.pytests/test_setup_cleanup_scripts.pytests/test_setup_skill_contract.py
💤 Files with no reviewable changes (4)
- src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py
- src/bmad_loop/data/skills/bmad-loop-setup/scripts/merge-help-csv.py
- tests/test_setup_cleanup_scripts.py
- src/bmad_loop/data/skills/bmad-loop-setup/scripts/merge-config.py
| ### Removed | ||
|
|
||
| - **The `bmad-auto` → `bmad-loop` rename compatibility is gone.** The rename shipped in 0.8.0 and | ||
| no pre-rename installs remain in the wild, so `init` no longer strips `bmad_auto`-marked hooks, | ||
| deletes `bmad-auto-*` skill dirs, carries `.automator/policy.toml` over to `.bmad-loop/`, or | ||
| prints the leftover-`.automator/` note. `bmad-loop-setup` drops its migration section with them. | ||
| A project still on `bmad-auto` should migrate on 0.9.0 before upgrading past it. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify the migration version boundary.
0.9.0 removes the compatibility behavior, but the final sentence tells remaining bmad-auto users to migrate “on 0.9.0.” Say they must migrate on 0.8.x before installing 0.9.0, or document a manual migration path; otherwise those users may upgrade after the cleanup logic is already gone. This follows the release note’s statement that init no longer performs the migration.
🤖 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 `@CHANGELOG.md` around lines 83 - 90, Update the final migration guidance in
the “Removed” changelog entry to state that projects still using bmad-auto must
migrate on a 0.8.x release before installing 0.9.0. Do not leave the instruction
implying migration can occur after upgrading to 0.9.0 unless a manual migration
path is documented.
| ### 6. Remove the BMAD module (BMAD-installer projects only) | ||
|
|
||
| If you ran `/bmad-loop-setup`, it registered the module in your BMAD config. Remove the | ||
| bmad-loop (`bmad-loop`) entries from: | ||
| There is nothing to hand-unregister: bmad-loop writes no BMAD config. If you installed the | ||
| module through the BMAD installer, remove it there — the installer owns `_bmad/bmad-loop/`, | ||
| the central `config.toml`, and the `/bmad-help` catalog, and regenerates all three on its | ||
| next run. | ||
|
|
||
| - `_bmad/config.yaml` and `_bmad/config.user.yaml` — drop the bmad-loop module config block | ||
| - `_bmad/module-help.csv` — drop the bmad-loop help rows | ||
|
|
||
| uv + `init`-only projects never write to `_bmad/` and can skip this step. | ||
| If you only ever ran `/bmad-loop-setup`, delete `_bmad/bmad-loop/module-help.csv` (the one | ||
| file it refreshes) — or the whole `_bmad/bmad-loop/` directory if the installer never | ||
| created it. uv + `init`-only projects that have no `_bmad/` can skip this step. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the step-6 heading with its documented scope.
The preceding rule says every project with a _bmad/ tree needs step 6, and the body explicitly covers /bmad-loop-setup-only projects. BMAD-installer projects only contradicts both and may cause setup-only users to skip cleanup. Rename the heading, for example:
Proposed fix
-### 6. Remove the BMAD module (BMAD-installer projects only)
+### 6. Remove BMAD filesThis follows the scope stated earlier in the guide and the setup-only branch in this section.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### 6. Remove the BMAD module (BMAD-installer projects only) | |
| If you ran `/bmad-loop-setup`, it registered the module in your BMAD config. Remove the | |
| bmad-loop (`bmad-loop`) entries from: | |
| There is nothing to hand-unregister: bmad-loop writes no BMAD config. If you installed the | |
| module through the BMAD installer, remove it there — the installer owns `_bmad/bmad-loop/`, | |
| the central `config.toml`, and the `/bmad-help` catalog, and regenerates all three on its | |
| next run. | |
| - `_bmad/config.yaml` and `_bmad/config.user.yaml` — drop the bmad-loop module config block | |
| - `_bmad/module-help.csv` — drop the bmad-loop help rows | |
| uv + `init`-only projects never write to `_bmad/` and can skip this step. | |
| If you only ever ran `/bmad-loop-setup`, delete `_bmad/bmad-loop/module-help.csv` (the one | |
| file it refreshes) — or the whole `_bmad/bmad-loop/` directory if the installer never | |
| created it. uv + `init`-only projects that have no `_bmad/` can skip this step. | |
| ### 6. Remove BMAD files | |
| There is nothing to hand-unregister: bmad-loop writes no BMAD config. If you installed the | |
| module through the BMAD installer, remove it there — the installer owns `_bmad/bmad-loop/`, | |
| the central `config.toml`, and the `/bmad-help` catalog, and regenerates all three on its | |
| next run. | |
| If you only ever ran `/bmad-loop-setup`, delete `_bmad/bmad-loop/module-help.csv` (the one | |
| file it refreshes) — or the whole `_bmad/bmad-loop/` directory if the installer never | |
| created it. uv + `init`-only projects that have no `_bmad/` can skip this step. |
🤖 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 `@docs/setup-guide.md` around lines 373 - 382, Rename the step-6 heading to
cover all projects with a _bmad/ tree, rather than limiting it to BMAD-installer
projects. Keep the existing installer-managed and /bmad-loop-setup-only cleanup
instructions unchanged.
| This module is unusual: alongside its automation skills it relies on the **bmad-loop orchestrator tool** — the Python program that actually drives the loop — installed as the `bmad-loop` package from its public Git repository. The skills do nothing on their own. | ||
|
|
||
| Module identity (name, code, version) comes from `./assets/module.yaml`. Collects user preferences and writes them to three files: | ||
| **The BMAD installer owns `_bmad/` registration; this skill does not.** When bmad-loop is installed through the BMAD installer (`npx bmad-method install`), the installer stages the skills under `_bmad/bmad-loop/`, writes that module's own config and help files, records it in `_bmad/_config/` (manifest, skill manifest, files manifest), and rebuilds the `/bmad-help` catalog. It regenerates the central `_bmad/config.toml` on every run, so nothing outside the installer should write there. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | grep -F 'src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md' || true
echo
echo "Target snippet with line numbers:"
cat -n src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md | sed -n '1,170p'
echo
echo "All bmad-method references:"
rg -n "bmad-method|_bmad|bmad-help|SKILL.md" src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md src || true
echo
echo "Similar installer references elsewhere:"
rg -n "bmad-method install|npx .* bmad-method" . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: bmad-code-org/bmad-loop
Length of output: 29261
🌐 Web query:
npm package bmad-method npx bmad-method install documentation
💡 Result:
The bmad-method npm package is an AI-driven agile development framework designed to guide users through structured, multi-step development processes using specialized agents and workflows [1][2]. Installation The primary method for installing and initializing the framework is using the following command in your terminal within your project directory: npx bmad-method install [1][3] For those who wish to use the newest prerelease build, use: npx bmad-method@next install [1][3] Installation Requirements: - Node.js: v20.12 or higher [1][4] - Python: 3.10 or higher [1][4] - Package Manager: uv [1][4] Documentation and Usage Once installed, the framework creates an _bmad/ folder for configuration, agents, and workflows, and an _bmad-output/ folder for project artifacts [5]. - Getting Started: After running the installation command and following the prompts, you can start by using the guide tool: bmad-help [5]. - Non-Interactive Installation: For CI/CD environments, you can bypass prompts using the --yes flag: npx bmad-method install --directory /path/to/project --modules bmm --tools claude-code --yes [1][3] Official documentation and further tutorials are available at https://docs.bmad-method.org [3]. The framework supports various official modules, such as BMad Builder (BMB) for custom agent creation, Test Architect (TEA), and the Creative Intelligence Suite (CIS) [1][3].
Citations:
- 1: https://www.npmjs.com/package/bmad-method
- 2: https://github.com/bmadcode/BMAD-METHOD/
- 3: https://docs.bmad-method.org/how-to/install-bmad/
- 4: https://github.com/bmad-code-org/BMAD-METHOD?tab=readme-ov-file
- 5: https://github.com/bmad-code-org/BMAD-METHOD/blob/main/docs/tutorials/getting-started.md
Pin the BMAD installer command.
These instructions execute the latest bmad-method release served by npm at setup time. Use the approved versioned invocation, e.g. npx bmad-method@<X.Y.Z> install, so module registration is not controlled by an unpinned supply-chain dependency.
src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md#L14-L14src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md#L32-L32src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md#L148-L148
🧰 Tools
🪛 SkillSpector (2.3.11)
[warning] 14: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 32: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 148: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
📍 Affects 1 file
src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md#L14-L14(this comment)src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md#L32-L32src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md#L148-L148
🤖 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 `@src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md` at line 14, Pin every
BMAD installer invocation in the bmad-loop setup instructions to the approved
explicit bmad-method version, replacing unversioned commands while preserving
the existing install flow. Apply this at
src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md lines 14, 32, and 148; all
three sites require the same command update.
Source: Linters/SAST tools
| bmad-loop init --project /path/to/project --cli claude # add --cli codex/gemini as needed | ||
| claude "/bmad-loop-setup accept all defaults" # registers _bmad/ config + help | ||
| claude "/bmad-loop-setup accept all defaults" # installs the tool + wires the project | ||
| ``` | ||
|
|
||
| `bmad-loop init` installs the `bmad-loop-*` skills into `.claude/skills/` | ||
| (claude) and/or `.agents/skills/` (codex/gemini), registers hooks, writes | ||
| `.bmad-loop/policy.toml`, and gitignores the runs dir. Existing skill dirs are | ||
| left untouched (`--force-skills` to overwrite, `--no-skills` to skip). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document all supported CLI targets.
The examples and .agents/skills/ description mention only codex and gemini, but copilot and antigravity are also supported and use that tree. Include them to avoid incomplete setup guidance. Based on src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md: all four adapters use .agents/skills/.
🤖 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 `@src/bmad_loop/data/skills/README.md` around lines 37 - 44, Update the CLI
examples and installation description in the README to document all supported
`.agents/skills/` targets: codex, gemini, copilot, and antigravity. Keep claude
associated with `.claude/skills/`, and ensure the setup guidance consistently
reflects these adapter mappings.
1 participant
Closes #258. Closes #259. Part of #287.
The problem
bmad-loop-setupwrote_bmad/config.yaml,_bmad/config.user.yamland a root_bmad/module-help.csv— the pre-v6.10 layout. BMAD's resolver never reads any of them:resolve_config.pymerges four TOML layers, and/bmad-helpreads a catalog assembled fromper-module
_bmad/<module>/module-help.csv. The scripts produced dead files.Why this is subtraction, not a TOML port
An earlier attempt (#285, closed unmerged) tried to port the writer to TOML. Reading upstream
shows why that direction was wrong:
bmad-modules.yamlwithmarketplace-plugin: true, shipped as the v6.10.0 headline feature._bmad/bmad-loop/with all three skill dirs, copiesassets/module-help.csvto that moduleroot, writes the per-module
config.yaml, adds rows tomanifest.yaml/skill-manifest.csv/files-manifest.csv, and rebuilds_config/bmad-help.csvby scanning_bmad/*/.[modules.bmad-loop]table for us to write.writeCentralConfigskipsmodules with no prompt answers, and our
module.yamldeclares none.config.tomlis alsoregenerated wholesale each install — anything written there from outside is discarded.
("the setup skill creation will be removed soon") and BMAD-METHOD#2591.
What changed
The skill.
merge-config.py,merge-help-csv.pyandcleanup-legacy.pyare removed.Setup writes exactly one file under
_bmad/— the per-modulemodule-help.csv, byte-for-bytewhat the installer places there, surviving its reruns and idempotent.
SKILL.mdkeeps only whatthe installer structurally cannot do: install or upgrade the orchestrator tool, run
bmad-loop init, preflight withvalidate. It readsuser_name/communication_languagethrough BMAD's own
resolve_config.pyinstead of collecting and storing them.assets/is deliberately untouched.plugin-resolver.jsstrategy 2 requiresassets/module.yaml,assets/module-help.csvand a directory name ending in-setup, orofficial-modules.jsthrows and the module stops installing. Nothing reads those files atruntime, so only a test keeps them honest — that test is new here.
#259 closes with them. No inline-dependency script is invoked with bare
python3any more,because none ships. The one remaining script call is the resolver, via
uv run.The bmad-auto rename compatibility is gone — dead since 0.8.0, with no installs left in the
wild.
initno longer stripsbmad_auto-marked hooks, deletesbmad-auto-*skill dirs, carries.automator/policy.tomlforward, or prints the leftover-state note. That also retirescleanup-legacy.py's last legitimate caller: its other invocation targeted_bmad/bmad-loop/,which is now the installer's staged skill payload and its copy source for
.claude/skills/.Tests
test_setup_cleanup_scripts.pyguarded "setup must never delete live BMAD config" (#64/#73);with no deleting code shipping, that regression is impossible. It is replaced by
test_setup_skill_contract.py, which pins the installer's resolver contract and guards againstprose drifting back toward the legacy paths. The strip-hook tests' one unique invariant — a
foreign
Stophook coexisting with our relay — was already covered bytest_merge_hooks_preserves_existing, verified before deleting.Verification
End-to-end against a real v6.10.1
_bmad/tree, running the documented steps twice:config.yaml,config.user.yamland the rootmodule-help.csvunchanged and uncreatedassets/module-help.csvbyte-for-byteresolve_config.pyused as the oracle: output byte-identical before and after, rc 0Double-
inithook regression (the riskiest edit, since removing the strip meant collapsing thewrite conditionals): registers on the first run, reports already-registered on the second, and a
pre-existing foreign
Stophook pluspermissionssurvive both.Not in scope
#154 (the runtime still resolves artifact paths from
_bmad/bmm/config.yaml) stays open under#287.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation