df72b3926f
Reviewer(repo_client, runner) with review_pr() -> Verdict and post_verdict() posting [review-gate] comments. Parses acceptance checklists + Files-allowed scope from the linked issue; runs pytest, optional build cmd (REVIEW_BUILD_CMD), git diff --check with timeouts (REVIEW_TEST_TIMEOUT_S, default 300); secret-pattern scan (sk_live_, AKIA, gh tokens, private keys, assigned credentials) quoting each added diff line as evidence (values redacted); optional configurable LLM pass that is validated — unquoted findings dropped. Verdict rules: failing tests -> BOUNCE (unless allow_test_failure in issue body); timeout -> BOUNCE; scope violation -> BOUNCE; blocking finding -> BOUNCE; empty acceptance -> NEEDS_GUIDANCE (never silent-approve); else APPROVE. FAIL-CLOSED: no merge call anywhere. gitea_client.py adds the Gitea seam (env token, urllib, mockable). 16 hermetic tests, no network, no real subprocesses.
321 lines
14 KiB
Python
321 lines
14 KiB
Python
"""Hermetic tests for the review gate (issue #5).
|
|
|
|
No network, no real subprocesses: FakeGiteaClient serves PR/issue/diff
|
|
data in-memory and FakeRunner scripted exits. The only subprocess-ish seam
|
|
tested is Runner's timeout handling, via an injected command runner.
|
|
|
|
Run with: python -m pytest tests/test_reviewer.py -q
|
|
"""
|
|
|
|
import ast
|
|
import subprocess
|
|
import sys
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
# PEP 420 namespace package: make the project root importable regardless of
|
|
# how pytest was invoked.
|
|
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
|
|
|
|
from gitea_saas.reviewer import ( # noqa: E402
|
|
APPROVE,
|
|
BOUNCE,
|
|
NEEDS_GUIDANCE,
|
|
Reviewer,
|
|
Runner,
|
|
Verdict,
|
|
parse_acceptance,
|
|
parse_allowed_files,
|
|
)
|
|
|
|
REVIEWER_SRC = Path(__file__).resolve().parent.parent / "gitea_saas" / "reviewer.py"
|
|
|
|
|
|
class FakeGiteaClient:
|
|
"""In-memory Gitea seam: PRs, issues and posted comments."""
|
|
|
|
def __init__(self, prs=None, issues=None):
|
|
self.prs = prs or {} # number -> {"head_sha", "changed_files", "diff"}
|
|
self.issues = issues or {} # number -> {"body", ...}
|
|
self.comments = [] # (repo, number, body)
|
|
|
|
def get_pr(self, repo, number):
|
|
pr = self.prs[number]
|
|
return {"head_sha": pr["head_sha"], "changed_files": list(pr["changed_files"])}
|
|
|
|
def get_pr_diff(self, repo, number):
|
|
return self.prs[number].get("diff", "")
|
|
|
|
def get_issue(self, repo, number):
|
|
return dict(self.issues[number])
|
|
|
|
def post_comment(self, repo, number, body):
|
|
self.comments.append((repo, number, body))
|
|
|
|
|
|
class FakeRunner:
|
|
"""Subprocess seam: scripted results, records commands."""
|
|
|
|
def __init__(self, results=None):
|
|
self.results = list(results or []) # popped per run
|
|
self.commands = []
|
|
|
|
def run(self, repo_dir, command, timeout_s=None):
|
|
self.commands.append((str(repo_dir), command, timeout_s))
|
|
if self.results:
|
|
return self.results.pop(0)
|
|
return {"exit": 0, "summary": ""}
|
|
|
|
|
|
def sample_issue(acceptance=None, allowed_files=None, extra=""):
|
|
body = "## Goal\nAutomated review gate.\n\n"
|
|
if allowed_files is not None:
|
|
body += ("**Files allowed:** " + ", ".join(f"`{f}`" for f in allowed_files)
|
|
+ ". Do NOT modify anything else.\n\n")
|
|
body += "## Acceptance\n"
|
|
body += "".join(f"- [ ] {a}\n" for a in (acceptance or ["tests pass", "no regressions"]))
|
|
body += extra
|
|
return {"body": body}
|
|
|
|
|
|
def sample_pr(diff="", files=None, head_sha="a" * 40):
|
|
return {"head_sha": head_sha,
|
|
"changed_files": files or ["gitea_saas/reviewer.py", "tests/test_reviewer.py"],
|
|
"diff": diff}
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Parsing
|
|
# ---------------------------------------------------------------------------
|
|
|
|
def test_parse_acceptance_checklist_and_section():
|
|
body = ("## Goal\nx\n\n## Acceptance\n- [ ] first criterion\n- [X] second criterion\n"
|
|
"- [x] third criterion\n\n## Notes\n- not acceptance\n")
|
|
assert parse_acceptance(body) == ["first criterion", "second criterion", "third criterion"]
|
|
assert parse_acceptance("no checklist here") == []
|
|
|
|
|
|
def test_parse_allowed_files_backticks():
|
|
body = "**Files allowed:** `gitea_saas/reviewer.py`, `gitea_saas/gitea_client.py` (extend), `tests/test_reviewer.py`. Do NOT modify anything else."
|
|
assert parse_allowed_files(body) == [
|
|
"gitea_saas/reviewer.py", "gitea_saas/gitea_client.py", "tests/test_reviewer.py"]
|
|
assert parse_allowed_files("no constraint declared") == []
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# APPROVE / BOUNCE / NEEDS_GUIDANCE
|
|
# ---------------------------------------------------------------------------
|
|
|
|
def test_approve_when_tests_pass_and_no_findings():
|
|
client = FakeGiteaClient(prs={1: sample_pr(files=["gitea_saas/reviewer.py"])},
|
|
issues={5: sample_issue(allowed_files=["gitea_saas/reviewer.py"])})
|
|
runner = FakeRunner([{"exit": 0, "summary": "12 passed"}, # pytest
|
|
{"exit": 0, "summary": "clean"}]) # git diff --check
|
|
reviewer = Reviewer(client, runner, config={"secret_scan": True})
|
|
|
|
verdict = reviewer.review_pr("gitea-saas-core", 1, client.issues[5], "/fake/repo")
|
|
|
|
assert verdict.verdict == APPROVE
|
|
assert verdict.tests == {"exit": 0, "summary": "12 passed"}
|
|
assert verdict.acceptance == ["tests pass", "no regressions"]
|
|
assert verdict.scope_violations == [] and verdict.findings == []
|
|
assert verdict.head_sha == "a" * 40
|
|
|
|
comment = reviewer.post_verdict("gitea-saas-core", 1, verdict)
|
|
assert "[review-gate] VERDICT: **APPROVE**" in comment
|
|
assert "- [ ] tests pass" in comment # checklist present
|
|
assert "No merge performed by the review gate" in comment
|
|
assert client.comments and client.comments[0][:2] == ("gitea-saas-core", 1)
|
|
|
|
|
|
def test_bounce_when_tests_fail():
|
|
client = FakeGiteaClient(prs={1: sample_pr()}, issues={5: sample_issue()})
|
|
runner = FakeRunner([{"exit": 1, "summary": "1 failed, 11 passed"}, {"exit": 0, "summary": "clean"}])
|
|
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, client.issues[5], "/fake/repo")
|
|
assert verdict.verdict == BOUNCE
|
|
assert "tests failed (exit=1)" in verdict.summary
|
|
# BOUNCE comment carries the test evidence
|
|
reviewer = Reviewer(client, runner)
|
|
comment = reviewer.post_verdict("gitea-saas-core", 1, verdict)
|
|
assert "VERDICT: **BOUNCE**" in comment and "1 failed, 11 passed" in comment
|
|
|
|
|
|
def test_bounce_on_scope_violation_even_with_green_tests():
|
|
issue = sample_issue(allowed_files=["gitea_saas/reviewer.py"])
|
|
pr = sample_pr(files=["gitea_saas/reviewer.py", "gitea_saas/server.py"])
|
|
client = FakeGiteaClient(prs={1: pr}, issues={5: issue})
|
|
runner = FakeRunner([{"exit": 0, "summary": "12 passed"}, {"exit": 0, "summary": "clean"}])
|
|
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue, "/fake/repo")
|
|
assert verdict.verdict == BOUNCE
|
|
assert verdict.scope_violations == ["gitea_saas/server.py"]
|
|
|
|
|
|
def test_needs_guidance_when_no_acceptance_criteria():
|
|
issue = {"body": "## Goal\nnothing to verify here\n"}
|
|
client = FakeGiteaClient(prs={1: sample_pr()}, issues={5: issue})
|
|
runner = FakeRunner([{"exit": 0, "summary": "12 passed"}, {"exit": 0, "summary": "clean"}])
|
|
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue, "/fake/repo")
|
|
assert verdict.verdict == NEEDS_GUIDANCE
|
|
assert "never silent-approve" in verdict.summary
|
|
|
|
|
|
def test_allow_test_failure_in_issue_body_overrides_bounce():
|
|
issue = sample_issue(extra="note: allow_test_failure for this transitional PR\n")
|
|
client = FakeGiteaClient(prs={1: sample_pr()}, issues={5: issue})
|
|
runner = FakeRunner([{"exit": 1, "summary": "known failure"}, {"exit": 0, "summary": "clean"}])
|
|
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue, "/fake/repo")
|
|
assert verdict.verdict == APPROVE # tolerated, acceptance present, nothing else blocking
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Findings quote the diff
|
|
# ---------------------------------------------------------------------------
|
|
|
|
SECRET_DIFF = (
|
|
"diff --git a/gitea_saas/server.py b/gitea_saas/server.py\n"
|
|
"--- a/gitea_saas/server.py\n+++ b/gitea_saas/server.py\n"
|
|
"@@ -10,6 +10,8 @@\n"
|
|
" def main():\n"
|
|
"+ api_key = 'sk_live_1234567890ABCDEF'\n"
|
|
"+ print('injected debug line')\n"
|
|
"+ # TODO: replace placeholder\n"
|
|
)
|
|
|
|
|
|
def test_secret_finding_quotes_and_redacts_diff_line():
|
|
issue = sample_issue(allowed_files=["gitea_saas/server.py"])
|
|
pr = sample_pr(diff=SECRET_DIFF, files=["gitea_saas/server.py"])
|
|
client = FakeGiteaClient(prs={1: pr}, issues={5: issue})
|
|
runner = FakeRunner([{"exit": 0, "summary": "ok"}, {"exit": 0, "summary": "clean"}])
|
|
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue, "/fake/repo")
|
|
blocking = [f for f in verdict.findings if f["severity"] == "blocking"]
|
|
assert blocking, "secret scan should produce a blocking finding"
|
|
f = blocking[0]
|
|
assert f["file"] == "gitea_saas/server.py"
|
|
assert f["line"] == 11
|
|
# Must quote the actual added line (with the secret value redacted).
|
|
assert f["evidence_quote"].startswith(" api_key = '")
|
|
assert "sk_live_" not in f["evidence_quote"] and "REDACTED" in f["evidence_quote"]
|
|
assert verdict.verdict == BOUNCE
|
|
# No unquoted assertion: every finding carries evidence_quote.
|
|
assert all(fi.get("evidence_quote") for fi in verdict.findings)
|
|
|
|
|
|
def test_todo_finding_is_warning_not_blocking():
|
|
issue = sample_issue()
|
|
pr = sample_pr(diff=SECRET_DIFF, files=["gitea_saas/server.py"])
|
|
client = FakeGiteaClient(prs={1: pr}, issues={5: issue})
|
|
runner = FakeRunner([{"exit": 0, "summary": "ok"}, {"exit": 0, "summary": "clean"}])
|
|
verdict = Reviewer(client, runner, config={"secret_scan": False}).review_pr(
|
|
"gitea-saas-core", 1, issue, "/fake/repo")
|
|
todos = [f for f in verdict.findings if f["severity"] == "warning"]
|
|
assert todos and todos[0]["note"] == "TODO/FIXME/HACK marker in added line"
|
|
assert todos[0]["evidence_quote"] == " # TODO: replace placeholder"
|
|
assert verdict.verdict == APPROVE # warnings alone do not bounce
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Timeouts
|
|
# ---------------------------------------------------------------------------
|
|
|
|
def test_timeout_bounces_with_timed_out_exit():
|
|
issue = sample_issue()
|
|
client = FakeGiteaClient(prs={1: sample_pr()}, issues={5: issue})
|
|
runner = FakeRunner([{"exit": "timed_out", "summary": "[timed out after 300s]"},
|
|
{"exit": 0, "summary": "clean"}])
|
|
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue, "/fake/repo")
|
|
assert verdict.verdict == BOUNCE
|
|
assert verdict.tests["exit"] == "timed_out"
|
|
assert "timed out" in verdict.summary
|
|
|
|
|
|
def test_runner_timeout_respected_without_real_subprocess():
|
|
def exploding_runner(command, **kwargs):
|
|
raise subprocess.TimeoutExpired(command, timeout=kwargs.get("timeout") or 0)
|
|
|
|
calls = []
|
|
def spy(command, **kwargs):
|
|
calls.append((command, kwargs.get("timeout")))
|
|
return exploding_runner(command, **kwargs)
|
|
|
|
runner = Runner(timeout_s=300, command_runner=spy)
|
|
result = runner.run("/fake/repo", "python -m pytest -q", timeout_s=7)
|
|
assert result["exit"] == "timed_out"
|
|
assert calls == [("python -m pytest -q", 7)] # timeout passed through
|
|
|
|
|
|
def test_runner_default_timeout_used():
|
|
seen = []
|
|
def spy(command, **kwargs):
|
|
seen.append(kwargs.get("timeout"))
|
|
class C:
|
|
returncode = 0
|
|
stdout = "ok"
|
|
stderr = ""
|
|
return C()
|
|
runner = Runner(timeout_s=120, command_runner=spy)
|
|
result = runner.run("/fake/repo", "true")
|
|
assert result["exit"] == 0 and seen == [120]
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Build command / hygiene / misc
|
|
# ---------------------------------------------------------------------------
|
|
|
|
def test_build_command_run_when_configured_and_build_failure_bounces():
|
|
issue = sample_issue()
|
|
client = FakeGiteaClient(prs={1: sample_pr()}, issues={5: issue})
|
|
runner = FakeRunner([{"exit": 0, "summary": "12 passed"}, # pytest
|
|
{"exit": 1, "summary": "build error"}, # build
|
|
{"exit": 0, "summary": "clean"}]) # diff --check
|
|
reviewer = Reviewer(client, runner, config={"build_cmd": "python -m compileall -q ."})
|
|
verdict = reviewer.review_pr("gitea-saas-core", 1, issue, "/fake/repo")
|
|
build = verdict.build
|
|
assert build is not None
|
|
assert build["exit"] == 1 and build["command"] == "python -m compileall -q ."
|
|
assert verdict.verdict == BOUNCE
|
|
assert "python -m compileall -q ." in "".join(runner.commands[1][1:2])
|
|
|
|
|
|
def test_diff_hygiene_failure_is_blocking_finding():
|
|
issue = sample_issue()
|
|
client = FakeGiteaClient(prs={1: sample_pr()}, issues={5: issue})
|
|
runner = FakeRunner([{"exit": 0, "summary": "12 passed"},
|
|
{"exit": 1, "summary": "trailing whitespace at gitea_saas/x.py:3"}])
|
|
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue, "/fake/repo")
|
|
assert verdict.verdict == BOUNCE
|
|
assert any("diff --check" in f["note"] for f in verdict.findings)
|
|
|
|
|
|
def test_verdict_shape_and_comment_never_mentions_merge_action():
|
|
issue = sample_issue()
|
|
client = FakeGiteaClient(prs={1: sample_pr()}, issues={5: issue})
|
|
runner = FakeRunner([{"exit": 0, "summary": "12 passed"}, {"exit": 0, "summary": "clean"}])
|
|
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue, "/fake/repo")
|
|
assert verdict.repo == "gitea-saas-core" and verdict.pr == 1
|
|
assert isinstance(verdict.acceptance, list)
|
|
assert isinstance(verdict.tests, dict) and "exit" in verdict.tests
|
|
assert isinstance(verdict.scope_violations, list)
|
|
assert isinstance(verdict.findings, list)
|
|
assert verdict.verdict in (APPROVE, BOUNCE, NEEDS_GUIDANCE)
|
|
comment = verdict.to_comment()
|
|
assert "[review-gate]" in comment
|
|
# Fail-closed: the comment only ever *disclaims* merging — never claims it.
|
|
assert "No merge performed by the review gate" in comment
|
|
assert "merged" not in comment
|
|
|
|
|
|
def test_no_merge_call_anywhere_in_reviewer_src():
|
|
tree = ast.parse(REVIEWER_SRC.read_text(encoding="utf-8"))
|
|
for node in ast.walk(tree):
|
|
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
|
|
assert node.name != "merge", f"merge function found: {node.lineno}"
|
|
if isinstance(node, ast.Call):
|
|
func = node.func
|
|
name = (func.id if isinstance(func, ast.Name) else
|
|
func.attr if isinstance(func, ast.Attribute) else "")
|
|
assert name != "merge", f"merge call found at {node.lineno}"
|
|
if isinstance(node, ast.Attribute):
|
|
assert node.attr != "merge", f"merge attribute access at {node.lineno}"
|