diff --git a/skills/bmad-module-builder/references/validate-module.md b/skills/bmad-module-builder/references/validate-module.md index 398eac2..d1b3459 100644 --- a/skills/bmad-module-builder/references/validate-module.md +++ b/skills/bmad-module-builder/references/validate-module.md @@ -13,7 +13,7 @@ You are a module quality reviewer. Your job is to verify that a BMad module's st Ask the user for the path to their module's skills folder (or a single skill folder for standalone modules). The validation script auto-detects the module type: - **Multi-skill module:** Identifies the setup skill (`*-setup`) and all other skill folders -- **Standalone module:** Detected when no setup skill exists and the folder contains a single skill with `assets/module.yaml`. Validates: `assets/module-setup.md`, `assets/module.yaml`, `assets/module-help.csv`, `scripts/merge-config.py`, `scripts/merge-help-csv.py` +- **Standalone module:** Detected when no setup skill exists and either the folder contains a single skill with `assets/module.yaml`, or the path given _is_ the standalone skill directory itself. Validates: `assets/module-setup.md`, `assets/module.yaml`, `assets/module-help.csv`, and the two merge scripts — either the scaffolder's dash form (`scripts/merge-config.py`, `scripts/merge-help-csv.py`) or the importable underscore form (`scripts/merge_config.py`, `scripts/merge_help_csv.py`) is accepted. ### 2. Run Structural Validation diff --git a/skills/bmad-module-builder/scripts/tests/test-validate-module.py b/skills/bmad-module-builder/scripts/tests/test-validate-module.py index f59cc38..8c0366e 100644 --- a/skills/bmad-module-builder/scripts/tests/test-validate-module.py +++ b/skills/bmad-module-builder/scripts/tests/test-validate-module.py @@ -235,8 +235,14 @@ def test_short_row_does_not_crash(): def create_standalone_module(tmp: Path, skill_name: str = "my-skill", csv_rows: str = "", yaml_content: str = "", include_setup_md: bool = True, - include_merge_scripts: bool = True) -> Path: - """Create a minimal standalone module structure for testing.""" + include_merge_scripts: bool = True, + merge_script_style: str = "dash") -> Path: + """Create a minimal standalone module structure for testing. + + ``merge_script_style`` selects the merge-script naming form: "dash" for the + scaffolder default (merge-config.py) or "underscore" for the importable form + (merge_config.py). Both are valid. + """ module_dir = tmp / "module" module_dir.mkdir() @@ -259,8 +265,12 @@ def create_standalone_module(tmp: Path, skill_name: str = "my-skill", if include_merge_scripts: scripts = skill / "scripts" scripts.mkdir() - (scripts / "merge-config.py").write_text("# merge-config\n") - (scripts / "merge-help-csv.py").write_text("# merge-help-csv\n") + if merge_script_style == "underscore": + (scripts / "merge_config.py").write_text("# merge_config\n") + (scripts / "merge_help_csv.py").write_text("# merge_help_csv\n") + else: + (scripts / "merge-config.py").write_text("# merge-config\n") + (scripts / "merge-help-csv.py").write_text("# merge-help-csv\n") return module_dir @@ -319,6 +329,70 @@ def test_standalone_csv_validation(): assert "DT" in dupes[0]["message"] +def test_standalone_underscore_merge_scripts(): + """Importable underscore-named merge scripts (merge_config.py) should pass.""" + with tempfile.TemporaryDirectory() as tmp: + tmp = Path(tmp) + module_dir = create_standalone_module(tmp, merge_script_style="underscore") + + code, data = run_validate(module_dir) + assert code == 0, f"Expected pass: {data}" + assert data["status"] == "pass" + assert data["info"].get("standalone") is True + assert data["summary"]["total_findings"] == 0 + + +def test_standalone_cross_module_before_after_ref(): + """Bare (colon-less) preceded-by/followed-by refs are cross-module positional, not flagged.""" + with tempfile.TemporaryDirectory() as tmp: + tmp = Path(tmp) + csv_rows = ('Test Module,my-skill,Do Thing,DT,Does thing,,,anytime,' + 'bmad-sprint-planning,bmad-retrospective,false,output_folder,artifact\n') + module_dir = create_standalone_module(tmp, csv_rows=csv_rows) + + code, data = run_validate(module_dir) + assert code == 0, f"Expected pass: {data}" + refs = [f for f in data["findings"] if f["category"] == "invalid-ref"] + assert refs == [], f"Cross-module bare refs should not be flagged: {refs}" + + +def test_standalone_given_skill_dir_directly(): + """Passing the standalone skill directory itself (not its parent) should work.""" + with tempfile.TemporaryDirectory() as tmp: + tmp = Path(tmp) + module_dir = create_standalone_module(tmp, skill_name="my-skill") + skill_dir = module_dir / "my-skill" + + code, data = run_validate(skill_dir) + assert code == 0, f"Expected pass: {data}" + assert data["status"] == "pass" + assert data["info"].get("standalone") is True + assert data["info"].get("skill_dir") == "my-skill" + + +def test_standalone_skill_dir_orphan_not_masked_by_sibling(): + """Validating a skill dir directly must still flag a CSV skill that only + exists as an unrelated sibling directory (not part of this standalone module).""" + with tempfile.TemporaryDirectory() as tmp: + tmp = Path(tmp) + csv_rows = ( + 'Test Module,my-skill,Do Thing,DT,Does thing,run,,anytime,,,false,output_folder,artifact\n' + 'Test Module,other-skill,Other,OT,Other thing,run,,anytime,,,false,output_folder,report\n' + ) + module_dir = create_standalone_module(tmp, skill_name="my-skill", csv_rows=csv_rows) + # A sibling skill dir next to the standalone skill (a different module). + sibling = module_dir / "other-skill" + sibling.mkdir() + (sibling / "SKILL.md").write_text("---\nname: other-skill\n---\n# other-skill\n") + + skill_dir = module_dir / "my-skill" + code, data = run_validate(skill_dir) + assert code == 1, f"Orphan entry should fail validation: {data}" + orphans = [f for f in data["findings"] if f["category"] == "orphan-entry"] + assert any("other-skill" in f["message"] for f in orphans), \ + f"Sibling skill must not mask the orphan: {data['findings']}" + + def test_multi_skill_not_detected_as_standalone(): """A folder with two skills and no setup skill should fail (not detected as standalone).""" with tempfile.TemporaryDirectory() as tmp: @@ -367,6 +441,10 @@ def test_nonexistent_directory(): test_standalone_missing_module_setup_md, test_standalone_missing_merge_scripts, test_standalone_csv_validation, + test_standalone_underscore_merge_scripts, + test_standalone_cross_module_before_after_ref, + test_standalone_given_skill_dir_directly, + test_standalone_skill_dir_orphan_not_masked_by_sibling, test_multi_skill_not_detected_as_standalone, test_nonexistent_directory, ] diff --git a/skills/bmad-module-builder/scripts/validate-module.py b/skills/bmad-module-builder/scripts/validate-module.py index 14327aa..8eeb734 100644 --- a/skills/bmad-module-builder/scripts/validate-module.py +++ b/skills/bmad-module-builder/scripts/validate-module.py @@ -51,7 +51,16 @@ def find_skill_folders(module_dir: Path, exclude_name: str = "") -> list[str]: def detect_standalone_module(module_dir: Path) -> Path | None: - """Detect a standalone module: single skill folder with assets/module.yaml.""" + """Detect a standalone module: a single skill folder with assets/module.yaml. + + Works whether ``module_dir`` is the parent folder that contains the skill, or + the standalone skill directory itself (the path a user is most likely to hand + over for a single-skill module). + """ + # Given the skill directory directly. + if (module_dir / "SKILL.md").is_file() and (module_dir / "assets" / "module.yaml").is_file(): + return module_dir + # Given the parent folder containing exactly one skill. skill_dirs = [ d for d in module_dir.iterdir() if d.is_dir() and (d / "SKILL.md").is_file() @@ -130,14 +139,32 @@ def finding(severity: str, category: str, message: str, detail: str = ""): "assets/module.yaml": skill_dir / "assets" / "module.yaml", "assets/module-help.csv": skill_dir / "assets" / "module-help.csv", "assets/module-setup.md": skill_dir / "assets" / "module-setup.md", - "scripts/merge-config.py": skill_dir / "scripts" / "merge-config.py", - "scripts/merge-help-csv.py": skill_dir / "scripts" / "merge-help-csv.py", } + # Merge scripts: accept either the dash form the scaffolder emits + # (merge-config.py) or the importable underscore form (merge_config.py). + # Both are valid — a module may rename them to be importable from + # module-setup.md without that being a structural defect. + required_any = { + "scripts/merge-config.py (or merge_config.py)": [ + skill_dir / "scripts" / "merge-config.py", + skill_dir / "scripts" / "merge_config.py", + ], + "scripts/merge-help-csv.py (or merge_help_csv.py)": [ + skill_dir / "scripts" / "merge-help-csv.py", + skill_dir / "scripts" / "merge_help_csv.py", + ], + } + ok = True for label, path in required_files.items(): if not path.is_file(): finding("critical", "structure", f"Missing required file: {label}") + ok = False + for label, candidates in required_any.items(): + if not any(p.is_file() for p in candidates): + finding("critical", "structure", f"Missing required file: {label}") + ok = False - if not all(p.is_file() for p in required_files.values()): + if not ok: return {"status": "fail", "findings": findings, "info": info} yaml_dir = skill_dir @@ -202,8 +229,12 @@ def finding(severity: str, category: str, message: str, detail: str = ""): # 6. Collect skills from CSV and filesystem csv_skills = {row.get("skill", "") for row in rows} - exclude_name = setup_dir.name if setup_dir else "" - skill_folders = find_skill_folders(module_dir, exclude_name) + if standalone_dir: + # The only valid skill is the standalone skill itself, whether we were + # handed the module's parent folder or the skill directory directly. + skill_folders = [standalone_dir.name] + else: + skill_folders = find_skill_folders(module_dir, setup_dir.name) info["skill_folders"] = skill_folders info["csv_skills"] = sorted(csv_skills) @@ -215,10 +246,15 @@ def finding(severity: str, category: str, message: str, detail: str = ""): # 8. Orphan CSV entries setup_name = setup_dir.name if setup_dir else "" for skill in csv_skills: - if skill not in skill_folders and skill != setup_name: - # Check if it's the setup skill itself (valid) - if not (module_dir / skill / "SKILL.md").is_file(): - finding("high", "orphan-entry", f"CSV references skill '{skill}' which does not exist in the module folder") + if skill in skill_folders or skill == setup_name: + continue + # For a standalone module, skill_folders already enumerates every valid + # skill, so any other CSV skill is an orphan — never look at the parent + # folder (which may hold unrelated sibling skills when validating a skill + # dir directly). For a multi-skill module, re-check the filesystem: the + # setup skill lives alongside the others and is excluded from skill_folders. + if standalone_dir or not (module_dir / skill / "SKILL.md").is_file(): + finding("high", "orphan-entry", f"CSV references skill '{skill}' which does not exist in the module folder") # 9. Unique menu codes menu_codes: dict[str, list[str]] = {} @@ -249,7 +285,15 @@ def finding(severity: str, category: str, message: str, detail: str = ""): # Can be comma-separated for ref in value.split(","): ref = ref.strip() - if ref and ref not in valid_refs: + if not ref: + continue + # A colon-less ref is a cross-module positional reference (a bare + # sibling-module skill name, e.g. "bmad-sprint-planning"). Other + # installed modules aren't visible here, so it can't be resolved + # and isn't a defect — only validate intra-module skill:action refs. + if ":" not in ref: + continue + if ref not in valid_refs: finding("medium", "invalid-ref", f"'{display}' {field} references '{ref}' which is not a valid capability", "Expected format: skill-name:action-name")