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
12 changes: 6 additions & 6 deletions docs/explanation/module-configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ If these cover your discoverability needs, you can skip the setup skill entirely
The CSV registers the module's capabilities with the help system. Each row describes one capability that users can discover and invoke. The file has 13 columns:

```csv
module,skill,display-name,menu-code,description,action,args,phase,after,before,required,output-location,outputs
module,skill,display-name,menu-code,description,action,args,phase,preceded-by,followed-by,required,output-location,outputs
```

### Column Guide
Expand All @@ -124,21 +124,21 @@ module,skill,display-name,menu-code,description,action,args,phase,after,before,r
| **action** | Action name within the skill. Distinguishes capabilities when one skill exposes multiple (e.g., `build-process`, `quality-optimizer`) |
| **args** | Arguments the capability accepts (e.g., `[-H] [path]`), shown in help output |
| **phase** | When the capability is available: `anytime` or a workflow phase like `1-analysis`, `2-planning` |
| **after** | Capabilities that should complete before this one: format `skill-name:action`, comma-separated for multiple |
| **before** | Capabilities that should run after this one, same format as `after` |
| **preceded-by** | Capabilities that should complete before this one (this capability is preceded by them): format `skill-name:action`, comma-separated for multiple |
| **followed-by** | Capabilities that should run after this one (this capability is followed by them), same format as `preceded-by` |
| **required** | `true` if this is a blocking gate for phase progression, `false` otherwise |
| **output-location** | Config variable name (e.g., `output_folder`, `bmad_builder_reports`); `bmad-help` resolves from config to scan for completion artifacts |
| **outputs** | File patterns `bmad-help` looks for in the output location to detect completion (e.g., "quality report", "agent skill") |

### How bmad-help Uses These Entries

The `after`/`before` columns create a **dependency graph** that `bmad-help` walks to recommend next steps. `required=true` entries are blocking gates; `bmad-help` will not suggest later-phase capabilities until required gates pass. The `output-location` and `outputs` columns enable **completion detection**: `bmad-help` scans those paths for matching artifacts to determine what's been done.
The `preceded-by`/`followed-by` columns create a **dependency graph** that `bmad-help` walks to recommend next steps. `required=true` entries are blocking gates; `bmad-help` will not suggest later-phase capabilities until required gates pass. The `output-location` and `outputs` columns enable **completion detection**: `bmad-help` scans those paths for matching artifacts to determine what's been done.

### Example Entry

```csv
module,skill,display-name,menu-code,description,action,args,phase,after,before,required,output-location,outputs
BMad Builder,bmad-agent-builder,Build an Agent,BA,"Create, edit, convert, or fix an agent skill.",build-process,"[-H] [description | path]",anytime,,bmad-agent-builder:quality-optimizer,false,output_folder,agent skill
module,skill,display-name,menu-code,description,action,args,phase,preceded-by,followed-by,required,output-location,outputs
BMad Builder,bmad-agent-builder,Build an Agent,BA,"Create, edit, convert, or fix an agent skill.",build-process,"[-H] [description | path]",anytime,,bmad-agent-builder:quality-analysis,false,output_folder,agent skill
```

During registration, these rows are merged into the project-wide `_bmad/module-help.csv`, replacing any existing rows for this module (anti-zombie pattern).
Expand Down
75 changes: 74 additions & 1 deletion skills/bmad-module-builder/scripts/tests/test-validate-module.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@

SCRIPT = Path(__file__).resolve().parent.parent / "validate-module.py"

CSV_HEADER = "module,skill,display-name,menu-code,description,action,args,phase,after,before,required,output-location,outputs\n"
CSV_HEADER = "module,skill,display-name,menu-code,description,action,args,phase,preceded-by,followed-by,required,output-location,outputs\n"
LEGACY_CSV_HEADER = "module,skill,display-name,menu-code,description,action,args,phase,after,before,required,output-location,outputs\n"


def create_module(tmp: Path, skills: list[str] | None = None, csv_rows: str = "",
Expand Down Expand Up @@ -162,6 +163,75 @@ def test_empty_csv():
assert len(empty) == 1


def test_canonical_header_accepted():
"""The canonical preceded-by/followed-by header must NOT produce a header finding."""
with tempfile.TemporaryDirectory() as tmp:
tmp = Path(tmp)
csv_rows = 'Test Module,tst-foo,Do Foo,DF,Does foo,run,,anytime,,,false,output_folder,report\n'
module_dir = create_module(tmp, skills=["tst-foo"], csv_rows=csv_rows)

code, data = run_validate(module_dir)
Comment thread
pbean marked this conversation as resolved.
assert code == 0, f"expected a clean pass: {data}"
assert data["status"] == "pass"
header_findings = [f for f in data["findings"] if f["category"] == "csv-header"]
assert header_findings == [], f"unexpected header findings: {header_findings}"


def test_legacy_after_before_header_flagged():
"""A module-help.csv using the old after/before column names must be flagged as
a header mismatch — canonical is preceded-by/followed-by (matches the templates
and bmad-help). Regression for the CSV_HEADER drift in validate-module.py."""
with tempfile.TemporaryDirectory() as tmp:
tmp = Path(tmp)
module_dir = tmp / "module"
module_dir.mkdir()
setup = module_dir / "tst-setup"
setup.mkdir()
(setup / "SKILL.md").write_text("---\nname: tst-setup\n---\n# Setup\n")
(setup / "assets").mkdir()
(setup / "assets" / "module.yaml").write_text(
'code: tst\nname: "Test Module"\ndescription: "A test module"\n'
)
(setup / "assets" / "module-help.csv").write_text(
LEGACY_CSV_HEADER
+ 'Test Module,tst-foo,Do Foo,DF,Does foo,run,,anytime,,,false,output_folder,report\n'
)
(module_dir / "tst-foo").mkdir()
(module_dir / "tst-foo" / "SKILL.md").write_text("---\nname: tst-foo\n---\n# tst-foo\n")

code, data = run_validate(module_dir)
assert code == 1, f"expected fail (high-severity header finding): {data}"
assert data["status"] == "fail"
header_findings = [f for f in data["findings"] if f["category"] == "csv-header"]
assert len(header_findings) == 1, f"expected a csv-header finding: {data['findings']}"
msg = header_findings[0]["message"]
# missing the new names, has the legacy ones
assert "preceded-by" in msg and "followed-by" in msg
assert "after" in msg and "before" in msg


def test_short_row_does_not_crash():
"""A CSV row with fewer fields than the header must not crash the validator and
must be reported as a column-count mismatch. DictReader fills the missing
columns with None by default, so the validator's `.strip()` calls would raise
AttributeError on a short row — restval="" keeps them safe. Regression test."""
with tempfile.TemporaryDirectory() as tmp:
tmp = Path(tmp)
# Only 5 of the 13 columns present (the remaining 8 are missing entirely).
csv_rows = 'Test Module,tst-foo,Do Foo,DF,Does foo\n'
module_dir = create_module(tmp, skills=["tst-foo"], csv_rows=csv_rows)

code, data = run_validate(module_dir)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
# Valid JSON with findings means the script completed instead of crashing
# with an uncaught traceback (which run_validate would surface as raw_*).
assert "findings" in data, f"validator crashed instead of reporting: {data}"
# A short row is a medium-severity finding: reported, but non-fatal.
assert code == 0 and data["status"] == "pass", f"expected non-fatal pass: {data}"
col_findings = [f for f in data["findings"] if f["category"] == "csv-columns"]
assert len(col_findings) == 1, f"expected a csv-columns finding: {data['findings']}"
assert "5 columns" in col_findings[0]["message"]


def create_standalone_module(tmp: Path, skill_name: str = "my-skill",
csv_rows: str = "", yaml_content: str = "",
include_setup_md: bool = True,
Expand Down Expand Up @@ -290,6 +360,9 @@ def test_nonexistent_directory():
test_invalid_before_after_ref,
test_missing_yaml_fields,
test_empty_csv,
test_canonical_header_accepted,
test_legacy_after_before_header_flagged,
test_short_row_does_not_crash,
test_valid_standalone_module,
test_standalone_missing_module_setup_md,
test_standalone_missing_merge_scripts,
Expand Down
37 changes: 24 additions & 13 deletions skills/bmad-module-builder/scripts/validate-module.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
- All skill folders have at least one capability entry in the CSV
- No orphan CSV entries pointing to nonexistent skills
- Menu codes are unique
- Before/after references point to real capability entries
- preceded-by/followed-by references point to real capability entries
- Required module.yaml fields are present
- CSV column count is consistent
"""
Expand All @@ -28,7 +28,7 @@
REQUIRED_YAML_FIELDS = {"code", "name", "description"}
CSV_HEADER = [
"module", "skill", "display-name", "menu-code", "description",
"action", "args", "phase", "after", "before", "required",
"action", "args", "phase", "preceded-by", "followed-by", "required",
"output-location", "outputs",
]

Expand Down Expand Up @@ -77,12 +77,22 @@ def parse_yaml_minimal(text: str) -> dict[str, str]:
return result


def parse_csv_rows(csv_text: str) -> tuple[list[str], list[dict[str, str]]]:
"""Parse CSV text into header and list of row dicts."""
reader = csv.DictReader(StringIO(csv_text))
def parse_csv_rows(csv_text: str) -> tuple[list[str], list[dict[str, str]], list[int]]:
"""Parse CSV text into (header, row dicts, raw column count per data row).

``restval=""`` fills missing trailing fields in a short row with empty strings
instead of ``None``, so downstream ``.strip()`` calls stay safe on malformed
rows. DictReader pads short rows to the header width, so ``len(row)`` cannot
reveal a field shortfall; the raw per-row column counts from ``csv.reader``
(blank lines skipped, to stay aligned with DictReader) are returned separately
for the column-count consistency check.
"""
reader = csv.DictReader(StringIO(csv_text), restval="")
header = reader.fieldnames or []
rows = list(reader)
return header, rows
raw_rows = list(csv.reader(StringIO(csv_text)))
col_counts = [len(r) for r in raw_rows[1:] if r != []]
return header, rows, col_counts


def validate(module_dir: Path, verbose: bool = False) -> dict:
Expand Down Expand Up @@ -163,7 +173,7 @@ def finding(severity: str, category: str, message: str, detail: str = ""):

# 4. Parse and validate CSV
csv_text = (csv_dir / "assets" / "module-help.csv").read_text(encoding="utf-8")
header, rows = parse_csv_rows(csv_text)
header, rows, col_counts = parse_csv_rows(csv_text)

# Check header
if header != CSV_HEADER:
Expand All @@ -182,11 +192,12 @@ def finding(severity: str, category: str, message: str, detail: str = ""):

info["csv_entries"] = len(rows)

# 5. Check column count consistency
# 5. Check column count consistency (using raw field counts: DictReader pads
# short rows to the header width, so len(row) alone can't detect a shortfall)
expected_cols = len(CSV_HEADER)
for i, row in enumerate(rows):
if len(row) != expected_cols:
finding("medium", "csv-columns", f"Row {i + 2} has {len(row)} columns, expected {expected_cols}",
for i, (row, n_cols) in enumerate(zip(rows, col_counts)):
if n_cols != expected_cols:
finding("medium", "csv-columns", f"Row {i + 2} has {n_cols} columns, expected {expected_cols}",
f"skill={row.get('skill', '?')}")

# 6. Collect skills from CSV and filesystem
Expand Down Expand Up @@ -220,7 +231,7 @@ def finding(severity: str, category: str, message: str, detail: str = ""):
if len(names) > 1:
finding("high", "duplicate-menu-code", f"Menu code '{code}' used by multiple entries: {', '.join(names)}")

# 10. Before/after reference validation
# 10. preceded-by/followed-by reference validation
# Build set of valid capability references (skill:action)
valid_refs = set()
for row in rows:
Expand All @@ -231,7 +242,7 @@ def finding(severity: str, category: str, message: str, detail: str = ""):

for row in rows:
display = row.get("display-name", "?")
for field in ("after", "before"):
for field in ("preceded-by", "followed-by"):
Comment thread
pbean marked this conversation as resolved.
value = row.get(field, "").strip()
if not value:
continue
Expand Down
Loading