From 7f94a36075b32e24a7dae13119c9047fcfbe9bd1 Mon Sep 17 00:00:00 2001 From: Manu Corujo Date: Fri, 7 Aug 2026 13:31:25 +0200 Subject: [PATCH] feat(code-review): review the diff against the linked issue Resolve closingIssuesReferences for the PR and pass those issues to the model alongside the PR body, which was being cut at 500 characters. Repos that describe the work in the issue and leave the PR body empty were reviewed against the diff alone. Falls back to a diff-only review when the token lacks issues: read, so no caller breaks. Closes #3 --- code-review/review.py | 90 +++++++++++++++++++++++++- code-review/system-prompt.md | 4 +- code-review/tests/test_review.py | 104 +++++++++++++++++++++++++++++- code-review/workflow-template.yml | 1 + 4 files changed, 192 insertions(+), 7 deletions(-) diff --git a/code-review/review.py b/code-review/review.py index a926cc5..c4f63fe 100755 --- a/code-review/review.py +++ b/code-review/review.py @@ -18,6 +18,10 @@ COMMENT_MARKER = "" GH_TIMEOUT_SECONDS = 60 +# PR bodies and linked issues carry the specification, so they get a far larger +# budget than the 500 chars that used to cut them off mid-sentence. +SPEC_TEXT_MAX_CHARS = 6000 +MAX_LINKED_ISSUES = 5 LLM_TIMEOUT_SECONDS = 300 LLM_MAX_ATTEMPTS = 3 LLM_BACKOFF_BASE = 2.0 @@ -55,6 +59,18 @@ ), ] +LINKED_ISSUES_QUERY = """ +query($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + closingIssuesReferences(first: %d) { + nodes { number title body } + } + } + } +} +""" % MAX_LINKED_ISSUES + log = logging.getLogger("code-review") @@ -211,6 +227,47 @@ def fetch_pr_checks(pr_number: str) -> list[dict]: return json.loads(raw) if raw.strip() else [] +def fetch_linked_issues(repo: str, pr_number: str) -> list[dict]: + """Issues the PR closes. Teams that describe the work in the issue and leave + the PR body empty keep the specification here and nowhere else.""" + owner, _, name = repo.partition("/") + try: + raw = run_gh( + [ + "api", + "graphql", + "-f", + f"query={LINKED_ISSUES_QUERY}", + "-F", + f"owner={owner}", + "-F", + f"repo={name}", + "-F", + f"number={pr_number}", + ] + ) + except ReviewError as exc: + # Most likely the caller's workflow grants no `issues: read`. Review the + # diff without the spec rather than failing the job. + log.warning("Could not fetch linked issues: %s", exc) + return [] + + try: + nodes = json.loads(raw)["data"]["repository"]["pullRequest"]["closingIssuesReferences"]["nodes"] + except (KeyError, TypeError, json.JSONDecodeError) as exc: + log.warning("Unexpected linked-issues response shape: %s", exc) + return [] + + return [ + { + "number": node.get("number"), + "title": node.get("title") or "", + "body": _truncate(node.get("body") or ""), + } + for node in nodes or [] + ] + + def summarize_ci(checks: list[dict]) -> tuple[str, bool]: if not checks: return "unknown — no checks reported", False @@ -228,6 +285,10 @@ def summarize_ci(checks: list[dict]) -> tuple[str, bool]: return "green — all checks passing", False +def _truncate(text: str, limit: int = SPEC_TEXT_MAX_CHARS) -> str: + return text if len(text) <= limit else text[:limit] + "…" + + def _compact_meta(meta: dict) -> dict: body = meta.get("body") or "" return { @@ -241,15 +302,37 @@ def _compact_meta(meta: dict) -> dict: "changed_files": meta.get("changedFiles", 0), "labels": [lb["name"] for lb in (meta.get("labels") or []) if lb.get("name")], "draft": meta.get("isDraft", False), - "body": body[:500] + ("…" if len(body) > 500 else ""), + "body": _truncate(body), } -def build_user_message(meta: dict, diff: str, ci_summary: str, repo_context: str) -> str: +def _render_linked_issues(issues: list[dict]) -> str: + if not issues: + return ( + "_No linked issue found. Judge the change against the PR body and the repository " + "context; if neither states the intent, say so rather than guessing it._" + ) + return "\n\n".join( + f"## Issue #{issue['number']}: {issue['title']}\n\n" + f"{issue['body'] or '_(empty body)_'}" + for issue in issues + ) + + +def build_user_message( + meta: dict, + diff: str, + ci_summary: str, + repo_context: str, + linked_issues: list[dict] | None = None, +) -> str: parts = [ "# Pull request metadata", json.dumps(_compact_meta(meta), indent=2), "", + "# Linked issues — the specification this PR is meant to satisfy", + _render_linked_issues(linked_issues or []), + "", f"# CI status\n\n{ci_summary}", "", "# Repository context", @@ -578,9 +661,10 @@ def run(cfg: Config) -> int: checks = fetch_pr_checks(cfg.pr_number) ci_summary, ci_failing = summarize_ci(checks) + linked_issues = fetch_linked_issues(cfg.repo, cfg.pr_number) system_prompt = load_system_prompt(cfg.action_path) repo_context = build_context() - user_message = build_user_message(meta, diff, ci_summary, repo_context) + user_message = build_user_message(meta, diff, ci_summary, repo_context, linked_issues) raw_review = call_litellm(cfg, system_prompt, user_message) review = normalize_review(raw_review) diff --git a/code-review/system-prompt.md b/code-review/system-prompt.md index eb7255a..68625ea 100644 --- a/code-review/system-prompt.md +++ b/code-review/system-prompt.md @@ -14,7 +14,9 @@ If you cannot name a concrete consequence (a bug, a security hole, data loss, a ## Task -Read the PR metadata, CI status, repository context (AGENTS.md, code-review skills, conventions, gotchas, secret-patterns), and the diff in the user message. Review **only what the diff changes** — do not demand rework of untouched code. Then emit a single JSON object — no prose, no markdown fences, no preamble. +Read the PR metadata, the linked issues, CI status, repository context (AGENTS.md, code-review skills, conventions, gotchas, secret-patterns), and the diff in the user message. Review **only what the diff changes** — do not demand rework of untouched code. Then emit a single JSON object — no prose, no markdown fences, no preamble. + +The linked issues state what the author said they would build. Where the diff plainly fails to do what the issue describes, that is a finding; the consequence is the gap itself. Where no issue is linked and the PR body is empty, review the diff alone and say the intent was not stated rather than inferring one. ## Output schema diff --git a/code-review/tests/test_review.py b/code-review/tests/test_review.py index 8b1cf19..fe0197f 100644 --- a/code-review/tests/test_review.py +++ b/code-review/tests/test_review.py @@ -7,6 +7,7 @@ from __future__ import annotations +import json import os import sys import tempfile @@ -255,12 +256,18 @@ def test_omits_verbose_github_fields(self): self.assertNotIn("reviewDecision", compact) self.assertNotIn("mergeable", compact) - def test_body_truncated_at_500_chars(self): - long_body = "x" * 600 + def test_body_truncated_at_spec_limit(self): + long_body = "x" * (review.SPEC_TEXT_MAX_CHARS + 100) compact = review._compact_meta(self._full_meta(body=long_body)) - self.assertEqual(len(compact["body"]), 501) # 500 chars + ellipsis + self.assertEqual(len(compact["body"]), review.SPEC_TEXT_MAX_CHARS + 1) self.assertTrue(compact["body"].endswith("…")) + def test_body_survives_a_realistic_spec(self): + # The old 500-char cap cut PR bodies off mid-sentence. + body = "Closes #482.\n\n" + ("Vocabulary: 8 phases. " * 100) + compact = review._compact_meta(self._full_meta(body=body)) + self.assertEqual(compact["body"], body) + def test_body_not_truncated_when_short(self): compact = review._compact_meta(self._full_meta(body="short")) self.assertEqual(compact["body"], "short") @@ -390,5 +397,96 @@ def test_cannot_review_is_preserved(self): self.assertEqual(review.derive_verdict(rev, ci_failing=True), "cannot_review") +class FetchLinkedIssuesTests(unittest.TestCase): + def setUp(self): + self._real_run_gh = review.run_gh + self.addCleanup(setattr, review, "run_gh", self._real_run_gh) + + def _stub_gh(self, payload): + self.calls = [] + + def fake_run_gh(args, **kwargs): + self.calls.append(args) + if isinstance(payload, Exception): + raise payload + return payload + + review.run_gh = fake_run_gh + + @staticmethod + def _graphql(nodes): + return json.dumps( + {"data": {"repository": {"pullRequest": {"closingIssuesReferences": {"nodes": nodes}}}}} + ) + + def test_returns_issue_number_title_and_body(self): + self._stub_gh(self._graphql([{"number": 517, "title": "Strengthen tests", "body": "Do the thing"}])) + issues = review.fetch_linked_issues("Innogando/rumi-api", "600") + self.assertEqual(issues, [{"number": 517, "title": "Strengthen tests", "body": "Do the thing"}]) + + def test_splits_owner_and_repo(self): + self._stub_gh(self._graphql([])) + review.fetch_linked_issues("Innogando/rumi-api", "600") + joined = " ".join(self.calls[0]) + self.assertIn("owner=Innogando", joined) + self.assertIn("repo=rumi-api", joined) + self.assertIn("number=600", joined) + + def test_missing_permission_degrades_to_empty(self): + self._stub_gh(review.ReviewError("`gh api` failed: Resource not accessible")) + self.assertEqual(review.fetch_linked_issues("Innogando/rumi-api", "600"), []) + + def test_unexpected_shape_degrades_to_empty(self): + self._stub_gh(json.dumps({"data": {"repository": None}})) + self.assertEqual(review.fetch_linked_issues("Innogando/rumi-api", "600"), []) + + def test_non_json_degrades_to_empty(self): + self._stub_gh("not json") + self.assertEqual(review.fetch_linked_issues("Innogando/rumi-api", "600"), []) + + def test_long_issue_body_is_truncated(self): + self._stub_gh( + self._graphql([{"number": 1, "title": "t", "body": "y" * (review.SPEC_TEXT_MAX_CHARS + 50)}]) + ) + issues = review.fetch_linked_issues("Innogando/rumi-api", "600") + self.assertEqual(len(issues[0]["body"]), review.SPEC_TEXT_MAX_CHARS + 1) + + +class RenderLinkedIssuesTests(unittest.TestCase): + def test_renders_number_title_and_body(self): + rendered = review._render_linked_issues( + [{"number": 517, "title": "Strengthen tests", "body": "Do the thing"}] + ) + self.assertIn("Issue #517: Strengthen tests", rendered) + self.assertIn("Do the thing", rendered) + + def test_empty_body_is_marked(self): + rendered = review._render_linked_issues([{"number": 1, "title": "t", "body": ""}]) + self.assertIn("_(empty body)_", rendered) + + def test_no_issues_tells_the_model_not_to_guess(self): + rendered = review._render_linked_issues([]) + self.assertIn("No linked issue found", rendered) + + +class BuildUserMessageTests(unittest.TestCase): + def test_includes_linked_issue_text(self): + message = review.build_user_message( + {"number": 1, "title": "t", "body": ""}, + "diff --git a/x b/x", + "green", + "context", + [{"number": 517, "title": "Strengthen tests", "body": "Do the thing"}], + ) + self.assertIn("Issue #517", message) + self.assertIn("Do the thing", message) + + def test_omitting_issues_still_builds(self): + message = review.build_user_message( + {"number": 1, "title": "t", "body": ""}, "diff", "green", "context" + ) + self.assertIn("No linked issue found", message) + + if __name__ == "__main__": unittest.main() diff --git a/code-review/workflow-template.yml b/code-review/workflow-template.yml index 596a54a..e9b4ae4 100644 --- a/code-review/workflow-template.yml +++ b/code-review/workflow-template.yml @@ -32,6 +32,7 @@ jobs: permissions: pull-requests: write # comment on the PR contents: read # checkout + diff + issues: read # read the issues the PR closes (the specification) steps: - uses: Innogando/github-workflows/code-review@v2 with: