diff --git a/README.md b/README.md index 1a00531..94f832d 100644 --- a/README.md +++ b/README.md @@ -347,6 +347,7 @@ The first sync after the merge creates `codeboarding/baseline`, catching up from | `github_token` | both | `${{ github.token }}` | Token for comments and sync delivery. | | `codeboarding_analysis_location` | both | `codeboarding_branch` | Where the synced branch's analysis is stored: `codeboarding_branch` (on `codeboarding/baseline`, for the default branch; the code is never written) or `in_place` (committed as `.codeboarding/` on the synced branch). Sync saves it there and review reads it from there. | | `warmstart_retention_days` | review | `1` | Days to keep the reusable analysis. Only the next run reads it. | +| `review_guide_url` | review | `''` | Experimental. The CodeBoarding service that writes the pull request's review guide; when set, the run sends it this analysis and stores `review_guide.json` in the review artifact. Never fails the run. | `target_branch` and `sync_strategy` are deprecated, and read as the `codeboarding_analysis_location` they stand for, with a warning: `sync_strategy: push` as `in_place`, `sync_strategy: pull_request` as `codeboarding_branch`. `target_branch` is accepted only when it names the branch the run is on: the push trigger picks the branch. `force_full: true` is ignored with a warning. Anything the new input cannot express fails, linking to [moving an existing setup](#moving-an-existing-setup). diff --git a/action.yml b/action.yml index 0900879..897e83c 100644 --- a/action.yml +++ b/action.yml @@ -143,6 +143,14 @@ inputs: description: 'Positive integer analysis depth cap, used for both review and sync, including full fallbacks.' required: false default: '2' + review_guide_url: + description: >- + Experimental. The CodeBoarding service that writes a pull request's review guide (main + changes, each change as numbered steps on the map). When set, a review run sends it this + run's analysis and stores the guide in the review artifact for the platform to show. Empty + leaves it off. + required: false + default: '' github_token: description: 'Token used for comments and sync delivery.' required: false @@ -686,6 +694,28 @@ runs: ANALYSED_FILES_CHANGED: ${{ steps.review_render.outputs.analysed_files_changed }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-artifact.sh" + # After the artifact holds the analysis and before it is uploaded: the guide is one more file + # in it. Experimental and off unless the workflow names the service; it never fails the run. + - name: Build review guide + id: review_guide + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && inputs.review_guide_url != '' + continue-on-error: true + shell: bash + env: + REVIEW_GUIDE_URL: ${{ inputs.review_guide_url }} + REPOSITORY: ${{ github.repository }} + PR_NUMBER: ${{ steps.guard.outputs.pr_number }} + ARTIFACT_DIR: ${{ steps.review_artifact.outputs.artifact_dir }} + HEAD_ANALYSIS_PATH: ${{ steps.review_analyze.outputs.analysis_path }} + BASE_ANALYSIS_PATH: ${{ steps.review_baseline.outputs.base_analysis_path }} + CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target + GITHUB_TOKEN: ${{ inputs.github_token }} + LICENSE_KEY: ${{ inputs.license_key }} + OLLAMA_API_KEY: ${{ inputs.ollama_api_key }} + OPENROUTER_API_KEY: ${{ inputs.openrouter_api_key }} + OPENAI_API_KEY: ${{ inputs.openai_api_key }} + run: python3 "$GITHUB_ACTION_PATH/scripts/review_guide.py" + - name: Upload review artifact id: upload_review_artifact_dotcom if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && github.server_url == 'https://github.com' diff --git a/scripts/review_guide.py b/scripts/review_guide.py new file mode 100644 index 0000000..8698c35 --- /dev/null +++ b/scripts/review_guide.py @@ -0,0 +1,252 @@ +#!/usr/bin/env python3 +"""Ask CodeBoarding's closed functions for this pull request's review guide, and keep it in the +review artifact as ``review_guide.json``. + +Runs after the review analysis, from what the run already has: both analyses and both call graphs +(the ``static_analysis.pkl`` beside each ``analysis.json``), the head checkout, and the diff between +the two analysed commits. It packs them into one gzipped bundle, posts it, and writes the guide the +service returns next to ``analysis.json``, where the webview reads it. Nothing here builds the guide +itself; that code is CodeBoarding's and stays on its side. + +Credentials follow the analysis: the run's own Ollama, OpenRouter or OpenAI key when it has one, +otherwise CodeBoarding's hosted model. Either way the service needs to know the caller is this +repository's run: a fresh GitHub Actions OIDC token (plus the license, the same bearer the hosted +proxy gets), or the run's GITHUB_TOKEN when the workflow cannot mint one. A failure leaves the +artifact without a guide and never fails the run. + +Environment: REVIEW_GUIDE_URL, REPOSITORY, PR_NUMBER, ARTIFACT_DIR, HEAD_ANALYSIS_PATH, +BASE_ANALYSIS_PATH, CHECKOUT_DIR, GITHUB_TOKEN, GITHUB_API_URL, and optionally LICENSE_KEY, +OLLAMA_API_KEY, OPENROUTER_API_KEY, OPENAI_API_KEY, ACTIONS_ID_TOKEN_REQUEST_URL/_TOKEN. +""" + +from __future__ import annotations + +import gzip +import importlib.util +import json +import os +import pickle +import sys +from pathlib import Path +from urllib.error import HTTPError, URLError +from urllib.parse import quote +from urllib.request import Request, urlopen + +# The service's Function URL takes at most 6 MB; the bundle is gzipped JSON, sent as is. +MAX_BUNDLE_BYTES = 5_800_000 +# Changed files whose contents are sent at both commits; past this the guide reads diffs only. +MAX_FILES = 300 +OWN_KEYS = (("OLLAMA_API_KEY", "ollama"), ("OPENROUTER_API_KEY", "openrouter"), ("OPENAI_API_KEY", "openai")) + + +class Skip(Exception): + """This run gets no guide, for a reason worth one line in the log.""" + + +def _github(path: str, accept: str = "application/vnd.github+json") -> bytes: + api = os.environ.get("GITHUB_API_URL", "https://api.github.com").rstrip("/") + req = Request( + f"{api}{path}", + headers={ + "Authorization": f"Bearer {os.environ['GITHUB_TOKEN']}", + "Accept": accept, + "User-Agent": "codeboarding-action", + }, + ) + with urlopen(req, timeout=30) as res: # nosec B310 - GitHub API URL + return res.read() + + +def _github_json(path: str): + return json.loads(_github(path)) + + +def call_graph(pkl: Path) -> dict: + """A static_analysis.pkl's call graph as the service reads it: nodes and edges with call sites. + The pickle holds CodeBoarding's own classes, which the installed engine provides.""" + with pkl.open("rb") as f: + results = pickle.load(f) # nosec B301 - written by this run's own analysis + nodes, edges = {}, [] + for lang in results.results.values(): + graph = lang.cfg.graph + for name, n in graph.nodes.items(): + nodes[name] = { + "file": n.file_path, + "start": n.line_start, + "end": n.line_end, + "callable": bool(n.is_callable()), + "cb": bool(n.is_callback_or_anonymous()), + } + for e in graph.edges: + sites = [] + for s in vars(e).get("_call_sites") or []: + v = s if isinstance(s, (tuple, list, dict)) else vars(s) if hasattr(s, "__dict__") else str(s) + sites.append(list(v) if isinstance(v, tuple) else v) + edges.append({"s": e.get_source(), "d": e.get_destination(), "sites": sites}) + return {"nodes": nodes, "edges": edges} + + +def _diff(repo: str, base: str, head: str) -> list[dict]: + """The files changed between the two analysed commits, in the shape of a pull request's files + (the compare API returns the same fields), so the guide reads exactly what was analysed.""" + out = _github_json(f"/repos/{repo}/compare/{base}...{head}") + return [ + { + k: f.get(k) + for k in ("filename", "status", "additions", "deletions", "changes", "patch", "previous_filename") + if k in f + } + for f in out.get("files") or [] + ] + + +def _contents(repo: str, sha: str, path: str) -> str | None: + try: + raw = _github(f"/repos/{repo}/contents/{quote(path)}?ref={sha}", accept="application/vnd.github.raw") + except HTTPError: + return None + try: + return raw.decode("utf-8") + except UnicodeDecodeError: + return None + + +def bundle(env: dict) -> dict: + """Everything the guide is built from, as the service's bundle format names it.""" + repo, pr = env["REPOSITORY"], int(env["PR_NUMBER"]) + artifact = Path(env["ARTIFACT_DIR"]) + head_dir, base_dir = Path(env["HEAD_ANALYSIS_PATH"]).parent, Path(env["BASE_ANALYSIS_PATH"]).parent + meta = json.loads((artifact / "metadata.json").read_text()) + head_sha = meta["head_sha"] + # The analysis compared against the merge base, so that is the base the guide reads too. + base_sha = meta.get("merge_base_sha") or meta.get("base_sha") + for p in (head_dir / "static_analysis.pkl", base_dir / "static_analysis.pkl"): + if not p.is_file(): + raise Skip(f"no call graph at {p}") + + pull = _github_json(f"/repos/{repo}/pulls/{pr}") + commits = _github_json(f"/repos/{repo}/pulls/{pr}/commits?per_page=100") + pr_files = _diff(repo, base_sha, head_sha) + checkout = Path(env["CHECKOUT_DIR"]) + files: dict[str, str] = {} + for f in pr_files[:MAX_FILES]: + if f.get("status") != "removed": + p = checkout / f["filename"] + try: + files[f"{head_sha}:{f['filename']}"] = p.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + pass + if f.get("status") != "added": + was = f.get("previous_filename") or f["filename"] + text = _contents(repo, base_sha, was) + if text is not None: + files[f"{base_sha}:{was}"] = text + + bundle_meta = {**meta, "base_sha": base_sha, "head_sha": head_sha, "pr_number": str(pr)} + inputs = { + "analysis.json": Path(env["HEAD_ANALYSIS_PATH"]).read_text(), + "base/analysis.json": Path(env["BASE_ANALYSIS_PATH"]).read_text(), + "metadata.json": json.dumps(bundle_meta), + "pr.json": json.dumps( + { + "title": pull.get("title") or "", + "body": pull.get("body") or "", + "headRefOid": head_sha, + "commits": [(c.get("commit") or {}).get("message", "").split("\n")[0] for c in commits], + } + ), + "pr-files.json": json.dumps(pr_files), + "head-cg.json": json.dumps(call_graph(head_dir / "static_analysis.pkl")), + "base-cg.json": json.dumps(call_graph(base_dir / "static_analysis.pkl")), + "files.json": json.dumps(files), + } + return {"version": 1, "repo": repo, "pr": pr, "files": inputs} + + +def _relay(): + spec = importlib.util.spec_from_file_location("oidc_relay", Path(__file__).with_name("oidc_relay.py")) + assert spec and spec.loader + mod = importlib.util.module_from_spec(spec) + sys.modules.setdefault(spec.name, mod) # its dataclass looks its own module up while it is defined + spec.loader.exec_module(sys.modules[spec.name]) + return sys.modules[spec.name] + + +def credentials(env: dict) -> dict[str, str]: + """The request headers: the run's own key for a provider the service speaks, else the hosted + model's bearer, the same one the hosted proxy gets (a fresh OIDC token, plus the license).""" + for name, provider in OWN_KEYS: + if env.get(name, "").strip(): + # The service still checks the caller is this repository's run. + return {**_bearer(env), "x-llm-api-key": env[name].strip(), "x-llm-provider": provider} + return _bearer(env) + + +def _bearer(env: dict) -> dict[str, str]: + url, token = env.get("ACTIONS_ID_TOKEN_REQUEST_URL", ""), env.get("ACTIONS_ID_TOKEN_REQUEST_TOKEN", "") + if not url or not token: + # Without `id-token: write` the run's own token proves the same thing: it can read this + # repository. It carries no license, so the owner's free bucket pays. + return {"Authorization": f"Bearer {env['GITHUB_TOKEN']}"} + relay = _relay() + license_file = None + if env.get("LICENSE_KEY", "").strip(): + license_file = Path(env.get("RUNNER_TEMP", "/tmp")) / "codeboarding-guide-license.txt" + license_file.write_text(env["LICENSE_KEY"].strip()) + try: + config = relay.RelayConfig( + upstream_base_url="", id_token_request_url=url, id_token_request_token=token, license_file=license_file + ) + return {"Authorization": relay._authorization(config)} + finally: + if license_file is not None: + license_file.unlink(missing_ok=True) + + +def request_guide(url: str, body: bytes, headers: dict[str, str]) -> dict: + req = Request( + f"{url.rstrip('/')}/v1/review-guide", + data=body, + headers={**headers, "Content-Type": "application/gzip", "User-Agent": "codeboarding-action"}, + method="POST", + ) + try: + with urlopen(req, timeout=600) as res: # nosec B310 - the service URL the workflow names + return json.loads(res.read()) + except HTTPError as e: + try: + message = json.loads(e.read()).get("message") or e.reason + except ValueError: + message = e.reason + raise Skip(f"the guide service answered {e.code}: {message}") from e + except (URLError, TimeoutError) as e: + raise Skip(f"the guide service did not answer: {type(e).__name__}") from e + + +def main(env: dict | None = None) -> int: + env = dict(os.environ if env is None else env) + try: + data = gzip.compress(json.dumps(bundle(env)).encode(), compresslevel=9) + if len(data) > MAX_BUNDLE_BYTES: + raise Skip(f"the bundle is {len(data) / 1e6:.1f} MB, over the service's limit") + answer = request_guide(env["REVIEW_GUIDE_URL"], data, credentials(env)) + guide = answer.get("guide") + if not isinstance(guide, dict): + raise Skip("the guide service returned no guide") + out = Path(env["ARTIFACT_DIR"]) / "review_guide.json" + out.write_text(json.dumps(guide, ensure_ascii=False, indent=1)) + usage = answer.get("usage") or {} + print( + f"Review guide: {len(guide.get('changes') or [])} changes in {len(guide.get('themes') or [])} main changes; " + f"{usage.get('provider')} {usage.get('model')}, {usage.get('promptTokens', 0) + usage.get('outputTokens', 0)} tokens " + f"({'hosted' if usage.get('hosted') else 'own key'}); bundle {len(data) / 1e6:.2f} MB" + ) + except Skip as e: + print(f"::notice::No review guide for this run: {e}") + except Exception as e: # never fail the review over its guide + print(f"::warning::Building the review guide failed: {type(e).__name__}: {e}") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_review_guide.py b/tests/test_review_guide.py new file mode 100644 index 0000000..1c0507c --- /dev/null +++ b/tests/test_review_guide.py @@ -0,0 +1,220 @@ +"""Tests for the review-guide step: the bundle it sends, the credentials, and that it never fails a run.""" + +from __future__ import annotations + +import gzip +import importlib.util +import json +import sys +import tempfile +import threading +import unittest +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +from pathlib import Path +from unittest import mock + +_SCRIPT = Path(__file__).resolve().parent.parent / "scripts" / "review_guide.py" +_SPEC = importlib.util.spec_from_file_location("review_guide", _SCRIPT) +assert _SPEC and _SPEC.loader +review_guide = importlib.util.module_from_spec(_SPEC) +sys.modules[_SPEC.name] = review_guide +_SPEC.loader.exec_module(review_guide) + + +class _Server: + """A local HTTP server answering every request with one handler.""" + + def __init__(self, respond): + outer = self + self.requests: list[dict] = [] + + class Handler(BaseHTTPRequestHandler): + def log_message(self, *_a): + pass + + def _any(self): + body = self.rfile.read(int(self.headers.get("Content-Length") or 0)) + outer.requests.append({"path": self.path, "headers": dict(self.headers), "body": body}) + status, payload = respond(self.path, body) + data = json.dumps(payload).encode() + self.send_response(status) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(data))) + self.end_headers() + self.wfile.write(data) + + do_GET = do_POST = _any + + self.server = ThreadingHTTPServer(("127.0.0.1", 0), Handler) + threading.Thread(target=self.server.serve_forever, daemon=True).start() + + @property + def url(self) -> str: + return f"http://127.0.0.1:{self.server.server_port}" + + def close(self) -> None: + self.server.shutdown() + + +def _workspace(tmp: Path) -> dict: + """A run's state as the step finds it: the artifact folder, both analyses, a head checkout.""" + artifact, head, base, checkout = (tmp / n for n in ("artifact", "head", "base", "checkout")) + for d in (artifact, head, base, checkout): + d.mkdir() + (artifact / "metadata.json").write_text( + json.dumps({"kind": "review", "head_sha": "H", "base_sha": "TIP", "merge_base_sha": "B", "pr_number": "7"}) + ) + for d in (head, base): + (d / "analysis.json").write_text(json.dumps({"components": [], "methods_index": {}})) + (d / "static_analysis.pkl").write_bytes(b"") + (checkout / "app.py").write_text("def f():\n return 2\n") + return { + "REVIEW_GUIDE_URL": "http://unused", + "REPOSITORY": "o/r", + "PR_NUMBER": "7", + "ARTIFACT_DIR": str(artifact), + "HEAD_ANALYSIS_PATH": str(head / "analysis.json"), + "BASE_ANALYSIS_PATH": str(base / "analysis.json"), + "CHECKOUT_DIR": str(checkout), + "GITHUB_TOKEN": "ghs_run", + "RUNNER_TEMP": str(tmp), + } + + +_COMPARE = { + "files": [ + {"filename": "app.py", "status": "modified", "additions": 1, "deletions": 1, "patch": "@@ -1,2 +1,2 @@"}, + {"filename": "new.py", "status": "added", "additions": 1, "deletions": 0, "patch": "@@ -0,0 +1 @@"}, + ] +} + + +def _github_json(path: str): + if path.startswith("/repos/o/r/compare/"): + return _COMPARE + if path.endswith("/commits?per_page=100"): + return [{"commit": {"message": "Return two\n\nbody"}}] + return {"title": "Two", "body": "Returns two"} + + +class BundleTests(unittest.TestCase): + def test_the_bundle_reads_the_analysed_commits_and_both_sides_of_each_file(self): + with tempfile.TemporaryDirectory() as t: + env = _workspace(Path(t)) + with ( + mock.patch.object(review_guide, "_github_json", side_effect=_github_json) as gh, + mock.patch.object(review_guide, "_contents", return_value="def f():\n return 1\n") as base, + mock.patch.object(review_guide, "call_graph", return_value={"nodes": {}, "edges": []}), + ): + b = review_guide.bundle(env) + # the diff is between the two analysed commits, the merge base not the branch tip + self.assertIn("/repos/o/r/compare/B...H", [c.args[0] for c in gh.call_args_list]) + files = json.loads(b["files"]["files.json"]) + self.assertEqual(files["H:app.py"], "def f():\n return 2\n") + self.assertEqual(files["B:app.py"], "def f():\n return 1\n") + self.assertNotIn("B:new.py", files) # an added file has no base side to fetch + base.assert_called_once_with("o/r", "B", "app.py") + meta = json.loads(b["files"]["metadata.json"]) + self.assertEqual((meta["base_sha"], meta["head_sha"]), ("B", "H")) + self.assertEqual(json.loads(b["files"]["pr.json"])["commits"], ["Return two"]) + self.assertEqual(sorted(b["files"]), sorted(review_guide_inputs())) + + def test_a_missing_call_graph_skips_the_guide_without_failing(self): + with tempfile.TemporaryDirectory() as t: + env = _workspace(Path(t)) + (Path(env["HEAD_ANALYSIS_PATH"]).parent / "static_analysis.pkl").unlink() + with mock.patch.object(review_guide, "_github_json", side_effect=_github_json): + with mock.patch("builtins.print") as out: + self.assertEqual(review_guide.main(env), 0) + self.assertIn("No review guide", out.call_args.args[0]) + self.assertFalse((Path(env["ARTIFACT_DIR"]) / "review_guide.json").exists()) + + +def review_guide_inputs(): + return [ + "analysis.json", + "base/analysis.json", + "metadata.json", + "pr.json", + "pr-files.json", + "head-cg.json", + "base-cg.json", + "files.json", + ] + + +class CredentialTests(unittest.TestCase): + def test_without_oidc_the_runs_own_token_proves_the_repository(self): + self.assertEqual(review_guide.credentials({"GITHUB_TOKEN": "ghs_run"}), {"Authorization": "Bearer ghs_run"}) + + def test_with_oidc_the_bearer_is_the_proxys_own_with_the_license(self): + mint = _Server(lambda path, body: (200, {"value": "jwt.token.sig"})) + try: + env = { + "ACTIONS_ID_TOKEN_REQUEST_URL": f"{mint.url}/token?x=1", + "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "req", + "LICENSE_KEY": "LIC", + "GITHUB_TOKEN": "ghs_run", + } + with tempfile.TemporaryDirectory() as t: + env["RUNNER_TEMP"] = t + headers = review_guide.credentials(env) + self.assertEqual(list(Path(t).iterdir()), []) # the license file does not outlive the call + self.assertEqual(headers, {"Authorization": "Bearer jwt.token.sig~codeboarding-license~LIC"}) + self.assertIn("audience=codeboarding-proxy", mint.requests[0]["path"]) + finally: + mint.close() + + def test_the_runs_own_provider_key_goes_along_and_the_caller_is_still_proved(self): + headers = review_guide.credentials({"GITHUB_TOKEN": "ghs_run", "OPENROUTER_API_KEY": " sk-or "}) + self.assertEqual( + headers, + {"Authorization": "Bearer ghs_run", "x-llm-api-key": "sk-or", "x-llm-provider": "openrouter"}, + ) + + +class RequestTests(unittest.TestCase): + def test_the_guide_lands_in_the_artifact_beside_the_analysis(self): + guide = {"version": 1, "repo": "o/r", "pr": 7, "themes": [], "changes": [{"id": "u1"}]} + service = _Server(lambda path, body: (200, {"guide": guide, "usage": {"provider": "ollama", "hosted": True}})) + try: + with tempfile.TemporaryDirectory() as t: + env = {**_workspace(Path(t)), "REVIEW_GUIDE_URL": service.url} + with ( + mock.patch.object(review_guide, "_github_json", side_effect=_github_json), + mock.patch.object(review_guide, "_contents", return_value="x"), + mock.patch.object(review_guide, "call_graph", return_value={"nodes": {}, "edges": []}), + ): + self.assertEqual(review_guide.main(env), 0) + written = json.loads((Path(env["ARTIFACT_DIR"]) / "review_guide.json").read_text()) + self.assertEqual(written, guide) + sent = service.requests[0] + self.assertEqual(sent["path"], "/v1/review-guide") + self.assertEqual(sent["headers"]["Authorization"], "Bearer ghs_run") + self.assertEqual(json.loads(gzip.decompress(sent["body"]))["repo"], "o/r") + finally: + service.close() + + def test_a_refusal_is_a_notice_and_the_run_goes_on(self): + service = _Server( + lambda path, body: (402, {"error": "quota", "message": "Resource exhausted: token limit reached"}) + ) + try: + with tempfile.TemporaryDirectory() as t: + env = {**_workspace(Path(t)), "REVIEW_GUIDE_URL": service.url} + with ( + mock.patch.object(review_guide, "_github_json", side_effect=_github_json), + mock.patch.object(review_guide, "_contents", return_value="x"), + mock.patch.object(review_guide, "call_graph", return_value={"nodes": {}, "edges": []}), + mock.patch("builtins.print") as out, + ): + self.assertEqual(review_guide.main(env), 0) + self.assertFalse((Path(env["ARTIFACT_DIR"]) / "review_guide.json").exists()) + self.assertIn("402", out.call_args.args[0]) + self.assertIn("token limit", out.call_args.args[0]) + finally: + service.close() + + +if __name__ == "__main__": + unittest.main()