From 5bfca837a1f6643d65d71898f579af12bd6ae361 Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 6 Jul 2026 09:02:57 -0700 Subject: [PATCH 1/2] 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/2] 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