2026-07-01 13:45:07 -04:00
|
|
|
"""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, _safe_json
|
|
|
|
|
|
|
|
|
|
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_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"
|
|
|
|
|
|
|
|
|
|
|
Ground reviews in the engineering-handbook
Feed the reviewer a distilled digest of the Sea Haven engineering-handbook
so findings reflect our naming, commit, PR, secrets, and IaC conventions
instead of generic code-review judgment.
A new handbook module keeps an app-managed shallow clone of the (private)
handbook, distills the review-relevant pages into a compact conventions
checklist via the Fireworks model once a day, caches it under ~/.cache,
and hands it to the reviewer to inject into every review's system prompt.
The refresh runs in-process at the start of each worker cycle; failures
keep the last good digest and back off, so a handbook outage never blocks
reviews. Set HANDBOOK_ENABLED=false to disable.
Extract a shared fireworks_complete helper used by both the reviewer and
the distiller, so the handbook provider needs no reviewer reference and
the guidance callable is set once at construction. Clone auth uses a
Basic http.extraHeader (GitHub git-over-HTTPS rejects Bearer), and the
distiller wraps its answer in delimiters to strip a reasoning model's
chain-of-thought preamble. Adds GET /api/handbook and a header status
line. Stdlib-only, no new dependencies.
2026-07-01 17:08:21 -04:00
|
|
|
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
|
|
|
|
|
|
|
|
|
|
|
2026-07-01 13:45:07 -04:00
|
|
|
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
|