Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 87 additions & 3 deletions code-review/review.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@

COMMENT_MARKER = "<!-- innogando-ai-code-review -->"
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
Expand Down Expand Up @@ -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")

Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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",
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 3 additions & 1 deletion code-review/system-prompt.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
104 changes: 101 additions & 3 deletions code-review/tests/test_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from __future__ import annotations

import json
import os
import sys
import tempfile
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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()
1 change: 1 addition & 0 deletions code-review/workflow-template.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down