From f675bdaf22561c45f792b50c362bfd37a9febd98 Mon Sep 17 00:00:00 2001 From: Brian Madison Date: Sun, 14 Jun 2026 00:05:34 -0500 Subject: [PATCH] bmad-architecture: address PR review (augment + coderabbit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - REF-03: use canonical "invoke the `skill` skill" language for all cross-skill references in SKILL.md (was "route to / offer / hand to / Next:") - lint_spine: scan frontmatter for unfilled template tokens / TBD (paradigm, scope, date were uncatchable in the body-only pass) - lint_spine: guard read_text() so a bad-encoding/unreadable spine returns error JSON and exit 0, honoring the documented contract - reviewer-gate: resolve the "always run" vs "may skip" ambiguity — the gate scales/skips by stakes, but finalize_reviewers always run once it does - tests: +3 (frontmatter token, frontmatter TBD, unreadable spine); use next() --- .../3-solutioning/bmad-architecture/SKILL.md | 10 +++---- .../references/reviewer-gate.md | 2 +- .../bmad-architecture/scripts/lint_spine.py | 27 ++++++++++++++++++- .../scripts/tests/test_lint_spine.py | 25 ++++++++++++++++- 4 files changed, 56 insertions(+), 8 deletions(-) diff --git a/src/bmm-skills/3-solutioning/bmad-architecture/SKILL.md b/src/bmm-skills/3-solutioning/bmad-architecture/SKILL.md index 70843baee..fd52bb8e5 100644 --- a/src/bmm-skills/3-solutioning/bmad-architecture/SKILL.md +++ b/src/bmm-skills/3-solutioning/bmad-architecture/SKILL.md @@ -22,13 +22,13 @@ You're a coach, and the **Coaching path is the default** — this runs against t Elicit, don't quiz: open-ended "how are you thinking about X?" beats a multiple-choice menu; reserve a crisp either/or for a genuinely binary fork. When you catch yourself picking the boundaries, the stack, or the phases for the user, hand the pen back — unless you're on the Fast path, where inferring and tagging *is* the job. -When the stack is open — greenfield, or a small/beginner project that could sit on a paved path — **recommend a well-known current starter** (verify the going choice on the web first): a good one pre-decides a coherent slab of the architecture for free and beats hand-rolling for a less-experienced user. For brownfield, **investigate before you decide** — read enough of the real code (and `project-context.md`; offer `bmad-document-project` if there is none) to ratify the conventions already there rather than invent new ones. +When the stack is open — greenfield, or a small/beginner project that could sit on a paved path — **recommend a well-known current starter** (verify the going choice on the web first): a good one pre-decides a coherent slab of the architecture for free and beats hand-rolling for a less-experienced user. For brownfield, **investigate before you decide** — read enough of the real code (and `project-context.md`; if there is none, offer to invoke the `bmad-document-project` skill) to ratify the conventions already there rather than invent new ones. ## Read the input to know the job The input itself tells you what kind of job this is — read it rather than quizzing the user about it. A spec package (`SPEC.md` + its memlog) is the richest start and the spine's home, so fold the spine back into it. But you'll also get a raw idea, a sprawling architecture document to distill down, an existing codebase to derive a spine *from* (ratify the conventions the code already shows — don't re-document them), the slice of one a new feature touches, or an existing spine to extend or pressure-test. Prefer a `.memlog.md` over re-reading the source it came from. Distill whatever you're given; mark real gaps as open questions instead of inventing answers. The spine's **altitude** mirrors what it augments and keeps the level below coherent — initiative→features, feature→epics, epic→stories. -**Inheriting a parent spine** (e.g. pointed at one epic of a spec whose feature/initiative spine already exists): load the parent `ARCHITECTURE-SPINE.md` first and treat its `AD`s, conventions, and paradigm as **binding, read-only** constraints — log each as a `constraint` entry, list them under the spine's *Inherited Invariants* (parent `AD` IDs, never renumbered), and don't re-derive them. Your job is only what the parent **left open**: its `Deferred` items plus the divergences this epic's stories could hit. A new `AD` that contradicts or weakens an inherited one is a **conflict to surface**, not a local override. An epic spine fixes the invariants the epic's stories must share — it does **not** expand per-story detail; that's deferred to `bmad-create-story` at story time. +**Inheriting a parent spine** (e.g. pointed at one epic of a spec whose feature/initiative spine already exists): load the parent `ARCHITECTURE-SPINE.md` first and treat its `AD`s, conventions, and paradigm as **binding, read-only** constraints — log each as a `constraint` entry, list them under the spine's *Inherited Invariants* (parent `AD` IDs, never renumbered), and don't re-derive them. Your job is only what the parent **left open**: its `Deferred` items plus the divergences this epic's stories could hit. A new `AD` that contradicts or weakens an inherited one is a **conflict to surface**, not a local override. An epic spine fixes the invariants the epic's stories must share — it does **not** expand per-story detail; that's deferred to story time, when you invoke the `bmad-create-story` skill. ## How a run works @@ -53,7 +53,7 @@ Writes go through the shared script (don't read the file back except on resume): 1. Resolve customization: `python3 {project-root}/_bmad/scripts/resolve_customization.py --skill {skill-root} --key workflow` (on failure read `{skill-root}/customize.toml`, use defaults). Run `{workflow.activation_steps_prepend}`, then `{workflow.activation_steps_append}`. Hold `{workflow.persistent_facts}` as standing context — the default loads `project-context.md`, load-bearing for brownfield — and consult `{workflow.external_sources}` on demand. 2. Load `{project-root}/_bmad/bmm/config.yaml` (+ `config.user.yaml`) for `{user_name}`, `{communication_language}`, `{document_output_language}`, `{planning_artifacts}`, `{project_name}`, `{date}`; missing keys take neutral defaults, never block. -3. Headless (no interactive user) → follow `references/headless.md` for the whole run. Otherwise greet `{user_name}` in `{communication_language}`. Detect the intent from the conversation and input — **create** (the default), **update** an existing spine, or **validate** one (see those sections). If the real ask is requirements / UX / a capability contract / epic breakdown / an agent, route to `bmad-prd` / `bmad-ux` / `bmad-spec` / `bmad-create-epics-and-stories` / `bmad-workflow-builder` (if the BMad Builder module is installed) instead. +3. Headless (no interactive user) → follow `references/headless.md` for the whole run. Otherwise greet `{user_name}` in `{communication_language}`. Detect the intent from the conversation and input — **create** (the default), **update** an existing spine, or **validate** one (see those sections). If the real ask is requirements / UX / a capability contract / epic breakdown / an agent, invoke the `bmad-prd`, `bmad-ux`, `bmad-spec`, `bmad-create-epics-and-stories`, or `bmad-workflow-builder` (if the BMad Builder module is installed) skill instead. 4. If a run folder for this target already exists under `{workflow.spine_output_path}`, offer to resume from its memlog rather than restart. 5. Interactive create: offer the working mode in `{communication_language}` — **Coaching path** (default) or **Fast path** (see *How you work*) — before any drafting; default to Coaching unless the user asks for speed. @@ -72,8 +72,8 @@ Walk the sequence; reviewer fixes land before polish. 3. **Reviewer pass.** Run the Reviewer Gate (`references/reviewer-gate.md`). Resolve before polish. 4. **Triage.** Open questions and `[ASSUMPTION]` tags: blockers (unsafe for what's next) resolved one at a time; the rest deferred with a revisit condition in the memlog. 5. **Renderings & polish.** The terse spine is the deliverable when the consumer is a downstream build agent. If the user needs a human-facing artifact instead, offer a fuller rendering (html or md solution design, deck, C4 set) and apply `{workflow.doc_standards}` polish only to that prose, never to the spine. -6. **External handoffs.** Run `{workflow.external_handoffs}`; surface returned URLs/IDs. Offer to hand the spine to `bmad-spec` as a companion, keeping `AD` IDs stable so downstream can cite them. -7. **Close.** Set the spine's own frontmatter `status: final`, `updated: {date}`; log a `memlog.py append --type event --text "spine finalized"` (the memlog has no status field). Share paths. Next: `bmad-spec`, `bmad-create-epics-and-stories`, or — epic altitude — `bmad-create-story`; `bmad-help` to route. +6. **External handoffs.** Run `{workflow.external_handoffs}`; surface returned URLs/IDs. Offer to invoke the `bmad-spec` skill to adopt the spine as a companion, keeping `AD` IDs stable so downstream can cite them. +7. **Close.** Set the spine's own frontmatter `status: final`, `updated: {date}`; log a `memlog.py append --type event --text "spine finalized"` (the memlog has no status field). Share paths. Next: invoke the `bmad-spec`, `bmad-create-epics-and-stories`, or — epic altitude — `bmad-create-story` skill; or invoke the `bmad-help` skill to route. 8. Run `{workflow.on_complete}`. ## Update diff --git a/src/bmm-skills/3-solutioning/bmad-architecture/references/reviewer-gate.md b/src/bmm-skills/3-solutioning/bmad-architecture/references/reviewer-gate.md index 67b612c81..729844c46 100644 --- a/src/bmm-skills/3-solutioning/bmad-architecture/references/reviewer-gate.md +++ b/src/bmm-skills/3-solutioning/bmad-architecture/references/reviewer-gate.md @@ -4,7 +4,7 @@ The spine's pre-handoff review. Runs at Finalize (after distill + reconcile) and Cheap deterministic pass first: `python3 {skill-root}/scripts/lint_spine.py --workspace {doc_workspace}` settles the mechanical misses (placeholders, duplicate `AD` IDs, missing Binds/Prevents/Rule, unpinned deps), so reviewers spend judgment on the semantic half. -Assemble the menu: a **rubric walker** that judges the spine against the good-spine checklist below, **+ every entry in `{workflow.finalize_reviewers}` (always run)**, + ad-hoc lenses you invent or offer as the spine's rigor, altitude, and criticality warrant — a security/compliance lens for regulated stakes, a seam reviewer cross-team, a data-integrity lens for a heavy data model. Scale the set to the stakes: a throwaway prototype may run it quietly or skip; a high-criticality or platform-altitude spine earns more lenses and the explicit all / subset / skip menu. +Assemble the menu: a **rubric walker** that judges the spine against the good-spine checklist below, **+ every entry in `{workflow.finalize_reviewers}`**, + ad-hoc lenses you invent or offer as the spine's rigor, altitude, and criticality warrant — a security/compliance lens for regulated stakes, a seam reviewer cross-team, a data-integrity lens for a heavy data model. Scale *whether and how heavily the gate runs* to the stakes: a throwaway prototype may run it quietly or skip the gate entirely; a high-criticality or platform-altitude spine earns more lenses and the explicit all / subset / skip menu. But once the gate runs, the `{workflow.finalize_reviewers}` always run — they are the configured floor, never cherry-picked out; only the ad-hoc lenses are optional. (Headless never skips the gate.) Dispatch every entry as a **parallel subagent against `ARCHITECTURE-SPINE.md`** (prefix convention: `skill:` / `file:` / plain text). Each writes its full review to `{doc_workspace}/review-{slug}.md` and returns ONLY a compact summary (verdict, top 2–5 findings, file path) — the parent never holds full review text. An inline self-check does not count: the independent context is the point, because a fresh reviewer finds the divergences the author talks past. If subagents are unavailable, run sequentially — write the file first, then flush it from context. diff --git a/src/bmm-skills/3-solutioning/bmad-architecture/scripts/lint_spine.py b/src/bmm-skills/3-solutioning/bmad-architecture/scripts/lint_spine.py index ec034f22c..88fa3e06c 100644 --- a/src/bmm-skills/3-solutioning/bmad-architecture/scripts/lint_spine.py +++ b/src/bmm-skills/3-solutioning/bmad-architecture/scripts/lint_spine.py @@ -88,6 +88,24 @@ def find_placeholders(body: str, offset: int) -> list[dict]: return findings +def find_frontmatter_placeholders(frontmatter: str) -> list[dict]: + """Catch unfilled tokens left in frontmatter (e.g. paradigm/scope/date) — part of the + spine contract, but outside the body that find_placeholders scans.""" + findings: list[dict] = [] + for rx, label, severity in ( + (PLACEHOLDER_WORD, "placeholder marker", "high"), + (TEMPLATE_TOKEN, "possible unfilled template token (verify)", "low"), + ): + for m in rx.finditer(frontmatter): + findings.append({ + "category": "placeholder", + "severity": severity, + "detail": f"frontmatter {label}: {m.group(0)!r}", + "location": f"{SPINE} frontmatter (line {1 + line_of(frontmatter, m.start())})", + }) + return findings + + def find_ad_issues(body: str, offset: int) -> list[dict]: findings: list[dict] = [] scan = blank_fences(body) # AD headings shown inside a code fence are not live ADs @@ -196,6 +214,7 @@ def _check_dep(item: str, findings: list[dict]) -> None: def lint(text: str) -> dict: frontmatter, body, offset = split_frontmatter(text) findings: list[dict] = [] + findings += find_frontmatter_placeholders(frontmatter) findings += find_placeholders(body, offset) findings += find_ad_issues(body, offset) findings += find_unpinned_deps(frontmatter) @@ -221,7 +240,13 @@ def main(argv: list[str] | None = None) -> int: if not spine_path.exists(): result = {"ok": False, "error": f"{spine_path} not found", "findings": [], "total_findings": 0} else: - result = lint(spine_path.read_text(encoding="utf-8")) + try: + text = spine_path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as e: + # honor the "exit code is always 0" contract: a read/decode failure travels in JSON + result = {"ok": False, "error": f"could not read {spine_path}: {e}", "findings": [], "total_findings": 0} + else: + result = lint(text) out = json.dumps(result, indent=2) if args.output: diff --git a/src/bmm-skills/3-solutioning/bmad-architecture/scripts/tests/test_lint_spine.py b/src/bmm-skills/3-solutioning/bmad-architecture/scripts/tests/test_lint_spine.py index 82fd28a85..220bb42e9 100644 --- a/src/bmm-skills/3-solutioning/bmad-architecture/scripts/tests/test_lint_spine.py +++ b/src/bmm-skills/3-solutioning/bmad-architecture/scripts/tests/test_lint_spine.py @@ -190,7 +190,7 @@ def test_placeholder_line_number_is_absolute(): "TBD here\n" ) result = lint_spine.lint(text) - ph = [f for f in result["findings"] if "TBD" in f["detail"]][0] + ph = next(f for f in result["findings"] if "TBD" in f["detail"]) n = int(re.search(r"line (\d+)", ph["location"]).group(1)) assert n == 13 @@ -201,5 +201,28 @@ def test_missing_spine_file_reports_error(tmp_path, capsys): assert rc == 0 and out["ok"] is False and "not found" in out["error"] +def test_frontmatter_unfilled_token_caught(): + # an unfilled {scope}/{paradigm}/{date} in frontmatter is part of the contract and must lint + text = "---\nname: 'x'\nscope: '{what this spine governs}'\n---\n\n## Invariants\n" + result = lint_spine.lint(text) + fm = [f for f in result["findings"] if f["category"] == "placeholder" and "frontmatter" in f["detail"]] + assert fm and any("template token" in f["detail"] for f in fm) + + +def test_frontmatter_tbd_caught(): + text = "---\nname: 'x'\nstatus: TBD\n---\n\n## Invariants\n" + result = lint_spine.lint(text) + assert any(f["category"] == "placeholder" and "frontmatter" in f["detail"] and "TBD" in f["detail"] + for f in result["findings"]) + + +def test_unreadable_spine_returns_error_not_crash(tmp_path, capsys): + # a spine that exists but can't be UTF-8 decoded must yield error JSON + exit 0, not a traceback + (tmp_path / lint_spine.SPINE).write_bytes(b"\xff\xfe bad bytes not utf-8") + rc = lint_spine.main(["--workspace", str(tmp_path)]) + out = json.loads(capsys.readouterr().out) + assert rc == 0 and out["ok"] is False and "could not read" in out["error"] + + if __name__ == "__main__": sys.exit(pytest.main([__file__, "-q"]))