mirror of
https://github.com/Sea-Haven-Industries/pr-reviewer.git
synced 2026-09-30 10:23:15 +00:00
Adam Moussa
667afd73f9
Dependabot dependency-update PRs do not benefit from BLOCK/FIX/NIT/QUESTION code notes. Route them to a dependency-risk assessment instead: the semver update type, a safe/low_risk/risky/breaking call, the packages bumped, and reasons, grounded in the Sea Haven Dependabot merge policy (patch/minor generally safe; majors need changelog review; grouped PRs assessed at the riskiest package). Feedback focuses on PR title/description quality. review() dispatches on the author to a dependabot or code path, each stamping a "_kind" so consumers can tell the shapes apart (missing "_kind" reads as code, keeping older cached reviews valid). Enum fields are clamped to allowlists with cautious defaults so a hallucinated or injected value cannot reach the posted event. The handbook distillation also captures the dependency policy, though the prompt carries it regardless.
333 lines
11 KiB
Python
333 lines
11 KiB
Python
"""Tests for app.reviewer: _safe_json, render_markdown, and review()."""
|
|
|
|
from __future__ import annotations
|
|
|
|
import json
|
|
|
|
import pytest
|
|
|
|
from app import reviewer as reviewer_mod
|
|
from app.reviewer import (
|
|
Reviewer,
|
|
_is_dependabot,
|
|
_safe_json,
|
|
_sanitize_dependabot,
|
|
)
|
|
|
|
from .conftest import FakeResponse, patch_httpx
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# _safe_json
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_safe_json_plain() -> None:
|
|
assert _safe_json('{"a": 1, "b": "x"}') == {"a": 1, "b": "x"}
|
|
|
|
|
|
def test_safe_json_json_fenced() -> None:
|
|
text = '```json\n{"summary": "hi", "block": []}\n```'
|
|
assert _safe_json(text) == {"summary": "hi", "block": []}
|
|
|
|
|
|
def test_safe_json_bare_fenced() -> None:
|
|
text = '```\n{"k": true}\n```'
|
|
assert _safe_json(text) == {"k": True}
|
|
|
|
|
|
def test_safe_json_prose_wrapped() -> None:
|
|
text = 'Sure, here is the review:\n{"summary": "ok"}\nHope that helps!'
|
|
assert _safe_json(text) == {"summary": "ok"}
|
|
|
|
|
|
def test_safe_json_skips_reasoning_braces() -> None:
|
|
# A reasoning preamble containing stray "{" must not confuse extraction.
|
|
text = 'We should return {the result}. Final: {"summary": "ok", "block": []} done.'
|
|
assert _safe_json(text) == {"summary": "ok", "block": []}
|
|
|
|
|
|
def test_safe_json_malformed_raises() -> None:
|
|
with pytest.raises(json.JSONDecodeError):
|
|
_safe_json("this is not json at all")
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# render_markdown
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def _render(cfg, pr, review):
|
|
return Reviewer(cfg).render_markdown(pr, review)
|
|
|
|
|
|
def test_render_mention_applied_for_configured_author(make_cfg) -> None:
|
|
cfg = make_cfg(MENTION_AUTHORS=["openswe"])
|
|
pr = {"author": "openswe"}
|
|
review = {
|
|
"summary": "Looks good.",
|
|
"block": [],
|
|
"fix": [],
|
|
"nit": [],
|
|
"question": [],
|
|
}
|
|
out = _render(cfg, pr, review)
|
|
assert out.startswith("**Summary:** @openswe Looks good.")
|
|
|
|
|
|
def test_render_mention_case_insensitive(make_cfg) -> None:
|
|
cfg = make_cfg(MENTION_AUTHORS=["openswe"])
|
|
pr = {"author": "OpenSWE"}
|
|
review = {"summary": "Nice."}
|
|
out = _render(cfg, pr, review)
|
|
# Mention preserves the actual author casing, but matching is case-insensitive.
|
|
assert "@OpenSWE Nice." in out
|
|
|
|
|
|
def test_render_no_mention_for_other_author(make_cfg) -> None:
|
|
cfg = make_cfg(MENTION_AUTHORS=["openswe"])
|
|
pr = {"author": "octocat"}
|
|
review = {"summary": "Fine."}
|
|
out = _render(cfg, pr, review)
|
|
assert "@octocat" not in out
|
|
assert out.startswith("**Summary:** Fine.")
|
|
|
|
|
|
def test_render_empty_categories_omitted(make_cfg) -> None:
|
|
cfg = make_cfg()
|
|
pr = {"author": "octocat"}
|
|
review = {
|
|
"summary": "s",
|
|
"block": ["real block"],
|
|
"fix": [], # omitted
|
|
"nit": ["", " "], # all blank -> omitted
|
|
"question": ["a question"],
|
|
}
|
|
out = _render(cfg, pr, review)
|
|
assert "## BLOCK" in out
|
|
assert "## FIX" not in out
|
|
assert "## NIT" not in out
|
|
assert "## QUESTION" in out
|
|
|
|
|
|
def test_render_section_order(make_cfg) -> None:
|
|
cfg = make_cfg()
|
|
pr = {"author": "octocat"}
|
|
review = {
|
|
"summary": "s",
|
|
"block": ["b"],
|
|
"fix": ["f"],
|
|
"nit": ["n"],
|
|
"question": ["q"],
|
|
"overall": "closing",
|
|
}
|
|
out = _render(cfg, pr, review)
|
|
order = [out.index(h) for h in ("## BLOCK", "## FIX", "## NIT", "## QUESTION")]
|
|
assert order == sorted(order)
|
|
# Overall renders last.
|
|
assert out.index("## Overall") > out.index("## QUESTION")
|
|
assert "closing" in out
|
|
|
|
|
|
def test_render_summary_line_present(make_cfg) -> None:
|
|
cfg = make_cfg()
|
|
out = _render(cfg, {"author": "x"}, {"summary": "top line"})
|
|
assert out.splitlines()[0] == "**Summary:** top line"
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# review()
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def _fireworks_response(payload_obj: dict) -> FakeResponse:
|
|
content = json.dumps(payload_obj)
|
|
return FakeResponse(
|
|
200,
|
|
json_data={"choices": [{"message": {"content": content}}]},
|
|
)
|
|
|
|
|
|
def test_review_parses_response_and_builds_payload(
|
|
monkeypatch, make_cfg, sample_pr
|
|
) -> None:
|
|
cfg = make_cfg()
|
|
review_obj = {
|
|
"summary": "Solid change.",
|
|
"block": [],
|
|
"fix": ["`x.py:1` handle None"],
|
|
"nit": [],
|
|
"question": [],
|
|
"overall": "LGTM with a fix",
|
|
"recommended_event": "COMMENT",
|
|
}
|
|
calls = patch_httpx(monkeypatch, reviewer_mod, _fireworks_response(review_obj))
|
|
|
|
result = Reviewer(cfg).review(sample_pr, "diff --git a/x b/x\n+hello\n")
|
|
|
|
assert result["summary"] == "Solid change."
|
|
assert result["recommended_event"] == "COMMENT"
|
|
# render_markdown output is attached.
|
|
assert "_body_markdown" in result
|
|
assert result["_body_markdown"].startswith("**Summary:**")
|
|
|
|
# One POST to chat/completions with the expected payload.
|
|
assert len(calls) == 1
|
|
call = calls[0]
|
|
assert call["method"] == "POST"
|
|
assert call["url"].endswith("/chat/completions")
|
|
body = call["json"]
|
|
assert body["model"] == cfg.FIREWORKS_MODEL
|
|
assert body["temperature"] == cfg.FIREWORKS_TEMPERATURE
|
|
assert body["max_tokens"] == cfg.FIREWORKS_MAX_TOKENS
|
|
assert body["messages"][0]["role"] == "system"
|
|
assert body["messages"][1]["role"] == "user"
|
|
|
|
|
|
def test_review_injects_handbook_guidance(monkeypatch, make_cfg, sample_pr) -> None:
|
|
cfg = make_cfg()
|
|
calls = patch_httpx(
|
|
monkeypatch, reviewer_mod, _fireworks_response({"summary": "ok"})
|
|
)
|
|
r = Reviewer(cfg, guidance_provider=lambda: "USE KEBAB-CASE NAMES")
|
|
r.review(sample_pr, "diff")
|
|
system = calls[0]["json"]["messages"][0]["content"]
|
|
assert "USE KEBAB-CASE NAMES" in system
|
|
assert "SEA HAVEN ENGINEERING CONVENTIONS" in system
|
|
|
|
|
|
def test_review_no_guidance_when_provider_returns_none(
|
|
monkeypatch, make_cfg, sample_pr
|
|
) -> None:
|
|
cfg = make_cfg()
|
|
calls = patch_httpx(
|
|
monkeypatch, reviewer_mod, _fireworks_response({"summary": "ok"})
|
|
)
|
|
r = Reviewer(cfg, guidance_provider=lambda: None)
|
|
r.review(sample_pr, "diff")
|
|
system = calls[0]["json"]["messages"][0]["content"]
|
|
assert "SEA HAVEN ENGINEERING CONVENTIONS" not in system
|
|
|
|
|
|
def test_review_survives_guidance_provider_error(
|
|
monkeypatch, make_cfg, sample_pr
|
|
) -> None:
|
|
cfg = make_cfg()
|
|
calls = patch_httpx(
|
|
monkeypatch, reviewer_mod, _fireworks_response({"summary": "ok"})
|
|
)
|
|
|
|
def boom() -> str:
|
|
raise RuntimeError("provider down")
|
|
|
|
out = Reviewer(cfg, guidance_provider=boom).review(sample_pr, "diff")
|
|
assert out["summary"] == "ok" # review still succeeds
|
|
system = calls[0]["json"]["messages"][0]["content"]
|
|
assert "SEA HAVEN ENGINEERING CONVENTIONS" not in system
|
|
|
|
|
|
def test_review_truncates_large_diff(monkeypatch, make_cfg, sample_pr) -> None:
|
|
cfg = make_cfg(MAX_DIFF_BYTES=500)
|
|
review_obj = {"summary": "ok"}
|
|
calls = patch_httpx(monkeypatch, reviewer_mod, _fireworks_response(review_obj))
|
|
|
|
# Use a marker char that appears nowhere else in the prompt scaffolding.
|
|
big_diff = "Z" * 5000
|
|
Reviewer(cfg).review(sample_pr, big_diff)
|
|
|
|
user_content = calls[0]["json"]["messages"][1]["content"]
|
|
assert "[diff truncated for length]" in user_content
|
|
# The huge original diff must not be sent in full.
|
|
assert "Z" * 5000 not in user_content
|
|
# Only up to MAX_DIFF_BYTES of the diff body survives.
|
|
assert user_content.count("Z") == cfg.MAX_DIFF_BYTES
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# Dependabot mode
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def _dep_pr() -> dict:
|
|
return {
|
|
"owner": "o",
|
|
"repo": "r",
|
|
"number": 9,
|
|
"title": "Bump requests from 2.31.0 to 2.32.0",
|
|
"author": "dependabot[bot]",
|
|
"body": "Bumps requests.",
|
|
}
|
|
|
|
|
|
def test_is_dependabot() -> None:
|
|
assert _is_dependabot({"author": "dependabot[bot]"})
|
|
assert _is_dependabot({"author": "dependabot-preview[bot]"})
|
|
assert not _is_dependabot({"author": "octocat"})
|
|
assert not _is_dependabot({}) # missing author
|
|
|
|
|
|
def test_review_routes_dependabot_to_assessment(monkeypatch, make_cfg) -> None:
|
|
cfg = make_cfg()
|
|
obj = {
|
|
"update_type": "minor",
|
|
"packages": [{"name": "requests", "from": "2.31.0", "to": "2.32.0"}],
|
|
"assessment": "safe",
|
|
"summary": "Minor bump.",
|
|
"reasons": ["patch/minor generally safe"],
|
|
"title_desc_notes": [],
|
|
"recommended_event": "APPROVE",
|
|
}
|
|
calls = patch_httpx(monkeypatch, reviewer_mod, _fireworks_response(obj))
|
|
out = Reviewer(cfg).review(_dep_pr(), "diff")
|
|
|
|
assert out["_kind"] == "dependabot"
|
|
assert out["assessment"] == "safe"
|
|
assert out["update_type"] == "minor"
|
|
# Used the Dependabot system prompt, not the code-review one.
|
|
system = calls[0]["json"]["messages"][0]["content"]
|
|
assert "Dependabot" in system
|
|
assert "Dependency review" in out["_body_markdown"]
|
|
|
|
|
|
def test_review_code_path_stamps_kind(monkeypatch, make_cfg, sample_pr) -> None:
|
|
cfg = make_cfg()
|
|
obj = {"summary": "ok", "recommended_event": "COMMENT"}
|
|
calls = patch_httpx(monkeypatch, reviewer_mod, _fireworks_response(obj))
|
|
out = Reviewer(cfg).review(sample_pr, "diff") # sample_pr author is octocat
|
|
assert out["_kind"] == "code"
|
|
system = calls[0]["json"]["messages"][0]["content"]
|
|
assert "Dependabot" not in system
|
|
|
|
|
|
def test_sanitize_dependabot_coerces_bad_values() -> None:
|
|
out = _sanitize_dependabot(
|
|
{
|
|
"update_type": "HUGE",
|
|
"assessment": "nuclear",
|
|
"recommended_event": "YOLO",
|
|
"packages": "nope",
|
|
"reasons": None,
|
|
}
|
|
)
|
|
assert out["update_type"] == "unknown"
|
|
assert out["assessment"] == "risky" # cautious default
|
|
assert out["recommended_event"] == "COMMENT"
|
|
assert out["packages"] == []
|
|
assert out["reasons"] == []
|
|
|
|
|
|
def test_render_dependabot_markdown(make_cfg) -> None:
|
|
cfg = make_cfg(MENTION_AUTHORS=[])
|
|
rv = {
|
|
"summary": "Minor bump.",
|
|
"update_type": "minor",
|
|
"assessment": "safe",
|
|
"packages": [{"name": "requests", "from": "2.31.0", "to": "2.32.0"}],
|
|
"reasons": ["patch/minor safe per policy"],
|
|
"title_desc_notes": ["title is clear"],
|
|
}
|
|
md = Reviewer(cfg).render_dependabot_markdown({"author": "dependabot[bot]"}, rv)
|
|
assert "Dependency review" in md
|
|
assert "`requests`: 2.31.0 -> 2.32.0" in md
|
|
assert "patch/minor safe per policy" in md
|
|
assert "title is clear" in md
|