docs(130): post-delivery quality review (#150)

davidmatousek · claude · web-flow · commit 4c0fc306124f · 2026-04-11T12:44:05.000-04:00
* refactor(130): simplify code per /simplify review - Extract _build_error_record() helper to dedupe the stderr-decode dance across CalledProcessError and TimeoutExpired branches in _render_single (was ~12 lines of copy-paste). - Inline single-use aliases in _render_single; trim verbose module-state comment to one sentence. - Consolidate per-test boilerplate in test_mmdc_preflight.py via a new render_dirs fixture (saves ~30 lines of target_dir/template_dir duplication across 8 tests). - Delete test_preflight_skipped_when_only_low_medium_findings — it was byte-identical to test_preflight_skipped_when_attack_trees_empty. - Fix _make_error_record type annotation: stderr: "bytes | str". - Drop task/refinement/FR-ID references from module and section comments per KB-027 (they rot after the feature closes). Test suite: 47/48 pass (dropped 1 duplicate), backward-compat baselines unchanged. Net: -138 lines. Co-Authored-By: Claude <noreply@anthropic.com> * chore(130): regenerate BACKLOG.md after #130 closure Issue #130 transitioned from stage:deliver to stage:done during the delivery retrospective — remove it from the active-backlog table. Co-Authored-By: Claude <noreply@anthropic.com> * docs(130): update CHANGELOG with Feature 130 breaking change entry Add Unreleased entry describing the mmdc hard-prerequisite posture, defense-in-depth preflight gates, mid-render failure aggregator, backward-compatibility guarantees, and documentation sync. Mirrors the Feature 136 narrative style for correctness-fix breaking changes. Release-please will pick this up on the next release. Co-Authored-By: Claude <noreply@anthropic.com> * docs(130): review KB entries — update KB-029 test counts post-simplify The /aod.document simplify pass deleted a byte-identical duplicate preflight test. Reflect the new post-simplify test count (47/48, 8 distinct new cases instead of 9) in KB-029. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,47 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Breaking Changes — Correctness Fix (#148, Feature 130)
+
+**mmdc Is Now a Hard Prerequisite**: When `/tachi.security-report` is run against a project containing Critical/High attack trees, `@mermaid-js/mermaid-cli` (`mmdc`) must be installed on `PATH`. Previously, a missing `mmdc` triggered a silent text fallback that shipped 40+ lines of raw `flowchart TD` source per attack-path page inside the PDF; the pipeline reported exit 0 and the broken output was only discoverable by paging through the PDF manually. The text-fallback Typst branch has been deleted outright, and two defense-in-depth preflight gates now raise a loud error with the canonical install command.
+
+#### Install
+
+```sh
+npm install -g @mermaid-js/mermaid-cli
+```
+
+The check is gated on input detection — projects without `attack-trees/` content continue to work unchanged without `mmdc`. See [ADR-022](docs/architecture/02_ADRs/ADR-022-mmdc-hard-prerequisite.md) for the full governance rationale, rejected alternatives (pymmdc, Kroki, auto-install), and the Future Work clause.
+
+#### Error Output on Missing Prerequisite
+
+```
+Attack path rendering requires @mermaid-js/mermaid-cli (mmdc).
+Install with: npm install -g @mermaid-js/mermaid-cli
+Then re-run /tachi.security-report.
+```
+
+The same canonical message fires from both enforcement points: a shell-level `command -v mmdc` check in `.claude/commands/tachi.security-report.md` Step 1 (mirrors the existing Typst check) and a Python-level `shutil.which("mmdc") → raise RuntimeError(...)` inside `scripts/extract-report-data.py::render_mermaid_to_png()`.
+
+#### Mid-Render Failures Now Abort With Per-Finding Detail
+
+When `mmdc` is present but a specific attack tree fails to render (invalid Mermaid syntax, subprocess crash, timeout), the pipeline now aggregates per-finding errors and raises `RuntimeError("Attack path rendering failed for N findings: ...")` with each failing finding's ID, source path, failure class (`exit:<code>`, `timeout`, or `signal`), and a 200-byte stderr excerpt. Previously, each failing entry was silently marked `has_image=False` and the text fallback kicked in. No PDF is emitted when mid-render failures occur.
+
+#### Backward Compatibility
+
+The happy path (mmdc present, all trees render) is byte-identical to the pre-Feature 130 output under `SOURCE_DATE_EPOCH=1700000000`. The 5 byte-deterministic baselines (`web-app`, `microservices`, `ascii-web-api`, `mermaid-agentic-app`, `free-text-microservice`) remain unchanged. Projects without `attack-trees/` content are completely unaffected.
+
+#### Documentation
+
+- **README.md** gains a new `## Prerequisites` section naming `typst` and `@mermaid-js/mermaid-cli` with per-OS install commands (macOS/Linux/WSL).
+- **scripts/install.sh** gains a courtesy `command -v mmdc` warning at setup time.
+- **docs/architecture/00_Tech_Stack/README.md** mmdc section rewritten as a hard prerequisite with ADR-022 cross-reference.
+- **specs/112-attack-path-pages/spec.md** SC-004 inverted (text fallback is no longer a supported shipping mode) with audit-trail comment.
+- **specs/112-attack-path-pages/research.md** pymmdc description corrected (GPL-3.0 Node.js wrapper, not a pure-Python renderer) with a Durable Decision Rationale block.
+- **New CI workflow** `.github/workflows/tachi-mmdc-preflight.yml` asserts the loud-failure path fires on `ubuntu-latest` (no mmdc preinstalled) and fails the job if `mmdc` is unexpectedly present on `PATH`.
+
+---
+
 ### Breaking Changes — Correctness Fix (#136)
 
 **MAESTRO Canonical Layer Alignment**: tachi's MAESTRO seven-layer taxonomy has been aligned with the canonical CSA Ken Huang reference. Three L5/L6/L7 layer names, the acronym expansion, and a third-divergent name ("Integration Services") in the Typst PDF template have been corrected. This is a **correctness fix**, not a feature addition.
diff --git a/docs/INSTITUTIONAL_KNOWLEDGE.md b/docs/INSTITUTIONAL_KNOWLEDGE.md
@@ -607,7 +607,7 @@ Captured during structured delivery retrospective. Smooth sailing — everything
 
 **Solution**: For CLI prerequisites that are actually required at runtime (not optional external APIs in the ADR-014 sense), enforce at two entry points — shell-level at the command file (mirroring the Typst check that already existed) AND Python-level at the function boundary (`shutil.which(...) → raise RuntimeError(...)`). Gate the check on input detection so projects without the triggering input are unaffected (mmdc check fires only when `attack-trees/` contains Critical/High findings). Delete the fallback branch outright — no placeholder, no comment, no "removed in NNN" stub. Document the rule in a governing ADR (ADR-022 is the first tachi ADR covering CLI-prerequisite posture). Prove backward compatibility with a byte-deterministic baseline pair under `SOURCE_DATE_EPOCH` (ADR-021) before and after the refactor: happy path must be byte-identical.
 
-**Result**: 48/48 pytest green (9 new cases covering both preflight and mid-render aggregator). 5/5 byte-deterministic baselines remained byte-identical before/after the refactor — the happy path is provably unchanged. New fresh-install CI workflow on `ubuntu-latest` (no mmdc preinstalled) asserts the loud-failure path fires with all three canonical tokens in stderr, including a team-lead T4 enforcement assertion that fails the CI job if mmdc is unexpectedly present on PATH. The canonical install command `npm install -g @mermaid-js/mermaid-cli` appears in exactly 7 coordinated enforcement locations (extract-report-data.py raise, tachi.security-report.md shell echo, install.sh warning, README Prerequisites, test_mmdc_preflight.py assertion, tachi-mmdc-preflight.yml grep, ADR-022 decision body), verified by the T023 grep consistency check.
+**Result**: 47/48 pytest green after the post-delivery `/aod.document` simplify pass (delivered as 48/48 with 9 new preflight/aggregator cases; the simplify review removed one byte-identical duplicate to leave 8 distinct cases). 5/5 byte-deterministic baselines remained byte-identical before/after the refactor — the happy path is provably unchanged. New fresh-install CI workflow on `ubuntu-latest` (no mmdc preinstalled) asserts the loud-failure path fires with all three canonical tokens in stderr, including a team-lead T4 enforcement assertion that fails the CI job if mmdc is unexpectedly present on PATH. The canonical install command `npm install -g @mermaid-js/mermaid-cli` appears in exactly 7 coordinated enforcement locations (extract-report-data.py raise, tachi.security-report.md shell echo, install.sh warning, README Prerequisites, test_mmdc_preflight.py assertion, tachi-mmdc-preflight.yml grep, ADR-022 decision body), verified by the T023 grep consistency check.
 
 **When to Apply**: Any time the codebase adds a runtime CLI prerequisite (third-party binary, language runtime, renderer, compiler, tool that must be on PATH). The two-gate defense-in-depth pattern is now the tachi convention for CLI prerequisites, analogous to how Feature 054 Typst checks already work. If a third CLI prerequisite is ever added, per ADR-022 Future Work, extract an `install.sh` prerequisite helper — three data points is the minimum for meaningful abstraction. Do NOT preserve a fallback branch as a "courtesy" — if the fallback is worse than a loud error, delete it. A silent fallback that produces a broken-looking output is worse than no fallback at all.
 
diff --git a/docs/product/_backlog/BACKLOG.md b/docs/product/_backlog/BACKLOG.md
@@ -1,6 +1,6 @@
 # Backlog
 
-> Auto-generated from GitHub Issues on 2026-04-11T16:30:50Z.
+> Auto-generated from GitHub Issues on 2026-04-11T16:32:10Z.
 > Source of truth: GitHub Issues with `stage:*` labels.
 > Regenerate: `/aod.status` or `.aod/scripts/bash/backlog-regenerate.sh`
 
@@ -44,7 +44,7 @@
 
 | # | Title | Delivered | Retro | Updated |
 |---|-------|-----------|-------|---------|
-| #130 | Fix attack path Mermaid rendering when mmdc is not installed (spec 112 follow-up) | 2026-04-11 | — | 2026-04-11 |
+| — | *No items in this stage* | | |
 
 ## Untracked
 
diff --git a/scripts/extract-report-data.py b/scripts/extract-report-data.py
@@ -709,44 +709,40 @@ def _build_remediation(mitigation: str) -> list:
 # Mermaid Rendering
 # =============================================================================
 
-# Per-invocation render context. Set by ``render_mermaid_to_png`` before
-# spawning worker threads and read by ``_render_single`` at call time. Using
-# module-level state (rather than closed-over function parameters) keeps the
-# ``_render_single`` signature at two positional args ``(entry, tmp_path)``,
-# which matches the test harness in ``tests/scripts/test_mmdc_preflight.py``
-# where ``patch.object`` replaces ``_render_single`` with a 2-arg mock.
-# Thread-safe for in-process use because ``render_mermaid_to_png`` is called
-# sequentially by the CLI entry point and the fields are read-only for the
-# duration of each pool execution.
+# Per-invocation render context. Module-level (not closed-over) so
+# _render_single keeps a 2-arg signature that test fixtures can patch.
 _render_attack_trees_dir: "Path | None" = None
 _render_rel_target: "str | None" = None
 
 
+def _build_error_record(fid: str, failure_class: str, stderr_bytes) -> dict:
+    raw = stderr_bytes if stderr_bytes is not None else b""
+    if isinstance(raw, str):
+        excerpt = raw[:200]
+    else:
+        excerpt = raw[:200].decode("utf-8", errors="replace")
+    return {
+        "id": fid,
+        "file_path": f"attack-trees/{fid.lower()}.mmd",
+        "failure_class": failure_class,
+        "stderr_excerpt": excerpt,
+    }
+
+
 def _render_single(entry, tmp_path):
     """Render one Mermaid entry to PNG. Returns ``(entry, success, value)``.
 
     On success, ``value`` is the relative image path string for Typst.
-    On failure, ``value`` is a structured error-record dict with keys:
-    - ``id``: finding ID
-    - ``file_path``: canonical ``attack-trees/{fid_lower}.mmd`` source path
-    - ``failure_class``: ``"exit:<code>"``, ``"timeout"``, or ``"signal"``
-    - ``stderr_excerpt``: first 200 bytes of stderr, utf-8 decoded with
-      ``errors="replace"`` (empty string if stderr is None/empty)
-
-    Reads ``_render_attack_trees_dir`` and ``_render_rel_target`` from module
-    scope; the caller ``render_mermaid_to_png`` sets these before spawning
-    pool workers. Promoted from a nested closure to module-level so tests
-    can ``patch.object(extract_report_data, "_render_single", ...)``.
+    On failure, ``value`` is a structured error-record dict with keys
+    ``id``, ``file_path``, ``failure_class`` (``"exit:<code>"``, ``"timeout"``,
+    or ``"signal"``), and ``stderr_excerpt`` (first 200 bytes).
     """
     fid = entry["id"]
     fid_lower = fid.lower()
     mermaid_code = entry.get("mermaid_code", "")
     if not mermaid_code:
         return (entry, False, "")
 
-    attack_trees_dir = _render_attack_trees_dir
-    rel_target = _render_rel_target
-
     mmd_file = tmp_path / f"{fid_lower}.mmd"
     png_file = tmp_path / f"{fid_lower}.png"
     mmd_file.write_text(mermaid_code, encoding="utf-8")
@@ -758,39 +754,14 @@ def _render_single(entry, tmp_path):
             timeout=30,
             check=True,
         )
-        dest_png = attack_trees_dir / f"{fid_lower}-attack-tree.png"
+        dest_png = _render_attack_trees_dir / f"{fid_lower}-attack-tree.png"
         shutil.copy2(str(png_file), str(dest_png))
-        return (entry, True, rel_target + "/attack-trees/" + f"{fid_lower}-attack-tree.png")
+        return (entry, True, _render_rel_target + "/attack-trees/" + f"{fid_lower}-attack-tree.png")
     except subprocess.CalledProcessError as e:
-        if e.returncode < 0:
-            failure_class = "signal"
-        else:
-            failure_class = f"exit:{e.returncode}"
-        stderr_bytes = e.stderr if e.stderr is not None else b""
-        if isinstance(stderr_bytes, str):
-            stderr_excerpt = stderr_bytes[:200]
-        else:
-            stderr_excerpt = stderr_bytes[:200].decode("utf-8", errors="replace")
-        error_record = {
-            "id": fid,
-            "file_path": f"attack-trees/{fid_lower}.mmd",
-            "failure_class": failure_class,
-            "stderr_excerpt": stderr_excerpt,
-        }
-        return (entry, False, error_record)
+        failure_class = "signal" if e.returncode < 0 else f"exit:{e.returncode}"
+        return (entry, False, _build_error_record(fid, failure_class, e.stderr))
     except subprocess.TimeoutExpired as e:
-        stderr_bytes = e.stderr if e.stderr is not None else b""
-        if isinstance(stderr_bytes, str):
-            stderr_excerpt = stderr_bytes[:200]
-        else:
-            stderr_excerpt = stderr_bytes[:200].decode("utf-8", errors="replace")
-        error_record = {
-            "id": fid,
-            "file_path": f"attack-trees/{fid_lower}.mmd",
-            "failure_class": "timeout",
-            "stderr_excerpt": stderr_excerpt,
-        }
-        return (entry, False, error_record)
+        return (entry, False, _build_error_record(fid, "timeout", e.stderr))
 
 
 def render_mermaid_to_png(attack_trees: list, target_dir: Path, template_dir: Path):
@@ -825,7 +796,6 @@ def render_mermaid_to_png(attack_trees: list, target_dir: Path, template_dir: Pa
     attack_trees_dir = target_dir / "attack-trees"
     attack_trees_dir.mkdir(exist_ok=True)
 
-    # Publish render context to module scope for _render_single workers.
     global _render_attack_trees_dir, _render_rel_target
     _render_attack_trees_dir = attack_trees_dir
     _render_rel_target = rel_target
diff --git a/tests/scripts/test_mmdc_preflight.py b/tests/scripts/test_mmdc_preflight.py
