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)
WalkthroughThe bmad-loop-setup cleanup and merge scripts now preserve live BMAD v6 config and installer-manifest state. Cleanup only removes verified redundant skill payloads, merge scripts keep legacy config and CSV files, and the setup docs, changelog, and regression 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 | ✅ 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 |
🤖 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.
| 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.
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
SKILL.md) while skipping protected live/manifest-related directories.