fix(setup): never delete live core BMAD config/manifest (#64)

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

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
 

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/<module>/ (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/<module>/, _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/, <module>/ 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,
     }