diff --git a/src/designdoc/stages/s5_mermaid.py b/src/designdoc/stages/s5_mermaid.py index 8478901..b84e94d 100644 --- a/src/designdoc/stages/s5_mermaid.py +++ b/src/designdoc/stages/s5_mermaid.py @@ -167,10 +167,19 @@ async def _emit_package_diagrams(state: PipelineState, packages_dir) -> None: validation = validate(merged) if not validation.ok: - # Fail soft: leave the README diagram-less rather than crash the - # stage. Stage 5 is best-effort by design (per-class diagrams - # already use the same HIL-or-ship pattern). - continue + # One retry with arrow labels stripped — mmdc's relationship-label + # grammar rejects `;`, sometimes chokes on `()`, and has fragile + # quoting rules. Stripping every label to a bare arrow is the + # universal escape hatch: less informative than a labelled + # diagram, but still a useful bird's-eye view. + stripped = _strip_arrow_labels(merged) + if stripped != merged and validate(stripped).ok: + merged = stripped + else: + # Fail soft: leave the README diagram-less rather than crash + # the stage. Stage 5 is best-effort by design (per-class + # diagrams already use the same HIL-or-ship pattern). + continue body = _strip_diagram_section(pkg_text) section = f"\n\n## Diagram\n\n```mermaid\n{merged}\n```\n" @@ -227,6 +236,22 @@ def _merge_class_diagrams(blocks: list[str]) -> str: return "\n".join(lines) +def _strip_arrow_labels(diagram_text: str) -> str: + """Drop the `: label` suffix from every relationship-arrow line in a + mermaid classDiagram. Used as a one-shot retry escape hatch when the + full merged diagram fails mmdc validation due to label-grammar quirks + (see issue #59 for the agent-brain config-package failure case). + """ + out: list[str] = [] + for line in diagram_text.splitlines(): + if any(op in line for op in _ARROW_OPS) and not line.lstrip().startswith("class "): + head, sep, _label = line.partition(":") + out.append(head.rstrip() if sep else line) + else: + out.append(line) + return "\n".join(out) + + def _strip_diagram_section(text: str) -> str: """Return the class doc text with any trailing '## Diagram' section removed.""" return _DIAGRAM_SECTION_RE.sub("", text).rstrip() + "\n" diff --git a/tests/unit/test_stage5_package_diagrams.py b/tests/unit/test_stage5_package_diagrams.py index c34ae34..3b3d8ae 100644 --- a/tests/unit/test_stage5_package_diagrams.py +++ b/tests/unit/test_stage5_package_diagrams.py @@ -12,7 +12,12 @@ from __future__ import annotations -from designdoc.stages.s5_mermaid import _merge_class_diagrams +from pathlib import Path + +import pytest + +from designdoc.stages import s5_mermaid +from designdoc.stages.s5_mermaid import _merge_class_diagrams, _strip_arrow_labels def test_merge_extracts_class_names_from_inputs(): @@ -106,3 +111,165 @@ def test_merge_ignores_non_classdiagram_blocks(): assert "class Gateway" in merged assert "flowchart" not in merged assert "A --> B" not in merged + + +# ----- label-stripping retry (issue #59) ----------------------------------- + + +def test_strip_arrow_labels_removes_text_after_colon_on_arrow_lines(): + """The agent-brain dogfood found `Settings ..> ValidationError : calls + get_api_key(); raises ValidationError` — `;` in the label broke mmdc. + Stripping everything after the `:` on arrow lines is the universal + escape hatch.""" + text = ( + "classDiagram\n" + " class Settings\n" + " class ValidationError\n" + " Settings ..> ValidationError : calls get_api_key(); raises ValidationError\n" + ) + + stripped = _strip_arrow_labels(text) + + assert "Settings ..> ValidationError" in stripped + arrow_line = next( + line for line in stripped.splitlines() if "Settings ..> ValidationError" in line + ) + assert ":" not in arrow_line, f"label not stripped from: {arrow_line!r}" + assert "calls get_api_key" not in stripped + + +def test_strip_arrow_labels_leaves_unlabelled_arrows_alone(): + """An arrow line with no `:` should be returned untouched — leading + indentation (which mermaid renders) preserved, no dropped lines.""" + text = "classDiagram\n class A\n class B\n A --> B\n" + + stripped = _strip_arrow_labels(text) + + assert "A --> B" in stripped + # Leading whitespace stays put — only the trailing `: label` is removed. + arrow_line = next(line for line in stripped.splitlines() if "A --> B" in line) + assert arrow_line == " A --> B" + + +def test_strip_arrow_labels_preserves_non_arrow_lines(): + """`class Foo` declarations contain no arrow ops, so their bodies (and + any `:` inside) must survive intact.""" + text = ( + "classDiagram\n" + " class Foo {\n" + " +bar: str\n" + " }\n" + " class Bar\n" + " Foo --> Bar : uses\n" + ) + + stripped = _strip_arrow_labels(text) + + # class-body `:` is preserved + assert "+bar: str" in stripped + # arrow-line label is stripped + assert "Foo --> Bar" in stripped + assert ": uses" not in stripped + + +@pytest.mark.anyio +async def test_emit_package_diagrams_retries_with_stripped_labels(tmp_path: Path, monkeypatch): + """When the first merged diagram fails mmdc validation, _emit_package_diagrams + should retry once with arrow labels stripped, and write the stripped + diagram if that retry passes. Without this, packages with one parse-unfriendly + label ship diagram-less even though the merge logic produced something useful.""" + from designdoc.mermaid.mmdc import ValidationResult + from designdoc.state import PipelineState + + output = tmp_path / "design" + pkg_dir = output / "packages" / "config" + classes_dir = pkg_dir / "classes" + classes_dir.mkdir(parents=True) + + # Two class docs whose mermaid blocks merge into a diagram that contains + # a label. Whether the label is real-world-toxic doesn't matter — we + # mock validate() below to simulate mmdc rejecting it. + (classes_dir / "Settings.md").write_text( + "# Settings\n\n## Diagram\n\n" + "```mermaid\n" + "classDiagram\n" + " class Settings\n" + " class ValidationError\n" + " Settings ..> ValidationError : calls get_api_key(); raises\n" + "```\n" + ) + (classes_dir / "Loader.md").write_text( + "# Loader\n\n## Diagram\n\n" + "```mermaid\n" + "classDiagram\n" + " class Loader\n" + " class Settings\n" + " Loader --> Settings : builds\n" + "```\n" + ) + (pkg_dir / "README.md").write_text("# config\n\n## Overview\nConfig handling.\n") + + # Capture every input handed to validate so we can assert the second + # call received label-stripped text. First call → fail, second → pass. + calls: list[str] = [] + + def fake_validate(text: str, *, timeout: float = 30.0): + calls.append(text) + if len(calls) == 1: + return ValidationResult(ok=False, stderr="Parse error (simulated)") + return ValidationResult(ok=True, stderr="") + + monkeypatch.setattr(s5_mermaid, "validate", fake_validate) + + state = PipelineState.load_or_new(output_dir=output, target_repo=tmp_path) + + await s5_mermaid._emit_package_diagrams(state, output / "packages") + + readme_text = (pkg_dir / "README.md").read_text() + assert "## Diagram" in readme_text, "label-strip retry must succeed and write a Diagram section" + assert "```mermaid" in readme_text + # First call had labels (": calls get_api_key" or ": builds"); the + # second call (which we approved) had them stripped. + assert len(calls) == 2, "expected one retry after the first failure" + assert ":" in calls[0].split("Settings ..> ValidationError", 1)[1].splitlines()[0] + # The written README contains the stripped version, not the labelled one. + assert "calls get_api_key" not in readme_text + assert "builds" not in readme_text.split("## Diagram", 1)[1] + + +@pytest.mark.anyio +async def test_emit_package_diagrams_stays_failsoft_when_retry_also_fails( + tmp_path: Path, monkeypatch +): + """If both the original merge AND the label-stripped retry fail mmdc, + behaviour should fall back to the v1 fail-soft path: leave the README + diagram-less rather than crash the stage.""" + from designdoc.mermaid.mmdc import ValidationResult + from designdoc.state import PipelineState + + output = tmp_path / "design" + pkg_dir = output / "packages" / "config" + classes_dir = pkg_dir / "classes" + classes_dir.mkdir(parents=True) + (classes_dir / "A.md").write_text( + "# A\n\n## Diagram\n\n" + "```mermaid\n" + "classDiagram\n class A\n class B\n A --> B : x\n" + "```\n" + ) + pre_readme = "# config\n\n## Overview\nx\n" + (pkg_dir / "README.md").write_text(pre_readme) + + monkeypatch.setattr( + s5_mermaid, + "validate", + lambda text, *, timeout=30.0: ValidationResult(ok=False, stderr="fail"), + ) + + state = PipelineState.load_or_new(output_dir=output, target_repo=tmp_path) + + await s5_mermaid._emit_package_diagrams(state, output / "packages") + + assert (pkg_dir / "README.md").read_text() == pre_readme, ( + "README must be untouched when both validate attempts fail" + )