fix(setup): never delete live core BMAD config/manifest (#64)#73
Conversation
bmad-loop-setup's cleanup scripts hardcoded `core` in their delete lists (and the SKILL passed `--also-remove _config`), so running /bmad-loop-setup in a multi-module BMAD v6 project deleted _bmad/core/config.yaml, per-module config, and the entire _bmad/_config/ manifest — breaking future `npx bmad-method install` upgrades and violating the "standard BMAD install is never modified" guarantee. Cleanup now removes a directory only when it is a verified-redundant skill payload: it contains a SKILL.md, carries no live config/manifest files, and (with --skills-dir) its skills are verified installed. Live config dirs (core/, per-module, _config/) are protected and reported under a new `directories_protected` field. The merge scripts still read legacy config as fallback defaults but no longer delete it (on v6 those files are live and manifest-tracked). Version-agnostic — driven by directory contents, no version check. Adds tests/test_setup_cleanup_scripts.py exercising the real scripts. Same root cause as upstream bmad-code-org/bmad-builder#96. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe setup cleanup flow now preserves live BMAD v6 config and manifest state. Cleanup only removes verified redundant payloads, merge scripts keep legacy inputs read-only, and the docs, changelog, and tests were updated to match. ChangesCleanup and merge preservation fix
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
🤖 Augment PR SummarySummary: Prevents Changes:
Technical Notes: The cleanup decision is content-based (no version checks) and aims to preserve shared BMAD state while still cleaning genuine pre-v6 staged payloads. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
cleanup-legacy.py discarded cleanup_directories()'s not_found return, so a removable dir that vanished between classify_dirs() and removal (TOCTOU, or a nested --also-remove target removed with its parent earlier in the loop) fell out of every JSON list — directories_removed, directories_protected, and directories_not_found — while safety_checks.skills_verified still printed true, misreporting what happened. Merge cleanup_directories()'s not_found into the classify-time not_found before building the result. The two are disjoint (cleanup only processes `removable`, which excludes classify's not_found), so plain concatenation is a strict superset that cannot change existing behavior. Adds a deterministic regression test that removes a nested --also-remove target whose parent is deleted first, asserting the child surfaces in directories_not_found. Addresses augmentcode[bot] review on PR #73. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
augment review |
| Everything else is left untouched. On a modern BMAD v6 install the per-module and | ||
| core directories hold only live config (no staged SKILL.md), and _bmad/_config/ is | ||
| the live installer manifest — so this script is a safe no-op there and never deletes | ||
| shared BMAD state. 'core' and '_config' are never removed. The removal decision is |
There was a problem hiding this comment.
The module docstring states that 'core' is never removed, but the implementation only hard-codes protection for _config by name (and otherwise relies on marker files). If core/ ever lacks those markers but contains SKILL.md payload, it looks eligible for removal, so the wording may be stronger than the actual guarantee.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Agreed — the docstring promised more than the code enforced. Fixed in 9d4973a by making the code match the docs: core is now protected by name exactly like _config (checked in both classify_dirs and is_config_bearing, reported as "live BMAD dir (protected by name)" in directories_protected). The cost is nil in practice — any real legacy core/ also carries config.yaml and was already marker-protected; the hypothetical marker-less core payload now becomes harmless preserved residue, the same trade-off SKILL.md already documents for the bauto rename case. Added test_cleanup_protects_core_by_name_even_as_pure_payload so the docstring guarantee is tested, not just asserted.
| missing = [] | ||
| if path.name == "_config": | ||
| return True | ||
| for marker in _CONFIG_MARKERS: |
There was a problem hiding this comment.
is_config_bearing() only checks for marker files directly under the candidate directory, while find_skill_dirs() scans recursively; if config/manifest markers can exist deeper in the tree, a directory could be misclassified as removable. Consider whether this guard should also be recursive (or otherwise more conservative) to avoid any chance of deleting live state.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Valid catch — a candidate with a SKILL.md plus nested live config (e.g. sub/config.yaml) was classified removable and rmtree would have taken the nested state with it. Fixed in 9d4973a: is_config_bearing() now scans the whole candidate tree (marker files and nested _config/ dirs).
One deliberate nuance: the scan is payload-aware rather than blindly recursive. Staged skill payloads legitimately ship marker-named files — bmad-loop-setup itself ships assets/module-help.csv — so a blind recursive scan would false-protect genuine payloads and permanently no-op the documented bauto rename cleanup. A marker only protects when it is not inside a SKILL.md-bearing subtree (strictly below the candidate root, so top-level markers always protect, preserving existing behavior). Regression tests cover both directions: test_cleanup_protects_dir_with_nested_live_config (nested live config → protected) and test_cleanup_removes_payload_whose_skill_ships_marker_named_assets (marker-named skill assets → still removed).
Resolve CHANGELOG.md conflict: keep main's Unreleased ### Added block followed by this branch's ### Fixed entry.
…#73 review) Two review fixes to the cleanup-legacy guard: - 'core' is now protected by name like '_config', so the docstring's "'core' and '_config' are never removed" is literally enforced; a marker-less core/ holding a SKILL.md payload is preserved (harmless residue) instead of removed. - is_config_bearing() now scans the whole candidate tree for config/manifest markers instead of only the top level, so nested live state (e.g. sub/config.yaml) protects the dir. The scan is payload-aware: marker-named files inside a SKILL.md-bearing subtree are skill assets (bmad-loop-setup itself ships assets/module-help.csv), not live state — a blind recursive scan would have false-protected genuine payloads and no-opped the documented bauto rename cleanup. Adds three regression tests covering name-protected core, nested live config, and a payload whose skill ships marker-named assets.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md (1)
184-196: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the active CLI’s skills tree here. This hardcodes
.claude/skills, but codex/gemini installs live under.agents/skills/, so the installed-skill check can point at the wrong tree on non-Claude setups.🤖 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` around lines 184 - 196, Update the cleanup confirmation in SKILL.md to use the active CLI’s skills directory instead of hardcoding .claude/skills, since the installed-skill verification in cleanup-legacy.py must target .agents/skills/ for codex/gemini installs. Locate the cleanup examples and the explanatory text around cleanup-legacy.py, and make the --skills-dir guidance resolve from the current CLI environment or explicitly mention the correct tree per CLI so the verification step checks the right installed skills path.
🤖 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.
Outside diff comments:
In `@src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md`:
- Around line 184-196: Update the cleanup confirmation in SKILL.md to use the
active CLI’s skills directory instead of hardcoding .claude/skills, since the
installed-skill verification in cleanup-legacy.py must target .agents/skills/
for codex/gemini installs. Locate the cleanup examples and the explanatory text
around cleanup-legacy.py, and make the --skills-dir guidance resolve from the
current CLI environment or explicitly mention the correct tree per CLI so the
verification step checks the right installed skills path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26355e2e-dba0-4fc0-a4b2-cce615b06a00
📒 Files selected for processing (4)
CHANGELOG.mdsrc/bmad_loop/data/skills/bmad-loop-setup/SKILL.mdsrc/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.pytests/test_setup_cleanup_scripts.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py
|
augment review |
|
Looks like you're out of credits for this review. Head to your account settings to top up and keep reviews running. https://app.augmentcode.com/account/subscription |
…eview) SKILL.md hardcoded .claude/skills as the cleanup verification tree, but codex/gemini/copilot/antigravity installs lay skills at .agents/skills/. On a non-Claude-only setup with a genuine legacy payload the documented command failed verification (fail-safe exit 1, nothing deleted) and wedged setup with a spurious missing-skills error. Guidance now names the per-CLI trees and notes any registered CLI's tree verifies (init installs the bundle into every selected tree). Script docstring and --skills-dir help aligned. Docs/help only — no behavior change.
|
@coderabbitai Valid — fixed in adc4d52. The Scope notes: the failure mode was already fail-safe — a wrong tree makes verification exit 1 without deleting anything — so this was a wedged-setup UX bug, not a data-loss path; the hardcode also predates this PR (this PR's diff only dropped |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /tmp
gh api repos/bmad-code-org/bmad-loop/commits/adc4d52 --jq '.commit.message' 2>&1 | head -20Length of output: 884
Agreed on scope: since the original hardcode was already fail-safe (wrong tree → exit 1, no deletion) and predates this PR, a docs/help-only fix is the right level of effort here rather than expanding (‐^ᴥ^‐) |
1 participant
Summary
Fixes #64. Running
/bmad-loop-setupin a multi-module BMAD v6 project deleted live, manifest-tracked BMAD infrastructure —_bmad/core/config.yaml, per-module config, and the entire_bmad/_config/manifest directory — breaking futurenpx bmad-method installupgrades and violating the README's "your standard BMAD install is never modified" guarantee.Four destructive paths were confirmed in source (the reporter named three; validation found the fourth):
cleanup-legacy.py—dirs_to_remove = [module_code, "core"] + also_removeshutil.rmtreeof_bmad/core/(and_config/via--also-remove)SKILL.mdcleanup command--also-remove _config→ manifest dirmerge-config.py—cleanup_legacy_configs()_bmad/core/config.yamlmerge-help-csv.py—cleanup_legacy_csvs()_bmad/core/module-help.csvThe
--skills-dir"safety check" only guarded dirs containing aSKILL.md; config-only dirs bypassed it and were removed while the JSON still reportedskills_verified: true.Root cause
The scripts are a fork of the
bmad-code-org/bmad-buildersetup-skill-template, which assumes the installer stages redundant skill payloads + config under_bmad/<module>/andcore/. On BMAD v6 that's false: per-moduleconfig.yamlis live, skills live at.claude/skills/, and_bmad/_config/is the live installer manifest. Same root cause as upstream bmad-code-org/bmad-builder#96 (whose maintainer plans to remove the scaffolder — which won't fix bmad-loop's already-shipped fork, so this fixes it here).Fix
Adopt #96's version-agnostic verified-redundant-payload guard. A directory is removed only when:
SKILL.md, andconfig.yaml,module-help.csv, installer manifests, or the_config/dir), and--skills-dir, every skill in it is verified installed.Live config dirs (
core/, per-module,_config/) are protected and reported in a newdirectories_protectedfield. The merge scripts still read legacy config as fallback defaults but no longer delete it. No version check needed — the decision is driven entirely by directory contents, so it safely no-ops on v6 while still cleaning genuine pre-v6 payload dirs.SKILL.mddrops--also-remove _configand its prose is updated accordingly.Behavior note
On a bmad-auto→bmad-loop rename, a stale
_bmad/bauto/config.yaml(if it holds only config) is now preserved rather than deleted — harmless residue, documented inSKILL.mdas a manual-cleanup option. This is the correct trade-off given the "never delete live config" invariant.Tests
New
tests/test_setup_cleanup_scripts.pyruns the real scripts end-to-end and asserts:core//_config//per-module config survive--module-code core --also-remove _configstill leaves them intact--skills-direrrors (exit 1) instead of deletingcore/config.yamlandcore/module-help.csvFull suite: 1425 passed, 1 skipped.
trunk checkclean.🤖 Generated with Claude Code
Summary by CodeRabbit
bmad-loop-setupcleanup to preserve live BMAD core configuration and installer/manifest metadata during upgrades, including when targeting legacy cleanup options.module-help.csv(merged outputs still take precedence).SKILL.md), reportsdirectories_protected, and safely handles missing installed-skill metadata; legacybmad-autocleanup is now conditional.