From 00e9d1db11e1fd70a7b1799e7c2fd6f46da872be Mon Sep 17 00:00:00 2001 From: pbean Date: Sun, 24 May 2026 16:05:23 -0700 Subject: [PATCH 1/4] fix(validate-module): align CSV header with preceded-by/followed-by columns The module-help.csv `after`/`before` columns were renamed to `preceded-by`/`followed-by` (#89), and the templates, root catalog, and bmad-help all use the new names. But validate-module.py still hard-coded the old `after`/`before` names in `CSV_HEADER` and in the reference-validation loop, so it flagged a spurious "CSV header mismatch" on every current-format module and never checked the actual preceded-by/followed-by reference columns. - Update `CSV_HEADER` to use `preceded-by`/`followed-by`. - Update the reference-validation loop to read those columns. - Refresh the descriptive comments/docstring. - Add regression tests: canonical header is accepted; a legacy after/before header is flagged as a mismatch. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../scripts/tests/test-validate-module.py | 48 ++++++++++++++++++- .../scripts/validate-module.py | 8 ++-- 2 files changed, 51 insertions(+), 5 deletions(-) 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 ac7e8e4..ea3e4c7 100644 --- a/skills/bmad-module-builder/scripts/tests/test-validate-module.py +++ b/skills/bmad-module-builder/scripts/tests/test-validate-module.py @@ -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 = "", @@ -162,6 +163,49 @@ 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) + 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) + 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 create_standalone_module(tmp: Path, skill_name: str = "my-skill", csv_rows: str = "", yaml_content: str = "", include_setup_md: bool = True, @@ -290,6 +334,8 @@ 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_valid_standalone_module, test_standalone_missing_module_setup_md, test_standalone_missing_merge_scripts, diff --git a/skills/bmad-module-builder/scripts/validate-module.py b/skills/bmad-module-builder/scripts/validate-module.py index ad0bbed..c4e488d 100644 --- a/skills/bmad-module-builder/scripts/validate-module.py +++ b/skills/bmad-module-builder/scripts/validate-module.py @@ -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 """ @@ -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", ] @@ -220,7 +220,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: @@ -231,7 +231,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"): value = row.get(field, "").strip() if not value: continue From 4e1a28531920348e18f7ed4c608ca49520e306cf Mon Sep 17 00:00:00 2001 From: pbean Date: Sun, 24 May 2026 17:38:54 -0700 Subject: [PATCH 2/4] docs(module-configuration): align help-CSV columns with preceded-by/followed-by The module-help.csv `after`/`before` columns were renamed to `preceded-by`/`followed-by` (#89) and validate-module.py was aligned in the previous commit, but docs/explanation/module-configuration.md still documented the old column names. - Update both CSV header code blocks (schema + example) to the canonical 13-column header. - Rename the Column Guide rows and reword them so the new names are self-documenting (preceded-by = "preceded by them", followed-by = "followed by them"), preserving the original semantics. - Update the dependency-graph prose to reference preceded-by/followed-by. - Fix the example's followed-by reference to a real action (bmad-agent-builder:quality-analysis; quality-optimizer does not exist). Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/explanation/module-configuration.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/explanation/module-configuration.md b/docs/explanation/module-configuration.md index 2e98a23..94b83f5 100644 --- a/docs/explanation/module-configuration.md +++ b/docs/explanation/module-configuration.md @@ -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 @@ -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). From 5090715fe83ff12a081224d969404ae2578d8c18 Mon Sep 17 00:00:00 2001 From: pbean Date: Sun, 24 May 2026 17:49:33 -0700 Subject: [PATCH 3/4] fix(validate-module): handle short CSV rows without crashing; harden tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses automated review feedback on #92. A module-help.csv row with fewer fields than the header made csv.DictReader fill the missing columns with None (restval defaults to None), so the subsequent `row.get(field, "").strip()` raised AttributeError and aborted validation with an uncaught traceback instead of reporting findings — exactly the malformed input a validator should report on. - parse_csv_rows now uses `restval=""`, so missing fields are empty strings and every `.strip()` call stays safe. - DictReader pads short rows to the header width, hiding the shortfall from the column-count check (`len(row)` was always 13). parse_csv_rows now also returns the raw per-row field counts from csv.reader, and check #5 uses those, so both short and long rows are flagged as column-count mismatches. - Strengthen the canonical/legacy header regression tests to assert exit code/status, so they can't pass silently if the validator fails for an unrelated reason. - Add a regression test for the short-row case (no crash + column-count finding). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../scripts/tests/test-validate-module.py | 25 ++++++++++++++++ .../scripts/validate-module.py | 29 +++++++++++++------ 2 files changed, 45 insertions(+), 9 deletions(-) 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 ea3e4c7..2aec881 100644 --- a/skills/bmad-module-builder/scripts/tests/test-validate-module.py +++ b/skills/bmad-module-builder/scripts/tests/test-validate-module.py @@ -171,6 +171,8 @@ def test_canonical_header_accepted(): module_dir = create_module(tmp, skills=["tst-foo"], csv_rows=csv_rows) code, data = run_validate(module_dir) + 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}" @@ -198,6 +200,8 @@ def test_legacy_after_before_header_flagged(): (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"] @@ -206,6 +210,26 @@ def test_legacy_after_before_header_flagged(): 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) + # 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}" + 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, @@ -336,6 +360,7 @@ def test_nonexistent_directory(): 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, diff --git a/skills/bmad-module-builder/scripts/validate-module.py b/skills/bmad-module-builder/scripts/validate-module.py index c4e488d..14327aa 100644 --- a/skills/bmad-module-builder/scripts/validate-module.py +++ b/skills/bmad-module-builder/scripts/validate-module.py @@ -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: @@ -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: @@ -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 From 8ca66dee837b2c300428bf50891fe151c8575405 Mon Sep 17 00:00:00 2001 From: pbean Date: Sun, 24 May 2026 17:56:05 -0700 Subject: [PATCH 4/4] test(validate-module): assert exit code in short-row regression test The short-row regression test unpacked `code` but never used it. Rather than silence it (the repo's ruff.toml selects only B/D3/E/F, so RUF059 is not enabled, and F841 exempts unpacking targets), make the binding meaningful: assert that a short row yields exit 0 / status "pass", documenting that a column-count mismatch is medium-severity and therefore non-fatal. This also brings the test in line with the other two header regression tests, which already assert exit code/status. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../bmad-module-builder/scripts/tests/test-validate-module.py | 2 ++ 1 file changed, 2 insertions(+) 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 2aec881..f59cc38 100644 --- a/skills/bmad-module-builder/scripts/tests/test-validate-module.py +++ b/skills/bmad-module-builder/scripts/tests/test-validate-module.py @@ -225,6 +225,8 @@ def test_short_row_does_not_crash(): # 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"]