From 5bfca837a1f6643d65d71898f579af12bd6ae361 Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 6 Jul 2026 09:02:57 -0700 Subject: [PATCH 1/4] fix(setup): never delete live core BMAD config/manifest (#64) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CHANGELOG.md | 13 ++ .../data/skills/bmad-loop-setup/SKILL.md | 22 +- .../bmad-loop-setup/scripts/cleanup-legacy.py | 160 +++++++------ .../bmad-loop-setup/scripts/merge-config.py | 37 +-- .../bmad-loop-setup/scripts/merge-help-csv.py | 30 +-- tests/test_setup_cleanup_scripts.py | 216 ++++++++++++++++++ 6 files changed, 372 insertions(+), 106 deletions(-) create mode 100644 tests/test_setup_cleanup_scripts.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ff2b2c..049d473 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,19 @@ All notable changes to `bmad-loop` are documented here. The format is based on [Semantic Versioning](https://semver.org/spec/v2.0.0.html). While the project is pre-1.0, breaking changes may land in a minor release. +## [Unreleased] + +### Fixed + +- **`bmad-loop-setup` no longer deletes live core BMAD config or the installer manifest.** In a + multi-module BMAD v6 project the setup scripts hardcoded `core` (and `--also-remove _config`) into + their delete lists, destroying `_bmad/core/config.yaml`, per-module config, and the whole + `_bmad/_config/` manifest — breaking future `npx bmad-method install` upgrades. Cleanup now removes + a directory only when it is a verified-redundant skill payload (has a `SKILL.md`, carries no + config/manifest, and its skills are installed); live config dirs are protected and reported under + `directories_protected`. The merge scripts read legacy config as fallback but never delete it. Same + root cause as upstream `bmad-code-org/bmad-builder#96`. (closes #64) + ## [0.8.1] — 2026-07-05 ### Fixed diff --git a/src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md b/src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md index 2739c37..4e0f053 100644 --- a/src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md +++ b/src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md @@ -28,7 +28,7 @@ Both config scripts use an anti-zombie pattern — existing entries for this mod 3. Check for per-module configuration at `{project-root}/_bmad/bmad-loop/config.yaml` and `{project-root}/_bmad/core/config.yaml`. If either file exists: - If `{project-root}/_bmad/config.yaml` does **not** yet have a section for this module: this is a **fresh install**. Inform the user that installer config was detected and values will be consolidated into the new format. - If `{project-root}/_bmad/config.yaml` **already** has a section for this module: this is a **legacy migration**. Inform the user that legacy per-module config was found alongside existing config, and legacy values will be used as fallback defaults. - - In both cases, per-module config files and directories will be cleaned up after setup. + - In both cases, legacy per-module config is consolidated into the new format. Live config files are **preserved** (never deleted); only genuinely redundant skill-payload directories, if any, are cleaned up after setup. **Decide fresh-install vs upgrade.** This drives whether the tool is upgraded and whether the per-project skills are refreshed (see "Install the Orchestrator Tool" below). Treat it as an **upgrade** when **any** of these hold: @@ -51,7 +51,7 @@ Only when you detected a **pre-rename install** ("Decide fresh-install vs upgrad 4. **Post-merge cleanup** — the anti-zombie merges key on the **new** names, so old rows and sections survive and need explicit removal: - Delete the leftover `bauto:` section from `{project-root}/_bmad/config.yaml`, plus any `bauto`-marked keys in `{project-root}/_bmad/config.user.yaml`. - Delete rows from `{project-root}/_bmad/module-help.csv` whose module column (column 1) reads `BMAD Automator Skills` — `merge-help-csv.py`'s anti-zombie filter keys on the new `BMAD Loop Skills`, so it leaves the old-named rows behind. - - The legacy installer dir `_bmad/bauto/` is removed by the cleanup step's `--also-remove bauto` (see "Cleanup Legacy Directories"). + - The legacy installer dir `_bmad/bauto/` is cleaned by the bauto cleanup step (see "Cleanup Legacy Directories") **only if** it holds redundant `bmad-auto-*` skill copies; if it holds only stale config it is preserved (harmless — remove it manually once satisfied). Then continue with the normal flow below. @@ -76,7 +76,7 @@ python3 ./scripts/merge-config.py --config-path "{project-root}/_bmad/config.yam python3 ./scripts/merge-help-csv.py --target "{project-root}/_bmad/module-help.csv" --source ./assets/module-help.csv --legacy-dir "{project-root}/_bmad" --module-code bmad-loop ``` -Both scripts output JSON to stdout with results. If either exits non-zero, surface the error and stop. The scripts automatically read legacy config values as fallback defaults, then delete the legacy files after a successful merge. Check `legacy_configs_deleted` and `legacy_csvs_deleted` in the output to confirm cleanup. +Both scripts output JSON to stdout with results. If either exits non-zero, surface the error and stop. The scripts automatically read legacy config values as fallback defaults. Legacy config files are **preserved, never deleted** — on BMAD v6 the per-module and core `config.yaml` / `module-help.csv` are live, manifest-tracked files (so `legacy_configs_deleted` / `legacy_csvs_deleted` are always empty). Run `./scripts/merge-config.py --help` or `./scripts/merge-help-csv.py --help` for full usage. @@ -177,25 +177,23 @@ Unless the user explicitly asked to skip it (e.g. `skills only` / `--no-tool`), ## Cleanup Legacy Directories -After both merge scripts complete successfully, remove the installer's package directories. Skills and agents in these directories are already installed at `.claude/skills/` — the `_bmad/` directory should only contain config files. +After both merge scripts complete successfully, remove any **redundant skill-payload** directories an older installer may have staged under `_bmad/` (past layouts duplicated skills that already live at `.claude/skills/`). On a modern BMAD v6 install nothing redundant is staged, so this step is a **safe no-op** — it never touches `_bmad/core/`, per-module config, or the `_bmad/_config/` manifest. -As with the merge scripts, replace `{project-root}` in the `--bmad-dir` and `--skills-dir` path arguments with the actual project root before running. +As with the merge scripts, replace `{project-root}` in the `--bmad-dir` and `--skills-dir` path arguments with the actual project root before running. Do **not** pass `--also-remove _config` (or any shared config/manifest dir): `_config/` is live BMAD infrastructure and the script refuses to remove it regardless. ```bash -python3 ./scripts/cleanup-legacy.py --bmad-dir "{project-root}/_bmad" --module-code bmad-loop --also-remove _config --skills-dir "{project-root}/.claude/skills" +python3 ./scripts/cleanup-legacy.py --bmad-dir "{project-root}/_bmad" --module-code bmad-loop --skills-dir "{project-root}/.claude/skills" ``` -The script verifies that every skill in the legacy directories exists at `.claude/skills/` before removing anything. Directories without skills (like `_config/`) are removed directly. If the script exits non-zero, surface the error and stop. Missing directories (already cleaned by a prior run) are not errors — the script is idempotent. +A directory is removed 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 at `.claude/skills/`. Live config directories (`core/`, per-module, `_config/`) are protected and reported under `directories_protected`. If the script exits non-zero, surface the error and stop. Missing directories (already cleaned by a prior run) are not errors — the script is idempotent. -**On a rename-upgrade from bmad-auto**, also remove the old `_bmad/bauto/` installer directory. When it was populated by the BMAD-method installer it holds the pre-rename `bmad-auto-*` skill copies, which no longer exist at `.claude/skills/` — so run this cleanup **without** `--skills-dir` to skip the installed-skill verification (their removal is exactly the point). It is a no-op when `_bmad/bauto/` isn't present: +**On a rename-upgrade from bmad-auto**, if the old `_bmad/bauto/` directory holds pre-rename `bmad-auto-*` skill copies (an older payload layout), run this cleanup **without** `--skills-dir` to skip installed-skill verification (their removal is the point). It is a no-op when `_bmad/bauto/` holds only config or isn't present: ```bash python3 ./scripts/cleanup-legacy.py --bmad-dir "{project-root}/_bmad" --module-code bauto ``` -(`--module-code bauto` also targets `core`, already handled above — a missing directory is a no-op.) - -Check `directories_removed` and `files_removed_count` in the JSON output for the confirmation step. Run `./scripts/cleanup-legacy.py --help` for full usage. +Check `directories_removed`, `directories_protected`, and `files_removed_count` in the JSON output for the confirmation step. Run `./scripts/cleanup-legacy.py --help` for full usage. ## Confirm @@ -206,7 +204,7 @@ Report the **tool** result according to the branch taken: - **Fresh install:** the installed `bmad-loop --version`, that `bmad-loop init` registered hooks, installed the `bmad-loop-*` skills, and wrote policy/gitignore for the selected coding CLI(s) (name each one — e.g. "hooks + skills installed for claude, codex"). - **Upgrade:** the before → after `bmad-loop --version` (e.g. "upgraded 0.3.1 → 0.3.2", or "already current at 0.3.2"), that the `bmad-loop-*` skills were **refreshed** (not skipped) with `--force-skills` in each CLI tree, and the re-stamped config version. -Also report the `bmad-loop validate` preflight result (pass, or the readiness checklist of what's still missing). If legacy files were deleted, mention the migration. If legacy directories were removed, report the count and list (e.g. "Cleaned up 106 installer package files from bmb/, core/, \_config/ — skills are installed at .claude/skills/"). Then display the `module_greeting` from `./assets/module.yaml` to the user. +Also report the `bmad-loop validate` preflight result (pass, or the readiness checklist of what's still missing). If legacy config was consolidated, mention the migration (live config files are preserved, not deleted). If any redundant skill-payload directories were removed, report the count and list from `directories_removed` (e.g. "Cleaned up a redundant skill-payload directory — skills are installed at .claude/skills/"); on a v6 install this is typically empty and `directories_protected` confirms `core/` and `_config/` were left intact. Then display the `module_greeting` from `./assets/module.yaml` to the user. ## Outcome diff --git a/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py b/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py index 0ecdb6c..4a4f0f8 100755 --- a/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py +++ b/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py @@ -3,17 +3,23 @@ # requires-python = ">=3.9" # dependencies = [] # /// -"""Remove legacy module directories from _bmad/ after config migration. +"""Remove redundant legacy skill-payload directories from _bmad/ after config migration. -After merge-config.py and merge-help-csv.py have migrated config data and -deleted individual legacy files, this script removes the now-redundant -directory trees. These directories contain skill files that are already -installed at .claude/skills/ (or equivalent) — only the config files at -_bmad/ root need to persist. +Older BMAD installers could stage a module's skill payload *and* its config into +_bmad// (and _bmad/core/), duplicating skills that also live at +.claude/skills/. This script removes only those *redundant skill-payload* trees. -When --skills-dir is provided, the script verifies that every skill found -in the legacy directories exists at the installed location before removing -anything. Directories without skills (like _config/) are removed directly. +A directory is removed ONLY when it is a verified-redundant skill payload: + * it contains >=1 SKILL.md, AND + * it carries no live config/manifest files (config.yaml, module-help.csv, + installer manifests, or the _config/ dir itself), AND + * when --skills-dir is given, every skill in it is verified installed there. + +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 +driven entirely by directory contents, so no version check is needed. Exit codes: 0=success (including nothing to remove), 1=validation error, 2=runtime error """ @@ -76,65 +82,93 @@ def find_skill_dirs(base_path: str) -> list: return sorted(set(skills)) -def verify_skills_installed( - bmad_dir: str, dirs_to_check: list, skills_dir: str, verbose: bool = False -) -> list: - """Verify that skills in legacy directories exist at the installed location. +# Markers that mean a directory holds LIVE BMAD config or installer-manifest +# state — never a disposable skill payload. Their presence protects the directory. +_CONFIG_MARKERS = ("config.yaml", "config.user.yaml", "module-help.csv", "manifest.yaml") - Scans each directory in dirs_to_check for skill folders (containing SKILL.md), - then checks that a matching directory exists under skills_dir. Directories - that contain no skills (like _config/) are silently skipped. - Returns: - List of verified skill names. +def is_config_bearing(path: Path) -> bool: + """True if the directory holds live BMAD config or installer-manifest state. - Raises SystemExit(1) if any skills are missing from skills_dir. + Such a directory (e.g. _bmad/core/, _bmad//, _bmad/_config/) is never a + redundant skill payload and must not be removed. Covers the _config/ manifest dir + by name, per-module/core config.yaml + module-help.csv, and installer manifests + (*-manifest.csv, manifest.yaml, bmad-help.csv). """ - all_verified = [] - missing = [] + if path.name == "_config": + return True + for marker in _CONFIG_MARKERS: + if (path / marker).exists(): + return True + for csv in path.glob("*.csv"): + if csv.name.endswith("-manifest.csv") or csv.name == "bmad-help.csv": + return True + return False + + +def classify_dirs( + bmad_dir: str, dirs_to_check: list, skills_dir: str, verbose: bool = False +) -> tuple: + """Partition requested directories into removable payloads vs protected. + + A directory is removable ONLY if it is a verified-redundant skill payload: it + contains >=1 SKILL.md, carries no live config/manifest files, and — when + skills_dir is given — every skill in it is verified installed there. + + Returns (removable, protected, not_found, verified_skills) where `protected` is a + list of {"dir": name, "reason": ...}. Raises SystemExit(1) if a payload dir's + skills are missing from skills_dir (the original safety contract). + """ + removable: list = [] + protected: list = [] + not_found: list = [] + verified: list = [] + missing: list = [] for dirname in dirs_to_check: - legacy_path = Path(bmad_dir) / dirname - if not legacy_path.exists(): + target = Path(bmad_dir) / dirname + if not target.exists() or not target.is_dir(): + not_found.append(dirname) + if verbose: + print(f"Not found (skipping): {target}", file=sys.stderr) continue - skill_names = find_skill_dirs(str(legacy_path)) + if is_config_bearing(target): + protected.append({"dir": dirname, "reason": "holds live config/manifest"}) + if verbose: + print(f"Protected (live config/manifest), not removing: {target}", file=sys.stderr) + continue + + skill_names = find_skill_dirs(str(target)) if not skill_names: + protected.append({"dir": dirname, "reason": "no skill payload"}) if verbose: - print( - f"No skills found in {dirname}/ — skipping verification", - file=sys.stderr, - ) + print(f"No skill payload, not removing: {target}", file=sys.stderr) continue - for skill_name in skill_names: - installed_path = Path(skills_dir) / skill_name - if installed_path.is_dir(): - all_verified.append(skill_name) - if verbose: - print( - f"Verified: {skill_name} exists at {installed_path}", - file=sys.stderr, - ) - else: - missing.append(skill_name) + if skills_dir: + dir_missing = [s for s in skill_names if not (Path(skills_dir) / s).is_dir()] + if dir_missing: + missing.extend(dir_missing) if verbose: - print( - f"MISSING: {skill_name} not found at {installed_path}", - file=sys.stderr, - ) + for s in dir_missing: + print(f"MISSING: {s} not found under {skills_dir}", file=sys.stderr) + continue + verified.extend(skill_names) + + removable.append(dirname) if missing: error_result = { "status": "error", "error": "Skills not found at installed location", - "missing_skills": missing, - "skills_dir": str(Path(skills_dir).resolve()), + "missing_skills": sorted(set(missing)), + "skills_dir": str(Path(skills_dir).resolve()) if skills_dir else None, } print(json.dumps(error_result, indent=2)) sys.exit(1) - return sorted(set(all_verified)) + return removable, protected, not_found, sorted(set(verified)) def count_files(path: Path) -> int: @@ -227,40 +261,38 @@ def main(): bmad_dir = args.bmad_dir module_code = args.module_code - # Build the list of directories to remove - dirs_to_remove = [module_code, "core"] + args.also_remove + # Candidate directories. 'core' is NEVER hardcoded here — it holds live core + # config on BMAD v6, not a disposable payload. Only the module's own dir plus + # any explicit --also-remove targets are considered, and each is still gated by + # classify_dirs (verified-redundant skill payload, never config-bearing). + dirs_to_check = [module_code] + args.also_remove # Deduplicate while preserving order seen = set() unique_dirs = [] - for d in dirs_to_remove: + for d in dirs_to_check: if d not in seen: seen.add(d) unique_dirs.append(d) - dirs_to_remove = unique_dirs + dirs_to_check = unique_dirs if args.verbose: - print(f"Directories to remove: {dirs_to_remove}", file=sys.stderr) + print(f"Candidate directories: {dirs_to_check}", file=sys.stderr) - # Safety check: verify skills are installed before removing - verified_skills = None - if args.skills_dir: - if args.verbose: - print( - f"Verifying skills installed at {args.skills_dir}", - file=sys.stderr, - ) - verified_skills = verify_skills_installed( - bmad_dir, dirs_to_remove, args.skills_dir, args.verbose - ) + # Classify: only verified-redundant skill payloads are removable; live + # config/manifest dirs (core/, / config, _config/) are protected. + removable, protected, not_found, verified_skills = classify_dirs( + bmad_dir, dirs_to_check, args.skills_dir, args.verbose + ) - # Remove directories - removed, not_found, total_files = cleanup_directories(bmad_dir, dirs_to_remove, args.verbose) + # Remove only the verified-redundant payload directories + removed, _, total_files = cleanup_directories(bmad_dir, removable, args.verbose) # Build result result = { "status": "success", "bmad_dir": str(Path(bmad_dir).resolve()), "directories_removed": removed, + "directories_protected": protected, "directories_not_found": not_found, "files_removed_count": total_files, } diff --git a/src/bmad_loop/data/skills/bmad-loop-setup/scripts/merge-config.py b/src/bmad_loop/data/skills/bmad-loop-setup/scripts/merge-config.py index 665d5f8..3aa621a 100755 --- a/src/bmad_loop/data/skills/bmad-loop-setup/scripts/merge-config.py +++ b/src/bmad_loop/data/skills/bmad-loop-setup/scripts/merge-config.py @@ -12,9 +12,10 @@ Legacy migration: when --legacy-dir is provided, reads old per-module config files from {legacy-dir}/{module-code}/config.yaml and {legacy-dir}/core/config.yaml. -Matching values serve as fallback defaults (answers override them). After a -successful merge, the legacy config.yaml files are deleted. Only the current -module and core directories are touched — other module directories are left alone. +Matching values serve as fallback defaults (answers override them). These legacy +files are READ ONLY and never deleted — on BMAD v6 the per-module and core +config.yaml are live, manifest-tracked files, and the consolidated _bmad/config.yaml +wins on read regardless. Other modules' config is never touched. Exit codes: 0=success, 1=validation error, 2=runtime error """ @@ -58,7 +59,8 @@ def parse_args(): parser.add_argument( "--legacy-dir", help="Path to _bmad/ directory to check for legacy per-module config files. " - "Matching values are used as fallback defaults, then legacy files are deleted.", + "Matching values are used as fallback defaults; the legacy files are read " + "only and never deleted (they are live, manifest-tracked config on BMAD v6).", ) parser.add_argument( "--verbose", @@ -160,19 +162,22 @@ def apply_legacy_defaults(answers: dict, legacy_core: dict, legacy_module: dict) def cleanup_legacy_configs(legacy_dir: str, module_code: str, verbose: bool = False) -> list: - """Delete legacy config.yaml files for this module and core only. - - Returns list of deleted file paths. + """Intentionally does NOT delete any legacy config files (returns an empty list). + + Legacy per-module (_bmad//config.yaml) and core (_bmad/core/config.yaml) + configs are read as fallback defaults (see load_legacy_values) but never deleted: + on BMAD v6 both are LIVE, manifest-tracked files, so removing core/config.yaml + destroys shared core config and removing /config.yaml desyncs + _bmad/_config/files-manifest.csv. The consolidated _bmad/config.yaml always wins + on read, so leaving the legacy files in place is harmless. Kept as a function so + callers and the result JSON stay stable. """ - deleted = [] - for subdir in (module_code, "core"): - legacy_path = Path(legacy_dir) / subdir / "config.yaml" - if legacy_path.exists(): - if verbose: - print(f"Deleting legacy config: {legacy_path}", file=sys.stderr) - legacy_path.unlink() - deleted.append(str(legacy_path)) - return deleted + if verbose: + print( + "Preserving legacy config files (live BMAD v6 config is never deleted)", + file=sys.stderr, + ) + return [] def extract_module_metadata(module_yaml: dict) -> dict: diff --git a/src/bmad_loop/data/skills/bmad-loop-setup/scripts/merge-help-csv.py b/src/bmad_loop/data/skills/bmad-loop-setup/scripts/merge-help-csv.py index 872b6aa..784b943 100755 --- a/src/bmad_loop/data/skills/bmad-loop-setup/scripts/merge-help-csv.py +++ b/src/bmad_loop/data/skills/bmad-loop-setup/scripts/merge-help-csv.py @@ -9,9 +9,10 @@ Uses an anti-zombie pattern: all existing rows matching the source module code are removed before appending fresh rows. -Legacy cleanup: when --legacy-dir and --module-code are provided, deletes old -per-module module-help.csv files from {legacy-dir}/{module-code}/ and -{legacy-dir}/core/. Only the current module and core are touched. +Legacy cleanup: DISABLED. Old per-module module-help.csv files (including +_bmad/core/module-help.csv) are never deleted — on BMAD v6 they are live, +manifest-tracked files. The --legacy-dir / --module-code flags are accepted for +backward compatibility but no longer remove anything. Exit codes: 0=success, 1=validation error, 2=runtime error """ @@ -122,19 +123,20 @@ def write_csv(path: str, header: list[str], rows: list[list[str]], verbose: bool def cleanup_legacy_csvs(legacy_dir: str, module_code: str, verbose: bool = False) -> list: - """Delete legacy per-module module-help.csv files for this module and core only. + """Intentionally does NOT delete any legacy CSV files (returns an empty list). - Returns list of deleted file paths. + Old per-module module-help.csv files — including _bmad/core/module-help.csv — are + live, manifest-tracked config on BMAD v6, so removing them destroys shared BMAD + state or desyncs _bmad/_config/files-manifest.csv. The anti-zombie merge into the + shared _bmad/module-help.csv already supersedes their entries, so leaving them in + place is harmless. Kept as a function so callers and the result JSON stay stable. """ - deleted = [] - for subdir in (module_code, "core"): - legacy_path = Path(legacy_dir) / subdir / "module-help.csv" - if legacy_path.exists(): - if verbose: - print(f"Deleting legacy CSV: {legacy_path}", file=sys.stderr) - legacy_path.unlink() - deleted.append(str(legacy_path)) - return deleted + if verbose: + print( + "Preserving legacy module-help.csv files (live BMAD v6 config is never deleted)", + file=sys.stderr, + ) + return [] def reject_unresolved_paths(named_paths: list[tuple[str, str]]) -> None: diff --git a/tests/test_setup_cleanup_scripts.py b/tests/test_setup_cleanup_scripts.py new file mode 100644 index 0000000..9a986f7 --- /dev/null +++ b/tests/test_setup_cleanup_scripts.py @@ -0,0 +1,216 @@ +"""Regression guard for bmad-loop#64: the bmad-loop-setup cleanup scripts must +never delete live, manifest-tracked BMAD config/state. + +On a BMAD v6 install ``_bmad/core/``, ``_bmad//`` and ``_bmad/_config/`` +hold live config + installer manifests (no staged ``SKILL.md``). The setup scripts +used to hardcode ``core`` and ``--also-remove _config`` into their delete lists, +destroying that shared state. These tests run the real scripts as the SKILL.md +documents and assert only genuine redundant skill-payload dirs are ever removed. + +Root cause is shared with upstream ``bmad-code-org/bmad-builder#96``. +""" + +from __future__ import annotations + +import json +import subprocess +import sys +from pathlib import Path + +REPO = Path(__file__).resolve().parent.parent +SCRIPTS = REPO / "src" / "bmad_loop" / "data" / "skills" / "bmad-loop-setup" / "scripts" +ASSETS = REPO / "src" / "bmad_loop" / "data" / "skills" / "bmad-loop-setup" / "assets" + + +def _run(script: str, *args: str) -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, str(SCRIPTS / script), *args], + capture_output=True, + text=True, + ) + + +def _run_json(script: str, *args: str) -> dict: + proc = _run(script, *args) + assert ( + proc.returncode == 0 + ), f"{script} exit {proc.returncode}\nOUT:{proc.stdout}\nERR:{proc.stderr}" + return json.loads(proc.stdout) + + +def _v6_bmad(tmp_path: Path) -> Path: + """A realistic BMAD v6 `_bmad/` tree: live config + manifest, no staged skills.""" + bmad = tmp_path / "_bmad" + (bmad / "core").mkdir(parents=True) + (bmad / "core" / "config.yaml").write_text("user_name: BMad\noutput_folder: out\n") + (bmad / "core" / "module-help.csv").write_text("module,skill\ncore,x\n") + (bmad / "bmm").mkdir() + (bmad / "bmm" / "config.yaml").write_text("dev_story_location: docs\n") + (bmad / "bmad-loop").mkdir() + (bmad / "bmad-loop" / "config.yaml").write_text("cadence: fast\n") + cfg = bmad / "_config" + cfg.mkdir() + (cfg / "manifest.yaml").write_text("installation:\n version: 6.10.0\n") + (cfg / "files-manifest.csv").write_text("path,hash\n_bmad/core/config.yaml,abc\n") + (cfg / "bmad-help.csv").write_text("module,skill\ncore,x\n") + return bmad + + +def _skills_dir(tmp_path: Path, *installed: str) -> Path: + skills = tmp_path / ".claude" / "skills" + skills.mkdir(parents=True) + for name in installed: + d = skills / name + d.mkdir() + (d / "SKILL.md").write_text(f"# {name}\n") + return skills + + +# ---------------------------------------------------------------- cleanup-legacy + + +def test_cleanup_preserves_core_and_config_on_v6(tmp_path): + """The documented invocation must be a no-op on a v6 install.""" + bmad = _v6_bmad(tmp_path) + skills = _skills_dir(tmp_path) # nothing installed — bmad-loop dir is config-bearing + + result = _run_json( + "cleanup-legacy.py", + "--bmad-dir", + str(bmad), + "--module-code", + "bmad-loop", + "--skills-dir", + str(skills), + ) + + assert result["directories_removed"] == [] + assert (bmad / "bmad-loop" / "config.yaml").exists() + assert (bmad / "core" / "config.yaml").exists() + assert (bmad / "_config" / "manifest.yaml").exists() + protected = {p["dir"] for p in result["directories_protected"]} + assert "bmad-loop" in protected + + +def test_cleanup_refuses_explicit_core_and_config(tmp_path): + """Even an explicit --module-code core / --also-remove _config must not delete them.""" + bmad = _v6_bmad(tmp_path) + skills = _skills_dir(tmp_path) + + result = _run_json( + "cleanup-legacy.py", + "--bmad-dir", + str(bmad), + "--module-code", + "core", + "--also-remove", + "_config", + "--skills-dir", + str(skills), + ) + + assert result["directories_removed"] == [] + assert (bmad / "core" / "config.yaml").exists() + assert (bmad / "_config" / "manifest.yaml").exists() + assert (bmad / "_config" / "files-manifest.csv").exists() + + +def test_cleanup_removes_genuine_redundant_payload(tmp_path): + """A dir with an installed SKILL.md payload and no config IS cleaned.""" + bmad = _v6_bmad(tmp_path) + payload = bmad / "legacy-mod" + payload.mkdir() + (payload / "SKILL.md").write_text("# legacy-mod\n") # skill name == 'legacy-mod' + skills = _skills_dir(tmp_path, "legacy-mod") + + result = _run_json( + "cleanup-legacy.py", + "--bmad-dir", + str(bmad), + "--module-code", + "legacy-mod", + "--skills-dir", + str(skills), + ) + + assert result["directories_removed"] == ["legacy-mod"] + assert not payload.exists() + # protecting v6 state is unaffected + assert (bmad / "core" / "config.yaml").exists() + + +def test_cleanup_errors_when_payload_skill_not_installed(tmp_path): + """A payload dir whose skill is missing from skills-dir is an error, not a delete.""" + bmad = _v6_bmad(tmp_path) + payload = bmad / "legacy-mod" + payload.mkdir() + (payload / "SKILL.md").write_text("# legacy-mod\n") + skills = _skills_dir(tmp_path) # legacy-mod NOT installed + + proc = _run( + "cleanup-legacy.py", + "--bmad-dir", + str(bmad), + "--module-code", + "legacy-mod", + "--skills-dir", + str(skills), + ) + assert proc.returncode == 1 + assert payload.exists() + err = json.loads(proc.stdout) + assert err["status"] == "error" + assert "legacy-mod" in err["missing_skills"] + + +# ---------------------------------------------------------------- merge-config + + +def test_merge_config_preserves_legacy_configs(tmp_path): + bmad = _v6_bmad(tmp_path) + answers = tmp_path / "answers.json" + answers.write_text("{}") + + result = _run_json( + "merge-config.py", + "--config-path", + str(bmad / "config.yaml"), + "--user-config-path", + str(bmad / "config.user.yaml"), + "--module-yaml", + str(ASSETS / "module.yaml"), + "--answers", + str(answers), + "--legacy-dir", + str(bmad), + ) + + assert result["legacy_configs_deleted"] == [] + # live per-module + core config survive + assert (bmad / "core" / "config.yaml").exists() + assert (bmad / "bmad-loop" / "config.yaml").exists() + # and the consolidated config was still written + assert (bmad / "config.yaml").exists() + + +# ---------------------------------------------------------------- merge-help-csv + + +def test_merge_help_csv_preserves_legacy_csvs(tmp_path): + bmad = _v6_bmad(tmp_path) + + result = _run_json( + "merge-help-csv.py", + "--target", + str(bmad / "module-help.csv"), + "--source", + str(ASSETS / "module-help.csv"), + "--legacy-dir", + str(bmad), + "--module-code", + "bmad-loop", + ) + + assert result["legacy_csvs_deleted"] == [] + assert (bmad / "core" / "module-help.csv").exists() + assert (bmad / "module-help.csv").exists() From bb0ff97d57d5ec4ca65625157eaf7ecbcc51c72e Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 6 Jul 2026 10:35:19 -0700 Subject: [PATCH 2/4] fix(setup): report removable dirs absent at removal time (#73 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../bmad-loop-setup/scripts/cleanup-legacy.py | 8 +++-- tests/test_setup_cleanup_scripts.py | 32 +++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py b/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py index 4a4f0f8..135b81c 100755 --- a/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py +++ b/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py @@ -284,8 +284,12 @@ def main(): bmad_dir, dirs_to_check, args.skills_dir, args.verbose ) - # Remove only the verified-redundant payload directories - removed, _, total_files = cleanup_directories(bmad_dir, removable, args.verbose) + # Remove only the verified-redundant payload directories. A removable dir can + # vanish between classify_dirs() and here (TOCTOU) or a nested --also-remove + # target can be removed with its parent earlier in this loop; surface those in + # directories_not_found rather than silently dropping them. + removed, removal_not_found, total_files = cleanup_directories(bmad_dir, removable, args.verbose) + not_found = not_found + removal_not_found # Build result result = { diff --git a/tests/test_setup_cleanup_scripts.py b/tests/test_setup_cleanup_scripts.py index 9a986f7..a3ecc2c 100644 --- a/tests/test_setup_cleanup_scripts.py +++ b/tests/test_setup_cleanup_scripts.py @@ -163,6 +163,38 @@ def test_cleanup_errors_when_payload_skill_not_installed(tmp_path): assert "legacy-mod" in err["missing_skills"] +def test_cleanup_reports_removable_dir_absent_at_removal_time(tmp_path): + """A removable dir gone by removal time must surface in directories_not_found. + + Regression for bmad-loop#73 review: cleanup_directories()'s not_found used to be + discarded, so such a dir silently vanished from every JSON list. Exercised + deterministically via a nested --also-remove target whose parent is removed first. + """ + bmad = _v6_bmad(tmp_path) + # 'legacy/' has no SKILL.md of its own; its only payload is 'legacy/child'. + (bmad / "legacy" / "child").mkdir(parents=True) + (bmad / "legacy" / "child" / "SKILL.md").write_text("# child\n") + skills = _skills_dir(tmp_path, "child") + + result = _run_json( + "cleanup-legacy.py", + "--bmad-dir", + str(bmad), + "--module-code", + "legacy", + "--also-remove", + "legacy/child", # removed together with its parent 'legacy' earlier in the run + "--skills-dir", + str(skills), + ) + + # Parent is removed; the nested child is already gone by its turn but must still + # be reported rather than silently dropped from all output lists. + assert result["directories_removed"] == ["legacy"] + assert "legacy/child" in result["directories_not_found"] + assert not (bmad / "legacy").exists() + + # ---------------------------------------------------------------- merge-config From 9d4973a9d7115f94471bb6290e5480b713dd1fcc Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 8 Jul 2026 17:20:21 -0700 Subject: [PATCH 3/4] fix(setup): name-protect core, deep-scan config markers payload-aware (#73 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../data/skills/bmad-loop-setup/SKILL.md | 2 +- .../bmad-loop-setup/scripts/cleanup-legacy.py | 64 ++++++++++--- tests/test_setup_cleanup_scripts.py | 90 +++++++++++++++++++ 3 files changed, 141 insertions(+), 15 deletions(-) diff --git a/src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md b/src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md index 4e0f053..dca4878 100644 --- a/src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md +++ b/src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md @@ -185,7 +185,7 @@ As with the merge scripts, replace `{project-root}` in the `--bmad-dir` and `--s python3 ./scripts/cleanup-legacy.py --bmad-dir "{project-root}/_bmad" --module-code bmad-loop --skills-dir "{project-root}/.claude/skills" ``` -A directory is removed 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 at `.claude/skills/`. Live config directories (`core/`, per-module, `_config/`) are protected and reported under `directories_protected`. If the script exits non-zero, surface the error and stop. Missing directories (already cleaned by a prior run) are not errors — the script is idempotent. +A directory is removed only when it is a **verified-redundant skill payload**: it contains a `SKILL.md`, carries no live config/manifest files anywhere in its tree (marker-named files inside a staged skill payload don't count — skills legitimately ship e.g. `assets/module-help.csv`), and (with `--skills-dir`) its skills are verified installed at `.claude/skills/`. Live config directories (per-module and nested config/manifests) are protected and reported under `directories_protected`; `core/` and `_config/` are protected by name regardless of contents. If the script exits non-zero, surface the error and stop. Missing directories (already cleaned by a prior run) are not errors — the script is idempotent. **On a rename-upgrade from bmad-auto**, if the old `_bmad/bauto/` directory holds pre-rename `bmad-auto-*` skill copies (an older payload layout), run this cleanup **without** `--skills-dir` to skip installed-skill verification (their removal is the point). It is a no-op when `_bmad/bauto/` holds only config or isn't present: diff --git a/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py b/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py index 135b81c..60b1d93 100755 --- a/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py +++ b/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py @@ -11,15 +11,17 @@ A directory is removed ONLY when it is a verified-redundant skill payload: * it contains >=1 SKILL.md, AND - * it carries no live config/manifest files (config.yaml, module-help.csv, - installer manifests, or the _config/ dir itself), AND + * it carries no live config/manifest files anywhere in its tree (config.yaml, + module-help.csv, installer manifests, or a _config/ dir) — marker-named files + inside a staged skill payload (under a SKILL.md-bearing dir) don't count, AND * when --skills-dir is given, every skill in it is verified installed there. 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 -driven entirely by directory contents, so no version check is needed. +shared BMAD state. 'core' and '_config' are never removed: both are protected by +name regardless of contents. Every other removal decision is driven entirely by +directory contents, so no version check is needed. Exit codes: 0=success (including nothing to remove), 1=validation error, 2=runtime error """ @@ -87,22 +89,50 @@ def find_skill_dirs(base_path: str) -> list: _CONFIG_MARKERS = ("config.yaml", "config.user.yaml", "module-help.csv", "manifest.yaml") +def _inside_skill_payload(item: Path, root: Path) -> bool: + """True if item sits inside a staged skill-payload subtree of root. + + A dir at or above item — strictly below root — containing a SKILL.md marks a + staged skill payload; marker-named files under it (e.g. a skill's own + assets/module-help.csv) are payload data, not live state. root itself is + excluded so markers at the candidate's top level always stay protective. + """ + d = item.parent + while d != root: + if (d / "SKILL.md").exists(): + return True + d = d.parent + return False + + +def _is_config_marker(item: Path) -> bool: + return ( + item.name in _CONFIG_MARKERS + or item.name.endswith("-manifest.csv") + or item.name == "bmad-help.csv" + ) + + def is_config_bearing(path: Path) -> bool: """True if the directory holds live BMAD config or installer-manifest state. Such a directory (e.g. _bmad/core/, _bmad//, _bmad/_config/) is never a - redundant skill payload and must not be removed. Covers the _config/ manifest dir - by name, per-module/core config.yaml + module-help.csv, and installer manifests - (*-manifest.csv, manifest.yaml, bmad-help.csv). + redundant skill payload and must not be removed. 'core' and '_config' are + protected by name regardless of contents. Otherwise the whole tree is scanned + for per-module/core config.yaml + module-help.csv, installer manifests + (*-manifest.csv, manifest.yaml, bmad-help.csv), and nested _config/ dirs — + skipping markers that belong to a staged skill payload (see + _inside_skill_payload), which are disposable copies rather than live state. """ - if path.name == "_config": + if path.name in ("_config", "core"): return True - for marker in _CONFIG_MARKERS: - if (path / marker).exists(): - return True - for csv in path.glob("*.csv"): - if csv.name.endswith("-manifest.csv") or csv.name == "bmad-help.csv": - return True + for item in path.rglob("*"): + if item.is_dir() and item.name == "_config": + if not _inside_skill_payload(item, path): + return True + elif item.is_file() and _is_config_marker(item): + if not _inside_skill_payload(item, path): + return True return False @@ -133,6 +163,12 @@ def classify_dirs( print(f"Not found (skipping): {target}", file=sys.stderr) continue + if target.name in ("_config", "core"): + protected.append({"dir": dirname, "reason": "live BMAD dir (protected by name)"}) + if verbose: + print(f"Protected by name, not removing: {target}", file=sys.stderr) + continue + if is_config_bearing(target): protected.append({"dir": dirname, "reason": "holds live config/manifest"}) if verbose: diff --git a/tests/test_setup_cleanup_scripts.py b/tests/test_setup_cleanup_scripts.py index a3ecc2c..2fa2bae 100644 --- a/tests/test_setup_cleanup_scripts.py +++ b/tests/test_setup_cleanup_scripts.py @@ -195,6 +195,96 @@ def test_cleanup_reports_removable_dir_absent_at_removal_time(tmp_path): assert not (bmad / "legacy").exists() +def test_cleanup_protects_core_by_name_even_as_pure_payload(tmp_path): + """core/ is never removed — even holding only a SKILL.md payload and no config. + + Regression for bmad-loop#73 review: the docstring promised 'core' is never + removed, but only _config was protected by name; a marker-less core payload + slipped through the config-bearing check. + """ + bmad = tmp_path / "_bmad" + core = bmad / "core" + core.mkdir(parents=True) + (core / "SKILL.md").write_text("# core\n") # no config markers at all + skills = _skills_dir(tmp_path, "core") + + result = _run_json( + "cleanup-legacy.py", + "--bmad-dir", + str(bmad), + "--module-code", + "core", + "--skills-dir", + str(skills), + ) + + assert result["directories_removed"] == [] + assert (core / "SKILL.md").exists() + protected = {p["dir"] for p in result["directories_protected"]} + assert "core" in protected + + +def test_cleanup_protects_dir_with_nested_live_config(tmp_path): + """A config marker nested below the top level still protects the whole dir. + + Regression for bmad-loop#73 review: is_config_bearing() only looked at the + candidate's top level while skill discovery was recursive, so a dir with a + SKILL.md and deeper live config was misclassified as a removable payload. + """ + bmad = _v6_bmad(tmp_path) + legacy = bmad / "legacy-mod" + (legacy / "sub").mkdir(parents=True) + (legacy / "SKILL.md").write_text("# legacy-mod\n") + (legacy / "sub" / "config.yaml").write_text("live: true\n") # nested live state + skills = _skills_dir(tmp_path, "legacy-mod") + + result = _run_json( + "cleanup-legacy.py", + "--bmad-dir", + str(bmad), + "--module-code", + "legacy-mod", + "--skills-dir", + str(skills), + ) + + assert result["directories_removed"] == [] + assert (legacy / "sub" / "config.yaml").exists() + protected = {p["dir"] for p in result["directories_protected"]} + assert "legacy-mod" in protected + + +def test_cleanup_removes_payload_whose_skill_ships_marker_named_assets(tmp_path): + """Marker-named files inside a SKILL.md-bearing subtree are payload, not live state. + + Staged skill payloads legitimately ship files like assets/module-help.csv + (bmad-loop-setup itself does); the recursive config scan must not + false-protect such a dir or the documented bauto rename-cleanup would no-op. + """ + bmad = _v6_bmad(tmp_path) + legacy = bmad / "legacy-mod" + foo = legacy / "skills" / "foo" + (foo / "assets").mkdir(parents=True) + (foo / "SKILL.md").write_text("# foo\n") + (foo / "assets" / "module-help.csv").write_text("module,skill\nfoo,x\n") + skills = _skills_dir(tmp_path, "foo") + + result = _run_json( + "cleanup-legacy.py", + "--bmad-dir", + str(bmad), + "--module-code", + "legacy-mod", + "--skills-dir", + str(skills), + ) + + assert result["directories_removed"] == ["legacy-mod"] + assert not legacy.exists() + # live v6 state is untouched + assert (bmad / "core" / "config.yaml").exists() + + # ---------------------------------------------------------------- merge-config From adc4d52aa0aa7293c489e5d23dd2f5a849c3d391 Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 8 Jul 2026 19:08:42 -0700 Subject: [PATCH 4/4] docs(setup): make cleanup --skills-dir guidance CLI-tree-aware (#73 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md | 10 +++++----- .../skills/bmad-loop-setup/scripts/cleanup-legacy.py | 10 ++++++---- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md b/src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md index dca4878..1bc2947 100644 --- a/src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md +++ b/src/bmad_loop/data/skills/bmad-loop-setup/SKILL.md @@ -151,7 +151,7 @@ Unless the user explicitly asked to skip it (e.g. `skills only` / `--no-tool`), Names must be exactly `claude`, `codex`, `gemini`, `copilot`, or `antigravity` — `init` errors on an unknown profile and lists the valid ones. `init` prints any one-time first-run notes per CLI (e.g. start `claude` once in the project and accept the workspace-trust + hooks-approval dialogs before `bmad-loop run` — spawned sessions can't answer first-run dialogs). Relay those notes to the user. - **Skills are installed automatically:** `init` lays the bundled `bmad-loop-*` skills into the right tree for each selected CLI — `.claude/skills/` for `claude`, `.agents/skills/` for `codex`/`gemini`. On a fresh install, existing skill dirs are left untouched; on an upgrade, `--force-skills` overwrites them with the bundled copies from the upgraded tool (use `--no-skills` to skip the step and manage skills yourself). + **Skills are installed automatically:** `init` lays the bundled `bmad-loop-*` skills into the right tree for each selected CLI — `.claude/skills/` for `claude`, `.agents/skills/` for `codex`/`gemini`/`copilot`/`antigravity`. On a fresh install, existing skill dirs are left untouched; on an upgrade, `--force-skills` overwrites them with the bundled copies from the upgraded tool (use `--no-skills` to skip the step and manage skills yourself). > **Note:** `--force-skills` also overwrites `bmad-loop-setup` itself (it ships in the same bundle). That's expected and safe — the freshly laid-down setup skill takes effect on the **next** invocation, and your `_bmad/custom/*.toml` overrides (keyed by skill directory name) are untouched. @@ -177,15 +177,15 @@ Unless the user explicitly asked to skip it (e.g. `skills only` / `--no-tool`), ## Cleanup Legacy Directories -After both merge scripts complete successfully, remove any **redundant skill-payload** directories an older installer may have staged under `_bmad/` (past layouts duplicated skills that already live at `.claude/skills/`). On a modern BMAD v6 install nothing redundant is staged, so this step is a **safe no-op** — it never touches `_bmad/core/`, per-module config, or the `_bmad/_config/` manifest. +After both merge scripts complete successfully, remove any **redundant skill-payload** directories an older installer may have staged under `_bmad/` (past layouts duplicated skills that already live in the CLI's skill tree, e.g. `.claude/skills/`). On a modern BMAD v6 install nothing redundant is staged, so this step is a **safe no-op** — it never touches `_bmad/core/`, per-module config, or the `_bmad/_config/` manifest. -As with the merge scripts, replace `{project-root}` in the `--bmad-dir` and `--skills-dir` path arguments with the actual project root before running. Do **not** pass `--also-remove _config` (or any shared config/manifest dir): `_config/` is live BMAD infrastructure and the script refuses to remove it regardless. +As with the merge scripts, replace `{project-root}` in the `--bmad-dir` and `--skills-dir` path arguments with the actual project root before running. For `--skills-dir`, use the skill tree of a CLI registered in step 3: `.claude/skills` for `claude`, `.agents/skills` for `codex`/`gemini`/`copilot`/`antigravity`. `init` lays the bundled skills into **every** selected CLI's tree, so any registered CLI's tree verifies correctly — the command below shows the `claude` tree; substitute `.agents/skills` if only non-Claude CLIs were registered. Do **not** pass `--also-remove _config` (or any shared config/manifest dir): `_config/` is live BMAD infrastructure and the script refuses to remove it regardless. ```bash python3 ./scripts/cleanup-legacy.py --bmad-dir "{project-root}/_bmad" --module-code bmad-loop --skills-dir "{project-root}/.claude/skills" ``` -A directory is removed only when it is a **verified-redundant skill payload**: it contains a `SKILL.md`, carries no live config/manifest files anywhere in its tree (marker-named files inside a staged skill payload don't count — skills legitimately ship e.g. `assets/module-help.csv`), and (with `--skills-dir`) its skills are verified installed at `.claude/skills/`. Live config directories (per-module and nested config/manifests) are protected and reported under `directories_protected`; `core/` and `_config/` are protected by name regardless of contents. If the script exits non-zero, surface the error and stop. Missing directories (already cleaned by a prior run) are not errors — the script is idempotent. +A directory is removed only when it is a **verified-redundant skill payload**: it contains a `SKILL.md`, carries no live config/manifest files anywhere in its tree (marker-named files inside a staged skill payload don't count — skills legitimately ship e.g. `assets/module-help.csv`), and (with `--skills-dir`) its skills are verified installed under the `--skills-dir` tree. Live config directories (per-module and nested config/manifests) are protected and reported under `directories_protected`; `core/` and `_config/` are protected by name regardless of contents. If the script exits non-zero, surface the error and stop. Missing directories (already cleaned by a prior run) are not errors — the script is idempotent. **On a rename-upgrade from bmad-auto**, if the old `_bmad/bauto/` directory holds pre-rename `bmad-auto-*` skill copies (an older payload layout), run this cleanup **without** `--skills-dir` to skip installed-skill verification (their removal is the point). It is a no-op when `_bmad/bauto/` holds only config or isn't present: @@ -204,7 +204,7 @@ Report the **tool** result according to the branch taken: - **Fresh install:** the installed `bmad-loop --version`, that `bmad-loop init` registered hooks, installed the `bmad-loop-*` skills, and wrote policy/gitignore for the selected coding CLI(s) (name each one — e.g. "hooks + skills installed for claude, codex"). - **Upgrade:** the before → after `bmad-loop --version` (e.g. "upgraded 0.3.1 → 0.3.2", or "already current at 0.3.2"), that the `bmad-loop-*` skills were **refreshed** (not skipped) with `--force-skills` in each CLI tree, and the re-stamped config version. -Also report the `bmad-loop validate` preflight result (pass, or the readiness checklist of what's still missing). If legacy config was consolidated, mention the migration (live config files are preserved, not deleted). If any redundant skill-payload directories were removed, report the count and list from `directories_removed` (e.g. "Cleaned up a redundant skill-payload directory — skills are installed at .claude/skills/"); on a v6 install this is typically empty and `directories_protected` confirms `core/` and `_config/` were left intact. Then display the `module_greeting` from `./assets/module.yaml` to the user. +Also report the `bmad-loop validate` preflight result (pass, or the readiness checklist of what's still missing). If legacy config was consolidated, mention the migration (live config files are preserved, not deleted). If any redundant skill-payload directories were removed, report the count and list from `directories_removed` (e.g. "Cleaned up a redundant skill-payload directory — skills are installed in the CLI's skill tree"); on a v6 install this is typically empty and `directories_protected` confirms `core/` and `_config/` were left intact. Then display the `module_greeting` from `./assets/module.yaml` to the user. ## Outcome diff --git a/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py b/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py index 60b1d93..df109bc 100755 --- a/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py +++ b/src/bmad_loop/data/skills/bmad-loop-setup/scripts/cleanup-legacy.py @@ -6,8 +6,9 @@ """Remove redundant legacy skill-payload directories from _bmad/ after config migration. Older BMAD installers could stage a module's skill payload *and* its config into -_bmad// (and _bmad/core/), duplicating skills that also live at -.claude/skills/. This script removes only those *redundant skill-payload* trees. +_bmad// (and _bmad/core/), duplicating skills that also live in the CLI's +installed-skills tree (e.g. .claude/skills/ or .agents/skills/). This script +removes only those *redundant skill-payload* trees. A directory is removed ONLY when it is a verified-redundant skill payload: * it contains >=1 SKILL.md, AND @@ -55,8 +56,9 @@ def parse_args(): ) parser.add_argument( "--skills-dir", - help="Path to .claude/skills/ — enables safety verification that skills " - "are installed before removing legacy copies", + help="Path to the CLI's installed-skills tree (.claude/skills/ for claude, " + ".agents/skills/ for codex/gemini/copilot/antigravity) — enables safety " + "verification that skills are installed before removing legacy copies", ) parser.add_argument( "--verbose",