From f2aaae053055656d1b85bac0419bd9cb05a36a99 Mon Sep 17 00:00:00 2001 From: Lukas Friedrich Date: Mon, 31 Aug 2026 21:33:14 +0200 Subject: [PATCH 1/2] Wire the PDF output, and stop ignoring ReportConfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #21. `ReportConfig.formats` was typed to accept only json and summary, the CLI never imported either PDF renderer, and `run --help` said nothing about PDF. So 354 lines of renderer — 282 statements at 91% coverage, 72 at 90% — could not be produced by any invocation of the shipped tool, while the README advertised `pip install "markproof[pdf]"` as though installing the extra got you one. The PDF is the artefact you hand to an auditor: the one output whose entire purpose is to be read by someone who will never run the tool. Having it exist only in the source tree was the gap least likely to be noticed and most likely to matter. `formats` now accepts `pdf` and `pdf-html`, and the renderer is imported at the point of use rather than at module scope — the default output path must never depend on a package the base install does not ship. When a renderer is unavailable, its own message is passed through: WeasyPrint's covers the case that actually bites people, where the wheel is installed but ctypes cannot find Pango or cairo, and nothing reconstructed here would say it better. Two neighbouring things the audit found, fixed with it: `report.sign_key` was validated and then ignored, so an operator who wrote `sign_key: env:CI_SIGNING_KEY` got an unsigned report and no hint why. MARKPROOF_SIGNING_KEY stays the fallback, because that is what CI documentation everywhere assumes. `report.output_dir` was likewise ignored. Asking for a PDF in the config and getting nothing because `--report-dir` was not also passed would discard the request silently, so a non-default format is now enough on its own. Writing the report was also outside the guarded block — the same defect as #22 one layer further on. A missing extra propagated as a traceback and exit 1, which in this tool means "a rule failed". It is exit 2 now, with a sentence. Six CLI tests, including that a real PDF is written (`%PDF-` and two pages), that asking for `[json]` alone does not also write a summary, and that a missing renderer produces the install command rather than a stack trace. Co-Authored-By: Claude Opus 5 --- README.md | 13 +++-- examples/markproof.yaml | 5 ++ src/markproof/cli.py | 114 +++++++++++++++++++++++++++++++--------- src/markproof/config.py | 17 +++++- tests/test_cli.py | 93 ++++++++++++++++++++++++++++++++ 5 files changed, 214 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index 1c1f51f..6523560 100644 --- a/README.md +++ b/README.md @@ -157,9 +157,16 @@ markproof verify-report report.json --key public.pem MARKPROOF_SIGNING_KEY: ${{ secrets.MARKPROOF_SIGNING_KEY }} ``` -The output is signed JSON plus a job summary — no system dependencies, so this path -works on any runner. (A PDF renderer exists in the source but is not yet wired to the -CLI: [#21](https://github.com/Tippel-AI/markproof/issues/21).) +The default output is signed JSON plus a job summary — no system dependencies, so +that path works on any runner. A PDF for the auditor is opt-in through the config: + +```yaml +report: + formats: [json, summary, pdf] # needs: pipx install "markproof[pdf]" +``` + +`pdf` is pure Python. `pdf-html` renders through WeasyPrint and wants Pango and +cairo, which pip does not install — so it is never on the default path. ## Related projects diff --git a/examples/markproof.yaml b/examples/markproof.yaml index 5f43e71..3fc89c2 100644 --- a/examples/markproof.yaml +++ b/examples/markproof.yaml @@ -50,3 +50,8 @@ text_marking: rulepack: art50-eu-2026.07 report: sign_key: env:MARKPROOF_SIGNING_KEY # Ed25519 private key, PEM + # json + summary need nothing beyond the base install, which is why they are the + # default: the path a CI runner takes must not depend on a system library. + # Add `pdf` for the artefact you hand an auditor — extra [pdf], pure Python. + # `pdf-html` renders through WeasyPrint and also wants Pango and cairo. + formats: [json, summary] diff --git a/src/markproof/cli.py b/src/markproof/cli.py index 61247a2..c9f2bf8 100644 --- a/src/markproof/cli.py +++ b/src/markproof/cli.py @@ -16,6 +16,7 @@ from __future__ import annotations +import importlib import json import os import sys @@ -35,6 +36,7 @@ HttpChatProbeConfig, MarkproofConfig, MediaProbeConfig, + ReportConfig, UiProbeConfig, load_config, ) @@ -42,7 +44,7 @@ from markproof.probes.http_chat import HttpChatProbe from markproof.probes.media import MediaProbe from markproof.probes.ui import UiProbe -from markproof.report.model import ProbeRecord, build_report +from markproof.report.model import ProbeRecord, Report, build_report from markproof.report.sign import ( SigningError, generate_keypair, @@ -107,6 +109,47 @@ def main( """markproof — Article 50 conformance checks for live endpoints.""" +def _signing_key(report_config: ReportConfig) -> str: + """Where the signing key comes from, config first, environment second. + + ``report.sign_key`` was validated and then ignored, so an operator who wrote + ``sign_key: env:CI_SIGNING_KEY`` in their config got an unsigned report and no + hint why. The environment variable stays the fallback because that is what CI + documentation everywhere assumes. + """ + declared = (report_config.sign_key or "").strip() + if declared.startswith("env:"): + return os.environ.get(declared[4:], "").strip() + if declared: + return declared + return os.environ.get("MARKPROOF_SIGNING_KEY", "").strip() + + +def _render_pdf(report: Report, path: Path, *, engine: str) -> None: + """Write the PDF, or explain which extra is missing. + + Imported here rather than at module scope: both renderers are optional, and a + top-level import would make the default output path — the one that must work + on any runner — depend on a package the base install does not ship. + """ + module = "pdf_reportlab" if engine == "pdf" else "pdf_weasy" + extra = "pdf" if engine == "pdf" else "pdf-html" + try: + renderer = importlib.import_module(f"markproof.report.{module}") + renderer.render_pdf(report, path) + except Exception as exc: + # Both renderers already raise their own well-worded unavailability error — + # WeasyPrint's covers the case that actually bites people, where the wheel + # is installed but ctypes cannot find Pango or cairo. Their message is + # better than anything reconstructed here, so it is passed through and only + # framed. Broad on purpose: whatever goes wrong producing an optional + # artefact, the caller gets a sentence and exit 2, never a traceback. + raise ConfigError( + f"could not write the {engine} report: {exc}\n" + f' If the extra is missing: pip install "markproof[{extra}]"' + ) from exc + + def _readable(exc: Exception) -> str: """A sentence for a stranger, not a repr. @@ -221,6 +264,7 @@ def _write_report( timestamp: str | None, applicability: Applicability, probes: tuple[ProbeRecord, ...], + report_config: ReportConfig, ) -> None: """Write report.json and summary.md, signing when a key is configured. @@ -237,7 +281,7 @@ def _write_report( probes=probes, ) - key_source = os.environ.get("MARKPROOF_SIGNING_KEY", "").strip() + key_source = _signing_key(report_config) if key_source: try: report = sign_report(report, load_private_key(key_source)) @@ -251,15 +295,22 @@ def _write_report( report_path = report_dir / "report.json" summary_path = report_dir / "summary.md" - report_path.write_text( - json.dumps(report.model_dump(mode="json", exclude_none=True), indent=2, sort_keys=True) - + "\n", - encoding="utf-8", - ) - summary_path.write_text(render_summary(report), encoding="utf-8") - - console.print(f" report written to [cyan]{report_path}[/cyan]") - console.print(f" summary written to [cyan]{summary_path}[/cyan]") + formats = report_config.formats + if "json" in formats: + report_path.write_text( + json.dumps(report.model_dump(mode="json", exclude_none=True), indent=2, sort_keys=True) + + "\n", + encoding="utf-8", + ) + console.print(f" report written to [cyan]{report_path}[/cyan]") + if "summary" in formats: + summary_path.write_text(render_summary(report), encoding="utf-8") + console.print(f" summary written to [cyan]{summary_path}[/cyan]") + for fmt in ("pdf", "pdf-html"): + if fmt in formats: + pdf_path = report_dir / ("report.pdf" if fmt == "pdf" else "report-html.pdf") + _render_pdf(report, pdf_path, engine=fmt) + console.print(f" {fmt} written to [cyan]{pdf_path}[/cyan]") if not key_source: console.print( " [dim]unsigned — set MARKPROOF_SIGNING_KEY to produce verifiable evidence[/dim]" @@ -369,19 +420,34 @@ def run( json_out.write_text(json.dumps(payload, indent=2, sort_keys=True) + "\n", encoding="utf-8") console.print(f" findings written to [cyan]{json_out}[/cyan]") - if report_dir is not None: - _write_report( - report_dir, - config.target.name, - rulepack, - findings, - timestamp, - config.applicability, - tuple( - ProbeRecord(id=p.id, kind=p.probe_kind.value, url=p.url) - for p in config.target.probes - ), - ) + destination = report_dir if report_dir is not None else None + if destination is None and set(config.report.formats) - {"json", "summary"}: + # An operator who asked for a PDF in the config meant it; falling through + # to "no report at all" because they did not also pass --report-dir would + # silently discard the request. + destination = Path(config.report.output_dir) + if destination is not None: + # Guarded like the evaluation above, and for the same reason: producing an + # optional artefact can fail — a missing extra, an unwritable directory — + # and that is "the run could not finish", not "a rule failed". Exit 1 is + # reserved for the second. + try: + _write_report( + destination, + config.target.name, + rulepack, + findings, + timestamp, + config.applicability, + tuple( + ProbeRecord(id=p.id, kind=p.probe_kind.value, url=p.url) + for p in config.target.probes + ), + config.report, + ) + except (ConfigError, OSError) as exc: + err_console.print(f"[bold red]error:[/] {exc}") + raise typer.Exit(code=2) from exc raise typer.Exit(code=exit_code_for(findings)) diff --git a/src/markproof/config.py b/src/markproof/config.py index ff30357..dc4d924 100644 --- a/src/markproof/config.py +++ b/src/markproof/config.py @@ -314,8 +314,23 @@ class ReportConfig(BaseModel): model_config = ConfigDict(extra="forbid", frozen=True) sign_key: str | None = None - formats: tuple[Literal["json", "summary"], ...] = ("json", "summary") + """Where the Ed25519 signing key comes from: ``env:NAME`` for an environment + variable, or a path. ``MARKPROOF_SIGNING_KEY`` is used when this is unset, so + CI needs no config change.""" + + formats: tuple[Literal["json", "summary", "pdf", "pdf-html"], ...] = ("json", "summary") + """Which artefacts to write. + + ``json`` and ``summary`` need nothing beyond the base install and are the + default for that reason — the output path a CI runner takes must never depend + on a system library. ``pdf`` needs the ``[pdf]`` extra (pure Python) and + ``pdf-html`` the ``[pdf-html]`` one, which wants Pango and cairo and is not + pip-installable. Ask for one you have not installed and the run stops with the + install command rather than a traceback. + """ + output_dir: str = "markproof-report" + """Where those artefacts go, unless ``--report-dir`` overrides it.""" class TextMarkingConfig(BaseModel): diff --git a/tests/test_cli.py b/tests/test_cli.py index 20bf35f..e76a693 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -337,3 +337,96 @@ def test_the_timestamp_can_be_pinned(self, tmp_path: Path) -> None: ) report = json.loads((out / "report.json").read_text(encoding="utf-8")) assert report["run"]["timestamp"] == stamp + + +class TestPdfOutput: + """Issue #21: 354 lines of renderer that no invocation could reach. + + `ReportConfig.formats` was typed to accept only json and summary, the CLI + never imported either renderer, and `run --help` said nothing about PDF — while + the README advertised `pip install "markproof[pdf]"` as if installing the extra + got you one. The PDF is the artefact you hand an auditor, so having it exist + only in the source tree was the gap least likely to be noticed and most likely + to matter. + """ + + @respx.mock + def test_a_pdf_is_written_when_the_config_asks_for_one(self, tmp_path: Path) -> None: + respx.post(_ENDPOINT).mock(return_value=_reply(_DISCLOSED)) + config = _config(tmp_path, extra="report:\n formats: [json, pdf]\n") + out = tmp_path / "out" + result = RUNNER.invoke(app, ["run", "-c", str(config), "--report-dir", str(out)]) + assert result.exit_code == 0, result.output + pdf = out / "report.pdf" + assert pdf.is_file() + assert pdf.read_bytes().startswith(b"%PDF-"), "not a PDF" + + @respx.mock + def test_formats_are_honoured_rather_than_validated_and_ignored(self, tmp_path: Path) -> None: + """Asking for json alone must not also produce a summary.""" + respx.post(_ENDPOINT).mock(return_value=_reply(_DISCLOSED)) + config = _config(tmp_path, extra="report:\n formats: [json]\n") + out = tmp_path / "out" + RUNNER.invoke(app, ["run", "-c", str(config), "--report-dir", str(out)]) + assert (out / "report.json").is_file() + assert not (out / "summary.md").exists() + + @respx.mock + def test_a_pdf_request_alone_is_enough_to_write_a_report(self, tmp_path: Path) -> None: + """Without --report-dir, output_dir from the config is used. + + An operator who asked for a PDF meant it; producing nothing because they + did not also pass a flag would discard the request silently. + """ + respx.post(_ENDPOINT).mock(return_value=_reply(_DISCLOSED)) + config = _config( + tmp_path, extra=f"report:\n formats: [pdf]\n output_dir: {tmp_path / 'declared'}\n" + ) + RUNNER.invoke(app, ["run", "-c", str(config)]) + assert (tmp_path / "declared" / "report.pdf").is_file() + + @respx.mock + def test_a_missing_extra_is_a_sentence_with_the_install_command( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """pdf-html needs Pango and cairo, which pip cannot install for you.""" + from markproof.report import pdf_weasy + + def _unavailable(*_: object, **__: object) -> None: + raise pdf_weasy.WeasyPrintUnavailableError( + "WeasyPrint is not installed. pip install 'markproof[pdf-html]' — " + "it also needs Pango and cairo, which pip does not install." + ) + + monkeypatch.setattr(pdf_weasy, "render_pdf", _unavailable) + respx.post(_ENDPOINT).mock(return_value=_reply(_DISCLOSED)) + config = _config(tmp_path, extra="report:\n formats: [pdf-html]\n") + result = RUNNER.invoke( + app, ["run", "-c", str(config), "--report-dir", str(tmp_path / "out")] + ) + assert result.exit_code == 2, result.output + assert "Traceback" not in result.output + assert "pdf-html" in result.output + + +class TestSignKeyFromConfig: + @respx.mock + def test_the_config_can_name_the_environment_variable( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """`sign_key` was validated and then ignored, so a report went out unsigned.""" + respx.post(_ENDPOINT).mock(return_value=_reply(_DISCLOSED)) + keys = tmp_path / "keys" + keys.mkdir() + RUNNER.invoke(app, ["keygen", "--out-dir", str(keys)]) + monkeypatch.setenv( + "CI_SIGNING_KEY", (keys / "markproof-signing-key.pem").read_text(encoding="utf-8") + ) + monkeypatch.delenv("MARKPROOF_SIGNING_KEY", raising=False) + + config = _config(tmp_path, extra="report:\n sign_key: env:CI_SIGNING_KEY\n") + out = tmp_path / "out" + result = RUNNER.invoke(app, ["run", "-c", str(config), "--report-dir", str(out)]) + assert result.exit_code == 0, result.output + report = json.loads((out / "report.json").read_text(encoding="utf-8")) + assert "signature" in report, "the configured key was ignored" From ffaa3ca6645ce2a84b39e0077be80946b7145086 Mon Sep 17 00:00:00 2001 From: Lukas Friedrich Date: Mon, 31 Aug 2026 21:36:33 +0200 Subject: [PATCH 2/2] Test the PDF path in CI rather than skipping it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pytest jobs installed only the dev extra, so the newly wired PDF output was untested on every runner — the same shape of gap as the one this PR closes. CI now installs [pdf] as well, and a contributor without it gets a skip rather than a puzzling failure. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 5 ++++- tests/test_cli.py | 10 ++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1906bd2..3a0e4c9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -71,7 +71,10 @@ jobs: with: enable-cache: true - name: pytest - run: uv run --python ${{ matrix.python-version }} --extra dev pytest + # --extra pdf as well: the PDF renderer is optional at install time but + # not optional to test — it is the artefact an auditor reads, and it went + # unreachable from the CLI for six milestones without CI noticing. + run: uv run --python ${{ matrix.python-version }} --extra dev --extra pdf pytest determinism: name: determinism (byte-identical report) diff --git a/tests/test_cli.py b/tests/test_cli.py index e76a693..2faad08 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -18,6 +18,7 @@ from __future__ import annotations +import importlib.util import json import stat from pathlib import Path @@ -31,6 +32,13 @@ RUNNER = CliRunner() +#: The PDF path needs an optional extra. CI installs it so the path is genuinely +#: covered; a contributor without it gets a skip rather than a puzzling failure. +needs_reportlab = pytest.mark.skipif( + importlib.util.find_spec("reportlab") is None, + reason="needs the [pdf] extra: pip install 'markproof[pdf]'", +) + _ENDPOINT = "https://api.example.invalid/v1/chat/completions" _DISCLOSED = "Hallo! Sie sprechen mit einer KI. Wie kann ich helfen?" _SILENT = "Hallo! Wie kann ich Ihnen helfen?" @@ -350,6 +358,7 @@ class TestPdfOutput: to matter. """ + @needs_reportlab @respx.mock def test_a_pdf_is_written_when_the_config_asks_for_one(self, tmp_path: Path) -> None: respx.post(_ENDPOINT).mock(return_value=_reply(_DISCLOSED)) @@ -371,6 +380,7 @@ def test_formats_are_honoured_rather_than_validated_and_ignored(self, tmp_path: assert (out / "report.json").is_file() assert not (out / "summary.md").exists() + @needs_reportlab @respx.mock def test_a_pdf_request_alone_is_enough_to_write_a_report(self, tmp_path: Path) -> None: """Without --report-dir, output_dir from the config is used.