Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion skills/bmad-module-builder/references/validate-module.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
86 changes: 82 additions & 4 deletions skills/bmad-module-builder/scripts/tests/test-validate-module.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand All @@ -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

Expand Down Expand Up @@ -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)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
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:
Expand Down Expand Up @@ -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,
]
Expand Down
66 changes: 55 additions & 11 deletions skills/bmad-module-builder/scripts/validate-module.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand All @@ -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]] = {}
Expand Down Expand Up @@ -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")
Expand Down
Loading