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..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 @@ -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,42 @@ 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. 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 = { "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..a3ecc2c --- /dev/null +++ b/tests/test_setup_cleanup_scripts.py @@ -0,0 +1,248 @@ +"""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"] + + +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 + + +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()