From 9af5192d6bb3175e73d9c7e0b57f240468fcacf8 Mon Sep 17 00:00:00 2001 From: Tom Ballard Date: Sat, 18 Jul 2026 15:10:20 +0000 Subject: [PATCH 1/2] feat(herald): add the Lore decisions-on-PR capability [roadmap:decisions-on-pr] --- README.md | 1 + herald/github/action.yml | 144 ++++++++++++++++++++++++++++++++++ herald/github/render.py | 164 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 309 insertions(+) create mode 100644 herald/github/action.yml create mode 100644 herald/github/render.py diff --git a/README.md b/README.md index a78eab8..c00f63c 100644 --- a/README.md +++ b/README.md @@ -17,6 +17,7 @@ published `rac-core` from PyPI (pin with the `rac-version` input). | Watchkeeper | [`watchkeeper/github/`](watchkeeper/github/) | `rac watchkeeper` (PR knowledge review) | `uses: itsthelore/rac-ci/watchkeeper/github@` | | Gatekeeper | [`gatekeeper/github/`](gatekeeper/github/) | `rac gate --sarif` (required merge gate) | `uses: itsthelore/rac-ci/gatekeeper/github@` | | Registrar | [`registrar/github/`](registrar/github/) | `rac validate --sarif` (well-formedness, ADR-058) | `uses: itsthelore/rac-ci/registrar/github@` | +| Herald | [`herald/github/`](herald/github/) | `rac decisions-for --json` (advisory governing-decisions comment on PRs) | `uses: itsthelore/rac-ci/herald/github@` | | Recordkeeper | [`recordkeeper/`](recordkeeper/) | read-access audit recorder (ADR-084) | *placeholder — not yet shipped* | A reusable Watchkeeper workflow is also published at diff --git a/herald/github/action.yml b/herald/github/action.yml new file mode 100644 index 0000000..60d481e --- /dev/null +++ b/herald/github/action.yml @@ -0,0 +1,144 @@ +# Lore Herald composite action (pr-decision-surfacing design; ADR-034 / +# ADR-063 / ADR-067). +# +# Proclaims the recorded decisions that govern a pull request's changed paths +# as ONE advisory comment: compute the merge-base diff, run +# `rac decisions-for --json` per changed path (the live-decisions lookup the +# CLI and MCP serve, `decision-to-code-proximity`), and post a terse, +# deduplicated comment that re-runs update in place. Facts, never verdicts +# (ADR-034): the comment names what governs and recommends review; it never +# gates the merge — the action succeeds whatever it finds, and only +# operational breakage fails the step. A thin consumer (ADR-063): matching +# and liveness live entirely in the engine; nothing is re-derived here. Named +# a `lore-*` surface per the design (ADR-068) — Herald announces, the +# Gatekeeper enforces. +# +# Siblings: Watchkeeper at `watchkeeper/github/`, the gate at +# `gatekeeper/github/`, validate at `registrar/github/`. This surface is +# referenced as `uses: itsthelore/rac-ci/herald/github@`. +# +# Requirements: a pull_request-triggered workflow, `fetch-depth: 0` on the +# checkout (the diff needs the merge base, like Watchkeeper), and +# `pull-requests: write` on the job for the comment. On forks, where the +# token is read-only, the comment falls back to the step summary instead of +# failing the check. +name: "Lore decisions on PR" +description: >- + Comment the recorded decisions that govern a pull request's changed paths — + id, title, and the declared scope that matched — as one advisory, + update-in-place comment. A thin wrapper over `rac decisions-for`; facts, + never a merge gate. +author: "Tom Ballard" +branding: + icon: "book-open" + color: "purple" +inputs: + path: + description: "The RAC corpus directory (passed to `rac decisions-for`)." + required: false + default: "rac" + max-inline: + description: >- + Governing decisions listed in full before the rest collapse into a + details expander. + required: false + default: "5" + rac-version: + description: >- + Exact rac-core version to install from PyPI. Empty installs the + latest release. + required: false + default: "" +runs: + using: "composite" + steps: + - uses: actions/setup-python@v5 + with: + python-version: "3.12" + - name: Install RAC + shell: bash + env: + RAC_VERSION: ${{ inputs.rac-version }} + run: | + python -m pip install --quiet --upgrade pip + if [ -n "$RAC_VERSION" ]; then + python -m pip install --quiet "rac-core==$RAC_VERSION" + else + python -m pip install --quiet rac-core + fi + # The PR's changed paths, from the merge base (triple-dot): what the PR + # itself changes, correct even when the base branch has moved ahead. + # Requires fetch-depth: 0 on the checkout. + - name: Compute changed paths + shell: bash + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + if [ -z "$BASE_SHA" ] || [ -z "$HEAD_SHA" ]; then + echo "::error::Lore decisions on PR runs on pull_request events only (no base/head sha in this event)." + exit 1 + fi + git diff --name-only "$BASE_SHA...$HEAD_SHA" > lore-changed-paths.txt + echo "$(wc -l < lore-changed-paths.txt) changed path(s)" + # One deterministic comment body: `rac decisions-for --json` per changed + # path, merged by decision id, sorted, no wall-clock input — the same + # bytes for the same corpus and diff. + - name: Render governing decisions + id: render + shell: bash + env: + INPUT_PATH: ${{ inputs.path }} + MAX_INLINE: ${{ inputs.max-inline }} + LINK_BASE: ${{ github.server_url }}/${{ github.repository }}/blob/${{ github.event.pull_request.head.sha }} + run: | + python "$GITHUB_ACTION_PATH/render.py" \ + --corpus "$INPUT_PATH" \ + --paths-file lore-changed-paths.txt \ + --link-base "$LINK_BASE" \ + --max-inline "$MAX_INLINE" \ + --out lore-decisions-comment.md + # One comment per PR, updated in place on re-runs (found by its marker). + # No comment is created when nothing governs; an existing comment is + # updated even to the empty state, so it never goes stale. A read-only + # token (forks) degrades to the step summary — advisory surfaces never + # fail the check. + - name: Post or update the comment + uses: actions/github-script@v7 + env: + HAS_DECISIONS: ${{ steps.render.outputs.has_decisions }} + with: + script: | + const fs = require('fs'); + const marker = ''; + const body = fs.readFileSync('lore-decisions-comment.md', 'utf8'); + const hasDecisions = process.env.HAS_DECISIONS === 'true'; + try { + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + per_page: 100, + }); + const existing = comments.find(c => c.body && c.body.includes(marker)); + if (existing) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body, + }); + } else if (hasDecisions) { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + body, + }); + } else { + core.info('No governing decisions and no existing comment — nothing to post.'); + } + } catch (error) { + core.warning(`Could not post the decisions comment (${error.message}); writing it to the step summary instead.`); + await core.summary.addRaw(body).write(); + } diff --git a/herald/github/render.py b/herald/github/render.py new file mode 100644 index 0000000..f5171d8 --- /dev/null +++ b/herald/github/render.py @@ -0,0 +1,164 @@ +#!/usr/bin/env python3 +"""Render the decisions-on-PR comment from `rac decisions-for --json`. + +The thin-client half of the `pr-decision-surfacing` design (ADR-063): run the +engine's live-decisions lookup for each changed path, merge by decision id, +and emit one deterministic Markdown comment — facts, never verdicts (ADR-034). +Everything semantic (scope matching, liveness) is the engine's; this script +only joins, sorts, and formats. No wall-clock input: the same corpus and diff +render the same bytes. + +Exit code is 0 whether or not decisions govern (advisory surfaces never fail +a check); only operational breakage — the `rac` binary missing outright — +exits non-zero. A lookup failure on one path is skipped with a warning so a +single odd path cannot take down the whole comment. +""" + +from __future__ import annotations + +import argparse +import json +import os +import subprocess +import sys + +MARKER = "" + +EMPTY_BODY = ( + f"{MARKER}\n" + "### Decisions governing this change\n\n" + "No recorded decisions govern the paths changed by this pull request.\n" +) + +# Changed paths listed per decision before collapsing to a count. +_PATHS_SHOWN = 3 + + +def _lookup(rac_bin: str, path: str, corpus: str) -> dict | None: + """One `rac decisions-for --json` call, parsed, or None.""" + try: + proc = subprocess.run( + [rac_bin, "decisions-for", path, corpus, "--json"], + capture_output=True, + text=True, + check=False, + ) + except FileNotFoundError: + print(f"error: rac binary not found: {rac_bin!r}", file=sys.stderr) + raise SystemExit(1) from None + if proc.returncode != 0: + print( + f"warning: decisions-for skipped {path!r} (exit {proc.returncode})", + file=sys.stderr, + ) + return None + try: + return json.loads(proc.stdout) + except json.JSONDecodeError: + print(f"warning: decisions-for emitted non-JSON for {path!r}", file=sys.stderr) + return None + + +def collect(rac_bin: str, corpus: str, changed_paths: list[str]) -> list[dict]: + """Governing decisions merged across paths, sorted by decision id. + + Each entry: id, title, status, path (the artifact file), matching entries + and changed paths as sorted lists. The engine already scopes the answer to + live decisions with a matching declared `## Applies To`. + """ + merged: dict[str, dict] = {} + for changed in changed_paths: + result = _lookup(rac_bin, changed, corpus) + if not result: + continue + for decision in result.get("decisions", []): + entry = merged.setdefault( + decision["id"], + { + "id": decision["id"], + "title": decision["title"], + "status": decision["status"], + "path": decision["path"], + "matching_entries": set(), + "changed_paths": set(), + }, + ) + entry["matching_entries"].add(decision["matching_entry"]) + entry["changed_paths"].add(changed) + decisions = [] + for entry in sorted(merged.values(), key=lambda e: e["id"]): + entry["matching_entries"] = sorted(entry["matching_entries"]) + entry["changed_paths"] = sorted(entry["changed_paths"]) + decisions.append(entry) + return decisions + + +def _bullet(decision: dict, link_base: str) -> str: + scopes = ", ".join(f"`{s}`" for s in decision["matching_entries"]) + shown = decision["changed_paths"][:_PATHS_SHOWN] + changed = ", ".join(f"`{p}`" for p in shown) + more = len(decision["changed_paths"]) - len(shown) + if more > 0: + changed += f" +{more} more" + link = f"{link_base}/{decision['path']}" if link_base else decision["path"] + return ( + f"- **[{decision['id']} — {decision['title']}]({link})**" + f" ({decision['status']}) — applies to {scopes} — changed: {changed}" + ) + + +def render(decisions: list[dict], link_base: str, max_inline: int) -> str: + """The comment body: marker, terse header, one bullet per decision.""" + if not decisions: + return EMPTY_BODY + count = len(decisions) + plural = "" if count == 1 else "s" + lines = [ + MARKER, + "### Decisions governing this change", + "", + f"This pull request touches paths governed by {count} recorded " + f"decision{plural} — review recommended.", + "", + ] + inline = decisions[:max_inline] if max_inline > 0 else decisions + rest = decisions[len(inline) :] + lines.extend(_bullet(d, link_base) for d in inline) + if rest: + lines.append("") + lines.append(f"
{len(rest)} more governing decision" + f"{'' if len(rest) == 1 else 's'}") + lines.append("") + lines.extend(_bullet(d, link_base) for d in rest) + lines.append("") + lines.append("
") + lines.append("") + return "\n".join(lines) + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--corpus", required=True) + parser.add_argument("--paths-file", required=True) + parser.add_argument("--link-base", default="") + parser.add_argument("--max-inline", type=int, default=5) + parser.add_argument("--out", required=True) + parser.add_argument("--rac-bin", default="rac") + args = parser.parse_args(argv) + with open(args.paths_file, encoding="utf-8") as fh: + changed_paths = sorted({line.strip() for line in fh if line.strip()}) + decisions = collect(args.rac_bin, args.corpus, changed_paths) + body = render(decisions, args.link_base.rstrip("/"), args.max_inline) + with open(args.out, "w", encoding="utf-8") as fh: + fh.write(body) + has_decisions = "true" if decisions else "false" + output_path = os.environ.get("GITHUB_OUTPUT") + if output_path: + with open(output_path, "a", encoding="utf-8") as fh: + fh.write(f"has_decisions={has_decisions}\n") + print(f"{len(decisions)} governing decision(s); has_decisions={has_decisions}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) From a96cff6bfc1112752f59d0b51ff1ad1bbf946bd9 Mon Sep 17 00:00:00 2001 From: Tom Ballard Date: Sat, 18 Jul 2026 15:14:19 +0000 Subject: [PATCH 2/2] test(herald): pin the action contract and renderer battery [roadmap:decisions-on-pr] --- .github/workflows/tests.yml | 23 ++++- tests/test_herald_action.py | 72 ++++++++++++++++ tests/test_herald_renderer.py | 154 ++++++++++++++++++++++++++++++++++ 3 files changed, 248 insertions(+), 1 deletion(-) create mode 100644 tests/test_herald_action.py create mode 100644 tests/test_herald_renderer.py diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 054cbb4..55825f1 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -26,4 +26,25 @@ jobs: run: python -m pip install --quiet --upgrade pip pytest pyyaml - name: Run action-contract tests - run: python -m pytest tests/ -q + run: python -m pytest tests/ -q --ignore=tests/test_herald_renderer.py + + # The Herald renderer is behavioral, not just structural: it shells the real + # `rac decisions-for --json` (published rac-core — the same contract the + # wrapper consumes in the field, ADR-063) and must render byte-identical + # comments for the same corpus and diff. Kept out of the structural job so + # that tier stays engine-free. + herald-renderer: + name: herald renderer (published engine) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + + - uses: actions/setup-python@v6 + with: + python-version: "3.12" + + - name: Install test deps and the published engine + run: python -m pip install --quiet --upgrade pip pytest rac-core + + - name: Run the renderer battery + run: python -m pytest tests/test_herald_renderer.py -q diff --git a/tests/test_herald_action.py b/tests/test_herald_action.py new file mode 100644 index 0000000..3b19dd0 --- /dev/null +++ b/tests/test_herald_action.py @@ -0,0 +1,72 @@ +"""Structural tests for the Lore decisions-on-PR (Herald) composite action. + +The action is a thin wrapper (ADR-063) over `rac decisions-for --json`: compute +the PR's merge-base diff, render one deterministic advisory comment, and post it +update-in-place. Matching and liveness are the engine's; these tests pin the +action's *contract* — inputs, the triple-dot diff, the renderer hand-off, and +the never-gate comment step — so the wiring cannot silently drift. +""" + +from __future__ import annotations + +from pathlib import Path + +import yaml + +ACTION = Path(__file__).parent.parent / "herald" / "github" / "action.yml" + + +def _action() -> dict: + return yaml.safe_load(ACTION.read_text(encoding="utf-8")) + + +def _step(name_fragment: str) -> dict: + for step in _action()["runs"]["steps"]: + if name_fragment in step.get("name", ""): + return step + raise AssertionError(f"no step named like {name_fragment!r}") + + +def test_action_is_composite(): + a = _action() + assert a["runs"]["using"] == "composite" + assert a["name"] == "Lore decisions on PR" + + +def test_action_declares_exactly_expected_inputs(): + inputs = _action()["inputs"] + assert set(inputs) == {"path", "max-inline", "rac-version"} + assert inputs["path"]["default"] == "rac" + assert inputs["max-inline"]["default"] == "5" + + +def test_action_installs_published_rac_core(): + # Published engine only (pinned via rac-version, else latest); no + # source-install mode reaching outside the action directory. + run_steps = " ".join(s.get("run", "") for s in _action()["runs"]["steps"]) + assert "rac-core" in run_steps + assert "GITHUB_ACTION_PATH/.." not in run_steps + + +def test_action_diffs_from_the_merge_base(): + diff = _step("Compute changed paths")["run"] + # Triple-dot: what the PR itself changes, even when base has moved ahead. + assert '"$BASE_SHA...$HEAD_SHA"' in diff + assert "pull_request events only" in diff + + +def test_action_delegates_rendering_to_render_py(): + render = _step("Render governing decisions")["run"] + assert "$GITHUB_ACTION_PATH/render.py" in render + assert (ACTION.parent / "render.py").is_file() + + +def test_action_comment_step_updates_in_place_and_never_gates(): + script = _step("Post or update the comment")["with"]["script"] + assert "lore-decisions-on-pr" in script # the update-in-place marker + assert "updateComment" in script + assert "createComment" in script + # Advisory surfaces never fail the check: a read-only token degrades to + # the step summary with a warning. + assert "core.warning" in script + assert "core.summary" in script diff --git a/tests/test_herald_renderer.py b/tests/test_herald_renderer.py new file mode 100644 index 0000000..3f712e9 --- /dev/null +++ b/tests/test_herald_renderer.py @@ -0,0 +1,154 @@ +"""Behavioral battery for the Herald renderer (`herald/github/render.py`). + +Unlike the structural action-contract tests, these exercise the real published +engine: build a tiny corpus, shell `render.py` (which shells +`rac decisions-for --json` per changed path — the same contract the wrapper +consumes in the field, ADR-063), and pin the rendered comment. Facts, never +verdicts (ADR-034): governed and ungoverned diffs both exit 0, and the same +corpus and diff must render byte-identical output. +""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +RENDER = Path(__file__).parent.parent / "herald" / "github" / "render.py" + +# The `rac` console script installed alongside the interpreter running pytest. +RAC_BIN = Path(sys.executable).parent / "rac" + +LINK_BASE = "https://example.com/blob/HEAD" + + +def _write_decision(repo: Path, stem: str, number: int, title: str, scope: str) -> None: + path = repo / "rac" / "decisions" / f"{stem}.md" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text( + f"""--- +schema_version: 1 +id: ACT-{number:04d} +type: decision +--- +# {title} + +## Context + +Recorded for the Herald renderer battery. + +## Decision + +{title}. + +## Consequences + +The scope below is governed. + +## Status + +Accepted + +## Applies To + +- {scope} +""", + encoding="utf-8", + ) + + +def _make_repo(tmp_path: Path, scopes: dict[str, str]) -> Path: + """A tmp repository: `.rac/config.yaml` plus one decision per scope.""" + repo = tmp_path / "repo" + (repo / ".rac").mkdir(parents=True) + (repo / ".rac" / "config.yaml").write_text( + "repository_key: ACT\n", encoding="utf-8" + ) + for number, (stem, scope) in enumerate(sorted(scopes.items()), start=1): + _write_decision(repo, stem, number, stem.replace("-", " ").title(), scope) + return repo + + +def _render( + repo: Path, changed_paths: list[str], max_inline: int = 5 +) -> tuple[subprocess.CompletedProcess, str]: + paths_file = repo / "changed-paths.txt" + paths_file.write_text("".join(f"{p}\n" for p in changed_paths), encoding="utf-8") + out = repo / "comment.md" + proc = subprocess.run( + [ + sys.executable, + str(RENDER), + "--corpus", + "rac", + "--paths-file", + str(paths_file), + "--link-base", + LINK_BASE, + "--max-inline", + str(max_inline), + "--out", + str(out), + "--rac-bin", + str(RAC_BIN), + ], + cwd=repo, + capture_output=True, + text=True, + check=False, + ) + assert proc.returncode == 0, proc.stderr + return proc, out.read_text(encoding="utf-8") + + +def test_governing_decisions_render_sorted_and_deduplicated(tmp_path): + repo = _make_repo( + tmp_path, + {"alpha-rule": "src/**/*.py", "beta-rule": "docs/**"}, + ) + proc, body = _render(repo, ["src/app.py", "src/pkg/util.py", "docs/guide.md"]) + assert "2 governing decision(s)" in proc.stdout + # One bullet per decision, sorted by id, however many changed paths matched. + assert body.count("- **[alpha-rule") == 1 + assert body.index("alpha-rule") < body.index("beta-rule") + assert "applies to `src/**/*.py`" in body + assert f"[alpha-rule — Alpha Rule]({LINK_BASE}/rac/decisions/alpha-rule.md)" in body + assert "`src/app.py`, `src/pkg/util.py`" in body + + +def test_ungoverned_diff_renders_the_empty_state(tmp_path): + repo = _make_repo(tmp_path, {"alpha-rule": "src/**/*.py"}) + proc, body = _render(repo, ["README.md"]) + assert "No recorded decisions govern" in body + assert "has_decisions=false" in proc.stdout + + +def test_a_bad_changed_path_does_not_take_down_the_comment(tmp_path): + repo = _make_repo(tmp_path, {"alpha-rule": "src/**/*.py"}) + proc, body = _render(repo, ["no/such/file.py", "src/app.py"]) + assert proc.returncode == 0 + assert "alpha-rule" in body + + +def test_overflow_collapses_into_a_details_expander(tmp_path): + repo = _make_repo( + tmp_path, + { + "alpha-rule": "src/**/*.py", + "beta-rule": "docs/**", + "gamma-rule": "tools/**", + }, + ) + _, body = _render( + repo, ["src/app.py", "docs/guide.md", "tools/run.sh"], max_inline=2 + ) + assert "
1 more governing decision" in body + assert body.index("gamma-rule") > body.index("
") + + +def test_rendering_is_byte_deterministic(tmp_path): + scopes = {"alpha-rule": "src/**/*.py", "beta-rule": "docs/**"} + changed = ["src/app.py", "docs/guide.md", "src/pkg/util.py"] + _, forward = _render(_make_repo(tmp_path / "a", scopes), changed) + _, backward = _render(_make_repo(tmp_path / "b", scopes), list(reversed(changed))) + assert forward == backward